Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33993 )
Change subject: mainboard/amd: Add padmelon board code ......................................................................
Patch Set 3:
(15 comments)
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/Kconfig:
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 64: Oddly enough, Remove the commentary?
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 64: and an
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 68: # Don't use AMD's Secure OS : config USE_PSPSECUREOS : def_bool n Ok.
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/bootblock/OemCustomize.c:
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 143: 0x00172051, 0x001721C7, 0x00172222, 0x00172310, Does padmelon use the same codec as gardenia? If not, these should probably be updated.
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 29: UAT UART
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 37: 0x3fb Use the register values in drivers/uart/8350reg.h for these values? Is padmelon restricted to using this serial port at 3f8? If not, consider not hardcoding the base address.
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/fan_init.c:
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 29: celcius celsius?
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 30: cpu_boudaries boundaries
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 106: return Maybe consider returning a value so you can print a warning that the setup wasn't completed?
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 115: 0x0220, 0xff Maybe some #defines to say what these are?
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 131: register Maybe describe WHAT register is being restored?
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/irq_tables.c:
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 51: /* Align the table to be 16 byte aligned. */ : addr += 15; : addr &= ~15; Isn't there a macro to do this?
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 65: PCI_DEVFN(0x14, 4); Macro?
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 69: 0x1002 Don't we have #defines for these?
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 80: PCI_DEVFN(0x14, 4) macro?