LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Shawn Lin <shawn.lin@rock-chips.com>
To: Jaehoon Chung <jh80.chung@samsung.com>
Cc: shawn.lin@rock-chips.com, Doug Anderson <dianders@chromium.org>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	Alim Akhtar <alim.akhtar@samsung.com>,
	Sonny Rao <sonnyrao@chromium.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Alexandru Stan <amstan@chromium.org>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] mmc: dw_mmc: Consider HLE errors to be data and command errors
Date: Thu, 26 May 2016 12:07:25 +0800	[thread overview]
Message-ID: <10d2520b-0919-0a5b-f5b6-23b133e63dbb@rock-chips.com> (raw)
In-Reply-To: <5746748B.7080305@samsung.com>

在 2016/5/26 11:59, Jaehoon Chung 写道:
> On 05/26/2016 11:23 AM, Shawn Lin wrote:
>> Hi Jaehoon,
>>
>> On 2016/5/19 21:07, Jaehoon Chung wrote:
>>> On 05/19/2016 08:31 PM, Shawn Lin wrote:
>>>> Hi,
>>>>
>>>> On 2016/5/19 1:37, Doug Anderson wrote:
>>>>> Hi,
>>>>>
>>>>> On Wed, May 18, 2016 at 2:14 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>>>>> Hi
>>>>>>
>>>>>>
>>>>>> On 2016-5-18 12:12, Doug Anderson wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Tue, May 17, 2016 at 6:59 PM, Shawn Lin
>>>>>>> <shawn.lin@kernel-upstream.org> wrote:
>>>>>>>>
>>>>>>>> Could you try this patch to see if you can still find HLE?
>>>>>>>>
>>>>>>>> @@ -2356,12 +2356,22 @@ static void dw_mci_cmd_interrupt(struct dw_mci
>>>>>>>> *host, u32 status)
>>>>>>>>   static void dw_mci_handle_cd(struct dw_mci *host)
>>>>>>>>   {
>>>>>>>>          int i;
>>>>>>>> +       int present;
>>>>>>>>
>>>>>>>>          for (i = 0; i < host->num_slots; i++) {
>>>>>>>>                  struct dw_mci_slot *slot = host->slot[i];
>>>>>>>>
>>>>>>>>                  if (!slot)
>>>>>>>>                          continue;
>>>>>>>>
>>>>>>>> +               present = !(mci_readl(slot->host, CDETECT) & (1 <<
>>>>>>>> slot->id));
>>>>>>>> +               if (present)
>>>>>>>> +                       set_bit(DW_MMC_CARD_PRESENT, &slot->flags);
>>>>>>>> +               else
>>>>>>>> +                       clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
>>>>>>>
>>>>>>>
>>>>>>> No, because we don't use the builtin card detect on veyron.  ;)
>>>>>>>
>>>>>>> We use GPIO card detect because we didn't like the way JTAG and SD
>>>>>>> interacted.  Also on rk3288 the builtin card detect line had the wrong
>>>>>>> voltage domain (you couldn't detect a card when the IO lines were
>>>>>>> powered off).  The builtin card detect line is always driven low on
>>>>>>> veyron.
>>>>>>
>>>>>>
>>>>>> Okay, I see.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I'm nearly certain that the root cause of my HLE errors is actually
>>>>>>> related to the same problem addressed by the commit 7c5209c315ea
>>>>>>> ("mmc: core: Increase delay for voltage to stabilize from 3.3V to
>>>>>>> 1.8V").  I think that on minnie we're still on the hairy edge and
>>>>>>> sometimes the line doesn't transition fast enough.
>>>>>>
>>>>>>
>>>>>> Things are not so simple from your details.
>>>>>>
>>>>>> I was not enabling SD3.0 support, then I also found HLE sometimes.
>>>>>> So it seems commit 7c5209c315ea does not contibute to this phenomenon.
>>>>>
>>>>> Just to clarify, in my case commit 7c5209c315ea didn't make the
>>>>> problem worse, but made it better.  Just not better enough.  ;)
>>>>>
>>>>>
>>>>>> The scenario looks like:
>>>>>> remove sd-card -> mmc_sd_detect -> send status(CMD13) ->power_off ->
>>>>>> set_ios -> setup_bus -> disabled clk , then HLE irq storm coming
>>>>>>
>>>>>> From the code of dw_mci_prepare_command:
>>>>>> SDMMC_CMD_PRV_DAT_WAIT will not be used for CMD13, so we don't
>>>>>> wait_busy here, then cmd code is loding into queue of dw_mmc but
>>>>>> still failing send out because it's in busy?
>>>>>>
>>>>>> With my patch, things go well:
>>>>>> remove sd-card -> clear bit of DW_MMC_CARD_PRESENT  -> send
>>>>>> status(CMD13) return directly -> power_off -> set_ios -> setup_bus ->
>>>>>> disable clk
>>>>>>
>>>>>> So why should we allow inquiry of card status if we sure the card is
>>>>>> removed? I mean no any further cmds should be delivered.
>>>>>
>>>>> Quite honestly just dealing with the HLE error (my patch or
>>>>> equivalent) might be a sane solution for the problem you describe.
>>>>
>>>> Yes, your patch looks good to me, so it should be merged firstly. :)
>>>> Then let's push it a bit further more that when HLEs are coming,
>>>> somethings must be wrong(currently I don't see a obvious clue from
>>>> the code itself although, I'm prone to think it belongs to the
>>>> software issue).
>>>
>>> We don't know what's main cause for HLE..But i also think it's relevant to SW issue.
>>> But we need to consider all possibilities..
>>>
>>>>
>>>>
>>>>>
>>>>> dw_mmc needs to be able to work with an external card detect GPIO.
>>>>> It's been part of the dw_mmc driver for a long time and is (in fact)
>>>>> in use upstream at least by rk3288-veyron.  Any solution that only
>>>>> works for internal card detect is not enough.  Just handling the HLE
>>>>> error to deal with the interrupt storm and then letting Linux remove
>>>>> the card (because of the card detect interrupt) seems totally OK to
>>>>> me.
>>>>>
>>>>
>>>> Sure, some of rockchip Socs use gpio for CD because they don't
>>>> have a internal CD, such as RK3036, right?
>>>>
>>>>> Note: I'd be very curious if your problems get better if you disable
>>>>
>>>> Not at all.
>>>>
>>>>> the "grf_force_jtag" bit in the GRF.  If you're using the builtin card
>>>>> detect and you use the boot default of "grf_force_jtag" then your pins
>>>>> will be unmuxed behind your back when the card is ejected.  This could
>>>>> be causing the dw_mmc controller to get confused.
>>>>
>>>> Right, grf_force_jtag is also not a friend of mine. :)
>>>> So I had disabled this function before I was debugging it.
>>>>
>>>>>
>>>>>
>>>>>> And another question: should we wait busy for cmd13?
>>>>>
>>>>> I don't think so.  As I understand it CMD13 uses only the CMD line for
>>>>> communication and it should be appropriate to send this when the bus
>>>>> is "busy" (which means that the DATA lines are low).
>>>>
>>>> Ahh... take back my question.. I was just considering a wired situation
>>>> that pins are unmuxed on the background(cmd line as well) when cmd13 is
>>>> delivering....
>>>>
>>>>
>>>>>
>>>>> Also: it seems odd that the HLE IRQ storm didn't come right after the
>>>>> CMD 13 in your description above.  Are you sure it was the CMD 13 that
>>>>> caused the HLEs, or could it has been something else?
>>>>
>>>> Actually no. Any cmds be issued can trigger HLEs, I think, after sd card is removed When I hacked mmc_sd_detecd to send other cmds intead
>>>> of cmd13.
>>>>
>>>> From dw_mmc databook v270a(7.2.3 Clock Programming) we can see:
>>>> The DWC_mobile_storage loads each of these registers only when the
>>>> start_cmd bit and the Update_clk_regs_only bit in the CMD register are
>>>> set. When a command is successfully loaded, the DWC_mobile_storage
>>>> clears this bit, unless the DWC_mobile_storage already has another
>>>> command in the queue, at which point it gives an HLE (Hardware Locked
>>>> Error); for details on HLEs, refer to “Error Handling” on page 233.
>>>> Software should look for the start_cmd and the Update_clk_regs_only
>>>> bits, and should also set the wait_prvdata_complete bit to ensure that
>>>> clock parameters do not change during data transfer.
>>>>
>>>> Maybe the cmd is trying to load(or somethings wrong with the
>>>> controller?) when we disable the clk? That may explain my observation
>>>> that HLEs came after disabling clk.
>>>
>>> I agreed.
>>>
>>> To Disable clock, it sends cmd with update_clk_regs_only and wait_prvdata_complete bit.
>>> I think it's problem..(waiting for prvdata..)
>>>
>>> If there are ongoing some data(read/write), then before disabling clock, waiting for completing previous data.
>>> (But card was already removed, and it couldn't do anything.)
>>> It's difficult to analyze the HLE..So After applying first, then we can solve this problem, step by step.
>>>
>>
>> I saw you send a PR for v4.7-fix to Ulf which didn't include this one.
>> Do you plan to add it into 4.8 materials? :)
>
> Yes, I think good that this is prepared for next. (Will apply on this weekend.)
> Do you have other opinion? :)

No, thanks. I just wanna make sure this one will be merged. :)

>
> If you have other opinion, i will reflect yours. Thanks!
>
> Best Regards,
> Jaehoon Chung
>
>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>> -Doug
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>
>
>
>


-- 
Best Regards
Shawn Lin

  reply	other threads:[~2016-05-26  4:07 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-10 15:48 Doug Anderson
2015-03-13 11:30 ` Jaehoon Chung
2015-03-13 20:27   ` Doug Anderson
2015-03-16  5:56     ` Jaehoon Chung
2015-03-30  0:55       ` Jaehoon Chung
2015-03-30 15:47         ` Doug Anderson
2016-05-18  0:47           ` Doug Anderson
2016-05-18  1:59             ` Shawn Lin
2016-05-18  4:12               ` Doug Anderson
2016-05-18  9:14                 ` Shawn Lin
2016-05-18 17:37                   ` Doug Anderson
2016-05-18 18:01                     ` Heiko Stuebner
2016-05-19 11:31                     ` Shawn Lin
2016-05-19 13:07                       ` Jaehoon Chung
2016-05-26  2:23                         ` Shawn Lin
2016-05-26  3:59                           ` Jaehoon Chung
2016-05-26  4:07                             ` Shawn Lin [this message]
2016-05-18  2:08             ` Jaehoon Chung
2016-05-18  4:13               ` Doug Anderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=10d2520b-0919-0a5b-f5b6-23b133e63dbb@rock-chips.com \
    --to=shawn.lin@rock-chips.com \
    --cc=alim.akhtar@samsung.com \
    --cc=amstan@chromium.org \
    --cc=dianders@chromium.org \
    --cc=heiko@sntech.de \
    --cc=jh80.chung@samsung.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mmc@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=sonnyrao@chromium.org \
    --cc=ulf.hansson@linaro.org \
    --subject='Re: [PATCH] mmc: dw_mmc: Consider HLE errors to be data and command errors' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).