Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46265 )
Change subject: mb/intel/adlrvp: Add ADL-P ramstage mainboard code ......................................................................
Patch Set 5: Code-Review+1
(6 comments)
https://review.coreboot.org/c/coreboot/+/46265/5/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/mainboard.c:
https://review.coreboot.org/c/coreboot/+/46265/5/src/mainboard/intel/adlrvp/... PS5, Line 14: /* sku{0..255} */ If you make `sku_id` an uint8_t, this comment can be dropped.
https://review.coreboot.org/c/coreboot/+/46265/5/src/mainboard/intel/adlrvp/... PS5, Line 15: 255 Is this supposed to be hardcoded?
https://review.coreboot.org/c/coreboot/+/46265/5/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/variants/adlrvp_p/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/46265/5/src/mainboard/intel/adlrvp/... PS5, Line 72: register "SataSalpSupport" = "1" : : register "SataPortsEnable[0]" = "1" : register "SataPortsEnable[1]" = "1" : register "SataPortsEnable[2]" = "1" : register "SataPortsEnable[3]" = "1" : : register "SataPortsDevSlp[0]" = "1" : register "SataPortsDevSlp[1]" = "1" : register "SataPortsDevSlp[2]" = "1" : register "SataPortsDevSlp[3]" = "1" Move these settings under the SATA PCI device scope?
Also, you can initialize the arrays like this:
register "SataPortsEnable" = "{ [0] = 1, [1] = 1, [2] = 1, [3] = 1, }"
register "SataPortsDevSlp" = "{ [0] = 1, [1] = 1, [2] = 1, [3] = 1, }"
https://review.coreboot.org/c/coreboot/+/46265/5/src/mainboard/intel/adlrvp/... PS5, Line 91: s0ix_enable Does S0ix entry work with this patch?
https://review.coreboot.org/c/coreboot/+/46265/5/src/mainboard/intel/adlrvp/... PS5, Line 104: PchSerialIoPci GSPI1 is disabled in the devicetree
https://review.coreboot.org/c/coreboot/+/46265/5/src/mainboard/intel/adlrvp/... PS5, Line 105: [PchSerialIoIndexGSPI2] = PchSerialIoDisabled, GSPI2 not in the devicetree?