| Message ID | 20211031122216.30212-11-semen.protsenko@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | watchdog: s3c2410: Add Exynos850 support | expand |
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. Best regards, Krzysztof
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. > Best regards, > Krzysztof
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. Guenter
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. > Guenter
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
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; void __iomem *reg_base; unsigned int count; spinlock_t lock; @@ -348,7 +350,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 = clk_get_rate(wdt->src_clk); unsigned int count; unsigned int divisor = 1; unsigned long wtcon; @@ -597,26 +599,43 @@ static int s3c2410wdt_probe(struct platform_device *pdev) goto err; } - wdt->clock = devm_clk_get(dev, "watchdog"); - if (IS_ERR(wdt->clock)) { - dev_err(dev, "failed to find watchdog clock source\n"); - ret = PTR_ERR(wdt->clock); + wdt->bus_clk = devm_clk_get(dev, "watchdog"); + if (IS_ERR(wdt->bus_clk)) { + dev_err(dev, "failed to find bus clock\n"); + ret = PTR_ERR(wdt->bus_clk); goto err; } - ret = clk_prepare_enable(wdt->clock); + ret = clk_prepare_enable(wdt->bus_clk); if (ret < 0) { - dev_err(dev, "failed to enable clock\n"); + dev_err(dev, "failed to enable bus clock\n"); return ret; } + /* + * "watchdog_src" clock is optional; if it's not present -- just skip it + * and use "watchdog" clock as both bus and source clock. + */ + wdt->src_clk = devm_clk_get(dev, "watchdog_src"); + if (!IS_ERR(wdt->src_clk)) { + wdt->has_src_clk = true; + ret = clk_prepare_enable(wdt->src_clk); + if (ret < 0) { + dev_err(dev, "failed to enable source clock\n"); + ret = PTR_ERR(wdt->src_clk); + goto err_bus_clk; + } + } else { + wdt->src_clk = wdt->bus_clk; + } + 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->src_clk); ret = s3c2410wdt_cpufreq_register(wdt); if (ret < 0) { dev_err(dev, "failed to register cpufreq\n"); - goto err_clk; + goto err_src_clk; } watchdog_set_drvdata(&wdt->wdt_device, wdt); @@ -694,8 +713,12 @@ static int s3c2410wdt_probe(struct platform_device *pdev) err_cpufreq: s3c2410wdt_cpufreq_deregister(wdt); - err_clk: - clk_disable_unprepare(wdt->clock); + err_src_clk: + if (wdt->has_src_clk) + clk_disable_unprepare(wdt->src_clk); + + err_bus_clk: + clk_disable_unprepare(wdt->bus_clk); err: return ret; @@ -714,7 +737,10 @@ static int s3c2410wdt_remove(struct platform_device *dev) s3c2410wdt_cpufreq_deregister(wdt); - clk_disable_unprepare(wdt->clock); + if (wdt->has_src_clk) + clk_disable_unprepare(wdt->src_clk); + + clk_disable_unprepare(wdt->bus_clk); return 0; }
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(-)