Re: [PATCH v2 10/12] watchdog: s3c2410: Support separate source clock
From: Guenter Roeck
Date: Sun Nov 07 2021 - 14:02:20 EST
On 11/7/21 10:51 AM, Sam Protsenko wrote:
On Sun, 7 Nov 2021 at 18:09, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
On 11/7/21 7:55 AM, Sam Protsenko wrote:
On Tue, 2 Nov 2021 at 12:15, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
---
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