Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36091 )
Change subject: mb/intel/tigerlake_rvp: Do initial mainboard commit ......................................................................
Patch Set 8:
(8 comments)
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... File src/mainboard/intel/tigerlake_rvp/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... PS8, Line 3: remove.
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... File src/mainboard/intel/tigerlake_rvp/bootblock.c:
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... PS8, Line 18: #include <soc/gpio.h> : #include <variant/gpio.h> All this gpio.h boilerplate makes it quite a bit harder to read. Can't you use the makefile to link in the gpio configuration you need instead of cascading through headers to end up using the baseboard gpio's?
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... File src/mainboard/intel/tigerlake_rvp/chromeos.c:
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... PS8, Line 15: : #include <arch/acpi.h> needed?
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... File src/mainboard/intel/tigerlake_rvp/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... PS8, Line 53: Scope (_SB.PCI0.LPCB) : { : /* ACPI code for EC SuperIO functions */ : #include <ec/google/chromeec/acpi/superio.asl> : /* ACPI code for EC functions */ : #include <ec/google/chromeec/acpi/ec.asl> : } use less indentation?
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... File src/mainboard/intel/tigerlake_rvp/romstage_fsp_params.c:
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... PS8, Line 30: static is not called here?
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... File src/mainboard/intel/tigerlake_rvp/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... PS8, Line 111: *__attribute__((weak)) : variant_early_gpio_table(size_t *num) Not empty weak functions are a bad idea.
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... File src/mainboard/intel/tigerlake_rvp/variants/tgl_up3/gpio.c:
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... PS8, Line 113: : *num = ARRAY_SIZE(early_gpio_table); : return early_gpio_table; *num 0 and return NULL ?
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... File src/mainboard/intel/tigerlake_rvp/variants/tgl_up3/memory.c:
https://review.coreboot.org/c/coreboot/+/36091/8/src/mainboard/intel/tigerla... PS8, Line 93: __weak variant_memory_params non empty __weak functions are a bad idea! Who is overriding this?
-
Arthur Heymans (Code Review)