Subrata Banik 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:
(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.
Ack
https://review.coreboot.org/c/coreboot/+/46265/5/src/mainboard/intel/adlrvp/... PS5, Line 15: 255
Is this supposed to be hardcoded?
This is what it is today but very nice feedback, ideally this is to uniquely identify the SKU, i believe we are good with board ID check. adding that now.
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? […]
Ack
https://review.coreboot.org/c/coreboot/+/46265/5/src/mainboard/intel/adlrvp/... PS5, Line 91: s0ix_enable
Does S0ix entry work with this patch?
Yes Angel, S0ix use case is not fully enabled but system is going to lower c-state and system is going to lower system states as well
https://review.coreboot.org/c/coreboot/+/46265/5/src/mainboard/intel/adlrvp/... PS5, Line 104: PchSerialIoPci
GSPI1 is disabled in the devicetree
Ack
https://review.coreboot.org/c/coreboot/+/46265/5/src/mainboard/intel/adlrvp/... PS5, Line 105: [PchSerialIoIndexGSPI2] = PchSerialIoDisabled,
GSPI2 not in the devicetree?
i guess you meant GSPI3 not 2 😊