ARM: dts: Use level interrupt for omap4 & 5 wlcore
diff mbox series

Message ID 20191009164344.41093-1-tony@atomide.com
State New
Headers show
Series
  • ARM: dts: Use level interrupt for omap4 & 5 wlcore
Related show

Commit Message

Tony Lindgren Oct. 9, 2019, 4:43 p.m. UTC
Commit 572cf7d7b07d ("ARM: dts: Improve omap l4per idling with wlcore edge
sensitive interrupt") changed wlcore interrupts to use edge interrupt based
on what's specified in the wl1835mod.pdf data sheet.

However, there are still cases where we can have lost interrupts as
described in omap_gpio_unidle(). And using a level interrupt instead of edge
interrupt helps as we avoid the check for untriggered GPIO interrupts in
omap_gpio_unidle().

And with commit e6818d29ea15 ("gpio: gpio-omap: configure edge detection
for level IRQs for idle wakeup") GPIOs idle just fine with level interrupts.

Let's change omap4 and 5 wlcore users back to using level interrupt
instead of edge interrupt. Let's not change the others as I've only seen
this on omap4 and 5, probably because the other SoCs don't have l4per idle
independent of the CPUs.

Fixes: 572cf7d7b07d ("ARM: dts: Improve omap l4per idling with wlcore edge sensitive interrupt")
Depends-on: e6818d29ea15 ("gpio: gpio-omap: configure edge detection for level IRQs for idle wakeup")
Cc: Anders Roxell <anders.roxell@linaro.org>
Cc: Eyal Reizer <eyalr@ti.com>
Cc: Guy Mishol <guym@ti.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/boot/dts/omap4-droid4-xt894.dts       | 2 +-
 arch/arm/boot/dts/omap4-panda-common.dtsi      | 2 +-
 arch/arm/boot/dts/omap4-sdp.dts                | 2 +-
 arch/arm/boot/dts/omap4-var-som-om44-wlan.dtsi | 2 +-
 arch/arm/boot/dts/omap5-board-common.dtsi      | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

Comments

Ulf Hansson Oct. 10, 2019, 7:29 a.m. UTC | #1
On Wed, 9 Oct 2019 at 18:43, Tony Lindgren <tony@atomide.com> wrote:
>
> Commit 572cf7d7b07d ("ARM: dts: Improve omap l4per idling with wlcore edge
> sensitive interrupt") changed wlcore interrupts to use edge interrupt based
> on what's specified in the wl1835mod.pdf data sheet.
>
> However, there are still cases where we can have lost interrupts as
> described in omap_gpio_unidle(). And using a level interrupt instead of edge
> interrupt helps as we avoid the check for untriggered GPIO interrupts in
> omap_gpio_unidle().
>
> And with commit e6818d29ea15 ("gpio: gpio-omap: configure edge detection
> for level IRQs for idle wakeup") GPIOs idle just fine with level interrupts.
>
> Let's change omap4 and 5 wlcore users back to using level interrupt
> instead of edge interrupt. Let's not change the others as I've only seen
> this on omap4 and 5, probably because the other SoCs don't have l4per idle
> independent of the CPUs.

I assume this relates to the implementation for support of SDIO IRQs
(and wakeups) in the omap_hsmmc driver?

In any case, just wanted to share some experience in the field, feel
free to do whatever you want with the below information. :-)

So, while I was working for ST-Ericsson on ux500, we had a very
similar approach to re-route the SDIO bus DAT1 line to a GPIO IRQ as a
remote/system wakeup (vendor hack in the mmci driver). In other words,
while runtime suspending the mmc host controller, we configured a GPIO
IRQ, via an always on logic, to capture the IRQ instead. The point is,
I believe we may have ended up looking at similar problems as you have
been facing on OMAP.

In hindsight, I realized that we actually violated the SDIO spec by
using this approach. More precisely, during runtime suspend we do
clock gating and then re-routes the IRQ. However, clock gating isn't
allowed before the SDIO bus width have been changed back from 4-bit
into 1-bit. This last piece of action, would be an interesting change
to see if it could affect the behaviour, but unfortunately I have
never been able to check this.

The tricky part, is that we can't issue a command to change the bus to
1-bit in omap_hsmmc ->runtime_suspend() callback (this needs to be
managed by the core in some way). However, we can make a simple test,
by simply always limit the bus width to 1-bit, as that should mean we
should conform to the SDIO spec.

Kind regards
Uffe

>
> Fixes: 572cf7d7b07d ("ARM: dts: Improve omap l4per idling with wlcore edge sensitive interrupt")
> Depends-on: e6818d29ea15 ("gpio: gpio-omap: configure edge detection for level IRQs for idle wakeup")
> Cc: Anders Roxell <anders.roxell@linaro.org>
> Cc: Eyal Reizer <eyalr@ti.com>
> Cc: Guy Mishol <guym@ti.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  arch/arm/boot/dts/omap4-droid4-xt894.dts       | 2 +-
>  arch/arm/boot/dts/omap4-panda-common.dtsi      | 2 +-
>  arch/arm/boot/dts/omap4-sdp.dts                | 2 +-
>  arch/arm/boot/dts/omap4-var-som-om44-wlan.dtsi | 2 +-
>  arch/arm/boot/dts/omap5-board-common.dtsi      | 2 +-
>  5 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/boot/dts/omap4-droid4-xt894.dts b/arch/arm/boot/dts/omap4-droid4-xt894.dts
> --- a/arch/arm/boot/dts/omap4-droid4-xt894.dts
> +++ b/arch/arm/boot/dts/omap4-droid4-xt894.dts
> @@ -369,7 +369,7 @@
>                 compatible = "ti,wl1285", "ti,wl1283";
>                 reg = <2>;
>                 /* gpio_100 with gpmc_wait2 pad as wakeirq */
> -               interrupts-extended = <&gpio4 4 IRQ_TYPE_EDGE_RISING>,
> +               interrupts-extended = <&gpio4 4 IRQ_TYPE_LEVEL_HIGH>,
>                                       <&omap4_pmx_core 0x4e>;
>                 interrupt-names = "irq", "wakeup";
>                 ref-clock-frequency = <26000000>;
> diff --git a/arch/arm/boot/dts/omap4-panda-common.dtsi b/arch/arm/boot/dts/omap4-panda-common.dtsi
> --- a/arch/arm/boot/dts/omap4-panda-common.dtsi
> +++ b/arch/arm/boot/dts/omap4-panda-common.dtsi
> @@ -474,7 +474,7 @@
>                 compatible = "ti,wl1271";
>                 reg = <2>;
>                 /* gpio_53 with gpmc_ncs3 pad as wakeup */
> -               interrupts-extended = <&gpio2 21 IRQ_TYPE_EDGE_RISING>,
> +               interrupts-extended = <&gpio2 21 IRQ_TYPE_LEVEL_HIGH>,
>                                       <&omap4_pmx_core 0x3a>;
>                 interrupt-names = "irq", "wakeup";
>                 ref-clock-frequency = <38400000>;
> diff --git a/arch/arm/boot/dts/omap4-sdp.dts b/arch/arm/boot/dts/omap4-sdp.dts
> --- a/arch/arm/boot/dts/omap4-sdp.dts
> +++ b/arch/arm/boot/dts/omap4-sdp.dts
> @@ -512,7 +512,7 @@
>                 compatible = "ti,wl1281";
>                 reg = <2>;
>                 interrupt-parent = <&gpio1>;
> -               interrupts = <21 IRQ_TYPE_EDGE_RISING>; /* gpio 53 */
> +               interrupts = <21 IRQ_TYPE_LEVEL_HIGH>; /* gpio 53 */
>                 ref-clock-frequency = <26000000>;
>                 tcxo-clock-frequency = <26000000>;
>         };
> diff --git a/arch/arm/boot/dts/omap4-var-som-om44-wlan.dtsi b/arch/arm/boot/dts/omap4-var-som-om44-wlan.dtsi
> --- a/arch/arm/boot/dts/omap4-var-som-om44-wlan.dtsi
> +++ b/arch/arm/boot/dts/omap4-var-som-om44-wlan.dtsi
> @@ -69,7 +69,7 @@
>                 compatible = "ti,wl1271";
>                 reg = <2>;
>                 interrupt-parent = <&gpio2>;
> -               interrupts = <9 IRQ_TYPE_EDGE_RISING>; /* gpio 41 */
> +               interrupts = <9 IRQ_TYPE_LEVEL_HIGH>; /* gpio 41 */
>                 ref-clock-frequency = <38400000>;
>         };
>  };
> diff --git a/arch/arm/boot/dts/omap5-board-common.dtsi b/arch/arm/boot/dts/omap5-board-common.dtsi
> --- a/arch/arm/boot/dts/omap5-board-common.dtsi
> +++ b/arch/arm/boot/dts/omap5-board-common.dtsi
> @@ -362,7 +362,7 @@
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&wlcore_irq_pin>;
>                 interrupt-parent = <&gpio1>;
> -               interrupts = <14 IRQ_TYPE_EDGE_RISING>; /* gpio 14 */
> +               interrupts = <14 IRQ_TYPE_LEVEL_HIGH>;  /* gpio 14 */
>                 ref-clock-frequency = <26000000>;
>         };
>  };
> --
> 2.23.0
Andreas Kemnade Oct. 10, 2019, 10:25 a.m. UTC | #2
On Thu, 10 Oct 2019 09:29:45 +0200
Ulf Hansson <ulf.hansson@linaro.org> wrote:

> On Wed, 9 Oct 2019 at 18:43, Tony Lindgren <tony@atomide.com> wrote:
> >
> > Commit 572cf7d7b07d ("ARM: dts: Improve omap l4per idling with wlcore edge
> > sensitive interrupt") changed wlcore interrupts to use edge interrupt based
> > on what's specified in the wl1835mod.pdf data sheet.
> >
> > However, there are still cases where we can have lost interrupts as
> > described in omap_gpio_unidle(). And using a level interrupt instead of edge
> > interrupt helps as we avoid the check for untriggered GPIO interrupts in
> > omap_gpio_unidle().
> >
> > And with commit e6818d29ea15 ("gpio: gpio-omap: configure edge detection
> > for level IRQs for idle wakeup") GPIOs idle just fine with level interrupts.
> >
> > Let's change omap4 and 5 wlcore users back to using level interrupt
> > instead of edge interrupt. Let's not change the others as I've only seen
> > this on omap4 and 5, probably because the other SoCs don't have l4per idle
> > independent of the CPUs.  
> 
> I assume this relates to the implementation for support of SDIO IRQs
> (and wakeups) in the omap_hsmmc driver?
> 
> In any case, just wanted to share some experience in the field, feel
> free to do whatever you want with the below information. :-)
> 
> So, while I was working for ST-Ericsson on ux500, we had a very
> similar approach to re-route the SDIO bus DAT1 line to a GPIO IRQ as a
> remote/system wakeup (vendor hack in the mmci driver). In other words,
> while runtime suspending the mmc host controller, we configured a GPIO
> IRQ, via an always on logic, to capture the IRQ instead. The point is,
> I believe we may have ended up looking at similar problems as you have
> been facing on OMAP.
> 
> In hindsight, I realized that we actually violated the SDIO spec by
> using this approach. More precisely, during runtime suspend we do
> clock gating and then re-routes the IRQ. However, clock gating isn't
> allowed before the SDIO bus width have been changed back from 4-bit
> into 1-bit. This last piece of action, would be an interesting change
> to see if it could affect the behaviour, but unfortunately I have
> never been able to check this.
> 
> The tricky part, is that we can't issue a command to change the bus to
> 1-bit in omap_hsmmc ->runtime_suspend() callback (this needs to be
> managed by the core in some way). However, we can make a simple test,
> by simply always limit the bus width to 1-bit, as that should mean we
> should conform to the SDIO spec.
> 

somehow matches that with my experiences with libertas + omap3.
SDIO irq seems to work only with runtime force-enabled in omap_hsmmc
or using 1bit mode.
And yes, I tried switching mode to 1bit in runtime_suspend() but as
you said, that cannot easily done.

Regards,
Andreas
Tony Lindgren Oct. 10, 2019, 1:44 p.m. UTC | #3
* Andreas Kemnade <andreas@kemnade.info> [191010 10:28]:
> On Thu, 10 Oct 2019 09:29:45 +0200
> Ulf Hansson <ulf.hansson@linaro.org> wrote:
> 
> > On Wed, 9 Oct 2019 at 18:43, Tony Lindgren <tony@atomide.com> wrote:
> > >
> > > Commit 572cf7d7b07d ("ARM: dts: Improve omap l4per idling with wlcore edge
> > > sensitive interrupt") changed wlcore interrupts to use edge interrupt based
> > > on what's specified in the wl1835mod.pdf data sheet.
> > >
> > > However, there are still cases where we can have lost interrupts as
> > > described in omap_gpio_unidle(). And using a level interrupt instead of edge
> > > interrupt helps as we avoid the check for untriggered GPIO interrupts in
> > > omap_gpio_unidle().
> > >
> > > And with commit e6818d29ea15 ("gpio: gpio-omap: configure edge detection
> > > for level IRQs for idle wakeup") GPIOs idle just fine with level interrupts.
> > >
> > > Let's change omap4 and 5 wlcore users back to using level interrupt
> > > instead of edge interrupt. Let's not change the others as I've only seen
> > > this on omap4 and 5, probably because the other SoCs don't have l4per idle
> > > independent of the CPUs.  
> > 
> > I assume this relates to the implementation for support of SDIO IRQs
> > (and wakeups) in the omap_hsmmc driver?

In the wlcore case, there's actually an out-of-band GPIO interrupt that
can be used. That is in addition to the SDIO bus DAT1 line, that is not
currently used for wlcore wake-up.

> > In any case, just wanted to share some experience in the field, feel
> > free to do whatever you want with the below information. :-)
> > 
> > So, while I was working for ST-Ericsson on ux500, we had a very
> > similar approach to re-route the SDIO bus DAT1 line to a GPIO IRQ as a
> > remote/system wakeup (vendor hack in the mmci driver). In other words,
> > while runtime suspending the mmc host controller, we configured a GPIO
> > IRQ, via an always on logic, to capture the IRQ instead. The point is,
> > I believe we may have ended up looking at similar problems as you have
> > been facing on OMAP.
> > 
> > In hindsight, I realized that we actually violated the SDIO spec by
> > using this approach. More precisely, during runtime suspend we do
> > clock gating and then re-routes the IRQ. However, clock gating isn't
> > allowed before the SDIO bus width have been changed back from 4-bit
> > into 1-bit. This last piece of action, would be an interesting change
> > to see if it could affect the behaviour, but unfortunately I have
> > never been able to check this.
> > 
> > The tricky part, is that we can't issue a command to change the bus to
> > 1-bit in omap_hsmmc ->runtime_suspend() callback (this needs to be
> > managed by the core in some way). However, we can make a simple test,
> > by simply always limit the bus width to 1-bit, as that should mean we
> > should conform to the SDIO spec.
> > 
> 
> somehow matches that with my experiences with libertas + omap3.
> SDIO irq seems to work only with runtime force-enabled in omap_hsmmc
> or using 1bit mode.
> And yes, I tried switching mode to 1bit in runtime_suspend() but as
> you said, that cannot easily done.

Yeah libertas still has a pending issue and we're missing the 1-bit
mode. From my experience, libertas is a power hog though when in use.

Meanwhile, mwifiex and wlcore SDIO have been behaving very nice with
PM for omap3 variants for many years. That is even without the 1-bit
change.

But on omap4 I've seen lost interrupts with wlcore GPIO interrupt,
while omap4 with mwifiex using SDIO DAT1 interrupt has been behaving.

The $subject patch fixes the lost wlcore GPIO interrupt issue, after
all the surrounding GPIO fixes done during this year.

FYI, my PM regression test is just ping -c1 an idle system over WLAN :)

Then based on the ping response time I know the device is idling
properly and the wake-up is working.

The test also confirms that all the surrounding stuff like
regulators, GPIOs, pinctrl wakeirq, and MMC layer are behaving.

Then a more advanced version of this test is to ssh to an idle system
and check the latency is OK for typing and measure power consumption
when idle.

Regards,

Tony

Patch
diff mbox series

diff --git a/arch/arm/boot/dts/omap4-droid4-xt894.dts b/arch/arm/boot/dts/omap4-droid4-xt894.dts
--- a/arch/arm/boot/dts/omap4-droid4-xt894.dts
+++ b/arch/arm/boot/dts/omap4-droid4-xt894.dts
@@ -369,7 +369,7 @@ 
 		compatible = "ti,wl1285", "ti,wl1283";
 		reg = <2>;
 		/* gpio_100 with gpmc_wait2 pad as wakeirq */
-		interrupts-extended = <&gpio4 4 IRQ_TYPE_EDGE_RISING>,
+		interrupts-extended = <&gpio4 4 IRQ_TYPE_LEVEL_HIGH>,
 				      <&omap4_pmx_core 0x4e>;
 		interrupt-names = "irq", "wakeup";
 		ref-clock-frequency = <26000000>;
diff --git a/arch/arm/boot/dts/omap4-panda-common.dtsi b/arch/arm/boot/dts/omap4-panda-common.dtsi
--- a/arch/arm/boot/dts/omap4-panda-common.dtsi
+++ b/arch/arm/boot/dts/omap4-panda-common.dtsi
@@ -474,7 +474,7 @@ 
 		compatible = "ti,wl1271";
 		reg = <2>;
 		/* gpio_53 with gpmc_ncs3 pad as wakeup */
-		interrupts-extended = <&gpio2 21 IRQ_TYPE_EDGE_RISING>,
+		interrupts-extended = <&gpio2 21 IRQ_TYPE_LEVEL_HIGH>,
 				      <&omap4_pmx_core 0x3a>;
 		interrupt-names = "irq", "wakeup";
 		ref-clock-frequency = <38400000>;
diff --git a/arch/arm/boot/dts/omap4-sdp.dts b/arch/arm/boot/dts/omap4-sdp.dts
--- a/arch/arm/boot/dts/omap4-sdp.dts
+++ b/arch/arm/boot/dts/omap4-sdp.dts
@@ -512,7 +512,7 @@ 
 		compatible = "ti,wl1281";
 		reg = <2>;
 		interrupt-parent = <&gpio1>;
-		interrupts = <21 IRQ_TYPE_EDGE_RISING>; /* gpio 53 */
+		interrupts = <21 IRQ_TYPE_LEVEL_HIGH>; /* gpio 53 */
 		ref-clock-frequency = <26000000>;
 		tcxo-clock-frequency = <26000000>;
 	};
diff --git a/arch/arm/boot/dts/omap4-var-som-om44-wlan.dtsi b/arch/arm/boot/dts/omap4-var-som-om44-wlan.dtsi
--- a/arch/arm/boot/dts/omap4-var-som-om44-wlan.dtsi
+++ b/arch/arm/boot/dts/omap4-var-som-om44-wlan.dtsi
@@ -69,7 +69,7 @@ 
 		compatible = "ti,wl1271";
 		reg = <2>;
 		interrupt-parent = <&gpio2>;
-		interrupts = <9 IRQ_TYPE_EDGE_RISING>; /* gpio 41 */
+		interrupts = <9 IRQ_TYPE_LEVEL_HIGH>; /* gpio 41 */
 		ref-clock-frequency = <38400000>;
 	};
 };
diff --git a/arch/arm/boot/dts/omap5-board-common.dtsi b/arch/arm/boot/dts/omap5-board-common.dtsi
--- a/arch/arm/boot/dts/omap5-board-common.dtsi
+++ b/arch/arm/boot/dts/omap5-board-common.dtsi
@@ -362,7 +362,7 @@ 
 		pinctrl-names = "default";
 		pinctrl-0 = <&wlcore_irq_pin>;
 		interrupt-parent = <&gpio1>;
-		interrupts = <14 IRQ_TYPE_EDGE_RISING>;	/* gpio 14 */
+		interrupts = <14 IRQ_TYPE_LEVEL_HIGH>;	/* gpio 14 */
 		ref-clock-frequency = <26000000>;
 	};
 };