[PATCH 00/17] misc: Some more misc patches

[PATCH 00/17] misc: Some more misc patches

[Simon Glass]

Various issues were discovered in getting Chromium OS verified boot
running on top of coreboot, booting into U-Boot.

Improvements include:
- enable serial console if even coreboot doesn't
- enable cache always
- show the BIOS date since Chromium OS's coreboot doesn't have a version
- update docs
- allow SPI driver to work without a PCH
- fix mtrr command when multi-processing (CONFIG_MP) is disabled
- add documentation about the memory map


Simon Glass (17):
  pci: Use const for pci_find_device_id() etc.
  x86: pci: Allow binding of some devices before relocation
  x86: Allow coreboot serial driver to guess the UART
  spi: ich: Don't require the PCH
  tpm: cr50: Drop unnecessary coral headers
  x86: Don't set up MTRRs if previously done
  x86: Update the MP constants to avoid conflicts
  x86: Do cache set-up by default when booting from coreboot
  x86: coreboot: Show the BIOS date
  x86: coral: Allow booting from coreboot
  x86: Add function comments to cb_sysinfo.h
  x86: coreboot: Use vendor in the Kconfig
  x86: coreboot: Enable the cbsysinfo command
  x86: coreboot: Document the memory map
  x86: Check ROM exists before building vboot
  dtoc: Check that a parent is not missing
  doc: Update documentation for cros-2021.04 release

 arch/x86/cpu/coreboot/Kconfig            |  3 +-
 arch/x86/cpu/i386/cpu.c                  |  2 +-
 arch/x86/dts/chromebook_coral.dts        |  2 +-
 arch/x86/dts/chromebook_samus.dts        |  2 +-
 arch/x86/include/asm/cb_sysinfo.h        | 16 ++++++
 arch/x86/include/asm/mp.h                | 12 +++--
 arch/x86/lib/init_helpers.c              |  5 +-
 board/coreboot/coreboot/Kconfig          | 12 +++--
 board/coreboot/coreboot/coreboot.c       |  3 ++
 board/google/chromebook_coral/coral.c    | 28 ++++++++++
 doc/board/coreboot/coreboot.rst          | 19 +++++++
 doc/chromium/run_vboot.rst               | 15 +++---
 doc/device-tree-bindings/pci/x86-pci.txt |  7 ++-
 drivers/pci/pci-uclass.c                 | 39 ++++++++++++--
 drivers/serial/serial_coreboot.c         | 68 +++++++++++++++++++++---
 drivers/spi/ich.c                        |  4 +-
 drivers/tpm/cr50_i2c.c                   |  2 -
 include/dt-bindings/pci/pci.h            | 12 +++++
 include/pci.h                            |  5 +-
 include/pci_ids.h                        |  1 +
 tools/dtoc/dtb_platdata.py               |  9 ++++
 tools/dtoc/test/dtoc_test_noparent.dts   | 32 +++++++++++
 tools/dtoc/test_dtoc.py                  | 10 ++++
 23 files changed, 264 insertions(+), 44 deletions(-)
 create mode 100644 include/dt-bindings/pci/pci.h
 create mode 100644 tools/dtoc/test/dtoc_test_noparent.dts

-- 
2.31.0.208.g409f899ff0-goog

[PATCH 01/17] pci: Use const for pci_find_device_id() etc.

[Simon Glass]

These functions don't modify the device-ID struct that is passed in, so
mark the argument as const, so the data structure can be declared that
way. This allows it to be placed in the rodata section.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/pci/pci-uclass.c | 6 +++---
 include/pci.h            | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index dfd54b339f4..dffe5297944 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -162,7 +162,7 @@ int dm_pci_bus_find_bdf(pci_dev_t bdf, struct udevice **devp)
 }
 
 static int pci_device_matches_ids(struct udevice *dev,
-				  struct pci_device_id *ids)
+				  const struct pci_device_id *ids)
 {
 	struct pci_child_plat *pplat;
 	int i;
@@ -179,7 +179,7 @@ static int pci_device_matches_ids(struct udevice *dev,
 	return -EINVAL;
 }
 
-int pci_bus_find_devices(struct udevice *bus, struct pci_device_id *ids,
+int pci_bus_find_devices(struct udevice *bus, const struct pci_device_id *ids,
 			 int *indexp, struct udevice **devp)
 {
 	struct udevice *dev;
@@ -199,7 +199,7 @@ int pci_bus_find_devices(struct udevice *bus, struct pci_device_id *ids,
 	return -ENODEV;
 }
 
-int pci_find_device_id(struct pci_device_id *ids, int index,
+int pci_find_device_id(const struct pci_device_id *ids, int index,
 		       struct udevice **devp)
 {
 	struct udevice *bus;
diff --git a/include/pci.h b/include/pci.h
index 5f36537b725..f2dfbda5b08 100644
--- a/include/pci.h
+++ b/include/pci.h
@@ -1070,7 +1070,7 @@ int pci_get_ff(enum pci_size_t size);
  * @devp:	Returns matching device if found
  * @return 0 if found, -ENODEV if not
  */
-int pci_bus_find_devices(struct udevice *bus, struct pci_device_id *ids,
+int pci_bus_find_devices(struct udevice *bus, const struct pci_device_id *ids,
 			 int *indexp, struct udevice **devp);
 
 /**
@@ -1082,7 +1082,7 @@ int pci_bus_find_devices(struct udevice *bus, struct pci_device_id *ids,
  * @devp:	Returns matching device if found
  * @return 0 if found, -ENODEV if not
  */
-int pci_find_device_id(struct pci_device_id *ids, int index,
+int pci_find_device_id(const struct pci_device_id *ids, int index,
 		       struct udevice **devp);
 
 /**
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 01/17] pci: Use const for pci_find_device_id() etc.

[Bin Meng]

On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
>
> These functions don't modify the device-ID struct that is passed in, so
> mark the argument as const, so the data structure can be declared that
> way. This allows it to be placed in the rodata section.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  drivers/pci/pci-uclass.c | 6 +++---
>  include/pci.h            | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 02/17] x86: pci: Allow binding of some devices before relocation

[Simon Glass]

At present only bridge devices are bound before relocation, to save space
in pre-relocation memory. In some cases we do actually want to bind a
device, e.g. because it provides the console UART. Add a devicetree
binding to support this.

Use the PCI_VENDEV() macro to encode the cell value. This is present in
U-Boot but not used, so move it to the binding header-file.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 doc/device-tree-bindings/pci/x86-pci.txt |  7 ++++-
 drivers/pci/pci-uclass.c                 | 33 +++++++++++++++++++++++-
 include/dt-bindings/pci/pci.h            | 12 +++++++++
 include/pci.h                            |  1 -
 4 files changed, 50 insertions(+), 3 deletions(-)
 create mode 100644 include/dt-bindings/pci/pci.h

diff --git a/doc/device-tree-bindings/pci/x86-pci.txt b/doc/device-tree-bindings/pci/x86-pci.txt
index 95e370b3e72..cf4e5ed595a 100644
--- a/doc/device-tree-bindings/pci/x86-pci.txt
+++ b/doc/device-tree-bindings/pci/x86-pci.txt
@@ -20,6 +20,10 @@ For PCI devices the following optional property is available:
 	output to be lost. This should not generally be used in production code,
 	although it is often harmless.
 
+- u-boot,pci-pre-reloc : List of vendor/device IDs to bind before relocation, even
+	if they are not bridges. This is useful if the device is needed (e.g. a
+	UART). The format is 0xvvvvdddd where d is the device ID and v is the
+	vendor ID.
 
 Example:
 
@@ -32,7 +36,8 @@ pci {
 		0x42000000 0x0 0xb0000000 0xb0000000 0 0x10000000
 		0x01000000 0x0 0x1000 0x1000 0 0xefff>;
 	u-boot,skip-auto-config-until-reloc;
-
+	u-boot,pci-pre-reloc = <
+		PCI_VENDEV(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_APL_UART2)>;
 
 	serial: serial at 18,2 {
 		reg = <0x0200c210 0 0 0 0>;
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index dffe5297944..ec3797746ba 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -19,6 +19,7 @@
 #if defined(CONFIG_X86) && defined(CONFIG_HAVE_FSP)
 #include <asm/fsp/fsp_support.h>
 #endif
+#include <dt-bindings/pci/pci.h>
 #include <linux/delay.h>
 #include "pci_internal.h"
 
@@ -673,6 +674,34 @@ static bool pci_match_one_id(const struct pci_device_id *id,
 	return false;
 }
 
+/**
+ * pci_need_device_pre_reloc() - Check if a device should be bound
+ *
+ * This checks a list of vendor/device-ID values indicating devices that should
+ * be bound before relocation.
+ *
+ * @bus: Bus to check
+ * @vendor: Vendor ID to check
+ * @device: Device ID to check
+ * @return true if the vendor/device is in the list, false if not
+ */
+static bool pci_need_device_pre_reloc(struct udevice *bus, uint vendor,
+				      uint device)
+{
+	u32 vendev;
+	int index;
+
+	for (index = 0;
+	     !dev_read_u32_index(bus, "u-boot,pci-pre-reloc", index,
+				 &vendev);
+	     index++) {
+		if (vendev == PCI_VENDEV(vendor, device))
+			return true;
+	}
+
+	return false;
+}
+
 /**
  * pci_find_and_bind_driver() - Find and bind the right PCI driver
  *
@@ -761,7 +790,9 @@ static int pci_find_and_bind_driver(struct udevice *parent,
 	 * precious memory space as on some platforms as that space is pretty
 	 * limited (ie: using Cache As RAM).
 	 */
-	if (!(gd->flags & GD_FLG_RELOC) && !bridge)
+	if (!(gd->flags & GD_FLG_RELOC) && !bridge &&
+	    !pci_need_device_pre_reloc(parent, find_id->vendor,
+				       find_id->device))
 		return log_msg_ret("notbr", -EPERM);
 
 	/* Bind a generic driver so that the device can be used */
diff --git a/include/dt-bindings/pci/pci.h b/include/dt-bindings/pci/pci.h
new file mode 100644
index 00000000000..e7290277b90
--- /dev/null
+++ b/include/dt-bindings/pci/pci.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * This header provides common constants for PCI bindings.
+ */
+
+#ifndef _DT_BINDINGS_PCI_PCI_H
+#define _DT_BINDINGS_PCI_PCI_H
+
+/* Encode a vendor and device ID into a single cell */
+#define PCI_VENDEV(v, d)	(((v) << 16) | (d))
+
+#endif /* _DT_BINDINGS_PCI_PCI_H */
diff --git a/include/pci.h b/include/pci.h
index f2dfbda5b08..fe7064b2a44 100644
--- a/include/pci.h
+++ b/include/pci.h
@@ -578,7 +578,6 @@ typedef int pci_dev_t;
 #define PCI_MASK_BUS(bdf)	((bdf) & 0xffff)
 #define PCI_ADD_BUS(bus, devfn)	(((bus) << 16) | (devfn))
 #define PCI_BDF(b, d, f)	((b) << 16 | PCI_DEVFN(d, f))
-#define PCI_VENDEV(v, d)	(((v) << 16) | (d))
 #define PCI_ANY_ID		(~0)
 
 /* Convert from Linux format to U-Boot format */
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 02/17] x86: pci: Allow binding of some devices before relocation

[Bin Meng]

Hi Simon,

On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
>
> At present only bridge devices are bound before relocation, to save space
> in pre-relocation memory. In some cases we do actually want to bind a
> device, e.g. because it provides the console UART. Add a devicetree
> binding to support this.
>
> Use the PCI_VENDEV() macro to encode the cell value. This is present in
> U-Boot but not used, so move it to the binding header-file.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  doc/device-tree-bindings/pci/x86-pci.txt |  7 ++++-
>  drivers/pci/pci-uclass.c                 | 33 +++++++++++++++++++++++-
>  include/dt-bindings/pci/pci.h            | 12 +++++++++
>  include/pci.h                            |  1 -
>  4 files changed, 50 insertions(+), 3 deletions(-)
>  create mode 100644 include/dt-bindings/pci/pci.h
>
> diff --git a/doc/device-tree-bindings/pci/x86-pci.txt b/doc/device-tree-bindings/pci/x86-pci.txt
> index 95e370b3e72..cf4e5ed595a 100644
> --- a/doc/device-tree-bindings/pci/x86-pci.txt
> +++ b/doc/device-tree-bindings/pci/x86-pci.txt
> @@ -20,6 +20,10 @@ For PCI devices the following optional property is available:
>         output to be lost. This should not generally be used in production code,
>         although it is often harmless.
>
> +- u-boot,pci-pre-reloc : List of vendor/device IDs to bind before relocation, even
> +       if they are not bridges. This is useful if the device is needed (e.g. a
> +       UART). The format is 0xvvvvdddd where d is the device ID and v is the
> +       vendor ID.

Can we reuse "u-boot,dm-pre-reloc" to do such thing?

The following is an example from arch/x86/dts/crownbay.dts

                                pciuart0: uart at a,1 {
                                        compatible = "pci8086,8811.00",
                                                        "pci8086,8811",
                                                        "pciclass,070002",
                                                        "pciclass,0700",
                                                        "ns16550";
                                        u-boot,dm-pre-reloc;
                                        reg = <0x00025100 0x0 0x0 0x0 0x0
                                               0x01025110 0x0 0x0 0x0 0x0>;
                                        reg-shift = <0>;
                                        clock-frequency = <1843200>;
                                        current-speed = <115200>;
                                };

Regards,
Bin

[PATCH 02/17] x86: pci: Allow binding of some devices before relocation

[Simon Glass]

Hi Bin,

On Thu, 8 Apr 2021 at 14:17, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > At present only bridge devices are bound before relocation, to save space
> > in pre-relocation memory. In some cases we do actually want to bind a
> > device, e.g. because it provides the console UART. Add a devicetree
> > binding to support this.
> >
> > Use the PCI_VENDEV() macro to encode the cell value. This is present in
> > U-Boot but not used, so move it to the binding header-file.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  doc/device-tree-bindings/pci/x86-pci.txt |  7 ++++-
> >  drivers/pci/pci-uclass.c                 | 33 +++++++++++++++++++++++-
> >  include/dt-bindings/pci/pci.h            | 12 +++++++++
> >  include/pci.h                            |  1 -
> >  4 files changed, 50 insertions(+), 3 deletions(-)
> >  create mode 100644 include/dt-bindings/pci/pci.h
> >
> > diff --git a/doc/device-tree-bindings/pci/x86-pci.txt b/doc/device-tree-bindings/pci/x86-pci.txt
> > index 95e370b3e72..cf4e5ed595a 100644
> > --- a/doc/device-tree-bindings/pci/x86-pci.txt
> > +++ b/doc/device-tree-bindings/pci/x86-pci.txt
> > @@ -20,6 +20,10 @@ For PCI devices the following optional property is available:
> >         output to be lost. This should not generally be used in production code,
> >         although it is often harmless.
> >
> > +- u-boot,pci-pre-reloc : List of vendor/device IDs to bind before relocation, even
> > +       if they are not bridges. This is useful if the device is needed (e.g. a
> > +       UART). The format is 0xvvvvdddd where d is the device ID and v is the
> > +       vendor ID.
>
> Can we reuse "u-boot,dm-pre-reloc" to do such thing?
>
> The following is an example from arch/x86/dts/crownbay.dts
>
>                                 pciuart0: uart at a,1 {
>                                         compatible = "pci8086,8811.00",
>                                                         "pci8086,8811",
>                                                         "pciclass,070002",
>                                                         "pciclass,0700",
>                                                         "ns16550";
>                                         u-boot,dm-pre-reloc;
>                                         reg = <0x00025100 0x0 0x0 0x0 0x0
>                                                0x01025110 0x0 0x0 0x0 0x0>;
>                                         reg-shift = <0>;
>                                         clock-frequency = <1843200>;
>                                         current-speed = <115200>;
>                                 };

Yes but only if we have the correct PCI BDF for it. But the goal of
the coreboot build is to be able to run on any hardware. So if we
require a devicetree (and coreboot doesn't supply it) then it cannot
work. We would need to have many devicetree files, one for each board.
Perhaps that will happen anyway, but for now I am trying to do things
automatically.

Of course this would be a lot easier if coreboot just used devicetree,
but that doesn't seem to be on the cards.

Regards,
Simon

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Simon Glass]

At present this driver relies on coreboot to provide information about
the console UART. However if coreboot is not compiled with the UART
enabled, the information is left out. This configuration is quite
common, e.g. with shipping x86-based Chrome OS Chromebooks.

Add a way to determine the UART settings in this case, using a
hard-coded list of PCI IDs.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/serial/serial_coreboot.c | 68 ++++++++++++++++++++++++++++----
 include/pci_ids.h                |  1 +
 2 files changed, 61 insertions(+), 8 deletions(-)

diff --git a/drivers/serial/serial_coreboot.c b/drivers/serial/serial_coreboot.c
index de09c8681f5..4b4619432d8 100644
--- a/drivers/serial/serial_coreboot.c
+++ b/drivers/serial/serial_coreboot.c
@@ -11,19 +11,71 @@
 #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) },
+	{},
+};
+
+/*
+ * 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 7ecedc7f04c..d91c1d08f1a 100644
--- a/include/pci_ids.h
+++ b/include/pci_ids.h
@@ -2987,6 +2987,7 @@
 #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_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
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Bin Meng]

Hi Simon,

On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
>
> At present this driver relies on coreboot to provide information about
> the console UART. However if coreboot is not compiled with the UART
> enabled, the information is left out. This configuration is quite
> common, e.g. with shipping x86-based Chrome OS Chromebooks.
>
> Add a way to determine the UART settings in this case, using a
> hard-coded list of PCI IDs.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  drivers/serial/serial_coreboot.c | 68 ++++++++++++++++++++++++++++----
>  include/pci_ids.h                |  1 +
>  2 files changed, 61 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/serial/serial_coreboot.c b/drivers/serial/serial_coreboot.c
> index de09c8681f5..4b4619432d8 100644
> --- a/drivers/serial/serial_coreboot.c
> +++ b/drivers/serial/serial_coreboot.c
> @@ -11,19 +11,71 @@
>  #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) },
> +       {},
> +};
> +
> +/*
> + * 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)

This is really not a guess, but use a pre-configured platform data.
Also this only work for Apollo Lake board, and will break other boards
if they don't have cbinfo available.

Why not just simply put a serial node in the device tree and we are all done?

> +{
> +       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 7ecedc7f04c..d91c1d08f1a 100644
> --- a/include/pci_ids.h
> +++ b/include/pci_ids.h
> @@ -2987,6 +2987,7 @@
>  #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_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
> --

Regards,
Bin

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Simon Glass]

Hi Bin,

On Thu, 8 Apr 2021 at 14:23, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > At present this driver relies on coreboot to provide information about
> > the console UART. However if coreboot is not compiled with the UART
> > enabled, the information is left out. This configuration is quite
> > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> >
> > Add a way to determine the UART settings in this case, using a
> > hard-coded list of PCI IDs.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  drivers/serial/serial_coreboot.c | 68 ++++++++++++++++++++++++++++----
> >  include/pci_ids.h                |  1 +
> >  2 files changed, 61 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/serial/serial_coreboot.c b/drivers/serial/serial_coreboot.c
> > index de09c8681f5..4b4619432d8 100644
> > --- a/drivers/serial/serial_coreboot.c
> > +++ b/drivers/serial/serial_coreboot.c
> > @@ -11,19 +11,71 @@
> >  #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) },
> > +       {},
> > +};
> > +
> > +/*
> > + * 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)
>
> This is really not a guess, but use a pre-configured platform data.
> Also this only work for Apollo Lake board, and will break other boards
> if they don't have cbinfo available.

Which bit of it breaks other boards?

>
> Why not just simply put a serial node in the device tree and we are all done?

See my other email...I am trying to make this boot on any board that
coreboot supports.

[..]

Regards,
Simon

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Bin Meng]

Hi Simon,

On Sat, Apr 24, 2021 at 12:56 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Thu, 8 Apr 2021 at 14:23, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > At present this driver relies on coreboot to provide information about
> > > the console UART. However if coreboot is not compiled with the UART
> > > enabled, the information is left out. This configuration is quite
> > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > >
> > > Add a way to determine the UART settings in this case, using a
> > > hard-coded list of PCI IDs.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > >  drivers/serial/serial_coreboot.c | 68 ++++++++++++++++++++++++++++----
> > >  include/pci_ids.h                |  1 +
> > >  2 files changed, 61 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/serial/serial_coreboot.c b/drivers/serial/serial_coreboot.c
> > > index de09c8681f5..4b4619432d8 100644
> > > --- a/drivers/serial/serial_coreboot.c
> > > +++ b/drivers/serial/serial_coreboot.c
> > > @@ -11,19 +11,71 @@
> > >  #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) },
> > > +       {},
> > > +};
> > > +
> > > +/*
> > > + * 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)
> >
> > This is really not a guess, but use a pre-configured platform data.
> > Also this only work for Apollo Lake board, and will break other boards
> > if they don't have cbinfo available.
>
> Which bit of it breaks other boards?

I see it does not return the error code to the caller, that's okay ...

>
> >
> > Why not just simply put a serial node in the device tree and we are all done?
>
> See my other email...I am trying to make this boot on any board that
> coreboot supports.

But this solution does not scale. One has to put all known UARTs into
serial_coreboot.c.

Why not patch coreboot instead? Why coreboot does not provide a cbinfo
with serial?

Regards,
Bin

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Simon Glass]

Hi Bin,

On Sun, 25 Apr 2021 at 13:49, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Sat, Apr 24, 2021 at 12:56 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Thu, 8 Apr 2021 at 14:23, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > At present this driver relies on coreboot to provide information about
> > > > the console UART. However if coreboot is not compiled with the UART
> > > > enabled, the information is left out. This configuration is quite
> > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > >
> > > > Add a way to determine the UART settings in this case, using a
> > > > hard-coded list of PCI IDs.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >
> > > >  drivers/serial/serial_coreboot.c | 68 ++++++++++++++++++++++++++++----
> > > >  include/pci_ids.h                |  1 +
> > > >  2 files changed, 61 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/serial/serial_coreboot.c b/drivers/serial/serial_coreboot.c
> > > > index de09c8681f5..4b4619432d8 100644
> > > > --- a/drivers/serial/serial_coreboot.c
> > > > +++ b/drivers/serial/serial_coreboot.c
> > > > @@ -11,19 +11,71 @@
> > > >  #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) },
> > > > +       {},
> > > > +};
> > > > +
> > > > +/*
> > > > + * 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)
> > >
> > > This is really not a guess, but use a pre-configured platform data.
> > > Also this only work for Apollo Lake board, and will break other boards
> > > if they don't have cbinfo available.
> >
> > Which bit of it breaks other boards?
>
> I see it does not return the error code to the caller, that's okay ...
>
> >
> > >
> > > Why not just simply put a serial node in the device tree and we are all done?
> >
> > See my other email...I am trying to make this boot on any board that
> > coreboot supports.
>
> But this solution does not scale. One has to put all known UARTs into
> serial_coreboot.c.

Yes that's right...but this is only for when coreboot does not enable
serial. Also the number of new platforms is not that great.

>
> Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> with serial?

Because it does not even set up the serial device in that case, so
doesn't know the details. The driver is completely missing.

Regards,
Simon

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Bin Meng]

Hi Simon,

On Sun, Apr 25, 2021 at 10:10 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Sun, 25 Apr 2021 at 13:49, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Thu, 8 Apr 2021 at 14:23, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > At present this driver relies on coreboot to provide information about
> > > > > the console UART. However if coreboot is not compiled with the UART
> > > > > enabled, the information is left out. This configuration is quite
> > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > >
> > > > > Add a way to determine the UART settings in this case, using a
> > > > > hard-coded list of PCI IDs.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > ---
> > > > >
> > > > >  drivers/serial/serial_coreboot.c | 68 ++++++++++++++++++++++++++++----
> > > > >  include/pci_ids.h                |  1 +
> > > > >  2 files changed, 61 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/serial/serial_coreboot.c b/drivers/serial/serial_coreboot.c
> > > > > index de09c8681f5..4b4619432d8 100644
> > > > > --- a/drivers/serial/serial_coreboot.c
> > > > > +++ b/drivers/serial/serial_coreboot.c
> > > > > @@ -11,19 +11,71 @@
> > > > >  #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) },
> > > > > +       {},
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * 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)
> > > >
> > > > This is really not a guess, but use a pre-configured platform data.
> > > > Also this only work for Apollo Lake board, and will break other boards
> > > > if they don't have cbinfo available.
> > >
> > > Which bit of it breaks other boards?
> >
> > I see it does not return the error code to the caller, that's okay ...
> >
> > >
> > > >
> > > > Why not just simply put a serial node in the device tree and we are all done?
> > >
> > > See my other email...I am trying to make this boot on any board that
> > > coreboot supports.
> >
> > But this solution does not scale. One has to put all known UARTs into
> > serial_coreboot.c.
>
> Yes that's right...but this is only for when coreboot does not enable
> serial. Also the number of new platforms is not that great.
>
> >
> > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > with serial?
>
> Because it does not even set up the serial device in that case, so
> doesn't know the details. The driver is completely missing.

Sigh. Is it possible to upstream a patch to coreboot to enable that?

I don't like the current approach because it ends up duplicating all
UART IDs/info in C.

Regards,
Bin

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Simon Glass]

Hi Bin,

On Sun, 25 Apr 2021 at 18:21, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Sun, Apr 25, 2021 at 10:10 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Sun, 25 Apr 2021 at 13:49, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > At present this driver relies on coreboot to provide information about
> > > > > > the console UART. However if coreboot is not compiled with the UART
> > > > > > enabled, the information is left out. This configuration is quite
> > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > > >
> > > > > > Add a way to determine the UART settings in this case, using a
> > > > > > hard-coded list of PCI IDs.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > ---
> > > > > >
> > > > > >  drivers/serial/serial_coreboot.c | 68 ++++++++++++++++++++++++++++----
> > > > > >  include/pci_ids.h                |  1 +
> > > > > >  2 files changed, 61 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/serial/serial_coreboot.c b/drivers/serial/serial_coreboot.c
> > > > > > index de09c8681f5..4b4619432d8 100644
> > > > > > --- a/drivers/serial/serial_coreboot.c
> > > > > > +++ b/drivers/serial/serial_coreboot.c
> > > > > > @@ -11,19 +11,71 @@
> > > > > >  #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) },
> > > > > > +       {},
> > > > > > +};
> > > > > > +
> > > > > > +/*
> > > > > > + * 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)
> > > > >
> > > > > This is really not a guess, but use a pre-configured platform data.
> > > > > Also this only work for Apollo Lake board, and will break other boards
> > > > > if they don't have cbinfo available.
> > > >
> > > > Which bit of it breaks other boards?
> > >
> > > I see it does not return the error code to the caller, that's okay ...
> > >
> > > >
> > > > >
> > > > > Why not just simply put a serial node in the device tree and we are all done?
> > > >
> > > > See my other email...I am trying to make this boot on any board that
> > > > coreboot supports.
> > >
> > > But this solution does not scale. One has to put all known UARTs into
> > > serial_coreboot.c.
> >
> > Yes that's right...but this is only for when coreboot does not enable
> > serial. Also the number of new platforms is not that great.
> >
> > >
> > > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > > with serial?
> >
> > Because it does not even set up the serial device in that case, so
> > doesn't know the details. The driver is completely missing.
>
> Sigh. Is it possible to upstream a patch to coreboot to enable that?

Well I'm not even sure upstream coreboot boots on the various
Chromebooks I am targetting. If it does, then serial is probablt
enabled. But certainly for chromebooks, it is not. My goal is to have
U-Boot boot on a chromebook in altfw mode with serial enabled.

>
> I don't like the current approach because it ends up duplicating all
> UART IDs/info in C.

Yes. Do you think it would be better to put it in the devicetree? I
suppose we could add some more stuff to the compatible string,
although U-Boot does not support the PCI compatible strings at
present.

What do you suggest?

Regards,
Simon

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Bin Meng]

Hi Simon,

On Fri, Apr 30, 2021 at 12:10 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Sun, 25 Apr 2021 at 18:21, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Sun, 25 Apr 2021 at 13:49, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Bin,
> > > > >
> > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > At present this driver relies on coreboot to provide information about
> > > > > > > the console UART. However if coreboot is not compiled with the UART
> > > > > > > enabled, the information is left out. This configuration is quite
> > > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > > > >
> > > > > > > Add a way to determine the UART settings in this case, using a
> > > > > > > hard-coded list of PCI IDs.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > ---
> > > > > > >
> > > > > > >  drivers/serial/serial_coreboot.c | 68 ++++++++++++++++++++++++++++----
> > > > > > >  include/pci_ids.h                |  1 +
> > > > > > >  2 files changed, 61 insertions(+), 8 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/serial/serial_coreboot.c b/drivers/serial/serial_coreboot.c
> > > > > > > index de09c8681f5..4b4619432d8 100644
> > > > > > > --- a/drivers/serial/serial_coreboot.c
> > > > > > > +++ b/drivers/serial/serial_coreboot.c
> > > > > > > @@ -11,19 +11,71 @@
> > > > > > >  #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) },
> > > > > > > +       {},
> > > > > > > +};
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * 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)
> > > > > >
> > > > > > This is really not a guess, but use a pre-configured platform data.
> > > > > > Also this only work for Apollo Lake board, and will break other boards
> > > > > > if they don't have cbinfo available.
> > > > >
> > > > > Which bit of it breaks other boards?
> > > >
> > > > I see it does not return the error code to the caller, that's okay ...
> > > >
> > > > >
> > > > > >
> > > > > > Why not just simply put a serial node in the device tree and we are all done?
> > > > >
> > > > > See my other email...I am trying to make this boot on any board that
> > > > > coreboot supports.
> > > >
> > > > But this solution does not scale. One has to put all known UARTs into
> > > > serial_coreboot.c.
> > >
> > > Yes that's right...but this is only for when coreboot does not enable
> > > serial. Also the number of new platforms is not that great.
> > >
> > > >
> > > > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > > > with serial?
> > >
> > > Because it does not even set up the serial device in that case, so
> > > doesn't know the details. The driver is completely missing.
> >
> > Sigh. Is it possible to upstream a patch to coreboot to enable that?
>
> Well I'm not even sure upstream coreboot boots on the various
> Chromebooks I am targetting. If it does, then serial is probablt
> enabled. But certainly for chromebooks, it is not. My goal is to have
> U-Boot boot on a chromebook in altfw mode with serial enabled.
>
> >
> > I don't like the current approach because it ends up duplicating all
> > UART IDs/info in C.
>
> Yes. Do you think it would be better to put it in the devicetree? I
> suppose we could add some more stuff to the compatible string,
> although U-Boot does not support the PCI compatible strings at
> present.

Putting it in the device tree also looks odd, because it only matters
on a dedicated board.

> What do you suggest?

Or parse the ACPI table coreboot has set up? But that might be another
huge monster :(

Regards,
Bin

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Simon Glass]

Hi Bin,

On Thu, 29 Apr 2021 at 17:01, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Fri, Apr 30, 2021 at 12:10 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Sun, 25 Apr 2021 at 18:21, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Bin,
> > > > > >
> > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > At present this driver relies on coreboot to provide information about
> > > > > > > > the console UART. However if coreboot is not compiled with the UART
> > > > > > > > enabled, the information is left out. This configuration is quite
> > > > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > > > > >
> > > > > > > > Add a way to determine the UART settings in this case, using a
> > > > > > > > hard-coded list of PCI IDs.
> > > > > > > >
> > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > ---
> > > > > > > >
> > > > > > > >  drivers/serial/serial_coreboot.c | 68 ++++++++++++++++++++++++++++----
> > > > > > > >  include/pci_ids.h                |  1 +
> > > > > > > >  2 files changed, 61 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/serial/serial_coreboot.c b/drivers/serial/serial_coreboot.c
> > > > > > > > index de09c8681f5..4b4619432d8 100644
> > > > > > > > --- a/drivers/serial/serial_coreboot.c
> > > > > > > > +++ b/drivers/serial/serial_coreboot.c
> > > > > > > > @@ -11,19 +11,71 @@
> > > > > > > >  #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) },
> > > > > > > > +       {},
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * 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)
> > > > > > >
> > > > > > > This is really not a guess, but use a pre-configured platform data.
> > > > > > > Also this only work for Apollo Lake board, and will break other boards
> > > > > > > if they don't have cbinfo available.
> > > > > >
> > > > > > Which bit of it breaks other boards?
> > > > >
> > > > > I see it does not return the error code to the caller, that's okay ...
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Why not just simply put a serial node in the device tree and we are all done?
> > > > > >
> > > > > > See my other email...I am trying to make this boot on any board that
> > > > > > coreboot supports.
> > > > >
> > > > > But this solution does not scale. One has to put all known UARTs into
> > > > > serial_coreboot.c.
> > > >
> > > > Yes that's right...but this is only for when coreboot does not enable
> > > > serial. Also the number of new platforms is not that great.
> > > >
> > > > >
> > > > > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > > > > with serial?
> > > >
> > > > Because it does not even set up the serial device in that case, so
> > > > doesn't know the details. The driver is completely missing.
> > >
> > > Sigh. Is it possible to upstream a patch to coreboot to enable that?
> >
> > Well I'm not even sure upstream coreboot boots on the various
> > Chromebooks I am targetting. If it does, then serial is probablt
> > enabled. But certainly for chromebooks, it is not. My goal is to have
> > U-Boot boot on a chromebook in altfw mode with serial enabled.
> >
> > >
> > > I don't like the current approach because it ends up duplicating all
> > > UART IDs/info in C.
> >
> > Yes. Do you think it would be better to put it in the devicetree? I
> > suppose we could add some more stuff to the compatible string,
> > although U-Boot does not support the PCI compatible strings at
> > present.
>
> Putting it in the device tree also looks odd, because it only matters
> on a dedicated board.

Right, but that is the nature of trying to run the same image on
different hardware.

>
> > What do you suggest?
>
> Or parse the ACPI table coreboot has set up? But that might be another
> huge monster :(

Yes, even worse...

Regards,
Simon

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Andy Shevchenko]

On Fri, Apr 30, 2021 at 9:14 PM Simon Glass <sjg@chromium.org> wrote:
> On Thu, 29 Apr 2021 at 17:01, Bin Meng <bmeng.cn@gmail.com> wrote:
> > On Fri, Apr 30, 2021 at 12:10 AM Simon Glass <sjg@chromium.org> wrote:
> > > On Sun, 25 Apr 2021 at 18:21, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > At present this driver relies on coreboot to provide information about
> > > > > > > > > the console UART. However if coreboot is not compiled with the UART
> > > > > > > > > enabled, the information is left out. This configuration is quite
> > > > > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > > > > > >
> > > > > > > > > Add a way to determine the UART settings in this case, using a
> > > > > > > > > hard-coded list of PCI IDs.

I don't like this either, so -1 from me.

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.

The patch is ugly hack in my opinion. Sorry.

You may make it less ugly by checking PCI class rather than IDs.

> > > > > > > > Why not just simply put a serial node in the device tree and we are all done?
> > > > > > >
> > > > > > > See my other email...I am trying to make this boot on any board that
> > > > > > > coreboot supports.
> > > > > >
> > > > > > But this solution does not scale. One has to put all known UARTs into
> > > > > > serial_coreboot.c.
> > > > >
> > > > > Yes that's right...but this is only for when coreboot does not enable
> > > > > serial. Also the number of new platforms is not that great.
> > > > >
> > > > > >
> > > > > > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > > > > > with serial?
> > > > >
> > > > > Because it does not even set up the serial device in that case, so
> > > > > doesn't know the details. The driver is completely missing.
> > > >
> > > > Sigh. Is it possible to upstream a patch to coreboot to enable that?
> > >
> > > Well I'm not even sure upstream coreboot boots on the various
> > > Chromebooks I am targetting. If it does, then serial is probablt
> > > enabled. But certainly for chromebooks, it is not. My goal is to have
> > > U-Boot boot on a chromebook in altfw mode with serial enabled.
> > >
> > > >
> > > > I don't like the current approach because it ends up duplicating all
> > > > UART IDs/info in C.
> > >
> > > Yes. Do you think it would be better to put it in the devicetree? I
> > > suppose we could add some more stuff to the compatible string,
> > > although U-Boot does not support the PCI compatible strings at
> > > present.
> >
> > Putting it in the device tree also looks odd, because it only matters
> > on a dedicated board.
>
> Right, but that is the nature of trying to run the same image on
> different hardware.
>
> >
> > > What do you suggest?
> >
> > Or parse the ACPI table coreboot has set up? But that might be another
> > huge monster :(
>
> Yes, even worse...

-- 
With Best Regards,
Andy Shevchenko

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Simon Glass]

Hi Bin and Andy,

On Fri, 30 Apr 2021 at 12:41, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Fri, Apr 30, 2021 at 9:14 PM Simon Glass <sjg@chromium.org> wrote:
> > On Thu, 29 Apr 2021 at 17:01, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > On Fri, Apr 30, 2021 at 12:10 AM Simon Glass <sjg@chromium.org> wrote:
> > > > On Sun, 25 Apr 2021 at 18:21, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > At present this driver relies on coreboot to provide information about
> > > > > > > > > > the console UART. However if coreboot is not compiled with the UART
> > > > > > > > > > enabled, the information is left out. This configuration is quite
> > > > > > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > > > > > > >
> > > > > > > > > > Add a way to determine the UART settings in this case, using a
> > > > > > > > > > hard-coded list of PCI IDs.
>
> I don't like this either, so -1 from me.
>
> 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.
>
> The patch is ugly hack in my opinion. Sorry.
>
> You may make it less ugly by checking PCI class rather than IDs.

I have not been able to distinguish a pattern on Intel SoCs yet.
Perhaps you can help with that as there must be a way...


>
> > > > > > > > > Why not just simply put a serial node in the device tree and we are all done?
> > > > > > > >
> > > > > > > > See my other email...I am trying to make this boot on any board that
> > > > > > > > coreboot supports.
> > > > > > >
> > > > > > > But this solution does not scale. One has to put all known UARTs into
> > > > > > > serial_coreboot.c.
> > > > > >
> > > > > > Yes that's right...but this is only for when coreboot does not enable
> > > > > > serial. Also the number of new platforms is not that great.
> > > > > >
> > > > > > >
> > > > > > > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > > > > > > with serial?
> > > > > >
> > > > > > Because it does not even set up the serial device in that case, so
> > > > > > doesn't know the details. The driver is completely missing.
> > > > >
> > > > > Sigh. Is it possible to upstream a patch to coreboot to enable that?
> > > >
> > > > Well I'm not even sure upstream coreboot boots on the various
> > > > Chromebooks I am targetting. If it does, then serial is probablt
> > > > enabled. But certainly for chromebooks, it is not. My goal is to have
> > > > U-Boot boot on a chromebook in altfw mode with serial enabled.
> > > >
> > > > >
> > > > > I don't like the current approach because it ends up duplicating all
> > > > > UART IDs/info in C.
> > > >
> > > > Yes. Do you think it would be better to put it in the devicetree? I
> > > > suppose we could add some more stuff to the compatible string,
> > > > although U-Boot does not support the PCI compatible strings at
> > > > present.
> > >
> > > Putting it in the device tree also looks odd, because it only matters
> > > on a dedicated board.
> >
> > Right, but that is the nature of trying to run the same image on
> > different hardware.
> >
> > >
> > > > What do you suggest?
> > >
> > > Or parse the ACPI table coreboot has set up? But that might be another
> > > huge monster :(
> >
> > Yes, even worse...

All of the suggestions so far do not solve the problem, so we are left
without serial output, or something else...?

Regards,
Simon

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Andy Shevchenko]

On Tue, May 04, 2021 at 09:26:19AM -0600, Simon Glass wrote:
> On Fri, 30 Apr 2021 at 12:41, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Apr 30, 2021 at 9:14 PM Simon Glass <sjg@chromium.org> wrote:
> > > On Thu, 29 Apr 2021 at 17:01, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > On Fri, Apr 30, 2021 at 12:10 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > On Sun, 25 Apr 2021 at 18:21, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > At present this driver relies on coreboot to provide information about
> > > > > > > > > > > the console UART. However if coreboot is not compiled with the UART
> > > > > > > > > > > enabled, the information is left out. This configuration is quite
> > > > > > > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > > > > > > > >
> > > > > > > > > > > Add a way to determine the UART settings in this case, using a
> > > > > > > > > > > hard-coded list of PCI IDs.
> >
> > I don't like this either, so -1 from me.
> >
> > 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.
> >
> > The patch is ugly hack in my opinion. Sorry.
> >
> > You may make it less ugly by checking PCI class rather than IDs.
> 
> I have not been able to distinguish a pattern on Intel SoCs yet.
> Perhaps you can help with that as there must be a way...
> 
> 
> >
> > > > > > > > > > Why not just simply put a serial node in the device tree and we are all done?
> > > > > > > > >
> > > > > > > > > See my other email...I am trying to make this boot on any board that
> > > > > > > > > coreboot supports.
> > > > > > > >
> > > > > > > > But this solution does not scale. One has to put all known UARTs into
> > > > > > > > serial_coreboot.c.
> > > > > > >
> > > > > > > Yes that's right...but this is only for when coreboot does not enable
> > > > > > > serial. Also the number of new platforms is not that great.
> > > > > > >
> > > > > > > >
> > > > > > > > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > > > > > > > with serial?
> > > > > > >
> > > > > > > Because it does not even set up the serial device in that case, so
> > > > > > > doesn't know the details. The driver is completely missing.
> > > > > >
> > > > > > Sigh. Is it possible to upstream a patch to coreboot to enable that?
> > > > >
> > > > > Well I'm not even sure upstream coreboot boots on the various
> > > > > Chromebooks I am targetting. If it does, then serial is probablt
> > > > > enabled. But certainly for chromebooks, it is not. My goal is to have
> > > > > U-Boot boot on a chromebook in altfw mode with serial enabled.
> > > > >
> > > > > >
> > > > > > I don't like the current approach because it ends up duplicating all
> > > > > > UART IDs/info in C.
> > > > >
> > > > > Yes. Do you think it would be better to put it in the devicetree? I
> > > > > suppose we could add some more stuff to the compatible string,
> > > > > although U-Boot does not support the PCI compatible strings at
> > > > > present.
> > > >
> > > > Putting it in the device tree also looks odd, because it only matters
> > > > on a dedicated board.
> > >
> > > Right, but that is the nature of trying to run the same image on
> > > different hardware.
> > >
> > > >
> > > > > What do you suggest?
> > > >
> > > > Or parse the ACPI table coreboot has set up? But that might be another
> > > > huge monster :(
> > >
> > > Yes, even worse...
> 
> All of the suggestions so far do not solve the problem, so we are left
> without serial output, or something else...?

You are truing to solve Coreboot issue in U-Boot, doesn't seem to be quite
right.

Yes, I prefer no serial than some "smart" hack. It will encourage people who
want to have serial out of Coreboot to whine to coreboot project and see what
they will do about it.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Simon Glass]

Hi Bin,

On Tue, 4 May 2021 at 08:26, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin and Andy,
>
> On Fri, 30 Apr 2021 at 12:41, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >
> > On Fri, Apr 30, 2021 at 9:14 PM Simon Glass <sjg@chromium.org> wrote:
> > > On Thu, 29 Apr 2021 at 17:01, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > On Fri, Apr 30, 2021 at 12:10 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > On Sun, 25 Apr 2021 at 18:21, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > At present this driver relies on coreboot to provide information about
> > > > > > > > > > > the console UART. However if coreboot is not compiled with the UART
> > > > > > > > > > > enabled, the information is left out. This configuration is quite
> > > > > > > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > > > > > > > >
> > > > > > > > > > > Add a way to determine the UART settings in this case, using a
> > > > > > > > > > > hard-coded list of PCI IDs.
> >
> > I don't like this either, so -1 from me.
> >
> > 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.
> >
> > The patch is ugly hack in my opinion. Sorry.
> >
> > You may make it less ugly by checking PCI class rather than IDs.
>
> I have not been able to distinguish a pattern on Intel SoCs yet.
> Perhaps you can help with that as there must be a way...
>
>
> >
> > > > > > > > > > Why not just simply put a serial node in the device tree and we are all done?
> > > > > > > > >
> > > > > > > > > See my other email...I am trying to make this boot on any board that
> > > > > > > > > coreboot supports.
> > > > > > > >
> > > > > > > > But this solution does not scale. One has to put all known UARTs into
> > > > > > > > serial_coreboot.c.
> > > > > > >
> > > > > > > Yes that's right...but this is only for when coreboot does not enable
> > > > > > > serial. Also the number of new platforms is not that great.
> > > > > > >
> > > > > > > >
> > > > > > > > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > > > > > > > with serial?
> > > > > > >
> > > > > > > Because it does not even set up the serial device in that case, so
> > > > > > > doesn't know the details. The driver is completely missing.
> > > > > >
> > > > > > Sigh. Is it possible to upstream a patch to coreboot to enable that?
> > > > >
> > > > > Well I'm not even sure upstream coreboot boots on the various
> > > > > Chromebooks I am targetting. If it does, then serial is probablt
> > > > > enabled. But certainly for chromebooks, it is not. My goal is to have
> > > > > U-Boot boot on a chromebook in altfw mode with serial enabled.
> > > > >
> > > > > >
> > > > > > I don't like the current approach because it ends up duplicating all
> > > > > > UART IDs/info in C.
> > > > >
> > > > > Yes. Do you think it would be better to put it in the devicetree? I
> > > > > suppose we could add some more stuff to the compatible string,
> > > > > although U-Boot does not support the PCI compatible strings at
> > > > > present.
> > > >
> > > > Putting it in the device tree also looks odd, because it only matters
> > > > on a dedicated board.
> > >
> > > Right, but that is the nature of trying to run the same image on
> > > different hardware.
> > >
> > > >
> > > > > What do you suggest?
> > > >
> > > > Or parse the ACPI table coreboot has set up? But that might be another
> > > > huge monster :(
> > >
> > > Yes, even worse...
>
> All of the suggestions so far do not solve the problem, so we are left
> without serial output, or something else...?

In the interested of getting the patches applied, shall we just drop
this one? I will revisit it when I try the next platform and see if I
can find a different pattern.

Regards,
Simon

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Bin Meng]

Hi Simon,

On Sat, May 8, 2021 at 9:42 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Tue, 4 May 2021 at 08:26, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin and Andy,
> >
> > On Fri, 30 Apr 2021 at 12:41, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Fri, Apr 30, 2021 at 9:14 PM Simon Glass <sjg@chromium.org> wrote:
> > > > On Thu, 29 Apr 2021 at 17:01, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > On Fri, Apr 30, 2021 at 12:10 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > On Sun, 25 Apr 2021 at 18:21, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > At present this driver relies on coreboot to provide information about
> > > > > > > > > > > > the console UART. However if coreboot is not compiled with the UART
> > > > > > > > > > > > enabled, the information is left out. This configuration is quite
> > > > > > > > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > > > > > > > > >
> > > > > > > > > > > > Add a way to determine the UART settings in this case, using a
> > > > > > > > > > > > hard-coded list of PCI IDs.
> > >
> > > I don't like this either, so -1 from me.
> > >
> > > 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.
> > >
> > > The patch is ugly hack in my opinion. Sorry.
> > >
> > > You may make it less ugly by checking PCI class rather than IDs.
> >
> > I have not been able to distinguish a pattern on Intel SoCs yet.
> > Perhaps you can help with that as there must be a way...
> >
> >
> > >
> > > > > > > > > > > Why not just simply put a serial node in the device tree and we are all done?
> > > > > > > > > >
> > > > > > > > > > See my other email...I am trying to make this boot on any board that
> > > > > > > > > > coreboot supports.
> > > > > > > > >
> > > > > > > > > But this solution does not scale. One has to put all known UARTs into
> > > > > > > > > serial_coreboot.c.
> > > > > > > >
> > > > > > > > Yes that's right...but this is only for when coreboot does not enable
> > > > > > > > serial. Also the number of new platforms is not that great.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > > > > > > > > with serial?
> > > > > > > >
> > > > > > > > Because it does not even set up the serial device in that case, so
> > > > > > > > doesn't know the details. The driver is completely missing.
> > > > > > >
> > > > > > > Sigh. Is it possible to upstream a patch to coreboot to enable that?
> > > > > >
> > > > > > Well I'm not even sure upstream coreboot boots on the various
> > > > > > Chromebooks I am targetting. If it does, then serial is probablt
> > > > > > enabled. But certainly for chromebooks, it is not. My goal is to have
> > > > > > U-Boot boot on a chromebook in altfw mode with serial enabled.
> > > > > >
> > > > > > >
> > > > > > > I don't like the current approach because it ends up duplicating all
> > > > > > > UART IDs/info in C.
> > > > > >
> > > > > > Yes. Do you think it would be better to put it in the devicetree? I
> > > > > > suppose we could add some more stuff to the compatible string,
> > > > > > although U-Boot does not support the PCI compatible strings at
> > > > > > present.
> > > > >
> > > > > Putting it in the device tree also looks odd, because it only matters
> > > > > on a dedicated board.
> > > >
> > > > Right, but that is the nature of trying to run the same image on
> > > > different hardware.
> > > >
> > > > >
> > > > > > What do you suggest?
> > > > >
> > > > > Or parse the ACPI table coreboot has set up? But that might be another
> > > > > huge monster :(
> > > >
> > > > Yes, even worse...
> >
> > All of the suggestions so far do not solve the problem, so we are left
> > without serial output, or something else...?
>
> In the interested of getting the patches applied, shall we just drop
> this one? I will revisit it when I try the next platform and see if I
> can find a different pattern.

I think we should drop this patch for now, and apply other patches in
this series. But I suspect the rest patches cannot be applied cleanly
if dropping this patch.

Regards,
Bin

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Simon Glass]

Hi Bin,

On Fri, 7 May 2021 at 18:48, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Sat, May 8, 2021 at 9:42 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Tue, 4 May 2021 at 08:26, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Bin and Andy,
> > >
> > > On Fri, 30 Apr 2021 at 12:41, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > >
> > > > On Fri, Apr 30, 2021 at 9:14 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > On Thu, 29 Apr 2021 at 17:01, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > On Fri, Apr 30, 2021 at 12:10 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > On Sun, 25 Apr 2021 at 18:21, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > At present this driver relies on coreboot to provide information about
> > > > > > > > > > > > > the console UART. However if coreboot is not compiled with the UART
> > > > > > > > > > > > > enabled, the information is left out. This configuration is quite
> > > > > > > > > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Add a way to determine the UART settings in this case, using a
> > > > > > > > > > > > > hard-coded list of PCI IDs.
> > > >
> > > > I don't like this either, so -1 from me.
> > > >
> > > > 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.
> > > >
> > > > The patch is ugly hack in my opinion. Sorry.
> > > >
> > > > You may make it less ugly by checking PCI class rather than IDs.
> > >
> > > I have not been able to distinguish a pattern on Intel SoCs yet.
> > > Perhaps you can help with that as there must be a way...
> > >
> > >
> > > >
> > > > > > > > > > > > Why not just simply put a serial node in the device tree and we are all done?
> > > > > > > > > > >
> > > > > > > > > > > See my other email...I am trying to make this boot on any board that
> > > > > > > > > > > coreboot supports.
> > > > > > > > > >
> > > > > > > > > > But this solution does not scale. One has to put all known UARTs into
> > > > > > > > > > serial_coreboot.c.
> > > > > > > > >
> > > > > > > > > Yes that's right...but this is only for when coreboot does not enable
> > > > > > > > > serial. Also the number of new platforms is not that great.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > > > > > > > > > with serial?
> > > > > > > > >
> > > > > > > > > Because it does not even set up the serial device in that case, so
> > > > > > > > > doesn't know the details. The driver is completely missing.
> > > > > > > >
> > > > > > > > Sigh. Is it possible to upstream a patch to coreboot to enable that?
> > > > > > >
> > > > > > > Well I'm not even sure upstream coreboot boots on the various
> > > > > > > Chromebooks I am targetting. If it does, then serial is probablt
> > > > > > > enabled. But certainly for chromebooks, it is not. My goal is to have
> > > > > > > U-Boot boot on a chromebook in altfw mode with serial enabled.
> > > > > > >
> > > > > > > >
> > > > > > > > I don't like the current approach because it ends up duplicating all
> > > > > > > > UART IDs/info in C.
> > > > > > >
> > > > > > > Yes. Do you think it would be better to put it in the devicetree? I
> > > > > > > suppose we could add some more stuff to the compatible string,
> > > > > > > although U-Boot does not support the PCI compatible strings at
> > > > > > > present.
> > > > > >
> > > > > > Putting it in the device tree also looks odd, because it only matters
> > > > > > on a dedicated board.
> > > > >
> > > > > Right, but that is the nature of trying to run the same image on
> > > > > different hardware.
> > > > >
> > > > > >
> > > > > > > What do you suggest?
> > > > > >
> > > > > > Or parse the ACPI table coreboot has set up? But that might be another
> > > > > > huge monster :(
> > > > >
> > > > > Yes, even worse...
> > >
> > > All of the suggestions so far do not solve the problem, so we are left
> > > without serial output, or something else...?
> >
> > In the interested of getting the patches applied, shall we just drop
> > this one? I will revisit it when I try the next platform and see if I
> > can find a different pattern.
>
> I think we should drop this patch for now, and apply other patches in
> this series. But I suspect the rest patches cannot be applied cleanly
> if dropping this patch.

That's OK with me. I did a little test and it applies cleanly without
this patch. If you have trouble I can resend it.

Regards,
Simon

[PATCH 03/17] x86: Allow coreboot serial driver to guess the UART

[Bin Meng]

Hi Simon,

On Sat, May 8, 2021 at 10:12 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Fri, 7 May 2021 at 18:48, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Sat, May 8, 2021 at 9:42 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Tue, 4 May 2021 at 08:26, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Bin and Andy,
> > > >
> > > > On Fri, 30 Apr 2021 at 12:41, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > >
> > > > > On Fri, Apr 30, 2021 at 9:14 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > On Thu, 29 Apr 2021 at 17:01, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > On Fri, Apr 30, 2021 at 12:10 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > On Sun, 25 Apr 2021 at 18:21, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > At present this driver relies on coreboot to provide information about
> > > > > > > > > > > > > > the console UART. However if coreboot is not compiled with the UART
> > > > > > > > > > > > > > enabled, the information is left out. This configuration is quite
> > > > > > > > > > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Add a way to determine the UART settings in this case, using a
> > > > > > > > > > > > > > hard-coded list of PCI IDs.
> > > > >
> > > > > I don't like this either, so -1 from me.
> > > > >
> > > > > 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.
> > > > >
> > > > > The patch is ugly hack in my opinion. Sorry.
> > > > >
> > > > > You may make it less ugly by checking PCI class rather than IDs.
> > > >
> > > > I have not been able to distinguish a pattern on Intel SoCs yet.
> > > > Perhaps you can help with that as there must be a way...
> > > >
> > > >
> > > > >
> > > > > > > > > > > > > Why not just simply put a serial node in the device tree and we are all done?
> > > > > > > > > > > >
> > > > > > > > > > > > See my other email...I am trying to make this boot on any board that
> > > > > > > > > > > > coreboot supports.
> > > > > > > > > > >
> > > > > > > > > > > But this solution does not scale. One has to put all known UARTs into
> > > > > > > > > > > serial_coreboot.c.
> > > > > > > > > >
> > > > > > > > > > Yes that's right...but this is only for when coreboot does not enable
> > > > > > > > > > serial. Also the number of new platforms is not that great.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > > > > > > > > > > with serial?
> > > > > > > > > >
> > > > > > > > > > Because it does not even set up the serial device in that case, so
> > > > > > > > > > doesn't know the details. The driver is completely missing.
> > > > > > > > >
> > > > > > > > > Sigh. Is it possible to upstream a patch to coreboot to enable that?
> > > > > > > >
> > > > > > > > Well I'm not even sure upstream coreboot boots on the various
> > > > > > > > Chromebooks I am targetting. If it does, then serial is probablt
> > > > > > > > enabled. But certainly for chromebooks, it is not. My goal is to have
> > > > > > > > U-Boot boot on a chromebook in altfw mode with serial enabled.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > I don't like the current approach because it ends up duplicating all
> > > > > > > > > UART IDs/info in C.
> > > > > > > >
> > > > > > > > Yes. Do you think it would be better to put it in the devicetree? I
> > > > > > > > suppose we could add some more stuff to the compatible string,
> > > > > > > > although U-Boot does not support the PCI compatible strings at
> > > > > > > > present.
> > > > > > >
> > > > > > > Putting it in the device tree also looks odd, because it only matters
> > > > > > > on a dedicated board.
> > > > > >
> > > > > > Right, but that is the nature of trying to run the same image on
> > > > > > different hardware.
> > > > > >
> > > > > > >
> > > > > > > > What do you suggest?
> > > > > > >
> > > > > > > Or parse the ACPI table coreboot has set up? But that might be another
> > > > > > > huge monster :(
> > > > > >
> > > > > > Yes, even worse...
> > > >
> > > > All of the suggestions so far do not solve the problem, so we are left
> > > > without serial output, or something else...?
> > >
> > > In the interested of getting the patches applied, shall we just drop
> > > this one? I will revisit it when I try the next platform and see if I
> > > can find a different pattern.
> >
> > I think we should drop this patch for now, and apply other patches in
> > this series. But I suspect the rest patches cannot be applied cleanly
> > if dropping this patch.
>
> That's OK with me. I did a little test and it applies cleanly without
> this patch. If you have trouble I can resend it.

Sure, will do soon.

Regards,
Bin

[PATCH 04/17] spi: ich: Don't require the PCH

[Simon Glass]

When booting from coreboot we may not have a PCH driver available. The
SPI driver can operate without the PCH but currently complains in this
case. Update it to continue to work normally. The only missing feature
is memory-mapping of SPI-flash contents, which is not essential.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/spi/ich.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
index 1cd410493b0..3d49c22a9da 100644
--- a/drivers/spi/ich.c
+++ b/drivers/spi/ich.c
@@ -114,7 +114,7 @@ static bool ich9_can_do_33mhz(struct udevice *dev)
 	struct ich_spi_priv *priv = dev_get_priv(dev);
 	u32 fdod, speed;
 
-	if (!CONFIG_IS_ENABLED(PCI))
+	if (!CONFIG_IS_ENABLED(PCI) || !priv->pch)
 		return false;
 	/* Observe SPI Descriptor Component Section 0 */
 	dm_pci_write_config32(priv->pch, 0xb0, 0x1000);
@@ -632,7 +632,7 @@ static int ich_spi_get_basics(struct udevice *bus, bool can_probe,
 		if (device_get_uclass_id(pch) != UCLASS_PCH) {
 			uclass_first_device(UCLASS_PCH, &pch);
 			if (!pch)
-				return log_msg_ret("uclass", -EPROTOTYPE);
+				; /* ignore this error since we don't need it */
 		}
 	}
 
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 04/17] spi: ich: Don't require the PCH

[Bin Meng]

On Wed, Apr 7, 2021 at 12:34 PM Simon Glass <sjg@chromium.org> wrote:
>
> When booting from coreboot we may not have a PCH driver available. The
> SPI driver can operate without the PCH but currently complains in this
> case. Update it to continue to work normally. The only missing feature
> is memory-mapping of SPI-flash contents, which is not essential.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  drivers/spi/ich.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

But I feel this driver is slightly becoming unmaintainable as there
are many combinations we need to consider ...

Regards,
Bin

[PATCH 04/17] spi: ich: Don't require the PCH

[Simon Glass]

Hi Bin,

On Thu, 8 Apr 2021 at 14:28, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Wed, Apr 7, 2021 at 12:34 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > When booting from coreboot we may not have a PCH driver available. The
> > SPI driver can operate without the PCH but currently complains in this
> > case. Update it to continue to work normally. The only missing feature
> > is memory-mapping of SPI-flash contents, which is not essential.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  drivers/spi/ich.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>
> But I feel this driver is slightly becoming unmaintainable as there
> are many combinations we need to consider ...

Yes it is a bit of a pain. The main one is that in some cases the SPI
bus appears on PCI and in others it is part of the PCH or LPC. This is
a hardware thing though, so I'm not sure what we can do about it.
Perhaps split up the driver?

Regards,
Simon

[PATCH 05/17] tpm: cr50: Drop unnecessary coral headers

[Simon Glass]

These headers are not actually used. Drop them so that this driver can
be used by other boards, e.g. coreboot.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/tpm/cr50_i2c.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tpm/cr50_i2c.c b/drivers/tpm/cr50_i2c.c
index 76432bdec1f..7a2b5a4faa5 100644
--- a/drivers/tpm/cr50_i2c.c
+++ b/drivers/tpm/cr50_i2c.c
@@ -18,8 +18,6 @@
 #include <acpi/acpi_device.h>
 #include <asm/gpio.h>
 #include <asm/io.h>
-#include <asm/arch/iomap.h>
-#include <asm/arch/pm.h>
 #include <linux/delay.h>
 #include <dm/acpi.h>
 
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 05/17] tpm: cr50: Drop unnecessary coral headers

[Bin Meng]

On Wed, Apr 7, 2021 at 12:33 PM Simon Glass <sjg@chromium.org> wrote:
>
> These headers are not actually used. Drop them so that this driver can
> be used by other boards, e.g. coreboot.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  drivers/tpm/cr50_i2c.c | 2 --
>  1 file changed, 2 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 06/17] x86: Don't set up MTRRs if previously done

[Simon Glass]

When starting U-Boot from a previous-stage bootloader we presumably don't
need to set up the variable MTRRs. In fact this could be harmful if the
existing settings are not what U-Boot uses.

Skip that step in this case.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/x86/cpu/i386/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c
index e59215cc20e..c7f6c5a013e 100644
--- a/arch/x86/cpu/i386/cpu.c
+++ b/arch/x86/cpu/i386/cpu.c
@@ -423,7 +423,7 @@ static void setup_mtrr(void)
 	u64 mtrr_cap;
 
 	/* Configure fixed range MTRRs for some legacy regions */
-	if (!gd->arch.has_mtrr)
+	if (!gd->arch.has_mtrr || !ll_boot_init())
 		return;
 
 	mtrr_cap = native_read_msr(MTRR_CAP_MSR);
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 06/17] x86: Don't set up MTRRs if previously done

[Bin Meng]

On Wed, Apr 7, 2021 at 12:33 PM Simon Glass <sjg@chromium.org> wrote:
>
> When starting U-Boot from a previous-stage bootloader we presumably don't
> need to set up the variable MTRRs. In fact this could be harmful if the
> existing settings are not what U-Boot uses.
>
> Skip that step in this case.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/cpu/i386/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 07/17] x86: Update the MP constants to avoid conflicts

[Simon Glass]

These constants conflict with error codes returned by the MP
implementation when something is wrong. In particular, mp_first_cpu()
returns MP_SELECT_BSP when running without multiprocessing enabled.
Since this is -2, it is interpreted as an error by callers, which
expect a positive CPU number for the first CPU.

Correct this by using a different range for the pre-defined CPU
numbers, above zero and out of the range of possible CPU values. For
now it is safe to assume there are no more than 64K CPUs.

This fixes the 'mtrr' command when CONFIG_SMP is not enabled.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/x86/include/asm/mp.h | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/mp.h b/arch/x86/include/asm/mp.h
index 1a3ae8e3950..e48ba051d92 100644
--- a/arch/x86/include/asm/mp.h
+++ b/arch/x86/include/asm/mp.h
@@ -10,18 +10,22 @@
 
 #include <asm/atomic.h>
 #include <asm/cache.h>
+#include <linux/bitops.h>
 
 struct udevice;
 
 enum {
-	/* Indicates that the function should run on all CPUs */
-	MP_SELECT_ALL	= -1,
+	/*
+	 * Indicates that the function should run on all CPUs. We use a large
+	 * number, above the number of real CPUs we expect to find.
+	 */
+	MP_SELECT_ALL	= BIT(16),
 
 	/* Run on boot CPUs */
-	MP_SELECT_BSP	= -2,
+	MP_SELECT_BSP,
 
 	/* Run on non-boot CPUs */
-	MP_SELECT_APS	= -3,
+	MP_SELECT_APS,
 };
 
 typedef int (*mp_callback_t)(struct udevice *cpu, void *arg);
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 07/17] x86: Update the MP constants to avoid conflicts

[Bin Meng]

On Wed, Apr 7, 2021 at 12:33 PM Simon Glass <sjg@chromium.org> wrote:
>
> These constants conflict with error codes returned by the MP
> implementation when something is wrong. In particular, mp_first_cpu()
> returns MP_SELECT_BSP when running without multiprocessing enabled.
> Since this is -2, it is interpreted as an error by callers, which
> expect a positive CPU number for the first CPU.
>
> Correct this by using a different range for the pre-defined CPU
> numbers, above zero and out of the range of possible CPU values. For
> now it is safe to assume there are no more than 64K CPUs.
>
> This fixes the 'mtrr' command when CONFIG_SMP is not enabled.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/include/asm/mp.h | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 08/17] x86: Do cache set-up by default when booting from coreboot

[Simon Glass]

A recent change to disable cache setup when booting from coreboot
assumed that this has been done by SPL. The result is that for the
coreboot board, the cache is disabled (in start.S) and never
re-enabled.

If the cache was turned off, as it is on boards without SPL, we should
turn it back on. Add this new condition.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/x86/lib/init_helpers.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/lib/init_helpers.c b/arch/x86/lib/init_helpers.c
index 67401b9ba79..0c55544a670 100644
--- a/arch/x86/lib/init_helpers.c
+++ b/arch/x86/lib/init_helpers.c
@@ -18,10 +18,7 @@ int init_cache_f_r(void)
 		 IS_ENABLED(CONFIG_FSP_VERSION2);
 	int ret;
 
-	if (!ll_boot_init())
-		return 0;
-
-	do_mtrr &= !IS_ENABLED(CONFIG_FSP_VERSION1) &&
+	do_mtrr &= !IS_ENABLED(CONFIG_SPL) && !IS_ENABLED(CONFIG_FSP_VERSION1) &&
 		!IS_ENABLED(CONFIG_SYS_SLIMBOOTLOADER);
 
 	if (do_mtrr) {
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 08/17] x86: Do cache set-up by default when booting from coreboot

[Bin Meng]

Hi Simon,

On Wed, Apr 7, 2021 at 12:33 PM Simon Glass <sjg@chromium.org> wrote:
>
> A recent change to disable cache setup when booting from coreboot
> assumed that this has been done by SPL. The result is that for the
> coreboot board, the cache is disabled (in start.S) and never
> re-enabled.
>
> If the cache was turned off, as it is on boards without SPL, we should
> turn it back on. Add this new condition.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/lib/init_helpers.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/arch/x86/lib/init_helpers.c b/arch/x86/lib/init_helpers.c
> index 67401b9ba79..0c55544a670 100644
> --- a/arch/x86/lib/init_helpers.c
> +++ b/arch/x86/lib/init_helpers.c
> @@ -18,10 +18,7 @@ int init_cache_f_r(void)
>                  IS_ENABLED(CONFIG_FSP_VERSION2);
>         int ret;
>
> -       if (!ll_boot_init())
> -               return 0;
> -
> -       do_mtrr &= !IS_ENABLED(CONFIG_FSP_VERSION1) &&

Can we add a comment block here for every configuration we support?

> +       do_mtrr &= !IS_ENABLED(CONFIG_SPL) && !IS_ENABLED(CONFIG_FSP_VERSION1) &&
>                 !IS_ENABLED(CONFIG_SYS_SLIMBOOTLOADER);
>
>         if (do_mtrr) {
> --

Otherwise,
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Regards,
Bin

[PATCH 09/17] x86: coreboot: Show the BIOS date

[Simon Glass]

The BIOS version may not be present, e.g. on a Chrome OS build. Add the
BIOS date as well, so we get some sort of indication of coreboot's
vintage.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 board/coreboot/coreboot/coreboot.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/board/coreboot/coreboot/coreboot.c b/board/coreboot/coreboot/coreboot.c
index 175d3ce691a..93f897893eb 100644
--- a/board/coreboot/coreboot/coreboot.c
+++ b/board/coreboot/coreboot/coreboot.c
@@ -39,6 +39,7 @@ int show_board_info(void)
 	const char *bios_ver = smbios_string(bios, t0->bios_ver);
 	const char *model = smbios_string(system, t1->product_name);
 	const char *manufacturer = smbios_string(system, t1->manufacturer);
+	const char *date = smbios_string(bios, t0->bios_release_date);
 
 	if (!model || !manufacturer || !bios_ver)
 		goto fallback;
@@ -46,6 +47,8 @@ int show_board_info(void)
 	printf("Vendor: %s\n", manufacturer);
 	printf("Model: %s\n", model);
 	printf("BIOS Version: %s\n", bios_ver);
+	if (date)
+		printf("BIOS date: %s\n", date);
 
 	return 0;
 
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 09/17] x86: coreboot: Show the BIOS date

[Bin Meng]

Hi Simon,

On Wed, Apr 7, 2021 at 12:33 PM Simon Glass <sjg@chromium.org> wrote:
>
> The BIOS version may not be present, e.g. on a Chrome OS build. Add the
> BIOS date as well, so we get some sort of indication of coreboot's
> vintage.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  board/coreboot/coreboot/coreboot.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/board/coreboot/coreboot/coreboot.c b/board/coreboot/coreboot/coreboot.c
> index 175d3ce691a..93f897893eb 100644
> --- a/board/coreboot/coreboot/coreboot.c
> +++ b/board/coreboot/coreboot/coreboot.c
> @@ -39,6 +39,7 @@ int show_board_info(void)
>         const char *bios_ver = smbios_string(bios, t0->bios_ver);

I would insert the line here and name the variable as bios_date:

       const char *bios_date = smbios_string(bios, t0->bios_release_date);

>         const char *model = smbios_string(system, t1->product_name);
>         const char *manufacturer = smbios_string(system, t1->manufacturer);
> +       const char *date = smbios_string(bios, t0->bios_release_date);
>
>         if (!model || !manufacturer || !bios_ver)
>                 goto fallback;
> @@ -46,6 +47,8 @@ int show_board_info(void)
>         printf("Vendor: %s\n", manufacturer);
>         printf("Model: %s\n", model);
>         printf("BIOS Version: %s\n", bios_ver);
> +       if (date)
> +               printf("BIOS date: %s\n", date);
>
>         return 0;

Regards,
Bin

[PATCH 10/17] x86: coral: Allow booting from coreboot

[Simon Glass]

Set up coral so that it can boot from coreboot, even though it is a
bare-metal build. This helps with testing since the same image can be used
in both cases.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 board/google/chromebook_coral/coral.c | 28 +++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/board/google/chromebook_coral/coral.c b/board/google/chromebook_coral/coral.c
index 3f9235c903b..b16252d2a8b 100644
--- a/board/google/chromebook_coral/coral.c
+++ b/board/google/chromebook_coral/coral.c
@@ -10,17 +10,21 @@
 #include <command.h>
 #include <cros_ec.h>
 #include <dm.h>
+#include <init.h>
 #include <log.h>
 #include <sysinfo.h>
 #include <acpi/acpigen.h>
 #include <asm-generic/gpio.h>
 #include <asm/acpi_nhlt.h>
+#include <asm/cb_sysinfo.h>
 #include <asm/intel_gnvs.h>
 #include <asm/intel_pinctrl.h>
 #include <dm/acpi.h>
 #include <linux/delay.h>
 #include "variant_gpio.h"
 
+DECLARE_GLOBAL_DATA_PTR;
+
 struct cros_gpio_info {
 	const char *linux_name;
 	enum cros_gpio_t type;
@@ -28,6 +32,30 @@ struct cros_gpio_info {
 	int flags;
 };
 
+int misc_init_f(void)
+{
+	if (!ll_boot_init()) {
+		printf("Running as secondary loader");
+		if (gd->arch.coreboot_table) {
+			int ret;
+
+			printf(" (found coreboot table at %lx)",
+			       gd->arch.coreboot_table);
+
+			ret = get_coreboot_info(&lib_sysinfo);
+			if (ret != 0) {
+				printf("Failed to parse coreboot tables (err=%d)\n",
+				       ret);
+				return ret;
+			}
+		}
+
+		printf("\n");
+	}
+
+	return 0;
+}
+
 int arch_misc_init(void)
 {
 	return 0;
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 10/17] x86: coral: Allow booting from coreboot

[Bin Meng]

Hi Simon,

On Wed, Apr 7, 2021 at 12:33 PM Simon Glass <sjg@chromium.org> wrote:
>
> Set up coral so that it can boot from coreboot, even though it is a
> bare-metal build. This helps with testing since the same image can be used
> in both cases.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  board/google/chromebook_coral/coral.c | 28 +++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/board/google/chromebook_coral/coral.c b/board/google/chromebook_coral/coral.c
> index 3f9235c903b..b16252d2a8b 100644
> --- a/board/google/chromebook_coral/coral.c
> +++ b/board/google/chromebook_coral/coral.c
> @@ -10,17 +10,21 @@
>  #include <command.h>
>  #include <cros_ec.h>
>  #include <dm.h>
> +#include <init.h>
>  #include <log.h>
>  #include <sysinfo.h>
>  #include <acpi/acpigen.h>
>  #include <asm-generic/gpio.h>
>  #include <asm/acpi_nhlt.h>
> +#include <asm/cb_sysinfo.h>
>  #include <asm/intel_gnvs.h>
>  #include <asm/intel_pinctrl.h>
>  #include <dm/acpi.h>
>  #include <linux/delay.h>
>  #include "variant_gpio.h"
>
> +DECLARE_GLOBAL_DATA_PTR;
> +
>  struct cros_gpio_info {
>         const char *linux_name;
>         enum cros_gpio_t type;
> @@ -28,6 +32,30 @@ struct cros_gpio_info {
>         int flags;
>  };
>
> +int misc_init_f(void)
> +{
> +       if (!ll_boot_init()) {
> +               printf("Running as secondary loader");
> +               if (gd->arch.coreboot_table) {
> +                       int ret;
> +
> +                       printf(" (found coreboot table at %lx)",
> +                              gd->arch.coreboot_table);

Missing an ending '\n', if the following get_coreboot_info() fails

> +
> +                       ret = get_coreboot_info(&lib_sysinfo);
> +                       if (ret != 0) {
> +                               printf("Failed to parse coreboot tables (err=%d)\n",
> +                                      ret);
> +                               return ret;
> +                       }
> +               }
> +
> +               printf("\n");
> +       }
> +
> +       return 0;
> +}
> +
>  int arch_misc_init(void)
>  {
>         return 0;
> --

Otherwise,
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Regards,
Bin

[PATCH 11/17] x86: Add function comments to cb_sysinfo.h

[Simon Glass]

Add a function comment for get_coreboot_info() and a declaration for
cb_get_sysinfo(), since this may be called from elsewhere.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/x86/include/asm/cb_sysinfo.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/x86/include/asm/cb_sysinfo.h b/arch/x86/include/asm/cb_sysinfo.h
index 675eef6f2c9..75901359f98 100644
--- a/arch/x86/include/asm/cb_sysinfo.h
+++ b/arch/x86/include/asm/cb_sysinfo.h
@@ -215,6 +215,22 @@ struct sysinfo_t {
 
 extern struct sysinfo_t lib_sysinfo;
 
+/**
+ * get_coreboot_info() - parse the coreboot sysinfo table
+ *
+ * Parses the coreboot table if found, setting the GD_FLG_SKIP_LL_INIT flag if
+ * so.
+ *
+ * @info: Place to put the parsed information
+ * @return 0 if OK, -ENOENT if no table found
+ */
 int get_coreboot_info(struct sysinfo_t *info);
 
+/**
+ * cb_get_sysinfo() - get a pointer to the parsed coreboot sysinfo
+ *
+ * @return pointer to sysinfo, or NULL if not available
+ */
+const struct sysinfo_t *cb_get_sysinfo(void);
+
 #endif
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 11/17] x86: Add function comments to cb_sysinfo.h

[Bin Meng]

On Wed, Apr 7, 2021 at 12:33 PM Simon Glass <sjg@chromium.org> wrote:
>
> Add a function comment for get_coreboot_info() and a declaration for
> cb_get_sysinfo(), since this may be called from elsewhere.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/include/asm/cb_sysinfo.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 12/17] x86: coreboot: Use vendor in the Kconfig

[Simon Glass]

Use VENDOR_COREBOOT instead of TARGET_COREBOOT so we can have multiple
coreboot boards, sharing options. Only SYS_CONFIG_NAME needs to be
defined TARGET_COREBOOT.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/x86/cpu/coreboot/Kconfig   |  2 +-
 board/coreboot/coreboot/Kconfig | 12 ++++++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/x86/cpu/coreboot/Kconfig b/arch/x86/cpu/coreboot/Kconfig
index 497d6284ac1..b97c2779041 100644
--- a/arch/x86/cpu/coreboot/Kconfig
+++ b/arch/x86/cpu/coreboot/Kconfig
@@ -1,4 +1,4 @@
-if TARGET_COREBOOT
+if VENDOR_COREBOOT
 
 config SYS_COREBOOT
 	bool
diff --git a/board/coreboot/coreboot/Kconfig b/board/coreboot/coreboot/Kconfig
index 5bd6465d989..05e9b3b6f75 100644
--- a/board/coreboot/coreboot/Kconfig
+++ b/board/coreboot/coreboot/Kconfig
@@ -1,4 +1,4 @@
-if TARGET_COREBOOT
+if VENDOR_COREBOOT
 
 config SYS_BOARD
 	default "coreboot"
@@ -9,9 +9,6 @@ config SYS_VENDOR
 config SYS_SOC
 	default "coreboot"
 
-config SYS_CONFIG_NAME
-	default "coreboot"
-
 config SYS_TEXT_BASE
 	default 0x01110000
 
@@ -31,4 +28,11 @@ config SYS_CAR_SIZE
 	help
 	  This option specifies the board specific Cache-As-RAM (CAR) size.
 
+endif  # CONFIG_VENDOR_COREBOOT
+
+if TARGET_COREBOOT
+
+config SYS_CONFIG_NAME
+	default "coreboot"
+
 endif
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 12/17] x86: coreboot: Use vendor in the Kconfig

[Bin Meng]

Hi Simon,

On Wed, Apr 7, 2021 at 12:33 PM Simon Glass <sjg@chromium.org> wrote:
>
> Use VENDOR_COREBOOT instead of TARGET_COREBOOT so we can have multiple
> coreboot boards, sharing options. Only SYS_CONFIG_NAME needs to be
> defined TARGET_COREBOOT.
>

I am not sure what use case this is? This change makes no difference
when U-Boot is built as a coreboot payload.

> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/cpu/coreboot/Kconfig   |  2 +-
>  board/coreboot/coreboot/Kconfig | 12 ++++++++----
>  2 files changed, 9 insertions(+), 5 deletions(-)
>

Regards,
Bin

[PATCH 12/17] x86: coreboot: Use vendor in the Kconfig

[Simon Glass]

Hi Bin,

On Thu, 8 Apr 2021 at 14:59, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Wed, Apr 7, 2021 at 12:33 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Use VENDOR_COREBOOT instead of TARGET_COREBOOT so we can have multiple
> > coreboot boards, sharing options. Only SYS_CONFIG_NAME needs to be
> > defined TARGET_COREBOOT.
> >
>
> I am not sure what use case this is? This change makes no difference
> when U-Boot is built as a coreboot payload.

This is because I want to add another coreboot board, called
'chromeos_coreboot'. So in that case it will have VENDOR_COREBOOT
enabled, but not TARGET_COREBOOT.

>
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  arch/x86/cpu/coreboot/Kconfig   |  2 +-
> >  board/coreboot/coreboot/Kconfig | 12 ++++++++----
> >  2 files changed, 9 insertions(+), 5 deletions(-)
> >
>
> Regards,
> Bin

Depending on a particular TARGET Kconfig is not a good idea, IMO.

Regards,
Simon

[PATCH 13/17] x86: coreboot: Enable the cbsysinfo command

[Simon Glass]

Enable this by default on coreboot, since it is quite useful there.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/x86/cpu/coreboot/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/cpu/coreboot/Kconfig b/arch/x86/cpu/coreboot/Kconfig
index b97c2779041..11385918d83 100644
--- a/arch/x86/cpu/coreboot/Kconfig
+++ b/arch/x86/cpu/coreboot/Kconfig
@@ -26,5 +26,6 @@ config SYS_COREBOOT
 	imply CBMEM_CONSOLE
 	imply X86_TSC_READ_BASE
 	select BINMAN if X86_64
+	imply CMD_CBSYSINFO
 
 endif
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 13/17] x86: coreboot: Enable the cbsysinfo command

[Bin Meng]

Hi Simon,

On Wed, Apr 7, 2021 at 12:33 PM Simon Glass <sjg@chromium.org> wrote:
>
> Enable this by default on coreboot, since it is quite useful there.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/cpu/coreboot/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/cpu/coreboot/Kconfig b/arch/x86/cpu/coreboot/Kconfig
> index b97c2779041..11385918d83 100644
> --- a/arch/x86/cpu/coreboot/Kconfig
> +++ b/arch/x86/cpu/coreboot/Kconfig
> @@ -26,5 +26,6 @@ config SYS_COREBOOT
>         imply CBMEM_CONSOLE
>         imply X86_TSC_READ_BASE
>         select BINMAN if X86_64
> +       imply CMD_CBSYSINFO

This is not needed as in cmd/Kconfig, we have:

config CMD_CBSYSINFO
        default y if SYS_COREBOOT

Regards,
Bin

[PATCH 14/17] x86: coreboot: Document the memory map

[Simon Glass]

Add information about memory usage when U-Boot is started from coreboot.
This is useful when debugging. Also, since coreboot takes a chunk of
memory in the middle of SDRAM for use by PCI devices, it can help avoid
overwriting this with a loaded kernel by accident.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 doc/board/coreboot/coreboot.rst | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/doc/board/coreboot/coreboot.rst b/doc/board/coreboot/coreboot.rst
index 9c44c025a48..e791b7e39f0 100644
--- a/doc/board/coreboot/coreboot.rst
+++ b/doc/board/coreboot/coreboot.rst
@@ -50,3 +50,22 @@ works by using a 32-bit SPL binary to switch to 64-bit for running U-Boot. It
 can be useful for running UEFI applications, for example.
 
 This has only been lightly tested.
+
+
+Memory map
+----------
+
+::
+
+    ffffffff  Top of ROM (and last byte of 32-bit address space)
+    7a9fd000  Typical top of memory available to U-Boot
+              (use cbsysinfo to see where memory range 'table' starts)
+    10000000  Memory reserved by coreboot for mapping PCI devices
+              (typical size 2151000, includes framebuffer)
+     1920000  CONFIG_SYS_CAR_ADDR, fake Cache-as-RAM memory, used during startup
+     1110000  CONFIG_SYS_TEXT_BASE (start address of U-Boot code, before reloc)
+      110000  CONFIG_BLOBLIST_ADDR (before being relocated)
+      100000  CONFIG_PRE_CON_BUF_ADDR
+       f0000  ACPI tables set up by U-Boot
+              (typically redirects to 7ab10030 or similar)
+         500  Location of coreboot sysinfo table, used during startup
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 14/17] x86: coreboot: Document the memory map

[Bin Meng]

Hi Simon,

On Wed, Apr 7, 2021 at 12:33 PM Simon Glass <sjg@chromium.org> wrote:
>
> Add information about memory usage when U-Boot is started from coreboot.
> This is useful when debugging. Also, since coreboot takes a chunk of
> memory in the middle of SDRAM for use by PCI devices, it can help avoid
> overwriting this with a loaded kernel by accident.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  doc/board/coreboot/coreboot.rst | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/doc/board/coreboot/coreboot.rst b/doc/board/coreboot/coreboot.rst
> index 9c44c025a48..e791b7e39f0 100644
> --- a/doc/board/coreboot/coreboot.rst
> +++ b/doc/board/coreboot/coreboot.rst
> @@ -50,3 +50,22 @@ works by using a 32-bit SPL binary to switch to 64-bit for running U-Boot. It
>  can be useful for running UEFI applications, for example.
>
>  This has only been lightly tested.
> +
> +
> +Memory map
> +----------
> +
> +::

Can we use the reST table syntax for the following table?

> +
> +    ffffffff  Top of ROM (and last byte of 32-bit address space)
> +    7a9fd000  Typical top of memory available to U-Boot
> +              (use cbsysinfo to see where memory range 'table' starts)
> +    10000000  Memory reserved by coreboot for mapping PCI devices
> +              (typical size 2151000, includes framebuffer)
> +     1920000  CONFIG_SYS_CAR_ADDR, fake Cache-as-RAM memory, used during startup
> +     1110000  CONFIG_SYS_TEXT_BASE (start address of U-Boot code, before reloc)
> +      110000  CONFIG_BLOBLIST_ADDR (before being relocated)
> +      100000  CONFIG_PRE_CON_BUF_ADDR
> +       f0000  ACPI tables set up by U-Boot
> +              (typically redirects to 7ab10030 or similar)
> +         500  Location of coreboot sysinfo table, used during startup
> --

Regards,
Bin

[PATCH 15/17] x86: Check ROM exists before building vboot

[Simon Glass]

All the x86 devicetree files are built at once, whichever board is
actually being built. If coreboot is the target build, CONFIG_ROM_SIZE
is not defined and samus cannot build Chromium OS verified boot. Add
this condition to avoid errors about CONFIG_ROM_SIZE being missing.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/x86/dts/chromebook_coral.dts | 2 +-
 arch/x86/dts/chromebook_samus.dts | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/dts/chromebook_coral.dts b/arch/x86/dts/chromebook_coral.dts
index c8cb4e21c6d..66c31efb6cd 100644
--- a/arch/x86/dts/chromebook_coral.dts
+++ b/arch/x86/dts/chromebook_coral.dts
@@ -10,7 +10,7 @@
 /include/ "rtc.dtsi"
 /include/ "tsc_timer.dtsi"
 
-#ifdef CONFIG_CHROMEOS_VBOOT
+#if defined(CONFIG_CHROMEOS_VBOOT) && defined(CONFIG_ROM_SIZE)
 #include "chromeos-x86.dtsi"
 #include "flashmap-x86-ro.dtsi"
 #include "flashmap-16mb-rw.dtsi"
diff --git a/arch/x86/dts/chromebook_samus.dts b/arch/x86/dts/chromebook_samus.dts
index adaeb1ea355..ad35ab2e3fd 100644
--- a/arch/x86/dts/chromebook_samus.dts
+++ b/arch/x86/dts/chromebook_samus.dts
@@ -11,7 +11,7 @@
 
 #include "smbios.dtsi"
 
-#ifdef CONFIG_CHROMEOS_VBOOT
+#if defined(CONFIG_CHROMEOS_VBOOT) && defined(CONFIG_ROM_SIZE)
 #include "chromeos-x86.dtsi"
 #include "flashmap-x86-ro.dtsi"
 #include "flashmap-8mb-rw.dtsi"
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 15/17] x86: Check ROM exists before building vboot

[Bin Meng]

Hi Simon,

On Wed, Apr 7, 2021 at 12:33 PM Simon Glass <sjg@chromium.org> wrote:
>
> All the x86 devicetree files are built at once, whichever board is
> actually being built. If coreboot is the target build, CONFIG_ROM_SIZE
> is not defined and samus cannot build Chromium OS verified boot. Add
> this condition to avoid errors about CONFIG_ROM_SIZE being missing.

The commit title should use "x86: chromebook" as the tags, since it's
not x86 generic thing.

>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/dts/chromebook_coral.dts | 2 +-
>  arch/x86/dts/chromebook_samus.dts | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>

Otherwise,
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Regards,
Bin

[PATCH 15/17] x86: Check ROM exists before building vboot

[Jaehoon Chung]

On 4/7/21 1:32 PM, Simon Glass wrote:
> All the x86 devicetree files are built at once, whichever board is
> actually being built. If coreboot is the target build, CONFIG_ROM_SIZE
> is not defined and samus cannot build Chromium OS verified boot. Add
> this condition to avoid errors about CONFIG_ROM_SIZE being missing.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> ---
> 
>  arch/x86/dts/chromebook_coral.dts | 2 +-
>  arch/x86/dts/chromebook_samus.dts | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/dts/chromebook_coral.dts b/arch/x86/dts/chromebook_coral.dts
> index c8cb4e21c6d..66c31efb6cd 100644
> --- a/arch/x86/dts/chromebook_coral.dts
> +++ b/arch/x86/dts/chromebook_coral.dts
> @@ -10,7 +10,7 @@
>  /include/ "rtc.dtsi"
>  /include/ "tsc_timer.dtsi"
>  
> -#ifdef CONFIG_CHROMEOS_VBOOT
> +#if defined(CONFIG_CHROMEOS_VBOOT) && defined(CONFIG_ROM_SIZE)
>  #include "chromeos-x86.dtsi"
>  #include "flashmap-x86-ro.dtsi"
>  #include "flashmap-16mb-rw.dtsi"
> diff --git a/arch/x86/dts/chromebook_samus.dts b/arch/x86/dts/chromebook_samus.dts
> index adaeb1ea355..ad35ab2e3fd 100644
> --- a/arch/x86/dts/chromebook_samus.dts
> +++ b/arch/x86/dts/chromebook_samus.dts
> @@ -11,7 +11,7 @@
>  
>  #include "smbios.dtsi"
>  
> -#ifdef CONFIG_CHROMEOS_VBOOT
> +#if defined(CONFIG_CHROMEOS_VBOOT) && defined(CONFIG_ROM_SIZE)
>  #include "chromeos-x86.dtsi"
>  #include "flashmap-x86-ro.dtsi"
>  #include "flashmap-8mb-rw.dtsi"
> 

[PATCH 16/17] dtoc: Check that a parent is not missing

[Simon Glass]

With of-platdata-inst we want to set up a reference to each devices'
parent device, if there is one. If we find that the device has a parent
(i.e. is not a root node) but it is not in the list of devices being
written, then we cannot create the reference.

Report an error in this case, since it indicates that the parent node
is either missing a compatible string, is disabled, or perhaps does not
have any properties because it was not tagged for SPL.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/dtoc/dtb_platdata.py             |  9 ++++++++
 tools/dtoc/test/dtoc_test_noparent.dts | 32 ++++++++++++++++++++++++++
 tools/dtoc/test_dtoc.py                | 10 ++++++++
 3 files changed, 51 insertions(+)
 create mode 100644 tools/dtoc/test/dtoc_test_noparent.dts

diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
index 1374f01c707..c303224e295 100644
--- a/tools/dtoc/dtb_platdata.py
+++ b/tools/dtoc/dtb_platdata.py
@@ -749,6 +749,15 @@ class DtbPlatdata():
                     break
 
         if node.parent and node.parent.parent:
+            if node.parent not in self._valid_nodes:
+                # This might indicate that the parent node is not in the
+                # SPL/TPL devicetree but the child is. For example if we are
+                # dealing with of-platdata in TPL, the parent has a
+                # u-boot,dm-spl tag but the child has u-boot,dm-pre-reloc. In
+                # this case the child node exists in TPL but the parent does
+                # not.
+                raise ValueError("Node '%s' requires parent node '%s' but it is not in the valid list" %
+                                 (node.path, node.parent.path))
             self.buf('\t.parent\t\t= DM_DEVICE_REF(%s),\n' %
                      node.parent.var_name)
         if priv_name:
diff --git a/tools/dtoc/test/dtoc_test_noparent.dts b/tools/dtoc/test/dtoc_test_noparent.dts
new file mode 100644
index 00000000000..5a820301e72
--- /dev/null
+++ b/tools/dtoc/test/dtoc_test_noparent.dts
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test device tree file for dtoc
+ *
+ * Copyright 2017 Google, Inc
+ */
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	i2c at 0 {
+		compatible = "sandbox,i2c";
+		u-boot,dm-pre-tpl;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		spl-test {
+			u-boot,dm-pre-reloc;
+			compatible = "sandbox,spl-test";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+			pmic at 9 {
+				compatible = "sandbox,pmic";
+				u-boot,dm-pre-reloc;
+				reg = <9>;
+				low-power;
+			};
+		};
+	};
+};
diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py
index a05e3d9ed65..e7397ae554b 100755
--- a/tools/dtoc/test_dtoc.py
+++ b/tools/dtoc/test_dtoc.py
@@ -1839,3 +1839,13 @@ U_BOOT_DRVINFO(spl_test2) = {
         dtb_file = get_dtb_file('dtoc_test_single_reg.dts')
         output = tools.GetOutputFilename('output')
         self.run_test(['struct'], dtb_file, output)
+
+    def test_missing_parent(self):
+        """Test detection of a parent node with no properties"""
+        dtb_file = get_dtb_file('dtoc_test_noparent.dts', capture_stderr=True)
+        output = tools.GetOutputFilename('output')
+        with self.assertRaises(ValueError) as exc:
+            self.run_test(['device'], dtb_file, output, instantiate=True)
+        self.assertIn("Node '/i2c at 0/spl-test/pmic at 9' requires parent node "
+                      "'/i2c at 0/spl-test' but it is not in the valid list",
+                      str(exc.exception))
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 17/17] doc: Update documentation for cros-2021.04 release

[Simon Glass]

With the new 2021.04 we have a new version of Chromium OS boot, which
supports sandbox, coral and coral-on-coreboot. Add documentation for
this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 doc/chromium/run_vboot.rst | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/doc/chromium/run_vboot.rst b/doc/chromium/run_vboot.rst
index 41b4f631835..a9e4408d55f 100644
--- a/doc/chromium/run_vboot.rst
+++ b/doc/chromium/run_vboot.rst
@@ -6,11 +6,15 @@
 Running U-Boot with Chromium OS verified boot
 =============================================
 
+Note: Once you use the source below you can obtain extra documentation with
+'make htmldocs'. See the 'Internal Documentation' link, under
+'Chromium OS-specific doc'.
+
 To obtain::
 
    git clone https://github.com/sjg20/u-boot.git
    cd u-boot
-   git checkout cros-master
+   git checkout cros-2021.04
 
    cd ..
    git clone https://chromium.googlesource.com/chromiumos/platform/vboot_reference
@@ -169,7 +173,8 @@ detect problems that affect the flow or particular vboot features.
 U-Boot without Chromium OS verified boot
 ----------------------------------------
 
-The following script can be used to boot a Chrome OS image on coral::
+The following script can be used to boot a Chrome OS image on coral. It is
+defined as the boot command in mainline::
 
    # Read the image header and obtain the address of the kernel
    # The offset 4f0 is defined by verified boot and may change for other
@@ -195,10 +200,4 @@ The following script can be used to boot a Chrome OS image on coral::
    zboot go
 
 
-TO DO
------
-
-Get the full ACPI tables working with Coral
-
-
 7 October 2018
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 17/17] doc: Update documentation for cros-2021.04 release

[Bin Meng]

Hi Simon,

On Wed, Apr 7, 2021 at 12:33 PM Simon Glass <sjg@chromium.org> wrote:
>
> With the new 2021.04 we have a new version of Chromium OS boot, which
> supports sandbox, coral and coral-on-coreboot. Add documentation for

Chrominum OS can run on Sandbox?

> this.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  doc/chromium/run_vboot.rst | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Regards,
Bin

[PATCH 17/17] doc: Update documentation for cros-2021.04 release

[Simon Glass]

Hi Bin,

On Thu, 8 Apr 2021 at 15:02, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Wed, Apr 7, 2021 at 12:33 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > With the new 2021.04 we have a new version of Chromium OS boot, which
> > supports sandbox, coral and coral-on-coreboot. Add documentation for
>
> Chrominum OS can run on Sandbox?

Well the U-Boot part, yes. Once it boots a kernel sandbox just exits.
This is useful for testing.


>
> > this.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  doc/chromium/run_vboot.rst | 15 +++++++--------
> >  1 file changed, 7 insertions(+), 8 deletions(-)
> >
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>

Regards,
Simon