* Re: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation [not found] ` <1415805483-26268-3-git-send-email-Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> @ 2014-11-13 4:09 ` Andrew Bresticker [not found] ` <CAL1qeaHrjccd4KYAUMvWEKqhM9DFNyKWE=-2k0ux+76XR+Q5tQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: Andrew Bresticker @ 2014-11-13 4:09 UTC (permalink / raw) To: Naidu Tellapati Cc: wim-IQzOog9fTRqzQB+pC5nmwQ, linux-0h96xk9xTtrk1uMJSBkQmQ, James Hartley, Ezequiel Garcia, linux-watchdog-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <CAL1qeaHrjccd4KYAUMvWEKqhM9DFNyKWE=-2k0ux+76XR+Q5tQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* RE: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation [not found] ` <CAL1qeaHrjccd4KYAUMvWEKqhM9DFNyKWE=-2k0ux+76XR+Q5tQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2014-11-13 12:58 ` Jude Abraham [not found] ` <89F3BC60EA3A0141B1F6C2A661D95E5E3F192BAA-C8yLA94LPOy3snIXRfWIHVBRoQTxkR7k@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: Jude Abraham @ 2014-11-13 12:58 UTC (permalink / raw) To: Andrew Bresticker, Naidu Tellapati Cc: wim-IQzOog9fTRqzQB+pC5nmwQ@public.gmane.org, linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org, James Hartley, Ezequiel Garcia, linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, James Hogan 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <89F3BC60EA3A0141B1F6C2A661D95E5E3F192BAA-C8yLA94LPOy3snIXRfWIHVBRoQTxkR7k@public.gmane.org>]
* Re: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation [not found] ` <89F3BC60EA3A0141B1F6C2A661D95E5E3F192BAA-C8yLA94LPOy3snIXRfWIHVBRoQTxkR7k@public.gmane.org> @ 2014-11-13 13:08 ` James Hogan [not found] ` <5464AD4B.1000509-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: James Hogan @ 2014-11-13 13:08 UTC (permalink / raw) To: Jude Abraham, Andrew Bresticker, Naidu Tellapati Cc: wim-IQzOog9fTRqzQB+pC5nmwQ@public.gmane.org, linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org, James Hartley, Ezequiel Garcia, linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <5464AD4B.1000509-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation [not found] ` <5464AD4B.1000509-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> @ 2014-11-13 19:07 ` Andrew Bresticker [not found] ` <CAL1qeaHEGZWs8PPrX5cNpUWXDL3D_5Y2w772eKBEO0G+5q_p-g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: Andrew Bresticker @ 2014-11-13 19:07 UTC (permalink / raw) To: James Hogan Cc: Jude Abraham, Naidu Tellapati, wim-IQzOog9fTRqzQB+pC5nmwQ@public.gmane.org, linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org, James Hartley, Ezequiel Garcia, linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <CAL1qeaHEGZWs8PPrX5cNpUWXDL3D_5Y2w772eKBEO0G+5q_p-g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* RE: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation [not found] ` <CAL1qeaHEGZWs8PPrX5cNpUWXDL3D_5Y2w772eKBEO0G+5q_p-g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2014-11-14 3:18 ` Naidu Tellapati 0 siblings, 0 replies; 11+ messages in thread From: Naidu Tellapati @ 2014-11-14 3:18 UTC (permalink / raw) To: Andrew Bresticker, James Hogan Cc: Jude Abraham, wim-IQzOog9fTRqzQB+pC5nmwQ@public.gmane.org, linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org, James Hartley, Ezequiel Garcia, linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <1415805483-26268-2-git-send-email-Naidu.Tellapati@imgtec.com>]
[parent not found: <1415805483-26268-2-git-send-email-Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver [not found] ` <1415805483-26268-2-git-send-email-Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> @ 2014-11-13 4:56 ` Andrew Bresticker 2014-11-13 12:58 ` Jude Abraham 2014-11-13 13:05 ` Ezequiel Garcia 1 sibling, 1 reply; 11+ messages in thread From: Andrew Bresticker @ 2014-11-13 4:56 UTC (permalink / raw) To: Naidu Tellapati Cc: wim-IQzOog9fTRqzQB+pC5nmwQ, linux-0h96xk9xTtrk1uMJSBkQmQ, James Hartley, Ezequiel Garcia, linux-watchdog-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver 2014-11-13 4:56 ` [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver Andrew Bresticker @ 2014-11-13 12:58 ` Jude Abraham [not found] ` <89F3BC60EA3A0141B1F6C2A661D95E5E3F192BB4-C8yLA94LPOy3snIXRfWIHVBRoQTxkR7k@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: Jude Abraham @ 2014-11-13 12:58 UTC (permalink / raw) To: Andrew Bresticker, Naidu Tellapati Cc: wim-IQzOog9fTRqzQB+pC5nmwQ@public.gmane.org, linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org, James Hartley, Ezequiel Garcia, linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, James Hogan [-- 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <89F3BC60EA3A0141B1F6C2A661D95E5E3F192BB4-C8yLA94LPOy3snIXRfWIHVBRoQTxkR7k@public.gmane.org>]
* Re: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver [not found] ` <89F3BC60EA3A0141B1F6C2A661D95E5E3F192BB4-C8yLA94LPOy3snIXRfWIHVBRoQTxkR7k@public.gmane.org> @ 2014-11-13 13:26 ` James Hogan [not found] ` <5464B191.9020401-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: James Hogan @ 2014-11-13 13:26 UTC (permalink / raw) To: Jude Abraham, Andrew Bresticker, Naidu Tellapati Cc: wim-IQzOog9fTRqzQB+pC5nmwQ@public.gmane.org, linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org, James Hartley, Ezequiel Garcia, linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <5464B191.9020401-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>]
* RE: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver [not found] ` <5464B191.9020401-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> @ 2014-11-14 13:08 ` Naidu Tellapati [not found] ` <27E62D98F903554192E3C13AFCC91C3C2F505E3E-C8yLA94LPOy3snIXRfWIHVBRoQTxkR7k@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: Naidu Tellapati @ 2014-11-14 13:08 UTC (permalink / raw) To: James Hogan, Jude Abraham, Andrew Bresticker Cc: wim-IQzOog9fTRqzQB+pC5nmwQ@public.gmane.org, linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org, James Hartley, Ezequiel Garcia, linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <27E62D98F903554192E3C13AFCC91C3C2F505E3E-C8yLA94LPOy3snIXRfWIHVBRoQTxkR7k@public.gmane.org>]
* Re: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver [not found] ` <27E62D98F903554192E3C13AFCC91C3C2F505E3E-C8yLA94LPOy3snIXRfWIHVBRoQTxkR7k@public.gmane.org> @ 2014-11-14 14:08 ` James Hartley 0 siblings, 0 replies; 11+ messages in thread From: James Hartley @ 2014-11-14 14:08 UTC (permalink / raw) To: Naidu Tellapati Cc: James Hogan, Jude Abraham, Andrew Bresticker, wim-IQzOog9fTRqzQB+pC5nmwQ@public.gmane.org, linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org, Ezequiel Garcia, linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver [not found] ` <1415805483-26268-2-git-send-email-Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> 2014-11-13 4:56 ` [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver Andrew Bresticker @ 2014-11-13 13:05 ` Ezequiel Garcia 1 sibling, 0 replies; 11+ messages in thread From: Ezequiel Garcia @ 2014-11-13 13:05 UTC (permalink / raw) To: Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA, wim-IQzOog9fTRqzQB+pC5nmwQ, linux-0h96xk9xTtrk1uMJSBkQmQ, James.Hartley-1AXoQHu6uovQT0dZR+AlfA, abrestic-F7+t8E8rja9g9hUCZPvPmw, Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA Cc: linux-watchdog-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-11-14 14:08 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1415805483-26268-1-git-send-email-Naidu.Tellapati@imgtec.com>
[not found] ` <1415805483-26268-3-git-send-email-Naidu.Tellapati@imgtec.com>
[not found] ` <1415805483-26268-3-git-send-email-Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
2014-11-13 4:09 ` [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation Andrew Bresticker
[not found] ` <CAL1qeaHrjccd4KYAUMvWEKqhM9DFNyKWE=-2k0ux+76XR+Q5tQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-11-13 12:58 ` Jude Abraham
[not found] ` <89F3BC60EA3A0141B1F6C2A661D95E5E3F192BAA-C8yLA94LPOy3snIXRfWIHVBRoQTxkR7k@public.gmane.org>
2014-11-13 13:08 ` James Hogan
[not found] ` <5464AD4B.1000509-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
2014-11-13 19:07 ` Andrew Bresticker
[not found] ` <CAL1qeaHEGZWs8PPrX5cNpUWXDL3D_5Y2w772eKBEO0G+5q_p-g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-11-14 3:18 ` Naidu Tellapati
[not found] ` <1415805483-26268-2-git-send-email-Naidu.Tellapati@imgtec.com>
[not found] ` <1415805483-26268-2-git-send-email-Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
2014-11-13 4:56 ` [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver Andrew Bresticker
2014-11-13 12:58 ` Jude Abraham
[not found] ` <89F3BC60EA3A0141B1F6C2A661D95E5E3F192BB4-C8yLA94LPOy3snIXRfWIHVBRoQTxkR7k@public.gmane.org>
2014-11-13 13:26 ` James Hogan
[not found] ` <5464B191.9020401-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
2014-11-14 13:08 ` Naidu Tellapati
[not found] ` <27E62D98F903554192E3C13AFCC91C3C2F505E3E-C8yLA94LPOy3snIXRfWIHVBRoQTxkR7k@public.gmane.org>
2014-11-14 14:08 ` James Hartley
2014-11-13 13:05 ` Ezequiel Garcia
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).