[PATCH v3 0/3] reset: Add support for shared reset controls

[PATCH v3 0/3] reset: Add support for shared reset controls

[Hans de Goede]

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

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

[Hans de Goede]

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 <hdegoede@redhat.com>
---
 drivers/reset/core.c  |  84 +++++++--------------------------
 include/linux/reset.h | 126 +++++++++++++++++++++++++++++++++++---------------
 2 files changed, 107 insertions(+), 103 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 8737663..f695429 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -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
diff --git a/include/linux/reset.h b/include/linux/reset.h
index c4c097d..1bb69a2 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -1,8 +1,8 @@
 #ifndef _LINUX_RESET_H_
 #define _LINUX_RESET_H_
 
-struct device;
-struct device_node;
+#include <linux/device.h>
+
 struct reset_control;
 
 #ifdef CONFIG_RESET_CONTROLLER
@@ -12,9 +12,11 @@ int reset_control_assert(struct reset_control *rstc);
 int reset_control_deassert(struct reset_control *rstc);
 int reset_control_status(struct reset_control *rstc);
 
-struct reset_control *reset_control_get(struct device *dev, const char *id);
+struct reset_control *__of_reset_control_get(struct device_node *node,
+					     const char *id, int index);
 void reset_control_put(struct reset_control *rstc);
-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);
 
 int __must_check device_reset(struct device *dev);
 
@@ -23,24 +25,6 @@ static inline int device_reset_optional(struct device *dev)
 	return device_reset(dev);
 }
 
-static inline struct reset_control *reset_control_get_optional(
-					struct device *dev, const char *id)
-{
-	return reset_control_get(dev, id);
-}
-
-static inline struct reset_control *devm_reset_control_get_optional(
-					struct device *dev, const char *id)
-{
-	return devm_reset_control_get(dev, id);
-}
-
-struct reset_control *of_reset_control_get(struct device_node *node,
-					   const char *id);
-
-struct reset_control *of_reset_control_get_by_index(
-					struct device_node *node, int index);
-
 #else
 
 static inline int reset_control_reset(struct reset_control *rstc)
@@ -77,44 +61,114 @@ static inline int device_reset_optional(struct device *dev)
 	return -ENOTSUPP;
 }
 
-static inline struct reset_control *__must_check reset_control_get(
-					struct device *dev, const char *id)
+static inline struct reset_control *__of_reset_control_get(
+					struct device_node *node,
+					const char *id, int index)
 {
-	WARN_ON(1);
 	return ERR_PTR(-EINVAL);
 }
 
-static inline struct reset_control *__must_check devm_reset_control_get(
-					struct device *dev, const char *id)
+static inline struct reset_control *__devm_reset_control_get(
+					struct device *dev,
+					const char *id, int index)
 {
-	WARN_ON(1);
 	return ERR_PTR(-EINVAL);
 }
 
-static inline struct reset_control *reset_control_get_optional(
+#endif /* CONFIG_RESET_CONTROLLER */
+
+/**
+ * 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.
+ */
+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);
 }
 
-static inline struct reset_control *devm_reset_control_get_optional(
+static inline struct reset_control *reset_control_get_optional(
 					struct device *dev, const char *id)
 {
-	return ERR_PTR(-ENOTSUPP);
+	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0);
 }
 
+/**
+ * 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.
+ */
 static inline struct reset_control *of_reset_control_get(
 				struct device_node *node, const char *id)
 {
-	return ERR_PTR(-ENOTSUPP);
+	return __of_reset_control_get(node, id, 0);
 }
 
+/**
+ * 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.
+ */
 static inline struct reset_control *of_reset_control_get_by_index(
-				struct device_node *node, int index)
+					struct device_node *node, int index)
 {
-	return ERR_PTR(-ENOTSUPP);
+	return __of_reset_control_get(node, NULL, index);
 }
 
-#endif /* CONFIG_RESET_CONTROLLER */
+/**
+ * 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.
+ */
+static inline struct reset_control *__must_check devm_reset_control_get(
+					struct device *dev, const char *id)
+{
+#ifndef CONFIG_RESET_CONTROLLER
+	WARN_ON(1);
+#endif
+	return __devm_reset_control_get(dev, id, 0);
+}
+
+static inline struct reset_control *devm_reset_control_get_optional(
+					struct device *dev, const char *id)
+{
+	return __devm_reset_control_get(dev, id, 0);
+}
+
+/**
+ * devm_reset_control_get_by_index - resource managed reset_control_get
+ * @dev: device to be reset by the controller
+ * @index: index of the reset controller
+ *
+ * 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.
+ */
+static inline struct reset_control *devm_reset_control_get_by_index(
+					struct device *dev, int index)
+{
+	return __devm_reset_control_get(dev, NULL, index);
+}
 
 #endif
-- 
2.5.0

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

[Philipp Zabel]

Hi Hans,

Am Mittwoch, den 27.01.2016, 19:15 +0100 schrieb Hans de Goede:
[...]
> +/**
> + * 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.
> + */
> +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:

	return __reset_control_get(dev, id, 0);

regards
Philipp

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

[Hans de Goede]

Hi,

On 04-02-16 17:54, Philipp Zabel wrote:
> Hi Hans,
>
> Am Mittwoch, den 27.01.2016, 19:15 +0100 schrieb Hans de Goede:
> [...]
>> +/**
>> + * 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.
>> + */
>> +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:
>
> 	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

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

[Philipp Zabel]

Am Freitag, den 05.02.2016, 19:30 +0100 schrieb Hans de Goede:
[...]
> > 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

> 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

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

[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 <hdegoede@redhat.com>
---
 drivers/reset/core.c             | 82 ++++++++++++++++++++++++++++++----------
 include/linux/reset-controller.h |  2 +
 2 files changed, 64 insertions(+), 20 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index f695429..e439ae2 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -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);
 
diff --git a/include/linux/reset-controller.h b/include/linux/reset-controller.h
index ce6b962..2decc20 100644
--- a/include/linux/reset-controller.h
+++ b/include/linux/reset-controller.h
@@ -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,
-- 
2.5.0

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

[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 <hdegoede@redhat.com>
---
 drivers/reset/core.c  | 61 ++++++++++++++++++++++++++++++++-------
 include/linux/reset.h | 79 +++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 114 insertions(+), 26 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index e439ae2..5554585 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -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);
diff --git a/include/linux/reset.h b/include/linux/reset.h
index 1bb69a2..cb35e83 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -13,10 +13,10 @@ int reset_control_deassert(struct reset_control *rstc);
 int reset_control_status(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);
 void reset_control_put(struct reset_control *rstc);
 struct reset_control *__devm_reset_control_get(struct device *dev,
-					       const char *id, int index);
+				     const char *id, int index, int shared);
 
 int __must_check device_reset(struct device *dev);
 
@@ -63,14 +63,14 @@ static inline int device_reset_optional(struct device *dev)
 
 static inline struct reset_control *__of_reset_control_get(
 					struct device_node *node,
-					const char *id, int index)
+					const char *id, int index, int shared)
 {
 	return ERR_PTR(-EINVAL);
 }
 
 static inline struct reset_control *__devm_reset_control_get(
 					struct device *dev,
-					const char *id, int index)
+					const char *id, int index, int shared)
 {
 	return ERR_PTR(-EINVAL);
 }
@@ -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
  *
@@ -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);
 }
 
 static inline struct reset_control *reset_control_get_optional(
 					struct device *dev, const char *id)
 {
-	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);
 }
 
 /**
- * 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.
+ *
+ * Use of id names is optional.
+ */
+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);
+}
+
+/**
+ * of_reset_control_get - Lookup and obtain an exclusive reference to a
+ *                        reset controller.
  * @node: device to be reset by the controller
  * @id: reset line name
  *
@@ -113,12 +131,12 @@ static inline struct reset_control *reset_control_get_optional(
 static inline struct reset_control *of_reset_control_get(
 				struct device_node *node, const char *id)
 {
-	return __of_reset_control_get(node, id, 0);
+	return __of_reset_control_get(node, id, 0, 0);
 }
 
 /**
- * of_reset_control_get_by_index - Lookup and obtain a reference to a reset
- * controller by index.
+ * of_reset_control_get_by_index - Lookup and obtain an exclusive reference to
+ *                                 a reset controller by index.
  * @node: device to be reset by the controller
  * @index: index of the reset controller
  *
@@ -129,7 +147,7 @@ static inline struct reset_control *of_reset_control_get(
 static inline struct reset_control *of_reset_control_get_by_index(
 					struct device_node *node, int index)
 {
-	return __of_reset_control_get(node, NULL, index);
+	return __of_reset_control_get(node, NULL, index, 0);
 }
 
 /**
@@ -147,13 +165,13 @@ static inline struct reset_control *__must_check devm_reset_control_get(
 #ifndef CONFIG_RESET_CONTROLLER
 	WARN_ON(1);
 #endif
-	return __devm_reset_control_get(dev, id, 0);
+	return __devm_reset_control_get(dev, id, 0, 0);
 }
 
 static inline struct reset_control *devm_reset_control_get_optional(
 					struct device *dev, const char *id)
 {
-	return __devm_reset_control_get(dev, id, 0);
+	return __devm_reset_control_get(dev, id, 0, 0);
 }
 
 /**
@@ -168,7 +186,38 @@ static inline struct reset_control *devm_reset_control_get_optional(
 static inline struct reset_control *devm_reset_control_get_by_index(
 					struct device *dev, int index)
 {
-	return __devm_reset_control_get(dev, NULL, index);
+	return __devm_reset_control_get(dev, NULL, index, 0);
+}
+
+/**
+ * devm_reset_control_get_shared - resource managed reset_control_get_shared()
+ * @dev: device to be reset by the controller
+ * @id: reset line name
+ *
+ * Managed reset_control_get_shared(). For reset controllers returned from
+ * this function, reset_control_put() is called automatically on driver detach.
+ * See reset_control_get_shared() for more information.
+ */
+static inline struct reset_control *devm_reset_control_get_shared(
+					struct device *dev, const char *id)
+{
+	return __devm_reset_control_get(dev, id, 0, 1);
+}
+
+/**
+ * devm_reset_control_get_shared_by_index - resource managed
+ * reset_control_get_shared
+ * @dev: device to be reset by the controller
+ * @index: index of the reset controller
+ *
+ * Managed reset_control_get_shared(). For reset controllers returned from
+ * this function, reset_control_put() is called automatically on driver detach.
+ * See reset_control_get_shared() for more information.
+ */
+static inline struct reset_control *devm_reset_control_get_shared_by_index(
+					struct device *dev, int index)
+{
+	return __devm_reset_control_get(dev, NULL, index, 1);
 }
 
 #endif
-- 
2.5.0

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

[Stephen Warren]

On 01/27/2016 11:15 AM, Hans de Goede wrote:
> 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.

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.

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

[Hans de Goede]

Hi,

On 01/28/2016 09:20 PM, Stephen Warren wrote:
>
> On 01/27/2016 11:15 AM, Hans de Goede wrote:
>> 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.
>
> 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.

> 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

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

[Stephen Warren]

On 01/28/2016 11:18 PM, Hans de Goede wrote:
> Hi,
>
> On 01/28/2016 09:20 PM, Stephen Warren wrote:
>>
>> On 01/27/2016 11:15 AM, Hans de Goede wrote:
>>> 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.
>>
>> 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.

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.

>> 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.

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.

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

[Philipp Zabel]

Hi Hans,

On Wed, Jan 27, 2016 at 07:15:16PM +0100, Hans de Goede wrote:
> 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.

Which is a good thing.

> 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 <hdegoede@redhat.com>

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.

> ---
>  drivers/reset/core.c  | 61 ++++++++++++++++++++++++++++++++-------
>  include/linux/reset.h | 79 +++++++++++++++++++++++++++++++++++++++++----------
>  2 files changed, 114 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index e439ae2..5554585 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -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;

Could we make this an
	enum reset_control_type type;

enum reset_control_type {
	RESET_CONTROL_EXCLUSIVE,
	RESET_CONTROL_SHARED,
};
instead?

[...]
> @@ -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) {

Then this would have to be

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

[...]
> @@ -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
>   *
> @@ -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);

[...]
>  /**
> - * 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?

> + * Use of id names is optional.
> + */
> +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

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

[Hans de Goede]

Hi,

On 01/30/2016 12:38 PM, Philipp Zabel wrote:
> Hi Hans,
>
> On Wed, Jan 27, 2016 at 07:15:16PM +0100, Hans de Goede wrote:
>> 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.
>
> Which is a good thing.
>
>> 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 <hdegoede@redhat.com>
>
> 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


>
>> ---
>>   drivers/reset/core.c  | 61 ++++++++++++++++++++++++++++++++-------
>>   include/linux/reset.h | 79 +++++++++++++++++++++++++++++++++++++++++----------
>>   2 files changed, 114 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
>> index e439ae2..5554585 100644
>> --- a/drivers/reset/core.c
>> +++ b/drivers/reset/core.c
>> @@ -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;
>
> Could we make this an
> 	enum reset_control_type type;
>
> enum reset_control_type {
> 	RESET_CONTROL_EXCLUSIVE,
> 	RESET_CONTROL_SHARED,
> };
> instead?
>
> [...]
>> @@ -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) {
>
> Then this would have to be
>
> 			if ((rstc->type == RESET_CONTROL_EXCLUSIVE) ||
> 			    (type == RESET_CONTROL_EXCLUSIVE)) {
> ...
>
> [...]
>> @@ -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
>>    *
>> @@ -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);
>
> [...]
>>   /**
>> - * 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?
>
>> + * Use of id names is optional.
>> + */
>> +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
>

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

[Philipp Zabel]

Hi Hans,

Am Sonntag, den 31.01.2016, 10:12 +0100 schrieb Hans de Goede:
> Hi,
> 
> On 01/30/2016 12:38 PM, Philipp Zabel wrote:
> > Hi Hans,
> >
> > On Wed, Jan 27, 2016 at 07:15:16PM +0100, Hans de Goede wrote:
> >> 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.
> >
> > Which is a good thing.
> >
> >> 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 <hdegoede@redhat.com>
> >
> > 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.

[...]
> >>   /**
> >> - * 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

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

[Philipp Zabel]

Am Mittwoch, den 27.01.2016, 19:15 +0100 schrieb Hans de Goede:
[...]
> @@ -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

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

[Hans de Goede]

Hi,

On 05-02-16 10:08, Philipp Zabel wrote:
> Am Mittwoch, den 27.01.2016, 19:15 +0100 schrieb Hans de Goede:
> [...]
>> @@ -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)) {
> 		...
> 	}

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

Regards,

Hans