[PATCH v4 0/2] Add reset control for mfd syscon devices

[PATCH v4 0/2] Add reset control for mfd syscon devices

[Jeremy Kerr]

This RFC series adds a facility for syscon devices to control a reset
line when probed; we have instances of simple register-only syscon
resources that need deassertion of a reset line for the register set to
be accessible.

Rather than requiring a specific driver to implement this, it'd be nice
to use the generic syscon device and the generic resets linkage to do
so.

Any comments/queries/etc are most welcome.

Cheers,


Jeremy

---
v2:
 - use direct syscon registration interface, rather than the (unused)
   syscon platform device code
 - consequently, add regmap infrastructure to attach a reset
   controller, in a similar way to attaching clocks
v3:
 - drop regmap reset control and just do a direct deassert from the syscon
   driver
v4:
 - collapse unnecessary else block in syscon driver reset control

---
Jeremy Kerr (2):
  dt-bindings: mfd/syscon: Add resets property
  mfd: syscon: allow reset control for syscon devices

 .../devicetree/bindings/mfd/syscon.yaml       |  3 +++
 drivers/mfd/syscon.c                          | 27 ++++++++++++++-----
 2 files changed, 24 insertions(+), 6 deletions(-)

-- 
2.38.1

[PATCH v4 1/2] dt-bindings: mfd/syscon: Add resets property

[Jeremy Kerr]

Simple syscon devices may require deassertion of a reset signal in order
to access their register set. This change adds the `resets` property from
reset.yaml#/properties/resets (referenced through core.yaml), specifying
a maxItems of 1 for a single (optional) reset descriptor.

This will allow a future change to the syscon driver to implement reset
control.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/mfd/syscon.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
index 4e4baf53796d..9dc5984d9147 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.yaml
+++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
@@ -86,6 +86,9 @@ properties:
       on the device.
     enum: [1, 2, 4, 8]
 
+  resets:
+    maxItems: 1
+
   hwlocks:
     maxItems: 1
     description:
-- 
2.38.1

Re: [PATCH v4 1/2] dt-bindings: mfd/syscon: Add resets property

[Lee Jones]

On Thu, 05 Jan 2023, Jeremy Kerr wrote:

> Simple syscon devices may require deassertion of a reset signal in order
> to access their register set. This change adds the `resets` property from
> reset.yaml#/properties/resets (referenced through core.yaml), specifying
> a maxItems of 1 for a single (optional) reset descriptor.
> 
> This will allow a future change to the syscon driver to implement reset
> control.
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/mfd/syscon.yaml | 3 +++
>  1 file changed, 3 insertions(+)

Applied, thanks

-- 
Lee Jones [李琼斯]

[PATCH v4 2/2] mfd: syscon: allow reset control for syscon devices

[Jeremy Kerr]

Simple syscon devices may require deassertion of a reset signal in order
to access their register set. Rather than requiring a custom driver to
implement this, we can use the generic "resets" specifiers to link a
reset line to the syscon.

This change adds an optional reset line to the syscon device
description, and deasserts the reset if detected.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>

---
v2:
 * do reset control in the early of_syscon_register() path, rather than
   the platform device init, which isn't used.
v3:
 * use a direct reset_control_deassert rather than handling in the
   regmap
v4:
 * collapse unnecessary `else` block
---
 drivers/mfd/syscon.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index bdb2ce7ff03b..57b29c325131 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -20,6 +20,7 @@
 #include <linux/platform_data/syscon.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/reset.h>
 #include <linux/mfd/syscon.h>
 #include <linux/slab.h>
 
@@ -31,6 +32,7 @@ static LIST_HEAD(syscon_list);
 struct syscon {
 	struct device_node *np;
 	struct regmap *regmap;
+	struct reset_control *reset;
 	struct list_head list;
 };
 
@@ -40,7 +42,7 @@ static const struct regmap_config syscon_regmap_config = {
 	.reg_stride = 4,
 };
 
-static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
+static struct syscon *of_syscon_register(struct device_node *np, bool check_res)
 {
 	struct clk *clk;
 	struct syscon *syscon;
@@ -50,6 +52,7 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
 	int ret;
 	struct regmap_config syscon_config = syscon_regmap_config;
 	struct resource res;
+	struct reset_control *reset;
 
 	syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
 	if (!syscon)
@@ -114,7 +117,7 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
 		goto err_regmap;
 	}
 
-	if (check_clk) {
+	if (check_res) {
 		clk = of_clk_get(np, 0);
 		if (IS_ERR(clk)) {
 			ret = PTR_ERR(clk);
@@ -124,8 +127,18 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
 		} else {
 			ret = regmap_mmio_attach_clk(regmap, clk);
 			if (ret)
-				goto err_attach;
+				goto err_attach_clk;
 		}
+
+		reset = of_reset_control_get_optional_exclusive(np, NULL);
+		if (IS_ERR(reset)) {
+			ret = PTR_ERR(reset);
+			goto err_attach_clk;
+		}
+
+		ret = reset_control_deassert(reset);
+		if (ret)
+			goto err_reset;
 	}
 
 	syscon->regmap = regmap;
@@ -137,7 +150,9 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
 
 	return syscon;
 
-err_attach:
+err_reset:
+	reset_control_put(reset);
+err_attach_clk:
 	if (!IS_ERR(clk))
 		clk_put(clk);
 err_clk:
@@ -150,7 +165,7 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
 }
 
 static struct regmap *device_node_get_regmap(struct device_node *np,
-					     bool check_clk)
+					     bool check_res)
 {
 	struct syscon *entry, *syscon = NULL;
 
@@ -165,7 +180,7 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
 	spin_unlock(&syscon_list_slock);
 
 	if (!syscon)
-		syscon = of_syscon_register(np, check_clk);
+		syscon = of_syscon_register(np, check_res);
 
 	if (IS_ERR(syscon))
 		return ERR_CAST(syscon);
-- 
2.38.1

Re: [PATCH v4 2/2] mfd: syscon: allow reset control for syscon devices

[Lee Jones]

On Thu, 05 Jan 2023, Jeremy Kerr wrote:

> Simple syscon devices may require deassertion of a reset signal in order
> to access their register set. Rather than requiring a custom driver to
> implement this, we can use the generic "resets" specifiers to link a
> reset line to the syscon.
> 
> This change adds an optional reset line to the syscon device
> description, and deasserts the reset if detected.
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> 
> ---
> v2:
>  * do reset control in the early of_syscon_register() path, rather than
>    the platform device init, which isn't used.
> v3:
>  * use a direct reset_control_deassert rather than handling in the
>    regmap
> v4:
>  * collapse unnecessary `else` block
> ---
>  drivers/mfd/syscon.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

Re: [PATCH v4 2/2] mfd: syscon: allow reset control for syscon devices

[Daniel Golle]

Hi Jeremy,

On Thu, Jan 05, 2023 at 08:50:10AM +0800, Jeremy Kerr wrote:
> Simple syscon devices may require deassertion of a reset signal in order
> to access their register set. Rather than requiring a custom driver to
> implement this, we can use the generic "resets" specifiers to link a
> reset line to the syscon.
> 
> This change adds an optional reset line to the syscon device
> description, and deasserts the reset if detected.
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> 
> ---
> v2:
>  * do reset control in the early of_syscon_register() path, rather than
>    the platform device init, which isn't used.
> v3:
>  * use a direct reset_control_deassert rather than handling in the
>    regmap
> v4:
>  * collapse unnecessary `else` block
> ---
>  drivers/mfd/syscon.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index bdb2ce7ff03b..57b29c325131 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -20,6 +20,7 @@
>  #include <linux/platform_data/syscon.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> +#include <linux/reset.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/slab.h>
>  
> @@ -31,6 +32,7 @@ static LIST_HEAD(syscon_list);
>  struct syscon {
>  	struct device_node *np;
>  	struct regmap *regmap;
> +	struct reset_control *reset;

You are reserving a field 'reset' in this struct but then never use it.
I stumbled upon this because I was wondering if it'd make sense to have
something like

int syscon_reset(struct syscon *syscon)
{
	if (!syscon->reset)
		return -EOPNOTSUPP;

	return reset_control_reset(syscon->reset);
}

But then, of course, why not have syscon_reset_assert, *_deassert, ... ?
Wrapping the reset control ops, of course, would have the advantage the
syscon driver would be able to track whether reset is asserted.

However, making the reset shared also allows the syscon user to use the
reset as well and is mich easier to implement:

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 57b29c3251312..55f3f855f0305 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -32,7 +32,6 @@ static LIST_HEAD(syscon_list);
 struct syscon {
 	struct device_node *np;
 	struct regmap *regmap;
-	struct reset_control *reset;
 	struct list_head list;
 };
 
@@ -130,15 +129,16 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_res)
 				goto err_attach_clk;
 		}
 
-		reset = of_reset_control_get_optional_exclusive(np, NULL);
+		reset = of_reset_control_get_shared(np, NULL);
 		if (IS_ERR(reset)) {
 			ret = PTR_ERR(reset);
-			goto err_attach_clk;
+			if (ret != -ENOENT)
+				goto err_attach_clk;
+		} else {
+			ret = reset_control_deassert(reset);
+			if (ret)
+				goto err_reset;
 		}
-
-		ret = reset_control_deassert(reset);
-		if (ret)
-			goto err_reset;
 	}
 
 	syscon->regmap = regmap;
---

Let me know what you think.


>  	struct list_head list;
>  };
>  
> @@ -40,7 +42,7 @@ static const struct regmap_config syscon_regmap_config = {
>  	.reg_stride = 4,
>  };
>  
> -static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
> +static struct syscon *of_syscon_register(struct device_node *np, bool check_res)
>  {
>  	struct clk *clk;
>  	struct syscon *syscon;
> @@ -50,6 +52,7 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
>  	int ret;
>  	struct regmap_config syscon_config = syscon_regmap_config;
>  	struct resource res;
> +	struct reset_control *reset;
>  
>  	syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
>  	if (!syscon)
> @@ -114,7 +117,7 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
>  		goto err_regmap;
>  	}
>  
> -	if (check_clk) {
> +	if (check_res) {
>  		clk = of_clk_get(np, 0);
>  		if (IS_ERR(clk)) {
>  			ret = PTR_ERR(clk);
> @@ -124,8 +127,18 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
>  		} else {
>  			ret = regmap_mmio_attach_clk(regmap, clk);
>  			if (ret)
> -				goto err_attach;
> +				goto err_attach_clk;
>  		}
> +
> +		reset = of_reset_control_get_optional_exclusive(np, NULL);
> +		if (IS_ERR(reset)) {
> +			ret = PTR_ERR(reset);
> +			goto err_attach_clk;
> +		}
> +
> +		ret = reset_control_deassert(reset);
> +		if (ret)
> +			goto err_reset;
>  	}
>  
>  	syscon->regmap = regmap;
> @@ -137,7 +150,9 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
>  
>  	return syscon;
>  
> -err_attach:
> +err_reset:
> +	reset_control_put(reset);
> +err_attach_clk:
>  	if (!IS_ERR(clk))
>  		clk_put(clk);
>  err_clk:
> @@ -150,7 +165,7 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
>  }
>  
>  static struct regmap *device_node_get_regmap(struct device_node *np,
> -					     bool check_clk)
> +					     bool check_res)
>  {
>  	struct syscon *entry, *syscon = NULL;
>  
> @@ -165,7 +180,7 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
>  	spin_unlock(&syscon_list_slock);
>  
>  	if (!syscon)
> -		syscon = of_syscon_register(np, check_clk);
> +		syscon = of_syscon_register(np, check_res);
>  
>  	if (IS_ERR(syscon))
>  		return ERR_CAST(syscon);
> -- 
> 2.38.1
>