| Message ID | 20211028183527.3050-6-semen.protsenko@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | watchdog: s3c2410: Add Exynos850 support | expand |
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. 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; >
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? > > 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; This does not look good. You are using fixed frequency (from probe). > 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... > + > 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
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; > > >
On Fri, 29 Oct 2021 at 11:18, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> 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@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; > > 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
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;
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. Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- drivers/watchdog/s3c2410_wdt.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)