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 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. […]
The AGESA, PSP and video binaries are not yet merged, and apparently won't be for a while. Need some new documentation from AMD.
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. […]
For a while I physically had both versions of padmelon. I have to release padmelon 006770F00 to prove code builds with no errors, thus allowing for other code (specially SIO) to be merged. As it's the same SIO in both versions, this works. However, the final intent is merlinfalcon, which unfortunately will depend on AMD.
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. […]
Ok, will do.
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. […]
Partially from Marc's work and partially from AMD's gardenia.
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.
I copy from Marc's code. Might be able to remove it now.
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. […]
As good as I could make based on Marc's work. I have not double check against schematic though.