* [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) 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 related [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) 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 related [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 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
* [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) 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 related [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
* 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
* 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
end of thread, other threads:[~2020-02-06 20:46 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).