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

[RFC PATCH v3 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 attach and just do a direct deassert from the syscon
   driver

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

[RFC PATCH v3 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.35.1

[RFC PATCH v3 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>

---
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
---
 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..05e286a69dbe 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,7 +127,17 @@ 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;
+		} else {
+			ret = reset_control_deassert(reset);
+			if (ret)
+				goto err_reset;
 		}
 	}
 
@@ -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.35.1

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

[Lee Jones]

On Sun, 11 Dec 2022, 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>
> 
> ---
> 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
> ---
>  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..05e286a69dbe 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,7 +127,17 @@ 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;
> +		} else {
> +			ret = reset_control_deassert(reset);
> +			if (ret)
> +				goto err_reset;
>  		}

The else is superfluous, right?

Arnd, besides this are you happy with the patch?

-- 
Lee Jones [李琼斯]

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

[Jeremy Kerr]

Hi Lee,

> > @@ -124,7 +127,17 @@ 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;
> > +               } else {
> > +                       ret = reset_control_deassert(reset);
> > +                       if (ret)
> > +                               goto err_reset;
> >                 }
> 
> The else is superfluous, right?

Yep, we could move that reset_control_deassert() out of the else block.
If there are no other changes, I'll send a v4 with that.

Cheers,


Jeremy

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

[Lee Jones]

On Thu, 05 Jan 2023, Jeremy Kerr wrote:

> Hi Lee,
> 
> > > @@ -124,7 +127,17 @@ 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;
> > > +               } else {
> > > +                       ret = reset_control_deassert(reset);
> > > +                       if (ret)
> > > +                               goto err_reset;
> > >                 }
> > 
> > The else is superfluous, right?
> 
> Yep, we could move that reset_control_deassert() out of the else block.
> If there are no other changes, I'll send a v4 with that.

I'd wait a little while to give Arnd a chance to respond.

Might save you a little work.

-- 
Lee Jones [李琼斯]

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

[Arnd Bergmann]

On Wed, Jan 4, 2023, at 18:57, Lee Jones wrote:
> On Thu, 05 Jan 2023, Jeremy Kerr wrote:
>
>> Hi Lee,
>> 
>> > > @@ -124,7 +127,17 @@ 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;
>> > > +               } else {
>> > > +                       ret = reset_control_deassert(reset);
>> > > +                       if (ret)
>> > > +                               goto err_reset;
>> > >                 }
>> > 
>> > The else is superfluous, right?
>> 
>> Yep, we could move that reset_control_deassert() out of the else block.
>> If there are no other changes, I'll send a v4 with that.
>
> I'd wait a little while to give Arnd a chance to respond.
>
> Might save you a little work.

Looks all good to me,

Reviewed-by: Arnd Bergmann <arnd@arndb.de>