Tim Crawford has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47892 )
Change subject: mb/system76/oryp5: Add System76 Oryx Pro 5 ......................................................................
Patch Set 3:
(10 comments)
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... File src/mainboard/system76/oryp5/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 12: # Send an extra VR mailbox command for the PS4 exit issue : register "SendVrMbxCmd" = "2"
Does this apply to your board?
I don't even know what it does. But I haven't noticed any bad behavior with it removed.
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 34: PchSerialIoPci
SkipInit?
Not available on Coffee Lake.
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 37: # PCI Express Graphics #0 x16, Clock 8 (NVIDIA GPU) : register "PcieClkSrcUsage[8]" = "0x40" : register "PcieClkSrcClkReq[8]" = "8"
Put under the PEG device? (01. […]
Done
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 79: Enable
Nope
They're all 0, so just removed the block.
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... File src/mainboard/system76/oryp5/gpio.h:
PS2:
This should be a gpio. […]
Done
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... File src/mainboard/system76/oryp5/romstage.c:
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 7: CH0D0/CH0D1/CH1D0/CH1D1.
Two of these do not exist
Comment from header. Removed from board.
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 19: /* : * For each channel, there are 3 sets of DQ byte mappings, : * where each set has a package 0 and a package 1 value (package 0 : * represents the first 64-bit lpddr4 chip combination, and package 1 : * represents the second 64-bit lpddr4 chip combination). : * The first three sets are for CLK, CMD, and CTL. : * The fsp package actually expects 6 sets, but the last 3 sets are : * not used in CNL, so we only define the three sets that are used : * and let the meminit_lpddr4() routine take care of clearing the : * unused fields for the caller. : */ : .dq_map[DDR_CH0] = { : {0x0F, 0xF0}, {0x00, 0xF0}, {0x0F, 0xF0}, : //{0x0F, 0x00}, {0xFF, 0x00}, {0xFF, 0x00} : }, : .dq_map[DDR_CH1] = { : {0x33, 0xCC}, {0x00, 0xCC}, {0x33, 0xCC}, : //{0x33, 0x00}, {0xFF, 0x00}, {0xFF, 0x00} : }, : : /* : * DQS CPU<>DRAM map Ch0 and Ch1. Each array entry represents a : * mapping of a dq bit on the CPU to the bit it's connected to on : * the memory part. The array index represents the dqs bit number : * on the memory part, and the values in the array represent which : * pin on the CPU that DRAM pin connects to. : */ : .dqs_map[DDR_CH0] = {0, 1, 2, 3, 4, 5, 6, 7}, : .dqs_map[DDR_CH1] = {0, 1, 2, 3, 4, 5, 6, 7},
Does this apply at all?
I checked with Jeremy and none of our boards use LPDDR, so I've removed them.
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 52: DRAM
This is wrong. […]
It seems all of these are copied from the header (error including). I've just gone ahead and removed them.
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 57: These will typically be the following : * values for Cannon Lake : { 80, 40, 40, 40, 30 }
Why copy this comment?
Comment from header. Removed from board.
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 62: /* : * Indicates whether memory is interleaved. : * Set to 1 for an interleaved design, : * set to 0 for non-interleaved design. : */
I think this is pretty obvious.
Comment from header. Removed from board.