reset: Add support for shared reset controls

PATCH Superseded Hans de Goede 4 participants 15 messages Jan 27, 2016 – Feb 8, 2016 Reset controller framework

lore

Share & tools

lore

Hans de Goede

author

2016-01-27 18:15 UTC #0 lore raw

v3 cover reset: Add support for shared reset controls

Hi All,

Here is a new version (rewrite) of my shared reset control support patch-set implementing the new API for this discussed on the list.

Regards,

Hans

Hans de Goede

author

2016-01-27 18:15 UTC #1 lore raw

PATCH v3 1/3 reset: Make [of_]reset_control_get[_foo] functions wrappers

With both the regular, _by_index and _optional variants we already have quite a few variants of [of_]reset_control_get[_foo], the upcoming addition of shared reset lines support makes this worse.

This commit changes all the variants into wrappers around common core functions. For completeness sake this commit also adds a new devm_get_reset_control_by_index wrapper.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/reset/core.c  |  84 +++++++--------------------------
 include/linux/reset.h | 126 +++++++++++++++++++++++++++++++++++---------------
 2 files changed, 107 insertions(+), 103 deletions(-)

drivers/reset/core.c +17 ~~-67~~

index 8737663..f695429 100644
@@ -139,18 +139,8 @@ int reset_control_status(struct reset_control *rstc)
 }
 EXPORT_SYMBOL_GPL(reset_control_status);
 
-/**
- * of_reset_control_get_by_index - Lookup and obtain a reference to a reset
- * controller by index.
- * @node: device to be reset by the controller
- * @index: index of the reset controller
- *
- * This is to be used to perform a list of resets for a device or power domain
- * in whatever order. Returns a struct reset_control or IS_ERR() condition
- * containing errno.
- */
-struct reset_control *of_reset_control_get_by_index(struct device_node *node,
-					   int index)
+struct reset_control *__of_reset_control_get(struct device_node *node,
+					     const char *id, int index)
 {
 	struct reset_control *rstc = ERR_PTR(-EPROBE_DEFER);
 	struct reset_controller_dev *r, *rcdev;

@@ -158,6 +148,16 @@ struct reset_control *of_reset_control_get_by_index(struct device_node *node,
 	int rstc_id;
 	int ret;
 
+	if (!node)
+		return ERR_PTR(-EINVAL);
+
+	if (id) {
+		index = of_property_match_string(node,
+						 "reset-names", id);
+		if (index < 0)
+			return ERR_PTR(-ENOENT);
+	}
+
 	ret = of_parse_phandle_with_args(node, "resets", "#reset-cells",
 					 index, &args);
 	if (ret)

@@ -198,49 +198,7 @@ struct reset_control *of_reset_control_get_by_index(struct device_node *node,
 
 	return rstc;
 }
-EXPORT_SYMBOL_GPL(of_reset_control_get_by_index);
-
-/**
- * of_reset_control_get - Lookup and obtain a reference to a reset controller.
- * @node: device to be reset by the controller
- * @id: reset line name
- *
- * Returns a struct reset_control or IS_ERR() condition containing errno.
- *
- * Use of id names is optional.
- */
-struct reset_control *of_reset_control_get(struct device_node *node,
-					   const char *id)
-{
-	int index = 0;
-
-	if (id) {
-		index = of_property_match_string(node,
-						 "reset-names", id);
-		if (index < 0)
-			return ERR_PTR(-ENOENT);
-	}
-	return of_reset_control_get_by_index(node, index);
-}
-EXPORT_SYMBOL_GPL(of_reset_control_get);
-
-/**
- * reset_control_get - Lookup and obtain a reference to a reset controller.
- * @dev: device to be reset by the controller
- * @id: reset line name
- *
- * Returns a struct reset_control or IS_ERR() condition containing errno.
- *
- * Use of id names is optional.
- */
-struct reset_control *reset_control_get(struct device *dev, const char *id)
-{
-	if (!dev)
-		return ERR_PTR(-EINVAL);
-
-	return of_reset_control_get(dev->of_node, id);
-}
-EXPORT_SYMBOL_GPL(reset_control_get);
+EXPORT_SYMBOL_GPL(__of_reset_control_get);
 
 /**
  * reset_control_put - free the reset controller

@@ -262,16 +220,8 @@ static void devm_reset_control_release(struct device *dev, void *res)
 	reset_control_put(*(struct reset_control **)res);
 }
 
-/**
- * devm_reset_control_get - resource managed reset_control_get()
- * @dev: device to be reset by the controller
- * @id: reset line name
- *
- * Managed reset_control_get(). For reset controllers returned from this
- * function, reset_control_put() is called automatically on driver detach.
- * See reset_control_get() for more information.
- */
-struct reset_control *devm_reset_control_get(struct device *dev, const char *id)
+struct reset_control *__devm_reset_control_get(struct device *dev,
+					       const char *id, int index)
 {
 	struct reset_control **ptr, *rstc;

@@ -280,7 +230,7 @@ struct reset_control *devm_reset_control_get(struct device *dev, const char *id)
 	if (!ptr)
 		return ERR_PTR(-ENOMEM);
 
-	rstc = reset_control_get(dev, id);
+	rstc = __of_reset_control_get(dev ? dev->of_node : NULL, id, index);
 	if (!IS_ERR(rstc)) {
 		*ptr = rstc;
 		devres_add(dev, ptr);

@@ -290,7 +240,7 @@ struct reset_control *devm_reset_control_get(struct device *dev, const char *id)
 
 	return rstc;
 }
-EXPORT_SYMBOL_GPL(devm_reset_control_get);
+EXPORT_SYMBOL_GPL(__devm_reset_control_get);
 
 /**
  * device_reset - find reset controller associated with the device

include/linux/reset.h +90 ~~-36~~

175 lines not on this page · raw message

Signature · 1 line

2.5.0

Hans de Goede

author

2016-02-05 18:30 UTC #3 lore raw

PATCH v3 1/3 reset: Make [of_]reset_control_get[_foo] functions wrappers

Hi,

Quoted 25 lines · Philipp Zabel

Hi Hans,
Am Mittwoch, den 27.01.2016, 19:15 +0100 schrieb Hans de Goede:
[...]
Quoted 7 lines
include/linux/reset.h +7 -0
+/**
+ * reset_control_get - Lookup and obtain a reference to a reset controller.
+ * @dev: device to be reset by the controller
+ * @id: reset line name
+ *
+ * Returns a struct reset_control or IS_ERR() condition containing errno.
+ *
include/linux/reset.h static inline int device_reset_optional(struct device *dev) · line 95
Quoted 10 lines
include/linux/reset.h +7 -1
+ * Use of id names is optional.
+ */
+static inline struct reset_control *__must_check reset_control_get(
  					struct device *dev, const char *id)
  {
-	return ERR_PTR(-ENOTSUPP);
+#ifndef CONFIG_RESET_CONTROLLER
+	WARN_ON(1);
+#endif
+	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0);
Even though we are device tree only at this point, I'd prefer to keep an exported function that takes a struct device argument, for example:

Quoted 1 line

	return __reset_control_get(dev, id, 0);

I don't really see a reason to do this, this is not userspace abi or some-such we can always later re-introduce an exported function which takes a device argument. But if you insist, you're the boss :) Let me know if you really want me to make this change for the next version, and I'll add it to v4 of this set.

Regards,

Hans

Philipp Zabel

maintainer

2016-02-08 09:58 UTC #4 lore raw

PATCH v3 1/3 reset: Make [of_]reset_control_get[_foo] functions wrappers

Am Freitag, den 05.02.2016, 19:30 +0100 schrieb Hans de Goede:
[...]

Quoted 8 lines

Quoted 4 lines
Even though we are device tree only at this point, I'd prefer to keep an exported function that takes a struct device argument, for example:
	return __reset_control_get(dev, id, 0);
I don't really see a reason to do this, this is not userspace abi or some-such we can always later re-introduce an exported function which takes a device argument. But if you insist, you're the boss :)

s/boss/janitor/ more like

Quoted 3 lines

Let me know if you really
want me to make this change for the next version, and I'll add it to v4 of this set.

Leave it as is. I have a patch to add it back, and yes it looks a bit superfluous on its own. I'll add apply that only once its actually needed.

regards
Philipp

Hans de Goede

author

2016-01-27 18:15 UTC #5 lore raw

PATCH v3 2/3 reset: Share struct reset_control between reset_control_get calls

In reply to Hans de Goede

Now that struct reset_control no longer stores the device pointer for the device calling reset_control_get we can share a single struct reset_control when multiple calls to reset_control_get are made for the same reset line (same id / index).

This is a preparation patch for adding support for shared reset lines.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/reset/core.c             | 82 ++++++++++++++++++++++++++++++----------
 include/linux/reset-controller.h |  2 +
 2 files changed, 64 insertions(+), 20 deletions(-)

drivers/reset/core.c +62 ~~-20~~

index f695429..e439ae2 100644
@@ -18,19 +18,23 @@
 #include <linux/reset-controller.h>
 #include <linux/slab.h>
 
-static DEFINE_MUTEX(reset_controller_list_mutex);
+static DEFINE_MUTEX(reset_list_mutex);
 static LIST_HEAD(reset_controller_list);
 
 /**
  * struct reset_control - a reset control
  * @rcdev: a pointer to the reset controller device
  *         this reset control belongs to
+ * @list: list entry for the rcdev's reset controller list
  * @id: ID of the reset controller in the reset
  *      controller device
+ * @refcnt: Number of gets of this reset_control
  */
 struct reset_control {
 	struct reset_controller_dev *rcdev;
+	struct list_head list;
 	unsigned int id;
+	unsigned int refcnt;
 };
 
 /**

@@ -65,9 +69,11 @@ int reset_controller_register(struct reset_controller_dev *rcdev)
 		rcdev->of_xlate = of_reset_simple_xlate;
 	}
 
-	mutex_lock(&reset_controller_list_mutex);
+	INIT_LIST_HEAD(&rcdev->reset_control_head);
+
+	mutex_lock(&reset_list_mutex);
 	list_add(&rcdev->list, &reset_controller_list);
-	mutex_unlock(&reset_controller_list_mutex);
+	mutex_unlock(&reset_list_mutex);
 
 	return 0;
 }

@@ -79,9 +85,9 @@ EXPORT_SYMBOL_GPL(reset_controller_register);
  */
 void reset_controller_unregister(struct reset_controller_dev *rcdev)
 {
-	mutex_lock(&reset_controller_list_mutex);
+	mutex_lock(&reset_list_mutex);
 	list_del(&rcdev->list);
-	mutex_unlock(&reset_controller_list_mutex);
+	mutex_unlock(&reset_list_mutex);
 }
 EXPORT_SYMBOL_GPL(reset_controller_unregister);

@@ -139,6 +145,48 @@ int reset_control_status(struct reset_control *rstc)
 }
 EXPORT_SYMBOL_GPL(reset_control_status);
 
+static struct reset_control *__reset_control_get(
+				struct reset_controller_dev *rcdev,
+				unsigned int index)
+{
+	struct reset_control *rstc;
+
+	lockdep_assert_held(&reset_list_mutex);
+
+	list_for_each_entry(rstc, &rcdev->reset_control_head, list) {
+		if (rstc->id == index) {
+			rstc->refcnt++;
+			return rstc;
+		}
+	}
+
+	rstc = kzalloc(sizeof(*rstc), GFP_KERNEL);
+	if (!rstc)
+		return ERR_PTR(-ENOMEM);
+
+	try_module_get(rcdev->owner);
+
+	rstc->rcdev = rcdev;
+	list_add(&rstc->list, &rcdev->reset_control_head);
+	rstc->id = index;
+	rstc->refcnt = 1;
+
+	return rstc;
+}
+
+static void __reset_control_put(struct reset_control *rstc)
+{
+	lockdep_assert_held(&reset_list_mutex);
+
+	if (--rstc->refcnt)
+		return;
+
+	module_put(rstc->rcdev->owner);
+
+	list_del(&rstc->list);
+	kfree(rstc);
+}
+
 struct reset_control *__of_reset_control_get(struct device_node *node,
 					     const char *id, int index)
 {

@@ -163,7 +211,7 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
 	if (ret)
 		return ERR_PTR(ret);
 
-	mutex_lock(&reset_controller_list_mutex);
+	mutex_lock(&reset_list_mutex);
 	rcdev = NULL;
 	list_for_each_entry(r, &reset_controller_list, list) {
 		if (args.np == r->of_node) {

@@ -174,27 +222,20 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
 	of_node_put(args.np);
 
 	if (!rcdev) {
-		mutex_unlock(&reset_controller_list_mutex);
+		mutex_unlock(&reset_list_mutex);
 		return ERR_PTR(-EPROBE_DEFER);
 	}
 
 	rstc_id = rcdev->of_xlate(rcdev, &args);
 	if (rstc_id < 0) {
-		mutex_unlock(&reset_controller_list_mutex);
+		mutex_unlock(&reset_list_mutex);
 		return ERR_PTR(rstc_id);
 	}
 
-	try_module_get(rcdev->owner);
-	mutex_unlock(&reset_controller_list_mutex);
-
-	rstc = kzalloc(sizeof(*rstc), GFP_KERNEL);
-	if (!rstc) {
-		module_put(rcdev->owner);
-		return ERR_PTR(-ENOMEM);
-	}
+	/* reset_list_mutex also protects the rcdev's reset_control list */
+	rstc = __reset_control_get(rcdev, rstc_id);
 
-	rstc->rcdev = rcdev;
-	rstc->id = rstc_id;
+	mutex_unlock(&reset_list_mutex);
 
 	return rstc;
 }

@@ -210,8 +251,9 @@ void reset_control_put(struct reset_control *rstc)
 	if (IS_ERR(rstc))
 		return;
 
-	module_put(rstc->rcdev->owner);
-	kfree(rstc);
+	mutex_lock(&reset_list_mutex);
+	__reset_control_put(rstc);
+	mutex_unlock(&reset_list_mutex);
 }
 EXPORT_SYMBOL_GPL(reset_control_put);

include/linux/reset-controller.h +2 -0

index ce6b962..2decc20 100644
@@ -31,6 +31,7 @@ struct of_phandle_args;
  * @ops: a pointer to device specific struct reset_control_ops
  * @owner: kernel module of the reset controller driver
  * @list: internal list of reset controller devices
+ * @reset_control_head: head of internal list of requested reset controls
  * @of_node: corresponding device tree node as phandle target
  * @of_reset_n_cells: number of cells in reset line specifiers
  * @of_xlate: translation function to translate from specifier as found in the

@@ -41,6 +42,7 @@ struct reset_controller_dev {
 	struct reset_control_ops *ops;
 	struct module *owner;
 	struct list_head list;
+	struct list_head reset_control_head;
 	struct device_node *of_node;
 	int of_reset_n_cells;
 	int (*of_xlate)(struct reset_controller_dev *rcdev,

Signature · 1 line

2.5.0

Hans de Goede

author

2016-01-27 18:15 UTC #6 lore raw

PATCH v3 3/3 reset: Add support for shared reset controls

In reply to Hans de Goede

In some SoCs some hw-blocks share a reset control. Add support for this setup by adding new:

reset_control_get_shared()
devm_reset_control_get_shared()
devm_reset_control_get_shared_by_index()

methods to get a reset_control. Note that this patch omits adding of_ variants, if these are needed later they can be easily added.

This patch also changes the behavior of the existing exclusive reset_control_get() variants, if these are now called more then once for the same reset_control they will return -EBUSY. To catch existing drivers triggering this error (there should not be any) a WARN_ON(1) is added in this path.

When a reset_control is shared, the behavior of reset_control_assert / deassert is changed, for shared reset_controls these will work like the clock-enable/disable and regulator-on/off functions. They will keep a deassert_count, and only (re-)assert the reset after reset_control_assert has been called as many times as reset_control_deassert was called.

Calling reset_control_assert without first calling reset_control_deassert is not allowed on a shared reset control. Calling reset_control_reset is also not allowed on a shared reset control.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/reset/core.c  | 61 ++++++++++++++++++++++++++++++++-------
 include/linux/reset.h | 79 +++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 114 insertions(+), 26 deletions(-)

drivers/reset/core.c +50 ~~-11~~

index e439ae2..5554585 100644
@@ -8,6 +8,7 @@
  * the Free Software Foundation; either version 2 of the License, or
  * (at your option) any later version.
  */
+#include <linux/atomic.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/export.h>

@@ -29,12 +30,16 @@ static LIST_HEAD(reset_controller_list);
  * @id: ID of the reset controller in the reset
  *      controller device
  * @refcnt: Number of gets of this reset_control
+ * @shared: Is this a shared (1), or an exclusive (0) reset_control?
+ * @deassert_cnt: Number of times this reset line has been deasserted
  */
 struct reset_control {
 	struct reset_controller_dev *rcdev;
 	struct list_head list;
 	unsigned int id;
 	unsigned int refcnt;
+	int shared;
+	atomic_t deassert_count;
 };
 
 /**

@@ -94,9 +99,16 @@ EXPORT_SYMBOL_GPL(reset_controller_unregister);
 /**
  * reset_control_reset - reset the controlled device
  * @rstc: reset controller
+ *
+ * Calling this on a shared reset controller is an error.
  */
 int reset_control_reset(struct reset_control *rstc)
 {
+	if (rstc->shared) {
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
 	if (rstc->rcdev->ops->reset)
 		return rstc->rcdev->ops->reset(rstc->rcdev, rstc->id);

@@ -107,26 +119,47 @@ EXPORT_SYMBOL_GPL(reset_control_reset);
 /**
  * reset_control_assert - asserts the reset line
  * @rstc: reset controller
+ *
+ * Calling this on an exclusive reset controller guarantees that the reset
+ * will be asserted. When called on a shared reset controller the line may
+ * still be deasserted, as long as other users keep it so.
  */
 int reset_control_assert(struct reset_control *rstc)
 {
-	if (rstc->rcdev->ops->assert)
-		return rstc->rcdev->ops->assert(rstc->rcdev, rstc->id);
+	if (!rstc->rcdev->ops->assert)
+		return -ENOTSUPP;
 
-	return -ENOTSUPP;
+	if (rstc->shared) {
+		if (atomic_read(&rstc->deassert_count) == 0) {
+			WARN_ON(1);
+			return -EINVAL;
+		}
+
+		if (atomic_dec_return(&rstc->deassert_count) != 0)
+			return 0;
+	}
+
+	return rstc->rcdev->ops->assert(rstc->rcdev, rstc->id);
 }
 EXPORT_SYMBOL_GPL(reset_control_assert);
 
 /**
  * reset_control_deassert - deasserts the reset line
  * @rstc: reset controller
+ *
+ * After calling this function, the reset is guaranteed to be deasserted.
  */
 int reset_control_deassert(struct reset_control *rstc)
 {
-	if (rstc->rcdev->ops->deassert)
-		return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->id);
+	if (!rstc->rcdev->ops->deassert)
+		return -ENOTSUPP;
 
-	return -ENOTSUPP;
+	if (rstc->shared) {
+		if (atomic_inc_return(&rstc->deassert_count) != 1)
+			return 0;
+	}
+
+	return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->id);
 }
 EXPORT_SYMBOL_GPL(reset_control_deassert);

@@ -147,7 +180,7 @@ EXPORT_SYMBOL_GPL(reset_control_status);
 
 static struct reset_control *__reset_control_get(
 				struct reset_controller_dev *rcdev,
-				unsigned int index)
+				unsigned int index, int shared)
 {
 	struct reset_control *rstc;

@@ -155,6 +188,10 @@ static struct reset_control *__reset_control_get(
 
 	list_for_each_entry(rstc, &rcdev->reset_control_head, list) {
 		if (rstc->id == index) {
+			if (!rstc->shared || !shared) {
+				WARN_ON(1);
+				return ERR_PTR(-EBUSY);
+			}
 			rstc->refcnt++;
 			return rstc;
 		}

@@ -170,6 +207,7 @@ static struct reset_control *__reset_control_get(
 	list_add(&rstc->list, &rcdev->reset_control_head);
 	rstc->id = index;
 	rstc->refcnt = 1;
+	rstc->shared = shared;
 
 	return rstc;
 }

@@ -188,7 +226,7 @@ static void __reset_control_put(struct reset_control *rstc)
 }
 
 struct reset_control *__of_reset_control_get(struct device_node *node,
-					     const char *id, int index)
+				     const char *id, int index, int shared)
 {
 	struct reset_control *rstc = ERR_PTR(-EPROBE_DEFER);
 	struct reset_controller_dev *r, *rcdev;

@@ -233,7 +271,7 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
 	}
 
 	/* reset_list_mutex also protects the rcdev's reset_control list */
-	rstc = __reset_control_get(rcdev, rstc_id);
+	rstc = __reset_control_get(rcdev, rstc_id, shared);
 
 	mutex_unlock(&reset_list_mutex);

@@ -263,7 +301,7 @@ static void devm_reset_control_release(struct device *dev, void *res)
 }
 
 struct reset_control *__devm_reset_control_get(struct device *dev,
-					       const char *id, int index)
+				     const char *id, int index, int shared)
 {
 	struct reset_control **ptr, *rstc;

@@ -272,7 +310,8 @@ struct reset_control *__devm_reset_control_get(struct device *dev,
 	if (!ptr)
 		return ERR_PTR(-ENOMEM);
 
-	rstc = __of_reset_control_get(dev ? dev->of_node : NULL, id, index);
+	rstc = __of_reset_control_get(dev ? dev->of_node : NULL,
+				      id, index, shared);
 	if (!IS_ERR(rstc)) {
 		*ptr = rstc;
 		devres_add(dev, ptr);

include/linux/reset.h +64 ~~-15~~

151 lines not on this page · raw message

Signature · 1 line

2.5.0

Stephen Warren

2016-01-28 20:20 UTC #7 lore raw

PATCH v3 3/3 reset: Add support for shared reset controls

Quoted 22 lines · Hans de Goede

In some SoCs some hw-blocks share a reset control. Add support for this setup by adding new:
reset_control_get_shared()
devm_reset_control_get_shared()
devm_reset_control_get_shared_by_index()
methods to get a reset_control. Note that this patch omits adding of_ variants, if these are needed later they can be easily added.
This patch also changes the behavior of the existing exclusive reset_control_get() variants, if these are now called more then once for the same reset_control they will return -EBUSY. To catch existing drivers triggering this error (there should not be any) a WARN_ON(1) is added in this path.
When a reset_control is shared, the behavior of reset_control_assert / deassert is changed, for shared reset_controls these will work like the clock-enable/disable and regulator-on/off functions. They will keep a deassert_count, and only (re-)assert the reset after reset_control_assert has been called as many times as reset_control_deassert was called.

Quoted 3 lines

Calling reset_control_assert without first calling reset_control_deassert is not allowed on a shared reset control. Calling reset_control_reset is also not allowed on a shared reset control.

Hmmm. Do you have some examples of how drivers are supposed to co-ordinate use of the shared reset? Reference counting implies all the drivers need to get together and all assert and de-assert the reset in unison when some event happens. That seems difficult to co-ordinate.

Instead, would it be better to require some central device to exclusively get the reset, and manage it. That device could then react to appropriate events to manage the reset. In this scheme, we wouldn't need the concept of shared resets at all. However, depending on why/when the reset needed to be re-asserted at run-time, this scheme wouldn't eliminate the co-ordination issue, but equally I don't believe it makes it any worse.

Hans de Goede

author

2016-01-29 06:18 UTC #8 lore raw

PATCH v3 3/3 reset: Add support for shared reset controls

Hi,

Quoted 24 lines · Stephen Warren

Quoted 22 lines · Hans de Goede
In some SoCs some hw-blocks share a reset control. Add support for this setup by adding new:
reset_control_get_shared()
devm_reset_control_get_shared()
devm_reset_control_get_shared_by_index()
methods to get a reset_control. Note that this patch omits adding of_ variants, if these are needed later they can be easily added.
This patch also changes the behavior of the existing exclusive reset_control_get() variants, if these are now called more then once for the same reset_control they will return -EBUSY. To catch existing drivers triggering this error (there should not be any) a WARN_ON(1) is added in this path.
When a reset_control is shared, the behavior of reset_control_assert / deassert is changed, for shared reset_controls these will work like the clock-enable/disable and regulator-on/off functions. They will keep a deassert_count, and only (re-)assert the reset after reset_control_assert has been called as many times as reset_control_deassert was called.

Quoted 5 lines

Quoted 3 lines
Calling reset_control_assert without first calling reset_control_deassert is not allowed on a shared reset control. Calling reset_control_reset is also not allowed on a shared reset control.
Hmmm. Do you have some examples of how drivers are supposed to co-ordinate use of the shared reset? Reference counting implies all the drivers need to get together and all assert and de-assert the reset in unison when some event happens. That seems difficult to co-ordinate.

Right, for now this is only intended for drivers which need to de-assert the reset and probe time and (re-)assert it when unbound (and maybe for suspend / resume, in which case the driver should be able to handle the reset never heaving been asserted on resume). This is also why reset_control_reset is not supported for shared resets.

I hope that we will never see a case where a reset is actually needed for error handling (so used outside of probe / suspend cases) and it is shared.

Quoted 1 line

Instead, would it be better to require some central device to exclusively get the reset, and manage it. That device could then react to appropriate events to manage the reset. In this scheme, we wouldn't need the concept of shared resets at all. However, depending on why/when the reset needed to be re-asserted at run-time, this scheme wouldn't eliminate the co-ordination issue, but equally I don't believe it makes it any worse.

If we ever need this kind of complexity, then yes a central service, with callbacks in to drivers requesting them to prepare for handling reset (like how usb does this) might be the best solution, but lets cross that bridge when we get there.

Regards,

Hans

Stephen Warren

2016-01-29 16:21 UTC #9 lore raw

PATCH v3 3/3 reset: Add support for shared reset controls

Quoted 48 lines · Hans de Goede

Hi,
Quoted 30 lines · Stephen Warren
Quoted 23 lines · Hans de Goede
In some SoCs some hw-blocks share a reset control. Add support for this setup by adding new:
reset_control_get_shared()
devm_reset_control_get_shared()
devm_reset_control_get_shared_by_index()
methods to get a reset_control. Note that this patch omits adding of_ variants, if these are needed later they can be easily added.
This patch also changes the behavior of the existing exclusive reset_control_get() variants, if these are now called more then once for the same reset_control they will return -EBUSY. To catch existing drivers triggering this error (there should not be any) a WARN_ON(1) is added in this path.
When a reset_control is shared, the behavior of reset_control_assert / deassert is changed, for shared reset_controls these will work like the clock-enable/disable and regulator-on/off functions. They will keep a deassert_count, and only (re-)assert the reset after reset_control_assert
has been called as many times as reset_control_deassert was called.
Quoted 4 lines
Calling reset_control_assert without first calling reset_control_deassert
is not allowed on a shared reset control. Calling reset_control_reset is also not allowed on a shared reset control.
Quoted 5 lines
Hmmm. Do you have some examples of how drivers are supposed to co-ordinate use of the shared reset? Reference counting implies all the drivers need to get together and all assert and de-assert the reset in unison when some event happens. That seems difficult to co-ordinate.
Right, for now this is only intended for drivers which need to de-assert the
reset and probe time and (re-)assert it when unbound (and maybe for suspend / resume, in which case the driver should be able to handle the reset
never heaving been asserted on resume). This is also why reset_control_reset
is not supported for shared resets.

Quoted 3 lines

I hope that we will never see a case where a reset is actually needed for error handling (so used outside of probe / suspend cases) and it is shared.

OK, that sounds reasonable.

I guess the assumption is that the reset starts out asserted, and the first de-assert clears the reset signal. What if the default HW state has the reset de-asserted; is there any way for a driver to guarantee that the first deassert call is actually a deassert rather than a no-op? That would be useful so that drivers could assume their module's registers were reset due to the deassert call.

Quoted 8 lines

Quoted 7 lines
Instead, would it be better to require some central device to exclusively get the reset, and manage it. That device could then react to appropriate events to manage the reset. In this scheme, we wouldn't need the concept of shared resets at all. However, depending on why/when the reset needed to be re-asserted at run-time, this scheme wouldn't eliminate the co-ordination issue, but equally I don't believe it makes it any worse.

Quoted 4 lines

If we ever need this kind of complexity, then yes a central service, with callbacks in to drivers requesting them to prepare for handling reset (like how usb does this) might be the best solution, but lets cross that bridge when we get there.

OK. Perhaps it's a good idea to spell out this usage model assumption explicitly somewhere? Perhaps it already is and I simply didn't look in enough detail.

Philipp Zabel

2016-01-30 11:38 UTC #10 lore raw

PATCH v3 3/3 reset: Add support for shared reset controls

In reply to Hans de Goede

Hi Hans,

Quoted 7 lines · Hans de Goede

In some SoCs some hw-blocks share a reset control. Add support for this setup by adding new:
reset_control_get_shared()
devm_reset_control_get_shared()
devm_reset_control_get_shared_by_index()

Quoted 8 lines

methods to get a reset_control. Note that this patch omits adding of_ variants, if these are needed later they can be easily added.
This patch also changes the behavior of the existing exclusive reset_control_get() variants, if these are now called more then once for the same reset_control they will return -EBUSY. To catch existing drivers triggering this error (there should not be any) a WARN_ON(1) is added in this path.

Which is a good thing.

Quoted 11 lines

When a reset_control is shared, the behavior of reset_control_assert / deassert is changed, for shared reset_controls these will work like the clock-enable/disable and regulator-on/off functions. They will keep a deassert_count, and only (re-)assert the reset after reset_control_assert has been called as many times as reset_control_deassert was called.
Calling reset_control_assert without first calling reset_control_deassert is not allowed on a shared reset control. Calling reset_control_reset is also not allowed on a shared reset control.
Signed-off-by Hans de Goede <[email protected]>

All three patches look very nice. I'll give them a test-drive next week. So far I have one small issue, and I like Stephens suggestion of elaborating on how shared resets are to be used a bit more.

Quoted 20 lines

---
 drivers/reset/core.c  | 61 ++++++++++++++++++++++++++++++++-------
 include/linux/reset.h | 79 +++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 114 insertions(+), 26 deletions(-)

drivers/reset/core.c +1 -0

index e439ae2..5554585 100644
@@ -8,6 +8,7 @@
  * the Free Software Foundation; either version 2 of the License, or
  * (at your option) any later version.
  */
+#include <linux/atomic.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/export.h>

@@ -29,12 +30,16 @@ static LIST_HEAD(reset_controller_list);
  * @id: ID of the reset controller in the reset
  *      controller device

drivers/reset/core.c static LIST_HEAD(reset_controller_list); · line 41

Quoted 10 lines

drivers/reset/core.c +3 -0

  * @refcnt: Number of gets of this reset_control
+ * @shared: Is this a shared (1), or an exclusive (0) reset_control?
+ * @deassert_cnt: Number of times this reset line has been deasserted
  */
 struct reset_control {
 	struct reset_controller_dev *rcdev;
 	struct list_head list;
 	unsigned int id;
 	unsigned int refcnt;
+	int shared;

Could we make this an

	enum reset_control_type type;

enum reset_control_type {
RESET_CONTROL_EXCLUSIVE,
RESET_CONTROL_SHARED,
};
instead?

[...]

Quoted 10 lines

drivers/reset/core.c +1 -1

@@ -147,7 +180,7 @@ EXPORT_SYMBOL_GPL(reset_control_status);
 
 static struct reset_control *__reset_control_get(
 				struct reset_controller_dev *rcdev,
-				unsigned int index)
+				unsigned int index, int shared)
 {
 	struct reset_control *rstc;

drivers/reset/core.c static struct reset_control *__reset_control_get( · line 191

Quoted 4 lines

drivers/reset/core.c +1 -0

@@ -155,6 +188,10 @@ static struct reset_control *__reset_control_get(
 
 	list_for_each_entry(rstc, &rcdev->reset_control_head, list) {
 		if (rstc->id == index) {
+			if (!rstc->shared || !shared) {

Then this would have to be

			if ((rstc->type == RESET_CONTROL_EXCLUSIVE) ||
			    (type == RESET_CONTROL_EXCLUSIVE)) {

...

[...]

Quoted 11 lines

drivers/reset/core.c +2 -1

@@ -78,7 +78,8 @@ static inline struct reset_control *__devm_reset_control_get(
 #endif /* CONFIG_RESET_CONTROLLER */
 
 /**
- * reset_control_get - Lookup and obtain a reference to a reset controller.
+ * reset_control_get - Lookup and obtain an exclusive reference to a
+ *                     reset controller.
  * @dev: device to be reset by the controller
  * @id: reset line name
  *

drivers/reset/core.c static inline struct reset_control *__must_check reset_control_get( · line 96

Quoted 5 lines

drivers/reset/core.c +1 -1

@@ -92,17 +93,34 @@ static inline struct reset_control *__must_check reset_control_get(
 #ifndef CONFIG_RESET_CONTROLLER
 	WARN_ON(1);
 #endif
-	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0);
+	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0);

... but these would't be as opaque:

	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0,
				      RESET_CONTROL_EXCLUSIVE);

[...]

Quoted 8 lines

/**
- * of_reset_control_get - Lookup and obtain a reference to a reset controller.
+ * reset_control_get_shared - Lookup and obtain a shared reference to a
+ * reset controller.
+ * @dev: device to be reset by the controller
+ * @id: reset line name
+ *
+ * Returns a struct reset_control or IS_ERR() condition containing errno.

How about addressing Stephen's suggestion by extending this kerneldoc comment a bit?

drivers/reset/core.c

Quoted 6 lines

+ * Use of id names is optional.
+ */

drivers/reset/core.c +4 -0

+static inline struct reset_control *reset_control_get_shared(
+					struct device *dev, const char *id)
+{
+	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 1);

	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0,
				      RESET_CONTROL_SHARED);

[...]

best regards
Philipp

Hans de Goede

author

2016-01-31 09:12 UTC #11 lore raw

PATCH v3 3/3 reset: Add support for shared reset controls

Hi,

Quoted 31 lines · Philipp Zabel

Hi Hans,
Quoted 7 lines · Hans de Goede
In some SoCs some hw-blocks share a reset control. Add support for this setup by adding new:
reset_control_get_shared()
devm_reset_control_get_shared()
devm_reset_control_get_shared_by_index()
Quoted 8 lines
methods to get a reset_control. Note that this patch omits adding of_ variants, if these are needed later they can be easily added.
This patch also changes the behavior of the existing exclusive reset_control_get() variants, if these are now called more then once for the same reset_control they will return -EBUSY. To catch existing drivers triggering this error (there should not be any) a WARN_ON(1) is added in this path.
Which is a good thing.
Quoted 10 lines
When a reset_control is shared, the behavior of reset_control_assert / deassert is changed, for shared reset_controls these will work like the clock-enable/disable and regulator-on/off functions. They will keep a deassert_count, and only (re-)assert the reset after reset_control_assert has been called as many times as reset_control_deassert was called.
Calling reset_control_assert without first calling reset_control_deassert is not allowed on a shared reset control. Calling reset_control_reset is also not allowed on a shared reset control.

Quoted 5 lines

Quoted 1 line
Signed-off-by Hans de Goede <[email protected]>
All three patches look very nice. I'll give them a test-drive next week. So far I have one small issue, and I like Stephens suggestion of elaborating on how shared resets are to be used a bit more.

I can do that, where would you like me to put this, a doxygen style comment in include/linux/reset.h ?

Regards,

Hans

Quoted 114 lines

Quoted 21 lines

---
  drivers/reset/core.c  | 61 ++++++++++++++++++++++++++++++++-------
  include/linux/reset.h | 79 +++++++++++++++++++++++++++++++++++++++++----------
  2 files changed, 114 insertions(+), 26 deletions(-)

drivers/reset/core.c +1 -0

index e439ae2..5554585 100644
@@ -8,6 +8,7 @@
   * the Free Software Foundation; either version 2 of the License, or
   * (at your option) any later version.
   */
+#include <linux/atomic.h>
  #include <linux/device.h>
  #include <linux/err.h>
  #include <linux/export.h>

@@ -29,12 +30,16 @@ static LIST_HEAD(reset_controller_list);
   * @id: ID of the reset controller in the reset
   *      controller device

drivers/reset/core.c static LIST_HEAD(reset_controller_list); · line 41

Quoted 10 lines

drivers/reset/core.c +3 -0

   * @refcnt: Number of gets of this reset_control
+ * @shared: Is this a shared (1), or an exclusive (0) reset_control?
+ * @deassert_cnt: Number of times this reset line has been deasserted
   */
  struct reset_control {
  	struct reset_controller_dev *rcdev;
  	struct list_head list;
  	unsigned int id;
  	unsigned int refcnt;
+	int shared;

Could we make this an

	enum reset_control_type type;

enum reset_control_type {
RESET_CONTROL_EXCLUSIVE,
RESET_CONTROL_SHARED,
};
instead?

[...]

Quoted 10 lines

drivers/reset/core.c +1 -1

@@ -147,7 +180,7 @@ EXPORT_SYMBOL_GPL(reset_control_status);
 
  static struct reset_control *__reset_control_get(
  				struct reset_controller_dev *rcdev,
-				unsigned int index)
+				unsigned int index, int shared)
  {
  	struct reset_control *rstc;

drivers/reset/core.c static struct reset_control *__reset_control_get( · line 191

Quoted 4 lines

drivers/reset/core.c +1 -0

@@ -155,6 +188,10 @@ static struct reset_control *__reset_control_get(
 
  	list_for_each_entry(rstc, &rcdev->reset_control_head, list) {
  		if (rstc->id == index) {
+			if (!rstc->shared || !shared) {

Then this would have to be

			if ((rstc->type == RESET_CONTROL_EXCLUSIVE) ||
			    (type == RESET_CONTROL_EXCLUSIVE)) {

...

[...]

Quoted 11 lines

drivers/reset/core.c +2 -1

@@ -78,7 +78,8 @@ static inline struct reset_control *__devm_reset_control_get(
  #endif /* CONFIG_RESET_CONTROLLER */
 
  /**
- * reset_control_get - Lookup and obtain a reference to a reset controller.
+ * reset_control_get - Lookup and obtain an exclusive reference to a
+ *                     reset controller.
   * @dev: device to be reset by the controller
   * @id: reset line name
   *

drivers/reset/core.c static inline struct reset_control *__must_check reset_control_get( · line 96

Quoted 5 lines

drivers/reset/core.c +1 -1

@@ -92,17 +93,34 @@ static inline struct reset_control *__must_check reset_control_get(
  #ifndef CONFIG_RESET_CONTROLLER
  	WARN_ON(1);
  #endif
-	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0);
+	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0);

... but these would't be as opaque:

	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0,
				      RESET_CONTROL_EXCLUSIVE);

[...]

Quoted 8 lines

/**
- * of_reset_control_get - Lookup and obtain a reference to a reset controller.
+ * reset_control_get_shared - Lookup and obtain a shared reference to a
+ * reset controller.
+ * @dev: device to be reset by the controller
+ * @id: reset line name
+ *
+ * Returns a struct reset_control or IS_ERR() condition containing errno.

How about addressing Stephen's suggestion by extending this kerneldoc comment a bit?

drivers/reset/core.c

Quoted 6 lines

+ * Use of id names is optional.
+ */

drivers/reset/core.c +4 -0

+static inline struct reset_control *reset_control_get_shared(
+					struct device *dev, const char *id)
+{
+	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 1);

	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0,
				      RESET_CONTROL_SHARED);

[...]

best regards
Philipp

Philipp Zabel

maintainer

2016-02-04 16:55 UTC #12 lore raw

PATCH v3 3/3 reset: Add support for shared reset controls

Hi Hans,

Am Sonntag, den 31.01.2016, 10:12 +0100 schrieb Hans de Goede:

Quoted 34 lines

Hi,
Quoted 31 lines · Philipp Zabel
Hi Hans,
Quoted 7 lines · Hans de Goede
In some SoCs some hw-blocks share a reset control. Add support for this setup by adding new:
reset_control_get_shared()
devm_reset_control_get_shared()
devm_reset_control_get_shared_by_index()
Quoted 8 lines
methods to get a reset_control. Note that this patch omits adding of_ variants, if these are needed later they can be easily added.
This patch also changes the behavior of the existing exclusive reset_control_get() variants, if these are now called more then once for the same reset_control they will return -EBUSY. To catch existing drivers triggering this error (there should not be any) a WARN_ON(1) is added in this path.
Which is a good thing.
Quoted 10 lines
When a reset_control is shared, the behavior of reset_control_assert / deassert is changed, for shared reset_controls these will work like the clock-enable/disable and regulator-on/off functions. They will keep a deassert_count, and only (re-)assert the reset after reset_control_assert has been called as many times as reset_control_deassert was called.
Calling reset_control_assert without first calling reset_control_deassert is not allowed on a shared reset control. Calling reset_control_reset is also not allowed on a shared reset control.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Signed-off-by Hans de Goede <[email protected]>
All three patches look very nice. I'll give them a test-drive next week. So far I have one small issue, and I like Stephens suggestion of elaborating on how shared resets are to be used a bit more.
I can do that, where would you like me to put this, a doxygen style comment in include/linux/reset.h ?

Sorry for the delay. I had just thought of extending the reset_control_get_shared comment below, but an overview comment in reset.h would be fine, too.

[...]

Quoted 11 lines

Quoted 11 lines
Quoted 8 lines
/**
- * of_reset_control_get - Lookup and obtain a reference to a reset controller.
+ * reset_control_get_shared - Lookup and obtain a shared reference to a
+ * reset controller.
+ * @dev: device to be reset by the controller
+ * @id: reset line name
+ *
+ * Returns a struct reset_control or IS_ERR() condition containing errno.
How about addressing Stephen's suggestion by extending this kerneldoc comment a bit?

^^^ this.

Now that I think of it, the reset_control_assert function comment should explicitly state that for shared resets the drivers can't expect their registers and internal state to be reset, but must be prepared for this to happen.

best regards
Philipp

Philipp Zabel

maintainer

2016-02-05 09:08 UTC #13 lore raw

PATCH v3 3/3 reset: Add support for shared reset controls

In reply to Hans de Goede

Am Mittwoch, den 27.01.2016, 19:15 +0100 schrieb Hans de Goede:
[...]

drivers/reset/core.c EXPORT_SYMBOL_GPL(reset_controller_unregister); · line 109

Quoted 12 lines

drivers/reset/core.c +5 -0

@@ -94,9 +99,16 @@ EXPORT_SYMBOL_GPL(reset_controller_unregister);
 /**
  * reset_control_reset - reset the controlled device
  * @rstc: reset controller
+ *
+ * Calling this on a shared reset controller is an error.
  */
 int reset_control_reset(struct reset_control *rstc)
 {
+	if (rstc->shared) {
+		WARN_ON(1);
+		return -EINVAL;

Occurrences of

	if (condition) {
		WARN_ON(1);
		...
	}
could be changed to
	if (WARN_ON(condition)) {
		...
	}

regards
Philipp

Hans de Goede

author

2016-02-05 18:28 UTC #14 lore raw

PATCH v3 3/3 reset: Add support for shared reset controls

Hi,

Quoted 16 lines · Philipp Zabel

Am Mittwoch, den 27.01.2016, 19:15 +0100 schrieb Hans de Goede:
[...]
drivers/reset/core.c EXPORT_SYMBOL_GPL(reset_controller_unregister); · line 109
Quoted 12 lines
drivers/reset/core.c +5 -0
@@ -94,9 +99,16 @@ EXPORT_SYMBOL_GPL(reset_controller_unregister);
  /**
   * reset_control_reset - reset the controlled device
   * @rstc: reset controller
+ *
+ * Calling this on a shared reset controller is an error.
   */
  int reset_control_reset(struct reset_control *rstc)
  {
+	if (rstc->shared) {
+		WARN_ON(1);
+		return -EINVAL;
Occurrences of

Quoted 8 lines

	if (condition) {
		WARN_ON(1);
		...
	}
could be changed to
	if (WARN_ON(condition)) {
		...
	}

Good one, will fix for v4 (v2 of the second attempt at this).

Regards,

Hans