[PATCH v2 00/12] watchdog: s3c2410: Add Exynos850 support

[PATCH v2 00/12] watchdog: s3c2410: Add Exynos850 support

[Sam Protsenko]

Exynos850 WDT IP-core differs a bit from existing platforms:
  - Another set of PMU registers
  - Separate WDT instance for each CPU cluster, having different PMU
    registers/bits
  - Source clock is different from PCLK

Implement all missing features above and enable Exynos850 WDT support.
While at it, do a bit of cleaning up.

Changes in v2:
  - Added patch to fail probe if no valid timeout was found, as
    suggested by Guenter Roeck (03/12)
  - Extracted code for separating disable/mask functions into separate
    patch (06/12)
  - Added patch for inverting mask register value (07/12)
  - Extracted PMU cleanup code to separate patch, to ease the review and
    porting (09/12)
  - Added patch for removing not needed 'err' label in probe function (11/12)
  - Addressed all outstanding review comments on mailing list

Sam Protsenko (12):
  dt-bindings: watchdog: Require samsung,syscon-phandle for Exynos7
  dt-bindings: watchdog: Document Exynos850 watchdog bindings
  watchdog: s3c2410: Fail probe if can't find valid timeout
  watchdog: s3c2410: Let kernel kick watchdog
  watchdog: s3c2410: Make reset disable register optional
  watchdog: s3c2410: Extract disable and mask code into separate
    functions
  watchdog: s3c2410: Implement a way to invert mask reg value
  watchdog: s3c2410: Add support for WDT counter enable register
  watchdog: s3c2410: Cleanup PMU related code
  watchdog: s3c2410: Support separate source clock
  watchdog: s3c2410: Remove superfluous err label
  watchdog: s3c2410: Add Exynos850 support

 .../bindings/watchdog/samsung-wdt.yaml        |  47 ++-
 drivers/watchdog/s3c2410_wdt.c                | 289 +++++++++++++-----
 2 files changed, 254 insertions(+), 82 deletions(-)

-- 
2.30.2

[PATCH v2 01/12] dt-bindings: watchdog: Require samsung,syscon-phandle for Exynos7

[Sam Protsenko]

Exynos7 watchdog driver is clearly indicating that its dts node must
define syscon phandle property. That was probably forgotten, so add it.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
Fixes: 2b9366b66967 ("watchdog: s3c2410_wdt: Add support for Watchdog device on Exynos7")
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
Changes in v2:
  - Added R-b tag by Krzysztof Kozlowski
  - Added "Fixes" tag

 Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
index 76cb9586ee00..93cd77a6e92c 100644
--- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
@@ -39,8 +39,8 @@ properties:
   samsung,syscon-phandle:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
-      Phandle to the PMU system controller node (in case of Exynos5250
-      and Exynos5420).
+      Phandle to the PMU system controller node (in case of Exynos5250,
+      Exynos5420 and Exynos7).
 
 required:
   - compatible
@@ -58,6 +58,7 @@ allOf:
             enum:
               - samsung,exynos5250-wdt
               - samsung,exynos5420-wdt
+              - samsung,exynos7-wdt
     then:
       required:
         - samsung,syscon-phandle
-- 
2.30.2

Re: [PATCH v2 01/12] dt-bindings: watchdog: Require samsung,syscon-phandle for Exynos7

[Rob Herring]

On Sun, 31 Oct 2021 14:22:05 +0200, Sam Protsenko wrote:
> Exynos7 watchdog driver is clearly indicating that its dts node must
> define syscon phandle property. That was probably forgotten, so add it.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> Fixes: 2b9366b66967 ("watchdog: s3c2410_wdt: Add support for Watchdog device on Exynos7")
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
> Changes in v2:
>   - Added R-b tag by Krzysztof Kozlowski
>   - Added "Fixes" tag
> 
>  Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 

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

[PATCH v2 02/12] dt-bindings: watchdog: Document Exynos850 watchdog bindings

[Sam Protsenko]

Exynos850 SoC has two CPU clusters:
  - cluster 0: contains CPUs #0, #1, #2, #3
  - cluster 1: contains CPUs #4, #5, #6, #7

Each cluster has its own dedicated watchdog timer. Those WDT instances
are controlled using different bits in PMU registers, new
"samsung,index" property is added to tell the driver which bits to use
for defined watchdog node.

Also on Exynos850 the peripheral clock and the source clock are two
different clocks. Provide a way to specify two clocks in watchdog device
tree node.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - Stated explicitly that Exynos850 driver requires 2 clocks
  - Used single compatible for Exynos850
  - Added "index" property to specify CPU cluster index
  - Fixed a typo in commit message: dedicater -> dedicated

 .../bindings/watchdog/samsung-wdt.yaml        | 44 +++++++++++++++++--
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
index 93cd77a6e92c..f29d0ca4eced 100644
--- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
@@ -22,25 +22,32 @@ properties:
       - samsung,exynos5250-wdt                # for Exynos5250
       - samsung,exynos5420-wdt                # for Exynos5420
       - samsung,exynos7-wdt                   # for Exynos7
+      - samsung,exynos850-wdt                 # for Exynos850
 
   reg:
     maxItems: 1
 
   clocks:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
 
   clock-names:
-    items:
-      - const: watchdog
+    minItems: 1
+    maxItems: 2
 
   interrupts:
     maxItems: 1
 
+  samsung,index:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Index of CPU cluster on which watchdog is running (in case of Exynos850)
+
   samsung,syscon-phandle:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
       Phandle to the PMU system controller node (in case of Exynos5250,
-      Exynos5420 and Exynos7).
+      Exynos5420, Exynos7 and Exynos850).
 
 required:
   - compatible
@@ -59,9 +66,38 @@ allOf:
               - samsung,exynos5250-wdt
               - samsung,exynos5420-wdt
               - samsung,exynos7-wdt
+              - samsung,exynos850-wdt
     then:
       required:
         - samsung,syscon-phandle
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - samsung,exynos850-wdt
+    then:
+      properties:
+        clocks:
+          items:
+            - description: Bus clock, used for register interface
+            - description: Source clock (driving watchdog counter)
+        clock-names:
+          items:
+            - const: watchdog
+            - const: watchdog_src
+        samsung,index:
+          enum: [0, 1]
+      required:
+        - samsung,index
+    else:
+      properties:
+        clocks:
+          items:
+            - description: Bus clock, which is also a source clock
+        clock-names:
+          items:
+            - const: watchdog
 
 unevaluatedProperties: false
 
-- 
2.30.2

Re: [PATCH v2 02/12] dt-bindings: watchdog: Document Exynos850 watchdog bindings

[Krzysztof Kozlowski]

On 31/10/2021 13:22, Sam Protsenko wrote:
> Exynos850 SoC has two CPU clusters:
>   - cluster 0: contains CPUs #0, #1, #2, #3
>   - cluster 1: contains CPUs #4, #5, #6, #7
> 
> Each cluster has its own dedicated watchdog timer. Those WDT instances
> are controlled using different bits in PMU registers, new
> "samsung,index" property is added to tell the driver which bits to use
> for defined watchdog node.
> 
> Also on Exynos850 the peripheral clock and the source clock are two
> different clocks. Provide a way to specify two clocks in watchdog device
> tree node.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - Stated explicitly that Exynos850 driver requires 2 clocks
>   - Used single compatible for Exynos850
>   - Added "index" property to specify CPU cluster index
>   - Fixed a typo in commit message: dedicater -> dedicated
> 
>  .../bindings/watchdog/samsung-wdt.yaml        | 44 +++++++++++++++++--
>  1 file changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
> index 93cd77a6e92c..f29d0ca4eced 100644
> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
> @@ -22,25 +22,32 @@ properties:
>        - samsung,exynos5250-wdt                # for Exynos5250
>        - samsung,exynos5420-wdt                # for Exynos5420
>        - samsung,exynos7-wdt                   # for Exynos7
> +      - samsung,exynos850-wdt                 # for Exynos850
>  
>    reg:
>      maxItems: 1
>  
>    clocks:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 2
>  
>    clock-names:
> -    items:
> -      - const: watchdog
> +    minItems: 1
> +    maxItems: 2
>  
>    interrupts:
>      maxItems: 1
>  
> +  samsung,index:

Slightly more descriptive, e.g.:
samsung,cluster-index

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Index of CPU cluster on which watchdog is running (in case of Exynos850)
> +
>    samsung,syscon-phandle:
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description:
>        Phandle to the PMU system controller node (in case of Exynos5250,
> -      Exynos5420 and Exynos7).
> +      Exynos5420, Exynos7 and Exynos850).
>  
>  required:
>    - compatible
> @@ -59,9 +66,38 @@ allOf:
>                - samsung,exynos5250-wdt
>                - samsung,exynos5420-wdt
>                - samsung,exynos7-wdt
> +              - samsung,exynos850-wdt
>      then:
>        required:
>          - samsung,syscon-phandle
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - samsung,exynos850-wdt
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: Bus clock, used for register interface
> +            - description: Source clock (driving watchdog counter)
> +        clock-names:
> +          items:
> +            - const: watchdog
> +            - const: watchdog_src
> +        samsung,index:
> +          enum: [0, 1]
> +      required:
> +        - samsung,index
> +    else:
> +      properties:
> +        clocks:
> +          items:
> +            - description: Bus clock, which is also a source clock
> +        clock-names:
> +          items:
> +            - const: watchdog

Also under this else:
   samsung,cluster-index: false

>  
>  unevaluatedProperties: false
>  
> 


Best regards,
Krzysztof

[PATCH v2 03/12] watchdog: s3c2410: Fail probe if can't find valid timeout

[Sam Protsenko]

Driver can't work properly if there no valid timeout was found in
s3c2410wdt_set_heartbeat(). Ideally, that function should be reworked in
a way that it's always able to find some valid timeout. As a temporary
solution let's for now just fail the driver probe in case the valid
timeout can't be found in s3c2410wdt_set_heartbeat() function.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Suggested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - (none): it's a new patch

 drivers/watchdog/s3c2410_wdt.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 2395f353e52d..00421cf22556 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -515,7 +515,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	struct s3c2410_wdt *wdt;
 	struct resource *wdt_irq;
 	unsigned int wtcon;
-	int started = 0;
 	int ret;
 
 	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
@@ -581,15 +580,15 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	ret = s3c2410wdt_set_heartbeat(&wdt->wdt_device,
 					wdt->wdt_device.timeout);
 	if (ret) {
-		started = s3c2410wdt_set_heartbeat(&wdt->wdt_device,
-					S3C2410_WATCHDOG_DEFAULT_TIME);
-
-		if (started == 0)
-			dev_info(dev,
-				 "tmr_margin value out of range, default %d used\n",
+		ret = s3c2410wdt_set_heartbeat(&wdt->wdt_device,
+					       S3C2410_WATCHDOG_DEFAULT_TIME);
+		if (ret == 0) {
+			dev_warn(dev, "tmr_margin value out of range, default %d used\n",
 				 S3C2410_WATCHDOG_DEFAULT_TIME);
-		else
-			dev_info(dev, "default timer value is out of range, cannot start\n");
+		} else {
+			dev_err(dev, "failed to use default timeout\n");
+			goto err_cpufreq;
+		}
 	}
 
 	ret = devm_request_irq(dev, wdt_irq->start, s3c2410wdt_irq, 0,
@@ -613,10 +612,10 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_unregister;
 
-	if (tmr_atboot && started == 0) {
+	if (tmr_atboot) {
 		dev_info(dev, "starting watchdog timer\n");
 		s3c2410wdt_start(&wdt->wdt_device);
-	} else if (!tmr_atboot) {
+	} else {
 		/* if we're not enabling the watchdog, then ensure it is
 		 * disabled if it has been left running from the bootloader
 		 * or other source */
-- 
2.30.2

Re: [PATCH v2 03/12] watchdog: s3c2410: Fail probe if can't find valid timeout

[Krzysztof Kozlowski]

On 31/10/2021 13:22, Sam Protsenko wrote:
> Driver can't work properly if there no valid timeout was found in
> s3c2410wdt_set_heartbeat(). Ideally, that function should be reworked in
> a way that it's always able to find some valid timeout. As a temporary
> solution let's for now just fail the driver probe in case the valid
> timeout can't be found in s3c2410wdt_set_heartbeat() function.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - (none): it's a new patch
> 
>  drivers/watchdog/s3c2410_wdt.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

[PATCH v2 04/12] watchdog: s3c2410: Let kernel kick watchdog

[Sam Protsenko]

When "tmr_atboot" module param is set, the watchdog is started in
driver's probe. In that case, also set WDOG_HW_RUNNING bit to let
watchdog core driver know it's running. This way watchdog core can kick
the watchdog for us (if CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED option is
enabled), until user space takes control.

WDOG_HW_RUNNING bit must be set before registering the watchdog. So the
"tmr_atboot" handling code is moved before watchdog registration, to
avoid performing the same check twice. This is also logical because
WDOG_HW_RUNNING bit makes WDT core expect actually running watchdog.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - Added explanation on moving the code block to commit message
  - [PATCH 03/12] handles the case when tmr_atboot is present but valid
    timeout wasn't found

 drivers/watchdog/s3c2410_wdt.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 00421cf22556..0845c05034a1 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -604,6 +604,21 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
 	wdt->wdt_device.parent = dev;
 
+	/*
+	 * If "tmr_atboot" param is non-zero, start the watchdog right now. Also
+	 * set WDOG_HW_RUNNING bit, so that watchdog core can kick the watchdog.
+	 *
+	 * If we're not enabling the watchdog, then ensure it is disabled if it
+	 * has been left running from the bootloader or other source.
+	 */
+	if (tmr_atboot) {
+		dev_info(dev, "starting watchdog timer\n");
+		s3c2410wdt_start(&wdt->wdt_device);
+		set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
+	} else {
+		s3c2410wdt_stop(&wdt->wdt_device);
+	}
+
 	ret = watchdog_register_device(&wdt->wdt_device);
 	if (ret)
 		goto err_cpufreq;
@@ -612,17 +627,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_unregister;
 
-	if (tmr_atboot) {
-		dev_info(dev, "starting watchdog timer\n");
-		s3c2410wdt_start(&wdt->wdt_device);
-	} else {
-		/* if we're not enabling the watchdog, then ensure it is
-		 * disabled if it has been left running from the bootloader
-		 * or other source */
-
-		s3c2410wdt_stop(&wdt->wdt_device);
-	}
-
 	platform_set_drvdata(pdev, wdt);
 
 	/* print out a statement of readiness */
-- 
2.30.2

Re: [PATCH v2 04/12] watchdog: s3c2410: Let kernel kick watchdog

[Krzysztof Kozlowski]

On 31/10/2021 13:22, Sam Protsenko wrote:
> When "tmr_atboot" module param is set, the watchdog is started in
> driver's probe. In that case, also set WDOG_HW_RUNNING bit to let
> watchdog core driver know it's running. This way watchdog core can kick
> the watchdog for us (if CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED option is
> enabled), until user space takes control.
> 
> WDOG_HW_RUNNING bit must be set before registering the watchdog. So the
> "tmr_atboot" handling code is moved before watchdog registration, to
> avoid performing the same check twice. This is also logical because
> WDOG_HW_RUNNING bit makes WDT core expect actually running watchdog.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - Added explanation on moving the code block to commit message
>   - [PATCH 03/12] handles the case when tmr_atboot is present but valid
>     timeout wasn't found
> 
>  drivers/watchdog/s3c2410_wdt.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

[PATCH v2 05/12] watchdog: s3c2410: Make reset disable register optional

[Sam Protsenko]

On new Exynos chips (e.g. Exynos850 and Exynos9) the
AUTOMATIC_WDT_RESET_DISABLE register was removed, and its value can be
thought of as "always 0x0". Add correspondig quirk bit, so that the
driver can omit accessing it if it's not present.

This commit doesn't bring any functional change to existing devices, but
merely provides an infrastructure for upcoming chips support.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - Used quirks instead of callbacks for all added PMU registers
  - Used BIT() macro
  - Extracted splitting the s3c2410wdt_mask_and_disable_reset() function
    to separate patch
  - Extracted cleanup code to separate patch to minimize changes and
    ease the review and porting

 drivers/watchdog/s3c2410_wdt.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 0845c05034a1..048ca47d0b8a 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -59,10 +59,12 @@
 #define QUIRK_HAS_PMU_CONFIG			(1 << 0)
 #define QUIRK_HAS_RST_STAT			(1 << 1)
 #define QUIRK_HAS_WTCLRINT_REG			(1 << 2)
+#define QUIRK_HAS_PMU_AUTO_DISABLE		(1 << 3)
 
 /* These quirks require that we have a PMU register map */
 #define QUIRKS_HAVE_PMUREG			(QUIRK_HAS_PMU_CONFIG | \
-						 QUIRK_HAS_RST_STAT)
+						 QUIRK_HAS_RST_STAT | \
+						 QUIRK_HAS_PMU_AUTO_DISABLE)
 
 static bool nowayout	= WATCHDOG_NOWAYOUT;
 static int tmr_margin;
@@ -137,7 +139,7 @@ static const struct s3c2410_wdt_variant drv_data_exynos5250  = {
 	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
 	.rst_stat_bit = 20,
 	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
-		  | QUIRK_HAS_WTCLRINT_REG,
+		  | QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_AUTO_DISABLE,
 };
 
 static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
@@ -147,7 +149,7 @@ static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
 	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
 	.rst_stat_bit = 9,
 	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
-		  | QUIRK_HAS_WTCLRINT_REG,
+		  | QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_AUTO_DISABLE,
 };
 
 static const struct s3c2410_wdt_variant drv_data_exynos7 = {
@@ -157,7 +159,7 @@ static const struct s3c2410_wdt_variant drv_data_exynos7 = {
 	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
 	.rst_stat_bit = 23,	/* A57 WDTRESET */
 	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
-		  | QUIRK_HAS_WTCLRINT_REG,
+		  | QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_AUTO_DISABLE,
 };
 
 static const struct of_device_id s3c2410_wdt_match[] = {
@@ -213,11 +215,13 @@ static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask)
 	if (mask)
 		val = mask_val;
 
-	ret = regmap_update_bits(wdt->pmureg,
-			wdt->drv_data->disable_reg,
-			mask_val, val);
-	if (ret < 0)
-		goto error;
+	if (wdt->drv_data->quirks & QUIRK_HAS_PMU_AUTO_DISABLE) {
+		ret = regmap_update_bits(wdt->pmureg,
+				wdt->drv_data->disable_reg,
+				mask_val, val);
+		if (ret < 0)
+			goto error;
+	}
 
 	ret = regmap_update_bits(wdt->pmureg,
 			wdt->drv_data->mask_reset_reg,
-- 
2.30.2

Re: [PATCH v2 05/12] watchdog: s3c2410: Make reset disable register optional

[Krzysztof Kozlowski]

On 31/10/2021 13:22, Sam Protsenko wrote:
> On new Exynos chips (e.g. Exynos850 and Exynos9) the
> AUTOMATIC_WDT_RESET_DISABLE register was removed, and its value can be
> thought of as "always 0x0". Add correspondig quirk bit, so that the
> driver can omit accessing it if it's not present.
> 
> This commit doesn't bring any functional change to existing devices, but
> merely provides an infrastructure for upcoming chips support.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - Used quirks instead of callbacks for all added PMU registers
>   - Used BIT() macro
>   - Extracted splitting the s3c2410wdt_mask_and_disable_reset() function
>     to separate patch
>   - Extracted cleanup code to separate patch to minimize changes and
>     ease the review and porting
> 
>  drivers/watchdog/s3c2410_wdt.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 0845c05034a1..048ca47d0b8a 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -59,10 +59,12 @@
>  #define QUIRK_HAS_PMU_CONFIG			(1 << 0)
>  #define QUIRK_HAS_RST_STAT			(1 << 1)
>  #define QUIRK_HAS_WTCLRINT_REG			(1 << 2)
> +#define QUIRK_HAS_PMU_AUTO_DISABLE		(1 << 3)
>  
>  /* These quirks require that we have a PMU register map */
>  #define QUIRKS_HAVE_PMUREG			(QUIRK_HAS_PMU_CONFIG | \
> -						 QUIRK_HAS_RST_STAT)
> +						 QUIRK_HAS_RST_STAT | \
> +						 QUIRK_HAS_PMU_AUTO_DISABLE)
>  
>  static bool nowayout	= WATCHDOG_NOWAYOUT;
>  static int tmr_margin;
> @@ -137,7 +139,7 @@ static const struct s3c2410_wdt_variant drv_data_exynos5250  = {
>  	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
>  	.rst_stat_bit = 20,
>  	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
> -		  | QUIRK_HAS_WTCLRINT_REG,
> +		  | QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_AUTO_DISABLE,
>  };
>  
>  static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
> @@ -147,7 +149,7 @@ static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
>  	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
>  	.rst_stat_bit = 9,
>  	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
> -		  | QUIRK_HAS_WTCLRINT_REG,
> +		  | QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_AUTO_DISABLE,
>  };
>  
>  static const struct s3c2410_wdt_variant drv_data_exynos7 = {
> @@ -157,7 +159,7 @@ static const struct s3c2410_wdt_variant drv_data_exynos7 = {
>  	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
>  	.rst_stat_bit = 23,	/* A57 WDTRESET */
>  	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
> -		  | QUIRK_HAS_WTCLRINT_REG,
> +		  | QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_AUTO_DISABLE,
>  };
>  
>  static const struct of_device_id s3c2410_wdt_match[] = {
> @@ -213,11 +215,13 @@ static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask)
>  	if (mask)
>  		val = mask_val;
>  
> -	ret = regmap_update_bits(wdt->pmureg,
> -			wdt->drv_data->disable_reg,
> -			mask_val, val);
> -	if (ret < 0)
> -		goto error;
> +	if (wdt->drv_data->quirks & QUIRK_HAS_PMU_AUTO_DISABLE) {
> +		ret = regmap_update_bits(wdt->pmureg,
> +				wdt->drv_data->disable_reg,
> +				mask_val, val);

While shuffling the code, please align the arguments with opening
parentheses.

Beside that looks ok:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

Best regards,
Krzysztof

[PATCH v2 06/12] watchdog: s3c2410: Extract disable and mask code into separate functions

[Sam Protsenko]

The s3c2410wdt_mask_and_disable_reset() function content is bound to be
changed further. Prepare it for upcoming changes by splitting into
separate "mask reset" and "disable reset" functions. But keep
s3c2410wdt_mask_and_disable_reset() function present as a facade.

This commit doesn't bring any functional change to existing devices, but
merely provides an infrastructure for upcoming chips support.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - (none): it's a new patch

 drivers/watchdog/s3c2410_wdt.c | 54 ++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 048ca47d0b8a..4ac0a30e835e 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -202,37 +202,53 @@ static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb)
 	return container_of(nb, struct s3c2410_wdt, freq_transition);
 }
 
-static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask)
+static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
 {
+	const u32 mask_val = BIT(wdt->drv_data->mask_bit);
+	const u32 val = mask ? mask_val : 0;
 	int ret;
-	u32 mask_val = 1 << wdt->drv_data->mask_bit;
-	u32 val = 0;
 
-	/* No need to do anything if no PMU CONFIG needed */
-	if (!(wdt->drv_data->quirks & QUIRK_HAS_PMU_CONFIG))
-		return 0;
+	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->disable_reg,
+				 mask_val, val);
+	if (ret < 0)
+		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
 
-	if (mask)
-		val = mask_val;
+	return ret;
+}
 
-	if (wdt->drv_data->quirks & QUIRK_HAS_PMU_AUTO_DISABLE) {
-		ret = regmap_update_bits(wdt->pmureg,
-				wdt->drv_data->disable_reg,
-				mask_val, val);
-		if (ret < 0)
-			goto error;
-	}
+static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
+{
+	const u32 mask_val = BIT(wdt->drv_data->mask_bit);
+	const u32 val = mask ? mask_val : 0;
+	int ret;
 
-	ret = regmap_update_bits(wdt->pmureg,
-			wdt->drv_data->mask_reset_reg,
-			mask_val, val);
- error:
+	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
+				 mask_val, val);
 	if (ret < 0)
 		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
 
 	return ret;
 }
 
+static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask)
+{
+	int ret;
+
+	if (wdt->drv_data->quirks & QUIRK_HAS_PMU_AUTO_DISABLE) {
+		ret = s3c2410wdt_disable_wdt_reset(wdt, mask);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (wdt->drv_data->quirks & QUIRK_HAS_PMU_CONFIG) {
+		ret = s3c2410wdt_mask_wdt_reset(wdt, mask);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
 {
 	struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
-- 
2.30.2

Re: [PATCH v2 06/12] watchdog: s3c2410: Extract disable and mask code into separate functions

[Krzysztof Kozlowski]

On 31/10/2021 13:22, Sam Protsenko wrote:
> The s3c2410wdt_mask_and_disable_reset() function content is bound to be
> changed further. Prepare it for upcoming changes by splitting into
> separate "mask reset" and "disable reset" functions. But keep
> s3c2410wdt_mask_and_disable_reset() function present as a facade.
> 
> This commit doesn't bring any functional change to existing devices, but
> merely provides an infrastructure for upcoming chips support.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - (none): it's a new patch
> 
>  drivers/watchdog/s3c2410_wdt.c | 54 ++++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 19 deletions(-)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

[PATCH v2 07/12] watchdog: s3c2410: Implement a way to invert mask reg value

[Sam Protsenko]

On new Exynos chips (like Exynos850) the MASK_WDT_RESET_REQUEST register
is replaced with CLUSTERx_NONCPU_INT_EN, and its mask bit value meaning
was reversed: for new register the bit value "1" means "Interrupt
enabled", while for MASK_WDT_RESET_REQUEST register "1" means "Mask the
interrupt" (i.e. "Interrupt disabled").

Introduce "mask_reset_inv" boolean field in driver data structure; when
that field is "true", mask register handling function will invert the
value before setting it to the register.

This commit doesn't bring any functional change to existing devices, but
merely provides an infrastructure for upcoming chips support.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - (none): it's a new patch

 drivers/watchdog/s3c2410_wdt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 4ac0a30e835e..2a61b6ea5602 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -92,6 +92,7 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, 0 to
  * timer reset functionality.
  * @mask_reset_reg: Offset in pmureg for the register that masks the watchdog
  * timer reset functionality.
+ * @mask_reset_inv: If set, mask_reset_reg value will have inverted meaning.
  * @mask_bit: Bit number for the watchdog timer in the disable register and the
  * mask reset register.
  * @rst_stat_reg: Offset in pmureg for the register that has the reset status.
@@ -103,6 +104,7 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, 0 to
 struct s3c2410_wdt_variant {
 	int disable_reg;
 	int mask_reset_reg;
+	bool mask_reset_inv;
 	int mask_bit;
 	int rst_stat_reg;
 	int rst_stat_bit;
@@ -219,7 +221,8 @@ static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
 static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
 {
 	const u32 mask_val = BIT(wdt->drv_data->mask_bit);
-	const u32 val = mask ? mask_val : 0;
+	const bool val_inv = wdt->drv_data->mask_reset_inv;
+	const u32 val = (mask ^ val_inv) ? mask_val : 0;
 	int ret;
 
 	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
-- 
2.30.2

Re: [PATCH v2 07/12] watchdog: s3c2410: Implement a way to invert mask reg value

[Krzysztof Kozlowski]

On 31/10/2021 13:22, Sam Protsenko wrote:
> On new Exynos chips (like Exynos850) the MASK_WDT_RESET_REQUEST register
> is replaced with CLUSTERx_NONCPU_INT_EN, and its mask bit value meaning
> was reversed: for new register the bit value "1" means "Interrupt
> enabled", while for MASK_WDT_RESET_REQUEST register "1" means "Mask the
> interrupt" (i.e. "Interrupt disabled").
> 
> Introduce "mask_reset_inv" boolean field in driver data structure; when
> that field is "true", mask register handling function will invert the
> value before setting it to the register.
> 
> This commit doesn't bring any functional change to existing devices, but
> merely provides an infrastructure for upcoming chips support.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - (none): it's a new patch
> 
>  drivers/watchdog/s3c2410_wdt.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

[PATCH v2 08/12] watchdog: s3c2410: Add support for WDT counter enable register

[Sam Protsenko]

On new Exynos chips (e.g. Exynos850) new CLUSTERx_NONCPU_OUT register is
introduced, where CNT_EN_WDT bit must be enabled to make watchdog
counter running. Add corresponding quirk and proper infrastructure to
handle that register if the quirk is set.

This commit doesn't bring any functional change to existing devices, but
merely provides an infrastructure for upcoming chips support.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - Used quirks instead of callbacks for all added PMU registers
  - Used BIT() macro
  - Extracted cleanup code to separate patch to minimize changes and
    ease the review and porting

 drivers/watchdog/s3c2410_wdt.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 2a61b6ea5602..ec341c876225 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -60,11 +60,13 @@
 #define QUIRK_HAS_RST_STAT			(1 << 1)
 #define QUIRK_HAS_WTCLRINT_REG			(1 << 2)
 #define QUIRK_HAS_PMU_AUTO_DISABLE		(1 << 3)
+#define QUIRK_HAS_PMU_CNT_EN			(1 << 4)
 
 /* These quirks require that we have a PMU register map */
 #define QUIRKS_HAVE_PMUREG			(QUIRK_HAS_PMU_CONFIG | \
 						 QUIRK_HAS_RST_STAT | \
-						 QUIRK_HAS_PMU_AUTO_DISABLE)
+						 QUIRK_HAS_PMU_AUTO_DISABLE | \
+						 QUIRK_HAS_PMU_CNT_EN)
 
 static bool nowayout	= WATCHDOG_NOWAYOUT;
 static int tmr_margin;
@@ -98,6 +100,8 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, 0 to
  * @rst_stat_reg: Offset in pmureg for the register that has the reset status.
  * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
  * reset.
+ * @cnt_en_reg: Offset in pmureg for the register that enables WDT counter.
+ * @cnt_en_bit: Bit number for "watchdog counter enable" in cnt_en register.
  * @quirks: A bitfield of quirks.
  */
 
@@ -108,6 +112,8 @@ struct s3c2410_wdt_variant {
 	int mask_bit;
 	int rst_stat_reg;
 	int rst_stat_bit;
+	int cnt_en_reg;
+	int cnt_en_bit;
 	u32 quirks;
 };
 
@@ -233,6 +239,20 @@ static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
 	return ret;
 }
 
+static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en)
+{
+	const u32 mask_val = BIT(wdt->drv_data->cnt_en_bit);
+	const u32 val = en ? mask_val : 0;
+	int ret;
+
+	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->cnt_en_reg,
+				 mask_val, val);
+	if (ret < 0)
+		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
+
+	return ret;
+}
+
 static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask)
 {
 	int ret;
@@ -249,6 +269,12 @@ static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask)
 			return ret;
 	}
 
+	if (wdt->drv_data->quirks & QUIRK_HAS_PMU_CNT_EN) {
+		ret = s3c2410wdt_enable_counter(wdt, !mask);
+		if (ret < 0)
+			return ret;
+	}
+
 	return 0;
 }
 
-- 
2.30.2

Re: [PATCH v2 08/12] watchdog: s3c2410: Add support for WDT counter enable register

[Krzysztof Kozlowski]

On 31/10/2021 13:22, Sam Protsenko wrote:
> On new Exynos chips (e.g. Exynos850) new CLUSTERx_NONCPU_OUT register is
> introduced, where CNT_EN_WDT bit must be enabled to make watchdog
> counter running. Add corresponding quirk and proper infrastructure to
> handle that register if the quirk is set.
> 
> This commit doesn't bring any functional change to existing devices, but
> merely provides an infrastructure for upcoming chips support.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - Used quirks instead of callbacks for all added PMU registers
>   - Used BIT() macro
>   - Extracted cleanup code to separate patch to minimize changes and
>     ease the review and porting
> 
>  drivers/watchdog/s3c2410_wdt.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

[PATCH v2 09/12] watchdog: s3c2410: Cleanup PMU related code

[Sam Protsenko]

Now that PMU enablement code was extended for new Exynos SoCs, it
doesn't look very cohesive and consistent anymore. Do a bit of renaming,
grouping and style changes, to make it look good again.

No functional change, just a refactoring commit.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - (none): it's a new patch

 drivers/watchdog/s3c2410_wdt.c | 48 ++++++++++++++++------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index ec341c876225..fdb1a1e9bd04 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -56,17 +56,16 @@
 #define EXYNOS5_RST_STAT_REG_OFFSET		0x0404
 #define EXYNOS5_WDT_DISABLE_REG_OFFSET		0x0408
 #define EXYNOS5_WDT_MASK_RESET_REG_OFFSET	0x040c
-#define QUIRK_HAS_PMU_CONFIG			(1 << 0)
-#define QUIRK_HAS_RST_STAT			(1 << 1)
-#define QUIRK_HAS_WTCLRINT_REG			(1 << 2)
+#define QUIRK_HAS_WTCLRINT_REG			(1 << 0)
+#define QUIRK_HAS_PMU_MASK_RESET		(1 << 1)
+#define QUIRK_HAS_PMU_RST_STAT			(1 << 2)
 #define QUIRK_HAS_PMU_AUTO_DISABLE		(1 << 3)
 #define QUIRK_HAS_PMU_CNT_EN			(1 << 4)
 
 /* These quirks require that we have a PMU register map */
-#define QUIRKS_HAVE_PMUREG			(QUIRK_HAS_PMU_CONFIG | \
-						 QUIRK_HAS_RST_STAT | \
-						 QUIRK_HAS_PMU_AUTO_DISABLE | \
-						 QUIRK_HAS_PMU_CNT_EN)
+#define QUIRKS_HAVE_PMUREG \
+	(QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_RST_STAT | \
+	 QUIRK_HAS_PMU_AUTO_DISABLE | QUIRK_HAS_PMU_CNT_EN)
 
 static bool nowayout	= WATCHDOG_NOWAYOUT;
 static int tmr_margin;
@@ -146,8 +145,8 @@ static const struct s3c2410_wdt_variant drv_data_exynos5250  = {
 	.mask_bit = 20,
 	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
 	.rst_stat_bit = 20,
-	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
-		  | QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_AUTO_DISABLE,
+	.quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | \
+		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE,
 };
 
 static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
@@ -156,8 +155,8 @@ static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
 	.mask_bit = 0,
 	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
 	.rst_stat_bit = 9,
-	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
-		  | QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_AUTO_DISABLE,
+	.quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | \
+		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE,
 };
 
 static const struct s3c2410_wdt_variant drv_data_exynos7 = {
@@ -166,8 +165,8 @@ static const struct s3c2410_wdt_variant drv_data_exynos7 = {
 	.mask_bit = 23,
 	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
 	.rst_stat_bit = 23,	/* A57 WDTRESET */
-	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \
-		  | QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_AUTO_DISABLE,
+	.quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | \
+		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE,
 };
 
 static const struct of_device_id s3c2410_wdt_match[] = {
@@ -253,24 +252,24 @@ static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en)
 	return ret;
 }
 
-static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask)
+static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en)
 {
 	int ret;
 
 	if (wdt->drv_data->quirks & QUIRK_HAS_PMU_AUTO_DISABLE) {
-		ret = s3c2410wdt_disable_wdt_reset(wdt, mask);
+		ret = s3c2410wdt_disable_wdt_reset(wdt, !en);
 		if (ret < 0)
 			return ret;
 	}
 
-	if (wdt->drv_data->quirks & QUIRK_HAS_PMU_CONFIG) {
-		ret = s3c2410wdt_mask_wdt_reset(wdt, mask);
+	if (wdt->drv_data->quirks & QUIRK_HAS_PMU_MASK_RESET) {
+		ret = s3c2410wdt_mask_wdt_reset(wdt, !en);
 		if (ret < 0)
 			return ret;
 	}
 
 	if (wdt->drv_data->quirks & QUIRK_HAS_PMU_CNT_EN) {
-		ret = s3c2410wdt_enable_counter(wdt, !mask);
+		ret = s3c2410wdt_enable_counter(wdt, en);
 		if (ret < 0)
 			return ret;
 	}
@@ -531,7 +530,7 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
 	unsigned int rst_stat;
 	int ret;
 
-	if (!(wdt->drv_data->quirks & QUIRK_HAS_RST_STAT))
+	if (!(wdt->drv_data->quirks & QUIRK_HAS_PMU_RST_STAT))
 		return 0;
 
 	ret = regmap_read(wdt->pmureg, wdt->drv_data->rst_stat_reg, &rst_stat);
@@ -672,7 +671,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_cpufreq;
 
-	ret = s3c2410wdt_mask_and_disable_reset(wdt, false);
+	ret = s3c2410wdt_enable(wdt, true);
 	if (ret < 0)
 		goto err_unregister;
 
@@ -707,7 +706,7 @@ static int s3c2410wdt_remove(struct platform_device *dev)
 	int ret;
 	struct s3c2410_wdt *wdt = platform_get_drvdata(dev);
 
-	ret = s3c2410wdt_mask_and_disable_reset(wdt, true);
+	ret = s3c2410wdt_enable(wdt, false);
 	if (ret < 0)
 		return ret;
 
@@ -724,8 +723,7 @@ static void s3c2410wdt_shutdown(struct platform_device *dev)
 {
 	struct s3c2410_wdt *wdt = platform_get_drvdata(dev);
 
-	s3c2410wdt_mask_and_disable_reset(wdt, true);
-
+	s3c2410wdt_enable(wdt, false);
 	s3c2410wdt_stop(&wdt->wdt_device);
 }
 
@@ -740,7 +738,7 @@ static int s3c2410wdt_suspend(struct device *dev)
 	wdt->wtcon_save = readl(wdt->reg_base + S3C2410_WTCON);
 	wdt->wtdat_save = readl(wdt->reg_base + S3C2410_WTDAT);
 
-	ret = s3c2410wdt_mask_and_disable_reset(wdt, true);
+	ret = s3c2410wdt_enable(wdt, false);
 	if (ret < 0)
 		return ret;
 
@@ -760,7 +758,7 @@ static int s3c2410wdt_resume(struct device *dev)
 	writel(wdt->wtdat_save, wdt->reg_base + S3C2410_WTCNT);/* Reset count */
 	writel(wdt->wtcon_save, wdt->reg_base + S3C2410_WTCON);
 
-	ret = s3c2410wdt_mask_and_disable_reset(wdt, false);
+	ret = s3c2410wdt_enable(wdt, true);
 	if (ret < 0)
 		return ret;
 
-- 
2.30.2

Re: [PATCH v2 09/12] watchdog: s3c2410: Cleanup PMU related code

[Krzysztof Kozlowski]

On 31/10/2021 13:22, Sam Protsenko wrote:
> Now that PMU enablement code was extended for new Exynos SoCs, it
> doesn't look very cohesive and consistent anymore. Do a bit of renaming,
> grouping and style changes, to make it look good again.
> 
> No functional change, just a refactoring commit.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - (none): it's a new patch
> 
>  drivers/watchdog/s3c2410_wdt.c | 48 ++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index ec341c876225..fdb1a1e9bd04 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -56,17 +56,16 @@
>  #define EXYNOS5_RST_STAT_REG_OFFSET		0x0404
>  #define EXYNOS5_WDT_DISABLE_REG_OFFSET		0x0408
>  #define EXYNOS5_WDT_MASK_RESET_REG_OFFSET	0x040c

Please document the meaning of each quirk with 1-2 sentences. Could be
in this commit (e.g. "Watchdog reset requests should be unmasked in
PMU", "Watchdog interrupt needs acking").

In general looks good:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

[PATCH v2 10/12] watchdog: s3c2410: Support separate source clock

[Sam Protsenko]

Right now all devices supported in the driver have the single clock: it
acts simultaneously as a bus clock (providing register interface
clocking) and source clock (driving watchdog counter). Some newer Exynos
chips, like Exynos850, have two separate clocks for that. In that case
two clocks will be passed to the driver from the resource provider, e.g.
Device Tree. Provide necessary infrastructure to support that case:
  - use source clock's rate for all timer related calculations
  - use bus clock to gate/ungate the register interface

All devices that use the single clock are kept intact: if only one clock
is passed from Device Tree, it will be used for both purposes as before.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - Reworded commit message to be more formal
  - Used separate "has_src_clk" trait to tell if source clock is present
  - Renamed clock variables to match their purpose
  - Removed caching source clock rate, obtaining it in place each time instead
  - Renamed err labels for more consistency

 drivers/watchdog/s3c2410_wdt.c | 52 +++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index fdb1a1e9bd04..c733917c5470 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -118,7 +118,9 @@ struct s3c2410_wdt_variant {
 
 struct s3c2410_wdt {
 	struct device		*dev;
-	struct clk		*clock;
+	struct clk		*bus_clk; /* for register interface (PCLK) */
+	struct clk		*src_clk; /* for WDT counter */
+	bool			has_src_clk;
 	void __iomem		*reg_base;
 	unsigned int		count;
 	spinlock_t		lock;
@@ -348,7 +350,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
 				    unsigned int timeout)
 {
 	struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
-	unsigned long freq = clk_get_rate(wdt->clock);
+	unsigned long freq = clk_get_rate(wdt->src_clk);
 	unsigned int count;
 	unsigned int divisor = 1;
 	unsigned long wtcon;
@@ -597,26 +599,43 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	wdt->clock = devm_clk_get(dev, "watchdog");
-	if (IS_ERR(wdt->clock)) {
-		dev_err(dev, "failed to find watchdog clock source\n");
-		ret = PTR_ERR(wdt->clock);
+	wdt->bus_clk = devm_clk_get(dev, "watchdog");
+	if (IS_ERR(wdt->bus_clk)) {
+		dev_err(dev, "failed to find bus clock\n");
+		ret = PTR_ERR(wdt->bus_clk);
 		goto err;
 	}
 
-	ret = clk_prepare_enable(wdt->clock);
+	ret = clk_prepare_enable(wdt->bus_clk);
 	if (ret < 0) {
-		dev_err(dev, "failed to enable clock\n");
+		dev_err(dev, "failed to enable bus clock\n");
 		return ret;
 	}
 
+	/*
+	 * "watchdog_src" clock is optional; if it's not present -- just skip it
+	 * and use "watchdog" clock as both bus and source clock.
+	 */
+	wdt->src_clk = devm_clk_get(dev, "watchdog_src");
+	if (!IS_ERR(wdt->src_clk)) {
+		wdt->has_src_clk = true;
+		ret = clk_prepare_enable(wdt->src_clk);
+		if (ret < 0) {
+			dev_err(dev, "failed to enable source clock\n");
+			ret = PTR_ERR(wdt->src_clk);
+			goto err_bus_clk;
+		}
+	} else {
+		wdt->src_clk = wdt->bus_clk;
+	}
+
 	wdt->wdt_device.min_timeout = 1;
-	wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock);
+	wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->src_clk);
 
 	ret = s3c2410wdt_cpufreq_register(wdt);
 	if (ret < 0) {
 		dev_err(dev, "failed to register cpufreq\n");
-		goto err_clk;
+		goto err_src_clk;
 	}
 
 	watchdog_set_drvdata(&wdt->wdt_device, wdt);
@@ -694,8 +713,12 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
  err_cpufreq:
 	s3c2410wdt_cpufreq_deregister(wdt);
 
- err_clk:
-	clk_disable_unprepare(wdt->clock);
+ err_src_clk:
+	if (wdt->has_src_clk)
+		clk_disable_unprepare(wdt->src_clk);
+
+ err_bus_clk:
+	clk_disable_unprepare(wdt->bus_clk);
 
  err:
 	return ret;
@@ -714,7 +737,10 @@ static int s3c2410wdt_remove(struct platform_device *dev)
 
 	s3c2410wdt_cpufreq_deregister(wdt);
 
-	clk_disable_unprepare(wdt->clock);
+	if (wdt->has_src_clk)
+		clk_disable_unprepare(wdt->src_clk);
+
+	clk_disable_unprepare(wdt->bus_clk);
 
 	return 0;
 }
-- 
2.30.2

Re: [PATCH v2 10/12] watchdog: s3c2410: Support separate source clock

[Krzysztof Kozlowski]

On 31/10/2021 13:22, Sam Protsenko wrote:
> Right now all devices supported in the driver have the single clock: it
> acts simultaneously as a bus clock (providing register interface
> clocking) and source clock (driving watchdog counter). Some newer Exynos
> chips, like Exynos850, have two separate clocks for that. In that case
> two clocks will be passed to the driver from the resource provider, e.g.
> Device Tree. Provide necessary infrastructure to support that case:
>   - use source clock's rate for all timer related calculations
>   - use bus clock to gate/ungate the register interface
> 
> All devices that use the single clock are kept intact: if only one clock
> is passed from Device Tree, it will be used for both purposes as before.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - Reworded commit message to be more formal
>   - Used separate "has_src_clk" trait to tell if source clock is present
>   - Renamed clock variables to match their purpose
>   - Removed caching source clock rate, obtaining it in place each time instead
>   - Renamed err labels for more consistency
> 
>  drivers/watchdog/s3c2410_wdt.c | 52 +++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index fdb1a1e9bd04..c733917c5470 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -118,7 +118,9 @@ struct s3c2410_wdt_variant {
>  
>  struct s3c2410_wdt {
>  	struct device		*dev;
> -	struct clk		*clock;
> +	struct clk		*bus_clk; /* for register interface (PCLK) */
> +	struct clk		*src_clk; /* for WDT counter */
> +	bool			has_src_clk;

Why do you need the has_src_clk value? If clk_get() fails, just store
there NULL and clk API will handle it.

Best regards,
Krzysztof

Re: [PATCH v2 10/12] watchdog: s3c2410: Support separate source clock

[Sam Protsenko]

On Tue, 2 Nov 2021 at 12:15, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 31/10/2021 13:22, Sam Protsenko wrote:
> > Right now all devices supported in the driver have the single clock: it
> > acts simultaneously as a bus clock (providing register interface
> > clocking) and source clock (driving watchdog counter). Some newer Exynos
> > chips, like Exynos850, have two separate clocks for that. In that case
> > two clocks will be passed to the driver from the resource provider, e.g.
> > Device Tree. Provide necessary infrastructure to support that case:
> >   - use source clock's rate for all timer related calculations
> >   - use bus clock to gate/ungate the register interface
> >
> > All devices that use the single clock are kept intact: if only one clock
> > is passed from Device Tree, it will be used for both purposes as before.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> > Changes in v2:
> >   - Reworded commit message to be more formal
> >   - Used separate "has_src_clk" trait to tell if source clock is present
> >   - Renamed clock variables to match their purpose
> >   - Removed caching source clock rate, obtaining it in place each time instead
> >   - Renamed err labels for more consistency
> >
> >  drivers/watchdog/s3c2410_wdt.c | 52 +++++++++++++++++++++++++---------
> >  1 file changed, 39 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > index fdb1a1e9bd04..c733917c5470 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -118,7 +118,9 @@ struct s3c2410_wdt_variant {
> >
> >  struct s3c2410_wdt {
> >       struct device           *dev;
> > -     struct clk              *clock;
> > +     struct clk              *bus_clk; /* for register interface (PCLK) */
> > +     struct clk              *src_clk; /* for WDT counter */
> > +     bool                    has_src_clk;
>
> Why do you need the has_src_clk value? If clk_get() fails, just store
> there NULL and clk API will handle it.
>

I've added that 'has_src_clk' field for somewhat different reason.

1. As we discussed earlier, in case when src_clk is not present, I do
'src_clk = bus_clk' in v2. This way I don't have to check if src_clk
is NULL every time and use bus_clk to get rate. If I set src_clk =
NULL, I'll have to add selector code, which wouldn't be so elegant,
and contradicts what we've discussed.
2. On the other hand, I don't want to enable bus_clk twice in case
when src_clk is not present, that just doesn't feel right (user would
see incorrect refcount value in DebugFS, etc). And if "clk_src =
bus_clk', and I haven't enabled it second time, then I shouldn't try
to disable it second time, right?

The only way I can see to accomplish (1) and (2) together, is to use
that 'has_src_clk' flag. For me it still looks better than enabling
bus_clk twice, or checking if clk_src is NULL every time we need to
get clock rate. Please let me know if you still have a strong opinion
on this one.

> Best regards,
> Krzysztof

Re: [PATCH v2 10/12] watchdog: s3c2410: Support separate source clock

[Guenter Roeck]

On 11/7/21 7:55 AM, Sam Protsenko wrote:
> On Tue, 2 Nov 2021 at 12:15, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> On 31/10/2021 13:22, Sam Protsenko wrote:
>>> Right now all devices supported in the driver have the single clock: it
>>> acts simultaneously as a bus clock (providing register interface
>>> clocking) and source clock (driving watchdog counter). Some newer Exynos
>>> chips, like Exynos850, have two separate clocks for that. In that case
>>> two clocks will be passed to the driver from the resource provider, e.g.
>>> Device Tree. Provide necessary infrastructure to support that case:
>>>    - use source clock's rate for all timer related calculations
>>>    - use bus clock to gate/ungate the register interface
>>>
>>> All devices that use the single clock are kept intact: if only one clock
>>> is passed from Device Tree, it will be used for both purposes as before.
>>>
>>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>>> ---
>>> Changes in v2:
>>>    - Reworded commit message to be more formal
>>>    - Used separate "has_src_clk" trait to tell if source clock is present
>>>    - Renamed clock variables to match their purpose
>>>    - Removed caching source clock rate, obtaining it in place each time instead
>>>    - Renamed err labels for more consistency
>>>
>>>   drivers/watchdog/s3c2410_wdt.c | 52 +++++++++++++++++++++++++---------
>>>   1 file changed, 39 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
>>> index fdb1a1e9bd04..c733917c5470 100644
>>> --- a/drivers/watchdog/s3c2410_wdt.c
>>> +++ b/drivers/watchdog/s3c2410_wdt.c
>>> @@ -118,7 +118,9 @@ struct s3c2410_wdt_variant {
>>>
>>>   struct s3c2410_wdt {
>>>        struct device           *dev;
>>> -     struct clk              *clock;
>>> +     struct clk              *bus_clk; /* for register interface (PCLK) */
>>> +     struct clk              *src_clk; /* for WDT counter */
>>> +     bool                    has_src_clk;
>>
>> Why do you need the has_src_clk value? If clk_get() fails, just store
>> there NULL and clk API will handle it.
>>
> 
> I've added that 'has_src_clk' field for somewhat different reason.
> 
> 1. As we discussed earlier, in case when src_clk is not present, I do
> 'src_clk = bus_clk' in v2. This way I don't have to check if src_clk
> is NULL every time and use bus_clk to get rate. If I set src_clk =
> NULL, I'll have to add selector code, which wouldn't be so elegant,
> and contradicts what we've discussed.
> 2. On the other hand, I don't want to enable bus_clk twice in case
> when src_clk is not present, that just doesn't feel right (user would
> see incorrect refcount value in DebugFS, etc). And if "clk_src =
> bus_clk', and I haven't enabled it second time, then I shouldn't try
> to disable it second time, right?
> 
> The only way I can see to accomplish (1) and (2) together, is to use
> that 'has_src_clk' flag. For me it still looks better than enabling
> bus_clk twice, or checking if clk_src is NULL every time we need to
> get clock rate. Please let me know if you still have a strong opinion
> on this one.
> 

This is odd. Why do you need to get the clock rate more than once,
instead of getting it once and storing it locally ? If the clock rate
can change, the watchdog timeout would be unpredictable.

Guenter

Re: [PATCH v2 10/12] watchdog: s3c2410: Support separate source clock

[Sam Protsenko]

On Sun, 7 Nov 2021 at 18:09, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 11/7/21 7:55 AM, Sam Protsenko wrote:
> > On Tue, 2 Nov 2021 at 12:15, Krzysztof Kozlowski
> > <krzysztof.kozlowski@canonical.com> wrote:
> >>
> >> On 31/10/2021 13:22, Sam Protsenko wrote:
> >>> Right now all devices supported in the driver have the single clock: it
> >>> acts simultaneously as a bus clock (providing register interface
> >>> clocking) and source clock (driving watchdog counter). Some newer Exynos
> >>> chips, like Exynos850, have two separate clocks for that. In that case
> >>> two clocks will be passed to the driver from the resource provider, e.g.
> >>> Device Tree. Provide necessary infrastructure to support that case:
> >>>    - use source clock's rate for all timer related calculations
> >>>    - use bus clock to gate/ungate the register interface
> >>>
> >>> All devices that use the single clock are kept intact: if only one clock
> >>> is passed from Device Tree, it will be used for both purposes as before.
> >>>
> >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> >>> ---
> >>> Changes in v2:
> >>>    - Reworded commit message to be more formal
> >>>    - Used separate "has_src_clk" trait to tell if source clock is present
> >>>    - Renamed clock variables to match their purpose
> >>>    - Removed caching source clock rate, obtaining it in place each time instead
> >>>    - Renamed err labels for more consistency
> >>>
> >>>   drivers/watchdog/s3c2410_wdt.c | 52 +++++++++++++++++++++++++---------
> >>>   1 file changed, 39 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> >>> index fdb1a1e9bd04..c733917c5470 100644
> >>> --- a/drivers/watchdog/s3c2410_wdt.c
> >>> +++ b/drivers/watchdog/s3c2410_wdt.c
> >>> @@ -118,7 +118,9 @@ struct s3c2410_wdt_variant {
> >>>
> >>>   struct s3c2410_wdt {
> >>>        struct device           *dev;
> >>> -     struct clk              *clock;
> >>> +     struct clk              *bus_clk; /* for register interface (PCLK) */
> >>> +     struct clk              *src_clk; /* for WDT counter */
> >>> +     bool                    has_src_clk;
> >>
> >> Why do you need the has_src_clk value? If clk_get() fails, just store
> >> there NULL and clk API will handle it.
> >>
> >
> > I've added that 'has_src_clk' field for somewhat different reason.
> >
> > 1. As we discussed earlier, in case when src_clk is not present, I do
> > 'src_clk = bus_clk' in v2. This way I don't have to check if src_clk
> > is NULL every time and use bus_clk to get rate. If I set src_clk =
> > NULL, I'll have to add selector code, which wouldn't be so elegant,
> > and contradicts what we've discussed.
> > 2. On the other hand, I don't want to enable bus_clk twice in case
> > when src_clk is not present, that just doesn't feel right (user would
> > see incorrect refcount value in DebugFS, etc). And if "clk_src =
> > bus_clk', and I haven't enabled it second time, then I shouldn't try
> > to disable it second time, right?
> >
> > The only way I can see to accomplish (1) and (2) together, is to use
> > that 'has_src_clk' flag. For me it still looks better than enabling
> > bus_clk twice, or checking if clk_src is NULL every time we need to
> > get clock rate. Please let me know if you still have a strong opinion
> > on this one.
> >
>
> This is odd. Why do you need to get the clock rate more than once,
> instead of getting it once and storing it locally ? If the clock rate
> can change, the watchdog timeout would be unpredictable.
>

For this particular driver it seems to be needed though. The driver
tracks CPU freq change and tries to adjust timeout w.r.t. new clock
frequency, as implemented in s3c2410wdt_cpufreq_*() functions. I don't
really want to touch that functionality in my series, my goal here is
to add Exynos850 support in the first place. But yeah, I did some
analysis, and it seems like 25 out of 35 watchdog drivers (that call
clk_get_rate()) just store the rate in probe.

> Guenter

Re: [PATCH v2 10/12] watchdog: s3c2410: Support separate source clock

[Guenter Roeck]

On 11/7/21 10:51 AM, Sam Protsenko wrote:
> On Sun, 7 Nov 2021 at 18:09, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 11/7/21 7:55 AM, Sam Protsenko wrote:
>>> On Tue, 2 Nov 2021 at 12:15, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@canonical.com> wrote:
>>>>
>>>> On 31/10/2021 13:22, Sam Protsenko wrote:
>>>>> Right now all devices supported in the driver have the single clock: it
>>>>> acts simultaneously as a bus clock (providing register interface
>>>>> clocking) and source clock (driving watchdog counter). Some newer Exynos
>>>>> chips, like Exynos850, have two separate clocks for that. In that case
>>>>> two clocks will be passed to the driver from the resource provider, e.g.
>>>>> Device Tree. Provide necessary infrastructure to support that case:
>>>>>     - use source clock's rate for all timer related calculations
>>>>>     - use bus clock to gate/ungate the register interface
>>>>>
>>>>> All devices that use the single clock are kept intact: if only one clock
>>>>> is passed from Device Tree, it will be used for both purposes as before.
>>>>>
>>>>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>>>>> ---
>>>>> Changes in v2:
>>>>>     - Reworded commit message to be more formal
>>>>>     - Used separate "has_src_clk" trait to tell if source clock is present
>>>>>     - Renamed clock variables to match their purpose
>>>>>     - Removed caching source clock rate, obtaining it in place each time instead
>>>>>     - Renamed err labels for more consistency
>>>>>
>>>>>    drivers/watchdog/s3c2410_wdt.c | 52 +++++++++++++++++++++++++---------
>>>>>    1 file changed, 39 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
>>>>> index fdb1a1e9bd04..c733917c5470 100644
>>>>> --- a/drivers/watchdog/s3c2410_wdt.c
>>>>> +++ b/drivers/watchdog/s3c2410_wdt.c
>>>>> @@ -118,7 +118,9 @@ struct s3c2410_wdt_variant {
>>>>>
>>>>>    struct s3c2410_wdt {
>>>>>         struct device           *dev;
>>>>> -     struct clk              *clock;
>>>>> +     struct clk              *bus_clk; /* for register interface (PCLK) */
>>>>> +     struct clk              *src_clk; /* for WDT counter */
>>>>> +     bool                    has_src_clk;
>>>>
>>>> Why do you need the has_src_clk value? If clk_get() fails, just store
>>>> there NULL and clk API will handle it.
>>>>
>>>
>>> I've added that 'has_src_clk' field for somewhat different reason.
>>>
>>> 1. As we discussed earlier, in case when src_clk is not present, I do
>>> 'src_clk = bus_clk' in v2. This way I don't have to check if src_clk
>>> is NULL every time and use bus_clk to get rate. If I set src_clk =
>>> NULL, I'll have to add selector code, which wouldn't be so elegant,
>>> and contradicts what we've discussed.
>>> 2. On the other hand, I don't want to enable bus_clk twice in case
>>> when src_clk is not present, that just doesn't feel right (user would
>>> see incorrect refcount value in DebugFS, etc). And if "clk_src =
>>> bus_clk', and I haven't enabled it second time, then I shouldn't try
>>> to disable it second time, right?
>>>
>>> The only way I can see to accomplish (1) and (2) together, is to use
>>> that 'has_src_clk' flag. For me it still looks better than enabling
>>> bus_clk twice, or checking if clk_src is NULL every time we need to
>>> get clock rate. Please let me know if you still have a strong opinion
>>> on this one.
>>>
>>
>> This is odd. Why do you need to get the clock rate more than once,
>> instead of getting it once and storing it locally ? If the clock rate
>> can change, the watchdog timeout would be unpredictable.
>>
> 
> For this particular driver it seems to be needed though. The driver
> tracks CPU freq change and tries to adjust timeout w.r.t. new clock
> frequency, as implemented in s3c2410wdt_cpufreq_*() functions. I don't
> really want to touch that functionality in my series, my goal here is
> to add Exynos850 support in the first place. But yeah, I did some
> analysis, and it seems like 25 out of 35 watchdog drivers (that call
> clk_get_rate()) just store the rate in probe.
> 
... and (hopefully) most of the others don't really need to call
clk_get_rate()) more than once either. Looks like the watchdog in
s3c2410 is using the wrong clock. That makes me wonder if it even
really works reliably, given that it turns itself off for a brief
period of time at each CPU frequency change. Oh well, it is what it is.

Guenter

[PATCH v2 11/12] watchdog: s3c2410: Remove superfluous err label

[Sam Protsenko]

'err' label in probe function is not really need, it just returns.
Remove it and replace all 'goto' statements with actual returns in
place.

No functional change here, just a cleanup patch.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - (none): it's a new patch

 drivers/watchdog/s3c2410_wdt.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index c733917c5470..8fdda2ede1c3 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -588,22 +588,18 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (wdt_irq == NULL) {
 		dev_err(dev, "no irq resource specified\n");
-		ret = -ENOENT;
-		goto err;
+		return -ENOENT;
 	}
 
 	/* get the memory region for the watchdog timer */
 	wdt->reg_base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(wdt->reg_base)) {
-		ret = PTR_ERR(wdt->reg_base);
-		goto err;
-	}
+	if (IS_ERR(wdt->reg_base))
+		return PTR_ERR(wdt->reg_base);
 
 	wdt->bus_clk = devm_clk_get(dev, "watchdog");
 	if (IS_ERR(wdt->bus_clk)) {
 		dev_err(dev, "failed to find bus clock\n");
-		ret = PTR_ERR(wdt->bus_clk);
-		goto err;
+		return PTR_ERR(wdt->bus_clk);
 	}
 
 	ret = clk_prepare_enable(wdt->bus_clk);
@@ -720,7 +716,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
  err_bus_clk:
 	clk_disable_unprepare(wdt->bus_clk);
 
- err:
 	return ret;
 }
 
-- 
2.30.2

Re: [PATCH v2 11/12] watchdog: s3c2410: Remove superfluous err label

[Krzysztof Kozlowski]

On 31/10/2021 13:22, Sam Protsenko wrote:
> 'err' label in probe function is not really need, it just returns.
> Remove it and replace all 'goto' statements with actual returns in
> place.
> 
> No functional change here, just a cleanup patch.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - (none): it's a new patch
> 
>  drivers/watchdog/s3c2410_wdt.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

[PATCH v2 12/12] watchdog: s3c2410: Add Exynos850 support

[Sam Protsenko]

Exynos850 is a bit different from SoCs already supported in WDT driver:
  - AUTOMATIC_WDT_RESET_DISABLE register is removed, so its value is
    always 0; .disable_auto_reset callback is not set for that reason
  - MASK_WDT_RESET_REQUEST register is replaced with
    CLUSTERx_NONCPU_IN_EN register; instead of masking (disabling) WDT
    reset interrupt it's now enabled with the same value; .mask_reset
    callback is reused for that functionality though
  - To make WDT functional, WDT counter needs to be enabled in
    CLUSTERx_NONCPU_OUT register; it's done using .enable_counter
    callback

Also Exynos850 has two CPU clusters, each has its own dedicated WDT
instance. Different PMU registers and bits are used for each cluster. So
driver data is now modified in probe, adding needed info depending on
cluster index passed from device tree.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - Used single compatible for Exynos850, populating missing driver data in
    probe
  - Added "index" property to specify CPU cluster index

 drivers/watchdog/s3c2410_wdt.c | 68 +++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 8fdda2ede1c3..457b725c30ac 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -56,6 +56,14 @@
 #define EXYNOS5_RST_STAT_REG_OFFSET		0x0404
 #define EXYNOS5_WDT_DISABLE_REG_OFFSET		0x0408
 #define EXYNOS5_WDT_MASK_RESET_REG_OFFSET	0x040c
+#define EXYNOS850_CLUSTER0_NONCPU_OUT		0x1220
+#define EXYNOS850_CLUSTER0_NONCPU_INT_EN	0x1244
+#define EXYNOS850_CLUSTER1_NONCPU_OUT		0x1620
+#define EXYNOS850_CLUSTER1_NONCPU_INT_EN	0x1644
+
+#define EXYNOS850_CLUSTER0_WDTRESET_BIT		24
+#define EXYNOS850_CLUSTER1_WDTRESET_BIT		23
+
 #define QUIRK_HAS_WTCLRINT_REG			(1 << 0)
 #define QUIRK_HAS_PMU_MASK_RESET		(1 << 1)
 #define QUIRK_HAS_PMU_RST_STAT			(1 << 2)
@@ -171,6 +179,21 @@ static const struct s3c2410_wdt_variant drv_data_exynos7 = {
 		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE,
 };
 
+static const struct s3c2410_wdt_variant drv_data_exynos850 = {
+	/*
+	 * Next fields will be set in probe(), based on cluster index:
+	 *   - .mask_reset_reg
+	 *   - .rst_stat_bit
+	 *   - .cnt_en_reg
+	 */
+	.mask_reset_inv = true,
+	.mask_bit = 2,
+	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
+	.cnt_en_bit = 7,
+	.quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | \
+		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN,
+};
+
 static const struct of_device_id s3c2410_wdt_match[] = {
 	{ .compatible = "samsung,s3c2410-wdt",
 	  .data = &drv_data_s3c2410 },
@@ -182,6 +205,8 @@ static const struct of_device_id s3c2410_wdt_match[] = {
 	  .data = &drv_data_exynos5420 },
 	{ .compatible = "samsung,exynos7-wdt",
 	  .data = &drv_data_exynos7 },
+	{ .compatible = "samsung,exynos850-wdt",
+	  .data = &drv_data_exynos850 },
 	{},
 };
 MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
@@ -548,15 +573,51 @@ static inline const struct s3c2410_wdt_variant *
 s3c2410_get_wdt_drv_data(struct platform_device *pdev)
 {
 	const struct s3c2410_wdt_variant *variant;
+	struct s3c2410_wdt_variant *data;
+	struct device *dev = &pdev->dev;
 
-	variant = of_device_get_match_data(&pdev->dev);
+	variant = of_device_get_match_data(dev);
 	if (!variant) {
 		/* Device matched by platform_device_id */
 		variant = (struct s3c2410_wdt_variant *)
 			   platform_get_device_id(pdev)->driver_data;
 	}
 
-	return variant;
+	/* Have to copy driver data over to keep its const qualifier intact */
+	data = devm_kmemdup(dev, variant, sizeof(*variant), GFP_KERNEL);
+	if (!data)
+		return NULL;
+
+	/* Populate missing fields for Exynos850 w.r.t. cluster index */
+	if (variant == &drv_data_exynos850) {
+		u32 index;
+		int err;
+
+		err = of_property_read_u32(dev->of_node, "samsung,index",
+					   &index);
+		if (err) {
+			dev_err(dev, "failed to get cluster index\n");
+			return NULL;
+		}
+
+		switch (index) {
+		case 0:
+			data->mask_reset_reg = EXYNOS850_CLUSTER0_NONCPU_INT_EN;
+			data->rst_stat_bit = EXYNOS850_CLUSTER0_WDTRESET_BIT;
+			data->cnt_en_reg = EXYNOS850_CLUSTER0_NONCPU_OUT;
+			break;
+		case 1:
+			data->mask_reset_reg = EXYNOS850_CLUSTER1_NONCPU_INT_EN;
+			data->rst_stat_bit = EXYNOS850_CLUSTER1_WDTRESET_BIT;
+			data->cnt_en_reg = EXYNOS850_CLUSTER1_NONCPU_OUT;
+			break;
+		default:
+			dev_err(dev, "wrong cluster index: %u\n", index);
+			return NULL;
+		}
+	}
+
+	return data;
 }
 
 static int s3c2410wdt_probe(struct platform_device *pdev)
@@ -576,6 +637,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	wdt->wdt_device = s3c2410_wdd;
 
 	wdt->drv_data = s3c2410_get_wdt_drv_data(pdev);
+	if (!wdt->drv_data)
+		return -EINVAL;
+
 	if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
 		wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
 						"samsung,syscon-phandle");
-- 
2.30.2

Re: [PATCH v2 12/12] watchdog: s3c2410: Add Exynos850 support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3739 bytes --]

Hi Sam,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on groeck-staging/hwmon-next linus/master v5.15-rc7 next-20211029]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sam-Protsenko/watchdog-s3c2410-Add-Exynos850-support/20211031-202352
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: hexagon-randconfig-r045-20211031 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 82ed106567063ea269c6d5669278b733e173a42f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/ff70b806cc12adc068bb54865f81d3aa29a69471
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sam-Protsenko/watchdog-s3c2410-Add-Exynos850-support/20211031-202352
        git checkout ff70b806cc12adc068bb54865f81d3aa29a69471
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/watchdog/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/watchdog/s3c2410_wdt.c:592:18: error: use of undeclared identifier 'drv_data_exynos850'
           if (variant == &drv_data_exynos850) {
                           ^
   1 error generated.


vim +/drv_data_exynos850 +592 drivers/watchdog/s3c2410_wdt.c

   571	
   572	static inline const struct s3c2410_wdt_variant *
   573	s3c2410_get_wdt_drv_data(struct platform_device *pdev)
   574	{
   575		const struct s3c2410_wdt_variant *variant;
   576		struct s3c2410_wdt_variant *data;
   577		struct device *dev = &pdev->dev;
   578	
   579		variant = of_device_get_match_data(dev);
   580		if (!variant) {
   581			/* Device matched by platform_device_id */
   582			variant = (struct s3c2410_wdt_variant *)
   583				   platform_get_device_id(pdev)->driver_data;
   584		}
   585	
   586		/* Have to copy driver data over to keep its const qualifier intact */
   587		data = devm_kmemdup(dev, variant, sizeof(*variant), GFP_KERNEL);
   588		if (!data)
   589			return NULL;
   590	
   591		/* Populate missing fields for Exynos850 w.r.t. cluster index */
 > 592		if (variant == &drv_data_exynos850) {
   593			u32 index;
   594			int err;
   595	
   596			err = of_property_read_u32(dev->of_node, "samsung,index",
   597						   &index);
   598			if (err) {
   599				dev_err(dev, "failed to get cluster index\n");
   600				return NULL;
   601			}
   602	
   603			switch (index) {
   604			case 0:
   605				data->mask_reset_reg = EXYNOS850_CLUSTER0_NONCPU_INT_EN;
   606				data->rst_stat_bit = EXYNOS850_CLUSTER0_WDTRESET_BIT;
   607				data->cnt_en_reg = EXYNOS850_CLUSTER0_NONCPU_OUT;
   608				break;
   609			case 1:
   610				data->mask_reset_reg = EXYNOS850_CLUSTER1_NONCPU_INT_EN;
   611				data->rst_stat_bit = EXYNOS850_CLUSTER1_WDTRESET_BIT;
   612				data->cnt_en_reg = EXYNOS850_CLUSTER1_NONCPU_OUT;
   613				break;
   614			default:
   615				dev_err(dev, "wrong cluster index: %u\n", index);
   616				return NULL;
   617			}
   618		}
   619	
   620		return data;
   621	}
   622	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25423 bytes --]

Re: [PATCH v2 12/12] watchdog: s3c2410: Add Exynos850 support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3893 bytes --]

Hi Sam,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on groeck-staging/hwmon-next linus/master v5.15-rc7 next-20211029]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sam-Protsenko/watchdog-s3c2410-Add-Exynos850-support/20211031-202352
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: xtensa-randconfig-r012-20211031 (attached as .config)
compiler: xtensa-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/ff70b806cc12adc068bb54865f81d3aa29a69471
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sam-Protsenko/watchdog-s3c2410-Add-Exynos850-support/20211031-202352
        git checkout ff70b806cc12adc068bb54865f81d3aa29a69471
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=xtensa SHELL=/bin/bash drivers/watchdog/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/watchdog/s3c2410_wdt.c: In function 's3c2410_get_wdt_drv_data':
>> drivers/watchdog/s3c2410_wdt.c:592:25: error: 'drv_data_exynos850' undeclared (first use in this function)
     592 |         if (variant == &drv_data_exynos850) {
         |                         ^~~~~~~~~~~~~~~~~~
   drivers/watchdog/s3c2410_wdt.c:592:25: note: each undeclared identifier is reported only once for each function it appears in


vim +/drv_data_exynos850 +592 drivers/watchdog/s3c2410_wdt.c

   571	
   572	static inline const struct s3c2410_wdt_variant *
   573	s3c2410_get_wdt_drv_data(struct platform_device *pdev)
   574	{
   575		const struct s3c2410_wdt_variant *variant;
   576		struct s3c2410_wdt_variant *data;
   577		struct device *dev = &pdev->dev;
   578	
   579		variant = of_device_get_match_data(dev);
   580		if (!variant) {
   581			/* Device matched by platform_device_id */
   582			variant = (struct s3c2410_wdt_variant *)
   583				   platform_get_device_id(pdev)->driver_data;
   584		}
   585	
   586		/* Have to copy driver data over to keep its const qualifier intact */
   587		data = devm_kmemdup(dev, variant, sizeof(*variant), GFP_KERNEL);
   588		if (!data)
   589			return NULL;
   590	
   591		/* Populate missing fields for Exynos850 w.r.t. cluster index */
 > 592		if (variant == &drv_data_exynos850) {
   593			u32 index;
   594			int err;
   595	
   596			err = of_property_read_u32(dev->of_node, "samsung,index",
   597						   &index);
   598			if (err) {
   599				dev_err(dev, "failed to get cluster index\n");
   600				return NULL;
   601			}
   602	
   603			switch (index) {
   604			case 0:
   605				data->mask_reset_reg = EXYNOS850_CLUSTER0_NONCPU_INT_EN;
   606				data->rst_stat_bit = EXYNOS850_CLUSTER0_WDTRESET_BIT;
   607				data->cnt_en_reg = EXYNOS850_CLUSTER0_NONCPU_OUT;
   608				break;
   609			case 1:
   610				data->mask_reset_reg = EXYNOS850_CLUSTER1_NONCPU_INT_EN;
   611				data->rst_stat_bit = EXYNOS850_CLUSTER1_WDTRESET_BIT;
   612				data->cnt_en_reg = EXYNOS850_CLUSTER1_NONCPU_OUT;
   613				break;
   614			default:
   615				dev_err(dev, "wrong cluster index: %u\n", index);
   616				return NULL;
   617			}
   618		}
   619	
   620		return data;
   621	}
   622	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31002 bytes --]

Re: [PATCH v2 12/12] watchdog: s3c2410: Add Exynos850 support

[Krzysztof Kozlowski]

On 31/10/2021 13:22, Sam Protsenko wrote:
> Exynos850 is a bit different from SoCs already supported in WDT driver:
>   - AUTOMATIC_WDT_RESET_DISABLE register is removed, so its value is
>     always 0; .disable_auto_reset callback is not set for that reason
>   - MASK_WDT_RESET_REQUEST register is replaced with
>     CLUSTERx_NONCPU_IN_EN register; instead of masking (disabling) WDT
>     reset interrupt it's now enabled with the same value; .mask_reset
>     callback is reused for that functionality though
>   - To make WDT functional, WDT counter needs to be enabled in
>     CLUSTERx_NONCPU_OUT register; it's done using .enable_counter
>     callback
> 
> Also Exynos850 has two CPU clusters, each has its own dedicated WDT
> instance. Different PMU registers and bits are used for each cluster. So
> driver data is now modified in probe, adding needed info depending on
> cluster index passed from device tree.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - Used single compatible for Exynos850, populating missing driver data in
>     probe
>   - Added "index" property to specify CPU cluster index
> 
>  drivers/watchdog/s3c2410_wdt.c | 68 +++++++++++++++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 8fdda2ede1c3..457b725c30ac 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -56,6 +56,14 @@
>  #define EXYNOS5_RST_STAT_REG_OFFSET		0x0404
>  #define EXYNOS5_WDT_DISABLE_REG_OFFSET		0x0408
>  #define EXYNOS5_WDT_MASK_RESET_REG_OFFSET	0x040c
> +#define EXYNOS850_CLUSTER0_NONCPU_OUT		0x1220
> +#define EXYNOS850_CLUSTER0_NONCPU_INT_EN	0x1244
> +#define EXYNOS850_CLUSTER1_NONCPU_OUT		0x1620
> +#define EXYNOS850_CLUSTER1_NONCPU_INT_EN	0x1644
> +
> +#define EXYNOS850_CLUSTER0_WDTRESET_BIT		24
> +#define EXYNOS850_CLUSTER1_WDTRESET_BIT		23
> +
>  #define QUIRK_HAS_WTCLRINT_REG			(1 << 0)
>  #define QUIRK_HAS_PMU_MASK_RESET		(1 << 1)
>  #define QUIRK_HAS_PMU_RST_STAT			(1 << 2)
> @@ -171,6 +179,21 @@ static const struct s3c2410_wdt_variant drv_data_exynos7 = {
>  		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE,
>  };
>  
> +static const struct s3c2410_wdt_variant drv_data_exynos850 = {
> +	/*
> +	 * Next fields will be set in probe(), based on cluster index:
> +	 *   - .mask_reset_reg
> +	 *   - .rst_stat_bit
> +	 *   - .cnt_en_reg
> +	 */
> +	.mask_reset_inv = true,
> +	.mask_bit = 2,
> +	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
> +	.cnt_en_bit = 7,
> +	.quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | \
> +		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN,
> +};
> +
>  static const struct of_device_id s3c2410_wdt_match[] = {
>  	{ .compatible = "samsung,s3c2410-wdt",
>  	  .data = &drv_data_s3c2410 },
> @@ -182,6 +205,8 @@ static const struct of_device_id s3c2410_wdt_match[] = {
>  	  .data = &drv_data_exynos5420 },
>  	{ .compatible = "samsung,exynos7-wdt",
>  	  .data = &drv_data_exynos7 },
> +	{ .compatible = "samsung,exynos850-wdt",
> +	  .data = &drv_data_exynos850 },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
> @@ -548,15 +573,51 @@ static inline const struct s3c2410_wdt_variant *
>  s3c2410_get_wdt_drv_data(struct platform_device *pdev)
>  {
>  	const struct s3c2410_wdt_variant *variant;
> +	struct s3c2410_wdt_variant *data;
> +	struct device *dev = &pdev->dev;
>  
> -	variant = of_device_get_match_data(&pdev->dev);
> +	variant = of_device_get_match_data(dev);
>  	if (!variant) {
>  		/* Device matched by platform_device_id */
>  		variant = (struct s3c2410_wdt_variant *)
>  			   platform_get_device_id(pdev)->driver_data;
>  	}
>  
> -	return variant;
> +	/* Have to copy driver data over to keep its const qualifier intact */
> +	data = devm_kmemdup(dev, variant, sizeof(*variant), GFP_KERNEL);
> +	if (!data)
> +		return NULL;
> +
> +	/* Populate missing fields for Exynos850 w.r.t. cluster index */
> +	if (variant == &drv_data_exynos850) {
> +		u32 index;
> +		int err;

Another approach is to:
1. Define two variants for Exynos850 (s3c2410_wdt_variants), kind of
like before,
2. if (variant == &drv_data_exynos850)
a. Read the index
b. If index is 0, return first variant,
c. If index is 1, return the second variant,
d. Else - NULL.

This way you won't need to copy the memory on the fly, just use
different const data. Benefits: less memory allocations, entire drvdata
set in one place (so nicely visible), drvdata populated safely via const.

> +
> +		err = of_property_read_u32(dev->of_node, "samsung,index",
> +					   &index);
> +		if (err) {
> +			dev_err(dev, "failed to get cluster index\n");
> +			return NULL;
> +		}
> +
> +		switch (index) {
> +		case 0:
> +			data->mask_reset_reg = EXYNOS850_CLUSTER0_NONCPU_INT_EN;
> +			data->rst_stat_bit = EXYNOS850_CLUSTER0_WDTRESET_BIT;
> +			data->cnt_en_reg = EXYNOS850_CLUSTER0_NONCPU_OUT;
> +			break;
> +		case 1:
> +			data->mask_reset_reg = EXYNOS850_CLUSTER1_NONCPU_INT_EN;
> +			data->rst_stat_bit = EXYNOS850_CLUSTER1_WDTRESET_BIT;
> +			data->cnt_en_reg = EXYNOS850_CLUSTER1_NONCPU_OUT;
> +			break;
> +		default:
> +			dev_err(dev, "wrong cluster index: %u\n", index);
> +			return NULL;
> +		}
> +	}
> +
> +	return data;
>  }
>  
>  static int s3c2410wdt_probe(struct platform_device *pdev)
> @@ -576,6 +637,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>  	wdt->wdt_device = s3c2410_wdd;
>  
>  	wdt->drv_data = s3c2410_get_wdt_drv_data(pdev);
> +	if (!wdt->drv_data)
> +		return -EINVAL;
> +
>  	if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
>  		wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
>  						"samsung,syscon-phandle");
> 


Best regards,
Krzysztof

Re: [PATCH v2 12/12] watchdog: s3c2410: Add Exynos850 support

[Sam Protsenko]

On Tue, 2 Nov 2021 at 12:27, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 31/10/2021 13:22, Sam Protsenko wrote:
> > Exynos850 is a bit different from SoCs already supported in WDT driver:
> >   - AUTOMATIC_WDT_RESET_DISABLE register is removed, so its value is
> >     always 0; .disable_auto_reset callback is not set for that reason
> >   - MASK_WDT_RESET_REQUEST register is replaced with
> >     CLUSTERx_NONCPU_IN_EN register; instead of masking (disabling) WDT
> >     reset interrupt it's now enabled with the same value; .mask_reset
> >     callback is reused for that functionality though
> >   - To make WDT functional, WDT counter needs to be enabled in
> >     CLUSTERx_NONCPU_OUT register; it's done using .enable_counter
> >     callback
> >
> > Also Exynos850 has two CPU clusters, each has its own dedicated WDT
> > instance. Different PMU registers and bits are used for each cluster. So
> > driver data is now modified in probe, adding needed info depending on
> > cluster index passed from device tree.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> > Changes in v2:
> >   - Used single compatible for Exynos850, populating missing driver data in
> >     probe
> >   - Added "index" property to specify CPU cluster index
> >
> >  drivers/watchdog/s3c2410_wdt.c | 68 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 66 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > index 8fdda2ede1c3..457b725c30ac 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -56,6 +56,14 @@
> >  #define EXYNOS5_RST_STAT_REG_OFFSET          0x0404
> >  #define EXYNOS5_WDT_DISABLE_REG_OFFSET               0x0408
> >  #define EXYNOS5_WDT_MASK_RESET_REG_OFFSET    0x040c
> > +#define EXYNOS850_CLUSTER0_NONCPU_OUT                0x1220
> > +#define EXYNOS850_CLUSTER0_NONCPU_INT_EN     0x1244
> > +#define EXYNOS850_CLUSTER1_NONCPU_OUT                0x1620
> > +#define EXYNOS850_CLUSTER1_NONCPU_INT_EN     0x1644
> > +
> > +#define EXYNOS850_CLUSTER0_WDTRESET_BIT              24
> > +#define EXYNOS850_CLUSTER1_WDTRESET_BIT              23
> > +
> >  #define QUIRK_HAS_WTCLRINT_REG                       (1 << 0)
> >  #define QUIRK_HAS_PMU_MASK_RESET             (1 << 1)
> >  #define QUIRK_HAS_PMU_RST_STAT                       (1 << 2)
> > @@ -171,6 +179,21 @@ static const struct s3c2410_wdt_variant drv_data_exynos7 = {
> >                 QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE,
> >  };
> >
> > +static const struct s3c2410_wdt_variant drv_data_exynos850 = {
> > +     /*
> > +      * Next fields will be set in probe(), based on cluster index:
> > +      *   - .mask_reset_reg
> > +      *   - .rst_stat_bit
> > +      *   - .cnt_en_reg
> > +      */
> > +     .mask_reset_inv = true,
> > +     .mask_bit = 2,
> > +     .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
> > +     .cnt_en_bit = 7,
> > +     .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | \
> > +               QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN,
> > +};
> > +
> >  static const struct of_device_id s3c2410_wdt_match[] = {
> >       { .compatible = "samsung,s3c2410-wdt",
> >         .data = &drv_data_s3c2410 },
> > @@ -182,6 +205,8 @@ static const struct of_device_id s3c2410_wdt_match[] = {
> >         .data = &drv_data_exynos5420 },
> >       { .compatible = "samsung,exynos7-wdt",
> >         .data = &drv_data_exynos7 },
> > +     { .compatible = "samsung,exynos850-wdt",
> > +       .data = &drv_data_exynos850 },
> >       {},
> >  };
> >  MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
> > @@ -548,15 +573,51 @@ static inline const struct s3c2410_wdt_variant *
> >  s3c2410_get_wdt_drv_data(struct platform_device *pdev)
> >  {
> >       const struct s3c2410_wdt_variant *variant;
> > +     struct s3c2410_wdt_variant *data;
> > +     struct device *dev = &pdev->dev;
> >
> > -     variant = of_device_get_match_data(&pdev->dev);
> > +     variant = of_device_get_match_data(dev);
> >       if (!variant) {
> >               /* Device matched by platform_device_id */
> >               variant = (struct s3c2410_wdt_variant *)
> >                          platform_get_device_id(pdev)->driver_data;
> >       }
> >
> > -     return variant;
> > +     /* Have to copy driver data over to keep its const qualifier intact */
> > +     data = devm_kmemdup(dev, variant, sizeof(*variant), GFP_KERNEL);
> > +     if (!data)
> > +             return NULL;
> > +
> > +     /* Populate missing fields for Exynos850 w.r.t. cluster index */
> > +     if (variant == &drv_data_exynos850) {
> > +             u32 index;
> > +             int err;
>
> Another approach is to:
> 1. Define two variants for Exynos850 (s3c2410_wdt_variants), kind of
> like before,
> 2. if (variant == &drv_data_exynos850)
> a. Read the index
> b. If index is 0, return first variant,
> c. If index is 1, return the second variant,
> d. Else - NULL.
>
> This way you won't need to copy the memory on the fly, just use
> different const data. Benefits: less memory allocations, entire drvdata
> set in one place (so nicely visible), drvdata populated safely via const.
>

That's definitely better. Not sure how I missed that. Anyway, thanks
for review. Will send v3 soon, addressing all your comments (except
the one about src_clk in PATCH 10/12 -- need to discuss it further, I
guess).

> > +
> > +             err = of_property_read_u32(dev->of_node, "samsung,index",
> > +                                        &index);
> > +             if (err) {
> > +                     dev_err(dev, "failed to get cluster index\n");
> > +                     return NULL;
> > +             }
> > +
> > +             switch (index) {
> > +             case 0:
> > +                     data->mask_reset_reg = EXYNOS850_CLUSTER0_NONCPU_INT_EN;
> > +                     data->rst_stat_bit = EXYNOS850_CLUSTER0_WDTRESET_BIT;
> > +                     data->cnt_en_reg = EXYNOS850_CLUSTER0_NONCPU_OUT;
> > +                     break;
> > +             case 1:
> > +                     data->mask_reset_reg = EXYNOS850_CLUSTER1_NONCPU_INT_EN;
> > +                     data->rst_stat_bit = EXYNOS850_CLUSTER1_WDTRESET_BIT;
> > +                     data->cnt_en_reg = EXYNOS850_CLUSTER1_NONCPU_OUT;
> > +                     break;
> > +             default:
> > +                     dev_err(dev, "wrong cluster index: %u\n", index);
> > +                     return NULL;
> > +             }
> > +     }
> > +
> > +     return data;
> >  }
> >
> >  static int s3c2410wdt_probe(struct platform_device *pdev)
> > @@ -576,6 +637,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >       wdt->wdt_device = s3c2410_wdd;
> >
> >       wdt->drv_data = s3c2410_get_wdt_drv_data(pdev);
> > +     if (!wdt->drv_data)
> > +             return -EINVAL;
> > +
> >       if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> >               wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> >                                               "samsung,syscon-phandle");
> >
>
>
> Best regards,
> Krzysztof