* [PATCH V2] dm: core: Add late driver remove option
@ 2020-08-02 15:06 Marek Vasut
2020-08-07 16:23 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Marek Vasut @ 2020-08-02 15:06 UTC (permalink / raw)
To: u-boot
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: Tom Rini <trini@konsulko.com>
---
V2: Fix DM tests
---
arch/arm/lib/bootm.c | 1 +
drivers/core/device-remove.c | 11 ++++++++---
drivers/core/root.c | 2 ++
drivers/core/uclass.c | 32 +++++++++++++++++++++++++-------
include/dm/device.h | 4 ++++
include/dm/uclass-internal.h | 3 ++-
test/dm/core.c | 21 ++++++++++++---------
test/dm/test-main.c | 30 +++++++++++++++++-------------
8 files changed, 71 insertions(+), 33 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/drivers/core/device-remove.c b/drivers/core/device-remove.c
index efdb0f2905..07b241b6bb 100644
--- a/drivers/core/device-remove.c
+++ b/drivers/core/device-remove.c
@@ -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 0726be6b79..21f3054125 100644
--- a/drivers/core/root.c
+++ b/drivers/core/root.c
@@ -168,6 +168,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;
@@ -393,6 +394,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..ac474d3ff8 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,15 +134,15 @@ 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);
- if (ret)
+ 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);
- if (ret)
+ if (ret) {
return log_msg_ret("unbind", ret);
+ }
}
uc_drv = uc->uc_drv;
diff --git a/include/dm/device.h b/include/dm/device.h
index 953706cf52..7b1db252bf 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -73,6 +73,8 @@ struct driver_info;
*/
#define DM_FLAG_REMOVE_WITH_PD_ON (1 << 13)
+#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 +97,8 @@ enum {
/* Don't power down any attached power domains */
DM_REMOVE_NO_PD = 1 << 1,
+
+ 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 d20c48443f..0000b10c5e 100644
--- a/test/dm/core.c
+++ b/test/dm/core.c
@@ -81,16 +81,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();
@@ -513,7 +516,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]);
diff --git a/test/dm/test-main.c b/test/dm/test-main.c
index 53e5ca321f..2002480bf5 100644
--- a/test/dm/test-main.c
+++ b/test/dm/test-main.c
@@ -58,19 +58,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;
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH V2] dm: core: Add late driver remove option
2020-08-02 15:06 [PATCH V2] dm: core: Add late driver remove option Marek Vasut
@ 2020-08-07 16:23 ` Simon Glass
2020-08-07 21:40 ` Marek Vasut
0 siblings, 1 reply; 6+ messages in thread
From: Simon Glass @ 2020-08-07 16:23 UTC (permalink / raw)
To: u-boot
Hi Marek,
On Sun, 2 Aug 2020 at 09:06, 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: Tom Rini <trini@konsulko.com>
> ---
> V2: Fix DM tests
> ---
> arch/arm/lib/bootm.c | 1 +
> drivers/core/device-remove.c | 11 ++++++++---
> drivers/core/root.c | 2 ++
> drivers/core/uclass.c | 32 +++++++++++++++++++++++++-------
> include/dm/device.h | 4 ++++
> include/dm/uclass-internal.h | 3 ++-
> test/dm/core.c | 21 ++++++++++++---------
> test/dm/test-main.c | 30 +++++++++++++++++-------------
> 8 files changed, 71 insertions(+), 33 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/drivers/core/device-remove.c b/drivers/core/device-remove.c
> index efdb0f2905..07b241b6bb 100644
> --- a/drivers/core/device-remove.c
> +++ b/drivers/core/device-remove.c
> @@ -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;
Why return here? Shouldn;'t you use flags_remove() ?
> +
> /*
> * 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 0726be6b79..21f3054125 100644
> --- a/drivers/core/root.c
> +++ b/drivers/core/root.c
> @@ -168,6 +168,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;
>
> @@ -393,6 +394,7 @@ struct acpi_ops root_acpi_ops = {
> U_BOOT_DRIVER(root_driver) = {
> .name = "root_driver",
> .id = UCLASS_ROOT,
> + .flags = DM_FLAG_REMOVE_LATE,
Why are you changing this flag for the root device?
> ACPI_OPS_PTR(&root_acpi_ops)
> };
>
> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
> index c3f1b73cd6..ac474d3ff8 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)
Is this intended to be exported? If so, please add a test. If not,
please make it static. In any case, it needs a full comment as to what
it does, and args.
> +{
> + 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,15 +134,15 @@ 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);
> - if (ret)
> + while (!uclass_find_device_by_drv_flag(uc, DM_FLAG_REMOVE_LATE, late, &dev)) {
> + ret = device_remove(dev, flag | DM_REMOVE_NO_PD);
What happened to DM_REMOVE_NORMAL?
> + if (ret) {
> return log_msg_ret("remove", ret);
> + }
> ret = device_unbind(dev);
> - if (ret)
> + if (ret) {
> return log_msg_ret("unbind", ret);
> + }
Don't need {}
> }
>
> uc_drv = uc->uc_drv;
> diff --git a/include/dm/device.h b/include/dm/device.h
> index 953706cf52..7b1db252bf 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -73,6 +73,8 @@ struct driver_info;
> */
> #define DM_FLAG_REMOVE_WITH_PD_ON (1 << 13)
>
> +#define DM_FLAG_REMOVE_LATE (1 << 14)
Needs a comment as to what it means.
> +
> /*
> * 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 +97,8 @@ enum {
>
> /* Don't power down any attached power domains */
> DM_REMOVE_NO_PD = 1 << 1,
> +
> + DM_REMOVE_LATE = 1 << 2,
Comment
> };
>
> /**
> 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);
Why is the flag added here? Does it mean that sometimes it will not
actually destroy the uclass? It needs a comment.
>
> #endif
> diff --git a/test/dm/core.c b/test/dm/core.c
> index d20c48443f..0000b10c5e 100644
> --- a/test/dm/core.c
> +++ b/test/dm/core.c
> @@ -81,16 +81,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();
> @@ -513,7 +516,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]);
>
> diff --git a/test/dm/test-main.c b/test/dm/test-main.c
> index 53e5ca321f..2002480bf5 100644
> --- a/test/dm/test-main.c
> +++ b/test/dm/test-main.c
> @@ -58,19 +58,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;
> --
> 2.27.0
>
Your new behaviour needs a sandbox test of some sort.
Regards,
SImon
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH V2] dm: core: Add late driver remove option
2020-08-07 16:23 ` Simon Glass
@ 2020-08-07 21:40 ` Marek Vasut
2020-08-18 13:22 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Marek Vasut @ 2020-08-07 21:40 UTC (permalink / raw)
To: u-boot
On 8/7/20 6:23 PM, Simon Glass wrote:
[...]
>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
>> index efdb0f2905..07b241b6bb 100644
>> --- a/drivers/core/device-remove.c
>> +++ b/drivers/core/device-remove.c
>> @@ -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;
>
> Why return here?
If the DM isn't in a late-remove phase and the driver has remove-late
flag, then the driver should not be removed yet. This is the case for
e.g. a clock driver.
> Shouldn;'t you use flags_remove() ?
Please explain ?
>> /*
>> * 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 0726be6b79..21f3054125 100644
>> --- a/drivers/core/root.c
>> +++ b/drivers/core/root.c
>> @@ -168,6 +168,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;
>>
>> @@ -393,6 +394,7 @@ struct acpi_ops root_acpi_ops = {
>> U_BOOT_DRIVER(root_driver) = {
>> .name = "root_driver",
>> .id = UCLASS_ROOT,
>> + .flags = DM_FLAG_REMOVE_LATE,
>
> Why are you changing this flag for the root device?
Because root device should only be removed last.
>> ACPI_OPS_PTR(&root_acpi_ops)
>> };
>>
>> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
>> index c3f1b73cd6..ac474d3ff8 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)
>
> Is this intended to be exported? If so, please add a test. If not,
> please make it static. In any case, it needs a full comment as to what
> it does, and args.
Its internal function, should be static.
>> +{
>> + 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,15 +134,15 @@ 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);
>> - if (ret)
>> + while (!uclass_find_device_by_drv_flag(uc, DM_FLAG_REMOVE_LATE, late, &dev)) {
>> + ret = device_remove(dev, flag | DM_REMOVE_NO_PD);
>
> What happened to DM_REMOVE_NORMAL?
See above, bool late, this uclass_destroy() is called twice now. Once
for normal devices, once for late devices, so that the ordering is correct.
>> + if (ret) {
>> return log_msg_ret("remove", ret);
>> + }
>> ret = device_unbind(dev);
>> - if (ret)
>> + if (ret) {
>> return log_msg_ret("unbind", ret);
>> + }
>
> Don't need {}
>
>> }
>>
>> uc_drv = uc->uc_drv;
>> diff --git a/include/dm/device.h b/include/dm/device.h
>> index 953706cf52..7b1db252bf 100644
>> --- a/include/dm/device.h
>> +++ b/include/dm/device.h
>> @@ -73,6 +73,8 @@ struct driver_info;
>> */
>> #define DM_FLAG_REMOVE_WITH_PD_ON (1 << 13)
>>
>> +#define DM_FLAG_REMOVE_LATE (1 << 14)
>
> Needs a comment as to what it means.
It means the driver should be removed after all the non-late drivers.
>> +
>> /*
>> * 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 +97,8 @@ enum {
>>
>> /* Don't power down any attached power domains */
>> DM_REMOVE_NO_PD = 1 << 1,
>> +
>> + DM_REMOVE_LATE = 1 << 2,
>
> Comment
>
>> };
>>
>> /**
>> 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);
>
> Why is the flag added here? Does it mean that sometimes it will not
> actually destroy the uclass? It needs a comment.
Yes, because the drivers in the uclass need to be removed in two steps,
first the ones which can be removed early, and then the rest which need
to be removed late (like clock drivers).
[...]
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH V2] dm: core: Add late driver remove option
2020-08-07 21:40 ` Marek Vasut
@ 2020-08-18 13:22 ` Simon Glass
2020-08-18 14:13 ` Stefan Roese
0 siblings, 1 reply; 6+ messages in thread
From: Simon Glass @ 2020-08-18 13:22 UTC (permalink / raw)
To: u-boot
Hi Marek,
+Stefan Roese who wrote the existing code.
On Fri, 7 Aug 2020 at 15:40, Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 8/7/20 6:23 PM, Simon Glass wrote:
>
> [...]
>
> >> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
> >> index efdb0f2905..07b241b6bb 100644
> >> --- a/drivers/core/device-remove.c
> >> +++ b/drivers/core/device-remove.c
> >> @@ -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))
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)
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.
> >> + return 0;
> >
> > Why return here?
>
> If the DM isn't in a late-remove phase and the driver has remove-late
> flag, then the driver should not be removed yet. This is the case for
> e.g. a clock driver.
>
> > Shouldn;'t you use flags_remove() ?
>
> Please explain ?
That function checks the flags so I think you should add your check
there rather than adding new code.
>
> >> /*
> >> * 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 0726be6b79..21f3054125 100644
> >> --- a/drivers/core/root.c
> >> +++ b/drivers/core/root.c
> >> @@ -168,6 +168,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;
> >>
> >> @@ -393,6 +394,7 @@ struct acpi_ops root_acpi_ops = {
> >> U_BOOT_DRIVER(root_driver) = {
> >> .name = "root_driver",
> >> .id = UCLASS_ROOT,
> >> + .flags = DM_FLAG_REMOVE_LATE,
> >
> > Why are you changing this flag for the root device?
>
> Because root device should only be removed last.
OK I see, thanks.
>
> >> ACPI_OPS_PTR(&root_acpi_ops)
> >> };
> >>
> >> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
> >> index c3f1b73cd6..ac474d3ff8 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)
> >
> > Is this intended to be exported? If so, please add a test. If not,
> > please make it static. In any case, it needs a full comment as to what
> > it does, and args.
>
> Its internal function, should be static.
OK so please add a comment.
>
> >> +{
> >> + 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,15 +134,15 @@ 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);
> >> - if (ret)
> >> + while (!uclass_find_device_by_drv_flag(uc, DM_FLAG_REMOVE_LATE, late, &dev)) {
> >> + ret = device_remove(dev, flag | DM_REMOVE_NO_PD);
> >
> > What happened to DM_REMOVE_NORMAL?
>
> See above, bool late, this uclass_destroy() is called twice now. Once
> for normal devices, once for late devices, so that the ordering is correct.
Actually DM_REMOVE_NO_PD looks broken to me. We have to describe the
devices in this function, but it looks like that won't happen as it is
written. The docs for uclass_destroy() don't say anything about it
sometimes not destroying the uclass.
Anyway, hopefully with the changes above, you won't need any changes here.
>
> >> + if (ret) {
> >> return log_msg_ret("remove", ret);
> >> + }
> >> ret = device_unbind(dev);
> >> - if (ret)
> >> + if (ret) {
> >> return log_msg_ret("unbind", ret);
> >> + }
> >
> > Don't need {}
> >
> >> }
> >>
> >> uc_drv = uc->uc_drv;
> >> diff --git a/include/dm/device.h b/include/dm/device.h
> >> index 953706cf52..7b1db252bf 100644
> >> --- a/include/dm/device.h
> >> +++ b/include/dm/device.h
> >> @@ -73,6 +73,8 @@ struct driver_info;
> >> */
> >> #define DM_FLAG_REMOVE_WITH_PD_ON (1 << 13)
> >>
> >> +#define DM_FLAG_REMOVE_LATE (1 << 14)
> >
> > Needs a comment as to what it means.
>
> It means the driver should be removed after all the non-late drivers.
OK, see above. It still needs a comment.
>
> >> +
> >> /*
> >> * 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 +97,8 @@ enum {
> >>
> >> /* Don't power down any attached power domains */
> >> DM_REMOVE_NO_PD = 1 << 1,
> >> +
> >> + DM_REMOVE_LATE = 1 << 2,
> >
> > Comment
> >
> >> };
> >>
> >> /**
> >> 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);
> >
> > Why is the flag added here? Does it mean that sometimes it will not
> > actually destroy the uclass? It needs a comment.
>
> Yes, because the drivers in the uclass need to be removed in two steps,
> first the ones which can be removed early, and then the rest which need
> to be removed late (like clock drivers).
It is OK to remove the devices from a uclass in stages, but destroying
the uclass should happen once. So far as I understand it, you should
not have this flag.
Also we need a sandbox test for this new behaviour as it is getting complicated.
Stefan, could you perhaps look at a test for the existing flags? We
should have some devices with different flags set and check that the
correct things happen when calling device_remove() with various flags.
Regards,
SImon
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH V2] dm: core: Add late driver remove option
2020-08-18 13:22 ` Simon Glass
@ 2020-08-18 14:13 ` Stefan Roese
2020-08-18 14:49 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Roese @ 2020-08-18 14:13 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 18.08.20 15:22, Simon Glass wrote:
> Hi Marek,
>
> +Stefan Roese who wrote the existing code.
Hmmm, I was not Cc'ed.
> On Fri, 7 Aug 2020 at 15:40, Marek Vasut <marek.vasut@gmail.com> wrote:
>>
>> On 8/7/20 6:23 PM, Simon Glass wrote:
>>
>> [...]
>>
>>>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
>>>> index efdb0f2905..07b241b6bb 100644
>>>> --- a/drivers/core/device-remove.c
>>>> +++ b/drivers/core/device-remove.c
>>>> @@ -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))
>
> 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)
>
> 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.
>
>>>> + return 0;
>>>
>>> Why return here?
>>
>> If the DM isn't in a late-remove phase and the driver has remove-late
>> flag, then the driver should not be removed yet. This is the case for
>> e.g. a clock driver.
>>
>>> Shouldn;'t you use flags_remove() ?
>>
>> Please explain ?
>
> That function checks the flags so I think you should add your check
> there rather than adding new code.
>
>>
>>>> /*
>>>> * 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 0726be6b79..21f3054125 100644
>>>> --- a/drivers/core/root.c
>>>> +++ b/drivers/core/root.c
>>>> @@ -168,6 +168,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;
>>>>
>>>> @@ -393,6 +394,7 @@ struct acpi_ops root_acpi_ops = {
>>>> U_BOOT_DRIVER(root_driver) = {
>>>> .name = "root_driver",
>>>> .id = UCLASS_ROOT,
>>>> + .flags = DM_FLAG_REMOVE_LATE,
>>>
>>> Why are you changing this flag for the root device?
>>
>> Because root device should only be removed last.
>
> OK I see, thanks.
>
>>
>>>> ACPI_OPS_PTR(&root_acpi_ops)
>>>> };
>>>>
>>>> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
>>>> index c3f1b73cd6..ac474d3ff8 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)
>>>
>>> Is this intended to be exported? If so, please add a test. If not,
>>> please make it static. In any case, it needs a full comment as to what
>>> it does, and args.
>>
>> Its internal function, should be static.
>
> OK so please add a comment.
>
>>
>>>> +{
>>>> + 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,15 +134,15 @@ 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);
>>>> - if (ret)
>>>> + while (!uclass_find_device_by_drv_flag(uc, DM_FLAG_REMOVE_LATE, late, &dev)) {
>>>> + ret = device_remove(dev, flag | DM_REMOVE_NO_PD);
>>>
>>> What happened to DM_REMOVE_NORMAL?
>>
>> See above, bool late, this uclass_destroy() is called twice now. Once
>> for normal devices, once for late devices, so that the ordering is correct.
>
> Actually DM_REMOVE_NO_PD looks broken to me. We have to describe the
> devices in this function, but it looks like that won't happen as it is
> written. The docs for uclass_destroy() don't say anything about it
> sometimes not destroying the uclass.
>
> Anyway, hopefully with the changes above, you won't need any changes here.
>
>>
>>>> + if (ret) {
>>>> return log_msg_ret("remove", ret);
>>>> + }
>>>> ret = device_unbind(dev);
>>>> - if (ret)
>>>> + if (ret) {
>>>> return log_msg_ret("unbind", ret);
>>>> + }
>>>
>>> Don't need {}
>>>
>>>> }
>>>>
>>>> uc_drv = uc->uc_drv;
>>>> diff --git a/include/dm/device.h b/include/dm/device.h
>>>> index 953706cf52..7b1db252bf 100644
>>>> --- a/include/dm/device.h
>>>> +++ b/include/dm/device.h
>>>> @@ -73,6 +73,8 @@ struct driver_info;
>>>> */
>>>> #define DM_FLAG_REMOVE_WITH_PD_ON (1 << 13)
>>>>
>>>> +#define DM_FLAG_REMOVE_LATE (1 << 14)
>>>
>>> Needs a comment as to what it means.
>>
>> It means the driver should be removed after all the non-late drivers.
>
> OK, see above. It still needs a comment.
>
>>
>>>> +
>>>> /*
>>>> * 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 +97,8 @@ enum {
>>>>
>>>> /* Don't power down any attached power domains */
>>>> DM_REMOVE_NO_PD = 1 << 1,
>>>> +
>>>> + DM_REMOVE_LATE = 1 << 2,
>>>
>>> Comment
>>>
>>>> };
>>>>
>>>> /**
>>>> 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);
>>>
>>> Why is the flag added here? Does it mean that sometimes it will not
>>> actually destroy the uclass? It needs a comment.
>>
>> Yes, because the drivers in the uclass need to be removed in two steps,
>> first the ones which can be removed early, and then the rest which need
>> to be removed late (like clock drivers).
>
> It is OK to remove the devices from a uclass in stages, but destroying
> the uclass should happen once. So far as I understand it, you should
> not have this flag.
>
> Also we need a sandbox test for this new behaviour as it is getting complicated.
>
> Stefan, could you perhaps look at a test for the existing flags? We
> should have some devices with different flags set and check that the
> correct things happen when calling device_remove() with various flags.
I did send a test at that time. Commit 24f927c527b0 ("dm: test: Add test
for device removal") adds this test. Is something missing in this test?
Thanks,
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH V2] dm: core: Add late driver remove option
2020-08-18 14:13 ` Stefan Roese
@ 2020-08-18 14:49 ` Simon Glass
0 siblings, 0 replies; 6+ messages in thread
From: Simon Glass @ 2020-08-18 14:49 UTC (permalink / raw)
To: u-boot
Hi Stefan,
On Tue, 18 Aug 2020 at 08:13, Stefan Roese <sr@denx.de> wrote:
>
> Hi Simon,
>
> On 18.08.20 15:22, Simon Glass wrote:
> > Hi Marek,
> >
> > +Stefan Roese who wrote the existing code.
>
> Hmmm, I was not Cc'ed.
>
> > On Fri, 7 Aug 2020 at 15:40, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>
> >> On 8/7/20 6:23 PM, Simon Glass wrote:
> >>
> >> [...]
> >>
> >>>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
> >>>> index efdb0f2905..07b241b6bb 100644
> >>>> --- a/drivers/core/device-remove.c
> >>>> +++ b/drivers/core/device-remove.c
> >>>> @@ -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))
> >
> > 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)
> >
> > 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.
> >
> >>>> + return 0;
> >>>
> >>> Why return here?
> >>
> >> If the DM isn't in a late-remove phase and the driver has remove-late
> >> flag, then the driver should not be removed yet. This is the case for
> >> e.g. a clock driver.
> >>
> >>> Shouldn;'t you use flags_remove() ?
> >>
> >> Please explain ?
> >
> > That function checks the flags so I think you should add your check
> > there rather than adding new code.
> >
> >>
> >>>> /*
> >>>> * 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 0726be6b79..21f3054125 100644
> >>>> --- a/drivers/core/root.c
> >>>> +++ b/drivers/core/root.c
> >>>> @@ -168,6 +168,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;
> >>>>
> >>>> @@ -393,6 +394,7 @@ struct acpi_ops root_acpi_ops = {
> >>>> U_BOOT_DRIVER(root_driver) = {
> >>>> .name = "root_driver",
> >>>> .id = UCLASS_ROOT,
> >>>> + .flags = DM_FLAG_REMOVE_LATE,
> >>>
> >>> Why are you changing this flag for the root device?
> >>
> >> Because root device should only be removed last.
> >
> > OK I see, thanks.
> >
> >>
> >>>> ACPI_OPS_PTR(&root_acpi_ops)
> >>>> };
> >>>>
> >>>> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
> >>>> index c3f1b73cd6..ac474d3ff8 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)
> >>>
> >>> Is this intended to be exported? If so, please add a test. If not,
> >>> please make it static. In any case, it needs a full comment as to what
> >>> it does, and args.
> >>
> >> Its internal function, should be static.
> >
> > OK so please add a comment.
> >
> >>
> >>>> +{
> >>>> + 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,15 +134,15 @@ 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);
> >>>> - if (ret)
> >>>> + while (!uclass_find_device_by_drv_flag(uc, DM_FLAG_REMOVE_LATE, late, &dev)) {
> >>>> + ret = device_remove(dev, flag | DM_REMOVE_NO_PD);
> >>>
> >>> What happened to DM_REMOVE_NORMAL?
> >>
> >> See above, bool late, this uclass_destroy() is called twice now. Once
> >> for normal devices, once for late devices, so that the ordering is correct.
> >
> > Actually DM_REMOVE_NO_PD looks broken to me. We have to describe the
> > devices in this function, but it looks like that won't happen as it is
> > written. The docs for uclass_destroy() don't say anything about it
> > sometimes not destroying the uclass.
> >
> > Anyway, hopefully with the changes above, you won't need any changes here.
> >
> >>
> >>>> + if (ret) {
> >>>> return log_msg_ret("remove", ret);
> >>>> + }
> >>>> ret = device_unbind(dev);
> >>>> - if (ret)
> >>>> + if (ret) {
> >>>> return log_msg_ret("unbind", ret);
> >>>> + }
> >>>
> >>> Don't need {}
> >>>
> >>>> }
> >>>>
> >>>> uc_drv = uc->uc_drv;
> >>>> diff --git a/include/dm/device.h b/include/dm/device.h
> >>>> index 953706cf52..7b1db252bf 100644
> >>>> --- a/include/dm/device.h
> >>>> +++ b/include/dm/device.h
> >>>> @@ -73,6 +73,8 @@ struct driver_info;
> >>>> */
> >>>> #define DM_FLAG_REMOVE_WITH_PD_ON (1 << 13)
> >>>>
> >>>> +#define DM_FLAG_REMOVE_LATE (1 << 14)
> >>>
> >>> Needs a comment as to what it means.
> >>
> >> It means the driver should be removed after all the non-late drivers.
> >
> > OK, see above. It still needs a comment.
> >
> >>
> >>>> +
> >>>> /*
> >>>> * 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 +97,8 @@ enum {
> >>>>
> >>>> /* Don't power down any attached power domains */
> >>>> DM_REMOVE_NO_PD = 1 << 1,
> >>>> +
> >>>> + DM_REMOVE_LATE = 1 << 2,
> >>>
> >>> Comment
> >>>
> >>>> };
> >>>>
> >>>> /**
> >>>> 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);
> >>>
> >>> Why is the flag added here? Does it mean that sometimes it will not
> >>> actually destroy the uclass? It needs a comment.
> >>
> >> Yes, because the drivers in the uclass need to be removed in two steps,
> >> first the ones which can be removed early, and then the rest which need
> >> to be removed late (like clock drivers).
> >
> > It is OK to remove the devices from a uclass in stages, but destroying
> > the uclass should happen once. So far as I understand it, you should
> > not have this flag.
> >
> > Also we need a sandbox test for this new behaviour as it is getting complicated.
> >
> > Stefan, could you perhaps look at a test for the existing flags? We
> > should have some devices with different flags set and check that the
> > correct things happen when calling device_remove() with various flags.
>
> I did send a test at that time. Commit 24f927c527b0 ("dm: test: Add test
> for device removal") adds this test. Is something missing in this test?
Ah yes I thought you did. I missed it, sorry. It is DM_REMOVE_NO_PD
that lacks a test.
Regards,
SImon
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-18 14:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-02 15:06 [PATCH V2] dm: core: Add late driver remove option Marek Vasut
2020-08-07 16:23 ` Simon Glass
2020-08-07 21:40 ` Marek Vasut
2020-08-18 13:22 ` Simon Glass
2020-08-18 14:13 ` Stefan Roese
2020-08-18 14:49 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.