| Message ID | 20230414102342.23696-2-bbhushan2@marvell.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] dt-bindings: watchdog: marvell octeonTX2 GTI system atchdog driver | expand |
On 14/04/2023 12:23, Bharat Bhushan wrote: > GTI watchdog timer are programmed in "interrupt + del3t + reset mode" > and del3t traps are not enabled. > GTI watchdog exception flow is: > - 1st timer expiration generates watchdog interrupt. > - 2nd timer expiration is ignored > - On 3rd timer expiration will trigger a system-wide core reset. > > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> > --- > drivers/watchdog/Kconfig | 9 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/octeontx2_wdt.c | 238 +++++++++++++++++++++++++++++++ > 3 files changed, 248 insertions(+) > create mode 100644 drivers/watchdog/octeontx2_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index f0872970daf9..31ff282c62ad 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -2212,4 +2212,13 @@ config KEEMBAY_WATCHDOG > To compile this driver as a module, choose M here: the > module will be called keembay_wdt. > > +config OCTEONTX2_WATCHDOG > + tristate "OCTEONTX2 Watchdog driver" > + depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT) Why it cannot be compile tested on 32-bit? > + help > + OCTEONTX2 GTI hardware supports watchdog timer. This watchdog timer are > + programmed in "interrupt + del3t + reset" mode. On first expiry it will > + generate interrupt. Second expiry (del3t) is ignored and system will reset > + on final timer expiry. > + > endif # WATCHDOG > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 9cbf6580f16c..aa1b813ad1f9 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -230,3 +230,4 @@ obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o > obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o > obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o > obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o > +obj-$(CONFIG_OCTEONTX2_WATCHDOG) += octeontx2_wdt.o Please tell me that you added it in some reasonable place, not just at the end... The same in Kconfig. > diff --git a/drivers/watchdog/octeontx2_wdt.c b/drivers/watchdog/octeontx2_wdt.c > new file mode 100644 > index 000000000000..7b78a092e83f > --- /dev/null > +++ b/drivers/watchdog/octeontx2_wdt.c > @@ -0,0 +1,238 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Marvell Octeontx2 Watchdog driver > + * > + * Copyright (C) 2023 Marvell International 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. Drop boilerplate. > + */ > + > +#include <linux/module.h> > +#include <linux/cpu.h> > +#include <linux/interrupt.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/watchdog.h> > +#include <linux/sched/debug.h> > + > +#include <asm/arch_timer.h> > + > +/* GTI CWD Watchdog Registers */ > +#define GTI_CWD_GLOBAL_WDOG_IDX 63 > +#define GTI_CWD_WDOG (0x8 * GTI_CWD_GLOBAL_WDOG_IDX) > +#define GTI_CWD_WDOG_MODE_INT_DEL3T_RST 0x3 > +#define GTI_CWD_WDOG_MODE_MASK 0x3 > +#define GTI_CWD_WDOG_LEN_SHIFT 4 > +#define GTI_CWD_WDOG_CNT_SHIFT 20 > + > +/* GTI Per-core Watchdog Interrupt Register */ > +#define GTI_CWD_INT 0x200 > +#define GTI_CWD_INT_PENDING_STATUS (1ULL << GTI_CWD_GLOBAL_WDOG_IDX) > + > +/* GTI Per-core Watchdog Interrupt Enable Clear Register */ > +#define GTI_CWD_INT_ENA_CLR 0x210 > +#define GTI_CWD_INT_ENA_CLR_VAL (1ULL << GTI_CWD_GLOBAL_WDOG_IDX) > + > +/* GTI Per-core Watchdog Interrupt Enable Set Register */ > +#define GTI_CWD_INT_ENA_SET 0x218 > +#define GTI_CWD_INT_ENA_SET_VAL (1ULL << GTI_CWD_GLOBAL_WDOG_IDX) > + > +/* GTI Per-core Watchdog Poke Registers */ > +#define GTI_CWD_POKE (0x10000 + 0x8 * GTI_CWD_GLOBAL_WDOG_IDX) > +#define GTI_CWD_POKE_VAL (1ULL << GTI_CWD_GLOBAL_WDOG_IDX) > + > +struct octeontx2_wdt_priv { > + struct watchdog_device wdev; > + void __iomem *base; > + u64 clock_freq; > + int irq; > +}; > + > +static irqreturn_t octeontx2_wdt_interrupt(int irq, void *data) > +{ > + panic("Kernel Watchdog"); > + return IRQ_HANDLED; > +} > + > +static int octeontx2_wdt_ping(struct watchdog_device *wdev) > +{ > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > + > + writeq(GTI_CWD_POKE_VAL, priv->base + GTI_CWD_POKE); > + return 0; > +} > + > +static int octeontx2_wdt_start(struct watchdog_device *wdev) > +{ > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > + u64 regval; > + > + set_bit(WDOG_HW_RUNNING, &wdev->status); > + > + /* Clear any pending interrupt */ > + writeq(GTI_CWD_INT_PENDING_STATUS, priv->base + GTI_CWD_INT); > + > + /* Enable Interrupt */ > + writeq(GTI_CWD_INT_ENA_SET_VAL, priv->base + GTI_CWD_INT_ENA_SET); > + > + /* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */ > + regval = readq(priv->base + GTI_CWD_WDOG); > + regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST; > + writeq(regval, priv->base + GTI_CWD_WDOG); > + > + return 0; > +} > + > +static int octeontx2_wdt_stop(struct watchdog_device *wdev) > +{ > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > + u64 regval; > + > + /* Disable Interrupt */ > + writeq(GTI_CWD_INT_ENA_CLR_VAL, priv->base + GTI_CWD_INT_ENA_CLR); > + > + /* Set GTI_CWD_WDOG.Mode = 0 to stop the timer */ > + regval = readq(priv->base + GTI_CWD_WDOG); > + regval &= ~GTI_CWD_WDOG_MODE_MASK; > + writeq(regval, priv->base + GTI_CWD_WDOG); > + > + return 0; > +} > + > +static int octeontx2_wdt_settimeout(struct watchdog_device *wdev, > + unsigned int timeout) > +{ > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > + u64 timeout_wdog, regval; > + > + /* Update new timeout */ > + wdev->timeout = timeout; > + > + /* Get clock cycles from timeout in second */ > + timeout_wdog = (u64)timeout * priv->clock_freq; > + > + /* Watchdog counts in 1024 cycle steps */ > + timeout_wdog = timeout_wdog >> 10; > + > + /* > + * Hardware allows programming of upper 16-bits of 24-bits cycles > + * Round up and use upper 16-bits only. > + * Set max if timeout more than h/w supported > + */ > + timeout_wdog = (timeout_wdog + 0xff) >> 8; > + if (timeout_wdog >= 0x10000) > + timeout_wdog = 0xffff; > + > + /* > + * GTI_CWD_WDOG.LEN have only upper 16-bits of 24-bits > + * GTI_CWD_WDOG.CNT, need addition shift of 8. > + */ > + regval = readq(priv->base + GTI_CWD_WDOG); > + regval &= GTI_CWD_WDOG_MODE_MASK; > + regval |= ((timeout_wdog) << (GTI_CWD_WDOG_CNT_SHIFT + 8)) | > + (timeout_wdog << GTI_CWD_WDOG_LEN_SHIFT); > + writeq(regval, priv->base + GTI_CWD_WDOG); > + return 0; > +} > + > +static const struct watchdog_info octeontx2_wdt_ident = { > + .identity = "OcteonTX2 watchdog", > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | > + WDIOF_CARDRESET, Weird indentation in = > +}; > + > +static const struct watchdog_ops octeontx2_wdt_ops = { > + .owner = THIS_MODULE, > + .start = octeontx2_wdt_start, > + .stop = octeontx2_wdt_stop, > + .ping = octeontx2_wdt_ping, > + .set_timeout = octeontx2_wdt_settimeout, > +}; > + > +static int octeontx2_wdt_probe(struct platform_device *pdev) > +{ > + struct octeontx2_wdt_priv *priv; > + struct device *dev = &pdev->dev; > + struct watchdog_device *wdog_dev; > + int irq; > + int err; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(priv->base)) > + return dev_err_probe(&pdev->dev, PTR_ERR(priv->base), > + "reg property not valid/found\n"); > + > + priv->clock_freq = arch_timer_get_cntfrq(); > + > + wdog_dev = &priv->wdev; > + wdog_dev->info = &octeontx2_wdt_ident, > + wdog_dev->ops = &octeontx2_wdt_ops, > + wdog_dev->parent = dev; > + wdog_dev->min_timeout = 1; > + wdog_dev->max_timeout = 16; > + wdog_dev->max_hw_heartbeat_ms = 16000; > + wdog_dev->timeout = 8; > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(&pdev->dev, "IRQ resource not found\n"); return dev_err_probe also in other places > + return -ENODEV; > + } > + > + err = request_irq(irq, octeontx2_wdt_interrupt, 0, pdev->name, priv); > + if (err) { > + dev_err(dev, "cannot register interrupt handler %d\n", err); > + return err; > + } > + > + priv->irq = irq; > + watchdog_set_drvdata(wdog_dev, priv); > + platform_set_drvdata(pdev, priv); > + watchdog_init_timeout(wdog_dev, wdog_dev->timeout, dev); > + octeontx2_wdt_settimeout(wdog_dev, wdog_dev->timeout); > + watchdog_stop_on_reboot(wdog_dev); > + watchdog_stop_on_unregister(wdog_dev); > + > + err = devm_watchdog_register_device(dev, wdog_dev); > + if (err) { > + free_irq(irq, priv); > + return err; > + } > + > + dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev->timeout); > + return 0; > +} > + > +static int octeontx2_wdt_remove(struct platform_device *pdev) > +{ > + struct octeontx2_wdt_priv *priv = platform_get_drvdata(pdev); > + > + if (priv->irq) Is it possible? > + free_irq(priv->irq, priv); Anyway, your order of cleanup is a bit surprising. It is expected to be reversed from probe. In probe() you requested IRQs before watchdog, but cleanup will be done before watchdog release? This does not look right. > + > + return 0; > +} > + > +static const struct of_device_id octeontx2_wdt_of_match[] = { > + { .compatible = "marvell-octeontx2-wdt", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, octeontx2_wdt_of_match); Best regards, Krzysztof
Please see inline > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Friday, April 14, 2023 4:58 PM > To: Bharat Bhushan <bbhushan2@marvell.com>; wim@linux-watchdog.org; > linux@roeck-us.net; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > linux-watchdog@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver > > External Email > > ---------------------------------------------------------------------- > On 14/04/2023 12:23, Bharat Bhushan wrote: > > GTI watchdog timer are programmed in "interrupt + del3t + reset mode" > > and del3t traps are not enabled. > > GTI watchdog exception flow is: > > - 1st timer expiration generates watchdog interrupt. > > - 2nd timer expiration is ignored > > - On 3rd timer expiration will trigger a system-wide core reset. > > > > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> > > --- > > drivers/watchdog/Kconfig | 9 ++ > > drivers/watchdog/Makefile | 1 + > > drivers/watchdog/octeontx2_wdt.c | 238 > > +++++++++++++++++++++++++++++++ > > 3 files changed, 248 insertions(+) > > create mode 100644 drivers/watchdog/octeontx2_wdt.c > > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index > > f0872970daf9..31ff282c62ad 100644 > > --- a/drivers/watchdog/Kconfig > > +++ b/drivers/watchdog/Kconfig > > @@ -2212,4 +2212,13 @@ config KEEMBAY_WATCHDOG > > To compile this driver as a module, choose M here: the > > module will be called keembay_wdt. > > > > +config OCTEONTX2_WATCHDOG > > + tristate "OCTEONTX2 Watchdog driver" > > + depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT) > > Why it cannot be compile tested on 32-bit? Used in 64 bit configuration but no harm getting compile tested for 32bit. Will change > > > + help > > + OCTEONTX2 GTI hardware supports watchdog timer. This watchdog > timer are > > + programmed in "interrupt + del3t + reset" mode. On first expiry it will > > + generate interrupt. Second expiry (del3t) is ignored and system will reset > > + on final timer expiry. > > + > > endif # WATCHDOG > > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > > index 9cbf6580f16c..aa1b813ad1f9 100644 > > --- a/drivers/watchdog/Makefile > > +++ b/drivers/watchdog/Makefile > > @@ -230,3 +230,4 @@ obj-$(CONFIG_MENZ069_WATCHDOG) += > menz69_wdt.o > > obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o > > obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o > > obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o > > +obj-$(CONFIG_OCTEONTX2_WATCHDOG) += octeontx2_wdt.o > > Please tell me that you added it in some reasonable place, not just at the end... > The same in Kconfig. Is it alphabetical order, any suggestion? > > > diff --git a/drivers/watchdog/octeontx2_wdt.c > > b/drivers/watchdog/octeontx2_wdt.c > > new file mode 100644 > > index 000000000000..7b78a092e83f > > --- /dev/null > > +++ b/drivers/watchdog/octeontx2_wdt.c > > @@ -0,0 +1,238 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* Marvell Octeontx2 Watchdog driver > > + * > > + * Copyright (C) 2023 Marvell International 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. > > Drop boilerplate. > > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/cpu.h> > > +#include <linux/interrupt.h> > > +#include <linux/of_platform.h> > > +#include <linux/platform_device.h> > > +#include <linux/watchdog.h> > > +#include <linux/sched/debug.h> > > + > > +#include <asm/arch_timer.h> > > + > > +/* GTI CWD Watchdog Registers */ > > +#define GTI_CWD_GLOBAL_WDOG_IDX 63 > > +#define GTI_CWD_WDOG (0x8 * > GTI_CWD_GLOBAL_WDOG_IDX) > > +#define GTI_CWD_WDOG_MODE_INT_DEL3T_RST 0x3 > > +#define GTI_CWD_WDOG_MODE_MASK 0x3 > > +#define GTI_CWD_WDOG_LEN_SHIFT 4 > > +#define GTI_CWD_WDOG_CNT_SHIFT 20 > > + > > +/* GTI Per-core Watchdog Interrupt Register */ > > +#define GTI_CWD_INT 0x200 > > +#define GTI_CWD_INT_PENDING_STATUS (1ULL << > GTI_CWD_GLOBAL_WDOG_IDX) > > + > > +/* GTI Per-core Watchdog Interrupt Enable Clear Register */ > > +#define GTI_CWD_INT_ENA_CLR 0x210 > > +#define GTI_CWD_INT_ENA_CLR_VAL (1ULL << > GTI_CWD_GLOBAL_WDOG_IDX) > > + > > +/* GTI Per-core Watchdog Interrupt Enable Set Register */ > > +#define GTI_CWD_INT_ENA_SET 0x218 > > +#define GTI_CWD_INT_ENA_SET_VAL (1ULL << > GTI_CWD_GLOBAL_WDOG_IDX) > > + > > +/* GTI Per-core Watchdog Poke Registers */ > > +#define GTI_CWD_POKE (0x10000 + 0x8 * > GTI_CWD_GLOBAL_WDOG_IDX) > > +#define GTI_CWD_POKE_VAL (1ULL << GTI_CWD_GLOBAL_WDOG_IDX) > > + > > +struct octeontx2_wdt_priv { > > + struct watchdog_device wdev; > > + void __iomem *base; > > + u64 clock_freq; > > + int irq; > > +}; > > + > > +static irqreturn_t octeontx2_wdt_interrupt(int irq, void *data) { > > + panic("Kernel Watchdog"); > > + return IRQ_HANDLED; > > +} > > + > > +static int octeontx2_wdt_ping(struct watchdog_device *wdev) { > > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > > + > > + writeq(GTI_CWD_POKE_VAL, priv->base + GTI_CWD_POKE); > > + return 0; > > +} > > + > > +static int octeontx2_wdt_start(struct watchdog_device *wdev) { > > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > > + u64 regval; > > + > > + set_bit(WDOG_HW_RUNNING, &wdev->status); > > + > > + /* Clear any pending interrupt */ > > + writeq(GTI_CWD_INT_PENDING_STATUS, priv->base + GTI_CWD_INT); > > + > > + /* Enable Interrupt */ > > + writeq(GTI_CWD_INT_ENA_SET_VAL, priv->base + > GTI_CWD_INT_ENA_SET); > > + > > + /* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */ > > + regval = readq(priv->base + GTI_CWD_WDOG); > > + regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST; > > + writeq(regval, priv->base + GTI_CWD_WDOG); > > + > > + return 0; > > +} > > + > > +static int octeontx2_wdt_stop(struct watchdog_device *wdev) { > > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > > + u64 regval; > > + > > + /* Disable Interrupt */ > > + writeq(GTI_CWD_INT_ENA_CLR_VAL, priv->base + > GTI_CWD_INT_ENA_CLR); > > + > > + /* Set GTI_CWD_WDOG.Mode = 0 to stop the timer */ > > + regval = readq(priv->base + GTI_CWD_WDOG); > > + regval &= ~GTI_CWD_WDOG_MODE_MASK; > > + writeq(regval, priv->base + GTI_CWD_WDOG); > > + > > + return 0; > > +} > > + > > +static int octeontx2_wdt_settimeout(struct watchdog_device *wdev, > > + unsigned int timeout) > > +{ > > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > > + u64 timeout_wdog, regval; > > + > > + /* Update new timeout */ > > + wdev->timeout = timeout; > > + > > + /* Get clock cycles from timeout in second */ > > + timeout_wdog = (u64)timeout * priv->clock_freq; > > + > > + /* Watchdog counts in 1024 cycle steps */ > > + timeout_wdog = timeout_wdog >> 10; > > + > > + /* > > + * Hardware allows programming of upper 16-bits of 24-bits cycles > > + * Round up and use upper 16-bits only. > > + * Set max if timeout more than h/w supported > > + */ > > + timeout_wdog = (timeout_wdog + 0xff) >> 8; > > + if (timeout_wdog >= 0x10000) > > + timeout_wdog = 0xffff; > > + > > + /* > > + * GTI_CWD_WDOG.LEN have only upper 16-bits of 24-bits > > + * GTI_CWD_WDOG.CNT, need addition shift of 8. > > + */ > > + regval = readq(priv->base + GTI_CWD_WDOG); > > + regval &= GTI_CWD_WDOG_MODE_MASK; > > + regval |= ((timeout_wdog) << (GTI_CWD_WDOG_CNT_SHIFT + 8)) | > > + (timeout_wdog << GTI_CWD_WDOG_LEN_SHIFT); > > + writeq(regval, priv->base + GTI_CWD_WDOG); > > + return 0; > > +} > > + > > +static const struct watchdog_info octeontx2_wdt_ident = { > > + .identity = "OcteonTX2 watchdog", > > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | > WDIOF_MAGICCLOSE | > > + WDIOF_CARDRESET, > > Weird indentation in = Do not know how it comes in email, they are correctly aligned when look into code. > > > +}; > > + > > +static const struct watchdog_ops octeontx2_wdt_ops = { > > + .owner = THIS_MODULE, > > + .start = octeontx2_wdt_start, > > + .stop = octeontx2_wdt_stop, > > + .ping = octeontx2_wdt_ping, > > + .set_timeout = octeontx2_wdt_settimeout, }; > > + > > +static int octeontx2_wdt_probe(struct platform_device *pdev) { > > + struct octeontx2_wdt_priv *priv; > > + struct device *dev = &pdev->dev; > > + struct watchdog_device *wdog_dev; > > + int irq; > > + int err; > > + > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(priv->base)) > > + return dev_err_probe(&pdev->dev, PTR_ERR(priv->base), > > + "reg property not valid/found\n"); > > + > > + priv->clock_freq = arch_timer_get_cntfrq(); > > + > > + wdog_dev = &priv->wdev; > > + wdog_dev->info = &octeontx2_wdt_ident, > > + wdog_dev->ops = &octeontx2_wdt_ops, > > + wdog_dev->parent = dev; > > + wdog_dev->min_timeout = 1; > > + wdog_dev->max_timeout = 16; > > + wdog_dev->max_hw_heartbeat_ms = 16000; > > + wdog_dev->timeout = 8; > > + > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) { > > + dev_err(&pdev->dev, "IRQ resource not found\n"); > > return dev_err_probe > also in other places okay > > > + return -ENODEV; > > + } > > + > > + err = request_irq(irq, octeontx2_wdt_interrupt, 0, pdev->name, priv); > > + if (err) { > > + dev_err(dev, "cannot register interrupt handler %d\n", err); > > + return err; > > + } > > + > > + priv->irq = irq; > > + watchdog_set_drvdata(wdog_dev, priv); > > + platform_set_drvdata(pdev, priv); > > + watchdog_init_timeout(wdog_dev, wdog_dev->timeout, dev); > > + octeontx2_wdt_settimeout(wdog_dev, wdog_dev->timeout); > > + watchdog_stop_on_reboot(wdog_dev); > > + watchdog_stop_on_unregister(wdog_dev); > > + > > + err = devm_watchdog_register_device(dev, wdog_dev); > > + if (err) { > > + free_irq(irq, priv); > > + return err; > > + } > > + > > + dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev- > >timeout); > > + return 0; > > +} > > + > > +static int octeontx2_wdt_remove(struct platform_device *pdev) { > > + struct octeontx2_wdt_priv *priv = platform_get_drvdata(pdev); > > + > > + if (priv->irq) > > Is it possible? > > > + free_irq(priv->irq, priv); > > Anyway, your order of cleanup is a bit surprising. It is expected to be reversed > from probe. In probe() you requested IRQs before watchdog, but cleanup will be > done before watchdog release? This does not look right. Watchdog release happen outside this driver as we used devm_*. Will convert request_irq to devm_request_irq(). Thanks -Bharat > > > + > > + return 0; > > +} > > + > > +static const struct of_device_id octeontx2_wdt_of_match[] = { > > + { .compatible = "marvell-octeontx2-wdt", }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(of, octeontx2_wdt_of_match); > > Best regards, > Krzysztof
On 14/04/2023 13:44, Bharat Bhushan wrote: > Please see inline > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Friday, April 14, 2023 4:58 PM >> To: Bharat Bhushan <bbhushan2@marvell.com>; wim@linux-watchdog.org; >> linux@roeck-us.net; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; >> linux-watchdog@vger.kernel.org; devicetree@vger.kernel.org; linux- >> kernel@vger.kernel.org >> Subject: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver >> >> External Email >> >> ---------------------------------------------------------------------- >> On 14/04/2023 12:23, Bharat Bhushan wrote: >>> GTI watchdog timer are programmed in "interrupt + del3t + reset mode" >>> and del3t traps are not enabled. >>> GTI watchdog exception flow is: >>> - 1st timer expiration generates watchdog interrupt. >>> - 2nd timer expiration is ignored >>> - On 3rd timer expiration will trigger a system-wide core reset. >>> >>> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> >>> --- >>> drivers/watchdog/Kconfig | 9 ++ >>> drivers/watchdog/Makefile | 1 + >>> drivers/watchdog/octeontx2_wdt.c | 238 >>> +++++++++++++++++++++++++++++++ >>> 3 files changed, 248 insertions(+) >>> create mode 100644 drivers/watchdog/octeontx2_wdt.c >>> >>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index >>> f0872970daf9..31ff282c62ad 100644 >>> --- a/drivers/watchdog/Kconfig >>> +++ b/drivers/watchdog/Kconfig >>> @@ -2212,4 +2212,13 @@ config KEEMBAY_WATCHDOG >>> To compile this driver as a module, choose M here: the >>> module will be called keembay_wdt. >>> >>> +config OCTEONTX2_WATCHDOG >>> + tristate "OCTEONTX2 Watchdog driver" >>> + depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT) >> >> Why it cannot be compile tested on 32-bit? > > Used in 64 bit configuration but no harm getting compile tested for 32bit. > Will change > >> >>> + help >>> + OCTEONTX2 GTI hardware supports watchdog timer. This watchdog >> timer are >>> + programmed in "interrupt + del3t + reset" mode. On first expiry it will >>> + generate interrupt. Second expiry (del3t) is ignored and system will reset >>> + on final timer expiry. >>> + >>> endif # WATCHDOG >>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile >>> index 9cbf6580f16c..aa1b813ad1f9 100644 >>> --- a/drivers/watchdog/Makefile >>> +++ b/drivers/watchdog/Makefile >>> @@ -230,3 +230,4 @@ obj-$(CONFIG_MENZ069_WATCHDOG) += >> menz69_wdt.o >>> obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o >>> obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o >>> obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o >>> +obj-$(CONFIG_OCTEONTX2_WATCHDOG) += octeontx2_wdt.o >> >> Please tell me that you added it in some reasonable place, not just at the end... >> The same in Kconfig. > > Is it alphabetical order, any suggestion? 'O' is not after 'S', thus for sure you did not add it in alphabetical order. Or what is a question? If so, then yes, usually we try to keep alphabetical order. Anyway adding entries to the end is conflictprone. >>> + dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev- >>> timeout); >>> + return 0; >>> +} >>> + >>> +static int octeontx2_wdt_remove(struct platform_device *pdev) { >>> + struct octeontx2_wdt_priv *priv = platform_get_drvdata(pdev); >>> + >>> + if (priv->irq) >> >> Is it possible? You did not reply, so I assume it is not possible. Drop. >> >>> + free_irq(priv->irq, priv); >> >> Anyway, your order of cleanup is a bit surprising. It is expected to be reversed >> from probe. In probe() you requested IRQs before watchdog, but cleanup will be >> done before watchdog release? This does not look right. > > Watchdog release happen outside this driver as we used devm_*. I know, this is why I raised the question. Don't repeat the obvious but rather address the problem mentioned here. > Will convert request_irq to devm_request_irq(). If interrupt is not shared, then looks like correct approach. Best regards, Krzysztof
On Fri, Apr 14, 2023 at 03:53:42PM +0530, Bharat Bhushan wrote: > GTI watchdog timer are programmed in "interrupt + del3t + reset mode" > and del3t traps are not enabled. > GTI watchdog exception flow is: > - 1st timer expiration generates watchdog interrupt. > - 2nd timer expiration is ignored > - On 3rd timer expiration will trigger a system-wide core reset. > This means the interrupt should not result in a panic, the programmed timeout value should be considered a pretimeout which is set to (timeout / 3), and the interrupt handler should call watchdog_notify_pretimeout(). Either case, the above should be documented in the driver but does not make much if any sense as patch description. If not, what are the other modes, and why is this mode used instead of any of those modes ? > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> > --- > drivers/watchdog/Kconfig | 9 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/octeontx2_wdt.c | 238 +++++++++++++++++++++++++++++++ > 3 files changed, 248 insertions(+) > create mode 100644 drivers/watchdog/octeontx2_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index f0872970daf9..31ff282c62ad 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -2212,4 +2212,13 @@ config KEEMBAY_WATCHDOG > To compile this driver as a module, choose M here: the > module will be called keembay_wdt. > > +config OCTEONTX2_WATCHDOG > + tristate "OCTEONTX2 Watchdog driver" > + depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT) > + help > + OCTEONTX2 GTI hardware supports watchdog timer. This watchdog timer are > + programmed in "interrupt + del3t + reset" mode. On first expiry it will > + generate interrupt. Second expiry (del3t) is ignored and system will reset > + on final timer expiry. > + The above should be part of the in-driver documentation but those details should not be in Kconfig. Is the documentation available in public ? I'd like to check if this mode, especially the ignored del3t mode, really makes sense. > endif # WATCHDOG > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 9cbf6580f16c..aa1b813ad1f9 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -230,3 +230,4 @@ obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o > obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o > obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o > obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o > +obj-$(CONFIG_OCTEONTX2_WATCHDOG) += octeontx2_wdt.o > diff --git a/drivers/watchdog/octeontx2_wdt.c b/drivers/watchdog/octeontx2_wdt.c > new file mode 100644 > index 000000000000..7b78a092e83f > --- /dev/null > +++ b/drivers/watchdog/octeontx2_wdt.c > @@ -0,0 +1,238 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Marvell Octeontx2 Watchdog driver > + * > + * Copyright (C) 2023 Marvell International 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. > + */ > + > +#include <linux/module.h> > +#include <linux/cpu.h> > +#include <linux/interrupt.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/watchdog.h> > +#include <linux/sched/debug.h> What is this include for ? > + > +#include <asm/arch_timer.h> > + > +/* GTI CWD Watchdog Registers */ > +#define GTI_CWD_GLOBAL_WDOG_IDX 63 > +#define GTI_CWD_WDOG (0x8 * GTI_CWD_GLOBAL_WDOG_IDX) > +#define GTI_CWD_WDOG_MODE_INT_DEL3T_RST 0x3 > +#define GTI_CWD_WDOG_MODE_MASK 0x3 > +#define GTI_CWD_WDOG_LEN_SHIFT 4 > +#define GTI_CWD_WDOG_CNT_SHIFT 20 > + > +/* GTI Per-core Watchdog Interrupt Register */ > +#define GTI_CWD_INT 0x200 > +#define GTI_CWD_INT_PENDING_STATUS (1ULL << GTI_CWD_GLOBAL_WDOG_IDX) > + > +/* GTI Per-core Watchdog Interrupt Enable Clear Register */ > +#define GTI_CWD_INT_ENA_CLR 0x210 > +#define GTI_CWD_INT_ENA_CLR_VAL (1ULL << GTI_CWD_GLOBAL_WDOG_IDX) > + > +/* GTI Per-core Watchdog Interrupt Enable Set Register */ > +#define GTI_CWD_INT_ENA_SET 0x218 > +#define GTI_CWD_INT_ENA_SET_VAL (1ULL << GTI_CWD_GLOBAL_WDOG_IDX) > + > +/* GTI Per-core Watchdog Poke Registers */ > +#define GTI_CWD_POKE (0x10000 + 0x8 * GTI_CWD_GLOBAL_WDOG_IDX) > +#define GTI_CWD_POKE_VAL (1ULL << GTI_CWD_GLOBAL_WDOG_IDX) > + > +struct octeontx2_wdt_priv { > + struct watchdog_device wdev; > + void __iomem *base; > + u64 clock_freq; > + int irq; > +}; > + > +static irqreturn_t octeontx2_wdt_interrupt(int irq, void *data) > +{ > + panic("Kernel Watchdog"); > + return IRQ_HANDLED; > +} > + > +static int octeontx2_wdt_ping(struct watchdog_device *wdev) > +{ > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > + > + writeq(GTI_CWD_POKE_VAL, priv->base + GTI_CWD_POKE); > + return 0; > +} > + > +static int octeontx2_wdt_start(struct watchdog_device *wdev) > +{ > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > + u64 regval; > + > + set_bit(WDOG_HW_RUNNING, &wdev->status); > + > + /* Clear any pending interrupt */ > + writeq(GTI_CWD_INT_PENDING_STATUS, priv->base + GTI_CWD_INT); > + > + /* Enable Interrupt */ > + writeq(GTI_CWD_INT_ENA_SET_VAL, priv->base + GTI_CWD_INT_ENA_SET); > + > + /* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */ > + regval = readq(priv->base + GTI_CWD_WDOG); > + regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST; > + writeq(regval, priv->base + GTI_CWD_WDOG); > + > + return 0; > +} > + > +static int octeontx2_wdt_stop(struct watchdog_device *wdev) > +{ > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > + u64 regval; > + > + /* Disable Interrupt */ > + writeq(GTI_CWD_INT_ENA_CLR_VAL, priv->base + GTI_CWD_INT_ENA_CLR); > + > + /* Set GTI_CWD_WDOG.Mode = 0 to stop the timer */ > + regval = readq(priv->base + GTI_CWD_WDOG); > + regval &= ~GTI_CWD_WDOG_MODE_MASK; > + writeq(regval, priv->base + GTI_CWD_WDOG); > + > + return 0; > +} > + > +static int octeontx2_wdt_settimeout(struct watchdog_device *wdev, > + unsigned int timeout) > +{ > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > + u64 timeout_wdog, regval; > + > + /* Update new timeout */ > + wdev->timeout = timeout; > + > + /* Get clock cycles from timeout in second */ > + timeout_wdog = (u64)timeout * priv->clock_freq; > + > + /* Watchdog counts in 1024 cycle steps */ > + timeout_wdog = timeout_wdog >> 10; > + > + /* > + * Hardware allows programming of upper 16-bits of 24-bits cycles > + * Round up and use upper 16-bits only. > + * Set max if timeout more than h/w supported This should be covered when setting max_timeout or max_hw_heartbeat_ms. Setting the actual timeout to a value smaller than configured may result in the watchdog firing before its configured timeout expires. > + */ > + timeout_wdog = (timeout_wdog + 0xff) >> 8; > + if (timeout_wdog >= 0x10000) > + timeout_wdog = 0xffff; > + > + /* > + * GTI_CWD_WDOG.LEN have only upper 16-bits of 24-bits > + * GTI_CWD_WDOG.CNT, need addition shift of 8. > + */ > + regval = readq(priv->base + GTI_CWD_WDOG); > + regval &= GTI_CWD_WDOG_MODE_MASK; > + regval |= ((timeout_wdog) << (GTI_CWD_WDOG_CNT_SHIFT + 8)) | > + (timeout_wdog << GTI_CWD_WDOG_LEN_SHIFT); () around timeout is unnecessary. Why does the timeout need to be programmed twice into the register ? The shift values are odd. Are you sure this does what you expect it to do ? > + writeq(regval, priv->base + GTI_CWD_WDOG); > + return 0; > +} > + > +static const struct watchdog_info octeontx2_wdt_ident = { > + .identity = "OcteonTX2 watchdog", > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | > + WDIOF_CARDRESET, > +}; > + > +static const struct watchdog_ops octeontx2_wdt_ops = { > + .owner = THIS_MODULE, > + .start = octeontx2_wdt_start, > + .stop = octeontx2_wdt_stop, > + .ping = octeontx2_wdt_ping, > + .set_timeout = octeontx2_wdt_settimeout, > +}; > + > +static int octeontx2_wdt_probe(struct platform_device *pdev) > +{ > + struct octeontx2_wdt_priv *priv; > + struct device *dev = &pdev->dev; > + struct watchdog_device *wdog_dev; > + int irq; > + int err; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(priv->base)) > + return dev_err_probe(&pdev->dev, PTR_ERR(priv->base), > + "reg property not valid/found\n"); > + > + priv->clock_freq = arch_timer_get_cntfrq(); > + > + wdog_dev = &priv->wdev; > + wdog_dev->info = &octeontx2_wdt_ident, > + wdog_dev->ops = &octeontx2_wdt_ops, > + wdog_dev->parent = dev; > + wdog_dev->min_timeout = 1; > + wdog_dev->max_timeout = 16; Setting max_timeout makes max_hw_heartbeat_ms useless. Use only max_hw_heartbeat_ms to enable larger soft timeouts, or use only max_timeout to set the hard limit, but not both. > + wdog_dev->max_hw_heartbeat_ms = 16000; > + wdog_dev->timeout = 8; > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(&pdev->dev, "IRQ resource not found\n"); > + return -ENODEV; > + } > + > + err = request_irq(irq, octeontx2_wdt_interrupt, 0, pdev->name, priv); > + if (err) { > + dev_err(dev, "cannot register interrupt handler %d\n", err); > + return err; > + } Use devm_request_irq() and request the interrupt after registering the watchdog. > + > + priv->irq = irq; > + watchdog_set_drvdata(wdog_dev, priv); > + platform_set_drvdata(pdev, priv); > + watchdog_init_timeout(wdog_dev, wdog_dev->timeout, dev); watchdog_init_timeout sets wdog_dev->timeout, so this is pointless. Calling watchdog_init_timeout() only makes sense if the parameter is either a timeout module parameter or 0 and the idea is to use a value from devicetree if configured. > + octeontx2_wdt_settimeout(wdog_dev, wdog_dev->timeout); > + watchdog_stop_on_reboot(wdog_dev); > + watchdog_stop_on_unregister(wdog_dev); > + > + err = devm_watchdog_register_device(dev, wdog_dev); > + if (err) { > + free_irq(irq, priv); > + return err; > + } > + > + dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev->timeout); > + return 0; > +} > + > +static int octeontx2_wdt_remove(struct platform_device *pdev) > +{ > + struct octeontx2_wdt_priv *priv = platform_get_drvdata(pdev); > + > + if (priv->irq) > + free_irq(priv->irq, priv); > + > + return 0; > +} > + > +static const struct of_device_id octeontx2_wdt_of_match[] = { > + { .compatible = "marvell-octeontx2-wdt", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, octeontx2_wdt_of_match); > + > +static struct platform_driver octeontx2_wdt_driver = { > + .driver = { > + .name = "octeontx2-wdt", > + .of_match_table = octeontx2_wdt_of_match, > + }, > + .probe = octeontx2_wdt_probe, > + .remove = octeontx2_wdt_remove, > +}; > +module_platform_driver(octeontx2_wdt_driver); > + > +MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>"); > +MODULE_DESCRIPTION("OcteonTX2 watchdog driver"); > -- > 2.17.1 >
Hi Bharat,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on groeck-staging/hwmon-next linus/master v6.3-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Bharat-Bhushan/Watchdog-Add-octeontx2-GTI-watchdog-driver/20230414-182508
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230414102342.23696-2-bbhushan2%40marvell.com
patch subject: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver
config: riscv-allmodconfig (https://download.01.org/0day-ci/archive/20230415/202304151147.xkLFltfQ-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/62fe2733152f0c68369948612f66ca68d43cb6f3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Bharat-Bhushan/Watchdog-Add-octeontx2-GTI-watchdog-driver/20230414-182508
git checkout 62fe2733152f0c68369948612f66ca68d43cb6f3
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/watchdog/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304151147.xkLFltfQ-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/watchdog/octeontx2_wdt.c:19:10: fatal error: asm/arch_timer.h: No such file or directory
19 | #include <asm/arch_timer.h>
| ^~~~~~~~~~~~~~~~~~
compilation terminated.
vim +19 drivers/watchdog/octeontx2_wdt.c
18
> 19 #include <asm/arch_timer.h>
20
Please see inline > -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Friday, April 14, 2023 8:05 PM > To: Bharat Bhushan <bbhushan2@marvell.com> > Cc: wim@linux-watchdog.org; robh+dt@kernel.org; > krzysztof.kozlowski+dt@linaro.org; linux-watchdog@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver > > External Email > > ---------------------------------------------------------------------- > On Fri, Apr 14, 2023 at 03:53:42PM +0530, Bharat Bhushan wrote: > > GTI watchdog timer are programmed in "interrupt + del3t + reset mode" > > and del3t traps are not enabled. > > GTI watchdog exception flow is: > > - 1st timer expiration generates watchdog interrupt. > > - 2nd timer expiration is ignored > > - On 3rd timer expiration will trigger a system-wide core reset. > > > > This means the interrupt should not result in a panic, the programmed timeout > value should be considered a pretimeout which is set to (timeout / 3), and the > interrupt handler should call watchdog_notify_pretimeout(). > > Either case, the above should be documented in the driver but does not make > much if any sense as patch description. If not, what are the other modes, and > why is this mode used instead of any of those modes ? Hardware supports following mode of operation: 1) Interrupt Only: This will generate the interrupt to arm core whenever timeout happens. 2) Interrupt + del3t (Interrupt to firmware (SCP processor)). This will generate interrupt to arm core whenever 1st timeout happens This will generate interrupt to SCP processor whenever 2nd timeout happens 3) Interrupt + Interrupt to SCP processor (called delt3t) + reboot. This will generate interrupt to arm core whenever 1st timeout happens This will generate interrupt to SCP processor whenever 2nd timeout happens, if interrupt is configured. This will reboot on 3rd timeout. We are going with mode-3 above so that system can reboot in case a hardware hang. Also h/w is configured not to generate SCP interrupt, so effectively 2nd timeout is ignored within hardware. Software is supposed to poke within 1st timeout. If poke does not happen then it will receive interrupt, interrupt handler will do panic. But for some reason if processor can not take interrupt then system will reboot on 3rd timeout. > > > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> > > --- > > drivers/watchdog/Kconfig | 9 ++ > > drivers/watchdog/Makefile | 1 + > > drivers/watchdog/octeontx2_wdt.c | 238 > > +++++++++++++++++++++++++++++++ > > 3 files changed, 248 insertions(+) > > create mode 100644 drivers/watchdog/octeontx2_wdt.c > > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index > > f0872970daf9..31ff282c62ad 100644 > > --- a/drivers/watchdog/Kconfig > > +++ b/drivers/watchdog/Kconfig > > @@ -2212,4 +2212,13 @@ config KEEMBAY_WATCHDOG > > To compile this driver as a module, choose M here: the > > module will be called keembay_wdt. > > > > +config OCTEONTX2_WATCHDOG > > + tristate "OCTEONTX2 Watchdog driver" > > + depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT) > > + help > > + OCTEONTX2 GTI hardware supports watchdog timer. This watchdog > timer are > > + programmed in "interrupt + del3t + reset" mode. On first expiry it will > > + generate interrupt. Second expiry (del3t) is ignored and system will reset > > + on final timer expiry. > > + > > The above should be part of the in-driver documentation but those details > should not be in Kconfig. Ok, > > Is the documentation available in public ? I'd like to check if this mode, especially > the ignored del3t mode, really makes sense. Documentation is not public. Provided some description above, let me know if I need to provide some more details. > > > endif # WATCHDOG > > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > > index 9cbf6580f16c..aa1b813ad1f9 100644 > > --- a/drivers/watchdog/Makefile > > +++ b/drivers/watchdog/Makefile > > @@ -230,3 +230,4 @@ obj-$(CONFIG_MENZ069_WATCHDOG) += > menz69_wdt.o > > obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o > > obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o > > obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o > > +obj-$(CONFIG_OCTEONTX2_WATCHDOG) += octeontx2_wdt.o > > diff --git a/drivers/watchdog/octeontx2_wdt.c > > b/drivers/watchdog/octeontx2_wdt.c > > new file mode 100644 > > index 000000000000..7b78a092e83f > > --- /dev/null > > +++ b/drivers/watchdog/octeontx2_wdt.c > > @@ -0,0 +1,238 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* Marvell Octeontx2 Watchdog driver > > + * > > + * Copyright (C) 2023 Marvell International 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. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/cpu.h> > > +#include <linux/interrupt.h> > > +#include <linux/of_platform.h> > > +#include <linux/platform_device.h> > > +#include <linux/watchdog.h> > > +#include <linux/sched/debug.h> > > What is this include for ? Taken from other driver when started coding this one. Will remove > > > + > > +#include <asm/arch_timer.h> > > + > > +/* GTI CWD Watchdog Registers */ > > +#define GTI_CWD_GLOBAL_WDOG_IDX 63 > > +#define GTI_CWD_WDOG (0x8 * > GTI_CWD_GLOBAL_WDOG_IDX) > > +#define GTI_CWD_WDOG_MODE_INT_DEL3T_RST 0x3 > > +#define GTI_CWD_WDOG_MODE_MASK 0x3 > > +#define GTI_CWD_WDOG_LEN_SHIFT 4 > > +#define GTI_CWD_WDOG_CNT_SHIFT 20 > > + > > +/* GTI Per-core Watchdog Interrupt Register */ > > +#define GTI_CWD_INT 0x200 > > +#define GTI_CWD_INT_PENDING_STATUS (1ULL << > GTI_CWD_GLOBAL_WDOG_IDX) > > + > > +/* GTI Per-core Watchdog Interrupt Enable Clear Register */ > > +#define GTI_CWD_INT_ENA_CLR 0x210 > > +#define GTI_CWD_INT_ENA_CLR_VAL (1ULL << > GTI_CWD_GLOBAL_WDOG_IDX) > > + > > +/* GTI Per-core Watchdog Interrupt Enable Set Register */ > > +#define GTI_CWD_INT_ENA_SET 0x218 > > +#define GTI_CWD_INT_ENA_SET_VAL (1ULL << > GTI_CWD_GLOBAL_WDOG_IDX) > > + > > +/* GTI Per-core Watchdog Poke Registers */ > > +#define GTI_CWD_POKE (0x10000 + 0x8 * > GTI_CWD_GLOBAL_WDOG_IDX) > > +#define GTI_CWD_POKE_VAL (1ULL << GTI_CWD_GLOBAL_WDOG_IDX) > > + > > +struct octeontx2_wdt_priv { > > + struct watchdog_device wdev; > > + void __iomem *base; > > + u64 clock_freq; > > + int irq; > > +}; > > + > > +static irqreturn_t octeontx2_wdt_interrupt(int irq, void *data) { > > + panic("Kernel Watchdog"); > > + return IRQ_HANDLED; > > +} > > + > > +static int octeontx2_wdt_ping(struct watchdog_device *wdev) { > > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > > + > > + writeq(GTI_CWD_POKE_VAL, priv->base + GTI_CWD_POKE); > > + return 0; > > +} > > + > > +static int octeontx2_wdt_start(struct watchdog_device *wdev) { > > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > > + u64 regval; > > + > > + set_bit(WDOG_HW_RUNNING, &wdev->status); > > + > > + /* Clear any pending interrupt */ > > + writeq(GTI_CWD_INT_PENDING_STATUS, priv->base + GTI_CWD_INT); > > + > > + /* Enable Interrupt */ > > + writeq(GTI_CWD_INT_ENA_SET_VAL, priv->base + > GTI_CWD_INT_ENA_SET); > > + > > + /* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */ > > + regval = readq(priv->base + GTI_CWD_WDOG); > > + regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST; > > + writeq(regval, priv->base + GTI_CWD_WDOG); > > + > > + return 0; > > +} > > + > > +static int octeontx2_wdt_stop(struct watchdog_device *wdev) { > > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > > + u64 regval; > > + > > + /* Disable Interrupt */ > > + writeq(GTI_CWD_INT_ENA_CLR_VAL, priv->base + > GTI_CWD_INT_ENA_CLR); > > + > > + /* Set GTI_CWD_WDOG.Mode = 0 to stop the timer */ > > + regval = readq(priv->base + GTI_CWD_WDOG); > > + regval &= ~GTI_CWD_WDOG_MODE_MASK; > > + writeq(regval, priv->base + GTI_CWD_WDOG); > > + > > + return 0; > > +} > > + > > +static int octeontx2_wdt_settimeout(struct watchdog_device *wdev, > > + unsigned int timeout) > > +{ > > + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); > > + u64 timeout_wdog, regval; > > + > > + /* Update new timeout */ > > + wdev->timeout = timeout; > > + > > + /* Get clock cycles from timeout in second */ > > + timeout_wdog = (u64)timeout * priv->clock_freq; > > + > > + /* Watchdog counts in 1024 cycle steps */ > > + timeout_wdog = timeout_wdog >> 10; > > + > > + /* > > + * Hardware allows programming of upper 16-bits of 24-bits cycles > > + * Round up and use upper 16-bits only. > > + * Set max if timeout more than h/w supported > > This should be covered when setting max_timeout or max_hw_heartbeat_ms. > Setting the actual timeout to a value smaller than configured may result in the > watchdog firing before its configured timeout expires. wdog_dev->max_timeout = 16 is set based on above description, will move this comment there. Just to explain why I added above comment here and below check, that was to ensure that if user provides timeout more than " wdog_dev->max_timeout" then program wdog_dev->max_timeout only. But now looking at the code "set_timeout()" will be called only if timeout is valid ( timeout is <= wdog_dev->max_timeout) So above comment is not valid, while below check can also be removed. > > > + */ > > + timeout_wdog = (timeout_wdog + 0xff) >> 8; > > + if (timeout_wdog >= 0x10000) > > + timeout_wdog = 0xffff; > > + > > + /* > > + * GTI_CWD_WDOG.LEN have only upper 16-bits of 24-bits > > + * GTI_CWD_WDOG.CNT, need addition shift of 8. > > + */ > > + regval = readq(priv->base + GTI_CWD_WDOG); > > + regval &= GTI_CWD_WDOG_MODE_MASK; > > + regval |= ((timeout_wdog) << (GTI_CWD_WDOG_CNT_SHIFT + 8)) | > > + (timeout_wdog << GTI_CWD_WDOG_LEN_SHIFT); > > () around timeout is unnecessary. Why does the timeout need to be programmed > twice into the register ? The shift values are odd. > Are you sure this does what you expect it to do ? This register have two timeouts: GTI_CWD_WDOG.CNT[43:20] (24bit) = this is something decrementing with a frequency. GTI_CWD_WDOG.LEN[19:4] (16bit) = this is something loaded in upper 16 bits of GTI_CWD_WDOG.CNT when poke happens. Shift looks odd but it works as expected. > > > + writeq(regval, priv->base + GTI_CWD_WDOG); > > + return 0; > > +} > > + > > +static const struct watchdog_info octeontx2_wdt_ident = { > > + .identity = "OcteonTX2 watchdog", > > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | > WDIOF_MAGICCLOSE | > > + WDIOF_CARDRESET, > > +}; > > + > > +static const struct watchdog_ops octeontx2_wdt_ops = { > > + .owner = THIS_MODULE, > > + .start = octeontx2_wdt_start, > > + .stop = octeontx2_wdt_stop, > > + .ping = octeontx2_wdt_ping, > > + .set_timeout = octeontx2_wdt_settimeout, }; > > + > > +static int octeontx2_wdt_probe(struct platform_device *pdev) { > > + struct octeontx2_wdt_priv *priv; > > + struct device *dev = &pdev->dev; > > + struct watchdog_device *wdog_dev; > > + int irq; > > + int err; > > + > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(priv->base)) > > + return dev_err_probe(&pdev->dev, PTR_ERR(priv->base), > > + "reg property not valid/found\n"); > > + > > + priv->clock_freq = arch_timer_get_cntfrq(); > > + > > + wdog_dev = &priv->wdev; > > + wdog_dev->info = &octeontx2_wdt_ident, > > + wdog_dev->ops = &octeontx2_wdt_ops, > > + wdog_dev->parent = dev; > > + wdog_dev->min_timeout = 1; > > + wdog_dev->max_timeout = 16; > > Setting max_timeout makes max_hw_heartbeat_ms useless. Use only > max_hw_heartbeat_ms to enable larger soft timeouts, or use only max_timeout > to set the hard limit, but not both. Okay, thanks for details > > > + wdog_dev->max_hw_heartbeat_ms = 16000; > > + wdog_dev->timeout = 8; > > + > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) { > > + dev_err(&pdev->dev, "IRQ resource not found\n"); > > + return -ENODEV; > > + } > > + > > + err = request_irq(irq, octeontx2_wdt_interrupt, 0, pdev->name, priv); > > + if (err) { > > + dev_err(dev, "cannot register interrupt handler %d\n", err); > > + return err; > > + } > > Use devm_request_irq() and request the interrupt after registering the watchdog. Okay, > > > + > > + priv->irq = irq; > > + watchdog_set_drvdata(wdog_dev, priv); > > + platform_set_drvdata(pdev, priv); > > + watchdog_init_timeout(wdog_dev, wdog_dev->timeout, dev); > > watchdog_init_timeout sets wdog_dev->timeout, so this is pointless. > Calling watchdog_init_timeout() only makes sense if the parameter is either a > timeout module parameter or 0 and the idea is to use a value from devicetree if > configured. Okay, thanks again for detail Regards -Bharat > > > + octeontx2_wdt_settimeout(wdog_dev, wdog_dev->timeout); > > + watchdog_stop_on_reboot(wdog_dev); > > + watchdog_stop_on_unregister(wdog_dev); > > + > > + err = devm_watchdog_register_device(dev, wdog_dev); > > + if (err) { > > + free_irq(irq, priv); > > + return err; > > + } > > + > > + dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev- > >timeout); > > + return 0; > > +} > > + > > +static int octeontx2_wdt_remove(struct platform_device *pdev) { > > + struct octeontx2_wdt_priv *priv = platform_get_drvdata(pdev); > > + > > + if (priv->irq) > > + free_irq(priv->irq, priv); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id octeontx2_wdt_of_match[] = { > > + { .compatible = "marvell-octeontx2-wdt", }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(of, octeontx2_wdt_of_match); > > + > > +static struct platform_driver octeontx2_wdt_driver = { > > + .driver = { > > + .name = "octeontx2-wdt", > > + .of_match_table = octeontx2_wdt_of_match, > > + }, > > + .probe = octeontx2_wdt_probe, > > + .remove = octeontx2_wdt_remove, > > +}; > > +module_platform_driver(octeontx2_wdt_driver); > > + > > +MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>"); > > +MODULE_DESCRIPTION("OcteonTX2 watchdog driver"); > > -- > > 2.17.1 > >
On 4/17/23 04:06, Bharat Bhushan wrote: > Please see inline > >> -----Original Message----- >> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck >> Sent: Friday, April 14, 2023 8:05 PM >> To: Bharat Bhushan <bbhushan2@marvell.com> >> Cc: wim@linux-watchdog.org; robh+dt@kernel.org; >> krzysztof.kozlowski+dt@linaro.org; linux-watchdog@vger.kernel.org; >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org >> Subject: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver >> >> External Email >> >> ---------------------------------------------------------------------- >> On Fri, Apr 14, 2023 at 03:53:42PM +0530, Bharat Bhushan wrote: >>> GTI watchdog timer are programmed in "interrupt + del3t + reset mode" >>> and del3t traps are not enabled. >>> GTI watchdog exception flow is: >>> - 1st timer expiration generates watchdog interrupt. >>> - 2nd timer expiration is ignored >>> - On 3rd timer expiration will trigger a system-wide core reset. >>> >> >> This means the interrupt should not result in a panic, the programmed timeout >> value should be considered a pretimeout which is set to (timeout / 3), and the >> interrupt handler should call watchdog_notify_pretimeout(). >> >> Either case, the above should be documented in the driver but does not make >> much if any sense as patch description. If not, what are the other modes, and >> why is this mode used instead of any of those modes ? > > Hardware supports following mode of operation: > 1) Interrupt Only: > This will generate the interrupt to arm core whenever timeout happens. > > 2) Interrupt + del3t (Interrupt to firmware (SCP processor)). > This will generate interrupt to arm core whenever 1st timeout happens > This will generate interrupt to SCP processor whenever 2nd timeout happens > > 3) Interrupt + Interrupt to SCP processor (called delt3t) + reboot. > This will generate interrupt to arm core whenever 1st timeout happens > This will generate interrupt to SCP processor whenever 2nd timeout happens, if interrupt is configured. > This will reboot on 3rd timeout. > > We are going with mode-3 above so that system can reboot in case a hardware hang. Also h/w is configured not to generate SCP interrupt, so effectively 2nd timeout is ignored within hardware. > > Software is supposed to poke within 1st timeout. If poke does not happen then it will receive interrupt, interrupt handler will do panic. > But for some reason if processor can not take interrupt then system will reboot on 3rd timeout. > Then, as I said, the first timeout shopuld be modeled as pretimeout. Thanks, Guenter >> >>> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> >>> --- >>> drivers/watchdog/Kconfig | 9 ++ >>> drivers/watchdog/Makefile | 1 + >>> drivers/watchdog/octeontx2_wdt.c | 238 >>> +++++++++++++++++++++++++++++++ >>> 3 files changed, 248 insertions(+) >>> create mode 100644 drivers/watchdog/octeontx2_wdt.c >>> >>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index >>> f0872970daf9..31ff282c62ad 100644 >>> --- a/drivers/watchdog/Kconfig >>> +++ b/drivers/watchdog/Kconfig >>> @@ -2212,4 +2212,13 @@ config KEEMBAY_WATCHDOG >>> To compile this driver as a module, choose M here: the >>> module will be called keembay_wdt. >>> >>> +config OCTEONTX2_WATCHDOG >>> + tristate "OCTEONTX2 Watchdog driver" >>> + depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT) >>> + help >>> + OCTEONTX2 GTI hardware supports watchdog timer. This watchdog >> timer are >>> + programmed in "interrupt + del3t + reset" mode. On first expiry it will >>> + generate interrupt. Second expiry (del3t) is ignored and system will reset >>> + on final timer expiry. >>> + >> >> The above should be part of the in-driver documentation but those details >> should not be in Kconfig. > > Ok, > >> >> Is the documentation available in public ? I'd like to check if this mode, especially >> the ignored del3t mode, really makes sense. > > Documentation is not public. Provided some description above, let me know if I need to provide some more details. > >> >>> endif # WATCHDOG >>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile >>> index 9cbf6580f16c..aa1b813ad1f9 100644 >>> --- a/drivers/watchdog/Makefile >>> +++ b/drivers/watchdog/Makefile >>> @@ -230,3 +230,4 @@ obj-$(CONFIG_MENZ069_WATCHDOG) += >> menz69_wdt.o >>> obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o >>> obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o >>> obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o >>> +obj-$(CONFIG_OCTEONTX2_WATCHDOG) += octeontx2_wdt.o >>> diff --git a/drivers/watchdog/octeontx2_wdt.c >>> b/drivers/watchdog/octeontx2_wdt.c >>> new file mode 100644 >>> index 000000000000..7b78a092e83f >>> --- /dev/null >>> +++ b/drivers/watchdog/octeontx2_wdt.c >>> @@ -0,0 +1,238 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* Marvell Octeontx2 Watchdog driver >>> + * >>> + * Copyright (C) 2023 Marvell International 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. >>> + */ >>> + >>> +#include <linux/module.h> >>> +#include <linux/cpu.h> >>> +#include <linux/interrupt.h> >>> +#include <linux/of_platform.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/watchdog.h> >>> +#include <linux/sched/debug.h> >> >> What is this include for ? > > Taken from other driver when started coding this one. Will remove > >> >>> + >>> +#include <asm/arch_timer.h> >>> + >>> +/* GTI CWD Watchdog Registers */ >>> +#define GTI_CWD_GLOBAL_WDOG_IDX 63 >>> +#define GTI_CWD_WDOG (0x8 * >> GTI_CWD_GLOBAL_WDOG_IDX) >>> +#define GTI_CWD_WDOG_MODE_INT_DEL3T_RST 0x3 >>> +#define GTI_CWD_WDOG_MODE_MASK 0x3 >>> +#define GTI_CWD_WDOG_LEN_SHIFT 4 >>> +#define GTI_CWD_WDOG_CNT_SHIFT 20 >>> + >>> +/* GTI Per-core Watchdog Interrupt Register */ >>> +#define GTI_CWD_INT 0x200 >>> +#define GTI_CWD_INT_PENDING_STATUS (1ULL << >> GTI_CWD_GLOBAL_WDOG_IDX) >>> + >>> +/* GTI Per-core Watchdog Interrupt Enable Clear Register */ >>> +#define GTI_CWD_INT_ENA_CLR 0x210 >>> +#define GTI_CWD_INT_ENA_CLR_VAL (1ULL << >> GTI_CWD_GLOBAL_WDOG_IDX) >>> + >>> +/* GTI Per-core Watchdog Interrupt Enable Set Register */ >>> +#define GTI_CWD_INT_ENA_SET 0x218 >>> +#define GTI_CWD_INT_ENA_SET_VAL (1ULL << >> GTI_CWD_GLOBAL_WDOG_IDX) >>> + >>> +/* GTI Per-core Watchdog Poke Registers */ >>> +#define GTI_CWD_POKE (0x10000 + 0x8 * >> GTI_CWD_GLOBAL_WDOG_IDX) >>> +#define GTI_CWD_POKE_VAL (1ULL << GTI_CWD_GLOBAL_WDOG_IDX) >>> + >>> +struct octeontx2_wdt_priv { >>> + struct watchdog_device wdev; >>> + void __iomem *base; >>> + u64 clock_freq; >>> + int irq; >>> +}; >>> + >>> +static irqreturn_t octeontx2_wdt_interrupt(int irq, void *data) { >>> + panic("Kernel Watchdog"); >>> + return IRQ_HANDLED; >>> +} >>> + >>> +static int octeontx2_wdt_ping(struct watchdog_device *wdev) { >>> + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); >>> + >>> + writeq(GTI_CWD_POKE_VAL, priv->base + GTI_CWD_POKE); >>> + return 0; >>> +} >>> + >>> +static int octeontx2_wdt_start(struct watchdog_device *wdev) { >>> + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); >>> + u64 regval; >>> + >>> + set_bit(WDOG_HW_RUNNING, &wdev->status); >>> + >>> + /* Clear any pending interrupt */ >>> + writeq(GTI_CWD_INT_PENDING_STATUS, priv->base + GTI_CWD_INT); >>> + >>> + /* Enable Interrupt */ >>> + writeq(GTI_CWD_INT_ENA_SET_VAL, priv->base + >> GTI_CWD_INT_ENA_SET); >>> + >>> + /* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */ >>> + regval = readq(priv->base + GTI_CWD_WDOG); >>> + regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST; >>> + writeq(regval, priv->base + GTI_CWD_WDOG); >>> + >>> + return 0; >>> +} >>> + >>> +static int octeontx2_wdt_stop(struct watchdog_device *wdev) { >>> + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); >>> + u64 regval; >>> + >>> + /* Disable Interrupt */ >>> + writeq(GTI_CWD_INT_ENA_CLR_VAL, priv->base + >> GTI_CWD_INT_ENA_CLR); >>> + >>> + /* Set GTI_CWD_WDOG.Mode = 0 to stop the timer */ >>> + regval = readq(priv->base + GTI_CWD_WDOG); >>> + regval &= ~GTI_CWD_WDOG_MODE_MASK; >>> + writeq(regval, priv->base + GTI_CWD_WDOG); >>> + >>> + return 0; >>> +} >>> + >>> +static int octeontx2_wdt_settimeout(struct watchdog_device *wdev, >>> + unsigned int timeout) >>> +{ >>> + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); >>> + u64 timeout_wdog, regval; >>> + >>> + /* Update new timeout */ >>> + wdev->timeout = timeout; >>> + >>> + /* Get clock cycles from timeout in second */ >>> + timeout_wdog = (u64)timeout * priv->clock_freq; >>> + >>> + /* Watchdog counts in 1024 cycle steps */ >>> + timeout_wdog = timeout_wdog >> 10; >>> + >>> + /* >>> + * Hardware allows programming of upper 16-bits of 24-bits cycles >>> + * Round up and use upper 16-bits only. >>> + * Set max if timeout more than h/w supported >> >> This should be covered when setting max_timeout or max_hw_heartbeat_ms. >> Setting the actual timeout to a value smaller than configured may result in the >> watchdog firing before its configured timeout expires. > > wdog_dev->max_timeout = 16 is set based on above description, will move this comment there. > > Just to explain why I added above comment here and below check, that was to ensure that if user provides timeout more than " wdog_dev->max_timeout" then program wdog_dev->max_timeout only. > But now looking at the code "set_timeout()" will be called only if timeout is valid ( timeout is <= wdog_dev->max_timeout) > > So above comment is not valid, while below check can also be removed. > >> >>> + */ >>> + timeout_wdog = (timeout_wdog + 0xff) >> 8; >>> + if (timeout_wdog >= 0x10000) >>> + timeout_wdog = 0xffff; >>> + >>> + /* >>> + * GTI_CWD_WDOG.LEN have only upper 16-bits of 24-bits >>> + * GTI_CWD_WDOG.CNT, need addition shift of 8. >>> + */ >>> + regval = readq(priv->base + GTI_CWD_WDOG); >>> + regval &= GTI_CWD_WDOG_MODE_MASK; >>> + regval |= ((timeout_wdog) << (GTI_CWD_WDOG_CNT_SHIFT + 8)) | >>> + (timeout_wdog << GTI_CWD_WDOG_LEN_SHIFT); >> >> () around timeout is unnecessary. Why does the timeout need to be programmed >> twice into the register ? The shift values are odd. >> Are you sure this does what you expect it to do ? > > This register have two timeouts: > GTI_CWD_WDOG.CNT[43:20] (24bit) = this is something decrementing with a frequency. > GTI_CWD_WDOG.LEN[19:4] (16bit) = this is something loaded in upper 16 bits of GTI_CWD_WDOG.CNT when poke happens. > > Shift looks odd but it works as expected. > >> >>> + writeq(regval, priv->base + GTI_CWD_WDOG); >>> + return 0; >>> +} >>> + >>> +static const struct watchdog_info octeontx2_wdt_ident = { >>> + .identity = "OcteonTX2 watchdog", >>> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | >> WDIOF_MAGICCLOSE | >>> + WDIOF_CARDRESET, >>> +}; >>> + >>> +static const struct watchdog_ops octeontx2_wdt_ops = { >>> + .owner = THIS_MODULE, >>> + .start = octeontx2_wdt_start, >>> + .stop = octeontx2_wdt_stop, >>> + .ping = octeontx2_wdt_ping, >>> + .set_timeout = octeontx2_wdt_settimeout, }; >>> + >>> +static int octeontx2_wdt_probe(struct platform_device *pdev) { >>> + struct octeontx2_wdt_priv *priv; >>> + struct device *dev = &pdev->dev; >>> + struct watchdog_device *wdog_dev; >>> + int irq; >>> + int err; >>> + >>> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); >>> + if (!priv) >>> + return -ENOMEM; >>> + >>> + priv->base = devm_platform_ioremap_resource(pdev, 0); >>> + if (IS_ERR(priv->base)) >>> + return dev_err_probe(&pdev->dev, PTR_ERR(priv->base), >>> + "reg property not valid/found\n"); >>> + >>> + priv->clock_freq = arch_timer_get_cntfrq(); >>> + >>> + wdog_dev = &priv->wdev; >>> + wdog_dev->info = &octeontx2_wdt_ident, >>> + wdog_dev->ops = &octeontx2_wdt_ops, >>> + wdog_dev->parent = dev; >>> + wdog_dev->min_timeout = 1; >>> + wdog_dev->max_timeout = 16; >> >> Setting max_timeout makes max_hw_heartbeat_ms useless. Use only >> max_hw_heartbeat_ms to enable larger soft timeouts, or use only max_timeout >> to set the hard limit, but not both. > > Okay, thanks for details > >> >>> + wdog_dev->max_hw_heartbeat_ms = 16000; >>> + wdog_dev->timeout = 8; >>> + >>> + irq = platform_get_irq(pdev, 0); >>> + if (irq < 0) { >>> + dev_err(&pdev->dev, "IRQ resource not found\n"); >>> + return -ENODEV; >>> + } >>> + >>> + err = request_irq(irq, octeontx2_wdt_interrupt, 0, pdev->name, priv); >>> + if (err) { >>> + dev_err(dev, "cannot register interrupt handler %d\n", err); >>> + return err; >>> + } >> >> Use devm_request_irq() and request the interrupt after registering the watchdog. > > Okay, > >> >>> + >>> + priv->irq = irq; >>> + watchdog_set_drvdata(wdog_dev, priv); >>> + platform_set_drvdata(pdev, priv); >>> + watchdog_init_timeout(wdog_dev, wdog_dev->timeout, dev); >> >> watchdog_init_timeout sets wdog_dev->timeout, so this is pointless. >> Calling watchdog_init_timeout() only makes sense if the parameter is either a >> timeout module parameter or 0 and the idea is to use a value from devicetree if >> configured. > > Okay, thanks again for detail > > Regards > -Bharat > >> >>> + octeontx2_wdt_settimeout(wdog_dev, wdog_dev->timeout); >>> + watchdog_stop_on_reboot(wdog_dev); >>> + watchdog_stop_on_unregister(wdog_dev); >>> + >>> + err = devm_watchdog_register_device(dev, wdog_dev); >>> + if (err) { >>> + free_irq(irq, priv); >>> + return err; >>> + } >>> + >>> + dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev- >>> timeout); >>> + return 0; >>> +} >>> + >>> +static int octeontx2_wdt_remove(struct platform_device *pdev) { >>> + struct octeontx2_wdt_priv *priv = platform_get_drvdata(pdev); >>> + >>> + if (priv->irq) >>> + free_irq(priv->irq, priv); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct of_device_id octeontx2_wdt_of_match[] = { >>> + { .compatible = "marvell-octeontx2-wdt", }, >>> + { }, >>> +}; >>> +MODULE_DEVICE_TABLE(of, octeontx2_wdt_of_match); >>> + >>> +static struct platform_driver octeontx2_wdt_driver = { >>> + .driver = { >>> + .name = "octeontx2-wdt", >>> + .of_match_table = octeontx2_wdt_of_match, >>> + }, >>> + .probe = octeontx2_wdt_probe, >>> + .remove = octeontx2_wdt_remove, >>> +}; >>> +module_platform_driver(octeontx2_wdt_driver); >>> + >>> +MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>"); >>> +MODULE_DESCRIPTION("OcteonTX2 watchdog driver"); >>> -- >>> 2.17.1 >>>
On Fri, Apr 14, 2023 at 03:53:42PM +0530, Bharat Bhushan wrote: > GTI watchdog timer are programmed in "interrupt + del3t + reset mode" > and del3t traps are not enabled. > GTI watchdog exception flow is: > - 1st timer expiration generates watchdog interrupt. > - 2nd timer expiration is ignored > - On 3rd timer expiration will trigger a system-wide core reset. > > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> > --- > drivers/watchdog/Kconfig | 9 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/octeontx2_wdt.c | 238 +++++++++++++++++++++++++++++++ > 3 files changed, 248 insertions(+) > create mode 100644 drivers/watchdog/octeontx2_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index f0872970daf9..31ff282c62ad 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -2212,4 +2212,13 @@ config KEEMBAY_WATCHDOG > To compile this driver as a module, choose M here: the > module will be called keembay_wdt. > > +config OCTEONTX2_WATCHDOG > + tristate "OCTEONTX2 Watchdog driver" > + depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT) > + help > + OCTEONTX2 GTI hardware supports watchdog timer. This watchdog timer are > + programmed in "interrupt + del3t + reset" mode. On first expiry it will > + generate interrupt. Second expiry (del3t) is ignored and system will reset > + on final timer expiry. > + > endif # WATCHDOG > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 9cbf6580f16c..aa1b813ad1f9 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -230,3 +230,4 @@ obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o > obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o > obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o > obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o > +obj-$(CONFIG_OCTEONTX2_WATCHDOG) += octeontx2_wdt.o > diff --git a/drivers/watchdog/octeontx2_wdt.c b/drivers/watchdog/octeontx2_wdt.c > new file mode 100644 > index 000000000000..7b78a092e83f > --- /dev/null > +++ b/drivers/watchdog/octeontx2_wdt.c > @@ -0,0 +1,238 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Marvell Octeontx2 Watchdog driver > + * > + * Copyright (C) 2023 Marvell International 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. > + */ > + > +#include <linux/module.h> > +#include <linux/cpu.h> > +#include <linux/interrupt.h> > +#include <linux/of_platform.h> It's doubtful you need anything from of_platform.h other than implicit includes. Use the header(s) you need directly. Rob
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index f0872970daf9..31ff282c62ad 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -2212,4 +2212,13 @@ config KEEMBAY_WATCHDOG To compile this driver as a module, choose M here: the module will be called keembay_wdt. +config OCTEONTX2_WATCHDOG + tristate "OCTEONTX2 Watchdog driver" + depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT) + help + OCTEONTX2 GTI hardware supports watchdog timer. This watchdog timer are + programmed in "interrupt + del3t + reset" mode. On first expiry it will + generate interrupt. Second expiry (del3t) is ignored and system will reset + on final timer expiry. + endif # WATCHDOG diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 9cbf6580f16c..aa1b813ad1f9 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -230,3 +230,4 @@ obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o +obj-$(CONFIG_OCTEONTX2_WATCHDOG) += octeontx2_wdt.o diff --git a/drivers/watchdog/octeontx2_wdt.c b/drivers/watchdog/octeontx2_wdt.c new file mode 100644 index 000000000000..7b78a092e83f --- /dev/null +++ b/drivers/watchdog/octeontx2_wdt.c @@ -0,0 +1,238 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Marvell Octeontx2 Watchdog driver + * + * Copyright (C) 2023 Marvell International 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. + */ + +#include <linux/module.h> +#include <linux/cpu.h> +#include <linux/interrupt.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/watchdog.h> +#include <linux/sched/debug.h> + +#include <asm/arch_timer.h> + +/* GTI CWD Watchdog Registers */ +#define GTI_CWD_GLOBAL_WDOG_IDX 63 +#define GTI_CWD_WDOG (0x8 * GTI_CWD_GLOBAL_WDOG_IDX) +#define GTI_CWD_WDOG_MODE_INT_DEL3T_RST 0x3 +#define GTI_CWD_WDOG_MODE_MASK 0x3 +#define GTI_CWD_WDOG_LEN_SHIFT 4 +#define GTI_CWD_WDOG_CNT_SHIFT 20 + +/* GTI Per-core Watchdog Interrupt Register */ +#define GTI_CWD_INT 0x200 +#define GTI_CWD_INT_PENDING_STATUS (1ULL << GTI_CWD_GLOBAL_WDOG_IDX) + +/* GTI Per-core Watchdog Interrupt Enable Clear Register */ +#define GTI_CWD_INT_ENA_CLR 0x210 +#define GTI_CWD_INT_ENA_CLR_VAL (1ULL << GTI_CWD_GLOBAL_WDOG_IDX) + +/* GTI Per-core Watchdog Interrupt Enable Set Register */ +#define GTI_CWD_INT_ENA_SET 0x218 +#define GTI_CWD_INT_ENA_SET_VAL (1ULL << GTI_CWD_GLOBAL_WDOG_IDX) + +/* GTI Per-core Watchdog Poke Registers */ +#define GTI_CWD_POKE (0x10000 + 0x8 * GTI_CWD_GLOBAL_WDOG_IDX) +#define GTI_CWD_POKE_VAL (1ULL << GTI_CWD_GLOBAL_WDOG_IDX) + +struct octeontx2_wdt_priv { + struct watchdog_device wdev; + void __iomem *base; + u64 clock_freq; + int irq; +}; + +static irqreturn_t octeontx2_wdt_interrupt(int irq, void *data) +{ + panic("Kernel Watchdog"); + return IRQ_HANDLED; +} + +static int octeontx2_wdt_ping(struct watchdog_device *wdev) +{ + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); + + writeq(GTI_CWD_POKE_VAL, priv->base + GTI_CWD_POKE); + return 0; +} + +static int octeontx2_wdt_start(struct watchdog_device *wdev) +{ + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); + u64 regval; + + set_bit(WDOG_HW_RUNNING, &wdev->status); + + /* Clear any pending interrupt */ + writeq(GTI_CWD_INT_PENDING_STATUS, priv->base + GTI_CWD_INT); + + /* Enable Interrupt */ + writeq(GTI_CWD_INT_ENA_SET_VAL, priv->base + GTI_CWD_INT_ENA_SET); + + /* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */ + regval = readq(priv->base + GTI_CWD_WDOG); + regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST; + writeq(regval, priv->base + GTI_CWD_WDOG); + + return 0; +} + +static int octeontx2_wdt_stop(struct watchdog_device *wdev) +{ + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); + u64 regval; + + /* Disable Interrupt */ + writeq(GTI_CWD_INT_ENA_CLR_VAL, priv->base + GTI_CWD_INT_ENA_CLR); + + /* Set GTI_CWD_WDOG.Mode = 0 to stop the timer */ + regval = readq(priv->base + GTI_CWD_WDOG); + regval &= ~GTI_CWD_WDOG_MODE_MASK; + writeq(regval, priv->base + GTI_CWD_WDOG); + + return 0; +} + +static int octeontx2_wdt_settimeout(struct watchdog_device *wdev, + unsigned int timeout) +{ + struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev); + u64 timeout_wdog, regval; + + /* Update new timeout */ + wdev->timeout = timeout; + + /* Get clock cycles from timeout in second */ + timeout_wdog = (u64)timeout * priv->clock_freq; + + /* Watchdog counts in 1024 cycle steps */ + timeout_wdog = timeout_wdog >> 10; + + /* + * Hardware allows programming of upper 16-bits of 24-bits cycles + * Round up and use upper 16-bits only. + * Set max if timeout more than h/w supported + */ + timeout_wdog = (timeout_wdog + 0xff) >> 8; + if (timeout_wdog >= 0x10000) + timeout_wdog = 0xffff; + + /* + * GTI_CWD_WDOG.LEN have only upper 16-bits of 24-bits + * GTI_CWD_WDOG.CNT, need addition shift of 8. + */ + regval = readq(priv->base + GTI_CWD_WDOG); + regval &= GTI_CWD_WDOG_MODE_MASK; + regval |= ((timeout_wdog) << (GTI_CWD_WDOG_CNT_SHIFT + 8)) | + (timeout_wdog << GTI_CWD_WDOG_LEN_SHIFT); + writeq(regval, priv->base + GTI_CWD_WDOG); + return 0; +} + +static const struct watchdog_info octeontx2_wdt_ident = { + .identity = "OcteonTX2 watchdog", + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | + WDIOF_CARDRESET, +}; + +static const struct watchdog_ops octeontx2_wdt_ops = { + .owner = THIS_MODULE, + .start = octeontx2_wdt_start, + .stop = octeontx2_wdt_stop, + .ping = octeontx2_wdt_ping, + .set_timeout = octeontx2_wdt_settimeout, +}; + +static int octeontx2_wdt_probe(struct platform_device *pdev) +{ + struct octeontx2_wdt_priv *priv; + struct device *dev = &pdev->dev; + struct watchdog_device *wdog_dev; + int irq; + int err; + + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(priv->base)) + return dev_err_probe(&pdev->dev, PTR_ERR(priv->base), + "reg property not valid/found\n"); + + priv->clock_freq = arch_timer_get_cntfrq(); + + wdog_dev = &priv->wdev; + wdog_dev->info = &octeontx2_wdt_ident, + wdog_dev->ops = &octeontx2_wdt_ops, + wdog_dev->parent = dev; + wdog_dev->min_timeout = 1; + wdog_dev->max_timeout = 16; + wdog_dev->max_hw_heartbeat_ms = 16000; + wdog_dev->timeout = 8; + + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_err(&pdev->dev, "IRQ resource not found\n"); + return -ENODEV; + } + + err = request_irq(irq, octeontx2_wdt_interrupt, 0, pdev->name, priv); + if (err) { + dev_err(dev, "cannot register interrupt handler %d\n", err); + return err; + } + + priv->irq = irq; + watchdog_set_drvdata(wdog_dev, priv); + platform_set_drvdata(pdev, priv); + watchdog_init_timeout(wdog_dev, wdog_dev->timeout, dev); + octeontx2_wdt_settimeout(wdog_dev, wdog_dev->timeout); + watchdog_stop_on_reboot(wdog_dev); + watchdog_stop_on_unregister(wdog_dev); + + err = devm_watchdog_register_device(dev, wdog_dev); + if (err) { + free_irq(irq, priv); + return err; + } + + dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev->timeout); + return 0; +} + +static int octeontx2_wdt_remove(struct platform_device *pdev) +{ + struct octeontx2_wdt_priv *priv = platform_get_drvdata(pdev); + + if (priv->irq) + free_irq(priv->irq, priv); + + return 0; +} + +static const struct of_device_id octeontx2_wdt_of_match[] = { + { .compatible = "marvell-octeontx2-wdt", }, + { }, +}; +MODULE_DEVICE_TABLE(of, octeontx2_wdt_of_match); + +static struct platform_driver octeontx2_wdt_driver = { + .driver = { + .name = "octeontx2-wdt", + .of_match_table = octeontx2_wdt_of_match, + }, + .probe = octeontx2_wdt_probe, + .remove = octeontx2_wdt_remove, +}; +module_platform_driver(octeontx2_wdt_driver); + +MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>"); +MODULE_DESCRIPTION("OcteonTX2 watchdog driver");
GTI watchdog timer are programmed in "interrupt + del3t + reset mode" and del3t traps are not enabled. GTI watchdog exception flow is: - 1st timer expiration generates watchdog interrupt. - 2nd timer expiration is ignored - On 3rd timer expiration will trigger a system-wide core reset. Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> --- drivers/watchdog/Kconfig | 9 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/octeontx2_wdt.c | 238 +++++++++++++++++++++++++++++++ 3 files changed, 248 insertions(+) create mode 100644 drivers/watchdog/octeontx2_wdt.c