qcom watchdog fixes

From: Matthew McClintock <[email protected]>

Update the compatible string to align with driver

CC [email protected]

Signed-off-by Matthew McClintock <[email protected]>

---
  Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/watchdog/qcom-wdt.txt +0 -0

index 4726924..60bb2f98 100644
@@ -4,9 +4,8 @@ Qualcomm Krait Processor Sub-system (KPSS) Watchdog
  Required properties :

Quoted 24 lines · Thomas Pedersen

From: Matthew McClintock <[email protected]>

Update the compatible string to align with driver

CC [email protected]

Signed-off-by Matthew McClintock <[email protected]>

---
  Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/watchdog/qcom-wdt.txt +2 -3

index 4726924..60bb2f98 100644
@@ -4,9 +4,8 @@ Qualcomm Krait Processor Sub-system (KPSS) Watchdog
  Required properties :
  - compatible : shall contain only one of the following:
 
-			"qcom,kpss-wdt-msm8960"
-			"qcom,kpss-wdt-apq8064"
-			"qcom,kpss-wdt-ipq8064"
+			"qcom,kpss-timer"
+			"qcom,scss-timer"
 

Documentation/devicetree/bindings/watchdog/qcom-wdt.txt Qualcomm Krait Processor Sub-system (KPSS) Watchdog · line 11

Quoted 3 lines

Documentation/devicetree/bindings/watchdog/qcom-wdt.txt +0 -0

  - reg : shall contain base register location and length
  - clocks : shall contain the input clock

Rob's earlier feedback to the same patch you had submitted in March:

"Keep the SoC specific ones even if they are not used. The DTS should have both strings."

Quoted 33 lines · Guenter Roeck

Quoted 19 lines · Thomas Pedersen

From: Matthew McClintock <[email protected]>

Update the compatible string to align with driver

CC [email protected]

Signed-off-by Matthew McClintock <[email protected]>

---
 Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt index 4726924..60bb2f98 100644

Documentation/devicetree/bindings/watchdog/qcom-wdt.txt +0 -0

@@ -4,9 +4,8 @@ Qualcomm Krait Processor Sub-system (KPSS) Watchdog
 Required properties :

Documentation/devicetree/bindings/watchdog/qcom-wdt.txt Qualcomm Krait Processor Sub-system (KPSS) Watchdog · line 11

Quoted 10 lines

Documentation/devicetree/bindings/watchdog/qcom-wdt.txt +2 -3

 - compatible : shall contain only one of the following:
 
-			"qcom,kpss-wdt-msm8960"
-			"qcom,kpss-wdt-apq8064"
-			"qcom,kpss-wdt-ipq8064"
+			"qcom,kpss-timer"
+			"qcom,scss-timer"
 
 - reg : shall contain base register location and length
 - clocks : shall contain the input clock

Rob's earlier feedback to the same patch you had submitted in March:

Quoted 2 lines

"Keep the SoC specific ones even if they are not used. The DTS should have both strings."

Why would we add the SoC specific compatible strings to the DTS if they're not
even used?

Quoted 37 lines · Thomas Pedersen

Quoted 35 lines · Guenter Roeck

Quoted 19 lines · Thomas Pedersen

From: Matthew McClintock <[email protected]>

Update the compatible string to align with driver

CC [email protected]

Signed-off-by Matthew McClintock <[email protected]>

---
 Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt index 4726924..60bb2f98 100644

Documentation/devicetree/bindings/watchdog/qcom-wdt.txt +0 -0

@@ -4,9 +4,8 @@ Qualcomm Krait Processor Sub-system (KPSS) Watchdog
 Required properties :

Documentation/devicetree/bindings/watchdog/qcom-wdt.txt Qualcomm Krait Processor Sub-system (KPSS) Watchdog · line 11

Quoted 10 lines

Documentation/devicetree/bindings/watchdog/qcom-wdt.txt +2 -3

 - compatible : shall contain only one of the following:
 
-			"qcom,kpss-wdt-msm8960"
-			"qcom,kpss-wdt-apq8064"
-			"qcom,kpss-wdt-ipq8064"
+			"qcom,kpss-timer"
+			"qcom,scss-timer"
 
 - reg : shall contain base register location and length
 - clocks : shall contain the input clock

Rob's earlier feedback to the same patch you had submitted in March:

"Keep the SoC specific ones even if they are not used. The DTS should have both strings."

Quoted 3 lines

Why would we add the SoC specific compatible strings to the DTS if they're not
even used?

Because they might be in future, and having them present in existing DTBs will make our lives easier. Otherwise, they cost very little.

From: Matthew McClintock <[email protected]>

Commit 0dfd582e026a ("watchdog: qcom: use timer devicetree binding") moved to use the watchdog as a subset timer register block. Some devices have the watchdog completely standalone with slightly different register offsets as well so let's account for the differences here.

Signed-off-by Matthew McClintock <[email protected]>

---
  drivers/watchdog/qcom-wdt.c | 73 ++++++++++++++++++++++++++++++++-------------
  1 file changed, 52 insertions(+), 21 deletions(-)

drivers/watchdog/qcom-wdt.c +52 -14

index a043fa4..70e42ed 100644
@@ -19,18 +19,40 @@
  #include <linux/platform_device.h>
  #include <linux/watchdog.h>
 
-#define WDT_RST		0x38
-#define WDT_EN		0x40
-#define WDT_STS		0x44
-#define WDT_BITE_TIME	0x5C
+enum wdt_reg {
+	WDT_RST,
+	WDT_EN,
+	WDT_STS,
+	WDT_BITE_TIME,
+};
+
+static const u32 reg_offset_data_apcs_tmr[] = {
+	[WDT_RST] = 0x38,
+	[WDT_EN] = 0x40,
+	[WDT_STS] = 0x44,
+	[WDT_BITE_TIME] = 0x5C,
+};
+
+static const u32 reg_offset_data_kpss[] = {
+	[WDT_RST] = 0x4,
+	[WDT_EN] = 0x8,
+	[WDT_STS] = 0xC,
+	[WDT_BITE_TIME] = 0x14,
+};
 
  struct qcom_wdt {
  	struct watchdog_device	wdd;
  	struct clk		*clk;
  	unsigned long		rate;
  	void __iomem		*base;
+	const u32		*layout;
  };
 
+static void __iomem *wdt_addr(struct qcom_wdt *wdt, enum wdt_reg reg)
+{
+	return wdt->base + wdt->layout[reg];
+}
+
  static inline
  struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
  {

@@ -41,10 +63,10 @@ static int qcom_wdt_start(struct watchdog_device *wdd)
  {
  	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
 
-	writel(0, wdt->base + WDT_EN);
-	writel(1, wdt->base + WDT_RST);
-	writel(wdd->timeout * wdt->rate, wdt->base + WDT_BITE_TIME);
-	writel(1, wdt->base + WDT_EN);
+	writel(0, wdt_addr(wdt, WDT_EN));
+	writel(1, wdt_addr(wdt, WDT_RST));
+	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
+	writel(1, wdt_addr(wdt, WDT_EN));
  	return 0;
  }
 

@@ -52,7 +74,7 @@ static int qcom_wdt_stop(struct watchdog_device *wdd)
  {
  	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
 
-	writel(0, wdt->base + WDT_EN);
+	writel(0, wdt_addr(wdt, WDT_EN));
  	return 0;
  }
 

@@ -60,7 +82,7 @@ static int qcom_wdt_ping(struct watchdog_device *wdd)
  {
  	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
 
-	writel(1, wdt->base + WDT_RST);
+	writel(1, wdt_addr(wdt, WDT_RST));
  	return 0;
  }
 

@@ -83,10 +105,10 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
  	 */
  	timeout = 128 * wdt->rate / 1000;
 
-	writel(0, wdt->base + WDT_EN);
-	writel(1, wdt->base + WDT_RST);
-	writel(timeout, wdt->base + WDT_BITE_TIME);
-	writel(1, wdt->base + WDT_EN);
+	writel(0, wdt_addr(wdt, WDT_EN));
+	writel(1, wdt_addr(wdt, WDT_RST));
+	writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
+	writel(1, wdt_addr(wdt, WDT_EN));
 
  	/*
  	 * Actually make sure the above sequence hits hardware before sleeping.

@@ -114,14 +136,29 @@ static const struct watchdog_info qcom_wdt_info = {
  	.identity	= KBUILD_MODNAME,
  };
 
+static const struct of_device_id qcom_wdt_of_table[] = {
+	{ .compatible = "qcom,kpss-timer", .data = reg_offset_data_apcs_tmr },
+	{ .compatible = "qcom,scss-timer", .data = reg_offset_data_apcs_tmr },
+	{ .compatible = "qcom,kpss-standalone", .data = &reg_offset_data_kpss},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, qcom_wdt_of_table);
+
  static int qcom_wdt_probe(struct platform_device *pdev)
  {
  	struct qcom_wdt *wdt;
  	struct resource *res;
  	struct device_node *np = pdev->dev.of_node;
+	const struct of_device_id *match;
  	u32 percpu_offset;
  	int ret;
 
+	match = of_match_node(qcom_wdt_of_table, np);
+	if (!match) {
+		dev_err(&pdev->dev, "Unsupported QCOM WDT module\n");
+		return -ENODEV;
+	}
+
  	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
  	if (!wdt)
  		return -ENOMEM;

@@ -172,6 +209,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
  	wdt->wdd.min_timeout = 1;
  	wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
  	wdt->wdd.parent = &pdev->dev;
+	wdt->layout = match->data;
 
  	if (readl(wdt->base + WDT_STS) & 1)
  		wdt->wdd.bootstatus = WDIOF_CARDRESET;

drivers/watchdog/qcom-wdt.c +0 -0

  static struct platform_driver qcom_watchdog_driver = {
  	.probe	= qcom_wdt_probe,
  	.remove	= qcom_wdt_remove,

From: Matthew McClintock <[email protected]>

For certain parts and some versions of TZ, TZ will reset the chip when a BARK is triggered even though it was not configured here. So by default let's configure this BARK time as well.

Signed-off-by Matthew McClintock <[email protected]>

---
  drivers/watchdog/qcom-wdt.c | 5 +++++
  1 file changed, 5 insertions(+)

drivers/watchdog/qcom-wdt.c +5 -0

index 70e42ed..e2ad6e1 100644
@@ -23,6 +23,7 @@ enum wdt_reg {
  	WDT_RST,
  	WDT_EN,
  	WDT_STS,
+	WDT_BARK_TIME,
  	WDT_BITE_TIME,
  };
 

@@ -30,6 +31,7 @@ static const u32 reg_offset_data_apcs_tmr[] = {
  	[WDT_RST] = 0x38,
  	[WDT_EN] = 0x40,
  	[WDT_STS] = 0x44,
+	[WDT_BARK_TIME] = 0x4C,
  	[WDT_BITE_TIME] = 0x5C,
  };
 

@@ -37,6 +39,7 @@ static const u32 reg_offset_data_kpss[] = {
  	[WDT_RST] = 0x4,
  	[WDT_EN] = 0x8,
  	[WDT_STS] = 0xC,
+	[WDT_BARK_TIME] = 0x10,
  	[WDT_BITE_TIME] = 0x14,
  };
 

@@ -65,6 +68,7 @@ static int qcom_wdt_start(struct watchdog_device *wdd)
 
  	writel(0, wdt_addr(wdt, WDT_EN));
  	writel(1, wdt_addr(wdt, WDT_RST));
+	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
  	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
  	writel(1, wdt_addr(wdt, WDT_EN));
  	return 0;

@@ -107,6 +111,7 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
 
  	writel(0, wdt_addr(wdt, WDT_EN));
  	writel(1, wdt_addr(wdt, WDT_RST));
+	writel(timeout, wdt_addr(wdt, WDT_BARK_TIME));
  	writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
  	writel(1, wdt_addr(wdt, WDT_EN));
 

Some boards (eg. qcom IPQ8064 based) Need these changes to avoid a spurious watchdog reset.

Matthew McClintock (3):
watchdog: qcom: update device tree bindings
watchdog: qcom: add option for standalone watchdog not in timer block
watchdog: qcom: configure BARK time in addition to BITE time

  .../devicetree/bindings/watchdog/qcom-wdt.txt      |  5 +-
  drivers/watchdog/qcom-wdt.c                        | 78 ++++++++++++++++------
  2 files changed, 59 insertions(+), 24 deletions(-)

Quoted 9 lines · Thomas Pedersen

Some boards (eg. qcom IPQ8064 based) Need these changes to avoid a spurious watchdog reset.

Matthew McClintock (3):
watchdog: qcom: update device tree bindings
watchdog: qcom: add option for standalone watchdog not in timer
block
watchdog: qcom: configure BARK time in addition to BITE time

Quoted 5 lines

  .../devicetree/bindings/watchdog/qcom-wdt.txt      |  5 +-
  drivers/watchdog/qcom-wdt.c                        | 78 
++++++++++++++++------
  2 files changed, 59 insertions(+), 24 deletions(-)

General comment: If you don't plan to address the comments made to the previous submission, please at least let the reviewers know what you are doing and why. Also, a note indicating that this is a resend would be helpful for those who did not see the original series.