[PATCH 0/3] watchdog: sbsa_gwdt: add support for Marvell ac5

[PATCH 0/3] watchdog: sbsa_gwdt: add support for Marvell ac5

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

Add support for Marvell ac5/x variant of the ARM
sbsa global watchdog. This watchdog deviates from
the standard driver by the following items:

1. Registers reside in secure register section.
   hence access is only possible via SMC calls to ATF.

2. There are couple more registers which reside in
   other register areas, which needs to be configured
   in order for the watchdog to properly generate
   reset through the SOC.

   The new Marvell compatibility string differentiates between
   the original sbsa mode of operation and the Marvell mode of
   operation.


Elad Nachman (3):
  dt-bindings: watchdog: add Marvell AC5 watchdog
  arm64: dts: ac5: add watchdog nodes
  watchdog: sbsa_gwdt: add support for Marvell ac5

 .../bindings/watchdog/arm,sbsa-gwdt.yaml      |  52 +++-
 arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi |  14 +
 arch/arm64/boot/dts/marvell/ac5-98dx35xx.dtsi |   8 +
 drivers/watchdog/sbsa_gwdt.c                  | 247 ++++++++++++++++--
 4 files changed, 298 insertions(+), 23 deletions(-)

-- 
2.25.1

[PATCH 1/3] dt-bindings: watchdog: add Marvell AC5 watchdog

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

Add definitions and examples for Marvell AC5 variant
of the sbsa watchdog.
Marvell variant requires more memory definitions,
since the initialization is more complex, and involves
several register sets.

Signed-off-by: Elad Nachman <enachman@marvell.com>
---
 .../bindings/watchdog/arm,sbsa-gwdt.yaml      | 52 ++++++++++++++++++-
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/arm,sbsa-gwdt.yaml b/Documentation/devicetree/bindings/watchdog/arm,sbsa-gwdt.yaml
index aa804f96acba..331e9aa7c2f7 100644
--- a/Documentation/devicetree/bindings/watchdog/arm,sbsa-gwdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/arm,sbsa-gwdt.yaml
@@ -20,12 +20,17 @@ allOf:
 
 properties:
   compatible:
-    const: arm,sbsa-gwdt
+    enum:
+      - arm,sbsa-gwdt
+      - marvell,ac5-wd
 
   reg:
     items:
       - description: Watchdog control frame
       - description: Refresh frame
+      - description: Marvell CPU control frame
+      - description: Marvell Management frame
+      - description: Marvell reset control unit frame
 
   interrupts:
     description: The Watchdog Signal 0 (WS0) SPI (Shared Peripheral Interrupt)
@@ -39,12 +44,55 @@ required:
 unevaluatedProperties: false
 
 examples:
+  # First example is for generic ARM one
+  # Next examples are for Marvell.
+  # They are organized as three sets:
+  # first set is for global watchdog, then CPU core #0 private watchdog,
+  # and finally CPU core #1 private watchdog
+  # Examples are given for AC5 or Ironman. For AC5X SOC, the last
+  # reg item's low address (0x840F8000) should be replaced with 0x944F8000
   - |
     watchdog@2a440000 {
         compatible = "arm,sbsa-gwdt";
         reg = <0x2a440000 0x1000>,
-              <0x2a450000 0x1000>;
+              <0x2a450000 0x1000>,
+              <0x0 0x0>,
+              <0x0 0x0>,
+              <0x0 0x0>;
         interrupts = <0 27 4>;
         timeout-sec = <30>;
     };
+  - |
+    watchdog@80216000 {
+        compatible = "marvell,ac5-wd";
+        reg = <0x80216000 0x1000>,
+              <0x80215000 0x1000>,
+              <0x80210000 0x1000>,
+              <0x7f900000 0x1000>,
+              <0x840F8000 0x1000>;
+        interrupts = <0 124 4>;
+        timeout-sec = <30>;
+    };
+  - |
+    watchdog@80212000 {
+        compatible = "marvell,ac5-wd";
+        reg = <0x80212000 0x1000>,
+              <0x80211000 0x1000>,
+              <0x80210000 0x1000>,
+              <0x7f900000 0x1000>,
+              <0x840F8000 0x1000>;
+        interrupts = <0 122 4>;
+        timeout-sec = <30>;
+    };
+  - |
+    watchdog@80214000 {
+        compatible = "marvell,ac5-wd";
+        reg = <0x80214000 0x1000>,
+              <0x80213000 0x1000>,
+              <0x80210000 0x1000>,
+              <0x7f900000 0x1000>,
+              <0x840F8000 0x1000>;
+        interrupts = <0 122 4>;
+        timeout-sec = <30>;
+    };
 ...
-- 
2.25.1

[PATCH 2/3] arm64: dts: ac5: add watchdog nodes

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

Add watchdog nodes to ac5 and ac5x device tree files

Signed-off-by: Elad Nachman <enachman@marvell.com>
---
 arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi | 14 ++++++++++++++
 arch/arm64/boot/dts/marvell/ac5-98dx35xx.dtsi |  8 ++++++++
 2 files changed, 22 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi b/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
index b5e042b8e929..e898c6bd31f0 100644
--- a/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
@@ -307,6 +307,20 @@ nand: nand-controller@805b0000 {
 			status = "disabled";
 		};
 
+/*
+ * Global Watchdog:
+ */
+		watchdog: watchdog@80216000 {
+			compatible = "marvell,ac5-wd";
+			reg = <0x0 0x80216000 0 0x1000>,
+			      <0x0 0x80215000 0 0x1000>,
+			      <0x0 0x80210000 0 0x1000>,
+			      <0x0 0x7f900000 0 0x1000>,
+			      <0x0 0x840F8000 0 0x1000>;
+			interrupts = <GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>;
+			timeout-sec = <30>;
+		};
+
 		gic: interrupt-controller@80600000 {
 			compatible = "arm,gic-v3";
 			#interrupt-cells = <3>;
diff --git a/arch/arm64/boot/dts/marvell/ac5-98dx35xx.dtsi b/arch/arm64/boot/dts/marvell/ac5-98dx35xx.dtsi
index 2ab72f854bea..d850c30db552 100644
--- a/arch/arm64/boot/dts/marvell/ac5-98dx35xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/ac5-98dx35xx.dtsi
@@ -15,3 +15,11 @@ / {
 &cnm_clock {
 	clock-frequency = <325000000>;
 };
+
+&watchdog {
+	reg = <0x0 0x80216000 0 0x1000>,
+	      <0x0 0x80215000 0 0x1000>,
+	      <0x0 0x80210000 0 0x1000>,
+	      <0x0 0x7f900000 0 0x1000>,
+	      <0x0 0x944F8000 0 0x1000>;
+};
-- 
2.25.1

[PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

Add support for Marvell ac5/x variant of the ARM
sbsa global watchdog. This watchdog deviates from
the standard driver by the following items:

1. Registers reside in secure register section.
   hence access is only possible via SMC calls to ATF.

2. There are couple more registers which reside in
   other register areas, which needs to be configured
   in order for the watchdog to properly generate
   reset through the SOC.

The new Marvell compatibility string differentiates between
the original sbsa mode of operation and the Marvell mode of
operation.

Signed-off-by: Elad Nachman <enachman@marvell.com>
---
 drivers/watchdog/sbsa_gwdt.c | 247 ++++++++++++++++++++++++++++++++---
 1 file changed, 226 insertions(+), 21 deletions(-)

diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
index 5f23913ce3b4..0bc6f53f0968 100644
--- a/drivers/watchdog/sbsa_gwdt.c
+++ b/drivers/watchdog/sbsa_gwdt.c
@@ -46,10 +46,13 @@
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/uaccess.h>
 #include <linux/watchdog.h>
 #include <asm/arch_timer.h>
+#include <linux/arm-smccc.h>
 
 #define DRV_NAME		"sbsa-gwdt"
 #define WATCHDOG_NAME		"SBSA Generic Watchdog"
@@ -75,6 +78,68 @@
 #define SBSA_GWDT_VERSION_MASK  0xF
 #define SBSA_GWDT_VERSION_SHIFT 16
 
+/* Marvell AC5/X SMCs, taken from arm trusted firmware */
+#define SMC_FID_READ_REG	0x80007FFE
+#define SMC_FID_WRITE_REG	0x80007FFD
+
+/* Marvell registers offsets: */
+#define SBSA_GWDT_MARVELL_CPU_WD_RST_EN_REG	0x30
+#define SBSA_GWDT_MARVELL_MNG_ID_REG		0x4C
+#define SBSA_GWDT_MARVELL_RST_CTRL_REG		0x0C
+
+#define SBSA_GWDT_MARVELL_ID_MASK	GENMASK(19, 12)
+#define SBSA_GWDT_MARVELL_AC5_ID	0xB4000
+#define SBSA_GWDT_MARVELL_AC5X_ID	0x98000
+#define SBSA_GWDT_MARVELL_IML_ID	0xA0000
+#define SBSA_GWDT_MARVELL_IMM_ID	0xA2000
+
+#define SBSA_GWDT_MARVELL_AC5_RST_UNIT_WD_BIT		BIT(6)
+/* The following applies to AC5X, IronMan L and M: */
+#define SBSA_GWDT_MARVELL_IRONMAN_RST_UNIT_WD_BIT	BIT(7)
+
+/*
+ * Action to perform after watchdog gets WS1 (watchdog signal 1) interrupt
+ * PWD = Private Watchdog, GWD - Global Watchdog, mpp - multi purpose pin
+ *
+ * 0 = Enable  1 = Disable (Default)
+ *
+ * BIT  0: CPU 0 reset by PWD 0
+ * BIT  1: CPU 1 reset by PWD 1
+ * BIT  2: CPU 0 reset by GWD
+ * BIT  3: CPU 1 reset by GWD
+ * BIT  4: PWD 0 sys reset out
+ * BIT  5: PWD 1 sys reset out
+ * BIT  6: GWD sys reset out
+ * BIT  7: Reserved
+ * BIT  8: PWD 0 mpp reset out
+ * BIT  9: PWD 1 mpp reset out
+ * BIT 10: GWD mpp reset out
+ *
+ */
+#define SBSA_GWDT_MARVELL_RST_CPU0_BY_PWD0	BIT(0)
+#define SBSA_GWDT_MARVELL_RST_CPU1_BY_PWD1	BIT(1)
+#define SBSA_GWDT_MARVELL_RST_CPU0_BY_GWD	BIT(2)
+#define SBSA_GWDT_MARVELL_RST_CPU1_BY_GWD	BIT(3)
+#define SBSA_GWDT_MARVELL_RST_SYSRST_BY_PWD0	BIT(4)
+#define SBSA_GWDT_MARVELL_RST_SYSRST_BY_PWD1	BIT(5)
+#define SBSA_GWDT_MARVELL_RST_SYSRST_BY_GWD	BIT(6)
+#define SBSA_GWDT_MARVELL_RST_RESERVED		BIT(7)
+#define SBSA_GWDT_MARVELL_RST_MPP_BY_PWD0	BIT(8)
+#define SBSA_GWDT_MARVELL_RST_MPP_BY_PWD1	BIT(9)
+#define SBSA_GWDT_MARVELL_RST_MPP_BY_GWD	BIT(10)
+
+/**
+ * struct sbsa_gwdt_regs_ops - ops for register read/write, depending on SOC
+ * @reg_read:			register read ops function
+ * @read_write:			register write ops function
+ */
+struct sbsa_gwdt_regs_ops {
+	u32 (*reg_read32)(void __iomem *ptr);
+	__u64 (*reg_read64)(void __iomem *ptr);
+	void (*reg_write32)(u32 val, void __iomem *ptr);
+	void (*reg_write64)(__u64 val, void __iomem *ptr);
+};
+
 /**
  * struct sbsa_gwdt - Internal representation of the SBSA GWDT
  * @wdd:		kernel watchdog_device structure
@@ -89,6 +154,7 @@ struct sbsa_gwdt {
 	int			version;
 	void __iomem		*refresh_base;
 	void __iomem		*control_base;
+	const struct sbsa_gwdt_regs_ops *soc_reg_ops;
 };
 
 #define DEFAULT_TIMEOUT		10 /* seconds */
@@ -116,6 +182,91 @@ MODULE_PARM_DESC(nowayout,
 		 "Watchdog cannot be stopped once started (default="
 		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
+/*
+ * By default, have the Global watchdog cause System Reset:
+ */
+static unsigned int reset = 0xFFFFFFFF ^ SBSA_GWDT_MARVELL_RST_SYSRST_BY_GWD;
+module_param(reset, uint, 0);
+MODULE_PARM_DESC(reset, "Action to perform after watchdog gets WS1 interrupt");
+
+/*
+ * Marvell AC5/X use SMC, while others use direct register access
+ */
+static u32 sbsa_gwdt_smc_readl(void __iomem *addr)
+{
+	struct arm_smccc_res smc_res;
+
+	arm_smccc_smc(SMC_FID_READ_REG, (unsigned long)addr,
+		      0, 0, 0, 0, 0, 0, &smc_res);
+	return (u32)smc_res.a0;
+}
+
+static void sbsa_gwdt_smc_writel(u32 val, void __iomem *addr)
+{
+	struct arm_smccc_res smc_res;
+
+	arm_smccc_smc(SMC_FID_WRITE_REG, (unsigned long)addr,
+		      (unsigned long)val, 0, 0, 0, 0, 0, &smc_res);
+}
+
+static inline u64 sbsa_gwdt_lo_hi_smc_readq(void __iomem *addr)
+{
+	u32 low, high;
+
+	low = sbsa_gwdt_smc_readl(addr);
+	high = sbsa_gwdt_smc_readl(addr + 4);
+	/* read twice, as a workaround to HW limitation */
+	low = sbsa_gwdt_smc_readl(addr);
+
+	return low + ((u64)high << 32);
+}
+
+static inline void sbsa_gwdt_lo_hi_smc_writeq(__u64 val, void __iomem *addr)
+{
+	u32 low, high;
+
+	low = val & 0xffffffff;
+	high = val >> 32;
+	sbsa_gwdt_smc_writel(low, addr);
+	sbsa_gwdt_smc_writel(high, addr + 4);
+	/* write twice, as a workaround to HW limitation */
+	sbsa_gwdt_smc_writel(low, addr);
+}
+
+static u32 sbsa_gwdt_direct_reg_readl(void __iomem *addr)
+{
+	return readl(addr);
+}
+
+static void sbsa_gwdt_direct_reg_writel(u32 val, void __iomem *addr)
+{
+	writel(val, addr);
+}
+
+static inline u64 sbsa_gwdt_lo_hi_direct_readq(void __iomem *addr)
+{
+	return lo_hi_readq(addr);
+}
+
+static inline void sbsa_gwdt_lo_hi_direct_writeq(__u64 val, void __iomem *addr)
+{
+	lo_hi_writeq(val, addr);
+}
+
+static const struct sbsa_gwdt_regs_ops smc_reg_ops = {
+	.reg_read32 = sbsa_gwdt_smc_readl,
+	.reg_read64 = sbsa_gwdt_lo_hi_smc_readq,
+	.reg_write32 = sbsa_gwdt_smc_writel,
+	.reg_write64 = sbsa_gwdt_lo_hi_smc_writeq
+};
+
+static const struct sbsa_gwdt_regs_ops direct_reg_ops = {
+	.reg_read32 = sbsa_gwdt_direct_reg_readl,
+	.reg_read64 = sbsa_gwdt_lo_hi_direct_readq,
+	.reg_write32 = sbsa_gwdt_direct_reg_writel,
+	.reg_write64 = sbsa_gwdt_lo_hi_smc_writeq
+};
+
 /*
  * Arm Base System Architecture 1.0 introduces watchdog v1 which
  * increases the length watchdog offset register to 48 bits.
@@ -127,17 +278,17 @@ MODULE_PARM_DESC(nowayout,
 static u64 sbsa_gwdt_reg_read(struct sbsa_gwdt *gwdt)
 {
 	if (gwdt->version == 0)
-		return readl(gwdt->control_base + SBSA_GWDT_WOR);
+		return gwdt->soc_reg_ops->reg_read32(gwdt->control_base + SBSA_GWDT_WOR);
 	else
-		return lo_hi_readq(gwdt->control_base + SBSA_GWDT_WOR);
+		return gwdt->soc_reg_ops->reg_read64(gwdt->control_base + SBSA_GWDT_WOR);
 }
 
 static void sbsa_gwdt_reg_write(u64 val, struct sbsa_gwdt *gwdt)
 {
 	if (gwdt->version == 0)
-		writel((u32)val, gwdt->control_base + SBSA_GWDT_WOR);
+		gwdt->soc_reg_ops->reg_write32((u32)val, gwdt->control_base + SBSA_GWDT_WOR);
 	else
-		lo_hi_writeq(val, gwdt->control_base + SBSA_GWDT_WOR);
+		gwdt->soc_reg_ops->reg_write64(val, gwdt->control_base + SBSA_GWDT_WOR);
 }
 
 /*
@@ -175,10 +326,11 @@ static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd)
 	 * timeleft = WOR + (WCV - system counter)
 	 */
 	if (!action &&
-	    !(readl(gwdt->control_base + SBSA_GWDT_WCS) & SBSA_GWDT_WCS_WS0))
+	    !(gwdt->soc_reg_ops->reg_read32(gwdt->control_base + SBSA_GWDT_WCS)
+					    & SBSA_GWDT_WCS_WS0))
 		timeleft += sbsa_gwdt_reg_read(gwdt);
 
-	timeleft += lo_hi_readq(gwdt->control_base + SBSA_GWDT_WCV) -
+	timeleft += gwdt->soc_reg_ops->reg_read64(gwdt->control_base + SBSA_GWDT_WCV) -
 		    arch_timer_read_counter();
 
 	do_div(timeleft, gwdt->clk);
@@ -194,7 +346,7 @@ static int sbsa_gwdt_keepalive(struct watchdog_device *wdd)
 	 * Writing WRR for an explicit watchdog refresh.
 	 * You can write anyting (like 0).
 	 */
-	writel(0, gwdt->refresh_base + SBSA_GWDT_WRR);
+	gwdt->soc_reg_ops->reg_write32(0, gwdt->refresh_base + SBSA_GWDT_WRR);
 
 	return 0;
 }
@@ -204,7 +356,7 @@ static void sbsa_gwdt_get_version(struct watchdog_device *wdd)
 	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
 	int ver;
 
-	ver = readl(gwdt->control_base + SBSA_GWDT_W_IIDR);
+	ver = gwdt->soc_reg_ops->reg_read32(gwdt->control_base + SBSA_GWDT_W_IIDR);
 	ver = (ver >> SBSA_GWDT_VERSION_SHIFT) & SBSA_GWDT_VERSION_MASK;
 
 	gwdt->version = ver;
@@ -215,7 +367,7 @@ static int sbsa_gwdt_start(struct watchdog_device *wdd)
 	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
 
 	/* writing WCS will cause an explicit watchdog refresh */
-	writel(SBSA_GWDT_WCS_EN, gwdt->control_base + SBSA_GWDT_WCS);
+	gwdt->soc_reg_ops->reg_write32(SBSA_GWDT_WCS_EN, gwdt->control_base + SBSA_GWDT_WCS);
 
 	return 0;
 }
@@ -225,7 +377,7 @@ static int sbsa_gwdt_stop(struct watchdog_device *wdd)
 	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
 
 	/* Simply write 0 to WCS to clean WCS_EN bit */
-	writel(0, gwdt->control_base + SBSA_GWDT_WCS);
+	gwdt->soc_reg_ops->reg_write32(0, gwdt->control_base + SBSA_GWDT_WCS);
 
 	return 0;
 }
@@ -257,24 +409,55 @@ static const struct watchdog_ops sbsa_gwdt_ops = {
 static int sbsa_gwdt_probe(struct platform_device *pdev)
 {
 	void __iomem *rf_base, *cf_base;
+	void __iomem *cpu_ctrl_base = NULL, *mng_base = NULL,
+		     *rst_ctrl_base = NULL;
 	struct device *dev = &pdev->dev;
+	struct device_node *np = pdev->dev.of_node;
 	struct watchdog_device *wdd;
 	struct sbsa_gwdt *gwdt;
+	struct resource *res;
 	int ret, irq;
-	u32 status;
+	bool marvell = false;
+	u32 status, id, val;
 
 	gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
 	if (!gwdt)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, gwdt);
 
-	cf_base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(cf_base))
-		return PTR_ERR(cf_base);
-
-	rf_base = devm_platform_ioremap_resource(pdev, 1);
-	if (IS_ERR(rf_base))
-		return PTR_ERR(rf_base);
+	if (of_device_is_compatible(np, "marvell,ac5-wd")) {
+		marvell = true;
+		gwdt->soc_reg_ops = &smc_reg_ops;
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (IS_ERR(res))
+			return PTR_ERR(res);
+		cf_base = res->start;
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		if (IS_ERR(res))
+			return PTR_ERR(res);
+		rf_base = res->start;
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+		if (IS_ERR(res))
+			return PTR_ERR(res);
+		cpu_ctrl_base = res->start;
+		mng_base = devm_platform_ioremap_resource(pdev, 3);
+		if (IS_ERR(mng_base))
+			return PTR_ERR(mng_base);
+		rst_ctrl_base = devm_platform_ioremap_resource(pdev, 4);
+		if (IS_ERR(rst_ctrl_base))
+			return PTR_ERR(rst_ctrl_base);
+	} else {
+		gwdt->soc_reg_ops = &direct_reg_ops;
+		cf_base = devm_platform_ioremap_resource(pdev, 0);
+		if (IS_ERR(cf_base))
+			return PTR_ERR(cf_base);
+
+		rf_base = devm_platform_ioremap_resource(pdev, 1);
+		if (IS_ERR(rf_base))
+			return PTR_ERR(rf_base);
+	}
 
 	/*
 	 * Get the frequency of system counter from the cp15 interface of ARM
@@ -299,7 +482,7 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
 	else
 		wdd->max_hw_heartbeat_ms = GENMASK_ULL(47, 0) / gwdt->clk * 1000;
 
-	status = readl(cf_base + SBSA_GWDT_WCS);
+	status = gwdt->soc_reg_ops->reg_read32(cf_base + SBSA_GWDT_WCS);
 	if (status & SBSA_GWDT_WCS_WS1) {
 		dev_warn(dev, "System reset by WDT.\n");
 		wdd->bootstatus |= WDIOF_CARDRESET;
@@ -317,7 +500,7 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
 			 * In case there is a pending ws0 interrupt, just ping
 			 * the watchdog before registering the interrupt routine
 			 */
-			writel(0, rf_base + SBSA_GWDT_WRR);
+			gwdt->soc_reg_ops->reg_write32(0, rf_base + SBSA_GWDT_WRR);
 			if (devm_request_irq(dev, irq, sbsa_gwdt_interrupt, 0,
 					     pdev->name, gwdt)) {
 				action = 0;
@@ -347,7 +530,28 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
 	ret = devm_watchdog_register_device(dev, wdd);
 	if (ret)
 		return ret;
-
+	/*
+	 * Marvell AC5/X/IM: need to configure the watchdog
+	 * HW to trigger reset on WS1 (Watchdog Signal 1):
+	 *
+	 * 1. Configure the watchdog signal enable (routing)
+	 *    according to configuration
+	 * 2. Unmask the wd_rst input signal to the reset unit
+	 */
+	if (marvell) {
+		gwdt->soc_reg_ops->reg_write32(reset, cpu_ctrl_base +
+					       SBSA_GWDT_MARVELL_CPU_WD_RST_EN_REG);
+		id = readl(mng_base + SBSA_GWDT_MARVELL_MNG_ID_REG) &
+			   SBSA_GWDT_MARVELL_ID_MASK;
+
+		if (id == SBSA_GWDT_MARVELL_AC5_ID)
+			val = SBSA_GWDT_MARVELL_AC5_RST_UNIT_WD_BIT;
+		else
+			val = SBSA_GWDT_MARVELL_IRONMAN_RST_UNIT_WD_BIT;
+
+		writel(readl(rst_ctrl_base + SBSA_GWDT_MARVELL_RST_CTRL_REG) & ~val,
+		       rst_ctrl_base + SBSA_GWDT_MARVELL_RST_CTRL_REG);
+	}
 	dev_info(dev, "Initialized with %ds timeout @ %u Hz, action=%d.%s\n",
 		 wdd->timeout, gwdt->clk, action,
 		 status & SBSA_GWDT_WCS_EN ? " [enabled]" : "");
@@ -383,6 +587,7 @@ static const struct dev_pm_ops sbsa_gwdt_pm_ops = {
 
 static const struct of_device_id sbsa_gwdt_of_match[] = {
 	{ .compatible = "arm,sbsa-gwdt", },
+	{ .compatible = "marvell,ac5-wd", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match);
-- 
2.25.1

Re: [PATCH 1/3] dt-bindings: watchdog: add Marvell AC5 watchdog

[Krzysztof Kozlowski]

On 14/12/2023 16:04, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Add definitions and examples for Marvell AC5 variant
> of the sbsa watchdog.
> Marvell variant requires more memory definitions,
> since the initialization is more complex, and involves
> several register sets.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

> 
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
>  .../bindings/watchdog/arm,sbsa-gwdt.yaml      | 52 ++++++++++++++++++-
>  1 file changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/arm,sbsa-gwdt.yaml b/Documentation/devicetree/bindings/watchdog/arm,sbsa-gwdt.yaml
> index aa804f96acba..331e9aa7c2f7 100644
> --- a/Documentation/devicetree/bindings/watchdog/arm,sbsa-gwdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/arm,sbsa-gwdt.yaml
> @@ -20,12 +20,17 @@ allOf:
>  
>  properties:
>    compatible:
> -    const: arm,sbsa-gwdt
> +    enum:
> +      - arm,sbsa-gwdt
> +      - marvell,ac5-wd
>  
>    reg:
>      items:
>        - description: Watchdog control frame
>        - description: Refresh frame
> +      - description: Marvell CPU control frame
> +      - description: Marvell Management frame
> +      - description: Marvell reset control unit frame

You just broke all the users... I doubt this was tested on ARM platforms.

>  
>    interrupts:
>      description: The Watchdog Signal 0 (WS0) SPI (Shared Peripheral Interrupt)
> @@ -39,12 +44,55 @@ required:
>  unevaluatedProperties: false
>  
>  examples:
> +  # First example is for generic ARM one
> +  # Next examples are for Marvell.

One new example could be enough... but if it differs with one property,
also not that much of benefit.

> +  # They are organized as three sets:
> +  # first set is for global watchdog, then CPU core #0 private watchdog,
> +  # and finally CPU core #1 private watchdog
> +  # Examples are given for AC5 or Ironman. For AC5X SOC, the last
> +  # reg item's low address (0x840F8000) should be replaced with 0x944F8000
>    - |
>      watchdog@2a440000 {
>          compatible = "arm,sbsa-gwdt";
>          reg = <0x2a440000 0x1000>,
> -              <0x2a450000 0x1000>;
> +              <0x2a450000 0x1000>,
> +              <0x0 0x0>,
> +              <0x0 0x0>,
> +              <0x0 0x0>;

No, drop.

>          interrupts = <0 27 4>;
>          timeout-sec = <30>;
>      };
> +  - |
> +    watchdog@80216000 {
> +        compatible = "marvell,ac5-wd";
> +        reg = <0x80216000 0x1000>,
> +              <0x80215000 0x1000>,
> +              <0x80210000 0x1000>,
> +              <0x7f900000 0x1000>,
> +              <0x840F8000 0x1000>;
> +        interrupts = <0 124 4>;

Use proper defines.

> +        timeout-sec = <30>;
> +    };
> +  - |
> +    watchdog@80212000 {

Drop example.

> +        compatible = "marvell,ac5-wd";
> +        reg = <0x80212000 0x1000>,
> +              <0x80211000 0x1000>,
> +              <0x80210000 0x1000>,
> +              <0x7f900000 0x1000>,
> +              <0x840F8000 0x1000>;
> +        interrupts = <0 122 4>;
> +        timeout-sec = <30>;
> +    };
> +  - |
> +    watchdog@80214000 {

Drop example.

Best regards,
Krzysztof

Re: [PATCH 2/3] arm64: dts: ac5: add watchdog nodes

[Krzysztof Kozlowski]

On 14/12/2023 16:04, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Add watchdog nodes to ac5 and ac5x device tree files
> 
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
>  arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi | 14 ++++++++++++++
>  arch/arm64/boot/dts/marvell/ac5-98dx35xx.dtsi |  8 ++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi b/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
> index b5e042b8e929..e898c6bd31f0 100644
> --- a/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
> +++ b/arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
> @@ -307,6 +307,20 @@ nand: nand-controller@805b0000 {
>  			status = "disabled";
>  		};
>  
> +/*
> + * Global Watchdog:
> + */

Messed indentation. Also unnecessary line breaks around comment, unless
you have some KPI per lines of code. If it is the only watchdog, why
even commenting on it?

> +		watchdog: watchdog@80216000 {
> +			compatible = "marvell,ac5-wd";
> +			reg = <0x0 0x80216000 0 0x1000>,
> +			      <0x0 0x80215000 0 0x1000>,
> +			      <0x0 0x80210000 0 0x1000>,
> +			      <0x0 0x7f900000 0 0x1000>,
> +			      <0x0 0x840F8000 0 0x1000>;

Lowercase hex.



Best regards,
Krzysztof

Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5

[Krzysztof Kozlowski]

On 14/12/2023 16:04, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Add support for Marvell ac5/x variant of the ARM
> sbsa global watchdog. This watchdog deviates from
> the standard driver by the following items:
> 
> 1. Registers reside in secure register section.
>    hence access is only possible via SMC calls to ATF.
> 
> 2. There are couple more registers which reside in
>    other register areas, which needs to be configured
>    in order for the watchdog to properly generate
>    reset through the SOC.
> 
> The new Marvell compatibility string differentiates between
> the original sbsa mode of operation and the Marvell mode of
> operation.
> 
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---

...

>  	gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
>  	if (!gwdt)
>  		return -ENOMEM;
>  	platform_set_drvdata(pdev, gwdt);
>  
> -	cf_base = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(cf_base))
> -		return PTR_ERR(cf_base);
> -
> -	rf_base = devm_platform_ioremap_resource(pdev, 1);
> -	if (IS_ERR(rf_base))
> -		return PTR_ERR(rf_base);
> +	if (of_device_is_compatible(np, "marvell,ac5-wd")) {

No, use match data. That's its purpose, don't put comaptibles inside code.

> +		marvell = true;
> +		gwdt->soc_reg_ops = &smc_reg_ops;
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (IS_ERR(res))
> +			return PTR_ERR(res);
> +		cf_base = res->start;

Why do you use entirely different code?

> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +		if (IS_ERR(res))
> +			return PTR_ERR(res);
> +		rf_base = res->start;
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +		if (IS_ERR(res))
> +			return PTR_ERR(res);
> +		cpu_ctrl_base = res->start;
> +		mng_base = devm_platform_ioremap_resource(pdev, 3);
> +		if (IS_ERR(mng_base))
> +			return PTR_ERR(mng_base);
> +		rst_ctrl_base = devm_platform_ioremap_resource(pdev, 4);
> +		if (IS_ERR(rst_ctrl_base))
> +			return PTR_ERR(rst_ctrl_base);
> +	} else {
> +		gwdt->soc_reg_ops = &direct_reg_ops;
> +		cf_base = devm_platform_ioremap_resource(pdev, 0);
> +		if (IS_ERR(cf_base))
> +			return PTR_ERR(cf_base);

Why? This is shared.

> +
> +		rf_base = devm_platform_ioremap_resource(pdev, 1);
> +		if (IS_ERR(rf_base))
> +			return PTR_ERR(rf_base);

Ditto

> +	}
>  
>  	/*
>  	 * Get the frequency of system counter from the cp15 interface of ARM
> @@ -299,7 +482,7 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
>  	else
>  		wdd->max_hw_heartbeat_ms = GENMASK_ULL(47, 0) / gwdt->clk * 1000;
>  
> -	status = readl(cf_base + SBSA_GWDT_WCS);
> +	status = gwdt->soc_reg_ops->reg_read32(cf_base + SBSA_GWDT_WCS);
>  	if (status & SBSA_GWDT_WCS_WS1) {
>  		dev_warn(dev, "System reset by WDT.\n");
>  		wdd->bootstatus |= WDIOF_CARDRESET;
> @@ -317,7 +500,7 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
>  			 * In case there is a pending ws0 interrupt, just ping
>  			 * the watchdog before registering the interrupt routine
>  			 */
> -			writel(0, rf_base + SBSA_GWDT_WRR);
> +			gwdt->soc_reg_ops->reg_write32(0, rf_base + SBSA_GWDT_WRR);
>  			if (devm_request_irq(dev, irq, sbsa_gwdt_interrupt, 0,
>  					     pdev->name, gwdt)) {
>  				action = 0;
> @@ -347,7 +530,28 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
>  	ret = devm_watchdog_register_device(dev, wdd);
>  	if (ret)
>  		return ret;
> -
> +	/*
> +	 * Marvell AC5/X/IM: need to configure the watchdog
> +	 * HW to trigger reset on WS1 (Watchdog Signal 1):
> +	 *
> +	 * 1. Configure the watchdog signal enable (routing)
> +	 *    according to configuration
> +	 * 2. Unmask the wd_rst input signal to the reset unit
> +	 */
> +	if (marvell) {
> +		gwdt->soc_reg_ops->reg_write32(reset, cpu_ctrl_base +
> +					       SBSA_GWDT_MARVELL_CPU_WD_RST_EN_REG);
> +		id = readl(mng_base + SBSA_GWDT_MARVELL_MNG_ID_REG) &
> +			   SBSA_GWDT_MARVELL_ID_MASK;
> +
> +		if (id == SBSA_GWDT_MARVELL_AC5_ID)
> +			val = SBSA_GWDT_MARVELL_AC5_RST_UNIT_WD_BIT;
> +		else
> +			val = SBSA_GWDT_MARVELL_IRONMAN_RST_UNIT_WD_BIT;
> +
> +		writel(readl(rst_ctrl_base + SBSA_GWDT_MARVELL_RST_CTRL_REG) & ~val,
> +		       rst_ctrl_base + SBSA_GWDT_MARVELL_RST_CTRL_REG);
> +	}
>  	dev_info(dev, "Initialized with %ds timeout @ %u Hz, action=%d.%s\n",
>  		 wdd->timeout, gwdt->clk, action,
>  		 status & SBSA_GWDT_WCS_EN ? " [enabled]" : "");
> @@ -383,6 +587,7 @@ static const struct dev_pm_ops sbsa_gwdt_pm_ops = {
>  
>  static const struct of_device_id sbsa_gwdt_of_match[] = {
>  	{ .compatible = "arm,sbsa-gwdt", },
> +	{ .compatible = "marvell,ac5-wd", },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match);

Best regards,
Krzysztof

Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5

[kernel test robot]

Hi Elad,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on groeck-staging/hwmon-next linus/master v6.7-rc5 next-20231214]
[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/Elad-Nachman/dt-bindings-watchdog-add-Marvell-AC5-watchdog/20231214-230812
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20231214150414.1849058-4-enachman%40marvell.com
patch subject: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20231215/202312150848.3mhXhH3Y-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231215/202312150848.3mhXhH3Y-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312150848.3mhXhH3Y-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/watchdog/sbsa_gwdt.c: In function 'sbsa_gwdt_probe':
>> drivers/watchdog/sbsa_gwdt.c:434:25: warning: assignment to 'void *' from 'resource_size_t' {aka 'long long unsigned int'} makes pointer from integer without a cast [-Wint-conversion]
     434 |                 cf_base = res->start;
         |                         ^
   drivers/watchdog/sbsa_gwdt.c:439:25: warning: assignment to 'void *' from 'resource_size_t' {aka 'long long unsigned int'} makes pointer from integer without a cast [-Wint-conversion]
     439 |                 rf_base = res->start;
         |                         ^
   drivers/watchdog/sbsa_gwdt.c:444:31: warning: assignment to 'void *' from 'resource_size_t' {aka 'long long unsigned int'} makes pointer from integer without a cast [-Wint-conversion]
     444 |                 cpu_ctrl_base = res->start;
         |                               ^
--
>> drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or member 'reg_read32' not described in 'sbsa_gwdt_regs_ops'
>> drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or member 'reg_read64' not described in 'sbsa_gwdt_regs_ops'
>> drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or member 'reg_write32' not described in 'sbsa_gwdt_regs_ops'
>> drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or member 'reg_write64' not described in 'sbsa_gwdt_regs_ops'
>> drivers/watchdog/sbsa_gwdt.c:141: warning: Excess struct member 'reg_read' description in 'sbsa_gwdt_regs_ops'
>> drivers/watchdog/sbsa_gwdt.c:141: warning: Excess struct member 'read_write' description in 'sbsa_gwdt_regs_ops'
>> drivers/watchdog/sbsa_gwdt.c:158: warning: Function parameter or member 'soc_reg_ops' not described in 'sbsa_gwdt'


vim +434 drivers/watchdog/sbsa_gwdt.c

   408	
   409	static int sbsa_gwdt_probe(struct platform_device *pdev)
   410	{
   411		void __iomem *rf_base, *cf_base;
   412		void __iomem *cpu_ctrl_base = NULL, *mng_base = NULL,
   413			     *rst_ctrl_base = NULL;
   414		struct device *dev = &pdev->dev;
   415		struct device_node *np = pdev->dev.of_node;
   416		struct watchdog_device *wdd;
   417		struct sbsa_gwdt *gwdt;
   418		struct resource *res;
   419		int ret, irq;
   420		bool marvell = false;
   421		u32 status, id, val;
   422	
   423		gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
   424		if (!gwdt)
   425			return -ENOMEM;
   426		platform_set_drvdata(pdev, gwdt);
   427	
   428		if (of_device_is_compatible(np, "marvell,ac5-wd")) {
   429			marvell = true;
   430			gwdt->soc_reg_ops = &smc_reg_ops;
   431			res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   432			if (IS_ERR(res))
   433				return PTR_ERR(res);
 > 434			cf_base = res->start;
   435	
   436			res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
   437			if (IS_ERR(res))
   438				return PTR_ERR(res);
   439			rf_base = res->start;
   440	
   441			res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
   442			if (IS_ERR(res))
   443				return PTR_ERR(res);
   444			cpu_ctrl_base = res->start;
   445			mng_base = devm_platform_ioremap_resource(pdev, 3);
   446			if (IS_ERR(mng_base))
   447				return PTR_ERR(mng_base);
   448			rst_ctrl_base = devm_platform_ioremap_resource(pdev, 4);
   449			if (IS_ERR(rst_ctrl_base))
   450				return PTR_ERR(rst_ctrl_base);
   451		} else {
   452			gwdt->soc_reg_ops = &direct_reg_ops;
   453			cf_base = devm_platform_ioremap_resource(pdev, 0);
   454			if (IS_ERR(cf_base))
   455				return PTR_ERR(cf_base);
   456	
   457			rf_base = devm_platform_ioremap_resource(pdev, 1);
   458			if (IS_ERR(rf_base))
   459				return PTR_ERR(rf_base);
   460		}
   461	
   462		/*
   463		 * Get the frequency of system counter from the cp15 interface of ARM
   464		 * Generic timer. We don't need to check it, because if it returns "0",
   465		 * system would panic in very early stage.
   466		 */
   467		gwdt->clk = arch_timer_get_cntfrq();
   468		gwdt->refresh_base = rf_base;
   469		gwdt->control_base = cf_base;
   470	
   471		wdd = &gwdt->wdd;
   472		wdd->parent = dev;
   473		wdd->info = &sbsa_gwdt_info;
   474		wdd->ops = &sbsa_gwdt_ops;
   475		wdd->min_timeout = 1;
   476		wdd->timeout = DEFAULT_TIMEOUT;
   477		watchdog_set_drvdata(wdd, gwdt);
   478		watchdog_set_nowayout(wdd, nowayout);
   479		sbsa_gwdt_get_version(wdd);
   480		if (gwdt->version == 0)
   481			wdd->max_hw_heartbeat_ms = U32_MAX / gwdt->clk * 1000;
   482		else
   483			wdd->max_hw_heartbeat_ms = GENMASK_ULL(47, 0) / gwdt->clk * 1000;
   484	
   485		status = gwdt->soc_reg_ops->reg_read32(cf_base + SBSA_GWDT_WCS);
   486		if (status & SBSA_GWDT_WCS_WS1) {
   487			dev_warn(dev, "System reset by WDT.\n");
   488			wdd->bootstatus |= WDIOF_CARDRESET;
   489		}
   490		if (status & SBSA_GWDT_WCS_EN)
   491			set_bit(WDOG_HW_RUNNING, &wdd->status);
   492	
   493		if (action) {
   494			irq = platform_get_irq(pdev, 0);
   495			if (irq < 0) {
   496				action = 0;
   497				dev_warn(dev, "unable to get ws0 interrupt.\n");
   498			} else {
   499				/*
   500				 * In case there is a pending ws0 interrupt, just ping
   501				 * the watchdog before registering the interrupt routine
   502				 */
   503				gwdt->soc_reg_ops->reg_write32(0, rf_base + SBSA_GWDT_WRR);
   504				if (devm_request_irq(dev, irq, sbsa_gwdt_interrupt, 0,
   505						     pdev->name, gwdt)) {
   506					action = 0;
   507					dev_warn(dev, "unable to request IRQ %d.\n",
   508						 irq);
   509				}
   510			}
   511			if (!action)
   512				dev_warn(dev, "falling back to single stage mode.\n");
   513		}
   514		/*
   515		 * In the single stage mode, The first signal (WS0) is ignored,
   516		 * the timeout is (WOR * 2), so the maximum timeout should be doubled.
   517		 */
   518		if (!action)
   519			wdd->max_hw_heartbeat_ms *= 2;
   520	
   521		watchdog_init_timeout(wdd, timeout, dev);
   522		/*
   523		 * Update timeout to WOR.
   524		 * Because of the explicit watchdog refresh mechanism,
   525		 * it's also a ping, if watchdog is enabled.
   526		 */
   527		sbsa_gwdt_set_timeout(wdd, wdd->timeout);
   528	
   529		watchdog_stop_on_reboot(wdd);
   530		ret = devm_watchdog_register_device(dev, wdd);
   531		if (ret)
   532			return ret;
   533		/*
   534		 * Marvell AC5/X/IM: need to configure the watchdog
   535		 * HW to trigger reset on WS1 (Watchdog Signal 1):
   536		 *
   537		 * 1. Configure the watchdog signal enable (routing)
   538		 *    according to configuration
   539		 * 2. Unmask the wd_rst input signal to the reset unit
   540		 */
   541		if (marvell) {
   542			gwdt->soc_reg_ops->reg_write32(reset, cpu_ctrl_base +
   543						       SBSA_GWDT_MARVELL_CPU_WD_RST_EN_REG);
   544			id = readl(mng_base + SBSA_GWDT_MARVELL_MNG_ID_REG) &
   545				   SBSA_GWDT_MARVELL_ID_MASK;
   546	
   547			if (id == SBSA_GWDT_MARVELL_AC5_ID)
   548				val = SBSA_GWDT_MARVELL_AC5_RST_UNIT_WD_BIT;
   549			else
   550				val = SBSA_GWDT_MARVELL_IRONMAN_RST_UNIT_WD_BIT;
   551	
   552			writel(readl(rst_ctrl_base + SBSA_GWDT_MARVELL_RST_CTRL_REG) & ~val,
   553			       rst_ctrl_base + SBSA_GWDT_MARVELL_RST_CTRL_REG);
   554		}
   555		dev_info(dev, "Initialized with %ds timeout @ %u Hz, action=%d.%s\n",
   556			 wdd->timeout, gwdt->clk, action,
   557			 status & SBSA_GWDT_WCS_EN ? " [enabled]" : "");
   558	
   559		return 0;
   560	}
   561	

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

Re: [PATCH 0/3] watchdog: sbsa_gwdt: add support for Marvell ac5

[Chris Packham]

On 15/12/23 04:04, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
>
> Add support for Marvell ac5/x variant of the ARM
> sbsa global watchdog. This watchdog deviates from
> the standard driver by the following items:
>
> 1. Registers reside in secure register section.
>     hence access is only possible via SMC calls to ATF.
>
> 2. There are couple more registers which reside in
>     other register areas, which needs to be configured
>     in order for the watchdog to properly generate
>     reset through the SOC.
>
>     The new Marvell compatibility string differentiates between
>     the original sbsa mode of operation and the Marvell mode of
>     operation.

I gave this a quick try on our AC5X based board and it worked well with 
both action=0/action=1

> Elad Nachman (3):
>    dt-bindings: watchdog: add Marvell AC5 watchdog
>    arm64: dts: ac5: add watchdog nodes
>    watchdog: sbsa_gwdt: add support for Marvell ac5
>
>   .../bindings/watchdog/arm,sbsa-gwdt.yaml      |  52 +++-
>   arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi |  14 +
>   arch/arm64/boot/dts/marvell/ac5-98dx35xx.dtsi |   8 +
>   drivers/watchdog/sbsa_gwdt.c                  | 247 ++++++++++++++++--
>   4 files changed, 298 insertions(+), 23 deletions(-)
>

Re: [PATCH 0/3] watchdog: sbsa_gwdt: add support for Marvell ac5

[Rob Herring]

On Thu, Dec 14, 2023 at 05:04:11PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Add support for Marvell ac5/x variant of the ARM
> sbsa global watchdog. This watchdog deviates from
> the standard driver by the following items:
> 
> 1. Registers reside in secure register section.
>    hence access is only possible via SMC calls to ATF.

Oops.

> 2. There are couple more registers which reside in
>    other register areas, which needs to be configured
>    in order for the watchdog to properly generate
>    reset through the SOC.

Your firmware should configure these.

> 
>    The new Marvell compatibility string differentiates between
>    the original sbsa mode of operation and the Marvell mode of
>    operation.
> 
> 
> Elad Nachman (3):
>   dt-bindings: watchdog: add Marvell AC5 watchdog
>   arm64: dts: ac5: add watchdog nodes
>   watchdog: sbsa_gwdt: add support for Marvell ac5
> 
>  .../bindings/watchdog/arm,sbsa-gwdt.yaml      |  52 +++-
>  arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi |  14 +
>  arch/arm64/boot/dts/marvell/ac5-98dx35xx.dtsi |   8 +
>  drivers/watchdog/sbsa_gwdt.c                  | 247 ++++++++++++++++--
>  4 files changed, 298 insertions(+), 23 deletions(-)
> 
> -- 
> 2.25.1
> 

Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5

[Rob Herring]

On Thu, Dec 14, 2023 at 05:04:14PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Add support for Marvell ac5/x variant of the ARM
> sbsa global watchdog. This watchdog deviates from
> the standard driver by the following items:
> 
> 1. Registers reside in secure register section.
>    hence access is only possible via SMC calls to ATF.
> 
> 2. There are couple more registers which reside in
>    other register areas, which needs to be configured
>    in order for the watchdog to properly generate
>    reset through the SOC.
> 
> The new Marvell compatibility string differentiates between
> the original sbsa mode of operation and the Marvell mode of
> operation.
> 
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
>  drivers/watchdog/sbsa_gwdt.c | 247 ++++++++++++++++++++++++++++++++---

That's more than half the existing driver...

>  1 file changed, 226 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
> index 5f23913ce3b4..0bc6f53f0968 100644
> --- a/drivers/watchdog/sbsa_gwdt.c
> +++ b/drivers/watchdog/sbsa_gwdt.c
> @@ -46,10 +46,13 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/uaccess.h>
>  #include <linux/watchdog.h>
>  #include <asm/arch_timer.h>
> +#include <linux/arm-smccc.h>
>  
>  #define DRV_NAME		"sbsa-gwdt"
>  #define WATCHDOG_NAME		"SBSA Generic Watchdog"
> @@ -75,6 +78,68 @@
>  #define SBSA_GWDT_VERSION_MASK  0xF
>  #define SBSA_GWDT_VERSION_SHIFT 16
>  
> +/* Marvell AC5/X SMCs, taken from arm trusted firmware */
> +#define SMC_FID_READ_REG	0x80007FFE
> +#define SMC_FID_WRITE_REG	0x80007FFD
> +
> +/* Marvell registers offsets: */
> +#define SBSA_GWDT_MARVELL_CPU_WD_RST_EN_REG	0x30
> +#define SBSA_GWDT_MARVELL_MNG_ID_REG		0x4C
> +#define SBSA_GWDT_MARVELL_RST_CTRL_REG		0x0C
> +
> +#define SBSA_GWDT_MARVELL_ID_MASK	GENMASK(19, 12)
> +#define SBSA_GWDT_MARVELL_AC5_ID	0xB4000
> +#define SBSA_GWDT_MARVELL_AC5X_ID	0x98000
> +#define SBSA_GWDT_MARVELL_IML_ID	0xA0000
> +#define SBSA_GWDT_MARVELL_IMM_ID	0xA2000
> +
> +#define SBSA_GWDT_MARVELL_AC5_RST_UNIT_WD_BIT		BIT(6)
> +/* The following applies to AC5X, IronMan L and M: */
> +#define SBSA_GWDT_MARVELL_IRONMAN_RST_UNIT_WD_BIT	BIT(7)
> +
> +/*
> + * Action to perform after watchdog gets WS1 (watchdog signal 1) interrupt
> + * PWD = Private Watchdog, GWD - Global Watchdog, mpp - multi purpose pin
> + *
> + * 0 = Enable  1 = Disable (Default)
> + *
> + * BIT  0: CPU 0 reset by PWD 0
> + * BIT  1: CPU 1 reset by PWD 1
> + * BIT  2: CPU 0 reset by GWD
> + * BIT  3: CPU 1 reset by GWD
> + * BIT  4: PWD 0 sys reset out
> + * BIT  5: PWD 1 sys reset out
> + * BIT  6: GWD sys reset out
> + * BIT  7: Reserved
> + * BIT  8: PWD 0 mpp reset out
> + * BIT  9: PWD 1 mpp reset out
> + * BIT 10: GWD mpp reset out
> + *
> + */
> +#define SBSA_GWDT_MARVELL_RST_CPU0_BY_PWD0	BIT(0)
> +#define SBSA_GWDT_MARVELL_RST_CPU1_BY_PWD1	BIT(1)
> +#define SBSA_GWDT_MARVELL_RST_CPU0_BY_GWD	BIT(2)
> +#define SBSA_GWDT_MARVELL_RST_CPU1_BY_GWD	BIT(3)
> +#define SBSA_GWDT_MARVELL_RST_SYSRST_BY_PWD0	BIT(4)
> +#define SBSA_GWDT_MARVELL_RST_SYSRST_BY_PWD1	BIT(5)
> +#define SBSA_GWDT_MARVELL_RST_SYSRST_BY_GWD	BIT(6)
> +#define SBSA_GWDT_MARVELL_RST_RESERVED		BIT(7)
> +#define SBSA_GWDT_MARVELL_RST_MPP_BY_PWD0	BIT(8)
> +#define SBSA_GWDT_MARVELL_RST_MPP_BY_PWD1	BIT(9)
> +#define SBSA_GWDT_MARVELL_RST_MPP_BY_GWD	BIT(10)
> +
> +/**
> + * struct sbsa_gwdt_regs_ops - ops for register read/write, depending on SOC
> + * @reg_read:			register read ops function
> + * @read_write:			register write ops function
> + */
> +struct sbsa_gwdt_regs_ops {
> +	u32 (*reg_read32)(void __iomem *ptr);
> +	__u64 (*reg_read64)(void __iomem *ptr);
> +	void (*reg_write32)(u32 val, void __iomem *ptr);
> +	void (*reg_write64)(__u64 val, void __iomem *ptr);
> +};
> +
>  /**
>   * struct sbsa_gwdt - Internal representation of the SBSA GWDT
>   * @wdd:		kernel watchdog_device structure
> @@ -89,6 +154,7 @@ struct sbsa_gwdt {
>  	int			version;
>  	void __iomem		*refresh_base;
>  	void __iomem		*control_base;
> +	const struct sbsa_gwdt_regs_ops *soc_reg_ops;
>  };
>  
>  #define DEFAULT_TIMEOUT		10 /* seconds */
> @@ -116,6 +182,91 @@ MODULE_PARM_DESC(nowayout,
>  		 "Watchdog cannot be stopped once started (default="
>  		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>  
> +/*
> + * By default, have the Global watchdog cause System Reset:
> + */
> +static unsigned int reset = 0xFFFFFFFF ^ SBSA_GWDT_MARVELL_RST_SYSRST_BY_GWD;
> +module_param(reset, uint, 0);
> +MODULE_PARM_DESC(reset, "Action to perform after watchdog gets WS1 interrupt");
> +
> +/*
> + * Marvell AC5/X use SMC, while others use direct register access
> + */
> +static u32 sbsa_gwdt_smc_readl(void __iomem *addr)
> +{
> +	struct arm_smccc_res smc_res;
> +
> +	arm_smccc_smc(SMC_FID_READ_REG, (unsigned long)addr,
> +		      0, 0, 0, 0, 0, 0, &smc_res);
> +	return (u32)smc_res.a0;
> +}
> +
> +static void sbsa_gwdt_smc_writel(u32 val, void __iomem *addr)
> +{
> +	struct arm_smccc_res smc_res;
> +
> +	arm_smccc_smc(SMC_FID_WRITE_REG, (unsigned long)addr,
> +		      (unsigned long)val, 0, 0, 0, 0, 0, &smc_res);
> +}
> +
> +static inline u64 sbsa_gwdt_lo_hi_smc_readq(void __iomem *addr)
> +{
> +	u32 low, high;
> +
> +	low = sbsa_gwdt_smc_readl(addr);
> +	high = sbsa_gwdt_smc_readl(addr + 4);
> +	/* read twice, as a workaround to HW limitation */
> +	low = sbsa_gwdt_smc_readl(addr);
> +
> +	return low + ((u64)high << 32);
> +}
> +
> +static inline void sbsa_gwdt_lo_hi_smc_writeq(__u64 val, void __iomem *addr)
> +{
> +	u32 low, high;
> +
> +	low = val & 0xffffffff;
> +	high = val >> 32;
> +	sbsa_gwdt_smc_writel(low, addr);
> +	sbsa_gwdt_smc_writel(high, addr + 4);
> +	/* write twice, as a workaround to HW limitation */
> +	sbsa_gwdt_smc_writel(low, addr);
> +}
> +
> +static u32 sbsa_gwdt_direct_reg_readl(void __iomem *addr)
> +{
> +	return readl(addr);
> +}
> +
> +static void sbsa_gwdt_direct_reg_writel(u32 val, void __iomem *addr)
> +{
> +	writel(val, addr);
> +}
> +
> +static inline u64 sbsa_gwdt_lo_hi_direct_readq(void __iomem *addr)
> +{
> +	return lo_hi_readq(addr);
> +}
> +
> +static inline void sbsa_gwdt_lo_hi_direct_writeq(__u64 val, void __iomem *addr)
> +{
> +	lo_hi_writeq(val, addr);
> +}
> +
> +static const struct sbsa_gwdt_regs_ops smc_reg_ops = {
> +	.reg_read32 = sbsa_gwdt_smc_readl,
> +	.reg_read64 = sbsa_gwdt_lo_hi_smc_readq,
> +	.reg_write32 = sbsa_gwdt_smc_writel,
> +	.reg_write64 = sbsa_gwdt_lo_hi_smc_writeq
> +};
> +
> +static const struct sbsa_gwdt_regs_ops direct_reg_ops = {
> +	.reg_read32 = sbsa_gwdt_direct_reg_readl,
> +	.reg_read64 = sbsa_gwdt_lo_hi_direct_readq,
> +	.reg_write32 = sbsa_gwdt_direct_reg_writel,
> +	.reg_write64 = sbsa_gwdt_lo_hi_smc_writeq
> +};

The watchdog_ops are already practically not much more than a register 
read or write. Do we really need 2 levels of ops here?

> +
>  /*
>   * Arm Base System Architecture 1.0 introduces watchdog v1 which
>   * increases the length watchdog offset register to 48 bits.
> @@ -127,17 +278,17 @@ MODULE_PARM_DESC(nowayout,
>  static u64 sbsa_gwdt_reg_read(struct sbsa_gwdt *gwdt)
>  {
>  	if (gwdt->version == 0)
> -		return readl(gwdt->control_base + SBSA_GWDT_WOR);
> +		return gwdt->soc_reg_ops->reg_read32(gwdt->control_base + SBSA_GWDT_WOR);
>  	else
> -		return lo_hi_readq(gwdt->control_base + SBSA_GWDT_WOR);
> +		return gwdt->soc_reg_ops->reg_read64(gwdt->control_base + SBSA_GWDT_WOR);
>  }

Here we already have a different way to abstract register accesses. 
Probably should have something that works for all 3 cases...

Rob

Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5

[Guenter Roeck]

On 12/15/23 10:01, Rob Herring wrote:
> On Thu, Dec 14, 2023 at 05:04:14PM +0200, Elad Nachman wrote:
>> From: Elad Nachman <enachman@marvell.com>
>>
>> Add support for Marvell ac5/x variant of the ARM
>> sbsa global watchdog. This watchdog deviates from
>> the standard driver by the following items:
>>
>> 1. Registers reside in secure register section.
>>     hence access is only possible via SMC calls to ATF.
>>
>> 2. There are couple more registers which reside in
>>     other register areas, which needs to be configured
>>     in order for the watchdog to properly generate
>>     reset through the SOC.
>>
>> The new Marvell compatibility string differentiates between
>> the original sbsa mode of operation and the Marvell mode of
>> operation.
>>
>> Signed-off-by: Elad Nachman <enachman@marvell.com>
>> ---
>>   drivers/watchdog/sbsa_gwdt.c | 247 ++++++++++++++++++++++++++++++++---
> 
> That's more than half the existing driver...
> 

... which makes me really unhappy and wonder if it is appropriate
to hack up the existing driver that much. it doesn't look like
Marvell ac5/x really implements SBSA. Given the large number of
device specific deviations, a separate driver may be more appropriate.

Guenter

Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5

[kernel test robot]

Hi Elad,

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.7-rc5 next-20231215]
[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/Elad-Nachman/dt-bindings-watchdog-add-Marvell-AC5-watchdog/20231214-230812
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20231214150414.1849058-4-enachman%40marvell.com
patch subject: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5
config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20231216/202312160648.h1CrZxtx-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231216/202312160648.h1CrZxtx-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312160648.h1CrZxtx-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/watchdog/sbsa_gwdt.c:434:11: error: incompatible integer to pointer conversion assigning to 'void *' from 'resource_size_t' (aka 'unsigned long long') [-Wint-conversion]
     434 |                 cf_base = res->start;
         |                         ^ ~~~~~~~~~~
   drivers/watchdog/sbsa_gwdt.c:439:11: error: incompatible integer to pointer conversion assigning to 'void *' from 'resource_size_t' (aka 'unsigned long long') [-Wint-conversion]
     439 |                 rf_base = res->start;
         |                         ^ ~~~~~~~~~~
   drivers/watchdog/sbsa_gwdt.c:444:17: error: incompatible integer to pointer conversion assigning to 'void *' from 'resource_size_t' (aka 'unsigned long long') [-Wint-conversion]
     444 |                 cpu_ctrl_base = res->start;
         |                               ^ ~~~~~~~~~~
   3 errors generated.


vim +434 drivers/watchdog/sbsa_gwdt.c

   408	
   409	static int sbsa_gwdt_probe(struct platform_device *pdev)
   410	{
   411		void __iomem *rf_base, *cf_base;
   412		void __iomem *cpu_ctrl_base = NULL, *mng_base = NULL,
   413			     *rst_ctrl_base = NULL;
   414		struct device *dev = &pdev->dev;
   415		struct device_node *np = pdev->dev.of_node;
   416		struct watchdog_device *wdd;
   417		struct sbsa_gwdt *gwdt;
   418		struct resource *res;
   419		int ret, irq;
   420		bool marvell = false;
   421		u32 status, id, val;
   422	
   423		gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
   424		if (!gwdt)
   425			return -ENOMEM;
   426		platform_set_drvdata(pdev, gwdt);
   427	
   428		if (of_device_is_compatible(np, "marvell,ac5-wd")) {
   429			marvell = true;
   430			gwdt->soc_reg_ops = &smc_reg_ops;
   431			res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   432			if (IS_ERR(res))
   433				return PTR_ERR(res);
 > 434			cf_base = res->start;
   435	
   436			res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
   437			if (IS_ERR(res))
   438				return PTR_ERR(res);
   439			rf_base = res->start;
   440	
   441			res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
   442			if (IS_ERR(res))
   443				return PTR_ERR(res);
   444			cpu_ctrl_base = res->start;
   445			mng_base = devm_platform_ioremap_resource(pdev, 3);
   446			if (IS_ERR(mng_base))
   447				return PTR_ERR(mng_base);
   448			rst_ctrl_base = devm_platform_ioremap_resource(pdev, 4);
   449			if (IS_ERR(rst_ctrl_base))
   450				return PTR_ERR(rst_ctrl_base);
   451		} else {
   452			gwdt->soc_reg_ops = &direct_reg_ops;
   453			cf_base = devm_platform_ioremap_resource(pdev, 0);
   454			if (IS_ERR(cf_base))
   455				return PTR_ERR(cf_base);
   456	
   457			rf_base = devm_platform_ioremap_resource(pdev, 1);
   458			if (IS_ERR(rf_base))
   459				return PTR_ERR(rf_base);
   460		}
   461	
   462		/*
   463		 * Get the frequency of system counter from the cp15 interface of ARM
   464		 * Generic timer. We don't need to check it, because if it returns "0",
   465		 * system would panic in very early stage.
   466		 */
   467		gwdt->clk = arch_timer_get_cntfrq();
   468		gwdt->refresh_base = rf_base;
   469		gwdt->control_base = cf_base;
   470	
   471		wdd = &gwdt->wdd;
   472		wdd->parent = dev;
   473		wdd->info = &sbsa_gwdt_info;
   474		wdd->ops = &sbsa_gwdt_ops;
   475		wdd->min_timeout = 1;
   476		wdd->timeout = DEFAULT_TIMEOUT;
   477		watchdog_set_drvdata(wdd, gwdt);
   478		watchdog_set_nowayout(wdd, nowayout);
   479		sbsa_gwdt_get_version(wdd);
   480		if (gwdt->version == 0)
   481			wdd->max_hw_heartbeat_ms = U32_MAX / gwdt->clk * 1000;
   482		else
   483			wdd->max_hw_heartbeat_ms = GENMASK_ULL(47, 0) / gwdt->clk * 1000;
   484	
   485		status = gwdt->soc_reg_ops->reg_read32(cf_base + SBSA_GWDT_WCS);
   486		if (status & SBSA_GWDT_WCS_WS1) {
   487			dev_warn(dev, "System reset by WDT.\n");
   488			wdd->bootstatus |= WDIOF_CARDRESET;
   489		}
   490		if (status & SBSA_GWDT_WCS_EN)
   491			set_bit(WDOG_HW_RUNNING, &wdd->status);
   492	
   493		if (action) {
   494			irq = platform_get_irq(pdev, 0);
   495			if (irq < 0) {
   496				action = 0;
   497				dev_warn(dev, "unable to get ws0 interrupt.\n");
   498			} else {
   499				/*
   500				 * In case there is a pending ws0 interrupt, just ping
   501				 * the watchdog before registering the interrupt routine
   502				 */
   503				gwdt->soc_reg_ops->reg_write32(0, rf_base + SBSA_GWDT_WRR);
   504				if (devm_request_irq(dev, irq, sbsa_gwdt_interrupt, 0,
   505						     pdev->name, gwdt)) {
   506					action = 0;
   507					dev_warn(dev, "unable to request IRQ %d.\n",
   508						 irq);
   509				}
   510			}
   511			if (!action)
   512				dev_warn(dev, "falling back to single stage mode.\n");
   513		}
   514		/*
   515		 * In the single stage mode, The first signal (WS0) is ignored,
   516		 * the timeout is (WOR * 2), so the maximum timeout should be doubled.
   517		 */
   518		if (!action)
   519			wdd->max_hw_heartbeat_ms *= 2;
   520	
   521		watchdog_init_timeout(wdd, timeout, dev);
   522		/*
   523		 * Update timeout to WOR.
   524		 * Because of the explicit watchdog refresh mechanism,
   525		 * it's also a ping, if watchdog is enabled.
   526		 */
   527		sbsa_gwdt_set_timeout(wdd, wdd->timeout);
   528	
   529		watchdog_stop_on_reboot(wdd);
   530		ret = devm_watchdog_register_device(dev, wdd);
   531		if (ret)
   532			return ret;
   533		/*
   534		 * Marvell AC5/X/IM: need to configure the watchdog
   535		 * HW to trigger reset on WS1 (Watchdog Signal 1):
   536		 *
   537		 * 1. Configure the watchdog signal enable (routing)
   538		 *    according to configuration
   539		 * 2. Unmask the wd_rst input signal to the reset unit
   540		 */
   541		if (marvell) {
   542			gwdt->soc_reg_ops->reg_write32(reset, cpu_ctrl_base +
   543						       SBSA_GWDT_MARVELL_CPU_WD_RST_EN_REG);
   544			id = readl(mng_base + SBSA_GWDT_MARVELL_MNG_ID_REG) &
   545				   SBSA_GWDT_MARVELL_ID_MASK;
   546	
   547			if (id == SBSA_GWDT_MARVELL_AC5_ID)
   548				val = SBSA_GWDT_MARVELL_AC5_RST_UNIT_WD_BIT;
   549			else
   550				val = SBSA_GWDT_MARVELL_IRONMAN_RST_UNIT_WD_BIT;
   551	
   552			writel(readl(rst_ctrl_base + SBSA_GWDT_MARVELL_RST_CTRL_REG) & ~val,
   553			       rst_ctrl_base + SBSA_GWDT_MARVELL_RST_CTRL_REG);
   554		}
   555		dev_info(dev, "Initialized with %ds timeout @ %u Hz, action=%d.%s\n",
   556			 wdd->timeout, gwdt->clk, action,
   557			 status & SBSA_GWDT_WCS_EN ? " [enabled]" : "");
   558	
   559		return 0;
   560	}
   561	

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

Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5

[kernel test robot]

Hi Elad,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on groeck-staging/hwmon-next linus/master v6.7-rc5 next-20231215]
[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/Elad-Nachman/dt-bindings-watchdog-add-Marvell-AC5-watchdog/20231214-230812
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20231214150414.1849058-4-enachman%40marvell.com
patch subject: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20231217/202312171047.mHNijCmt-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231217/202312171047.mHNijCmt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312171047.mHNijCmt-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or struct member 'reg_read32' not described in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or struct member 'reg_read64' not described in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or struct member 'reg_write32' not described in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Function parameter or struct member 'reg_write64' not described in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Excess struct member 'reg_read' description in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Excess struct member 'read_write' description in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Excess struct member 'read_write' description in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:141: warning: Excess struct member 'reg_read' description in 'sbsa_gwdt_regs_ops'
>> scripts/kernel-doc: drivers/watchdog/sbsa_gwdt.c:158: warning: Function parameter or struct member 'soc_reg_ops' not described in 'sbsa_gwdt'

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

Re: [PATCH 3/3] watchdog: sbsa_gwdt: add support for Marvell ac5

[Rob Herring]

On Thu, Dec 14, 2023 at 9:05 AM Elad Nachman <enachman@marvell.com> wrote:
>
> From: Elad Nachman <enachman@marvell.com>
>
> Add support for Marvell ac5/x variant of the ARM
> sbsa global watchdog. This watchdog deviates from
> the standard driver by the following items:
>
> 1. Registers reside in secure register section.
>    hence access is only possible via SMC calls to ATF.
>
> 2. There are couple more registers which reside in
>    other register areas, which needs to be configured
>    in order for the watchdog to properly generate
>    reset through the SOC.
>
> The new Marvell compatibility string differentiates between
> the original sbsa mode of operation and the Marvell mode of
> operation.
>
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
>  drivers/watchdog/sbsa_gwdt.c | 247 ++++++++++++++++++++++++++++++++---
>  1 file changed, 226 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
> index 5f23913ce3b4..0bc6f53f0968 100644
> --- a/drivers/watchdog/sbsa_gwdt.c
> +++ b/drivers/watchdog/sbsa_gwdt.c
> @@ -46,10 +46,13 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/uaccess.h>
>  #include <linux/watchdog.h>
>  #include <asm/arch_timer.h>
> +#include <linux/arm-smccc.h>
>
>  #define DRV_NAME               "sbsa-gwdt"
>  #define WATCHDOG_NAME          "SBSA Generic Watchdog"
> @@ -75,6 +78,68 @@
>  #define SBSA_GWDT_VERSION_MASK  0xF
>  #define SBSA_GWDT_VERSION_SHIFT 16
>
> +/* Marvell AC5/X SMCs, taken from arm trusted firmware */
> +#define SMC_FID_READ_REG       0x80007FFE
> +#define SMC_FID_WRITE_REG      0x80007FFD

One more thing, these IDs are part of the Arm arch range and can't be
used. You should be using the SIP range AIUI.

Perhaps you should look at arm_smc_wdt.c and make that work on your
system. Despite the name, my understanding is it is a ChromeOS defined
watchdog, not an Arm (Ltd) one.

Rob