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
