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

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:

quoted

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.

quoted

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

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

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

[...]

quoted

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

[...]

quoted

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

[...]

quoted

  /**
- * 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?

quoted

+ * 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