[U-Boot] [RFC PATCH 1/3] dm: core: Add pre-OS remove flag to device_remove()
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 sr@denx.de Cc: Simon Glass sjg@chromium.org --- 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(-)
diff --git a/arch/x86/cpu/queensbay/tnc.c b/arch/x86/cpu/queensbay/tnc.c index f307c622c8..32157f4347 100644 --- a/arch/x86/cpu/queensbay/tnc.c +++ b/arch/x86/cpu/queensbay/tnc.c @@ -76,13 +76,13 @@ static int __maybe_unused disable_igd(void) * * So the only option we have is to manually remove these two devices. */ - ret = device_remove(igd); + ret = device_remove(igd, false); if (ret) return ret; ret = device_unbind(igd); if (ret) return ret; - ret = device_remove(sdvo); + ret = device_remove(sdvo, false); if (ret) return ret; ret = device_unbind(sdvo); diff --git a/cmd/cros_ec.c b/cmd/cros_ec.c index 9d42f870dc..abadfee860 100644 --- a/cmd/cros_ec.c +++ b/cmd/cros_ec.c @@ -110,7 +110,7 @@ static int do_cros_ec(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) /* Remove any existing device */ ret = uclass_find_device(UCLASS_CROS_EC, 0, &udev); if (!ret) - device_remove(udev); + device_remove(udev, false); ret = uclass_get_device(UCLASS_CROS_EC, 0, &udev); if (ret) { printf("Could not init cros_ec device (err %d)\n", ret); diff --git a/cmd/sf.c b/cmd/sf.c index 65b117feee..15e5b435fa 100644 --- a/cmd/sf.c +++ b/cmd/sf.c @@ -124,7 +124,7 @@ static int do_spi_flash_probe(int argc, char * const argv[]) /* Remove the old device, otherwise probe will just be a nop */ ret = spi_find_bus_and_cs(bus, cs, &bus_dev, &new); if (!ret) { - device_remove(new); + device_remove(new, false); } flash = NULL; ret = spi_flash_probe_bus_cs(bus, cs, speed, mode, &new); diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c index 38cb9388da..41fd983976 100644 --- a/drivers/block/blk-uclass.c +++ b/drivers/block/blk-uclass.c @@ -530,7 +530,7 @@ int blk_unbind_all(int if_type) struct blk_desc *desc = dev_get_uclass_platdata(dev);
if (desc->if_type == if_type) { - ret = device_remove(dev); + ret = device_remove(dev, false); if (ret) return ret; ret = device_unbind(dev); diff --git a/drivers/block/sandbox.c b/drivers/block/sandbox.c index 36c2ff3007..1e78d0bbb3 100644 --- a/drivers/block/sandbox.c +++ b/drivers/block/sandbox.c @@ -98,7 +98,7 @@ int host_dev_bind(int devnum, char *filename) /* Remove and unbind the old device, if any */ ret = blk_get_device(IF_TYPE_HOST, devnum, &dev); if (ret == 0) { - ret = device_remove(dev); + ret = device_remove(dev, false); if (ret) return ret; ret = device_unbind(dev); diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c index a7f77b4a21..4725d4751c 100644 --- a/drivers/core/device-remove.c +++ b/drivers/core/device-remove.c @@ -46,9 +46,10 @@ static int device_chld_unbind(struct udevice *dev) /** * device_chld_remove() - Stop all device's children * @dev: The device whose children are to be removed + * @pre_os_remove: Flag, if this functions is called in the pre-OS stage * @return 0 on success, -ve on error */ -static int device_chld_remove(struct udevice *dev) +static int device_chld_remove(struct udevice *dev, bool pre_os_remove) { struct udevice *pos, *n; int ret; @@ -56,7 +57,7 @@ static int device_chld_remove(struct udevice *dev) assert(dev);
list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) { - ret = device_remove(pos); + ret = device_remove(pos, pre_os_remove); if (ret) return ret; } @@ -151,7 +152,7 @@ void device_free(struct udevice *dev) devres_release_probe(dev); }
-int device_remove(struct udevice *dev) +int device_remove(struct udevice *dev, bool pre_os_remove) { const struct driver *drv; int ret; @@ -169,7 +170,7 @@ int device_remove(struct udevice *dev) if (ret) return ret;
- ret = device_chld_remove(dev); + ret = device_chld_remove(dev, pre_os_remove); if (ret) goto err;
diff --git a/drivers/core/device.c b/drivers/core/device.c index 70fcfc23e0..bede787b1b 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -378,7 +378,7 @@ int device_probe(struct udevice *dev)
return 0; fail_uclass: - if (device_remove(dev)) { + if (device_remove(dev, false)) { dm_warn("%s: Device '%s' failed to remove on error path\n", __func__, dev->name); } diff --git a/drivers/core/root.c b/drivers/core/root.c index 175fd3fb25..583daa88b0 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -176,7 +176,7 @@ int dm_init(void)
int dm_uninit(void) { - device_remove(dm_root()); + device_remove(dm_root(), false); device_unbind(dm_root());
return 0; diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index 7de370644d..61144174ae 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -116,7 +116,7 @@ int uclass_destroy(struct uclass *uc) while (!list_empty(&uc->dev_head)) { dev = list_first_entry(&uc->dev_head, struct udevice, uclass_node); - ret = device_remove(dev); + ret = device_remove(dev, false); if (ret) return ret; ret = device_unbind(dev); diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index 5bb446bcc2..9f75fff5a5 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -232,7 +232,7 @@ int mmc_unbind(struct udevice *dev)
device_find_first_child(dev, &bdev); if (bdev) { - device_remove(bdev); + device_remove(bdev, false); device_unbind(bdev); }
diff --git a/drivers/mtd/spi/sandbox.c b/drivers/mtd/spi/sandbox.c index 36a50fe3a1..8cf6bdc4df 100644 --- a/drivers/mtd/spi/sandbox.c +++ b/drivers/mtd/spi/sandbox.c @@ -595,7 +595,7 @@ void sandbox_sf_unbind_emul(struct sandbox_state *state, int busnum, int cs) struct udevice *dev;
dev = state->spi[busnum][cs].emul; - device_remove(dev); + device_remove(dev, false); device_unbind(dev); state->spi[busnum][cs].emul = NULL; } diff --git a/drivers/mtd/spi/sf-uclass.c b/drivers/mtd/spi/sf-uclass.c index 19de964e61..55eba5ed79 100644 --- a/drivers/mtd/spi/sf-uclass.c +++ b/drivers/mtd/spi/sf-uclass.c @@ -46,7 +46,7 @@ struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs,
void spi_flash_free(struct spi_flash *flash) { - device_remove(flash->spi->dev); + device_remove(flash->spi->dev, false); }
int spi_flash_probe_bus_cs(unsigned int busnum, unsigned int cs, diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index ac17da0777..900590f93d 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -343,7 +343,7 @@ err: debug("%s: Error path, created=%d, device '%s'\n", __func__, created, dev->name); if (created) { - device_remove(dev); + device_remove(dev, false); device_unbind(dev); }
@@ -384,7 +384,7 @@ struct spi_slave *spi_setup_slave(unsigned int busnum, unsigned int cs,
void spi_free_slave(struct spi_slave *slave) { - device_remove(slave->dev); + device_remove(slave->dev, false); slave->dev = NULL; }
diff --git a/drivers/usb/emul/sandbox_hub.c b/drivers/usb/emul/sandbox_hub.c index c3a8e73389..def5591904 100644 --- a/drivers/usb/emul/sandbox_hub.c +++ b/drivers/usb/emul/sandbox_hub.c @@ -156,7 +156,7 @@ static int clrset_post_state(struct udevice *hub, int port, int clear, int set) } else if (clear & USB_PORT_STAT_POWER) { debug("%s: %s: power off, removed, ret=%d\n", __func__, dev->name, ret); - ret = device_remove(dev); + ret = device_remove(dev, false); clear |= USB_PORT_STAT_CONNECTION; } } diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index 5cf1e9a36c..763cb24e6c 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -154,7 +154,7 @@ int usb_stop(void) uc_priv = uc->priv;
uclass_foreach_dev(bus, uc) { - ret = device_remove(bus); + ret = device_remove(bus, false); if (ret && !err) err = ret; } @@ -358,7 +358,7 @@ int usb_setup_ehci_gadget(struct ehci_ctrl **ctlrp) ret = uclass_find_device_by_seq(UCLASS_USB, 0, true, &dev); if (ret) return ret; - ret = device_remove(dev); + ret = device_remove(dev, false); if (ret) return ret;
diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h index 0bf8707493..0518304b34 100644 --- a/include/dm/device-internal.h +++ b/include/dm/device-internal.h @@ -96,12 +96,13 @@ int device_probe(struct udevice *dev); * children are deactivated first. * * @dev: Pointer to device to remove + * @pre_os_remove: Flag, if this functions is called in the pre-OS stage * @return 0 if OK, -ve on error (an error here is normally a very bad thing) */ #if CONFIG_IS_ENABLED(DM_DEVICE_REMOVE) -int device_remove(struct udevice *dev); +int device_remove(struct udevice *dev, bool pre_os_remove); #else -static inline int device_remove(struct udevice *dev) { return 0; } +static inline int device_remove(struct udevice *dev, bool pre_os_remove) { return 0; } #endif
/** diff --git a/include/dm/device.h b/include/dm/device.h index 4e95fb7773..037eaa4423 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -46,6 +46,9 @@ struct driver_info;
#define DM_FLAG_OF_PLATDATA (1 << 8)
+/* Call driver remove function before the OS is called (U-Boot exit) */ +#define DM_FLAG_PRE_OS_REMOVE (1 << 9) + /** * struct udevice - An instance of a driver * diff --git a/test/dm/bus.c b/test/dm/bus.c index d94dcf7a60..03acc7ce47 100644 --- a/test/dm/bus.c +++ b/test/dm/bus.c @@ -222,7 +222,7 @@ static int test_bus_parent_data(struct unit_test_state *uts) /* Check that it starts at 0 and goes away when device is removed */ parent_data->sum += 5; ut_asserteq(5, parent_data->sum); - device_remove(dev); + device_remove(dev, false); ut_asserteq_ptr(NULL, dev_get_parent_priv(dev));
/* Check that we can do this twice */ @@ -323,7 +323,7 @@ static int dm_test_bus_parent_ops(struct unit_test_state *uts) continue; parent_data = dev_get_parent_priv(dev); ut_asserteq(FLAG_CHILD_PROBED, parent_data->flag); - ut_assertok(device_remove(dev)); + ut_assertok(device_remove(dev, false)); ut_asserteq_ptr(NULL, dev_get_parent_priv(dev)); ut_asserteq_ptr(dms->removed, dev); } @@ -360,7 +360,7 @@ static int test_bus_parent_platdata(struct unit_test_state *uts) plat->count++; ut_asserteq(1, plat->count); device_probe(dev); - device_remove(dev); + device_remove(dev, false);
ut_asserteq_ptr(plat, dev_get_parent_platdata(dev)); ut_asserteq(1, plat->count); @@ -370,7 +370,7 @@ static int test_bus_parent_platdata(struct unit_test_state *uts) ut_asserteq(3, child_count);
/* Removing the bus should also have no effect (it is still bound) */ - device_remove(bus); + device_remove(bus, false); for (device_find_first_child(bus, &dev), child_count = 0; dev; device_find_next_child(&dev)) { diff --git a/test/dm/core.c b/test/dm/core.c index 70bf4d0605..2ccab09b5b 100644 --- a/test/dm/core.c +++ b/test/dm/core.c @@ -319,7 +319,7 @@ static int dm_test_lifecycle(struct unit_test_state *uts)
/* Now remove device 3 */ ut_asserteq(0, dm_testdrv_op_count[DM_TEST_OP_PRE_REMOVE]); - ut_assertok(device_remove(dev)); + ut_assertok(device_remove(dev, false)); ut_asserteq(1, dm_testdrv_op_count[DM_TEST_OP_PRE_REMOVE]);
ut_asserteq(0, dm_testdrv_op_count[DM_TEST_OP_UNBIND]); @@ -352,7 +352,7 @@ static int dm_test_ordering(struct unit_test_state *uts) ut_assert(dev_last);
/* Now remove device 3 */ - ut_assertok(device_remove(dev)); + ut_assertok(device_remove(dev, false)); ut_assertok(device_unbind(dev));
/* The device numbering should have shifted down one */ @@ -371,9 +371,9 @@ static int dm_test_ordering(struct unit_test_state *uts) ut_assert(pingret == 102);
/* Remove 3 and 4 */ - ut_assertok(device_remove(dev_penultimate)); + ut_assertok(device_remove(dev_penultimate, false)); ut_assertok(device_unbind(dev_penultimate)); - ut_assertok(device_remove(dev_last)); + ut_assertok(device_remove(dev_last, false)); ut_assertok(device_unbind(dev_last));
/* Our device should now be in position 3 */ @@ -381,7 +381,7 @@ static int dm_test_ordering(struct unit_test_state *uts) ut_assert(dev == test_dev);
/* Now remove device 3 */ - ut_assertok(device_remove(dev)); + ut_assertok(device_remove(dev, false)); ut_assertok(device_unbind(dev));
return 0; @@ -457,7 +457,7 @@ static int dm_test_remove(struct unit_test_state *uts) ut_assert(dev); ut_assertf(dev->flags & DM_FLAG_ACTIVATED, "Driver %d/%s not activated", i, dev->name); - ut_assertok(device_remove(dev)); + ut_assertok(device_remove(dev, false)); ut_assertf(!(dev->flags & DM_FLAG_ACTIVATED), "Driver %d/%s should have deactivated", i, dev->name); @@ -611,14 +611,14 @@ static int dm_test_children(struct unit_test_state *uts) ut_asserteq(total, dm_testdrv_op_count[DM_TEST_OP_PROBE]);
/* Remove a top-level child and check that the children are removed */ - ut_assertok(device_remove(top[2])); + ut_assertok(device_remove(top[2], false)); ut_asserteq(NODE_COUNT + 1, dm_testdrv_op_count[DM_TEST_OP_REMOVE]); dm_testdrv_op_count[DM_TEST_OP_REMOVE] = 0;
/* Try one with grandchildren */ ut_assertok(uclass_get_device(UCLASS_TEST, 5, &dev)); ut_asserteq_ptr(dev, top[5]); - ut_assertok(device_remove(dev)); + ut_assertok(device_remove(dev, false)); ut_asserteq(1 + NODE_COUNT * (1 + NODE_COUNT), dm_testdrv_op_count[DM_TEST_OP_REMOVE]);
diff --git a/test/dm/eth.c b/test/dm/eth.c index 6288ae24ab..674bdb817d 100644 --- a/test/dm/eth.c +++ b/test/dm/eth.c @@ -116,7 +116,7 @@ static int dm_test_eth_act(struct unit_test_state *uts) for (i = 0; i < DM_TEST_ETH_NUM; i++) { ut_assertok(uclass_find_device_by_name(UCLASS_ETH, ethname[i], &dev[i])); - ut_assertok(device_remove(dev[i])); + ut_assertok(device_remove(dev[i], false));
/* Invalidate MAC address */ strcpy(ethaddr[i], getenv(addrname[i])); diff --git a/test/dm/spi.c b/test/dm/spi.c index f52cb733c7..078a29eaad 100644 --- a/test/dm/spi.c +++ b/test/dm/spi.c @@ -36,7 +36,7 @@ static int dm_test_spi_find(struct unit_test_state *uts) ut_asserteq(0, uclass_get_device_by_seq(UCLASS_SPI, busnum, &bus)); ut_assertok(spi_cs_info(bus, cs, &info)); of_offset = dev_of_offset(info.dev); - device_remove(info.dev); + device_remove(info.dev, false); device_unbind(info.dev);
/*
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 sr@denx.de Cc: Simon Glass sjg@chromium.org --- 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(+)
diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c index 4725d4751c..0dfb20cdce 100644 --- a/drivers/core/device-remove.c +++ b/drivers/core/device-remove.c @@ -166,6 +166,13 @@ int device_remove(struct udevice *dev, bool pre_os_remove) drv = dev->driver; assert(drv);
+ /* + * 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; + ret = uclass_pre_remove_device(dev); if (ret) return ret; diff --git a/drivers/core/root.c b/drivers/core/root.c index 583daa88b0..1468a15db4 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -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 */ diff --git a/drivers/core/simple-bus.c b/drivers/core/simple-bus.c index a300217d39..4142626ff6 100644 --- a/drivers/core/simple-bus.c +++ b/drivers/core/simple-bus.c @@ -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, }; diff --git a/include/dm/root.h b/include/dm/root.h index 3cf730dcee..8ac7bc3512 100644 --- a/include/dm/root.h +++ b/include/dm/root.h @@ -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
Hi Stefan,
On 1 March 2017 at 03:23, Stefan Roese sr@denx.de wrote:
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 sr@denx.de Cc: Simon Glass sjg@chromium.org
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(+)
diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c index 4725d4751c..0dfb20cdce 100644 --- a/drivers/core/device-remove.c +++ b/drivers/core/device-remove.c @@ -166,6 +166,13 @@ int device_remove(struct udevice *dev, bool pre_os_remove) drv = dev->driver; assert(drv);
/** 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?
ret = uclass_pre_remove_device(dev); if (ret) return ret;diff --git a/drivers/core/root.c b/drivers/core/root.c index 583daa88b0..1468a15db4 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -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 */ diff --git a/drivers/core/simple-bus.c b/drivers/core/simple-bus.c index a300217d39..4142626ff6 100644 --- a/drivers/core/simple-bus.c +++ b/drivers/core/simple-bus.c @@ -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,}; diff --git a/include/dm/root.h b/include/dm/root.h index 3cf730dcee..8ac7bc3512 100644 --- a/include/dm/root.h +++ b/include/dm/root.h @@ -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
Regards, Simon
Hi Simon,
On 03.03.2017 05:53, Simon Glass wrote:
On 1 March 2017 at 03:23, Stefan Roese sr@denx.de wrote:
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 sr@denx.de Cc: Simon Glass sjg@chromium.org
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(+)
diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c index 4725d4751c..0dfb20cdce 100644 --- a/drivers/core/device-remove.c +++ b/drivers/core/device-remove.c @@ -166,6 +166,13 @@ int device_remove(struct udevice *dev, bool pre_os_remove) drv = dev->driver; assert(drv);
/** 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.
Yes, thats better, I agree.
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?
Yes. I'll change this in the next patchset version accordingly.
Thanks, Stefan
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 sr@denx.de Cc: Simon Glass sjg@chromium.org --- arch/arm/lib/bootm.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index 8125cf023f..84f3415c1e 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -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(); + cleanup_before_linux(); }
Hi Stefan,
On 1 March 2017 at 03:23, Stefan Roese sr@denx.de wrote:
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 sr@denx.de Cc: Simon Glass sjg@chromium.org
arch/arm/lib/bootm.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index 8125cf023f..84f3415c1e 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -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()?
cleanup_before_linux();}
-- 2.12.0
Also (once we have things figured out) this needs some sort of test in test/.
Regards, Simon
Hi Simon,
On 03.03.2017 05:53, Simon Glass wrote:
On 1 March 2017 at 03:23, Stefan Roese sr@denx.de wrote:
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 sr@denx.de Cc: Simon Glass sjg@chromium.org
arch/arm/lib/bootm.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index 8125cf023f..84f3415c1e 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -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()?
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?
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
Hi Stefan,
On 2 March 2017 at 23:35, Stefan Roese sr@denx.de wrote:
Hi Simon,
On 03.03.2017 05:53, Simon Glass wrote:
On 1 March 2017 at 03:23, Stefan Roese sr@denx.de wrote:
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 sr@denx.de Cc: Simon Glass sjg@chromium.org
arch/arm/lib/bootm.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index 8125cf023f..84f3415c1e 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -91,6 +91,14 @@ static void announce_and_cleanup(int fake)
board_quiesce_devices();
/** Call remove function of all devices with the pre-OS removeflag
* set (DM_FLAG_PRE_OS_REMOVE). This may be useful for last-stage* operations, like cancelling of DMA operation or releasingdevice
* 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()?
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?
Seems reasonable - see my previous email for a flag naming idea (i.e. having an explicit ALL flag to avoid all the flags having to use negative logic).
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
Regards, Simon
Hi Stefan,
On 1 March 2017 at 03:23, Stefan Roese sr@denx.de wrote:
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 sr@denx.de Cc: Simon Glass sjg@chromium.org
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(-)
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?
Regards, Simon
Hi Simon,
On 03.03.2017 05:53, Simon Glass wrote:
On 1 March 2017 at 03:23, Stefan Roese sr@denx.de wrote:
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 sr@denx.de Cc: Simon Glass sjg@chromium.org
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(-)
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);
?
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?
Thanks, Stefan
Hi Stefan,
On 2 March 2017 at 23:24, Stefan Roese sr@denx.de wrote:
Hi Simon,
On 03.03.2017 05:53, Simon Glass wrote:
On 1 March 2017 at 03:23, Stefan Roese sr@denx.de wrote:
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 sr@denx.de Cc: Simon Glass sjg@chromium.org
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(-)
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'
?
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?
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 */ };
Then, DM_REMOVE_ACTIVE_ALL means everything will be removed, and if that flag is not set, the other flags can be used.
I am assuming that there actually will be other cases - your email suggests that could be true.
Regards, Simon
Hi Simon,
On 08.03.2017 22:01, Simon Glass wrote:
Hi Stefan,
On 2 March 2017 at 23:24, Stefan Roese sr@denx.de wrote:
Hi Simon,
On 03.03.2017 05:53, Simon Glass wrote:
On 1 March 2017 at 03:23, Stefan Roese sr@denx.de wrote:
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 sr@denx.de Cc: Simon Glass sjg@chromium.org
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(-)
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'
?
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?
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...
Thanks, Stefan
Hi Simon,
On 10.03.2017 12:31, Stefan Roese wrote:
Hi Simon,
On 08.03.2017 22:01, Simon Glass wrote:
Hi Stefan,
On 2 March 2017 at 23:24, Stefan Roese sr@denx.de wrote:
Hi Simon,
On 03.03.2017 05:53, Simon Glass wrote:
On 1 March 2017 at 03:23, Stefan Roese sr@denx.de wrote:
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 sr@denx.de Cc: Simon Glass sjg@chromium.org
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(-)
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'
?
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?
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...
Thinking about it, we need a bit for the "normal" remove case as well. How about this naming:
enum { DM_REMOVE_NORMAL = 1 << 0, /* Remove all devices */ DM_REMOVE_ACTIVE_ALL = 1 << 1, /* Remove devices with any active flag set */ DM_REMOVE_ACTIVE_DMA = 1 << 2, /* Remove only devices with active DMA */ /* Add more use cases here */ };
Or do you have another suggestion in mind for this "normal" device remove case?
Thanks, Stefan
participants (2)
-
Simon Glass -
Stefan Roese