[PATCHv2 0/2] ARM: dts: socfpga: fix booting with SD/MMC

[PATCHv2 0/2] ARM: dts: socfpga: fix booting with SD/MMC

[dinguyen at opensource.altera.com]

From: Dinh Nguyen <dinguyen@opensource.altera.com>

The SOCFPGA dev kit was hanging during bootup on the SD/MMC driver loading.
The first patch fixes the booting and the 2nd patch adds a regulator node
for the SD/MMC driver to use.

v2 diff(s): Patch 2/2 ARM: dts: socfpga: Add a 3.3V fixed regulator node
	- Move the regulator nodes to their respective board dts file and
	  correctly rename the regulator to match the schematic

Dinh Nguyen (2):
  ARM: dts: socfpga: Fix SD card detect
  ARM: dts: socfpga: Add a 3.3V fixed regulator node

 arch/arm/boot/dts/socfpga_arria5.dtsi         |  2 +-
 arch/arm/boot/dts/socfpga_arria5_socdk.dts    | 14 ++++++++++++++
 arch/arm/boot/dts/socfpga_cyclone5_socdk.dts  | 13 ++++++++++++-
 arch/arm/boot/dts/socfpga_cyclone5_sockit.dts | 14 ++++++++++++++
 4 files changed, 41 insertions(+), 2 deletions(-)

-- 
2.0.3

[PATCHv2 1/2] ARM: dts: socfpga: Fix SD card detect

[dinguyen at opensource.altera.com]

From: Dinh Nguyen <dinguyen@opensource.altera.com>

Without this patch, the booting the SOCFPGA platform would hang at the
SDMMC driver loading. There were 2 patches that caused this to happen:

- Patch 9795a846e10 "mmc: dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed
  looking for "cd-gpios", since mmc_of_parse was getting called.
- Patch 3cf890fc42b "mmc: dw_mmc: Pass back errors from mmc_of_parse()" would
  hang the system at the SDMMC driver loading.

This patch will fix booting with SDMMC enabled on SOCFPGA dev kit.

Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
---
 arch/arm/boot/dts/socfpga_cyclone5_socdk.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts b/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
index d7296a5..739c3b7 100644
--- a/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
+++ b/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
@@ -69,7 +69,7 @@
 };
 
 &mmc0 {
-	cd-gpios = <&gpio1 18 0>;
+	cd = <&gpio1 18 0>;
 };
 
 &usb1 {
-- 
2.0.3

[PATCHv2 2/2] ARM: dts: socfpga: Add a 3.3V fixed regulator node

[dinguyen at opensource.altera.com]

From: Dinh Nguyen <dinguyen@opensource.altera.com>

Without the 3.3V regulator node, the SDMMC driver will give these warnings:

dw_mmc ff704000.dwmmc0: No vmmc regulator found
dw_mmc ff704000.dwmmc0: No vqmmc regulator found

This patch adds the regulator node, and points the SD/MMC to the regulator.

Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
---
v2: Move the regulator nodes to their respective board dts file and
    correctly rename them to match the schematic
---
 arch/arm/boot/dts/socfpga_arria5.dtsi         |  2 +-
 arch/arm/boot/dts/socfpga_arria5_socdk.dts    | 14 ++++++++++++++
 arch/arm/boot/dts/socfpga_cyclone5_socdk.dts  | 11 +++++++++++
 arch/arm/boot/dts/socfpga_cyclone5_sockit.dts | 14 ++++++++++++++
 4 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/socfpga_arria5.dtsi b/arch/arm/boot/dts/socfpga_arria5.dtsi
index 03e8268..1907cc6 100644
--- a/arch/arm/boot/dts/socfpga_arria5.dtsi
+++ b/arch/arm/boot/dts/socfpga_arria5.dtsi
@@ -29,7 +29,7 @@
 			};
 		};
 
-		dwmmc0 at ff704000 {
+		mmc0: dwmmc0 at ff704000 {
 			num-slots = <1>;
 			broken-cd;
 			bus-width = <4>;
diff --git a/arch/arm/boot/dts/socfpga_arria5_socdk.dts b/arch/arm/boot/dts/socfpga_arria5_socdk.dts
index 27d551c..1f64811 100644
--- a/arch/arm/boot/dts/socfpga_arria5_socdk.dts
+++ b/arch/arm/boot/dts/socfpga_arria5_socdk.dts
@@ -37,6 +37,15 @@
 		*/
 		ethernet0 = &gmac1;
 	};
+
+	regulator_3_3v: regulator {
+		compatible = "regulator-fixed";
+		regulator-name = "3.3V";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-boot-on;
+		regulator-always-on;
+	};
 };
 
 &gmac1 {
@@ -68,6 +77,11 @@
 	};
 };
 
+&mmc0 {
+	vmmc-supply = <&regulator_3_3v>;
+	vqmmc-supply = <&regulator_3_3v>;
+};
+
 &usb1 {
 	status = "okay";
 };
diff --git a/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts b/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
index 739c3b7..0f624a8 100644
--- a/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
+++ b/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
@@ -37,6 +37,15 @@
 		 */
 		ethernet0 = &gmac1;
 	};
+
+	regulator_3_3v: regulator {
+		compatible = "regulator-fixed";
+		regulator-name = "3.3V";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-boot-on;
+		regulator-always-on;
+	};
 };
 
 &gmac1 {
@@ -70,6 +79,8 @@
 
 &mmc0 {
 	cd = <&gpio1 18 0>;
+	vmmc-supply = <&regulator_3_3v>;
+	vqmmc-supply = <&regulator_3_3v>;
 };
 
 &usb1 {
diff --git a/arch/arm/boot/dts/socfpga_cyclone5_sockit.dts b/arch/arm/boot/dts/socfpga_cyclone5_sockit.dts
index d26f155..3e0eff2 100644
--- a/arch/arm/boot/dts/socfpga_cyclone5_sockit.dts
+++ b/arch/arm/boot/dts/socfpga_cyclone5_sockit.dts
@@ -37,6 +37,15 @@
 		 */
 		ethernet0 = &gmac1;
 	};
+
+	regulator_3_3v: regulator {
+		compatible = "regulator-fixed";
+		regulator-name = "VCC3P3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-boot-on;
+		regulator-always-on;
+	};
 };
 
 &gmac1 {
@@ -53,6 +62,11 @@
 	rxc-skew-ps = <2000>;
 };
 
+&mmc0 {
+	vmmc-supply = <&regulator_3_3v>;
+	vqmmc-supply = <&regulator_3_3v>;
+};
+
 &usb1 {
 	status = "okay";
 };
-- 
2.0.3

[PATCHv2 1/2] ARM: dts: socfpga: Fix SD card detect

[Doug Anderson]

Dinh,

On Mon, Oct 20, 2014 at 8:31 AM,  <dinguyen@opensource.altera.com> wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>
> Without this patch, the booting the SOCFPGA platform would hang at the
> SDMMC driver loading. There were 2 patches that caused this to happen:
>
> - Patch 9795a846e10 "mmc: dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed
>   looking for "cd-gpios", since mmc_of_parse was getting called.
> - Patch 3cf890fc42b "mmc: dw_mmc: Pass back errors from mmc_of_parse()" would
>   hang the system at the SDMMC driver loading.
>
> This patch will fix booting with SDMMC enabled on SOCFPGA dev kit.
>
> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> ---
>  arch/arm/boot/dts/socfpga_cyclone5_socdk.dts | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts b/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
> index d7296a5..739c3b7 100644
> --- a/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
> +++ b/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
> @@ -69,7 +69,7 @@
>  };
>
>  &mmc0 {
> -       cd-gpios = <&gpio1 18 0>;
> +       cd = <&gpio1 18 0>;

This doesn't look right to me.  What was the error that was passed back?

I think your change has the same net effect as just deleting the
"cd-gpios" line.  ...or is there some code somewhere that is parsing
the "cd" property.

[PATCHv2 2/2] ARM: dts: socfpga: Add a 3.3V fixed regulator node

[Doug Anderson]

Dinh,

On Mon, Oct 20, 2014 at 8:31 AM,  <dinguyen@opensource.altera.com> wrote:
> +++ b/arch/arm/boot/dts/socfpga_arria5_socdk.dts
> @@ -37,6 +37,15 @@
>                 */
>                 ethernet0 = &gmac1;
>         };
> +
> +       regulator_3_3v: regulator {

I think it's better to give this a real node name.  The
"regulator_3_3v" will get dropped when you compile this into a binary
format (for shipping), so you're creating a node that's just called
"regulator".  If you had more than one fixed regulator like this it
just won't work.

I haven't seen any official guidelines, but I tend to use "regulator"
as a suffix and then put the schematic name.  Like:

  3_3v_regulator: 3-3-v-regulator


> +               compatible = "regulator-fixed";
> +               regulator-name = "3.3V";
> +               regulator-min-microvolt = <3300000>;
> +               regulator-max-microvolt = <3300000>;
> +               regulator-boot-on;
> +               regulator-always-on;

It's probably not necessary to specify "boot-on" and "always-on" since
there's no GPIO backing this (in other words, those properties are
implied since there's no way to turn this regulator off).


I haven't gone back and checked this against your schematics, but they
look good to me.  Once you've fixed above you are free to add my
Reviewed-by if you wish.

-Doug

[PATCHv2 1/2] ARM: dts: socfpga: Fix SD card detect

[Dinh Nguyen]

On 10/20/2014 10:46 AM, Doug Anderson wrote:
> Dinh,
> 
> On Mon, Oct 20, 2014 at 8:31 AM,  <dinguyen@opensource.altera.com> wrote:
>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>
>> Without this patch, the booting the SOCFPGA platform would hang at the
>> SDMMC driver loading. There were 2 patches that caused this to happen:
>>
>> - Patch 9795a846e10 "mmc: dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed
>>   looking for "cd-gpios", since mmc_of_parse was getting called.
>> - Patch 3cf890fc42b "mmc: dw_mmc: Pass back errors from mmc_of_parse()" would
>>   hang the system at the SDMMC driver loading.
>>
>> This patch will fix booting with SDMMC enabled on SOCFPGA dev kit.
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
>> ---
>>  arch/arm/boot/dts/socfpga_cyclone5_socdk.dts | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts b/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
>> index d7296a5..739c3b7 100644
>> --- a/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
>> +++ b/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
>> @@ -69,7 +69,7 @@
>>  };
>>
>>  &mmc0 {
>> -       cd-gpios = <&gpio1 18 0>;
>> +       cd = <&gpio1 18 0>;
> 
> This doesn't look right to me.  What was the error that was passed back?
> 
> I think your change has the same net effect as just deleting the
> "cd-gpios" line.  ...or is there some code somewhere that is parsing
> the "cd" property.
> 

It just hangs here:

dw_mmc ff704000.dwmmc0: Using PIO mode.
dw_mmc ff704000.dwmmc0: Version ID is 240a
dw_mmc ff704000.dwmmc0: DW MMC controller at irq 171, 32 bit host data
width, 1024 deep fifo
platform ff704000.dwmmc0: Driver dw_mmc requests probe deferral


Without this patch :
mmc_of_parse ret=-517 (EPROBE_DEFER)

With this patch or deleting the "cd-gpios" line then
mmc_of_parse ret=0


So does the driver go into polling for a card removal when neither cd or
cd-gpios are specified? Because I can see that card removal and
insertion working without any cd/cd-gpios entry in the DTS?

Thanks,
Dinh

[PATCHv2 1/2] ARM: dts: socfpga: Fix SD card detect

[Mark Rutland]

On Mon, Oct 20, 2014 at 04:31:18PM +0100, dinguyen at opensource.altera.com wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> Without this patch, the booting the SOCFPGA platform would hang at the
> SDMMC driver loading. There were 2 patches that caused this to happen:
> 
> - Patch 9795a846e10 "mmc: dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed
>   looking for "cd-gpios", since mmc_of_parse was getting called.
> - Patch 3cf890fc42b "mmc: dw_mmc: Pass back errors from mmc_of_parse()" would
>   hang the system at the SDMMC driver loading.

Regardless of which patches caused the issue, the existing DTB should
continue to function. This is a kernel bug, not a DTB bug.

How did you track down those two patches as being the cause(s)?

I've heard a report of a similar issue on a sunxi platform (cubietruck),
but I have not had the time to investigate.

> This patch will fix booting with SDMMC enabled on SOCFPGA dev kit.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> ---
>  arch/arm/boot/dts/socfpga_cyclone5_socdk.dts | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts b/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
> index d7296a5..739c3b7 100644
> --- a/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
> +++ b/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
> @@ -69,7 +69,7 @@
>  };
>  
>  &mmc0 {
> -	cd-gpios = <&gpio1 18 0>;
> +	cd = <&gpio1 18 0>;

This change should not be necessary.

Mark.

[PATCHv2 1/2] ARM: dts: socfpga: Fix SD card detect

[Dinh Nguyen]

On 10/20/2014 01:41 PM, Mark Rutland wrote:
> On Mon, Oct 20, 2014 at 04:31:18PM +0100, dinguyen at opensource.altera.com wrote:
>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>
>> Without this patch, the booting the SOCFPGA platform would hang at the
>> SDMMC driver loading. There were 2 patches that caused this to happen:
>>
>> - Patch 9795a846e10 "mmc: dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed
>>   looking for "cd-gpios", since mmc_of_parse was getting called.
>> - Patch 3cf890fc42b "mmc: dw_mmc: Pass back errors from mmc_of_parse()" would
>>   hang the system at the SDMMC driver loading.
> 
> Regardless of which patches caused the issue, the existing DTB should
> continue to function. This is a kernel bug, not a DTB bug.

I apologize. I made the mistake when I looked at mmc_of_parse(). I made
the mistake when I saw this line of code:

mmc_gpiod_request_cd(host, "cd", 0, true, 0, &gpio_invert);

I thought it was looking for a "cd" property, but it's not.

> 
> How did you track down those two patches as being the cause(s)?

If I revert patch 3cf890fc42b "mmc: dw_mmc: Pass back errors from
mmc_of_parse()", then the system boots and gets past the SDMMC driver
loading, without doing anything else.

Basically, if I remove this change from the 3cf890fc42b patch, then the
system boots and does not hang at the sd driver:

+       ret = mmc_of_parse(mmc);
+       if (ret)
+               goto err_host_allocated;


Patch 9795a846e10 "mmc: dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" is
probably not the cause of the problem.


> 
> I've heard a report of a similar issue on a sunxi platform (cubietruck),
> but I have not had the time to investigate.
> 
>> This patch will fix booting with SDMMC enabled on SOCFPGA dev kit.
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
>> ---
>>  arch/arm/boot/dts/socfpga_cyclone5_socdk.dts | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts b/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
>> index d7296a5..739c3b7 100644
>> --- a/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
>> +++ b/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
>> @@ -69,7 +69,7 @@
>>  };
>>  
>>  &mmc0 {
>> -	cd-gpios = <&gpio1 18 0>;
>> +	cd = <&gpio1 18 0>;
> 
> This change should not be necessary.
> 

Agreed...

Thanks,
Dinh

[PATCHv2 1/2] ARM: dts: socfpga: Fix SD card detect

[Doug Anderson]

Mark,

On Mon, Oct 20, 2014 at 11:41 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Oct 20, 2014 at 04:31:18PM +0100, dinguyen at opensource.altera.com wrote:
>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>
>> Without this patch, the booting the SOCFPGA platform would hang at the
>> SDMMC driver loading. There were 2 patches that caused this to happen:
>>
>> - Patch 9795a846e10 "mmc: dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed
>>   looking for "cd-gpios", since mmc_of_parse was getting called.
>> - Patch 3cf890fc42b "mmc: dw_mmc: Pass back errors from mmc_of_parse()" would
>>   hang the system at the SDMMC driver loading.
>
> Regardless of which patches caused the issue, the existing DTB should
> continue to function. This is a kernel bug, not a DTB bug.

Right.  The kernel bug is that there is no "dtb fixup" stage of the
kernel to fix up old dtbs with this dtb bug.

Said another way:

1. The old dtb was (possibly) not specifying the cd-gpio properly.

2. The kernel had a bug where it was ignoring that error.  Things may
have been working because of some other side effect (maybe polling was
working).

3. If we fix the kernel bug, what should we do?  The only sensible
thing (if we need to support old DTB with no changes) is to add a DTB
fixup stage.

...or did someone add that stage and I missed it?

-Doug

[PATCHv2 1/2] ARM: dts: socfpga: Fix SD card detect

[Doug Anderson]

Dinh,

On Mon, Oct 20, 2014 at 9:31 AM, Dinh Nguyen
<dinguyen@opensource.altera.com> wrote:
>>>  &mmc0 {
>>> -       cd-gpios = <&gpio1 18 0>;
>>> +       cd = <&gpio1 18 0>;
>>
>> This doesn't look right to me.  What was the error that was passed back?
>>
>> I think your change has the same net effect as just deleting the
>> "cd-gpios" line.  ...or is there some code somewhere that is parsing
>> the "cd" property.
>>
>
> It just hangs here:
>
> dw_mmc ff704000.dwmmc0: Using PIO mode.
> dw_mmc ff704000.dwmmc0: Version ID is 240a
> dw_mmc ff704000.dwmmc0: DW MMC controller at irq 171, 32 bit host data
> width, 1024 deep fifo
> platform ff704000.dwmmc0: Driver dw_mmc requests probe deferral
>
>
> Without this patch :
> mmc_of_parse ret=-517 (EPROBE_DEFER)

I think you need to dig more into this error.  Why are you getting an
-EPROBE_DEFER when you asked for this GPIO?

The -EPROBE_DEFER tells the system that a resource is not available
yet and that it should try to re-run the dw_mmc init later once the
resource becomes available.  I believe that some other bug is causing
the resource to never become available.

Guesses:

* The GPIO specifier is wrong in the DTB and is pointing to a GPIO
that would never exist.

* Something is happening in the GPIO driver that is causing it to fail.


...so we need to dig in further.

-Doug

[PATCHv2 1/2] ARM: dts: socfpga: Fix SD card detect

[Doug Anderson]

Dinh,

On Mon, Oct 20, 2014 at 12:30 PM, Doug Anderson <dianders@chromium.org> wrote:
> Dinh,
>
> On Mon, Oct 20, 2014 at 9:31 AM, Dinh Nguyen
> <dinguyen@opensource.altera.com> wrote:
>>>>  &mmc0 {
>>>> -       cd-gpios = <&gpio1 18 0>;
>>>> +       cd = <&gpio1 18 0>;
>>>
>>> This doesn't look right to me.  What was the error that was passed back?
>>>
>>> I think your change has the same net effect as just deleting the
>>> "cd-gpios" line.  ...or is there some code somewhere that is parsing
>>> the "cd" property.
>>>
>>
>> It just hangs here:
>>
>> dw_mmc ff704000.dwmmc0: Using PIO mode.
>> dw_mmc ff704000.dwmmc0: Version ID is 240a
>> dw_mmc ff704000.dwmmc0: DW MMC controller at irq 171, 32 bit host data
>> width, 1024 deep fifo
>> platform ff704000.dwmmc0: Driver dw_mmc requests probe deferral
>>
>>
>> Without this patch :
>> mmc_of_parse ret=-517 (EPROBE_DEFER)
>
> I think you need to dig more into this error.  Why are you getting an
> -EPROBE_DEFER when you asked for this GPIO?
>
> The -EPROBE_DEFER tells the system that a resource is not available
> yet and that it should try to re-run the dw_mmc init later once the
> resource becomes available.  I believe that some other bug is causing
> the resource to never become available.
>
> Guesses:
>
> * The GPIO specifier is wrong in the DTB and is pointing to a GPIO
> that would never exist.
>
> * Something is happening in the GPIO driver that is causing it to fail.
>
>
> ...so we need to dig in further.

One odd thing is that it looks like the GPIO controller you're
referencing is disabled.  On today's linuxnext, you can see the
"disabled":

arch/arm/boot/dts/socfpga.dtsi:         gpio at ff709000 {
arch/arm/boot/dts/socfpga.dtsi-                 #address-cells = <1>;
arch/arm/boot/dts/socfpga.dtsi-                 #size-cells = <0>;
arch/arm/boot/dts/socfpga.dtsi-                 compatible = "snps,dw-apb-gpio";
arch/arm/boot/dts/socfpga.dtsi-                 reg = <0xff709000 0x1000>;
arch/arm/boot/dts/socfpga.dtsi-                 clocks = <&per_base_clk>;
arch/arm/boot/dts/socfpga.dtsi-                 status = "disabled";
arch/arm/boot/dts/socfpga.dtsi-
arch/arm/boot/dts/socfpga.dtsi-                 gpio1: gpio-controller at 0 {
arch/arm/boot/dts/socfpga.dtsi-                         compatible =
"snps,dw-apb-gpio-port";
arch/arm/boot/dts/socfpga.dtsi-                         gpio-controller;

I don't see anyone else referencing that node to enable it.  ...to me
that means that you can't use GPIOs on GPIO1 (???).


...or did I find something wrong?


-Doug

[PATCHv2 1/2] ARM: dts: socfpga: Fix SD card detect

[Dinh Nguyen]

On 10/20/2014 02:48 PM, Doug Anderson wrote:
> Dinh,
> 
> On Mon, Oct 20, 2014 at 12:30 PM, Doug Anderson <dianders@chromium.org> wrote:
>> Dinh,
>>
>> On Mon, Oct 20, 2014 at 9:31 AM, Dinh Nguyen
>> <dinguyen@opensource.altera.com> wrote:
>>>>>  &mmc0 {
>>>>> -       cd-gpios = <&gpio1 18 0>;
>>>>> +       cd = <&gpio1 18 0>;
>>>>
>>>> This doesn't look right to me.  What was the error that was passed back?
>>>>
>>>> I think your change has the same net effect as just deleting the
>>>> "cd-gpios" line.  ...or is there some code somewhere that is parsing
>>>> the "cd" property.
>>>>
>>>
>>> It just hangs here:
>>>
>>> dw_mmc ff704000.dwmmc0: Using PIO mode.
>>> dw_mmc ff704000.dwmmc0: Version ID is 240a
>>> dw_mmc ff704000.dwmmc0: DW MMC controller at irq 171, 32 bit host data
>>> width, 1024 deep fifo
>>> platform ff704000.dwmmc0: Driver dw_mmc requests probe deferral
>>>
>>>
>>> Without this patch :
>>> mmc_of_parse ret=-517 (EPROBE_DEFER)
>>
>> I think you need to dig more into this error.  Why are you getting an
>> -EPROBE_DEFER when you asked for this GPIO?
>>
>> The -EPROBE_DEFER tells the system that a resource is not available
>> yet and that it should try to re-run the dw_mmc init later once the
>> resource becomes available.  I believe that some other bug is causing
>> the resource to never become available.
>>
>> Guesses:
>>
>> * The GPIO specifier is wrong in the DTB and is pointing to a GPIO
>> that would never exist.
>>
>> * Something is happening in the GPIO driver that is causing it to fail.
>>
>>
>> ...so we need to dig in further.
> 
> One odd thing is that it looks like the GPIO controller you're
> referencing is disabled.  On today's linuxnext, you can see the
> "disabled":
> 
> arch/arm/boot/dts/socfpga.dtsi:         gpio at ff709000 {
> arch/arm/boot/dts/socfpga.dtsi-                 #address-cells = <1>;
> arch/arm/boot/dts/socfpga.dtsi-                 #size-cells = <0>;
> arch/arm/boot/dts/socfpga.dtsi-                 compatible = "snps,dw-apb-gpio";
> arch/arm/boot/dts/socfpga.dtsi-                 reg = <0xff709000 0x1000>;
> arch/arm/boot/dts/socfpga.dtsi-                 clocks = <&per_base_clk>;
> arch/arm/boot/dts/socfpga.dtsi-                 status = "disabled";
> arch/arm/boot/dts/socfpga.dtsi-
> arch/arm/boot/dts/socfpga.dtsi-                 gpio1: gpio-controller at 0 {
> arch/arm/boot/dts/socfpga.dtsi-                         compatible =
> "snps,dw-apb-gpio-port";
> arch/arm/boot/dts/socfpga.dtsi-                         gpio-controller;
> 
> I don't see anyone else referencing that node to enable it.  ...to me
> that means that you can't use GPIOs on GPIO1 (???).
> 
> 
> ...or did I find something wrong?
> 

Ah yes, yes! Thanks so much for find this Doug!

With the following patch, boots fine with SD driver loading.


--- a/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
+++ b/arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
@@ -37,6 +37,12 @@
                 */
                ethernet0 = &gmac1;
        };
+
+       soc {
+               gpio at ff709000 {
+                       status = "okay";
+               };
+       };

Thanks,
Dinh

[PATCHv2 2/2] ARM: dts: socfpga: Add a 3.3V fixed regulator node

[Dinh Nguyen]

On 10/20/2014 10:51 AM, Doug Anderson wrote:
> Dinh,
> 
> On Mon, Oct 20, 2014 at 8:31 AM,  <dinguyen@opensource.altera.com> wrote:
>> +++ b/arch/arm/boot/dts/socfpga_arria5_socdk.dts
>> @@ -37,6 +37,15 @@
>>                 */
>>                 ethernet0 = &gmac1;
>>         };
>> +
>> +       regulator_3_3v: regulator {
> 
> I think it's better to give this a real node name.  The
> "regulator_3_3v" will get dropped when you compile this into a binary
> format (for shipping), so you're creating a node that's just called
> "regulator".  If you had more than one fixed regulator like this it
> just won't work.
> 
> I haven't seen any official guidelines, but I tend to use "regulator"
> as a suffix and then put the schematic name.  Like:
> 
>   3_3v_regulator: 3-3-v-regulator
> 
>

Not sure if there's a limit on a node cannot start with numeric value?
But if I change it to your suggestion, the DTC does not compile:

Error: arch/arm/boot/dts/socfpga_cyclone5_socdk.dts:41.16-17 syntax error
FATAL ERROR: Unable to parse input tree


But regulator_3_3v: 3-3-v-regulator   will work fine.

Dinh

[PATCHv2 1/2] ARM: dts: socfpga: Fix SD card detect

[Mark Rutland]

On Mon, Oct 20, 2014 at 08:26:55PM +0100, Doug Anderson wrote:
> Mark,
> 
> On Mon, Oct 20, 2014 at 11:41 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, Oct 20, 2014 at 04:31:18PM +0100, dinguyen at opensource.altera.com wrote:
> >> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> >>
> >> Without this patch, the booting the SOCFPGA platform would hang at the
> >> SDMMC driver loading. There were 2 patches that caused this to happen:
> >>
> >> - Patch 9795a846e10 "mmc: dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed
> >>   looking for "cd-gpios", since mmc_of_parse was getting called.
> >> - Patch 3cf890fc42b "mmc: dw_mmc: Pass back errors from mmc_of_parse()" would
> >>   hang the system at the SDMMC driver loading.
> >
> > Regardless of which patches caused the issue, the existing DTB should
> > continue to function. This is a kernel bug, not a DTB bug.
> 
> Right.  The kernel bug is that there is no "dtb fixup" stage of the
> kernel to fix up old dtbs with this dtb bug.
> 
> Said another way:
> 
> 1. The old dtb was (possibly) not specifying the cd-gpio properly.
> 
> 2. The kernel had a bug where it was ignoring that error.  Things may
> have been working because of some other side effect (maybe polling was
> working).
> 
> 3. If we fix the kernel bug, what should we do?  The only sensible
> thing (if we need to support old DTB with no changes) is to add a DTB
> fixup stage.
> 
> ...or did someone add that stage and I missed it?

Unfortunately, we have no generic DTB fixup stage currently.

What exactly was wrong with this cd-gpios description that previously
allowed it to function? Can we not print a warning and fall back to the
old behaviour in this case?

Thanks,
Mark

[PATCHv2 1/2] ARM: dts: socfpga: Fix SD card detect

[Jaehoon Chung]

On 10/21/2014 07:41 AM, Mark Rutland wrote:
> On Mon, Oct 20, 2014 at 08:26:55PM +0100, Doug Anderson wrote:
>> Mark,
>>
>> On Mon, Oct 20, 2014 at 11:41 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> On Mon, Oct 20, 2014 at 04:31:18PM +0100, dinguyen at opensource.altera.com wrote:
>>>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>>>
>>>> Without this patch, the booting the SOCFPGA platform would hang at the
>>>> SDMMC driver loading. There were 2 patches that caused this to happen:
>>>>
>>>> - Patch 9795a846e10 "mmc: dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed
>>>>   looking for "cd-gpios", since mmc_of_parse was getting called.
>>>> - Patch 3cf890fc42b "mmc: dw_mmc: Pass back errors from mmc_of_parse()" would
>>>>   hang the system at the SDMMC driver loading.
>>>
>>> Regardless of which patches caused the issue, the existing DTB should
>>> continue to function. This is a kernel bug, not a DTB bug.
>>
>> Right.  The kernel bug is that there is no "dtb fixup" stage of the
>> kernel to fix up old dtbs with this dtb bug.
>>
>> Said another way:
>>
>> 1. The old dtb was (possibly) not specifying the cd-gpio properly.
>>
>> 2. The kernel had a bug where it was ignoring that error.  Things may
>> have been working because of some other side effect (maybe polling was
>> working).
>>
>> 3. If we fix the kernel bug, what should we do?  The only sensible
>> thing (if we need to support old DTB with no changes) is to add a DTB
>> fixup stage.
>>
>> ...or did someone add that stage and I missed it?
> 
> Unfortunately, we have no generic DTB fixup stage currently.
> 
> What exactly was wrong with this cd-gpios description that previously
> allowed it to function? Can we not print a warning and fall back to the
> old behaviour in this case?

I think this is Dinh's mistake.
Doug found the reason of this problem, and it seems that Dinh has checked after fixing.
He didn't enable the gpio controller.

Best Regards,
Jaehoon Chung

> 
> Thanks,
> Mark
> 

[PATCHv2 1/2] ARM: dts: socfpga: Fix SD card detect

[Doug Anderson]

Mark,

On Mon, Oct 20, 2014 at 3:41 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Oct 20, 2014 at 08:26:55PM +0100, Doug Anderson wrote:
>> Mark,
>>
>> On Mon, Oct 20, 2014 at 11:41 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Mon, Oct 20, 2014 at 04:31:18PM +0100, dinguyen at opensource.altera.com wrote:
>> >> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>> >>
>> >> Without this patch, the booting the SOCFPGA platform would hang at the
>> >> SDMMC driver loading. There were 2 patches that caused this to happen:
>> >>
>> >> - Patch 9795a846e10 "mmc: dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed
>> >>   looking for "cd-gpios", since mmc_of_parse was getting called.
>> >> - Patch 3cf890fc42b "mmc: dw_mmc: Pass back errors from mmc_of_parse()" would
>> >>   hang the system at the SDMMC driver loading.
>> >
>> > Regardless of which patches caused the issue, the existing DTB should
>> > continue to function. This is a kernel bug, not a DTB bug.
>>
>> Right.  The kernel bug is that there is no "dtb fixup" stage of the
>> kernel to fix up old dtbs with this dtb bug.
>>
>> Said another way:
>>
>> 1. The old dtb was (possibly) not specifying the cd-gpio properly.
>>
>> 2. The kernel had a bug where it was ignoring that error.  Things may
>> have been working because of some other side effect (maybe polling was
>> working).
>>
>> 3. If we fix the kernel bug, what should we do?  The only sensible
>> thing (if we need to support old DTB with no changes) is to add a DTB
>> fixup stage.
>>
>> ...or did someone add that stage and I missed it?
>
> Unfortunately, we have no generic DTB fixup stage currently.

Right.  ...and that's the bug.

I think we may need to modify the general inclination to respond to
dts change requests with "the DTS can't have a bug in it".  DTS files
can indeed have bugs in it.  In this case the dts file was claiming
that the card detect GPIO was a GPIO on a controller that the same dts
claimed was "disabled".  If that's not a bug in the DTS I'm not sure
what it is.

There are all sorts of broken ways that we could work around this in
the driver.  We could pretend that EPROBE_DEFER really meant "I'm all
good".  We could add a special case for this particular board in
dw_mmc (do we check the overall device tree compatible string?).  We
could do all sorts of hacks.  None of them are right.  The "right"
behavior if we really care about maintaining compatbility with old DTB
files is to add a fixup stage for this particular broken board.

Given that no such fixup stage exists, if someone really wants old DTB
files to work then we should add one.

-Doug

[PATCHv2 1/2] ARM: dts: socfpga: Fix SD card detect

[Mark Rutland]

On Tue, Oct 21, 2014 at 01:02:28AM +0100, Doug Anderson wrote:
> Mark,
> 
> On Mon, Oct 20, 2014 at 3:41 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, Oct 20, 2014 at 08:26:55PM +0100, Doug Anderson wrote:
> >> Mark,
> >>
> >> On Mon, Oct 20, 2014 at 11:41 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > On Mon, Oct 20, 2014 at 04:31:18PM +0100, dinguyen at opensource.altera.com wrote:
> >> >> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> >> >>
> >> >> Without this patch, the booting the SOCFPGA platform would hang at the
> >> >> SDMMC driver loading. There were 2 patches that caused this to happen:
> >> >>
> >> >> - Patch 9795a846e10 "mmc: dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed
> >> >>   looking for "cd-gpios", since mmc_of_parse was getting called.
> >> >> - Patch 3cf890fc42b "mmc: dw_mmc: Pass back errors from mmc_of_parse()" would
> >> >>   hang the system at the SDMMC driver loading.
> >> >
> >> > Regardless of which patches caused the issue, the existing DTB should
> >> > continue to function. This is a kernel bug, not a DTB bug.
> >>
> >> Right.  The kernel bug is that there is no "dtb fixup" stage of the
> >> kernel to fix up old dtbs with this dtb bug.
> >>
> >> Said another way:
> >>
> >> 1. The old dtb was (possibly) not specifying the cd-gpio properly.
> >>
> >> 2. The kernel had a bug where it was ignoring that error.  Things may
> >> have been working because of some other side effect (maybe polling was
> >> working).
> >>
> >> 3. If we fix the kernel bug, what should we do?  The only sensible
> >> thing (if we need to support old DTB with no changes) is to add a DTB
> >> fixup stage.
> >>
> >> ...or did someone add that stage and I missed it?
> >
> > Unfortunately, we have no generic DTB fixup stage currently.
> 
> Right.  ...and that's the bug.
> 
> I think we may need to modify the general inclination to respond to
> dts change requests with "the DTS can't have a bug in it".  DTS files
> can indeed have bugs in it.  In this case the dts file was claiming
> that the card detect GPIO was a GPIO on a controller that the same dts
> claimed was "disabled".  If that's not a bug in the DTS I'm not sure
> what it is.

While it's unusual, it's not necessarily a bug in general-- the link
might be true (i.e. that particular GPIO might be attached to the CD
line), but for some reason the GPIO controller is unusable on a
particular board. Perhaps we need to distinguish not ready yet from will
never be ready -- at least for provider nodes with status = "disabled"
that should be obvious.

That said, this was not an intentional property of this DTB.

> There are all sorts of broken ways that we could work around this in
> the driver.  We could pretend that EPROBE_DEFER really meant "I'm all
> good".  We could add a special case for this particular board in
> dw_mmc (do we check the overall device tree compatible string?).  We
> could do all sorts of hacks.  None of them are right.  The "right"
> behavior if we really care about maintaining compatbility with old DTB
> files is to add a fixup stage for this particular broken board.

While a DTB fixup stage is something which we are likely to need at some
point, it comes with its own (rather large) cost. I disagree that DTB
fixup is the one and only way of handling this kind of issue.

> Given that no such fixup stage exists, if someone really wants old DTB
> files to work then we should add one.

Perhaps. In this case I guess this comes down to whether socfpga users
are happy to update their DTBs.

Thanks,
Mark.