[PATCH V2] dm: core: Add late driver remove option
Simon Glass
sjg at chromium.org
Fri Aug 7 18:23:31 CEST 2020
Hi Marek,
On Sun, 2 Aug 2020 at 09:06, Marek Vasut <marek.vasut at 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 at gmail.com>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Tom Rini <trini at 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
More information about the U-Boot
mailing list