Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33993 )
Change subject: mainboard/amd: Add padmelon board code ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/c/coreboot/+/33993/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33993/3//COMMIT_MSG@9 PS3, Line 9: With almost everything in place, it's time to add padmelon board. This is kind of wordy IMO. BTW, what is not in place yet?
https://review.coreboot.org/c/coreboot/+/33993/3//COMMIT_MSG@11 PS3, Line 11: This code was intended for : merlinfalcon version, but as there are some legal issues (documentation) : blocking its merge to coreboot, it'll be released for 00670F00, Why not simply say this is for the 670F00 version. A follow up change will add merlin falcon. No point in confusing the commit message with what you started working on before changing directions.
https://review.coreboot.org/c/coreboot/+/33993/3//COMMIT_MSG@15 PS3, Line 15: Even both use fanned versions of SOC, the actual fan control : is done through a SIO, fintek f81803a. I would remove this, and probably not talk about fanned parts at all. "fanned" has no relevance for fan control. Fanned is the opposite of fanless, which is the term used for low power SKUs and do not require the system designed with a fan.
The only reason that terminology is in coreboot is that different SMU firmware is required.
https://review.coreboot.org/c/coreboot/+/33993/3//COMMIT_MSG@17 PS3, Line 17: I assume you copied the majority of these files from another board, then made a handful of changes. Can you specify what board they were copied from, then what the changes were?
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 29: CPPFLAGS_common += -I$(src)/vendorcode/amd/pi/00670F00 : CPPFLAGS_common += -I$(src)/vendorcode/amd/pi/00670F00/Proc/Fch/Common : CPPFLAGS_common += -I$(src)/vendorcode/amd/pi/00670F00/Proc/Common : CPPFLAGS_common += -I$(src)/vendorcode/amd/pi/00670F00/binaryPI Maybe I need to go back and look at your soc code. These don't belong in mainboard.
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/gpio.c:
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 29: onst struct soc_amd_gpio gpio_set_stage_reset[] = { : /* NFC PU */ : PAD_GPO(GPIO_64, HIGH), : /* PCIe presence detect */ : PAD_GPO(GPIO_64, HIGH), : /* MUX for Power Express Eval */ : PAD_GPI(GPIO_116, PULL_DOWN), : /* SD power */ : PAD_GPO(GPIO_64, HIGH), : }; : : const struct soc_amd_gpio gpio_set_stage_ram[] = { : /* BT radio disable */ : PAD_GPO(GPIO_14, HIGH), : /* NFC wake */ : PAD_GPO(GPIO_65, HIGH), : /* Webcam */ : PAD_GPO(GPIO_66, HIGH), : /* GPS sleep */ : PAD_GPO(GPIO_70, HIGH), : } Are these real values? They look like stripped down copies of Gardenia. It would be very unusual for an AMD Embedded design to look anything like a Client design.