dm: device_remove: Don't return in device_chld_remove() upon error

This new flag can be added to DM device drivers, which need to do some final configuration before U-Boot exits and the OS (e.g. Linux) is started. The remove functions of those drivers will get called at this stage to do these last-stage configuration steps.

Signed-off-by Stefan Roese <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

---
 drivers/core/device-remove.c | 17 ++++++++++++-----
 include/dm/device.h          | 11 ++++++++++-
 2 files changed, 22 insertions(+), 6 deletions(-)

drivers/core/device-remove.c +7 -5

+               return 1;
+
+       return 0;
+}
+
 int device_remove(struct udevice *dev, uint flags)
 {
        const struct driver *drv;

@@ -174,9 +184,7 @@ int device_remove(struct udevice *dev, uint flags)
         * Remove the device if called with the "normal" remove flag set,
         * or if the remove flag matches any of the drivers remove flags
         */
-       if (drv->remove &&
-           ((flags & DM_REMOVE_NORMAL) ||
-            (flags & (drv->flags & DM_FLAG_ACTIVE_DMA)))) {
+       if (drv->remove && flags_remove(flags, drv->flags)) {
                ret = drv->remove(dev);
                if (ret)
                        goto err_remove;

@@ -190,8 +198,7 @@ int device_remove(struct udevice *dev, uint flags)
                }
        }
 
-       if ((flags & DM_REMOVE_NORMAL) ||
-           (flags & (drv->flags & DM_FLAG_ACTIVE_DMA))) {
+       if (flags_remove(flags, drv->flags)) {
                device_free(dev);
 
                dev->seq = -1;

include/dm/device.h +10 -2

index 079ec57003..c49b8e688e 100644
@@ -55,6 +55,12 @@ struct driver_info;
 #define DM_FLAG_ACTIVE_DMA             (1 << 9)
 
 /*
+ * Call driver remove function to do some final configuration, before
+ * U-Boot exits and the OS is started
+ */
+#define DM_FLAG_PRE_OS_FINALIZE                (1 << 10)
+
+/*
  * One or multiple of these flags are passed to device_remove() so that
  * a selective device removal as specified by the remove-stage and the
  * driver flags can be done.

@@ -66,10 +72,13 @@ enum {
        /* Remove devices with active DMA */
        DM_REMOVE_ACTIVE_DMA = DM_FLAG_ACTIVE_DMA,
 
+       /* Remove devices which need some pre-OS finalization */
+       DM_REMOVE_PRE_OS_FINALIZE = DM_FLAG_PRE_OS_FINALIZE,
+
        /* Add more use cases here */
 
        /* Remove devices with any active flag */
-       DM_REMOVE_ACTIVE_ALL = DM_REMOVE_ACTIVE_DMA,
+       DM_REMOVE_ACTIVE_ALL = DM_REMOVE_ACTIVE_DMA | DM_REMOVE_PRE_OS_FINALIZE,
 };
 
 /**
--

2.12.2

Hi Stefan,

Quoted 5 lines · Stefan Roese

This new flag can be added to DM device drivers, which need to do some final configuration before U-Boot exits and the OS (e.g. Linux) is started. The remove functions of those drivers will get called at this stage to do these last-stage configuration steps.

Quoted 7 lines

Signed-off-by Stefan Roese <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

---
 drivers/core/device-remove.c | 17 ++++++++++++-----
 include/dm/device.h          | 11 ++++++++++-
 2 files changed, 22 insertions(+), 6 deletions(-)

Reviewed-by Simon Glass <[email protected]>

You could perhaps have a separate patch to move the code into flags_remove(), but I suppose it isn't important.

nit below.

Quoted 9 lines

drivers/core/device-remove.c +0 -0

index 8b46f3343e..390be5a0d8 100644
@@ -148,6 +148,16 @@ void device_free(struct udevice *dev)
        devres_release_probe(dev);
 }
 

drivers/core/device-remove.c void device_free(struct udevice *dev) · line 151

Quoted 1 line

drivers/core/device-remove.c +1 -0

+static int flags_remove(uint flags, uint drv_flags)

Can this be bool, and return true/false?

drivers/core/device-remove.c void device_free(struct udevice *dev) · line 155

Quoted 4 lines

drivers/core/device-remove.c +4 -0

+{
+       if ((flags & DM_REMOVE_NORMAL) ||
+           (flags & (drv_flags &
+                     (DM_FLAG_ACTIVE_DMA | DM_FLAG_PRE_OS_FINALIZE))))

What do you think about OS_PREPARE instead? It doesn't really finalize the OS...

This patch adds a call to dm_remove_devices_flags() to bootm_announce_and_cleanup() so that drivers that have one of the removal flags set (e.g. DM_FLAG_ACTIVE_DMA_REMOVE) in their driver struct, may do some last-stage cleanup before the OS is started.

Signed-off-by Stefan Roese <[email protected]>

Cc Bin Meng <[email protected]>

Cc Simon Glass <[email protected]>

---
 arch/x86/lib/bootm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

This patch adds a remove function to the Intel ICH SPI driver, that will be called upon U-Boot exit, directly before the OS (Linux) is started. This function takes care of configuring the BIOS registers in the SPI controller (similar to what a "standard" BIOS or coreboot does), so that the Linux MTD device driver is able to correctly read/write to the SPI NOR chip. Without this, the chip is not detected at all.

Signed-off-by Stefan Roese <[email protected]>

Cc Bin Meng <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/spi/ich.c | 18 ++++++++++++++++++
 drivers/spi/ich.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 65 insertions(+), 7 deletions(-)

drivers/spi/ich.c +18 -0

index 893fe33b66..60f40c5cf5 100644
@@ -617,6 +617,22 @@ static int ich_spi_probe(struct udevice *dev)
        return 0;
 }
 
+static int ich_spi_remove(struct udevice *bus)
+{
+       struct ich_spi_priv *ctlr = dev_get_priv(bus);
+
+       /*
+        * Configure SPI controller so that the Linux MTD driver can fully
+        * access the SPI NOR chip
+        */
+       ich_writew(ctlr, SPI_OPPREFIX, ctlr->preop);
+       ich_writew(ctlr, SPI_OPTYPE, ctlr->optype);
+       ich_writel(ctlr, SPI_OPMENU_LOWER, ctlr->opmenu);
+       ich_writel(ctlr, SPI_OPMENU_UPPER, ctlr->opmenu + sizeof(u32));
+
+       return 0;
+}
+
 static int ich_spi_set_speed(struct udevice *bus, uint speed)
 {
        struct ich_spi_priv *priv = dev_get_priv(bus);

@@ -700,4 +716,6 @@ U_BOOT_DRIVER(ich_spi) = {
        .priv_auto_alloc_size = sizeof(struct ich_spi_priv),
        .child_pre_probe = ich_spi_child_pre_probe,
        .probe  = ich_spi_probe,
+       .remove = ich_spi_remove,
+       .flags  = DM_FLAG_PRE_OS_FINALIZE,
 };

drivers/spi/ich.h +37 -7

index bd0a820809..dcb8a9048f 100644
@@ -102,13 +102,6 @@ enum {
 };
 
 enum {
-       SPI_OPCODE_TYPE_READ_NO_ADDRESS =       0,
-       SPI_OPCODE_TYPE_WRITE_NO_ADDRESS =      1,
-       SPI_OPCODE_TYPE_READ_WITH_ADDRESS =     2,
-       SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS =    3
-};
-
-enum {
        ICH_MAX_CMD_LEN         = 5,
 };
 

@@ -124,8 +117,55 @@ struct spi_trans {
        uint32_t offset;
 };
 
+#define SPI_OPCODE_WRSR                0x01
+#define SPI_OPCODE_PAGE_PROGRAM        0x02
+#define SPI_OPCODE_READ                0x03
+#define SPI_OPCODE_WRDIS       0x04
+#define SPI_OPCODE_RDSR                0x05
 #define SPI_OPCODE_WREN                0x06
 #define SPI_OPCODE_FAST_READ   0x0b
+#define SPI_OPCODE_ERASE_SECT  0x20
+#define SPI_OPCODE_READ_ID     0x9f
+#define SPI_OPCODE_ERASE_BLOCK 0xd8
+
+#define SPI_OPCODE_TYPE_READ_NO_ADDRESS                0
+#define SPI_OPCODE_TYPE_WRITE_NO_ADDRESS       1
+#define SPI_OPCODE_TYPE_READ_WITH_ADDRESS      2
+#define SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS     3
+
+#define SPI_OPMENU_0   SPI_OPCODE_WRSR
+#define SPI_OPTYPE_0   SPI_OPCODE_TYPE_WRITE_NO_ADDRESS
+
+#define SPI_OPMENU_1   SPI_OPCODE_PAGE_PROGRAM
+#define SPI_OPTYPE_1   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
+
+#define SPI_OPMENU_2   SPI_OPCODE_READ
+#define SPI_OPTYPE_2   SPI_OPCODE_TYPE_READ_WITH_ADDRESS
+
+#define SPI_OPMENU_3   SPI_OPCODE_RDSR
+#define SPI_OPTYPE_3   SPI_OPCODE_TYPE_READ_NO_ADDRESS
+
+#define SPI_OPMENU_4   SPI_OPCODE_ERASE_SECT
+#define SPI_OPTYPE_4   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
+
+#define SPI_OPMENU_5   SPI_OPCODE_READ_ID
+#define SPI_OPTYPE_5   SPI_OPCODE_TYPE_READ_NO_ADDRESS
+
+#define SPI_OPMENU_6   SPI_OPCODE_ERASE_BLOCK
+#define SPI_OPTYPE_6   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
+
+#define SPI_OPMENU_7   SPI_OPCODE_FAST_READ
+#define SPI_OPTYPE_7   SPI_OPCODE_TYPE_READ_WITH_ADDRESS

This patch adds a remove function to the Intel ICH SPI driver, that will be called upon U-Boot exit, directly before the OS (Linux) is started. This function takes care of configuring the BIOS registers in the SPI controller (similar to what a "standard" BIOS or coreboot does), so that the Linux MTD device driver is able to correctly read/write to the SPI NOR chip. Without this, the chip is not detected at all.

Signed-off-by Stefan Roese <[email protected]>

Cc Bin Meng <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/spi/ich.c | 18 ++++++++++++++++++
 drivers/spi/ich.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 65 insertions(+), 7 deletions(-)

On my x86 platform I've noticed, that calling dm_uninit() or the new function dm_remove_devices_flags() does not remove the desired device at all. Debugging showed, that the serial uclass returns -EPERM in serial_pre_remove() and this leads to a complete stop of the device removal pretty early, as the serial device is one of the first ones in the DM. Here the dm tree output:

=> dm tree
Class Probed Name
----------------------------------------

 root        [ + ]    root_driver
 rsa_mod_exp [   ]    |-- mod_exp_sw
 serial      [ + ]    |-- serial
 rtc         [   ]    |-- rtc
 timer       [ + ]    |-- tsc-timer
 syscon      [ + ]    |-- pch_pinctrl

...

In this example, device_remove(root) will stop directly after trying to remove the "serial" device.

To solve this problem, this patch removes the return upon error check in the device_remove() call in device_chld_remove(). This leads to device_chld_remove() continuing with the device_remove() call to the following child devices.

Signed-off-by Stefan Roese <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

---
 drivers/core/device-remove.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

drivers/core/device-remove.c +0 -1

index cc0043b990..8b46f3343e 100644
@@ -52,15 +52,11 @@ static int device_chld_unbind(struct udevice *dev)
 static int device_chld_remove(struct udevice *dev, uint flags)
 {
        struct udevice *pos, *n;
-       int ret;

drivers/core/device-remove.c +0 -1

        return 0;
 }
--

2.12.2

Hi Stefan,

Quoted 21 lines · Stefan Roese

On my x86 platform I've noticed, that calling dm_uninit() or the new function dm_remove_devices_flags() does not remove the desired device at all. Debugging showed, that the serial uclass returns -EPERM in serial_pre_remove() and this leads to a complete stop of the device removal pretty early, as the serial device is one of the first ones in the DM. Here the dm tree output:

=> dm tree
Class Probed Name
----------------------------------------

 root        [ + ]    root_driver
 rsa_mod_exp [   ]    |-- mod_exp_sw
 serial      [ + ]    |-- serial
 rtc         [   ]    |-- rtc
 timer       [ + ]    |-- tsc-timer
 syscon      [ + ]    |-- pch_pinctrl

...

In this example, device_remove(root) will stop directly after trying to remove the "serial" device.

Quoted 4 lines

To solve this problem, this patch removes the return upon error check in the device_remove() call in device_chld_remove(). This leads to device_chld_remove() continuing with the device_remove() call to the following child devices.

I think the right solution is to find out why stdio_deregister_dev() fails. It is probably because the device is in use within the stdio variables.

Perhaps you need to remove it first?

Quoted 20 lines

Signed-off-by Stefan Roese <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

---
 drivers/core/device-remove.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

drivers/core/device-remove.c +0 -1

index cc0043b990..8b46f3343e 100644
@@ -52,15 +52,11 @@ static int device_chld_unbind(struct udevice *dev)
 static int device_chld_remove(struct udevice *dev, uint flags)
 {
        struct udevice *pos, *n;
-       int ret;
 
        assert(dev);
 

drivers/core/device-remove.c static int device_chld_unbind(struct udevice *dev) · line 59

Quoted 7 lines

drivers/core/device-remove.c +2 -5

-       list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) {
-               ret = device_remove(pos, flags);
-               if (ret)
-                       return ret;
-       }
+       list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node)
+               device_remove(pos, flags);

I think we should keep the error checking here.

Hi Simon,

sorry for the late replay - just back from vacation....

Quoted 32 lines · Simon Glass

Hi Stefan,

Quoted 19 lines · Stefan Roese

On my x86 platform I've noticed, that calling dm_uninit() or the new function dm_remove_devices_flags() does not remove the desired device at all. Debugging showed, that the serial uclass returns -EPERM in serial_pre_remove() and this leads to a complete stop of the device removal pretty early, as the serial device is one of the first ones in the DM. Here the dm tree output:

=> dm tree
Class Probed Name
----------------------------------------

 root        [ + ]    root_driver
 rsa_mod_exp [   ]    |-- mod_exp_sw
 serial      [ + ]    |-- serial
 rtc         [   ]    |-- rtc
 timer       [ + ]    |-- tsc-timer
 syscon      [ + ]    |-- pch_pinctrl

...

Quoted 7 lines

In this example, device_remove(root) will stop directly after trying to remove the "serial" device.

To solve this problem, this patch removes the return upon error check in the device_remove() call in device_chld_remove(). This leads to device_chld_remove() continuing with the device_remove() call to the following child devices.

Quoted 3 lines

I think the right solution is to find out why stdio_deregister_dev() fails. It is probably because the device is in use within the stdio variables.

This is most likely the case, yes.

Quoted 1 line

Perhaps you need to remove it first?

Not sure if this should / could be done in this general case of removal of all devices (or all devices matching a remove-flag)? Please think of dm_uninit() being called. This functions should have no internal knowledge of usage of devices. One thing I could do, probably best in a separate patch, is to use the force flag of stdio_deregister_dev() in serial_pre_remove() to force the serial device(s) to be removed in this case. What do you think?

Quoted 21 lines

Quoted 21 lines

Signed-off-by Stefan Roese <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

---
 drivers/core/device-remove.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

drivers/core/device-remove.c +0 -1

index cc0043b990..8b46f3343e 100644
@@ -52,15 +52,11 @@ static int device_chld_unbind(struct udevice *dev)
 static int device_chld_remove(struct udevice *dev, uint flags)
 {
        struct udevice *pos, *n;
-       int ret;
 
        assert(dev);
 

Quoted 11 lines

drivers/core/device-remove.c static int device_chld_unbind(struct udevice *dev) · line 59

Quoted 8 lines

drivers/core/device-remove.c +2 -5

-       list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node)
 {
-               ret = device_remove(pos, flags);
-               if (ret)
-                       return ret;
-       }
+       list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node)
+               device_remove(pos, flags);

I think we should keep the error checking here.

The main fix with this patch is, that removing of child devices does not stop once an error is encountered. Even if an error occurs with one child-device of a parent, all other child-devices of this parent should still be removed - or at least tried to.

So what does this error code buy us here? Should I print a log message here in this error case?