Date:	Thu, 4 Jun 2015 09:32:19 +0200
From:	Ulf Hansson <ulf.hansson@...aro.org>
To:	Chaotian Jing <chaotian.jing@...iatek.com>
Cc:	Rob Herring <robh+dt@...nel.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Chris Ball <chris@...ntf.net>,
	Mark Rutland <mark.rutland@....com>,
	James Liao <jamesjj.liao@...iatek.com>,
	srv_heupstream <srv_heupstream@...iatek.com>,
	Arnd Bergmann <arnd@...db.de>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Hongzhou Yang <hongzhou.yang@...iatek.com>,
	Catalin Marinas <catalin.marinas@....com>,
	linux-mmc <linux-mmc@...r.kernel.org>,
	Will Deacon <will.deacon@....com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	Sascha Hauer <kernel@...gutronix.de>,
	"Joe.C" <yingjoe.chen@...iatek.com>,
	Eddie Huang <eddie.huang@...iatek.com>,
	Bin Zhang (章斌) <bin.zhang@...iatek.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v4 2/7] mmc: mediatek: Add Mediatek MMC driver

On 27 May 2015 at 13:34, Chaotian Jing <chaotian.jing@...iatek.com> wrote:
> On Tue, 2015-05-26 at 14:33 +0200, Ulf Hansson wrote:
>> [...]
>>
>> >> >> > +{
>> >> >> > +       unsigned long tmo = jiffies + msecs_to_jiffies(20);
>> >> >> > +
>> >> >> > +       while ((readl(host->base + SDC_STS) & SDC_STS_CMDBUSY)
>> >> >> > +                       && time_before(jiffies, tmo))
>> >> >> > +               continue;
>> >> >> > +
>> >> >> > +       if (readl(host->base + SDC_STS) & SDC_STS_CMDBUSY) {
>> >> >> > +               dev_err(host->dev, "CMD bus busy detected\n");
>> >> >> > +               host->error |= REQ_CMD_BUSY;
>> >> >> > +               msdc_cmd_done(host, MSDC_INT_CMDTMO, mrq, cmd);
>> >> >> > +               return false;
>> >> >> > +       }
>> >> >> > +
>> >> >> > +       if (mmc_resp_type(cmd) == MMC_RSP_R1B || cmd->data) {
>> >> >> > +               /* R1B or with data, should check SDCBUSY */
>> >> >> > +               while (readl(host->base + SDC_STS) & SDC_STS_SDCBUSY)
>> >> >> > +                       cpu_relax();
>> >> >> > +       }
>> >> >>
>> >> >> MSDC seems to be handling card busy detection in HW, right?
>> >> >>
>> >> > Do not have this ability, HW only know if CMD/DAT is low, but do not
>> >> > have any interrupt for it,
>> >>
>> >> I see, but doesn't the above polling mean that msdc will not propagate
>> >> the response until the card have stopped signal busy? That's what
>> >> MMC_CAP_WAIT_WHILE_BUSY shall be used for.
>> >>
>> > As you see, we only check the "busy state" BEFORE issue a R1B command or
>> > with data command, but do not check if AFTER the request was done, that
>> > would do not match the "MMC_CAP_WAIT_WHILE_BUSY"(eg. CMD5 to sleep)
>>
>> Okay, fair enough. Still, I don't understand why you want to do that?
>>
> Below is the description of these 2 bits:
> 1       CMDBSY  CMDBUSY SD Command line busy status
> S/W should always read this bit to make sure the command line is not
> busy before sending the next command.
> If the command is R1B or data read/write command, S/W should check
> SDCBUSY bit too.
> Note: When Auto command 12 is enabled, this bit will be asserted
> immediately after SDC_CMD is written and de-asserted after auto-command
> 12 finishes.
> 1'b0: No transmission is going on CMD line on SD bus
> 1'b1: There exists transmission going on CMD line on SD bus
> 0       SDCBSY  SDCBUSY SD controller busy status
> 1'b0: SD controller is idle
> 1'b1: SD controller is busy
>

Thanks, for elaborating. I get it know. :-)

>> > In addition, about CMD5, I find that the suspend/resume flow of EMMC is
>> > stranger in new kernel version, when suspend, it may issue CMD5 to enter
>> > sleep mode, then power off MMC, but when resume, it will
>> > re-initialization, So that why need do the redundant CMD5 in suspend ?
>>
>> Both CMD0 and CMD5 are valid as wakeup commands.
>>
>> To be able to use CMD5, the VCCQ regulator must be kept enabled in the
>> sleep state. That's when it becomes a bit tricky, due to the range of
>> different host drivers and SoCs for how VCCQ is managed.
>>
>> To be safe we have chosen to use CMD0, since it works for *all* cases
>> no matter if VCCQ get gated or not.
>>
>> Moreover, using CMD5 as the wakeup command would require added
>> complexity in the code dealing with suspend/resume. I don't think the
>> effort is worth it, at least until someone has proven that the resume
>> time is greatly decreased by using CMD5.
>>
>> >
>> >> Perhaps you should remove the above polling, and rely on the MMC core
>> >> to poll with CMD13 instead?
>> > before any read/write command, core will issue CMD13 to confirm card
>> > status, here is just only do double confirm to avoid HW issue.
>>
>> What HW issue and why do you need to double confirm? It seems strange.
> Some fake SD card, even the CMD13 get card status is in transfer state,
> but when we do read/write, the data0 was pull low.

Okay. So in this case your mmc host driver will take care of this. That's fine.

My concern is then how other mmc host drivers will deal with these
kind of cards that violates the spec. It would be nice if you could
add a card quirk for these cards, if you still recall what cards that
were causing these issues.

>
> By the way, do you think need do some spinlock protect in our code ?
> I cannot find a good reason to use spin lock, it seems no need
> it,although there will be many thread(mmcqd, mmcqd/0boot1,) and the irq
> handler.

>From mmc core layer point of view, you shouldn't be needing any spinlocks.

Whether your driver has a potential concurrent access of the same
registers, I don't know. If that's the case you probably want to
protect this from happening.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists