Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33993 )
Change subject: mainboard/amd: Add padmelon board code ......................................................................
Patch Set 7:
(22 comments)
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.
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.
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 ?
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 36: temp |= DECODE_ENABLE_SERIAL_PORT0; Probably wrong
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 38: reg = inb(base_port + UART8250_MCR); arch/io.h
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.
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.4 ?
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?
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?
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 43: device pci 10.0 on end # USB xHCI ?
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 45: device pci 12.0 on end # USB EHCI ?
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 51: device i2c 52 on end Contradicts SPD map above.
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?
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 30: u8 cpu_boudaries[FINTEK_BOUNDARIES_SIZE] = { static boundaries ?
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.
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
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 all.
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.4 used to be the parallel PCI bus bridge.
https://review.coreboot.org/c/coreboot/+/33993/7/src/mainboard/amd/padmelon/... PS7, Line 79: /* pci bridge */ Hmm.. bogus entries?
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.
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?
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.x stil there?