Messages in this thread | | | Date | Mon, 17 Apr 2023 07:10:29 -0700 | Subject | Re: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver | From | Guenter Roeck <> |
| |
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 >>>
| |