Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33993 )
Change subject: mainboard/amd: Add padmelon board code ......................................................................
Patch Set 10:
(5 comments)
di
https://review.coreboot.org/c/coreboot/+/33993/10/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/BiosCallOuts.c:
https://review.coreboot.org/c/coreboot/+/33993/10/src/mainboard/amd/padmelon... PS10, Line 16: #include <amdblocks/agesawrapper.h> : #include <amdblocks/BiosCallOuts.h> : #include <soc/southbridge.h> : #include <stdlib.h> : #include <string.h>
do you use all of them ?
Currently this is just a place holder with only an empty function. I could remove most of them, except BiosCallOuts.
https://review.coreboot.org/c/coreboot/+/33993/10/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/acpi/routing.asl:
https://review.coreboot.org/c/coreboot/+/33993/10/src/mainboard/amd/padmelon... PS10, Line 18: DefinitionBlock ("DSDT.AML","DSDT",0x01,"XXXXXX","XXXXXXXX",0x00010001
#include <arch/acpi.h> […]
Not sure why it's not there, it's on my local main tree. Will fix. Here's my local branch: #include <arch/acpi.h> DefinitionBlock ( "DSDT.AML", /* Output filename */ "DSDT", /* Signature */ 0x02, /* DSDT Revision, needs to be 2 for 64bit */ "AMD ", /* OEMID */ "COREBOOT", /* TABLE ID */ 0x00010001 /* OEM Revision */ ) { /* Start of ASL file */
https://review.coreboot.org/c/coreboot/+/33993/10/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/fan_init.c:
https://review.coreboot.org/c/coreboot/+/33993/10/src/mainboard/amd/padmelon... PS10, Line 17: include <console/console.h>
maybe not used
Will remove. Was being used for debug.
https://review.coreboot.org/c/coreboot/+/33993/10/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/gpio.h:
https://review.coreboot.org/c/coreboot/+/33993/10/src/mainboard/amd/padmelon... PS10, Line 21: size_t
not defined
Really? Builds fine for me, I thought it was defined in some include from gpio_banks.h. Will check what's happening.
https://review.coreboot.org/c/coreboot/+/33993/10/src/mainboard/amd/padmelon... File src/mainboard/amd/padmelon/gpio.c:
https://review.coreboot.org/c/coreboot/+/33993/10/src/mainboard/amd/padmelon... PS10, Line 20: #include <soc/gpio.h>
you may include <gpio. […]
They are 2 different files with same name. One (below) is local, the other is a file under soc/amd/stoneyridge/include/soc. However, if I make the local include the remote gpio.h instead of gpio_banks.h (as it's currently), it should be ok as the remote one does include gpio_banks.h. This way I remove the line you marked, and only include the local gpio.h.