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

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

[Bharat Bhushan]

Add binding documentation for the Marvell GTI system
watchdog driver.

Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
---
v5:
 - Added wdt-timer-index property
 - Get clock frequency from clocks/clock-name device tree property

 .../bindings/watchdog/marvell,gti-wdt.yaml    | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml

diff --git a/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml b/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
new file mode 100644
index 000000000000..e3315653f961
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/marvell,gti-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell Global Timer (GTI) system watchdog
+
+allOf:
+  - $ref: watchdog.yaml#
+
+maintainers:
+  - Bharat Bhushan <bbhushan2@marvell.com>
+
+properties:
+  compatible:
+    const: marvell,gti-wdt
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  wdt-timer-index:
+    maxItems: 1
+    description:
+      This contains the timer number out of total 64 timers supported
+      by GTI hardware block.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - wdt-timer-index
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        watchdog@802000040000 {
+            compatible = "marvell,gti-wdt";
+            reg = <0x00008020 0x00040000 0x00000000 0x00020000>;
+            interrupts = <0 38 IRQ_TYPE_EDGE_RISING>;
+            wdt-timer-index = <63>;
+        };
+    };
+
+...
-- 
2.17.1

[PATCH 2/2 v5] Watchdog: Add marvell GTI watchdog driver

[Bharat Bhushan]

This patch add support for Marvell GTI watchdog.  Global timer
unit (GTI) support hardware watchdog timer. Software programs
watchdog timer to generate interrupt on first timeout, second
timeout is configured to be ignored and system reboots on
third timeout.

Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
---
v5:
 - Get clock frequency from clocks/clock-name device tree property
   So removed arch_timer_get_cntfrq() and asm include arch_timer.h
 - Removed GTI_CWD_GLOBAL_WDOG_IDX, use wdt-timer-index device tree
   property for timer
 - Some other clean-up as per review comment

 drivers/watchdog/Kconfig           |  11 +
 drivers/watchdog/Makefile          |   1 +
 drivers/watchdog/marvell_gti_wdt.c | 326 +++++++++++++++++++++++++++++
 3 files changed, 338 insertions(+)
 create mode 100644 drivers/watchdog/marvell_gti_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index f0872970daf9..a96e7d7c0ad2 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1779,6 +1779,17 @@ config OCTEON_WDT
 	  from the first interrupt, it is then only poked when the
 	  device is written.
 
+config MARVELL_GTI_WDT
+	tristate "Marvell GTI Watchdog driver"
+	depends on ARCH_THUNDER || COMPILE_TEST
+	help
+	 Marvell GTI hardware supports watchdog timer. First timeout
+	 works as watchdog pretimeout and installed interrupt handler
+	 will be called on first timeout. Hardware can generate interrupt
+	 to SCP on second timeout but it is not enabled, So second
+	 timeout is ignored. If device poke does not happen then system
+	 will reboot on third timeout.
+
 config BCM2835_WDT
 	tristate "Broadcom BCM2835 hardware watchdog"
 	depends on ARCH_BCM2835 || (OF && COMPILE_TEST)
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 9cbf6580f16c..bd425408fcaa 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -98,6 +98,7 @@ obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o
 obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
 obj-$(CONFIG_APPLE_WATCHDOG) += apple_wdt.o
 obj-$(CONFIG_SUNPLUS_WATCHDOG) += sunplus_wdt.o
+obj-$(CONFIG_MARVELL_GTI_WDT) += marvell_gti_wdt.o
 
 # X86 (i386 + ia64 + x86_64) Architecture
 obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
diff --git a/drivers/watchdog/marvell_gti_wdt.c b/drivers/watchdog/marvell_gti_wdt.c
new file mode 100644
index 000000000000..d0a2e6374dfb
--- /dev/null
+++ b/drivers/watchdog/marvell_gti_wdt.c
@@ -0,0 +1,326 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Marvell GTI Watchdog driver
+ *
+ * Copyright (C) 2023 Marvell.
+ */
+
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+#include <linux/clk.h>
+
+/*
+ * 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 on 1st timeout happens
+ *    This will generate interrupt to SCP processor on 2nd timeout happens
+ *
+ * 3) Interrupt + Interrupt to SCP processor (called delt3t) + reboot.
+ *    This will generate interrupt to arm core on 1st timeout happens
+ *    Will generate interrupt to SCP processor on 2nd timeout happens,
+ *    if interrupt is configured.
+ *    Reboot on 3rd timeout.
+ *
+ * Driver will use hardware in 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.
+ *
+ * First timeout is effectively watchdog pretimeout.
+ */
+
+/* GTI CWD Watchdog (GTI_CWD_WDOG) Register */
+#define GTI_CWD_WDOG(reg_offset)	(0x8 * reg_offset)
+#define GTI_CWD_WDOG_MODE_INT_DEL3T_RST	0x3
+#define GTI_CWD_WDOG_MODE_MASK		GENMASK_ULL(1, 0)
+#define GTI_CWD_WDOG_LEN_SHIFT		4
+#define GTI_CWD_WDOG_LEN_MASK		GENMASK_ULL(19, 4)
+#define GTI_CWD_WDOG_CNT_SHIFT		20
+#define GTI_CWD_WDOG_CNT_MASK		GENMASK_ULL(43, 20)
+
+/* GTI CWD Watchdog Interrupt (GTI_CWD_INT) Register */
+#define GTI_CWD_INT			0x200
+#define GTI_CWD_INT_PENDING_STATUS(bit)	(1 << bit)
+
+/* GTI CWD Watchdog Interrupt Enable Clear (GTI_CWD_INT_ENA_CLR) Register */
+#define GTI_CWD_INT_ENA_CLR		0x210
+#define GTI_CWD_INT_ENA_CLR_VAL(bit)	(1 << bit)
+
+/* GTI CWD Watchdog Interrupt Enable Set (GTI_CWD_INT_ENA_SET) Register */
+#define GTI_CWD_INT_ENA_SET		0x218
+#define GTI_CWD_INT_ENA_SET_VAL(bit)	(1 << bit)
+
+/* GTI CWD Watchdog Poke (GTI_CWD_POKE) Registers */
+#define GTI_CWD_POKE(reg_offset)	(0x10000 + 0x8 * reg_offset)
+#define GTI_CWD_POKE_VAL		1
+
+struct gti_wdt_priv {
+	struct watchdog_device wdev;
+	void __iomem *base;
+	u32 clock_freq;
+	struct clk *sclk;
+	/*
+	 * GTI hardware block supports total 64 timers, wdt_timer_idx
+	 * used for timer to be used for system watchdog.
+	 */
+	u32 wdt_timer_idx;
+};
+
+static irqreturn_t gti_wdt_interrupt(int irq, void *data)
+{
+	struct watchdog_device *wdev = data;
+	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+	/* Clear Interrupt Pending Status */
+	writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
+	       priv->base + GTI_CWD_INT);
+
+	watchdog_notify_pretimeout(wdev);
+
+	return IRQ_HANDLED;
+}
+
+static int gti_wdt_ping(struct watchdog_device *wdev)
+{
+	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+	writeq(GTI_CWD_POKE_VAL,
+	       priv->base + GTI_CWD_POKE(priv->wdt_timer_idx));
+
+	return 0;
+}
+
+static int gti_wdt_start(struct watchdog_device *wdev)
+{
+	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	u64 regval;
+
+	if (!wdev->pretimeout)
+		return -EINVAL;
+
+	set_bit(WDOG_HW_RUNNING, &wdev->status);
+
+	/* Clear any pending interrupt */
+	writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
+	       priv->base + GTI_CWD_INT);
+
+	/* Enable Interrupt */
+	writeq(GTI_CWD_INT_ENA_SET_VAL(priv->wdt_timer_idx),
+	       priv->base + GTI_CWD_INT_ENA_SET);
+
+	/* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */
+	regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
+	regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST;
+	writeq(regval, priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
+
+	return 0;
+}
+
+static int gti_wdt_stop(struct watchdog_device *wdev)
+{
+	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	u64 regval;
+
+	/* Disable Interrupt */
+	writeq(GTI_CWD_INT_ENA_CLR_VAL(priv->wdt_timer_idx),
+	       priv->base + GTI_CWD_INT_ENA_CLR);
+
+	/* Set GTI_CWD_WDOG.Mode = 0 to stop the timer */
+	regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
+	regval &= ~GTI_CWD_WDOG_MODE_MASK;
+	writeq(regval, priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
+
+	return 0;
+}
+
+static int gti_wdt_settimeout(struct watchdog_device *wdev,
+					unsigned int timeout)
+{
+	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	u64 timeout_wdog, regval;
+
+	/* Update new timeout */
+	wdev->timeout = timeout;
+
+	/* Pretimeout is 1/3 of timeout */
+	wdev->pretimeout = timeout / 3;
+
+	/* Get clock cycles from pretimeout */
+	timeout_wdog = (u64)priv->clock_freq * wdev->pretimeout;
+
+	/* Watchdog counts in 1024 cycle steps */
+	timeout_wdog = timeout_wdog >> 10;
+
+	/* GTI_CWD_WDOG.CNT: reload counter is 16-bit */
+	timeout_wdog = (timeout_wdog + 0xff) >> 8;
+	if (timeout_wdog >= 0x10000)
+		timeout_wdog = 0xffff;
+
+	/*
+	 * GTI_CWD_WDOG.LEN is 24bit, lower 8-bits should be zero and
+	 * upper 16-bits are same as GTI_CWD_WDOG.CNT
+	 */
+	regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
+	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(priv->wdt_timer_idx));
+
+	return 0;
+}
+
+static int gti_wdt_set_pretimeout(struct watchdog_device *wdev,
+					unsigned int timeout)
+{
+	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	struct watchdog_device *wdog_dev = &priv->wdev;
+
+	/* pretimeout should 1/3 of max_timeout */
+	if (timeout * 3 <= wdog_dev->max_timeout)
+		return gti_wdt_settimeout(wdev, timeout * 3);
+
+	return -EINVAL;
+}
+
+static void gti_clk_disable_unprepare(void *data)
+{
+	clk_disable_unprepare(data);
+}
+
+static int gti_wdt_get_cntfrq(struct platform_device *pdev,
+			      struct gti_wdt_priv *priv)
+{
+	int err;
+
+	priv->sclk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->sclk))
+		return PTR_ERR(priv->sclk);
+
+	err = clk_prepare_enable(priv->sclk);
+	if (err)
+		return err;
+
+	err = devm_add_action_or_reset(&pdev->dev,
+				       gti_clk_disable_unprepare, priv->sclk);
+	if (err)
+		return err;
+
+	priv->clock_freq = clk_get_rate(priv->sclk);
+	if (!priv->clock_freq)
+		return -EINVAL;
+
+	return 0;
+}
+
+static const struct watchdog_info gti_wdt_ident = {
+	.identity = "Marvell GTI watchdog",
+	.options = WDIOF_SETTIMEOUT | WDIOF_PRETIMEOUT | WDIOF_KEEPALIVEPING |
+		   WDIOF_MAGICCLOSE | WDIOF_CARDRESET,
+};
+
+static const struct watchdog_ops gti_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = gti_wdt_start,
+	.stop = gti_wdt_stop,
+	.ping = gti_wdt_ping,
+	.set_timeout = gti_wdt_settimeout,
+	.set_pretimeout = gti_wdt_set_pretimeout,
+};
+
+static int gti_wdt_probe(struct platform_device *pdev)
+{
+	struct gti_wdt_priv *priv;
+	struct device *dev = &pdev->dev;
+	struct watchdog_device *wdog_dev;
+	u64 max_pretimeout;
+	u32 wdt_idx;
+	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");
+
+	err = gti_wdt_get_cntfrq(pdev, priv);
+	if (err)
+		return dev_err_probe(&pdev->dev, err,
+				     "GTI clock frequency not valid/found");
+
+	/*
+	 * GTI hardware block supports total 64 timers, "wdt-timer-index"
+	 * property used for timer to be used for system watchdog.
+	 */
+	err = of_property_read_u32(dev->of_node, "wdt-timer-index", &wdt_idx);
+	if (err || wdt_idx > 63)
+		return dev_err_probe(&pdev->dev, err,
+				     "GTI wdog timer index not valid/found");
+
+	priv->wdt_timer_idx = wdt_idx;
+	wdog_dev = &priv->wdev;
+	wdog_dev->info = &gti_wdt_ident,
+	wdog_dev->ops = &gti_wdt_ops,
+	wdog_dev->parent = dev;
+	/*
+	 * Watchdog counter is 24 bit where lower 8 bits are zeros
+	 * This counter decrements every 1024 clock cycles.
+	 */
+	max_pretimeout = (GTI_CWD_WDOG_CNT_MASK >> GTI_CWD_WDOG_CNT_SHIFT);
+	max_pretimeout &= ~0xFFUL;
+	max_pretimeout = (max_pretimeout * 1024) / priv->clock_freq;
+	wdog_dev->pretimeout = max_pretimeout;
+
+	/* Maximum timeout is 3 times the pretimeout */
+	wdog_dev->max_timeout = max_pretimeout * 3;
+	/* Minimum first timeout (pretimeout) is 1, so min_timeout as 3 */
+	wdog_dev->min_timeout = 3;
+	wdog_dev->timeout = wdog_dev->pretimeout;
+
+	watchdog_set_drvdata(wdog_dev, priv);
+	platform_set_drvdata(pdev, priv);
+	gti_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)
+		return err;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return dev_err_probe(&pdev->dev, irq, "IRQ resource not found\n");
+
+	err = devm_request_irq(dev, irq, gti_wdt_interrupt, 0,
+			       pdev->name, &priv->wdev);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to register interrupt handler\n");
+
+	dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev->timeout);
+	return 0;
+}
+
+static const struct of_device_id gti_wdt_of_match[] = {
+	{ .compatible = "marvell,gti-wdt", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, gti_wdt_of_match);
+
+static struct platform_driver gti_wdt_driver = {
+	.driver = {
+		.name = "gti-wdt",
+		.of_match_table = gti_wdt_of_match,
+	},
+	.probe = gti_wdt_probe,
+};
+module_platform_driver(gti_wdt_driver);
+
+MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>");
+MODULE_DESCRIPTION("Marvell GTI watchdog driver");
-- 
2.17.1

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

[Krzysztof Kozlowski]

On 03/05/2023 14:10, Bharat Bhushan wrote:
> Add binding documentation for the Marvell GTI system
> watchdog driver.
> 
> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> ---
> v5:
>  - Added wdt-timer-index property

I did not ask for it...

>  - Get clock frequency from clocks/clock-name device tree property

Where? It's not possible in current code. I don't think you tested this
at all.

> 
>  .../bindings/watchdog/marvell,gti-wdt.yaml    | 54 +++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml b/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
> new file mode 100644
> index 000000000000..e3315653f961
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/marvell,gti-wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell Global Timer (GTI) system watchdog
> +
> +allOf:
> +  - $ref: watchdog.yaml#
> +
> +maintainers:
> +  - Bharat Bhushan <bbhushan2@marvell.com>
> +
> +properties:
> +  compatible:
> +    const: marvell,gti-wdt
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  wdt-timer-index:

missing vendor prefix

missing type

> +    maxItems: 1

???

> +    description:
> +      This contains the timer number out of total 64 timers supported
> +      by GTI hardware block.

Why do you need it? What does it represent?

We do not keep indices of devices other than something in reg, so please
justify why exception must be made here.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - wdt-timer-index
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        watchdog@802000040000 {
> +            compatible = "marvell,gti-wdt";
> +            reg = <0x00008020 0x00040000 0x00000000 0x00020000>;
> +            interrupts = <0 38 IRQ_TYPE_EDGE_RISING>;

Use defines for flags.

> +            wdt-timer-index = <63>;


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 03/05/2023 14:10, Bharat Bhushan wrote:
> This patch add support for Marvell GTI watchdog.  Global timer
> unit (GTI) support hardware watchdog timer. Software programs
> watchdog timer to generate interrupt on first timeout, second
> timeout is configured to be ignored and system reboots on
> third timeout.
> 
> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>



> +
> +static void gti_clk_disable_unprepare(void *data)
> +{
> +	clk_disable_unprepare(data);
> +}
> +
> +static int gti_wdt_get_cntfrq(struct platform_device *pdev,
> +			      struct gti_wdt_priv *priv)
> +{
> +	int err;
> +
> +	priv->sclk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->sclk))
> +		return PTR_ERR(priv->sclk);

NAK. Undocumented property. Test your DTS. There is no way it was
working. Either you did not test DTS against bindings or you did not
test driver and DTS together.

Better actually to upstream your DTS...

Best regards,
Krzysztof

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

[Bharat Bhushan]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Thursday, May 4, 2023 12:25 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
> Subject: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system
> watchdog driver
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 03/05/2023 14:10, Bharat Bhushan wrote:
> > Add binding documentation for the Marvell GTI system watchdog driver.
> >
> > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> > ---
> > v5:
> >  - Added wdt-timer-index property
> 
> I did not ask for it...
> 
> >  - Get clock frequency from clocks/clock-name device tree property
> 
> Where? It's not possible in current code. I don't think you tested this at all.

My bad, Missed clock related binding changes while doing last minute rework.
Will send updated patch adding the dt-binding properties.

It is testing exactly with below node:
                watchdog@802000040000 {
                        compatible = "marvell,gti-wdt";
                        reg = <0x8020 0x40000 0x0 0x20000>;
                        interrupts = <0 62 1>;
                        wdt-timer-index = <63>;
                        clocks = <&sclk>;
                        clock-names = "ref_clk";

> 
> >
> >  .../bindings/watchdog/marvell,gti-wdt.yaml    | 54 +++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
> > b/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
> > new file mode 100644
> > index 000000000000..e3315653f961
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
> > @@ -0,0 +1,54 @@
> > +# 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-2Cgti-2Dwdt.yaml-
> 23&d=DwICaQ&c=nKjWec2b6R0mOyP
> >
> +az7xtfQ&r=PAAlWswPe7d8gHlGbCLmy2YezyK7O3Hv_t2heGnouBw&m=ts41IUd
> c422tK
> > +gF0cnYI7jTqJ1dvTm-FNq1pILCyrOuwqKu2UVnwWEVy-
> aZAMsme&s=fVo903PvFGVqR_P
> > +G6r91BBtzOTLz4Mixh1Tqu1GAp6E&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=ts41IUdc422tKgF0cnYI7jTqJ1
> dvTm
> > +-FNq1pILCyrOuwqKu2UVnwWEVy-
> aZAMsme&s=ebh6bxE3wbSmwrOnHmUHMM_L77f6bY6W
> > +Ifye_sXDNzI&e=
> > +
> > +title: Marvell Global Timer (GTI) system watchdog
> > +
> > +allOf:
> > +  - $ref: watchdog.yaml#
> > +
> > +maintainers:
> > +  - Bharat Bhushan <bbhushan2@marvell.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: marvell,gti-wdt
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  wdt-timer-index:
> 
> missing vendor prefix

ack

> 
> missing type

Will add

> 
> > +    maxItems: 1
> 
> ???

Removed

> 
> > +    description:
> > +      This contains the timer number out of total 64 timers supported
> > +      by GTI hardware block.
> 
> Why do you need it? What does it represent?
> 
> We do not keep indices of devices other than something in reg, so please justify
> why exception must be made here.

Different platform have different number of GTI timers, for example some platform have total 64 timer and other have 32 timers.
So which GTI timer will be used for watchdog will depend on platform. So added this property to enable this driver on platforms.

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - wdt-timer-index
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        watchdog@802000040000 {
> > +            compatible = "marvell,gti-wdt";
> > +            reg = <0x00008020 0x00040000 0x00000000 0x00020000>;
> > +            interrupts = <0 38 IRQ_TYPE_EDGE_RISING>;
> 
> Use defines for flags.

Okay.

Thanks
-Bharat

> 
> > +            wdt-timer-index = <63>;
> 
> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 04/05/2023 11:02, Bharat Bhushan wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Thursday, May 4, 2023 12:25 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
>> Subject: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system
>> watchdog driver
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> On 03/05/2023 14:10, Bharat Bhushan wrote:
>>> Add binding documentation for the Marvell GTI system watchdog driver.
>>>
>>> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
>>> ---
>>> v5:
>>>  - Added wdt-timer-index property
>>
>> I did not ask for it...
>>
>>>  - Get clock frequency from clocks/clock-name device tree property
>>
>> Where? It's not possible in current code. I don't think you tested this at all.
> 
> My bad, Missed clock related binding changes while doing last minute rework.
> Will send updated patch adding the dt-binding properties.
> 
> It is testing exactly with below node:
>                 watchdog@802000040000 {
>                         compatible = "marvell,gti-wdt";
>                         reg = <0x8020 0x40000 0x0 0x20000>;
>                         interrupts = <0 62 1>;
>                         wdt-timer-index = <63>;
>                         clocks = <&sclk>;
>                         clock-names = "ref_clk";
> 
>>
>>>
>>>  .../bindings/watchdog/marvell,gti-wdt.yaml    | 54 +++++++++++++++++++
>>>  1 file changed, 54 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
>>> b/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
>>> new file mode 100644
>>> index 000000000000..e3315653f961
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
>>> @@ -0,0 +1,54 @@
>>> +# 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-2Cgti-2Dwdt.yaml-
>> 23&d=DwICaQ&c=nKjWec2b6R0mOyP
>>>
>> +az7xtfQ&r=PAAlWswPe7d8gHlGbCLmy2YezyK7O3Hv_t2heGnouBw&m=ts41IUd
>> c422tK
>>> +gF0cnYI7jTqJ1dvTm-FNq1pILCyrOuwqKu2UVnwWEVy-
>> aZAMsme&s=fVo903PvFGVqR_P
>>> +G6r91BBtzOTLz4Mixh1Tqu1GAp6E&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=ts41IUdc422tKgF0cnYI7jTqJ1
>> dvTm
>>> +-FNq1pILCyrOuwqKu2UVnwWEVy-
>> aZAMsme&s=ebh6bxE3wbSmwrOnHmUHMM_L77f6bY6W
>>> +Ifye_sXDNzI&e=
>>> +
>>> +title: Marvell Global Timer (GTI) system watchdog
>>> +
>>> +allOf:
>>> +  - $ref: watchdog.yaml#
>>> +
>>> +maintainers:
>>> +  - Bharat Bhushan <bbhushan2@marvell.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: marvell,gti-wdt

So I guess we all thought "gti" means some soc. Now it is clear - you
miss specific compatibles. Generic blocks alone or wildcards are not
allowed.

And we have it clearly documented:

https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42

>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  wdt-timer-index:
>>
>> missing vendor prefix
> 
> ack
> 
>>
>> missing type
> 
> Will add
> 
>>
>>> +    maxItems: 1
>>
>> ???
> 
> Removed
> 
>>
>>> +    description:
>>> +      This contains the timer number out of total 64 timers supported
>>> +      by GTI hardware block.
>>
>> Why do you need it? What does it represent?
>>
>> We do not keep indices of devices other than something in reg, so please justify
>> why exception must be made here.
> 
> Different platform have different number of GTI timers, for example some platform have total 64 timer and other have 32 timers.
> So which GTI timer will be used for watchdog will depend on platform. So added this property to enable this driver on platforms.

This should be deducted from compatible.

Best regards,
Krzysztof

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

[Bharat Bhushan]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Thursday, May 4, 2023 4:38 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
> Subject: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system
> watchdog driver
> 
> On 04/05/2023 11:02, Bharat Bhushan wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: Thursday, May 4, 2023 12:25 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
> >> Subject: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI
> >> system watchdog driver
> >>
> >> External Email
> >>
> >> ---------------------------------------------------------------------
> >> - On 03/05/2023 14:10, Bharat Bhushan wrote:
> >>> Add binding documentation for the Marvell GTI system watchdog driver.
> >>>
> >>> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> >>> ---
> >>> v5:
> >>>  - Added wdt-timer-index property
> >>
> >> I did not ask for it...
> >>
> >>>  - Get clock frequency from clocks/clock-name device tree property
> >>
> >> Where? It's not possible in current code. I don't think you tested this at all.
> >
> > My bad, Missed clock related binding changes while doing last minute rework.
> > Will send updated patch adding the dt-binding properties.
> >
> > It is testing exactly with below node:
> >                 watchdog@802000040000 {
> >                         compatible = "marvell,gti-wdt";
> >                         reg = <0x8020 0x40000 0x0 0x20000>;
> >                         interrupts = <0 62 1>;
> >                         wdt-timer-index = <63>;
> >                         clocks = <&sclk>;
> >                         clock-names = "ref_clk";
> >
> >>
> >>>
> >>>  .../bindings/watchdog/marvell,gti-wdt.yaml    | 54 +++++++++++++++++++
> >>>  1 file changed, 54 insertions(+)
> >>>  create mode 100644
> >>> Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
> >>>
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
> >>> b/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yaml
> >>> new file mode 100644
> >>> index 000000000000..e3315653f961
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/watchdog/marvell,gti-wdt.yam
> >>> +++ l
> >>> @@ -0,0 +1,54 @@
> >>> +# 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-2Cgti-2Dwdt.yaml-
> >> 23&d=DwICaQ&c=nKjWec2b6R0mOyP
> >>>
> >>
> +az7xtfQ&r=PAAlWswPe7d8gHlGbCLmy2YezyK7O3Hv_t2heGnouBw&m=ts41IUd
> >> c422tK
> >>> +gF0cnYI7jTqJ1dvTm-FNq1pILCyrOuwqKu2UVnwWEVy-
> >> aZAMsme&s=fVo903PvFGVqR_P
> >>> +G6r91BBtzOTLz4Mixh1Tqu1GAp6E&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=ts41IUdc422tKgF0cnYI7jTqJ1
> >> dvTm
> >>> +-FNq1pILCyrOuwqKu2UVnwWEVy-
> >> aZAMsme&s=ebh6bxE3wbSmwrOnHmUHMM_L77f6bY6W
> >>> +Ifye_sXDNzI&e=
> >>> +
> >>> +title: Marvell Global Timer (GTI) system watchdog
> >>> +
> >>> +allOf:
> >>> +  - $ref: watchdog.yaml#
> >>> +
> >>> +maintainers:
> >>> +  - Bharat Bhushan <bbhushan2@marvell.com>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: marvell,gti-wdt
> 
> So I guess we all thought "gti" means some soc. Now it is clear - you miss specific
> compatibles. Generic blocks alone or wildcards are not allowed.
> 
> And we have it clearly documented:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__elixir.bootlin.com_linux_v6.1-
> 2Drc1_source_Documentation_devicetree_bindings_writing-2Dbindings.rst-
> 23L42&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PAAlWswPe7d8gHlGbCLmy
> 2YezyK7O3Hv_t2heGnouBw&m=uy18AXnbGKMIcyPkTnWrPZoVBj8yzyjlGeNemR
> MFgMVqkT6-4JVU5hWsVbcKPzTJ&s=XkqN7nbFOrtRnOJVqrgEDA9zinZxML4-
> qSYQPElzVeg&e=

So Say currently Marvell have GTI watchdog timer in following series of SoCs
 - octeon
 - octeontx2
 - cn10k

Compatible for octeon series is "marvell,octeon-wdt"
Compatible for octeontx2 series is "marvell,octeontx2-wdt"
Compatible for cn10k series is "marvell,cn10k-wdt"

So effectively we will have 3 compatibles, Is that correct?

> 
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  interrupts:
> >>> +    maxItems: 1
> >>> +
> >>> +  wdt-timer-index:
> >>
> >> missing vendor prefix
> >
> > ack
> >
> >>
> >> missing type
> >
> > Will add
> >
> >>
> >>> +    maxItems: 1
> >>
> >> ???
> >
> > Removed
> >
> >>
> >>> +    description:
> >>> +      This contains the timer number out of total 64 timers supported
> >>> +      by GTI hardware block.
> >>
> >> Why do you need it? What does it represent?
> >>
> >> We do not keep indices of devices other than something in reg, so
> >> please justify why exception must be made here.
> >
> > Different platform have different number of GTI timers, for example some
> platform have total 64 timer and other have 32 timers.
> > So which GTI timer will be used for watchdog will depend on platform. So
> added this property to enable this driver on platforms.
> 
> This should be deducted from compatible.

If I understood correctly, we should add different compatible for each soc and use same to get the information we tried to get using "wdt-timer-index" property, is that correct?

But each series have many socs (10s) and GTI hardware is same except number of timers they supports, so should we add that many compatibles or add a property like this?

Thanks
-Bharat

> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 04/05/2023 19:10, Bharat Bhushan wrote:>>>>> +maintainers:
>>>>> +  - Bharat Bhushan <bbhushan2@marvell.com>
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: marvell,gti-wdt
>>
>> So I guess we all thought "gti" means some soc. Now it is clear - you miss specific
>> compatibles. Generic blocks alone or wildcards are not allowed.
>>
>> And we have it clearly documented:
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-
>> 3A__elixir.bootlin.com_linux_v6.1-
>> 2Drc1_source_Documentation_devicetree_bindings_writing-2Dbindings.rst-
>> 23L42&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PAAlWswPe7d8gHlGbCLmy
>> 2YezyK7O3Hv_t2heGnouBw&m=uy18AXnbGKMIcyPkTnWrPZoVBj8yzyjlGeNemR
>> MFgMVqkT6-4JVU5hWsVbcKPzTJ&s=XkqN7nbFOrtRnOJVqrgEDA9zinZxML4-
>> qSYQPElzVeg&e=
> 
> So Say currently Marvell have GTI watchdog timer in following series of SoCs
>  - octeon
>  - octeontx2
>  - cn10k
> 
> Compatible for octeon series is "marvell,octeon-wdt"
> Compatible for octeontx2 series is "marvell,octeontx2-wdt"
> Compatible for cn10k series is "marvell,cn10k-wdt"
> 
> So effectively we will have 3 compatibles, Is that correct?

I don't know your SoCs. I assume you should know.

> 
>>
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  interrupts:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  wdt-timer-index:
>>>>
>>>> missing vendor prefix
>>>
>>> ack
>>>
>>>>
>>>> missing type
>>>
>>> Will add
>>>
>>>>
>>>>> +    maxItems: 1
>>>>
>>>> ???
>>>
>>> Removed
>>>
>>>>
>>>>> +    description:
>>>>> +      This contains the timer number out of total 64 timers supported
>>>>> +      by GTI hardware block.
>>>>
>>>> Why do you need it? What does it represent?
>>>>
>>>> We do not keep indices of devices other than something in reg, so
>>>> please justify why exception must be made here.
>>>
>>> Different platform have different number of GTI timers, for example some
>> platform have total 64 timer and other have 32 timers.
>>> So which GTI timer will be used for watchdog will depend on platform. So
>> added this property to enable this driver on platforms.
>>
>> This should be deducted from compatible.
> 
> If I understood correctly, we should add different compatible for each soc and use same to get the information we tried to get using "wdt-timer-index" property, is that correct?
> 
> But each series have many socs (10s) and GTI hardware is same except number of timers they supports, so should we add that many compatibles or add a property like this?

Same story every time... and was discussed many, many times on the lists.

https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42

You need anyway SoC specific compatibles. Once you add proper
compatibles, you will see that property is not needed.


Best regards,
Krzysztof

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

[Bharat Bhushan]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Friday, May 5, 2023 12:08 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
> Subject: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system
> watchdog driver
> 
> On 04/05/2023 19:10, Bharat Bhushan wrote:>>>>> +maintainers:
> >>>>> +  - Bharat Bhushan <bbhushan2@marvell.com>
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    const: marvell,gti-wdt
> >>
> >> So I guess we all thought "gti" means some soc. Now it is clear - you
> >> miss specific compatibles. Generic blocks alone or wildcards are not allowed.
> >>
> >> And we have it clearly documented:
> >>
> >> https://urldefense.proofpoint.com/v2/url?u=https-
> >> 3A__elixir.bootlin.com_linux_v6.1-
> >> 2Drc1_source_Documentation_devicetree_bindings_writing-2Dbindings.rst
> >> -
> 23L42&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PAAlWswPe7d8gHlGbCLmy
> >>
> 2YezyK7O3Hv_t2heGnouBw&m=uy18AXnbGKMIcyPkTnWrPZoVBj8yzyjlGeNemR
> >> MFgMVqkT6-4JVU5hWsVbcKPzTJ&s=XkqN7nbFOrtRnOJVqrgEDA9zinZxML4-
> >> qSYQPElzVeg&e=
> >
> > So Say currently Marvell have GTI watchdog timer in following series
> > of SoCs
> >  - octeon
> >  - octeontx2
> >  - cn10k
> >
> > Compatible for octeon series is "marvell,octeon-wdt"
> > Compatible for octeontx2 series is "marvell,octeontx2-wdt"
> > Compatible for cn10k series is "marvell,cn10k-wdt"
> >
> > So effectively we will have 3 compatibles, Is that correct?
> 
> I don't know your SoCs. I assume you should know.
> 
> >
> >>
> >>>>> +
> >>>>> +  reg:
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  interrupts:
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  wdt-timer-index:
> >>>>
> >>>> missing vendor prefix
> >>>
> >>> ack
> >>>
> >>>>
> >>>> missing type
> >>>
> >>> Will add
> >>>
> >>>>
> >>>>> +    maxItems: 1
> >>>>
> >>>> ???
> >>>
> >>> Removed
> >>>
> >>>>
> >>>>> +    description:
> >>>>> +      This contains the timer number out of total 64 timers supported
> >>>>> +      by GTI hardware block.
> >>>>
> >>>> Why do you need it? What does it represent?
> >>>>
> >>>> We do not keep indices of devices other than something in reg, so
> >>>> please justify why exception must be made here.
> >>>
> >>> Different platform have different number of GTI timers, for example
> >>> some
> >> platform have total 64 timer and other have 32 timers.
> >>> So which GTI timer will be used for watchdog will depend on
> >>> platform. So
> >> added this property to enable this driver on platforms.
> >>
> >> This should be deducted from compatible.
> >
> > If I understood correctly, we should add different compatible for each soc and
> use same to get the information we tried to get using "wdt-timer-index"
> property, is that correct?
> >
> > But each series have many socs (10s) and GTI hardware is same except number
> of timers they supports, so should we add that many compatibles or add a
> property like this?
> 
> Same story every time... and was discussed many, many times on the lists.
> 
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__elixir.bootlin.com_linux_v6.1-
> 2Drc1_source_Documentation_devicetree_bindings_writing-2Dbindings.rst-
> 23L42&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PAAlWswPe7d8gHlGbCLmy
> 2YezyK7O3Hv_t2heGnouBw&m=3aeejmHQ5C8TyLM5odlaq6KnLYHt4PhpJp70FQa
> qbNf7w15KFHA3fmeDR2V-en4m&s=FKeW5tpOG-
> CJoV_JKuqTa0k_tRrYWAQZG1UfqlW3c74&e=
> 
> You need anyway SoC specific compatibles. Once you add proper compatibles,
> you will see that property is not needed.

Looks odd to add N number of compatible for N socs belong to one class of soc, but anyways will do.

Thanks
-Bharat

> 
> 
> Best regards,
> Krzysztof

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

[Bharat Bhushan]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Friday, May 5, 2023 12:08 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
> Subject: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system
> watchdog driver
> 
> On 04/05/2023 19:10, Bharat Bhushan wrote:>>>>> +maintainers:
> >>>>> +  - Bharat Bhushan <bbhushan2@marvell.com>
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    const: marvell,gti-wdt
> >>
> >> So I guess we all thought "gti" means some soc. Now it is clear - you
> >> miss specific compatibles. Generic blocks alone or wildcards are not allowed.
> >>
> >> And we have it clearly documented:
> >>
> >> https://urldefense.proofpoint.com/v2/url?u=https-
> >> 3A__elixir.bootlin.com_linux_v6.1-
> >> 2Drc1_source_Documentation_devicetree_bindings_writing-2Dbindings.rst
> >> -
> 23L42&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PAAlWswPe7d8gHlGbCLmy
> >>
> 2YezyK7O3Hv_t2heGnouBw&m=uy18AXnbGKMIcyPkTnWrPZoVBj8yzyjlGeNemR
> >> MFgMVqkT6-4JVU5hWsVbcKPzTJ&s=XkqN7nbFOrtRnOJVqrgEDA9zinZxML4-
> >> qSYQPElzVeg&e=
> >
> > So Say currently Marvell have GTI watchdog timer in following series
> > of SoCs
> >  - octeon
> >  - octeontx2
> >  - cn10k
> >
> > Compatible for octeon series is "marvell,octeon-wdt"
> > Compatible for octeontx2 series is "marvell,octeontx2-wdt"
> > Compatible for cn10k series is "marvell,cn10k-wdt"
> >
> > So effectively we will have 3 compatibles, Is that correct?
> 
> I don't know your SoCs. I assume you should know.
> 
> >
> >>
> >>>>> +
> >>>>> +  reg:
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  interrupts:
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  wdt-timer-index:
> >>>>
> >>>> missing vendor prefix
> >>>
> >>> ack
> >>>
> >>>>
> >>>> missing type
> >>>
> >>> Will add
> >>>
> >>>>
> >>>>> +    maxItems: 1
> >>>>
> >>>> ???
> >>>
> >>> Removed
> >>>
> >>>>
> >>>>> +    description:
> >>>>> +      This contains the timer number out of total 64 timers supported
> >>>>> +      by GTI hardware block.
> >>>>
> >>>> Why do you need it? What does it represent?
> >>>>
> >>>> We do not keep indices of devices other than something in reg, so
> >>>> please justify why exception must be made here.
> >>>
> >>> Different platform have different number of GTI timers, for example
> >>> some
> >> platform have total 64 timer and other have 32 timers.
> >>> So which GTI timer will be used for watchdog will depend on
> >>> platform. So
> >> added this property to enable this driver on platforms.
> >>
> >> This should be deducted from compatible.
> >
> > If I understood correctly, we should add different compatible for each soc and
> use same to get the information we tried to get using "wdt-timer-index"
> property, is that correct?
> >
> > But each series have many socs (10s) and GTI hardware is same except number
> of timers they supports, so should we add that many compatibles or add a
> property like this?
> 
> Same story every time... and was discussed many, many times on the lists.
> 
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__elixir.bootlin.com_linux_v6.1-
> 2Drc1_source_Documentation_devicetree_bindings_writing-2Dbindings.rst-
> 23L42&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PAAlWswPe7d8gHlGbCLmy
> 2YezyK7O3Hv_t2heGnouBw&m=3aeejmHQ5C8TyLM5odlaq6KnLYHt4PhpJp70FQa
> qbNf7w15KFHA3fmeDR2V-en4m&s=FKeW5tpOG-
> CJoV_JKuqTa0k_tRrYWAQZG1UfqlW3c74&e=
> 
> You need anyway SoC specific compatibles. Once you add proper compatibles,
> you will see that property is not needed.

Also on a given soc, firmware can configure one of 64 timer to be used as system watchdog time then compatible will not work.

Thanks
-Bharat

> 
> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 05/05/2023 09:17, Bharat Bhushan wrote:
>> Same story every time... and was discussed many, many times on the lists.
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-
>> 3A__elixir.bootlin.com_linux_v6.1-
>> 2Drc1_source_Documentation_devicetree_bindings_writing-2Dbindings.rst-
>> 23L42&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PAAlWswPe7d8gHlGbCLmy
>> 2YezyK7O3Hv_t2heGnouBw&m=3aeejmHQ5C8TyLM5odlaq6KnLYHt4PhpJp70FQa
>> qbNf7w15KFHA3fmeDR2V-en4m&s=FKeW5tpOG-
>> CJoV_JKuqTa0k_tRrYWAQZG1UfqlW3c74&e=
>>
>> You need anyway SoC specific compatibles. Once you add proper compatibles,
>> you will see that property is not needed.
> 
> Looks odd to add N number of compatible for N socs belong to one class of soc, but anyways will do.

Why this is odd? How does it differ from other SoCs?

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 05/05/2023 09:55, Bharat Bhushan wrote:
>>>>> Different platform have different number of GTI timers, for example
>>>>> some
>>>> platform have total 64 timer and other have 32 timers.
>>>>> So which GTI timer will be used for watchdog will depend on
>>>>> platform. So
>>>> added this property to enable this driver on platforms.
>>>>
>>>> This should be deducted from compatible.
>>>
>>> If I understood correctly, we should add different compatible for each soc and
>> use same to get the information we tried to get using "wdt-timer-index"
>> property, is that correct?
>>>
>>> But each series have many socs (10s) and GTI hardware is same except number
>> of timers they supports, so should we add that many compatibles or add a
>> property like this?
>>
>> Same story every time... and was discussed many, many times on the lists.
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-
>> 3A__elixir.bootlin.com_linux_v6.1-
>> 2Drc1_source_Documentation_devicetree_bindings_writing-2Dbindings.rst-
>> 23L42&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PAAlWswPe7d8gHlGbCLmy
>> 2YezyK7O3Hv_t2heGnouBw&m=3aeejmHQ5C8TyLM5odlaq6KnLYHt4PhpJp70FQa
>> qbNf7w15KFHA3fmeDR2V-en4m&s=FKeW5tpOG-
>> CJoV_JKuqTa0k_tRrYWAQZG1UfqlW3c74&e=
>>
>> You need anyway SoC specific compatibles. Once you add proper compatibles,
>> you will see that property is not needed.
> 
> Also on a given soc, firmware can configure one of 64 timer to be used as system watchdog time then compatible will not work.

Can't you query the firmware for that? Or can't you just choose first
unused timer? DT is for non-discoverable properties.

Best regards,
Krzysztof

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

[Bharat Bhushan]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Friday, May 5, 2023 4:04 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
> Subject: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system
> watchdog driver
> 
> On 05/05/2023 09:55, Bharat Bhushan wrote:
> >>>>> Different platform have different number of GTI timers, for
> >>>>> example some
> >>>> platform have total 64 timer and other have 32 timers.
> >>>>> So which GTI timer will be used for watchdog will depend on
> >>>>> platform. So
> >>>> added this property to enable this driver on platforms.
> >>>>
> >>>> This should be deducted from compatible.
> >>>
> >>> If I understood correctly, we should add different compatible for
> >>> each soc and
> >> use same to get the information we tried to get using "wdt-timer-index"
> >> property, is that correct?
> >>>
> >>> But each series have many socs (10s) and GTI hardware is same except
> >>> number
> >> of timers they supports, so should we add that many compatibles or
> >> add a property like this?
> >>
> >> Same story every time... and was discussed many, many times on the lists.
> >>
> >> https://urldefense.proofpoint.com/v2/url?u=https-
> >> 3A__elixir.bootlin.com_linux_v6.1-
> >> 2Drc1_source_Documentation_devicetree_bindings_writing-2Dbindings.rst
> >> -
> 23L42&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=PAAlWswPe7d8gHlGbCLmy
> >>
> 2YezyK7O3Hv_t2heGnouBw&m=3aeejmHQ5C8TyLM5odlaq6KnLYHt4PhpJp70FQa
> >> qbNf7w15KFHA3fmeDR2V-en4m&s=FKeW5tpOG-
> >> CJoV_JKuqTa0k_tRrYWAQZG1UfqlW3c74&e=
> >>
> >> You need anyway SoC specific compatibles. Once you add proper
> >> compatibles, you will see that property is not needed.
> >
> > Also on a given soc, firmware can configure one of 64 timer to be used as
> system watchdog time then compatible will not work.
> 
> Can't you query the firmware for that? Or can't you just choose first unused
> timer? DT is for non-discoverable properties.

Query to firmware required arm SMC call, to me that does not look correct approach. Thought of using first one but that is already used and moving that is as same as this.

Hardcoding to 63 will make it work on some SoCs but not all.

Thanks
-Bharat

> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 05/05/2023 12:41, Bharat Bhushan wrote:
>>>>
>>>> You need anyway SoC specific compatibles. Once you add proper
>>>> compatibles, you will see that property is not needed.
>>>
>>> Also on a given soc, firmware can configure one of 64 timer to be used as
>> system watchdog time then compatible will not work.
>>
>> Can't you query the firmware for that? Or can't you just choose first unused
>> timer? DT is for non-discoverable properties.
> 
> Query to firmware required arm SMC call, to me that does not look correct approach. Thought of using first one but that is already used and moving that is as same as this.
> 
> Hardcoding to 63 will make it work on some SoCs but not all.

But you know which one is started or is not. GTI_CWD_WDOG tells you this.

Best regards,
Krzysztof

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

[Bharat Bhushan]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Friday, May 5, 2023 4:27 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
> Subject: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system
> watchdog driver
> 
> On 05/05/2023 12:41, Bharat Bhushan wrote:
> >>>>
> >>>> You need anyway SoC specific compatibles. Once you add proper
> >>>> compatibles, you will see that property is not needed.
> >>>
> >>> Also on a given soc, firmware can configure one of 64 timer to be
> >>> used as
> >> system watchdog time then compatible will not work.
> >>
> >> Can't you query the firmware for that? Or can't you just choose first
> >> unused timer? DT is for non-discoverable properties.
> >
> > Query to firmware required arm SMC call, to me that does not look correct
> approach. Thought of using first one but that is already used and moving that is as
> same as this.
> >
> > Hardcoding to 63 will make it work on some SoCs but not all.
> 
> But you know which one is started or is not. GTI_CWD_WDOG tells you this.

On a given SoC, Firmware can reserve and/or use one or more timer for some other use case (customer use) and configure one of the timer as watchdog timer. Linux have to use the configured timer only and cannot decide by its own.

Thanks
-Bharat

> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 05/05/2023 13:15, Bharat Bhushan wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Friday, May 5, 2023 4:27 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
>> Subject: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system
>> watchdog driver
>>
>> On 05/05/2023 12:41, Bharat Bhushan wrote:
>>>>>>
>>>>>> You need anyway SoC specific compatibles. Once you add proper
>>>>>> compatibles, you will see that property is not needed.
>>>>>
>>>>> Also on a given soc, firmware can configure one of 64 timer to be
>>>>> used as
>>>> system watchdog time then compatible will not work.
>>>>
>>>> Can't you query the firmware for that? Or can't you just choose first
>>>> unused timer? DT is for non-discoverable properties.
>>>
>>> Query to firmware required arm SMC call, to me that does not look correct
>> approach. Thought of using first one but that is already used and moving that is as
>> same as this.
>>>
>>> Hardcoding to 63 will make it work on some SoCs but not all.
>>
>> But you know which one is started or is not. GTI_CWD_WDOG tells you this.
> 
> On a given SoC, Firmware can reserve and/or use one or more timer for some other use case (customer use) and configure one of the timer as watchdog timer. Linux have to use the configured timer only and cannot decide by its own.

Then the SoCs which have such firmware could use proposed property.
Provide some rationale property description in your next version (adding
necessary compatibles).

Best regards,
Krzysztof

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

[Bharat Bhushan]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Friday, May 5, 2023 5:27 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
> Subject: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell GTI system
> watchdog driver
> 
> On 05/05/2023 13:15, Bharat Bhushan wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: Friday, May 5, 2023 4:27 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
> >> Subject: Re: [EXT] Re: [PATCH 1/2 v5] dt-bindings: watchdog: marvell
> >> GTI system watchdog driver
> >>
> >> On 05/05/2023 12:41, Bharat Bhushan wrote:
> >>>>>>
> >>>>>> You need anyway SoC specific compatibles. Once you add proper
> >>>>>> compatibles, you will see that property is not needed.
> >>>>>
> >>>>> Also on a given soc, firmware can configure one of 64 timer to be
> >>>>> used as
> >>>> system watchdog time then compatible will not work.
> >>>>
> >>>> Can't you query the firmware for that? Or can't you just choose
> >>>> first unused timer? DT is for non-discoverable properties.
> >>>
> >>> Query to firmware required arm SMC call, to me that does not look
> >>> correct
> >> approach. Thought of using first one but that is already used and
> >> moving that is as same as this.
> >>>
> >>> Hardcoding to 63 will make it work on some SoCs but not all.
> >>
> >> But you know which one is started or is not. GTI_CWD_WDOG tells you this.
> >
> > On a given SoC, Firmware can reserve and/or use one or more timer for some
> other use case (customer use) and configure one of the timer as watchdog timer.
> Linux have to use the configured timer only and cannot decide by its own.
> 
> Then the SoCs which have such firmware could use proposed property.
> Provide some rationale property description in your next version (adding
> necessary compatibles).

Will add compatible as below:

properties:
  compatible:
    enum:
     - marvell,cn10k-wdt
     - marvell,octeontx2-wdt

And define this as optional property 
  marvell,wdt-timer-index:
    $ref: /schemas/types.yaml#/definitions/uint32
    minimum: 0
    maximum: 63
    description:
      An SoC have many timers (up to 64), firmware can reserve one or more timer 
      for some other use case and configures one of the global timer as watchdog
      timer. Firmware will update this field with the timer number configured
      as watchdog timer.

Let me know if this looks good.

Thanks
-Bharat

> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 05/05/2023 14:19, Bharat Bhushan wrote:

>>>>> Query to firmware required arm SMC call, to me that does not look
>>>>> correct
>>>> approach. Thought of using first one but that is already used and
>>>> moving that is as same as this.
>>>>>
>>>>> Hardcoding to 63 will make it work on some SoCs but not all.
>>>>
>>>> But you know which one is started or is not. GTI_CWD_WDOG tells you this.
>>>
>>> On a given SoC, Firmware can reserve and/or use one or more timer for some
>> other use case (customer use) and configure one of the timer as watchdog timer.
>> Linux have to use the configured timer only and cannot decide by its own.
>>
>> Then the SoCs which have such firmware could use proposed property.
>> Provide some rationale property description in your next version (adding
>> necessary compatibles).
> 
> Will add compatible as below:
> 
> properties:
>   compatible:
>     enum:
>      - marvell,cn10k-wdt
>      - marvell,octeontx2-wdt

And the rest? You said you have 10 SoCs.

You made them not compatible with each other, so I assume on purpose and
your driver will make use of it.

> 
> And define this as optional property 
>   marvell,wdt-timer-index:
>     $ref: /schemas/types.yaml#/definitions/uint32
>     minimum: 0
>     maximum: 63
>     description:
>       An SoC have many timers (up to 64), firmware can reserve one or more timer 
>       for some other use case and configures one of the global timer as watchdog
>       timer. Firmware will update this field with the timer number configured
>       as watchdog timer.

Looks ok.

Best regards,
Krzysztof