Skip to contentSkip to conversation

dm: core: Add flags parameter to device_remove()

PATCH New Stefan Roese 2 participants 13 messages Mar 20, 2017 – Apr 5, 2017

Subsystems (8)
Arm (32-bit) x86 USB Block layer +4 more subsystems
lore
Share & tools
lore

Stefan Roese

author
#0 lore raw

PATCH v2 1/4 dm: core: Add flags parameter to device_remove()

This patch adds the flags parameter to device_remove() and changes all calls to this function to provide the default value of DM_REMOVE_NORMAL for "normal" device removal.

This is in preparation for the driver specific pre-OS (e.g. DMA cancelling) remove support.

Signed-off-by Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>

---
v2:
- Changes DM_FLAG macro as suggested by Simon
- Minor comment change as suggested by Simon

 arch/x86/cpu/queensbay/tnc.c   |  4 ++--
 cmd/cros_ec.c                  |  2 +-
 cmd/sf.c                       |  2 +-
 drivers/block/blk-uclass.c     |  2 +-
 drivers/block/sandbox.c        |  2 +-
 drivers/core/device-remove.c   |  9 +++++----
 drivers/core/device.c          |  2 +-
 drivers/core/root.c            |  2 +-
 drivers/core/uclass.c          |  2 +-
 drivers/mmc/mmc-uclass.c       |  2 +-
 drivers/mtd/spi/sandbox.c      |  2 +-
 drivers/mtd/spi/sf-uclass.c    |  2 +-
 drivers/spi/spi-uclass.c       |  4 ++--
 drivers/usb/emul/sandbox_hub.c |  2 +-
 drivers/usb/host/usb-uclass.c  |  4 ++--
 include/dm/device-internal.h   |  5 +++--
 include/dm/device.h            | 26 ++++++++++++++++++++++++++
 test/dm/bus.c                  |  8 ++++----
 test/dm/core.c                 | 16 ++++++++--------
 test/dm/eth.c                  |  2 +-
 test/dm/spi.c                  |  2 +-
 21 files changed, 65 insertions(+), 37 deletions(-)
arch/x86/cpu/queensbay/tnc.c +2 -2
index f307c622c8..94668a4fda 100644
@@ -76,13 +76,13 @@ static int __maybe_unused disable_igd(void)
 	 *
 	 * So the only option we have is to manually remove these two devices.
 	 */
-	ret = device_remove(igd);
+	ret = device_remove(igd, DM_REMOVE_NORMAL);
 	if (ret)
 		return ret;
 	ret = device_unbind(igd);
 	if (ret)
 		return ret;
-	ret = device_remove(sdvo);
+	ret = device_remove(sdvo, DM_REMOVE_NORMAL);
 	if (ret)
 		return ret;
 	ret = device_unbind(sdvo);
cmd/cros_ec.c +1 -1
index 9d42f870dc..af0b4eee78 100644
@@ -110,7 +110,7 @@ static int do_cros_ec(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		/* Remove any existing device */
 		ret = uclass_find_device(UCLASS_CROS_EC, 0, &udev);
 		if (!ret)
-			device_remove(udev);
+			device_remove(udev, DM_REMOVE_NORMAL);
 		ret = uclass_get_device(UCLASS_CROS_EC, 0, &udev);
 		if (ret) {
 			printf("Could not init cros_ec device (err %d)\n", ret);
cmd/sf.c +1 -1
index 65b117feee..f971eec781 100644
@@ -124,7 +124,7 @@ static int do_spi_flash_probe(int argc, char * const argv[])
 	/* Remove the old device, otherwise probe will just be a nop */
 	ret = spi_find_bus_and_cs(bus, cs, &bus_dev, &new);
 	if (!ret) {
-		device_remove(new);
+		device_remove(new, DM_REMOVE_NORMAL);
 	}
 	flash = NULL;
 	ret = spi_flash_probe_bus_cs(bus, cs, speed, mode, &new);
drivers/block/blk-uclass.c +1 -1
index 38cb9388da..af3c35f6d0 100644
@@ -530,7 +530,7 @@ int blk_unbind_all(int if_type)
 		struct blk_desc *desc = dev_get_uclass_platdata(dev);
 
 		if (desc->if_type == if_type) {
-			ret = device_remove(dev);
+			ret = device_remove(dev, DM_REMOVE_NORMAL);
 			if (ret)
 				return ret;
 			ret = device_unbind(dev);
drivers/block/sandbox.c +1 -1
index 36c2ff3007..34d1c638bc 100644
@@ -98,7 +98,7 @@ int host_dev_bind(int devnum, char *filename)
 	/* Remove and unbind the old device, if any */
 	ret = blk_get_device(IF_TYPE_HOST, devnum, &dev);
 	if (ret == 0) {
-		ret = device_remove(dev);
+		ret = device_remove(dev, DM_REMOVE_NORMAL);
 		if (ret)
 			return ret;
 		ret = device_unbind(dev);
drivers/core/device-remove.c +5 -4
index a7f77b4a21..b80bf52320 100644
@@ -46,9 +46,10 @@ static int device_chld_unbind(struct udevice *dev)
 /**
  * device_chld_remove() - Stop all device's children
  * @dev:	The device whose children are to be removed
+ * @pre_os_remove: Flag, if this functions is called in the pre-OS stage
  * @return 0 on success, -ve on error
  */
-static int device_chld_remove(struct udevice *dev)
+static int device_chld_remove(struct udevice *dev, uint flags)
 {
 	struct udevice *pos, *n;
 	int ret;

@@ -56,7 +57,7 @@ static int device_chld_remove(struct udevice *dev)
 	assert(dev);
 
 	list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) {
-		ret = device_remove(pos);
+		ret = device_remove(pos, flags);
 		if (ret)
 			return ret;
 	}

@@ -151,7 +152,7 @@ void device_free(struct udevice *dev)
 	devres_release_probe(dev);
 }
 
-int device_remove(struct udevice *dev)
+int device_remove(struct udevice *dev, uint flags)
 {
 	const struct driver *drv;
 	int ret;

@@ -169,7 +170,7 @@ int device_remove(struct udevice *dev)
 	if (ret)
 		return ret;
 
-	ret = device_chld_remove(dev);
+	ret = device_chld_remove(dev, flags);
 	if (ret)
 		goto err;
drivers/core/device.c +1 -1
index 70fcfc23e0..e1b0ebffc5 100644
@@ -378,7 +378,7 @@ int device_probe(struct udevice *dev)
 
 	return 0;
 fail_uclass:
-	if (device_remove(dev)) {
+	if (device_remove(dev, DM_REMOVE_NORMAL)) {
 		dm_warn("%s: Device '%s' failed to remove on error path\n",
 			__func__, dev->name);
 	}
drivers/core/root.c +1 -1
index 175fd3fb25..6dafc6ecfc 100644
@@ -176,7 +176,7 @@ int dm_init(void)
 
 int dm_uninit(void)
 {
-	device_remove(dm_root());
+	device_remove(dm_root(), DM_REMOVE_NORMAL);
 	device_unbind(dm_root());
 
 	return 0;
drivers/core/uclass.c +1 -1
index 7de370644d..d94d43a98d 100644
@@ -116,7 +116,7 @@ int uclass_destroy(struct uclass *uc)
 	while (!list_empty(&uc->dev_head)) {
 		dev = list_first_entry(&uc->dev_head, struct udevice,
 				       uclass_node);
-		ret = device_remove(dev);
+		ret = device_remove(dev, DM_REMOVE_NORMAL);
 		if (ret)
 			return ret;
 		ret = device_unbind(dev);
drivers/mmc/mmc-uclass.c +1 -1
index 5bb446bcc2..9c07871d3a 100644
@@ -232,7 +232,7 @@ int mmc_unbind(struct udevice *dev)
 
 	device_find_first_child(dev, &bdev);
 	if (bdev) {
-		device_remove(bdev);
+		device_remove(bdev, DM_REMOVE_NORMAL);
 		device_unbind(bdev);
 	}
drivers/mtd/spi/sandbox.c +1 -1
index 36a50fe3a1..a53f4ebc68 100644
@@ -595,7 +595,7 @@ void sandbox_sf_unbind_emul(struct sandbox_state *state, int busnum, int cs)
 	struct udevice *dev;
 
 	dev = state->spi[busnum][cs].emul;
-	device_remove(dev);
+	device_remove(dev, DM_REMOVE_NORMAL);
 	device_unbind(dev);
 	state->spi[busnum][cs].emul = NULL;
 }
drivers/mtd/spi/sf-uclass.c +1 -1
index 19de964e61..83876485fe 100644
@@ -46,7 +46,7 @@ struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs,
 
 void spi_flash_free(struct spi_flash *flash)
 {
-	device_remove(flash->spi->dev);
+	device_remove(flash->spi->dev, DM_REMOVE_NORMAL);
 }
 
 int spi_flash_probe_bus_cs(unsigned int busnum, unsigned int cs,
drivers/spi/spi-uclass.c +2 -2
index ac17da0777..c061c05443 100644
@@ -343,7 +343,7 @@ err:
 	debug("%s: Error path, created=%d, device '%s'\n", __func__,
 	      created, dev->name);
 	if (created) {
-		device_remove(dev);
+		device_remove(dev, DM_REMOVE_NORMAL);
 		device_unbind(dev);
 	}
 

@@ -384,7 +384,7 @@ struct spi_slave *spi_setup_slave(unsigned int busnum, unsigned int cs,
 
 void spi_free_slave(struct spi_slave *slave)
 {
-	device_remove(slave->dev);
+	device_remove(slave->dev, DM_REMOVE_NORMAL);
 	slave->dev = NULL;
 }
drivers/usb/emul/sandbox_hub.c +1 -1
index c3a8e73389..f0939b19f4 100644
@@ -156,7 +156,7 @@ static int clrset_post_state(struct udevice *hub, int port, int clear, int set)
 			} else if (clear & USB_PORT_STAT_POWER) {
 				debug("%s: %s: power off, removed, ret=%d\n",
 				      __func__, dev->name, ret);
-				ret = device_remove(dev);
+				ret = device_remove(dev, DM_REMOVE_NORMAL);
 				clear |= USB_PORT_STAT_CONNECTION;
 			}
 		}
drivers/usb/host/usb-uclass.c +2 -2
index 5cf1e9a36c..6eded4abad 100644
@@ -154,7 +154,7 @@ int usb_stop(void)
 	uc_priv = uc->priv;
 
 	uclass_foreach_dev(bus, uc) {
-		ret = device_remove(bus);
+		ret = device_remove(bus, DM_REMOVE_NORMAL);
 		if (ret && !err)
 			err = ret;
 	}

@@ -358,7 +358,7 @@ int usb_setup_ehci_gadget(struct ehci_ctrl **ctlrp)
 	ret = uclass_find_device_by_seq(UCLASS_USB, 0, true, &dev);
 	if (ret)
 		return ret;
-	ret = device_remove(dev);
+	ret = device_remove(dev, DM_REMOVE_NORMAL);
 	if (ret)
 		return ret;
include/dm/device-internal.h +3 -2
index 0bf8707493..2cabc87338 100644
@@ -96,12 +96,13 @@ int device_probe(struct udevice *dev);
  * children are deactivated first.
  *
  * @dev: Pointer to device to remove
+ * @flags: Flags for selective device removal
  * @return 0 if OK, -ve on error (an error here is normally a very bad thing)
  */
 #if CONFIG_IS_ENABLED(DM_DEVICE_REMOVE)
-int device_remove(struct udevice *dev);
+int device_remove(struct udevice *dev, uint flags);
 #else
-static inline int device_remove(struct udevice *dev) { return 0; }
+static inline int device_remove(struct udevice *dev, uint flags) { return 0; }
 #endif
 
 /**
include/dm/device.h +26 -0
index 4e95fb7773..079ec57003 100644
@@ -46,6 +46,32 @@ struct driver_info;
 
 #define DM_FLAG_OF_PLATDATA		(1 << 8)
 
+/*
+ * Call driver remove function to stop currently active DMA transfers or
+ * give DMA buffers back to the HW / controller. This may be needed for
+ * some drivers to do some final stage cleanup before the OS is called
+ * (U-Boot exit)
+ */
+#define DM_FLAG_ACTIVE_DMA		(1 << 9)
+
+/*
+ * 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
+ * driver flags can be done.
+ */
+enum {
+	/* Normal remove, remove all devices */
+	DM_REMOVE_NORMAL     = 1 << 0,
+
+	/* Remove devices with active DMA */
+	DM_REMOVE_ACTIVE_DMA = DM_FLAG_ACTIVE_DMA,
+
+	/* Add more use cases here */
+
+	/* Remove devices with any active flag */
+	DM_REMOVE_ACTIVE_ALL = DM_REMOVE_ACTIVE_DMA,
+};
+
 /**
  * struct udevice - An instance of a driver
  *
test/dm/bus.c +4 -4
index d94dcf7a60..6a2773565e 100644
@@ -222,7 +222,7 @@ static int test_bus_parent_data(struct unit_test_state *uts)
 	/* Check that it starts@0 and goes away when device is removed */
 	parent_data->sum += 5;
 	ut_asserteq(5, parent_data->sum);
-	device_remove(dev);
+	device_remove(dev, DM_REMOVE_NORMAL);
 	ut_asserteq_ptr(NULL, dev_get_parent_priv(dev));
 
 	/* Check that we can do this twice */

@@ -323,7 +323,7 @@ static int dm_test_bus_parent_ops(struct unit_test_state *uts)
 			continue;
 		parent_data = dev_get_parent_priv(dev);
 		ut_asserteq(FLAG_CHILD_PROBED, parent_data->flag);
-		ut_assertok(device_remove(dev));
+		ut_assertok(device_remove(dev, DM_REMOVE_NORMAL));
 		ut_asserteq_ptr(NULL, dev_get_parent_priv(dev));
 		ut_asserteq_ptr(dms->removed, dev);
 	}

@@ -360,7 +360,7 @@ static int test_bus_parent_platdata(struct unit_test_state *uts)
 		plat->count++;
 		ut_asserteq(1, plat->count);
 		device_probe(dev);
-		device_remove(dev);
+		device_remove(dev, DM_REMOVE_NORMAL);
 
 		ut_asserteq_ptr(plat, dev_get_parent_platdata(dev));
 		ut_asserteq(1, plat->count);

@@ -370,7 +370,7 @@ static int test_bus_parent_platdata(struct unit_test_state *uts)
 	ut_asserteq(3, child_count);
 
 	/* Removing the bus should also have no effect (it is still bound) */
-	device_remove(bus);
+	device_remove(bus, DM_REMOVE_NORMAL);
 	for (device_find_first_child(bus, &dev), child_count = 0;
 	     dev;
 	     device_find_next_child(&dev)) {
test/dm/core.c +8 -8
index 70bf4d0605..07b2419ea4 100644
@@ -319,7 +319,7 @@ static int dm_test_lifecycle(struct unit_test_state *uts)
 
 	/* Now remove device 3 */
 	ut_asserteq(0, dm_testdrv_op_count[DM_TEST_OP_PRE_REMOVE]);
-	ut_assertok(device_remove(dev));
+	ut_assertok(device_remove(dev, DM_REMOVE_NORMAL));
 	ut_asserteq(1, dm_testdrv_op_count[DM_TEST_OP_PRE_REMOVE]);
 
 	ut_asserteq(0, dm_testdrv_op_count[DM_TEST_OP_UNBIND]);

@@ -352,7 +352,7 @@ static int dm_test_ordering(struct unit_test_state *uts)
 	ut_assert(dev_last);
 
 	/* Now remove device 3 */
-	ut_assertok(device_remove(dev));
+	ut_assertok(device_remove(dev, DM_REMOVE_NORMAL));
 	ut_assertok(device_unbind(dev));
 
 	/* The device numbering should have shifted down one */

@@ -371,9 +371,9 @@ static int dm_test_ordering(struct unit_test_state *uts)
 	ut_assert(pingret == 102);
 
 	/* Remove 3 and 4 */
-	ut_assertok(device_remove(dev_penultimate));
+	ut_assertok(device_remove(dev_penultimate, DM_REMOVE_NORMAL));
 	ut_assertok(device_unbind(dev_penultimate));
-	ut_assertok(device_remove(dev_last));
+	ut_assertok(device_remove(dev_last, DM_REMOVE_NORMAL));
 	ut_assertok(device_unbind(dev_last));
 
 	/* Our device should now be in position 3 */

@@ -381,7 +381,7 @@ static int dm_test_ordering(struct unit_test_state *uts)
 	ut_assert(dev == test_dev);
 
 	/* Now remove device 3 */
-	ut_assertok(device_remove(dev));
+	ut_assertok(device_remove(dev, DM_REMOVE_NORMAL));
 	ut_assertok(device_unbind(dev));
 
 	return 0;

@@ -457,7 +457,7 @@ static int dm_test_remove(struct unit_test_state *uts)
 		ut_assert(dev);
 		ut_assertf(dev->flags & DM_FLAG_ACTIVATED,
 			   "Driver %d/%s not activated", i, dev->name);
-		ut_assertok(device_remove(dev));
+		ut_assertok(device_remove(dev, DM_REMOVE_NORMAL));
 		ut_assertf(!(dev->flags & DM_FLAG_ACTIVATED),
 			   "Driver %d/%s should have deactivated", i,
 			   dev->name);

@@ -611,14 +611,14 @@ static int dm_test_children(struct unit_test_state *uts)
 	ut_asserteq(total, dm_testdrv_op_count[DM_TEST_OP_PROBE]);
 
 	/* Remove a top-level child and check that the children are removed */
-	ut_assertok(device_remove(top[2]));
+	ut_assertok(device_remove(top[2], DM_REMOVE_NORMAL));
 	ut_asserteq(NODE_COUNT + 1, dm_testdrv_op_count[DM_TEST_OP_REMOVE]);
 	dm_testdrv_op_count[DM_TEST_OP_REMOVE] = 0;
 
 	/* Try one with grandchildren */
 	ut_assertok(uclass_get_device(UCLASS_TEST, 5, &dev));
 	ut_asserteq_ptr(dev, top[5]);
-	ut_assertok(device_remove(dev));
+	ut_assertok(device_remove(dev, DM_REMOVE_NORMAL));
 	ut_asserteq(1 + NODE_COUNT * (1 + NODE_COUNT),
 		    dm_testdrv_op_count[DM_TEST_OP_REMOVE]);
test/dm/eth.c +1 -1
index 6288ae24ab..564ad36916 100644
@@ -116,7 +116,7 @@ static int dm_test_eth_act(struct unit_test_state *uts)
 	for (i = 0; i < DM_TEST_ETH_NUM; i++) {
 		ut_assertok(uclass_find_device_by_name(UCLASS_ETH,
 						       ethname[i], &dev[i]));
-		ut_assertok(device_remove(dev[i]));
+		ut_assertok(device_remove(dev[i], DM_REMOVE_NORMAL));
 
 		/* Invalidate MAC address */
 		strcpy(ethaddr[i], getenv(addrname[i]));
test/dm/spi.c +1 -1
index f52cb733c7..24fa2a48ae 100644
@@ -36,7 +36,7 @@ static int dm_test_spi_find(struct unit_test_state *uts)
 	ut_asserteq(0, uclass_get_device_by_seq(UCLASS_SPI, busnum, &bus));
 	ut_assertok(spi_cs_info(bus, cs, &info));
 	of_offset = dev_of_offset(info.dev);
-	device_remove(info.dev);
+	device_remove(info.dev, DM_REMOVE_NORMAL);
 	device_unbind(info.dev);
 
 	/*
Signature · 1 line
2.12.0
author
#1 lore raw

PATCH v2 2/4 dm: core: Add dm_remove_devices_flags() and hook it into device_remove()

The new function dm_remove_devices_flags() is intented for driver specific last-stage cleanup operations before the OS is started. This patch adds this functionality and hooks it into the common device_remove() function.

Drivers wanting to use this feature for some last-stage removal calls, need to add one of the DM_REMOVE_xx flags to their driver .flags.

Signed-off-by Stefan Roese <[email protected]>
Reviewed-by Simon Glass <[email protected]>

---
v2:
- Added Simons Reviewed-by

drivers/core/device-remove.c | 15 +++++++++++----

 drivers/core/root.c          |  7 +++++++
 include/dm/root.h            | 12 ++++++++++++
 3 files changed, 30 insertions(+), 4 deletions(-)
drivers/core/device-remove.c +11 -4
index b80bf52320..ca4680f7c2 100644
@@ -174,7 +174,12 @@ int device_remove(struct udevice *dev, uint flags)
 	if (ret)
 		goto err;
 
-	if (drv->remove) {
+	/*
+	 * Remove the device if called with the "normal" remove flag set,
+	 * or if the remove flag matches the driver flags
+	 */
+	if (drv->remove &&
+	    ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags))) {
 		ret = drv->remove(dev);
 		if (ret)
 			goto err_remove;

@@ -188,10 +193,12 @@ int device_remove(struct udevice *dev, uint flags)
 		}
 	}
 
-	device_free(dev);
+	if ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags)) {
+		device_free(dev);
 
-	dev->seq = -1;
-	dev->flags &= ~DM_FLAG_ACTIVATED;
+		dev->seq = -1;
+		dev->flags &= ~DM_FLAG_ACTIVATED;
+	}
 
 	return ret;
drivers/core/root.c +7 -0
index 6dafc6ecfc..4c4642a8a4 100644
@@ -182,6 +182,13 @@ int dm_uninit(void)
 	return 0;
 }
 
+int dm_remove_devices_flags(uint flags)
+{
+	device_remove(dm_root(), flags);
+
+	return 0;
+}
+
 int dm_scan_platdata(bool pre_reloc_only)
 {
 	int ret;
include/dm/root.h +12 -0
index 3cf730dcee..b13c005c17 100644
@@ -115,4 +115,16 @@ int dm_init(void);
  */
 int dm_uninit(void);
 
+/**
+ * dm_remove_devices_flags - Call remove function of all drivers with
+ *                           specific removal flags set to selectively
+ *                           remove drivers
+ *
+ * All devices with the matching flags set will be removed
+ *
+ * @flags: Flags for selective device removal
+ * @return 0 if OK, -ve on error
+ */
+int dm_remove_devices_flags(uint flags);
+
 #endif
Signature · 1 line
2.12.0
author
#2 lore raw

PATCH v2 3/4 arm: bootm: Add dm_remove_devices_flags() call to announce_and_cleanup()

In reply to Stefan Roese

This patch adds a call to dm_remove_devices_flags() to announce_and_cleanup() so that drivers that have one of the removal flags set (e.g. DM_FLAG_ACTIVE_DMA_REMOVE) in their driver struct, may do some last-stage cleanup before the OS is started.

Signed-off-by Stefan Roese <[email protected]>
Reviewed-by Simon Glass <[email protected]>

---
v2:
- Added Simons Reviewed-by

 arch/arm/lib/bootm.c | 9 +++++++++
 1 file changed, 9 insertions(+)
arch/arm/lib/bootm.c +9 -0
index 8125cf023f..426bee6da5 100644
@@ -14,6 +14,8 @@
 
 #include <common.h>
 #include <command.h>
+#include <dm/device.h>
+#include <dm/root.h>
 #include <image.h>
 #include <u-boot/zlib.h>
 #include <asm/byteorder.h>

@@ -91,6 +93,13 @@ static void announce_and_cleanup(int fake)
 
 	board_quiesce_devices();
 
+	/*
+	 * Call remove function of all devices with a removal flag set.
+	 * This may be useful for last-stage operations, like cancelling
+	 * of DMA operation or releasing device internal buffers.
+	 */
+	dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
+
 	cleanup_before_linux();
 }
Signature · 1 line
2.12.0
#3 lore raw

PATCH v2 3/4 arm: bootm: Add dm_remove_devices_flags() call to announce_and_cleanup()

Quoted 5 lines · Stefan Roese

This patch adds a call to dm_remove_devices_flags() to announce_and_cleanup() so that drivers that have one of the removal flags set (e.g. DM_FLAG_ACTIVE_DMA_REMOVE) in their driver struct, may do some last-stage cleanup before the OS is started.

Quoted 9 lines
Signed-off-by Stefan Roese <[email protected]>
Reviewed-by Simon Glass <[email protected]>

---
v2:
- Added Simons Reviewed-by

 arch/arm/lib/bootm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Applied to u-boot-dm, thanks!

author
#4 lore raw

PATCH v2 4/4 dm: test: Add test for device removal

In reply to Stefan Roese

Add a test for the correct device removal. Currently two different ways for device removal are supported:

- Normal device removal via the device_remove() API
- Removal via selective device driver flags (DM_FLAG_ACTIVE_DMA)

This new test "remove_active_dma" adds tests cases for those both ways of removal.

Signed-off-by Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>

---
v2:
- New patch in patchset

 test/dm/core.c        | 41 +++++++++++++++++++++++++++++++++++++++++
 test/dm/test-driver.c |  1 +
 2 files changed, 42 insertions(+)
test/dm/core.c +41 -0
index 07b2419ea4..ef85e6b79c 100644
@@ -656,6 +656,47 @@ static int dm_test_pre_reloc(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_pre_reloc, 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_dma(struct unit_test_state *uts)
+{
+	struct dm_test_state *dms = uts->priv;
+	struct udevice *dev;
+
+	ut_assertok(device_bind_by_name(dms->root, false, &driver_info_manual,
+					&dev));
+	ut_assert(dev);
+
+	/* Probe the device */
+	ut_assertok(device_probe(dev));
+
+	/* Test if device is active right now */
+	ut_asserteq(true, device_active(dev));
+
+	/* Remove the device via selective remove flag */
+	dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
+
+	/* Test if device is inactive right now */
+	ut_asserteq(false, device_active(dev));
+
+	/* Probe the device again */
+	ut_assertok(device_probe(dev));
+
+	/* Test if device is active right now */
+	ut_asserteq(true, device_active(dev));
+
+	/* Remove the device via "normal" remove API */
+	ut_assertok(device_remove(dev, DM_REMOVE_NORMAL));
+
+	/* Test if device is inactive right now */
+	ut_asserteq(false, device_active(dev));
+
+	return 0;
+}
+DM_TEST(dm_test_remove_active_dma, 0);
+
 static int dm_test_uclass_before_ready(struct unit_test_state *uts)
 {
 	struct uclass *uc;
test/dm/test-driver.c +1 -0
index d10af51147..1a2932e519 100644
@@ -145,6 +145,7 @@ U_BOOT_DRIVER(test_manual_drv) = {
 	.probe	= test_manual_probe,
 	.remove	= test_manual_remove,
 	.unbind	= test_manual_unbind,
+	.flags	= DM_FLAG_ACTIVE_DMA,
 };
 
 U_BOOT_DRIVER(test_pre_reloc_drv) = {
Signature · 1 line
2.12.0
#5 lore raw

PATCH v2 4/4 dm: test: Add test for device removal

Hi Stefan,

Quoted 50 lines · Stefan Roese

Add a test for the correct device removal. Currently two different ways for device removal are supported:

- Normal device removal via the device_remove() API
- Removal via selective device driver flags (DM_FLAG_ACTIVE_DMA)

This new test "remove_active_dma" adds tests cases for those both ways of removal.

Signed-off-by Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>

---
v2:
- New patch in patchset

 test/dm/core.c        | 41 +++++++++++++++++++++++++++++++++++++++++
 test/dm/test-driver.c |  1 +
 2 files changed, 42 insertions(+)
test/dm/core.c +23 -0
index 07b2419ea4..ef85e6b79c 100644
@@ -656,6 +656,47 @@ static int dm_test_pre_reloc(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_pre_reloc, 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_dma(struct unit_test_state *uts)
+{
+       struct dm_test_state *dms = uts->priv;
+       struct udevice *dev;
+
+       ut_assertok(device_bind_by_name(dms->root, false, &driver_info_manual,
+                                       &dev));
+       ut_assert(dev);
+
+       /* Probe the device */
+       ut_assertok(device_probe(dev));
+
+       /* Test if device is active right now */
+       ut_asserteq(true, device_active(dev));
+
+       /* Remove the device via selective remove flag */
+       dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
+
+       /* Test if device is inactive right now */

test/dm/core.c static int dm_test_pre_reloc(struct unit_test_state *uts) · line 691

Quoted 10 lines
test/dm/core.c +10 -0
+       ut_asserteq(false, device_active(dev));
+
+       /* Probe the device again */
+       ut_assertok(device_probe(dev));
+
+       /* Test if device is active right now */
+       ut_asserteq(true, device_active(dev));
+
+       /* Remove the device via "normal" remove API */
+       ut_assertok(device_remove(dev, DM_REMOVE_NORMAL));

This doesn't actually test a device not getting removed (due to its flag) I think. Can you add a test for that?

Quoted 24 lines
test/dm/core.c +8 -0
+
+       /* Test if device is inactive right now */
+       ut_asserteq(false, device_active(dev));
+
+       return 0;
+}
+DM_TEST(dm_test_remove_active_dma, 0);
+
 static int dm_test_uclass_before_ready(struct unit_test_state *uts)
 {
        struct uclass *uc;
test/dm/test-driver.c +1 -1
index d10af51147..1a2932e519 100644
@@ -145,6 +145,7 @@ U_BOOT_DRIVER(test_manual_drv) = {
        .probe  = test_manual_probe,
        .remove = test_manual_remove,
        .unbind = test_manual_unbind,
+       .flags  = DM_FLAG_ACTIVE_DMA,
 };
 
 U_BOOT_DRIVER(test_pre_reloc_drv) = {
--
Quoted 2 lines

2.12.0

Regards,
Simon

author
#6 lore raw

PATCH v2 4/4 dm: test: Add test for device removal

Hi Simon,

Quoted 56 lines · Simon Glass
Quoted 55 lines · Stefan Roese

Add a test for the correct device removal. Currently two different ways for device removal are supported:

- Normal device removal via the device_remove() API
- Removal via selective device driver flags (DM_FLAG_ACTIVE_DMA)

This new test "remove_active_dma" adds tests cases for those both ways of removal.

Signed-off-by Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>

---
v2:
- New patch in patchset

 test/dm/core.c        | 41 +++++++++++++++++++++++++++++++++++++++++
 test/dm/test-driver.c |  1 +
 2 files changed, 42 insertions(+)
test/dm/core.c +28 -0
index 07b2419ea4..ef85e6b79c 100644
@@ -656,6 +656,47 @@ static int dm_test_pre_reloc(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_pre_reloc, 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_dma(struct unit_test_state *uts)
+{
+       struct dm_test_state *dms = uts->priv;
+       struct udevice *dev;
+
+       ut_assertok(device_bind_by_name(dms->root, false, &driver_info_manual,
+                                       &dev));
+       ut_assert(dev);
+
+       /* Probe the device */
+       ut_assertok(device_probe(dev));
+
+       /* Test if device is active right now */
+       ut_asserteq(true, device_active(dev));
+
+       /* Remove the device via selective remove flag */
+       dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
+
+       /* Test if device is inactive right now */
+       ut_asserteq(false, device_active(dev));
+
+       /* Probe the device again */
+       ut_assertok(device_probe(dev));
+
Quoted 8 lines

test/dm/core.c static int dm_test_pre_reloc(struct unit_test_state *uts) · line 691

Quoted 5 lines
test/dm/core.c +5 -0
+       /* Test if device is active right now */
+       ut_asserteq(true, device_active(dev));
+
+       /* Remove the device via "normal" remove API */
+       ut_assertok(device_remove(dev, DM_REMOVE_NORMAL));

This doesn't actually test a device not getting removed (due to its flag) I think. Can you add a test for that?

Yes, I forgot to add a test for this "non-removal case". Will add in the next version.

Thanks,
Stefan

author
#7 lore raw

PATCH v3 2/4 dm: core: Add dm_remove_devices_flags() and hook it into device_remove()

In reply to Stefan Roese

The new function dm_remove_devices_flags() is intented for driver specific last-stage cleanup operations before the OS is started. This patch adds this functionality and hooks it into the common device_remove() function.

Drivers wanting to use this feature for some last-stage removal calls, need to add one of the DM_REMOVE_xx flags to their driver .flags.

Signed-off-by Stefan Roese <[email protected]>
Reviewed-by Simon Glass <[email protected]>

---
v3:
- Add conditional compilation to fix compilation breakage on platforms without DM and DM_DEVICE_REMOVE support. With this change, Travis compiles all targets without any error

v2:
- Added Simons Reviewed-by

 drivers/core/device-remove.c | 15 +++++++++++----
 drivers/core/root.c          |  9 +++++++++
 include/dm/root.h            | 16 ++++++++++++++++
 3 files changed, 36 insertions(+), 4 deletions(-)
drivers/core/device-remove.c +11 -4
index b80bf52320..ca4680f7c2 100644
@@ -174,7 +174,12 @@ int device_remove(struct udevice *dev, uint flags)
 	if (ret)
 		goto err;
 
-	if (drv->remove) {
+	/*
+	 * Remove the device if called with the "normal" remove flag set,
+	 * or if the remove flag matches the driver flags
+	 */
+	if (drv->remove &&
+	    ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags))) {
 		ret = drv->remove(dev);
 		if (ret)
 			goto err_remove;

@@ -188,10 +193,12 @@ int device_remove(struct udevice *dev, uint flags)
 		}
 	}
 
-	device_free(dev);
+	if ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags)) {
+		device_free(dev);
 
-	dev->seq = -1;
-	dev->flags &= ~DM_FLAG_ACTIVATED;
+		dev->seq = -1;
+		dev->flags &= ~DM_FLAG_ACTIVATED;
+	}
 
 	return ret;
drivers/core/root.c +9 -0
index 4d2033bfad..deefd00687 100644
@@ -182,6 +182,15 @@ int dm_uninit(void)
 	return 0;
 }
 
+#if CONFIG_IS_ENABLED(DM_DEVICE_REMOVE)
+int dm_remove_devices_flags(uint flags)
+{
+	device_remove(dm_root(), flags);
+
+	return 0;
+}
+#endif
+
 int dm_scan_platdata(bool pre_reloc_only)
 {
 	int ret;
include/dm/root.h +16 -0
index 3cf730dcee..058eb98923 100644
@@ -115,4 +115,20 @@ int dm_init(void);
  */
 int dm_uninit(void);
 
+#if CONFIG_IS_ENABLED(DM_DEVICE_REMOVE)
+/**
+ * dm_remove_devices_flags - Call remove function of all drivers with
+ *                           specific removal flags set to selectively
+ *                           remove drivers
+ *
+ * All devices with the matching flags set will be removed
+ *
+ * @flags: Flags for selective device removal
+ * @return 0 if OK, -ve on error
+ */
+int dm_remove_devices_flags(uint flags);
+#else
+static inline int dm_remove_devices_flags(uint flags) { return 0; }
+#endif
+
 #endif
Signature · 1 line
2.12.0
#8 lore raw

PATCH v3 2/4 dm: core: Add dm_remove_devices_flags() and hook it into device_remove()

Hi Stefan,

Quoted 29 lines · Stefan Roese

The new function dm_remove_devices_flags() is intented for driver specific last-stage cleanup operations before the OS is started. This patch adds this functionality and hooks it into the common device_remove() function.

Drivers wanting to use this feature for some last-stage removal calls, need to add one of the DM_REMOVE_xx flags to their driver .flags.

Signed-off-by Stefan Roese <[email protected]>
Reviewed-by Simon Glass <[email protected]>

---
v3:
- Add conditional compilation to fix compilation breakage on platforms without DM and DM_DEVICE_REMOVE support. With this change, Travis compiles all targets without any error

v2:
- Added Simons Reviewed-by

 drivers/core/device-remove.c | 15 +++++++++++----
 drivers/core/root.c          |  9 +++++++++
 include/dm/root.h            | 16 ++++++++++++++++
 3 files changed, 36 insertions(+), 4 deletions(-)
drivers/core/device-remove.c +0 -0
index b80bf52320..ca4680f7c2 100644

drivers/core/device-remove.c int device_remove(struct udevice *dev, uint flags) · line 182

Quoted 10 lines
drivers/core/device-remove.c +6 -1
@@ -174,7 +174,12 @@ int device_remove(struct udevice *dev, uint flags)
        if (ret)
                goto err;
 
-       if (drv->remove) {
+       /*
+        * Remove the device if called with the "normal" remove flag set,
+        * or if the remove flag matches the driver flags
+        */
+       if (drv->remove &&
+           ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags))) {

You are changing the condition here but still call the post_remove() method immediately below. If you decide note to remove the device then I think you should not call that function.

Quoted 9 lines
drivers/core/device-remove.c +1 -1
                ret = drv->remove(dev);
                if (ret)
                        goto err_remove;

@@ -188,10 +193,12 @@ int device_remove(struct udevice *dev, uint flags)
                }
        }
 
-       device_free(dev);
+       if ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags)) {

drivers/core/device-remove.c int device_remove(struct udevice *dev, uint flags) · line 204

Quoted 10 lines
drivers/core/device-remove.c +4 -2
+               device_free(dev);
 
-       dev->seq = -1;
-       dev->flags &= ~DM_FLAG_ACTIVATED;
+               dev->seq = -1;
+               dev->flags &= ~DM_FLAG_ACTIVATED;
+       }
 
        return ret;

Regards,
Simon

#9 lore raw

PATCH v3 2/4 dm: core: Add dm_remove_devices_flags() and hook it into device_remove()

Hi Stefan,

Quoted 35 lines · Simon Glass

Hi Stefan,

Quoted 32 lines · Stefan Roese

The new function dm_remove_devices_flags() is intented for driver specific last-stage cleanup operations before the OS is started. This patch adds this functionality and hooks it into the common device_remove() function.

Drivers wanting to use this feature for some last-stage removal calls, need to add one of the DM_REMOVE_xx flags to their driver .flags.

Signed-off-by Stefan Roese <[email protected]>
Reviewed-by Simon Glass <[email protected]>

---
v3:
- Add conditional compilation to fix compilation breakage on platforms without DM and DM_DEVICE_REMOVE support. With this change, Travis compiles all targets without any error

v2:
- Added Simons Reviewed-by

 drivers/core/device-remove.c | 15 +++++++++++----
 drivers/core/root.c          |  9 +++++++++
 include/dm/root.h            | 16 ++++++++++++++++
 3 files changed, 36 insertions(+), 4 deletions(-)
drivers/core/device-remove.c +0 -0
index b80bf52320..ca4680f7c2 100644
@@ -174,7 +174,12 @@ int device_remove(struct udevice *dev, uint flags)
        if (ret)
                goto err;
Quoted 7 lines

drivers/core/device-remove.c int device_remove(struct udevice *dev, uint flags) · line 182

Quoted 7 lines
drivers/core/device-remove.c +6 -1
-       if (drv->remove) {
+       /*
+        * Remove the device if called with the "normal" remove flag set,
+        * or if the remove flag matches the driver flags
+        */
+       if (drv->remove &&
+           ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags))) {

This seems to be comparing different things. The DM_REMOVE_NORMAL flag is not from the same enum as drv->flags, is it?

Quoted 16 lines

You are changing the condition here but still call the post_remove() method immediately below. If you decide note to remove the device then I think you should not call that function.

Quoted 11 lines
drivers/core/device-remove.c +2 -1
                ret = drv->remove(dev);
                if (ret)
                        goto err_remove;

@@ -188,10 +193,12 @@ int device_remove(struct udevice *dev, uint flags)
                }
        }
 
-       device_free(dev);
+       if ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags)) {
+               device_free(dev);
Quoted 7 lines

drivers/core/device-remove.c int device_remove(struct udevice *dev, uint flags) · line 203

Quoted 7 lines
drivers/core/device-remove.c +3 -2
-       dev->seq = -1;
-       dev->flags &= ~DM_FLAG_ACTIVATED;
+               dev->seq = -1;
+               dev->flags &= ~DM_FLAG_ACTIVATED;
+       }
 
        return ret;

Regards,
Simon

author
#10 lore raw

PATCH v3 2/4 dm: core: Add dm_remove_devices_flags() and hook it into device_remove()

Hi Simon,

Quoted 36 lines · Simon Glass
Quoted 35 lines · Simon Glass

Hi Stefan,

Quoted 32 lines · Stefan Roese

The new function dm_remove_devices_flags() is intented for driver specific last-stage cleanup operations before the OS is started. This patch adds this functionality and hooks it into the common device_remove() function.

Drivers wanting to use this feature for some last-stage removal calls, need to add one of the DM_REMOVE_xx flags to their driver .flags.

Signed-off-by Stefan Roese <[email protected]>
Reviewed-by Simon Glass <[email protected]>

---
v3:
- Add conditional compilation to fix compilation breakage on platforms without DM and DM_DEVICE_REMOVE support. With this change, Travis compiles all targets without any error

v2:
- Added Simons Reviewed-by

 drivers/core/device-remove.c | 15 +++++++++++----
 drivers/core/root.c          |  9 +++++++++
 include/dm/root.h            | 16 ++++++++++++++++
 3 files changed, 36 insertions(+), 4 deletions(-)
drivers/core/device-remove.c +0 -0
index b80bf52320..ca4680f7c2 100644
@@ -174,7 +174,12 @@ int device_remove(struct udevice *dev, uint flags)
        if (ret)
                goto err;
Quoted 10 lines
Quoted 7 lines

drivers/core/device-remove.c int device_remove(struct udevice *dev, uint flags) · line 182

Quoted 7 lines
drivers/core/device-remove.c +6 -1
-       if (drv->remove) {
+       /*
+        * Remove the device if called with the "normal" remove flag set,
+        * or if the remove flag matches the driver flags
+        */
+       if (drv->remove &&
+           ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags))) {

This seems to be comparing different things. The DM_REMOVE_NORMAL flag is not from the same enum as drv->flags, is it?

I have to admit that this comparison above is a bit "vague". I'll make it more explicit in the next patch version.

Thanks,
Stefan

#11 lore raw

PATCH v2 1/4 dm: core: Add flags parameter to device_remove()

In reply to Stefan Roese
Quoted 28 lines · Stefan Roese

This patch adds the flags parameter to device_remove() and changes all calls to this function to provide the default value of DM_REMOVE_NORMAL for "normal" device removal.

This is in preparation for the driver specific pre-OS (e.g. DMA cancelling) remove support.

Signed-off-by Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>

---
v2:
- Changes DM_FLAG macro as suggested by Simon
- Minor comment change as suggested by Simon

 arch/x86/cpu/queensbay/tnc.c   |  4 ++--
 cmd/cros_ec.c                  |  2 +-
 cmd/sf.c                       |  2 +-
 drivers/block/blk-uclass.c     |  2 +-
 drivers/block/sandbox.c        |  2 +-
 drivers/core/device-remove.c   |  9 +++++----
 drivers/core/device.c          |  2 +-
 drivers/core/root.c            |  2 +-
 drivers/core/uclass.c          |  2 +-
 drivers/mmc/mmc-uclass.c       |  2 +-
 drivers/mtd/spi/sandbox.c      |  2 +-
 drivers/mtd/spi/sf-uclass.c    |  2 +-
 drivers/spi/spi-uclass.c       |  4 ++--
 drivers/usb/emul/sandbox_hub.c |  2 +-
Quoted 9 lines
 drivers/usb/host/usb-uclass.c  |  4 ++--
 include/dm/device-internal.h   |  5 +++--
 include/dm/device.h            | 26 ++++++++++++++++++++++++++
 test/dm/bus.c                  |  8 ++++----
 test/dm/core.c                 | 16 ++++++++--------
 test/dm/eth.c                  |  2 +-
 test/dm/spi.c                  |  2 +-
 21 files changed, 65 insertions(+), 37 deletions(-)
Acked-by Simon Glass <[email protected]>
#12 lore raw

PATCH v2 1/4 dm: core: Add flags parameter to device_remove()

Quoted 39 lines · Simon Glass
Quoted 28 lines · Stefan Roese

This patch adds the flags parameter to device_remove() and changes all calls to this function to provide the default value of DM_REMOVE_NORMAL for "normal" device removal.

This is in preparation for the driver specific pre-OS (e.g. DMA cancelling) remove support.

Signed-off-by Stefan Roese <[email protected]>
Cc Simon Glass <[email protected]>

---
v2:
- Changes DM_FLAG macro as suggested by Simon
- Minor comment change as suggested by Simon

 arch/x86/cpu/queensbay/tnc.c   |  4 ++--
 cmd/cros_ec.c                  |  2 +-
 cmd/sf.c                       |  2 +-
 drivers/block/blk-uclass.c     |  2 +-
 drivers/block/sandbox.c        |  2 +-
 drivers/core/device-remove.c   |  9 +++++----
 drivers/core/device.c          |  2 +-
 drivers/core/root.c            |  2 +-
 drivers/core/uclass.c          |  2 +-
 drivers/mmc/mmc-uclass.c       |  2 +-
 drivers/mtd/spi/sandbox.c      |  2 +-
 drivers/mtd/spi/sf-uclass.c    |  2 +-
 drivers/spi/spi-uclass.c       |  4 ++--
 drivers/usb/emul/sandbox_hub.c |  2 +-
Quoted 9 lines
 drivers/usb/host/usb-uclass.c  |  4 ++--
 include/dm/device-internal.h   |  5 +++--
 include/dm/device.h            | 26 ++++++++++++++++++++++++++
 test/dm/bus.c                  |  8 ++++----
 test/dm/core.c                 | 16 ++++++++--------
 test/dm/eth.c                  |  2 +-
 test/dm/spi.c                  |  2 +-
 21 files changed, 65 insertions(+), 37 deletions(-)
Quoted 1 line
Acked-by Simon Glass <[email protected]>

Applied to u-boot-dm, thanks!