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

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

[Bharat Bhushan]

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

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

[PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver

[Bharat Bhushan]

GTI watchdog timer are programmed in "interrupt + del3t + reset mode"
and del3t traps are not enabled.
GTI watchdog exception flow is:
 - 1st timer expiration generates watchdog interrupt.
 - 2nd timer expiration is ignored
 - On 3rd timer expiration will trigger a system-wide core reset.

Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
---
 drivers/watchdog/Kconfig         |   9 ++
 drivers/watchdog/Makefile        |   1 +
 drivers/watchdog/octeontx2_wdt.c | 238 +++++++++++++++++++++++++++++++
 3 files changed, 248 insertions(+)
 create mode 100644 drivers/watchdog/octeontx2_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index f0872970daf9..31ff282c62ad 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -2212,4 +2212,13 @@ config KEEMBAY_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called keembay_wdt.
 
+config OCTEONTX2_WATCHDOG
+	tristate "OCTEONTX2 Watchdog driver"
+	depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT)
+	help
+	 OCTEONTX2 GTI hardware supports watchdog timer. This watchdog timer are
+	 programmed in "interrupt + del3t + reset" mode. On first expiry it will
+	 generate interrupt. Second expiry (del3t) is ignored and system will reset
+	 on final timer expiry.
+
 endif # WATCHDOG
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 9cbf6580f16c..aa1b813ad1f9 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -230,3 +230,4 @@ obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o
 obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
 obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
 obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o
+obj-$(CONFIG_OCTEONTX2_WATCHDOG) += octeontx2_wdt.o
diff --git a/drivers/watchdog/octeontx2_wdt.c b/drivers/watchdog/octeontx2_wdt.c
new file mode 100644
index 000000000000..7b78a092e83f
--- /dev/null
+++ b/drivers/watchdog/octeontx2_wdt.c
@@ -0,0 +1,238 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Marvell Octeontx2 Watchdog driver
+ *
+ * Copyright (C) 2023 Marvell International Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/cpu.h>
+#include <linux/interrupt.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+#include <linux/sched/debug.h>
+
+#include <asm/arch_timer.h>
+
+/* GTI CWD Watchdog Registers */
+#define GTI_CWD_GLOBAL_WDOG_IDX		63
+#define GTI_CWD_WDOG			(0x8 * GTI_CWD_GLOBAL_WDOG_IDX)
+#define GTI_CWD_WDOG_MODE_INT_DEL3T_RST	0x3
+#define GTI_CWD_WDOG_MODE_MASK		0x3
+#define GTI_CWD_WDOG_LEN_SHIFT		4
+#define GTI_CWD_WDOG_CNT_SHIFT		20
+
+/* GTI Per-core Watchdog Interrupt Register */
+#define GTI_CWD_INT			0x200
+#define GTI_CWD_INT_PENDING_STATUS	(1ULL << GTI_CWD_GLOBAL_WDOG_IDX)
+
+/* GTI Per-core Watchdog Interrupt Enable Clear Register */
+#define GTI_CWD_INT_ENA_CLR		0x210
+#define GTI_CWD_INT_ENA_CLR_VAL		(1ULL << GTI_CWD_GLOBAL_WDOG_IDX)
+
+/* GTI Per-core Watchdog Interrupt Enable Set Register */
+#define GTI_CWD_INT_ENA_SET		0x218
+#define GTI_CWD_INT_ENA_SET_VAL		(1ULL << GTI_CWD_GLOBAL_WDOG_IDX)
+
+/* GTI Per-core Watchdog Poke Registers */
+#define GTI_CWD_POKE		(0x10000 + 0x8 * GTI_CWD_GLOBAL_WDOG_IDX)
+#define GTI_CWD_POKE_VAL	(1ULL << GTI_CWD_GLOBAL_WDOG_IDX)
+
+struct octeontx2_wdt_priv {
+	struct watchdog_device wdev;
+	void __iomem *base;
+	u64 clock_freq;
+	int irq;
+};
+
+static irqreturn_t octeontx2_wdt_interrupt(int irq, void *data)
+{
+	panic("Kernel Watchdog");
+	return IRQ_HANDLED;
+}
+
+static int octeontx2_wdt_ping(struct watchdog_device *wdev)
+{
+	struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+	writeq(GTI_CWD_POKE_VAL, priv->base + GTI_CWD_POKE);
+	return 0;
+}
+
+static int octeontx2_wdt_start(struct watchdog_device *wdev)
+{
+	struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	u64 regval;
+
+	set_bit(WDOG_HW_RUNNING, &wdev->status);
+
+	/* Clear any pending interrupt */
+	writeq(GTI_CWD_INT_PENDING_STATUS, priv->base + GTI_CWD_INT);
+
+	/* Enable Interrupt */
+	writeq(GTI_CWD_INT_ENA_SET_VAL, priv->base + GTI_CWD_INT_ENA_SET);
+
+	/* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */
+	regval = readq(priv->base + GTI_CWD_WDOG);
+	regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST;
+	writeq(regval, priv->base + GTI_CWD_WDOG);
+
+	return 0;
+}
+
+static int octeontx2_wdt_stop(struct watchdog_device *wdev)
+{
+	struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	u64 regval;
+
+	/* Disable Interrupt */
+	writeq(GTI_CWD_INT_ENA_CLR_VAL, priv->base + GTI_CWD_INT_ENA_CLR);
+
+	/* Set GTI_CWD_WDOG.Mode = 0 to stop the timer */
+	regval = readq(priv->base + GTI_CWD_WDOG);
+	regval &= ~GTI_CWD_WDOG_MODE_MASK;
+	writeq(regval, priv->base + GTI_CWD_WDOG);
+
+	return 0;
+}
+
+static int octeontx2_wdt_settimeout(struct watchdog_device *wdev,
+					unsigned int timeout)
+{
+	struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	u64 timeout_wdog, regval;
+
+	/* Update new timeout */
+	wdev->timeout = timeout;
+
+	/* Get clock cycles from timeout in second */
+	timeout_wdog = (u64)timeout * priv->clock_freq;
+
+	/* Watchdog counts in 1024 cycle steps */
+	timeout_wdog = timeout_wdog >> 10;
+
+	/*
+	 * Hardware allows programming of upper 16-bits of 24-bits cycles
+	 * Round up and use upper 16-bits only.
+	 * Set max if timeout more than h/w supported
+	 */
+	timeout_wdog = (timeout_wdog + 0xff) >> 8;
+	if (timeout_wdog >= 0x10000)
+		timeout_wdog = 0xffff;
+
+	/*
+	 * GTI_CWD_WDOG.LEN have only upper 16-bits of 24-bits
+	 * GTI_CWD_WDOG.CNT, need addition shift of 8.
+	 */
+	regval = readq(priv->base + GTI_CWD_WDOG);
+	regval &= GTI_CWD_WDOG_MODE_MASK;
+	regval |= ((timeout_wdog) << (GTI_CWD_WDOG_CNT_SHIFT + 8)) |
+		   (timeout_wdog << GTI_CWD_WDOG_LEN_SHIFT);
+	writeq(regval, priv->base + GTI_CWD_WDOG);
+	return 0;
+}
+
+static const struct watchdog_info octeontx2_wdt_ident = {
+	.identity = "OcteonTX2 watchdog",
+	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE |
+			  WDIOF_CARDRESET,
+};
+
+static const struct watchdog_ops octeontx2_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = octeontx2_wdt_start,
+	.stop = octeontx2_wdt_stop,
+	.ping = octeontx2_wdt_ping,
+	.set_timeout = octeontx2_wdt_settimeout,
+};
+
+static int octeontx2_wdt_probe(struct platform_device *pdev)
+{
+	struct octeontx2_wdt_priv *priv;
+	struct device *dev = &pdev->dev;
+	struct watchdog_device *wdog_dev;
+	int irq;
+	int err;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return dev_err_probe(&pdev->dev, PTR_ERR(priv->base),
+			      "reg property not valid/found\n");
+
+	priv->clock_freq = arch_timer_get_cntfrq();
+
+	wdog_dev = &priv->wdev;
+	wdog_dev->info = &octeontx2_wdt_ident,
+	wdog_dev->ops = &octeontx2_wdt_ops,
+	wdog_dev->parent = dev;
+	wdog_dev->min_timeout = 1;
+	wdog_dev->max_timeout = 16;
+	wdog_dev->max_hw_heartbeat_ms = 16000;
+	wdog_dev->timeout = 8;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "IRQ resource not found\n");
+		return -ENODEV;
+	}
+
+	err = request_irq(irq, octeontx2_wdt_interrupt, 0, pdev->name, priv);
+	if (err) {
+		dev_err(dev, "cannot register interrupt handler %d\n", err);
+		return err;
+	}
+
+	priv->irq = irq;
+	watchdog_set_drvdata(wdog_dev, priv);
+	platform_set_drvdata(pdev, priv);
+	watchdog_init_timeout(wdog_dev, wdog_dev->timeout, dev);
+	octeontx2_wdt_settimeout(wdog_dev, wdog_dev->timeout);
+	watchdog_stop_on_reboot(wdog_dev);
+	watchdog_stop_on_unregister(wdog_dev);
+
+	err = devm_watchdog_register_device(dev, wdog_dev);
+	if (err) {
+		free_irq(irq, priv);
+		return err;
+	}
+
+	dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev->timeout);
+	return 0;
+}
+
+static int octeontx2_wdt_remove(struct platform_device *pdev)
+{
+	struct octeontx2_wdt_priv *priv = platform_get_drvdata(pdev);
+
+	if (priv->irq)
+		free_irq(priv->irq, priv);
+
+	return 0;
+}
+
+static const struct of_device_id octeontx2_wdt_of_match[] = {
+	{ .compatible = "marvell-octeontx2-wdt", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, octeontx2_wdt_of_match);
+
+static struct platform_driver octeontx2_wdt_driver = {
+	.driver = {
+		.name = "octeontx2-wdt",
+		.of_match_table = octeontx2_wdt_of_match,
+	},
+	.probe = octeontx2_wdt_probe,
+	.remove = octeontx2_wdt_remove,
+};
+module_platform_driver(octeontx2_wdt_driver);
+
+MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>");
+MODULE_DESCRIPTION("OcteonTX2 watchdog driver");
-- 
2.17.1

Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver

[Krzysztof Kozlowski]

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

RE: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver

[Bharat Bhushan]

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

Re: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver

[Krzysztof Kozlowski]

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

Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver

[Guenter Roeck]

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
> 

RE: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver

[Bharat Bhushan]

Please see inline

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Friday, April 14, 2023 8:05 PM
> To: Bharat Bhushan <bbhushan2@marvell.com>
> Cc: wim@linux-watchdog.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; linux-watchdog@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Fri, Apr 14, 2023 at 03:53:42PM +0530, Bharat Bhushan wrote:
> > GTI watchdog timer are programmed in "interrupt + del3t + reset mode"
> > and del3t traps are not enabled.
> > GTI watchdog exception flow is:
> >  - 1st timer expiration generates watchdog interrupt.
> >  - 2nd timer expiration is ignored
> >  - On 3rd timer expiration will trigger a system-wide core reset.
> >
> 
> This means the interrupt should not result in a panic, the programmed timeout
> value should be considered a pretimeout which is set to (timeout / 3), and the
> interrupt handler should call watchdog_notify_pretimeout().
> 
> Either case, the above should be documented in the driver but does not make
> much if any sense as patch description. If not, what are the other modes, and
> why is this mode used instead of any of those modes ?

Hardware supports following mode of operation:
1) Interrupt Only:
    This will generate the interrupt to arm core whenever timeout happens.

2) Interrupt + del3t (Interrupt to firmware (SCP processor)).
     This will generate interrupt to arm core whenever 1st timeout happens
     This will generate interrupt to SCP processor whenever 2nd timeout happens

3) Interrupt + Interrupt to SCP processor (called delt3t) + reboot.
     This will generate interrupt to arm core whenever 1st timeout happens
     This will generate interrupt to SCP processor whenever 2nd timeout happens, if interrupt is configured.
     This will reboot on 3rd timeout.

We are going with mode-3 above so that system can reboot in case a hardware hang. Also h/w is configured not to generate SCP interrupt, so effectively 2nd timeout is ignored within hardware.

Software is supposed to poke within 1st timeout. If poke does not happen then it will receive interrupt, interrupt handler will do panic.
But for some reason if processor can not take interrupt then system will reboot on 3rd timeout.

> 
> > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> > ---
> >  drivers/watchdog/Kconfig         |   9 ++
> >  drivers/watchdog/Makefile        |   1 +
> >  drivers/watchdog/octeontx2_wdt.c | 238
> > +++++++++++++++++++++++++++++++
> >  3 files changed, 248 insertions(+)
> >  create mode 100644 drivers/watchdog/octeontx2_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
> > f0872970daf9..31ff282c62ad 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -2212,4 +2212,13 @@ config KEEMBAY_WATCHDOG
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called keembay_wdt.
> >
> > +config OCTEONTX2_WATCHDOG
> > +	tristate "OCTEONTX2 Watchdog driver"
> > +	depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT)
> > +	help
> > +	 OCTEONTX2 GTI hardware supports watchdog timer. This watchdog
> timer are
> > +	 programmed in "interrupt + del3t + reset" mode. On first expiry it will
> > +	 generate interrupt. Second expiry (del3t) is ignored and system will reset
> > +	 on final timer expiry.
> > +
> 
> The above should be part of the in-driver documentation but those details
> should not be in Kconfig.

Ok,

> 
> Is the documentation available in public ? I'd like to check if this mode, especially
> the ignored del3t mode, really makes sense.

Documentation is not public. Provided some description above, let me know if I need to provide some more details.

> 
> >  endif # WATCHDOG
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 9cbf6580f16c..aa1b813ad1f9 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -230,3 +230,4 @@ obj-$(CONFIG_MENZ069_WATCHDOG) +=
> menz69_wdt.o
> >  obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
> >  obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
> >  obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o
> > +obj-$(CONFIG_OCTEONTX2_WATCHDOG) += octeontx2_wdt.o
> > diff --git a/drivers/watchdog/octeontx2_wdt.c
> > b/drivers/watchdog/octeontx2_wdt.c
> > new file mode 100644
> > index 000000000000..7b78a092e83f
> > --- /dev/null
> > +++ b/drivers/watchdog/octeontx2_wdt.c
> > @@ -0,0 +1,238 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Marvell Octeontx2 Watchdog driver
> > + *
> > + * Copyright (C) 2023 Marvell International Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/cpu.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/watchdog.h>
> > +#include <linux/sched/debug.h>
> 
> What is this include for ?

Taken from other driver when started coding this one. Will remove

> 
> > +
> > +#include <asm/arch_timer.h>
> > +
> > +/* GTI CWD Watchdog Registers */
> > +#define GTI_CWD_GLOBAL_WDOG_IDX		63
> > +#define GTI_CWD_WDOG			(0x8 *
> GTI_CWD_GLOBAL_WDOG_IDX)
> > +#define GTI_CWD_WDOG_MODE_INT_DEL3T_RST	0x3
> > +#define GTI_CWD_WDOG_MODE_MASK		0x3
> > +#define GTI_CWD_WDOG_LEN_SHIFT		4
> > +#define GTI_CWD_WDOG_CNT_SHIFT		20
> > +
> > +/* GTI Per-core Watchdog Interrupt Register */
> > +#define GTI_CWD_INT			0x200
> > +#define GTI_CWD_INT_PENDING_STATUS	(1ULL <<
> GTI_CWD_GLOBAL_WDOG_IDX)
> > +
> > +/* GTI Per-core Watchdog Interrupt Enable Clear Register */
> > +#define GTI_CWD_INT_ENA_CLR		0x210
> > +#define GTI_CWD_INT_ENA_CLR_VAL		(1ULL <<
> GTI_CWD_GLOBAL_WDOG_IDX)
> > +
> > +/* GTI Per-core Watchdog Interrupt Enable Set Register */
> > +#define GTI_CWD_INT_ENA_SET		0x218
> > +#define GTI_CWD_INT_ENA_SET_VAL		(1ULL <<
> GTI_CWD_GLOBAL_WDOG_IDX)
> > +
> > +/* GTI Per-core Watchdog Poke Registers */
> > +#define GTI_CWD_POKE		(0x10000 + 0x8 *
> GTI_CWD_GLOBAL_WDOG_IDX)
> > +#define GTI_CWD_POKE_VAL	(1ULL << GTI_CWD_GLOBAL_WDOG_IDX)
> > +
> > +struct octeontx2_wdt_priv {
> > +	struct watchdog_device wdev;
> > +	void __iomem *base;
> > +	u64 clock_freq;
> > +	int irq;
> > +};
> > +
> > +static irqreturn_t octeontx2_wdt_interrupt(int irq, void *data) {
> > +	panic("Kernel Watchdog");
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int octeontx2_wdt_ping(struct watchdog_device *wdev) {
> > +	struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +	writeq(GTI_CWD_POKE_VAL, priv->base + GTI_CWD_POKE);
> > +	return 0;
> > +}
> > +
> > +static int octeontx2_wdt_start(struct watchdog_device *wdev) {
> > +	struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +	u64 regval;
> > +
> > +	set_bit(WDOG_HW_RUNNING, &wdev->status);
> > +
> > +	/* Clear any pending interrupt */
> > +	writeq(GTI_CWD_INT_PENDING_STATUS, priv->base + GTI_CWD_INT);
> > +
> > +	/* Enable Interrupt */
> > +	writeq(GTI_CWD_INT_ENA_SET_VAL, priv->base +
> GTI_CWD_INT_ENA_SET);
> > +
> > +	/* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */
> > +	regval = readq(priv->base + GTI_CWD_WDOG);
> > +	regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST;
> > +	writeq(regval, priv->base + GTI_CWD_WDOG);
> > +
> > +	return 0;
> > +}
> > +
> > +static int octeontx2_wdt_stop(struct watchdog_device *wdev) {
> > +	struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +	u64 regval;
> > +
> > +	/* Disable Interrupt */
> > +	writeq(GTI_CWD_INT_ENA_CLR_VAL, priv->base +
> GTI_CWD_INT_ENA_CLR);
> > +
> > +	/* Set GTI_CWD_WDOG.Mode = 0 to stop the timer */
> > +	regval = readq(priv->base + GTI_CWD_WDOG);
> > +	regval &= ~GTI_CWD_WDOG_MODE_MASK;
> > +	writeq(regval, priv->base + GTI_CWD_WDOG);
> > +
> > +	return 0;
> > +}
> > +
> > +static int octeontx2_wdt_settimeout(struct watchdog_device *wdev,
> > +					unsigned int timeout)
> > +{
> > +	struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +	u64 timeout_wdog, regval;
> > +
> > +	/* Update new timeout */
> > +	wdev->timeout = timeout;
> > +
> > +	/* Get clock cycles from timeout in second */
> > +	timeout_wdog = (u64)timeout * priv->clock_freq;
> > +
> > +	/* Watchdog counts in 1024 cycle steps */
> > +	timeout_wdog = timeout_wdog >> 10;
> > +
> > +	/*
> > +	 * Hardware allows programming of upper 16-bits of 24-bits cycles
> > +	 * Round up and use upper 16-bits only.
> > +	 * Set max if timeout more than h/w supported
> 
> This should be covered when setting max_timeout or max_hw_heartbeat_ms.
> Setting the actual timeout to a value smaller than configured may result in the
> watchdog firing before its configured timeout expires.

wdog_dev->max_timeout = 16 is set based on above description, will move this comment there.

Just to explain why I added above comment here and below check, that was to ensure that if user provides timeout more than " wdog_dev->max_timeout" then program wdog_dev->max_timeout only.
But now looking at the code "set_timeout()" will be called only if timeout is valid ( timeout is <= wdog_dev->max_timeout)

So above comment is not valid, while below check can also be removed.

> 
> > +	 */
> > +	timeout_wdog = (timeout_wdog + 0xff) >> 8;
> > +	if (timeout_wdog >= 0x10000)
> > +		timeout_wdog = 0xffff;
> > +
> > +	/*
> > +	 * GTI_CWD_WDOG.LEN have only upper 16-bits of 24-bits
> > +	 * GTI_CWD_WDOG.CNT, need addition shift of 8.
> > +	 */
> > +	regval = readq(priv->base + GTI_CWD_WDOG);
> > +	regval &= GTI_CWD_WDOG_MODE_MASK;
> > +	regval |= ((timeout_wdog) << (GTI_CWD_WDOG_CNT_SHIFT + 8)) |
> > +		   (timeout_wdog << GTI_CWD_WDOG_LEN_SHIFT);
> 
> () around timeout is unnecessary. Why does the timeout need to be programmed
> twice into the register ? The shift values are odd.
> Are you sure this does what you expect it to do ?

This register have two timeouts:
   GTI_CWD_WDOG.CNT[43:20] (24bit) = this is something decrementing with a frequency.
   GTI_CWD_WDOG.LEN[19:4] (16bit) = this is something loaded in upper 16 bits of GTI_CWD_WDOG.CNT when poke happens.

Shift looks odd but it works as expected.

> 
> > +	writeq(regval, priv->base + GTI_CWD_WDOG);
> > +	return 0;
> > +}
> > +
> > +static const struct watchdog_info octeontx2_wdt_ident = {
> > +	.identity = "OcteonTX2 watchdog",
> > +	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> WDIOF_MAGICCLOSE |
> > +			  WDIOF_CARDRESET,
> > +};
> > +
> > +static const struct watchdog_ops octeontx2_wdt_ops = {
> > +	.owner = THIS_MODULE,
> > +	.start = octeontx2_wdt_start,
> > +	.stop = octeontx2_wdt_stop,
> > +	.ping = octeontx2_wdt_ping,
> > +	.set_timeout = octeontx2_wdt_settimeout, };
> > +
> > +static int octeontx2_wdt_probe(struct platform_device *pdev) {
> > +	struct octeontx2_wdt_priv *priv;
> > +	struct device *dev = &pdev->dev;
> > +	struct watchdog_device *wdog_dev;
> > +	int irq;
> > +	int err;
> > +
> > +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->base))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->base),
> > +			      "reg property not valid/found\n");
> > +
> > +	priv->clock_freq = arch_timer_get_cntfrq();
> > +
> > +	wdog_dev = &priv->wdev;
> > +	wdog_dev->info = &octeontx2_wdt_ident,
> > +	wdog_dev->ops = &octeontx2_wdt_ops,
> > +	wdog_dev->parent = dev;
> > +	wdog_dev->min_timeout = 1;
> > +	wdog_dev->max_timeout = 16;
> 
> Setting max_timeout makes max_hw_heartbeat_ms useless. Use only
> max_hw_heartbeat_ms to enable larger soft timeouts, or use only max_timeout
> to set the hard limit, but not both.

Okay, thanks for details

> 
> > +	wdog_dev->max_hw_heartbeat_ms = 16000;
> > +	wdog_dev->timeout = 8;
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		dev_err(&pdev->dev, "IRQ resource not found\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	err = request_irq(irq, octeontx2_wdt_interrupt, 0, pdev->name, priv);
> > +	if (err) {
> > +		dev_err(dev, "cannot register interrupt handler %d\n", err);
> > +		return err;
> > +	}
> 
> Use devm_request_irq() and request the interrupt after registering the watchdog.

Okay,

> 
> > +
> > +	priv->irq = irq;
> > +	watchdog_set_drvdata(wdog_dev, priv);
> > +	platform_set_drvdata(pdev, priv);
> > +	watchdog_init_timeout(wdog_dev, wdog_dev->timeout, dev);
> 
> watchdog_init_timeout sets wdog_dev->timeout, so this is pointless.
> Calling watchdog_init_timeout() only makes sense if the parameter is either a
> timeout module parameter or 0 and the idea is to use a value from devicetree if
> configured.

Okay, thanks again for detail

Regards
-Bharat

> 
> > +	octeontx2_wdt_settimeout(wdog_dev, wdog_dev->timeout);
> > +	watchdog_stop_on_reboot(wdog_dev);
> > +	watchdog_stop_on_unregister(wdog_dev);
> > +
> > +	err = devm_watchdog_register_device(dev, wdog_dev);
> > +	if (err) {
> > +		free_irq(irq, priv);
> > +		return err;
> > +	}
> > +
> > +	dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev-
> >timeout);
> > +	return 0;
> > +}
> > +
> > +static int octeontx2_wdt_remove(struct platform_device *pdev) {
> > +	struct octeontx2_wdt_priv *priv = platform_get_drvdata(pdev);
> > +
> > +	if (priv->irq)
> > +		free_irq(priv->irq, priv);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id octeontx2_wdt_of_match[] = {
> > +	{ .compatible = "marvell-octeontx2-wdt", },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, octeontx2_wdt_of_match);
> > +
> > +static struct platform_driver octeontx2_wdt_driver = {
> > +	.driver = {
> > +		.name = "octeontx2-wdt",
> > +		.of_match_table = octeontx2_wdt_of_match,
> > +	},
> > +	.probe = octeontx2_wdt_probe,
> > +	.remove = octeontx2_wdt_remove,
> > +};
> > +module_platform_driver(octeontx2_wdt_driver);
> > +
> > +MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>");
> > +MODULE_DESCRIPTION("OcteonTX2 watchdog driver");
> > --
> > 2.17.1
> >

Re: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver

[Guenter Roeck]

On 4/17/23 04:06, Bharat Bhushan wrote:
> Please see inline
> 
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>> Sent: Friday, April 14, 2023 8:05 PM
>> To: Bharat Bhushan <bbhushan2@marvell.com>
>> Cc: wim@linux-watchdog.org; robh+dt@kernel.org;
>> krzysztof.kozlowski+dt@linaro.org; linux-watchdog@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> On Fri, Apr 14, 2023 at 03:53:42PM +0530, Bharat Bhushan wrote:
>>> GTI watchdog timer are programmed in "interrupt + del3t + reset mode"
>>> and del3t traps are not enabled.
>>> GTI watchdog exception flow is:
>>>   - 1st timer expiration generates watchdog interrupt.
>>>   - 2nd timer expiration is ignored
>>>   - On 3rd timer expiration will trigger a system-wide core reset.
>>>
>>
>> This means the interrupt should not result in a panic, the programmed timeout
>> value should be considered a pretimeout which is set to (timeout / 3), and the
>> interrupt handler should call watchdog_notify_pretimeout().
>>
>> Either case, the above should be documented in the driver but does not make
>> much if any sense as patch description. If not, what are the other modes, and
>> why is this mode used instead of any of those modes ?
> 
> Hardware supports following mode of operation:
> 1) Interrupt Only:
>      This will generate the interrupt to arm core whenever timeout happens.
> 
> 2) Interrupt + del3t (Interrupt to firmware (SCP processor)).
>       This will generate interrupt to arm core whenever 1st timeout happens
>       This will generate interrupt to SCP processor whenever 2nd timeout happens
> 
> 3) Interrupt + Interrupt to SCP processor (called delt3t) + reboot.
>       This will generate interrupt to arm core whenever 1st timeout happens
>       This will generate interrupt to SCP processor whenever 2nd timeout happens, if interrupt is configured.
>       This will reboot on 3rd timeout.
> 
> We are going with mode-3 above so that system can reboot in case a hardware hang. Also h/w is configured not to generate SCP interrupt, so effectively 2nd timeout is ignored within hardware.
> 
> Software is supposed to poke within 1st timeout. If poke does not happen then it will receive interrupt, interrupt handler will do panic.
> But for some reason if processor can not take interrupt then system will reboot on 3rd timeout.
> 
Then, as I said, the first timeout shopuld be modeled as pretimeout.

Thanks,
Guenter

>>
>>> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
>>> ---
>>>   drivers/watchdog/Kconfig         |   9 ++
>>>   drivers/watchdog/Makefile        |   1 +
>>>   drivers/watchdog/octeontx2_wdt.c | 238
>>> +++++++++++++++++++++++++++++++
>>>   3 files changed, 248 insertions(+)
>>>   create mode 100644 drivers/watchdog/octeontx2_wdt.c
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
>>> f0872970daf9..31ff282c62ad 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -2212,4 +2212,13 @@ config KEEMBAY_WATCHDOG
>>>   	  To compile this driver as a module, choose M here: the
>>>   	  module will be called keembay_wdt.
>>>
>>> +config OCTEONTX2_WATCHDOG
>>> +	tristate "OCTEONTX2 Watchdog driver"
>>> +	depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT)
>>> +	help
>>> +	 OCTEONTX2 GTI hardware supports watchdog timer. This watchdog
>> timer are
>>> +	 programmed in "interrupt + del3t + reset" mode. On first expiry it will
>>> +	 generate interrupt. Second expiry (del3t) is ignored and system will reset
>>> +	 on final timer expiry.
>>> +
>>
>> The above should be part of the in-driver documentation but those details
>> should not be in Kconfig.
> 
> Ok,
> 
>>
>> Is the documentation available in public ? I'd like to check if this mode, especially
>> the ignored del3t mode, really makes sense.
> 
> Documentation is not public. Provided some description above, let me know if I need to provide some more details.
> 
>>
>>>   endif # WATCHDOG
>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>> index 9cbf6580f16c..aa1b813ad1f9 100644
>>> --- a/drivers/watchdog/Makefile
>>> +++ b/drivers/watchdog/Makefile
>>> @@ -230,3 +230,4 @@ obj-$(CONFIG_MENZ069_WATCHDOG) +=
>> menz69_wdt.o
>>>   obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
>>>   obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
>>>   obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o
>>> +obj-$(CONFIG_OCTEONTX2_WATCHDOG) += octeontx2_wdt.o
>>> diff --git a/drivers/watchdog/octeontx2_wdt.c
>>> b/drivers/watchdog/octeontx2_wdt.c
>>> new file mode 100644
>>> index 000000000000..7b78a092e83f
>>> --- /dev/null
>>> +++ b/drivers/watchdog/octeontx2_wdt.c
>>> @@ -0,0 +1,238 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/* Marvell Octeontx2 Watchdog driver
>>> + *
>>> + * Copyright (C) 2023 Marvell International Ltd.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> +modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/cpu.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/watchdog.h>
>>> +#include <linux/sched/debug.h>
>>
>> What is this include for ?
> 
> Taken from other driver when started coding this one. Will remove
> 
>>
>>> +
>>> +#include <asm/arch_timer.h>
>>> +
>>> +/* GTI CWD Watchdog Registers */
>>> +#define GTI_CWD_GLOBAL_WDOG_IDX		63
>>> +#define GTI_CWD_WDOG			(0x8 *
>> GTI_CWD_GLOBAL_WDOG_IDX)
>>> +#define GTI_CWD_WDOG_MODE_INT_DEL3T_RST	0x3
>>> +#define GTI_CWD_WDOG_MODE_MASK		0x3
>>> +#define GTI_CWD_WDOG_LEN_SHIFT		4
>>> +#define GTI_CWD_WDOG_CNT_SHIFT		20
>>> +
>>> +/* GTI Per-core Watchdog Interrupt Register */
>>> +#define GTI_CWD_INT			0x200
>>> +#define GTI_CWD_INT_PENDING_STATUS	(1ULL <<
>> GTI_CWD_GLOBAL_WDOG_IDX)
>>> +
>>> +/* GTI Per-core Watchdog Interrupt Enable Clear Register */
>>> +#define GTI_CWD_INT_ENA_CLR		0x210
>>> +#define GTI_CWD_INT_ENA_CLR_VAL		(1ULL <<
>> GTI_CWD_GLOBAL_WDOG_IDX)
>>> +
>>> +/* GTI Per-core Watchdog Interrupt Enable Set Register */
>>> +#define GTI_CWD_INT_ENA_SET		0x218
>>> +#define GTI_CWD_INT_ENA_SET_VAL		(1ULL <<
>> GTI_CWD_GLOBAL_WDOG_IDX)
>>> +
>>> +/* GTI Per-core Watchdog Poke Registers */
>>> +#define GTI_CWD_POKE		(0x10000 + 0x8 *
>> GTI_CWD_GLOBAL_WDOG_IDX)
>>> +#define GTI_CWD_POKE_VAL	(1ULL << GTI_CWD_GLOBAL_WDOG_IDX)
>>> +
>>> +struct octeontx2_wdt_priv {
>>> +	struct watchdog_device wdev;
>>> +	void __iomem *base;
>>> +	u64 clock_freq;
>>> +	int irq;
>>> +};
>>> +
>>> +static irqreturn_t octeontx2_wdt_interrupt(int irq, void *data) {
>>> +	panic("Kernel Watchdog");
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int octeontx2_wdt_ping(struct watchdog_device *wdev) {
>>> +	struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>> +
>>> +	writeq(GTI_CWD_POKE_VAL, priv->base + GTI_CWD_POKE);
>>> +	return 0;
>>> +}
>>> +
>>> +static int octeontx2_wdt_start(struct watchdog_device *wdev) {
>>> +	struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>> +	u64 regval;
>>> +
>>> +	set_bit(WDOG_HW_RUNNING, &wdev->status);
>>> +
>>> +	/* Clear any pending interrupt */
>>> +	writeq(GTI_CWD_INT_PENDING_STATUS, priv->base + GTI_CWD_INT);
>>> +
>>> +	/* Enable Interrupt */
>>> +	writeq(GTI_CWD_INT_ENA_SET_VAL, priv->base +
>> GTI_CWD_INT_ENA_SET);
>>> +
>>> +	/* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */
>>> +	regval = readq(priv->base + GTI_CWD_WDOG);
>>> +	regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST;
>>> +	writeq(regval, priv->base + GTI_CWD_WDOG);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int octeontx2_wdt_stop(struct watchdog_device *wdev) {
>>> +	struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>> +	u64 regval;
>>> +
>>> +	/* Disable Interrupt */
>>> +	writeq(GTI_CWD_INT_ENA_CLR_VAL, priv->base +
>> GTI_CWD_INT_ENA_CLR);
>>> +
>>> +	/* Set GTI_CWD_WDOG.Mode = 0 to stop the timer */
>>> +	regval = readq(priv->base + GTI_CWD_WDOG);
>>> +	regval &= ~GTI_CWD_WDOG_MODE_MASK;
>>> +	writeq(regval, priv->base + GTI_CWD_WDOG);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int octeontx2_wdt_settimeout(struct watchdog_device *wdev,
>>> +					unsigned int timeout)
>>> +{
>>> +	struct octeontx2_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>> +	u64 timeout_wdog, regval;
>>> +
>>> +	/* Update new timeout */
>>> +	wdev->timeout = timeout;
>>> +
>>> +	/* Get clock cycles from timeout in second */
>>> +	timeout_wdog = (u64)timeout * priv->clock_freq;
>>> +
>>> +	/* Watchdog counts in 1024 cycle steps */
>>> +	timeout_wdog = timeout_wdog >> 10;
>>> +
>>> +	/*
>>> +	 * Hardware allows programming of upper 16-bits of 24-bits cycles
>>> +	 * Round up and use upper 16-bits only.
>>> +	 * Set max if timeout more than h/w supported
>>
>> This should be covered when setting max_timeout or max_hw_heartbeat_ms.
>> Setting the actual timeout to a value smaller than configured may result in the
>> watchdog firing before its configured timeout expires.
> 
> wdog_dev->max_timeout = 16 is set based on above description, will move this comment there.
> 
> Just to explain why I added above comment here and below check, that was to ensure that if user provides timeout more than " wdog_dev->max_timeout" then program wdog_dev->max_timeout only.
> But now looking at the code "set_timeout()" will be called only if timeout is valid ( timeout is <= wdog_dev->max_timeout)
> 
> So above comment is not valid, while below check can also be removed.
> 
>>
>>> +	 */
>>> +	timeout_wdog = (timeout_wdog + 0xff) >> 8;
>>> +	if (timeout_wdog >= 0x10000)
>>> +		timeout_wdog = 0xffff;
>>> +
>>> +	/*
>>> +	 * GTI_CWD_WDOG.LEN have only upper 16-bits of 24-bits
>>> +	 * GTI_CWD_WDOG.CNT, need addition shift of 8.
>>> +	 */
>>> +	regval = readq(priv->base + GTI_CWD_WDOG);
>>> +	regval &= GTI_CWD_WDOG_MODE_MASK;
>>> +	regval |= ((timeout_wdog) << (GTI_CWD_WDOG_CNT_SHIFT + 8)) |
>>> +		   (timeout_wdog << GTI_CWD_WDOG_LEN_SHIFT);
>>
>> () around timeout is unnecessary. Why does the timeout need to be programmed
>> twice into the register ? The shift values are odd.
>> Are you sure this does what you expect it to do ?
> 
> This register have two timeouts:
>     GTI_CWD_WDOG.CNT[43:20] (24bit) = this is something decrementing with a frequency.
>     GTI_CWD_WDOG.LEN[19:4] (16bit) = this is something loaded in upper 16 bits of GTI_CWD_WDOG.CNT when poke happens.
> 
> Shift looks odd but it works as expected.
> 
>>
>>> +	writeq(regval, priv->base + GTI_CWD_WDOG);
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct watchdog_info octeontx2_wdt_ident = {
>>> +	.identity = "OcteonTX2 watchdog",
>>> +	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
>> WDIOF_MAGICCLOSE |
>>> +			  WDIOF_CARDRESET,
>>> +};
>>> +
>>> +static const struct watchdog_ops octeontx2_wdt_ops = {
>>> +	.owner = THIS_MODULE,
>>> +	.start = octeontx2_wdt_start,
>>> +	.stop = octeontx2_wdt_stop,
>>> +	.ping = octeontx2_wdt_ping,
>>> +	.set_timeout = octeontx2_wdt_settimeout, };
>>> +
>>> +static int octeontx2_wdt_probe(struct platform_device *pdev) {
>>> +	struct octeontx2_wdt_priv *priv;
>>> +	struct device *dev = &pdev->dev;
>>> +	struct watchdog_device *wdog_dev;
>>> +	int irq;
>>> +	int err;
>>> +
>>> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return -ENOMEM;
>>> +
>>> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
>>> +	if (IS_ERR(priv->base))
>>> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->base),
>>> +			      "reg property not valid/found\n");
>>> +
>>> +	priv->clock_freq = arch_timer_get_cntfrq();
>>> +
>>> +	wdog_dev = &priv->wdev;
>>> +	wdog_dev->info = &octeontx2_wdt_ident,
>>> +	wdog_dev->ops = &octeontx2_wdt_ops,
>>> +	wdog_dev->parent = dev;
>>> +	wdog_dev->min_timeout = 1;
>>> +	wdog_dev->max_timeout = 16;
>>
>> Setting max_timeout makes max_hw_heartbeat_ms useless. Use only
>> max_hw_heartbeat_ms to enable larger soft timeouts, or use only max_timeout
>> to set the hard limit, but not both.
> 
> Okay, thanks for details
> 
>>
>>> +	wdog_dev->max_hw_heartbeat_ms = 16000;
>>> +	wdog_dev->timeout = 8;
>>> +
>>> +	irq = platform_get_irq(pdev, 0);
>>> +	if (irq < 0) {
>>> +		dev_err(&pdev->dev, "IRQ resource not found\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	err = request_irq(irq, octeontx2_wdt_interrupt, 0, pdev->name, priv);
>>> +	if (err) {
>>> +		dev_err(dev, "cannot register interrupt handler %d\n", err);
>>> +		return err;
>>> +	}
>>
>> Use devm_request_irq() and request the interrupt after registering the watchdog.
> 
> Okay,
> 
>>
>>> +
>>> +	priv->irq = irq;
>>> +	watchdog_set_drvdata(wdog_dev, priv);
>>> +	platform_set_drvdata(pdev, priv);
>>> +	watchdog_init_timeout(wdog_dev, wdog_dev->timeout, dev);
>>
>> watchdog_init_timeout sets wdog_dev->timeout, so this is pointless.
>> Calling watchdog_init_timeout() only makes sense if the parameter is either a
>> timeout module parameter or 0 and the idea is to use a value from devicetree if
>> configured.
> 
> Okay, thanks again for detail
> 
> Regards
> -Bharat
> 
>>
>>> +	octeontx2_wdt_settimeout(wdog_dev, wdog_dev->timeout);
>>> +	watchdog_stop_on_reboot(wdog_dev);
>>> +	watchdog_stop_on_unregister(wdog_dev);
>>> +
>>> +	err = devm_watchdog_register_device(dev, wdog_dev);
>>> +	if (err) {
>>> +		free_irq(irq, priv);
>>> +		return err;
>>> +	}
>>> +
>>> +	dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev-
>>> timeout);
>>> +	return 0;
>>> +}
>>> +
>>> +static int octeontx2_wdt_remove(struct platform_device *pdev) {
>>> +	struct octeontx2_wdt_priv *priv = platform_get_drvdata(pdev);
>>> +
>>> +	if (priv->irq)
>>> +		free_irq(priv->irq, priv);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id octeontx2_wdt_of_match[] = {
>>> +	{ .compatible = "marvell-octeontx2-wdt", },
>>> +	{ },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, octeontx2_wdt_of_match);
>>> +
>>> +static struct platform_driver octeontx2_wdt_driver = {
>>> +	.driver = {
>>> +		.name = "octeontx2-wdt",
>>> +		.of_match_table = octeontx2_wdt_of_match,
>>> +	},
>>> +	.probe = octeontx2_wdt_probe,
>>> +	.remove = octeontx2_wdt_remove,
>>> +};
>>> +module_platform_driver(octeontx2_wdt_driver);
>>> +
>>> +MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>");
>>> +MODULE_DESCRIPTION("OcteonTX2 watchdog driver");
>>> --
>>> 2.17.1
>>>

Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver

[kernel test robot]

Hi Bharat,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on groeck-staging/hwmon-next linus/master v6.3-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bharat-Bhushan/Watchdog-Add-octeontx2-GTI-watchdog-driver/20230414-182508
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230414102342.23696-2-bbhushan2%40marvell.com
patch subject: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver
config: riscv-allmodconfig (https://download.01.org/0day-ci/archive/20230415/202304151147.xkLFltfQ-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/62fe2733152f0c68369948612f66ca68d43cb6f3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Bharat-Bhushan/Watchdog-Add-octeontx2-GTI-watchdog-driver/20230414-182508
        git checkout 62fe2733152f0c68369948612f66ca68d43cb6f3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/watchdog/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304151147.xkLFltfQ-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/watchdog/octeontx2_wdt.c:19:10: fatal error: asm/arch_timer.h: No such file or directory
      19 | #include <asm/arch_timer.h>
         |          ^~~~~~~~~~~~~~~~~~
   compilation terminated.


vim +19 drivers/watchdog/octeontx2_wdt.c

    18	
  > 19	#include <asm/arch_timer.h>
    20	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver

[Rob Herring]

On Fri, Apr 14, 2023 at 03:53:42PM +0530, Bharat Bhushan wrote:
> GTI watchdog timer are programmed in "interrupt + del3t + reset mode"
> and del3t traps are not enabled.
> GTI watchdog exception flow is:
>  - 1st timer expiration generates watchdog interrupt.
>  - 2nd timer expiration is ignored
>  - On 3rd timer expiration will trigger a system-wide core reset.
> 
> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> ---
>  drivers/watchdog/Kconfig         |   9 ++
>  drivers/watchdog/Makefile        |   1 +
>  drivers/watchdog/octeontx2_wdt.c | 238 +++++++++++++++++++++++++++++++
>  3 files changed, 248 insertions(+)
>  create mode 100644 drivers/watchdog/octeontx2_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index f0872970daf9..31ff282c62ad 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -2212,4 +2212,13 @@ config KEEMBAY_WATCHDOG
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called keembay_wdt.
>  
> +config OCTEONTX2_WATCHDOG
> +	tristate "OCTEONTX2 Watchdog driver"
> +	depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT)
> +	help
> +	 OCTEONTX2 GTI hardware supports watchdog timer. This watchdog timer are
> +	 programmed in "interrupt + del3t + reset" mode. On first expiry it will
> +	 generate interrupt. Second expiry (del3t) is ignored and system will reset
> +	 on final timer expiry.
> +
>  endif # WATCHDOG
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 9cbf6580f16c..aa1b813ad1f9 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -230,3 +230,4 @@ obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o
>  obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
>  obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
>  obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o
> +obj-$(CONFIG_OCTEONTX2_WATCHDOG) += octeontx2_wdt.o
> diff --git a/drivers/watchdog/octeontx2_wdt.c b/drivers/watchdog/octeontx2_wdt.c
> new file mode 100644
> index 000000000000..7b78a092e83f
> --- /dev/null
> +++ b/drivers/watchdog/octeontx2_wdt.c
> @@ -0,0 +1,238 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Marvell Octeontx2 Watchdog driver
> + *
> + * Copyright (C) 2023 Marvell International Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/cpu.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_platform.h>

It's doubtful you need anything from of_platform.h other than implicit 
includes. Use the header(s) you need directly.

Rob

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

[Krzysztof Kozlowski]

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

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

[Bharat Bhushan]

> -----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

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

[Krzysztof Kozlowski]

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

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

[Bharat Bhushan]

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

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

[Guenter Roeck]

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