* [PATCH 0/2] watchdog: Add support for ImgTec PowerDown Controller Watchdog Timer @ 2014-11-12 15:18 Naidu.Tellapati 2014-11-12 15:18 ` [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver Naidu.Tellapati 2014-11-12 15:18 ` [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation Naidu.Tellapati 0 siblings, 2 replies; 14+ messages in thread From: Naidu.Tellapati @ 2014-11-12 15:18 UTC (permalink / raw) To: wim, linux, James.Hartley, abrestic, Ezequiel.Garcia Cc: linux-watchdog, devicetree, Jude.Abraham, Naidu Tellapati From: Naidu Tellapati <Naidu.Tellapati@imgtec.com> Hi all, This patchset provides support for the PowerDown Controller (PDC) Watchdog Timer found on Pistachio SOC from Imagination Technologies (ImgTec). Upstream support for the SoC is on the way and will be submitted soon. The series is based on 3.18-rc4. We appreciate your feedback. Naidu Tellapati (2): watchdog: ImgTec PDC Watchdog Timer Driver DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation .../devicetree/bindings/watchdog/imgpdc-wdt.txt | 18 + drivers/watchdog/Kconfig | 167 +++------ drivers/watchdog/Makefile | 12 +- drivers/watchdog/imgpdc_wdt.c | 388 ++++++++++++++++++++ 4 files changed, 468 insertions(+), 117 deletions(-) create mode 100644 Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt create mode 100644 drivers/watchdog/imgpdc_wdt.c ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver 2014-11-12 15:18 [PATCH 0/2] watchdog: Add support for ImgTec PowerDown Controller Watchdog Timer Naidu.Tellapati @ 2014-11-12 15:18 ` Naidu.Tellapati 2014-11-13 4:56 ` Andrew Bresticker 2014-11-13 13:05 ` Ezequiel Garcia 2014-11-12 15:18 ` [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation Naidu.Tellapati 1 sibling, 2 replies; 14+ messages in thread From: Naidu.Tellapati @ 2014-11-12 15:18 UTC (permalink / raw) To: wim, linux, James.Hartley, abrestic, Ezequiel.Garcia Cc: linux-watchdog, devicetree, Jude.Abraham, Naidu Tellapati 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> --- drivers/watchdog/Kconfig | 167 ++++++------------ drivers/watchdog/Makefile | 12 +- drivers/watchdog/imgpdc_wdt.c | 388 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 450 insertions(+), 117 deletions(-) create mode 100644 drivers/watchdog/imgpdc_wdt.c diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index d0107d4..13fb1b2 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -87,15 +87,6 @@ config DA9055_WATCHDOG This driver can also be built as a module. If so, the module will be called da9055_wdt. -config DA9063_WATCHDOG - tristate "Dialog DA9063 Watchdog" - depends on MFD_DA9063 - select WATCHDOG_CORE - help - Support for the watchdog in the DA9063 PMIC. - - This driver can be built as a module. The module name is da9063_wdt. - config GPIO_WATCHDOG tristate "Watchdog device controlled through GPIO-line" depends on OF_GPIO @@ -104,16 +95,6 @@ config GPIO_WATCHDOG If you say yes here you get support for watchdog device controlled through GPIO-line. -config MENF21BMC_WATCHDOG - tristate "MEN 14F021P00 BMC Watchdog" - depends on MFD_MENF21BMC - select WATCHDOG_CORE - help - Say Y here to include support for the MEN 14F021P00 BMC Watchdog. - - This driver can also be built as a module. If so the module - will be called menf21bmc_wdt. - config WM831X_WATCHDOG tristate "WM831x watchdog" depends on MFD_WM831X @@ -130,16 +111,6 @@ config WM8350_WATCHDOG Support for the watchdog in the WM8350 AudioPlus PMIC. When the watchdog triggers the system will be reset. -config XILINX_WATCHDOG - tristate "Xilinx Watchdog timer" - depends on HAS_IOMEM - select WATCHDOG_CORE - help - Watchdog driver for the xps_timebase_wdt ip core. - - To compile this driver as a module, choose M here: the - module will be called of_xilinx_wdt. - # ALPHA Architecture # ARM Architecture @@ -167,14 +138,6 @@ config AT91SAM9X_WATCHDOG Watchdog timer embedded into AT91SAM9X and AT91CAP9 chips. This will reboot your system when the timeout is reached. -config CADENCE_WATCHDOG - tristate "Cadence Watchdog Timer" - depends on ARM - select WATCHDOG_CORE - help - Say Y here if you want to include support for the watchdog - timer in the Xilinx Zynq. - config 21285_WATCHDOG tristate "DC21285 watchdog" depends on FOOTBRIDGE @@ -300,7 +263,7 @@ config PNX4008_WATCHDOG config IOP_WATCHDOG tristate "IOP Watchdog" - depends on ARCH_IOP13XX + depends on PLAT_IOP select WATCHDOG_NOWAYOUT if (ARCH_IOP32X || ARCH_IOP33X) help Say Y here if to include support for the watchdog timer @@ -329,7 +292,7 @@ config DAVINCI_WATCHDOG config ORION_WATCHDOG tristate "Orion watchdog" - depends on ARCH_ORION5X || ARCH_DOVE || MACH_DOVE || ARCH_MVEBU + depends on ARCH_ORION5X || ARCH_KIRKWOOD || ARCH_DOVE select WATCHDOG_CORE help Say Y here if to include support for the watchdog timer @@ -337,17 +300,6 @@ config ORION_WATCHDOG To compile this driver as a module, choose M here: the module will be called orion_wdt. -config RN5T618_WATCHDOG - tristate "Ricoh RN5T618 watchdog" - depends on MFD_RN5T618 - select WATCHDOG_CORE - help - If you say yes here you get support for watchdog on the Ricoh - RN5T618 PMIC. - - This driver can also be built as a module. If so, the module - will be called rn5t618_wdt. - config SUNXI_WATCHDOG tristate "Allwinner SoCs watchdog support" depends on ARCH_SUNXI @@ -417,8 +369,6 @@ config MAX63XX_WATCHDOG config IMX2_WDT tristate "IMX2+ Watchdog" depends on ARCH_MXC - select REGMAP_MMIO - select WATCHDOG_CORE help This is the driver for the hardware watchdog on the Freescale IMX2 and later processors. @@ -471,40 +421,6 @@ config SIRFSOC_WATCHDOG Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When the watchdog triggers the system will be reset. -config TEGRA_WATCHDOG - tristate "Tegra watchdog" - depends on (ARCH_TEGRA || COMPILE_TEST) && HAS_IOMEM - select WATCHDOG_CORE - help - Say Y here to include support for the watchdog timer - embedded in NVIDIA Tegra SoCs. - - To compile this driver as a module, choose M here: the - module will be called tegra_wdt. - -config QCOM_WDT - tristate "QCOM watchdog" - depends on HAS_IOMEM - depends on ARCH_QCOM - select WATCHDOG_CORE - help - Say Y here to include Watchdog timer support for the watchdog found - on QCOM chipsets. Currently supported targets are the MSM8960, - APQ8064, and IPQ8064. - - To compile this driver as a module, choose M here: the - module will be called qcom_wdt. - -config MESON_WATCHDOG - tristate "Amlogic Meson SoCs watchdog support" - depends on ARCH_MESON - select WATCHDOG_CORE - help - Say Y here to include support for the watchdog timer - in Amlogic Meson SoCs. - To compile this driver as a module, choose M here: the - module will be called meson_wdt. - # AVR32 Architecture config AT32AP700X_WDT @@ -617,7 +533,7 @@ config GEODE_WDT config SC520_WDT tristate "AMD Elan SC520 processor Watchdog" - depends on MELAN + depends on X86 help This is the driver for the hardware watchdog built in to the AMD "Elan" SC520 microcomputer commonly used in embedded systems. @@ -727,19 +643,6 @@ config INTEL_SCU_WATCHDOG To compile this driver as a module, choose M here. -config INTEL_MID_WATCHDOG - tristate "Intel MID Watchdog Timer" - depends on X86_INTEL_MID - select WATCHDOG_CORE - ---help--- - Watchdog timer driver built into the Intel SCU for Intel MID - Platforms. - - This driver currently supports only the watchdog evolution - implementation in SCU, available for Merrifield generation. - - To compile this driver as a module, choose M here. - config ITCO_WDT tristate "Intel TCO Timer/Watchdog" depends on (X86 || IA64) && PCI @@ -912,7 +815,7 @@ config 60XX_WDT config SBC8360_WDT tristate "SBC8360 Watchdog Timer" - depends on X86_32 + depends on X86 ---help--- This is the driver for the hardware watchdog on the SBC8360 Single @@ -1015,6 +918,36 @@ config W83627HF_WDT Most people will say N. +config W83697HF_WDT + tristate "W83697HF/W83697HG Watchdog Timer" + depends on X86 + ---help--- + This is the driver for the hardware watchdog on the W83697HF/HG + chipset as used in Dedibox/VIA motherboards (and likely others). + This watchdog simply watches your kernel to make sure it doesn't + freeze, and if it does, it reboots your computer after a certain + amount of time. + + To compile this driver as a module, choose M here: the + module will be called w83697hf_wdt. + + Most people will say N. + +config W83697UG_WDT + tristate "W83697UG/W83697UF Watchdog Timer" + depends on X86 + ---help--- + This is the driver for the hardware watchdog on the W83697UG/UF + chipset as used in MSI Fuzzy CX700 VIA motherboards (and likely others). + This watchdog simply watches your kernel to make sure it doesn't + freeze, and if it does, it reboots your computer after a certain + amount of time. + + To compile this driver as a module, choose M here: the + module will be called w83697ug_wdt. + + Most people will say N. + config W83877F_WDT tristate "W83877F (EMACS) Watchdog Timer" depends on X86 @@ -1090,6 +1023,18 @@ config M54xx_WATCHDOG # MicroBlaze Architecture +config XILINX_WATCHDOG + tristate "Xilinx Watchdog timer" + depends on MICROBLAZE + ---help--- + Watchdog driver for the xps_timebase_wdt ip core. + + IMPORTANT: The xps_timebase_wdt parent must have the property + "clock-frequency" at device tree. + + To compile this driver as a module, choose M here: the + module will be called of_xilinx_wdt. + # MIPS Architecture config ATH79_WDT @@ -1215,7 +1160,7 @@ config BCM2835_WDT config BCM_KONA_WDT tristate "BCM Kona Watchdog" - depends on ARCH_BCM_MOBILE + depends on ARCH_BCM select WATCHDOG_CORE help Support for the watchdog timer on the following Broadcom BCM281xx @@ -1235,6 +1180,15 @@ config BCM_KONA_WDT_DEBUG If in doubt, say 'N'. +config IMGPDC_WDT + tristate "Imagination Technologies PDC Watchdog Timer" + help + Driver for Imagination Technologies PowerDown Controller + Watchdog Timer. + + To compile this driver as a loadable module, choose M here. + The module will be called imgpdc_wdt. + config LANTIQ_WDT tristate "Lantiq SoC watchdog" depends on LANTIQ @@ -1342,20 +1296,17 @@ config WATCHDOG_RTAS # S390 Architecture -config DIAG288_WATCHDOG - tristate "System z diag288 Watchdog" +config ZVM_WATCHDOG + tristate "z/VM Watchdog Timer" depends on S390 - select WATCHDOG_CORE help IBM s/390 and zSeries machines running under z/VM 5.1 or later provide a virtual watchdog timer to their guest that cause a user define Control Program command to be executed after a timeout. - LPAR provides a very similar interface. This driver handles - both. To compile this driver as a module, choose M here. The module - will be called diag288_wdt. + will be called vmwatchdog. # SUPERH (sh + sh64) Architecture diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index c569ec8..467973c 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -32,7 +32,6 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o -obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o obj-$(CONFIG_OMAP_WATCHDOG) += omap_wdt.o obj-$(CONFIG_TWL4030_WATCHDOG) += twl4030_wdt.o obj-$(CONFIG_21285_WATCHDOG) += wdt285.o @@ -48,7 +47,6 @@ obj-$(CONFIG_IOP_WATCHDOG) += iop_wdt.o obj-$(CONFIG_DAVINCI_WATCHDOG) += davinci_wdt.o obj-$(CONFIG_ORION_WATCHDOG) += orion_wdt.o obj-$(CONFIG_SUNXI_WATCHDOG) += sunxi_wdt.o -obj-$(CONFIG_RN5T618_WATCHDOG) += rn5t618_wdt.o obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o obj-$(CONFIG_STMP3XXX_RTC_WATCHDOG) += stmp3xxx_rtc_wdt.o obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o @@ -59,10 +57,7 @@ obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o -obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o -obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o -obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o @@ -111,12 +106,13 @@ obj-$(CONFIG_SMSC_SCH311X_WDT) += sch311x_wdt.o obj-$(CONFIG_SMSC37B787_WDT) += smsc37b787_wdt.o obj-$(CONFIG_VIA_WDT) += via_wdt.o obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o +obj-$(CONFIG_W83697HF_WDT) += w83697hf_wdt.o +obj-$(CONFIG_W83697UG_WDT) += w83697ug_wdt.o obj-$(CONFIG_W83877F_WDT) += w83877f_wdt.o obj-$(CONFIG_W83977F_WDT) += w83977f_wdt.o obj-$(CONFIG_MACHZ_WDT) += machzwd.o obj-$(CONFIG_SBC_EPX_C3_WATCHDOG) += sbc_epx_c3.o obj-$(CONFIG_INTEL_SCU_WATCHDOG) += intel_scu_watchdog.o -obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o # M32R Architecture @@ -142,6 +138,7 @@ obj-$(CONFIG_OCTEON_WDT) += octeon-wdt.o octeon-wdt-y := octeon-wdt-main.o octeon-wdt-nmi.o obj-$(CONFIG_LANTIQ_WDT) += lantiq_wdt.o obj-$(CONFIG_RALINK_WDT) += rt2880_wdt.o +obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o # PARISC Architecture @@ -157,7 +154,6 @@ obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o # S390 Architecture -obj-$(CONFIG_DIAG288_WATCHDOG) += diag288_wdt.o # SUPERH (sh + sh64) Architecture obj-$(CONFIG_SH_WDT) += shwdt.o @@ -177,10 +173,8 @@ obj-$(CONFIG_XEN_WDT) += xen_wdt.o # Architecture Independent obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o -obj-$(CONFIG_DA9063_WATCHDOG) += da9063_wdt.o obj-$(CONFIG_GPIO_WATCHDOG) += gpio_wdt.o obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o obj-$(CONFIG_WM8350_WATCHDOG) += wm8350_wdt.o obj-$(CONFIG_MAX63XX_WATCHDOG) += max63xx_wdt.o obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o -obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o 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> + +/* 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 */ + +/* 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)*/ + +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); + 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; + + 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; +} + + +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; + } + + 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; + } + + 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); + watchdog_set_drvdata(&pdc_wdt->wdt_dev, NULL); + + 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) ")"); + +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"); -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver 2014-11-12 15:18 ` [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver Naidu.Tellapati @ 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; 14+ messages in thread From: Andrew Bresticker @ 2014-11-13 4:56 UTC (permalink / raw) To: Naidu Tellapati Cc: wim, linux, James Hartley, Ezequiel Garcia, linux-watchdog, devicetree@vger.kernel.org, Jude.Abraham 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/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@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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver 2014-11-13 4:56 ` Andrew Bresticker @ 2014-11-13 12:58 ` Jude Abraham 2014-11-13 13:26 ` James Hogan 0 siblings, 1 reply; 14+ messages in thread From: Jude Abraham @ 2014-11-13 12:58 UTC (permalink / raw) To: Andrew Bresticker, Naidu Tellapati Cc: wim@iguana.be, linux@roeck-us.net, James Hartley, Ezequiel Garcia, linux-watchdog@vger.kernel.org, devicetree@vger.kernel.org, James Hogan 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver 2014-11-13 12:58 ` Jude Abraham @ 2014-11-13 13:26 ` James Hogan 2014-11-14 13:08 ` Naidu Tellapati 0 siblings, 1 reply; 14+ messages in thread From: James Hogan @ 2014-11-13 13:26 UTC (permalink / raw) To: Jude Abraham, Andrew Bresticker, Naidu Tellapati Cc: wim@iguana.be, linux@roeck-us.net, James Hartley, Ezequiel Garcia, linux-watchdog@vger.kernel.org, devicetree@vger.kernel.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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver 2014-11-13 13:26 ` James Hogan @ 2014-11-14 13:08 ` Naidu Tellapati 2014-11-14 14:08 ` James Hartley 0 siblings, 1 reply; 14+ messages in thread From: Naidu Tellapati @ 2014-11-14 13:08 UTC (permalink / raw) To: James Hogan, Jude Abraham, Andrew Bresticker Cc: wim@iguana.be, linux@roeck-us.net, James Hartley, Ezequiel Garcia, linux-watchdog@vger.kernel.org, devicetree@vger.kernel.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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver 2014-11-14 13:08 ` Naidu Tellapati @ 2014-11-14 14:08 ` James Hartley 0 siblings, 0 replies; 14+ 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@iguana.be, linux@roeck-us.net, Ezequiel Garcia, linux-watchdog@vger.kernel.org, devicetree@vger.kernel.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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver 2014-11-12 15:18 ` [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver Naidu.Tellapati 2014-11-13 4:56 ` Andrew Bresticker @ 2014-11-13 13:05 ` Ezequiel Garcia 1 sibling, 0 replies; 14+ messages in thread From: Ezequiel Garcia @ 2014-11-13 13:05 UTC (permalink / raw) To: Naidu.Tellapati, wim, linux, James.Hartley, abrestic, Jude.Abraham Cc: linux-watchdog, devicetree On 11/12/2014 12:18 PM, Naidu.Tellapati@imgtec.com 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation 2014-11-12 15:18 [PATCH 0/2] watchdog: Add support for ImgTec PowerDown Controller Watchdog Timer Naidu.Tellapati 2014-11-12 15:18 ` [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver Naidu.Tellapati @ 2014-11-12 15:18 ` Naidu.Tellapati 2014-11-13 4:09 ` Andrew Bresticker 1 sibling, 1 reply; 14+ messages in thread From: Naidu.Tellapati @ 2014-11-12 15:18 UTC (permalink / raw) To: wim, linux, James.Hartley, abrestic, Ezequiel.Garcia Cc: linux-watchdog, devicetree, Jude.Abraham, Naidu Tellapati 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> --- .../devicetree/bindings/watchdog/imgpdc-wdt.txt | 18 ++++++++++++++++++ 1 files changed, 18 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt 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" +- reg : Should contain WDT registers location and length +- clock-names: Should contain "wdt" +- clocks: phandles to input clocks +- interrupts : Should contain WDT interrupt + +Examples: + +wdt@18102100 { + compatible = "img,pistachio-pdc-wdt"; + reg = <0x18102100 0x20>; + clocks = <&pdc_wdt_clk>; + clock-names = "wdt"; + interrupts = <0 52 IRQ_TYPE_LEVEL_HIGH>; +}; -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation 2014-11-12 15:18 ` [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation Naidu.Tellapati @ 2014-11-13 4:09 ` Andrew Bresticker 2014-11-13 12:58 ` Jude Abraham 0 siblings, 1 reply; 14+ messages in thread From: Andrew Bresticker @ 2014-11-13 4:09 UTC (permalink / raw) To: Naidu Tellapati Cc: wim, linux, James Hartley, Ezequiel Garcia, linux-watchdog, devicetree@vger.kernel.org, Jude.Abraham 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". > +- 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation 2014-11-13 4:09 ` Andrew Bresticker @ 2014-11-13 12:58 ` Jude Abraham 2014-11-13 13:08 ` James Hogan 0 siblings, 1 reply; 14+ messages in thread From: Jude Abraham @ 2014-11-13 12:58 UTC (permalink / raw) To: Andrew Bresticker, Naidu Tellapati Cc: wim@iguana.be, linux@roeck-us.net, James Hartley, Ezequiel Garcia, linux-watchdog@vger.kernel.org, devicetree@vger.kernel.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] 14+ messages in thread
* Re: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation 2014-11-13 12:58 ` Jude Abraham @ 2014-11-13 13:08 ` James Hogan 2014-11-13 19:07 ` Andrew Bresticker 0 siblings, 1 reply; 14+ messages in thread From: James Hogan @ 2014-11-13 13:08 UTC (permalink / raw) To: Jude Abraham, Andrew Bresticker, Naidu Tellapati Cc: wim@iguana.be, linux@roeck-us.net, James Hartley, Ezequiel Garcia, linux-watchdog@vger.kernel.org, devicetree@vger.kernel.org Hi, On 13/11/14 12:58, Jude Abraham wrote: > On Wed, Nov 12, 2014 at 7:18 AM, <Naidu.Tellapati@imgtec.com> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation 2014-11-13 13:08 ` James Hogan @ 2014-11-13 19:07 ` Andrew Bresticker 2014-11-14 3:18 ` Naidu Tellapati 0 siblings, 1 reply; 14+ messages in thread From: Andrew Bresticker @ 2014-11-13 19:07 UTC (permalink / raw) To: James Hogan Cc: Jude Abraham, Naidu Tellapati, wim@iguana.be, linux@roeck-us.net, James Hartley, Ezequiel Garcia, linux-watchdog@vger.kernel.org, devicetree@vger.kernel.org On Thu, Nov 13, 2014 at 5:08 AM, James Hogan <james.hogan@imgtec.com> wrote: > Hi, > > On 13/11/14 12:58, Jude Abraham wrote: >> On Wed, Nov 12, 2014 at 7:18 AM, <Naidu.Tellapati@imgtec.com> 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation 2014-11-13 19:07 ` Andrew Bresticker @ 2014-11-14 3:18 ` Naidu Tellapati 0 siblings, 0 replies; 14+ messages in thread From: Naidu Tellapati @ 2014-11-14 3:18 UTC (permalink / raw) To: Andrew Bresticker, James Hogan Cc: Jude Abraham, wim@iguana.be, linux@roeck-us.net, James Hartley, Ezequiel Garcia, linux-watchdog@vger.kernel.org, devicetree@vger.kernel.org Hi James Hogan & Andrew, Many thanks for your feedback. > On Thu, Nov 13, 2014 at 5:08 AM, James Hogan <james.hogan@imgtec.com> wrote: >> Hi, >> >> On 13/11/14 12:58, Jude Abraham wrote: >>> On Wed, Nov 12, 2014 at 7:18 AM, <Naidu.Tellapati@imgtec.com> 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-11-14 14:07 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-12 15:18 [PATCH 0/2] watchdog: Add support for ImgTec PowerDown Controller Watchdog Timer Naidu.Tellapati 2014-11-12 15:18 ` [PATCH 1/2] watchdog: ImgTec PDC Watchdog Timer Driver Naidu.Tellapati 2014-11-13 4:56 ` Andrew Bresticker 2014-11-13 12:58 ` Jude Abraham 2014-11-13 13:26 ` James Hogan 2014-11-14 13:08 ` Naidu Tellapati 2014-11-14 14:08 ` James Hartley 2014-11-13 13:05 ` Ezequiel Garcia 2014-11-12 15:18 ` [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation Naidu.Tellapati 2014-11-13 4:09 ` Andrew Bresticker 2014-11-13 12:58 ` Jude Abraham 2014-11-13 13:08 ` James Hogan 2014-11-13 19:07 ` Andrew Bresticker 2014-11-14 3:18 ` Naidu Tellapati
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox