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

newer
[PATCH v1 0/2] arm: s5p4418: fix...

older
[PATCH v4 0/5] Populate kaslr seed...

Simon Glass

22 Sep 2023 22 Sep '23

11:38 p.m.

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

-- 2.42.0.515.g380fc7ccd1-goog

Show replies by date

Simon Glass

22 Sep 22 Sep

11:38 p.m.

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

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

Simon Glass

11:38 p.m.

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

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

Simon Glass

11:38 p.m.

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

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

Simon Glass

11:38 p.m.

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

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

Mattijs Korpershoek

23 Sep 23 Sep

6:39 p.m.

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

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

Simon Glass

12 Nov 12 Nov

9:01 p.m.

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

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

546

Age (days ago)

597

Last active (days ago)

List overview

Download

6 comments

2 participants

tags (0)

participants (2)

Mattijs Korpershoek
Simon Glass