Re: [PATCH 5/7] watchdog: s3c2410: Introduce separate source clock

[Sam Protsenko]

On Fri, 29 Oct 2021 at 11:18, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxxxxx> wrote:
>
> On 28/10/2021 20:35, Sam Protsenko wrote:
> > 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.

> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@xxxxxxxxxx>
> > ---
> >  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.

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

> > +
> >   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