Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46054 )
Change subject: mb/intel/adlrvp: Add initial ADL-P mainboard code ......................................................................
Patch Set 4:
(10 comments)
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/Kconfig:
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... PS3, Line 54: ADL_EC
This choice doesn't need a name, see CB:43836
Ack
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/board_info.txt:
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... PS3, Line 6: y
Nice, but I see that cros-flashrom treats everything starting from Sunrise Point the same.
Ideally flashrom don't expect to see any change as we are in SPI HW-seq mode since SPT PCH where SPI access method for basic operation remain as is.
There's differences in descriptor contents starting from Cannon Point (300-series PCH), and upstream flashrom handles these chipsets slightly differently to report the correct information. flashrom can use the descriptor contents as a layout file, but needs to be able to parse it correctly.
For Descriptor related changes we have IFD and https://review.coreboot.org/q/topic:%22IFDTOOL%22+(status:open%20OR%20status...) lately fixed.
Unfortunately I don't have any SPI Programming Guide for ADL. I see that TGL-LP has the FLMAP3 register, which contains the descriptor Major and Minor revision IDs.
https://review.coreboot.org/c/coreboot/+/44818
If TGL-LP and ADL have the same descriptor revision IDs, then I imagine the rest of fields would be the same, which would be good enough to add descriptor support to upstream flashrom.
I have added those now as part of IFD latest, if you have TGL coreboot or downloaded from CPFE, you could give a try with latest IFD tool
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/bootblock.c:
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... PS3, Line 10: const struct pad_config *pads; : size_t num; : : pads = variant_early_gpio_table(&num); : gpio_configure_pads(pads, num);
I would replace this with a single function call: variant_configure_early_gpio_pads(); […]
Ack
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/chromeos.fmd:
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... PS3, Line 1: FLASH@0xfe000000 0x2000000 {
there's some FLASH_xM aliases that could be used here
Ack
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... PS3, Line 2: SI_ALL@0x0 0x1081000 {
Specifying the offset and size of everything isn't necessary. […]
Ack
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... PS3, Line 16:
OK, please mention this in the commit message.
Ack
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/variants/adlrvp_p/gpio.c:
PS3:
nit: rename to early_gpio.c to differentiate from gpio. […]
Ack
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... PS3, Line 15: const struct pad_config *variant_early_gpio_table(size_t *num) : { : *num = ARRAY_SIZE(early_gpio_table); : return early_gpio_table; : }
Instead of passing the arrays, I would simply make this a function that programs the table above:
void variant_configure_early_gpio_pads(void) { gpio_configure_pads(early_gpio_table, ARRAY_SIZE(early_gpio_table)); }
This is much simpler than passing pointers around, isn't it? 😉
Awesome, this is how it should be. Thanks
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/variants/baseboard/include/baseboard/ec.h:
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... PS3, Line 44: #define MAINBOARD_EC_S0IX_WAKE_EVENTS (MAINBOARD_EC_S3_WAKE_EVENTS)
Is this used?
removed for now, will add with ramstage changes
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/46054/3/src/mainboard/intel/adlrvp/... PS3, Line 13: const struct pad_config *variant_early_gpio_table(size_t *num);
Update the comment and this prototype: […]
Ack