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:
(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: and
an
Thanks
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 64: Oddly enough,
Remove the commentary?
I don't think so, as this is the opposite of what is standard. The index address must have bit 0 set, and the data address is the next address. It could have been 221, 223 or 225. 2277 would be invalid because 228 would be outside the 8 bytes IO window. OTOH, I do have code in place that forces bit 0 to be set, so I could use an even address... with the caveat that 226 would be invalid (would become 227).
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.
Otherwise it won't even come out of reset...
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.
Probably... as it's not working under windows and I only have embedded Linux installed, I can't actually test. Before you ask, Windows problem is ACPI, and will be fixed some time in the future.
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
Thanks
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? […]
Padmelon has com1 and com2, but com1 is what is used for serial debug. Might be a good idea to have it based out of an input base address and run the code for both... I needed it because of my serial cable that does not have the internal jumpers that it's used as a work around, and my serial to USB dongle required all signals. Will do.
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?
Yes, boundaries are the threshold temperature between 2 sections, each with a different fan speed.
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 30: cpu_boudaries
boundaries
Thanks
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?
check_status() does the actual check of the statues, and will print a message if status is not 0. However, it's only fatal error is status is negative, positive values are warnings and fan setup continues.
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?
Will do.
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 131: register
Maybe describe WHAT register is being restored?
Will do.
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?
Believe so, though I don't remember it out of my mind. Will look.
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 65: PCI_DEVFN(0x14, 4);
Macro?
There might already be one for it. Will check
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 69: 0x1002
Don't we have #defines for these?
Not sure, will check.
https://review.coreboot.org/c/coreboot/+/33993/3/src/mainboard/amd/padmelon/... PS3, Line 80: PCI_DEVFN(0x14, 4)
macro?
Same as above