[PATCH 06/17] watchdog: qcom: update device tree bindings

[PATCH 06/17] watchdog: qcom: update device tree bindings

[Matthew McClintock]

Update the compatible string to align with driver

CC: linux-watchdog@vger.kernel.org
Signed-off-by: Matthew McClintock <mmcclint@codeaurora.org>
---
 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
--- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
@@ -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"
 
 - reg : shall contain base register location and length
 - clocks : shall contain the input clock
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 06/17] watchdog: qcom: update device tree bindings

[Stephen Boyd]

On 03/23/2016 03:05 PM, Matthew McClintock wrote:
> Update the compatible string to align with driver
>
> CC: linux-watchdog@vger.kernel.org
> Signed-off-by: Matthew McClintock <mmcclint@codeaurora.org>

I had a patch similar to this before
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/325235.html


> ---
>  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
> --- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> @@ -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"
>  
>  - reg : shall contain base register location and length
>  - clocks : shall contain the input clock

No example update?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 06/17] watchdog: qcom: update device tree bindings

[Matthew McClintock]

> On Mar 23, 2016, at 5:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> 
> On 03/23/2016 03:05 PM, Matthew McClintock wrote:
>> Update the compatible string to align with driver
>> 
>> CC: linux-watchdog@vger.kernel.org
>> Signed-off-by: Matthew McClintock <mmcclint@codeaurora.org>
> 
> I had a patch similar to this before
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/325235.html

Whoops, your patch looks better. Will drop this one.

-M

Re: [PATCH 06/17] watchdog: qcom: update device tree bindings

[Rob Herring]

On Wed, Mar 23, 2016 at 05:05:01PM -0500, Matthew McClintock wrote:
> Update the compatible string to align with driver
> 
> CC: linux-watchdog@vger.kernel.org
> Signed-off-by: Matthew McClintock <mmcclint@codeaurora.org>
> ---
>  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
> --- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> @@ -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"

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

Rob

[PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Matthew McClintock]

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 <mmcclint@codeaurora.org>
---
 drivers/watchdog/qcom-wdt.c | 69 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 20 deletions(-)

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 20563cc..e46f18d 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -19,17 +19,37 @@
 #include <linux/platform_device.h>
 #include <linux/watchdog.h>
 
-#define WDT_RST		0x38
-#define WDT_EN		0x40
-#define WDT_BITE_TIME	0x5C
+enum wdt_reg {
+	WDT_RST,
+	WDT_EN,
+	WDT_BITE_TIME,
+};
+
+static const u32 reg_offset_data_apcs_tmr[] = {
+	[WDT_RST] = 0x38,
+	[WDT_EN] = 0x40,
+	[WDT_BITE_TIME] = 0x5C,
+};
+
+static const u32 reg_offset_data_kpss[] = {
+	[WDT_RST] = 0x4,
+	[WDT_EN] = 0x8,
+	[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)
 {
@@ -40,10 +60,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;
 }
 
@@ -51,7 +71,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;
 }
 
@@ -59,7 +79,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;
 }
 
@@ -82,10 +102,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.
@@ -112,14 +132,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;
@@ -170,6 +205,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 'timeout-sec' unspecified in devicetree, assume a 30 second
@@ -202,13 +238,6 @@ static int qcom_wdt_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id qcom_wdt_of_table[] = {
-	{ .compatible = "qcom,kpss-timer" },
-	{ .compatible = "qcom,scss-timer" },
-	{ },
-};
-MODULE_DEVICE_TABLE(of, qcom_wdt_of_table);
-
 static struct platform_driver qcom_watchdog_driver = {
 	.probe	= qcom_wdt_probe,
 	.remove	= qcom_wdt_remove,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Stephen Boyd]

On 03/23, Matthew McClintock wrote:
> @@ -202,13 +238,6 @@ static int qcom_wdt_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static const struct of_device_id qcom_wdt_of_table[] = {
> -	{ .compatible = "qcom,kpss-timer" },
> -	{ .compatible = "qcom,scss-timer" },
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(of, qcom_wdt_of_table);
> -

Leave this here and use of_device_get_match_data() in probe
instead.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Guenter Roeck]

On Wed, Mar 23, 2016 at 05:05:02PM -0500, Matthew McClintock wrote:
> 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 <mmcclint@codeaurora.org>
> ---
>  drivers/watchdog/qcom-wdt.c | 69 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 20563cc..e46f18d 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -19,17 +19,37 @@
>  #include <linux/platform_device.h>
>  #include <linux/watchdog.h>
>  
> -#define WDT_RST		0x38
> -#define WDT_EN		0x40
> -#define WDT_BITE_TIME	0x5C
> +enum wdt_reg {
> +	WDT_RST,
> +	WDT_EN,
> +	WDT_BITE_TIME,
> +};
> +
> +static const u32 reg_offset_data_apcs_tmr[] = {
> +	[WDT_RST] = 0x38,
> +	[WDT_EN] = 0x40,
> +	[WDT_BITE_TIME] = 0x5C,
> +};
> +
> +static const u32 reg_offset_data_kpss[] = {
> +	[WDT_RST] = 0x4,
> +	[WDT_EN] = 0x8,

Does this work ? In the datasheet I have in front of me (APQ8064), the watchdog
at this address uses different bits. At address 0x40 (eg GSS_A5_APCS_WDT0_EN),
bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg
LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is
undefined. Or does "qcom,kpss-standalone" refer to some other watchdog ?

Thanks,
Guenter

> +	[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)
>  {
> @@ -40,10 +60,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;
>  }
>  
> @@ -51,7 +71,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;
>  }
>  
> @@ -59,7 +79,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;
>  }
>  
> @@ -82,10 +102,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.
> @@ -112,14 +132,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;
> @@ -170,6 +205,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 'timeout-sec' unspecified in devicetree, assume a 30 second
> @@ -202,13 +238,6 @@ static int qcom_wdt_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static const struct of_device_id qcom_wdt_of_table[] = {
> -	{ .compatible = "qcom,kpss-timer" },
> -	{ .compatible = "qcom,scss-timer" },
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(of, qcom_wdt_of_table);
> -
>  static struct platform_driver qcom_watchdog_driver = {
>  	.probe	= qcom_wdt_probe,
>  	.remove	= qcom_wdt_remove,
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Matthew McClintock]

> On Mar 25, 2016, at 11:23 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> -#define WDT_RST		0x38
>> -#define WDT_EN		0x40
>> -#define WDT_BITE_TIME	0x5C
>> +enum wdt_reg {
>> +	WDT_RST,
>> +	WDT_EN,
>> +	WDT_BITE_TIME,
>> +};
>> +
>> +static const u32 reg_offset_data_apcs_tmr[] = {
>> +	[WDT_RST] = 0x38,
>> +	[WDT_EN] = 0x40,
>> +	[WDT_BITE_TIME] = 0x5C,
>> +};
>> +
>> +static const u32 reg_offset_data_kpss[] = {
>> +	[WDT_RST] = 0x4,
>> +	[WDT_EN] = 0x8,
> 
> Does this work ? In the datasheet I have in front of me (APQ8064), the watchdog
> at this address uses different bits. At address 0x40 (eg GSS_A5_APCS_WDT0_EN),

0x40 is acps_tmr, and looks fine.

> bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg
> LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is
> undefined.

I honestly don’t see anything at 0x8 for either blocks that looks like this. For the new block bit 0 is enabling and bit 1 enabled interrupts.

> Or does "qcom,kpss-standalone" refer to some other watchdog ?

APQ8064 would be the apcs_tmr block variant which is unchanged. MSM8916 as well as IPQ4019 would use the new kpss variant.

I went with block names I found internally here, but I will be the first to admit I am terrible at names. The old block name for APQ was CPU0_ACPS_TMR (where really the watchdog is a subset of a timer block), and on the IPQ4019 it’s called APCS_KPSS_WDT and it’s really just a watchdog block.

I kept the same driver because the register’s currently in use were compatible. By the way, I tested this on an IPQ806x and IPQ4019 both new and old blocks.

Let me know if you need more details.

-M

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Guenter Roeck]

On Mon, Mar 28, 2016 at 11:55:28AM -0500, Matthew McClintock wrote:
> 
> > On Mar 25, 2016, at 11:23 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> >> -#define WDT_RST		0x38
> >> -#define WDT_EN		0x40
> >> -#define WDT_BITE_TIME	0x5C
> >> +enum wdt_reg {
> >> +	WDT_RST,
> >> +	WDT_EN,
> >> +	WDT_BITE_TIME,
> >> +};
> >> +
> >> +static const u32 reg_offset_data_apcs_tmr[] = {
> >> +	[WDT_RST] = 0x38,
> >> +	[WDT_EN] = 0x40,
> >> +	[WDT_BITE_TIME] = 0x5C,
> >> +};
> >> +
> >> +static const u32 reg_offset_data_kpss[] = {
> >> +	[WDT_RST] = 0x4,
> >> +	[WDT_EN] = 0x8,
> > 
> > Does this work ? In the datasheet I have in front of me (APQ8064), the watchdog
> > at this address uses different bits. At address 0x40 (eg GSS_A5_APCS_WDT0_EN),
> 
> 0x40 is acps_tmr, and looks fine.
> 
> > bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg
> > LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is
> > undefined.
> 
> I honestly don’t see anything at 0x8 for either blocks that looks like this. For the new block bit 0 is enabling and bit 1 enabled interrupts.
> 
That is from the APQ8064 datasheet. 

> > Or does "qcom,kpss-standalone" refer to some other watchdog ?
> 
> APQ8064 would be the apcs_tmr block variant which is unchanged. MSM8916 as well as IPQ4019 would use the new kpss variant.
> 
Unfortunately I don't have access to those datasheets.

> I went with block names I found internally here, but I will be the first to admit I am terrible at names. The old block name for APQ was CPU0_ACPS_TMR (where really the watchdog is a subset of a timer block), and on the IPQ4019 it’s called APCS_KPSS_WDT and it’s really just a watchdog block.
> 
> I kept the same driver because the register’s currently in use were compatible. By the way, I tested this on an IPQ806x and IPQ4019 both new and old blocks.
> 

The property name should probably be something like 'qcom,kpss-wdt'
(or 'qcom,kpss-watchdog' ?), possibly in addition to 'qcom,kpss-ipq4019-wdt'
and 'qcom,kpss-msm8916-wdt'.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Matthew McClintock]

On Mar 28, 2016, at 1:13 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>>> bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg
>>> LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is
>>> undefined.
>> 
>> I honestly don’t see anything at 0x8 for either blocks that looks like this. For the new block bit 0 is enabling and bit 1 enabled interrupts.
>> 
> That is from the APQ8064 datasheet. 

So taken from the timer offset 0x0208A000 I just have a generic counter register CPU0_APCS_GPT0_CNT at 0x8

What doc are you looking at?

-M

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Guenter Roeck]

On Mon, Mar 28, 2016 at 03:40:58PM -0500, Matthew McClintock wrote:
> On Mar 28, 2016, at 1:13 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> >>> bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg
> >>> LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is
> >>> undefined.
> >> 
> >> I honestly don’t see anything at 0x8 for either blocks that looks like this. For the new block bit 0 is enabling and bit 1 enabled interrupts.
> >> 
> > That is from the APQ8064 datasheet. 
> 
> So taken from the timer offset 0x0208A000 I just have a generic counter register CPU0_APCS_GPT0_CNT at 0x8
> 
> What doc are you looking at?
> 
"Qualcomm Snapdragon 600 Processor APQ8064 Hardware Register Description"

It is available for download from the Qualcomm web site.

See chapter 12.10.3, "Watchdog timer registers". The register block is at
0x28882000. Registers are almost the same, except for the offset and the
definition of the bits in the enable register.

LPASS is "Low Power Audio Subsystem". Maybe it has its own watchdog.

Guenter

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Matthew McClintock]

On Mar 28, 2016, at 4:56 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> So taken from the timer offset 0x0208A000 I just have a generic counter register CPU0_APCS_GPT0_CNT at 0x8
>> 
>> What doc are you looking at?
>> 
> "Qualcomm Snapdragon 600 Processor APQ8064 Hardware Register Description"
> 
> It is available for download from the Qualcomm web site.
> 
> See chapter 12.10.3, "Watchdog timer registers". The register block is at
> 0x28882000. Registers are almost the same, except for the offset and the
> definition of the bits in the enable register.
> 
> LPASS is "Low Power Audio Subsystem". Maybe it has its own watchdog.

This block is here:

11.15 KPSS CPU0 Timer Registers (0x0208A000 CPU0_APCS_TMR_BASE)

-M--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/17] watchdog: qcom: configure BARK time in addition to BITE time

[Matthew McClintock]

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 <mmcclint@codeaurora.org>
---
 drivers/watchdog/qcom-wdt.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index e46f18d..53f57c3 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -22,18 +22,21 @@
 enum wdt_reg {
 	WDT_RST,
 	WDT_EN,
+	WDT_BARK_TIME,
 	WDT_BITE_TIME,
 };
 
 static const u32 reg_offset_data_apcs_tmr[] = {
 	[WDT_RST] = 0x38,
 	[WDT_EN] = 0x40,
+	[WDT_BARK_TIME] = 0x4C,
 	[WDT_BITE_TIME] = 0x5C,
 };
 
 static const u32 reg_offset_data_kpss[] = {
 	[WDT_RST] = 0x4,
 	[WDT_EN] = 0x8,
+	[WDT_BARK_TIME] = 0x10,
 	[WDT_BITE_TIME] = 0x14,
 };
 
@@ -62,6 +65,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;
@@ -104,6 +108,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));
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 08/17] watchdog: qcom: configure BARK time in addition to BITE time

[Stephen Boyd]

On 03/23, Matthew McClintock wrote:
> 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.
> 

Why isn't TZ configuring the bark time to what it wants? I'm lost
why we have to do this for them.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 08/17] watchdog: qcom: configure BARK time in addition to BITE time

[Matthew McClintock]

On Mar 23, 2016, at 5:42 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> 
> On 03/23, Matthew McClintock wrote:
>> 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.
>> 
> 
> Why isn't TZ configuring the bark time to what it wants? I'm lost
> why we have to do this for them.

So it was done like this to ensure we had a valid upgrade. The bootloader is using the watchdog to ensure the system is bootable and if not it will revert back to the working images.

Bottom line is, for some versions of TZ out there, if we enable watchdog coming out of boot the bark time is already configured by the boot loader and TZ is configured to intercept this interrupt and do some register saving (for crashdump) and we end up getting a watchdog reset during boot.

It’s even a little more complex, because in order for the TZ to save the registers you need to pad the BITE time a bit higher than the BARK time, but I was leaving that for another day.

-M

Re: [PATCH 08/17] watchdog: qcom: configure BARK time in addition to BITE time

[Guenter Roeck]

On Thu, Mar 24, 2016 at 10:46:42AM -0500, Matthew McClintock wrote:
> On Mar 23, 2016, at 5:42 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > 
> > On 03/23, Matthew McClintock wrote:
> >> 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.
> >> 
> > 
> > Why isn't TZ configuring the bark time to what it wants? I'm lost
> > why we have to do this for them.
> 
> So it was done like this to ensure we had a valid upgrade. The bootloader is using the watchdog to ensure the system is bootable and if not it will revert back to the working images.
> 
> Bottom line is, for some versions of TZ out there, if we enable watchdog coming out of boot the bark time is already configured by the boot loader and TZ is configured to intercept this interrupt and do some register saving (for crashdump) and we end up getting a watchdog reset during boot.
> 
> It’s even a little more complex, because in order for the TZ to save the registers you need to pad the BITE time a bit higher than the BARK time, but I was leaving that for another day.
> 
Sounds like an op[timal target for using pretimeout ?

Guenter

Re: [PATCH 08/17] watchdog: qcom: configure BARK time in addition to BITE time

[Matthew McClintock]

On Mar 24, 2016, at 11:17 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>>> Why isn't TZ configuring the bark time to what it wants? I'm lost
>>> why we have to do this for them.
>> 
>> So it was done like this to ensure we had a valid upgrade. The bootloader is using the watchdog to ensure the system is bootable and if not it will revert back to the working images.
>> 
>> Bottom line is, for some versions of TZ out there, if we enable watchdog coming out of boot the bark time is already configured by the boot loader and TZ is configured to intercept this interrupt and do some register saving (for crashdump) and we end up getting a watchdog reset during boot.
>> 
>> It’s even a little more complex, because in order for the TZ to save the registers you need to pad the BITE time a bit higher than the BARK time, but I was leaving that for another day.
>> 
> Sounds like an op[timal target for using pretimeout ?

So the bark is basically a pretimeout, sure I think that will work. We can configure it to be off by default.

Thanks for the heads up, I’ll take a look.

-M

Re: [PATCH 08/17] watchdog: qcom: configure BARK time in addition to BITE time

[Guenter Roeck]

On 03/23/2016 03:05 PM, Matthew McClintock wrote:
> 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 <mmcclint@codeaurora.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/qcom-wdt.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index e46f18d..53f57c3 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -22,18 +22,21 @@
>   enum wdt_reg {
>   	WDT_RST,
>   	WDT_EN,
> +	WDT_BARK_TIME,
>   	WDT_BITE_TIME,
>   };
>
>   static const u32 reg_offset_data_apcs_tmr[] = {
>   	[WDT_RST] = 0x38,
>   	[WDT_EN] = 0x40,
> +	[WDT_BARK_TIME] = 0x4C,
>   	[WDT_BITE_TIME] = 0x5C,
>   };
>
>   static const u32 reg_offset_data_kpss[] = {
>   	[WDT_RST] = 0x4,
>   	[WDT_EN] = 0x8,
> +	[WDT_BARK_TIME] = 0x10,
>   	[WDT_BITE_TIME] = 0x14,
>   };
>
> @@ -62,6 +65,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;
> @@ -104,6 +108,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));
>
>

[PATCH 09/17] watchdog: qcom: add kpss-standalone to device tree binding

[Matthew McClintock]

Update the compatible string to add new device tree binding

CC: linux-watchdog@vger.kernel.org
Signed-off-by: Matthew McClintock <mmcclint@codeaurora.org>
---
 Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
index 60bb2f98..45b37cf 100644
--- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
@@ -6,6 +6,7 @@ Required properties :
 
 			"qcom,kpss-timer"
 			"qcom,scss-timer"
+			"qcom,kpss-standalone"
 
 - reg : shall contain base register location and length
 - clocks : shall contain the input clock
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 09/17] watchdog: qcom: add kpss-standalone to device tree binding

[Rob Herring]

On Wed, Mar 23, 2016 at 05:05:04PM -0500, Matthew McClintock wrote:
> Update the compatible string to add new device tree binding
> 
> CC: linux-watchdog@vger.kernel.org
> Signed-off-by: Matthew McClintock <mmcclint@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> index 60bb2f98..45b37cf 100644
> --- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> @@ -6,6 +6,7 @@ Required properties :
>  
>  			"qcom,kpss-timer"
>  			"qcom,scss-timer"
> +			"qcom,kpss-standalone"

What SoC(s) is this in. Use SoC specific compatible strings please.

Rob

Re: [PATCH 09/17] watchdog: qcom: add kpss-standalone to device tree binding

[Matthew McClintock]

On Mar 25, 2016, at 9:15 AM, Rob Herring <robh@kernel.org> wrote:
> 
> On Wed, Mar 23, 2016 at 05:05:04PM -0500, Matthew McClintock wrote:
>> Update the compatible string to add new device tree binding
>> 
>> CC: linux-watchdog@vger.kernel.org
>> Signed-off-by: Matthew McClintock <mmcclint@codeaurora.org>
>> ---
>> Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 1 +
>> 1 file changed, 1 insertion(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
>> index 60bb2f98..45b37cf 100644
>> --- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
>> @@ -6,6 +6,7 @@ Required properties :
>> 
>> 			"qcom,kpss-timer"
>> 			"qcom,scss-timer"
>> +			"qcom,kpss-standalone"
> 
> What SoC(s) is this in. Use SoC specific compatible strings please.

So ipq4019 wins the race because we are the first to try to enable watchdog for this block?

qcom,kpss-ipq4019 ?

-M

Re: [PATCH 09/17] watchdog: qcom: add kpss-standalone to device tree binding

[Rob Herring]

On Mon, Mar 28, 2016 at 12:02 PM, Matthew McClintock
<mmcclint@codeaurora.org> wrote:
> On Mar 25, 2016, at 9:15 AM, Rob Herring <robh@kernel.org> wrote:
>>
>> On Wed, Mar 23, 2016 at 05:05:04PM -0500, Matthew McClintock wrote:
>>> Update the compatible string to add new device tree binding
>>>
>>> CC: linux-watchdog@vger.kernel.org
>>> Signed-off-by: Matthew McClintock <mmcclint@codeaurora.org>
>>> ---
>>> Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
>>> index 60bb2f98..45b37cf 100644
>>> --- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
>>> +++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
>>> @@ -6,6 +6,7 @@ Required properties :
>>>
>>>                      "qcom,kpss-timer"
>>>                      "qcom,scss-timer"
>>> +                    "qcom,kpss-standalone"
>>
>> What SoC(s) is this in. Use SoC specific compatible strings please.
>
> So ipq4019 wins the race because we are the first to try to enable watchdog for this block?

Yep, that's how it is supposed to work. Newer chips claim
compatibility with older ones.

> qcom,kpss-ipq4019 ?

Yes, but generally <vendor>,<soc>-<block> is preferred order.

Rob

Re: [PATCH 09/17] watchdog: qcom: add kpss-standalone to device tree binding

[Guenter Roeck]

On Mon, Mar 28, 2016 at 12:02:47PM -0500, Matthew McClintock wrote:
> On Mar 25, 2016, at 9:15 AM, Rob Herring <robh@kernel.org> wrote:
> > 
> > On Wed, Mar 23, 2016 at 05:05:04PM -0500, Matthew McClintock wrote:
> >> Update the compatible string to add new device tree binding
> >> 
> >> CC: linux-watchdog@vger.kernel.org
> >> Signed-off-by: Matthew McClintock <mmcclint@codeaurora.org>
> >> ---
> >> Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 1 +
> >> 1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> >> index 60bb2f98..45b37cf 100644
> >> --- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> >> +++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> >> @@ -6,6 +6,7 @@ Required properties :
> >> 
> >> 			"qcom,kpss-timer"
> >> 			"qcom,scss-timer"
> >> +			"qcom,kpss-standalone"
> > 
> > What SoC(s) is this in. Use SoC specific compatible strings please.
> 
> So ipq4019 wins the race because we are the first to try to enable watchdog for this block?
> 
> qcom,kpss-ipq4019 ?
> 
It is a dedicated watchdog block, isn't it ? "qcom,kpss-ipq4019" would not
refer to a specific block. Devicetree maintainers may have a better idea,
but it seems to me that there should be 'wdt' or 'watchdog' in the property
name.

Guenter

Re: [PATCH 09/17] watchdog: qcom: add kpss-standalone to device tree binding

[Matthew McClintock]

On Mar 28, 2016, at 1:15 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>>> What SoC(s) is this in. Use SoC specific compatible strings please.
>> 
>> So ipq4019 wins the race because we are the first to try to enable watchdog for this block?
>> 
>> qcom,kpss-ipq4019 ?
>> 
> It is a dedicated watchdog block, isn't it ? "qcom,kpss-ipq4019" would not
> refer to a specific block. Devicetree maintainers may have a better idea,
> but it seems to me that there should be 'wdt' or 'watchdog' in the property
> name.

Sounds fine to me, if no one has any other comments I’ll use this one in the next spin of the patch.

-M--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html