On Fri, 29 Oct 2021 at 11:18, Krzysztof Kozlowski
[off-list ref] wrote:

On 28/10/2021 20:35, Sam Protsenko wrote:

quoted

Some Exynos chips (like Exynos850) have dedicated source clock. That
clock is provided from device tree as "watchdog_src" clock. In such
case, "watchdog" clock is just a peripheral clock used for register
interface. If "watchdog_src" is present, use its rate instead of
"watchdog" for all timer related calculations.

Please explain what is this source clock and remove the reference to
devicetree. Instead describe rather real HW. It's confusing now to have
one clock called watchdog and one watchdog source.

The source clock is the actual clock driving watchdog and it's counter,
right? Then let's document it and rename the variables to match reality
- one is pclk (or apb?) and second is counter or source?

Done, will be present in v2.

quoted

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/watchdog/s3c2410_wdt.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index a5ef7171a90e..bfc5872ca497 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c

@@ -126,6 +126,8 @@ struct s3c2410_wdt_variant {
 struct s3c2410_wdt {
      struct device           *dev;
      struct clk              *clock;
+     struct clk              *clock_src;
+     unsigned long           freq_src;
      void __iomem            *reg_base;
      unsigned int            count;
      spinlock_t              lock;

@@ -213,10 +215,8 @@ MODULE_DEVICE_TABLE(platform, s3c2410_wdt_ids);

 /* functions */

-static inline unsigned int s3c2410wdt_max_timeout(struct clk *clock)
+static inline unsigned int s3c2410wdt_max_timeout(unsigned long freq)
 {
-     unsigned long freq = clk_get_rate(clock);
-
      return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
                                     / S3C2410_WTCON_MAXDIV);
 }

@@ -364,7 +364,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 = wdt->freq_src;

This does not look good. You are using fixed frequency (from probe).

Ok, will avoid caching this value in v2.

quoted

      unsigned int count;
      unsigned int divisor = 1;
      unsigned long wtcon;

@@ -627,13 +627,27 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
              return ret;
      }

+     /* "watchdog_src" clock is optional; if it's not present -- just skip */
+     wdt->clock_src = devm_clk_get(dev, "watchdog_src");
+     if (!IS_ERR(wdt->clock_src)) {
+             ret = clk_prepare_enable(wdt->clock_src);
+             if (ret < 0) {
+                     dev_err(dev, "failed to enable source clock\n");
+                     ret = PTR_ERR(wdt->clock_src);
+                     goto err_clk;
+             }
+             wdt->freq_src = clk_get_rate(wdt->clock_src);
+     } else {
+             wdt->freq_src = clk_get_rate(wdt->clock);
+     }
+
      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->freq_src);

      ret = s3c2410wdt_cpufreq_register(wdt);
      if (ret < 0) {
              dev_err(dev, "failed to register cpufreq\n");
-             goto err_clk;
+             goto err_clk_src;
      }

      watchdog_set_drvdata(&wdt->wdt_device, wdt);

@@ -707,6 +721,10 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
  err_cpufreq:
      s3c2410wdt_cpufreq_deregister(wdt);

+ err_clk_src:
+     if (!IS_ERR(wdt->clock_src))
+             clk_disable_unprepare(wdt->clock_src);

No. Errors in getting source clock should not be ignored, so you should
never store here ERR. You could store NULL. If() is anyway not needed in
both cases.

You can simplify all this and take pclk twice if src clock is missing.
Or assign src=pclk...

Hmm, I don't want to take the same clock twice. It'll increase its
refcount twice, which might be confusing in some cases. I guess I'll
rework it to be like this in v2:
  - add "has_src_clk" bool field to struct wdt
  - if "watchdog_src" is provided: set has_src_clk "true"
  - if "watchdog_src" is not provided: set has_src_clk "false"
(default BSS val) and assign src=pclk
  - only enable/disable src clock when has_src_clk is "true"

That simplifies clock using, fixes stored pointer value, and avoids
taking the clock twice, all at the same time. Hope that way is fine
with you.

quoted

+
  err_clk:
      clk_disable_unprepare(wdt->clock);

@@ -727,6 +745,9 @@ static int s3c2410wdt_remove(struct platform_device *dev)

      s3c2410wdt_cpufreq_deregister(wdt);

+     if (!IS_ERR(wdt->clock_src))
+             clk_disable_unprepare(wdt->clock_src);
+
      clk_disable_unprepare(wdt->clock);

      return 0;

Best regards,
Krzysztof