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 7:
(26 comments)
Patch Set 7:
Please add Documentation as well:
- how to access the flash IC
- a recommended flashing method
- a picture of the mainboard
- debug ports
- known issues (if any)
- Additional notes useful for developers
I'm accessing with dediprog. For now dediprog. Should I create a document folder for these?
https://review.coreboot.org/c/coreboot/+/33993/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33993/3//COMMIT_MSG@17 PS3, Line 17:
Please note it somewhere. Preferably in the commit message.
Ok, will do.
https://review.coreboot.org/c/coreboot/+/33993/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33993/7//COMMIT_MSG@15 PS7, Line 15: a SIO, fintek f81803a
through a Fintek F81803A SIO.
Will do.
https://review.coreboot.org/c/coreboot/+/33993/7//COMMIT_MSG@15 PS7, Line 15: actual
filler word. doesn't add any information here.
Will remove
https://review.coreboot.org/c/coreboot/+/33993/7//COMMIT_MSG@18 PS7, Line 18: Both versions tested and boot to Linux using SeaBIOS
I'm confused. […]
I know that it supports both boards I have, by simply replacing the select SOC_AMD_MERLINFALCON with SOC_AMD_STONEYRIDGE_FP4 Will see how to rework it.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/bootblock/OemCustomize.c:
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 18: #define FILECODE PROC_GNB_PCIE_FAMILY_0X15_F15PCIECOMPLEXCONFIG_FILECODE
Can we get rid of this FILECODE stuff.
Possibly... will try.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 28: #define SERIAL_DEV PNP_DEV(0x4e, F81803A_SP1)
This and fintek includes seem unnecessary, since you are not doing any PNP config actions.
I was in the past, code moved elsewhere and I missed to remove it. Will do.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 31: static void full_serial(unsigned int base_port, unsigned int io_enable)
full ?
This code is in the process of changing.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 36: temp |= DECODE_ENABLE_SERIAL_PORT0;
Probably wrong
I was doing it only for com port 0, and it's correct. However, code is changing to make it more generic and support ports 0 and 1, as padmelon has both.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 38: reg = inb(base_port + UART8250_MCR);
arch/io. […]
Not sure what you want here, I copied this from an Intel code that also used UART8250_MCR. And it builds fine.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 39: reg |= UART8250_MCR_DTR | UART8250_MCR_RTS;
Why? Hardware flow control is not desired for boot firmware.
It's needed for my setup, I'll get no serial if I don't enable it. Maybe if I had a modified serial cable that does the trick internally to the socket (RTS to CTS, and so on...) it would work, but I don't have such a cable. And unfortunately the cable I bought is solid plastic on the connector, I can't do the solder on it. If I had the problem, others might. On a final product, this code could be drop. Would it be better if I add a comment saying so?
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 34: device pci 2.0 on end # PCIe Host Bridge
Is this really a PCI bridge or a "placeholder" to configure 0:2.1 to 0:2. […]
Code copied from previous owner, will check schematics.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 35: device pci 2.1 on end # No x4 PCIe slot
Slot or no slot?
same
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 37: device pci 2.3 on end # NC
If not routed, why on?
same
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 43: device pci 10.0 on end # USB
xHCI ?
same
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 45: device pci 12.0 on end # USB
EHCI ?
same
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 51: device i2c 52 on end
Contradicts SPD map above.
Not sure I get it. Please explain.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/fan_init.c:
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 24: #include <soc/pci_devs.h>
some spurious includes there?
Maybe. Some of the code that was initially in this file was moved to fintek folder. Will check.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 30: u8 cpu_boudaries[FINTEK_BOUNDARIES_SIZE] = {
static […]
probably static. will fix.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/gpio.h:
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 19: const struct soc_amd_gpio *early_gpio_table(size_t *size);
Missing include for the struct.
Will investigate. Maybe this file isn't needed anymore, or maybe the file that includes it already included what is needed.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/gpio.c:
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 29: const struct soc_amd_gpio gpio_set_stage_reset[] = {
static
And below too. Will fix.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/irq_tables.c:
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 41: unsigned long write_pirq_routing_table(unsigned long addr)
In general, I'd just submit without MP and PIRQ tables since they are probably not much tested at al […]
Only test so far was booting to Linux.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 65: pirq->rtr_devfn = PCI_DEVFN(0x14, 4);
I don't remember what this field in PIRQ structure is supposed to be. But 0:14. […]
Will check.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 79: /* pci bridge */
Hmm.. […]
Maybe, need to check. Code started by Marc, I did not review some sections.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/mainboard.c:
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 35: * MP Tables. TODO: Make ACPI use these values too.
Well ACPI is the more important one to get correct. Perhaps just drop MP and PIRQ table support.
These are about programming the registers before loading OS, not about creating a table for the OS. However, it's another table I did not check for correctness.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... File src/mainboard/amd/padmelon/mptable.c:
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 116: /* PCI slots */
Where are they?
Will check... this section of code was not created by me.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 143: PCI_INT(0x0, 0x15, 0x0, 0x10);
Is pci 0:15. […]
Will check