Angel Pons 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 3:
(12 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
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 I don't think flashrom supports ADL yet.
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();
gpio.c also has comments.
https://review.coreboot.org/c/coreboot/+/46054/2/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/chromeos.fmd:
https://review.coreboot.org/c/coreboot/+/46054/2/src/mainboard/intel/adlrvp/... PS2, Line 2: SI_ALL@0x0 0x1081000 { Specifying the offset and size of everything isn't necessary. For CBFSes, the size should be unconstrained so that they fill all available space. I think you can use BUILD_TIMELESS=1 to check if the removals change the binary.
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
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. For CBFSes, the size should be unconstrained so that they fill all available space. I think you can use BUILD_TIMELESS=1 to check if the removals change the binary.
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: No ACPI?
https://review.coreboot.org/c/coreboot/+/46054/2/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/variants/adlrvp_p/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/46054/2/src/mainboard/intel/adlrvp/... PS2, Line 28: device pci 0d.1 off end # USB xDCI (OTG) nit: please align the `end` keywords
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.c in ramstage?
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? 😉
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?
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:
void variant_configure_early_gpio_pads(void);