[PATCH V3] dm: core: Add late driver remove option
Add another flag to the DM core which could be assigned to drivers and which makes those drivers call their remove callbacks last, just before booting OS and after all the other drivers finished with their remove callbacks. This is necessary for things like clock drivers, where the other drivers might depend on the clock driver in their remove callbacks. Prime example is the mmc subsystem, which can reconfigure a card from HS mode to slower modes in the remove callback and for that it needs to reconfigure the controller clock.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Stefan Roese sr@denx.de Cc: Tom Rini trini@konsulko.com --- V2: Fix DM tests V3: - Address feedback from V2, drop extra {} - Add test --- arch/arm/lib/bootm.c | 1 + board/Marvell/octeontx2/board.c | 4 +- drivers/core/device-remove.c | 13 +++-- drivers/core/root.c | 2 + drivers/core/uclass.c | 26 +++++++-- include/dm/device.h | 10 ++++ include/dm/uclass-internal.h | 3 +- test/dm/core.c | 99 ++++++++++++++++++++++++++++++--- test/dm/test-driver.c | 11 ++++ test/dm/test-main.c | 30 +++++----- 10 files changed, 165 insertions(+), 34 deletions(-)
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index 1206e306db..f9091a3d41 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -120,6 +120,7 @@ static void announce_and_cleanup(int fake) * of DMA operation or releasing device internal buffers. */ dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL); + dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_LATE);
cleanup_before_linux(); } diff --git a/board/Marvell/octeontx2/board.c b/board/Marvell/octeontx2/board.c index 50e903d9aa..d1e2372a37 100644 --- a/board/Marvell/octeontx2/board.c +++ b/board/Marvell/octeontx2/board.c @@ -65,7 +65,7 @@ void board_quiesce_devices(void) /* Removes all RVU PF devices */ ret = uclass_get(UCLASS_ETH, &uc_dev); if (uc_dev) - ret = uclass_destroy(uc_dev); + ret = uclass_destroy(uc_dev, DM_REMOVE_NORMAL); if (ret) printf("couldn't remove rvu pf devices\n");
@@ -77,7 +77,7 @@ void board_quiesce_devices(void) /* Removes all CGX and RVU AF devices */ ret = uclass_get(UCLASS_MISC, &uc_dev); if (uc_dev) - ret = uclass_destroy(uc_dev); + ret = uclass_destroy(uc_dev, DM_REMOVE_NORMAL); if (ret) printf("couldn't remove misc (cgx/rvu_af) devices\n");
diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c index efdb0f2905..a387d5666c 100644 --- a/drivers/core/device-remove.c +++ b/drivers/core/device-remove.c @@ -152,7 +152,7 @@ void device_free(struct udevice *dev) static bool flags_remove(uint flags, uint drv_flags) { if ((flags & DM_REMOVE_NORMAL) || - (flags & (drv_flags & (DM_FLAG_ACTIVE_DMA | DM_FLAG_OS_PREPARE)))) + (flags && (drv_flags & (DM_FLAG_ACTIVE_DMA | DM_FLAG_OS_PREPARE)))) return true;
return false; @@ -172,14 +172,19 @@ int device_remove(struct udevice *dev, uint flags) drv = dev->driver; assert(drv);
- ret = uclass_pre_remove_device(dev); - if (ret) - return ret; + if (!(flags & DM_REMOVE_LATE)) { + ret = uclass_pre_remove_device(dev); + if (ret) + return ret; + }
ret = device_chld_remove(dev, NULL, flags); if (ret) goto err;
+ if (!(flags & DM_REMOVE_LATE) && (drv->flags & DM_FLAG_REMOVE_LATE)) + return 0; + /* * Remove the device if called with the "normal" remove flag set, * or if the remove flag matches any of the drivers remove flags diff --git a/drivers/core/root.c b/drivers/core/root.c index 5f10d7a39c..5eb33f809e 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -163,6 +163,7 @@ int dm_init(bool of_live) int dm_uninit(void) { device_remove(dm_root(), DM_REMOVE_NORMAL); + device_remove(dm_root(), DM_REMOVE_LATE); device_unbind(dm_root()); gd->dm_root = NULL;
@@ -391,6 +392,7 @@ struct acpi_ops root_acpi_ops = { U_BOOT_DRIVER(root_driver) = { .name = "root_driver", .id = UCLASS_ROOT, + .flags = DM_FLAG_REMOVE_LATE, ACPI_OPS_PTR(&root_acpi_ops) };
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index c3f1b73cd6..a20f7b41ce 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -104,10 +104,28 @@ fail_mem: return ret; }
-int uclass_destroy(struct uclass *uc) +int uclass_find_device_by_drv_flag(struct uclass *uc, const unsigned int flag, + const bool neg, struct udevice **devp) +{ + struct udevice *dev; + + *devp = NULL; + uclass_foreach_dev(dev, uc) { + if ((neg && (dev->driver->flags & flag)) || + (!neg && !(dev->driver->flags & flag))) { + *devp = dev; + return 0; + } + } + + return -ENODEV; +} + +int uclass_destroy(struct uclass *uc, unsigned int flag) { struct uclass_driver *uc_drv; struct udevice *dev; + bool late = flag & DM_REMOVE_LATE; int ret;
/* @@ -116,10 +134,8 @@ int uclass_destroy(struct uclass *uc) * unbound (by the recursion in the call to device_unbind() below). * We can loop until the list is empty. */ - while (!list_empty(&uc->dev_head)) { - dev = list_first_entry(&uc->dev_head, struct udevice, - uclass_node); - ret = device_remove(dev, DM_REMOVE_NORMAL | DM_REMOVE_NO_PD); + while (!uclass_find_device_by_drv_flag(uc, DM_FLAG_REMOVE_LATE, late, &dev)) { + ret = device_remove(dev, flag | DM_REMOVE_NO_PD); if (ret) return log_msg_ret("remove", ret); ret = device_unbind(dev); diff --git a/include/dm/device.h b/include/dm/device.h index 5bef484247..0770b20f66 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -73,6 +73,13 @@ struct driver_info; */ #define DM_FLAG_REMOVE_WITH_PD_ON (1 << 13)
+/* + * Device is removed late, after all regular devices were removed. This + * is useful e.g. for clock, which need to be active during the device + * remove phase. + */ +#define DM_FLAG_REMOVE_LATE (1 << 14) + /* * 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 @@ -95,6 +102,9 @@ enum {
/* Don't power down any attached power domains */ DM_REMOVE_NO_PD = 1 << 1, + + /* Remove device after all regular devices are removed */ + DM_REMOVE_LATE = 1 << 2, };
/** diff --git a/include/dm/uclass-internal.h b/include/dm/uclass-internal.h index 6e3f15c2b0..b5926b0f5c 100644 --- a/include/dm/uclass-internal.h +++ b/include/dm/uclass-internal.h @@ -247,8 +247,9 @@ struct uclass *uclass_find(enum uclass_id key); * Destroy a uclass and all its devices * * @uc: uclass to destroy + * @flag: driver flags (DM_REMOVE_NORMAL or DM_REMOVE_LATE) * @return 0 on success, -ve on error */ -int uclass_destroy(struct uclass *uc); +int uclass_destroy(struct uclass *uc, unsigned int flag);
#endif diff --git a/test/dm/core.c b/test/dm/core.c index 6f380a574c..629fa5ef87 100644 --- a/test/dm/core.c +++ b/test/dm/core.c @@ -72,6 +72,10 @@ static struct driver_info driver_info_act_dma = { .name = "test_act_dma_drv", };
+static struct driver_info driver_info_act_late_clk = { + .name = "test_act_late_clk_drv", +}; + void dm_leak_check_start(struct unit_test_state *uts) { uts->start = mallinfo(); @@ -82,16 +86,19 @@ void dm_leak_check_start(struct unit_test_state *uts) int dm_leak_check_end(struct unit_test_state *uts) { struct mallinfo end; - int id, diff; + int i, id, diff;
/* Don't delete the root class, since we started with that */ - for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) { - struct uclass *uc; - - uc = uclass_find(id); - if (!uc) - continue; - ut_assertok(uclass_destroy(uc)); + for (i = 0; i < 2; i++) { + for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) { + struct uclass *uc; + + uc = uclass_find(id); + if (!uc) + continue; + ut_assertok(uclass_destroy(uc, + i ? DM_REMOVE_LATE : DM_REMOVE_NORMAL)); + } }
end = mallinfo(); @@ -514,7 +521,7 @@ static int dm_test_uclass(struct unit_test_state *uts) ut_asserteq(0, dm_testdrv_op_count[DM_TEST_OP_DESTROY]); ut_assert(uc->priv);
- ut_assertok(uclass_destroy(uc)); + ut_assertok(uclass_destroy(uc, DM_REMOVE_LATE)); ut_asserteq(1, dm_testdrv_op_count[DM_TEST_OP_INIT]); ut_asserteq(1, dm_testdrv_op_count[DM_TEST_OP_DESTROY]);
@@ -883,6 +890,80 @@ static int dm_test_remove_active_dma(struct unit_test_state *uts) } DM_TEST(dm_test_remove_active_dma, 0);
+/* + * Test that removal of devices, either via the "normal" device_remove() + * API or via the device driver selective flag works as expected + */ +static int dm_test_remove_active_late(struct unit_test_state *uts) +{ + struct dm_test_state *dms = uts->priv; + struct udevice *devn, *devl; + + /* Skip the behaviour in test_post_probe() */ + dms->skip_post_probe = 1; + + ut_assertok(device_bind_by_name(dms->root, false, + &driver_info_act_late_clk, + &devl)); + ut_assert(devl); + + ut_assertok(device_bind_by_name(dms->root, false, + &driver_info_manual, + &devn)); + ut_assert(devn); + + /* Part 1, DM_REMOVE_ACTIVE_ALL */ + + /* Probe the devices */ + ut_assertok(device_probe(devl)); + ut_assertok(device_probe(devn)); + + /* Test if devices are active right now */ + ut_asserteq(true, device_active(devl)); + ut_asserteq(true, device_active(devn)); + + /* Remove normal devices via selective remove flag */ + dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NORMAL); + + /* Test if normal devices are inactive right now */ + ut_asserteq(true, device_active(devl)); + ut_asserteq(false, device_active(devn)); + + /* Remove late devices via selective remove flag */ + dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_LATE); + + /* Test if all devices are inactive right now */ + ut_asserteq(false, device_active(devl)); + ut_asserteq(false, device_active(devn)); + + /* Part 2, device_remove() */ + + /* Probe the devices again */ + ut_assertok(device_probe(devl)); + ut_assertok(device_probe(devn)); + + /* Test if devices are active right now */ + ut_asserteq(true, device_active(devl)); + ut_asserteq(true, device_active(devn)); + + /* Remove the devices via "normal" remove API */ + ut_assertok(device_remove(devn, DM_REMOVE_NORMAL)); + ut_assertok(device_remove(devl, DM_REMOVE_NORMAL)); + + /* Test if normal devices are inactive right now */ + ut_asserteq(true, device_active(devl)); + ut_asserteq(false, device_active(devn)); + + ut_assertok(device_remove(devl, DM_REMOVE_LATE)); + + /* Test if devices are inactive right now */ + ut_asserteq(false, device_active(devl)); + ut_asserteq(false, device_active(devn)); + + return 0; +} +DM_TEST(dm_test_remove_active_late, 0); + static int dm_test_uclass_before_ready(struct unit_test_state *uts) { struct uclass *uc; diff --git a/test/dm/test-driver.c b/test/dm/test-driver.c index 08bdf01194..837a511d7f 100644 --- a/test/dm/test-driver.c +++ b/test/dm/test-driver.c @@ -169,3 +169,14 @@ U_BOOT_DRIVER(test_act_dma_drv) = { .unbind = test_manual_unbind, .flags = DM_FLAG_ACTIVE_DMA, }; + +U_BOOT_DRIVER(test_act_late_clk_drv) = { + .name = "test_act_late_clk_drv", + .id = UCLASS_TEST, + .ops = &test_manual_ops, + .bind = test_manual_bind, + .probe = test_manual_probe, + .remove = test_manual_remove, + .unbind = test_manual_unbind, + .flags = DM_FLAG_OS_PREPARE | DM_FLAG_REMOVE_LATE, +}; diff --git a/test/dm/test-main.c b/test/dm/test-main.c index fd24635006..6407e96a39 100644 --- a/test/dm/test-main.c +++ b/test/dm/test-main.c @@ -59,19 +59,23 @@ static int do_autoprobe(struct unit_test_state *uts)
static int dm_test_destroy(struct unit_test_state *uts) { - int id; - - for (id = 0; id < UCLASS_COUNT; id++) { - struct uclass *uc; - - /* - * If the uclass doesn't exist we don't want to create it. So - * check that here before we call uclass_find_device(). - */ - uc = uclass_find(id); - if (!uc) - continue; - ut_assertok(uclass_destroy(uc)); + int i, id; + + for (i = 0; i < 2; i++) { + for (id = 0; id < UCLASS_COUNT; id++) { + struct uclass *uc; + + /* + * If the uclass doesn't exist we don't want to + * create it. So check that here before we call + * uclass_find_device(). + */ + uc = uclass_find(id); + if (!uc) + continue; + ut_assertok(uclass_destroy(uc, + i ? DM_REMOVE_LATE : DM_REMOVE_NORMAL)); + } }
return 0;
Hi Marek,
On Sun, 8 Nov 2020 at 07:09, Marek Vasut marek.vasut@gmail.com wrote:
Add another flag to the DM core which could be assigned to drivers and which makes those drivers call their remove callbacks last, just before booting OS and after all the other drivers finished with their remove callbacks. This is necessary for things like clock drivers, where the other drivers might depend on the clock driver in their remove callbacks. Prime example is the mmc subsystem, which can reconfigure a card from HS mode to slower modes in the remove callback and for that it needs to reconfigure the controller clock.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Stefan Roese sr@denx.de Cc: Tom Rini trini@konsulko.com
V2: Fix DM tests V3: - Address feedback from V2, drop extra {} - Add test
arch/arm/lib/bootm.c | 1 + board/Marvell/octeontx2/board.c | 4 +- drivers/core/device-remove.c | 13 +++-- drivers/core/root.c | 2 + drivers/core/uclass.c | 26 +++++++-- include/dm/device.h | 10 ++++ include/dm/uclass-internal.h | 3 +- test/dm/core.c | 99 ++++++++++++++++++++++++++++++--- test/dm/test-driver.c | 11 ++++ test/dm/test-main.c | 30 +++++----- 10 files changed, 165 insertions(+), 34 deletions(-)
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index 1206e306db..f9091a3d41 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -120,6 +120,7 @@ static void announce_and_cleanup(int fake) * of DMA operation or releasing device internal buffers. */ dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_LATE);
Please see my previous comments about the naming and semantics. I'm repeating them here:
Firstly I think we should use a different name. 'Late' doesn't really tell me anything.
If I understand it correctly, this is supposed to be after OS_PREPARE but before booting the OS?
I think we need to separate the flag names as they are too similar.
I think DM_FLAG_BASIC and DM_REMOVE_NON_BASIC would be better (or some term that explains that this is a device used by many others)
If BASIC is too terrible, perhaps CORE, or VITAL, or PRIME or CRITICAL or something like that?
That way we are describing the property of the device rather than what we want to do with it.
Note also the semantics of what is going on here. The idea of the existing OS_PREPARE and ACTIVE_DMA flags is that the default for device_remove() is to remove everything, but if you provide a flag then it just removes those things. Your flag is the opposite to that, which is why you are changing so much code.
So I think we could change this to DM_REMOVE_NON_BASIC and make that a separate step before we do a final remove with flags of 0.
cleanup_before_linux();} diff --git a/board/Marvell/octeontx2/board.c b/board/Marvell/octeontx2/board.c index 50e903d9aa..d1e2372a37 100644 --- a/board/Marvell/octeontx2/board.c +++ b/board/Marvell/octeontx2/board.c
Should be in a separate patch
@@ -65,7 +65,7 @@ void board_quiesce_devices(void) /* Removes all RVU PF devices */ ret = uclass_get(UCLASS_ETH, &uc_dev); if (uc_dev)
ret = uclass_destroy(uc_dev);
ret = uclass_destroy(uc_dev, DM_REMOVE_NORMAL); if (ret) printf("couldn't remove rvu pf devices\n");@@ -77,7 +77,7 @@ void board_quiesce_devices(void) /* Removes all CGX and RVU AF devices */ ret = uclass_get(UCLASS_MISC, &uc_dev); if (uc_dev)
ret = uclass_destroy(uc_dev);
ret = uclass_destroy(uc_dev, DM_REMOVE_NORMAL); if (ret) printf("couldn't remove misc (cgx/rvu_af) devices\n");diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c index efdb0f2905..a387d5666c 100644 --- a/drivers/core/device-remove.c +++ b/drivers/core/device-remove.c @@ -152,7 +152,7 @@ void device_free(struct udevice *dev) static bool flags_remove(uint flags, uint drv_flags) { if ((flags & DM_REMOVE_NORMAL) ||
(flags & (drv_flags & (DM_FLAG_ACTIVE_DMA | DM_FLAG_OS_PREPARE))))
(flags && (drv_flags & (DM_FLAG_ACTIVE_DMA | DM_FLAG_OS_PREPARE))))
This looks like a bug fix. In any case it should be in its own patch.
return true; return false;@@ -172,14 +172,19 @@ int device_remove(struct udevice *dev, uint flags) drv = dev->driver; assert(drv);
ret = uclass_pre_remove_device(dev);if (ret)return ret;
if (!(flags & DM_REMOVE_LATE)) {ret = uclass_pre_remove_device(dev);if (ret)return ret;} ret = device_chld_remove(dev, NULL, flags); if (ret) goto err;if (!(flags & DM_REMOVE_LATE) && (drv->flags & DM_FLAG_REMOVE_LATE))return 0;/* * Remove the device if called with the "normal" remove flag set, * or if the remove flag matches any of the drivers remove flagsdiff --git a/drivers/core/root.c b/drivers/core/root.c index 5f10d7a39c..5eb33f809e 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -163,6 +163,7 @@ int dm_init(bool of_live) int dm_uninit(void) { device_remove(dm_root(), DM_REMOVE_NORMAL);
device_remove(dm_root(), DM_REMOVE_LATE); device_unbind(dm_root()); gd->dm_root = NULL;@@ -391,6 +392,7 @@ struct acpi_ops root_acpi_ops = { U_BOOT_DRIVER(root_driver) = { .name = "root_driver", .id = UCLASS_ROOT,
.flags = DM_FLAG_REMOVE_LATE, ACPI_OPS_PTR(&root_acpi_ops)};
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index c3f1b73cd6..a20f7b41ce 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -104,10 +104,28 @@ fail_mem: return ret; }
-int uclass_destroy(struct uclass *uc) +int uclass_find_device_by_drv_flag(struct uclass *uc, const unsigned int flag,
const bool neg, struct udevice **devp)+{
struct udevice *dev;*devp = NULL;uclass_foreach_dev(dev, uc) {if ((neg && (dev->driver->flags & flag)) ||(!neg && !(dev->driver->flags & flag))) {*devp = dev;return 0;}}return -ENODEV;+}
+int uclass_destroy(struct uclass *uc, unsigned int flag) { struct uclass_driver *uc_drv; struct udevice *dev;
bool late = flag & DM_REMOVE_LATE; int ret; /*@@ -116,10 +134,8 @@ int uclass_destroy(struct uclass *uc) * unbound (by the recursion in the call to device_unbind() below). * We can loop until the list is empty. */
while (!list_empty(&uc->dev_head)) {dev = list_first_entry(&uc->dev_head, struct udevice,uclass_node);ret = device_remove(dev, DM_REMOVE_NORMAL | DM_REMOVE_NO_PD);
while (!uclass_find_device_by_drv_flag(uc, DM_FLAG_REMOVE_LATE, late, &dev)) {ret = device_remove(dev, flag | DM_REMOVE_NO_PD);
I think this logic will get a lot simpler when you change the semantics as above.
if (ret) return log_msg_ret("remove", ret); ret = device_unbind(dev);diff --git a/include/dm/device.h b/include/dm/device.h index 5bef484247..0770b20f66 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -73,6 +73,13 @@ struct driver_info; */ #define DM_FLAG_REMOVE_WITH_PD_ON (1 << 13)
+/*
- Device is removed late, after all regular devices were removed. This
- is useful e.g. for clock, which need to be active during the device
- remove phase.
- */
+#define DM_FLAG_REMOVE_LATE (1 << 14)
/*
- 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
@@ -95,6 +102,9 @@ enum {
/* Don't power down any attached power domains */ DM_REMOVE_NO_PD = 1 << 1,
/* Remove device after all regular devices are removed */DM_REMOVE_LATE = 1 << 2,};
/** diff --git a/include/dm/uclass-internal.h b/include/dm/uclass-internal.h index 6e3f15c2b0..b5926b0f5c 100644 --- a/include/dm/uclass-internal.h +++ b/include/dm/uclass-internal.h @@ -247,8 +247,9 @@ struct uclass *uclass_find(enum uclass_id key);
- Destroy a uclass and all its devices
- @uc: uclass to destroy
*/
- @flag: driver flags (DM_REMOVE_NORMAL or DM_REMOVE_LATE)
- @return 0 on success, -ve on error
-int uclass_destroy(struct uclass *uc); +int uclass_destroy(struct uclass *uc, unsigned int flag);
#endif diff --git a/test/dm/core.c b/test/dm/core.c index 6f380a574c..629fa5ef87 100644 --- a/test/dm/core.c +++ b/test/dm/core.c @@ -72,6 +72,10 @@ static struct driver_info driver_info_act_dma = { .name = "test_act_dma_drv", };
+static struct driver_info driver_info_act_late_clk = {
.name = "test_act_late_clk_drv",+};
void dm_leak_check_start(struct unit_test_state *uts) { uts->start = mallinfo(); @@ -82,16 +86,19 @@ void dm_leak_check_start(struct unit_test_state *uts) int dm_leak_check_end(struct unit_test_state *uts) { struct mallinfo end;
int id, diff;
int i, id, diff; /* Don't delete the root class, since we started with that */
for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) {struct uclass *uc;uc = uclass_find(id);if (!uc)continue;ut_assertok(uclass_destroy(uc));
for (i = 0; i < 2; i++) {for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) {struct uclass *uc;uc = uclass_find(id);if (!uc)continue;ut_assertok(uclass_destroy(uc,i ? DM_REMOVE_LATE : DM_REMOVE_NORMAL));}
Why do we need to destroy the classes in two steps? I understand removing devices, but destroying the uclasses seems like it could stay as it is?
} end = mallinfo();@@ -514,7 +521,7 @@ static int dm_test_uclass(struct unit_test_state *uts) ut_asserteq(0, dm_testdrv_op_count[DM_TEST_OP_DESTROY]); ut_assert(uc->priv);
ut_assertok(uclass_destroy(uc));
ut_assertok(uclass_destroy(uc, DM_REMOVE_LATE)); ut_asserteq(1, dm_testdrv_op_count[DM_TEST_OP_INIT]); ut_asserteq(1, dm_testdrv_op_count[DM_TEST_OP_DESTROY]);@@ -883,6 +890,80 @@ static int dm_test_remove_active_dma(struct unit_test_state *uts) } DM_TEST(dm_test_remove_active_dma, 0);
+/*
- Test that removal of devices, either via the "normal" device_remove()
- API or via the device driver selective flag works as expected
- */
+static int dm_test_remove_active_late(struct unit_test_state *uts) +{
struct dm_test_state *dms = uts->priv;struct udevice *devn, *devl;/* Skip the behaviour in test_post_probe() */dms->skip_post_probe = 1;ut_assertok(device_bind_by_name(dms->root, false,&driver_info_act_late_clk,&devl));ut_assert(devl);ut_assertok(device_bind_by_name(dms->root, false,&driver_info_manual,&devn));ut_assert(devn);/* Part 1, DM_REMOVE_ACTIVE_ALL *//* Probe the devices */ut_assertok(device_probe(devl));ut_assertok(device_probe(devn));/* Test if devices are active right now */ut_asserteq(true, device_active(devl));ut_asserteq(true, device_active(devn));/* Remove normal devices via selective remove flag */dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NORMAL);/* Test if normal devices are inactive right now */ut_asserteq(true, device_active(devl));ut_asserteq(false, device_active(devn));/* Remove late devices via selective remove flag */dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_LATE);/* Test if all devices are inactive right now */ut_asserteq(false, device_active(devl));ut_asserteq(false, device_active(devn));/* Part 2, device_remove() *//* Probe the devices again */ut_assertok(device_probe(devl));ut_assertok(device_probe(devn));/* Test if devices are active right now */ut_asserteq(true, device_active(devl));ut_asserteq(true, device_active(devn));/* Remove the devices via "normal" remove API */ut_assertok(device_remove(devn, DM_REMOVE_NORMAL));ut_assertok(device_remove(devl, DM_REMOVE_NORMAL));/* Test if normal devices are inactive right now */ut_asserteq(true, device_active(devl));ut_asserteq(false, device_active(devn));ut_assertok(device_remove(devl, DM_REMOVE_LATE));/* Test if devices are inactive right now */ut_asserteq(false, device_active(devl));ut_asserteq(false, device_active(devn));return 0;+} +DM_TEST(dm_test_remove_active_late, 0);
This test looks good to me.
static int dm_test_uclass_before_ready(struct unit_test_state *uts) { struct uclass *uc; diff --git a/test/dm/test-driver.c b/test/dm/test-driver.c index 08bdf01194..837a511d7f 100644 --- a/test/dm/test-driver.c +++ b/test/dm/test-driver.c @@ -169,3 +169,14 @@ U_BOOT_DRIVER(test_act_dma_drv) = { .unbind = test_manual_unbind, .flags = DM_FLAG_ACTIVE_DMA, };
+U_BOOT_DRIVER(test_act_late_clk_drv) = {
.name = "test_act_late_clk_drv",.id = UCLASS_TEST,.ops = &test_manual_ops,.bind = test_manual_bind,.probe = test_manual_probe,.remove = test_manual_remove,.unbind = test_manual_unbind,.flags = DM_FLAG_OS_PREPARE | DM_FLAG_REMOVE_LATE,+}; diff --git a/test/dm/test-main.c b/test/dm/test-main.c index fd24635006..6407e96a39 100644 --- a/test/dm/test-main.c +++ b/test/dm/test-main.c @@ -59,19 +59,23 @@ static int do_autoprobe(struct unit_test_state *uts)
static int dm_test_destroy(struct unit_test_state *uts) {
int id;for (id = 0; id < UCLASS_COUNT; id++) {struct uclass *uc;/** If the uclass doesn't exist we don't want to create it. So* check that here before we call uclass_find_device().*/uc = uclass_find(id);if (!uc)continue;ut_assertok(uclass_destroy(uc));
int i, id;for (i = 0; i < 2; i++) {for (id = 0; id < UCLASS_COUNT; id++) {struct uclass *uc;/** If the uclass doesn't exist we don't want to* create it. So check that here before we call* uclass_find_device().*/uc = uclass_find(id);if (!uc)continue;ut_assertok(uclass_destroy(uc,i ? DM_REMOVE_LATE : DM_REMOVE_NORMAL));}
See comments about uclass destroy above.
} return 0;-- 2.28.0
Regadrs, Simon
On 11/9/20 1:21 AM, Simon Glass wrote:
Hi Marek,
[...]
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index 1206e306db..f9091a3d41 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -120,6 +120,7 @@ static void announce_and_cleanup(int fake) * of DMA operation or releasing device internal buffers. */ dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_LATE);Please see my previous comments about the naming and semantics. I'm repeating them here:
Firstly I think we should use a different name. 'Late' doesn't really tell me anything.
If I understand it correctly, this is supposed to be after OS_PREPARE but before booting the OS?
No. The drivers which are marked as remove-late should be removed late, after all the normal drivers were removed. The example in the commit message explains where this is needed -- first remove mmc driver to set eMMC out of HS mode and change the bus clock and after that remove clock driver ; the clock driver is still required during the removal of the mmc driver.
I think we need to separate the flag names as they are too similar.
I think DM_FLAG_BASIC and DM_REMOVE_NON_BASIC would be better (or some term that explains that this is a device used by many others)
If BASIC is too terrible, perhaps CORE, or VITAL, or PRIME or CRITICAL or something like that?
This makes no sense to me.
That way we are describing the property of the device rather than what we want to do with it.
The device is not critical or vital, it just needs to be torn down late.
Note also the semantics of what is going on here. The idea of the existing OS_PREPARE and ACTIVE_DMA flags is that the default for device_remove() is to remove everything, but if you provide a flag then it just removes those things. Your flag is the opposite to that, which is why you are changing so much code.
I obviously cannot remove everything, see the example above.
So I think we could change this to DM_REMOVE_NON_BASIC and make that a separate step before we do a final remove with flags of 0.
[...]
@@ -82,16 +86,19 @@ void dm_leak_check_start(struct unit_test_state *uts) int dm_leak_check_end(struct unit_test_state *uts) { struct mallinfo end;
int id, diff;
int i, id, diff; /* Don't delete the root class, since we started with that */
for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) {struct uclass *uc;uc = uclass_find(id);if (!uc)continue;ut_assertok(uclass_destroy(uc));
for (i = 0; i < 2; i++) {for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) {struct uclass *uc;uc = uclass_find(id);if (!uc)continue;ut_assertok(uclass_destroy(uc,i ? DM_REMOVE_LATE : DM_REMOVE_NORMAL));}Why do we need to destroy the classes in two steps? I understand removing devices, but destroying the uclasses seems like it could stay as it is?
Because if you destroy clock uclass before mmc uclass, then you cannot remove the mmc drivers and reconfigure the bus clock anymore in the remove callback. So that needs to be done in two steps.
[...]
Hi Marek,
On Sun, 15 Nov 2020 at 06:19, Marek Vasut marex@denx.de wrote:
On 11/9/20 1:21 AM, Simon Glass wrote:
Hi Marek,
[...]
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index 1206e306db..f9091a3d41 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -120,6 +120,7 @@ static void announce_and_cleanup(int fake) * of DMA operation or releasing device internal buffers. */ dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_LATE);Please see my previous comments about the naming and semantics. I'm repeating them here:
Firstly I think we should use a different name. 'Late' doesn't really tell me anything.
If I understand it correctly, this is supposed to be after OS_PREPARE but before booting the OS?
No. The drivers which are marked as remove-late should be removed late, after all the normal drivers were removed. The example in the commit message explains where this is needed -- first remove mmc driver to set eMMC out of HS mode and change the bus clock and after that remove clock driver ; the clock driver is still required during the removal of the mmc driver.
I think we need to separate the flag names as they are too similar.
I think DM_FLAG_BASIC and DM_REMOVE_NON_BASIC would be better (or some term that explains that this is a device used by many others)
If BASIC is too terrible, perhaps CORE, or VITAL, or PRIME or CRITICAL or something like that?
This makes no sense to me.
I don't want the word 'late'. Then we'll end up with 'later' and 'fairly late', etc. and it will get out of hand. We need a word that describes what is special about the devices, not when to do stuff with them.
That way we are describing the property of the device rather than what we want to do with it.
The device is not critical or vital, it just needs to be torn down late.
What is it about the device that requires it to be torn down 'late'?
Note also the semantics of what is going on here. The idea of the existing OS_PREPARE and ACTIVE_DMA flags is that the default for device_remove() is to remove everything, but if you provide a flag then it just removes those things. Your flag is the opposite to that, which is why you are changing so much code.
I obviously cannot remove everything, see the example above.
Do you understand what I am saying about inverting the flag?
So I think we could change this to DM_REMOVE_NON_BASIC and make that a separate step before we do a final remove with flags of 0.
[...]
@@ -82,16 +86,19 @@ void dm_leak_check_start(struct unit_test_state *uts) int dm_leak_check_end(struct unit_test_state *uts) { struct mallinfo end;
int id, diff;
int i, id, diff; /* Don't delete the root class, since we started with that */
for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) {struct uclass *uc;uc = uclass_find(id);if (!uc)continue;ut_assertok(uclass_destroy(uc));
for (i = 0; i < 2; i++) {for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) {struct uclass *uc;uc = uclass_find(id);if (!uc)continue;ut_assertok(uclass_destroy(uc,i ? DM_REMOVE_LATE : DM_REMOVE_NORMAL));}Why do we need to destroy the classes in two steps? I understand removing devices, but destroying the uclasses seems like it could stay as it is?
Because if you destroy clock uclass before mmc uclass, then you cannot remove the mmc drivers and reconfigure the bus clock anymore in the remove callback. So that needs to be done in two steps.
Yes I understand that. All of my comments are about the implementation rather than the problem you are solving. See the existing DM_REMOVE_ flags for examples.
If you like, I could have a try at this. Perhaps it is not as straightforward as I imagine.
Regards, Simon
On Wed, Nov 18, 2020 at 07:37:27AM -0700, Simon Glass wrote:
Hi Marek,
On Sun, 15 Nov 2020 at 06:19, Marek Vasut marex@denx.de wrote:
On 11/9/20 1:21 AM, Simon Glass wrote:
Hi Marek,
[...]
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index 1206e306db..f9091a3d41 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -120,6 +120,7 @@ static void announce_and_cleanup(int fake) * of DMA operation or releasing device internal buffers. */ dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_LATE);Please see my previous comments about the naming and semantics. I'm repeating them here:
Firstly I think we should use a different name. 'Late' doesn't really tell me anything.
If I understand it correctly, this is supposed to be after OS_PREPARE but before booting the OS?
No. The drivers which are marked as remove-late should be removed late, after all the normal drivers were removed. The example in the commit message explains where this is needed -- first remove mmc driver to set eMMC out of HS mode and change the bus clock and after that remove clock driver ; the clock driver is still required during the removal of the mmc driver.
I think we need to separate the flag names as they are too similar.
I think DM_FLAG_BASIC and DM_REMOVE_NON_BASIC would be better (or some term that explains that this is a device used by many others)
If BASIC is too terrible, perhaps CORE, or VITAL, or PRIME or CRITICAL or something like that?
This makes no sense to me.
I don't want the word 'late'. Then we'll end up with 'later' and 'fairly late', etc. and it will get out of hand. We need a word that describes what is special about the devices, not when to do stuff with them.
That way we are describing the property of the device rather than what we want to do with it.
The device is not critical or vital, it just needs to be torn down late.
What is it about the device that requires it to be torn down 'late'?
I think perhaps the problem isn't that it needs to be "late", it's that it has perhaps not obviously described children. Which gets back to what you just said as well about "later" and "fairly late". It's an ordering problem.
Hi Tom,
On Wed, 18 Nov 2020 at 08:53, Tom Rini trini@konsulko.com wrote:
On Wed, Nov 18, 2020 at 07:37:27AM -0700, Simon Glass wrote:
Hi Marek,
On Sun, 15 Nov 2020 at 06:19, Marek Vasut marex@denx.de wrote:
On 11/9/20 1:21 AM, Simon Glass wrote:
Hi Marek,
[...]
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index 1206e306db..f9091a3d41 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -120,6 +120,7 @@ static void announce_and_cleanup(int fake) * of DMA operation or releasing device internal buffers. */ dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_LATE);Please see my previous comments about the naming and semantics. I'm repeating them here:
Firstly I think we should use a different name. 'Late' doesn't really tell me anything.
If I understand it correctly, this is supposed to be after OS_PREPARE but before booting the OS?
No. The drivers which are marked as remove-late should be removed late, after all the normal drivers were removed. The example in the commit message explains where this is needed -- first remove mmc driver to set eMMC out of HS mode and change the bus clock and after that remove clock driver ; the clock driver is still required during the removal of the mmc driver.
I think we need to separate the flag names as they are too similar.
I think DM_FLAG_BASIC and DM_REMOVE_NON_BASIC would be better (or some term that explains that this is a device used by many others)
If BASIC is too terrible, perhaps CORE, or VITAL, or PRIME or CRITICAL or something like that?
This makes no sense to me.
I don't want the word 'late'. Then we'll end up with 'later' and 'fairly late', etc. and it will get out of hand. We need a word that describes what is special about the devices, not when to do stuff with them.
That way we are describing the property of the device rather than what we want to do with it.
The device is not critical or vital, it just needs to be torn down late.
What is it about the device that requires it to be torn down 'late'?
I think perhaps the problem isn't that it needs to be "late", it's that it has perhaps not obviously described children. Which gets back to what you just said as well about "later" and "fairly late". It's an ordering problem.
Yes it is.
We currently don't record devices that depend on others. It would be possible to add a refcount to DM to cope with this and implement it for clocks. I wonder if that might be better than what we have here?
Regards, Simon
On 11/22/20 12:07 AM, Simon Glass wrote: [...]
That way we are describing the property of the device rather than what we want to do with it.
The device is not critical or vital, it just needs to be torn down late.
What is it about the device that requires it to be torn down 'late'?
I think perhaps the problem isn't that it needs to be "late", it's that it has perhaps not obviously described children. Which gets back to what you just said as well about "later" and "fairly late". It's an ordering problem.
Yes it is.
We currently don't record devices that depend on others. It would be possible to add a refcount to DM to cope with this and implement it for clocks. I wonder if that might be better than what we have here?
This is still a bootloader, not a general-purpose OS, so I would argue we should not complicate this more than is necessary. The DM already adds a lot of bloat to U-Boot, no need to make that worse unless there is a real good reason for that. Also, in V1 of this patch, Simon did suggest that a simple approach is OK if I recall correctly.
On 11/21/20 9:13 PM, Marek Vasut wrote:
On 11/22/20 12:07 AM, Simon Glass wrote: [...]
That way we are describing the property of the device rather than what we want to do with it.
The device is not critical or vital, it just needs to be torn down late.
What is it about the device that requires it to be torn down 'late'?
I think perhaps the problem isn't that it needs to be "late", it's that it has perhaps not obviously described children. Which gets back to what you just said as well about "later" and "fairly late". It's an ordering problem.
Yes it is.
We currently don't record devices that depend on others. It would be possible to add a refcount to DM to cope with this and implement it for clocks. I wonder if that might be better than what we have here?
This is still a bootloader, not a general-purpose OS, so I would argue we should not complicate this more than is necessary. The DM already addsa lot of bloat to U-Boot, no need to make that worse unless there is a real good reason for that. Also, in V1 of this patch, Simon did suggest that a simple approach is OK if I recall correctly.
FWIW the clock subsystem already does refcounting of clocks (though it is for struct clk and not for the devices themselves). However, I think it would be difficult to use those counts to determine when the clock driver is no longer needed. For example, some devices (such as CPUs) may use clocks, but should never stop those clocks.
A quick-and-dirty method could be to create a linked list of all probed devices, and then remove devices in reverse order. So an example call sequence could be
... 1. mmc's probe() 2. clk_get_*() 3. clk's probe() 4. clk's finishes; clk added to probed list 5. mmc's probe finishes; mmc added to probed list ... 7. mmc's remove() called 8. clk's remove() called
Note that this would be a change to dm_uninit, not device_remove; we would still need the recursive removal logic for when devices are removed via other means. Unfortunately, this method would require yet another list_head in udevice, so perhaps it should just be an option?
--Sean
On Sun, Nov 22, 2020 at 03:13:15AM +0100, Marek Vasut wrote:
On 11/22/20 12:07 AM, Simon Glass wrote: [...]
That way we are describing the property of the device rather than what we want to do with it.
The device is not critical or vital, it just needs to be torn down late.
What is it about the device that requires it to be torn down 'late'?
I think perhaps the problem isn't that it needs to be "late", it's that it has perhaps not obviously described children. Which gets back to what you just said as well about "later" and "fairly late". It's an ordering problem.
Yes it is.
We currently don't record devices that depend on others. It would be possible to add a refcount to DM to cope with this and implement it for clocks. I wonder if that might be better than what we have here?
This is still a bootloader, not a general-purpose OS, so I would argue we should not complicate this more than is necessary. The DM already adds a lot of bloat to U-Boot, no need to make that worse unless there is a real good reason for that. Also, in V1 of this patch, Simon did suggest that a simple approach is OK if I recall correctly.
Perhaps now that it's clear to everyone what "late" means in this context, we can just solve it with a flag + documentation that ...whatever the name is... means that it's for ensuring that we have unwound the other parts of the system which require this to be enabled first.
On 11/18/20 3:37 PM, Simon Glass wrote:
Hi,
[...]
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index 1206e306db..f9091a3d41 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -120,6 +120,7 @@ static void announce_and_cleanup(int fake) * of DMA operation or releasing device internal buffers. */ dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_LATE);Please see my previous comments about the naming and semantics. I'm repeating them here:
Firstly I think we should use a different name. 'Late' doesn't really tell me anything.
If I understand it correctly, this is supposed to be after OS_PREPARE but before booting the OS?
No. The drivers which are marked as remove-late should be removed late, after all the normal drivers were removed. The example in the commit message explains where this is needed -- first remove mmc driver to set eMMC out of HS mode and change the bus clock and after that remove clock driver ; the clock driver is still required during the removal of the mmc driver.
I think we need to separate the flag names as they are too similar.
I think DM_FLAG_BASIC and DM_REMOVE_NON_BASIC would be better (or some term that explains that this is a device used by many others)
If BASIC is too terrible, perhaps CORE, or VITAL, or PRIME or CRITICAL or something like that?
This makes no sense to me.
I don't want the word 'late'. Then we'll end up with 'later' and 'fairly late', etc. and it will get out of hand. We need a word that describes what is special about the devices, not when to do stuff with them.
If there is a need for "later" , then we will need some more complex code. I suggest we cross that bridge when we come to it.
That way we are describing the property of the device rather than what we want to do with it.
The device is not critical or vital, it just needs to be torn down late.
What is it about the device that requires it to be torn down 'late'?
For example clock driver needs to be turn down after mmc controller, since the mmc controller might need to reconfigure the card in .remove callback, for which it still needs to clock to be active. That's the usecase I have on real hardware.
Note also the semantics of what is going on here. The idea of the existing OS_PREPARE and ACTIVE_DMA flags is that the default for device_remove() is to remove everything, but if you provide a flag then it just removes those things. Your flag is the opposite to that, which is why you are changing so much code.
I obviously cannot remove everything, see the example above.
Do you understand what I am saying about inverting the flag?
No, please elaborate.
So I think we could change this to DM_REMOVE_NON_BASIC and make that a separate step before we do a final remove with flags of 0.
[...]
@@ -82,16 +86,19 @@ void dm_leak_check_start(struct unit_test_state *uts) int dm_leak_check_end(struct unit_test_state *uts) { struct mallinfo end;
int id, diff;
int i, id, diff; /* Don't delete the root class, since we started with that */
for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) {struct uclass *uc;uc = uclass_find(id);if (!uc)continue;ut_assertok(uclass_destroy(uc));
for (i = 0; i < 2; i++) {for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) {struct uclass *uc;uc = uclass_find(id);if (!uc)continue;ut_assertok(uclass_destroy(uc,i ? DM_REMOVE_LATE : DM_REMOVE_NORMAL));}Why do we need to destroy the classes in two steps? I understand removing devices, but destroying the uclasses seems like it could stay as it is?
Because if you destroy clock uclass before mmc uclass, then you cannot remove the mmc drivers and reconfigure the bus clock anymore in the remove callback. So that needs to be done in two steps.
Yes I understand that. All of my comments are about the implementation rather than the problem you are solving. See the existing DM_REMOVE_ flags for examples.
If you like, I could have a try at this. Perhaps it is not as straightforward as I imagine.
I think it would be a good idea if you looked at the use case for this first.
Hi Marek,
On Sat, 5 Dec 2020 at 08:19, Marek Vasut marex@denx.de wrote:
On 11/18/20 3:37 PM, Simon Glass wrote:
Hi,
[...]
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index 1206e306db..f9091a3d41 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -120,6 +120,7 @@ static void announce_and_cleanup(int fake) * of DMA operation or releasing device internal buffers. */ dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_LATE);Please see my previous comments about the naming and semantics. I'm repeating them here:
Firstly I think we should use a different name. 'Late' doesn't really tell me anything.
If I understand it correctly, this is supposed to be after OS_PREPARE but before booting the OS?
No. The drivers which are marked as remove-late should be removed late, after all the normal drivers were removed. The example in the commit message explains where this is needed -- first remove mmc driver to set eMMC out of HS mode and change the bus clock and after that remove clock driver ; the clock driver is still required during the removal of the mmc driver.
I think we need to separate the flag names as they are too similar.
I think DM_FLAG_BASIC and DM_REMOVE_NON_BASIC would be better (or some term that explains that this is a device used by many others)
If BASIC is too terrible, perhaps CORE, or VITAL, or PRIME or CRITICAL or something like that?
This makes no sense to me.
I don't want the word 'late'. Then we'll end up with 'later' and 'fairly late', etc. and it will get out of hand. We need a word that describes what is special about the devices, not when to do stuff with them.
If there is a need for "later" , then we will need some more complex code. I suggest we cross that bridge when we come to it.
I'm OK with that.
That way we are describing the property of the device rather than what we want to do with it.
The device is not critical or vital, it just needs to be torn down late.
What is it about the device that requires it to be torn down 'late'?
For example clock driver needs to be turn down after mmc controller, since the mmc controller might need to reconfigure the card in .remove callback, for which it still needs to clock to be active. That's the usecase I have on real hardware.
Yes I understand that.
Note also the semantics of what is going on here. The idea of the existing OS_PREPARE and ACTIVE_DMA flags is that the default for device_remove() is to remove everything, but if you provide a flag then it just removes those things. Your flag is the opposite to that, which is why you are changing so much code.
I obviously cannot remove everything, see the example above.
Do you understand what I am saying about inverting the flag?
No, please elaborate.
The normal situation should be to remove everything. Removing only non-late things (which I want to call 'basic', or something like that) should be an option, like we have DM_REMOVE_OS_PREPARE.
So I think we could change this to DM_REMOVE_NON_BASIC and make that a separate step before we do a final remove with flags of 0.
[...]
@@ -82,16 +86,19 @@ void dm_leak_check_start(struct unit_test_state *uts) int dm_leak_check_end(struct unit_test_state *uts) { struct mallinfo end;
int id, diff;
int i, id, diff; /* Don't delete the root class, since we started with that */
for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) {struct uclass *uc;uc = uclass_find(id);if (!uc)continue;ut_assertok(uclass_destroy(uc));
for (i = 0; i < 2; i++) {for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) {struct uclass *uc;uc = uclass_find(id);if (!uc)continue;ut_assertok(uclass_destroy(uc,i ? DM_REMOVE_LATE : DM_REMOVE_NORMAL));}Why do we need to destroy the classes in two steps? I understand removing devices, but destroying the uclasses seems like it could stay as it is?
Because if you destroy clock uclass before mmc uclass, then you cannot remove the mmc drivers and reconfigure the bus clock anymore in the remove callback. So that needs to be done in two steps.
Yes I understand that. All of my comments are about the implementation rather than the problem you are solving. See the existing DM_REMOVE_ flags for examples.
If you like, I could have a try at this. Perhaps it is not as straightforward as I imagine.
I think it would be a good idea if you looked at the use case for this first.
I'm not sure what it is about this use case that I don't understand. It seems fairly straightforward to me. We have decided to leave reference counting for another day and all I am really commenting on is the implementation.
Regards, Simon
On 12/10/20 6:44 PM, Simon Glass wrote:
[...]
Note also the semantics of what is going on here. The idea of the existing OS_PREPARE and ACTIVE_DMA flags is that the default for device_remove() is to remove everything, but if you provide a flag then it just removes those things. Your flag is the opposite to that, which is why you are changing so much code.
I obviously cannot remove everything, see the example above.
Do you understand what I am saying about inverting the flag?
No, please elaborate.
The normal situation should be to remove everything. Removing only non-late things (which I want to call 'basic', or something like that) should be an option, like we have DM_REMOVE_OS_PREPARE.
We cannot remove everything at once, because then various real hardware cannot work properly. So there needs to be some way to remove e.g. the clock drivers later. Marking a couple of drivers as "remove-late" is much less intrusive than marking most drivers as "do-not-remove-early".
How do you propose this "inverted" remove flag would look like ?
[...]
Hi Marek,
On Thu, 7 Jan 2021 at 05:42, Marek Vasut marex@denx.de wrote:
On 12/10/20 6:44 PM, Simon Glass wrote:
[...]
Note also the semantics of what is going on here. The idea of the existing OS_PREPARE and ACTIVE_DMA flags is that the default for device_remove() is to remove everything, but if you provide a flag then it just removes those things. Your flag is the opposite to that, which is why you are changing so much code.
I obviously cannot remove everything, see the example above.
Do you understand what I am saying about inverting the flag?
No, please elaborate.
The normal situation should be to remove everything. Removing only non-late things (which I want to call 'basic', or something like that) should be an option, like we have DM_REMOVE_OS_PREPARE.
We cannot remove everything at once, because then various real hardware cannot work properly. So there needs to be some way to remove e.g. the clock drivers later. Marking a couple of drivers as "remove-late" is much less intrusive than marking most drivers as "do-not-remove-early".
I'm not suggesting marking drivers as 'do not remove early'. I am referring to the parameter of the device_remove() function. Mostly what I am asking for is better naming, and (I hope) a simpler implementation.
How do you propose this "inverted" remove flag would look like ?
Similar to the existing flags. See for example DM_FLAG_ACTIVE_DMA.
I have this on my list to look at more closely and should do so in the next week.
Regards, Simon
participants (5)
-
Marek Vasut -
Marek Vasut -
Sean Anderson -
Simon Glass -
Tom Rini