[PATCH V3] dm: core: Add late driver remove option

[PATCH V3] dm: core: Add late driver remove option

[Marek Vasut]

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;
-- 
2.28.0

[PATCH V3] dm: core: Add late driver remove option

[Simon Glass]

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 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);

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

[PATCH V3] dm: core: Add late driver remove option

[Marek Vasut]

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.

[...]

[PATCH V3] dm: core: Add late driver remove option

[Simon Glass]

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

[PATCH V3] dm: core: Add late driver remove option

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201118/422f84a7/attachment.sig>

[PATCH V3] dm: core: Add late driver remove option

[Simon Glass]

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

[PATCH V3] dm: core: Add late driver remove option

[Marek Vasut]

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.

[PATCH V3] dm: core: Add late driver remove option

[Sean Anderson]

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

[PATCH V3] dm: core: Add late driver remove option

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201123/d9aeb5e3/attachment.sig>

[PATCH V3] dm: core: Add late driver remove option

[Marek Vasut]

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.

[PATCH V3] dm: core: Add late driver remove option

[Simon Glass]

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

[PATCH V3] dm: core: Add late driver remove option

[Marek Vasut]

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 ?

[...]

[PATCH V3] dm: core: Add late driver remove option

[Simon Glass]

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