| Message ID | 20170812184318.10144-5-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: Consolidate FTWDT010 derivatives | expand |
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
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
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
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
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
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
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
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
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),
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