Skip to contentSkip to conversation

watchdog: qcom-wdt: add option to skip pretimeout

PATCH In review Ansuel Smith 3 participants 11 messages Feb 4, 2020 – Feb 6, 2020

Subsystems (3)
Device tree bindings Arm Qualcomm mailing list Watchdog device drivers
lore
Share & tools
lore

Ansuel Smith

author
#0 lore raw

PATCH 1/3 watchdog: qcom-wdt: add option to skip pretimeout

Some platform like ipq806x doesn't support pretimeout. As the driver check if there are available interrupts and ipq806x use msm-timer that require interrupts, the watchdog fail to probe as request_irq tries to use a ppi interrupt. Add an option to skip pretimeout setup and use the normal watchdog probe.

Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/watchdog/qcom-wdt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/watchdog/qcom-wdt.c +4 -1
index a494543d3ae1..e689e97e883e 100644
@@ -189,6 +189,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
 	u32 percpu_offset;
 	int irq, ret;
 	struct clk *clk;
+	bool nopretimeout;
 
 	regs = of_device_get_match_data(dev);
 	if (!regs) {

@@ -204,6 +205,8 @@ static int qcom_wdt_probe(struct platform_device *pdev)
 	if (!res)
 		return -ENOMEM;
 
+	nopretimeout = of_property_read_bool(np, "no-pretimeout");
+
 	/* We use CPU0's DGT for the watchdog */
 	if (of_property_read_u32(np, "cpu-offset", &percpu_offset))
 		percpu_offset = 0;

@@ -247,7 +250,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
 
 	/* check if there is pretimeout support */
 	irq = platform_get_irq(pdev, 0);
-	if (irq > 0) {
+	if (!nopretimeout && irq > 0) {
 		ret = devm_request_irq(dev, irq, qcom_wdt_isr,
 				       IRQF_TRIGGER_RISING,
 				       "wdt_bark", &wdt->wdd);
Signature · 1 line
2.24.0
author
#1 lore raw

PATCH 2/3 Documentation: watchdog: qcom-wdt: Remove deprecated compatible

"qcom,kpss-wdt-msm8960"
"qcom,kpss-wdt-apq8064"
"qcom,kpss-wdt-ipq8064"
"qcom,kpss-wdt-ipq4019"

and deprectaed and not used in the driver code at all. Drop them and fix the example.

Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
Documentation/devicetree/bindings/watchdog/qcom-wdt.txt +1 -5
index 41aeaa2ff0f8..33081bd33637 100644
@@ -4,10 +4,6 @@ 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-wdt-ipq4019"
 			"qcom,kpss-timer"
 			"qcom,scss-timer"
 			"qcom,kpss-wdt"

@@ -21,7 +17,7 @@ Optional properties :
 
 Example:
 	watchdog@208a038 {
-		compatible = "qcom,kpss-wdt-ipq8064";
+		compatible = "qcom,kpss-timer";
 		reg = <0x0208a038 0x40>;
 		clocks = <&sleep_clk>;
 		timeout-sec = <10>;
Signature · 1 line
2.24.0
maintainer
#2 lore raw

Re: [PATCH 2/3] Documentation: watchdog: qcom-wdt: Remove deprecated compatible

Quoted 8 lines · Ansuel Smith

"qcom,kpss-wdt-msm8960"
"qcom,kpss-wdt-apq8064"
"qcom,kpss-wdt-ipq8064"
"qcom,kpss-wdt-ipq4019"

and deprectaed and not used in the driver code at all. Drop them and fix the example.

Rob may correct me, but I don't think you can just remove deprecated properties. It doesn't matter if the driver supports it or not; after all, DT property descriptions are supposed to be OS independent.

Guenter

Quoted 32 lines
Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
Documentation/devicetree/bindings/watchdog/qcom-wdt.txt +1 -5
index 41aeaa2ff0f8..33081bd33637 100644
@@ -4,10 +4,6 @@ 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-wdt-ipq4019"
 			"qcom,kpss-timer"
 			"qcom,scss-timer"
 			"qcom,kpss-wdt"

@@ -21,7 +17,7 @@ Optional properties :
 
 Example:
 	watchdog@208a038 {
-		compatible = "qcom,kpss-wdt-ipq8064";
+		compatible = "qcom,kpss-timer";
 		reg = <0x0208a038 0x40>;
 		clocks = <&sleep_clk>;
 		timeout-sec = <10>;
Signature · 1 line
2.24.0
maintainer
#3 lore raw

Re: [PATCH 2/3] Documentation: watchdog: qcom-wdt: Remove deprecated compatible

Quoted 6 lines · Guenter Roeck
Quoted 5 lines · Ansuel Smith

"qcom,kpss-wdt-msm8960"
"qcom,kpss-wdt-apq8064"
"qcom,kpss-wdt-ipq8064"
"qcom,kpss-wdt-ipq4019"

Quoted 7 lines
Quoted 3 lines

and deprectaed and not used in the driver code at all. Drop them and fix the example.

Rob may correct me, but I don't think you can just remove deprecated properties. It doesn't matter if the driver supports it or not; after all, DT property descriptions are supposed to be OS independent.

Right.

Also, there's a conversion of this to schema under review, so it will need to be refactored if not dropped.

Rob

author
#4 lore raw

PATCH 3/3 Documentation: watchdog: qcom-wdt: add new no-pretimeout option

In reply to Ansuel Smith

Add description for new no-pretimeout function to force legacy probe if any interrupt is defined.

Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 2 ++
 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/watchdog/qcom-wdt.txt +2 -0
index 33081bd33637..01978bff74ee 100644
@@ -14,6 +14,8 @@ Required properties :
 Optional properties :
 - timeout-sec : shall contain the default watchdog timeout in seconds,
                 if unset, the default timeout is 30 seconds
+- no-pretimeout : shall be set if the platform have some interrupts
+                  defined in the node but doesn't support pretimeout
 
 Example:
 	watchdog@208a038 {
Signature · 1 line
2.24.0
maintainer
#5 lore raw

Re: [PATCH 3/3] Documentation: watchdog: qcom-wdt: add new no-pretimeout option

Quoted 13 lines · Ansuel Smith

Add description for new no-pretimeout function to force legacy probe if any interrupt is defined.

Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 2 ++
 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/watchdog/qcom-wdt.txt +0 -0
index 33081bd33637..01978bff74ee 100644

Documentation/devicetree/bindings/watchdog/qcom-wdt.txt Required properties : · line 18

Quoted 5 lines
Documentation/devicetree/bindings/watchdog/qcom-wdt.txt +2 -0
@@ -14,6 +14,8 @@ Required properties :
 Optional properties :
 - timeout-sec : shall contain the default watchdog timeout in seconds,
                 if unset, the default timeout is 30 seconds
+- no-pretimeout : shall be set if the platform have some interrupts
+                  defined in the node but doesn't support pretimeout

As mentioned in the other patch, why specify an interrupt in the first place in that situation ?

Guenter

maintainer
#6 lore raw
In reply to Ansuel Smith
Quoted 33 lines · Ansuel Smith

Some platform like ipq806x doesn't support pretimeout. As the driver check if there are available interrupts and ipq806x use msm-timer that require interrupts, the watchdog fail to probe as request_irq tries to use a ppi interrupt. Add an option to skip pretimeout setup and use the normal watchdog probe.

Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/watchdog/qcom-wdt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/watchdog/qcom-wdt.c +3 -0
index a494543d3ae1..e689e97e883e 100644
@@ -189,6 +189,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
 	u32 percpu_offset;
 	int irq, ret;
 	struct clk *clk;
+	bool nopretimeout;
 
 	regs = of_device_get_match_data(dev);
 	if (!regs) {

@@ -204,6 +205,8 @@ static int qcom_wdt_probe(struct platform_device *pdev)
 	if (!res)
 		return -ENOMEM;
 
+	nopretimeout = of_property_read_bool(np, "no-pretimeout");
+
 	/* We use CPU0's DGT for the watchdog */
 	if (of_property_read_u32(np, "cpu-offset", &percpu_offset))
 		percpu_offset = 0;

drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device *pdev) · line 253

Quoted 5 lines
drivers/watchdog/qcom-wdt.c +1 -1
@@ -247,7 +250,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
 
 	/* check if there is pretimeout support */
 	irq = platform_get_irq(pdev, 0);
-	if (irq > 0) {
+	if (!nopretimeout && irq > 0) {

That is unnecessary; such platforms should simply not provide an interrupt. Or, in other words, what is the point of assigning an interrupt to be used for pretimeout if the platform doesn't support it ? And then to add yet another attribute to tell the driver not to use it ?

Guenter

Quoted 6 lines
drivers/watchdog/qcom-wdt.c +0 -0
 		ret = devm_request_irq(dev, irq, qcom_wdt_isr,
 				       IRQF_TRIGGER_RISING,
 				       "wdt_bark", &wdt->wdd);
Signature · 1 line
2.24.0
author
#7 lore raw

R: [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout

If something like this is used, msm-timer require interrupts. Without this configuration, the device is unbootable as the system froze on system bootup.

timer@200a000 {
			compatible = "qcom,kpss-timer", "qcom,msm-timer";
			interrupts = <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 5 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>;
			no-pretimeout;
			reg = <0x0200a000 0x100>;
			clock-frequency = <25000000>,
					  <32768>;
			clocks = <&sleep_clk>;
			clock-names = "sleep";
			cpu-offset = <0x80000>;
		};
Quoted 38 lines · Ansuel Smith

drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device

Quoted 16 lines

Some platform like ipq806x doesn't support pretimeout. As the driver check if there are available interrupts and ipq806x use msm-timer that require interrupts, the watchdog fail to probe as request_irq tries to use a ppi interrupt. Add an option to skip pretimeout setup and use the normal watchdog probe.

Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/watchdog/qcom-wdt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/watchdog/qcom-wdt.c +0 -0
index a494543d3ae1..e689e97e883e 100644
@@ -189,6 +189,7 @@ static int qcom_wdt_probe(struct platform_device

*pdev)

drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device

Quoted 8 lines
drivers/watchdog/qcom-wdt.c +1 -0
 	u32 percpu_offset;
 	int irq, ret;
 	struct clk *clk;
+	bool nopretimeout;
 
 	regs = of_device_get_match_data(dev);
 	if (!regs) {

@@ -204,6 +205,8 @@ static int qcom_wdt_probe(struct platform_device

*pdev)

drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device

Quoted 9 lines
drivers/watchdog/qcom-wdt.c +2 -0
 	if (!res)
 		return -ENOMEM;
 
+	nopretimeout = of_property_read_bool(np, "no-pretimeout");
+
 	/* We use CPU0's DGT for the watchdog */
 	if (of_property_read_u32(np, "cpu-offset", &percpu_offset))
 		percpu_offset = 0;

@@ -247,7 +250,7 @@ static int qcom_wdt_probe(struct platform_device

*pdev)

Quoted 1 line
Quoted 6 lines

drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device *pdev) · line 253

Quoted 4 lines
drivers/watchdog/qcom-wdt.c +1 -1
 	/* check if there is pretimeout support */
 	irq = platform_get_irq(pdev, 0);
-	if (irq > 0) {
+	if (!nopretimeout && irq > 0) {

That is unnecessary; such platforms should simply not provide an

interrupt.

Quoted 12 lines

Or, in other words, what is the point of assigning an interrupt to be used for pretimeout if the platform doesn't support it ? And then to add yet another attribute to tell the driver not to use it ?

Guenter

Quoted 6 lines
drivers/watchdog/qcom-wdt.c +0 -1
 		ret = devm_request_irq(dev, irq, qcom_wdt_isr,
 				       IRQF_TRIGGER_RISING,
 				       "wdt_bark", &wdt->wdd);
--

2.24.0

maintainer
#8 lore raw

Re: R: [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout

Quoted 16 lines · On Tue, Feb 04, 2020 at 05:16:34PM +0100, [email protected] wrote:

If something like this is used, msm-timer require interrupts. Without this configuration, the device is unbootable as the system froze on system bootup.

timer@200a000 {
			compatible = "qcom,kpss-timer", "qcom,msm-timer";
			interrupts = <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 5 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>;
Quoted 9 lines
			no-pretimeout;
			reg = <0x0200a000 0x100>;
			clock-frequency = <25000000>,
					  <32768>;
			clocks = <&sleep_clk>;
			clock-names = "sleep";
			cpu-offset = <0x80000>;
		};

I think this is all wrong; the new property shows up in a node which is completely unrelated to a watchdog. Maybe it wasn't such a good idea to tie the watchdog to the timer node. At the very least, the situation should be handled in the driver via of_table flags. If the situation can't be handled that way, something is even more wrong. In that case it might be better to revert commit 36375491a439 until that is sorted out properly.

Guenter

Quoted 58 lines
Quoted 38 lines · Ansuel Smith

drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device

Quoted 16 lines

Some platform like ipq806x doesn't support pretimeout. As the driver check if there are available interrupts and ipq806x use msm-timer that require interrupts, the watchdog fail to probe as request_irq tries to use a ppi interrupt. Add an option to skip pretimeout setup and use the normal watchdog probe.

Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/watchdog/qcom-wdt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/watchdog/qcom-wdt.c +0 -0
index a494543d3ae1..e689e97e883e 100644
@@ -189,6 +189,7 @@ static int qcom_wdt_probe(struct platform_device

*pdev)

drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device

Quoted 8 lines
drivers/watchdog/qcom-wdt.c +1 -0
 	u32 percpu_offset;
 	int irq, ret;
 	struct clk *clk;
+	bool nopretimeout;
 
 	regs = of_device_get_match_data(dev);
 	if (!regs) {

@@ -204,6 +205,8 @@ static int qcom_wdt_probe(struct platform_device

*pdev)

drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device

Quoted 9 lines
drivers/watchdog/qcom-wdt.c +2 -0
 	if (!res)
 		return -ENOMEM;
 
+	nopretimeout = of_property_read_bool(np, "no-pretimeout");
+
 	/* We use CPU0's DGT for the watchdog */
 	if (of_property_read_u32(np, "cpu-offset", &percpu_offset))
 		percpu_offset = 0;

@@ -247,7 +250,7 @@ static int qcom_wdt_probe(struct platform_device

*pdev)

Quoted 1 line
Quoted 6 lines

drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device *pdev) · line 253

Quoted 4 lines
drivers/watchdog/qcom-wdt.c +1 -1
 	/* check if there is pretimeout support */
 	irq = platform_get_irq(pdev, 0);
-	if (irq > 0) {
+	if (!nopretimeout && irq > 0) {

That is unnecessary; such platforms should simply not provide an

interrupt.

Quoted 12 lines

Or, in other words, what is the point of assigning an interrupt to be used for pretimeout if the platform doesn't support it ? And then to add yet another attribute to tell the driver not to use it ?

Guenter

Quoted 6 lines
drivers/watchdog/qcom-wdt.c +0 -1
 		ret = devm_request_irq(dev, irq, qcom_wdt_isr,
 				       IRQF_TRIGGER_RISING,
 				       "wdt_bark", &wdt->wdd);
--

2.24.0

author
#9 lore raw

R: R: [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout

Quoted 2 lines · On Tue, Feb 04, 2020 at 05:16:34PM +0100, [email protected] wrote:
Quoted 1 line

If something like this is used, msm-timer require interrupts. Without

this

Quoted 25 lines
Quoted 15 lines

configuration, the device is unbootable as the system froze on system bootup.

timer@200a000 {
			compatible = "qcom,kpss-timer", "qcom,msm-timer";
			interrupts = <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 5 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>;
Quoted 9 lines
			no-pretimeout;
			reg = <0x0200a000 0x100>;
			clock-frequency = <25000000>,
					  <32768>;
			clocks = <&sleep_clk>;
			clock-names = "sleep";
			cpu-offset = <0x80000>;
		};
Quoted 9 lines

I think this is all wrong; the new property shows up in a node which is completely unrelated to a watchdog. Maybe it wasn't such a good idea to tie the watchdog to the timer node. At the very least, the situation should be handled in the driver via of_table flags. If the situation can't be handled that way, something is even more wrong. In that case it might be better to revert commit 36375491a439 until that is sorted out properly.

Guenter

So pretimeout should be enabled only for kpss-wdt and disabled with a flag in the of_table of the driver?

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines · Ansuel Smith
Quoted 6 lines

Some platform like ipq806x doesn't support pretimeout. As the driver check if there are available interrupts and ipq806x use msm-timer that require interrupts, the watchdog fail to probe as request_irq tries to use a ppi interrupt. Add an option to skip pretimeout setup and use the normal watchdog probe.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

Review

Quoted 6 lines
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/watchdog/qcom-wdt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
(quoted diff) +0 -0

b/drivers/watchdog/qcom-wdt.c

Quoted 25 lines
Quoted 25 lines
Quoted 25 lines

drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device

Quoted 4 lines

index a494543d3ae1..e689e97e883e 100644

drivers/watchdog/qcom-wdt.c +0 -0
@@ -189,6 +189,7 @@ static int qcom_wdt_probe(struct platform_device

*pdev)

drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device

Quoted 8 lines
drivers/watchdog/qcom-wdt.c +1 -0
 	u32 percpu_offset;
 	int irq, ret;
 	struct clk *clk;
+	bool nopretimeout;
 
 	regs = of_device_get_match_data(dev);
 	if (!regs) {

@@ -204,6 +205,8 @@ static int qcom_wdt_probe(struct platform_device

*pdev)

drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device

Quoted 9 lines
drivers/watchdog/qcom-wdt.c +2 -0
 	if (!res)
 		return -ENOMEM;
 
+	nopretimeout = of_property_read_bool(np, "no-pretimeout");
+
 	/* We use CPU0's DGT for the watchdog */
 	if (of_property_read_u32(np, "cpu-offset", &percpu_offset))
 		percpu_offset = 0;

@@ -247,7 +250,7 @@ static int qcom_wdt_probe(struct platform_device

*pdev)

Quoted 1 line
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines

drivers/watchdog/qcom-wdt.c static int qcom_wdt_probe(struct platform_device *pdev) · line 253

Quoted 4 lines
drivers/watchdog/qcom-wdt.c +1 -1
 	/* check if there is pretimeout support */
 	irq = platform_get_irq(pdev, 0);
-	if (irq > 0) {
+	if (!nopretimeout && irq > 0) {

That is unnecessary; such platforms should simply not provide an

interrupt.

Quoted 1 line

Or, in other words, what is the point of assigning an interrupt to be

used

Quoted 1 line
Quoted 1 line
Quoted 1 line

for pretimeout if the platform doesn't support it ? And then to add

yet

Quoted 11 lines
Quoted 11 lines
Quoted 10 lines

another attribute to tell the driver not to use it ?

Guenter

Quoted 6 lines
drivers/watchdog/qcom-wdt.c +0 -1
 		ret = devm_request_irq(dev, irq, qcom_wdt_isr,
 				       IRQF_TRIGGER_RISING,
 				       "wdt_bark", &wdt->wdd);
--

2.24.0

maintainer
#10 lore raw

Re: R: R: [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout

Quoted 35 lines · On Tue, Feb 04, 2020 at 06:30:40PM +0100, [email protected] wrote:
Quoted 2 lines · On Tue, Feb 04, 2020 at 05:16:34PM +0100, [email protected] wrote:
Quoted 1 line

If something like this is used, msm-timer require interrupts. Without

this

Quoted 32 lines
Quoted 15 lines

configuration, the device is unbootable as the system froze on system bootup.

timer@200a000 {
			compatible = "qcom,kpss-timer", "qcom,msm-timer";
			interrupts = <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>,
				     <GIC_PPI 5 (GIC_CPU_MASK_SIMPLE(2) |
						 IRQ_TYPE_EDGE_RISING)>;
Quoted 9 lines
			no-pretimeout;
			reg = <0x0200a000 0x100>;
			clock-frequency = <25000000>,
					  <32768>;
			clocks = <&sleep_clk>;
			clock-names = "sleep";
			cpu-offset = <0x80000>;
		};

I think this is all wrong; the new property shows up in a node which is completely unrelated to a watchdog. Maybe it wasn't such a good idea to tie the watchdog to the timer node. At the very least, the situation should be handled in the driver via of_table flags. If the situation can't be handled that way, something is even more wrong. In that case it might be better to revert commit 36375491a439 until that is sorted out properly.

Quoted 6 lines
Quoted 2 lines

Guenter

So pretimeout should be enabled only for kpss-wdt and disabled with a flag in the of_table of the driver?

Correct, if that is the determining factor.

Guenter