4430sdp nfsroot broken with ff5c9059

4430sdp nfsroot broken with ff5c9059

[Tony Lindgren]

Hi Jon,

Looks like at least 4430sdp nfsroot got broken with commit
ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
property).

Do we need to pass the GPIO edge/level info now?

Regards,

Tony

Re: 4430sdp nfsroot broken with ff5c9059

[Jon Hunter]

Hi Tony,

On 04/09/2013 04:23 PM, Tony Lindgren wrote:
> Hi Jon,
> 
> Looks like at least 4430sdp nfsroot got broken with commit
> ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
> property).

Thanks for reporting. I am actually amazed that ethernet is 
working on any OMAP board (with device-tree) that requires a
gpio as an interrupt because we have still not come to an
agreement on [1]. Looking at the OMAP4 SDP I believe this is
working by luck because there are other gpios in the same
bank that are active and so the bank is enabled. If that were
not the case then this would not work. 

Hence, I have not been testing ethernet when booting with 
device-tree because we need to fix [1] which has been really
crippling my testing. So this needs to be resolved so ethernet
can work consistently on all omap boards.
 
> Do we need to pass the GPIO edge/level info now?

Yes and here is a patch. I have tested on my OMAP4 SDP this
morning. Reviewing all the OMAP/AM dts(i) files I believe
that only the SDP and OMAP4-VAR-SOM are impacted by this. I
don't see any other boards currently using the gpio as an 
interrupt-controller.

Cheers
Jon

[1] comments.gmane.org/gmane.linux.ports.arm.omap/92192

>From ed3e66bcd4c0cb3df4cb70e75c45fa4462f70565 Mon Sep 17 00:00:00 2001
From: Jon Hunter <jon-hunter@ti.com>
Date: Wed, 10 Apr 2013 11:44:44 -0500
Subject: [PATCH] ARM: dts: OMAP4: Fix ethernet IRQ for OMAP4 boards

Commit ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
property) updated the number of interrupt cells required for configuring
gpios as interrupts for other devices (such as ethernet controllers).
This update allowed the interrupt type (edge, level, etc) to be
configured via device-tree (as described in the
Documentation/devicetree/bindings/gpio/gpio-omap.txt).

This broke ethernet support on the OMAP4 SDP board that defines a gpio
as the ethernet IRQ because the interrupt type (level, edge, etc) was
not getting configured correctly. This board use the ks8851 ethernet
chip which has an active low interrupt. Fix this by defining the gpio
interrupt as active-low in the device-tree binding.

Please note that the OMAP4-VAR-SOM also uses the same ethernet
controller and it is expected it will have the same problem. So the
same fix is also applied to this board.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/boot/dts/omap4-sdp.dts     |    2 +-
 arch/arm/boot/dts/omap4-var-som.dts |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/omap4-sdp.dts b/arch/arm/boot/dts/omap4-sdp.dts
index c387bdc..25cbe43 100644
--- a/arch/arm/boot/dts/omap4-sdp.dts
+++ b/arch/arm/boot/dts/omap4-sdp.dts
@@ -363,7 +363,7 @@
 		spi-max-frequency = <24000000>;
 		reg = <0>;
 		interrupt-parent = <&gpio2>;
-		interrupts = <2>; /* gpio line 34 */
+		interrupts = <2 8>; /* gpio line 34, low triggered */
 		vdd-supply = <&vdd_eth>;
 	};
 };
diff --git a/arch/arm/boot/dts/omap4-var-som.dts b/arch/arm/boot/dts/omap4-var-som.dts
index 222a413..7e04103 100644
--- a/arch/arm/boot/dts/omap4-var-som.dts
+++ b/arch/arm/boot/dts/omap4-var-som.dts
@@ -68,7 +68,7 @@
 		spi-max-frequency = <24000000>;
 		reg = <0>;
 		interrupt-parent = <&gpio6>;
-		interrupts = <11>; /* gpio line 171 */
+		interrupts = <11 8>; /* gpio line 171, low triggered */
 		vdd-supply = <&vdd_eth>;
 	};
 };
-- 
1.7.10.4

Re: 4430sdp nfsroot broken with ff5c9059

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [130410 10:35]:
> Hi Tony,
> 
> On 04/09/2013 04:23 PM, Tony Lindgren wrote:
> > Hi Jon,
> > 
> > Looks like at least 4430sdp nfsroot got broken with commit
> > ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
> > property).
> 
> Thanks for reporting. I am actually amazed that ethernet is 
> working on any OMAP board (with device-tree) that requires a
> gpio as an interrupt because we have still not come to an
> agreement on [1]. Looking at the OMAP4 SDP I believe this is
> working by luck because there are other gpios in the same
> bank that are active and so the bank is enabled. If that were
> not the case then this would not work. 
> 
> Hence, I have not been testing ethernet when booting with 
> device-tree because we need to fix [1] which has been really
> crippling my testing. So this needs to be resolved so ethernet
> can work consistently on all omap boards.

Cool.
  
> > Do we need to pass the GPIO edge/level info now?
> 
> Yes and here is a patch. I have tested on my OMAP4 SDP this
> morning. Reviewing all the OMAP/AM dts(i) files I believe
> that only the SDP and OMAP4-VAR-SOM are impacted by this. I
> don't see any other boards currently using the gpio as an 
> interrupt-controller.

Great. Just one comment: You might be able to already use
the DT preprocessor now easily with linux next for the edge
info.

Regards,

Tony
 
> [1] comments.gmane.org/gmane.linux.ports.arm.omap/92192
> 
> From ed3e66bcd4c0cb3df4cb70e75c45fa4462f70565 Mon Sep 17 00:00:00 2001
> From: Jon Hunter <jon-hunter@ti.com>
> Date: Wed, 10 Apr 2013 11:44:44 -0500
> Subject: [PATCH] ARM: dts: OMAP4: Fix ethernet IRQ for OMAP4 boards
> 
> Commit ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
> property) updated the number of interrupt cells required for configuring
> gpios as interrupts for other devices (such as ethernet controllers).
> This update allowed the interrupt type (edge, level, etc) to be
> configured via device-tree (as described in the
> Documentation/devicetree/bindings/gpio/gpio-omap.txt).
> 
> This broke ethernet support on the OMAP4 SDP board that defines a gpio
> as the ethernet IRQ because the interrupt type (level, edge, etc) was
> not getting configured correctly. This board use the ks8851 ethernet
> chip which has an active low interrupt. Fix this by defining the gpio
> interrupt as active-low in the device-tree binding.
> 
> Please note that the OMAP4-VAR-SOM also uses the same ethernet
> controller and it is expected it will have the same problem. So the
> same fix is also applied to this board.
> 
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> ---
>  arch/arm/boot/dts/omap4-sdp.dts     |    2 +-
>  arch/arm/boot/dts/omap4-var-som.dts |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/omap4-sdp.dts b/arch/arm/boot/dts/omap4-sdp.dts
> index c387bdc..25cbe43 100644
> --- a/arch/arm/boot/dts/omap4-sdp.dts
> +++ b/arch/arm/boot/dts/omap4-sdp.dts
> @@ -363,7 +363,7 @@
>  		spi-max-frequency = <24000000>;
>  		reg = <0>;
>  		interrupt-parent = <&gpio2>;
> -		interrupts = <2>; /* gpio line 34 */
> +		interrupts = <2 8>; /* gpio line 34, low triggered */
>  		vdd-supply = <&vdd_eth>;
>  	};
>  };
> diff --git a/arch/arm/boot/dts/omap4-var-som.dts b/arch/arm/boot/dts/omap4-var-som.dts
> index 222a413..7e04103 100644
> --- a/arch/arm/boot/dts/omap4-var-som.dts
> +++ b/arch/arm/boot/dts/omap4-var-som.dts
> @@ -68,7 +68,7 @@
>  		spi-max-frequency = <24000000>;
>  		reg = <0>;
>  		interrupt-parent = <&gpio6>;
> -		interrupts = <11>; /* gpio line 171 */
> +		interrupts = <11 8>; /* gpio line 171, low triggered */
>  		vdd-supply = <&vdd_eth>;
>  	};
>  };
> -- 
> 1.7.10.4

Re: 4430sdp nfsroot broken with ff5c9059

[Jon Hunter]

On 04/10/2013 01:20 PM, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [130410 10:35]:
>> Hi Tony,
>>
>> On 04/09/2013 04:23 PM, Tony Lindgren wrote:
>>> Hi Jon,
>>>
>>> Looks like at least 4430sdp nfsroot got broken with commit
>>> ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
>>> property).
>>
>> Thanks for reporting. I am actually amazed that ethernet is 
>> working on any OMAP board (with device-tree) that requires a
>> gpio as an interrupt because we have still not come to an
>> agreement on [1]. Looking at the OMAP4 SDP I believe this is
>> working by luck because there are other gpios in the same
>> bank that are active and so the bank is enabled. If that were
>> not the case then this would not work. 
>>
>> Hence, I have not been testing ethernet when booting with 
>> device-tree because we need to fix [1] which has been really
>> crippling my testing. So this needs to be resolved so ethernet
>> can work consistently on all omap boards.
> 
> Cool.
>   
>>> Do we need to pass the GPIO edge/level info now?
>>
>> Yes and here is a patch. I have tested on my OMAP4 SDP this
>> morning. Reviewing all the OMAP/AM dts(i) files I believe
>> that only the SDP and OMAP4-VAR-SOM are impacted by this. I
>> don't see any other boards currently using the gpio as an 
>> interrupt-controller.
> 
> Great. Just one comment: You might be able to already use
> the DT preprocessor now easily with linux next for the edge
> info.

Good idea. We could do something like the below, but it would
be better to include the "arm-gic.h" from the omap4.dtsi file
so that the definitions can be used there too. The problem is
that if I include "arm-gic.h" in the omap4.dtsi file I get a
DTC compiler error because of the "#include" statement. It
appears that the compiler does not like it if a file includes
a file using "/include/" and the file it is including uses
"#include". So really I think that we need to switch all the
dts/dtsi files to use "#include" first.

Jon

>From fe74fee9ac4d011703918557f7b9b638bfead300 Mon Sep 17 00:00:00 2001
From: Jon Hunter <jon-hunter@ti.com>
Date: Wed, 10 Apr 2013 11:44:44 -0500
Subject: [PATCH] ARM: dts: OMAP4: Fix ethernet IRQ for OMAP4 boards

Commit ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
property) updated the number of interrupt cells required for configuring
gpios as interrupts for other devices (such as ethernet controllers).
This update allowed the interrupt type (edge, level, etc) to be
configured via device-tree (as described in the
Documentation/devicetree/bindings/gpio/gpio-omap.txt).

This broke ethernet support on the OMAP4 SDP board that defines a gpio
as the ethernet IRQ because the interrupt type (level, edge, etc) was
not getting configured correctly. This board use the ks8851 ethernet
chip which has an active low interrupt. Fix this by defining the gpio
interrupt as active-low in the device-tree binding.

Please note that the OMAP4-VAR-SOM also uses the same ethernet
controller and it is expected it will have the same problem. So the
same fix is also applied to this board.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/boot/dts/omap4-sdp.dts     |    3 ++-
 arch/arm/boot/dts/omap4-var-som.dts |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/omap4-sdp.dts b/arch/arm/boot/dts/omap4-sdp.dts
index c387bdc..5d0c925 100644
--- a/arch/arm/boot/dts/omap4-sdp.dts
+++ b/arch/arm/boot/dts/omap4-sdp.dts
@@ -9,6 +9,7 @@
 
 /include/ "omap443x.dtsi"
 /include/ "elpida_ecb240abacn.dtsi"
+#include <dt-bindings/interrupt-controller/arm-gic.h>
 
 / {
 	model = "TI OMAP4 SDP board";
@@ -363,7 +364,7 @@
 		spi-max-frequency = <24000000>;
 		reg = <0>;
 		interrupt-parent = <&gpio2>;
-		interrupts = <2>; /* gpio line 34 */
+		interrupts = <2 IRQ_TYPE_LEVEL_LOW>; /* gpio line 34 */
 		vdd-supply = <&vdd_eth>;
 	};
 };
diff --git a/arch/arm/boot/dts/omap4-var-som.dts b/arch/arm/boot/dts/omap4-var-som.dts
index 222a413..ade7d46 100644
--- a/arch/arm/boot/dts/omap4-var-som.dts
+++ b/arch/arm/boot/dts/omap4-var-som.dts
@@ -8,6 +8,7 @@
 /dts-v1/;
 
 /include/ "omap443x.dtsi"
+#include <dt-bindings/interrupt-controller/arm-gic.h>
 
 / {
 	model = "Variscite OMAP4 SOM";
@@ -68,7 +69,7 @@
 		spi-max-frequency = <24000000>;
 		reg = <0>;
 		interrupt-parent = <&gpio6>;
-		interrupts = <11>; /* gpio line 171 */
+		interrupts = <11 IRQ_TYPE_LEVEL_LOW>; /* gpio line 171 */
 		vdd-supply = <&vdd_eth>;
 	};
 };
-- 
1.7.10.4

Re: 4430sdp nfsroot broken with ff5c9059

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [130410 13:47]:
> 
> On 04/10/2013 01:20 PM, Tony Lindgren wrote:
> > * Jon Hunter <jon-hunter@ti.com> [130410 10:35]:
> >> Hi Tony,
> >>
> >> On 04/09/2013 04:23 PM, Tony Lindgren wrote:
> >>> Hi Jon,
> >>>
> >>> Looks like at least 4430sdp nfsroot got broken with commit
> >>> ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
> >>> property).
> >>
> >> Thanks for reporting. I am actually amazed that ethernet is 
> >> working on any OMAP board (with device-tree) that requires a
> >> gpio as an interrupt because we have still not come to an
> >> agreement on [1]. Looking at the OMAP4 SDP I believe this is
> >> working by luck because there are other gpios in the same
> >> bank that are active and so the bank is enabled. If that were
> >> not the case then this would not work. 
> >>
> >> Hence, I have not been testing ethernet when booting with 
> >> device-tree because we need to fix [1] which has been really
> >> crippling my testing. So this needs to be resolved so ethernet
> >> can work consistently on all omap boards.
> > 
> > Cool.
> >   
> >>> Do we need to pass the GPIO edge/level info now?
> >>
> >> Yes and here is a patch. I have tested on my OMAP4 SDP this
> >> morning. Reviewing all the OMAP/AM dts(i) files I believe
> >> that only the SDP and OMAP4-VAR-SOM are impacted by this. I
> >> don't see any other boards currently using the gpio as an 
> >> interrupt-controller.
> > 
> > Great. Just one comment: You might be able to already use
> > the DT preprocessor now easily with linux next for the edge
> > info.
> 
> Good idea. We could do something like the below, but it would
> be better to include the "arm-gic.h" from the omap4.dtsi file
> so that the definitions can be used there too. The problem is
> that if I include "arm-gic.h" in the omap4.dtsi file I get a
> DTC compiler error because of the "#include" statement. It
> appears that the compiler does not like it if a file includes
> a file using "/include/" and the file it is including uses
> "#include". So really I think that we need to switch all the
> dts/dtsi files to use "#include" first.

OK maybe note that in the description too.

Also, is there need to back port these two patches to v3.9-rc?
If nobody has noticed, I guess not?

Regards,

Tony

Re: 4430sdp nfsroot broken with ff5c9059

[Jon Hunter]

On 04/10/2013 03:54 PM, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [130410 13:47]:
>>
>> On 04/10/2013 01:20 PM, Tony Lindgren wrote:
>>> * Jon Hunter <jon-hunter@ti.com> [130410 10:35]:
>>>> Hi Tony,
>>>>
>>>> On 04/09/2013 04:23 PM, Tony Lindgren wrote:
>>>>> Hi Jon,
>>>>>
>>>>> Looks like at least 4430sdp nfsroot got broken with commit
>>>>> ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
>>>>> property).
>>>>
>>>> Thanks for reporting. I am actually amazed that ethernet is 
>>>> working on any OMAP board (with device-tree) that requires a
>>>> gpio as an interrupt because we have still not come to an
>>>> agreement on [1]. Looking at the OMAP4 SDP I believe this is
>>>> working by luck because there are other gpios in the same
>>>> bank that are active and so the bank is enabled. If that were
>>>> not the case then this would not work. 
>>>>
>>>> Hence, I have not been testing ethernet when booting with 
>>>> device-tree because we need to fix [1] which has been really
>>>> crippling my testing. So this needs to be resolved so ethernet
>>>> can work consistently on all omap boards.
>>>
>>> Cool.
>>>   
>>>>> Do we need to pass the GPIO edge/level info now?
>>>>
>>>> Yes and here is a patch. I have tested on my OMAP4 SDP this
>>>> morning. Reviewing all the OMAP/AM dts(i) files I believe
>>>> that only the SDP and OMAP4-VAR-SOM are impacted by this. I
>>>> don't see any other boards currently using the gpio as an 
>>>> interrupt-controller.
>>>
>>> Great. Just one comment: You might be able to already use
>>> the DT preprocessor now easily with linux next for the edge
>>> info.
>>
>> Good idea. We could do something like the below, but it would
>> be better to include the "arm-gic.h" from the omap4.dtsi file
>> so that the definitions can be used there too. The problem is
>> that if I include "arm-gic.h" in the omap4.dtsi file I get a
>> DTC compiler error because of the "#include" statement. It
>> appears that the compiler does not like it if a file includes
>> a file using "/include/" and the file it is including uses
>> "#include". So really I think that we need to switch all the
>> dts/dtsi files to use "#include" first.
> 
> OK maybe note that in the description too.

Are you saying that you want to use the version with that is using the
pre-processor definitions? I was thinking that may be we could do that
as a clean-up for v3.11 and just use the original version I posted
earlier. Seems cleaner to me.

> Also, is there need to back port these two patches to v3.9-rc?
> If nobody has noticed, I guess not?

This should only be applicable to v3.10 so no need to backport. Or are
you saying that you are seeing this with v3.9 also? That would be odd.

Cheers
Jon

Re: 4430sdp nfsroot broken with ff5c9059

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [130410 15:30]:
> 
> Are you saying that you want to use the version with that is using the
> pre-processor definitions? I was thinking that may be we could do that
> as a clean-up for v3.11 and just use the original version I posted
> earlier. Seems cleaner to me.

No let's do that the preprocessor conversion for v3.11.
 
> > Also, is there need to back port these two patches to v3.9-rc?
> > If nobody has noticed, I guess not?
> 
> This should only be applicable to v3.10 so no need to backport. Or are
> you saying that you are seeing this with v3.9 also? That would be odd.

Not seeing it in v3.9, but I was wondering if it's lurking there too
in some format. But I guess not.

Regards,

Tony

Re: 4430sdp nfsroot broken with ff5c9059

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [130410 15:32]:
> * Jon Hunter <jon-hunter@ti.com> [130410 15:30]:
> > 
> > Are you saying that you want to use the version with that is using the
> > pre-processor definitions? I was thinking that may be we could do that
> > as a clean-up for v3.11 and just use the original version I posted
> > earlier. Seems cleaner to me.
> 
> No let's do that the preprocessor conversion for v3.11.

Hmm looks like there are few more 3430sdp dt nfsroot exposed
issues in today's linux next.

To get nfsroot to behave, I had to have your earlier fix
from this thread and also revert a2797bea (gpio/omap: force
restore if context loss is not detectable).

Otherwise nfsroot fails, but not necessarily every time?

I could not git bisect it down to the commit above, had to
manually figure it out.. There may also be DMA related
issues, but I don't know for sure any longer. I made a
patch to fix SPI PIO mode and then hacked SPI to always use
PIO to leave out the DMA related parts. Anyways, will post
that separately, let's hope the DMA related issues I saw
earlier are also related to dt + a2797bea.

Regards,

Tony

Re: 4430sdp nfsroot broken with ff5c9059

[Jon Hunter]

On 04/12/2013 07:06 PM, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [130410 15:32]:
>> * Jon Hunter <jon-hunter@ti.com> [130410 15:30]:
>>>
>>> Are you saying that you want to use the version with that is using the
>>> pre-processor definitions? I was thinking that may be we could do that
>>> as a clean-up for v3.11 and just use the original version I posted
>>> earlier. Seems cleaner to me.
>>
>> No let's do that the preprocessor conversion for v3.11.
> 
> Hmm looks like there are few more 3430sdp dt nfsroot exposed
> issues in today's linux next.

I don't even see any ethernet devices defined in the omap3430-sdp.dts
file. Is this something that you have added on top?

> To get nfsroot to behave, I had to have your earlier fix
> from this thread and also revert a2797bea (gpio/omap: force
> restore if context loss is not detectable).
> 
> Otherwise nfsroot fails, but not necessarily every time?

Well this patch is going to force a gpio restore everytime we call
pm_runtime_get() when the use-count is 0. Yes this is not efficient,
however, without this patch you run the risk of context being lost and
you would never know. Per the changelog, long term a better solution is
needed.

> I could not git bisect it down to the commit above, had to
> manually figure it out.. There may also be DMA related
> issues, but I don't know for sure any longer. I made a
> patch to fix SPI PIO mode and then hacked SPI to always use
> PIO to leave out the DMA related parts. Anyways, will post
> that separately, let's hope the DMA related issues I saw
> earlier are also related to dt + a2797bea.

Ok, I don't follow that. I am not sure how gpio is related to DMA in
this case.

Cheers
Jon

Re: 4430sdp nfsroot broken with ff5c9059

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [130412 19:22]:
> 
> On 04/12/2013 07:06 PM, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [130410 15:32]:
> >> * Jon Hunter <jon-hunter@ti.com> [130410 15:30]:
> >>>
> >>> Are you saying that you want to use the version with that is using the
> >>> pre-processor definitions? I was thinking that may be we could do that
> >>> as a clean-up for v3.11 and just use the original version I posted
> >>> earlier. Seems cleaner to me.
> >>
> >> No let's do that the preprocessor conversion for v3.11.
> > 
> > Hmm looks like there are few more 3430sdp dt nfsroot exposed
> > issues in today's linux next.
> 
> I don't even see any ethernet devices defined in the omap3430-sdp.dts
> file. Is this something that you have added on top?

Oops sorry I meant the ks8851 on SPI on 4430sdp, not 3430.
 
> > To get nfsroot to behave, I had to have your earlier fix
> > from this thread and also revert a2797bea (gpio/omap: force
> > restore if context loss is not detectable).
> > 
> > Otherwise nfsroot fails, but not necessarily every time?
> 
> Well this patch is going to force a gpio restore everytime we call
> pm_runtime_get() when the use-count is 0. Yes this is not efficient,
> however, without this patch you run the risk of context being lost and
> you would never know. Per the changelog, long term a better solution is
> needed.

It seems that this patch kills the ks8851 GPIO interrupt somehow,
at least most of the time.
 
> > I could not git bisect it down to the commit above, had to
> > manually figure it out.. There may also be DMA related
> > issues, but I don't know for sure any longer. I made a
> > patch to fix SPI PIO mode and then hacked SPI to always use
> > PIO to leave out the DMA related parts. Anyways, will post
> > that separately, let's hope the DMA related issues I saw
> > earlier are also related to dt + a2797bea.
> 
> Ok, I don't follow that. I am not sure how gpio is related to DMA in
> this case.

I think the DMA issues might be related to the GPIO interrupt
not working properly.

Regards,

Tony

Re: 4430sdp nfsroot broken with ff5c9059

[Jon Hunter]

On 04/13/2013 11:50 AM, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [130412 19:22]:
>>
>> On 04/12/2013 07:06 PM, Tony Lindgren wrote:
>>> * Tony Lindgren <tony@atomide.com> [130410 15:32]:
>>>> * Jon Hunter <jon-hunter@ti.com> [130410 15:30]:
>>>>>
>>>>> Are you saying that you want to use the version with that is using the
>>>>> pre-processor definitions? I was thinking that may be we could do that
>>>>> as a clean-up for v3.11 and just use the original version I posted
>>>>> earlier. Seems cleaner to me.
>>>>
>>>> No let's do that the preprocessor conversion for v3.11.
>>>
>>> Hmm looks like there are few more 3430sdp dt nfsroot exposed
>>> issues in today's linux next.
>>
>> I don't even see any ethernet devices defined in the omap3430-sdp.dts
>> file. Is this something that you have added on top?
> 
> Oops sorry I meant the ks8851 on SPI on 4430sdp, not 3430.

Ok makes sense.

>>> To get nfsroot to behave, I had to have your earlier fix
>>> from this thread and also revert a2797bea (gpio/omap: force
>>> restore if context loss is not detectable).
>>>
>>> Otherwise nfsroot fails, but not necessarily every time?
>>
>> Well this patch is going to force a gpio restore everytime we call
>> pm_runtime_get() when the use-count is 0. Yes this is not efficient,
>> however, without this patch you run the risk of context being lost and
>> you would never know. Per the changelog, long term a better solution is
>> needed.
> 
> It seems that this patch kills the ks8851 GPIO interrupt somehow,
> at least most of the time.

Hmmm, let me go back and re-test this.

>>> I could not git bisect it down to the commit above, had to
>>> manually figure it out.. There may also be DMA related
>>> issues, but I don't know for sure any longer. I made a
>>> patch to fix SPI PIO mode and then hacked SPI to always use
>>> PIO to leave out the DMA related parts. Anyways, will post
>>> that separately, let's hope the DMA related issues I saw
>>> earlier are also related to dt + a2797bea.
>>
>> Ok, I don't follow that. I am not sure how gpio is related to DMA in
>> this case.
> 
> I think the DMA issues might be related to the GPIO interrupt
> not working properly.

What is the use-case? DMA + SPI? Talking to what on what board?

Cheers
Jon

Re: 4430sdp nfsroot broken with ff5c9059

[Jon Hunter]

On 04/15/2013 11:57 AM, Jon Hunter wrote:
> 
> On 04/13/2013 11:50 AM, Tony Lindgren wrote:
>> * Jon Hunter <jon-hunter@ti.com> [130412 19:22]:
>>>
>>> On 04/12/2013 07:06 PM, Tony Lindgren wrote:
>>>> * Tony Lindgren <tony@atomide.com> [130410 15:32]:
>>>>> * Jon Hunter <jon-hunter@ti.com> [130410 15:30]:
>>>>>>
>>>>>> Are you saying that you want to use the version with that is using the
>>>>>> pre-processor definitions? I was thinking that may be we could do that
>>>>>> as a clean-up for v3.11 and just use the original version I posted
>>>>>> earlier. Seems cleaner to me.
>>>>>
>>>>> No let's do that the preprocessor conversion for v3.11.
>>>>
>>>> Hmm looks like there are few more 3430sdp dt nfsroot exposed
>>>> issues in today's linux next.
>>>
>>> I don't even see any ethernet devices defined in the omap3430-sdp.dts
>>> file. Is this something that you have added on top?
>>
>> Oops sorry I meant the ks8851 on SPI on 4430sdp, not 3430.
> 
> Ok makes sense.
> 
>>>> To get nfsroot to behave, I had to have your earlier fix
>>>> from this thread and also revert a2797bea (gpio/omap: force
>>>> restore if context loss is not detectable).
>>>>
>>>> Otherwise nfsroot fails, but not necessarily every time?
>>>
>>> Well this patch is going to force a gpio restore everytime we call
>>> pm_runtime_get() when the use-count is 0. Yes this is not efficient,
>>> however, without this patch you run the risk of context being lost and
>>> you would never know. Per the changelog, long term a better solution is
>>> needed.
>>
>> It seems that this patch kills the ks8851 GPIO interrupt somehow,
>> at least most of the time.
> 
> Hmmm, let me go back and re-test this.

Ok I am seeing the problem. Looks like this patch exposed another
bug in the gpio driver. Seems that we never initialise the gpio
context during the probe and so on the first pm_runtime_get_sync()
during the probe we are restoring crap. Can you try this? If this
works, I will add a changelog and post as a fix.

Jon

>From 56598ba51a75481b050433bb38b7ae31a5ed4ae8 Mon Sep 17 00:00:00 2001
From: Jon Hunter <jon-hunter@ti.com>
Date: Mon, 15 Apr 2013 13:06:54 -0500
Subject: [PATCH] gpio/omap: ensure gpio context is initialised

---
 drivers/gpio/gpio-omap.c |   42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 0557529..53bb8d5 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -70,6 +70,7 @@ struct gpio_bank {
 	bool is_mpuio;
 	bool dbck_flag;
 	bool loses_context;
+	bool context_setup;
 	int stride;
 	u32 width;
 	int context_loss_count;
@@ -1085,6 +1086,7 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
 }
 
 static const struct of_device_id omap_gpio_match[];
+static void omap_gpio_setup_context(struct gpio_bank *p);
 
 static int omap_gpio_probe(struct platform_device *pdev)
 {
@@ -1179,8 +1181,10 @@ static int omap_gpio_probe(struct platform_device *pdev)
 	omap_gpio_chip_init(bank);
 	omap_gpio_show_rev(bank);
 
-	if (bank->loses_context)
+	if (bank->loses_context) {
 		bank->get_context_loss_count = pdata->get_context_loss_count;
+		omap_gpio_setup_context(bank);
+	}
 
 	pm_runtime_put(bank->dev);
 
@@ -1385,8 +1389,43 @@ void omap2_gpio_resume_after_idle(void)
 }
 
 #if defined(CONFIG_PM_RUNTIME)
+static void omap_gpio_setup_context(struct gpio_bank *p)
+{
+	struct omap_gpio_reg_offs *regs = p->regs;
+	void __iomem *base = p->base;
+
+	p->context.wake_en = __raw_readl(base + regs->wkup_en);
+	p->context.ctrl = __raw_readl(base + regs->ctrl);
+	p->context.ctrl = __raw_readl(base + regs->ctrl);
+	p->context.leveldetect0 = __raw_readl(base + regs->leveldetect0);
+	p->context.leveldetect1 = __raw_readl(base + regs->leveldetect1);
+	p->context.risingdetect = __raw_readl(base + regs->risingdetect);
+	p->context.fallingdetect = __raw_readl(base + regs->fallingdetect);
+
+	if (regs->set_dataout && p->regs->clr_dataout)
+		p->context.dataout = __raw_readl(base + regs->set_dataout);
+	else
+		p->context.dataout = __raw_readl(base + regs->dataout);
+
+	p->context.oe = __raw_readl(base + regs->direction);
+
+	if (p->dbck_enable_mask) {
+		p->context.debounce = __raw_readl(base + regs->debounce);
+		p->context.debounce_en = __raw_readl(base + regs->debounce_en);
+	}
+
+	p->context.irqenable1 = __raw_readl(base + regs->irqenable);
+	p->context.irqenable2 = __raw_readl(base + regs->irqenable2);
+
+	p->context_setup = true;
+}
+
 static void omap_gpio_restore_context(struct gpio_bank *bank)
 {
+	/* If context has not been initialised yet, return */
+	if (!bank->context_setup)
+		return;
+
 	__raw_writel(bank->context.wake_en,
 				bank->base + bank->regs->wkup_en);
 	__raw_writel(bank->context.ctrl, bank->base + bank->regs->ctrl);
@@ -1422,6 +1461,7 @@ static void omap_gpio_restore_context(struct gpio_bank *bank)
 #else
 #define omap_gpio_runtime_suspend NULL
 #define omap_gpio_runtime_resume NULL
+#define omap_gpio_setup_context NULL
 #endif
 
 static const struct dev_pm_ops gpio_pm_ops = {
-- 
1.7.10.4

Re: 4430sdp nfsroot broken with ff5c9059

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [130415 11:15]:
> 
> On 04/15/2013 11:57 AM, Jon Hunter wrote:
> > 
> > On 04/13/2013 11:50 AM, Tony Lindgren wrote:
> >> * Jon Hunter <jon-hunter@ti.com> [130412 19:22]:
> >>>
> >>> On 04/12/2013 07:06 PM, Tony Lindgren wrote:
> >>>> * Tony Lindgren <tony@atomide.com> [130410 15:32]:
> >>>>> * Jon Hunter <jon-hunter@ti.com> [130410 15:30]:
> >>>>>>
> >>>>>> Are you saying that you want to use the version with that is using the
> >>>>>> pre-processor definitions? I was thinking that may be we could do that
> >>>>>> as a clean-up for v3.11 and just use the original version I posted
> >>>>>> earlier. Seems cleaner to me.
> >>>>>
> >>>>> No let's do that the preprocessor conversion for v3.11.
> >>>>
> >>>> Hmm looks like there are few more 3430sdp dt nfsroot exposed
> >>>> issues in today's linux next.
> >>>
> >>> I don't even see any ethernet devices defined in the omap3430-sdp.dts
> >>> file. Is this something that you have added on top?
> >>
> >> Oops sorry I meant the ks8851 on SPI on 4430sdp, not 3430.
> > 
> > Ok makes sense.
> > 
> >>>> To get nfsroot to behave, I had to have your earlier fix
> >>>> from this thread and also revert a2797bea (gpio/omap: force
> >>>> restore if context loss is not detectable).
> >>>>
> >>>> Otherwise nfsroot fails, but not necessarily every time?
> >>>
> >>> Well this patch is going to force a gpio restore everytime we call
> >>> pm_runtime_get() when the use-count is 0. Yes this is not efficient,
> >>> however, without this patch you run the risk of context being lost and
> >>> you would never know. Per the changelog, long term a better solution is
> >>> needed.
> >>
> >> It seems that this patch kills the ks8851 GPIO interrupt somehow,
> >> at least most of the time.
> > 
> > Hmmm, let me go back and re-test this.
> 
> Ok I am seeing the problem. Looks like this patch exposed another
> bug in the gpio driver. Seems that we never initialise the gpio
> context during the probe and so on the first pm_runtime_get_sync()
> during the probe we are restoring crap. Can you try this? If this
> works, I will add a changelog and post as a fix.
> 
> Jon
> 
> From 56598ba51a75481b050433bb38b7ae31a5ed4ae8 Mon Sep 17 00:00:00 2001
> From: Jon Hunter <jon-hunter@ti.com>
> Date: Mon, 15 Apr 2013 13:06:54 -0500
> Subject: [PATCH] gpio/omap: ensure gpio context is initialised

Seems to work thanks:

Tested-by: Tony Lindgren <tony@atomide.com> 

>  drivers/gpio/gpio-omap.c |   42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 0557529..53bb8d5 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -70,6 +70,7 @@ struct gpio_bank {
>  	bool is_mpuio;
>  	bool dbck_flag;
>  	bool loses_context;
> +	bool context_setup;
>  	int stride;
>  	u32 width;
>  	int context_loss_count;
> @@ -1085,6 +1086,7 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
>  }
>  
>  static const struct of_device_id omap_gpio_match[];
> +static void omap_gpio_setup_context(struct gpio_bank *p);
>  
>  static int omap_gpio_probe(struct platform_device *pdev)
>  {
> @@ -1179,8 +1181,10 @@ static int omap_gpio_probe(struct platform_device *pdev)
>  	omap_gpio_chip_init(bank);
>  	omap_gpio_show_rev(bank);
>  
> -	if (bank->loses_context)
> +	if (bank->loses_context) {
>  		bank->get_context_loss_count = pdata->get_context_loss_count;
> +		omap_gpio_setup_context(bank);
> +	}
>  
>  	pm_runtime_put(bank->dev);
>  
> @@ -1385,8 +1389,43 @@ void omap2_gpio_resume_after_idle(void)
>  }
>  
>  #if defined(CONFIG_PM_RUNTIME)
> +static void omap_gpio_setup_context(struct gpio_bank *p)
> +{
> +	struct omap_gpio_reg_offs *regs = p->regs;
> +	void __iomem *base = p->base;
> +
> +	p->context.wake_en = __raw_readl(base + regs->wkup_en);
> +	p->context.ctrl = __raw_readl(base + regs->ctrl);
> +	p->context.ctrl = __raw_readl(base + regs->ctrl);
> +	p->context.leveldetect0 = __raw_readl(base + regs->leveldetect0);
> +	p->context.leveldetect1 = __raw_readl(base + regs->leveldetect1);
> +	p->context.risingdetect = __raw_readl(base + regs->risingdetect);
> +	p->context.fallingdetect = __raw_readl(base + regs->fallingdetect);
> +
> +	if (regs->set_dataout && p->regs->clr_dataout)
> +		p->context.dataout = __raw_readl(base + regs->set_dataout);
> +	else
> +		p->context.dataout = __raw_readl(base + regs->dataout);
> +
> +	p->context.oe = __raw_readl(base + regs->direction);
> +
> +	if (p->dbck_enable_mask) {
> +		p->context.debounce = __raw_readl(base + regs->debounce);
> +		p->context.debounce_en = __raw_readl(base + regs->debounce_en);
> +	}
> +
> +	p->context.irqenable1 = __raw_readl(base + regs->irqenable);
> +	p->context.irqenable2 = __raw_readl(base + regs->irqenable2);
> +
> +	p->context_setup = true;
> +}
> +
>  static void omap_gpio_restore_context(struct gpio_bank *bank)
>  {
> +	/* If context has not been initialised yet, return */
> +	if (!bank->context_setup)
> +		return;
> +
>  	__raw_writel(bank->context.wake_en,
>  				bank->base + bank->regs->wkup_en);
>  	__raw_writel(bank->context.ctrl, bank->base + bank->regs->ctrl);
> @@ -1422,6 +1461,7 @@ static void omap_gpio_restore_context(struct gpio_bank *bank)
>  #else
>  #define omap_gpio_runtime_suspend NULL
>  #define omap_gpio_runtime_resume NULL
> +#define omap_gpio_setup_context NULL
>  #endif
>  
>  static const struct dev_pm_ops gpio_pm_ops = {
> -- 
> 1.7.10.4
> 

Re: 4430sdp nfsroot broken with ff5c9059

[Jon Hunter]

On 04/15/2013 04:27 PM, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [130415 11:15]:

...

>> From 56598ba51a75481b050433bb38b7ae31a5ed4ae8 Mon Sep 17 00:00:00 2001
>> From: Jon Hunter <jon-hunter@ti.com>
>> Date: Mon, 15 Apr 2013 13:06:54 -0500
>> Subject: [PATCH] gpio/omap: ensure gpio context is initialised
> 
> Seems to work thanks:
> 
> Tested-by: Tony Lindgren <tony@atomide.com> 

Thanks. I have been cleaning this up and updating a little. Care to
re-test this version? If it is good I will send it out.

By the way, I realise I need to add a similar fix for dmtimer :-p

Cheers
Jon

>From 3980952af35054e50d743691fc3e0867de5b0f4d Mon Sep 17 00:00:00 2001
From: Jon Hunter <jon-hunter@ti.com>
Date: Mon, 15 Apr 2013 13:06:54 -0500
Subject: [PATCH] gpio/omap: ensure gpio context is initialised

Commit a2797be (gpio/omap: force restore if context loss is not
detectable) broke gpio support for OMAP when booting with device-tree
because a restore of the gpio context being performed without ever
initialising the gpio context. In other words, the context restored was
bad.

This problem could also occur in the non device-tree case, however, it
is much less likely because when booting without device-tree we can
detect context loss via a platform specific API and so context restore
is performed less often.

Nevertheless we should ensure that the gpio context is initialised
during the probe for gpio banks that could lose their state regardless
of whether we are booting with device-tree or not.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 drivers/gpio/gpio-omap.c |   53 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 0557529..444451b 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -70,6 +70,7 @@ struct gpio_bank {
 	bool is_mpuio;
 	bool dbck_flag;
 	bool loses_context;
+	bool context_valid;
 	int stride;
 	u32 width;
 	int context_loss_count;
@@ -1085,6 +1086,7 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
 }
 
 static const struct of_device_id omap_gpio_match[];
+static void omap_gpio_init_context(struct gpio_bank *p);
 
 static int omap_gpio_probe(struct platform_device *pdev)
 {
@@ -1179,8 +1181,10 @@ static int omap_gpio_probe(struct platform_device *pdev)
 	omap_gpio_chip_init(bank);
 	omap_gpio_show_rev(bank);
 
-	if (bank->loses_context)
+	if (bank->loses_context) {
 		bank->get_context_loss_count = pdata->get_context_loss_count;
+		omap_gpio_init_context(bank);
+	}
 
 	pm_runtime_put(bank->dev);
 
@@ -1269,6 +1273,14 @@ static int omap_gpio_runtime_resume(struct device *dev)
 	int c;
 
 	spin_lock_irqsave(&bank->lock, flags);
+
+	/*
+	 * On the first resume during the probe, the context has not
+	 * been initialised and so if the context is not valid return.
+	 */
+	if (!bank->context_valid)
+		goto done;
+
 	_gpio_dbck_enable(bank);
 
 	/*
@@ -1287,19 +1299,15 @@ static int omap_gpio_runtime_resume(struct device *dev)
 			omap_gpio_restore_context(bank);
 		} else {
 			c = bank->get_context_loss_count(bank->dev);
-			if (c != bank->context_loss_count) {
+			if (c != bank->context_loss_count)
 				omap_gpio_restore_context(bank);
-			} else {
-				spin_unlock_irqrestore(&bank->lock, flags);
-				return 0;
-			}
+			else
+				goto done;
 		}
 	}
 
-	if (!bank->workaround_enabled) {
-		spin_unlock_irqrestore(&bank->lock, flags);
-		return 0;
-	}
+	if (!bank->workaround_enabled)
+		goto done;
 
 	l = __raw_readl(bank->base + bank->regs->datain);
 
@@ -1352,6 +1360,7 @@ static int omap_gpio_runtime_resume(struct device *dev)
 	}
 
 	bank->workaround_enabled = false;
+done:
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	return 0;
@@ -1385,6 +1394,29 @@ void omap2_gpio_resume_after_idle(void)
 }
 
 #if defined(CONFIG_PM_RUNTIME)
+static void omap_gpio_init_context(struct gpio_bank *p)
+{
+	struct omap_gpio_reg_offs *regs = p->regs;
+	void __iomem *base = p->base;
+
+	p->context.ctrl		= __raw_readl(base + regs->ctrl);
+	p->context.oe		= __raw_readl(base + regs->direction);
+	p->context.wake_en	= __raw_readl(base + regs->wkup_en);
+	p->context.leveldetect0	= __raw_readl(base + regs->leveldetect0);
+	p->context.leveldetect1	= __raw_readl(base + regs->leveldetect1);
+	p->context.risingdetect	= __raw_readl(base + regs->risingdetect);
+	p->context.fallingdetect = __raw_readl(base + regs->fallingdetect);
+	p->context.irqenable1	= __raw_readl(base + regs->irqenable);
+	p->context.irqenable2	= __raw_readl(base + regs->irqenable2);
+
+	if (regs->set_dataout && p->regs->clr_dataout)
+		p->context.dataout = __raw_readl(base + regs->set_dataout);
+	else
+		p->context.dataout = __raw_readl(base + regs->dataout);
+
+	p->context_valid = true;
+}
+
 static void omap_gpio_restore_context(struct gpio_bank *bank)
 {
 	__raw_writel(bank->context.wake_en,
@@ -1422,6 +1454,7 @@ static void omap_gpio_restore_context(struct gpio_bank *bank)
 #else
 #define omap_gpio_runtime_suspend NULL
 #define omap_gpio_runtime_resume NULL
+#define omap_gpio_init_context NULL
 #endif
 
 static const struct dev_pm_ops gpio_pm_ops = {
-- 
1.7.10.4

Re: 4430sdp nfsroot broken with ff5c9059

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [130416 10:19]:
> 
> On 04/15/2013 04:27 PM, Tony Lindgren wrote:
> > * Jon Hunter <jon-hunter@ti.com> [130415 11:15]:
> 
> ...
> 
> >> From 56598ba51a75481b050433bb38b7ae31a5ed4ae8 Mon Sep 17 00:00:00 2001
> >> From: Jon Hunter <jon-hunter@ti.com>
> >> Date: Mon, 15 Apr 2013 13:06:54 -0500
> >> Subject: [PATCH] gpio/omap: ensure gpio context is initialised
> > 
> > Seems to work thanks:
> > 
> > Tested-by: Tony Lindgren <tony@atomide.com> 
> 
> Thanks. I have been cleaning this up and updating a little. Care to
> re-test this version? If it is good I will send it out.

Yes this works too, you can recycle my Tested-by on this one.
 
> By the way, I realise I need to add a similar fix for dmtimer :-p

OK good to find out now rather than after the merge window.

Regards,

Tony

Re: 4430sdp nfsroot broken with ff5c9059

[Jon Hunter]

On 04/16/2013 01:06 PM, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [130416 10:19]:
>>
>> On 04/15/2013 04:27 PM, Tony Lindgren wrote:
>>> * Jon Hunter <jon-hunter@ti.com> [130415 11:15]:
>>
>> ...
>>
>>>> From 56598ba51a75481b050433bb38b7ae31a5ed4ae8 Mon Sep 17 00:00:00 2001
>>>> From: Jon Hunter <jon-hunter@ti.com>
>>>> Date: Mon, 15 Apr 2013 13:06:54 -0500
>>>> Subject: [PATCH] gpio/omap: ensure gpio context is initialised
>>>
>>> Seems to work thanks:
>>>
>>> Tested-by: Tony Lindgren <tony@atomide.com> 
>>
>> Thanks. I have been cleaning this up and updating a little. Care to
>> re-test this version? If it is good I will send it out.
> 
> Yes this works too, you can recycle my Tested-by on this one.

Thanks. Final version below to fix build error if pm-runtime is not
enabled (this was breaking omap1 build). 
  
>> By the way, I realise I need to add a similar fix for dmtimer :-p
> 
> OK good to find out now rather than after the merge window.

Actually, I am wrong here. The dmtimer registers that are restored
all default to zero and so we don't need a similar fix for dmtimers. 

Cheers
Jon

>From 79fb33de1ada7cce7527adb15376bb7c5ddc4f14 Mon Sep 17 00:00:00 2001
From: Jon Hunter <jon-hunter@ti.com>
Date: Mon, 15 Apr 2013 13:06:54 -0500
Subject: [PATCH] gpio/omap: ensure gpio context is initialised

Commit a2797be (gpio/omap: force restore if context loss is not
detectable) broke gpio support for OMAP when booting with device-tree
because a restore of the gpio context being performed without ever
initialising the gpio context. In other words, the context restored was
bad.

This problem could also occur in the non device-tree case, however, it
is much less likely because when booting without device-tree we can
detect context loss via a platform specific API and so context restore
is performed less often.

Nevertheless we should ensure that the gpio context is initialised
during the probe for gpio banks that could lose their state regardless
of whether we are booting with device-tree or not.

Reported-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Jon Hunter <jon-hunter@ti.com>
Tested-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpio/gpio-omap.c |   53 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 0557529..0ba5cb9 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -70,6 +70,7 @@ struct gpio_bank {
 	bool is_mpuio;
 	bool dbck_flag;
 	bool loses_context;
+	bool context_valid;
 	int stride;
 	u32 width;
 	int context_loss_count;
@@ -1085,6 +1086,7 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
 }
 
 static const struct of_device_id omap_gpio_match[];
+static void omap_gpio_init_context(struct gpio_bank *p);
 
 static int omap_gpio_probe(struct platform_device *pdev)
 {
@@ -1179,8 +1181,10 @@ static int omap_gpio_probe(struct platform_device *pdev)
 	omap_gpio_chip_init(bank);
 	omap_gpio_show_rev(bank);
 
-	if (bank->loses_context)
+	if (bank->loses_context) {
 		bank->get_context_loss_count = pdata->get_context_loss_count;
+		omap_gpio_init_context(bank);
+	}
 
 	pm_runtime_put(bank->dev);
 
@@ -1269,6 +1273,14 @@ static int omap_gpio_runtime_resume(struct device *dev)
 	int c;
 
 	spin_lock_irqsave(&bank->lock, flags);
+
+	/*
+	 * On the first resume during the probe, the context has not
+	 * been initialised and so if the context is not valid return.
+	 */
+	if (!bank->context_valid)
+		goto done;
+
 	_gpio_dbck_enable(bank);
 
 	/*
@@ -1287,19 +1299,15 @@ static int omap_gpio_runtime_resume(struct device *dev)
 			omap_gpio_restore_context(bank);
 		} else {
 			c = bank->get_context_loss_count(bank->dev);
-			if (c != bank->context_loss_count) {
+			if (c != bank->context_loss_count)
 				omap_gpio_restore_context(bank);
-			} else {
-				spin_unlock_irqrestore(&bank->lock, flags);
-				return 0;
-			}
+			else
+				goto done;
 		}
 	}
 
-	if (!bank->workaround_enabled) {
-		spin_unlock_irqrestore(&bank->lock, flags);
-		return 0;
-	}
+	if (!bank->workaround_enabled)
+		goto done;
 
 	l = __raw_readl(bank->base + bank->regs->datain);
 
@@ -1352,6 +1360,7 @@ static int omap_gpio_runtime_resume(struct device *dev)
 	}
 
 	bank->workaround_enabled = false;
+done:
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	return 0;
@@ -1385,6 +1394,29 @@ void omap2_gpio_resume_after_idle(void)
 }
 
 #if defined(CONFIG_PM_RUNTIME)
+static void omap_gpio_init_context(struct gpio_bank *p)
+{
+	struct omap_gpio_reg_offs *regs = p->regs;
+	void __iomem *base = p->base;
+
+	p->context.ctrl		= __raw_readl(base + regs->ctrl);
+	p->context.oe		= __raw_readl(base + regs->direction);
+	p->context.wake_en	= __raw_readl(base + regs->wkup_en);
+	p->context.leveldetect0	= __raw_readl(base + regs->leveldetect0);
+	p->context.leveldetect1	= __raw_readl(base + regs->leveldetect1);
+	p->context.risingdetect	= __raw_readl(base + regs->risingdetect);
+	p->context.fallingdetect = __raw_readl(base + regs->fallingdetect);
+	p->context.irqenable1	= __raw_readl(base + regs->irqenable);
+	p->context.irqenable2	= __raw_readl(base + regs->irqenable2);
+
+	if (regs->set_dataout && p->regs->clr_dataout)
+		p->context.dataout = __raw_readl(base + regs->set_dataout);
+	else
+		p->context.dataout = __raw_readl(base + regs->dataout);
+
+	p->context_valid = true;
+}
+
 static void omap_gpio_restore_context(struct gpio_bank *bank)
 {
 	__raw_writel(bank->context.wake_en,
@@ -1422,6 +1454,7 @@ static void omap_gpio_restore_context(struct gpio_bank *bank)
 #else
 #define omap_gpio_runtime_suspend NULL
 #define omap_gpio_runtime_resume NULL
+static void omap_gpio_init_context(struct gpio_bank *p) {}
 #endif
 
 static const struct dev_pm_ops gpio_pm_ops = {
-- 
1.7.10.4

Re: 4430sdp nfsroot broken with ff5c9059

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [130416 11:45]:
> 
> On 04/16/2013 01:06 PM, Tony Lindgren wrote:
> > * Jon Hunter <jon-hunter@ti.com> [130416 10:19]:
> >>
> >> On 04/15/2013 04:27 PM, Tony Lindgren wrote:
> >>> * Jon Hunter <jon-hunter@ti.com> [130415 11:15]:
> >>
> >> ...
> >>
> >>>> From 56598ba51a75481b050433bb38b7ae31a5ed4ae8 Mon Sep 17 00:00:00 2001
> >>>> From: Jon Hunter <jon-hunter@ti.com>
> >>>> Date: Mon, 15 Apr 2013 13:06:54 -0500
> >>>> Subject: [PATCH] gpio/omap: ensure gpio context is initialised
> >>>
> >>> Seems to work thanks:
> >>>
> >>> Tested-by: Tony Lindgren <tony@atomide.com> 
> >>
> >> Thanks. I have been cleaning this up and updating a little. Care to
> >> re-test this version? If it is good I will send it out.
> > 
> > Yes this works too, you can recycle my Tested-by on this one.
> 
> Thanks. Final version below to fix build error if pm-runtime is not
> enabled (this was breaking omap1 build). 
>   
> >> By the way, I realise I need to add a similar fix for dmtimer :-p
> > 
> > OK good to find out now rather than after the merge window.
> 
> Actually, I am wrong here. The dmtimer registers that are restored
> all default to zero and so we don't need a similar fix for dmtimers. 

OK assuming you'll send this patch to the usual drivers/gpio
maintainers so it can be merged.

Regards,

Tony

Re: 4430sdp nfsroot broken with ff5c9059

[Javier Martinez Canillas]

On Wed, Apr 10, 2013 at 7:31 PM, Jon Hunter <jon-hunter@ti.com> wrote:
> Hi Tony,
>
> On 04/09/2013 04:23 PM, Tony Lindgren wrote:
>> Hi Jon,
>>
>> Looks like at least 4430sdp nfsroot got broken with commit
>> ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
>> property).
>
> Thanks for reporting. I am actually amazed that ethernet is
> working on any OMAP board (with device-tree) that requires a
> gpio as an interrupt because we have still not come to an
> agreement on [1]. Looking at the OMAP4 SDP I believe this is
> working by luck because there are other gpios in the same
> bank that are active and so the bank is enabled. If that were
> not the case then this would not work.
>

Hi Jon,

Ethernet is working on 4430sdp since the optional "gpio" property is
specified on the fixed regulator used by the eth device node.

>From arch/arm/boot/dts/omap4-sdp.dts:

        vdd_eth: fixedregulator-vdd-eth {
                compatible = "regulator-fixed";
                regulator-name = "VDD_ETH";
                regulator-min-microvolt = <3300000>;
                regulator-max-microvolt = <3300000>;
                gpio = <&gpio2 16 0>;  /* gpio line 48 */
                enable-active-high;
                regulator-boot-on;
        };
...
&mcspi1 {
        eth@0 {
                compatible = "ks8851";
                spi-max-frequency = <24000000>;
                reg = <0>;
                interrupt-parent = <&gpio2>;
                interrupts = <2>; /* gpio line 34 */
                vdd-supply = <&vdd_eth>;
        };
};

So is the regulator who is calling gpio_request() and enabling the
GPIO bank and no the ks881 ethernet driver. That's why it was working
although I think is just a DT hack and should be changed once we found
a proper solution to fhis.

Best regards,
Javier

Re: 4430sdp nfsroot broken with ff5c9059

[Jon Hunter]

On 04/10/2013 06:58 PM, Javier Martinez Canillas wrote:
> On Wed, Apr 10, 2013 at 7:31 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>> Hi Tony,
>>
>> On 04/09/2013 04:23 PM, Tony Lindgren wrote:
>>> Hi Jon,
>>>
>>> Looks like at least 4430sdp nfsroot got broken with commit
>>> ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
>>> property).
>>
>> Thanks for reporting. I am actually amazed that ethernet is
>> working on any OMAP board (with device-tree) that requires a
>> gpio as an interrupt because we have still not come to an
>> agreement on [1]. Looking at the OMAP4 SDP I believe this is
>> working by luck because there are other gpios in the same
>> bank that are active and so the bank is enabled. If that were
>> not the case then this would not work.
>>
> 
> Hi Jon,
> 
> Ethernet is working on 4430sdp since the optional "gpio" property is
> specified on the fixed regulator used by the eth device node.
> 
> From arch/arm/boot/dts/omap4-sdp.dts:
> 
>         vdd_eth: fixedregulator-vdd-eth {
>                 compatible = "regulator-fixed";
>                 regulator-name = "VDD_ETH";
>                 regulator-min-microvolt = <3300000>;
>                 regulator-max-microvolt = <3300000>;
>                 gpio = <&gpio2 16 0>;  /* gpio line 48 */
>                 enable-active-high;
>                 regulator-boot-on;
>         };
> ...
> &mcspi1 {
>         eth@0 {
>                 compatible = "ks8851";
>                 spi-max-frequency = <24000000>;
>                 reg = <0>;
>                 interrupt-parent = <&gpio2>;
>                 interrupts = <2>; /* gpio line 34 */
>                 vdd-supply = <&vdd_eth>;
>         };
> };
> 
> So is the regulator who is calling gpio_request() and enabling the
> GPIO bank and no the ks881 ethernet driver. That's why it was working
> although I think is just a DT hack and should be changed once we found
> a proper solution to fhis.

Right, in the above I mentioned it is working because of other gpios in
the bank that are enabled. So I understand why it works, but wanted to
point out that this is lucky :-)

Jon

Re: 4430sdp nfsroot broken with ff5c9059

[Javier Martinez Canillas]

On Thu, Apr 11, 2013 at 2:28 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 04/10/2013 06:58 PM, Javier Martinez Canillas wrote:
>> On Wed, Apr 10, 2013 at 7:31 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>>> Hi Tony,
>>>
>>> On 04/09/2013 04:23 PM, Tony Lindgren wrote:
>>>> Hi Jon,
>>>>
>>>> Looks like at least 4430sdp nfsroot got broken with commit
>>>> ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
>>>> property).
>>>
>>> Thanks for reporting. I am actually amazed that ethernet is
>>> working on any OMAP board (with device-tree) that requires a
>>> gpio as an interrupt because we have still not come to an
>>> agreement on [1]. Looking at the OMAP4 SDP I believe this is
>>> working by luck because there are other gpios in the same
>>> bank that are active and so the bank is enabled. If that were
>>> not the case then this would not work.
>>>
>>
>> Hi Jon,
>>
>> Ethernet is working on 4430sdp since the optional "gpio" property is
>> specified on the fixed regulator used by the eth device node.
>>
>> From arch/arm/boot/dts/omap4-sdp.dts:
>>
>>         vdd_eth: fixedregulator-vdd-eth {
>>                 compatible = "regulator-fixed";
>>                 regulator-name = "VDD_ETH";
>>                 regulator-min-microvolt = <3300000>;
>>                 regulator-max-microvolt = <3300000>;
>>                 gpio = <&gpio2 16 0>;  /* gpio line 48 */
>>                 enable-active-high;
>>                 regulator-boot-on;
>>         };
>> ...
>> &mcspi1 {
>>         eth@0 {
>>                 compatible = "ks8851";
>>                 spi-max-frequency = <24000000>;
>>                 reg = <0>;
>>                 interrupt-parent = <&gpio2>;
>>                 interrupts = <2>; /* gpio line 34 */
>>                 vdd-supply = <&vdd_eth>;
>>         };
>> };
>>
>> So is the regulator who is calling gpio_request() and enabling the
>> GPIO bank and no the ks881 ethernet driver. That's why it was working
>> although I think is just a DT hack and should be changed once we found
>> a proper solution to fhis.
>
> Right, in the above I mentioned it is working because of other gpios in
> the bank that are enabled. So I understand why it works, but wanted to
> point out that this is lucky :-)
>
> Jon
>

Indeed :-)

I just thought it could be good to explain this so others will
understand why it was working.

Sorry for the noise and I hope we find a solution for this soon

Javier

Re: 4430sdp nfsroot broken with ff5c9059

[Benoit Cousson]

Hi Javier,

On 04/11/2013 01:58 AM, Javier Martinez Canillas wrote:
> On Wed, Apr 10, 2013 at 7:31 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>> Hi Tony,
>>
>> On 04/09/2013 04:23 PM, Tony Lindgren wrote:
>>> Hi Jon,
>>>
>>> Looks like at least 4430sdp nfsroot got broken with commit
>>> ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
>>> property).
>>
>> Thanks for reporting. I am actually amazed that ethernet is
>> working on any OMAP board (with device-tree) that requires a
>> gpio as an interrupt because we have still not come to an
>> agreement on [1]. Looking at the OMAP4 SDP I believe this is
>> working by luck because there are other gpios in the same
>> bank that are active and so the bank is enabled. If that were
>> not the case then this would not work.
>>
> 
> Hi Jon,
> 
> Ethernet is working on 4430sdp since the optional "gpio" property is
> specified on the fixed regulator used by the eth device node.
> 
> From arch/arm/boot/dts/omap4-sdp.dts:
> 
>         vdd_eth: fixedregulator-vdd-eth {
>                 compatible = "regulator-fixed";
>                 regulator-name = "VDD_ETH";
>                 regulator-min-microvolt = <3300000>;
>                 regulator-max-microvolt = <3300000>;
>                 gpio = <&gpio2 16 0>;  /* gpio line 48 */
>                 enable-active-high;
>                 regulator-boot-on;
>         };
> ...
> &mcspi1 {
>         eth@0 {
>                 compatible = "ks8851";
>                 spi-max-frequency = <24000000>;
>                 reg = <0>;
>                 interrupt-parent = <&gpio2>;
>                 interrupts = <2>; /* gpio line 34 */
>                 vdd-supply = <&vdd_eth>;
>         };
> };
> 
> So is the regulator who is calling gpio_request() and enabling the
> GPIO bank and no the ks881 ethernet driver. That's why it was working
> although I think is just a DT hack and should be changed once we found
> a proper solution to fhis.

It is not really a hack. There is a regulator controlled by a GPIO that
can control the Ethernet chip power. Both gpio48 and gpio34 are used for
Ethernet. On some SDP version there is even a third line, but I was not
able to find any evidence in the schematic I was using.

Regards,
Benoit

Re: 4430sdp nfsroot broken with ff5c9059

[Javier Martinez Canillas]

On Thu, Apr 11, 2013 at 11:22 AM, Benoit Cousson <b-cousson@ti.com> wrote:
> Hi Javier,
>
> On 04/11/2013 01:58 AM, Javier Martinez Canillas wrote:
>> On Wed, Apr 10, 2013 at 7:31 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>>> Hi Tony,
>>>
>>> On 04/09/2013 04:23 PM, Tony Lindgren wrote:
>>>> Hi Jon,
>>>>
>>>> Looks like at least 4430sdp nfsroot got broken with commit
>>>> ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
>>>> property).
>>>
>>> Thanks for reporting. I am actually amazed that ethernet is
>>> working on any OMAP board (with device-tree) that requires a
>>> gpio as an interrupt because we have still not come to an
>>> agreement on [1]. Looking at the OMAP4 SDP I believe this is
>>> working by luck because there are other gpios in the same
>>> bank that are active and so the bank is enabled. If that were
>>> not the case then this would not work.
>>>
>>
>> Hi Jon,
>>
>> Ethernet is working on 4430sdp since the optional "gpio" property is
>> specified on the fixed regulator used by the eth device node.
>>
>> From arch/arm/boot/dts/omap4-sdp.dts:
>>
>>         vdd_eth: fixedregulator-vdd-eth {
>>                 compatible = "regulator-fixed";
>>                 regulator-name = "VDD_ETH";
>>                 regulator-min-microvolt = <3300000>;
>>                 regulator-max-microvolt = <3300000>;
>>                 gpio = <&gpio2 16 0>;  /* gpio line 48 */
>>                 enable-active-high;
>>                 regulator-boot-on;
>>         };
>> ...
>> &mcspi1 {
>>         eth@0 {
>>                 compatible = "ks8851";
>>                 spi-max-frequency = <24000000>;
>>                 reg = <0>;
>>                 interrupt-parent = <&gpio2>;
>>                 interrupts = <2>; /* gpio line 34 */
>>                 vdd-supply = <&vdd_eth>;
>>         };
>> };
>>
>> So is the regulator who is calling gpio_request() and enabling the
>> GPIO bank and no the ks881 ethernet driver. That's why it was working
>> although I think is just a DT hack and should be changed once we found
>> a proper solution to fhis.
>
> It is not really a hack. There is a regulator controlled by a GPIO that
> can control the Ethernet chip power. Both gpio48 and gpio34 are used for
> Ethernet. On some SDP version there is even a third line, but I was not
> able to find any evidence in the schematic I was using.
>
> Regards,
> Benoit
>

Hi Benoit,

I (wrongly) assumed that this was made just to bypass the fact that
neither the ks881 driver nor the DT core call omap_gpio_request() to
enable the GPIO bank module.

But now thanks to your explanation I understand that gpio48 is also
necessary to control the regulator and as Jon said the lucky fact is
that both GPIO 48 and 34 are from the same GPIO bank module (gpio2).
If that was not the case, this would probably not work.

I should learn to not comment without looking at the schematic first.

Thanks a lot and best regards,
Javier

Re: 4430sdp nfsroot broken with ff5c9059

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [130410 10:35]:
> Hi Tony,
> 
> On 04/09/2013 04:23 PM, Tony Lindgren wrote:
> > Hi Jon,
> > 
> > Looks like at least 4430sdp nfsroot got broken with commit
> > ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
> > property).
> 
> Thanks for reporting. I am actually amazed that ethernet is 
> working on any OMAP board (with device-tree) that requires a
> gpio as an interrupt because we have still not come to an
> agreement on [1]. Looking at the OMAP4 SDP I believe this is
> working by luck because there are other gpios in the same
> bank that are active and so the bank is enabled. If that were
> not the case then this would not work. 
> 
> Hence, I have not been testing ethernet when booting with 
> device-tree because we need to fix [1] which has been really
> crippling my testing. So this needs to be resolved so ethernet
> can work consistently on all omap boards.
>  
> > Do we need to pass the GPIO edge/level info now?
> 
> Yes and here is a patch. I have tested on my OMAP4 SDP this
> morning. Reviewing all the OMAP/AM dts(i) files I believe
> that only the SDP and OMAP4-VAR-SOM are impacted by this. I
> don't see any other boards currently using the gpio as an 
> interrupt-controller.

I'll apply this into omap-for-v3.10/dt-fixes. Benoit, I'll have
only dt stuff in that branch so it should play along with whatever
you'll be applying as well.

Regards,

Tony
 
> Cheers
> Jon
> 
> [1] comments.gmane.org/gmane.linux.ports.arm.omap/92192
> 
> From ed3e66bcd4c0cb3df4cb70e75c45fa4462f70565 Mon Sep 17 00:00:00 2001
> From: Jon Hunter <jon-hunter@ti.com>
> Date: Wed, 10 Apr 2013 11:44:44 -0500
> Subject: [PATCH] ARM: dts: OMAP4: Fix ethernet IRQ for OMAP4 boards
> 
> Commit ff5c9059 (ARM: dts: OMAP3+: Correct gpio #interrupts-cells
> property) updated the number of interrupt cells required for configuring
> gpios as interrupts for other devices (such as ethernet controllers).
> This update allowed the interrupt type (edge, level, etc) to be
> configured via device-tree (as described in the
> Documentation/devicetree/bindings/gpio/gpio-omap.txt).
> 
> This broke ethernet support on the OMAP4 SDP board that defines a gpio
> as the ethernet IRQ because the interrupt type (level, edge, etc) was
> not getting configured correctly. This board use the ks8851 ethernet
> chip which has an active low interrupt. Fix this by defining the gpio
> interrupt as active-low in the device-tree binding.
> 
> Please note that the OMAP4-VAR-SOM also uses the same ethernet
> controller and it is expected it will have the same problem. So the
> same fix is also applied to this board.
> 
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> ---
>  arch/arm/boot/dts/omap4-sdp.dts     |    2 +-
>  arch/arm/boot/dts/omap4-var-som.dts |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/omap4-sdp.dts b/arch/arm/boot/dts/omap4-sdp.dts
> index c387bdc..25cbe43 100644
> --- a/arch/arm/boot/dts/omap4-sdp.dts
> +++ b/arch/arm/boot/dts/omap4-sdp.dts
> @@ -363,7 +363,7 @@
>  		spi-max-frequency = <24000000>;
>  		reg = <0>;
>  		interrupt-parent = <&gpio2>;
> -		interrupts = <2>; /* gpio line 34 */
> +		interrupts = <2 8>; /* gpio line 34, low triggered */
>  		vdd-supply = <&vdd_eth>;
>  	};
>  };
> diff --git a/arch/arm/boot/dts/omap4-var-som.dts b/arch/arm/boot/dts/omap4-var-som.dts
> index 222a413..7e04103 100644
> --- a/arch/arm/boot/dts/omap4-var-som.dts
> +++ b/arch/arm/boot/dts/omap4-var-som.dts
> @@ -68,7 +68,7 @@
>  		spi-max-frequency = <24000000>;
>  		reg = <0>;
>  		interrupt-parent = <&gpio6>;
> -		interrupts = <11>; /* gpio line 171 */
> +		interrupts = <11 8>; /* gpio line 171, low triggered */
>  		vdd-supply = <&vdd_eth>;
>  	};
>  };
> -- 
> 1.7.10.4