dm: core: Add pre-OS remove flag to device_remove()

The new function dm_pre_os_remove() is intented for driver specific last-stage cleanup operations before the OS is started. This patch adds this functionality and hooks it into the common device_remove() function.

To enable usage for custom driver (e.g. ethernet drivers), this patch also sets the pre-OS remove flag for the root driver and simple-bus drivers. Otherwise the recursive call starting from the root device would not reach the drivers in need for this specific remove call.

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

Cc Simon Glass <[email protected]>

---
 drivers/core/device-remove.c | 7 +++++++
 drivers/core/root.c          | 8 ++++++++
 drivers/core/simple-bus.c    | 1 +
 include/dm/root.h            | 9 +++++++++
 4 files changed, 25 insertions(+)

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

index 4725d4751c..0dfb20cdce 100644

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

        ret = uclass_pre_remove_device(dev);
        if (ret)
                return ret;

drivers/core/root.c +8 -0

index 583daa88b0..1468a15db4 100644
@@ -182,6 +182,13 @@ int dm_uninit(void)
        return 0;
 }
 
+int dm_pre_os_remove(void)
+{
+       device_remove(dm_root(), true);
+
+       return 0;
+}
+
 int dm_scan_platdata(bool pre_reloc_only)
 {
        int ret;

@@ -280,6 +287,7 @@ U_BOOT_DRIVER(root_driver) = {
        .name   = "root_driver",
        .id     = UCLASS_ROOT,
        .priv_auto_alloc_size = sizeof(struct root_priv),
+       .flags  = DM_FLAG_PRE_OS_REMOVE,
 };
 
 /* This is the root uclass */

drivers/core/simple-bus.c +1 -0

index a300217d39..4142626ff6 100644
@@ -64,4 +64,5 @@ U_BOOT_DRIVER(simple_bus_drv) = {
        .name   = "generic_simple_bus",
        .id     = UCLASS_SIMPLE_BUS,
        .of_match = generic_simple_bus_ids,
+       .flags  = DM_FLAG_PRE_OS_REMOVE,
 };

include/dm/root.h +9 -1

index 3cf730dcee..8ac7bc3512 100644
@@ -115,4 +115,13 @@ int dm_init(void);
  */
 int dm_uninit(void);
 
+/**
+ * dm_pre_os_remove - Call remove function of all drivers with the pre-OS
+ *                   remove flag set
+ *
+ * All devices with the pre-OS remove flag set, will be removed
+ * @return 0 if OK, -ve on error
+ */
+int dm_pre_os_remove(void);
+
 #endif
--

2.12.0

Quoted 27 lines · Stefan Roese

The new function dm_pre_os_remove() is intented for driver specific last-stage cleanup operations before the OS is started. This patch adds this functionality and hooks it into the common device_remove() function.

To enable usage for custom driver (e.g. ethernet drivers), this patch also sets the pre-OS remove flag for the root driver and simple-bus drivers. Otherwise the recursive call starting from the root device would not reach the drivers in need for this specific remove call.

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

Cc Simon Glass <[email protected]>

---
 drivers/core/device-remove.c | 7 +++++++
 drivers/core/root.c          | 8 ++++++++
 drivers/core/simple-bus.c    | 1 +
 include/dm/root.h            | 9 +++++++++
 4 files changed, 25 insertions(+)

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

index 4725d4751c..0dfb20cdce 100644
@@ -166,6 +166,13 @@ int device_remove(struct udevice *dev, bool pre_os_remove)
        drv = dev->driver;
        assert(drv);
 

drivers/core/device-remove.c int device_remove(struct udevice *dev, bool pre_os_remove) · line 175

Quoted 7 lines

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

+       /*
+        * If pre-OS remove is requested, only continue for drivers with this
+        * flag set
+        */
+       if (pre_os_remove && !(drv->flags & DM_FLAG_PRE_OS_REMOVE))
+               return 0;
+

This doesn't seem good to me. I think it should scan the whole tree, and process devices with the flag set. That way you don't need the root node to have the flag.

If a device has DMA, it will be removed. But its parent may not, in which case the parent will not be removed. However any children of the device will need to be removed, even if they don't have DMA, because we cannot have active children of an inactive device. Does that make sense?

This patch adds a call to dm_pre_os_remove() to announce_and_cleanup() so that drivers that have the flag DM_FLAG_PRE_OS_REMOVE set may do some last-stage cleanup before the OS is started.

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

Cc Simon Glass <[email protected]>

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

arch/arm/lib/bootm.c +0 -0

index 8125cf023f..84f3415c1e 100644

arch/arm/lib/bootm.c +1 -1

+
        cleanup_before_linux();
 }
 
--

2.12.0

Quoted 15 lines · Stefan Roese

This patch adds a call to dm_pre_os_remove() to announce_and_cleanup() so that drivers that have the flag DM_FLAG_PRE_OS_REMOVE set may do some last-stage cleanup before the OS is started.

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

Cc Simon Glass <[email protected]>

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

arch/arm/lib/bootm.c +0 -0

index 8125cf023f..84f3415c1e 100644

arch/arm/lib/bootm.c static void announce_and_cleanup(int fake) · line 100

Quoted 10 lines

arch/arm/lib/bootm.c +7 -0

@@ -91,6 +91,14 @@ static void announce_and_cleanup(int fake)
 
        board_quiesce_devices();
 
+       /*
+        * Call remove function of all devices with the pre-OS remove flag
+        * set (DM_FLAG_PRE_OS_REMOVE). This may be useful for last-stage
+        * operations, like cancelling of DMA operation or releasing device
+        * internal buffers.
+        */
+       dm_pre_os_remove();

In a full DM world we could perhaps have devices which use the DMA uclass, so we can tell which ones need to be removed.

How about dm_remove_dma_devices()?

Quoted 7 lines

arch/arm/lib/bootm.c +1 -1

+
        cleanup_before_linux();
 }
 
--

2.12.0

Also (once we have things figured out) this needs some sort of test in test/.

Hi Simon,

Quoted 35 lines · Simon Glass

Quoted 25 lines · Stefan Roese

This patch adds a call to dm_pre_os_remove() to announce_and_cleanup() so that drivers that have the flag DM_FLAG_PRE_OS_REMOVE set may do some last-stage cleanup before the OS is started.

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

Cc Simon Glass <[email protected]>

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

arch/arm/lib/bootm.c +2 -0

index 8125cf023f..84f3415c1e 100644
@@ -91,6 +91,14 @@ static void announce_and_cleanup(int fake)
 
        board_quiesce_devices();
 
+       /*
+        * Call remove function of all devices with the pre-OS remove

flag

arch/arm/lib/bootm.c +2 -0

+        * set (DM_FLAG_PRE_OS_REMOVE). This may be useful for last-stage
+        * operations, like cancelling of DMA operation or releasing

device

arch/arm/lib/bootm.c static void announce_and_cleanup(int fake) · line 100

Quoted 3 lines

arch/arm/lib/bootm.c +3 -0

+        * internal buffers.
+        */
+       dm_pre_os_remove();

In a full DM world we could perhaps have devices which use the DMA uclass, so we can tell which ones need to be removed.

Quoted 1 line

How about dm_remove_dma_devices()?

I'm not so sure. As we would perhaps need to add other calls for further pre-OS removal reasons (e.g. the stop timer example). How about calling a function with the removal flags as parameter:

        dm_remove_devices_conditional(ONLY_REMOVE_ACTIVE_DMA | ...);

or

        dm_remove_devices_flags(ONLY_REMOVE_ACTIVE_DMA | ...);

What do you think?

Quoted 11 lines

Quoted 8 lines

+

        cleanup_before_linux();

}

--
2.12.0

Also (once we have things figured out) this needs some sort of test in test/.

I already feared that. ;)

Sure, I'll try to add some test, once the path is clear.

Thanks,
Stefan

This patch adds the pre_os_remove boolean flag to device_remove() and changes all calls to this function to provide the default value of "false". This is in preparation for the driver specific pre-OS remove support.

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

Cc Simon Glass <[email protected]>

---
 arch/x86/cpu/queensbay/tnc.c   |  4 ++--
 cmd/cros_ec.c                  |  2 +-
 cmd/sf.c                       |  2 +-
 drivers/block/blk-uclass.c     |  2 +-
 drivers/block/sandbox.c        |  2 +-
 drivers/core/device-remove.c   |  9 +++++----
 drivers/core/device.c          |  2 +-
 drivers/core/root.c            |  2 +-
 drivers/core/uclass.c          |  2 +-
 drivers/mmc/mmc-uclass.c       |  2 +-
 drivers/mtd/spi/sandbox.c      |  2 +-
 drivers/mtd/spi/sf-uclass.c    |  2 +-
 drivers/spi/spi-uclass.c       |  4 ++--
 drivers/usb/emul/sandbox_hub.c |  2 +-

 drivers/usb/host/usb-uclass.c  |  4 ++--
 include/dm/device-internal.h   |  5 +++--
 include/dm/device.h            |  3 +++
 test/dm/bus.c                  |  8 ++++----
 test/dm/core.c                 | 16 ++++++++--------
 test/dm/eth.c                  |  2 +-
 test/dm/spi.c                  |  2 +-
 21 files changed, 42 insertions(+), 37 deletions(-)

Quoted 22 lines · Stefan Roese

This patch adds the pre_os_remove boolean flag to device_remove() and changes all calls to this function to provide the default value of "false". This is in preparation for the driver specific pre-OS remove support.

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

Cc Simon Glass <[email protected]>

---
 arch/x86/cpu/queensbay/tnc.c   |  4 ++--
 cmd/cros_ec.c                  |  2 +-
 cmd/sf.c                       |  2 +-
 drivers/block/blk-uclass.c     |  2 +-
 drivers/block/sandbox.c        |  2 +-
 drivers/core/device-remove.c   |  9 +++++----
 drivers/core/device.c          |  2 +-
 drivers/core/root.c            |  2 +-
 drivers/core/uclass.c          |  2 +-
 drivers/mmc/mmc-uclass.c       |  2 +-
 drivers/mtd/spi/sandbox.c      |  2 +-
 drivers/mtd/spi/sf-uclass.c    |  2 +-
 drivers/spi/spi-uclass.c       |  4 ++--
 drivers/usb/emul/sandbox_hub.c |  2 +-

Quoted 8 lines

 drivers/usb/host/usb-uclass.c  |  4 ++--
 include/dm/device-internal.h   |  5 +++--
 include/dm/device.h            |  3 +++
 test/dm/bus.c                  |  8 ++++----
 test/dm/core.c                 | 16 ++++++++--------
 test/dm/eth.c                  |  2 +-
 test/dm/spi.c                  |  2 +-
 21 files changed, 42 insertions(+), 37 deletions(-)

I think adding a parameter to device_remove() makes sense, but how about using flags instead? The true/false meaning is not clear here and your comment in device.h doesn't really help.

Also I think it is better to name it after the required function rather than state related to the caller. IOW instead of 'pre-os' use something like 'active_dma_only' or as a flag ONLY_REMOVE_ACTIVE_DMA.

Do you think the presence of DMA should be a device flag?

Hi Simon,

Quoted 32 lines · Simon Glass

Quoted 30 lines · Stefan Roese

This patch adds the pre_os_remove boolean flag to device_remove() and changes all calls to this function to provide the default value of "false". This is in preparation for the driver specific pre-OS remove support.

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

Cc Simon Glass <[email protected]>

---
 arch/x86/cpu/queensbay/tnc.c   |  4 ++--
 cmd/cros_ec.c                  |  2 +-
 cmd/sf.c                       |  2 +-
 drivers/block/blk-uclass.c     |  2 +-
 drivers/block/sandbox.c        |  2 +-
 drivers/core/device-remove.c   |  9 +++++----
 drivers/core/device.c          |  2 +-
 drivers/core/root.c            |  2 +-
 drivers/core/uclass.c          |  2 +-
 drivers/mmc/mmc-uclass.c       |  2 +-
 drivers/mtd/spi/sandbox.c      |  2 +-
 drivers/mtd/spi/sf-uclass.c    |  2 +-
 drivers/spi/spi-uclass.c       |  4 ++--
 drivers/usb/emul/sandbox_hub.c |  2 +-
 drivers/usb/host/usb-uclass.c  |  4 ++--
 include/dm/device-internal.h   |  5 +++--
 include/dm/device.h            |  3 +++
 test/dm/bus.c                  |  8 ++++----
 test/dm/core.c                 | 16 ++++++++--------
 test/dm/eth.c                  |  2 +-
 test/dm/spi.c                  |  2 +-
 21 files changed, 42 insertions(+), 37 deletions(-)

Quoted 3 lines

I think adding a parameter to device_remove() makes sense, but how about using flags instead? The true/false meaning is not clear here and your comment in device.h doesn't really help.

So you are suggesting something like this:

int device_remove(struct udevice *dev, uin32_t remove_flags);

?

Quoted 5 lines

Also I think it is better to name it after the required function rather than state related to the caller. IOW instead of 'pre-os' use something like 'active_dma_only' or as a flag ONLY_REMOVE_ACTIVE_DMA.

Do you think the presence of DMA should be a device flag?

The usage of flags instead of this pre-os parameter could make sense to me, as its much more flexible. But I'm not so sure about the flag (re-)naming to something specific like DMA. As there could be multiple reasons other than DMA related for this last-stage driver cleanup / configuration before the OS is started. E.g. if a driver needs to stop an internal timer before the OS is started, it would need to "abuse" this DMA flag to get called at the last pre-OS stage. Or is your thinking that in such cases (e.g. stopping of timer) a new flag should get introduced and added to this "remove_flags" parameter in bootm?

Hi Stefan,

Quoted 35 lines · Stefan Roese

Hi Simon,

Quoted 32 lines · Simon Glass

Quoted 30 lines · Stefan Roese

This patch adds the pre_os_remove boolean flag to device_remove() and changes all calls to this function to provide the default value of "false". This is in preparation for the driver specific pre-OS remove support.

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

Cc Simon Glass <[email protected]>

---
 arch/x86/cpu/queensbay/tnc.c   |  4 ++--
 cmd/cros_ec.c                  |  2 +-
 cmd/sf.c                       |  2 +-
 drivers/block/blk-uclass.c     |  2 +-
 drivers/block/sandbox.c        |  2 +-
 drivers/core/device-remove.c   |  9 +++++----
 drivers/core/device.c          |  2 +-
 drivers/core/root.c            |  2 +-
 drivers/core/uclass.c          |  2 +-
 drivers/mmc/mmc-uclass.c       |  2 +-
 drivers/mtd/spi/sandbox.c      |  2 +-
 drivers/mtd/spi/sf-uclass.c    |  2 +-
 drivers/spi/spi-uclass.c       |  4 ++--
 drivers/usb/emul/sandbox_hub.c |  2 +-
 drivers/usb/host/usb-uclass.c  |  4 ++--
 include/dm/device-internal.h   |  5 +++--
 include/dm/device.h            |  3 +++
 test/dm/bus.c                  |  8 ++++----
 test/dm/core.c                 | 16 ++++++++--------
 test/dm/eth.c                  |  2 +-
 test/dm/spi.c                  |  2 +-
 21 files changed, 42 insertions(+), 37 deletions(-)

Quoted 7 lines

Quoted 3 lines

I think adding a parameter to device_remove() makes sense, but how about using flags instead? The true/false meaning is not clear here and your comment in device.h doesn't really help.

So you are suggesting something like this:

int device_remove(struct udevice *dev, uin32_t remove_flags);

Yes, or really 'uint remove_flags'

Quoted 9 lines

?

Quoted 5 lines

Also I think it is better to name it after the required function rather than state related to the caller. IOW instead of 'pre-os' use something like 'active_dma_only' or as a flag ONLY_REMOVE_ACTIVE_DMA.

Do you think the presence of DMA should be a device flag?

Quoted 10 lines

The usage of flags instead of this pre-os parameter could make sense to me, as its much more flexible. But I'm not so sure about the flag (re-)naming to something specific like DMA. As there could be multiple reasons other than DMA related for this last-stage driver cleanup / configuration before the OS is started. E.g. if a driver needs to stop an internal timer before the OS is started, it would need to "abuse" this DMA flag to get called at the last pre-OS stage. Or is your thinking that in such cases (e.g. stopping of timer) a new flag should get introduced and added to this "remove_flags" parameter in bootm?

Yes, so that it is explicit. Another approach would be:

enum {
DM_REMOVE_ACTIVE_ALL = 1 << 0, /* Remove all devices */
DM_REMOVE_ACTIVE_DMA = 1 << 1, /* Remove only devices with active DMA */ /* Add more use cases here */
};

Hi Simon,

Quoted 68 lines · Simon Glass

Hi Stefan,

Quoted 35 lines · Stefan Roese

Hi Simon,

Quoted 32 lines · Simon Glass

Quoted 30 lines · Stefan Roese

This patch adds the pre_os_remove boolean flag to device_remove() and changes all calls to this function to provide the default value of "false". This is in preparation for the driver specific pre-OS remove support.

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

Cc Simon Glass <[email protected]>

---
 arch/x86/cpu/queensbay/tnc.c   |  4 ++--
 cmd/cros_ec.c                  |  2 +-
 cmd/sf.c                       |  2 +-
 drivers/block/blk-uclass.c     |  2 +-
 drivers/block/sandbox.c        |  2 +-
 drivers/core/device-remove.c   |  9 +++++----
 drivers/core/device.c          |  2 +-
 drivers/core/root.c            |  2 +-
 drivers/core/uclass.c          |  2 +-
 drivers/mmc/mmc-uclass.c       |  2 +-
 drivers/mtd/spi/sandbox.c      |  2 +-
 drivers/mtd/spi/sf-uclass.c    |  2 +-
 drivers/spi/spi-uclass.c       |  4 ++--
 drivers/usb/emul/sandbox_hub.c |  2 +-
 drivers/usb/host/usb-uclass.c  |  4 ++--
 include/dm/device-internal.h   |  5 +++--
 include/dm/device.h            |  3 +++
 test/dm/bus.c                  |  8 ++++----
 test/dm/core.c                 | 16 ++++++++--------
 test/dm/eth.c                  |  2 +-
 test/dm/spi.c                  |  2 +-
 21 files changed, 42 insertions(+), 37 deletions(-)

Quoted 7 lines

Quoted 3 lines

I think adding a parameter to device_remove() makes sense, but how about using flags instead? The true/false meaning is not clear here and your comment in device.h doesn't really help.

So you are suggesting something like this:

int device_remove(struct udevice *dev, uin32_t remove_flags);

Yes, or really 'uint remove_flags'

Quoted 9 lines

?

Quoted 5 lines

Also I think it is better to name it after the required function rather than state related to the caller. IOW instead of 'pre-os' use something like 'active_dma_only' or as a flag ONLY_REMOVE_ACTIVE_DMA.

Do you think the presence of DMA should be a device flag?

Quoted 10 lines

The usage of flags instead of this pre-os parameter could make sense to me, as its much more flexible. But I'm not so sure about the flag (re-)naming to something specific like DMA. As there could be multiple reasons other than DMA related for this last-stage driver cleanup / configuration before the OS is started. E.g. if a driver needs to stop an internal timer before the OS is started, it would need to "abuse" this DMA flag to get called at the last pre-OS stage. Or is your thinking that in such cases (e.g. stopping of timer) a new flag should get introduced and added to this "remove_flags" parameter in bootm?

Quoted 8 lines

Yes, so that it is explicit. Another approach would be:

enum {
DM_REMOVE_ACTIVE_ALL = 1 << 0, /* Remove all devices */
DM_REMOVE_ACTIVE_DMA = 1 << 1, /* Remove only devices with active
DMA */
/* Add more use cases here */
};

Okay, I'll go forward with this suggestion and will generate a new patchset version soon. Stay tuned...