[PATCH v2 0/2] watchdog: sp805: add reset control support

[PATCH v2 0/2] watchdog: sp805: add reset control support

[Yang Xiwen via B4 Relay]

Deassert the reset if it's available.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
Changes in v2:
- commit log: dt-binding->dt-bindings (Krzysztof Kozlowski)
- binding: remove "|", join two lines. (Krzysztof Kozlowski)
- Link to v1: https://lore.kernel.org/r/20240217-hisi-wdt-v1-0-cdb9ddcab968@outlook.com

---
Yang Xiwen (2):
      watchdog: sp805_wdt: deassert the reset if available
      dt-bindings: watchdog: arm,sp805: document the reset signal

 Documentation/devicetree/bindings/watchdog/arm,sp805.yaml | 5 +++++
 drivers/watchdog/sp805_wdt.c                              | 9 +++++++++
 2 files changed, 14 insertions(+)
---
base-commit: 8d3dea210042f54b952b481838c1e7dfc4ec751d
change-id: 20240217-hisi-wdt-3b52971a6591

Best regards,
-- 
Yang Xiwen <forbidden405@outlook.com>

[PATCH v2 1/2] watchdog: sp805_wdt: deassert the reset if available

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

According to the datasheet, the core has an WDOGRESn input signal that
needs to be deasserted before being operational. Implement it in the
driver.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 drivers/watchdog/sp805_wdt.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
index 2756ed54ca3d..b4bcfdeb39e6 100644
--- a/drivers/watchdog/sp805_wdt.c
+++ b/drivers/watchdog/sp805_wdt.c
@@ -25,6 +25,7 @@
 #include <linux/moduleparam.h>
 #include <linux/pm.h>
 #include <linux/property.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
@@ -59,6 +60,7 @@
  * @lock: spin lock protecting dev structure and io access
  * @base: base address of wdt
  * @clk: (optional) clock structure of wdt
+ * @rst: (optional) reset control signal of wdt
  * @rate: (optional) clock rate when provided via properties
  * @adev: amba device structure of wdt
  * @status: current status of wdt
@@ -69,6 +71,7 @@ struct sp805_wdt {
 	spinlock_t			lock;
 	void __iomem			*base;
 	struct clk			*clk;
+	struct reset_control		*rst;
 	u64				rate;
 	struct amba_device		*adev;
 	unsigned int			load_val;
@@ -264,6 +267,12 @@ sp805_wdt_probe(struct amba_device *adev, const struct amba_id *id)
 		return -ENODEV;
 	}
 
+	wdt->rst = devm_reset_control_get_optional(&adev->dev, NULL);
+	if (IS_ERR(wdt->rst))
+		return dev_err_probe(&adev->dev, PTR_ERR(wdt->rst), "Can not get reset\n");
+
+	reset_control_deassert(wdt->rst);
+
 	wdt->adev = adev;
 	wdt->wdd.info = &wdt_info;
 	wdt->wdd.ops = &wdt_ops;

-- 
2.43.0

[PATCH v2 2/2] dt-bindings: watchdog: arm,sp805: document the reset signal

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

The reset signal needs to be deasserted before operation of sp805
module. Document in the binding.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 Documentation/devicetree/bindings/watchdog/arm,sp805.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/arm,sp805.yaml b/Documentation/devicetree/bindings/watchdog/arm,sp805.yaml
index 7aea255b301b..bd7c09ed1938 100644
--- a/Documentation/devicetree/bindings/watchdog/arm,sp805.yaml
+++ b/Documentation/devicetree/bindings/watchdog/arm,sp805.yaml
@@ -50,6 +50,10 @@ properties:
       - const: wdog_clk
       - const: apb_pclk
 
+  resets:
+    maxItems: 1
+    description: WDOGRESn input reset signal for sp805 module.
+
 required:
   - compatible
   - reg
@@ -67,4 +71,5 @@ examples:
         interrupts = <GIC_SPI 406 IRQ_TYPE_LEVEL_HIGH>;
         clocks = <&wdt_clk>, <&apb_pclk>;
         clock-names = "wdog_clk", "apb_pclk";
+        resets = <&wdt_rst>;
     };

-- 
2.43.0

Re: [PATCH v2 1/2] watchdog: sp805_wdt: deassert the reset if available

[Christophe JAILLET]

Le 19/02/2024 à 19:14, Yang Xiwen via B4 Relay a écrit :
> From: Yang Xiwen <forbidden405-1ViLX0X+lBJBDgjK7y7TUQ@public.gmane.org>
> 
> According to the datasheet, the core has an WDOGRESn input signal that
> needs to be deasserted before being operational. Implement it in the
> driver.
> 
> Signed-off-by: Yang Xiwen <forbidden405-1ViLX0X+lBJBDgjK7y7TUQ@public.gmane.org>
> ---
>   drivers/watchdog/sp805_wdt.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
> index 2756ed54ca3d..b4bcfdeb39e6 100644
> --- a/drivers/watchdog/sp805_wdt.c
> +++ b/drivers/watchdog/sp805_wdt.c
> @@ -25,6 +25,7 @@
>   #include <linux/moduleparam.h>
>   #include <linux/pm.h>
>   #include <linux/property.h>
> +#include <linux/reset.h>
>   #include <linux/slab.h>
>   #include <linux/spinlock.h>
>   #include <linux/types.h>
> @@ -59,6 +60,7 @@
>    * @lock: spin lock protecting dev structure and io access
>    * @base: base address of wdt
>    * @clk: (optional) clock structure of wdt
> + * @rst: (optional) reset control signal of wdt
>    * @rate: (optional) clock rate when provided via properties
>    * @adev: amba device structure of wdt
>    * @status: current status of wdt
> @@ -69,6 +71,7 @@ struct sp805_wdt {
>   	spinlock_t			lock;
>   	void __iomem			*base;
>   	struct clk			*clk;
> +	struct reset_control		*rst;
>   	u64				rate;
>   	struct amba_device		*adev;
>   	unsigned int			load_val;
> @@ -264,6 +267,12 @@ sp805_wdt_probe(struct amba_device *adev, const struct amba_id *id)
>   		return -ENODEV;
>   	}
>   
> +	wdt->rst = devm_reset_control_get_optional(&adev->dev, NULL);
> +	if (IS_ERR(wdt->rst))
> +		return dev_err_probe(&adev->dev, PTR_ERR(wdt->rst), "Can not get reset\n");
> +
> +	reset_control_deassert(wdt->rst);
> +

Hi,

Is a corresponding reset_control_assert() needed in the remove function?

CJ

>   	wdt->adev = adev;
>   	wdt->wdd.info = &wdt_info;
>   	wdt->wdd.ops = &wdt_ops;
> 

Re: [PATCH v2 1/2] watchdog: sp805_wdt: deassert the reset if available

[Yang Xiwen]

On 2/20/2024 2:22 AM, Christophe JAILLET wrote:
> Le 19/02/2024 à 19:14, Yang Xiwen via B4 Relay a écrit :
>> From: Yang Xiwen <forbidden405-1ViLX0X+lBJBDgjK7y7TUQ@public.gmane.org>
>>
>> According to the datasheet, the core has an WDOGRESn input signal that
>> needs to be deasserted before being operational. Implement it in the
>> driver.
>>
>> Signed-off-by: Yang Xiwen 
>> <forbidden405-1ViLX0X+lBJBDgjK7y7TUQ@public.gmane.org>
>> ---
>>   drivers/watchdog/sp805_wdt.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
>> index 2756ed54ca3d..b4bcfdeb39e6 100644
>> --- a/drivers/watchdog/sp805_wdt.c
>> +++ b/drivers/watchdog/sp805_wdt.c
>> @@ -25,6 +25,7 @@
>>   #include <linux/moduleparam.h>
>>   #include <linux/pm.h>
>>   #include <linux/property.h>
>> +#include <linux/reset.h>
>>   #include <linux/slab.h>
>>   #include <linux/spinlock.h>
>>   #include <linux/types.h>
>> @@ -59,6 +60,7 @@
>>    * @lock: spin lock protecting dev structure and io access
>>    * @base: base address of wdt
>>    * @clk: (optional) clock structure of wdt
>> + * @rst: (optional) reset control signal of wdt
>>    * @rate: (optional) clock rate when provided via properties
>>    * @adev: amba device structure of wdt
>>    * @status: current status of wdt
>> @@ -69,6 +71,7 @@ struct sp805_wdt {
>>       spinlock_t            lock;
>>       void __iomem            *base;
>>       struct clk            *clk;
>> +    struct reset_control        *rst;
>>       u64                rate;
>>       struct amba_device        *adev;
>>       unsigned int            load_val;
>> @@ -264,6 +267,12 @@ sp805_wdt_probe(struct amba_device *adev, const 
>> struct amba_id *id)
>>           return -ENODEV;
>>       }
>>   +    wdt->rst = devm_reset_control_get_optional(&adev->dev, NULL);
>> +    if (IS_ERR(wdt->rst))
>> +        return dev_err_probe(&adev->dev, PTR_ERR(wdt->rst), "Can not 
>> get reset\n");
>> +
>> +    reset_control_deassert(wdt->rst);
>> +
>
> Hi,
>
> Is a corresponding reset_control_assert() needed in the remove function?
I'll add it in next version. Though it's not very critical I think. It's 
already disabled from the control register.
>
> CJ
>
>>       wdt->adev = adev;
>>       wdt->wdd.info = &wdt_info;
>>       wdt->wdd.ops = &wdt_ops;
>>
>

-- 
Regards,
Yang Xiwen

Re: [PATCH v2 1/2] watchdog: sp805_wdt: deassert the reset if available

[Yang Xiwen]

On 2/20/2024 2:22 AM, Christophe JAILLET wrote:
> Le 19/02/2024 à 19:14, Yang Xiwen via B4 Relay a écrit :
>> From: Yang Xiwen <forbidden405-1ViLX0X+lBJBDgjK7y7TUQ@public.gmane.org>
>>
>> According to the datasheet, the core has an WDOGRESn input signal that
>> needs to be deasserted before being operational. Implement it in the
>> driver.
>>
>> Signed-off-by: Yang Xiwen 
>> <forbidden405-1ViLX0X+lBJBDgjK7y7TUQ@public.gmane.org>
>> ---
>>   drivers/watchdog/sp805_wdt.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
>> index 2756ed54ca3d..b4bcfdeb39e6 100644
>> --- a/drivers/watchdog/sp805_wdt.c
>> +++ b/drivers/watchdog/sp805_wdt.c
>> @@ -25,6 +25,7 @@
>>   #include <linux/moduleparam.h>
>>   #include <linux/pm.h>
>>   #include <linux/property.h>
>> +#include <linux/reset.h>
>>   #include <linux/slab.h>
>>   #include <linux/spinlock.h>
>>   #include <linux/types.h>
>> @@ -59,6 +60,7 @@
>>    * @lock: spin lock protecting dev structure and io access
>>    * @base: base address of wdt
>>    * @clk: (optional) clock structure of wdt
>> + * @rst: (optional) reset control signal of wdt
>>    * @rate: (optional) clock rate when provided via properties
>>    * @adev: amba device structure of wdt
>>    * @status: current status of wdt
>> @@ -69,6 +71,7 @@ struct sp805_wdt {
>>       spinlock_t            lock;
>>       void __iomem            *base;
>>       struct clk            *clk;
>> +    struct reset_control        *rst;
>>       u64                rate;
>>       struct amba_device        *adev;
>>       unsigned int            load_val;
>> @@ -264,6 +267,12 @@ sp805_wdt_probe(struct amba_device *adev, const 
>> struct amba_id *id)
>>           return -ENODEV;
>>       }
>>   +    wdt->rst = devm_reset_control_get_optional(&adev->dev, NULL);
>> +    if (IS_ERR(wdt->rst))
>> +        return dev_err_probe(&adev->dev, PTR_ERR(wdt->rst), "Can not 
>> get reset\n");
>> +
>> +    reset_control_deassert(wdt->rst);
>> +
>
> Hi,
>
> Is a corresponding reset_control_assert() needed in the remove function?

I don't think it makes much sense. Many drivers in kernel does not 
reassert the resets in their driver's remove callback too.

Maybe it can save some power. But since it's already disabled in control 
register. Can't say it's too much.

>
> CJ
>
>>       wdt->adev = adev;
>>       wdt->wdd.info = &wdt_info;
>>       wdt->wdd.ops = &wdt_ops;
>>
>

-- 
Regards,
Yang Xiwen

Re: [PATCH v2 1/2] watchdog: sp805_wdt: deassert the reset if available

[Philipp Zabel]

On Di, 2024-02-20 at 02:14 +0800, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> According to the datasheet, the core has an WDOGRESn input signal that
> needs to be deasserted before being operational. Implement it in the
> driver.
> 
> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> ---
>  drivers/watchdog/sp805_wdt.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
> index 2756ed54ca3d..b4bcfdeb39e6 100644
> --- a/drivers/watchdog/sp805_wdt.c
> +++ b/drivers/watchdog/sp805_wdt.c
> @@ -25,6 +25,7 @@
>  #include <linux/moduleparam.h>
>  #include <linux/pm.h>
>  #include <linux/property.h>
> +#include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/types.h>
> @@ -59,6 +60,7 @@
>   * @lock: spin lock protecting dev structure and io access
>   * @base: base address of wdt
>   * @clk: (optional) clock structure of wdt
> + * @rst: (optional) reset control signal of wdt
>   * @rate: (optional) clock rate when provided via properties
>   * @adev: amba device structure of wdt
>   * @status: current status of wdt
> @@ -69,6 +71,7 @@ struct sp805_wdt {
>  	spinlock_t			lock;
>  	void __iomem			*base;
>  	struct clk			*clk;
> +	struct reset_control		*rst;

This can be a local variable in sp805_wdt_probe().

>  	u64				rate;
>  	struct amba_device		*adev;
>  	unsigned int			load_val;
> @@ -264,6 +267,12 @@ sp805_wdt_probe(struct amba_device *adev, const struct amba_id *id)
>  		return -ENODEV;
>  	}
>  
> +	wdt->rst = devm_reset_control_get_optional(&adev->dev, NULL);

Please use devm_reset_control_get_optional_exclusive() directly.

> +	if (IS_ERR(wdt->rst))
> +		return dev_err_probe(&adev->dev, PTR_ERR(wdt->rst), "Can not get reset\n");
> +
> +	reset_control_deassert(wdt->rst);
> +
>  	wdt->adev = adev;
>  	wdt->wdd.info = &wdt_info;
>  	wdt->wdd.ops = &wdt_ops;

regards
Philipp

Re: [PATCH v2 1/2] watchdog: sp805_wdt: deassert the reset if available

[Philipp Zabel]

On Di, 2024-02-20 at 03:00 +0800, Yang Xiwen wrote:
> On 2/20/2024 2:22 AM, Christophe JAILLET wrote:
> > Le 19/02/2024 à 19:14, Yang Xiwen via B4 Relay a écrit :
> > > From: Yang Xiwen <forbidden405-1ViLX0X+lBJBDgjK7y7TUQ@public.gmane.org>
> > > 
> > > According to the datasheet, the core has an WDOGRESn input signal that
> > > needs to be deasserted before being operational. Implement it in the
> > > driver.
> > > 
> > > Signed-off-by: Yang Xiwen 
> > > <forbidden405-1ViLX0X+lBJBDgjK7y7TUQ@public.gmane.org>
> > > ---
> > >   drivers/watchdog/sp805_wdt.c | 9 +++++++++
> > >   1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
> > > index 2756ed54ca3d..b4bcfdeb39e6 100644
> > > --- a/drivers/watchdog/sp805_wdt.c
> > > +++ b/drivers/watchdog/sp805_wdt.c
> > > @@ -25,6 +25,7 @@
> > >   #include <linux/moduleparam.h>
> > >   #include <linux/pm.h>
> > >   #include <linux/property.h>
> > > +#include <linux/reset.h>
> > >   #include <linux/slab.h>
> > >   #include <linux/spinlock.h>
> > >   #include <linux/types.h>
> > > @@ -59,6 +60,7 @@
> > >    * @lock: spin lock protecting dev structure and io access
> > >    * @base: base address of wdt
> > >    * @clk: (optional) clock structure of wdt
> > > + * @rst: (optional) reset control signal of wdt
> > >    * @rate: (optional) clock rate when provided via properties
> > >    * @adev: amba device structure of wdt
> > >    * @status: current status of wdt
> > > @@ -69,6 +71,7 @@ struct sp805_wdt {
> > >       spinlock_t            lock;
> > >       void __iomem            *base;
> > >       struct clk            *clk;
> > > +    struct reset_control        *rst;
> > >       u64                rate;
> > >       struct amba_device        *adev;
> > >       unsigned int            load_val;
> > > @@ -264,6 +267,12 @@ sp805_wdt_probe(struct amba_device *adev, const 
> > > struct amba_id *id)
> > >           return -ENODEV;
> > >       }
> > >   +    wdt->rst = devm_reset_control_get_optional(&adev->dev, NULL);
> > > +    if (IS_ERR(wdt->rst))
> > > +        return dev_err_probe(&adev->dev, PTR_ERR(wdt->rst), "Can not 
> > > get reset\n");
> > > +
> > > +    reset_control_deassert(wdt->rst);
> > > +
> > 
> > Hi,
> > 
> > Is a corresponding reset_control_assert() needed in the remove function?
> 
> I don't think it makes much sense. Many drivers in kernel does not 
> reassert the resets in their driver's remove callback too.

In general, balancing deassert/assert is only required for shared reset
controls.

For exclusive reset controls, it depends on whether the driver has any
expectations that the reset was asserted at some point before deassert
is called, possibly to reset registers or some internal state.
E.g., if this driver is unbound and then bound again, the deassert
during second probe is a no-op.

regards
Philipp