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

[PATCH 1/2 v8] 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>
---
v8:
  - Compatible name as per soc name

 .../watchdog/marvell,octeontx2-wdt.yaml       | 73 +++++++++++++++++++
 1 file changed, 73 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..3c642359d960
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
@@ -0,0 +1,73 @@
+# 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 Global Timer (GTI) system watchdog
+
+allOf:
+  - $ref: watchdog.yaml#
+
+maintainers:
+  - Bharat Bhushan <bbhushan2@marvell.com>
+
+properties:
+  compatible:
+    enum:
+      - marvell,cn9670-wdt
+      - marvell,cn9880-wdt
+      - marvell,cnf9535-wdt
+      - marvell,cn10624-wdt
+      - marvell,cn10308-wdt
+      - marvell,cnf10518-wdt
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    minItems: 1
+
+  clock-names:
+    minItems: 1
+
+  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.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        watchdog@802000040000 {
+            compatible = "marvell,cn9670-wdt";
+            reg = <0x00008020 0x00040000 0x00000000 0x00020000>;
+            interrupts = <GIC_SPI 38 IRQ_TYPE_EDGE_RISING>;
+            clocks = <&sclk>;
+            clock-names = "ref_clk";
+            marvell,wdt-timer-index = <63>;
+        };
+    };
+
+...
-- 
2.17.1

[PATCH 2/2 v8] 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>
---
v8:
  - Compatible names as per soc names
  - This driver run on ARM64 based architecture, Added 64BIT config
    dependency to avoid compilation error related to readq/writeq on
    32 as platform (Hexagon)
 
 drivers/watchdog/Kconfig           |  13 ++
 drivers/watchdog/Makefile          |   1 +
 drivers/watchdog/marvell_gti_wdt.c | 346 +++++++++++++++++++++++++++++
 3 files changed, 360 insertions(+)
 create mode 100644 drivers/watchdog/marvell_gti_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index f22138709bf5..bbdc20f33dbf 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1779,6 +1779,19 @@ 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 && 64BIT)
+	default y
+	select WATCHDOG_CORE
+	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 b4c4ccf2d703..a164cd161ef3 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..976bb9306115
--- /dev/null
+++ b/drivers/watchdog/marvell_gti_wdt.c
@@ -0,0 +1,346 @@
+// 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_match_data {
+	u32 gti_num_timers;
+};
+
+static const struct gti_match_data match_data_octeontx2 = {
+	.gti_num_timers = 54,
+};
+
+static const struct gti_match_data match_data_cn10k = {
+	.gti_num_timers = 64,
+};
+
+struct gti_wdt_priv {
+	struct watchdog_device wdev;
+	void __iomem *base;
+	u32 clock_freq;
+	struct clk *sclk;
+	/* wdt_timer_idx used for timer to be used for system watchdog */
+	u32 wdt_timer_idx;
+	const struct gti_match_data *data;
+};
+
+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");
+
+	priv->data = of_device_get_match_data(dev);
+
+	/* default use last timer for watchdog */
+	priv->wdt_timer_idx = priv->data->gti_num_timers - 1;
+
+	err = of_property_read_u32(dev->of_node, "marvell,wdt-timer-index",
+				   &wdt_idx);
+	if (!err) {
+		if (wdt_idx >= priv->data->gti_num_timers)
+			return dev_err_probe(&pdev->dev, err,
+				"GTI wdog timer index not valid");
+
+		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,cn9670-wdt", .data = &match_data_octeontx2},
+	{ .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
+	{ .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},
+	{ .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},
+	{ .compatible = "marvell,cn10308-wdt", .data = &match_data_cn10k},
+	{ .compatible = "marvell,cnf10518-wdt", .data = &match_data_cn10k},
+	{ },
+};
+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 2/2 v8] Watchdog: Add marvell 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.4-rc3 next-20230525]
[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-marvell-GTI-watchdog-driver/20230526-142851
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230526062626.1180-2-bbhushan2%40marvell.com
patch subject: [PATCH 2/2 v8] Watchdog: Add marvell GTI watchdog driver
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20230528/202305280038.Fo8aOfsW-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        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/b5f326e7e3943850db45d2f06d737dc9ac37a575
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Bharat-Bhushan/Watchdog-Add-marvell-GTI-watchdog-driver/20230526-142851
        git checkout b5f326e7e3943850db45d2f06d737dc9ac37a575
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/

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

All errors (new ones prefixed by >>):

   drivers/watchdog/marvell_gti_wdt.c: In function 'gti_wdt_interrupt':
>> drivers/watchdog/marvell_gti_wdt.c:89:9: error: implicit declaration of function 'writeq' [-Werror=implicit-function-declaration]
      89 |         writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
         |         ^~~~~~
   drivers/watchdog/marvell_gti_wdt.c: In function 'gti_wdt_start':
>> drivers/watchdog/marvell_gti_wdt.c:126:18: error: implicit declaration of function 'readq' [-Werror=implicit-function-declaration]
     126 |         regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
         |                  ^~~~~
   cc1: some warnings being treated as errors


vim +/writeq +89 drivers/watchdog/marvell_gti_wdt.c

    82	
    83	static irqreturn_t gti_wdt_interrupt(int irq, void *data)
    84	{
    85		struct watchdog_device *wdev = data;
    86		struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
    87	
    88		/* Clear Interrupt Pending Status */
  > 89		writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
    90		       priv->base + GTI_CWD_INT);
    91	
    92		watchdog_notify_pretimeout(wdev);
    93	
    94		return IRQ_HANDLED;
    95	}
    96	
    97	static int gti_wdt_ping(struct watchdog_device *wdev)
    98	{
    99		struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
   100	
   101		writeq(GTI_CWD_POKE_VAL,
   102		       priv->base + GTI_CWD_POKE(priv->wdt_timer_idx));
   103	
   104		return 0;
   105	}
   106	
   107	static int gti_wdt_start(struct watchdog_device *wdev)
   108	{
   109		struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
   110		u64 regval;
   111	
   112		if (!wdev->pretimeout)
   113			return -EINVAL;
   114	
   115		set_bit(WDOG_HW_RUNNING, &wdev->status);
   116	
   117		/* Clear any pending interrupt */
   118		writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
   119		       priv->base + GTI_CWD_INT);
   120	
   121		/* Enable Interrupt */
   122		writeq(GTI_CWD_INT_ENA_SET_VAL(priv->wdt_timer_idx),
   123		       priv->base + GTI_CWD_INT_ENA_SET);
   124	
   125		/* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */
 > 126		regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
   127		regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST;
   128		writeq(regval, priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
   129	
   130		return 0;
   131	}
   132	

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

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

[Guenter Roeck]

On 5/25/23 23:26, 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>
> ---
> v8:
>    - Compatible names as per soc names
>    - This driver run on ARM64 based architecture, Added 64BIT config
>      dependency to avoid compilation error related to readq/writeq on
>      32 as platform (Hexagon)
>   

In the future, please provide complete change logs.

>   drivers/watchdog/Kconfig           |  13 ++
>   drivers/watchdog/Makefile          |   1 +
>   drivers/watchdog/marvell_gti_wdt.c | 346 +++++++++++++++++++++++++++++
>   3 files changed, 360 insertions(+)
>   create mode 100644 drivers/watchdog/marvell_gti_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index f22138709bf5..bbdc20f33dbf 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1779,6 +1779,19 @@ 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 && 64BIT)
> +	default y

Does this watchdog exist on all Thunder processors ?
If not please drop.

> +	select WATCHDOG_CORE
> +	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 b4c4ccf2d703..a164cd161ef3 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..976bb9306115
> --- /dev/null
> +++ b/drivers/watchdog/marvell_gti_wdt.c
> @@ -0,0 +1,346 @@
> +// 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>
> +
Alphabetic order, please. Also, include io.h since
you are using io functions (writeq).

> +/*
> + * 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)

reg_offset in ()

> +#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)

Please use 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_match_data {
> +	u32 gti_num_timers;
> +};
> +
> +static const struct gti_match_data match_data_octeontx2 = {

The driver depends on ARCH_THUNDER but not ARCH_THUNDER2.

> +	.gti_num_timers = 54,
> +};
> +
> +static const struct gti_match_data match_data_cn10k = {
> +	.gti_num_timers = 64,
> +};
> +
> +struct gti_wdt_priv {
> +	struct watchdog_device wdev;
> +	void __iomem *base;
> +	u32 clock_freq;
> +	struct clk *sclk;
> +	/* wdt_timer_idx used for timer to be used for system watchdog */
> +	u32 wdt_timer_idx;
> +	const struct gti_match_data *data;
> +};
> +
> +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;
> +

Any reason for not using devm_clk_get_enabled() ?

> +	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");
> +
> +	priv->data = of_device_get_match_data(dev);
> +
> +	/* default use last timer for watchdog */
> +	priv->wdt_timer_idx = priv->data->gti_num_timers - 1;
> +
> +	err = of_property_read_u32(dev->of_node, "marvell,wdt-timer-index",
> +				   &wdt_idx);
> +	if (!err) {
> +		if (wdt_idx >= priv->data->gti_num_timers)
> +			return dev_err_probe(&pdev->dev, err,
> +				"GTI wdog timer index not valid");
> +
> +		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,cn9670-wdt", .data = &match_data_octeontx2},
> +	{ .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
> +	{ .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},
> +	{ .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},
> +	{ .compatible = "marvell,cn10308-wdt", .data = &match_data_cn10k},
> +	{ .compatible = "marvell,cnf10518-wdt", .data = &match_data_cn10k},
> +	{ },
> +};
> +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");

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

[Bharat Bhushan]

Please see inline

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, May 29, 2023 7:08 PM
> To: Bharat Bhushan <bbhushan2@marvell.com>; 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; Sunil Kovvuri Goutham <sgoutham@marvell.com>
> Subject: [EXT] Re: [PATCH 2/2 v8] Watchdog: Add marvell GTI watchdog driver
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 5/25/23 23:26, 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>
> > ---
> > v8:
> >    - Compatible names as per soc names
> >    - This driver run on ARM64 based architecture, Added 64BIT config
> >      dependency to avoid compilation error related to readq/writeq on
> >      32 as platform (Hexagon)
> >
> 
> In the future, please provide complete change logs.
> 
> >   drivers/watchdog/Kconfig           |  13 ++
> >   drivers/watchdog/Makefile          |   1 +
> >   drivers/watchdog/marvell_gti_wdt.c | 346
> +++++++++++++++++++++++++++++
> >   3 files changed, 360 insertions(+)
> >   create mode 100644 drivers/watchdog/marvell_gti_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
> > f22138709bf5..bbdc20f33dbf 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -1779,6 +1779,19 @@ 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 && 64BIT)
> > +	default y
> 
> Does this watchdog exist on all Thunder processors ?
> If not please drop.

This driver is not for all thunder but Octeon tx2 and cn10k  series of processor (which are arm64 based). 

> 
> > +	select WATCHDOG_CORE
> > +	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
> > b4c4ccf2d703..a164cd161ef3 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..976bb9306115
> > --- /dev/null
> > +++ b/drivers/watchdog/marvell_gti_wdt.c
> > @@ -0,0 +1,346 @@
> > +// 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>
> > +
> Alphabetic order, please. Also, include io.h since you are using io functions
> (writeq).

Ok,

> 
> > +/*
> > + * 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)
> 
> reg_offset in ()

Ok.

> 
> > +#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)
> 
> Please use BIT().

Okay,

> 
> > +
> > +/* 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_match_data {
> > +	u32 gti_num_timers;
> > +};
> > +
> > +static const struct gti_match_data match_data_octeontx2 = {
> 
> The driver depends on ARCH_THUNDER but not ARCH_THUNDER2.

ARCH_THUNDER2 if different series.

> 
> > +	.gti_num_timers = 54,
> > +};
> > +
> > +static const struct gti_match_data match_data_cn10k = {
> > +	.gti_num_timers = 64,
> > +};
> > +
> > +struct gti_wdt_priv {
> > +	struct watchdog_device wdev;
> > +	void __iomem *base;
> > +	u32 clock_freq;
> > +	struct clk *sclk;
> > +	/* wdt_timer_idx used for timer to be used for system watchdog */
> > +	u32 wdt_timer_idx;
> > +	const struct gti_match_data *data;
> > +};
> > +
> > +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;
> > +
> 
> Any reason for not using devm_clk_get_enabled() ?

No reason, just missed it.

Thanks
-Bharat

> 
> > +	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");
> > +
> > +	priv->data = of_device_get_match_data(dev);
> > +
> > +	/* default use last timer for watchdog */
> > +	priv->wdt_timer_idx = priv->data->gti_num_timers - 1;
> > +
> > +	err = of_property_read_u32(dev->of_node, "marvell,wdt-timer-index",
> > +				   &wdt_idx);
> > +	if (!err) {
> > +		if (wdt_idx >= priv->data->gti_num_timers)
> > +			return dev_err_probe(&pdev->dev, err,
> > +				"GTI wdog timer index not valid");
> > +
> > +		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,cn9670-wdt", .data = &match_data_octeontx2},
> > +	{ .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
> > +	{ .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},
> > +	{ .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},
> > +	{ .compatible = "marvell,cn10308-wdt", .data = &match_data_cn10k},
> > +	{ .compatible = "marvell,cnf10518-wdt", .data = &match_data_cn10k},
> > +	{ },
> > +};
> > +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");

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2585 bytes --]

Yo Bharat,

On Fri, May 26, 2023 at 11:56:25AM +0530, Bharat Bhushan wrote:
> Add binding documentation for the Marvell GTI system
> watchdog driver.
> 
> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> ---
> v8:
>   - Compatible name as per soc name

I am sorry, but I do not understand this.

> 
>  .../watchdog/marvell,octeontx2-wdt.yaml       | 73 +++++++++++++++++++
>  1 file changed, 73 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..3c642359d960
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
> @@ -0,0 +1,73 @@
> +# 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 Global Timer (GTI) system watchdog
> +
> +allOf:
> +  - $ref: watchdog.yaml#

From v7:
Put allOf after maintainers:.

> +
> +maintainers:
> +  - Bharat Bhushan <bbhushan2@marvell.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - marvell,cn9670-wdt
> +      - marvell,cn9880-wdt
> +      - marvell,cnf9535-wdt
> +      - marvell,cn10624-wdt
> +      - marvell,cn10308-wdt
> +      - marvell,cnf10518-wdt

static const struct of_device_id gti_wdt_of_match[] = {
       { .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
       { .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
       { .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},
       { .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},
       { .compatible = "marvell,cn10308-wdt", .data = &match_data_cn10k},
       { .compatible = "marvell,cnf10518-wdt", .data = &match_data_cn10k},

This is a fat hint that you should be using fallback compatibles here.
You even had a fallback setup in your last revision, but you seem to
have removed it alongside the removal of the wildcards. Why did you do
that?

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1

From v7:
maxItems instead. You see it is different than above properties?

> +
> +  clock-names:
> +    minItems: 1

From v7:
Need to define names.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Bharat Bhushan]

Hi Conor,

Please see inline 

> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: Saturday, May 27, 2023 1:07 AM
> To: Bharat Bhushan <bbhushan2@marvell.com>
> Cc: 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; Sunil Kovvuri
> Goutham <sgoutham@marvell.com>
> Subject: [EXT] Re: [PATCH 1/2 v8] dt-bindings: watchdog: marvell GTI system
> watchdog driver
> 
> External Email
> 
> ----------------------------------------------------------------------
> Yo Bharat,
> 
> On Fri, May 26, 2023 at 11:56:25AM +0530, Bharat Bhushan wrote:
> > Add binding documentation for the Marvell GTI system watchdog driver.
> >
> > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> > ---
> > v8:
> >   - Compatible name as per soc name
> 
> I am sorry, but I do not understand this.

I mean to say replaced soc family name from compatible with actual soc names

> 
> >
> >  .../watchdog/marvell,octeontx2-wdt.yaml       | 73 +++++++++++++++++++
> >  1 file changed, 73 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..3c642359d960
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt
> > +++ .yaml
> > @@ -0,0 +1,73 @@
> > +# 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 Global Timer (GTI) system watchdog
> > +
> > +allOf:
> > +  - $ref: watchdog.yaml#
> 
> From v7:
> Put allOf after maintainers:.

Thanks for pointing, I am sorry, missed almost all comments on v7. Will resolve this and below as well in next version

> 
> > +
> > +maintainers:
> > +  - Bharat Bhushan <bbhushan2@marvell.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - marvell,cn9670-wdt
> > +      - marvell,cn9880-wdt
> > +      - marvell,cnf9535-wdt
> > +      - marvell,cn10624-wdt
> > +      - marvell,cn10308-wdt
> > +      - marvell,cnf10518-wdt
> 
> static const struct of_device_id gti_wdt_of_match[] = {
>        { .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
>        { .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
>        { .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},
>        { .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},
>        { .compatible = "marvell,cn10308-wdt", .data = &match_data_cn10k},
>        { .compatible = "marvell,cnf10518-wdt", .data = &match_data_cn10k},
> 
> This is a fat hint that you should be using fallback compatibles here.
> You even had a fallback setup in your last revision, but you seem to have
> removed it alongside the removal of the wildcards. Why did you do that?

Not sure I understand this comment, Compatible in last version was as below:

+ properties:
+ compatible:
+    oneOf:
+      - const: marvell,octeontx2-wdt
+      - items:
+          - enum:
+              - marvell,octeontx2-95xx-wdt
+              - marvell,octeontx2-96xx-wdt
+              - marvell,octeontx2-98xx-wdt
+          - const: marvell,octeontx2-wdt
+      - const: marvell,cn10k-wdt
+      - items:
+          - enum:
+              - marvell,cn10kx-wdt
+              - marvell,cnf10kx-wdt
+          - const: marvell,cn10k-wdt

By fallback do you mean " const: marvell,cn10k-wdt" and " const: marvell,octeontx2-wdt" ? If yes I removed because "octeontx2" and "cn10k" are soc family name and no a specific soc.

Thanks
-Bharat

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 1
> 
> From v7:
> maxItems instead. You see it is different than above properties?
> 
> > +
> > +  clock-names:
> > +    minItems: 1
> 
> From v7:
> Need to define names.
> 
> Cheers,
> Conor.

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2734 bytes --]

On Sat, May 27, 2023 at 02:53:25PM +0000, Bharat Bhushan wrote:
> From: Conor Dooley <conor@kernel.org>
> > On Fri, May 26, 2023 at 11:56:25AM +0530, Bharat Bhushan wrote:

> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - marvell,cn9670-wdt
> > > +      - marvell,cn9880-wdt
> > > +      - marvell,cnf9535-wdt
> > > +      - marvell,cn10624-wdt
> > > +      - marvell,cn10308-wdt
> > > +      - marvell,cnf10518-wdt
> > 
> > static const struct of_device_id gti_wdt_of_match[] = {
> >        { .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
> >        { .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
> >        { .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},
> >        { .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},
> >        { .compatible = "marvell,cn10308-wdt", .data = &match_data_cn10k},
> >        { .compatible = "marvell,cnf10518-wdt", .data = &match_data_cn10k},
> > 
> > This is a fat hint that you should be using fallback compatibles here.
> > You even had a fallback setup in your last revision, but you seem to have
> > removed it alongside the removal of the wildcards. Why did you do that?
> 
> Not sure I understand this comment, Compatible in last version was as below:
> 
> + properties:
> + compatible:
> +    oneOf:
> +      - const: marvell,octeontx2-wdt
> +      - items:
> +          - enum:
> +              - marvell,octeontx2-95xx-wdt
> +              - marvell,octeontx2-96xx-wdt
> +              - marvell,octeontx2-98xx-wdt
> +          - const: marvell,octeontx2-wdt
> +      - const: marvell,cn10k-wdt
> +      - items:
> +          - enum:
> +              - marvell,cn10kx-wdt
> +              - marvell,cnf10kx-wdt
> +          - const: marvell,cn10k-wdt
> 
> By fallback do you mean " const: marvell,cn10k-wdt" and
> "const: marvell,octeontx2-wdt" ? If yes I removed because "octeontx2"
> and "cn10k" are soc family name and no a specific soc.

No, I meant that you should permit
	compatible = "marvell,cn9880-wdt", "marvell,cn9670-wdt";
and
	compatible = "marvell,cnf9535-wdt", "marvell,cn9670-wdt";
and
	compatible = "marvell,cn9670-wdt";
so the driver only needs to contain
	{ .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
instead of 
	{ .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
	{ .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
	{ .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},

Note that using fallback compatibles is separate from using wildcards,
and I was not suggesting that you go back to wildcards ;)

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Bharat Bhushan]

Please see inline

> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: Saturday, May 27, 2023 9:40 PM
> To: Bharat Bhushan <bbhushan2@marvell.com>
> Cc: 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; Sunil Kovvuri
> Goutham <sgoutham@marvell.com>
> Subject: Re: [EXT] Re: [PATCH 1/2 v8] dt-bindings: watchdog: marvell GTI system
> watchdog driver
> 
> On Sat, May 27, 2023 at 02:53:25PM +0000, Bharat Bhushan wrote:
> > From: Conor Dooley <conor@kernel.org>
> > > On Fri, May 26, 2023 at 11:56:25AM +0530, Bharat Bhushan wrote:
> 
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - marvell,cn9670-wdt
> > > > +      - marvell,cn9880-wdt
> > > > +      - marvell,cnf9535-wdt
> > > > +      - marvell,cn10624-wdt
> > > > +      - marvell,cn10308-wdt
> > > > +      - marvell,cnf10518-wdt
> > >
> > > static const struct of_device_id gti_wdt_of_match[] = {
> > >        { .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
> > >        { .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
> > >        { .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},
> > >        { .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},
> > >        { .compatible = "marvell,cn10308-wdt", .data = &match_data_cn10k},
> > >        { .compatible = "marvell,cnf10518-wdt", .data =
> > > &match_data_cn10k},
> > >
> > > This is a fat hint that you should be using fallback compatibles here.
> > > You even had a fallback setup in your last revision, but you seem to
> > > have removed it alongside the removal of the wildcards. Why did you do
> that?
> >
> > Not sure I understand this comment, Compatible in last version was as below:
> >
> > + properties:
> > + compatible:
> > +    oneOf:
> > +      - const: marvell,octeontx2-wdt
> > +      - items:
> > +          - enum:
> > +              - marvell,octeontx2-95xx-wdt
> > +              - marvell,octeontx2-96xx-wdt
> > +              - marvell,octeontx2-98xx-wdt
> > +          - const: marvell,octeontx2-wdt
> > +      - const: marvell,cn10k-wdt
> > +      - items:
> > +          - enum:
> > +              - marvell,cn10kx-wdt
> > +              - marvell,cnf10kx-wdt
> > +          - const: marvell,cn10k-wdt
> >
> > By fallback do you mean " const: marvell,cn10k-wdt" and
> > "const: marvell,octeontx2-wdt" ? If yes I removed because "octeontx2"
> > and "cn10k" are soc family name and no a specific soc.
> 
> No, I meant that you should permit
> 	compatible = "marvell,cn9880-wdt", "marvell,cn9670-wdt"; and
> 	compatible = "marvell,cnf9535-wdt", "marvell,cn9670-wdt"; and
> 	compatible = "marvell,cn9670-wdt";
> so the driver only needs to contain
> 	{ .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
> instead of
> 	{ .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
> 	{ .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
> 	{ .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},
> 
> Note that using fallback compatibles is separate from using wildcards, and I was
> not suggesting that you go back to wildcards ;)

Fallback you mentioned make code look simple. Is below representation correct for above mentioned fallback? 

properties:
  compatible:
    oneOf:
      - const: marvell,cn9670-wdt
      - items:
          - enum:
              - marvell,cn9880-wdt
              - marvell,cnf9535-wdt
          - const: marvell,cn9670-wdt
      - const: marvell,cn10624-wdt
      - items:
          - enum:
              - marvell,cn10308-wdt
              - marvell,cnf10518-wdt
          - const: marvell,cn10624-wdt


And driver will have
        { .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
        { .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},


Thanks
-Bharat

> 
> Cheers,
> Conor.

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1517 bytes --]

Hey Bharat,

On Mon, May 29, 2023 at 06:14:07AM +0000, Bharat Bhushan wrote:

> Fallback you mentioned make code look simple. Is below representation correct for above mentioned fallback? 
> 
> properties:
>   compatible:
>     oneOf:
>       - const: marvell,cn9670-wdt
>       - items:
>           - enum:
>               - marvell,cn9880-wdt
>               - marvell,cnf9535-wdt
>           - const: marvell,cn9670-wdt
>       - const: marvell,cn10624-wdt
>       - items:
>           - enum:
>               - marvell,cn10308-wdt
>               - marvell,cnf10518-wdt
>           - const: marvell,cn10624-wdt

Instead of having const: bits for each of the single-compatible ones, if
you are not going to add descriptions, I'd probably do:
properties:
  compatible:
    oneOf:
      - enum:
          - marvell,cn9670-wdt
          - marvell,cn10624-wdt

      - items:
          - enum:
              - marvell,cn9880-wdt
              - marvell,cnf9535-wdt
          - const: marvell,cn9670-wdt

      - items:
          - enum:
              - marvell,cn10308-wdt
              - marvell,cnf10518-wdt
          - const: marvell,cn10624-wdt

> And driver will have
>         { .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
>         { .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},

Otherwise, looks good to me. You should probably also rename the file to
match one of the compatibles contained in it.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]