[PATCH 1/7] dt-bindings: watchdog: dw-wdt: Replace legacy bindings file with YAML-based one

[PATCH 1/7] dt-bindings: watchdog: dw-wdt: Replace legacy bindings file with YAML-based one

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Modern device tree bindings are supposed to be created as YAML-files
in accordane with dt-schema. This commit replaces the DW Watchdog
legacy bare text bindings with YAML file. As before the the bindings
states that the corresponding dts node is supposed to have a registers
range reference, at least one clocks phandle reference, optional reset
lines. Seeing all the platforms with DW Watchdog provide the watchdog
interrupt property and since in further commit we'll alter the driver
to use it for pre-timeout functionality implementation, lets declare
the IRQ property to be required.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 .../devicetree/bindings/watchdog/dw_wdt.txt   | 24 -------
 .../bindings/watchdog/snps,dw-wdt.yaml        | 66 +++++++++++++++++++
 2 files changed, 66 insertions(+), 24 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/watchdog/dw_wdt.txt
 create mode 100644 Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml

diff --git a/Documentation/devicetree/bindings/watchdog/dw_wdt.txt b/Documentation/devicetree/bindings/watchdog/dw_wdt.txt
deleted file mode 100644
index eb0914420c7c..000000000000
--- a/Documentation/devicetree/bindings/watchdog/dw_wdt.txt
+++ /dev/null
@@ -1,24 +0,0 @@
-Synopsys Designware Watchdog Timer
-
-Required Properties:
-
-- compatible	: Should contain "snps,dw-wdt"
-- reg		: Base address and size of the watchdog timer registers.
-- clocks	: phandle + clock-specifier for the clock that drives the
-		watchdog timer.
-
-Optional Properties:
-
-- interrupts	: The interrupt used for the watchdog timeout warning.
-- resets	: phandle pointing to the system reset controller with
-		line index for the watchdog.
-
-Example:
-
-	watchdog0: wd@ffd02000 {
-		compatible = "snps,dw-wdt";
-		reg = <0xffd02000 0x1000>;
-		interrupts = <0 171 4>;
-		clocks = <&per_base_clk>;
-		resets = <&rst WDT0_RESET>;
-	};
diff --git a/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml b/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml
new file mode 100644
index 000000000000..8b30f9601c38
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (C) 2019 BAIKAL ELECTRONICS, JSC
+#
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/snps,dw-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synopsys Designware Watchdog Timer
+
+allOf:
+  - $ref: "watchdog.yaml#"
+
+maintainers:
+  - Jamie Iles <jamie@jamieiles.com>
+
+properties:
+  compatible:
+    const: snps,dw-wdt
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    description: DW Watchdog pre-timeout interrupts.
+    maxItems: 1
+
+  clocks:
+    minItems: 1
+    items:
+      - description: Watchdog timer reference clock.
+      - description: APB3 interface clock.
+
+  clock-names:
+    minItems: 1
+    items:
+      - const: tclk
+      - const: pclk
+
+  assigned-clocks: true
+
+  assigned-clock-rates: true
+
+  resets:
+    description: Phandle to the DW Watchdog reset lane.
+    maxItems: 1
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+examples:
+  - |
+    watchdog0: watchdog@ffd02000 {
+      compatible = "snps,dw-wdt";
+      reg = <0xffd02000 0x1000>;
+      interrupts = <0 171 4>;
+      clocks = <&per_base_clk>;
+      resets = <&wdt_rst>;
+    };
+...
-- 
2.25.1

Re: [PATCH 1/7] dt-bindings: watchdog: dw-wdt: Replace legacy bindings file with YAML-based one

[Guenter Roeck]

On 3/6/20 5:27 AM, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> Modern device tree bindings are supposed to be created as YAML-files
> in accordane with dt-schema. This commit replaces the DW Watchdog
> legacy bare text bindings with YAML file. As before the the bindings
> states that the corresponding dts node is supposed to have a registers
> range reference, at least one clocks phandle reference, optional reset
> lines. Seeing all the platforms with DW Watchdog provide the watchdog
> interrupt property and since in further commit we'll alter the driver
> to use it for pre-timeout functionality implementation, lets declare
> the IRQ property to be required.
> 

First, this is not just a replacement - it changes semantics.

Second, I disagree with making interrupts mandatory. They are only needed
for pretimeout functionality, and not everyone may want to enable that.
I don't see the point of forcing everyone to enable and provide functionality
that is neither wanted or needed for a given use case. Yes, the interrupt
is provided by all users today, but we may have one coming up tomorrow
where the interrupt line is not even wired up. What then ?

Guenter

> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---
>  .../devicetree/bindings/watchdog/dw_wdt.txt   | 24 -------
>  .../bindings/watchdog/snps,dw-wdt.yaml        | 66 +++++++++++++++++++
>  2 files changed, 66 insertions(+), 24 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/watchdog/dw_wdt.txt
>  create mode 100644 Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/dw_wdt.txt b/Documentation/devicetree/bindings/watchdog/dw_wdt.txt
> deleted file mode 100644
> index eb0914420c7c..000000000000
> --- a/Documentation/devicetree/bindings/watchdog/dw_wdt.txt
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -Synopsys Designware Watchdog Timer
> -
> -Required Properties:
> -
> -- compatible	: Should contain "snps,dw-wdt"
> -- reg		: Base address and size of the watchdog timer registers.
> -- clocks	: phandle + clock-specifier for the clock that drives the
> -		watchdog timer.
> -
> -Optional Properties:
> -
> -- interrupts	: The interrupt used for the watchdog timeout warning.
> -- resets	: phandle pointing to the system reset controller with
> -		line index for the watchdog.
> -
> -Example:
> -
> -	watchdog0: wd@ffd02000 {
> -		compatible = "snps,dw-wdt";
> -		reg = <0xffd02000 0x1000>;
> -		interrupts = <0 171 4>;
> -		clocks = <&per_base_clk>;
> -		resets = <&rst WDT0_RESET>;
> -	};
> diff --git a/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml b/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml
> new file mode 100644
> index 000000000000..8b30f9601c38
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (C) 2019 BAIKAL ELECTRONICS, JSC
> +#
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/snps,dw-wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Synopsys Designware Watchdog Timer
> +
> +allOf:
> +  - $ref: "watchdog.yaml#"
> +
> +maintainers:
> +  - Jamie Iles <jamie@jamieiles.com>
> +
> +properties:
> +  compatible:
> +    const: snps,dw-wdt
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    description: DW Watchdog pre-timeout interrupts.
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1
> +    items:
> +      - description: Watchdog timer reference clock.
> +      - description: APB3 interface clock.
> +
> +  clock-names:
> +    minItems: 1
> +    items:
> +      - const: tclk
> +      - const: pclk
> +
> +  assigned-clocks: true
> +
> +  assigned-clock-rates: true
> +
> +  resets:
> +    description: Phandle to the DW Watchdog reset lane.
> +    maxItems: 1
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +examples:
> +  - |
> +    watchdog0: watchdog@ffd02000 {
> +      compatible = "snps,dw-wdt";
> +      reg = <0xffd02000 0x1000>;
> +      interrupts = <0 171 4>;
> +      clocks = <&per_base_clk>;
> +      resets = <&wdt_rst>;
> +    };
> +...
> 

Re: [PATCH 1/7] dt-bindings: watchdog: dw-wdt: Replace legacy bindings file with YAML-based one

[Sergey Semin]

Hi Guenter,

On Fri, Mar 06, 2020 at 07:18:35AM -0800, Guenter Roeck wrote:
> On 3/6/20 5:27 AM, Sergey.Semin@baikalelectronics.ru wrote:
> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > Modern device tree bindings are supposed to be created as YAML-files
> > in accordane with dt-schema. This commit replaces the DW Watchdog
> > legacy bare text bindings with YAML file. As before the the bindings
> > states that the corresponding dts node is supposed to have a registers
> > range reference, at least one clocks phandle reference, optional reset
> > lines. Seeing all the platforms with DW Watchdog provide the watchdog
> > interrupt property and since in further commit we'll alter the driver
> > to use it for pre-timeout functionality implementation, lets declare
> > the IRQ property to be required.
> > 
> 
> First, this is not just a replacement - it changes semantics.
> 
> Second, I disagree with making interrupts mandatory. They are only needed
> for pretimeout functionality, and not everyone may want to enable that.
> I don't see the point of forcing everyone to enable and provide functionality
> that is neither wanted or needed for a given use case. Yes, the interrupt
> is provided by all users today, but we may have one coming up tomorrow
> where the interrupt line is not even wired up. What then ?

Ok. I'll leave the interrupts optional, though I would have to implement
it in the driver as well. Is this all semantic changes you were referring to?

There is one more change, which you may have also considered as semantic
update. It's async clocks support - "pclk" clock. Would you like me to
unpin this alteration into an additional patch? Rob?

I'll also provide the next fixes in v2:
- single license with GPL-2.0-only,
- remove copyrights (it's not right to add our copyrights here),
- replace "additionalProperties: false" with "unevaluatedProperties: false"
- Remove "assigned-clocks" and "assigned-clock-rates" properties

Regards,
-Sergey

> 
> Guenter
> 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > ---
> >  .../devicetree/bindings/watchdog/dw_wdt.txt   | 24 -------
> >  .../bindings/watchdog/snps,dw-wdt.yaml        | 66 +++++++++++++++++++
> >  2 files changed, 66 insertions(+), 24 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/watchdog/dw_wdt.txt
> >  create mode 100644 Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/watchdog/dw_wdt.txt b/Documentation/devicetree/bindings/watchdog/dw_wdt.txt
> > deleted file mode 100644
> > index eb0914420c7c..000000000000
> > --- a/Documentation/devicetree/bindings/watchdog/dw_wdt.txt
> > +++ /dev/null
> > @@ -1,24 +0,0 @@
> > -Synopsys Designware Watchdog Timer
> > -
> > -Required Properties:
> > -
> > -- compatible	: Should contain "snps,dw-wdt"
> > -- reg		: Base address and size of the watchdog timer registers.
> > -- clocks	: phandle + clock-specifier for the clock that drives the
> > -		watchdog timer.
> > -
> > -Optional Properties:
> > -
> > -- interrupts	: The interrupt used for the watchdog timeout warning.
> > -- resets	: phandle pointing to the system reset controller with
> > -		line index for the watchdog.
> > -
> > -Example:
> > -
> > -	watchdog0: wd@ffd02000 {
> > -		compatible = "snps,dw-wdt";
> > -		reg = <0xffd02000 0x1000>;
> > -		interrupts = <0 171 4>;
> > -		clocks = <&per_base_clk>;
> > -		resets = <&rst WDT0_RESET>;
> > -	};
> > diff --git a/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml b/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml
> > new file mode 100644
> > index 000000000000..8b30f9601c38
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml
> > @@ -0,0 +1,66 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Copyright (C) 2019 BAIKAL ELECTRONICS, JSC
> > +#
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/watchdog/snps,dw-wdt.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Synopsys Designware Watchdog Timer
> > +
> > +allOf:
> > +  - $ref: "watchdog.yaml#"
> > +
> > +maintainers:
> > +  - Jamie Iles <jamie@jamieiles.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: snps,dw-wdt
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    description: DW Watchdog pre-timeout interrupts.
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 1
> > +    items:
> > +      - description: Watchdog timer reference clock.
> > +      - description: APB3 interface clock.
> > +
> > +  clock-names:
> > +    minItems: 1
> > +    items:
> > +      - const: tclk
> > +      - const: pclk
> > +
> > +  assigned-clocks: true
> > +
> > +  assigned-clock-rates: true
> > +
> > +  resets:
> > +    description: Phandle to the DW Watchdog reset lane.
> > +    maxItems: 1
> > +
> > +additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +
> > +examples:
> > +  - |
> > +    watchdog0: watchdog@ffd02000 {
> > +      compatible = "snps,dw-wdt";
> > +      reg = <0xffd02000 0x1000>;
> > +      interrupts = <0 171 4>;
> > +      clocks = <&per_base_clk>;
> > +      resets = <&wdt_rst>;
> > +    };
> > +...
> > 
> 

[PATCH 2/7] dt-bindings: watchdog: dw-wdt: Add watchdog TOPs array property

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

In case if DW Watchdog IP core is built with WDT_USE_FIX_TOP == false,
a custom timeout periods are used to preset the timer counter. In
this case that periods should be specified in a new "snps,watchdog-tops"
property of the DW watchdog dts node.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 .../bindings/watchdog/snps,dw-wdt.yaml        | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml b/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml
index 8b30f9601c38..1b3b71351628 100644
--- a/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml
@@ -46,6 +46,21 @@ properties:
     description: Phandle to the DW Watchdog reset lane.
     maxItems: 1
 
+  snps,watchdog-tops:
+    description: |
+      DW APB Watchdog custom timer intervals - Timeout Period ranges (TOPs).
+      Each TOP is a number loaded into the watchdog counter at the moment of
+      the timer restart. The counter decrementing happens each tick of the
+      reference clock. Therefore the TOPs array is equivalent to an array of
+      the timer expiration intervals supported by the DW APB Watchdog. Note
+      DW APB Watchdog IP-cores might be synthesized with fixed TOP values,
+      in which case this property is unnecessary.
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32-array
+      - items:
+          minItems: 16
+          maxItems: 16
+
 additionalProperties: false
 
 required:
@@ -63,4 +78,19 @@ examples:
       clocks = <&per_base_clk>;
       resets = <&wdt_rst>;
     };
+
+  - |
+    watchdog1: watchdog@ffd02000 {
+      compatible = "snps,dw-wdt";
+      reg = <0xffd02000 0x1000>;
+      interrupts = <0 171 4>;
+      clocks = <&per_base_clk>;
+      clock-names = "tclk";
+      snps,watchdog-tops = <0x000000FF 0x000001FF 0x000003FF
+                            0x000007FF 0x0000FFFF 0x0001FFFF
+                            0x0003FFFF 0x0007FFFF 0x000FFFFF
+                            0x001FFFFF 0x003FFFFF 0x007FFFFF
+                            0x00FFFFFF 0x01FFFFFF 0x03FFFFFF
+                            0x07FFFFFF>;
+    };
 ...
-- 
2.25.1

Re: [PATCH 2/7] dt-bindings: watchdog: dw-wdt: Add watchdog TOPs array property

[Rob Herring]

On Fri, 6 Mar 2020 16:27:42 +0300, <Sergey.Semin@baikalelectronics.ru> wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> In case if DW Watchdog IP core is built with WDT_USE_FIX_TOP == false,
> a custom timeout periods are used to preset the timer counter. In
> this case that periods should be specified in a new "snps,watchdog-tops"
> property of the DW watchdog dts node.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---
>  .../bindings/watchdog/snps,dw-wdt.yaml        | 30 +++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH 3/7] watchdog: watchdog_dev: Use generic msec-per-sec macro

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

For better readability replace the numeric literals with globally
available xSEC_PER_SEC macro.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 drivers/watchdog/watchdog_dev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 8b5c742f24e8..a1a3bbe21653 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -99,7 +99,7 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
 {
 	/* All variables in milli-seconds */
 	unsigned int hm = wdd->max_hw_heartbeat_ms;
-	unsigned int t = wdd->timeout * 1000;
+	unsigned int t = wdd->timeout * MSEC_PER_SEC;
 
 	/*
 	 * A worker to generate heartbeat requests is needed if all of the
@@ -121,7 +121,7 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
 static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd)
 {
 	struct watchdog_core_data *wd_data = wdd->wd_data;
-	unsigned int timeout_ms = wdd->timeout * 1000;
+	unsigned int timeout_ms = wdd->timeout * MSEC_PER_SEC;
 	ktime_t keepalive_interval;
 	ktime_t last_heartbeat, latest_heartbeat;
 	ktime_t virt_timeout;
-- 
2.25.1

Re: [PATCH 3/7] watchdog: watchdog_dev: Use generic msec-per-sec macro

[Guenter Roeck]

On 3/6/20 5:27 AM, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> For better readability replace the numeric literals with globally
> available xSEC_PER_SEC macro.
> 

This is really completely unrelated to the rest of the series,
and I don't really see the point. I am fine with such changes if there
are some context changes around it, but otherwise they add no value
other than being a potential source of backport conflicts.

Guenter

> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---
>  drivers/watchdog/watchdog_dev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 8b5c742f24e8..a1a3bbe21653 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -99,7 +99,7 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>  {
>  	/* All variables in milli-seconds */
>  	unsigned int hm = wdd->max_hw_heartbeat_ms;
> -	unsigned int t = wdd->timeout * 1000;
> +	unsigned int t = wdd->timeout * MSEC_PER_SEC;
>  
>  	/*
>  	 * A worker to generate heartbeat requests is needed if all of the
> @@ -121,7 +121,7 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>  static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd)
>  {
>  	struct watchdog_core_data *wd_data = wdd->wd_data;
> -	unsigned int timeout_ms = wdd->timeout * 1000;
> +	unsigned int timeout_ms = wdd->timeout * MSEC_PER_SEC;
>  	ktime_t keepalive_interval;
>  	ktime_t last_heartbeat, latest_heartbeat;
>  	ktime_t virt_timeout;
> 

Re: [PATCH 3/7] watchdog: watchdog_dev: Use generic msec-per-sec macro

[Sergey Semin]

On Fri, Mar 06, 2020 at 07:20:29AM -0800, Guenter Roeck wrote:
> On 3/6/20 5:27 AM, Sergey.Semin@baikalelectronics.ru wrote:
> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > For better readability replace the numeric literals with globally
> > available xSEC_PER_SEC macro.
> > 
> 
> This is really completely unrelated to the rest of the series,
> and I don't really see the point. I am fine with such changes if there
> are some context changes around it, but otherwise they add no value
> other than being a potential source of backport conflicts.

It's up to you, since you are the subsystem maintainer. I'll drop the patch
in v2.

-Sergey

> 
> Guenter
> 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > ---
> >  drivers/watchdog/watchdog_dev.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> > index 8b5c742f24e8..a1a3bbe21653 100644
> > --- a/drivers/watchdog/watchdog_dev.c
> > +++ b/drivers/watchdog/watchdog_dev.c
> > @@ -99,7 +99,7 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
> >  {
> >  	/* All variables in milli-seconds */
> >  	unsigned int hm = wdd->max_hw_heartbeat_ms;
> > -	unsigned int t = wdd->timeout * 1000;
> > +	unsigned int t = wdd->timeout * MSEC_PER_SEC;
> >  
> >  	/*
> >  	 * A worker to generate heartbeat requests is needed if all of the
> > @@ -121,7 +121,7 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
> >  static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd)
> >  {
> >  	struct watchdog_core_data *wd_data = wdd->wd_data;
> > -	unsigned int timeout_ms = wdd->timeout * 1000;
> > +	unsigned int timeout_ms = wdd->timeout * MSEC_PER_SEC;
> >  	ktime_t keepalive_interval;
> >  	ktime_t last_heartbeat, latest_heartbeat;
> >  	ktime_t virt_timeout;
> > 
> 

[PATCH 4/7] watchdog: dw_wdt: Support devices with non-fixed TOP values

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

In case if the DW Watchdog IP core is synthesised with
WDT_USE_FIX_TOP == false, the TOP interval indexes make the device
to load a custom periods to the counter. These periods are hardwired
at the synthesis stage and can be within [2^8, 2^(WDT_CNT_WIDTH - 1)].
Alas their values can't be detected at runtime and must be somehow
supplied to the driver so one could properly determine the watchdog
timeout intervals. For this purpose we suggest to have a vendor-
specific dts property "snps,watchdog-tops" utilized, which would
provide an array of sixteen counter values. At device probe stage they
will be used to initialize the watchdog device timeouts determined
from the array values and current clocks source rate.

In order to have custom TOP values supported the driver must be
altered in the following way. First of all the fixed-top values
ready-to-use array must be determined for compatibility with currently
supported devices, which were synthesised with WDT_USE_FIX_TOP == true.
Secondly we must redefine the timer period search functions. For
generality they are redesigned in a way to support the TOP array with
no limitations on the items order or value. Finally an array with
pre-defined timeouts must be calculated at probe stage from either
the custom or fixed TOP values depending on the DW watchdog component
parameter WDT_USE_FIX_TOP value.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 drivers/watchdog/dw_wdt.c | 145 +++++++++++++++++++++++++++++++-------
 1 file changed, 119 insertions(+), 26 deletions(-)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index fba21de2bbad..4a57b7d777dc 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/limits.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/err.h>
@@ -34,12 +35,24 @@
 #define WDOG_CURRENT_COUNT_REG_OFFSET	    0x08
 #define WDOG_COUNTER_RESTART_REG_OFFSET     0x0c
 #define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
+#define WDOG_COMP_PARAMS_1_REG_OFFSET       0xf4
+#define WDOG_COMP_PARAMS_1_USE_FIX_TOP      BIT(6)
 
-/* The maximum TOP (timeout period) value that can be set in the watchdog. */
-#define DW_WDT_MAX_TOP		15
+/* There are sixteen TOPs (timeout periods) that can be set in the watchdog. */
+#define DW_WDT_NUM_TOPS		16
+#define DW_WDT_FIX_TOP(_idx)	(1U << (16 + _idx))
 
 #define DW_WDT_DEFAULT_SECONDS	30
 
+static const u32 dw_wdt_fix_tops[DW_WDT_NUM_TOPS] = {
+	DW_WDT_FIX_TOP(0), DW_WDT_FIX_TOP(1), DW_WDT_FIX_TOP(2),
+	DW_WDT_FIX_TOP(3), DW_WDT_FIX_TOP(4), DW_WDT_FIX_TOP(5),
+	DW_WDT_FIX_TOP(6), DW_WDT_FIX_TOP(7), DW_WDT_FIX_TOP(8),
+	DW_WDT_FIX_TOP(9), DW_WDT_FIX_TOP(10), DW_WDT_FIX_TOP(11),
+	DW_WDT_FIX_TOP(12), DW_WDT_FIX_TOP(13), DW_WDT_FIX_TOP(14),
+	DW_WDT_FIX_TOP(15)
+};
+
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
@@ -49,6 +62,8 @@ struct dw_wdt {
 	void __iomem		*regs;
 	struct clk		*clk;
 	unsigned long		rate;
+	unsigned int		max_top;
+	unsigned int		timeouts[DW_WDT_NUM_TOPS];
 	struct watchdog_device	wdd;
 	struct reset_control	*rst;
 	/* Save/restore */
@@ -64,20 +79,68 @@ static inline int dw_wdt_is_enabled(struct dw_wdt *dw_wdt)
 		WDOG_CONTROL_REG_WDT_EN_MASK;
 }
 
-static inline int dw_wdt_top_in_seconds(struct dw_wdt *dw_wdt, unsigned top)
+static unsigned int dw_wdt_find_best_top(struct dw_wdt *dw_wdt,
+					 unsigned int timeout, u32 *top)
 {
+	u32 diff = UINT_MAX, tmp;
+	int idx;
+
 	/*
-	 * There are 16 possible timeout values in 0..15 where the number of
-	 * cycles is 2 ^ (16 + i) and the watchdog counts down.
+	 * In general case of non-fixed timeout values they can be arranged in
+	 * any order so we have to traverse all the array values. We also try
+	 * to find a closest timeout number and make sure its value is greater
+	 * than the requested timeout. Note we'll return a maximum timeout
+	 * if reachable value couldn't be found.
 	 */
-	return (1U << (16 + top)) / dw_wdt->rate;
+	for (*top = dw_wdt->max_top, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
+		if (dw_wdt->timeouts[idx] < timeout)
+			continue;
+
+		tmp = dw_wdt->timeouts[idx] - timeout;
+		if (tmp < diff) {
+			diff = tmp;
+			*top = idx;
+		}
+	}
+
+	return dw_wdt->timeouts[*top];
+}
+
+static unsigned int dw_wdt_find_min_timeout(struct dw_wdt *dw_wdt)
+{
+	u32 min_timeout = UINT_MAX, top;
+	int idx;
+
+	for (top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
+		if (dw_wdt->timeouts[idx] <= min_timeout) {
+			min_timeout = dw_wdt->timeouts[idx];
+			top = idx;
+		}
+	}
+
+	return dw_wdt->timeouts[top];
+}
+
+static unsigned int dw_wdt_find_max_top(struct dw_wdt *dw_wdt, u32 *top)
+{
+	u32 max_timeout = 0;
+	int idx;
+
+	for (*top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
+		if (dw_wdt->timeouts[idx] >= max_timeout) {
+			max_timeout = dw_wdt->timeouts[idx];
+			*top = idx;
+		}
+	}
+
+	return dw_wdt->timeouts[*top];
 }
 
-static int dw_wdt_get_top(struct dw_wdt *dw_wdt)
+static unsigned int dw_wdt_get_timeout(struct dw_wdt *dw_wdt)
 {
 	int top = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
 
-	return dw_wdt_top_in_seconds(dw_wdt, top);
+	return dw_wdt->timeouts[top];
 }
 
 static int dw_wdt_ping(struct watchdog_device *wdd)
@@ -90,20 +153,13 @@ static int dw_wdt_ping(struct watchdog_device *wdd)
 	return 0;
 }
 
-static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
+static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
 {
 	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
-	int i, top_val = DW_WDT_MAX_TOP;
+	unsigned int timeout;
+	u32 top;
 
-	/*
-	 * Iterate over the timeout values until we find the closest match. We
-	 * always look for >=.
-	 */
-	for (i = 0; i <= DW_WDT_MAX_TOP; ++i)
-		if (dw_wdt_top_in_seconds(dw_wdt, i) >= top_s) {
-			top_val = i;
-			break;
-		}
+	timeout = dw_wdt_find_best_top(dw_wdt, req * MSEC_PER_SEC, &top);
 
 	/*
 	 * Set the new value in the watchdog.  Some versions of dw_wdt
@@ -111,7 +167,7 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
 	 * CP_WDT_DUAL_TOP in WDT_COMP_PARAMS_1).  On those we
 	 * effectively get a pat of the watchdog right here.
 	 */
-	writel(top_val | top_val << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
+	writel(top | top << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
 	       dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
 
 	/*
@@ -119,10 +175,10 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
 	 * kernel(watchdog_dev.c) helps to feed watchdog before
 	 * wdd->max_hw_heartbeat_ms
 	 */
-	if (top_s * 1000 <= wdd->max_hw_heartbeat_ms)
-		wdd->timeout = dw_wdt_top_in_seconds(dw_wdt, top_val);
+	if (req * MSEC_PER_SEC > wdd->max_hw_heartbeat_ms)
+		wdd->timeout = req;
 	else
-		wdd->timeout = top_s;
+		wdd->timeout = timeout / MSEC_PER_SEC;
 
 	return 0;
 }
@@ -238,6 +294,41 @@ static int dw_wdt_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(dw_wdt_pm_ops, dw_wdt_suspend, dw_wdt_resume);
 
+static void dw_wdt_init_timeouts(struct dw_wdt *dw_wdt, struct device *dev)
+{
+	u32 data, of_tops[DW_WDT_NUM_TOPS];
+	const u32 *tops;
+	int ret, idx;
+
+	/*
+	 * Retrieve custom or fixed counter values depending on the
+	 * WDT_USE_FIX_TOP flag found in the component specific parameters
+	 * #1 register.
+	 */
+	data = readl(dw_wdt->regs + WDOG_COMP_PARAMS_1_REG_OFFSET);
+	if (data & WDOG_COMP_PARAMS_1_USE_FIX_TOP) {
+		tops = dw_wdt_fix_tops;
+	} else {
+		ret = of_property_read_variable_u32_array(dev_of_node(dev),
+			"snps,watchdog-tops", of_tops, DW_WDT_NUM_TOPS,
+			DW_WDT_NUM_TOPS);
+		if (ret < 0) {
+			dev_warn(dev, "No valid TOPs array specified\n");
+			tops = dw_wdt_fix_tops;
+		} else {
+			tops = of_tops;
+		}
+	}
+
+	/*
+	 * We'll keep the timeout values in ms to approximate requested
+	 * timeouts with better accuracy.
+	 */
+	for (idx = 0; idx < DW_WDT_NUM_TOPS; ++idx)
+		dw_wdt->timeouts[idx] =
+			mult_frac(tops[idx], MSEC_PER_SEC, dw_wdt->rate);
+}
+
 static int dw_wdt_drv_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -275,12 +366,14 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 
 	reset_control_deassert(dw_wdt->rst);
 
+	dw_wdt_init_timeouts(dw_wdt, dev);
+
 	wdd = &dw_wdt->wdd;
 	wdd->info = &dw_wdt_ident;
 	wdd->ops = &dw_wdt_ops;
-	wdd->min_timeout = 1;
+	wdd->min_timeout = dw_wdt_find_min_timeout(dw_wdt) / MSEC_PER_SEC;
 	wdd->max_hw_heartbeat_ms =
-		dw_wdt_top_in_seconds(dw_wdt, DW_WDT_MAX_TOP) * 1000;
+		dw_wdt_find_max_top(dw_wdt, &dw_wdt->max_top);
 	wdd->parent = dev;
 
 	watchdog_set_drvdata(wdd, dw_wdt);
@@ -293,7 +386,7 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 	 * devicetree.
 	 */
 	if (dw_wdt_is_enabled(dw_wdt)) {
-		wdd->timeout = dw_wdt_get_top(dw_wdt);
+		wdd->timeout = dw_wdt_get_timeout(dw_wdt) / MSEC_PER_SEC;
 		set_bit(WDOG_HW_RUNNING, &wdd->status);
 	} else {
 		wdd->timeout = DW_WDT_DEFAULT_SECONDS;
-- 
2.25.1

Re: [PATCH 4/7] watchdog: dw_wdt: Support devices with non-fixed TOP values

[Guenter Roeck]

On Fri, Mar 06, 2020 at 04:27:44PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> In case if the DW Watchdog IP core is synthesised with
> WDT_USE_FIX_TOP == false, the TOP interval indexes make the device
> to load a custom periods to the counter. These periods are hardwired
> at the synthesis stage and can be within [2^8, 2^(WDT_CNT_WIDTH - 1)].
> Alas their values can't be detected at runtime and must be somehow
> supplied to the driver so one could properly determine the watchdog
> timeout intervals. For this purpose we suggest to have a vendor-
> specific dts property "snps,watchdog-tops" utilized, which would
> provide an array of sixteen counter values. At device probe stage they
> will be used to initialize the watchdog device timeouts determined
> from the array values and current clocks source rate.
> 
> In order to have custom TOP values supported the driver must be
> altered in the following way. First of all the fixed-top values
> ready-to-use array must be determined for compatibility with currently
> supported devices, which were synthesised with WDT_USE_FIX_TOP == true.
> Secondly we must redefine the timer period search functions. For
> generality they are redesigned in a way to support the TOP array with
> no limitations on the items order or value. Finally an array with
> pre-defined timeouts must be calculated at probe stage from either
> the custom or fixed TOP values depending on the DW watchdog component
> parameter WDT_USE_FIX_TOP value.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---
>  drivers/watchdog/dw_wdt.c | 145 +++++++++++++++++++++++++++++++-------
>  1 file changed, 119 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index fba21de2bbad..4a57b7d777dc 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <linux/bitops.h>
> +#include <linux/limits.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
> @@ -34,12 +35,24 @@
>  #define WDOG_CURRENT_COUNT_REG_OFFSET	    0x08
>  #define WDOG_COUNTER_RESTART_REG_OFFSET     0x0c
>  #define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
> +#define WDOG_COMP_PARAMS_1_REG_OFFSET       0xf4
> +#define WDOG_COMP_PARAMS_1_USE_FIX_TOP      BIT(6)
>  
> -/* The maximum TOP (timeout period) value that can be set in the watchdog. */
> -#define DW_WDT_MAX_TOP		15
> +/* There are sixteen TOPs (timeout periods) that can be set in the watchdog. */
> +#define DW_WDT_NUM_TOPS		16
> +#define DW_WDT_FIX_TOP(_idx)	(1U << (16 + _idx))
>  
>  #define DW_WDT_DEFAULT_SECONDS	30
>  
> +static const u32 dw_wdt_fix_tops[DW_WDT_NUM_TOPS] = {
> +	DW_WDT_FIX_TOP(0), DW_WDT_FIX_TOP(1), DW_WDT_FIX_TOP(2),
> +	DW_WDT_FIX_TOP(3), DW_WDT_FIX_TOP(4), DW_WDT_FIX_TOP(5),
> +	DW_WDT_FIX_TOP(6), DW_WDT_FIX_TOP(7), DW_WDT_FIX_TOP(8),
> +	DW_WDT_FIX_TOP(9), DW_WDT_FIX_TOP(10), DW_WDT_FIX_TOP(11),
> +	DW_WDT_FIX_TOP(12), DW_WDT_FIX_TOP(13), DW_WDT_FIX_TOP(14),
> +	DW_WDT_FIX_TOP(15)
> +};
> +
>  static bool nowayout = WATCHDOG_NOWAYOUT;
>  module_param(nowayout, bool, 0);
>  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> @@ -49,6 +62,8 @@ struct dw_wdt {
>  	void __iomem		*regs;
>  	struct clk		*clk;
>  	unsigned long		rate;
> +	unsigned int		max_top;
> +	unsigned int		timeouts[DW_WDT_NUM_TOPS];
>  	struct watchdog_device	wdd;
>  	struct reset_control	*rst;
>  	/* Save/restore */
> @@ -64,20 +79,68 @@ static inline int dw_wdt_is_enabled(struct dw_wdt *dw_wdt)
>  		WDOG_CONTROL_REG_WDT_EN_MASK;
>  }
>  
> -static inline int dw_wdt_top_in_seconds(struct dw_wdt *dw_wdt, unsigned top)
> +static unsigned int dw_wdt_find_best_top(struct dw_wdt *dw_wdt,
> +					 unsigned int timeout, u32 *top)
>  {
> +	u32 diff = UINT_MAX, tmp;
> +	int idx;
> +
>  	/*
> -	 * There are 16 possible timeout values in 0..15 where the number of
> -	 * cycles is 2 ^ (16 + i) and the watchdog counts down.
> +	 * In general case of non-fixed timeout values they can be arranged in
> +	 * any order so we have to traverse all the array values. We also try
> +	 * to find a closest timeout number and make sure its value is greater
> +	 * than the requested timeout. Note we'll return a maximum timeout
> +	 * if reachable value couldn't be found.
>  	 */
> -	return (1U << (16 + top)) / dw_wdt->rate;
> +	for (*top = dw_wdt->max_top, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
> +		if (dw_wdt->timeouts[idx] < timeout)
> +			continue;
> +
> +		tmp = dw_wdt->timeouts[idx] - timeout;
> +		if (tmp < diff) {
> +			diff = tmp;
> +			*top = idx;
> +		}
> +	}
> +
> +	return dw_wdt->timeouts[*top];
> +}
> +
> +static unsigned int dw_wdt_find_min_timeout(struct dw_wdt *dw_wdt)

I would appreciate if the names of functions returning ms end with _ms
to avoid confusion.

> +{
> +	u32 min_timeout = UINT_MAX, top;
> +	int idx;
> +
> +	for (top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
> +		if (dw_wdt->timeouts[idx] <= min_timeout) {
> +			min_timeout = dw_wdt->timeouts[idx];
> +			top = idx;
> +		}
> +	}
> +
> +	return dw_wdt->timeouts[top];
> +}
> +
> +static unsigned int dw_wdt_find_max_top(struct dw_wdt *dw_wdt, u32 *top)
> +{
> +	u32 max_timeout = 0;
> +	int idx;
> +
> +	for (*top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
> +		if (dw_wdt->timeouts[idx] >= max_timeout) {
> +			max_timeout = dw_wdt->timeouts[idx];
> +			*top = idx;
> +		}
> +	}
> +
> +	return dw_wdt->timeouts[*top];
>  }
>  
> -static int dw_wdt_get_top(struct dw_wdt *dw_wdt)
> +static unsigned int dw_wdt_get_timeout(struct dw_wdt *dw_wdt)
>  {
>  	int top = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
>  
> -	return dw_wdt_top_in_seconds(dw_wdt, top);
> +	return dw_wdt->timeouts[top];
>  }
>  
>  static int dw_wdt_ping(struct watchdog_device *wdd)
> @@ -90,20 +153,13 @@ static int dw_wdt_ping(struct watchdog_device *wdd)
>  	return 0;
>  }
>  
> -static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> +static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
>  {
>  	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> -	int i, top_val = DW_WDT_MAX_TOP;
> +	unsigned int timeout;
> +	u32 top;
>  
> -	/*
> -	 * Iterate over the timeout values until we find the closest match. We
> -	 * always look for >=.
> -	 */
> -	for (i = 0; i <= DW_WDT_MAX_TOP; ++i)
> -		if (dw_wdt_top_in_seconds(dw_wdt, i) >= top_s) {
> -			top_val = i;
> -			break;
> -		}
> +	timeout = dw_wdt_find_best_top(dw_wdt, req * MSEC_PER_SEC, &top);
>  
>  	/*
>  	 * Set the new value in the watchdog.  Some versions of dw_wdt
> @@ -111,7 +167,7 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
>  	 * CP_WDT_DUAL_TOP in WDT_COMP_PARAMS_1).  On those we
>  	 * effectively get a pat of the watchdog right here.
>  	 */
> -	writel(top_val | top_val << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
> +	writel(top | top << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
>  	       dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
>  
>  	/*
> @@ -119,10 +175,10 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
>  	 * kernel(watchdog_dev.c) helps to feed watchdog before
>  	 * wdd->max_hw_heartbeat_ms
>  	 */
> -	if (top_s * 1000 <= wdd->max_hw_heartbeat_ms)
> -		wdd->timeout = dw_wdt_top_in_seconds(dw_wdt, top_val);
> +	if (req * MSEC_PER_SEC > wdd->max_hw_heartbeat_ms)
> +		wdd->timeout = req;
>  	else
> -		wdd->timeout = top_s;
> +		wdd->timeout = timeout / MSEC_PER_SEC;
>  
>  	return 0;
>  }
> @@ -238,6 +294,41 @@ static int dw_wdt_resume(struct device *dev)
>  
>  static SIMPLE_DEV_PM_OPS(dw_wdt_pm_ops, dw_wdt_suspend, dw_wdt_resume);
>  
> +static void dw_wdt_init_timeouts(struct dw_wdt *dw_wdt, struct device *dev)
> +{
> +	u32 data, of_tops[DW_WDT_NUM_TOPS];
> +	const u32 *tops;
> +	int ret, idx;
> +
> +	/*
> +	 * Retrieve custom or fixed counter values depending on the
> +	 * WDT_USE_FIX_TOP flag found in the component specific parameters
> +	 * #1 register.
> +	 */
> +	data = readl(dw_wdt->regs + WDOG_COMP_PARAMS_1_REG_OFFSET);
> +	if (data & WDOG_COMP_PARAMS_1_USE_FIX_TOP) {
> +		tops = dw_wdt_fix_tops;
> +	} else {
> +		ret = of_property_read_variable_u32_array(dev_of_node(dev),
> +			"snps,watchdog-tops", of_tops, DW_WDT_NUM_TOPS,
> +			DW_WDT_NUM_TOPS);
> +		if (ret < 0) {
> +			dev_warn(dev, "No valid TOPs array specified\n");
> +			tops = dw_wdt_fix_tops;
> +		} else {
> +			tops = of_tops;
> +		}
> +	}
> +
> +	/*
> +	 * We'll keep the timeout values in ms to approximate requested
> +	 * timeouts with better accuracy.
> +	 */
> +	for (idx = 0; idx < DW_WDT_NUM_TOPS; ++idx)
> +		dw_wdt->timeouts[idx] =
> +			mult_frac(tops[idx], MSEC_PER_SEC, dw_wdt->rate);

tops[idx] type is u32. Its value can be up to 0xffffffff. That means
dw_wdt->rate must be >= 1000 to avoid overflow, which you should check.

Note that I don't see the point of keeping the timeout values in ms.
The code selects a larger value (in seconds) anyway. All this does is
to add a lot of multiply and divide operations, plus a source of bugs
and confusion, for little if any gain.

> +}
> +
>  static int dw_wdt_drv_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -275,12 +366,14 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>  
>  	reset_control_deassert(dw_wdt->rst);
>  
> +	dw_wdt_init_timeouts(dw_wdt, dev);
> +
>  	wdd = &dw_wdt->wdd;
>  	wdd->info = &dw_wdt_ident;
>  	wdd->ops = &dw_wdt_ops;
> -	wdd->min_timeout = 1;
> +	wdd->min_timeout = dw_wdt_find_min_timeout(dw_wdt) / MSEC_PER_SEC;

dw_wdt_find_min_timeout can return a value < 1000. In that case min_timeout
would be 0, ie unspecified.

>  	wdd->max_hw_heartbeat_ms =
> -		dw_wdt_top_in_seconds(dw_wdt, DW_WDT_MAX_TOP) * 1000;
> +		dw_wdt_find_max_top(dw_wdt, &dw_wdt->max_top);
>  	wdd->parent = dev;
>  
>  	watchdog_set_drvdata(wdd, dw_wdt);
> @@ -293,7 +386,7 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>  	 * devicetree.
>  	 */
>  	if (dw_wdt_is_enabled(dw_wdt)) {
> -		wdd->timeout = dw_wdt_get_top(dw_wdt);
> +		wdd->timeout = dw_wdt_get_timeout(dw_wdt) / MSEC_PER_SEC;
>  		set_bit(WDOG_HW_RUNNING, &wdd->status);
>  	} else {
>  		wdd->timeout = DW_WDT_DEFAULT_SECONDS;

Re: [PATCH 4/7] watchdog: dw_wdt: Support devices with non-fixed TOP values

[Sergey Semin]

On Sun, Mar 15, 2020 at 07:12:38AM -0700, Guenter Roeck wrote:
> On Fri, Mar 06, 2020 at 04:27:44PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > In case if the DW Watchdog IP core is synthesised with
> > WDT_USE_FIX_TOP == false, the TOP interval indexes make the device
> > to load a custom periods to the counter. These periods are hardwired
> > at the synthesis stage and can be within [2^8, 2^(WDT_CNT_WIDTH - 1)].
> > Alas their values can't be detected at runtime and must be somehow
> > supplied to the driver so one could properly determine the watchdog
> > timeout intervals. For this purpose we suggest to have a vendor-
> > specific dts property "snps,watchdog-tops" utilized, which would
> > provide an array of sixteen counter values. At device probe stage they
> > will be used to initialize the watchdog device timeouts determined
> > from the array values and current clocks source rate.
> > 
> > In order to have custom TOP values supported the driver must be
> > altered in the following way. First of all the fixed-top values
> > ready-to-use array must be determined for compatibility with currently
> > supported devices, which were synthesised with WDT_USE_FIX_TOP == true.
> > Secondly we must redefine the timer period search functions. For
> > generality they are redesigned in a way to support the TOP array with
> > no limitations on the items order or value. Finally an array with
> > pre-defined timeouts must be calculated at probe stage from either
> > the custom or fixed TOP values depending on the DW watchdog component
> > parameter WDT_USE_FIX_TOP value.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > ---
> >  drivers/watchdog/dw_wdt.c | 145 +++++++++++++++++++++++++++++++-------
> >  1 file changed, 119 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> > index fba21de2bbad..4a57b7d777dc 100644
> > --- a/drivers/watchdog/dw_wdt.c
> > +++ b/drivers/watchdog/dw_wdt.c
> > @@ -13,6 +13,7 @@
> >   */
> >  
> >  #include <linux/bitops.h>
> > +#include <linux/limits.h>
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> >  #include <linux/err.h>
> > @@ -34,12 +35,24 @@
> >  #define WDOG_CURRENT_COUNT_REG_OFFSET	    0x08
> >  #define WDOG_COUNTER_RESTART_REG_OFFSET     0x0c
> >  #define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
> > +#define WDOG_COMP_PARAMS_1_REG_OFFSET       0xf4
> > +#define WDOG_COMP_PARAMS_1_USE_FIX_TOP      BIT(6)
> >  
> > -/* The maximum TOP (timeout period) value that can be set in the watchdog. */
> > -#define DW_WDT_MAX_TOP		15
> > +/* There are sixteen TOPs (timeout periods) that can be set in the watchdog. */
> > +#define DW_WDT_NUM_TOPS		16
> > +#define DW_WDT_FIX_TOP(_idx)	(1U << (16 + _idx))
> >  
> >  #define DW_WDT_DEFAULT_SECONDS	30
> >  
> > +static const u32 dw_wdt_fix_tops[DW_WDT_NUM_TOPS] = {
> > +	DW_WDT_FIX_TOP(0), DW_WDT_FIX_TOP(1), DW_WDT_FIX_TOP(2),
> > +	DW_WDT_FIX_TOP(3), DW_WDT_FIX_TOP(4), DW_WDT_FIX_TOP(5),
> > +	DW_WDT_FIX_TOP(6), DW_WDT_FIX_TOP(7), DW_WDT_FIX_TOP(8),
> > +	DW_WDT_FIX_TOP(9), DW_WDT_FIX_TOP(10), DW_WDT_FIX_TOP(11),
> > +	DW_WDT_FIX_TOP(12), DW_WDT_FIX_TOP(13), DW_WDT_FIX_TOP(14),
> > +	DW_WDT_FIX_TOP(15)
> > +};
> > +
> >  static bool nowayout = WATCHDOG_NOWAYOUT;
> >  module_param(nowayout, bool, 0);
> >  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> > @@ -49,6 +62,8 @@ struct dw_wdt {
> >  	void __iomem		*regs;
> >  	struct clk		*clk;
> >  	unsigned long		rate;
> > +	unsigned int		max_top;
> > +	unsigned int		timeouts[DW_WDT_NUM_TOPS];
> >  	struct watchdog_device	wdd;
> >  	struct reset_control	*rst;
> >  	/* Save/restore */
> > @@ -64,20 +79,68 @@ static inline int dw_wdt_is_enabled(struct dw_wdt *dw_wdt)
> >  		WDOG_CONTROL_REG_WDT_EN_MASK;
> >  }
> >  
> > -static inline int dw_wdt_top_in_seconds(struct dw_wdt *dw_wdt, unsigned top)
> > +static unsigned int dw_wdt_find_best_top(struct dw_wdt *dw_wdt,
> > +					 unsigned int timeout, u32 *top)
> >  {
> > +	u32 diff = UINT_MAX, tmp;
> > +	int idx;
> > +
> >  	/*
> > -	 * There are 16 possible timeout values in 0..15 where the number of
> > -	 * cycles is 2 ^ (16 + i) and the watchdog counts down.
> > +	 * In general case of non-fixed timeout values they can be arranged in
> > +	 * any order so we have to traverse all the array values. We also try
> > +	 * to find a closest timeout number and make sure its value is greater
> > +	 * than the requested timeout. Note we'll return a maximum timeout
> > +	 * if reachable value couldn't be found.
> >  	 */
> > -	return (1U << (16 + top)) / dw_wdt->rate;
> > +	for (*top = dw_wdt->max_top, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
> > +		if (dw_wdt->timeouts[idx] < timeout)
> > +			continue;
> > +
> > +		tmp = dw_wdt->timeouts[idx] - timeout;
> > +		if (tmp < diff) {
> > +			diff = tmp;
> > +			*top = idx;
> > +		}
> > +	}
> > +
> > +	return dw_wdt->timeouts[*top];
> > +}
> > +
> > +static unsigned int dw_wdt_find_min_timeout(struct dw_wdt *dw_wdt)
> 
> I would appreciate if the names of functions returning ms end with _ms
> to avoid confusion.

Ok. I'll also modify the functions a bit, so only the
dw_wdt_find_best_top_ms() and dw_wdt_find_max_top_ms() methods would
return timeouts in milliseconds. Though if you insist in keeping seconds
in the timeouts array (see the comment after the next one), it'll be
dw_wdt_find_max_top_ms() only.

> 
> > +{
> > +	u32 min_timeout = UINT_MAX, top;
> > +	int idx;
> > +
> > +	for (top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
> > +		if (dw_wdt->timeouts[idx] <= min_timeout) {
> > +			min_timeout = dw_wdt->timeouts[idx];
> > +			top = idx;
> > +		}
> > +	}
> > +
> > +	return dw_wdt->timeouts[top];
> > +}
> > +
> > +static unsigned int dw_wdt_find_max_top(struct dw_wdt *dw_wdt, u32 *top)
> > +{
> > +	u32 max_timeout = 0;
> > +	int idx;
> > +
> > +	for (*top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
> > +		if (dw_wdt->timeouts[idx] >= max_timeout) {
> > +			max_timeout = dw_wdt->timeouts[idx];
> > +			*top = idx;
> > +		}
> > +	}
> > +
> > +	return dw_wdt->timeouts[*top];
> >  }
> >  
> > -static int dw_wdt_get_top(struct dw_wdt *dw_wdt)
> > +static unsigned int dw_wdt_get_timeout(struct dw_wdt *dw_wdt)
> >  {
> >  	int top = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
> >  
> > -	return dw_wdt_top_in_seconds(dw_wdt, top);
> > +	return dw_wdt->timeouts[top];
> >  }
> >  
> >  static int dw_wdt_ping(struct watchdog_device *wdd)
> > @@ -90,20 +153,13 @@ static int dw_wdt_ping(struct watchdog_device *wdd)
> >  	return 0;
> >  }
> >  
> > -static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> > +static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
> >  {
> >  	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> > -	int i, top_val = DW_WDT_MAX_TOP;
> > +	unsigned int timeout;
> > +	u32 top;
> >  
> > -	/*
> > -	 * Iterate over the timeout values until we find the closest match. We
> > -	 * always look for >=.
> > -	 */
> > -	for (i = 0; i <= DW_WDT_MAX_TOP; ++i)
> > -		if (dw_wdt_top_in_seconds(dw_wdt, i) >= top_s) {
> > -			top_val = i;
> > -			break;
> > -		}
> > +	timeout = dw_wdt_find_best_top(dw_wdt, req * MSEC_PER_SEC, &top);
> >  
> >  	/*
> >  	 * Set the new value in the watchdog.  Some versions of dw_wdt
> > @@ -111,7 +167,7 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> >  	 * CP_WDT_DUAL_TOP in WDT_COMP_PARAMS_1).  On those we
> >  	 * effectively get a pat of the watchdog right here.
> >  	 */
> > -	writel(top_val | top_val << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
> > +	writel(top | top << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
> >  	       dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> >  
> >  	/*
> > @@ -119,10 +175,10 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> >  	 * kernel(watchdog_dev.c) helps to feed watchdog before
> >  	 * wdd->max_hw_heartbeat_ms
> >  	 */
> > -	if (top_s * 1000 <= wdd->max_hw_heartbeat_ms)
> > -		wdd->timeout = dw_wdt_top_in_seconds(dw_wdt, top_val);
> > +	if (req * MSEC_PER_SEC > wdd->max_hw_heartbeat_ms)
> > +		wdd->timeout = req;
> >  	else
> > -		wdd->timeout = top_s;
> > +		wdd->timeout = timeout / MSEC_PER_SEC;
> >  
> >  	return 0;
> >  }
> > @@ -238,6 +294,41 @@ static int dw_wdt_resume(struct device *dev)
> >  
> >  static SIMPLE_DEV_PM_OPS(dw_wdt_pm_ops, dw_wdt_suspend, dw_wdt_resume);
> >  
> > +static void dw_wdt_init_timeouts(struct dw_wdt *dw_wdt, struct device *dev)
> > +{
> > +	u32 data, of_tops[DW_WDT_NUM_TOPS];
> > +	const u32 *tops;
> > +	int ret, idx;
> > +
> > +	/*
> > +	 * Retrieve custom or fixed counter values depending on the
> > +	 * WDT_USE_FIX_TOP flag found in the component specific parameters
> > +	 * #1 register.
> > +	 */
> > +	data = readl(dw_wdt->regs + WDOG_COMP_PARAMS_1_REG_OFFSET);
> > +	if (data & WDOG_COMP_PARAMS_1_USE_FIX_TOP) {
> > +		tops = dw_wdt_fix_tops;
> > +	} else {
> > +		ret = of_property_read_variable_u32_array(dev_of_node(dev),
> > +			"snps,watchdog-tops", of_tops, DW_WDT_NUM_TOPS,
> > +			DW_WDT_NUM_TOPS);
> > +		if (ret < 0) {
> > +			dev_warn(dev, "No valid TOPs array specified\n");
> > +			tops = dw_wdt_fix_tops;
> > +		} else {
> > +			tops = of_tops;
> > +		}
> > +	}
> > +
> > +	/*
> > +	 * We'll keep the timeout values in ms to approximate requested
> > +	 * timeouts with better accuracy.
> > +	 */
> > +	for (idx = 0; idx < DW_WDT_NUM_TOPS; ++idx)
> > +		dw_wdt->timeouts[idx] =
> > +			mult_frac(tops[idx], MSEC_PER_SEC, dw_wdt->rate);
> 
> tops[idx] type is u32. Its value can be up to 0xffffffff. That means
> dw_wdt->rate must be >= 1000 to avoid overflow, which you should check.

Right. I don't think that TOPs with timeouts bigger than
0xffffffff milliseconds have any valuable usecases, so I'll just round
the overflows down to FFs.

> 
> Note that I don't see the point of keeping the timeout values in ms.
> The code selects a larger value (in seconds) anyway. All this does is
> to add a lot of multiply and divide operations, plus a source of bugs
> and confusion, for little if any gain.

As I said in the comment to the code the idea of keeping values in ms was
to better approximate the requested timeouts. This is necessary since
unlike the fixed TOPs case in which each next timeout is doubled with
respect to a previous one (65536, 131072, etc), the unfixed TOPs set may
have any values within [2^8; (2^WDT_CNT_WIDTH - 1)]. Actual numerical
values of the set are defined by a SoC engineer at the moment of the
IP-core synthesis. So in general they can be unordered and can differ one
from another in very small time deltas, like ms and even us. It depends
on the ways the watchdog was supposed to be utilized in accordance with
the system requirements and the reference clock rate. In this case how to
distinguish the values if we had only seconds array? In this patch I
suggest an approach to at least cover the case of the TOPs with
milliseconds granularity.

I don't deny that this might be not that much gain seeing the watchdog
core supports the timeouts in seconds only, but at least it provides a
way to distinguish one TOP from another instead of picking a first found
one. As I see it there aren't that much multiplication and division
caused by such solution and it's a small prices with respect to an
ability to find a better timeout approximation. A few tens nsecs of
the code execution is much smaller than milliseconds accuracy of the
watchdog timeout. Though OS-wise such accuracy might be redundant.

Regarding bugs and confusion. Well I find confusing a numerical literals
usage while there are self-documented macros in the kernel available,
which would better represent the code context and would point out what
they are used for.) That's why I've sent the previous patch in the first
place. But as I said in the response to your review comments there, while
it doesn't contradict to the kernel requirements with ceteris paribus
it's up to you which approach to choose since you are the subsystem
maintainer and it will be your duty to continue the code maintenance in
future. The same thing is here. It's up to you what approach to choose.
So if my reasoning above didn't make you to change your mind, could you
please explicitly respond to this message that you'd better see the
timeouts array having seconds instead of milliseconds?

> 
> > +}
> > +
> >  static int dw_wdt_drv_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -275,12 +366,14 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >  
> >  	reset_control_deassert(dw_wdt->rst);
> >  
> > +	dw_wdt_init_timeouts(dw_wdt, dev);
> > +
> >  	wdd = &dw_wdt->wdd;
> >  	wdd->info = &dw_wdt_ident;
> >  	wdd->ops = &dw_wdt_ops;
> > -	wdd->min_timeout = 1;
> > +	wdd->min_timeout = dw_wdt_find_min_timeout(dw_wdt) / MSEC_PER_SEC;
> 
> dw_wdt_find_min_timeout can return a value < 1000. In that case min_timeout
> would be 0, ie unspecified.

Ok. I'll implement this limitation in the dw_wdt_find_min_timeout()
method. It will return seconds in v2.

Regards,
-Sergey

> 
> >  	wdd->max_hw_heartbeat_ms =
> > -		dw_wdt_top_in_seconds(dw_wdt, DW_WDT_MAX_TOP) * 1000;
> > +		dw_wdt_find_max_top(dw_wdt, &dw_wdt->max_top);
> >  	wdd->parent = dev;
> >  
> >  	watchdog_set_drvdata(wdd, dw_wdt);
> > @@ -293,7 +386,7 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >  	 * devicetree.
> >  	 */
> >  	if (dw_wdt_is_enabled(dw_wdt)) {
> > -		wdd->timeout = dw_wdt_get_top(dw_wdt);
> > +		wdd->timeout = dw_wdt_get_timeout(dw_wdt) / MSEC_PER_SEC;
> >  		set_bit(WDOG_HW_RUNNING, &wdd->status);
> >  	} else {
> >  		wdd->timeout = DW_WDT_DEFAULT_SECONDS;

Re: [PATCH 4/7] watchdog: dw_wdt: Support devices with non-fixed TOP values

[Guenter Roeck]

On 4/10/20 5:59 AM, Sergey Semin wrote:
> On Sun, Mar 15, 2020 at 07:12:38AM -0700, Guenter Roeck wrote:
>> On Fri, Mar 06, 2020 at 04:27:44PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
>>> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
>>>
>>> In case if the DW Watchdog IP core is synthesised with
>>> WDT_USE_FIX_TOP == false, the TOP interval indexes make the device
>>> to load a custom periods to the counter. These periods are hardwired
>>> at the synthesis stage and can be within [2^8, 2^(WDT_CNT_WIDTH - 1)].
>>> Alas their values can't be detected at runtime and must be somehow
>>> supplied to the driver so one could properly determine the watchdog
>>> timeout intervals. For this purpose we suggest to have a vendor-
>>> specific dts property "snps,watchdog-tops" utilized, which would
>>> provide an array of sixteen counter values. At device probe stage they
>>> will be used to initialize the watchdog device timeouts determined
>>> from the array values and current clocks source rate.
>>>
>>> In order to have custom TOP values supported the driver must be
>>> altered in the following way. First of all the fixed-top values
>>> ready-to-use array must be determined for compatibility with currently
>>> supported devices, which were synthesised with WDT_USE_FIX_TOP == true.
>>> Secondly we must redefine the timer period search functions. For
>>> generality they are redesigned in a way to support the TOP array with
>>> no limitations on the items order or value. Finally an array with
>>> pre-defined timeouts must be calculated at probe stage from either
>>> the custom or fixed TOP values depending on the DW watchdog component
>>> parameter WDT_USE_FIX_TOP value.
>>>
>>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
>>> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
>>> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
>>> Cc: Paul Burton <paulburton@kernel.org>
>>> Cc: Ralf Baechle <ralf@linux-mips.org>
>>> ---
>>>  drivers/watchdog/dw_wdt.c | 145 +++++++++++++++++++++++++++++++-------
>>>  1 file changed, 119 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
>>> index fba21de2bbad..4a57b7d777dc 100644
>>> --- a/drivers/watchdog/dw_wdt.c
>>> +++ b/drivers/watchdog/dw_wdt.c
>>> @@ -13,6 +13,7 @@
>>>   */
>>>  
>>>  #include <linux/bitops.h>
>>> +#include <linux/limits.h>
>>>  #include <linux/clk.h>
>>>  #include <linux/delay.h>
>>>  #include <linux/err.h>
>>> @@ -34,12 +35,24 @@
>>>  #define WDOG_CURRENT_COUNT_REG_OFFSET	    0x08
>>>  #define WDOG_COUNTER_RESTART_REG_OFFSET     0x0c
>>>  #define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
>>> +#define WDOG_COMP_PARAMS_1_REG_OFFSET       0xf4
>>> +#define WDOG_COMP_PARAMS_1_USE_FIX_TOP      BIT(6)
>>>  
>>> -/* The maximum TOP (timeout period) value that can be set in the watchdog. */
>>> -#define DW_WDT_MAX_TOP		15
>>> +/* There are sixteen TOPs (timeout periods) that can be set in the watchdog. */
>>> +#define DW_WDT_NUM_TOPS		16
>>> +#define DW_WDT_FIX_TOP(_idx)	(1U << (16 + _idx))
>>>  
>>>  #define DW_WDT_DEFAULT_SECONDS	30
>>>  
>>> +static const u32 dw_wdt_fix_tops[DW_WDT_NUM_TOPS] = {
>>> +	DW_WDT_FIX_TOP(0), DW_WDT_FIX_TOP(1), DW_WDT_FIX_TOP(2),
>>> +	DW_WDT_FIX_TOP(3), DW_WDT_FIX_TOP(4), DW_WDT_FIX_TOP(5),
>>> +	DW_WDT_FIX_TOP(6), DW_WDT_FIX_TOP(7), DW_WDT_FIX_TOP(8),
>>> +	DW_WDT_FIX_TOP(9), DW_WDT_FIX_TOP(10), DW_WDT_FIX_TOP(11),
>>> +	DW_WDT_FIX_TOP(12), DW_WDT_FIX_TOP(13), DW_WDT_FIX_TOP(14),
>>> +	DW_WDT_FIX_TOP(15)
>>> +};
>>> +
>>>  static bool nowayout = WATCHDOG_NOWAYOUT;
>>>  module_param(nowayout, bool, 0);
>>>  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
>>> @@ -49,6 +62,8 @@ struct dw_wdt {
>>>  	void __iomem		*regs;
>>>  	struct clk		*clk;
>>>  	unsigned long		rate;
>>> +	unsigned int		max_top;
>>> +	unsigned int		timeouts[DW_WDT_NUM_TOPS];
>>>  	struct watchdog_device	wdd;
>>>  	struct reset_control	*rst;
>>>  	/* Save/restore */
>>> @@ -64,20 +79,68 @@ static inline int dw_wdt_is_enabled(struct dw_wdt *dw_wdt)
>>>  		WDOG_CONTROL_REG_WDT_EN_MASK;
>>>  }
>>>  
>>> -static inline int dw_wdt_top_in_seconds(struct dw_wdt *dw_wdt, unsigned top)
>>> +static unsigned int dw_wdt_find_best_top(struct dw_wdt *dw_wdt,
>>> +					 unsigned int timeout, u32 *top)
>>>  {
>>> +	u32 diff = UINT_MAX, tmp;
>>> +	int idx;
>>> +
>>>  	/*
>>> -	 * There are 16 possible timeout values in 0..15 where the number of
>>> -	 * cycles is 2 ^ (16 + i) and the watchdog counts down.
>>> +	 * In general case of non-fixed timeout values they can be arranged in
>>> +	 * any order so we have to traverse all the array values. We also try
>>> +	 * to find a closest timeout number and make sure its value is greater
>>> +	 * than the requested timeout. Note we'll return a maximum timeout
>>> +	 * if reachable value couldn't be found.
>>>  	 */
>>> -	return (1U << (16 + top)) / dw_wdt->rate;
>>> +	for (*top = dw_wdt->max_top, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
>>> +		if (dw_wdt->timeouts[idx] < timeout)
>>> +			continue;
>>> +
>>> +		tmp = dw_wdt->timeouts[idx] - timeout;
>>> +		if (tmp < diff) {
>>> +			diff = tmp;
>>> +			*top = idx;
>>> +		}
>>> +	}
>>> +
>>> +	return dw_wdt->timeouts[*top];
>>> +}
>>> +
>>> +static unsigned int dw_wdt_find_min_timeout(struct dw_wdt *dw_wdt)
>>
>> I would appreciate if the names of functions returning ms end with _ms
>> to avoid confusion.
> 
> Ok. I'll also modify the functions a bit, so only the
> dw_wdt_find_best_top_ms() and dw_wdt_find_max_top_ms() methods would
> return timeouts in milliseconds. Though if you insist in keeping seconds
> in the timeouts array (see the comment after the next one), it'll be
> dw_wdt_find_max_top_ms() only.
> 
>>
>>> +{
>>> +	u32 min_timeout = UINT_MAX, top;
>>> +	int idx;
>>> +
>>> +	for (top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
>>> +		if (dw_wdt->timeouts[idx] <= min_timeout) {
>>> +			min_timeout = dw_wdt->timeouts[idx];
>>> +			top = idx;
>>> +		}
>>> +	}
>>> +
>>> +	return dw_wdt->timeouts[top];
>>> +}
>>> +
>>> +static unsigned int dw_wdt_find_max_top(struct dw_wdt *dw_wdt, u32 *top)
>>> +{
>>> +	u32 max_timeout = 0;
>>> +	int idx;
>>> +
>>> +	for (*top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
>>> +		if (dw_wdt->timeouts[idx] >= max_timeout) {
>>> +			max_timeout = dw_wdt->timeouts[idx];
>>> +			*top = idx;
>>> +		}
>>> +	}
>>> +
>>> +	return dw_wdt->timeouts[*top];
>>>  }
>>>  
>>> -static int dw_wdt_get_top(struct dw_wdt *dw_wdt)
>>> +static unsigned int dw_wdt_get_timeout(struct dw_wdt *dw_wdt)
>>>  {
>>>  	int top = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
>>>  
>>> -	return dw_wdt_top_in_seconds(dw_wdt, top);
>>> +	return dw_wdt->timeouts[top];
>>>  }
>>>  
>>>  static int dw_wdt_ping(struct watchdog_device *wdd)
>>> @@ -90,20 +153,13 @@ static int dw_wdt_ping(struct watchdog_device *wdd)
>>>  	return 0;
>>>  }
>>>  
>>> -static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
>>> +static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
>>>  {
>>>  	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
>>> -	int i, top_val = DW_WDT_MAX_TOP;
>>> +	unsigned int timeout;
>>> +	u32 top;
>>>  
>>> -	/*
>>> -	 * Iterate over the timeout values until we find the closest match. We
>>> -	 * always look for >=.
>>> -	 */
>>> -	for (i = 0; i <= DW_WDT_MAX_TOP; ++i)
>>> -		if (dw_wdt_top_in_seconds(dw_wdt, i) >= top_s) {
>>> -			top_val = i;
>>> -			break;
>>> -		}
>>> +	timeout = dw_wdt_find_best_top(dw_wdt, req * MSEC_PER_SEC, &top);
>>>  
>>>  	/*
>>>  	 * Set the new value in the watchdog.  Some versions of dw_wdt
>>> @@ -111,7 +167,7 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
>>>  	 * CP_WDT_DUAL_TOP in WDT_COMP_PARAMS_1).  On those we
>>>  	 * effectively get a pat of the watchdog right here.
>>>  	 */
>>> -	writel(top_val | top_val << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
>>> +	writel(top | top << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
>>>  	       dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
>>>  
>>>  	/*
>>> @@ -119,10 +175,10 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
>>>  	 * kernel(watchdog_dev.c) helps to feed watchdog before
>>>  	 * wdd->max_hw_heartbeat_ms
>>>  	 */
>>> -	if (top_s * 1000 <= wdd->max_hw_heartbeat_ms)
>>> -		wdd->timeout = dw_wdt_top_in_seconds(dw_wdt, top_val);
>>> +	if (req * MSEC_PER_SEC > wdd->max_hw_heartbeat_ms)
>>> +		wdd->timeout = req;
>>>  	else
>>> -		wdd->timeout = top_s;
>>> +		wdd->timeout = timeout / MSEC_PER_SEC;
>>>  
>>>  	return 0;
>>>  }
>>> @@ -238,6 +294,41 @@ static int dw_wdt_resume(struct device *dev)
>>>  
>>>  static SIMPLE_DEV_PM_OPS(dw_wdt_pm_ops, dw_wdt_suspend, dw_wdt_resume);
>>>  
>>> +static void dw_wdt_init_timeouts(struct dw_wdt *dw_wdt, struct device *dev)
>>> +{
>>> +	u32 data, of_tops[DW_WDT_NUM_TOPS];
>>> +	const u32 *tops;
>>> +	int ret, idx;
>>> +
>>> +	/*
>>> +	 * Retrieve custom or fixed counter values depending on the
>>> +	 * WDT_USE_FIX_TOP flag found in the component specific parameters
>>> +	 * #1 register.
>>> +	 */
>>> +	data = readl(dw_wdt->regs + WDOG_COMP_PARAMS_1_REG_OFFSET);
>>> +	if (data & WDOG_COMP_PARAMS_1_USE_FIX_TOP) {
>>> +		tops = dw_wdt_fix_tops;
>>> +	} else {
>>> +		ret = of_property_read_variable_u32_array(dev_of_node(dev),
>>> +			"snps,watchdog-tops", of_tops, DW_WDT_NUM_TOPS,
>>> +			DW_WDT_NUM_TOPS);
>>> +		if (ret < 0) {
>>> +			dev_warn(dev, "No valid TOPs array specified\n");
>>> +			tops = dw_wdt_fix_tops;
>>> +		} else {
>>> +			tops = of_tops;
>>> +		}
>>> +	}
>>> +
>>> +	/*
>>> +	 * We'll keep the timeout values in ms to approximate requested
>>> +	 * timeouts with better accuracy.
>>> +	 */
>>> +	for (idx = 0; idx < DW_WDT_NUM_TOPS; ++idx)
>>> +		dw_wdt->timeouts[idx] =
>>> +			mult_frac(tops[idx], MSEC_PER_SEC, dw_wdt->rate);
>>
>> tops[idx] type is u32. Its value can be up to 0xffffffff. That means
>> dw_wdt->rate must be >= 1000 to avoid overflow, which you should check.
> 
> Right. I don't think that TOPs with timeouts bigger than
> 0xffffffff milliseconds have any valuable usecases, so I'll just round
> the overflows down to FFs.
> 

Neither do unsorted random timeouts milli-seconds apart. You see the need
to address one, so addressing other weaknesses is appropriate.

>>
>> Note that I don't see the point of keeping the timeout values in ms.
>> The code selects a larger value (in seconds) anyway. All this does is
>> to add a lot of multiply and divide operations, plus a source of bugs
>> and confusion, for little if any gain.
> 
> As I said in the comment to the code the idea of keeping values in ms was
> to better approximate the requested timeouts. This is necessary since
> unlike the fixed TOPs case in which each next timeout is doubled with
> respect to a previous one (65536, 131072, etc), the unfixed TOPs set may
> have any values within [2^8; (2^WDT_CNT_WIDTH - 1)]. Actual numerical
> values of the set are defined by a SoC engineer at the moment of the
> IP-core synthesis. So in general they can be unordered and can differ one
> from another in very small time deltas, like ms and even us. It depends
> on the ways the watchdog was supposed to be utilized in accordance with
> the system requirements and the reference clock rate. In this case how to
> distinguish the values if we had only seconds array? In this patch I
> suggest an approach to at least cover the case of the TOPs with
> milliseconds granularity.
> 
> I don't deny that this might be not that much gain seeing the watchdog
> core supports the timeouts in seconds only, but at least it provides a
> way to distinguish one TOP from another instead of picking a first found
> one. As I see it there aren't that much multiplication and division
> caused by such solution and it's a small prices with respect to an
> ability to find a better timeout approximation. A few tens nsecs of
> the code execution is much smaller than milliseconds accuracy of the
> watchdog timeout. Though OS-wise such accuracy might be redundant.
> 
> Regarding bugs and confusion. Well I find confusing a numerical literals
> usage while there are self-documented macros in the kernel available,
> which would better represent the code context and would point out what
> they are used for.) That's why I've sent the previous patch in the first
> place. But as I said in the response to your review comments there, while
> it doesn't contradict to the kernel requirements with ceteris paribus
> it's up to you which approach to choose since you are the subsystem
> maintainer and it will be your duty to continue the code maintenance in
> future. The same thing is here. It's up to you what approach to choose.
> So if my reasoning above didn't make you to change your mind, could you
> please explicitly respond to this message that you'd better see the
> timeouts array having seconds instead of milliseconds?
> 
Please include a summary of the above in the driver to explain the need
for the complexity for others in the future.

Guenter

>>
>>> +}
>>> +
>>>  static int dw_wdt_drv_probe(struct platform_device *pdev)
>>>  {
>>>  	struct device *dev = &pdev->dev;
>>> @@ -275,12 +366,14 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>>>  
>>>  	reset_control_deassert(dw_wdt->rst);
>>>  
>>> +	dw_wdt_init_timeouts(dw_wdt, dev);
>>> +
>>>  	wdd = &dw_wdt->wdd;
>>>  	wdd->info = &dw_wdt_ident;
>>>  	wdd->ops = &dw_wdt_ops;
>>> -	wdd->min_timeout = 1;
>>> +	wdd->min_timeout = dw_wdt_find_min_timeout(dw_wdt) / MSEC_PER_SEC;
>>
>> dw_wdt_find_min_timeout can return a value < 1000. In that case min_timeout
>> would be 0, ie unspecified.
> 
> Ok. I'll implement this limitation in the dw_wdt_find_min_timeout()
> method. It will return seconds in v2.
> 
> Regards,
> -Sergey
> 
>>
>>>  	wdd->max_hw_heartbeat_ms =
>>> -		dw_wdt_top_in_seconds(dw_wdt, DW_WDT_MAX_TOP) * 1000;
>>> +		dw_wdt_find_max_top(dw_wdt, &dw_wdt->max_top);
>>>  	wdd->parent = dev;
>>>  
>>>  	watchdog_set_drvdata(wdd, dw_wdt);
>>> @@ -293,7 +386,7 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>>>  	 * devicetree.
>>>  	 */
>>>  	if (dw_wdt_is_enabled(dw_wdt)) {
>>> -		wdd->timeout = dw_wdt_get_top(dw_wdt);
>>> +		wdd->timeout = dw_wdt_get_timeout(dw_wdt) / MSEC_PER_SEC;
>>>  		set_bit(WDOG_HW_RUNNING, &wdd->status);
>>>  	} else {
>>>  		wdd->timeout = DW_WDT_DEFAULT_SECONDS;

Re: [PATCH 4/7] watchdog: dw_wdt: Support devices with non-fixed TOP values

[Sergey Semin]

On Fri, Apr 10, 2020 at 09:21:35AM -0700, Guenter Roeck wrote:
> On 4/10/20 5:59 AM, Sergey Semin wrote:
> > On Sun, Mar 15, 2020 at 07:12:38AM -0700, Guenter Roeck wrote:
> >> On Fri, Mar 06, 2020 at 04:27:44PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> >>> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> >>>
> >>> In case if the DW Watchdog IP core is synthesised with
> >>> WDT_USE_FIX_TOP == false, the TOP interval indexes make the device
> >>> to load a custom periods to the counter. These periods are hardwired
> >>> at the synthesis stage and can be within [2^8, 2^(WDT_CNT_WIDTH - 1)].
> >>> Alas their values can't be detected at runtime and must be somehow
> >>> supplied to the driver so one could properly determine the watchdog
> >>> timeout intervals. For this purpose we suggest to have a vendor-
> >>> specific dts property "snps,watchdog-tops" utilized, which would
> >>> provide an array of sixteen counter values. At device probe stage they
> >>> will be used to initialize the watchdog device timeouts determined
> >>> from the array values and current clocks source rate.
> >>>
> >>> In order to have custom TOP values supported the driver must be
> >>> altered in the following way. First of all the fixed-top values
> >>> ready-to-use array must be determined for compatibility with currently
> >>> supported devices, which were synthesised with WDT_USE_FIX_TOP == true.
> >>> Secondly we must redefine the timer period search functions. For
> >>> generality they are redesigned in a way to support the TOP array with
> >>> no limitations on the items order or value. Finally an array with
> >>> pre-defined timeouts must be calculated at probe stage from either
> >>> the custom or fixed TOP values depending on the DW watchdog component
> >>> parameter WDT_USE_FIX_TOP value.
> >>>
> >>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> >>> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> >>> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> >>> Cc: Paul Burton <paulburton@kernel.org>
> >>> Cc: Ralf Baechle <ralf@linux-mips.org>
> >>> ---
> >>>  drivers/watchdog/dw_wdt.c | 145 +++++++++++++++++++++++++++++++-------
> >>>  1 file changed, 119 insertions(+), 26 deletions(-)
> >>>
> >>> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> >>> index fba21de2bbad..4a57b7d777dc 100644
> >>> --- a/drivers/watchdog/dw_wdt.c
> >>> +++ b/drivers/watchdog/dw_wdt.c
> >>> @@ -13,6 +13,7 @@
> >>>   */
> >>>  
> >>>  #include <linux/bitops.h>
> >>> +#include <linux/limits.h>
> >>>  #include <linux/clk.h>
> >>>  #include <linux/delay.h>
> >>>  #include <linux/err.h>
> >>> @@ -34,12 +35,24 @@
> >>>  #define WDOG_CURRENT_COUNT_REG_OFFSET	    0x08
> >>>  #define WDOG_COUNTER_RESTART_REG_OFFSET     0x0c
> >>>  #define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
> >>> +#define WDOG_COMP_PARAMS_1_REG_OFFSET       0xf4
> >>> +#define WDOG_COMP_PARAMS_1_USE_FIX_TOP      BIT(6)
> >>>  
> >>> -/* The maximum TOP (timeout period) value that can be set in the watchdog. */
> >>> -#define DW_WDT_MAX_TOP		15
> >>> +/* There are sixteen TOPs (timeout periods) that can be set in the watchdog. */
> >>> +#define DW_WDT_NUM_TOPS		16
> >>> +#define DW_WDT_FIX_TOP(_idx)	(1U << (16 + _idx))
> >>>  
> >>>  #define DW_WDT_DEFAULT_SECONDS	30
> >>>  
> >>> +static const u32 dw_wdt_fix_tops[DW_WDT_NUM_TOPS] = {
> >>> +	DW_WDT_FIX_TOP(0), DW_WDT_FIX_TOP(1), DW_WDT_FIX_TOP(2),
> >>> +	DW_WDT_FIX_TOP(3), DW_WDT_FIX_TOP(4), DW_WDT_FIX_TOP(5),
> >>> +	DW_WDT_FIX_TOP(6), DW_WDT_FIX_TOP(7), DW_WDT_FIX_TOP(8),
> >>> +	DW_WDT_FIX_TOP(9), DW_WDT_FIX_TOP(10), DW_WDT_FIX_TOP(11),
> >>> +	DW_WDT_FIX_TOP(12), DW_WDT_FIX_TOP(13), DW_WDT_FIX_TOP(14),
> >>> +	DW_WDT_FIX_TOP(15)
> >>> +};
> >>> +
> >>>  static bool nowayout = WATCHDOG_NOWAYOUT;
> >>>  module_param(nowayout, bool, 0);
> >>>  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> >>> @@ -49,6 +62,8 @@ struct dw_wdt {
> >>>  	void __iomem		*regs;
> >>>  	struct clk		*clk;
> >>>  	unsigned long		rate;
> >>> +	unsigned int		max_top;
> >>> +	unsigned int		timeouts[DW_WDT_NUM_TOPS];
> >>>  	struct watchdog_device	wdd;
> >>>  	struct reset_control	*rst;
> >>>  	/* Save/restore */
> >>> @@ -64,20 +79,68 @@ static inline int dw_wdt_is_enabled(struct dw_wdt *dw_wdt)
> >>>  		WDOG_CONTROL_REG_WDT_EN_MASK;
> >>>  }
> >>>  
> >>> -static inline int dw_wdt_top_in_seconds(struct dw_wdt *dw_wdt, unsigned top)
> >>> +static unsigned int dw_wdt_find_best_top(struct dw_wdt *dw_wdt,
> >>> +					 unsigned int timeout, u32 *top)
> >>>  {
> >>> +	u32 diff = UINT_MAX, tmp;
> >>> +	int idx;
> >>> +
> >>>  	/*
> >>> -	 * There are 16 possible timeout values in 0..15 where the number of
> >>> -	 * cycles is 2 ^ (16 + i) and the watchdog counts down.
> >>> +	 * In general case of non-fixed timeout values they can be arranged in
> >>> +	 * any order so we have to traverse all the array values. We also try
> >>> +	 * to find a closest timeout number and make sure its value is greater
> >>> +	 * than the requested timeout. Note we'll return a maximum timeout
> >>> +	 * if reachable value couldn't be found.
> >>>  	 */
> >>> -	return (1U << (16 + top)) / dw_wdt->rate;
> >>> +	for (*top = dw_wdt->max_top, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
> >>> +		if (dw_wdt->timeouts[idx] < timeout)
> >>> +			continue;
> >>> +
> >>> +		tmp = dw_wdt->timeouts[idx] - timeout;
> >>> +		if (tmp < diff) {
> >>> +			diff = tmp;
> >>> +			*top = idx;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	return dw_wdt->timeouts[*top];
> >>> +}
> >>> +
> >>> +static unsigned int dw_wdt_find_min_timeout(struct dw_wdt *dw_wdt)
> >>
> >> I would appreciate if the names of functions returning ms end with _ms
> >> to avoid confusion.
> > 
> > Ok. I'll also modify the functions a bit, so only the
> > dw_wdt_find_best_top_ms() and dw_wdt_find_max_top_ms() methods would
> > return timeouts in milliseconds. Though if you insist in keeping seconds
> > in the timeouts array (see the comment after the next one), it'll be
> > dw_wdt_find_max_top_ms() only.
> > 
> >>
> >>> +{
> >>> +	u32 min_timeout = UINT_MAX, top;
> >>> +	int idx;
> >>> +
> >>> +	for (top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
> >>> +		if (dw_wdt->timeouts[idx] <= min_timeout) {
> >>> +			min_timeout = dw_wdt->timeouts[idx];
> >>> +			top = idx;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	return dw_wdt->timeouts[top];
> >>> +}
> >>> +
> >>> +static unsigned int dw_wdt_find_max_top(struct dw_wdt *dw_wdt, u32 *top)
> >>> +{
> >>> +	u32 max_timeout = 0;
> >>> +	int idx;
> >>> +
> >>> +	for (*top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
> >>> +		if (dw_wdt->timeouts[idx] >= max_timeout) {
> >>> +			max_timeout = dw_wdt->timeouts[idx];
> >>> +			*top = idx;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	return dw_wdt->timeouts[*top];
> >>>  }
> >>>  
> >>> -static int dw_wdt_get_top(struct dw_wdt *dw_wdt)
> >>> +static unsigned int dw_wdt_get_timeout(struct dw_wdt *dw_wdt)
> >>>  {
> >>>  	int top = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
> >>>  
> >>> -	return dw_wdt_top_in_seconds(dw_wdt, top);
> >>> +	return dw_wdt->timeouts[top];
> >>>  }
> >>>  
> >>>  static int dw_wdt_ping(struct watchdog_device *wdd)
> >>> @@ -90,20 +153,13 @@ static int dw_wdt_ping(struct watchdog_device *wdd)
> >>>  	return 0;
> >>>  }
> >>>  
> >>> -static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> >>> +static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
> >>>  {
> >>>  	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> >>> -	int i, top_val = DW_WDT_MAX_TOP;
> >>> +	unsigned int timeout;
> >>> +	u32 top;
> >>>  
> >>> -	/*
> >>> -	 * Iterate over the timeout values until we find the closest match. We
> >>> -	 * always look for >=.
> >>> -	 */
> >>> -	for (i = 0; i <= DW_WDT_MAX_TOP; ++i)
> >>> -		if (dw_wdt_top_in_seconds(dw_wdt, i) >= top_s) {
> >>> -			top_val = i;
> >>> -			break;
> >>> -		}
> >>> +	timeout = dw_wdt_find_best_top(dw_wdt, req * MSEC_PER_SEC, &top);
> >>>  
> >>>  	/*
> >>>  	 * Set the new value in the watchdog.  Some versions of dw_wdt
> >>> @@ -111,7 +167,7 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> >>>  	 * CP_WDT_DUAL_TOP in WDT_COMP_PARAMS_1).  On those we
> >>>  	 * effectively get a pat of the watchdog right here.
> >>>  	 */
> >>> -	writel(top_val | top_val << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
> >>> +	writel(top | top << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
> >>>  	       dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> >>>  
> >>>  	/*
> >>> @@ -119,10 +175,10 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> >>>  	 * kernel(watchdog_dev.c) helps to feed watchdog before
> >>>  	 * wdd->max_hw_heartbeat_ms
> >>>  	 */
> >>> -	if (top_s * 1000 <= wdd->max_hw_heartbeat_ms)
> >>> -		wdd->timeout = dw_wdt_top_in_seconds(dw_wdt, top_val);
> >>> +	if (req * MSEC_PER_SEC > wdd->max_hw_heartbeat_ms)
> >>> +		wdd->timeout = req;
> >>>  	else
> >>> -		wdd->timeout = top_s;
> >>> +		wdd->timeout = timeout / MSEC_PER_SEC;
> >>>  
> >>>  	return 0;
> >>>  }
> >>> @@ -238,6 +294,41 @@ static int dw_wdt_resume(struct device *dev)
> >>>  
> >>>  static SIMPLE_DEV_PM_OPS(dw_wdt_pm_ops, dw_wdt_suspend, dw_wdt_resume);
> >>>  
> >>> +static void dw_wdt_init_timeouts(struct dw_wdt *dw_wdt, struct device *dev)
> >>> +{
> >>> +	u32 data, of_tops[DW_WDT_NUM_TOPS];
> >>> +	const u32 *tops;
> >>> +	int ret, idx;
> >>> +
> >>> +	/*
> >>> +	 * Retrieve custom or fixed counter values depending on the
> >>> +	 * WDT_USE_FIX_TOP flag found in the component specific parameters
> >>> +	 * #1 register.
> >>> +	 */
> >>> +	data = readl(dw_wdt->regs + WDOG_COMP_PARAMS_1_REG_OFFSET);
> >>> +	if (data & WDOG_COMP_PARAMS_1_USE_FIX_TOP) {
> >>> +		tops = dw_wdt_fix_tops;
> >>> +	} else {
> >>> +		ret = of_property_read_variable_u32_array(dev_of_node(dev),
> >>> +			"snps,watchdog-tops", of_tops, DW_WDT_NUM_TOPS,
> >>> +			DW_WDT_NUM_TOPS);
> >>> +		if (ret < 0) {
> >>> +			dev_warn(dev, "No valid TOPs array specified\n");
> >>> +			tops = dw_wdt_fix_tops;
> >>> +		} else {
> >>> +			tops = of_tops;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	/*
> >>> +	 * We'll keep the timeout values in ms to approximate requested
> >>> +	 * timeouts with better accuracy.
> >>> +	 */
> >>> +	for (idx = 0; idx < DW_WDT_NUM_TOPS; ++idx)
> >>> +		dw_wdt->timeouts[idx] =
> >>> +			mult_frac(tops[idx], MSEC_PER_SEC, dw_wdt->rate);
> >>
> >> tops[idx] type is u32. Its value can be up to 0xffffffff. That means
> >> dw_wdt->rate must be >= 1000 to avoid overflow, which you should check.
> > 
> > Right. I don't think that TOPs with timeouts bigger than
> > 0xffffffff milliseconds have any valuable usecases, so I'll just round
> > the overflows down to FFs.
> > 
> 
> Neither do unsorted random timeouts milli-seconds apart. You see the need
> to address one, so addressing other weaknesses is appropriate.

Don't really understand what you mean. Do you intend to filter the
unreachable timeouts out from the timeouts array? If so this isn't
possible with current design. I would have to implement a more complex
data structure, like an array of pairs {TOP, timeout} and refactor the
timeout search algorithm. Don't really think this optimization is required
seeing watchdog timeout set operation is normally performed just a few times
per watchdog usage session.

> 
> >>
> >> Note that I don't see the point of keeping the timeout values in ms.
> >> The code selects a larger value (in seconds) anyway. All this does is
> >> to add a lot of multiply and divide operations, plus a source of bugs
> >> and confusion, for little if any gain.
> > 
> > As I said in the comment to the code the idea of keeping values in ms was
> > to better approximate the requested timeouts. This is necessary since
> > unlike the fixed TOPs case in which each next timeout is doubled with
> > respect to a previous one (65536, 131072, etc), the unfixed TOPs set may
> > have any values within [2^8; (2^WDT_CNT_WIDTH - 1)]. Actual numerical
> > values of the set are defined by a SoC engineer at the moment of the
> > IP-core synthesis. So in general they can be unordered and can differ one
> > from another in very small time deltas, like ms and even us. It depends
> > on the ways the watchdog was supposed to be utilized in accordance with
> > the system requirements and the reference clock rate. In this case how to
> > distinguish the values if we had only seconds array? In this patch I
> > suggest an approach to at least cover the case of the TOPs with
> > milliseconds granularity.
> > 
> > I don't deny that this might be not that much gain seeing the watchdog
> > core supports the timeouts in seconds only, but at least it provides a
> > way to distinguish one TOP from another instead of picking a first found
> > one. As I see it there aren't that much multiplication and division
> > caused by such solution and it's a small prices with respect to an
> > ability to find a better timeout approximation. A few tens nsecs of
> > the code execution is much smaller than milliseconds accuracy of the
> > watchdog timeout. Though OS-wise such accuracy might be redundant.
> > 
> > Regarding bugs and confusion. Well I find confusing a numerical literals
> > usage while there are self-documented macros in the kernel available,
> > which would better represent the code context and would point out what
> > they are used for.) That's why I've sent the previous patch in the first
> > place. But as I said in the response to your review comments there, while
> > it doesn't contradict to the kernel requirements with ceteris paribus
> > it's up to you which approach to choose since you are the subsystem
> > maintainer and it will be your duty to continue the code maintenance in
> > future. The same thing is here. It's up to you what approach to choose.
> > So if my reasoning above didn't make you to change your mind, could you
> > please explicitly respond to this message that you'd better see the
> > timeouts array having seconds instead of milliseconds?
> > 
> Please include a summary of the above in the driver to explain the need
> for the complexity for others in the future.

Ok.

-Sergey

> 
> Guenter
> 
> >>
> >>> +}
> >>> +
> >>>  static int dw_wdt_drv_probe(struct platform_device *pdev)
> >>>  {
> >>>  	struct device *dev = &pdev->dev;
> >>> @@ -275,12 +366,14 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >>>  
> >>>  	reset_control_deassert(dw_wdt->rst);
> >>>  
> >>> +	dw_wdt_init_timeouts(dw_wdt, dev);
> >>> +
> >>>  	wdd = &dw_wdt->wdd;
> >>>  	wdd->info = &dw_wdt_ident;
> >>>  	wdd->ops = &dw_wdt_ops;
> >>> -	wdd->min_timeout = 1;
> >>> +	wdd->min_timeout = dw_wdt_find_min_timeout(dw_wdt) / MSEC_PER_SEC;
> >>
> >> dw_wdt_find_min_timeout can return a value < 1000. In that case min_timeout
> >> would be 0, ie unspecified.
> > 
> > Ok. I'll implement this limitation in the dw_wdt_find_min_timeout()
> > method. It will return seconds in v2.
> > 
> > Regards,
> > -Sergey
> > 
> >>
> >>>  	wdd->max_hw_heartbeat_ms =
> >>> -		dw_wdt_top_in_seconds(dw_wdt, DW_WDT_MAX_TOP) * 1000;
> >>> +		dw_wdt_find_max_top(dw_wdt, &dw_wdt->max_top);
> >>>  	wdd->parent = dev;
> >>>  
> >>>  	watchdog_set_drvdata(wdd, dw_wdt);
> >>> @@ -293,7 +386,7 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >>>  	 * devicetree.
> >>>  	 */
> >>>  	if (dw_wdt_is_enabled(dw_wdt)) {
> >>> -		wdd->timeout = dw_wdt_get_top(dw_wdt);
> >>> +		wdd->timeout = dw_wdt_get_timeout(dw_wdt) / MSEC_PER_SEC;
> >>>  		set_bit(WDOG_HW_RUNNING, &wdd->status);
> >>>  	} else {
> >>>  		wdd->timeout = DW_WDT_DEFAULT_SECONDS;
> 

Re: [PATCH 4/7] watchdog: dw_wdt: Support devices with non-fixed TOP values

[Guenter Roeck]

On 4/10/20 12:45 PM, Sergey Semin wrote:
> On Fri, Apr 10, 2020 at 09:21:35AM -0700, Guenter Roeck wrote:
>> On 4/10/20 5:59 AM, Sergey Semin wrote:
>>> On Sun, Mar 15, 2020 at 07:12:38AM -0700, Guenter Roeck wrote:
>>>> On Fri, Mar 06, 2020 at 04:27:44PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
>>>>> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
>>>>>
>>>>> In case if the DW Watchdog IP core is synthesised with
>>>>> WDT_USE_FIX_TOP == false, the TOP interval indexes make the device
>>>>> to load a custom periods to the counter. These periods are hardwired
>>>>> at the synthesis stage and can be within [2^8, 2^(WDT_CNT_WIDTH - 1)].
>>>>> Alas their values can't be detected at runtime and must be somehow
>>>>> supplied to the driver so one could properly determine the watchdog
>>>>> timeout intervals. For this purpose we suggest to have a vendor-
>>>>> specific dts property "snps,watchdog-tops" utilized, which would
>>>>> provide an array of sixteen counter values. At device probe stage they
>>>>> will be used to initialize the watchdog device timeouts determined
>>>>> from the array values and current clocks source rate.
>>>>>
>>>>> In order to have custom TOP values supported the driver must be
>>>>> altered in the following way. First of all the fixed-top values
>>>>> ready-to-use array must be determined for compatibility with currently
>>>>> supported devices, which were synthesised with WDT_USE_FIX_TOP == true.
>>>>> Secondly we must redefine the timer period search functions. For
>>>>> generality they are redesigned in a way to support the TOP array with
>>>>> no limitations on the items order or value. Finally an array with
>>>>> pre-defined timeouts must be calculated at probe stage from either
>>>>> the custom or fixed TOP values depending on the DW watchdog component
>>>>> parameter WDT_USE_FIX_TOP value.
>>>>>
>>>>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
>>>>> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
>>>>> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
>>>>> Cc: Paul Burton <paulburton@kernel.org>
>>>>> Cc: Ralf Baechle <ralf@linux-mips.org>
>>>>> ---
>>>>>  drivers/watchdog/dw_wdt.c | 145 +++++++++++++++++++++++++++++++-------
>>>>>  1 file changed, 119 insertions(+), 26 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
>>>>> index fba21de2bbad..4a57b7d777dc 100644
>>>>> --- a/drivers/watchdog/dw_wdt.c
>>>>> +++ b/drivers/watchdog/dw_wdt.c
>>>>> @@ -13,6 +13,7 @@
>>>>>   */
>>>>>  
>>>>>  #include <linux/bitops.h>
>>>>> +#include <linux/limits.h>
>>>>>  #include <linux/clk.h>
>>>>>  #include <linux/delay.h>
>>>>>  #include <linux/err.h>
>>>>> @@ -34,12 +35,24 @@
>>>>>  #define WDOG_CURRENT_COUNT_REG_OFFSET	    0x08
>>>>>  #define WDOG_COUNTER_RESTART_REG_OFFSET     0x0c
>>>>>  #define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
>>>>> +#define WDOG_COMP_PARAMS_1_REG_OFFSET       0xf4
>>>>> +#define WDOG_COMP_PARAMS_1_USE_FIX_TOP      BIT(6)
>>>>>  
>>>>> -/* The maximum TOP (timeout period) value that can be set in the watchdog. */
>>>>> -#define DW_WDT_MAX_TOP		15
>>>>> +/* There are sixteen TOPs (timeout periods) that can be set in the watchdog. */
>>>>> +#define DW_WDT_NUM_TOPS		16
>>>>> +#define DW_WDT_FIX_TOP(_idx)	(1U << (16 + _idx))
>>>>>  
>>>>>  #define DW_WDT_DEFAULT_SECONDS	30
>>>>>  
>>>>> +static const u32 dw_wdt_fix_tops[DW_WDT_NUM_TOPS] = {
>>>>> +	DW_WDT_FIX_TOP(0), DW_WDT_FIX_TOP(1), DW_WDT_FIX_TOP(2),
>>>>> +	DW_WDT_FIX_TOP(3), DW_WDT_FIX_TOP(4), DW_WDT_FIX_TOP(5),
>>>>> +	DW_WDT_FIX_TOP(6), DW_WDT_FIX_TOP(7), DW_WDT_FIX_TOP(8),
>>>>> +	DW_WDT_FIX_TOP(9), DW_WDT_FIX_TOP(10), DW_WDT_FIX_TOP(11),
>>>>> +	DW_WDT_FIX_TOP(12), DW_WDT_FIX_TOP(13), DW_WDT_FIX_TOP(14),
>>>>> +	DW_WDT_FIX_TOP(15)
>>>>> +};
>>>>> +
>>>>>  static bool nowayout = WATCHDOG_NOWAYOUT;
>>>>>  module_param(nowayout, bool, 0);
>>>>>  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
>>>>> @@ -49,6 +62,8 @@ struct dw_wdt {
>>>>>  	void __iomem		*regs;
>>>>>  	struct clk		*clk;
>>>>>  	unsigned long		rate;
>>>>> +	unsigned int		max_top;
>>>>> +	unsigned int		timeouts[DW_WDT_NUM_TOPS];
>>>>>  	struct watchdog_device	wdd;
>>>>>  	struct reset_control	*rst;
>>>>>  	/* Save/restore */
>>>>> @@ -64,20 +79,68 @@ static inline int dw_wdt_is_enabled(struct dw_wdt *dw_wdt)
>>>>>  		WDOG_CONTROL_REG_WDT_EN_MASK;
>>>>>  }
>>>>>  
>>>>> -static inline int dw_wdt_top_in_seconds(struct dw_wdt *dw_wdt, unsigned top)
>>>>> +static unsigned int dw_wdt_find_best_top(struct dw_wdt *dw_wdt,
>>>>> +					 unsigned int timeout, u32 *top)
>>>>>  {
>>>>> +	u32 diff = UINT_MAX, tmp;
>>>>> +	int idx;
>>>>> +
>>>>>  	/*
>>>>> -	 * There are 16 possible timeout values in 0..15 where the number of
>>>>> -	 * cycles is 2 ^ (16 + i) and the watchdog counts down.
>>>>> +	 * In general case of non-fixed timeout values they can be arranged in
>>>>> +	 * any order so we have to traverse all the array values. We also try
>>>>> +	 * to find a closest timeout number and make sure its value is greater
>>>>> +	 * than the requested timeout. Note we'll return a maximum timeout
>>>>> +	 * if reachable value couldn't be found.
>>>>>  	 */
>>>>> -	return (1U << (16 + top)) / dw_wdt->rate;
>>>>> +	for (*top = dw_wdt->max_top, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
>>>>> +		if (dw_wdt->timeouts[idx] < timeout)
>>>>> +			continue;
>>>>> +
>>>>> +		tmp = dw_wdt->timeouts[idx] - timeout;
>>>>> +		if (tmp < diff) {
>>>>> +			diff = tmp;
>>>>> +			*top = idx;
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	return dw_wdt->timeouts[*top];
>>>>> +}
>>>>> +
>>>>> +static unsigned int dw_wdt_find_min_timeout(struct dw_wdt *dw_wdt)
>>>>
>>>> I would appreciate if the names of functions returning ms end with _ms
>>>> to avoid confusion.
>>>
>>> Ok. I'll also modify the functions a bit, so only the
>>> dw_wdt_find_best_top_ms() and dw_wdt_find_max_top_ms() methods would
>>> return timeouts in milliseconds. Though if you insist in keeping seconds
>>> in the timeouts array (see the comment after the next one), it'll be
>>> dw_wdt_find_max_top_ms() only.
>>>
>>>>
>>>>> +{
>>>>> +	u32 min_timeout = UINT_MAX, top;
>>>>> +	int idx;
>>>>> +
>>>>> +	for (top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
>>>>> +		if (dw_wdt->timeouts[idx] <= min_timeout) {
>>>>> +			min_timeout = dw_wdt->timeouts[idx];
>>>>> +			top = idx;
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	return dw_wdt->timeouts[top];
>>>>> +}
>>>>> +
>>>>> +static unsigned int dw_wdt_find_max_top(struct dw_wdt *dw_wdt, u32 *top)
>>>>> +{
>>>>> +	u32 max_timeout = 0;
>>>>> +	int idx;
>>>>> +
>>>>> +	for (*top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
>>>>> +		if (dw_wdt->timeouts[idx] >= max_timeout) {
>>>>> +			max_timeout = dw_wdt->timeouts[idx];
>>>>> +			*top = idx;
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	return dw_wdt->timeouts[*top];
>>>>>  }
>>>>>  
>>>>> -static int dw_wdt_get_top(struct dw_wdt *dw_wdt)
>>>>> +static unsigned int dw_wdt_get_timeout(struct dw_wdt *dw_wdt)
>>>>>  {
>>>>>  	int top = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
>>>>>  
>>>>> -	return dw_wdt_top_in_seconds(dw_wdt, top);
>>>>> +	return dw_wdt->timeouts[top];
>>>>>  }
>>>>>  
>>>>>  static int dw_wdt_ping(struct watchdog_device *wdd)
>>>>> @@ -90,20 +153,13 @@ static int dw_wdt_ping(struct watchdog_device *wdd)
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> -static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
>>>>> +static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
>>>>>  {
>>>>>  	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
>>>>> -	int i, top_val = DW_WDT_MAX_TOP;
>>>>> +	unsigned int timeout;
>>>>> +	u32 top;
>>>>>  
>>>>> -	/*
>>>>> -	 * Iterate over the timeout values until we find the closest match. We
>>>>> -	 * always look for >=.
>>>>> -	 */
>>>>> -	for (i = 0; i <= DW_WDT_MAX_TOP; ++i)
>>>>> -		if (dw_wdt_top_in_seconds(dw_wdt, i) >= top_s) {
>>>>> -			top_val = i;
>>>>> -			break;
>>>>> -		}
>>>>> +	timeout = dw_wdt_find_best_top(dw_wdt, req * MSEC_PER_SEC, &top);
>>>>>  
>>>>>  	/*
>>>>>  	 * Set the new value in the watchdog.  Some versions of dw_wdt
>>>>> @@ -111,7 +167,7 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
>>>>>  	 * CP_WDT_DUAL_TOP in WDT_COMP_PARAMS_1).  On those we
>>>>>  	 * effectively get a pat of the watchdog right here.
>>>>>  	 */
>>>>> -	writel(top_val | top_val << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
>>>>> +	writel(top | top << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
>>>>>  	       dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
>>>>>  
>>>>>  	/*
>>>>> @@ -119,10 +175,10 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
>>>>>  	 * kernel(watchdog_dev.c) helps to feed watchdog before
>>>>>  	 * wdd->max_hw_heartbeat_ms
>>>>>  	 */
>>>>> -	if (top_s * 1000 <= wdd->max_hw_heartbeat_ms)
>>>>> -		wdd->timeout = dw_wdt_top_in_seconds(dw_wdt, top_val);
>>>>> +	if (req * MSEC_PER_SEC > wdd->max_hw_heartbeat_ms)
>>>>> +		wdd->timeout = req;
>>>>>  	else
>>>>> -		wdd->timeout = top_s;
>>>>> +		wdd->timeout = timeout / MSEC_PER_SEC;
>>>>>  
>>>>>  	return 0;
>>>>>  }
>>>>> @@ -238,6 +294,41 @@ static int dw_wdt_resume(struct device *dev)
>>>>>  
>>>>>  static SIMPLE_DEV_PM_OPS(dw_wdt_pm_ops, dw_wdt_suspend, dw_wdt_resume);
>>>>>  
>>>>> +static void dw_wdt_init_timeouts(struct dw_wdt *dw_wdt, struct device *dev)
>>>>> +{
>>>>> +	u32 data, of_tops[DW_WDT_NUM_TOPS];
>>>>> +	const u32 *tops;
>>>>> +	int ret, idx;
>>>>> +
>>>>> +	/*
>>>>> +	 * Retrieve custom or fixed counter values depending on the
>>>>> +	 * WDT_USE_FIX_TOP flag found in the component specific parameters
>>>>> +	 * #1 register.
>>>>> +	 */
>>>>> +	data = readl(dw_wdt->regs + WDOG_COMP_PARAMS_1_REG_OFFSET);
>>>>> +	if (data & WDOG_COMP_PARAMS_1_USE_FIX_TOP) {
>>>>> +		tops = dw_wdt_fix_tops;
>>>>> +	} else {
>>>>> +		ret = of_property_read_variable_u32_array(dev_of_node(dev),
>>>>> +			"snps,watchdog-tops", of_tops, DW_WDT_NUM_TOPS,
>>>>> +			DW_WDT_NUM_TOPS);
>>>>> +		if (ret < 0) {
>>>>> +			dev_warn(dev, "No valid TOPs array specified\n");
>>>>> +			tops = dw_wdt_fix_tops;
>>>>> +		} else {
>>>>> +			tops = of_tops;
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	/*
>>>>> +	 * We'll keep the timeout values in ms to approximate requested
>>>>> +	 * timeouts with better accuracy.
>>>>> +	 */
>>>>> +	for (idx = 0; idx < DW_WDT_NUM_TOPS; ++idx)
>>>>> +		dw_wdt->timeouts[idx] =
>>>>> +			mult_frac(tops[idx], MSEC_PER_SEC, dw_wdt->rate);
>>>>
>>>> tops[idx] type is u32. Its value can be up to 0xffffffff. That means
>>>> dw_wdt->rate must be >= 1000 to avoid overflow, which you should check.
>>>
>>> Right. I don't think that TOPs with timeouts bigger than
>>> 0xffffffff milliseconds have any valuable usecases, so I'll just round
>>> the overflows down to FFs.
>>>
>>
>> Neither do unsorted random timeouts milli-seconds apart. You see the need
>> to address one, so addressing other weaknesses is appropriate.
> 
> Don't really understand what you mean. Do you intend to filter the
> unreachable timeouts out from the timeouts array? If so this isn't
> possible with current design. I would have to implement a more complex
> data structure, like an array of pairs {TOP, timeout} and refactor the
> timeout search algorithm. Don't really think this optimization is required
> seeing watchdog timeout set operation is normally performed just a few times
> per watchdog usage session.
> 

You state that defining a tops[idx] value > 0xffffffff / 1000 would be
unreasonable, while at the same time you argue that a sequence of tops[idx]
values of, say, 45, 46, 47, 43, 42, 99, 98, 55 would be perfectly reasonable
and needs to be handled. All I am saying is that you need to deal with all odd
cases, and that you can not assume that there is no tops[idx] value that results
in an overflow. If "45, 46, 47, 43, 42, 99, 98, 55" is reasonable, so is
0xffffffff.

Guenter

Re: [PATCH 4/7] watchdog: dw_wdt: Support devices with non-fixed TOP values

[Sergey Semin]

On Fri, Apr 10, 2020 at 06:15:30PM -0700, Guenter Roeck wrote:
> On 4/10/20 12:45 PM, Sergey Semin wrote:
> > On Fri, Apr 10, 2020 at 09:21:35AM -0700, Guenter Roeck wrote:
> >> On 4/10/20 5:59 AM, Sergey Semin wrote:
> >>> On Sun, Mar 15, 2020 at 07:12:38AM -0700, Guenter Roeck wrote:
> >>>> On Fri, Mar 06, 2020 at 04:27:44PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> >>>>> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> >>>>>
> >>>>> In case if the DW Watchdog IP core is synthesised with
> >>>>> WDT_USE_FIX_TOP == false, the TOP interval indexes make the device
> >>>>> to load a custom periods to the counter. These periods are hardwired
> >>>>> at the synthesis stage and can be within [2^8, 2^(WDT_CNT_WIDTH - 1)].
> >>>>> Alas their values can't be detected at runtime and must be somehow
> >>>>> supplied to the driver so one could properly determine the watchdog
> >>>>> timeout intervals. For this purpose we suggest to have a vendor-
> >>>>> specific dts property "snps,watchdog-tops" utilized, which would
> >>>>> provide an array of sixteen counter values. At device probe stage they
> >>>>> will be used to initialize the watchdog device timeouts determined
> >>>>> from the array values and current clocks source rate.
> >>>>>
> >>>>> In order to have custom TOP values supported the driver must be
> >>>>> altered in the following way. First of all the fixed-top values
> >>>>> ready-to-use array must be determined for compatibility with currently
> >>>>> supported devices, which were synthesised with WDT_USE_FIX_TOP == true.
> >>>>> Secondly we must redefine the timer period search functions. For
> >>>>> generality they are redesigned in a way to support the TOP array with
> >>>>> no limitations on the items order or value. Finally an array with
> >>>>> pre-defined timeouts must be calculated at probe stage from either
> >>>>> the custom or fixed TOP values depending on the DW watchdog component
> >>>>> parameter WDT_USE_FIX_TOP value.
> >>>>>
> >>>>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> >>>>> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> >>>>> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> >>>>> Cc: Paul Burton <paulburton@kernel.org>
> >>>>> Cc: Ralf Baechle <ralf@linux-mips.org>
> >>>>> ---
> >>>>>  drivers/watchdog/dw_wdt.c | 145 +++++++++++++++++++++++++++++++-------
> >>>>>  1 file changed, 119 insertions(+), 26 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> >>>>> index fba21de2bbad..4a57b7d777dc 100644
> >>>>> --- a/drivers/watchdog/dw_wdt.c
> >>>>> +++ b/drivers/watchdog/dw_wdt.c
> >>>>> @@ -13,6 +13,7 @@
> >>>>>   */
> >>>>>  
> >>>>>  #include <linux/bitops.h>
> >>>>> +#include <linux/limits.h>
> >>>>>  #include <linux/clk.h>
> >>>>>  #include <linux/delay.h>
> >>>>>  #include <linux/err.h>
> >>>>> @@ -34,12 +35,24 @@
> >>>>>  #define WDOG_CURRENT_COUNT_REG_OFFSET	    0x08
> >>>>>  #define WDOG_COUNTER_RESTART_REG_OFFSET     0x0c
> >>>>>  #define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
> >>>>> +#define WDOG_COMP_PARAMS_1_REG_OFFSET       0xf4
> >>>>> +#define WDOG_COMP_PARAMS_1_USE_FIX_TOP      BIT(6)
> >>>>>  
> >>>>> -/* The maximum TOP (timeout period) value that can be set in the watchdog. */
> >>>>> -#define DW_WDT_MAX_TOP		15
> >>>>> +/* There are sixteen TOPs (timeout periods) that can be set in the watchdog. */
> >>>>> +#define DW_WDT_NUM_TOPS		16
> >>>>> +#define DW_WDT_FIX_TOP(_idx)	(1U << (16 + _idx))
> >>>>>  
> >>>>>  #define DW_WDT_DEFAULT_SECONDS	30
> >>>>>  
> >>>>> +static const u32 dw_wdt_fix_tops[DW_WDT_NUM_TOPS] = {
> >>>>> +	DW_WDT_FIX_TOP(0), DW_WDT_FIX_TOP(1), DW_WDT_FIX_TOP(2),
> >>>>> +	DW_WDT_FIX_TOP(3), DW_WDT_FIX_TOP(4), DW_WDT_FIX_TOP(5),
> >>>>> +	DW_WDT_FIX_TOP(6), DW_WDT_FIX_TOP(7), DW_WDT_FIX_TOP(8),
> >>>>> +	DW_WDT_FIX_TOP(9), DW_WDT_FIX_TOP(10), DW_WDT_FIX_TOP(11),
> >>>>> +	DW_WDT_FIX_TOP(12), DW_WDT_FIX_TOP(13), DW_WDT_FIX_TOP(14),
> >>>>> +	DW_WDT_FIX_TOP(15)
> >>>>> +};
> >>>>> +
> >>>>>  static bool nowayout = WATCHDOG_NOWAYOUT;
> >>>>>  module_param(nowayout, bool, 0);
> >>>>>  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> >>>>> @@ -49,6 +62,8 @@ struct dw_wdt {
> >>>>>  	void __iomem		*regs;
> >>>>>  	struct clk		*clk;
> >>>>>  	unsigned long		rate;
> >>>>> +	unsigned int		max_top;
> >>>>> +	unsigned int		timeouts[DW_WDT_NUM_TOPS];
> >>>>>  	struct watchdog_device	wdd;
> >>>>>  	struct reset_control	*rst;
> >>>>>  	/* Save/restore */
> >>>>> @@ -64,20 +79,68 @@ static inline int dw_wdt_is_enabled(struct dw_wdt *dw_wdt)
> >>>>>  		WDOG_CONTROL_REG_WDT_EN_MASK;
> >>>>>  }
> >>>>>  
> >>>>> -static inline int dw_wdt_top_in_seconds(struct dw_wdt *dw_wdt, unsigned top)
> >>>>> +static unsigned int dw_wdt_find_best_top(struct dw_wdt *dw_wdt,
> >>>>> +					 unsigned int timeout, u32 *top)
> >>>>>  {
> >>>>> +	u32 diff = UINT_MAX, tmp;
> >>>>> +	int idx;
> >>>>> +
> >>>>>  	/*
> >>>>> -	 * There are 16 possible timeout values in 0..15 where the number of
> >>>>> -	 * cycles is 2 ^ (16 + i) and the watchdog counts down.
> >>>>> +	 * In general case of non-fixed timeout values they can be arranged in
> >>>>> +	 * any order so we have to traverse all the array values. We also try
> >>>>> +	 * to find a closest timeout number and make sure its value is greater
> >>>>> +	 * than the requested timeout. Note we'll return a maximum timeout
> >>>>> +	 * if reachable value couldn't be found.
> >>>>>  	 */
> >>>>> -	return (1U << (16 + top)) / dw_wdt->rate;
> >>>>> +	for (*top = dw_wdt->max_top, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
> >>>>> +		if (dw_wdt->timeouts[idx] < timeout)
> >>>>> +			continue;
> >>>>> +
> >>>>> +		tmp = dw_wdt->timeouts[idx] - timeout;
> >>>>> +		if (tmp < diff) {
> >>>>> +			diff = tmp;
> >>>>> +			*top = idx;
> >>>>> +		}
> >>>>> +	}
> >>>>> +
> >>>>> +	return dw_wdt->timeouts[*top];
> >>>>> +}
> >>>>> +
> >>>>> +static unsigned int dw_wdt_find_min_timeout(struct dw_wdt *dw_wdt)
> >>>>
> >>>> I would appreciate if the names of functions returning ms end with _ms
> >>>> to avoid confusion.
> >>>
> >>> Ok. I'll also modify the functions a bit, so only the
> >>> dw_wdt_find_best_top_ms() and dw_wdt_find_max_top_ms() methods would
> >>> return timeouts in milliseconds. Though if you insist in keeping seconds
> >>> in the timeouts array (see the comment after the next one), it'll be
> >>> dw_wdt_find_max_top_ms() only.
> >>>
> >>>>
> >>>>> +{
> >>>>> +	u32 min_timeout = UINT_MAX, top;
> >>>>> +	int idx;
> >>>>> +
> >>>>> +	for (top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
> >>>>> +		if (dw_wdt->timeouts[idx] <= min_timeout) {
> >>>>> +			min_timeout = dw_wdt->timeouts[idx];
> >>>>> +			top = idx;
> >>>>> +		}
> >>>>> +	}
> >>>>> +
> >>>>> +	return dw_wdt->timeouts[top];
> >>>>> +}
> >>>>> +
> >>>>> +static unsigned int dw_wdt_find_max_top(struct dw_wdt *dw_wdt, u32 *top)
> >>>>> +{
> >>>>> +	u32 max_timeout = 0;
> >>>>> +	int idx;
> >>>>> +
> >>>>> +	for (*top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
> >>>>> +		if (dw_wdt->timeouts[idx] >= max_timeout) {
> >>>>> +			max_timeout = dw_wdt->timeouts[idx];
> >>>>> +			*top = idx;
> >>>>> +		}
> >>>>> +	}
> >>>>> +
> >>>>> +	return dw_wdt->timeouts[*top];
> >>>>>  }
> >>>>>  
> >>>>> -static int dw_wdt_get_top(struct dw_wdt *dw_wdt)
> >>>>> +static unsigned int dw_wdt_get_timeout(struct dw_wdt *dw_wdt)
> >>>>>  {
> >>>>>  	int top = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
> >>>>>  
> >>>>> -	return dw_wdt_top_in_seconds(dw_wdt, top);
> >>>>> +	return dw_wdt->timeouts[top];
> >>>>>  }
> >>>>>  
> >>>>>  static int dw_wdt_ping(struct watchdog_device *wdd)
> >>>>> @@ -90,20 +153,13 @@ static int dw_wdt_ping(struct watchdog_device *wdd)
> >>>>>  	return 0;
> >>>>>  }
> >>>>>  
> >>>>> -static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> >>>>> +static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
> >>>>>  {
> >>>>>  	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> >>>>> -	int i, top_val = DW_WDT_MAX_TOP;
> >>>>> +	unsigned int timeout;
> >>>>> +	u32 top;
> >>>>>  
> >>>>> -	/*
> >>>>> -	 * Iterate over the timeout values until we find the closest match. We
> >>>>> -	 * always look for >=.
> >>>>> -	 */
> >>>>> -	for (i = 0; i <= DW_WDT_MAX_TOP; ++i)
> >>>>> -		if (dw_wdt_top_in_seconds(dw_wdt, i) >= top_s) {
> >>>>> -			top_val = i;
> >>>>> -			break;
> >>>>> -		}
> >>>>> +	timeout = dw_wdt_find_best_top(dw_wdt, req * MSEC_PER_SEC, &top);
> >>>>>  
> >>>>>  	/*
> >>>>>  	 * Set the new value in the watchdog.  Some versions of dw_wdt
> >>>>> @@ -111,7 +167,7 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> >>>>>  	 * CP_WDT_DUAL_TOP in WDT_COMP_PARAMS_1).  On those we
> >>>>>  	 * effectively get a pat of the watchdog right here.
> >>>>>  	 */
> >>>>> -	writel(top_val | top_val << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
> >>>>> +	writel(top | top << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
> >>>>>  	       dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> >>>>>  
> >>>>>  	/*
> >>>>> @@ -119,10 +175,10 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> >>>>>  	 * kernel(watchdog_dev.c) helps to feed watchdog before
> >>>>>  	 * wdd->max_hw_heartbeat_ms
> >>>>>  	 */
> >>>>> -	if (top_s * 1000 <= wdd->max_hw_heartbeat_ms)
> >>>>> -		wdd->timeout = dw_wdt_top_in_seconds(dw_wdt, top_val);
> >>>>> +	if (req * MSEC_PER_SEC > wdd->max_hw_heartbeat_ms)
> >>>>> +		wdd->timeout = req;
> >>>>>  	else
> >>>>> -		wdd->timeout = top_s;
> >>>>> +		wdd->timeout = timeout / MSEC_PER_SEC;
> >>>>>  
> >>>>>  	return 0;
> >>>>>  }
> >>>>> @@ -238,6 +294,41 @@ static int dw_wdt_resume(struct device *dev)
> >>>>>  
> >>>>>  static SIMPLE_DEV_PM_OPS(dw_wdt_pm_ops, dw_wdt_suspend, dw_wdt_resume);
> >>>>>  
> >>>>> +static void dw_wdt_init_timeouts(struct dw_wdt *dw_wdt, struct device *dev)
> >>>>> +{
> >>>>> +	u32 data, of_tops[DW_WDT_NUM_TOPS];
> >>>>> +	const u32 *tops;
> >>>>> +	int ret, idx;
> >>>>> +
> >>>>> +	/*
> >>>>> +	 * Retrieve custom or fixed counter values depending on the
> >>>>> +	 * WDT_USE_FIX_TOP flag found in the component specific parameters
> >>>>> +	 * #1 register.
> >>>>> +	 */
> >>>>> +	data = readl(dw_wdt->regs + WDOG_COMP_PARAMS_1_REG_OFFSET);
> >>>>> +	if (data & WDOG_COMP_PARAMS_1_USE_FIX_TOP) {
> >>>>> +		tops = dw_wdt_fix_tops;
> >>>>> +	} else {
> >>>>> +		ret = of_property_read_variable_u32_array(dev_of_node(dev),
> >>>>> +			"snps,watchdog-tops", of_tops, DW_WDT_NUM_TOPS,
> >>>>> +			DW_WDT_NUM_TOPS);
> >>>>> +		if (ret < 0) {
> >>>>> +			dev_warn(dev, "No valid TOPs array specified\n");
> >>>>> +			tops = dw_wdt_fix_tops;
> >>>>> +		} else {
> >>>>> +			tops = of_tops;
> >>>>> +		}
> >>>>> +	}
> >>>>> +
> >>>>> +	/*
> >>>>> +	 * We'll keep the timeout values in ms to approximate requested
> >>>>> +	 * timeouts with better accuracy.
> >>>>> +	 */
> >>>>> +	for (idx = 0; idx < DW_WDT_NUM_TOPS; ++idx)
> >>>>> +		dw_wdt->timeouts[idx] =
> >>>>> +			mult_frac(tops[idx], MSEC_PER_SEC, dw_wdt->rate);
> >>>>
> >>>> tops[idx] type is u32. Its value can be up to 0xffffffff. That means
> >>>> dw_wdt->rate must be >= 1000 to avoid overflow, which you should check.
> >>>
> >>> Right. I don't think that TOPs with timeouts bigger than
> >>> 0xffffffff milliseconds have any valuable usecases, so I'll just round
> >>> the overflows down to FFs.
> >>>
> >>
> >> Neither do unsorted random timeouts milli-seconds apart. You see the need
> >> to address one, so addressing other weaknesses is appropriate.
> > 
> > Don't really understand what you mean. Do you intend to filter the
> > unreachable timeouts out from the timeouts array? If so this isn't
> > possible with current design. I would have to implement a more complex
> > data structure, like an array of pairs {TOP, timeout} and refactor the
> > timeout search algorithm. Don't really think this optimization is required
> > seeing watchdog timeout set operation is normally performed just a few times
> > per watchdog usage session.
> > 
> 
> You state that defining a tops[idx] value > 0xffffffff / 1000 would be
> unreasonable, while at the same time you argue that a sequence of tops[idx]
> values of, say, 45, 46, 47, 43, 42, 99, 98, 55 would be perfectly reasonable
> and needs to be handled. All I am saying is that you need to deal with all odd
> cases, and that you can not assume that there is no tops[idx] value that results
> in an overflow. If "45, 46, 47, 43, 42, 99, 98, 55" is reasonable, so is
> 0xffffffff.

I am not saying, that TOPs like "45, 46, 47, 43, 42, 99, 98, 55" are
reasonable. Depending on the reference clock rate, most likely they will
be unreachable by the watchdog core. The core requests timeout in
seconds with minimum of 1 second, while my timeout-search algorithm returns
the closest but bigger than the requested timeout, which effectively
filters those TOPs out of any permitted selection. BTW there is another problem.
What if due to the too high reference clock frequency non of the TOPs
are able to reach 0.5 second timeout?.. The same problem could have happened
even without an upgrade provided by my patches.

Getting back to the issue. Suppose we had TOPs that amongst others
corresponded to the next timeouts: 1.01 and 1.99 sec. So if watchdog
core requests to set 1 s timeout, my search loop will select TOP
with 1.01 s. Since watchdog core is working with timeouts of 1 s
granularity, then TOP with 1.99 s will never be selected. If I didn't
have the milliseconds values of the TOPs, but the seconds only, I
wouldn't be able to make a proper decision between those two timeouts
seeing the TOPs might be in any order.

Regarding all the weaknesses. In order to address all of them (please
also note, that the previous version of the code didn't lack of some of
them) I would have to come up with much cleverer algorithm than the one
currently implemented. It would have to filter out all the unreachable
timeouts out of the array.

Regarding 0xffffffff being unreasonable. You're right. I shouldn't have
assumed this. Depending on the reference clock rate, such TOP might be
not that big value in seconds.

To sum up our discussion AFAICS the best way would be to create an array
of structures:
	struct dw_wdt_timeout {
		u32 top;
		unsigned int timeout;
	} timeouts[16];
which would have only reachable timeouts in !seconds! If we had a set
of uniquely reachable TOPs we wouldn't need to keep values in
milliseconds. We'd also get rid of the redundant unreachable values and
would solve a problem if non of the specified TOPs is reachable with
current reference clock rate. What do you think?

-Sergey

> 
> Guenter

[PATCH 5/7] watchdog: dw_wdt: Support devices with asynch clocks

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

DW Watchdog IP core can be synthesised with asynchronous timer/APB
clocks support (WDT_ASYNC_CLK_MODE_ENABLE == 1). In this case
separate clock signals are supposed to be used to feed watchdog timer
and APB interface of the device. Currently the driver supports
the synchronous mode only. Since there is no way to determine which
mode was actually activated for device from its registers, we have to
rely on the platform device configuration data. If optional "pclk"
clock source is supplied, we consider the device working in asynchronous
mode, otherwise the driver falls back to the synchronous configuration.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 drivers/watchdog/dw_wdt.c | 48 +++++++++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index 4a57b7d777dc..eb909c63a1b5 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -61,6 +61,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 struct dw_wdt {
 	void __iomem		*regs;
 	struct clk		*clk;
+	struct clk		*pclk;
 	unsigned long		rate;
 	unsigned int		max_top;
 	unsigned int		timeouts[DW_WDT_NUM_TOPS];
@@ -270,6 +271,7 @@ static int dw_wdt_suspend(struct device *dev)
 	dw_wdt->control = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
 	dw_wdt->timeout = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
 
+	clk_disable_unprepare(dw_wdt->pclk);
 	clk_disable_unprepare(dw_wdt->clk);
 
 	return 0;
@@ -283,6 +285,12 @@ static int dw_wdt_resume(struct device *dev)
 	if (err)
 		return err;
 
+	err = clk_prepare_enable(dw_wdt->pclk);
+	if (err) {
+		clk_disable_unprepare(dw_wdt->clk);
+		return err;
+	}
+
 	writel(dw_wdt->timeout, dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
 	writel(dw_wdt->control, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
 
@@ -344,9 +352,18 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 	if (IS_ERR(dw_wdt->regs))
 		return PTR_ERR(dw_wdt->regs);
 
-	dw_wdt->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(dw_wdt->clk))
-		return PTR_ERR(dw_wdt->clk);
+	/*
+	 * Try to request the watchdog dedicated timer clock source. It must
+	 * be supplied if asynchronous mode is enabled. Otherwise fallback
+	 * to the common timer/bus clocks configuration, in which the very first
+	 * found clocks supply both timer and APB signals.
+	 */
+	dw_wdt->clk = devm_clk_get(dev, "tclk");
+	if (IS_ERR(dw_wdt->clk)) {
+		dw_wdt->clk = devm_clk_get(dev, NULL);
+		if (IS_ERR(dw_wdt->clk))
+			return PTR_ERR(dw_wdt->clk);
+	}
 
 	ret = clk_prepare_enable(dw_wdt->clk);
 	if (ret)
@@ -358,10 +375,27 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 		goto out_disable_clk;
 	}
 
+	/*
+	 * Request APB clocks if device is configured with async clocks mode.
+	 * In this case both tclk and pclk clocks are supposed to be specified.
+	 * Alas we can't know for sure whether async mode was really activated,
+	 * so the pclk reference is left optional. If it it's failed to be
+	 * found we consider the device configured in synchronous clocks mode.
+	 */
+	dw_wdt->pclk = devm_clk_get_optional(dev, "pclk");
+	if (IS_ERR(dw_wdt->pclk)) {
+		ret = PTR_ERR(dw_wdt->pclk);
+		goto out_disable_clk;
+	}
+
+	ret = clk_prepare_enable(dw_wdt->pclk);
+	if (ret)
+		goto out_disable_clk;
+
 	dw_wdt->rst = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
 	if (IS_ERR(dw_wdt->rst)) {
 		ret = PTR_ERR(dw_wdt->rst);
-		goto out_disable_clk;
+		goto out_disable_pclk;
 	}
 
 	reset_control_deassert(dw_wdt->rst);
@@ -399,10 +433,13 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 
 	ret = watchdog_register_device(wdd);
 	if (ret)
-		goto out_disable_clk;
+		goto out_disable_pclk;
 
 	return 0;
 
+out_disable_pclk:
+	clk_disable_unprepare(dw_wdt->pclk);
+
 out_disable_clk:
 	clk_disable_unprepare(dw_wdt->clk);
 	return ret;
@@ -414,6 +451,7 @@ static int dw_wdt_drv_remove(struct platform_device *pdev)
 
 	watchdog_unregister_device(&dw_wdt->wdd);
 	reset_control_assert(dw_wdt->rst);
+	clk_disable_unprepare(dw_wdt->pclk);
 	clk_disable_unprepare(dw_wdt->clk);
 
 	return 0;
-- 
2.25.1

Re: [PATCH 5/7] watchdog: dw_wdt: Support devices with asynch clocks

[Guenter Roeck]

On Fri, Mar 06, 2020 at 04:27:45PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> DW Watchdog IP core can be synthesised with asynchronous timer/APB
> clocks support (WDT_ASYNC_CLK_MODE_ENABLE == 1). In this case
> separate clock signals are supposed to be used to feed watchdog timer
> and APB interface of the device. Currently the driver supports
> the synchronous mode only. Since there is no way to determine which
> mode was actually activated for device from its registers, we have to
> rely on the platform device configuration data. If optional "pclk"
> clock source is supplied, we consider the device working in asynchronous
> mode, otherwise the driver falls back to the synchronous configuration.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---
>  drivers/watchdog/dw_wdt.c | 48 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index 4a57b7d777dc..eb909c63a1b5 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -61,6 +61,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
>  struct dw_wdt {
>  	void __iomem		*regs;
>  	struct clk		*clk;
> +	struct clk		*pclk;
>  	unsigned long		rate;
>  	unsigned int		max_top;
>  	unsigned int		timeouts[DW_WDT_NUM_TOPS];
> @@ -270,6 +271,7 @@ static int dw_wdt_suspend(struct device *dev)
>  	dw_wdt->control = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
>  	dw_wdt->timeout = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
>  
> +	clk_disable_unprepare(dw_wdt->pclk);
>  	clk_disable_unprepare(dw_wdt->clk);
>  
>  	return 0;
> @@ -283,6 +285,12 @@ static int dw_wdt_resume(struct device *dev)
>  	if (err)
>  		return err;
>  
> +	err = clk_prepare_enable(dw_wdt->pclk);
> +	if (err) {
> +		clk_disable_unprepare(dw_wdt->clk);
> +		return err;
> +	}
> +
>  	writel(dw_wdt->timeout, dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
>  	writel(dw_wdt->control, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
>  
> @@ -344,9 +352,18 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>  	if (IS_ERR(dw_wdt->regs))
>  		return PTR_ERR(dw_wdt->regs);
>  
> -	dw_wdt->clk = devm_clk_get(dev, NULL);
> -	if (IS_ERR(dw_wdt->clk))
> -		return PTR_ERR(dw_wdt->clk);
> +	/*
> +	 * Try to request the watchdog dedicated timer clock source. It must
> +	 * be supplied if asynchronous mode is enabled. Otherwise fallback
> +	 * to the common timer/bus clocks configuration, in which the very first
> +	 * found clocks supply both timer and APB signals.
> +	 */
> +	dw_wdt->clk = devm_clk_get(dev, "tclk");
> +	if (IS_ERR(dw_wdt->clk)) {
> +		dw_wdt->clk = devm_clk_get(dev, NULL);
> +		if (IS_ERR(dw_wdt->clk))
> +			return PTR_ERR(dw_wdt->clk);
> +	}
>  
>  	ret = clk_prepare_enable(dw_wdt->clk);
>  	if (ret)
> @@ -358,10 +375,27 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>  		goto out_disable_clk;
>  	}
>  
> +	/*
> +	 * Request APB clocks if device is configured with async clocks mode.
> +	 * In this case both tclk and pclk clocks are supposed to be specified.
> +	 * Alas we can't know for sure whether async mode was really activated,
> +	 * so the pclk reference is left optional. If it it's failed to be
> +	 * found we consider the device configured in synchronous clocks mode.
> +	 */
> +	dw_wdt->pclk = devm_clk_get_optional(dev, "pclk");
> +	if (IS_ERR(dw_wdt->pclk)) {
> +		ret = PTR_ERR(dw_wdt->pclk);
> +		goto out_disable_clk;
> +	}
> +
> +	ret = clk_prepare_enable(dw_wdt->pclk);

Not every implementation of clk_enable() checks for a NULL parameter.
Some return an error. This can not be trusted to work on all platforms /
architectures.

> +	if (ret)
> +		goto out_disable_clk;
> +
>  	dw_wdt->rst = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
>  	if (IS_ERR(dw_wdt->rst)) {
>  		ret = PTR_ERR(dw_wdt->rst);
> -		goto out_disable_clk;
> +		goto out_disable_pclk;
>  	}
>  
>  	reset_control_deassert(dw_wdt->rst);
> @@ -399,10 +433,13 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>  
>  	ret = watchdog_register_device(wdd);
>  	if (ret)
> -		goto out_disable_clk;
> +		goto out_disable_pclk;
>  
>  	return 0;
>  
> +out_disable_pclk:
> +	clk_disable_unprepare(dw_wdt->pclk);
> +
>  out_disable_clk:
>  	clk_disable_unprepare(dw_wdt->clk);
>  	return ret;
> @@ -414,6 +451,7 @@ static int dw_wdt_drv_remove(struct platform_device *pdev)
>  
>  	watchdog_unregister_device(&dw_wdt->wdd);
>  	reset_control_assert(dw_wdt->rst);
> +	clk_disable_unprepare(dw_wdt->pclk);
>  	clk_disable_unprepare(dw_wdt->clk);
>  
>  	return 0;

Re: [PATCH 5/7] watchdog: dw_wdt: Support devices with asynch clocks

[Sergey Semin]

Michael, Stephen, could you take a look at the issue we've got here?

Guenter, my comment is below.

On Sun, Mar 15, 2020 at 07:22:07AM -0700, Guenter Roeck wrote:
> On Fri, Mar 06, 2020 at 04:27:45PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > DW Watchdog IP core can be synthesised with asynchronous timer/APB
> > clocks support (WDT_ASYNC_CLK_MODE_ENABLE == 1). In this case
> > separate clock signals are supposed to be used to feed watchdog timer
> > and APB interface of the device. Currently the driver supports
> > the synchronous mode only. Since there is no way to determine which
> > mode was actually activated for device from its registers, we have to
> > rely on the platform device configuration data. If optional "pclk"
> > clock source is supplied, we consider the device working in asynchronous
> > mode, otherwise the driver falls back to the synchronous configuration.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > ---
> >  drivers/watchdog/dw_wdt.c | 48 +++++++++++++++++++++++++++++++++++----
> >  1 file changed, 43 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> > index 4a57b7d777dc..eb909c63a1b5 100644
> > --- a/drivers/watchdog/dw_wdt.c
> > +++ b/drivers/watchdog/dw_wdt.c
> > @@ -61,6 +61,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> >  struct dw_wdt {
> >  	void __iomem		*regs;
> >  	struct clk		*clk;
> > +	struct clk		*pclk;
> >  	unsigned long		rate;
> >  	unsigned int		max_top;
> >  	unsigned int		timeouts[DW_WDT_NUM_TOPS];
> > @@ -270,6 +271,7 @@ static int dw_wdt_suspend(struct device *dev)
> >  	dw_wdt->control = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> >  	dw_wdt->timeout = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> >  
> > +	clk_disable_unprepare(dw_wdt->pclk);
> >  	clk_disable_unprepare(dw_wdt->clk);
> >  
> >  	return 0;
> > @@ -283,6 +285,12 @@ static int dw_wdt_resume(struct device *dev)
> >  	if (err)
> >  		return err;
> >  
> > +	err = clk_prepare_enable(dw_wdt->pclk);
> > +	if (err) {
> > +		clk_disable_unprepare(dw_wdt->clk);
> > +		return err;
> > +	}
> > +
> >  	writel(dw_wdt->timeout, dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> >  	writel(dw_wdt->control, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> >  
> > @@ -344,9 +352,18 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >  	if (IS_ERR(dw_wdt->regs))
> >  		return PTR_ERR(dw_wdt->regs);
> >  
> > -	dw_wdt->clk = devm_clk_get(dev, NULL);
> > -	if (IS_ERR(dw_wdt->clk))
> > -		return PTR_ERR(dw_wdt->clk);
> > +	/*
> > +	 * Try to request the watchdog dedicated timer clock source. It must
> > +	 * be supplied if asynchronous mode is enabled. Otherwise fallback
> > +	 * to the common timer/bus clocks configuration, in which the very first
> > +	 * found clocks supply both timer and APB signals.
> > +	 */
> > +	dw_wdt->clk = devm_clk_get(dev, "tclk");
> > +	if (IS_ERR(dw_wdt->clk)) {
> > +		dw_wdt->clk = devm_clk_get(dev, NULL);
> > +		if (IS_ERR(dw_wdt->clk))
> > +			return PTR_ERR(dw_wdt->clk);
> > +	}
> >  
> >  	ret = clk_prepare_enable(dw_wdt->clk);
> >  	if (ret)
> > @@ -358,10 +375,27 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >  		goto out_disable_clk;
> >  	}
> >  
> > +	/*
> > +	 * Request APB clocks if device is configured with async clocks mode.
> > +	 * In this case both tclk and pclk clocks are supposed to be specified.
> > +	 * Alas we can't know for sure whether async mode was really activated,
> > +	 * so the pclk reference is left optional. If it it's failed to be
> > +	 * found we consider the device configured in synchronous clocks mode.
> > +	 */
> > +	dw_wdt->pclk = devm_clk_get_optional(dev, "pclk");
> > +	if (IS_ERR(dw_wdt->pclk)) {
> > +		ret = PTR_ERR(dw_wdt->pclk);
> > +		goto out_disable_clk;
> > +	}
> > +
> > +	ret = clk_prepare_enable(dw_wdt->pclk);
> 
> Not every implementation of clk_enable() checks for a NULL parameter.
> Some return an error. This can not be trusted to work on all platforms /
> architectures.

Hm, this was unexpected twist. I've submitted not a single patch with optional
clock API usage. It was first time I've got a comment like this, that the
API isn't cross-platform. As I see it this isn't the patch problem, but the
platforms/common clock bug. The platforms code must have been submitted before
the optional clock API was introduced or the API hasn't been properly
implemented or we don't understand something.

Stephen, Michael could you clarify the situation with the
cross-platformness of the optional clock API.

-Sergey

> 
> > +	if (ret)
> > +		goto out_disable_clk;
> > +
> >  	dw_wdt->rst = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
> >  	if (IS_ERR(dw_wdt->rst)) {
> >  		ret = PTR_ERR(dw_wdt->rst);
> > -		goto out_disable_clk;
> > +		goto out_disable_pclk;
> >  	}
> >  
> >  	reset_control_deassert(dw_wdt->rst);
> > @@ -399,10 +433,13 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >  
> >  	ret = watchdog_register_device(wdd);
> >  	if (ret)
> > -		goto out_disable_clk;
> > +		goto out_disable_pclk;
> >  
> >  	return 0;
> >  
> > +out_disable_pclk:
> > +	clk_disable_unprepare(dw_wdt->pclk);
> > +
> >  out_disable_clk:
> >  	clk_disable_unprepare(dw_wdt->clk);
> >  	return ret;
> > @@ -414,6 +451,7 @@ static int dw_wdt_drv_remove(struct platform_device *pdev)
> >  
> >  	watchdog_unregister_device(&dw_wdt->wdd);
> >  	reset_control_assert(dw_wdt->rst);
> > +	clk_disable_unprepare(dw_wdt->pclk);
> >  	clk_disable_unprepare(dw_wdt->clk);
> >  
> >  	return 0;

Re: [PATCH 5/7] watchdog: dw_wdt: Support devices with asynch clocks

[Stephen Boyd]

Quoting Sergey Semin (2020-04-10 11:59:34)
> Michael, Stephen, could you take a look at the issue we've got here?
> 
> Guenter, my comment is below.
> 
> On Sun, Mar 15, 2020 at 07:22:07AM -0700, Guenter Roeck wrote:
> > On Fri, Mar 06, 2020 at 04:27:45PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> > > @@ -358,10 +375,27 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> > >             goto out_disable_clk;
> > >     }
> > >  
> > > +   /*
> > > +    * Request APB clocks if device is configured with async clocks mode.
> > > +    * In this case both tclk and pclk clocks are supposed to be specified.
> > > +    * Alas we can't know for sure whether async mode was really activated,
> > > +    * so the pclk reference is left optional. If it it's failed to be
> > > +    * found we consider the device configured in synchronous clocks mode.
> > > +    */
> > > +   dw_wdt->pclk = devm_clk_get_optional(dev, "pclk");
> > > +   if (IS_ERR(dw_wdt->pclk)) {
> > > +           ret = PTR_ERR(dw_wdt->pclk);
> > > +           goto out_disable_clk;
> > > +   }
> > > +
> > > +   ret = clk_prepare_enable(dw_wdt->pclk);
> > 
> > Not every implementation of clk_enable() checks for a NULL parameter.
> > Some return an error. This can not be trusted to work on all platforms /
> > architectures.
> 
> Hm, this was unexpected twist. I've submitted not a single patch with optional
> clock API usage. It was first time I've got a comment like this, that the
> API isn't cross-platform. As I see it this isn't the patch problem, but the
> platforms/common clock bug. The platforms code must have been submitted before
> the optional clock API was introduced or the API hasn't been properly
> implemented or we don't understand something.
> 
> Stephen, Michael could you clarify the situation with the
> cross-platformness of the optional clock API.
> 

NULL is a valid clk to return from clk_get(). And the documentation of
clk_enable() says that "If the clock can not be enabled/disabled, this
should return success". Given that a NULL pointer can't do much of
anything I think any platform that returns an error in this situation is
deviating from the documentation of the clk API.

Does any platform that uses this driver use one of these non-common clk
framework implementations? All of this may not matter if they all use
the CCF.

Re: [PATCH 5/7] watchdog: dw_wdt: Support devices with asynch clocks

[Guenter Roeck]

On 4/13/20 1:52 PM, Stephen Boyd wrote:
> Quoting Sergey Semin (2020-04-10 11:59:34)
>> Michael, Stephen, could you take a look at the issue we've got here?
>>
>> Guenter, my comment is below.
>>
>> On Sun, Mar 15, 2020 at 07:22:07AM -0700, Guenter Roeck wrote:
>>> On Fri, Mar 06, 2020 at 04:27:45PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
>>>> @@ -358,10 +375,27 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>>>>             goto out_disable_clk;
>>>>     }
>>>>  
>>>> +   /*
>>>> +    * Request APB clocks if device is configured with async clocks mode.
>>>> +    * In this case both tclk and pclk clocks are supposed to be specified.
>>>> +    * Alas we can't know for sure whether async mode was really activated,
>>>> +    * so the pclk reference is left optional. If it it's failed to be
>>>> +    * found we consider the device configured in synchronous clocks mode.
>>>> +    */
>>>> +   dw_wdt->pclk = devm_clk_get_optional(dev, "pclk");
>>>> +   if (IS_ERR(dw_wdt->pclk)) {
>>>> +           ret = PTR_ERR(dw_wdt->pclk);
>>>> +           goto out_disable_clk;
>>>> +   }
>>>> +
>>>> +   ret = clk_prepare_enable(dw_wdt->pclk);
>>>
>>> Not every implementation of clk_enable() checks for a NULL parameter.
>>> Some return an error. This can not be trusted to work on all platforms /
>>> architectures.
>>
>> Hm, this was unexpected twist. I've submitted not a single patch with optional
>> clock API usage. It was first time I've got a comment like this, that the
>> API isn't cross-platform. As I see it this isn't the patch problem, but the
>> platforms/common clock bug. The platforms code must have been submitted before
>> the optional clock API was introduced or the API hasn't been properly
>> implemented or we don't understand something.
>>
>> Stephen, Michael could you clarify the situation with the
>> cross-platformness of the optional clock API.
>>
> 
> NULL is a valid clk to return from clk_get(). And the documentation of
> clk_enable() says that "If the clock can not be enabled/disabled, this
> should return success". Given that a NULL pointer can't do much of
> anything I think any platform that returns an error in this situation is
> deviating from the documentation of the clk API.
> 

This is not about returning an error; some platforms don't check for NULL.

> Does any platform that uses this driver use one of these non-common clk
> framework implementations? All of this may not matter if they all use
> the CCF.
> 

Currently the driver is only used on arm and arm64. Maybe those are safe ?

Also, it looks like clk_enable() exists in the non-standard implementations,
but clk_prepare (and thus clk_prepare_enable) only exist in the standard
implementation. With that, maybe it is always safe to call
clk_prepare_enable() with a NULL parameter ?

Thanks,
Guenter

Re: [PATCH 5/7] watchdog: dw_wdt: Support devices with asynch clocks

[Sergey Semin]

On Mon, Apr 13, 2020 at 07:55:42PM -0700, Guenter Roeck wrote:
> On 4/13/20 1:52 PM, Stephen Boyd wrote:
> > Quoting Sergey Semin (2020-04-10 11:59:34)
> >> Michael, Stephen, could you take a look at the issue we've got here?
> >>
> >> Guenter, my comment is below.
> >>
> >> On Sun, Mar 15, 2020 at 07:22:07AM -0700, Guenter Roeck wrote:
> >>> On Fri, Mar 06, 2020 at 04:27:45PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> >>>> @@ -358,10 +375,27 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >>>>             goto out_disable_clk;
> >>>>     }
> >>>>  
> >>>> +   /*
> >>>> +    * Request APB clocks if device is configured with async clocks mode.
> >>>> +    * In this case both tclk and pclk clocks are supposed to be specified.
> >>>> +    * Alas we can't know for sure whether async mode was really activated,
> >>>> +    * so the pclk reference is left optional. If it it's failed to be
> >>>> +    * found we consider the device configured in synchronous clocks mode.
> >>>> +    */
> >>>> +   dw_wdt->pclk = devm_clk_get_optional(dev, "pclk");
> >>>> +   if (IS_ERR(dw_wdt->pclk)) {
> >>>> +           ret = PTR_ERR(dw_wdt->pclk);
> >>>> +           goto out_disable_clk;
> >>>> +   }
> >>>> +
> >>>> +   ret = clk_prepare_enable(dw_wdt->pclk);
> >>>
> >>> Not every implementation of clk_enable() checks for a NULL parameter.
> >>> Some return an error. This can not be trusted to work on all platforms /
> >>> architectures.
> >>
> >> Hm, this was unexpected twist. I've submitted not a single patch with optional
> >> clock API usage. It was first time I've got a comment like this, that the
> >> API isn't cross-platform. As I see it this isn't the patch problem, but the
> >> platforms/common clock bug. The platforms code must have been submitted before
> >> the optional clock API was introduced or the API hasn't been properly
> >> implemented or we don't understand something.
> >>
> >> Stephen, Michael could you clarify the situation with the
> >> cross-platformness of the optional clock API.
> >>
> > 
> > NULL is a valid clk to return from clk_get(). And the documentation of
> > clk_enable() says that "If the clock can not be enabled/disabled, this
> > should return success". Given that a NULL pointer can't do much of
> > anything I think any platform that returns an error in this situation is
> > deviating from the documentation of the clk API.
> > 

Stephen, thanks for clarification. AFAICS regarding ARM the next three
platforms don't follow that rule:
- arch/arm/mach-ep93xx: No CCF support. clk_enable() returns -EINVAL if NULL
  clk parameter specified.
- arch/arm/mach-mmp: If no CCF enabled, then clk_enable() doesn't check
  the parameter for NULLness.
- arch/arm/mach-omap1: No CCF support. clk_enable() returns -EINVAL if
  NULL clk parameter specified.

Though these platforms statically instantiate the platform devices
except mmp, which may use dtb on some systems. Who knows how the drivers
are using the clocks after the instantiation. Maybe they don't.

> 
> This is not about returning an error; some platforms don't check for NULL.

Please, see the comment above. For ARM it's just three platforms. Though
as Stephen said returning error is wrong in this case too.

> 
> > Does any platform that uses this driver use one of these non-common clk
> > framework implementations? All of this may not matter if they all use
> > the CCF.
> > 
> 
> Currently the driver is only used on arm and arm64. Maybe those are safe ?

AFAICS these are only platforms using snps,dw-wdt at the moment:
	ARM64: Synaptics Berkin 4CT
	ARM64: Intel/Altera SOCFPGAs 
	ARM64: Rockchip PX30/RK33xx
	ARM: Altera SOCFPGA Arria10
	ARM: Marvell Berlin SoCs
	ARM: Rockhip RK3xxx
	ARM: Realview RV1108

All of them rely on CCF. In addition to them there is going to be
Baikal-T1 SoC based on MIPS P5600 CPU core, which clocks also require
CCF. So yes, we are safe to use the optional clocks API at the moment.

> 
> Also, it looks like clk_enable() exists in the non-standard implementations,
> but clk_prepare (and thus clk_prepare_enable) only exist in the standard
> implementation. With that, maybe it is always safe to call
> clk_prepare_enable() with a NULL parameter ?

No. clk_prepare_enable() calls both clk_prepare() and clk_enable()
sequentially. So if some of them don't expect NULL pointer passed, then
the system will blow up. Though this isn't problem for snps,dw-wdt
driver, since all the platforms using it rely on CCF, which has optional
clocks support.

If in future we have a platform, which don't use CCF and don't support
NULL-value of the clk parameter, then this will be the platform problem,
since it doesn't implement the clock API correctly.

Regards,
-Sergey

> 
> Thanks,
> Guenter

[PATCH 6/7] watchdog: dw_wdt: Add pre-timeouts support

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

DW Watchdog can rise an interrupt in case if IRQ request mode
is enabled and timer reaches the zero value. In this case the IRQ
lane is left pending until either the next watchdog kick event
(watchdog restart) or until the WDT_EOI register is read or
the device/system reset. This interface can be used to implement
the pre-timeout functionality optionally provided by the Linux kernel
watchdog devices.

IRQ mode provides a two stages timeout interface. It means the IRQ is
raised when the counter reaches zero, while the system reset occurs
only after subsequent timeout if the timer restart is not performed.
Due to this peculiarity the pre-timeout value is actually set to the
achieved hardware timeout, while the real watchdog timeout is
considered to be twice as much of it. This applies a significant
limitation on the pre-timeout values, so current implementation
supports either zero value, which disables the pre-timeout events, or
non-zero values, which imply the pre-timeout to be at least half
of the current watchdog timeout.

Note that we ask the interrupt controller to detect the rising-edge
pre-timeout interrupts to prevent the high-level-IRQs flood, since
if the pre-timeout happens, the IRQ lane will be left pending until
it's cleared by the timer restart.

Seeing all currently supported platforms, which have the DW Watchdog
installed, provide the interrupt property in the corresponding watchdog
dts node, we can define the IRQ to be mandatory.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 drivers/watchdog/dw_wdt.c | 125 +++++++++++++++++++++++++++++++++++---
 1 file changed, 117 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index eb909c63a1b5..3000120f7e39 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -21,6 +21,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/interrupt.h>
 #include <linux/of.h>
 #include <linux/pm.h>
 #include <linux/platform_device.h>
@@ -35,6 +36,8 @@
 #define WDOG_CURRENT_COUNT_REG_OFFSET	    0x08
 #define WDOG_COUNTER_RESTART_REG_OFFSET     0x0c
 #define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
+#define WDOG_INTERRUPT_STATUS_REG_OFFSET    0x10
+#define WDOG_INTERRUPT_CLEAR_REG_OFFSET     0x14
 #define WDOG_COMP_PARAMS_1_REG_OFFSET       0xf4
 #define WDOG_COMP_PARAMS_1_USE_FIX_TOP      BIT(6)
 
@@ -58,11 +61,17 @@ module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 		 "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
+enum dw_wdt_rmod {
+	DW_WDT_RMOD_RESET = 1,
+	DW_WDT_RMOD_IRQ = 2
+};
+
 struct dw_wdt {
 	void __iomem		*regs;
 	struct clk		*clk;
 	struct clk		*pclk;
 	unsigned long		rate;
+	enum dw_wdt_rmod	rmod;
 	unsigned int		max_top;
 	unsigned int		timeouts[DW_WDT_NUM_TOPS];
 	struct watchdog_device	wdd;
@@ -80,6 +89,20 @@ static inline int dw_wdt_is_enabled(struct dw_wdt *dw_wdt)
 		WDOG_CONTROL_REG_WDT_EN_MASK;
 }
 
+static void dw_wdt_update_mode(struct dw_wdt *dw_wdt, enum dw_wdt_rmod rmod)
+{
+	u32 val;
+
+	val = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
+	if (rmod == DW_WDT_RMOD_IRQ)
+		val |= WDOG_CONTROL_REG_RESP_MODE_MASK;
+	else
+		val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
+	writel(val, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
+
+	dw_wdt->rmod = rmod;
+}
+
 static unsigned int dw_wdt_find_best_top(struct dw_wdt *dw_wdt,
 					 unsigned int timeout, u32 *top)
 {
@@ -141,7 +164,11 @@ static unsigned int dw_wdt_get_timeout(struct dw_wdt *dw_wdt)
 {
 	int top = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
 
-	return dw_wdt->timeouts[top];
+	/*
+	 * In IRQ mode due to the two stages counter, the actual timeout is
+	 * twice greater than the TOP setting.
+	 */
+	return (dw_wdt->timeouts[top] * dw_wdt->rmod);
 }
 
 static int dw_wdt_ping(struct watchdog_device *wdd)
@@ -160,7 +187,21 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
 	unsigned int timeout;
 	u32 top;
 
-	timeout = dw_wdt_find_best_top(dw_wdt, req * MSEC_PER_SEC, &top);
+	/*
+	 * We try to find a timeout achievable by the device or set the maximum
+	 * one. Note IRQ mode being enabled means having a non-zero pre-timeout
+	 * setup. In this case we try to find a TOP as close to the half of the
+	 * requested timeout as possible since DW Watchdog IRQ mode is designed
+	 * in two stages way - first timeout rises the pre-timeout interrupt,
+	 * second timeout performs the system reset.
+	 */
+	timeout = dw_wdt_find_best_top(dw_wdt,
+		req * (MSEC_PER_SEC / dw_wdt->rmod), &top);
+	timeout = (timeout * dw_wdt->rmod) / MSEC_PER_SEC;
+	if (dw_wdt->rmod == DW_WDT_RMOD_IRQ)
+		wdd->pretimeout = timeout / dw_wdt->rmod;
+	else
+		wdd->pretimeout = 0;
 
 	/*
 	 * Set the new value in the watchdog.  Some versions of dw_wdt
@@ -171,6 +212,10 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
 	writel(top | top << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
 	       dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
 
+	/* Kick new TOP value into the watchdog counter if activated. */
+	if (watchdog_active(wdd))
+		dw_wdt_ping(wdd);
+
 	/*
 	 * In case users set bigger timeout value than HW can support,
 	 * kernel(watchdog_dev.c) helps to feed watchdog before
@@ -179,7 +224,22 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
 	if (req * MSEC_PER_SEC > wdd->max_hw_heartbeat_ms)
 		wdd->timeout = req;
 	else
-		wdd->timeout = timeout / MSEC_PER_SEC;
+		wdd->timeout = timeout;
+
+	return 0;
+}
+
+static int dw_wdt_set_pretimeout(struct watchdog_device *wdd, unsigned int req)
+{
+	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
+
+	/*
+	 * We ignore actual value of the timeout passed from user-space
+	 * using it as a flag whether the pretimeout functionality is intended
+	 * to be activated.
+	 */
+	dw_wdt_update_mode(dw_wdt, req ? DW_WDT_RMOD_IRQ : DW_WDT_RMOD_RESET);
+	dw_wdt_set_timeout(wdd, wdd->timeout);
 
 	return 0;
 }
@@ -188,8 +248,11 @@ static void dw_wdt_arm_system_reset(struct dw_wdt *dw_wdt)
 {
 	u32 val = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
 
-	/* Disable interrupt mode; always perform system reset. */
-	val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
+	/* Disable/enable interrupt mode depending on the RMOD flag. */
+	if (dw_wdt->rmod == DW_WDT_RMOD_IRQ)
+		val |= WDOG_CONTROL_REG_RESP_MODE_MASK;
+	else
+		val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
 	/* Enable watchdog. */
 	val |= WDOG_CONTROL_REG_WDT_EN_MASK;
 	writel(val, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
@@ -227,6 +290,7 @@ static int dw_wdt_restart(struct watchdog_device *wdd,
 	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
 
 	writel(0, dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
+	dw_wdt_update_mode(dw_wdt, DW_WDT_RMOD_RESET);
 	if (dw_wdt_is_enabled(dw_wdt))
 		writel(WDOG_COUNTER_RESTART_KICK_VALUE,
 		       dw_wdt->regs + WDOG_COUNTER_RESTART_REG_OFFSET);
@@ -242,14 +306,24 @@ static int dw_wdt_restart(struct watchdog_device *wdd,
 static unsigned int dw_wdt_get_timeleft(struct watchdog_device *wdd)
 {
 	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
+	unsigned int time;
+	u32 val;
+
+	val = readl(dw_wdt->regs + WDOG_CURRENT_COUNT_REG_OFFSET);
+	time = val / dw_wdt->rate;
+
+	if (dw_wdt->rmod == DW_WDT_RMOD_IRQ) {
+		val = readl(dw_wdt->regs + WDOG_INTERRUPT_STATUS_REG_OFFSET);
+		if (!val)
+			time += wdd->pretimeout;
+	}
 
-	return readl(dw_wdt->regs + WDOG_CURRENT_COUNT_REG_OFFSET) /
-		dw_wdt->rate;
+	return time;
 }
 
 static const struct watchdog_info dw_wdt_ident = {
 	.options	= WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
-			  WDIOF_MAGICCLOSE,
+			  WDIOF_PRETIMEOUT | WDIOF_MAGICCLOSE,
 	.identity	= "Synopsys DesignWare Watchdog",
 };
 
@@ -259,10 +333,29 @@ static const struct watchdog_ops dw_wdt_ops = {
 	.stop		= dw_wdt_stop,
 	.ping		= dw_wdt_ping,
 	.set_timeout	= dw_wdt_set_timeout,
+	.set_pretimeout	= dw_wdt_set_pretimeout,
 	.get_timeleft	= dw_wdt_get_timeleft,
 	.restart	= dw_wdt_restart,
 };
 
+static irqreturn_t dw_wdt_irq(int irq, void *devid)
+{
+	struct dw_wdt *dw_wdt = devid;
+	u32 val;
+
+	/*
+	 * We don't clear the IRQ status. It's supposed to be done by following
+	 * ping operations.
+	 */
+	val = readl(dw_wdt->regs + WDOG_INTERRUPT_STATUS_REG_OFFSET);
+	if (!val)
+		return IRQ_NONE;
+
+	watchdog_notify_pretimeout(&dw_wdt->wdd);
+
+	return IRQ_HANDLED;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int dw_wdt_suspend(struct device *dev)
 {
@@ -398,10 +491,26 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 		goto out_disable_pclk;
 	}
 
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0)
+		goto out_disable_pclk;
+
+	/*
+	 * We must request rising-edge IRQ, since the lane is left pending
+	 * either until the next watchdog kick event or up to the system reset.
+	 */
+	ret = devm_request_irq(dev, ret, dw_wdt_irq,
+			       IRQF_SHARED | IRQF_TRIGGER_RISING,
+			       pdev->name, dw_wdt);
+	if (ret)
+		goto out_disable_pclk;
+
 	reset_control_deassert(dw_wdt->rst);
 
 	dw_wdt_init_timeouts(dw_wdt, dev);
 
+	dw_wdt_update_mode(dw_wdt, DW_WDT_RMOD_RESET);
+
 	wdd = &dw_wdt->wdd;
 	wdd->info = &dw_wdt_ident;
 	wdd->ops = &dw_wdt_ops;
-- 
2.25.1

Re: [PATCH 6/7] watchdog: dw_wdt: Add pre-timeouts support

[Guenter Roeck]

On 3/6/20 5:27 AM, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> DW Watchdog can rise an interrupt in case if IRQ request mode
> is enabled and timer reaches the zero value. In this case the IRQ
> lane is left pending until either the next watchdog kick event
> (watchdog restart) or until the WDT_EOI register is read or
> the device/system reset. This interface can be used to implement
> the pre-timeout functionality optionally provided by the Linux kernel
> watchdog devices.
> 
> IRQ mode provides a two stages timeout interface. It means the IRQ is
> raised when the counter reaches zero, while the system reset occurs
> only after subsequent timeout if the timer restart is not performed.
> Due to this peculiarity the pre-timeout value is actually set to the
> achieved hardware timeout, while the real watchdog timeout is
> considered to be twice as much of it. This applies a significant
> limitation on the pre-timeout values, so current implementation
> supports either zero value, which disables the pre-timeout events, or
> non-zero values, which imply the pre-timeout to be at least half
> of the current watchdog timeout.
> 
> Note that we ask the interrupt controller to detect the rising-edge
> pre-timeout interrupts to prevent the high-level-IRQs flood, since
> if the pre-timeout happens, the IRQ lane will be left pending until
> it's cleared by the timer restart.
> 
> Seeing all currently supported platforms, which have the DW Watchdog
> installed, provide the interrupt property in the corresponding watchdog
> dts node, we can define the IRQ to be mandatory.

I don't see this as valid argument. It is only needed if one wants to have
pretimeout support, and if one doesn't it is simply not necessary.
"Everyone uses it" is not an argument - someone else might come tomorrow
and not want to use it.

Guenter

> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---
>  drivers/watchdog/dw_wdt.c | 125 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 117 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index eb909c63a1b5..3000120f7e39 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -21,6 +21,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> +#include <linux/interrupt.h>
>  #include <linux/of.h>
>  #include <linux/pm.h>
>  #include <linux/platform_device.h>
> @@ -35,6 +36,8 @@
>  #define WDOG_CURRENT_COUNT_REG_OFFSET	    0x08
>  #define WDOG_COUNTER_RESTART_REG_OFFSET     0x0c
>  #define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
> +#define WDOG_INTERRUPT_STATUS_REG_OFFSET    0x10
> +#define WDOG_INTERRUPT_CLEAR_REG_OFFSET     0x14
>  #define WDOG_COMP_PARAMS_1_REG_OFFSET       0xf4
>  #define WDOG_COMP_PARAMS_1_USE_FIX_TOP      BIT(6)
>  
> @@ -58,11 +61,17 @@ module_param(nowayout, bool, 0);
>  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
>  		 "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>  
> +enum dw_wdt_rmod {
> +	DW_WDT_RMOD_RESET = 1,
> +	DW_WDT_RMOD_IRQ = 2
> +};
> +
>  struct dw_wdt {
>  	void __iomem		*regs;
>  	struct clk		*clk;
>  	struct clk		*pclk;
>  	unsigned long		rate;
> +	enum dw_wdt_rmod	rmod;
>  	unsigned int		max_top;
>  	unsigned int		timeouts[DW_WDT_NUM_TOPS];
>  	struct watchdog_device	wdd;
> @@ -80,6 +89,20 @@ static inline int dw_wdt_is_enabled(struct dw_wdt *dw_wdt)
>  		WDOG_CONTROL_REG_WDT_EN_MASK;
>  }
>  
> +static void dw_wdt_update_mode(struct dw_wdt *dw_wdt, enum dw_wdt_rmod rmod)
> +{
> +	u32 val;
> +
> +	val = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> +	if (rmod == DW_WDT_RMOD_IRQ)
> +		val |= WDOG_CONTROL_REG_RESP_MODE_MASK;
> +	else
> +		val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
> +	writel(val, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> +
> +	dw_wdt->rmod = rmod;
> +}
> +
>  static unsigned int dw_wdt_find_best_top(struct dw_wdt *dw_wdt,
>  					 unsigned int timeout, u32 *top)
>  {
> @@ -141,7 +164,11 @@ static unsigned int dw_wdt_get_timeout(struct dw_wdt *dw_wdt)
>  {
>  	int top = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
>  
> -	return dw_wdt->timeouts[top];
> +	/*
> +	 * In IRQ mode due to the two stages counter, the actual timeout is
> +	 * twice greater than the TOP setting.
> +	 */
> +	return (dw_wdt->timeouts[top] * dw_wdt->rmod);
>  }
>  
>  static int dw_wdt_ping(struct watchdog_device *wdd)
> @@ -160,7 +187,21 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
>  	unsigned int timeout;
>  	u32 top;
>  
> -	timeout = dw_wdt_find_best_top(dw_wdt, req * MSEC_PER_SEC, &top);
> +	/*
> +	 * We try to find a timeout achievable by the device or set the maximum
> +	 * one. Note IRQ mode being enabled means having a non-zero pre-timeout
> +	 * setup. In this case we try to find a TOP as close to the half of the
> +	 * requested timeout as possible since DW Watchdog IRQ mode is designed
> +	 * in two stages way - first timeout rises the pre-timeout interrupt,
> +	 * second timeout performs the system reset.
> +	 */
> +	timeout = dw_wdt_find_best_top(dw_wdt,
> +		req * (MSEC_PER_SEC / dw_wdt->rmod), &top);
> +	timeout = (timeout * dw_wdt->rmod) / MSEC_PER_SEC;
> +	if (dw_wdt->rmod == DW_WDT_RMOD_IRQ)
> +		wdd->pretimeout = timeout / dw_wdt->rmod;
> +	else
> +		wdd->pretimeout = 0;
>  
>  	/*
>  	 * Set the new value in the watchdog.  Some versions of dw_wdt
> @@ -171,6 +212,10 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
>  	writel(top | top << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
>  	       dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
>  
> +	/* Kick new TOP value into the watchdog counter if activated. */
> +	if (watchdog_active(wdd))
> +		dw_wdt_ping(wdd);
> +
>  	/*
>  	 * In case users set bigger timeout value than HW can support,
>  	 * kernel(watchdog_dev.c) helps to feed watchdog before
> @@ -179,7 +224,22 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
>  	if (req * MSEC_PER_SEC > wdd->max_hw_heartbeat_ms)
>  		wdd->timeout = req;
>  	else
> -		wdd->timeout = timeout / MSEC_PER_SEC;
> +		wdd->timeout = timeout;
> +
> +	return 0;
> +}
> +
> +static int dw_wdt_set_pretimeout(struct watchdog_device *wdd, unsigned int req)
> +{
> +	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> +
> +	/*
> +	 * We ignore actual value of the timeout passed from user-space
> +	 * using it as a flag whether the pretimeout functionality is intended
> +	 * to be activated.
> +	 */
> +	dw_wdt_update_mode(dw_wdt, req ? DW_WDT_RMOD_IRQ : DW_WDT_RMOD_RESET);
> +	dw_wdt_set_timeout(wdd, wdd->timeout);
>  
>  	return 0;
>  }
> @@ -188,8 +248,11 @@ static void dw_wdt_arm_system_reset(struct dw_wdt *dw_wdt)
>  {
>  	u32 val = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
>  
> -	/* Disable interrupt mode; always perform system reset. */
> -	val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
> +	/* Disable/enable interrupt mode depending on the RMOD flag. */
> +	if (dw_wdt->rmod == DW_WDT_RMOD_IRQ)
> +		val |= WDOG_CONTROL_REG_RESP_MODE_MASK;
> +	else
> +		val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
>  	/* Enable watchdog. */
>  	val |= WDOG_CONTROL_REG_WDT_EN_MASK;
>  	writel(val, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> @@ -227,6 +290,7 @@ static int dw_wdt_restart(struct watchdog_device *wdd,
>  	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
>  
>  	writel(0, dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> +	dw_wdt_update_mode(dw_wdt, DW_WDT_RMOD_RESET);
>  	if (dw_wdt_is_enabled(dw_wdt))
>  		writel(WDOG_COUNTER_RESTART_KICK_VALUE,
>  		       dw_wdt->regs + WDOG_COUNTER_RESTART_REG_OFFSET);
> @@ -242,14 +306,24 @@ static int dw_wdt_restart(struct watchdog_device *wdd,
>  static unsigned int dw_wdt_get_timeleft(struct watchdog_device *wdd)
>  {
>  	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> +	unsigned int time;
> +	u32 val;
> +
> +	val = readl(dw_wdt->regs + WDOG_CURRENT_COUNT_REG_OFFSET);
> +	time = val / dw_wdt->rate;
> +
> +	if (dw_wdt->rmod == DW_WDT_RMOD_IRQ) {
> +		val = readl(dw_wdt->regs + WDOG_INTERRUPT_STATUS_REG_OFFSET);
> +		if (!val)
> +			time += wdd->pretimeout;
> +	}
>  
> -	return readl(dw_wdt->regs + WDOG_CURRENT_COUNT_REG_OFFSET) /
> -		dw_wdt->rate;
> +	return time;
>  }
>  
>  static const struct watchdog_info dw_wdt_ident = {
>  	.options	= WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
> -			  WDIOF_MAGICCLOSE,
> +			  WDIOF_PRETIMEOUT | WDIOF_MAGICCLOSE,
>  	.identity	= "Synopsys DesignWare Watchdog",
>  };
>  
> @@ -259,10 +333,29 @@ static const struct watchdog_ops dw_wdt_ops = {
>  	.stop		= dw_wdt_stop,
>  	.ping		= dw_wdt_ping,
>  	.set_timeout	= dw_wdt_set_timeout,
> +	.set_pretimeout	= dw_wdt_set_pretimeout,
>  	.get_timeleft	= dw_wdt_get_timeleft,
>  	.restart	= dw_wdt_restart,
>  };
>  
> +static irqreturn_t dw_wdt_irq(int irq, void *devid)
> +{
> +	struct dw_wdt *dw_wdt = devid;
> +	u32 val;
> +
> +	/*
> +	 * We don't clear the IRQ status. It's supposed to be done by following
> +	 * ping operations.
> +	 */
> +	val = readl(dw_wdt->regs + WDOG_INTERRUPT_STATUS_REG_OFFSET);
> +	if (!val)
> +		return IRQ_NONE;
> +
> +	watchdog_notify_pretimeout(&dw_wdt->wdd);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  #ifdef CONFIG_PM_SLEEP
>  static int dw_wdt_suspend(struct device *dev)
>  {
> @@ -398,10 +491,26 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>  		goto out_disable_pclk;
>  	}
>  
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0)
> +		goto out_disable_pclk;
> +
> +	/*
> +	 * We must request rising-edge IRQ, since the lane is left pending
> +	 * either until the next watchdog kick event or up to the system reset.
> +	 */
> +	ret = devm_request_irq(dev, ret, dw_wdt_irq,
> +			       IRQF_SHARED | IRQF_TRIGGER_RISING,
> +			       pdev->name, dw_wdt);
> +	if (ret)
> +		goto out_disable_pclk;
> +
>  	reset_control_deassert(dw_wdt->rst);
>  
>  	dw_wdt_init_timeouts(dw_wdt, dev);
>  
> +	dw_wdt_update_mode(dw_wdt, DW_WDT_RMOD_RESET);
> +
>  	wdd = &dw_wdt->wdd;
>  	wdd->info = &dw_wdt_ident;
>  	wdd->ops = &dw_wdt_ops;
> 

Re: [PATCH 6/7] watchdog: dw_wdt: Add pre-timeouts support

[Sergey Semin]

On Fri, Mar 06, 2020 at 07:14:50AM -0800, Guenter Roeck wrote:
> On 3/6/20 5:27 AM, Sergey.Semin@baikalelectronics.ru wrote:
> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > DW Watchdog can rise an interrupt in case if IRQ request mode
> > is enabled and timer reaches the zero value. In this case the IRQ
> > lane is left pending until either the next watchdog kick event
> > (watchdog restart) or until the WDT_EOI register is read or
> > the device/system reset. This interface can be used to implement
> > the pre-timeout functionality optionally provided by the Linux kernel
> > watchdog devices.
> > 
> > IRQ mode provides a two stages timeout interface. It means the IRQ is
> > raised when the counter reaches zero, while the system reset occurs
> > only after subsequent timeout if the timer restart is not performed.
> > Due to this peculiarity the pre-timeout value is actually set to the
> > achieved hardware timeout, while the real watchdog timeout is
> > considered to be twice as much of it. This applies a significant
> > limitation on the pre-timeout values, so current implementation
> > supports either zero value, which disables the pre-timeout events, or
> > non-zero values, which imply the pre-timeout to be at least half
> > of the current watchdog timeout.
> > 
> > Note that we ask the interrupt controller to detect the rising-edge
> > pre-timeout interrupts to prevent the high-level-IRQs flood, since
> > if the pre-timeout happens, the IRQ lane will be left pending until
> > it's cleared by the timer restart.
> > 
> > Seeing all currently supported platforms, which have the DW Watchdog
> > installed, provide the interrupt property in the corresponding watchdog
> > dts node, we can define the IRQ to be mandatory.
> 
> I don't see this as valid argument. It is only needed if one wants to have
> pretimeout support, and if one doesn't it is simply not necessary.
> "Everyone uses it" is not an argument - someone else might come tomorrow
> and not want to use it.

Ok. I'll make it optional.

-Sergey

> 
> Guenter
> 
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > ---
> >  drivers/watchdog/dw_wdt.c | 125 +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 117 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> > index eb909c63a1b5..3000120f7e39 100644
> > --- a/drivers/watchdog/dw_wdt.c
> > +++ b/drivers/watchdog/dw_wdt.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/moduleparam.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/of.h>
> >  #include <linux/pm.h>
> >  #include <linux/platform_device.h>
> > @@ -35,6 +36,8 @@
> >  #define WDOG_CURRENT_COUNT_REG_OFFSET	    0x08
> >  #define WDOG_COUNTER_RESTART_REG_OFFSET     0x0c
> >  #define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
> > +#define WDOG_INTERRUPT_STATUS_REG_OFFSET    0x10
> > +#define WDOG_INTERRUPT_CLEAR_REG_OFFSET     0x14
> >  #define WDOG_COMP_PARAMS_1_REG_OFFSET       0xf4
> >  #define WDOG_COMP_PARAMS_1_USE_FIX_TOP      BIT(6)
> >  
> > @@ -58,11 +61,17 @@ module_param(nowayout, bool, 0);
> >  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> >  		 "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> >  
> > +enum dw_wdt_rmod {
> > +	DW_WDT_RMOD_RESET = 1,
> > +	DW_WDT_RMOD_IRQ = 2
> > +};
> > +
> >  struct dw_wdt {
> >  	void __iomem		*regs;
> >  	struct clk		*clk;
> >  	struct clk		*pclk;
> >  	unsigned long		rate;
> > +	enum dw_wdt_rmod	rmod;
> >  	unsigned int		max_top;
> >  	unsigned int		timeouts[DW_WDT_NUM_TOPS];
> >  	struct watchdog_device	wdd;
> > @@ -80,6 +89,20 @@ static inline int dw_wdt_is_enabled(struct dw_wdt *dw_wdt)
> >  		WDOG_CONTROL_REG_WDT_EN_MASK;
> >  }
> >  
> > +static void dw_wdt_update_mode(struct dw_wdt *dw_wdt, enum dw_wdt_rmod rmod)
> > +{
> > +	u32 val;
> > +
> > +	val = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> > +	if (rmod == DW_WDT_RMOD_IRQ)
> > +		val |= WDOG_CONTROL_REG_RESP_MODE_MASK;
> > +	else
> > +		val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
> > +	writel(val, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> > +
> > +	dw_wdt->rmod = rmod;
> > +}
> > +
> >  static unsigned int dw_wdt_find_best_top(struct dw_wdt *dw_wdt,
> >  					 unsigned int timeout, u32 *top)
> >  {
> > @@ -141,7 +164,11 @@ static unsigned int dw_wdt_get_timeout(struct dw_wdt *dw_wdt)
> >  {
> >  	int top = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
> >  
> > -	return dw_wdt->timeouts[top];
> > +	/*
> > +	 * In IRQ mode due to the two stages counter, the actual timeout is
> > +	 * twice greater than the TOP setting.
> > +	 */
> > +	return (dw_wdt->timeouts[top] * dw_wdt->rmod);
> >  }
> >  
> >  static int dw_wdt_ping(struct watchdog_device *wdd)
> > @@ -160,7 +187,21 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
> >  	unsigned int timeout;
> >  	u32 top;
> >  
> > -	timeout = dw_wdt_find_best_top(dw_wdt, req * MSEC_PER_SEC, &top);
> > +	/*
> > +	 * We try to find a timeout achievable by the device or set the maximum
> > +	 * one. Note IRQ mode being enabled means having a non-zero pre-timeout
> > +	 * setup. In this case we try to find a TOP as close to the half of the
> > +	 * requested timeout as possible since DW Watchdog IRQ mode is designed
> > +	 * in two stages way - first timeout rises the pre-timeout interrupt,
> > +	 * second timeout performs the system reset.
> > +	 */
> > +	timeout = dw_wdt_find_best_top(dw_wdt,
> > +		req * (MSEC_PER_SEC / dw_wdt->rmod), &top);
> > +	timeout = (timeout * dw_wdt->rmod) / MSEC_PER_SEC;
> > +	if (dw_wdt->rmod == DW_WDT_RMOD_IRQ)
> > +		wdd->pretimeout = timeout / dw_wdt->rmod;
> > +	else
> > +		wdd->pretimeout = 0;
> >  
> >  	/*
> >  	 * Set the new value in the watchdog.  Some versions of dw_wdt
> > @@ -171,6 +212,10 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
> >  	writel(top | top << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
> >  	       dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> >  
> > +	/* Kick new TOP value into the watchdog counter if activated. */
> > +	if (watchdog_active(wdd))
> > +		dw_wdt_ping(wdd);
> > +
> >  	/*
> >  	 * In case users set bigger timeout value than HW can support,
> >  	 * kernel(watchdog_dev.c) helps to feed watchdog before
> > @@ -179,7 +224,22 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
> >  	if (req * MSEC_PER_SEC > wdd->max_hw_heartbeat_ms)
> >  		wdd->timeout = req;
> >  	else
> > -		wdd->timeout = timeout / MSEC_PER_SEC;
> > +		wdd->timeout = timeout;
> > +
> > +	return 0;
> > +}
> > +
> > +static int dw_wdt_set_pretimeout(struct watchdog_device *wdd, unsigned int req)
> > +{
> > +	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> > +
> > +	/*
> > +	 * We ignore actual value of the timeout passed from user-space
> > +	 * using it as a flag whether the pretimeout functionality is intended
> > +	 * to be activated.
> > +	 */
> > +	dw_wdt_update_mode(dw_wdt, req ? DW_WDT_RMOD_IRQ : DW_WDT_RMOD_RESET);
> > +	dw_wdt_set_timeout(wdd, wdd->timeout);
> >  
> >  	return 0;
> >  }
> > @@ -188,8 +248,11 @@ static void dw_wdt_arm_system_reset(struct dw_wdt *dw_wdt)
> >  {
> >  	u32 val = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> >  
> > -	/* Disable interrupt mode; always perform system reset. */
> > -	val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
> > +	/* Disable/enable interrupt mode depending on the RMOD flag. */
> > +	if (dw_wdt->rmod == DW_WDT_RMOD_IRQ)
> > +		val |= WDOG_CONTROL_REG_RESP_MODE_MASK;
> > +	else
> > +		val &= ~WDOG_CONTROL_REG_RESP_MODE_MASK;
> >  	/* Enable watchdog. */
> >  	val |= WDOG_CONTROL_REG_WDT_EN_MASK;
> >  	writel(val, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
> > @@ -227,6 +290,7 @@ static int dw_wdt_restart(struct watchdog_device *wdd,
> >  	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> >  
> >  	writel(0, dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> > +	dw_wdt_update_mode(dw_wdt, DW_WDT_RMOD_RESET);
> >  	if (dw_wdt_is_enabled(dw_wdt))
> >  		writel(WDOG_COUNTER_RESTART_KICK_VALUE,
> >  		       dw_wdt->regs + WDOG_COUNTER_RESTART_REG_OFFSET);
> > @@ -242,14 +306,24 @@ static int dw_wdt_restart(struct watchdog_device *wdd,
> >  static unsigned int dw_wdt_get_timeleft(struct watchdog_device *wdd)
> >  {
> >  	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> > +	unsigned int time;
> > +	u32 val;
> > +
> > +	val = readl(dw_wdt->regs + WDOG_CURRENT_COUNT_REG_OFFSET);
> > +	time = val / dw_wdt->rate;
> > +
> > +	if (dw_wdt->rmod == DW_WDT_RMOD_IRQ) {
> > +		val = readl(dw_wdt->regs + WDOG_INTERRUPT_STATUS_REG_OFFSET);
> > +		if (!val)
> > +			time += wdd->pretimeout;
> > +	}
> >  
> > -	return readl(dw_wdt->regs + WDOG_CURRENT_COUNT_REG_OFFSET) /
> > -		dw_wdt->rate;
> > +	return time;
> >  }
> >  
> >  static const struct watchdog_info dw_wdt_ident = {
> >  	.options	= WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
> > -			  WDIOF_MAGICCLOSE,
> > +			  WDIOF_PRETIMEOUT | WDIOF_MAGICCLOSE,
> >  	.identity	= "Synopsys DesignWare Watchdog",
> >  };
> >  
> > @@ -259,10 +333,29 @@ static const struct watchdog_ops dw_wdt_ops = {
> >  	.stop		= dw_wdt_stop,
> >  	.ping		= dw_wdt_ping,
> >  	.set_timeout	= dw_wdt_set_timeout,
> > +	.set_pretimeout	= dw_wdt_set_pretimeout,
> >  	.get_timeleft	= dw_wdt_get_timeleft,
> >  	.restart	= dw_wdt_restart,
> >  };
> >  
> > +static irqreturn_t dw_wdt_irq(int irq, void *devid)
> > +{
> > +	struct dw_wdt *dw_wdt = devid;
> > +	u32 val;
> > +
> > +	/*
> > +	 * We don't clear the IRQ status. It's supposed to be done by following
> > +	 * ping operations.
> > +	 */
> > +	val = readl(dw_wdt->regs + WDOG_INTERRUPT_STATUS_REG_OFFSET);
> > +	if (!val)
> > +		return IRQ_NONE;
> > +
> > +	watchdog_notify_pretimeout(&dw_wdt->wdd);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  #ifdef CONFIG_PM_SLEEP
> >  static int dw_wdt_suspend(struct device *dev)
> >  {
> > @@ -398,10 +491,26 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >  		goto out_disable_pclk;
> >  	}
> >  
> > +	ret = platform_get_irq(pdev, 0);
> > +	if (ret < 0)
> > +		goto out_disable_pclk;
> > +
> > +	/*
> > +	 * We must request rising-edge IRQ, since the lane is left pending
> > +	 * either until the next watchdog kick event or up to the system reset.
> > +	 */
> > +	ret = devm_request_irq(dev, ret, dw_wdt_irq,
> > +			       IRQF_SHARED | IRQF_TRIGGER_RISING,
> > +			       pdev->name, dw_wdt);
> > +	if (ret)
> > +		goto out_disable_pclk;
> > +
> >  	reset_control_deassert(dw_wdt->rst);
> >  
> >  	dw_wdt_init_timeouts(dw_wdt, dev);
> >  
> > +	dw_wdt_update_mode(dw_wdt, DW_WDT_RMOD_RESET);
> > +
> >  	wdd = &dw_wdt->wdd;
> >  	wdd->info = &dw_wdt_ident;
> >  	wdd->ops = &dw_wdt_ops;
> > 
> 

[PATCH 7/7] watchdog: dw_wdt: Add DebugFS files

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

For the sake of the easier device-driver debug procedure, we added a
few DebugFS files with the next content: watchdog timeout, watchdog
pre-timeout, watchdog ping/kick operation, watchdog start/stop, device
registers dump. They are available only if kernel is configured with
DebugFS support.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 drivers/watchdog/dw_wdt.c | 150 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 150 insertions(+)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index 3000120f7e39..9353d83f3e45 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -27,6 +27,7 @@
 #include <linux/platform_device.h>
 #include <linux/reset.h>
 #include <linux/watchdog.h>
+#include <linux/debugfs.h>
 
 #define WDOG_CONTROL_REG_OFFSET		    0x00
 #define WDOG_CONTROL_REG_WDT_EN_MASK	    0x01
@@ -38,8 +39,14 @@
 #define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
 #define WDOG_INTERRUPT_STATUS_REG_OFFSET    0x10
 #define WDOG_INTERRUPT_CLEAR_REG_OFFSET     0x14
+#define WDOG_COMP_PARAMS_5_REG_OFFSET       0xe4
+#define WDOG_COMP_PARAMS_4_REG_OFFSET       0xe8
+#define WDOG_COMP_PARAMS_3_REG_OFFSET       0xec
+#define WDOG_COMP_PARAMS_2_REG_OFFSET       0xf0
 #define WDOG_COMP_PARAMS_1_REG_OFFSET       0xf4
 #define WDOG_COMP_PARAMS_1_USE_FIX_TOP      BIT(6)
+#define WDOG_COMP_VERSION_REG_OFFSET        0xf8
+#define WDOG_COMP_TYPE_REG_OFFSET           0xfc
 
 /* There are sixteen TOPs (timeout periods) that can be set in the watchdog. */
 #define DW_WDT_NUM_TOPS		16
@@ -79,6 +86,10 @@ struct dw_wdt {
 	/* Save/restore */
 	u32			control;
 	u32			timeout;
+
+#ifdef CONFIG_DEBUG_FS
+	struct dentry		*dbgfs_dir;
+#endif
 };
 
 #define to_dw_wdt(wdd)	container_of(wdd, struct dw_wdt, wdd)
@@ -430,6 +441,141 @@ static void dw_wdt_init_timeouts(struct dw_wdt *dw_wdt, struct device *dev)
 			mult_frac(tops[idx], MSEC_PER_SEC, dw_wdt->rate);
 }
 
+#ifdef CONFIG_DEBUG_FS
+
+static int dw_wdt_dbgfs_timeout_get(void *priv, u64 *val)
+{
+	struct dw_wdt *dw_wdt = priv;
+
+	*val = dw_wdt_get_timeout(dw_wdt) / MSEC_PER_SEC;
+
+	return 0;
+}
+
+static int dw_wdt_dbgfs_timeout_set(void *priv, u64 val)
+{
+	struct dw_wdt *dw_wdt = priv;
+
+	return dw_wdt_set_timeout(&dw_wdt->wdd, val);
+}
+DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_timeout_fops,
+	dw_wdt_dbgfs_timeout_get, dw_wdt_dbgfs_timeout_set, "%llu\n");
+
+static int dw_wdt_dbgfs_pretimeout_get(void *priv, u64 *val)
+{
+	struct dw_wdt *dw_wdt = priv;
+
+	*val = dw_wdt->wdd.pretimeout;
+
+	return 0;
+}
+
+static int dw_wdt_dbgfs_pretimeout_set(void *priv, u64 val)
+{
+	struct dw_wdt *dw_wdt = priv;
+
+	return dw_wdt_set_pretimeout(&dw_wdt->wdd, val);
+}
+DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_pretimeout_fops,
+	dw_wdt_dbgfs_pretimeout_get, dw_wdt_dbgfs_pretimeout_set, "%llu\n");
+
+static int dw_wdt_dbgfs_ping_set(void *priv, u64 val)
+{
+	struct dw_wdt *dw_wdt = priv;
+
+	dw_wdt_ping(&dw_wdt->wdd);
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_ping_fops,
+	NULL, dw_wdt_dbgfs_ping_set, "%llu\n");
+
+static int dw_wdt_dbgfs_en_get(void *priv, u64 *val)
+{
+	struct dw_wdt *dw_wdt = priv;
+
+	*val = !!dw_wdt_is_enabled(dw_wdt);
+
+	return 0;
+}
+
+static int dw_wdt_dbgfs_en_set(void *priv, u64 val)
+{
+	struct dw_wdt *dw_wdt = priv;
+
+	if (val)
+		dw_wdt_start(&dw_wdt->wdd);
+	else
+		dw_wdt_stop(&dw_wdt->wdd);
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_en_fops,
+	dw_wdt_dbgfs_en_get, dw_wdt_dbgfs_en_set, "%llu\n");
+
+#define DW_WDT_DBGFS_REG(_name, _off) \
+{				      \
+	.name = _name,		      \
+	.offset = _off		      \
+}
+
+static const struct debugfs_reg32 dw_wdt_dbgfs_regs[] = {
+	DW_WDT_DBGFS_REG("cr", WDOG_CONTROL_REG_OFFSET),
+	DW_WDT_DBGFS_REG("torr", WDOG_TIMEOUT_RANGE_REG_OFFSET),
+	DW_WDT_DBGFS_REG("ccvr", WDOG_CURRENT_COUNT_REG_OFFSET),
+	DW_WDT_DBGFS_REG("crr", WDOG_COUNTER_RESTART_REG_OFFSET),
+	DW_WDT_DBGFS_REG("stat", WDOG_INTERRUPT_STATUS_REG_OFFSET),
+	DW_WDT_DBGFS_REG("param5", WDOG_COMP_PARAMS_5_REG_OFFSET),
+	DW_WDT_DBGFS_REG("param4", WDOG_COMP_PARAMS_4_REG_OFFSET),
+	DW_WDT_DBGFS_REG("param3", WDOG_COMP_PARAMS_3_REG_OFFSET),
+	DW_WDT_DBGFS_REG("param2", WDOG_COMP_PARAMS_2_REG_OFFSET),
+	DW_WDT_DBGFS_REG("param1", WDOG_COMP_PARAMS_1_REG_OFFSET),
+	DW_WDT_DBGFS_REG("version", WDOG_COMP_VERSION_REG_OFFSET),
+	DW_WDT_DBGFS_REG("type", WDOG_COMP_TYPE_REG_OFFSET)
+};
+
+static void dw_wdt_dbgfs_init(struct dw_wdt *dw_wdt)
+{
+	struct device *dev = dw_wdt->wdd.parent;
+	struct debugfs_regset32 *regset;
+
+	regset = devm_kzalloc(dev, sizeof(*regset), GFP_KERNEL);
+	if (!regset)
+		return;
+
+	regset->regs = dw_wdt_dbgfs_regs;
+	regset->nregs = ARRAY_SIZE(dw_wdt_dbgfs_regs);
+	regset->base = dw_wdt->regs;
+
+	dw_wdt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
+
+	debugfs_create_regset32("registers", 0444, dw_wdt->dbgfs_dir, regset);
+
+	debugfs_create_file_unsafe("timeout", 0600, dw_wdt->dbgfs_dir,
+				   dw_wdt, &dw_wdt_dbgfs_timeout_fops);
+
+	debugfs_create_file_unsafe("pretimeout", 0600, dw_wdt->dbgfs_dir,
+				   dw_wdt, &dw_wdt_dbgfs_pretimeout_fops);
+
+	debugfs_create_file_unsafe("ping", 0200, dw_wdt->dbgfs_dir,
+				   dw_wdt, &dw_wdt_dbgfs_ping_fops);
+
+	debugfs_create_file_unsafe("enable", 0600, dw_wdt->dbgfs_dir,
+				   dw_wdt, &dw_wdt_dbgfs_en_fops);
+}
+
+static void dw_wdt_dbgfs_clear(struct dw_wdt *dw_wdt)
+{
+	debugfs_remove_recursive(dw_wdt->dbgfs_dir);
+}
+
+#else /* !CONFIG_DEBUG_FS */
+
+static void dw_wdt_dbgfs_init(struct dw_wdt *dw_wdt) {}
+static void dw_wdt_dbgfs_clear(struct dw_wdt *dw_wdt) {}
+
+#endif /* !CONFIG_DEBUG_FS */
+
 static int dw_wdt_drv_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -544,6 +690,8 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 	if (ret)
 		goto out_disable_pclk;
 
+	dw_wdt_dbgfs_init(dw_wdt);
+
 	return 0;
 
 out_disable_pclk:
@@ -558,6 +706,8 @@ static int dw_wdt_drv_remove(struct platform_device *pdev)
 {
 	struct dw_wdt *dw_wdt = platform_get_drvdata(pdev);
 
+	dw_wdt_dbgfs_clear(dw_wdt);
+
 	watchdog_unregister_device(&dw_wdt->wdd);
 	reset_control_assert(dw_wdt->rst);
 	clk_disable_unprepare(dw_wdt->pclk);
-- 
2.25.1

Re: [PATCH 7/7] watchdog: dw_wdt: Add DebugFS files

[Guenter Roeck]

On 3/6/20 5:27 AM, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> For the sake of the easier device-driver debug procedure, we added a
> few DebugFS files with the next content: watchdog timeout, watchdog
> pre-timeout, watchdog ping/kick operation, watchdog start/stop, device
> registers dump. They are available only if kernel is configured with
> DebugFS support.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---
>  drivers/watchdog/dw_wdt.c | 150 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)
> 
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index 3000120f7e39..9353d83f3e45 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -27,6 +27,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/reset.h>
>  #include <linux/watchdog.h>
> +#include <linux/debugfs.h>
>  
>  #define WDOG_CONTROL_REG_OFFSET		    0x00
>  #define WDOG_CONTROL_REG_WDT_EN_MASK	    0x01
> @@ -38,8 +39,14 @@
>  #define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
>  #define WDOG_INTERRUPT_STATUS_REG_OFFSET    0x10
>  #define WDOG_INTERRUPT_CLEAR_REG_OFFSET     0x14
> +#define WDOG_COMP_PARAMS_5_REG_OFFSET       0xe4
> +#define WDOG_COMP_PARAMS_4_REG_OFFSET       0xe8
> +#define WDOG_COMP_PARAMS_3_REG_OFFSET       0xec
> +#define WDOG_COMP_PARAMS_2_REG_OFFSET       0xf0
>  #define WDOG_COMP_PARAMS_1_REG_OFFSET       0xf4
>  #define WDOG_COMP_PARAMS_1_USE_FIX_TOP      BIT(6)
> +#define WDOG_COMP_VERSION_REG_OFFSET        0xf8
> +#define WDOG_COMP_TYPE_REG_OFFSET           0xfc
>  
>  /* There are sixteen TOPs (timeout periods) that can be set in the watchdog. */
>  #define DW_WDT_NUM_TOPS		16
> @@ -79,6 +86,10 @@ struct dw_wdt {
>  	/* Save/restore */
>  	u32			control;
>  	u32			timeout;
> +
> +#ifdef CONFIG_DEBUG_FS
> +	struct dentry		*dbgfs_dir;
> +#endif
>  };
>  
>  #define to_dw_wdt(wdd)	container_of(wdd, struct dw_wdt, wdd)
> @@ -430,6 +441,141 @@ static void dw_wdt_init_timeouts(struct dw_wdt *dw_wdt, struct device *dev)
>  			mult_frac(tops[idx], MSEC_PER_SEC, dw_wdt->rate);
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
> +
> +static int dw_wdt_dbgfs_timeout_get(void *priv, u64 *val)
> +{
> +	struct dw_wdt *dw_wdt = priv;
> +
> +	*val = dw_wdt_get_timeout(dw_wdt) / MSEC_PER_SEC;
> +
> +	return 0;
> +}
> +
> +static int dw_wdt_dbgfs_timeout_set(void *priv, u64 val)
> +{
> +	struct dw_wdt *dw_wdt = priv;
> +
> +	return dw_wdt_set_timeout(&dw_wdt->wdd, val);
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_timeout_fops,
> +	dw_wdt_dbgfs_timeout_get, dw_wdt_dbgfs_timeout_set, "%llu\n");
> +
> +static int dw_wdt_dbgfs_pretimeout_get(void *priv, u64 *val)
> +{
> +	struct dw_wdt *dw_wdt = priv;
> +
> +	*val = dw_wdt->wdd.pretimeout;
> +
> +	return 0;
> +}
> +
> +static int dw_wdt_dbgfs_pretimeout_set(void *priv, u64 val)
> +{
> +	struct dw_wdt *dw_wdt = priv;
> +
> +	return dw_wdt_set_pretimeout(&dw_wdt->wdd, val);
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_pretimeout_fops,
> +	dw_wdt_dbgfs_pretimeout_get, dw_wdt_dbgfs_pretimeout_set, "%llu\n");
> +
> +static int dw_wdt_dbgfs_ping_set(void *priv, u64 val)
> +{
> +	struct dw_wdt *dw_wdt = priv;
> +
> +	dw_wdt_ping(&dw_wdt->wdd);
> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_ping_fops,
> +	NULL, dw_wdt_dbgfs_ping_set, "%llu\n");
> +
> +static int dw_wdt_dbgfs_en_get(void *priv, u64 *val)
> +{
> +	struct dw_wdt *dw_wdt = priv;
> +
> +	*val = !!dw_wdt_is_enabled(dw_wdt);
> +
> +	return 0;
> +}
> +
> +static int dw_wdt_dbgfs_en_set(void *priv, u64 val)
> +{
> +	struct dw_wdt *dw_wdt = priv;
> +
> +	if (val)
> +		dw_wdt_start(&dw_wdt->wdd);
> +	else
> +		dw_wdt_stop(&dw_wdt->wdd);
> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_en_fops,
> +	dw_wdt_dbgfs_en_get, dw_wdt_dbgfs_en_set, "%llu\n");
> +
> +#define DW_WDT_DBGFS_REG(_name, _off) \
> +{				      \
> +	.name = _name,		      \
> +	.offset = _off		      \
> +}
> +
> +static const struct debugfs_reg32 dw_wdt_dbgfs_regs[] = {
> +	DW_WDT_DBGFS_REG("cr", WDOG_CONTROL_REG_OFFSET),
> +	DW_WDT_DBGFS_REG("torr", WDOG_TIMEOUT_RANGE_REG_OFFSET),
> +	DW_WDT_DBGFS_REG("ccvr", WDOG_CURRENT_COUNT_REG_OFFSET),
> +	DW_WDT_DBGFS_REG("crr", WDOG_COUNTER_RESTART_REG_OFFSET),
> +	DW_WDT_DBGFS_REG("stat", WDOG_INTERRUPT_STATUS_REG_OFFSET),
> +	DW_WDT_DBGFS_REG("param5", WDOG_COMP_PARAMS_5_REG_OFFSET),
> +	DW_WDT_DBGFS_REG("param4", WDOG_COMP_PARAMS_4_REG_OFFSET),
> +	DW_WDT_DBGFS_REG("param3", WDOG_COMP_PARAMS_3_REG_OFFSET),
> +	DW_WDT_DBGFS_REG("param2", WDOG_COMP_PARAMS_2_REG_OFFSET),
> +	DW_WDT_DBGFS_REG("param1", WDOG_COMP_PARAMS_1_REG_OFFSET),
> +	DW_WDT_DBGFS_REG("version", WDOG_COMP_VERSION_REG_OFFSET),
> +	DW_WDT_DBGFS_REG("type", WDOG_COMP_TYPE_REG_OFFSET)
> +};
> +
> +static void dw_wdt_dbgfs_init(struct dw_wdt *dw_wdt)
> +{
> +	struct device *dev = dw_wdt->wdd.parent;
> +	struct debugfs_regset32 *regset;
> +
> +	regset = devm_kzalloc(dev, sizeof(*regset), GFP_KERNEL);
> +	if (!regset)
> +		return;
> +
> +	regset->regs = dw_wdt_dbgfs_regs;
> +	regset->nregs = ARRAY_SIZE(dw_wdt_dbgfs_regs);
> +	regset->base = dw_wdt->regs;
> +
> +	dw_wdt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
> +
> +	debugfs_create_regset32("registers", 0444, dw_wdt->dbgfs_dir, regset);
> +
> +	debugfs_create_file_unsafe("timeout", 0600, dw_wdt->dbgfs_dir,
> +				   dw_wdt, &dw_wdt_dbgfs_timeout_fops);
> +
> +	debugfs_create_file_unsafe("pretimeout", 0600, dw_wdt->dbgfs_dir,
> +				   dw_wdt, &dw_wdt_dbgfs_pretimeout_fops);
> +
> +	debugfs_create_file_unsafe("ping", 0200, dw_wdt->dbgfs_dir,
> +				   dw_wdt, &dw_wdt_dbgfs_ping_fops);
> +
> +	debugfs_create_file_unsafe("enable", 0600, dw_wdt->dbgfs_dir,
> +				   dw_wdt, &dw_wdt_dbgfs_en_fops);
> +}

I don't oppose debugfs support in general, but I really don't see
the point replicating watchdog core functionality for it - even more so
since this bypasses the watchdog core. This lets one, for example,
enable the watchdog and then unload the driver with the watchdog
running. This is not only unnecessary, it is way too dangerous,
debugfs or not.

NACK for that part.

Guenter

> +
> +static void dw_wdt_dbgfs_clear(struct dw_wdt *dw_wdt)
> +{
> +	debugfs_remove_recursive(dw_wdt->dbgfs_dir);
> +}
> +
> +#else /* !CONFIG_DEBUG_FS */
> +
> +static void dw_wdt_dbgfs_init(struct dw_wdt *dw_wdt) {}
> +static void dw_wdt_dbgfs_clear(struct dw_wdt *dw_wdt) {}
> +
> +#endif /* !CONFIG_DEBUG_FS */
> +
>  static int dw_wdt_drv_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -544,6 +690,8 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto out_disable_pclk;
>  
> +	dw_wdt_dbgfs_init(dw_wdt);
> +
>  	return 0;
>  
>  out_disable_pclk:
> @@ -558,6 +706,8 @@ static int dw_wdt_drv_remove(struct platform_device *pdev)
>  {
>  	struct dw_wdt *dw_wdt = platform_get_drvdata(pdev);
>  
> +	dw_wdt_dbgfs_clear(dw_wdt);
> +
>  	watchdog_unregister_device(&dw_wdt->wdd);
>  	reset_control_assert(dw_wdt->rst);
>  	clk_disable_unprepare(dw_wdt->pclk);
> 

Re: [PATCH 7/7] watchdog: dw_wdt: Add DebugFS files

[Sergey Semin]

On Fri, Mar 06, 2020 at 07:12:32AM -0800, Guenter Roeck wrote:
> On 3/6/20 5:27 AM, Sergey.Semin@baikalelectronics.ru wrote:
> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > For the sake of the easier device-driver debug procedure, we added a
> > few DebugFS files with the next content: watchdog timeout, watchdog
> > pre-timeout, watchdog ping/kick operation, watchdog start/stop, device
> > registers dump. They are available only if kernel is configured with
> > DebugFS support.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > ---
> >  drivers/watchdog/dw_wdt.c | 150 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 150 insertions(+)
> > 
> > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> > index 3000120f7e39..9353d83f3e45 100644
> > --- a/drivers/watchdog/dw_wdt.c
> > +++ b/drivers/watchdog/dw_wdt.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/reset.h>
> >  #include <linux/watchdog.h>
> > +#include <linux/debugfs.h>
> >  
> >  #define WDOG_CONTROL_REG_OFFSET		    0x00
> >  #define WDOG_CONTROL_REG_WDT_EN_MASK	    0x01
> > @@ -38,8 +39,14 @@
> >  #define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
> >  #define WDOG_INTERRUPT_STATUS_REG_OFFSET    0x10
> >  #define WDOG_INTERRUPT_CLEAR_REG_OFFSET     0x14
> > +#define WDOG_COMP_PARAMS_5_REG_OFFSET       0xe4
> > +#define WDOG_COMP_PARAMS_4_REG_OFFSET       0xe8
> > +#define WDOG_COMP_PARAMS_3_REG_OFFSET       0xec
> > +#define WDOG_COMP_PARAMS_2_REG_OFFSET       0xf0
> >  #define WDOG_COMP_PARAMS_1_REG_OFFSET       0xf4
> >  #define WDOG_COMP_PARAMS_1_USE_FIX_TOP      BIT(6)
> > +#define WDOG_COMP_VERSION_REG_OFFSET        0xf8
> > +#define WDOG_COMP_TYPE_REG_OFFSET           0xfc
> >  
> >  /* There are sixteen TOPs (timeout periods) that can be set in the watchdog. */
> >  #define DW_WDT_NUM_TOPS		16
> > @@ -79,6 +86,10 @@ struct dw_wdt {
> >  	/* Save/restore */
> >  	u32			control;
> >  	u32			timeout;
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > +	struct dentry		*dbgfs_dir;
> > +#endif
> >  };
> >  
> >  #define to_dw_wdt(wdd)	container_of(wdd, struct dw_wdt, wdd)
> > @@ -430,6 +441,141 @@ static void dw_wdt_init_timeouts(struct dw_wdt *dw_wdt, struct device *dev)
> >  			mult_frac(tops[idx], MSEC_PER_SEC, dw_wdt->rate);
> >  }
> >  
> > +#ifdef CONFIG_DEBUG_FS
> > +
> > +static int dw_wdt_dbgfs_timeout_get(void *priv, u64 *val)
> > +{
> > +	struct dw_wdt *dw_wdt = priv;
> > +
> > +	*val = dw_wdt_get_timeout(dw_wdt) / MSEC_PER_SEC;
> > +
> > +	return 0;
> > +}
> > +
> > +static int dw_wdt_dbgfs_timeout_set(void *priv, u64 val)
> > +{
> > +	struct dw_wdt *dw_wdt = priv;
> > +
> > +	return dw_wdt_set_timeout(&dw_wdt->wdd, val);
> > +}
> > +DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_timeout_fops,
> > +	dw_wdt_dbgfs_timeout_get, dw_wdt_dbgfs_timeout_set, "%llu\n");
> > +
> > +static int dw_wdt_dbgfs_pretimeout_get(void *priv, u64 *val)
> > +{
> > +	struct dw_wdt *dw_wdt = priv;
> > +
> > +	*val = dw_wdt->wdd.pretimeout;
> > +
> > +	return 0;
> > +}
> > +
> > +static int dw_wdt_dbgfs_pretimeout_set(void *priv, u64 val)
> > +{
> > +	struct dw_wdt *dw_wdt = priv;
> > +
> > +	return dw_wdt_set_pretimeout(&dw_wdt->wdd, val);
> > +}
> > +DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_pretimeout_fops,
> > +	dw_wdt_dbgfs_pretimeout_get, dw_wdt_dbgfs_pretimeout_set, "%llu\n");
> > +
> > +static int dw_wdt_dbgfs_ping_set(void *priv, u64 val)
> > +{
> > +	struct dw_wdt *dw_wdt = priv;
> > +
> > +	dw_wdt_ping(&dw_wdt->wdd);
> > +
> > +	return 0;
> > +}
> > +DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_ping_fops,
> > +	NULL, dw_wdt_dbgfs_ping_set, "%llu\n");
> > +
> > +static int dw_wdt_dbgfs_en_get(void *priv, u64 *val)
> > +{
> > +	struct dw_wdt *dw_wdt = priv;
> > +
> > +	*val = !!dw_wdt_is_enabled(dw_wdt);
> > +
> > +	return 0;
> > +}
> > +
> > +static int dw_wdt_dbgfs_en_set(void *priv, u64 val)
> > +{
> > +	struct dw_wdt *dw_wdt = priv;
> > +
> > +	if (val)
> > +		dw_wdt_start(&dw_wdt->wdd);
> > +	else
> > +		dw_wdt_stop(&dw_wdt->wdd);
> > +
> > +	return 0;
> > +}
> > +DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_en_fops,
> > +	dw_wdt_dbgfs_en_get, dw_wdt_dbgfs_en_set, "%llu\n");
> > +
> > +#define DW_WDT_DBGFS_REG(_name, _off) \
> > +{				      \
> > +	.name = _name,		      \
> > +	.offset = _off		      \
> > +}
> > +
> > +static const struct debugfs_reg32 dw_wdt_dbgfs_regs[] = {
> > +	DW_WDT_DBGFS_REG("cr", WDOG_CONTROL_REG_OFFSET),
> > +	DW_WDT_DBGFS_REG("torr", WDOG_TIMEOUT_RANGE_REG_OFFSET),
> > +	DW_WDT_DBGFS_REG("ccvr", WDOG_CURRENT_COUNT_REG_OFFSET),
> > +	DW_WDT_DBGFS_REG("crr", WDOG_COUNTER_RESTART_REG_OFFSET),
> > +	DW_WDT_DBGFS_REG("stat", WDOG_INTERRUPT_STATUS_REG_OFFSET),
> > +	DW_WDT_DBGFS_REG("param5", WDOG_COMP_PARAMS_5_REG_OFFSET),
> > +	DW_WDT_DBGFS_REG("param4", WDOG_COMP_PARAMS_4_REG_OFFSET),
> > +	DW_WDT_DBGFS_REG("param3", WDOG_COMP_PARAMS_3_REG_OFFSET),
> > +	DW_WDT_DBGFS_REG("param2", WDOG_COMP_PARAMS_2_REG_OFFSET),
> > +	DW_WDT_DBGFS_REG("param1", WDOG_COMP_PARAMS_1_REG_OFFSET),
> > +	DW_WDT_DBGFS_REG("version", WDOG_COMP_VERSION_REG_OFFSET),
> > +	DW_WDT_DBGFS_REG("type", WDOG_COMP_TYPE_REG_OFFSET)
> > +};
> > +
> > +static void dw_wdt_dbgfs_init(struct dw_wdt *dw_wdt)
> > +{
> > +	struct device *dev = dw_wdt->wdd.parent;
> > +	struct debugfs_regset32 *regset;
> > +
> > +	regset = devm_kzalloc(dev, sizeof(*regset), GFP_KERNEL);
> > +	if (!regset)
> > +		return;
> > +
> > +	regset->regs = dw_wdt_dbgfs_regs;
> > +	regset->nregs = ARRAY_SIZE(dw_wdt_dbgfs_regs);
> > +	regset->base = dw_wdt->regs;
> > +
> > +	dw_wdt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
> > +
> > +	debugfs_create_regset32("registers", 0444, dw_wdt->dbgfs_dir, regset);
> > +
> > +	debugfs_create_file_unsafe("timeout", 0600, dw_wdt->dbgfs_dir,
> > +				   dw_wdt, &dw_wdt_dbgfs_timeout_fops);
> > +
> > +	debugfs_create_file_unsafe("pretimeout", 0600, dw_wdt->dbgfs_dir,
> > +				   dw_wdt, &dw_wdt_dbgfs_pretimeout_fops);
> > +
> > +	debugfs_create_file_unsafe("ping", 0200, dw_wdt->dbgfs_dir,
> > +				   dw_wdt, &dw_wdt_dbgfs_ping_fops);
> > +
> > +	debugfs_create_file_unsafe("enable", 0600, dw_wdt->dbgfs_dir,
> > +				   dw_wdt, &dw_wdt_dbgfs_en_fops);
> > +}
> 
> I don't oppose debugfs support in general, but I really don't see
> the point replicating watchdog core functionality for it - even more so
> since this bypasses the watchdog core. This lets one, for example,
> enable the watchdog and then unload the driver with the watchdog
> running. This is not only unnecessary, it is way too dangerous,
> debugfs or not.
> 
> NACK for that part.

I see your point. What if I keep the registers dump and read-only
timeout, pretimeout, enable and timeleft nodes? This won't be harmful,
but the info still can be useful to debug the driver.

-Sergey

> 
> Guenter
> 
> > +
> > +static void dw_wdt_dbgfs_clear(struct dw_wdt *dw_wdt)
> > +{
> > +	debugfs_remove_recursive(dw_wdt->dbgfs_dir);
> > +}
> > +
> > +#else /* !CONFIG_DEBUG_FS */
> > +
> > +static void dw_wdt_dbgfs_init(struct dw_wdt *dw_wdt) {}
> > +static void dw_wdt_dbgfs_clear(struct dw_wdt *dw_wdt) {}
> > +
> > +#endif /* !CONFIG_DEBUG_FS */
> > +
> >  static int dw_wdt_drv_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -544,6 +690,8 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		goto out_disable_pclk;
> >  
> > +	dw_wdt_dbgfs_init(dw_wdt);
> > +
> >  	return 0;
> >  
> >  out_disable_pclk:
> > @@ -558,6 +706,8 @@ static int dw_wdt_drv_remove(struct platform_device *pdev)
> >  {
> >  	struct dw_wdt *dw_wdt = platform_get_drvdata(pdev);
> >  
> > +	dw_wdt_dbgfs_clear(dw_wdt);
> > +
> >  	watchdog_unregister_device(&dw_wdt->wdd);
> >  	reset_control_assert(dw_wdt->rst);
> >  	clk_disable_unprepare(dw_wdt->pclk);
> > 
> 

Re: [PATCH 0/7] watchdog: dw_wdt: Take Baikal-T1 DW WDT peculiarities into account

[Sergey Semin]

On Fri, Mar 06, 2020 at 04:27:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <fancer.lancer@gmail.com>
> 
> There were a few features enabled at the time of the Baikal-T1 SoC DW WDT
> IP synthesis, which weren't taken into account in the DW WDT driver available
> in the kernel. First of all the SoC engineers synthesized the watchdog core
> with WDT_USE_FIX_TOP set to false (don't really know why, but they did).
> Due to this the timer reset values weren't fixed as the driver expected
> but were initialized with a pre-defined values selected by the engineers.
> Secondly the driver expected that the watchdog APB bus and the timer had
> synchronous reference clocks, while Baikal-T1 SoC DW WDT was created with
> asynchronous ones. So the driver should enable two clock devices: APB bus
> clocks and a separate timer reference clock. Finally DW Watchdog Timer is
> capable of generating a pre-timeout interrupt if corresponding config is
> enabled. The problem was that the pre-timeout IRQ happens when the set
> timeout elapses, while the actual WDT expiration and subsequent reboot take
> place in the next timeout. This makes the pre-timeout functionality
> implementation a bit tricky, since in this case we would have to find a
> WDT timeout twice smaller the requested timeout. All of the changes described
> above are provided by the patches in this patchset.
> 
> In addition traditionally we replaced the legacy plain text-based dt-binding
> file with yaml-based one, made some cleanups in the watchdog core code (just
> replaced time-unit numerical literals with corresponding macro) and added
> DebugFS nodes to ease the driver debug procedure.
> 
> This patchset is rebased and tested on the mainline Linux kernel 5.6-rc4:
> commit 98d54f81e36b ("Linux 5.6-rc4").
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru>
> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru>
> Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> Serge Semin (7):
>   dt-bindings: watchdog: dw-wdt: Replace legacy bindings file with
>     YAML-based one
>   dt-bindings: watchdog: dw-wdt: Add watchdog TOPs array property
>   watchdog: watchdog_dev: Use generic msec-per-sec macro
>   watchdog: dw_wdt: Support devices with non-fixed TOP values
>   watchdog: dw_wdt: Support devices with asynch clocks
>   watchdog: dw_wdt: Add pre-timeouts support
>   watchdog: dw_wdt: Add DebugFS files
> 
>  .../devicetree/bindings/watchdog/dw_wdt.txt   |  24 -
>  .../bindings/watchdog/snps,dw-wdt.yaml        |  96 ++++
>  drivers/watchdog/dw_wdt.c                     | 460 ++++++++++++++++--
>  drivers/watchdog/watchdog_dev.c               |   4 +-
>  4 files changed, 523 insertions(+), 61 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/watchdog/dw_wdt.txt
>  create mode 100644 Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml
> 
> -- 
> 2.25.1
> 

Folks,

It appears our corporate email server changes the Message-Id field of 
messages passing through it. Due to that the emails threading gets to be
broken. I'll resubmit the properly structured v2 patchset as soon as our
system administrator fixes the problem and all the questions, already raised by
the maintainers/reviewer, are settled. Sorry for the inconvenience caused me.

Regards,
-Sergey