[PATCH v2 0/4] Resolve issues with booting distros on x86

[PATCH v2 0/4] Resolve issues with booting distros on x86

[Simon Glass]

This little series reprises the EFI-video fix, fixes a USB problem and
enables a boot script for coreboot.

With these changes it is possible to boot a Linux distro automatically
with U-Boot on x86, including when U-Boot is the second-stage
bootloader.

Changes in v2:
- Rebase to -next
- Add some more comments to the header file
- Add fixes tag
- Add new patch to add a return code to bootflow menu
- Add new patch to add a coreboot boot script
- Add new patch to avoid unbinding devices in use by bootflows

Simon Glass (4):
  efi: Correct handling of frame buffer
  bootstd: Add a return code to bootflow menu
  x86: coreboot: Add a boot script
  usb: Avoid unbinding devices in use by bootflows

 boot/bootm.c                  |  2 +-
 cmd/bootflow.c                |  6 +++---
 common/usb.c                  |  7 ++++++-
 configs/coreboot64_defconfig  |  1 +
 configs/coreboot_defconfig    |  1 +
 drivers/usb/host/usb-uclass.c | 14 ++++++++++++--
 include/usb.h                 | 15 ++++++++++++++-
 include/video.h               |  9 ++++++---
 lib/efi_loader/efi_gop.c      | 12 +++++++-----
 9 files changed, 51 insertions(+), 16 deletions(-)

-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH v2 1/4] efi: Correct handling of frame buffer

[Simon Glass]

The efi_gop driver uses private fields from the video uclass to obtain a
pointer to the frame buffer. Use the platform data instead.

Check the VIDEO_COPY setting to determine which frame buffer to use. Once
the next stage is running (and making use of U-Boot's EFI boot services)
U-Boot does not handle copying from priv->fb to the hardware framebuffer,
so we must allow EFI to write directly to the hardware framebuffer.

We could provide a function to read this, but it seems better to just
document how it works. The original change ignored an explicit comment
in the video.h file ("Things that are private to the uclass: don't use
these in the driver") which is why this was missed when the VIDEO_COPY
feature was added.

Signed-off-by: Simon Glass <sjg@chromium.org>
Fixes: 8f661a5b662 ("efi_loader: gop: Expose fb when 32bpp")
---

Changes in v2:
- Rebase to -next
- Add some more comments to the header file
- Add fixes tag

 include/video.h          |  9 ++++++---
 lib/efi_loader/efi_gop.c | 12 +++++++-----
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/include/video.h b/include/video.h
index 5048116a3d5..4d8df9baaad 100644
--- a/include/video.h
+++ b/include/video.h
@@ -21,9 +21,12 @@ struct udevice;
  * @align: Frame-buffer alignment, indicating the memory boundary the frame
  *	buffer should start on. If 0, 1MB is assumed
  * @size: Frame-buffer size, in bytes
- * @base: Base address of frame buffer, 0 if not yet known
- * @copy_base: Base address of a hardware copy of the frame buffer. See
- *	CONFIG_VIDEO_COPY.
+ * @base: Base address of frame buffer, 0 if not yet known. If CONFIG_VIDEO_COPY
+ *	is enabled, this is the software copy, so writes to this will not be
+ *	visible until vidconsole_sync_copy() is called. If CONFIG_VIDEO_COPY is
+ *	disabled, this is the hardware framebuffer.
+ * @copy_base: Base address of a hardware copy of the frame buffer. If
+ *	CONFIG_VIDEO_COPY is disabled, this is not used.
  * @copy_size: Size of copy framebuffer, used if @size is 0
  * @hide_logo: Hide the logo (used for testing)
  */
diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
index 778b693f983..a09db31eb46 100644
--- a/lib/efi_loader/efi_gop.c
+++ b/lib/efi_loader/efi_gop.c
@@ -10,6 +10,7 @@
 #include <efi_loader.h>
 #include <log.h>
 #include <malloc.h>
+#include <mapmem.h>
 #include <video.h>
 #include <asm/global_data.h>
 
@@ -467,10 +468,10 @@ efi_status_t efi_gop_register(void)
 	struct efi_gop_obj *gopobj;
 	u32 bpix, format, col, row;
 	u64 fb_base, fb_size;
-	void *fb;
 	efi_status_t ret;
 	struct udevice *vdev;
 	struct video_priv *priv;
+	struct video_uc_plat *plat;
 
 	/* We only support a single video output device for now */
 	if (uclass_first_device_err(UCLASS_VIDEO, &vdev)) {
@@ -483,9 +484,10 @@ efi_status_t efi_gop_register(void)
 	format = priv->format;
 	col = video_get_xsize(vdev);
 	row = video_get_ysize(vdev);
-	fb_base = (uintptr_t)priv->fb;
-	fb_size = priv->fb_size;
-	fb = priv->fb;
+
+	plat = dev_get_uclass_plat(vdev);
+	fb_base = IS_ENABLED(CONFIG_VIDEO_COPY) ? plat->copy_base : plat->base;
+	fb_size = plat->size;
 
 	switch (bpix) {
 	case VIDEO_BPP16:
@@ -547,7 +549,7 @@ efi_status_t efi_gop_register(void)
 	}
 	gopobj->info.pixels_per_scanline = col;
 	gopobj->bpix = bpix;
-	gopobj->fb = fb;
+	gopobj->fb = map_sysmem(fb_base, fb_size);
 
 	return EFI_SUCCESS;
 }
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH v2 2/4] bootstd: Add a return code to bootflow menu

[Simon Glass]

Return an error when the user does not select an OS, so we know whether
to boot or not.

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

Changes in v2:
- Add new patch to add a return code to bootflow menu

 cmd/bootflow.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/cmd/bootflow.c b/cmd/bootflow.c
index 300ad3aaa76..dbc47961a76 100644
--- a/cmd/bootflow.c
+++ b/cmd/bootflow.c
@@ -459,10 +459,10 @@ static int do_bootflow_menu(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (ret) {
 		if (ret == -EAGAIN)
 			printf("Nothing chosen\n");
-		else {
+		else
 			printf("Menu failed (err=%d)\n", ret);
-			return CMD_RET_FAILURE;
-		}
+
+		return CMD_RET_FAILURE;
 	}
 
 	printf("Selected: %s\n", bflow->os_name ? bflow->os_name : bflow->name);
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH v2 3/4] x86: coreboot: Add a boot script

[Simon Glass]

Provide the user with a list of available boot options. Selecting one
causes it to be booted. Pressing <ESC> causes U-Boot to return to the
command-line prompt.

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

Changes in v2:
- Add new patch to add a coreboot boot script

 configs/coreboot64_defconfig | 1 +
 configs/coreboot_defconfig   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/configs/coreboot64_defconfig b/configs/coreboot64_defconfig
index 555d281ef3c..dfd47ebb1cf 100644
--- a/configs/coreboot64_defconfig
+++ b/configs/coreboot64_defconfig
@@ -17,6 +17,7 @@ CONFIG_SYS_MONITOR_BASE=0x01120000
 CONFIG_SHOW_BOOT_PROGRESS=y
 CONFIG_USE_BOOTARGS=y
 CONFIG_BOOTARGS="root=/dev/sdb3 init=/sbin/init rootwait ro"
+CONFIG_BOOTCOMMAND="bootflow scan -l; if bootflow menu; then bootflow boot; fi"
 CONFIG_PRE_CONSOLE_BUFFER=y
 CONFIG_SYS_CONSOLE_INFO_QUIET=y
 CONFIG_LOG=y
diff --git a/configs/coreboot_defconfig b/configs/coreboot_defconfig
index edc38f1f592..ca5e581f8f2 100644
--- a/configs/coreboot_defconfig
+++ b/configs/coreboot_defconfig
@@ -15,6 +15,7 @@ CONFIG_SYS_MONITOR_BASE=0x01110000
 CONFIG_SHOW_BOOT_PROGRESS=y
 CONFIG_USE_BOOTARGS=y
 CONFIG_BOOTARGS="root=/dev/sdb3 init=/sbin/init rootwait ro"
+CONFIG_BOOTCOMMAND="bootflow scan -l; if bootflow menu; then bootflow boot; fi"
 CONFIG_PRE_CONSOLE_BUFFER=y
 CONFIG_SYS_CONSOLE_INFO_QUIET=y
 CONFIG_LOG=y
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH v2 4/4] usb: Avoid unbinding devices in use by bootflows

[Simon Glass]

When a USB device is unbound, it causes any bootflows attached to it to
be removed, via a call to bootdev_clear_bootflows() from
bootdev_pre_unbind(). This obviously makes it impossible to boot the
bootflow.

However, when booting a bootflow that relies on USB, usb_stop() is
called, which unbinds the device. For EFI, this happens in
efi_exit_boot_services() which means that the bootflow disappears
before it is finished with.

There is no need to unbind all the USB devices just to quiesce them.
Add a new usb_pause() call which removes them but leaves them bound.

This resolves a hang on x86 when booting a distro from USB. This was
found using a device with 4 bootflows, the last of which was USB.

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

Changes in v2:
- Add new patch to avoid unbinding devices in use by bootflows

 boot/bootm.c                  |  2 +-
 common/usb.c                  |  7 ++++++-
 drivers/usb/host/usb-uclass.c | 14 ++++++++++++--
 include/usb.h                 | 15 ++++++++++++++-
 4 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/boot/bootm.c b/boot/bootm.c
index b1c3afe0a3a..5c9ba083e64 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -501,7 +501,7 @@ ulong bootm_disable_interrupts(void)
 	 * updated every 1 ms within the HCCA structure in SDRAM! For more
 	 * details see the OpenHCI specification.
 	 */
-	usb_stop();
+	usb_pause();
 #endif
 	return iflag;
 }
diff --git a/common/usb.c b/common/usb.c
index 836506dcd9e..4d6ac69111e 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -126,7 +126,7 @@ int usb_init(void)
 /******************************************************************************
  * Stop USB this stops the LowLevel Part and deregisters USB devices.
  */
-int usb_stop(void)
+int usb_pause(void)
 {
 	int i;
 
@@ -144,6 +144,11 @@ int usb_stop(void)
 	return 0;
 }
 
+int usb_stop(void)
+{
+	return usb_pause();
+}
+
 /******************************************************************************
  * Detect if a USB device has been plugged or unplugged.
  */
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index a1cd0ad2d66..c26c65d7986 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -173,7 +173,7 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size)
 	return ops->get_max_xfer_size(bus, size);
 }
 
-int usb_stop(void)
+static int usb_finish(bool unbind_all)
 {
 	struct udevice *bus;
 	struct udevice *rh;
@@ -195,7 +195,7 @@ int usb_stop(void)
 
 		/* Locate root hub device */
 		device_find_first_child(bus, &rh);
-		if (rh) {
+		if (rh && unbind_all) {
 			/*
 			 * All USB devices are children of root hub.
 			 * Unbinding root hub will unbind all of its children.
@@ -222,6 +222,16 @@ int usb_stop(void)
 	return err;
 }
 
+int usb_stop(void)
+{
+	return usb_finish(true);
+}
+
+int usb_pause(void)
+{
+	return usb_finish(false);
+}
+
 static void usb_scan_bus(struct udevice *bus, bool recurse)
 {
 	struct usb_bus_priv *priv;
diff --git a/include/usb.h b/include/usb.h
index 09e3f0cb309..ad39b09a6e4 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -265,7 +265,20 @@ int usb_kbd_deregister(int force);
  */
 int usb_init(void);
 
-int usb_stop(void); /* stop the USB Controller */
+/**
+ * usb_stop() - stop the USB Controller and unbind all USB controllers/devices
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int usb_stop(void);
+
+/**
+ * usb_pause() - stop the USB Controller DMA, etc.
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int usb_pause(void);
+
 int usb_detect_change(void); /* detect if a USB device has been (un)plugged */
 
 
-- 
2.42.0.515.g380fc7ccd1-goog

Re: [PATCH v2 4/4] usb: Avoid unbinding devices in use by bootflows

[Mattijs Korpershoek]

Hi Simon,

Thank you for your patch.

On ven., sept. 22, 2023 at 15:38, Simon Glass <sjg@chromium.org> wrote:

> When a USB device is unbound, it causes any bootflows attached to it to
> be removed, via a call to bootdev_clear_bootflows() from
> bootdev_pre_unbind(). This obviously makes it impossible to boot the
> bootflow.
>
> However, when booting a bootflow that relies on USB, usb_stop() is
> called, which unbinds the device. For EFI, this happens in
> efi_exit_boot_services() which means that the bootflow disappears
> before it is finished with.
>
> There is no need to unbind all the USB devices just to quiesce them.
> Add a new usb_pause() call which removes them but leaves them bound.
>
> This resolves a hang on x86 when booting a distro from USB. This was
> found using a device with 4 bootflows, the last of which was USB.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v2:
> - Add new patch to avoid unbinding devices in use by bootflows
>
>  boot/bootm.c                  |  2 +-
>  common/usb.c                  |  7 ++++++-
>  drivers/usb/host/usb-uclass.c | 14 ++++++++++++--
>  include/usb.h                 | 15 ++++++++++++++-
>  4 files changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/boot/bootm.c b/boot/bootm.c
> index b1c3afe0a3a..5c9ba083e64 100644
> --- a/boot/bootm.c
> +++ b/boot/bootm.c
> @@ -501,7 +501,7 @@ ulong bootm_disable_interrupts(void)
>  	 * updated every 1 ms within the HCCA structure in SDRAM! For more
>  	 * details see the OpenHCI specification.
>  	 */
> -	usb_stop();
> +	usb_pause();
>  #endif
>  	return iflag;
>  }
> diff --git a/common/usb.c b/common/usb.c
> index 836506dcd9e..4d6ac69111e 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -126,7 +126,7 @@ int usb_init(void)
>  /******************************************************************************
>   * Stop USB this stops the LowLevel Part and deregisters USB devices.
>   */
> -int usb_stop(void)
> +int usb_pause(void)

nit: I know this is the "legacy" (pre-DM) stuff, but isn't renaming the above
function making the comment wrong?

Can't we keep this function usb_stop() and create another one named
usb_pause() which calls usb_stop() ?

>  {
>  	int i;
>  
> @@ -144,6 +144,11 @@ int usb_stop(void)
>  	return 0;
>  }
>  
> +int usb_stop(void)
> +{
> +	return usb_pause();
> +}
> +
>  /******************************************************************************
>   * Detect if a USB device has been plugged or unplugged.
>   */
> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> index a1cd0ad2d66..c26c65d7986 100644
> --- a/drivers/usb/host/usb-uclass.c
> +++ b/drivers/usb/host/usb-uclass.c
> @@ -173,7 +173,7 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size)
>  	return ops->get_max_xfer_size(bus, size);
>  }
>  
> -int usb_stop(void)
> +static int usb_finish(bool unbind_all)
>  {
>  	struct udevice *bus;
>  	struct udevice *rh;
> @@ -195,7 +195,7 @@ int usb_stop(void)
>  
>  		/* Locate root hub device */
>  		device_find_first_child(bus, &rh);
> -		if (rh) {
> +		if (rh && unbind_all) {
>  			/*
>  			 * All USB devices are children of root hub.
>  			 * Unbinding root hub will unbind all of its children.
> @@ -222,6 +222,16 @@ int usb_stop(void)
>  	return err;
>  }
>  
> +int usb_stop(void)
> +{
> +	return usb_finish(true);
> +}
> +
> +int usb_pause(void)
> +{
> +	return usb_finish(false);
> +}

What happens if someone calls usb_pause() followed by usb_init() on a
root hub ?

I think it behaves properly because usb_init() calls
remove_inactive_children().

> +
>  static void usb_scan_bus(struct udevice *bus, bool recurse)
>  {
>  	struct usb_bus_priv *priv;
> diff --git a/include/usb.h b/include/usb.h
> index 09e3f0cb309..ad39b09a6e4 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -265,7 +265,20 @@ int usb_kbd_deregister(int force);
>   */
>  int usb_init(void);
>  
> -int usb_stop(void); /* stop the USB Controller */
> +/**
> + * usb_stop() - stop the USB Controller and unbind all USB controllers/devices
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int usb_stop(void);
> +
> +/**
> + * usb_pause() - stop the USB Controller DMA, etc.
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int usb_pause(void);
> +
>  int usb_detect_change(void); /* detect if a USB device has been (un)plugged */
>  
>  
> -- 
> 2.42.0.515.g380fc7ccd1-goog

Re: [PATCH v2 4/4] usb: Avoid unbinding devices in use by bootflows

[Simon Glass]

Hi Mattijs,

On Sat, 23 Sept 2023 at 10:39, Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
>
> Hi Simon,
>
> Thank you for your patch.

Thanks for reviewing it!

>
> On ven., sept. 22, 2023 at 15:38, Simon Glass <sjg@chromium.org> wrote:
>
> > When a USB device is unbound, it causes any bootflows attached to it to
> > be removed, via a call to bootdev_clear_bootflows() from
> > bootdev_pre_unbind(). This obviously makes it impossible to boot the
> > bootflow.
> >
> > However, when booting a bootflow that relies on USB, usb_stop() is
> > called, which unbinds the device. For EFI, this happens in
> > efi_exit_boot_services() which means that the bootflow disappears
> > before it is finished with.
> >
> > There is no need to unbind all the USB devices just to quiesce them.
> > Add a new usb_pause() call which removes them but leaves them bound.
> >
> > This resolves a hang on x86 when booting a distro from USB. This was
> > found using a device with 4 bootflows, the last of which was USB.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Add new patch to avoid unbinding devices in use by bootflows
> >
> >  boot/bootm.c                  |  2 +-
> >  common/usb.c                  |  7 ++++++-
> >  drivers/usb/host/usb-uclass.c | 14 ++++++++++++--
> >  include/usb.h                 | 15 ++++++++++++++-
> >  4 files changed, 33 insertions(+), 5 deletions(-)
> >
> > diff --git a/boot/bootm.c b/boot/bootm.c
> > index b1c3afe0a3a..5c9ba083e64 100644
> > --- a/boot/bootm.c
> > +++ b/boot/bootm.c
> > @@ -501,7 +501,7 @@ ulong bootm_disable_interrupts(void)
> >        * updated every 1 ms within the HCCA structure in SDRAM! For more
> >        * details see the OpenHCI specification.
> >        */
> > -     usb_stop();
> > +     usb_pause();
> >  #endif
> >       return iflag;
> >  }
> > diff --git a/common/usb.c b/common/usb.c
> > index 836506dcd9e..4d6ac69111e 100644
> > --- a/common/usb.c
> > +++ b/common/usb.c
> > @@ -126,7 +126,7 @@ int usb_init(void)
> >  /******************************************************************************
> >   * Stop USB this stops the LowLevel Part and deregisters USB devices.
> >   */
> > -int usb_stop(void)
> > +int usb_pause(void)
>
> nit: I know this is the "legacy" (pre-DM) stuff, but isn't renaming the above
> function making the comment wrong?
>
> Can't we keep this function usb_stop() and create another one named
> usb_pause() which calls usb_stop() ?

I missed this comment but will do this for v4.

>
> >  {
> >       int i;
> >
> > @@ -144,6 +144,11 @@ int usb_stop(void)
> >       return 0;
> >  }
> >
> > +int usb_stop(void)
> > +{
> > +     return usb_pause();
> > +}
> > +
> >  /******************************************************************************
> >   * Detect if a USB device has been plugged or unplugged.
> >   */
> > diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> > index a1cd0ad2d66..c26c65d7986 100644
> > --- a/drivers/usb/host/usb-uclass.c
> > +++ b/drivers/usb/host/usb-uclass.c
> > @@ -173,7 +173,7 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size)
> >       return ops->get_max_xfer_size(bus, size);
> >  }
> >
> > -int usb_stop(void)
> > +static int usb_finish(bool unbind_all)
> >  {
> >       struct udevice *bus;
> >       struct udevice *rh;
> > @@ -195,7 +195,7 @@ int usb_stop(void)
> >
> >               /* Locate root hub device */
> >               device_find_first_child(bus, &rh);
> > -             if (rh) {
> > +             if (rh && unbind_all) {
> >                       /*
> >                        * All USB devices are children of root hub.
> >                        * Unbinding root hub will unbind all of its children.
> > @@ -222,6 +222,16 @@ int usb_stop(void)
> >       return err;
> >  }
> >
> > +int usb_stop(void)
> > +{
> > +     return usb_finish(true);
> > +}
> > +
> > +int usb_pause(void)
> > +{
> > +     return usb_finish(false);
> > +}
>
> What happens if someone calls usb_pause() followed by usb_init() on a
> root hub ?
>
> I think it behaves properly because usb_init() calls
> remove_inactive_children().

I'm not sure about that. The USB enumeration binds new devices, which
is why we unbind them before enumerating.

remove_inactive_children() only removes them (makes them inactive /
unprobed). It does not unbind them.

>
> > +
> >  static void usb_scan_bus(struct udevice *bus, bool recurse)
> >  {
> >       struct usb_bus_priv *priv;
> > diff --git a/include/usb.h b/include/usb.h
> > index 09e3f0cb309..ad39b09a6e4 100644
> > --- a/include/usb.h
> > +++ b/include/usb.h
> > @@ -265,7 +265,20 @@ int usb_kbd_deregister(int force);
> >   */
> >  int usb_init(void);
> >
> > -int usb_stop(void); /* stop the USB Controller */
> > +/**
> > + * usb_stop() - stop the USB Controller and unbind all USB controllers/devices
> > + *
> > + * Return: 0 if OK, -ve on error
> > + */
> > +int usb_stop(void);
> > +
> > +/**
> > + * usb_pause() - stop the USB Controller DMA, etc.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + */
> > +int usb_pause(void);
> > +
> >  int usb_detect_change(void); /* detect if a USB device has been (un)plugged */
> >
> >
> > --
> > 2.42.0.515.g380fc7ccd1-goog

Regards,
Simon