| Message ID | 20230414102342.23696-1-bbhushan2@marvell.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] dt-bindings: watchdog: marvell octeonTX2 GTI system atchdog driver | expand |
On 14/04/2023 12:23, Bharat Bhushan wrote: > Add binding documentation for the Marvell octeonTX2 > GTI system watchdog driver. Subject: typo: atchdog > > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> > --- > .../watchdog/marvell-octeontx2-wdt.yaml | 49 +++++++++++++++++++ > 1 file changed, 49 insertions(+) > create mode 100644 Documentation/devicetree/bindings/watchdog/marvell-octeontx2-wdt.yaml > > diff --git a/Documentation/devicetree/bindings/watchdog/marvell-octeontx2-wdt.yaml b/Documentation/devicetree/bindings/watchdog/marvell-octeontx2-wdt.yaml > new file mode 100644 > index 000000000000..e509f26c61b9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/watchdog/marvell-octeontx2-wdt.yaml > @@ -0,0 +1,49 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/watchdog/marvell-octeontx2-wdt.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Marvell OcteonTX2 GTI system watchdog > + > +allOf: > + - $ref: watchdog.yaml# > + > +maintainers: > + - Bharat Bhushan <bbhushan2@marvell.com> > + > +properties: > + compatible: > + const: marvell-octeontx2-wdt That's not correct compatible. marvell is a vendor prefix. Did you test the bindings before sending? > + > + reg: > + maxItems: 1 > + description: > + OcteonTX2 GTI system watchdog register space Drop description, it is obvious. > + > + interrupts: > + maxItems: 1 > + description: > + OcteonTX2 GTI system watchdog interrupt number Ditto > + > +required: > + - compatible > + - reg > + - interrupts > + > +additionalProperties: false unevaluatedProperties: false instead > + > +examples: > + - | > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + > + watchdog@802000040000 { > + compatible = "marvell-octeontx2-wdt"; > + reg = <0x8020 0x40000 0x0 0x20000>; Are you sure that this is correct DTS? 32-bit numbers are usually 8-digit long. Plus size of 0x20000 is crazy huge. And the unit address is a bit unusual. Are you sure dtc W=1 does not say about any errors in your DTS? > + interrupts = <0 38 1>; Use defines for common flags. > + }; > + }; > + > +... Best regards, Krzysztof
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
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Friday, April 14, 2023 4:52 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 1/2] dt-bindings: watchdog: marvell octeonTX2 GTI > system atchdog driver > > External Email > > ---------------------------------------------------------------------- > On 14/04/2023 12:23, Bharat Bhushan wrote: > > Add binding documentation for the Marvell octeonTX2 GTI system > > watchdog driver. > > Subject: typo: atchdog Will fix, thanks > > > > > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> > > --- > > .../watchdog/marvell-octeontx2-wdt.yaml | 49 +++++++++++++++++++ > > 1 file changed, 49 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/watchdog/marvell-octeontx2-wdt.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/watchdog/marvell-octeontx2-wdt.yam > > l > > b/Documentation/devicetree/bindings/watchdog/marvell-octeontx2-wdt.yam > > l > > new file mode 100644 > > index 000000000000..e509f26c61b9 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/watchdog/marvell-octeontx2-wdt > > +++ .yaml > > @@ -0,0 +1,49 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > > +--- > > +$id: > > +https://urldefense.proofpoint.com/v2/url?u=http-3A__devicetree.org_sc > > +hemas_watchdog_marvell-2Docteontx2-2Dwdt.yaml- > 23&d=DwICaQ&c=nKjWec2b6 > > > +R0mOyPaz7xtfQ&r=PAAlWswPe7d8gHlGbCLmy2YezyK7O3Hv_t2heGnouBw&m > =wkYAMhB > > +eHDmObOkMU_NKOHC7amkjDT6HfYf4lFfIUDZg4Oon51wGGw5jD- > WmvjGD&s=YiPlJAjSU > > +9eDReE-1_nRYFsMDoi4cKjvJDRy6jS1S4w&e= > > +$schema: > > +https://urldefense.proofpoint.com/v2/url?u=http-3A__devicetree.org_me > > +ta-2Dschemas_core.yaml- > 23&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PAAlWsw > > > +Pe7d8gHlGbCLmy2YezyK7O3Hv_t2heGnouBw&m=wkYAMhBeHDmObOkMU_N > KOHC7amkjDT > > +6HfYf4lFfIUDZg4Oon51wGGw5jD- > WmvjGD&s=cMu8mewA0U1pRNMAp3MO6pf1fx9cLqXY > > +n4botCarvzU&e= > > + > > +title: Marvell OcteonTX2 GTI system watchdog > > + > > +allOf: > > + - $ref: watchdog.yaml# > > + > > +maintainers: > > + - Bharat Bhushan <bbhushan2@marvell.com> > > + > > +properties: > > + compatible: > > + const: marvell-octeontx2-wdt > > That's not correct compatible. marvell is a vendor prefix. > > Did you test the bindings before sending? Will convert this to marvell,octeontx2-wdt > > > + > > + reg: > > + maxItems: 1 > > + description: > > + OcteonTX2 GTI system watchdog register space > > Drop description, it is obvious. Ok, > > > + > > + interrupts: > > + maxItems: 1 > > + description: > > + OcteonTX2 GTI system watchdog interrupt number > > Ditto > > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + > > +additionalProperties: false > > unevaluatedProperties: false instead Will remove > > > + > > +examples: > > + - | > > + soc { > > + #address-cells = <2>; > > + #size-cells = <2>; > > + > > + watchdog@802000040000 { > > + compatible = "marvell-octeontx2-wdt"; > > + reg = <0x8020 0x40000 0x0 0x20000>; > > Are you sure that this is correct DTS? 32-bit numbers are usually 8-digit long. Plus > size of 0x20000 is crazy huge. And the unit address is a bit unusual. Are you sure > dtc W=1 does not say about any errors in your DTS? Each cell is 32bit, so if we specify less than upper values becomes zeros (0s). > > > > + interrupts = <0 38 1>; > > Use defines for common flags. Ok, Thanks for review -Bharat > > > + }; > > + }; > > + > > +... > > Best regards, > Krzysztof
On 14/04/2023 13:29, Bharat Bhushan wrote: > >> >>> + >>> +examples: >>> + - | >>> + soc { >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + >>> + watchdog@802000040000 { >>> + compatible = "marvell-octeontx2-wdt"; >>> + reg = <0x8020 0x40000 0x0 0x20000>; >> >> Are you sure that this is correct DTS? 32-bit numbers are usually 8-digit long. Plus >> size of 0x20000 is crazy huge. And the unit address is a bit unusual. Are you sure >> dtc W=1 does not say about any errors in your DTS? > > Each cell is 32bit, so if we specify less than upper values becomes zeros (0s). ... and what is the convention/coding style of your subarch? Is to have short or 0-padded reg addresses? Anyway, I have doubts this was tested, so please confirm that dtbs W=1 and dtbs_check produce no errors on your DTS. BTW, where is the DTS? Best regards, Krzysztof
Please see inline > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Friday, April 14, 2023 5:02 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: Re: [EXT] Re: [PATCH 1/2] dt-bindings: watchdog: marvell octeonTX2 GTI > system atchdog driver > > On 14/04/2023 13:29, Bharat Bhushan wrote: > > > > >> > >>> + > >>> +examples: > >>> + - | > >>> + soc { > >>> + #address-cells = <2>; > >>> + #size-cells = <2>; > >>> + > >>> + watchdog@802000040000 { > >>> + compatible = "marvell-octeontx2-wdt"; > >>> + reg = <0x8020 0x40000 0x0 0x20000>; > >> > >> Are you sure that this is correct DTS? 32-bit numbers are usually > >> 8-digit long. Plus size of 0x20000 is crazy huge. And the unit > >> address is a bit unusual. Are you sure dtc W=1 does not say about any errors in > your DTS? > > > > Each cell is 32bit, so if we specify less than upper values becomes zeros (0s). > > ... and what is the convention/coding style of your subarch? Is to have short or 0- > padded reg addresses? > > Anyway, I have doubts this was tested, so please confirm that dtbs W=1 and > dtbs_check produce no errors on your DTS. BTW, where is the DTS? Our device tree are part of firmware and not in Linux. I will check if dtb compilation used "W=1" flag. With this device tree node driver works expectedly. Thanks -Bharat > > 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 >
On Fri, Apr 14, 2023 at 11:34:36AM +0000, Bharat Bhushan wrote: > Please see inline > > > -----Original Message----- > > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Sent: Friday, April 14, 2023 5:02 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: Re: [EXT] Re: [PATCH 1/2] dt-bindings: watchdog: marvell octeonTX2 GTI > > system atchdog driver > > > > On 14/04/2023 13:29, Bharat Bhushan wrote: > > > > > > > >> > > >>> + > > >>> +examples: > > >>> + - | > > >>> + soc { > > >>> + #address-cells = <2>; > > >>> + #size-cells = <2>; > > >>> + > > >>> + watchdog@802000040000 { > > >>> + compatible = "marvell-octeontx2-wdt"; > > >>> + reg = <0x8020 0x40000 0x0 0x20000>; > > >> > > >> Are you sure that this is correct DTS? 32-bit numbers are usually > > >> 8-digit long. Plus size of 0x20000 is crazy huge. And the unit > > >> address is a bit unusual. Are you sure dtc W=1 does not say about any errors in > > your DTS? > > > > > > Each cell is 32bit, so if we specify less than upper values becomes zeros (0s). > > > > ... and what is the convention/coding style of your subarch? Is to have short or 0- > > padded reg addresses? > > > > Anyway, I have doubts this was tested, so please confirm that dtbs W=1 and > > dtbs_check produce no errors on your DTS. BTW, where is the DTS? > > Our device tree are part of firmware and not in Linux. Odd argument. That is true for pretty much every devicetree file. Guenter
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 >>>
diff --git a/Documentation/devicetree/bindings/watchdog/marvell-octeontx2-wdt.yaml b/Documentation/devicetree/bindings/watchdog/marvell-octeontx2-wdt.yaml new file mode 100644 index 000000000000..e509f26c61b9 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/marvell-octeontx2-wdt.yaml @@ -0,0 +1,49 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/watchdog/marvell-octeontx2-wdt.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Marvell OcteonTX2 GTI system watchdog + +allOf: + - $ref: watchdog.yaml# + +maintainers: + - Bharat Bhushan <bbhushan2@marvell.com> + +properties: + compatible: + const: marvell-octeontx2-wdt + + reg: + maxItems: 1 + description: + OcteonTX2 GTI system watchdog register space + + interrupts: + maxItems: 1 + description: + OcteonTX2 GTI system watchdog interrupt number + +required: + - compatible + - reg + - interrupts + +additionalProperties: false + +examples: + - | + soc { + #address-cells = <2>; + #size-cells = <2>; + + watchdog@802000040000 { + compatible = "marvell-octeontx2-wdt"; + reg = <0x8020 0x40000 0x0 0x20000>; + interrupts = <0 38 1>; + }; + }; + +...
Add binding documentation for the Marvell octeonTX2 GTI system watchdog driver. Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> --- .../watchdog/marvell-octeontx2-wdt.yaml | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/marvell-octeontx2-wdt.yaml