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

[Guenter Roeck <linux@roeck-us.net>]

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