[04/11] watchdog: ftwdt010: Add clock support

Message ID 20170812184318.10144-5-linus.walleij@linaro.org
State New
Headers show
Series
  • watchdog: Consolidate FTWDT010 derivatives
Related show

Commit Message

Linus Walleij Aug. 12, 2017, 6:43 p.m.
The Gemini platform now provides a proper clock look-up for this
and other IPs, so add clock support to the driver. This also aids
in using the same driver with other platforms such as MOXA ART.

The IP has two clock inputs: PCLK (the IP peripheral clock) and
EXTCLK (an external clock). We are a bit elaborate around this:
on Gemini the EXTCLK is used by default today and it's 5MHz, and
on MOXA ART the PCLK is used. On Aspeed the EXTCLK is used and
it's 1MHz. So add some clever code to fall back to platform
defaults if PCLK or EXTCLK is not provided by the device tree.

Take this opportnity to implement .remove() for the driver that
stops the watchdog and disables the clocks.

Add credits that this code is inspired by MOXA ART.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
 drivers/watchdog/ftwdt010_wdt.c | 79 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 71 insertions(+), 8 deletions(-)

-- 
2.13.4

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Guenter Roeck Aug. 14, 2017, 4:05 p.m. | #1
On Sat, Aug 12, 2017 at 08:43:11PM +0200, Linus Walleij wrote:
> The Gemini platform now provides a proper clock look-up for this

> and other IPs, so add clock support to the driver. This also aids

> in using the same driver with other platforms such as MOXA ART.

> 

> The IP has two clock inputs: PCLK (the IP peripheral clock) and

> EXTCLK (an external clock). We are a bit elaborate around this:

> on Gemini the EXTCLK is used by default today and it's 5MHz, and

> on MOXA ART the PCLK is used. On Aspeed the EXTCLK is used and

> it's 1MHz. So add some clever code to fall back to platform

> defaults if PCLK or EXTCLK is not provided by the device tree.

> 

> Take this opportnity to implement .remove() for the driver that


opportunity

> stops the watchdog and disables the clocks.

> 

> Add credits that this code is inspired by MOXA ART.

> 

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

>  drivers/watchdog/ftwdt010_wdt.c | 79 ++++++++++++++++++++++++++++++++++++-----

>  1 file changed, 71 insertions(+), 8 deletions(-)

> 

> diff --git a/drivers/watchdog/ftwdt010_wdt.c b/drivers/watchdog/ftwdt010_wdt.c

> index ab38a3a89300..680279f5c679 100644

> --- a/drivers/watchdog/ftwdt010_wdt.c

> +++ b/drivers/watchdog/ftwdt010_wdt.c

> @@ -5,6 +5,8 @@

>   *

>   * Inspired by the out-of-tree drivers from OpenWRT:

>   * Copyright (C) 2009 Paulius Zaleckas <paulius.zaleckas@teltonika.lt>

> + * Inspired by the MOXA ART driver from Jonas Jensen:

> + * Copyright (C) 2013 Jonas Jensen <jonas.jensen@gmail.com>

>   *

>   * This program is free software; you can redistribute it and/or modify

>   * it under the terms of the GNU General Public License version 2 as

> @@ -18,9 +20,11 @@

>  #include <linux/kernel.h>

>  #include <linux/module.h>

>  #include <linux/of_device.h>

> +#include <linux/of.h>

>  #include <linux/platform_device.h>

>  #include <linux/slab.h>

>  #include <linux/watchdog.h>

> +#include <linux/clk.h>


Any chance to maintain alphabetic order with include files ?

>  

>  #define FTWDT010_WDCOUNTER	0x0

>  #define FTWDT010_WDLOAD		0x4

> @@ -29,19 +33,21 @@

>  

>  #define WDRESTART_MAGIC		0x5AB9

>  

> -#define WDCR_CLOCK_5MHZ		BIT(4)

> +#define WDCR_EXTCLK		BIT(4)

>  #define WDCR_WDEXT		BIT(3)

>  #define WDCR_WDINTR		BIT(2)

>  #define WDCR_SYS_RST		BIT(1)

>  #define WDCR_ENABLE		BIT(0)

>  

> -#define WDT_CLOCK		5000000		/* 5 MHz */

> -

>  struct ftwdt010_wdt {

>  	struct watchdog_device	wdd;

>  	struct device		*dev;

>  	void __iomem		*base;

>  	bool			has_irq;

> +	struct clk		*pclk;

> +	struct clk		*extclk;

> +	unsigned int		clk_freq;

> +	bool			use_extclk;

>  };

>  

>  static inline

> @@ -55,12 +61,13 @@ static int ftwdt010_wdt_start(struct watchdog_device *wdd)

>  	struct ftwdt010_wdt *gwdt = to_ftwdt010_wdt(wdd);

>  	u32 enable;

>  

> -	writel(wdd->timeout * WDT_CLOCK, gwdt->base + FTWDT010_WDLOAD);

> +	writel(wdd->timeout * gwdt->clk_freq, gwdt->base + FTWDT010_WDLOAD);

>  	writel(WDRESTART_MAGIC, gwdt->base + FTWDT010_WDRESTART);

>  	/* set clock before enabling */

> -	enable = WDCR_CLOCK_5MHZ | WDCR_SYS_RST;

> +	enable = WDCR_SYS_RST;

> +	if (gwdt->use_extclk)

> +		enable |= WDCR_EXTCLK;

>  	writel(enable, gwdt->base + FTWDT010_WDCR);

> -	enable |= WDCR_CLOCK_5MHZ;

>  	if (gwdt->has_irq)

>  		enable |= WDCR_WDINTR;

>  	enable |= WDCR_ENABLE;

> @@ -125,6 +132,7 @@ static const struct watchdog_info ftwdt010_wdt_info = {

>  static int ftwdt010_wdt_probe(struct platform_device *pdev)

>  {

>  	struct device *dev = &pdev->dev;

> +	struct device_node *np = dev->of_node;

>  	struct resource *res;

>  	struct ftwdt010_wdt *gwdt;

>  	unsigned int reg;

> @@ -140,11 +148,51 @@ static int ftwdt010_wdt_probe(struct platform_device *pdev)

>  	if (IS_ERR(gwdt->base))

>  		return PTR_ERR(gwdt->base);

>  

> +	gwdt->use_extclk = of_property_read_bool(np, "faraday,use-extclk");

> +

> +	gwdt->pclk = devm_clk_get(dev, "PCLK");

> +	if (!IS_ERR(gwdt->pclk)) {


devm_clk_get() can return NULL (if the clock subsystem is not enabled).

> +		ret = clk_prepare_enable(gwdt->pclk);


Why enable pclk if extclk is used ?

> +		if (ret) {

> +			dev_err(&pdev->dev, "unable to enable PCLK\n");

> +			return ret;

> +		}

> +		if (!gwdt->use_extclk)

> +			gwdt->clk_freq = clk_get_rate(gwdt->pclk);

> +	} else {

> +		dev_info(dev, "PCLK clock not found assume always-on\n");


Those info messages seem more like debug messages to me. Is this and the message
below about 5MHz clock on Gemini really necessary ?

> +	}

> +

> +	gwdt->extclk = devm_clk_get(dev, "EXTCLK");

> +	if (!IS_ERR(gwdt->extclk)) {


devm_clk_get() can return NULL.

> +		/* Only enable and get frequency from EXTCLK if it's in use */

> +		if (gwdt->use_extclk) {

> +			ret = clk_prepare_enable(gwdt->extclk);

> +			if (ret) {

> +				dev_err(&pdev->dev,

> +					"unable to enable EXTCLK\n");

> +				return ret;

> +			}

> +			gwdt->clk_freq = clk_get_rate(gwdt->extclk);

> +		}

> +	} else {

> +		if (of_device_is_compatible(np, "cortina,gemini-watchdog")) {

> +			gwdt->clk_freq = 5000000;

> +			gwdt->use_extclk = true;

> +			dev_info(dev, "assume 5MHz EXTCLK on Gemini\n");

> +		}

> +	}

> +

> +	if (gwdt->clk_freq == 0) {

> +		dev_err(dev, "no clocking available\n");

> +		return -EINVAL;


So far this situation defaulted to 5 MHz (as there was nothing else).
Is this a new restriction or did it just not happen ?

Also, this can at least in theory happen if clk_get_rate() returns 0,
which would leave the clock enabled (although that would be an odd
situation).

> +	}

> +

>  	gwdt->dev = dev;

>  	gwdt->wdd.info = &ftwdt010_wdt_info;

>  	gwdt->wdd.ops = &ftwdt010_wdt_ops;

>  	gwdt->wdd.min_timeout = 1;

> -	gwdt->wdd.max_timeout = 0xFFFFFFFF / WDT_CLOCK;

> +	gwdt->wdd.max_timeout = UINT_MAX / gwdt->clk_freq;

>  	gwdt->wdd.parent = dev;

>  


devm_watchdog_register_device() can fail, which would leave the clocks
enabled. Also see below.

>  	/*

> @@ -178,7 +226,21 @@ static int ftwdt010_wdt_probe(struct platform_device *pdev)

>  

>  	/* Set up platform driver data */

>  	platform_set_drvdata(pdev, gwdt);

> -	dev_info(dev, "FTWDT010 watchdog driver enabled\n");

> +	dev_info(dev, "FTWDT010 watchdog driver @%uHz\n",

> +		 gwdt->clk_freq);

> +

> +	return 0;

> +}

> +

> +static int ftwdt010_wdt_remove(struct platform_device *pdev)

> +{

> +	struct ftwdt010_wdt *gwdt = platform_get_drvdata(pdev);

> +

> +	writel(0, gwdt->base + FTWDT010_WDCR);

> +	if (!IS_ERR(gwdt->pclk))

> +		clk_disable_unprepare(gwdt->pclk);

> +	if (!IS_ERR(gwdt->extclk) && gwdt->use_extclk)

> +		clk_disable_unprepare(gwdt->extclk);


One of those many situations where devm_clk_prepare_enable() would have
been very useful :-(. This disables the clocks while the watchdog itself
as well as its interrupt handler is still registered. I don't know if this
will have adverse affects, but it makes me quite concerned. Please consider
adding devm_add_action() calls to clean up the clocks. Note that I would
resist replacing all the devm_ functions with non-devm equivalents just
because the clock subsystem doesn't provide the necessary API functions.

Side note: Maybe we _should_ introduce devm_watchdog_clk_prepare_enable()
since this problem affects several watchdog drivers.

Guenter

>  

>  	return 0;

>  }

> @@ -225,6 +287,7 @@ MODULE_DEVICE_TABLE(of, ftwdt010_wdt_match);

>  

>  static struct platform_driver ftwdt010_wdt_driver = {

>  	.probe		= ftwdt010_wdt_probe,

> +	.remove		= ftwdt010_wdt_remove,

>  	.driver		= {

>  		.name	= "ftwdt010-wdt",

>  		.of_match_table = of_match_ptr(ftwdt010_wdt_match),

> -- 

> 2.13.4

> 

> --

> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in

> the body of a message to majordomo@vger.kernel.org

> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Aug. 24, 2017, 8:32 p.m. | #2
On Mon, Aug 14, 2017 at 6:05 PM, Guenter Roeck <linux@roeck-us.net> wrote:

>> +     gwdt->pclk = devm_clk_get(dev, "PCLK");

>> +     if (!IS_ERR(gwdt->pclk)) {

>

> devm_clk_get() can return NULL (if the clock subsystem is not enabled).


That is fine I think? Because if the clock subsysten is not enabled
all the clk_prepare() etc becomes stubs as well and the driver
is happy. I think this is intended.

>> +             ret = clk_prepare_enable(gwdt->pclk);

>

> Why enable pclk if extclk is used ?


It is used to clock the silicon in the IP block (so one can access the
registers etc) even if the timer per se uses EXTCLK.

So it must always be enabled.

>> +             if (ret) {

>> +                     dev_err(&pdev->dev, "unable to enable PCLK\n");

>> +                     return ret;

>> +             }

>> +             if (!gwdt->use_extclk)

>> +                     gwdt->clk_freq = clk_get_rate(gwdt->pclk);

>> +     } else {

>> +             dev_info(dev, "PCLK clock not found assume always-on\n");

>

> Those info messages seem more like debug messages to me. Is this and the message

> below about 5MHz clock on Gemini really necessary ?


Depends on whether one is pr_info()/dev_info() minimalist or
maximalist or something. I guess the extreme minimalist would be
happiest of their dmesg was 0 lines if all is fine. Maybe it could
just say the Linux version.

I even had the idea to add the subsystem maintainers preference
for this to MAINTAINERS.

I tend to like a bit of blather about the state of things in dmesg,
(as in *info has a purpose) but I'm happy to do whatever the
subsystem maintainer likes.

So I can surely cut a whole slew of them if that is your preference?

>> +     gwdt->extclk = devm_clk_get(dev, "EXTCLK");

>> +     if (!IS_ERR(gwdt->extclk)) {

>

> devm_clk_get() can return NULL.


Same answer: should be fine.

>> +     if (gwdt->clk_freq == 0) {

>> +             dev_err(dev, "no clocking available\n");

>> +             return -EINVAL;

>

> So far this situation defaulted to 5 MHz (as there was nothing else).

> Is this a new restriction or did it just not happen ?


This comes from Jonas' Moxart driver.

I guess this only happens if someone compiles out the
clk subsystem so they get frequency 0 from the stub.

The driver strictly needs this frequency
so we cannot really work without it and it needs to fail
over here.

> Also, this can at least in theory happen if clk_get_rate() returns 0,

> which would leave the clock enabled (although that would be an odd

> situation).


Yeah I should clk_disable_unprepare() on the error path,
thanks. Fixing it.

> devm_watchdog_register_device() can fail, which would leave the clocks

> enabled. Also see below.


Fixed it.

>> +     if (!IS_ERR(gwdt->pclk))

>> +             clk_disable_unprepare(gwdt->pclk);

>> +     if (!IS_ERR(gwdt->extclk) && gwdt->use_extclk)

>> +             clk_disable_unprepare(gwdt->extclk);

>

> One of those many situations where devm_clk_prepare_enable() would have

> been very useful :-(. This disables the clocks while the watchdog itself

> as well as its interrupt handler is still registered.


Oh hm yeah. I ran into this thing with the IRQs still being
enabled while the clocking get shut off. It is a problem in the
entire kernel. I don't even have a good intuition for what
order the devm_* things get cleaned up, I guess in the
inverse order that one use them in probe()?

>I don't know if this

> will have adverse affects, but it makes me quite concerned. Please consider

> adding devm_add_action() calls to clean up the clocks.


I don't know if that is a very good idea. If we later get proper
devm_* clock disabling functions then that gets messy
to clean up.

Stephen/Mike: what's your opinion?

> Note that I would

> resist replacing all the devm_ functions with non-devm equivalents just

> because the clock subsystem doesn't provide the necessary API functions.


I've seen people do this for this reason though :/

> Side note: Maybe we _should_ introduce devm_watchdog_clk_prepare_enable()

> since this problem affects several watchdog drivers.


Hmmmmmmmmm a special watchdog primitive may be apropriate.
Dunno what the clk maintainers think?

Stephen/Mike: do you like that or would you rather see a primitive
inside the clock subsystem for this?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd Aug. 25, 2017, 11:28 p.m. | #3
On 08/24, Linus Walleij wrote:
> On Mon, Aug 14, 2017 at 6:05 PM, Guenter Roeck <linux@roeck-us.net> wrote:

> 

> > Side note: Maybe we _should_ introduce devm_watchdog_clk_prepare_enable()

> > since this problem affects several watchdog drivers.

> 

> Hmmmmmmmmm a special watchdog primitive may be apropriate.

> Dunno what the clk maintainers think?

> 

> Stephen/Mike: do you like that or would you rather see a primitive

> inside the clock subsystem for this?

> 


devm_clk_prepare_enable() was already proposed and then the
thread went quiet. Re-kickstart it?

I'd still prefer we just disable clks on clk_put(), but Russell
said we needed to fix all non-common clk implementations of the
clk API to do that and then I forgot about the topic (so
anti-climatic). I'm pretty much OK with us merging the temporary
solution. We can churn again later and remove it all once we
convert everything into one CCF.

I'd prefer we also change common clk framework to actually do the
disable on put though so we flush out any issues early. If the
two things are packaged together I would be ultra happy. It's not
like people are going to convert to CCF just so they can get clk
disable on clk_put() as a feature, but at least we can have it as
a feature.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck Aug. 27, 2017, 5:06 p.m. | #4
On Thu, Aug 24, 2017 at 10:32:22PM +0200, Linus Walleij wrote:
> On Mon, Aug 14, 2017 at 6:05 PM, Guenter Roeck <linux@roeck-us.net> wrote:

> 

> >> +     gwdt->pclk = devm_clk_get(dev, "PCLK");

> >> +     if (!IS_ERR(gwdt->pclk)) {

> >

> > devm_clk_get() can return NULL (if the clock subsystem is not enabled).

> 

> That is fine I think? Because if the clock subsysten is not enabled

> all the clk_prepare() etc becomes stubs as well and the driver

> is happy. I think this is intended.


If I understand your comment below correctly, the driver won't work
without clock subsystem because the clock frequency would in that case
be 0. Why not catch that situation here, or even better make the driver
depends on the clock subsystem ?

> 

> >> +             ret = clk_prepare_enable(gwdt->pclk);

> >

> > Why enable pclk if extclk is used ?

> 

> It is used to clock the silicon in the IP block (so one can access the

> registers etc) even if the timer per se uses EXTCLK.

> 

> So it must always be enabled.

> 

> >> +             if (ret) {

> >> +                     dev_err(&pdev->dev, "unable to enable PCLK\n");

> >> +                     return ret;

> >> +             }

> >> +             if (!gwdt->use_extclk)

> >> +                     gwdt->clk_freq = clk_get_rate(gwdt->pclk);

> >> +     } else {

> >> +             dev_info(dev, "PCLK clock not found assume always-on\n");

> >

> > Those info messages seem more like debug messages to me. Is this and the message

> > below about 5MHz clock on Gemini really necessary ?

> 

> Depends on whether one is pr_info()/dev_info() minimalist or

> maximalist or something. I guess the extreme minimalist would be

> happiest of their dmesg was 0 lines if all is fine. Maybe it could

> just say the Linux version.

> 

> I even had the idea to add the subsystem maintainers preference

> for this to MAINTAINERS.

> 

> I tend to like a bit of blather about the state of things in dmesg,

> (as in *info has a purpose) but I'm happy to do whatever the

> subsystem maintainer likes.

> 

> So I can surely cut a whole slew of them if that is your preference?

> 

Minimalist.

> >> +     gwdt->extclk = devm_clk_get(dev, "EXTCLK");

> >> +     if (!IS_ERR(gwdt->extclk)) {

> >

> > devm_clk_get() can return NULL.

> 

> Same answer: should be fine.

> 

> >> +     if (gwdt->clk_freq == 0) {

> >> +             dev_err(dev, "no clocking available\n");

> >> +             return -EINVAL;

> >

> > So far this situation defaulted to 5 MHz (as there was nothing else).

> > Is this a new restriction or did it just not happen ?

> 

> This comes from Jonas' Moxart driver.

> 

> I guess this only happens if someone compiles out the

> clk subsystem so they get frequency 0 from the stub.

> 

> The driver strictly needs this frequency

> so we cannot really work without it and it needs to fail

> over here.

> 

Repeating from above, doesn't that mean that the driver depends
on the clock subsystem ? Or am I missong some context ?

> > Also, this can at least in theory happen if clk_get_rate() returns 0,

> > which would leave the clock enabled (although that would be an odd

> > situation).

> 

> Yeah I should clk_disable_unprepare() on the error path,

> thanks. Fixing it.

> 

> > devm_watchdog_register_device() can fail, which would leave the clocks

> > enabled. Also see below.

> 

> Fixed it.

> 

> >> +     if (!IS_ERR(gwdt->pclk))

> >> +             clk_disable_unprepare(gwdt->pclk);

> >> +     if (!IS_ERR(gwdt->extclk) && gwdt->use_extclk)

> >> +             clk_disable_unprepare(gwdt->extclk);

> >

> > One of those many situations where devm_clk_prepare_enable() would have

> > been very useful :-(. This disables the clocks while the watchdog itself

> > as well as its interrupt handler is still registered.

> 

> Oh hm yeah. I ran into this thing with the IRQs still being

> enabled while the clocking get shut off. It is a problem in the

> entire kernel. I don't even have a good intuition for what

> order the devm_* things get cleaned up, I guess in the

> inverse order that one use them in probe()?

> 

Correct.

> >I don't know if this

> > will have adverse affects, but it makes me quite concerned. Please consider

> > adding devm_add_action() calls to clean up the clocks.

> 

> I don't know if that is a very good idea. If we later get proper

> devm_* clock disabling functions then that gets messy

> to clean up.


No, it is equivalent.

> 

> Stephen/Mike: what's your opinion?

> 

> > Note that I would

> > resist replacing all the devm_ functions with non-devm equivalents just

> > because the clock subsystem doesn't provide the necessary API functions.

> 

> I've seen people do this for this reason though :/

> 

Yes, but that doesn't make it better.

Guenter

> > Side note: Maybe we _should_ introduce devm_watchdog_clk_prepare_enable()

> > since this problem affects several watchdog drivers.

> 

> Hmmmmmmmmm a special watchdog primitive may be apropriate.

> Dunno what the clk maintainers think?

> 

> Stephen/Mike: do you like that or would you rather see a primitive

> inside the clock subsystem for this?

> 

> Yours,

> Linus Walleij

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck Aug. 27, 2017, 5:12 p.m. | #5
On Fri, Aug 25, 2017 at 04:28:46PM -0700, Stephen Boyd wrote:
> On 08/24, Linus Walleij wrote:

> > On Mon, Aug 14, 2017 at 6:05 PM, Guenter Roeck <linux@roeck-us.net> wrote:

> > 

> > > Side note: Maybe we _should_ introduce devm_watchdog_clk_prepare_enable()

> > > since this problem affects several watchdog drivers.

> > 

> > Hmmmmmmmmm a special watchdog primitive may be apropriate.

> > Dunno what the clk maintainers think?

> > 

> > Stephen/Mike: do you like that or would you rather see a primitive

> > inside the clock subsystem for this?

> > 

> 

> devm_clk_prepare_enable() was already proposed and then the

> thread went quiet. Re-kickstart it?

> 


It was propsed several times, and each time it went nowhere.
I got the impression that it won't be accepted, presumably because
it can be misused. I have all but given up on it, and I have it on
my task list to replace pretty much each pair of clk_prepare_enable() /
clk_prepare_disable() calls in the watchdog subsystem with
devm_add_action(). Not that I like that "solution", but life
isn't perfect.

Guenter

> I'd still prefer we just disable clks on clk_put(), but Russell

> said we needed to fix all non-common clk implementations of the

> clk API to do that and then I forgot about the topic (so

> anti-climatic). I'm pretty much OK with us merging the temporary

> solution. We can churn again later and remove it all once we

> convert everything into one CCF.

> 

> I'd prefer we also change common clk framework to actually do the

> disable on put though so we flush out any issues early. If the

> two things are packaged together I would be ultra happy. It's not

> like people are going to convert to CCF just so they can get clk

> disable on clk_put() as a feature, but at least we can have it as

> a feature.

> 

> -- 

> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,

> a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Oct. 10, 2017, 7:51 p.m. | #6
On Sun, Aug 27, 2017 at 7:06 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Thu, Aug 24, 2017 at 10:32:22PM +0200, Linus Walleij wrote:

>> On Mon, Aug 14, 2017 at 6:05 PM, Guenter Roeck <linux@roeck-us.net> wrote:

>>

>> >> +     gwdt->pclk = devm_clk_get(dev, "PCLK");

>> >> +     if (!IS_ERR(gwdt->pclk)) {

>> >

>> > devm_clk_get() can return NULL (if the clock subsystem is not enabled).

>>

>> That is fine I think? Because if the clock subsysten is not enabled

>> all the clk_prepare() etc becomes stubs as well and the driver

>> is happy. I think this is intended.

>

> If I understand your comment below correctly, the driver won't work

> without clock subsystem because the clock frequency would in that case

> be 0. Why not catch that situation here, or even better make the driver

> depends on the clock subsystem ?


OK fair enough I can do that.

I think Aspeed is merging its clock driver this kernel cycle
so all variant SoCs actually have clocks.

>> The driver strictly needs this frequency

>> so we cannot really work without it and it needs to fail

>> over here.

>>

> Repeating from above, doesn't that mean that the driver depends

> on the clock subsystem ? Or am I missong some context ?


Usually in the kernel, it is acceptable to let stub clocks
kick in when compiling out the clock subsystem.

I think people assume that in this state, all clocks are simply
turned on and at nominal speed, all over the place. So it
usually doesn't matter that it gets compiled out.

But in a sense that is simplistic and as this driver shows
we really do depend on the clocks to run some business
like timers, so in practice it is a bad idea. Clocksources
definately cannot live without their clocks for example.
So I think strictly requireing it is OK.

>> > Note that I would

>> > resist replacing all the devm_ functions with non-devm equivalents just

>> > because the clock subsystem doesn't provide the necessary API functions.

>>

>> I've seen people do this for this reason though :/

>>

> Yes, but that doesn't make it better.


Stephen Boyd <sboyd@codeaurora.org> wrote:

> I'd still prefer we just disable clks on clk_put(),


Which means that devm_clk_get() will do the job
implicitly already.

> but Russell

> said we needed to fix all non-common clk implementations of the

> clk API to do that and then I forgot about the topic (so

> anti-climatic). I'm pretty much OK with us merging the temporary

> solution.


I guess that means the watchdog-local hack that
Guenther suggested.

But I'm not in any hurry. Instead of solving the simple problem,
why not solve the supercomplicated problem.

So the task is to look over all non generic clock implementations
and make them conform to disabling the clocks on clk_put().
No big deal. (Famous last words.)

How many local clock implementations can there be?
I will look into it.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Oct. 10, 2017, 8:06 p.m. | #7
So need info from Joel Stanley here:

On Tue, Oct 10, 2017 at 9:51 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sun, Aug 27, 2017 at 7:06 PM, Guenter Roeck <linux@roeck-us.net> wrote:

>> On Thu, Aug 24, 2017 at 10:32:22PM +0200, Linus Walleij wrote:

>>> On Mon, Aug 14, 2017 at 6:05 PM, Guenter Roeck <linux@roeck-us.net> wrote:


>> If I understand your comment below correctly, the driver won't work

>> without clock subsystem because the clock frequency would in that case

>> be 0. Why not catch that situation here, or even better make the driver

>> depends on the clock subsystem ?

>

> OK fair enough I can do that.

>

> I think Aspeed is merging its clock driver this kernel cycle

> so all variant SoCs actually have clocks.


So when I later in the patch series convert Aspeed to use this driver
we get a problem because Joel is currently working on the clock
driver for Aspeed and it's not ready for merge yet as it looks.

I can of course wait with the Aspeed conversion to use the common
driver.

Also I can slap in a fixed-rate clock in the device tree @1 MHz.
But that is cheating.

So I guess I rest the Aspeed conversion.

Joel: does your clock patch set cover the 1MHz used by
the watchdog EXTCLK?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joel Stanley Oct. 12, 2017, 3:39 a.m. | #8
On Wed, Oct 11, 2017 at 4:06 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> So need info from Joel Stanley here:

>

> On Tue, Oct 10, 2017 at 9:51 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> On Sun, Aug 27, 2017 at 7:06 PM, Guenter Roeck <linux@roeck-us.net> wrote:

>>> On Thu, Aug 24, 2017 at 10:32:22PM +0200, Linus Walleij wrote:

>>>> On Mon, Aug 14, 2017 at 6:05 PM, Guenter Roeck <linux@roeck-us.net> wrote:

>

>>> If I understand your comment below correctly, the driver won't work

>>> without clock subsystem because the clock frequency would in that case

>>> be 0. Why not catch that situation here, or even better make the driver

>>> depends on the clock subsystem ?

>>

>> OK fair enough I can do that.

>>

>> I think Aspeed is merging its clock driver this kernel cycle

>> so all variant SoCs actually have clocks.

>

> So when I later in the patch series convert Aspeed to use this driver

> we get a problem because Joel is currently working on the clock

> driver for Aspeed and it's not ready for merge yet as it looks.

>

> I can of course wait with the Aspeed conversion to use the common

> driver.

>

> Also I can slap in a fixed-rate clock in the device tree @1 MHz.

> But that is cheating.


All of our clocks in the device tree are cheating at the moment :)

>

> So I guess I rest the Aspeed conversion.

> Joel: does your clock patch set cover the 1MHz used by

> the watchdog EXTCLK?


It doesn't; none of the documentation I have describes where it comes
from. I will add something in the next iteration.

Cheers,

Joel
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/watchdog/ftwdt010_wdt.c b/drivers/watchdog/ftwdt010_wdt.c
index ab38a3a89300..680279f5c679 100644
--- a/drivers/watchdog/ftwdt010_wdt.c
+++ b/drivers/watchdog/ftwdt010_wdt.c
@@ -5,6 +5,8 @@ 
  *
  * Inspired by the out-of-tree drivers from OpenWRT:
  * Copyright (C) 2009 Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
+ * Inspired by the MOXA ART driver from Jonas Jensen:
+ * Copyright (C) 2013 Jonas Jensen <jonas.jensen@gmail.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -18,9 +20,11 @@ 
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/watchdog.h>
+#include <linux/clk.h>
 
 #define FTWDT010_WDCOUNTER	0x0
 #define FTWDT010_WDLOAD		0x4
@@ -29,19 +33,21 @@ 
 
 #define WDRESTART_MAGIC		0x5AB9
 
-#define WDCR_CLOCK_5MHZ		BIT(4)
+#define WDCR_EXTCLK		BIT(4)
 #define WDCR_WDEXT		BIT(3)
 #define WDCR_WDINTR		BIT(2)
 #define WDCR_SYS_RST		BIT(1)
 #define WDCR_ENABLE		BIT(0)
 
-#define WDT_CLOCK		5000000		/* 5 MHz */
-
 struct ftwdt010_wdt {
 	struct watchdog_device	wdd;
 	struct device		*dev;
 	void __iomem		*base;
 	bool			has_irq;
+	struct clk		*pclk;
+	struct clk		*extclk;
+	unsigned int		clk_freq;
+	bool			use_extclk;
 };
 
 static inline
@@ -55,12 +61,13 @@  static int ftwdt010_wdt_start(struct watchdog_device *wdd)
 	struct ftwdt010_wdt *gwdt = to_ftwdt010_wdt(wdd);
 	u32 enable;
 
-	writel(wdd->timeout * WDT_CLOCK, gwdt->base + FTWDT010_WDLOAD);
+	writel(wdd->timeout * gwdt->clk_freq, gwdt->base + FTWDT010_WDLOAD);
 	writel(WDRESTART_MAGIC, gwdt->base + FTWDT010_WDRESTART);
 	/* set clock before enabling */
-	enable = WDCR_CLOCK_5MHZ | WDCR_SYS_RST;
+	enable = WDCR_SYS_RST;
+	if (gwdt->use_extclk)
+		enable |= WDCR_EXTCLK;
 	writel(enable, gwdt->base + FTWDT010_WDCR);
-	enable |= WDCR_CLOCK_5MHZ;
 	if (gwdt->has_irq)
 		enable |= WDCR_WDINTR;
 	enable |= WDCR_ENABLE;
@@ -125,6 +132,7 @@  static const struct watchdog_info ftwdt010_wdt_info = {
 static int ftwdt010_wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
 	struct resource *res;
 	struct ftwdt010_wdt *gwdt;
 	unsigned int reg;
@@ -140,11 +148,51 @@  static int ftwdt010_wdt_probe(struct platform_device *pdev)
 	if (IS_ERR(gwdt->base))
 		return PTR_ERR(gwdt->base);
 
+	gwdt->use_extclk = of_property_read_bool(np, "faraday,use-extclk");
+
+	gwdt->pclk = devm_clk_get(dev, "PCLK");
+	if (!IS_ERR(gwdt->pclk)) {
+		ret = clk_prepare_enable(gwdt->pclk);
+		if (ret) {
+			dev_err(&pdev->dev, "unable to enable PCLK\n");
+			return ret;
+		}
+		if (!gwdt->use_extclk)
+			gwdt->clk_freq = clk_get_rate(gwdt->pclk);
+	} else {
+		dev_info(dev, "PCLK clock not found assume always-on\n");
+	}
+
+	gwdt->extclk = devm_clk_get(dev, "EXTCLK");
+	if (!IS_ERR(gwdt->extclk)) {
+		/* Only enable and get frequency from EXTCLK if it's in use */
+		if (gwdt->use_extclk) {
+			ret = clk_prepare_enable(gwdt->extclk);
+			if (ret) {
+				dev_err(&pdev->dev,
+					"unable to enable EXTCLK\n");
+				return ret;
+			}
+			gwdt->clk_freq = clk_get_rate(gwdt->extclk);
+		}
+	} else {
+		if (of_device_is_compatible(np, "cortina,gemini-watchdog")) {
+			gwdt->clk_freq = 5000000;
+			gwdt->use_extclk = true;
+			dev_info(dev, "assume 5MHz EXTCLK on Gemini\n");
+		}
+	}
+
+	if (gwdt->clk_freq == 0) {
+		dev_err(dev, "no clocking available\n");
+		return -EINVAL;
+	}
+
 	gwdt->dev = dev;
 	gwdt->wdd.info = &ftwdt010_wdt_info;
 	gwdt->wdd.ops = &ftwdt010_wdt_ops;
 	gwdt->wdd.min_timeout = 1;
-	gwdt->wdd.max_timeout = 0xFFFFFFFF / WDT_CLOCK;
+	gwdt->wdd.max_timeout = UINT_MAX / gwdt->clk_freq;
 	gwdt->wdd.parent = dev;
 
 	/*
@@ -178,7 +226,21 @@  static int ftwdt010_wdt_probe(struct platform_device *pdev)
 
 	/* Set up platform driver data */
 	platform_set_drvdata(pdev, gwdt);
-	dev_info(dev, "FTWDT010 watchdog driver enabled\n");
+	dev_info(dev, "FTWDT010 watchdog driver @%uHz\n",
+		 gwdt->clk_freq);
+
+	return 0;
+}
+
+static int ftwdt010_wdt_remove(struct platform_device *pdev)
+{
+	struct ftwdt010_wdt *gwdt = platform_get_drvdata(pdev);
+
+	writel(0, gwdt->base + FTWDT010_WDCR);
+	if (!IS_ERR(gwdt->pclk))
+		clk_disable_unprepare(gwdt->pclk);
+	if (!IS_ERR(gwdt->extclk) && gwdt->use_extclk)
+		clk_disable_unprepare(gwdt->extclk);
 
 	return 0;
 }
@@ -225,6 +287,7 @@  MODULE_DEVICE_TABLE(of, ftwdt010_wdt_match);
 
 static struct platform_driver ftwdt010_wdt_driver = {
 	.probe		= ftwdt010_wdt_probe,
+	.remove		= ftwdt010_wdt_remove,
 	.driver		= {
 		.name	= "ftwdt010-wdt",
 		.of_match_table = of_match_ptr(ftwdt010_wdt_match),