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

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

[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 <ansuelsmth@gmail.com>
---
 drivers/watchdog/qcom-wdt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index a494543d3ae1..e689e97e883e 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -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);
-- 
2.24.0

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

[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.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
index 41aeaa2ff0f8..33081bd33637 100644
--- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
@@ -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>;
-- 
2.24.0

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

[Guenter Roeck]

On Tue, Feb 04, 2020 at 04:21:02PM +0100, Ansuel Smith wrote:
> "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

> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> index 41aeaa2ff0f8..33081bd33637 100644
> --- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> @@ -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>;
> -- 
> 2.24.0
> 

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

[Rob Herring]

On Tue, Feb 04, 2020 at 08:09:58AM -0800, Guenter Roeck wrote:
> On Tue, Feb 04, 2020 at 04:21:02PM +0100, Ansuel Smith wrote:
> > "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.

Right.

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

Rob

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

[Ansuel Smith]

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

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
index 33081bd33637..01978bff74ee 100644
--- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
@@ -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 {
-- 
2.24.0

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

[Guenter Roeck]

On Tue, Feb 04, 2020 at 04:21:03PM +0100, Ansuel Smith wrote:
> Add description for new no-pretimeout function to force legacy
> probe if any interrupt is defined.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  Documentation/devicetree/bindings/watchdog/qcom-wdt.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> index 33081bd33637..01978bff74ee 100644
> --- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> @@ -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

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

[Guenter Roeck]

On Tue, Feb 04, 2020 at 04:21:01PM +0100, Ansuel Smith wrote:
> 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 <ansuelsmth@gmail.com>
> ---
>  drivers/watchdog/qcom-wdt.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index a494543d3ae1..e689e97e883e 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -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) {

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

>  		ret = devm_request_irq(dev, irq, qcom_wdt_isr,
>  				       IRQF_TRIGGER_RISING,
>  				       "wdt_bark", &wdt->wdd);
> -- 
> 2.24.0
> 

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

[ansuelsmth]

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>;
		};

> On Tue, Feb 04, 2020 at 04:21:01PM +0100, Ansuel Smith wrote:
> > 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 <ansuelsmth@gmail.com>
> > ---
> >  drivers/watchdog/qcom-wdt.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> > index a494543d3ae1..e689e97e883e 100644
> > --- a/drivers/watchdog/qcom-wdt.c
> > +++ b/drivers/watchdog/qcom-wdt.c
> > @@ -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) {
> 
> 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
> 
> >  		ret = devm_request_irq(dev, irq, qcom_wdt_isr,
> >  				       IRQF_TRIGGER_RISING,
> >  				       "wdt_bark", &wdt->wdd);
> > --
> > 2.24.0
> >

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

[Guenter Roeck]

On Tue, Feb 04, 2020 at 05:16:34PM +0100, ansuelsmth@gmail.com 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)>;
> 			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

> > On Tue, Feb 04, 2020 at 04:21:01PM +0100, Ansuel Smith wrote:
> > > 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 <ansuelsmth@gmail.com>
> > > ---
> > >  drivers/watchdog/qcom-wdt.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> > > index a494543d3ae1..e689e97e883e 100644
> > > --- a/drivers/watchdog/qcom-wdt.c
> > > +++ b/drivers/watchdog/qcom-wdt.c
> > > @@ -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) {
> > 
> > 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
> > 
> > >  		ret = devm_request_irq(dev, irq, qcom_wdt_isr,
> > >  				       IRQF_TRIGGER_RISING,
> > >  				       "wdt_bark", &wdt->wdd);
> > > --
> > > 2.24.0
> > >
> 

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

[ansuelsmth]

> On Tue, Feb 04, 2020 at 05:16:34PM +0100, ansuelsmth@gmail.com 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)>;
> > 			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
> 

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

> > > On Tue, Feb 04, 2020 at 04:21:01PM +0100, Ansuel Smith wrote:
> > > > 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 <ansuelsmth@gmail.com>
> > > > ---
> > > >  drivers/watchdog/qcom-wdt.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/watchdog/qcom-wdt.c
b/drivers/watchdog/qcom-wdt.c
> > > > index a494543d3ae1..e689e97e883e 100644
> > > > --- a/drivers/watchdog/qcom-wdt.c
> > > > +++ b/drivers/watchdog/qcom-wdt.c
> > > > @@ -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) {
> > >
> > > 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
> > >
> > > >  		ret = devm_request_irq(dev, irq, qcom_wdt_isr,
> > > >  				       IRQF_TRIGGER_RISING,
> > > >  				       "wdt_bark", &wdt->wdd);
> > > > --
> > > > 2.24.0
> > > >
> >

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

[Guenter Roeck]

On Tue, Feb 04, 2020 at 06:30:40PM +0100, ansuelsmth@gmail.com wrote:
> > On Tue, Feb 04, 2020 at 05:16:34PM +0100, ansuelsmth@gmail.com 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)>;
> > > 			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
> > 
> 
> 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