Re: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation

Re: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation

[Andrew Bresticker]

On Wed, Nov 12, 2014 at 7:18 AM,  <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> From: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>
> Add the devicetree binding document for ImgTec PDC Watchdog Timer.
>
> Signed-off-by: Jude Abraham <Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>

> diff --git a/Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt b/Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt
> new file mode 100644
> index 0000000..2f13896
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt
> @@ -0,0 +1,18 @@
> +*ImgTec PowerDown Controller (PDC) Watchdog Timer (WDT)
> +
> +Required properties:
> +- compatible : Should be "img,pistachio-pdc-wdt"

I don't see anything Pistachio-specific about this driver or binding
and it looks like IP revision is probable via the WD_CORE_REV
register, so I think you can drop the "pistachio".

> +- reg : Should contain WDT registers location and length
> +- clock-names: Should contain "wdt"

I believe there are two clocks: the 32kHz watchdog timer operating
clock and the system interface gate clock.  Perhaps call them "wdt"
and "sys"?

> +- clocks: phandles to input clocks
> +- interrupts : Should contain WDT interrupt

Not sure the interrupt will really be necessary.

> +Examples:
> +
> +wdt@18102100 {
> +       compatible = "img,pistachio-pdc-wdt";
> +       reg = <0x18102100 0x20>;

The TRM I have shows the watchdog registers occupying a full 256 bytes.

> +       clocks = <&pdc_wdt_clk>;
> +       clock-names = "wdt";
> +       interrupts = <0 52 IRQ_TYPE_LEVEL_HIGH>;
> +};
> --
> 1.7.0.4
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation

[Jude Abraham]

Hi Andrew,

Many thanks for reviewing. Please see my inline comments below.


On Wed, Nov 12, 2014 at 7:18 AM,  <Naidu.Tellapati@imgtec.com> wrote:
> > From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
> >
> > Add the devicetree binding document for ImgTec PDC Watchdog Timer.
> >
> > Signed-off-by: Jude Abraham <Jude.Abraham@imgtec.com>
> > Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com>

> > diff --git a/Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt 
> > b/Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt
> > new file mode 100644
> > index 0000000..2f13896
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt
> >@@ -0,0 +1,18 @@
> > +*ImgTec PowerDown Controller (PDC) Watchdog Timer (WDT)
> > +
> > +Required properties:
> >  +- compatible : Should be "img,pistachio-pdc-wdt"

>  I don't see anything Pistachio-specific about this driver or binding and it looks like IP revision is probable
>  via the WD_CORE_REV register, so I think you can drop the "pistachio".

Ok. We will remove pistachio.

> > +- reg : Should contain WDT registers location and length
> > +- clock-names: Should contain "wdt"

>  I believe there are two clocks: the 32kHz watchdog timer operating clock and the system interface gate clock.  Perhaps call them "wdt"
> and "sys"?

Ok, We will add sys clock .

> > +- clocks: phandles to input clocks
> > +- interrupts : Should contain WDT interrupt

>  Not sure the interrupt will really be necessary.

Ok, We will remove the interrupt.

> > +Examples:
> > +
> > +wdt@18102100 {
> >  +       compatible = "img,pistachio-pdc-wdt";
> >  +       reg = <0x18102100 0x20>;

> The TRM I have shows the watchdog registers occupying a full 256 bytes.

OK, will correct it.

> +       clocks = <&pdc_wdt_clk>;
> +       clock-names = "wdt";
> +       interrupts = <0 52 IRQ_TYPE_LEVEL_HIGH>; };
> --
> 1.7.0.4
>

Thanks and regards,
Jude.

Re: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation

[James Hogan]

Hi,

On 13/11/14 12:58, Jude Abraham wrote:
> On Wed, Nov 12, 2014 at 7:18 AM,  <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>>> diff --git a/Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt 

>>> +- interrupts : Should contain WDT interrupt
> 
>>  Not sure the interrupt will really be necessary.
> 
> Ok, We will remove the interrupt.

Regardless, there is still an interrupt line coming out of the WDT and
interrupt status/clear/enable registers for the single reminder
interrupt. IMO it makes sense to describe it in DT even if the driver
doesn't [yet] make use it.

BTW, please do CC me on future versions of this patchset.

Cheers
James
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation

[Andrew Bresticker]

On Thu, Nov 13, 2014 at 5:08 AM, James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> Hi,
>
> On 13/11/14 12:58, Jude Abraham wrote:
>> On Wed, Nov 12, 2014 at 7:18 AM,  <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>>>> diff --git a/Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt
>
>>>> +- interrupts : Should contain WDT interrupt
>>
>>>  Not sure the interrupt will really be necessary.
>>
>> Ok, We will remove the interrupt.
>
> Regardless, there is still an interrupt line coming out of the WDT and
> interrupt status/clear/enable registers for the single reminder
> interrupt. IMO it makes sense to describe it in DT even if the driver
> doesn't [yet] make use it.

Yes, you're right.  Even if we don't currently use the interrupt, it's
a correct description of the hardware.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation

[Naidu Tellapati]

Hi James Hogan & Andrew,

Many thanks for your feedback.


> On Thu, Nov 13, 2014 at 5:08 AM, James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>> Hi,
>>
>> On 13/11/14 12:58, Jude Abraham wrote:
>>> On Wed, Nov 12, 2014 at 7:18 AM,  <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt
>>
>>>>> +- interrupts : Should contain WDT interrupt
>>>
>>>>  Not sure the interrupt will really be necessary.
>>>
>>> Ok, We will remove the interrupt.
>>
>> Regardless, there is still an interrupt line coming out of the WDT and
>> interrupt status/clear/enable registers for the single reminder
>> interrupt. IMO it makes sense to describe it in DT even if the driver
>> doesn't [yet] make use it.

> Yes, you're right.  Even if we don't currently use the interrupt, it's
> a correct description of the hardware.

OK. We will describe the interrupt in DT. But we will remove all references
to pre-timeout & interrupt from the driver code.


Regards,
Naidu.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver

[Andrew Bresticker]

On Wed, Nov 12, 2014 at 7:18 AM,  <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> From: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>
> This commit adds support for ImgTec PowerDown Controller Watchdog Timer.
>
> Signed-off-by: Jude Abraham <Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>

> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index d0107d4..13fb1b2 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig

> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index c569ec8..467973c 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile

These two diffs are full of unrelated changes for some reason.

> diff --git a/drivers/watchdog/imgpdc_wdt.c b/drivers/watchdog/imgpdc_wdt.c
> new file mode 100644
> index 0000000..df9ff1c
> --- /dev/null
> +++ b/drivers/watchdog/imgpdc_wdt.c
> @@ -0,0 +1,388 @@
> +/*
> + * Imagination Technologies PowerDown Controller Watchdog Timer.
> + *
> + * Copyright (c) 2014 Imagination Technologies Ltd.
> + *
> + * 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 published by
> + * the Free Software Foundation.
> + *
> + * Based on drivers/watchdog/sunxi_wdt.c Copyright (c) 2013 Carlo Caione
> + *                                                     2012 Henrik Nordstrom
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/watchdog.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/log2.h>
> +#include <linux/slab.h>

#includes should be sorted in alphabetical order.

> +/* registers */
> +#define PDC_WD_SW_RESET                        0x000
> +#define PDC_WD_CONFIG                  0x004
> +#define PDC_WD_TICKLE1                 0x008
> +#define PDC_WD_TICKLE2                 0x00c
> +#define PDC_WD_IRQ_STATUS              0x010
> +#define PDC_WD_IRQ_CLEAR               0x014
> +#define PDC_WD_IRQ_EN                  0x018
> +
> +/* field masks */
> +#define PDC_WD_CONFIG_REMIND           0x00001f00
> +#define PDC_WD_CONFIG_REMIND_SHIFT     8
> +#define PDC_WD_CONFIG_DELAY            0x0000001f
> +#define PDC_WD_CONFIG_DELAY_SHIFT      0
> +#define PDC_WD_TICKLE_STATUS           0x00000007
> +#define PDC_WD_TICKLE_STATUS_SHIFT     0
> +#define PDC_WD_IRQ_REMIND              0x00000001
> +
> +/* constants */
> +#define PDC_WD_TICKLE1_MAGIC           0xabcd1234
> +#define PDC_WD_TICKLE2_MAGIC           0x4321dcba
> +#define PDC_WD_TICKLE_STATUS_HRESET    0x0  /* Hard reset */
> +#define PDC_WD_TICKLE_STATUS_TIMEOUT   0x1  /* Timeout */
> +#define PDC_WD_TICKLE_STATUS_TICKLE    0x2  /* Tickled incorrectly */
> +#define PDC_WD_TICKLE_STATUS_SRESET    0x3  /* Soft reset */
> +#define PDC_WD_TICKLE_STATUS_USER      0x4  /* User reset */

Perhaps put the register field masks/shifts #define next to their
corresponding register #define?

Also, I prefer to #define the unshifted mask, but I'm not sure there's
any rule about that.

> +/* timeout in seconds */
> +#define PDC_WD_MIN_TIMEOUT             1
> +#define PDC_WD_MAX_TIMEOUT             131072
> +#define PDC_WD_DEFAULT_TIMEOUT         64
> +#define PDC_WD_DEFAULT_PRETIMEOUT      PDC_WD_MAX_TIMEOUT
> +#define MIN_TIMEOUT_SHIFT              14  /* Clock rate 32768Hz=2^(14+1)*/

The input clock is not fixed at 32kHz.  I believe it can be configured
to run at a different rate.

> +static int timeout = PDC_WD_DEFAULT_TIMEOUT;
> +static int pretimeout = PDC_WD_DEFAULT_PRETIMEOUT;
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +
> +struct pdc_wdt_dev {
> +       struct watchdog_device wdt_dev;
> +       int irq;
> +       struct clk *clk;
> +       void __iomem *base;
> +       unsigned int pretimeout;
> +};
> +
> +static int pdc_wdt_keepalive(struct watchdog_device *wdt_dev)
> +{
> +       struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +       writel(PDC_WD_TICKLE1_MAGIC, wdt->base + PDC_WD_TICKLE1);
> +       writel(PDC_WD_TICKLE2_MAGIC, wdt->base + PDC_WD_TICKLE2);
> +
> +       return 0;
> +}
> +
> +static int pdc_wdt_stop(struct watchdog_device *wdt_dev)
> +{
> +       unsigned int val;
> +       struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +       val = readl(wdt->base + PDC_WD_CONFIG);
> +       val &= ~BIT(31);

This should be a #define, e.g. PDC_WD_CONFIG_ENABLE.

> +       writel(val, wdt->base + PDC_WD_CONFIG);
> +       /* Must tickle to finish the stop */
> +       pdc_wdt_keepalive(wdt_dev);
> +
> +       return 0;
> +}
> +
> +static int pdc_wdt_set_timeout(struct watchdog_device *wdt_dev,
> +                              unsigned int new_timeout)
> +{
> +       unsigned int val;
> +       struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +       if (new_timeout < PDC_WD_MIN_TIMEOUT ||
> +           new_timeout > PDC_WD_MAX_TIMEOUT)
> +               return -EINVAL;
> +
> +       wdt->wdt_dev.timeout = new_timeout;
> +
> +       /* round up to the next power of 2 */
> +       new_timeout = order_base_2(new_timeout);
> +       val = readl(wdt->base + PDC_WD_CONFIG);
> +
> +       /* number of 32.768KHz clocks, 2^(n+1) (14 is 1 sec) */
> +       val &= ~PDC_WD_CONFIG_DELAY;
> +       val |= (new_timeout + MIN_TIMEOUT_SHIFT) << PDC_WD_CONFIG_DELAY_SHIFT;
> +       writel(val, wdt->base + PDC_WD_CONFIG);

Like I mentioned above, I don't think the input clock is fixed at
32kHz.  You need to program the right value based on the input clock's
rate.

> +
> +       return 0;
> +}
> +
> +static int pdc_wdt_set_pretimeout(struct watchdog_device *wdt_dev,
> +                                 unsigned int new_pretimeout)
> +{
> +       int delay;
> +       unsigned int val;
> +       struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +       /*
> +       * Pretimeout is measured in seconds before main timeout.
> +       * Subtract and round it once, and it will effectively change
> +       * if the main timeout is changed.
> +       */
> +       delay = wdt->wdt_dev.timeout;
> +       if (!new_pretimeout)
> +               new_pretimeout = PDC_WD_MAX_TIMEOUT;
> +       else if (new_pretimeout > 0 && new_pretimeout < delay)
> +               new_pretimeout = delay - new_pretimeout;
> +       else
> +               return -EINVAL;
> +
> +       pretimeout = new_pretimeout;
> +       new_pretimeout = ilog2(new_pretimeout);
> +       val = readl(wdt->base + PDC_WD_CONFIG);
> +
> +       /* number of 32.768KHz clocks, 2^(n+1) (14 is 1 sec) */
> +       val &= ~PDC_WD_CONFIG_REMIND;
> +       val |= (new_pretimeout + MIN_TIMEOUT_SHIFT)
> +                               << PDC_WD_CONFIG_REMIND_SHIFT;
> +       writel(val, wdt->base + PDC_WD_CONFIG);
> +       wdt->pretimeout = pretimeout;
> +
> +       return 0;
> +}
> +
> +/* Start the watchdog timer (delay should already be set */
> +static int pdc_wdt_start(struct watchdog_device *wdt_dev)
> +{
> +       int ret;
> +       unsigned int val;
> +       struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +       ret = pdc_wdt_set_timeout(&wdt->wdt_dev, wdt->wdt_dev.timeout);
> +       if (ret < 0)
> +               return ret;
> +
> +       val = readl(wdt->base + PDC_WD_CONFIG);
> +       val |= BIT(31);
> +       writel(val, wdt->base + PDC_WD_CONFIG);
> +
> +       return 0;
> +}
> +
> +static long pdc_wdt_ioctl(struct watchdog_device  *wdt_dev,
> +                         unsigned int cmd, unsigned long arg)
> +{
> +       int ret = -ENOIOCTLCMD;
> +       unsigned int new_pretimeout;
> +       void __user *argp = (void __user *)arg;
> +       int __user *p = argp;
> +       struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +       switch (cmd) {
> +       case WDIOC_SETPRETIMEOUT:
> +               if (get_user(new_pretimeout, p))
> +                       return -EFAULT;
> +
> +               ret = pdc_wdt_set_pretimeout(wdt_dev, new_pretimeout);
> +               if (ret < 0)
> +                       return ret;
> +
> +               /* fallthrough */
> +       case WDIOC_GETPRETIMEOUT:
> +               ret = put_user(wdt->pretimeout, (int __user *)arg);
> +               break;
> +       }
> +
> +       return ret;
> +}

I'm not sure what the point of this pre-timeout stuff is.  It looks
like it's just used to trigger an interrupt, which we then just
silently ACK.  Perhaps it can be removed, along with the interrupt
handling?

> +static irqreturn_t pdc_wdt_isr(int irq, void *dev_id)
> +{
> +       struct pdc_wdt_dev *pdc_wdt = platform_get_drvdata(dev_id);
> +       unsigned int stat = readl(pdc_wdt->base + PDC_WD_IRQ_STATUS);
> +       /*
> +        * The behaviour of the remind interrupt should depend on what userland
> +        * asks for, either do nothing, panic the system or inform userland.
> +        * Unfortunately this is not part of the main linux interface, and is
> +        * currently implemented only in the ipmi watchdog driver (in
> +        * drivers/char). This could be added at a later time.
> +        */
> +       writel(stat, pdc_wdt->base + PDC_WD_IRQ_CLEAR);
> +       return IRQ_HANDLED;
> +}
> +
> +static struct watchdog_info pdc_wdt_info = {
> +       .options                = WDIOF_SETTIMEOUT |
> +                                 WDIOF_KEEPALIVEPING |
> +                                 WDIOF_PRETIMEOUT |
> +                                 WDIOF_MAGICCLOSE,
> +       .identity = "PDC Watchdog",
> +};
> +
> +/* Kernel interface */
> +
> +static const struct watchdog_ops pdc_wdt_ops = {
> +       .owner          = THIS_MODULE,
> +       .start          = pdc_wdt_start,
> +       .stop           = pdc_wdt_stop,
> +       .ping           = pdc_wdt_keepalive,
> +       .set_timeout    = pdc_wdt_set_timeout,
> +       .ioctl          = pdc_wdt_ioctl,
> +};
> +
> +static int pdc_wdt_probe(struct platform_device *pdev)
> +{
> +       int ret, val, irq;
> +       struct resource *res;
> +       struct pdc_wdt_dev *pdc_wdt;
> +
> +       pdc_wdt = devm_kzalloc(&pdev->dev, sizeof(*pdc_wdt), GFP_KERNEL);
> +       if (!pdc_wdt)
> +               return -ENOMEM;
> +
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq < 0) {
> +               dev_err(&pdev->dev, "cannot find wdt IRQ resource\n");
> +               return irq;
> +       }
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       pdc_wdt->base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(pdc_wdt->base))
> +               return PTR_ERR(pdc_wdt->base);
> +
> +       pdc_wdt->clk = devm_clk_get(&pdev->dev, "wdt");
> +       if (IS_ERR(pdc_wdt->clk)) {
> +               dev_err(&pdev->dev, "failed to get the clock.\n");
> +               return PTR_ERR(pdc_wdt->clk);
> +       }
> +
> +       ret = clk_prepare_enable(pdc_wdt->clk);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "could not prepare or enable wdt clock.\n");
> +               return ret;
> +       }

Like I said in the binding doc, I think there are two clocks you need
to get and enable here.

> +       pdc_wdt->wdt_dev.info = &pdc_wdt_info;
> +       pdc_wdt->wdt_dev.ops = &pdc_wdt_ops;
> +       pdc_wdt->wdt_dev.timeout = PDC_WD_DEFAULT_TIMEOUT;
> +       pdc_wdt->wdt_dev.max_timeout = PDC_WD_MAX_TIMEOUT;
> +       pdc_wdt->wdt_dev.min_timeout = PDC_WD_MIN_TIMEOUT;
> +       pdc_wdt->wdt_dev.parent = &pdev->dev;
> +
> +       watchdog_init_timeout(&pdc_wdt->wdt_dev, timeout, &pdev->dev);
> +
> +       /* Enable remind interrupts */
> +       writel(PDC_WD_IRQ_REMIND, pdc_wdt->base + PDC_WD_IRQ_EN);
> +       pdc_wdt_stop(&pdc_wdt->wdt_dev);
> +       /* Set timeouts before userland has a chance to start the timer */
> +       pdc_wdt_set_timeout(&pdc_wdt->wdt_dev, timeout);
> +       pdc_wdt_set_pretimeout(&pdc_wdt->wdt_dev, pretimeout);
> +
> +       /* Find what caused the last reset */
> +       val = readl(pdc_wdt->base + PDC_WD_TICKLE1);
> +       val = (val & PDC_WD_TICKLE_STATUS) >> PDC_WD_TICKLE_STATUS_SHIFT;
> +       switch (val) {
> +       case PDC_WD_TICKLE_STATUS_TICKLE:
> +       case PDC_WD_TICKLE_STATUS_TIMEOUT:
> +               pdc_wdt->wdt_dev.bootstatus |= WDIOF_CARDRESET;
> +               dev_info(&pdev->dev,
> +                       "watchdog module last reset due to timeout\n");
> +               break;
> +       case PDC_WD_TICKLE_STATUS_HRESET:
> +               dev_info(&pdev->dev,
> +                       "watchdog module last reset due to hard reset\n");
> +               break;
> +       case PDC_WD_TICKLE_STATUS_SRESET:
> +               dev_info(&pdev->dev,
> +                       "watchdog module last reset due to soft reset\n");
> +               break;
> +       case PDC_WD_TICKLE_STATUS_USER:
> +               dev_info(&pdev->dev,
> +                       "watchdog module last reset due to user reset\n");
> +               break;
> +       default:
> +               dev_info(&pdev->dev,
> +                       "contains an illegal status code (%08x)\n", val);
> +               break;
> +       }

Set pdc_wdt->wdt_dev.bootstatus based on this value?

> +
> +       watchdog_set_nowayout(&pdc_wdt->wdt_dev, nowayout);
> +
> +       platform_set_drvdata(pdev, pdc_wdt);
> +       watchdog_set_drvdata(&pdc_wdt->wdt_dev, pdc_wdt);
> +
> +       ret = watchdog_register_device(&pdc_wdt->wdt_dev);
> +       if (ret) {
> +               clk_disable_unprepare(pdc_wdt->clk);
> +               return ret;
> +       }
> +
> +       ret = devm_request_irq(&pdev->dev, irq, pdc_wdt_isr,
> +                              IRQ_TYPE_LEVEL_HIGH, dev_name(&pdev->dev), pdev);
> +       if (ret) {
> +               watchdog_unregister_device(&pdc_wdt->wdt_dev);
> +               clk_disable_unprepare(pdc_wdt->clk);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int pdc_wdt_remove(struct platform_device *pdev)
> +{
> +       struct pdc_wdt_dev *pdc_wdt = platform_get_drvdata(pdev);
> +
> +       watchdog_unregister_device(&pdc_wdt->wdt_dev);
> +       clk_disable_unprepare(pdc_wdt->clk);

Probably should disable the watchdog as well.

> +       watchdog_set_drvdata(&pdc_wdt->wdt_dev, NULL);

Not necessary.

> +
> +       return 0;
> +}
> +
> +static void pdc_wdt_shutdown(struct platform_device *pdev)
> +{
> +       struct pdc_wdt_dev *pdc_wdt = platform_get_drvdata(pdev);
> +
> +       pdc_wdt_stop(&pdc_wdt->wdt_dev);
> +}
> +
> +static const struct of_device_id pdc_wdt_match[] = {
> +       { .compatible = "img,pistachio-pdc-wdt" },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, pdc_wdt_match);
> +
> +static struct platform_driver pdc_wdt_driver = {
> +       .driver = {
> +               .name = "imgpdc-wdt",
> +               .of_match_table = pdc_wdt_match,
> +       },
> +       .probe = pdc_wdt_probe,
> +       .remove = pdc_wdt_remove,
> +       .shutdown = pdc_wdt_shutdown,
> +};
> +module_platform_driver(pdc_wdt_driver);
> +
> +module_param(timeout, int, 0);
> +MODULE_PARM_DESC(timeout,
> +       "PDC watchdog timer delay in seconds ("
> +       __MODULE_STRING(PDC_WD_MIN_TIMEOUT)" <= timeout <= "
> +       __MODULE_STRING(PDC_WD_MAX_TIMEOUT)
> +       "; default = " __MODULE_STRING(PDC_WD_DEFAULT_TIMEOUT) ")");
> +
> +module_param(pretimeout, int, 0);
> +MODULE_PARM_DESC(pretimeout,
> +       "PDC watchdog timer remind in seconds ("
> +       __MODULE_STRING(PDC_WD_MIN_TIMEOUT)" <= pretimeout <= "
> +       __MODULE_STRING(PDC_WD_MAX_TIMEOUT)
> +       "; default = " __MODULE_STRING(PDC_WD_DEFAULT_PRETIMEOUT) ")");
> +
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout,
> +       "Watchdog cannot be stopped once started (default = "
> +               __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");

Maybe put this module_param stuff up where their corresponding
variables are defined?

> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Jude Abraham <Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>");
> +MODULE_AUTHOR("Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("Imagination Technologies PDC Watchdog Timer Driver");
> +MODULE_ALIAS("platform:img_pdc_wdt");

This driver probes via DT, so I don't think the MODULE_ALIAS is necessary.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver

[Jude Abraham]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 18356 bytes --]

Hi Andrew,

Many thanks for reviewing the code. Please see my inline comments below.


On Wed, Nov 12, 2014 at 7:18 AM,  <Naidu.Tellapati@imgtec.com> wrote:
> > From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
> > 
> >  This commit adds support for ImgTec PowerDown Controller Watchdog Timer.
> > 
> >  Signed-off-by: Jude Abraham <Jude.Abraham@imgtec.com>
> >  Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com>


> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile 
> > index c569ec8..467973c 100644
> >  --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile

> These two diffs are full of unrelated changes for some reason.

Sorry. There was a mistake from our end. We will correct it in the next Patch set.

> >  diff --git a/drivers/watchdog/imgpdc_wdt.c 
> >  b/drivers/watchdog/imgpdc_wdt.c new file mode 100644 index 
> >  0000000..df9ff1c
> >  --- /dev/null
> >  +++ b/drivers/watchdog/imgpdc_wdt.c
> >  @@ -0,0 +1,388 @@
> >  +/*
> >  + * Imagination Technologies PowerDown Controller Watchdog Timer.
> >  + *
> >  + * Copyright (c) 2014 Imagination Technologies Ltd.
> >  + *
> >  + * 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 
> >  +published by
> >  + * the Free Software Foundation.
> >  + *
> >  + * Based on drivers/watchdog/sunxi_wdt.c Copyright (c) 2013 Carlo Caione
> >  + *                                                     2012 Henrik Nordstrom
> >  + */
> >  +
> >  +#include <linux/clk.h>
> >  +#include <linux/module.h>
> >  +#include <linux/watchdog.h>
> >  +#include <linux/platform_device.h>
> >  +#include <linux/io.h>
> >  +#include <linux/interrupt.h>
> >  +#include <linux/log2.h>
> >  +#include <linux/slab.h>

> #includes should be sorted in alphabetical order.

Ok. Will correct this.

> >  +/* registers */
> >  +#define PDC_WD_SW_RESET                        0x000
> >  +#define PDC_WD_CONFIG                  0x004
> >  +#define PDC_WD_TICKLE1                 0x008
> >  +#define PDC_WD_TICKLE2                 0x00c
> >  +#define PDC_WD_IRQ_STATUS              0x010
> >  +#define PDC_WD_IRQ_CLEAR               0x014
> >  +#define PDC_WD_IRQ_EN                  0x018
> >  +
> >  +/* field masks */
> >  +#define PDC_WD_CONFIG_REMIND           0x00001f00
> >  +#define PDC_WD_CONFIG_REMIND_SHIFT     8
> > +#define PDC_WD_CONFIG_DELAY            0x0000001f
> >  +#define PDC_WD_CONFIG_DELAY_SHIFT      0
> >  +#define PDC_WD_TICKLE_STATUS           0x00000007
> >  +#define PDC_WD_TICKLE_STATUS_SHIFT     0
> >  +#define PDC_WD_IRQ_REMIND              0x00000001
> >  +
> >  +/* constants */
> >  +#define PDC_WD_TICKLE1_MAGIC           0xabcd1234
> >  +#define PDC_WD_TICKLE2_MAGIC           0x4321dcba
> >  +#define PDC_WD_TICKLE_STATUS_HRESET    0x0  /* Hard reset */
> >  +#define PDC_WD_TICKLE_STATUS_TIMEOUT   0x1  /* Timeout */
> >  +#define PDC_WD_TICKLE_STATUS_TICKLE    0x2  /* Tickled incorrectly */
> >  +#define PDC_WD_TICKLE_STATUS_SRESET    0x3  /* Soft reset */
> >  +#define PDC_WD_TICKLE_STATUS_USER      0x4  /* User reset */

> Perhaps put the register field masks/shifts #define next to their corresponding register #define?

Ok, will do it.

> Also, I prefer to #define the unshifted mask, but I'm not sure there's any rule about that.

Ok. Will do it.

> >  +/* timeout in seconds */
> >  +#define PDC_WD_MIN_TIMEOUT             1
> >  +#define PDC_WD_MAX_TIMEOUT             131072
> > +#define PDC_WD_DEFAULT_TIMEOUT         64
> >  +#define PDC_WD_DEFAULT_PRETIMEOUT      PDC_WD_MAX_TIMEOUT
> >  +#define MIN_TIMEOUT_SHIFT              14  /* Clock rate 32768Hz=2^(14+1)*/

> The input clock is not fixed at 32kHz.  I believe it can be configured to run at a different rate.

I think it is  a 32 Khz fixed clock to the block. We are speaking to my hardware team for confirmation.
We will address the review comment after receive feedback from  my hardware team.

> > +static int timeout = PDC_WD_DEFAULT_TIMEOUT; static int pretimeout = 
> > +PDC_WD_DEFAULT_PRETIMEOUT; static bool nowayout = WATCHDOG_NOWAYOUT;
> > +
> > +struct pdc_wdt_dev {
> > +       struct watchdog_device wdt_dev;
> > +       int irq;
>>  +       struct clk *clk;
> > +       void __iomem *base;
> > +       unsigned int pretimeout;
> > +};
> > +
> > +static int pdc_wdt_keepalive(struct watchdog_device *wdt_dev) {
> > +       struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> > +
> > +       writel(PDC_WD_TICKLE1_MAGIC, wdt->base + PDC_WD_TICKLE1);
> > +       writel(PDC_WD_TICKLE2_MAGIC, wdt->base + PDC_WD_TICKLE2);
> > +
> > +       return 0;
> > +}
> > +
> > +static int pdc_wdt_stop(struct watchdog_device *wdt_dev) {
> > +       unsigned int val;
> > +       struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> > +
> > +       val = readl(wdt->base + PDC_WD_CONFIG);
> > +       val &= ~BIT(31);

> This should be a #define, e.g. PDC_WD_CONFIG_ENABLE.

Ok, will do it.

> > +       writel(val, wdt->base + PDC_WD_CONFIG);
> > +       /* Must tickle to finish the stop */
> > +       pdc_wdt_keepalive(wdt_dev);
> > +
> >  +       return 0;
>  > +}
> >  +
> >  +static int pdc_wdt_set_timeout(struct watchdog_device *wdt_dev,
>  > +                              unsigned int new_timeout) {
> >  +       unsigned int val;
>  > +       struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
>  > +
>  > +       if (new_timeout < PDC_WD_MIN_TIMEOUT ||
> >  +           new_timeout > PDC_WD_MAX_TIMEOUT)
> >  +               return -EINVAL;
> >  +
>  > +       wdt->wdt_dev.timeout = new_timeout;
> >  +
> >  +       /* round up to the next power of 2 */
> >  +       new_timeout = order_base_2(new_timeout);
> >  +       val = readl(wdt->base + PDC_WD_CONFIG);
>  > +
> >  +       /* number of 32.768KHz clocks, 2^(n+1) (14 is 1 sec) */
> >  +       val &= ~PDC_WD_CONFIG_DELAY;
> >  +       val |= (new_timeout + MIN_TIMEOUT_SHIFT) << PDC_WD_CONFIG_DELAY_SHIFT;
> >  +       writel(val, wdt->base + PDC_WD_CONFIG);

> Like I mentioned above, I don't think the input clock is fixed at 32kHz.  You need to program the right value based on the input clock's rate.

I think it is  a 32 Khz fixed clock to the block. We are speaking to our  hardware team for confirmation. We will address the review comment
 after receive feedback from  my hardware team.

> > +
> > +       return 0;
> > +}
> > +
> > +static int pdc_wdt_set_pretimeout(struct watchdog_device *wdt_dev,
> > +                                 unsigned int new_pretimeout) {
> > +       int delay;
> > +       unsigned int val;
> > +       struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> > +
> > +       /*
> > +       * Pretimeout is measured in seconds before main timeout.
> > +       * Subtract and round it once, and it will effectively change
> > +       * if the main timeout is changed.
> > +       */
> > +       delay = wdt->wdt_dev.timeout;
> > +       if (!new_pretimeout)
> > +               new_pretimeout = PDC_WD_MAX_TIMEOUT;
> > +       else if (new_pretimeout > 0 && new_pretimeout < delay)
> > +               new_pretimeout = delay - new_pretimeout;
> > +       else
> > +               return -EINVAL;
> > +
> > +       pretimeout = new_pretimeout;
> > +       new_pretimeout = ilog2(new_pretimeout);
> > +       val = readl(wdt->base + PDC_WD_CONFIG);
> > +
> > +       /* number of 32.768KHz clocks, 2^(n+1) (14 is 1 sec) */
> > +       val &= ~PDC_WD_CONFIG_REMIND;
> > +       val |= (new_pretimeout + MIN_TIMEOUT_SHIFT)
> > +                               << PDC_WD_CONFIG_REMIND_SHIFT;
> > +       writel(val, wdt->base + PDC_WD_CONFIG);
> > +       wdt->pretimeout = pretimeout;
> > +
> > +       return 0;
> > +}
> > +
> > +/* Start the watchdog timer (delay should already be set */ static 
> > +int pdc_wdt_start(struct watchdog_device *wdt_dev) {
> > +       int ret;
> > +       unsigned int val;
> > +       struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> > +
> > +       ret = pdc_wdt_set_timeout(&wdt->wdt_dev, wdt->wdt_dev.timeout);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       val = readl(wdt->base + PDC_WD_CONFIG);
> > +       val |= BIT(31);
> > +       writel(val, wdt->base + PDC_WD_CONFIG);
> > +
> > +       return 0;
> > +}
> > +
> > +static long pdc_wdt_ioctl(struct watchdog_device  *wdt_dev,
> > +                         unsigned int cmd, unsigned long arg) {
> > +       int ret = -ENOIOCTLCMD;
> > +       unsigned int new_pretimeout;
> > +       void __user *argp = (void __user *)arg;
> > +       int __user *p = argp;
> > +       struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> > +
> > +       switch (cmd) {
> > +       case WDIOC_SETPRETIMEOUT:
> > +               if (get_user(new_pretimeout, p))
> > +                       return -EFAULT;
> > +
> > +               ret = pdc_wdt_set_pretimeout(wdt_dev, new_pretimeout);
> > +               if (ret < 0)
> > +                       return ret;
> > +
> > +               /* fallthrough */
> > +       case WDIOC_GETPRETIMEOUT:
> >  +               ret = put_user(wdt->pretimeout, (int __user *)arg);
> >  +               break;
> >  +       }
> >   +
> >   +       return ret;
> >  +}

>  I'm not sure what the point of this pre-timeout stuff is.  It looks like it's just used to trigger an interrupt, which we then just silently ACK.
> Perhaps it can be removed, along with the interrupt handling?

Ok, we will remove everything related to pre-timeout.

> >  +static irqreturn_t pdc_wdt_isr(int irq, void *dev_id) {
> >  +       struct pdc_wdt_dev *pdc_wdt = platform_get_drvdata(dev_id);
> > +       unsigned int stat = readl(pdc_wdt->base + PDC_WD_IRQ_STATUS);
> >  +       /*
> >  +        * The behaviour of the remind interrupt should depend on what userland
> > +        * asks for, either do nothing, panic the system or inform userland.
> >  +        * Unfortunately this is not part of the main linux interface, and is
> >  +        * currently implemented only in the ipmi watchdog driver (in
> >  +        * drivers/char). This could be added at a later time.
> >  +        */
> > +       writel(stat, pdc_wdt->base + PDC_WD_IRQ_CLEAR);
> >  +       return IRQ_HANDLED;
> > +}
> >  +
> > +static struct watchdog_info pdc_wdt_info = {
> > +       .options                = WDIOF_SETTIMEOUT |
> > +                                 WDIOF_KEEPALIVEPING |
> >  +                                 WDIOF_PRETIMEOUT |
> > +                                 WDIOF_MAGICCLOSE,
> > +       .identity = "PDC Watchdog",
> > +};
> >  +
> > +/* Kernel interface */
> > +
> > +static const struct watchdog_ops pdc_wdt_ops = {
> > +       .owner          = THIS_MODULE,
> > +       .start          = pdc_wdt_start,
> > +       .stop           = pdc_wdt_stop,
> > +       .ping           = pdc_wdt_keepalive,
> > +       .set_timeout    = pdc_wdt_set_timeout,
> > +       .ioctl          = pdc_wdt_ioctl,
> > +};
> > +
> > +static int pdc_wdt_probe(struct platform_device *pdev) {
> > +       int ret, val, irq;
> > +       struct resource *res;
> > +       struct pdc_wdt_dev *pdc_wdt;
> > +
> > +       pdc_wdt = devm_kzalloc(&pdev->dev, sizeof(*pdc_wdt), GFP_KERNEL);
> > +       if (!pdc_wdt)
> > +               return -ENOMEM;
> > +
> > +       irq = platform_get_irq(pdev, 0);
> > +       if (irq < 0) {
> > +               dev_err(&pdev->dev, "cannot find wdt IRQ resource\n");
> > +               return irq;
> > +       }
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       pdc_wdt->base = devm_ioremap_resource(&pdev->dev, res);
> > +       if (IS_ERR(pdc_wdt->base))
> > +               return PTR_ERR(pdc_wdt->base);
> > +
> > +       pdc_wdt->clk = devm_clk_get(&pdev->dev, "wdt");
> > +       if (IS_ERR(pdc_wdt->clk)) {
> > +               dev_err(&pdev->dev, "failed to get the clock.\n");
> > +               return PTR_ERR(pdc_wdt->clk);
> > +       }
> >  +
> > +       ret = clk_prepare_enable(pdc_wdt->clk);
> > +       if (ret < 0) {
> > +               dev_err(&pdev->dev, "could not prepare or enable wdt clock.\n");
> > +               return ret;
> > +       }

> Like I said in the binding doc, I think there are two clocks you need to get and enable here.

Ok, will do it.

> > +       pdc_wdt->wdt_dev.info = &pdc_wdt_info;
> > +       pdc_wdt->wdt_dev.ops = &pdc_wdt_ops;
> > +       pdc_wdt->wdt_dev.timeout = PDC_WD_DEFAULT_TIMEOUT;
> > +       pdc_wdt->wdt_dev.max_timeout = PDC_WD_MAX_TIMEOUT;
> > +       pdc_wdt->wdt_dev.min_timeout = PDC_WD_MIN_TIMEOUT;
> > +       pdc_wdt->wdt_dev.parent = &pdev->dev;
> > +
> > +       watchdog_init_timeout(&pdc_wdt->wdt_dev, timeout, &pdev->dev);
> > +
> > +       /* Enable remind interrupts */
> > +       writel(PDC_WD_IRQ_REMIND, pdc_wdt->base + PDC_WD_IRQ_EN);
> > +       pdc_wdt_stop(&pdc_wdt->wdt_dev);
> > +       /* Set timeouts before userland has a chance to start the timer */
> > +       pdc_wdt_set_timeout(&pdc_wdt->wdt_dev, timeout);
> > +       pdc_wdt_set_pretimeout(&pdc_wdt->wdt_dev, pretimeout);
> > +
> > +        /* Find what caused the last reset */
> > +       val = readl(pdc_wdt->base + PDC_WD_TICKLE1);
> > +       val = (val & PDC_WD_TICKLE_STATUS) >> PDC_WD_TICKLE_STATUS_SHIFT;
> > +       switch (val) {
> > +       case PDC_WD_TICKLE_STATUS_TICKLE:
> > +       case PDC_WD_TICKLE_STATUS_TIMEOUT:
> > +               pdc_wdt->wdt_dev.bootstatus |= WDIOF_CARDRESET;
> > +               dev_info(&pdev->dev,
> > +                       "watchdog module last reset due to timeout\n");
> > +               break;
> > +       case PDC_WD_TICKLE_STATUS_HRESET:
> > +               dev_info(&pdev->dev,
> > +                       "watchdog module last reset due to hard reset\n");
> > +               break;
> > +       case PDC_WD_TICKLE_STATUS_SRESET:
> > +               dev_info(&pdev->dev,
> > +                       "watchdog module last reset due to soft reset\n");
> > +               break;
> > +       case PDC_WD_TICKLE_STATUS_USER:
> > +               dev_info(&pdev->dev,
> > +                       "watchdog module last reset due to user reset\n");
> > +               break;
> > +       default:
> > +               dev_info(&pdev->dev,
> > +                       "contains an illegal status code (%08x)\n", val);
> > +               break;
> > +       }

> Set pdc_wdt->wdt_dev.bootstatus based on this value?

Ok, will do it.

> > +
> > +       watchdog_set_nowayout(&pdc_wdt->wdt_dev, nowayout);
> > +
> > +       platform_set_drvdata(pdev, pdc_wdt);
> > +       watchdog_set_drvdata(&pdc_wdt->wdt_dev, pdc_wdt);
> > +
> > +       ret = watchdog_register_device(&pdc_wdt->wdt_dev);
> > +       if (ret) {
> > +               clk_disable_unprepare(pdc_wdt->clk);
> > +               return ret;
> > +       }
> > +
> > +       ret = devm_request_irq(&pdev->dev, irq, pdc_wdt_isr,
> > +                              IRQ_TYPE_LEVEL_HIGH, dev_name(&pdev->dev), pdev);
>>  +       if (ret) {
>> +               watchdog_unregister_device(&pdc_wdt->wdt_dev);
> > +                clk_disable_unprepare(pdc_wdt->clk);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
>> +}
>> +
>>  +static int pdc_wdt_remove(struct platform_device *pdev) {
> > +       struct pdc_wdt_dev *pdc_wdt = platform_get_drvdata(pdev);
> > +
> > +       watchdog_unregister_device(&pdc_wdt->wdt_dev);
> > +       clk_disable_unprepare(pdc_wdt->clk);

>  Probably should disable the watchdog as well.

Yes. We will do it.

>>  +       watchdog_set_drvdata(&pdc_wdt->wdt_dev, NULL);

>  Not necessary.

Ok, will do it.

>>  +
> > +       return 0;
> > +}
> > +
> > +static void pdc_wdt_shutdown(struct platform_device *pdev) {
> > +       struct pdc_wdt_dev *pdc_wdt = platform_get_drvdata(pdev);
> > +
> > +       pdc_wdt_stop(&pdc_wdt->wdt_dev); }
> > +
> > +static const struct of_device_id pdc_wdt_match[] = {
> > +       { .compatible = "img,pistachio-pdc-wdt" },
> > +       {}
> > +};
> > +MODULE_DEVICE_TABLE(of, pdc_wdt_match);
> > +
> > +static struct platform_driver pdc_wdt_driver = {
> > +       .driver = {
> > +               .name = "imgpdc-wdt",
> > +               .of_match_table = pdc_wdt_match,
> > +       },
> > +       .probe = pdc_wdt_probe,
> > +       .remove = pdc_wdt_remove,
>>  +       .shutdown = pdc_wdt_shutdown,
> > +};
>>  +module_platform_driver(pdc_wdt_driver);
> > +
> > +module_param(timeout, int, 0);
> > +MODULE_PARM_DESC(timeout,
> > +       "PDC watchdog timer delay in seconds ("
> > +       __MODULE_STRING(PDC_WD_MIN_TIMEOUT)" <= timeout <= "
> > +       __MODULE_STRING(PDC_WD_MAX_TIMEOUT)
> > +       "; default = " __MODULE_STRING(PDC_WD_DEFAULT_TIMEOUT) ")");
> > +
>  > +module_param(pretimeout, int, 0);
> > +MODULE_PARM_DESC(pretimeout,
> > +       "PDC watchdog timer remind in seconds ("
> > +       __MODULE_STRING(PDC_WD_MIN_TIMEOUT)" <= pretimeout <= "
> > +       __MODULE_STRING(PDC_WD_MAX_TIMEOUT)
> > +       "; default = " __MODULE_STRING(PDC_WD_DEFAULT_PRETIMEOUT) 
> > +")");
> > +
> > +module_param(nowayout, bool, 0);
> > +MODULE_PARM_DESC(nowayout,
> > +       "Watchdog cannot be stopped once started (default = "
> > +               __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");

> Maybe put this module_param stuff up where their corresponding variables are defined?

Ok, will do it.

> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR("Jude Abraham <Jude.Abraham@imgtec.com>"); 
> > +MODULE_AUTHOR("Naidu Tellapati <Naidu.Tellapati@imgtec.com>"); 
> > +MODULE_DESCRIPTION("Imagination Technologies PDC Watchdog Timer 
> > +Driver"); MODULE_ALIAS("platform:img_pdc_wdt");

> This driver probes via DT, so I don't think the MODULE_ALIAS is necessary.

Ok, will do it.


Thanks and regards,
Jude.

N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

Re: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver

[James Hogan]

On 13/11/14 12:58, Jude Abraham wrote:
>>>  +/* timeout in seconds */
>>>  +#define PDC_WD_MIN_TIMEOUT             1
>>>  +#define PDC_WD_MAX_TIMEOUT             131072
>>> +#define PDC_WD_DEFAULT_TIMEOUT         64
>>>  +#define PDC_WD_DEFAULT_PRETIMEOUT      PDC_WD_MAX_TIMEOUT
>>>  +#define MIN_TIMEOUT_SHIFT              14  /* Clock rate 32768Hz=2^(14+1)*/
> 
>> The input clock is not fixed at 32kHz.  I believe it can be configured to run at a different rate.
> 
> I think it is  a 32 Khz fixed clock to the block. We are speaking to my hardware team for confirmation.
> We will address the review comment after receive feedback from  my hardware team.

It should ideally be 32KHz, but that doesn't mean it will be guaranteed
to be. The input clock rate is still dependent on the SoC clock setup to
provide the clock, and that can usually be reconfigured i.e. from a
dedicated external oscillator on the board if provided (hopefully
providing the right frequency), or derived from a shared oscillator of
some other frequency.

For TZ1090 SoC with this IP block, powering down the rest of the SoC
happened to reset the low power clock configuration and it would switch
clock source to the main oscillator with a fixed divide, which certainly
wasn't 32khz most of the time. Each of the low power drivers had to then
take this into account in their configuration (img-ir for IR timings,
wdt to a lesser extent, and most importantly rtc so as not to lose time
or wake up at the wrong time!).

Cheers
James
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver

[Naidu Tellapati]

Hi James & James,

> On 13/11/14 12:58, Jude Abraham wrote:
>>>>  +/* timeout in seconds */
>>>>  +#define PDC_WD_MIN_TIMEOUT             1
>>>>  +#define PDC_WD_MAX_TIMEOUT             131072
>>>> +#define PDC_WD_DEFAULT_TIMEOUT         64
>>>>  +#define PDC_WD_DEFAULT_PRETIMEOUT      PDC_WD_MAX_TIMEOUT
>>>>  +#define MIN_TIMEOUT_SHIFT              14  /* Clock rate 32768Hz=2^(14+1)*/
>>
>>> The input clock is not fixed at 32kHz.  I believe it can be configured to run at a different rate.
>>
>> I think it is  a 32 Khz fixed clock to the block. We are speaking to my hardware team for confirmation.
>> We will address the review comment after receive feedback from  my hardware team.

> It should ideally be 32KHz, but that doesn't mean it will be guaranteed
> to be. The input clock rate is still dependent on the SoC clock setup to
> provide the clock, and that can usually be reconfigured i.e. from a
> dedicated external oscillator on the board if provided (hopefully
> providing the right frequency), or derived from a shared oscillator of
> some other frequency.

> For TZ1090 SoC with this IP block, powering down the rest of the SoC
> happened to reset the low power clock configuration and it would switch
> clock source to the main oscillator with a fixed divide, which certainly
> wasn't 32khz most of the time. Each of the low power drivers had to then
> take this into account in their configuration (img-ir for IR timings,
> wdt to a lesser extent, and most importantly rtc so as not to lose time
>or wake up at the wrong time!).

Please suggest us any valid minimum and maximum clock rates for the Watchdog on Pistachio.

Thanks and regards,
Naidu.--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver

[James Hartley]

On 11/14/14 13:08, Naidu Tellapati wrote:
> Hi James & James,
>
>> On 13/11/14 12:58, Jude Abraham wrote:
>>>>>   +/* timeout in seconds */
>>>>>   +#define PDC_WD_MIN_TIMEOUT             1
>>>>>   +#define PDC_WD_MAX_TIMEOUT             131072
>>>>> +#define PDC_WD_DEFAULT_TIMEOUT         64
>>>>>   +#define PDC_WD_DEFAULT_PRETIMEOUT      PDC_WD_MAX_TIMEOUT
>>>>>   +#define MIN_TIMEOUT_SHIFT              14  /* Clock rate 32768Hz=2^(14+1)*/
>>>> The input clock is not fixed at 32kHz.  I believe it can be configured to run at a different rate.
>>> I think it is  a 32 Khz fixed clock to the block. We are speaking to my hardware team for confirmation.
>>> We will address the review comment after receive feedback from  my hardware team.
>> It should ideally be 32KHz, but that doesn't mean it will be guaranteed
>> to be. The input clock rate is still dependent on the SoC clock setup to
>> provide the clock, and that can usually be reconfigured i.e. from a
>> dedicated external oscillator on the board if provided (hopefully
>> providing the right frequency), or derived from a shared oscillator of
>> some other frequency.
>> For TZ1090 SoC with this IP block, powering down the rest of the SoC
>> happened to reset the low power clock configuration and it would switch
>> clock source to the main oscillator with a fixed divide, which certainly
>> wasn't 32khz most of the time. Each of the low power drivers had to then
>> take this into account in their configuration (img-ir for IR timings,
>> wdt to a lesser extent, and most importantly rtc so as not to lose time
>> or wake up at the wrong time!).
> Please suggest us any valid minimum and maximum clock rates for the Watchdog on Pistachio.
>
> Thanks and regards,
> Naidu.
There are 2 dividers in the clock path, with an input signal of 400MHz 
under normal conditions.  Each divider can divide by up to 128, so the 
minimum frequency is 24.414kHz.  The maximum frequency that could be 
tolerated is 50MHz.  As previously discussed the expected operating 
frequency is 32kHz.

James.

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

Re: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver

[Ezequiel Garcia]

On 11/12/2014 12:18 PM, Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org wrote:
[..]
> +
> +static int pdc_wdt_keepalive(struct watchdog_device *wdt_dev)
> +{
> +	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +	writel(PDC_WD_TICKLE1_MAGIC, wdt->base + PDC_WD_TICKLE1);
> +	writel(PDC_WD_TICKLE2_MAGIC, wdt->base + PDC_WD_TICKLE2);
> +
> +	return 0;
> +}
> +
> +static int pdc_wdt_stop(struct watchdog_device *wdt_dev)
> +{
> +	unsigned int val;
> +	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +	val = readl(wdt->base + PDC_WD_CONFIG);
> +	val &= ~BIT(31);
> +	writel(val, wdt->base + PDC_WD_CONFIG);
> +	/* Must tickle to finish the stop */
> +	pdc_wdt_keepalive(wdt_dev);
> +
> +	return 0;
> +}
> +
> +static int pdc_wdt_set_timeout(struct watchdog_device *wdt_dev,
> +			       unsigned int new_timeout)
> +{
> +	unsigned int val;
> +	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +	if (new_timeout < PDC_WD_MIN_TIMEOUT ||
> +	    new_timeout > PDC_WD_MAX_TIMEOUT)
> +		return -EINVAL;
> +
> +	wdt->wdt_dev.timeout = new_timeout;
> +
> +	/* round up to the next power of 2 */
> +	new_timeout = order_base_2(new_timeout);
> +	val = readl(wdt->base + PDC_WD_CONFIG);
> +
> +	/* number of 32.768KHz clocks, 2^(n+1) (14 is 1 sec) */
> +	val &= ~PDC_WD_CONFIG_DELAY;
> +	val |= (new_timeout + MIN_TIMEOUT_SHIFT) << PDC_WD_CONFIG_DELAY_SHIFT;
> +	writel(val, wdt->base + PDC_WD_CONFIG);
> +
> +	return 0;
> +}
> +
> +static int pdc_wdt_set_pretimeout(struct watchdog_device *wdt_dev,
> +				  unsigned int new_pretimeout)
> +{
> +	int delay;
> +	unsigned int val;
> +	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +       /*
> +	* Pretimeout is measured in seconds before main timeout.
> +	* Subtract and round it once, and it will effectively change
> +	* if the main timeout is changed.
> +	*/
> +	delay = wdt->wdt_dev.timeout;
> +	if (!new_pretimeout)
> +		new_pretimeout = PDC_WD_MAX_TIMEOUT;
> +	else if (new_pretimeout > 0 && new_pretimeout < delay)
> +		new_pretimeout = delay - new_pretimeout;
> +	else
> +		return -EINVAL;
> +
> +	pretimeout = new_pretimeout;
> +	new_pretimeout = ilog2(new_pretimeout);
> +	val = readl(wdt->base + PDC_WD_CONFIG);
> +
> +	/* number of 32.768KHz clocks, 2^(n+1) (14 is 1 sec) */
> +	val &= ~PDC_WD_CONFIG_REMIND;
> +	val |= (new_pretimeout + MIN_TIMEOUT_SHIFT)
> +				<< PDC_WD_CONFIG_REMIND_SHIFT;
> +	writel(val, wdt->base + PDC_WD_CONFIG);
> +	wdt->pretimeout = pretimeout;
> +
> +	return 0;
> +}
> +
> +/* Start the watchdog timer (delay should already be set */
> +static int pdc_wdt_start(struct watchdog_device *wdt_dev)
> +{
> +	int ret;
> +	unsigned int val;
> +	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +	ret = pdc_wdt_set_timeout(&wdt->wdt_dev, wdt->wdt_dev.timeout);
> +	if (ret < 0)
> +		return ret;
> +

Please double check it, but I think you don't need to set the timeout
here. You set the default at probe time; and then it'll get set by the
ioctl if it changes.

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