| Message ID | 55ED3695.2030101@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On 7 September 2015 at 15:02, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 06-09-15 16:47, Yousong Zhou wrote: >> >> On Sep 6, 2015 10:14 PM, "Shawn Lin" <shawn.lin@rock-chips.com> wrote: >>> >>> >>> ? 2015/9/6 20:09, Yousong Zhou ??: >>>> >>>> >>>> Hi, >>>> >>>> On 6 September 2015 at 08:12, Shawn Lin <shawn.lin@rock-chips.com> >>>> wrote: >>>>> >>>>> >>>>> On 2015/9/5 22:58, Hans de Goede wrote: >>>>>> >>>>>> >>>>>> >>>>>> Hi Shawn Lin, >>>>>> >>>>>> On 05-09-15 16:07, Shawn Lin wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 2015/9/5 18:19, Yousong Zhou wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> A SD card with sunxi-mmc can fail with the following error message >>>>>>>> (RCD for >>>>>>>> response CRC error) when trying to switch to highspeed mode. >>>>>>>> Setting >>>>>>>> the bus >>>>>>>> clock before the access mode switch fixed it. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> No, that's wrong! >>>>>>> >>>>>>> Before this card is switched to highspeed, it works under >>>>>>> identification mode(From spec: bus clock <= 400KHz). How could you >>>>>>> raise bus clock to higher clk rate which I _guess_ is 52MHz before >>>>>>> you >>>>>>> notify sd cards to run into highspeed mode? >>>>>>> >>>>>>> Althought it works for this card, this patch will not please the >>>>>>> other >>>>>>> cards that they might not reply CMDs any longer including the >>>>>>> following CMD6. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Thanks for the feedback, this is exactly why I asked Yousong Zhou to >>>>>> take this >>>>>> to the mmc list. >>>>>> >>>>>> So if this is not the proper fix for the problem that Yousong Zhou is >>>>>> seeing, then >>>>>> what might be the proper fix ? >>>>>> >>>>> >>>>> From my knowledge of mmc, there hadn't have a way to deal with this >> >> "broken" >>>>> >>>>> case. In another word, IMO,it's ANTI-SPEC. We can't be too spec >> >> sometimes, >>>>> >>>>> but at least we shouldn't violate it. >>>>> >>>> >>>> Maybe the fix is anti-spec. But the fact is that the card works on >>>> many other platforms with the builtin card reader (not through an USB >>>> adapter), including Mac OS X, my old Nokia Symbian phone, and Windows. >>>> >>>>>> Could it be that the sunxi-mmc is doing some things in the wrong order >>>>>> when >>>>>> changing the clock, or is this all under control of the mmc core ? >>>>>> >>>>> >>>>> all of this is under control of the mmc core. >>>>> So if Yongsong does want this card to work for any linux-based mmc >> >> stack, I >>>>> >>>>> guess something like that should be "better"? >>>>> >>>>> if (switch to HS fail) { >>>>> set_bus_clk >>>>> goto retry switch to HS >>>>> } >>>>> >>>>> BUT...I admit it seems strange as well. >>>>> >>>> >>>> The SD Specification (simplified version) says "If CRC error occurs on >>>> the status data, the host should issue a power cycle.", so I guess the >>>> above approach is anti-spec in some way :) >>>> >>> >>> Right?I was guessing that way from your intention. >>> >>> >>>> In case it may help debug this problem, I'd like to add that the card >>>> previously worked fine with U-Boot before commit [1]. This can also >>>> be confirmed by Debian Jessie installer image with which the old >>>> U-Boot there worked fine while the kernel (3.16) did not. >>>> >>>> [1] sunxi: mmc: Properly setup mod-clk and clock sampling phases, >>>> >> >> http://git.denx.de/?p=u-boot.git;a=commit;h=fc3a832576ce7bb597b1823935bfb7dcca235c3c > > > Interesting, one thing that patch does is introduce a switch from parent pll > when switching from 400KHz to higher clocks. Can you try the attached patch > ? > Previously I tried inserting udelay(1000) immediately after many writel calls: the result were all the same. I just tried that patch adding mdelay(100) after mmc_set_mod_clk(), still no luck, the same error message just as before. > If reverting u-boot commit fc3a832576ce7bb597b1823935bfb7dcca235c3c fixes > things, then it is probably best to focus on fixing u-boot first, and then > we can likely apply the same fix to the kernel. > > With which SoC(s) are you seeing this problem ? I believe that there > may be some differences between the mmc controller used in the A10/13 vs > later SoCs so this may be a SoC specific issue. It's a Cubieboard2 with Allwinner A20. Regards, yousong > > Regards, > > Hans > > > >> OpenWrt ticket 20387 has more info about the U-Boot failure. >> >> https://dev.openwrt.org/ticket/20387 >> >> Anyway, I have no idea what's the effect of those magic numbers on >> "sampling phases". Never played with such things before :) >> >>> I happended to fix some problems which seems *similar* to yours(but I'm >> >> not sure just from commit[1]'s msg): >>> >>> >>> https://patchwork.kernel.org/patch/7119661/ >>> >>>> Cheers >>>> >>>> yousong >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >>>> >>>> >>> >>> >>> -- >>> Best Regards >>> Shawn Lin >>> >>> -- >>> You received this message because you are subscribed to the Google Groups >> >> "linux-sunxi" group. >>> >>> To unsubscribe from this group and stop receiving emails from it, send an >> >> email to linux-sunxi+unsubscribe@googlegroups.com. >>> >>> For more options, visit https://groups.google.com/d/optout. >> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7 September 2015 at 15:02, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 06-09-15 16:47, Yousong Zhou wrote: >> >> On Sep 6, 2015 10:14 PM, "Shawn Lin" <shawn.lin@rock-chips.com> wrote: >>> >>> >>> ? 2015/9/6 20:09, Yousong Zhou ??: >>>> >>>> >>>> Hi, >>>> >>>> On 6 September 2015 at 08:12, Shawn Lin <shawn.lin@rock-chips.com> >>>> wrote: >>>>> >>>>> >>>>> On 2015/9/5 22:58, Hans de Goede wrote: >>>>>> >>>>>> >>>>>> >>>>>> Hi Shawn Lin, >>>>>> >>>>>> On 05-09-15 16:07, Shawn Lin wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 2015/9/5 18:19, Yousong Zhou wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> A SD card with sunxi-mmc can fail with the following error message >>>>>>>> (RCD for >>>>>>>> response CRC error) when trying to switch to highspeed mode. >>>>>>>> Setting >>>>>>>> the bus >>>>>>>> clock before the access mode switch fixed it. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> No, that's wrong! >>>>>>> >>>>>>> Before this card is switched to highspeed, it works under >>>>>>> identification mode(From spec: bus clock <= 400KHz). How could you >>>>>>> raise bus clock to higher clk rate which I _guess_ is 52MHz before >>>>>>> you >>>>>>> notify sd cards to run into highspeed mode? >>>>>>> >>>>>>> Althought it works for this card, this patch will not please the >>>>>>> other >>>>>>> cards that they might not reply CMDs any longer including the >>>>>>> following CMD6. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Thanks for the feedback, this is exactly why I asked Yousong Zhou to >>>>>> take this >>>>>> to the mmc list. >>>>>> >>>>>> So if this is not the proper fix for the problem that Yousong Zhou is >>>>>> seeing, then >>>>>> what might be the proper fix ? >>>>>> >>>>> >>>>> From my knowledge of mmc, there hadn't have a way to deal with this >> >> "broken" >>>>> >>>>> case. In another word, IMO,it's ANTI-SPEC. We can't be too spec >> >> sometimes, >>>>> >>>>> but at least we shouldn't violate it. >>>>> >>>> >>>> Maybe the fix is anti-spec. But the fact is that the card works on >>>> many other platforms with the builtin card reader (not through an USB >>>> adapter), including Mac OS X, my old Nokia Symbian phone, and Windows. >>>> >>>>>> Could it be that the sunxi-mmc is doing some things in the wrong order >>>>>> when >>>>>> changing the clock, or is this all under control of the mmc core ? >>>>>> >>>>> >>>>> all of this is under control of the mmc core. >>>>> So if Yongsong does want this card to work for any linux-based mmc >> >> stack, I >>>>> >>>>> guess something like that should be "better"? >>>>> >>>>> if (switch to HS fail) { >>>>> set_bus_clk >>>>> goto retry switch to HS >>>>> } >>>>> >>>>> BUT...I admit it seems strange as well. >>>>> >>>> >>>> The SD Specification (simplified version) says "If CRC error occurs on >>>> the status data, the host should issue a power cycle.", so I guess the >>>> above approach is anti-spec in some way :) >>>> >>> >>> Right?I was guessing that way from your intention. >>> >>> >>>> In case it may help debug this problem, I'd like to add that the card >>>> previously worked fine with U-Boot before commit [1]. This can also >>>> be confirmed by Debian Jessie installer image with which the old >>>> U-Boot there worked fine while the kernel (3.16) did not. >>>> >>>> [1] sunxi: mmc: Properly setup mod-clk and clock sampling phases, >>>> >> >> http://git.denx.de/?p=u-boot.git;a=commit;h=fc3a832576ce7bb597b1823935bfb7dcca235c3c > > > Interesting, one thing that patch does is introduce a switch from parent pll > when switching from 400KHz to higher clocks. Can you try the attached patch > ? > > If reverting u-boot commit fc3a832576ce7bb597b1823935bfb7dcca235c3c fixes > things, then it is probably best to focus on fixing u-boot first, and then > we can likely apply the same fix to the kernel. > I just tried a method from allwinner's u-boot sources: retry the mode switch in case of an error [1]. It was accompanied with a comment quoted below: //retry for Toshiba emmc;for the first time Toshiba emmc change to HS //it will return response crc err,so retry Well, that 1GB card of mine is labeled as "pqi" made in Korea. This workaround really worked and it succeeded on the second try, though I am not sure how anti-spec this can be (the spec says a power reset SHOULD be done in the case of crc error in the status data). The above change alone can make the card work. I also noticed that allwinner's uboot will disable the clock first before setting it and re-enable it thereafter [2]. I can only guess that this is possibly some bugfix made by engineers of allwinner. So I mimicked this approach and attached a patch here. [1] https://github.com/allwinner-zh/bootloader/blob/master/u-boot-2011.09/drivers/mmc/mmc.c#L2220 [2] https://github.com/allwinner-zh/bootloader/blob/master/u-boot-2011.09/drivers/mmc/sunxi_mmc.c#L822 Regards. yousong > With which SoC(s) are you seeing this problem ? I believe that there > may be some differences between the mmc controller used in the A10/13 vs > later SoCs so this may be a SoC specific issue. > > Regards, > > Hans > > > >> OpenWrt ticket 20387 has more info about the U-Boot failure. >> >> https://dev.openwrt.org/ticket/20387 >> >> Anyway, I have no idea what's the effect of those magic numbers on >> "sampling phases". Never played with such things before :) >> >>> I happended to fix some problems which seems *similar* to yours(but I'm >> >> not sure just from commit[1]'s msg): >>> >>> >>> https://patchwork.kernel.org/patch/7119661/ >>> >>>> Cheers >>>> >>>> yousong >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >>>> >>>> >>> >>> >>> -- >>> Best Regards >>> Shawn Lin >>> >>> -- >>> You received this message because you are subscribed to the Google Groups >> >> "linux-sunxi" group. >>> >>> To unsubscribe from this group and stop receiving emails from it, send an >> >> email to linux-sunxi+unsubscribe@googlegroups.com. >>> >>> For more options, visit https://groups.google.com/d/optout. >> >> >
Hi, On 08-09-15 17:38, Yousong Zhou wrote: > I just tried a method from allwinner's u-boot sources: retry the mode switch in > case of an error [1]. It was accompanied with a comment quoted below: > > //retry for Toshiba emmc;for the first time Toshiba emmc change to HS > //it will return response crc err,so retry > > Well, that 1GB card of mine is labeled as "pqi" made in Korea. This workaround > really worked and it succeeded on the second try, though I am not sure how > anti-spec this can be (the spec says a power reset SHOULD be done in the case > of crc error in the status data). > > The above change alone can make the card work. I also noticed that allwinner's > uboot will disable the clock first before setting it and re-enable it > thereafter [2]. I can only guess that this is possibly some bugfix made > by engineers of allwinner. So I mimicked this approach and attached a patch > here. Thanks for this. So if I understand you correctly then just the: "mmc: retry in the case of failing to switch to HS mode" Patch is enough to make things work, right ? That makes sense because the clock dance you are subscribing is something which allwinner does in many places and it really is not necessary. Can you do a similar patch for the kernel and test that too ? And once you've a working kernel patch submit that to Ulf, with the mmc list in the Cc? If Ulf is willing to take this fix on the kernel side (I'm the sunxi-mmc maintainer in the kernel, and the fix will get my ack), then I'm willing to take the u-boot version of this fix. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From 1523e6fbd27b5da22b0c3cb4e8eda50bce7562cf Mon Sep 17 00:00:00 2001 From: Hans de Goede <hdegoede@redhat.com> Date: Mon, 7 Sep 2015 08:56:52 +0200 Subject: [PATCH] sunxi: mmc: Add a delay after changing the PLL to allow clock to stabilize Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/mmc/sunxi_mmc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c index 25f18ad..db3c489 100644 --- a/drivers/mmc/sunxi_mmc.c +++ b/drivers/mmc/sunxi_mmc.c @@ -204,6 +204,8 @@ static int mmc_config_clock(struct mmc *mmc) if (mmc_set_mod_clk(mmchost, mmc->clock)) return -1; + mdelay(100); + /* Clear internal divider */ rval &= ~SUNXI_MMC_CLK_DIVIDER_MASK; writel(rval, &mmchost->reg->clkcr); -- 2.4.3