| Message ID | 1413819079-17120-2-git-send-email-dinguyen@opensource.altera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
On Mon, Oct 20, 2014 at 04:31:18PM +0100, 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. 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.
On 10/20/2014 01:41 PM, Mark Rutland wrote: > On Mon, Oct 20, 2014 at 04:31:18PM +0100, 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. > > 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
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@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
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
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@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@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
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@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
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@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 >
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@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
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@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.
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 {