| Message ID | 20230220194927.476708-7-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | x86: Various minor enhancements for coreboot | expand |
+Andy Hi Simon, On Tue, Feb 21, 2023 at 3:49 AM Simon Glass <sjg@chromium.org> wrote: > > When coreboot does not pass a UART in its sysinfo struct, there is no > easy way to find it out. Add a way to specify known UARTs so we can > find them without needing help from coreboot. > > Since coreboot does not actually init the serial device when serial is > disabled, it is not possible to make it add this information to the > sysinfo table. > > Also, we cannot use the class information, since we don't know which > UART is being used. For example, with Alder Lake there are two: > > 00.16.00 0x8086 0x51e0 Simple comm. controller 0x80 > 00.1e.00 0x8086 0x51a8 Simple comm. controller 0x80 > > In our case the second one is the right one, but thre is no way to > distinguish it from the first one without using the device ID. > > If we have Adler Lake hardware which uses a different UART, we could > perhaps look at the ACPI tables, or the machine information passed in > the SMBIOS tables. > > This was discussed previously before: [1] > > [1] https://patchwork.ozlabs.org/project/uboot/patch/ > 20210407163159.3.I967ea8c85e009f870c7aa944372d32c990f1b14a@changeid/ > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > arch/x86/dts/coreboot.dts | 4 ++ > drivers/serial/serial_coreboot.c | 69 ++++++++++++++++++++++++++++---- > include/pci_ids.h | 3 ++ > 3 files changed, 68 insertions(+), 8 deletions(-) > Last time we discussed this, both Andy and I thought this was a hack. I cited Andy's point below: "What coreboot should do is either provide serial information or SPCR ACPI table. Otherwise if it does not provide it, I think it's on purpose, and we have to respect this." So maybe somehow we should parse the SPCR ACPI table instead? Regards, Bin
Hi Bin, On Mon, 20 Mar 2023 at 20:56, Bin Meng <bmeng.cn@gmail.com> wrote: > > +Andy > > Hi Simon, > > On Tue, Feb 21, 2023 at 3:49 AM Simon Glass <sjg@chromium.org> wrote: > > > > When coreboot does not pass a UART in its sysinfo struct, there is no > > easy way to find it out. Add a way to specify known UARTs so we can > > find them without needing help from coreboot. > > > > Since coreboot does not actually init the serial device when serial is > > disabled, it is not possible to make it add this information to the > > sysinfo table. > > > > Also, we cannot use the class information, since we don't know which > > UART is being used. For example, with Alder Lake there are two: > > > > 00.16.00 0x8086 0x51e0 Simple comm. controller 0x80 > > 00.1e.00 0x8086 0x51a8 Simple comm. controller 0x80 > > > > In our case the second one is the right one, but thre is no way to > > distinguish it from the first one without using the device ID. > > > > If we have Adler Lake hardware which uses a different UART, we could > > perhaps look at the ACPI tables, or the machine information passed in > > the SMBIOS tables. > > > > This was discussed previously before: [1] > > > > [1] https://patchwork.ozlabs.org/project/uboot/patch/ > > 20210407163159.3.I967ea8c85e009f870c7aa944372d32c990f1b14a@changeid/ > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > arch/x86/dts/coreboot.dts | 4 ++ > > drivers/serial/serial_coreboot.c | 69 ++++++++++++++++++++++++++++---- > > include/pci_ids.h | 3 ++ > > 3 files changed, 68 insertions(+), 8 deletions(-) > > > > Last time we discussed this, both Andy and I thought this was a hack. > I cited Andy's point below: > > "What coreboot should do is either provide serial information or SPCR > ACPI table. Otherwise if it does not provide it, I think it's on > purpose, and we have to respect this." We cannot change what coreboot does. I have written up a design for it but it seems unlikely that anything will happen there in the short term, perhaps ever. I will keep pushing. In the meantime U-Boot cannot be used as a coreboot payload in this (common) situation. So we do need a solution. > > So maybe somehow we should parse the SPCR ACPI table instead? I don't see that table present, but there is DBG2 so I will take a look. Regards, Simon
diff --git a/arch/x86/dts/coreboot.dts b/arch/x86/dts/coreboot.dts index d21978d6e09..3cd23b797ab 100644 --- a/arch/x86/dts/coreboot.dts +++ b/arch/x86/dts/coreboot.dts @@ -14,6 +14,8 @@ /include/ "rtc.dtsi" #include "tsc_timer.dtsi" +#include <dt-bindings/pci/pci.h> +#include <pci_ids.h> / { model = "coreboot x86 payload"; @@ -34,6 +36,8 @@ pci { compatible = "pci-x86"; u-boot,dm-pre-reloc; + u-boot,pci-pre-reloc = < + PCI_VENDEV(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ADP_P_UART0) >; }; serial: serial { diff --git a/drivers/serial/serial_coreboot.c b/drivers/serial/serial_coreboot.c index de09c8681f5..4d960bbe04d 100644 --- a/drivers/serial/serial_coreboot.c +++ b/drivers/serial/serial_coreboot.c @@ -11,19 +11,72 @@ #include <serial.h> #include <asm/cb_sysinfo.h> +static const struct pci_device_id ids[] = { + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_APL_UART2) }, + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ADP_P_UART0) }, + {}, +}; + +/* + * Coreboot only sets up the UART if it uses it and doesn't bother to put the + * details in sysinfo if it doesn't. Try to guess in that case, using devices + * we know about + * + * @plat: Platform data to fill in + * @return 0 if found, -ve if no UART was found + */ +static int guess_uart(struct ns16550_plat *plat) +{ + struct udevice *bus, *dev; + ulong addr; + int index; + int ret; + + ret = uclass_first_device_err(UCLASS_PCI, &bus); + if (ret) + return ret; + index = 0; + ret = pci_bus_find_devices(bus, ids, &index, &dev); + if (ret) + return ret; + addr = dm_pci_read_bar32(dev, 0); + plat->base = addr; + plat->reg_shift = 2; + plat->reg_width = 4; + plat->clock = 1843200; + plat->fcr = UART_FCR_DEFVAL; + plat->flags = 0; + + return 0; +} + static int coreboot_of_to_plat(struct udevice *dev) { struct ns16550_plat *plat = dev_get_plat(dev); struct cb_serial *cb_info = lib_sysinfo.serial; - plat->base = cb_info->baseaddr; - plat->reg_shift = cb_info->regwidth == 4 ? 2 : 0; - plat->reg_width = cb_info->regwidth; - plat->clock = cb_info->input_hertz; - plat->fcr = UART_FCR_DEFVAL; - plat->flags = 0; - if (cb_info->type == CB_SERIAL_TYPE_IO_MAPPED) - plat->flags |= NS16550_FLAG_IO; + if (cb_info) { + plat->base = cb_info->baseaddr; + plat->reg_shift = cb_info->regwidth == 4 ? 2 : 0; + plat->reg_width = cb_info->regwidth; + plat->clock = cb_info->input_hertz; + plat->fcr = UART_FCR_DEFVAL; + plat->flags = 0; + if (cb_info->type == CB_SERIAL_TYPE_IO_MAPPED) + plat->flags |= NS16550_FLAG_IO; + } else if (CONFIG_IS_ENABLED(PCI)) { + int ret; + + ret = guess_uart(plat); + if (ret) { + /* + * Returning an error will cause U-Boot to complain that + * there is no UART, which may panic. So stay silent and + * pray that the video console will work. + */ + log_debug("Cannot detect UART\n"); + } + } return 0; } diff --git a/include/pci_ids.h b/include/pci_ids.h index 88b0a640458..5ae1b9b7fb6 100644 --- a/include/pci_ids.h +++ b/include/pci_ids.h @@ -2992,6 +2992,9 @@ #define PCI_DEVICE_ID_INTEL_UNC_R3QPI1 0x3c45 #define PCI_DEVICE_ID_INTEL_JAKETOWN_UBOX 0x3ce0 #define PCI_DEVICE_ID_INTEL_IOAT_SNB 0x402f +#define PCI_DEVICE_ID_INTEL_ADP_P_UART0 0x51a8 +#define PCI_DEVICE_ID_INTEL_ADP_P_UART1 0x51a9 +#define PCI_DEVICE_ID_INTEL_APL_UART2 0x5ac0 #define PCI_DEVICE_ID_INTEL_5100_16 0x65f0 #define PCI_DEVICE_ID_INTEL_5100_19 0x65f3 #define PCI_DEVICE_ID_INTEL_5100_21 0x65f5
When coreboot does not pass a UART in its sysinfo struct, there is no easy way to find it out. Add a way to specify known UARTs so we can find them without needing help from coreboot. Since coreboot does not actually init the serial device when serial is disabled, it is not possible to make it add this information to the sysinfo table. Also, we cannot use the class information, since we don't know which UART is being used. For example, with Alder Lake there are two: 00.16.00 0x8086 0x51e0 Simple comm. controller 0x80 00.1e.00 0x8086 0x51a8 Simple comm. controller 0x80 In our case the second one is the right one, but thre is no way to distinguish it from the first one without using the device ID. If we have Adler Lake hardware which uses a different UART, we could perhaps look at the ACPI tables, or the machine information passed in the SMBIOS tables. This was discussed previously before: [1] [1] https://patchwork.ozlabs.org/project/uboot/patch/ 20210407163159.3.I967ea8c85e009f870c7aa944372d32c990f1b14a@changeid/ Signed-off-by: Simon Glass <sjg@chromium.org> --- arch/x86/dts/coreboot.dts | 4 ++ drivers/serial/serial_coreboot.c | 69 ++++++++++++++++++++++++++++---- include/pci_ids.h | 3 ++ 3 files changed, 68 insertions(+), 8 deletions(-)