diff mbox series

[06/13] x86: Allow locating UARTs by device ID

Message ID 20230220194927.476708-7-sjg@chromium.org
State Superseded
Delegated to: Bin Meng
Headers show
Series x86: Various minor enhancements for coreboot | expand

Commit Message

Simon Glass Feb. 20, 2023, 7:49 p.m. UTC
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(-)

Comments

Bin Meng March 20, 2023, 7:56 a.m. UTC | #1
+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
Simon Glass March 23, 2023, 6:28 p.m. UTC | #2
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 mbox series

Patch

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