diff mbox series

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

Message ID 20211031122216.30212-11-semen.protsenko@linaro.org (mailing list archive)
State Superseded
Headers show
Series watchdog: s3c2410: Add Exynos850 support | expand

Commit Message

Sam Protsenko Oct. 31, 2021, 12:22 p.m. UTC
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(-)

Comments

Krzysztof Kozlowski Nov. 2, 2021, 10:15 a.m. UTC | #1
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
Sam Protsenko Nov. 7, 2021, 3:55 p.m. UTC | #2
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
Guenter Roeck Nov. 7, 2021, 4:09 p.m. UTC | #3
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
Sam Protsenko Nov. 7, 2021, 6:51 p.m. UTC | #4
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
Guenter Roeck Nov. 7, 2021, 7:01 p.m. UTC | #5
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 mbox series

Patch

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;
 }