Re: [PATCH] mmc: dw_mmc: Consider HLE errors to be data and command errors

[Shawn Lin]

å 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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