Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout
@ 2020-02-04 15:21 Ansuel Smith
  2020-02-04 15:21 ` [PATCH 2/3] Documentation: watchdog: qcom-wdt: Remove deprecated compatible Ansuel Smith
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Ansuel Smith @ 2020-02-04 15:21 UTC (permalink / raw)
  To: unlisted-recipients:; (no To-header on input)
  Cc: Ansuel Smith, Andy Gross, Bjorn Andersson, Wim Van Sebroeck,
	Guenter Roeck, Rob Herring, Mark Rutland, linux-arm-msm,
	linux-watchdog, devicetree, linux-kernel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/3] Documentation: watchdog: qcom-wdt: Remove deprecated compatible
  2020-02-04 15:21 [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout Ansuel Smith
@ 2020-02-04 15:21 ` Ansuel Smith
  2020-02-04 16:09   ` Guenter Roeck
  2020-02-04 15:21 ` [PATCH 3/3] Documentation: watchdog: qcom-wdt: add new no-pretimeout option Ansuel Smith
  2020-02-04 16:08 ` [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout Guenter Roeck
  2 siblings, 1 reply; 11+ messages in thread
From: Ansuel Smith @ 2020-02-04 15:21 UTC (permalink / raw)
  To: unlisted-recipients:; (no To-header on input)
  Cc: Ansuel Smith, Andy Gross, Bjorn Andersson, Wim Van Sebroeck,
	Guenter Roeck, Rob Herring, Mark Rutland, linux-arm-msm,
	linux-watchdog, devicetree, linux-kernel

"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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 3/3] Documentation: watchdog: qcom-wdt: add new no-pretimeout option
  2020-02-04 15:21 [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout Ansuel Smith
  2020-02-04 15:21 ` [PATCH 2/3] Documentation: watchdog: qcom-wdt: Remove deprecated compatible Ansuel Smith
@ 2020-02-04 15:21 ` Ansuel Smith
  2020-02-04 16:11   ` Guenter Roeck
  2020-02-04 16:08 ` [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout Guenter Roeck
  2 siblings, 1 reply; 11+ messages in thread
From: Ansuel Smith @ 2020-02-04 15:21 UTC (permalink / raw)
  To: unlisted-recipients:; (no To-header on input)
  Cc: Ansuel Smith, Andy Gross, Bjorn Andersson, Wim Van Sebroeck,
	Guenter Roeck, Rob Herring, Mark Rutland, linux-arm-msm,
	linux-watchdog, devicetree, linux-kernel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout
  2020-02-04 15:21 [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout Ansuel Smith
  2020-02-04 15:21 ` [PATCH 2/3] Documentation: watchdog: qcom-wdt: Remove deprecated compatible Ansuel Smith
  2020-02-04 15:21 ` [PATCH 3/3] Documentation: watchdog: qcom-wdt: add new no-pretimeout option Ansuel Smith
@ 2020-02-04 16:08 ` Guenter Roeck
  2020-02-04 16:16   ` R: " ansuelsmth
  2 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2020-02-04 16:08 UTC (permalink / raw)
  To: Ansuel Smith
  Cc: Andy Gross, Bjorn Andersson, Wim Van Sebroeck, Rob Herring,
	Mark Rutland, linux-arm-msm, linux-watchdog, devicetree,
	linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] Documentation: watchdog: qcom-wdt: Remove deprecated compatible
  2020-02-04 15:21 ` [PATCH 2/3] Documentation: watchdog: qcom-wdt: Remove deprecated compatible Ansuel Smith
@ 2020-02-04 16:09   ` Guenter Roeck
  2020-02-06 19:15     ` Rob Herring
  0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2020-02-04 16:09 UTC (permalink / raw)
  To: Ansuel Smith
  Cc: Andy Gross, Bjorn Andersson, Wim Van Sebroeck, Rob Herring,
	Mark Rutland, linux-arm-msm, linux-watchdog, devicetree,
	linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] Documentation: watchdog: qcom-wdt: add new no-pretimeout option
  2020-02-04 15:21 ` [PATCH 3/3] Documentation: watchdog: qcom-wdt: add new no-pretimeout option Ansuel Smith
@ 2020-02-04 16:11   ` Guenter Roeck
  0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2020-02-04 16:11 UTC (permalink / raw)
  To: Ansuel Smith
  Cc: Andy Gross, Bjorn Andersson, Wim Van Sebroeck, Rob Herring,
	Mark Rutland, linux-arm-msm, linux-watchdog, devicetree,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* R: [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout
  2020-02-04 16:08 ` [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout Guenter Roeck
@ 2020-02-04 16:16   ` " ansuelsmth
  2020-02-04 16:25     ` Guenter Roeck
  0 siblings, 1 reply; 11+ messages in thread
From: ansuelsmth @ 2020-02-04 16:16 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Andy Gross, Bjorn Andersson, Wim Van Sebroeck, Rob Herring,
	Mark Rutland, linux-arm-msm, linux-watchdog, devicetree,
	linux-kernel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: R: [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout
  2020-02-04 16:16   ` R: " ansuelsmth
@ 2020-02-04 16:25     ` Guenter Roeck
  2020-02-04 17:30       ` R: " ansuelsmth
  0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2020-02-04 16:25 UTC (permalink / raw)
  To: ansuelsmth
  Cc: Andy Gross, Bjorn Andersson, Wim Van Sebroeck, Rob Herring,
	Mark Rutland, linux-arm-msm, linux-watchdog, devicetree,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* R: R: [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout
  2020-02-04 16:25     ` Guenter Roeck
@ 2020-02-04 17:30       ` " ansuelsmth
  2020-02-04 17:37         ` Guenter Roeck
  0 siblings, 1 reply; 11+ messages in thread
From: ansuelsmth @ 2020-02-04 17:30 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Andy Gross, Bjorn Andersson, Wim Van Sebroeck, Rob Herring,
	Mark Rutland, linux-arm-msm, linux-watchdog, devicetree,
	linux-kernel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: R: R: [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout
  2020-02-04 17:30       ` R: " ansuelsmth
@ 2020-02-04 17:37         ` Guenter Roeck
  0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2020-02-04 17:37 UTC (permalink / raw)
  To: ansuelsmth
  Cc: Andy Gross, Bjorn Andersson, Wim Van Sebroeck, Rob Herring,
	Mark Rutland, linux-arm-msm, linux-watchdog, devicetree,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] Documentation: watchdog: qcom-wdt: Remove deprecated compatible
  2020-02-04 16:09   ` Guenter Roeck
@ 2020-02-06 19:15     ` Rob Herring
  0 siblings, 0 replies; 11+ messages in thread
From: Rob Herring @ 2020-02-06 19:15 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Ansuel Smith, Andy Gross, Bjorn Andersson, Wim Van Sebroeck,
	Mark Rutland, linux-arm-msm, linux-watchdog, devicetree,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-04 15:21 [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout Ansuel Smith
2020-02-04 15:21 ` [PATCH 2/3] Documentation: watchdog: qcom-wdt: Remove deprecated compatible Ansuel Smith
2020-02-04 16:09   ` Guenter Roeck
2020-02-06 19:15     ` Rob Herring
2020-02-04 15:21 ` [PATCH 3/3] Documentation: watchdog: qcom-wdt: add new no-pretimeout option Ansuel Smith
2020-02-04 16:11   ` Guenter Roeck
2020-02-04 16:08 ` [PATCH 1/3] watchdog: qcom-wdt: add option to skip pretimeout Guenter Roeck
2020-02-04 16:16   ` R: " ansuelsmth
2020-02-04 16:25     ` Guenter Roeck
2020-02-04 17:30       ` R: " ansuelsmth
2020-02-04 17:37         ` Guenter Roeck

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git