Angel Pons 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 2:
(13 comments)
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... File src/mainboard/system76/oryp5/acpi/gpe.asl:
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 8: One 1
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?
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 26: register "SaGv" = "SaGv_Enabled" I'm not sure if this has any effect on Halo
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 34: PchSerialIoPci SkipInit?
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.0)
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 79: Enable Nope
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.c
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... File src/mainboard/system76/oryp5/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 25: // Enable DMIC microphone on ALC1220 : 0x02050036, : 0x02042a6a, What does the microphone have to do with beep?
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
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?
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 52: DRAM This is wrong. Does this comment actually help?
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?
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.