Re: Re: [RFC] mmc: core: Set clock before switching to highspeed mode.

[Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5321 bytes --]

Hi,

On 06-09-15 16:47, Yousong Zhou wrote:
> On Sep 6, 2015 10:14 PM, "Shawn Lin" <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>>
>> 在 2015/9/6 20:09, Yousong Zhou 写道:
>>>
>>> Hi,
>>>
>>> On 6 September 2015 at 08:12, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> 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.

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
>> For more options, visit https://groups.google.com/d/optout.
>

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: 0001-sunxi-mmc-Add-a-delay-after-changing-the-PLL-to-allo.patch --]
[-- Type: text/x-patch, Size: 826 bytes --]

>From 1523e6fbd27b5da22b0c3cb4e8eda50bce7562cf Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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);