Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211028183527.3050-1-semen.protsenko@linaro.org>
 <20211028183527.3050-6-semen.protsenko@linaro.org> <51128b74-de5d-1758-282b-1d4c5250a38d@roeck-us.net>
In-Reply-To: <51128b74-de5d-1758-282b-1d4c5250a38d@roeck-us.net>
From:   Sam Protsenko <semen.protsenko@linaro.org>
Date:   Sat, 30 Oct 2021 15:39:23 +0300
Message-ID: <CAPLW+4mnAV0yHdYFtosYfS2jcAUa4qvScvmohBKvqTBHz-5CFQ@mail.gmail.com>
Subject: Re: [PATCH 5/7] watchdog: s3c2410: Introduce separate source clock
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Wim Van Sebroeck <wim@linux-watchdog.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
        linux-watchdog@vger.kernel.org,
        devicetree <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
        Linux Samsung SOC <linux-samsung-soc@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 29 Oct 2021 at 03:21, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/28/21 11:35 AM, 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.
> >
>
> If the "watchdog_src" clock is present, is "watchdog" clock still needed ?
> Please state that explicitly, since it is kind of unusual.
>

Done, I've reworded the commit message. Will send v2 soon, thanks.

> Guenter
>
> > 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;
> >       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);
> > +
> >    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;
> >
>