diff mbox series

[1/2] dt-bindings: watchdog: marvell octeonTX2 GTI system atchdog driver

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

Commit Message

Bharat Bhushan April 14, 2023, 10:23 a.m. UTC 
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

Comments

Krzysztof Kozlowski April 14, 2023, 11:21 a.m. UTC | #1 
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
Krzysztof Kozlowski April 14, 2023, 11:27 a.m. UTC | #2 
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
Bharat Bhushan April 14, 2023, 11:29 a.m. UTC | #3 
> -----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
Bharat Bhushan April 14, 2023, 11:34 a.m. UTC | #4 
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
Bharat Bhushan April 14, 2023, 11:44 a.m. UTC | #5 
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
Guenter Roeck April 14, 2023, 2:35 p.m. UTC | #6 
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
Guenter Roeck April 17, 2023, 2:10 p.m. UTC | #7 
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 mbox series

Patch

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>;
+        };
+    };
+
+...