ARM: dts: socfpga: fix booting with SD/MMC
v2 cover ARM: dts: socfpga: fix booting with SD/MMC
From: Dinh Nguyen <[email protected]>
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(-)
Signature · 1 line
2.0.3
PATCH v2 1/2 ARM: dts: socfpga: Fix SD card detect
From: Dinh Nguyen <[email protected]>
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.
--- arch/arm/boot/dts/socfpga_cyclone5_socdk.dts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts +1 -1
@@ -69,7 +69,7 @@ }; &mmc0 { - cd-gpios = <&gpio1 18 0>; + cd = <&gpio1 18 0>; }; &usb1 {
Signature · 1 line
2.0.3
PATCH v2 1/2 ARM: dts: socfpga: Fix SD card detect
Dinh,
Quoted 22 lines · On Mon, Oct 20, 2014 at 8:31 AM, <[email protected]> wrote:
From: Dinh Nguyen <[email protected]>
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", sincemmc_of_parsewas getting called.
- Patch3cf890fc42b"mmc:dw_mmc: Pass back errors frommmc_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 <[email protected]>--- arch/arm/boot/dts/socfpga_cyclone5_socdk.dts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm/boot/dts/socfpga_cyclone5_socdk.dts +0
-0
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts line 72
Quoted 5 lines
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts +1
-1@@ -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.
PATCH v2 1/2 ARM: dts: socfpga: Fix SD card detect
Quoted 33 lines · Doug Anderson
Dinh,
Quoted 22 lines · On Mon, Oct 20, 2014 at 8:31 AM, <[email protected]> wrote:
From: Dinh Nguyen <[email protected]>
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", sincemmc_of_parsewas getting called.
- Patch3cf890fc42b"mmc:dw_mmc: Pass back errors frommmc_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 <[email protected]>--- arch/arm/boot/dts/socfpga_cyclone5_socdk.dts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm/boot/dts/socfpga_cyclone5_socdk.dts +0
-0arch/arm/boot/dts/socfpga_cyclone5_socdk.dts line 72
Quoted 5 lines
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts +1
-1@@ -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?
Quoted 4 lines
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 240adw_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
PATCH v2 1/2 ARM: dts: socfpga: Fix SD card detect
Dinh,
Quoted 20 lines · Dinh Nguyen
Quoted 10 lines
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts line 72
Quoted 3 lines
&mmc0 {
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts +1
-1- 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_mmcff704000.dwmmc0: Using PIO mode.dw_mmcff704000.dwmmc0: Version ID is 240adw_mmcff704000.dwmmc0: DW MMC controller at irq 171, 32 bit host data width, 1024 deep fifo
platform ff704000.dwmmc0: Driverdw_mmcrequests probe deferral
Quoted 2 lines
Without this patch :
mmc_of_parseret=-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
PATCH v2 1/2 ARM: dts: socfpga: Fix SD card detect
Dinh,
Quoted 37 lines · Doug Anderson
Dinh,
Quoted 20 lines · Dinh Nguyen
Quoted 10 lines
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts line 72
Quoted 3 lines
&mmc0 {
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts +1
-1- 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_mmcff704000.dwmmc0: Using PIO mode.dw_mmcff704000.dwmmc0: Version ID is 240adw_mmcff704000.dwmmc0: DW MMC controller at irq 171, 32 bit host data width, 1024 deep fifo
platform ff704000.dwmmc0: Driverdw_mmcrequests probe deferralQuoted 2 lines
Without this patch :
mmc_of_parseret=-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_mmcinit later once the resource becomes available. I believe that some other bug is causing the resource to never become available.Guesses:
Quoted 7 lines
* 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
PATCH v2 1/2 ARM: dts: socfpga: Fix SD card detect
Quoted 65 lines · Doug Anderson
Dinh,
Quoted 37 lines · Doug Anderson
Dinh,
Quoted 20 lines · Dinh Nguyen
Quoted 10 lines
Review
Quoted 3 lines
&mmc0 {
(quoted diff) +1
-1- 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_mmcff704000.dwmmc0: Using PIO mode.dw_mmcff704000.dwmmc0: Version ID is 240adw_mmcff704000.dwmmc0: DW MMC controller at irq 171, 32 bit host data width, 1024 deep fifo
platform ff704000.dwmmc0: Driverdw_mmcrequests probe deferralQuoted 2 lines
Without this patch :
mmc_of_parseret=-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_mmcinit later once the resource becomes available. I believe that some other bug is causing the resource to never become available.Guesses:
Quoted 7 lines
* 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 atff709000{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;
Quoted 6 lines
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.
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts +6 -0
@@ -37,6 +37,12 @@ */ ethernet0 = &gmac1; }; + + soc { + gpio at ff709000 { + status = "okay"; + }; + };
Thanks,
Dinh
PATCH v2 1/2 ARM: dts: socfpga: Fix SD card detect
In reply to dinguyen at opensource.altera.comQuoted 9 lines · dinguyen at opensource.altera.com
From: Dinh Nguyen <[email protected]>
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", sincemmc_of_parsewas getting called.
- Patch3cf890fc42b"mmc:dw_mmc: Pass back errors frommmc_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.
Quoted 12 lines
This patch will fix booting with SDMMC enabled on SOCFPGA dev kit.
Signed-off-by Dinh Nguyen <[email protected]>--- arch/arm/boot/dts/socfpga_cyclone5_socdk.dts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm/boot/dts/socfpga_cyclone5_socdk.dts +0
-0
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts line 72
Quoted 5 lines
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts +1
-1@@ -69,7 +69,7 @@ }; &mmc0 { - cd-gpios = <&gpio1 18 0>; + cd = <&gpio1 18 0>;
This change should not be necessary.
Mark.
PATCH v2 1/2 ARM: dts: socfpga: Fix SD card detect
Quoted 6 lines · Mark Rutland
Quoted 5 lines · dinguyen at opensource.altera.com
From: Dinh Nguyen <[email protected]>
Without this patch, the booting the SOCFPGA platform would hang at the SDMMC driver loading. There were 2 patches that caused this to happen:
Quoted 7 lines
Quoted 4 lines
- Patch
9795a846e10"mmc:dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed looking for "cd-gpios", sincemmc_of_parsewas getting called.
- Patch3cf890fc42b"mmc:dw_mmc: Pass back errors frommmc_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.
Quoted 2 lines
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.
Quoted 18 lines
I've heard a report of a similar issue on a sunxi platform (cubietruck), but I have not had the time to investigate.
Quoted 14 lines
This patch will fix booting with SDMMC enabled on SOCFPGA dev kit.
Signed-off-by Dinh Nguyen <[email protected]>--- arch/arm/boot/dts/socfpga_cyclone5_socdk.dts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm/boot/dts/socfpga_cyclone5_socdk.dts +0
-0@@ -69,7 +69,7 @@ };
Quoted 6 lines
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts line 72
Quoted 3 lines
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts +1
-1&mmc0 { - cd-gpios = <&gpio1 18 0>; + cd = <&gpio1 18 0>;This change should not be necessary.
Agreed...
Thanks,
Dinh
PATCH v2 1/2 ARM: dts: socfpga: Fix SD card detect
In reply to Mark RutlandMark,
Quoted 6 lines · Mark Rutland
Quoted 5 lines · dinguyen at opensource.altera.com
From: Dinh Nguyen <[email protected]>
Without this patch, the booting the SOCFPGA platform would hang at the SDMMC driver loading. There were 2 patches that caused this to happen:
Quoted 7 lines
Quoted 4 lines
- Patch
9795a846e10"mmc:dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed looking for "cd-gpios", sincemmc_of_parsewas getting called.
- Patch3cf890fc42b"mmc:dw_mmc: Pass back errors frommmc_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
PATCH v2 1/2 ARM: dts: socfpga: Fix SD card detect
Quoted 28 lines · Doug Anderson
Mark,
Quoted 6 lines · Mark Rutland
Quoted 5 lines · dinguyen at opensource.altera.com
From: Dinh Nguyen <[email protected]>
Without this patch, the booting the SOCFPGA platform would hang at the SDMMC driver loading. There were 2 patches that caused this to happen:
Quoted 7 lines
Quoted 4 lines
- Patch
9795a846e10"mmc:dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed looking for "cd-gpios", sincemmc_of_parsewas getting called.
- Patch3cf890fc42b"mmc:dw_mmc: Pass back errors frommmc_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).
Quoted 5 lines
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
PATCH v2 1/2 ARM: dts: socfpga: Fix SD card detect
Quoted 33 lines · Mark Rutland
Quoted 32 lines · Doug Anderson
Mark,
Quoted 6 lines · Mark Rutland
Quoted 5 lines · dinguyen at opensource.altera.com
From: Dinh Nguyen <[email protected]>
Without this patch, the booting the SOCFPGA platform would hang at the SDMMC driver loading. There were 2 patches that caused this to happen:
Quoted 7 lines
Quoted 4 lines
- Patch
9795a846e10"mmc:dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed looking for "cd-gpios", sincemmc_of_parsewas getting called.
- Patch3cf890fc42b"mmc:dw_mmc: Pass back errors frommmc_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.
Quoted 7 lines
Quoted 1 line
...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
Quoted 4 lines
Thanks,
Mark
PATCH v2 1/2 ARM: dts: socfpga: Fix SD card detect
In reply to Mark RutlandMark,
Quoted 29 lines · Mark Rutland
Quoted 28 lines · Doug Anderson
Mark,
Quoted 6 lines · Mark Rutland
Quoted 5 lines · dinguyen at opensource.altera.com
From: Dinh Nguyen <[email protected]>
Without this patch, the booting the SOCFPGA platform would hang at the SDMMC driver loading. There were 2 patches that caused this to happen:
Quoted 7 lines
Quoted 4 lines
- Patch
9795a846e10"mmc:dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed looking for "cd-gpios", sincemmc_of_parsewas getting called.
- Patch3cf890fc42b"mmc:dw_mmc: Pass back errors frommmc_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).
Quoted 7 lines
Quoted 5 lines
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
PATCH v2 1/2 ARM: dts: socfpga: Fix SD card detect
Quoted 40 lines · Doug Anderson
Mark,
Quoted 29 lines · Mark Rutland
Quoted 28 lines · Doug Anderson
Mark,
Quoted 6 lines · Mark Rutland
Quoted 5 lines · dinguyen at opensource.altera.com
From: Dinh Nguyen <[email protected]>
Without this patch, the booting the SOCFPGA platform would hang at the SDMMC driver loading. There were 2 patches that caused this to happen:
Quoted 7 lines
Quoted 4 lines
- Patch
9795a846e10"mmc:dw_mmc: remove dw_mci_of_cd_gpio/wp_gpio()" removed looking for "cd-gpios", sincemmc_of_parsewas getting called.
- Patch3cf890fc42b"mmc:dw_mmc: Pass back errors frommmc_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).
Quoted 7 lines
Quoted 5 lines
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.
Quoted 8 lines
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.
Quoted 7 lines
There are all sorts of broken ways that we could work around this in the driver. We could pretend that
EPROBE_DEFERreally meant "I'm all good". We could add a special case for this particular board indw_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.
Quoted 2 lines
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.
PATCH v2 2/2 ARM: dts: socfpga: Add a 3.3V fixed regulator node
In reply to dinguyen at opensource.altera.comFrom: Dinh Nguyen <[email protected]>
Without the 3.3V regulator node, the SDMMC driver will give these warnings:
dw_mmc ff704000.dwmmc0: No vmmc regulator founddw_mmc ff704000.dwmmc0: No vqmmc regulator found
This patch adds the regulator node, and points the SD/MMC to the regulator.
---
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(-)
arch/arm/boot/dts/socfpga_arria5.dtsi +1 -1
@@ -29,7 +29,7 @@ }; }; - dwmmc0 at ff704000 { + mmc0: dwmmc0 at ff704000 { num-slots = <1>; broken-cd; bus-width = <4>;
arch/arm/boot/dts/socfpga_arria5_socdk.dts +14 -0
@@ -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 = <®ulator_3_3v>; + vqmmc-supply = <®ulator_3_3v>; +}; + &usb1 { status = "okay"; };
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts +11 -0
@@ -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 = <®ulator_3_3v>; + vqmmc-supply = <®ulator_3_3v>; }; &usb1 {
arch/arm/boot/dts/socfpga_cyclone5_sockit.dts +14 -0
@@ -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 = <®ulator_3_3v>; + vqmmc-supply = <®ulator_3_3v>; +}; + &usb1 { status = "okay"; };
Signature · 1 line
2.0.3
PATCH v2 2/2 ARM: dts: socfpga: Add a 3.3V fixed regulator node
Dinh,
arch/arm/boot/dts/socfpga_arria5_socdk.dts line 41
Quoted 7 lines · On Mon, Oct 20, 2014 at 8:31 AM, <[email protected]> wrote:
+++
b/arch/arm/boot/dts/socfpga_arria5_socdk.dtsarch/arm/boot/dts/socfpga_arria5_socdk.dts +2
-0@@ -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
arch/arm/boot/dts/socfpga_arria5_socdk.dts line 47
Quoted 6 lines
arch/arm/boot/dts/socfpga_arria5_socdk.dts +6
-0+ 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
PATCH v2 2/2 ARM: dts: socfpga: Add a 3.3V fixed regulator node
Quoted 17 lines · Doug Anderson
Dinh,
arch/arm/boot/dts/socfpga_arria5_socdk.dts line 41
Quoted 7 lines · On Mon, Oct 20, 2014 at 8:31 AM, <[email protected]> wrote:
+++
b/arch/arm/boot/dts/socfpga_arria5_socdk.dtsarch/arm/boot/dts/socfpga_arria5_socdk.dts +2
-0@@ -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.
Quoted 6 lines
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