* Re: [PATCH v8 09/16] mmc: sdhci: Factor out sdhci_enable_clock
From: Adrian Hunter @ 2016-11-18 13:56 UTC (permalink / raw)
To: Ritesh Harjani, ulf.hansson, linux-mmc, sboyd, andy.gross
Cc: shawn.lin, devicetree, linux-clk, david.brown, linux-arm-msm,
georgi.djakov, alex.lemberg, mateusz.nowak, Yuliy.Izrailov,
asutoshd, david.griego, stummala, venkatg, rnayak, pramod.gurav,
jeremymc
In-Reply-To: <1479312052-22396-10-git-send-email-riteshh@codeaurora.org>
On 16/11/16 18:00, Ritesh Harjani wrote:
> Factor out sdhci_enable_clock from sdhci_set_clock
> and make it EXPORT_SYMBOL so that it can be called.
This seems fine apart from minor comments below.
>
> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
> ---
> drivers/mmc/host/sdhci.c | 28 +++++++++++++++++-----------
> drivers/mmc/host/sdhci.h | 1 +
> 2 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index d08d507..8a89d89 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1344,20 +1344,10 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
> }
> EXPORT_SYMBOL_GPL(sdhci_calc_clk);
>
> -void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> +void sdhci_enable_clock(struct sdhci_host *host, unsigned short clk)
We write a u16 so let's make clk a u16 instead of unsigned short.
I would have called it sdhci_enable_clk() to go with sdhci_calc_clk() and
slightly different from sdhci_set_clock() which is a callback whereas the
other 2 are helpers.
> {
> - u16 clk;
> unsigned long timeout;
>
> - host->mmc->actual_clock = 0;
> -
> - sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> -
> - if (clock == 0)
> - return;
> -
> - clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock);
> -
> clk |= SDHCI_CLOCK_INT_EN;
> sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>
> @@ -1378,6 +1368,22 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> clk |= SDHCI_CLOCK_CARD_EN;
> sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> }
> +EXPORT_SYMBOL_GPL(sdhci_enable_clock);
> +
> +void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> + u16 clk;
> +
> + host->mmc->actual_clock = 0;
> +
> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> +
> + if (clock == 0)
> + return;
> +
> + clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock);
> + sdhci_enable_clock(host, clk);
> +}
> EXPORT_SYMBOL_GPL(sdhci_set_clock);
>
> static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode,
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 766df17..8e77a3b 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -681,6 +681,7 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
> u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
> unsigned int *actual_clock);
> void sdhci_set_clock(struct sdhci_host *host, unsigned int clock);
> +void sdhci_enable_clock(struct sdhci_host *host, unsigned short clk);
> void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> unsigned short vdd);
> void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode,
>
^ permalink raw reply
* Re: [PATCH 9/9] mmc: core: Allow CMD13 polling when switch to HS400ES mode
From: Adrian Hunter @ 2016-11-18 13:35 UTC (permalink / raw)
To: Ulf Hansson, linux-mmc
Cc: Jaehoon Chung, Linus Walleij, Chaotian Jing, Stephen Boyd,
Michael Walle, Yong Mao, Shawn Lin
In-Reply-To: <1479293481-20186-10-git-send-email-ulf.hansson@linaro.org>
On 16/11/16 12:51, Ulf Hansson wrote:
> In cases when the mmc host doesn't support HW busy detection, polling for
> busy by using CMD13 is beneficial. The reasons have already been explained
> in earlier change logs.
>
> Moreover, when polling with CMD13 during bus timing changes, we should
> retry instead of fail when we get CRC errors.
>
> Switching to HS400ES includes several steps, where each step changes the
> bus speed timing. Let's improve the behaviour during these sequences, by
> allowing CMD13 polling for each of the step. Let's also make sure the CMD13
> polling becomes retried, while receiving a CRC error.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/mmc/core/mmc.c | 35 +++++++++++------------------------
> 1 file changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 24b9e72..b6f0035 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1227,31 +1227,24 @@ static int mmc_select_hs400es(struct mmc_card *card)
> goto out_err;
>
> /* Switch card to HS mode */
> - err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> - EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
> - card->ext_csd.generic_cmd6_time, 0,
> - true, false, true);
> + err = mmc_select_hs(card);
> if (err) {
> pr_err("%s: switch to hs for hs400es failed, err:%d\n",
> mmc_hostname(host), err);
> goto out_err;
> }
>
> - mmc_set_timing(host, MMC_TIMING_MMC_HS);
> - err = mmc_switch_status(card);
> - if (err)
> - goto out_err;
> -
> mmc_set_clock(host, card->ext_csd.hs_max_dtr);
>
> - /* Switch card to DDR with strobe bit */
> + /* Switch card to HS DDR with strobe bit */
> val = EXT_CSD_DDR_BUS_WIDTH_8 | EXT_CSD_BUS_WIDTH_STROBE;
> - err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> - EXT_CSD_BUS_WIDTH,
> - val,
> - card->ext_csd.generic_cmd6_time);
> + err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> + EXT_CSD_BUS_WIDTH, val,
> + card->ext_csd.generic_cmd6_time,
> + MMC_TIMING_MMC_DDR52,
> + true, true, true);
> if (err) {
> - pr_err("%s: switch to bus width for hs400es failed, err:%d\n",
> + pr_err("%s: switch to hs ddr for hs400es failed, err:%d\n",
> mmc_hostname(host), err);
> goto out_err;
> }
> @@ -1261,26 +1254,20 @@ static int mmc_select_hs400es(struct mmc_card *card)
> card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> EXT_CSD_HS_TIMING, val,
> - card->ext_csd.generic_cmd6_time, 0,
> - true, false, true);
> + card->ext_csd.generic_cmd6_time,
> + MMC_TIMING_MMC_HS400,
> + true, true, true);
This could be a problem because the CMD13 is being sent to a card in HS400
enhanced strobe mode but we haven't enabled enhanced strobe on the host
controller yet. Previously mmc_switch_status(card) was below
host->ops->hs400_enhanced_strobe().
> if (err) {
> pr_err("%s: switch to hs400es failed, err:%d\n",
> mmc_hostname(host), err);
> goto out_err;
> }
>
> - /* Set host controller to HS400 timing and frequency */
> - mmc_set_timing(host, MMC_TIMING_MMC_HS400);
> -
> /* Controller enable enhanced strobe function */
> host->ios.enhanced_strobe = true;
> if (host->ops->hs400_enhanced_strobe)
> host->ops->hs400_enhanced_strobe(host, &host->ios);
>
> - err = mmc_switch_status(card);
> - if (err)
> - goto out_err;
> -
> return 0;
>
> out_err:
>
^ permalink raw reply
* Re: arasan,sdhci.txt "compatibility" DT binding
From: Michal Simek @ 2016-11-18 13:22 UTC (permalink / raw)
To: Mason, Rameshwar Sahu
Cc: Ulf Hansson, Suman Tripathi, Heiko Stuebner, Shawn Lin,
Adrian Hunter, Jisheng Zhang, Russell King, Anton Vorontsov,
Michal Simek, Soren Brinkmann, Linus Walleij, P L Sai Krishna,
Zach Brown, Sebastian Frias, Arnd Bergmann, Suneel Garapati,
Linux ARM, Michal Simek, linux-mmc, Douglas Anderson,
Maxime Ripard, Xiaobo Xie
In-Reply-To: <582EF744.8090307@free.fr>
On 18.11.2016 13:42, Mason wrote:
> On 18/11/2016 11:49, Rameshwar Sahu wrote:
>
>> Mason wrote:
>>
>>> Suman/APM added "arasan,sdhci-4.9a" in 308f3f8d8112
>>> @Suman, @Rameshwar: what specific IP block does your SoC embed?
>>> What does 4.9a refer to? The documentation revision number?
>>
>> We have Arasan SD3.0/ SDIO3.0/ eMMC4.41 AHB Host Controller IP
>> embedded in our SoC and 4.9a is documentation revision number which
>> was given by Arasan.
>> FYI this documentation date was May, 2012.
>
> Hello Ram,
>
> Thanks for the information.
>
> Xilinx is using "SD2.0/SDIO2.0/MMC3.31 AHB Host Controller"
in Xilinx Zynq device
and
Xilinx ZynqMP device is using
"SD3.0/SDIO3.0/eMMC4.51 AHB Host Controller"
Thanks,
Michal
^ permalink raw reply
* Re: [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode
From: Ulf Hansson @ 2016-11-18 13:16 UTC (permalink / raw)
To: Adrian Hunter
Cc: linux-mmc, Jaehoon Chung, Linus Walleij, Chaotian Jing,
Stephen Boyd, Michael Walle, Yong Mao, Shawn Lin
In-Reply-To: <c0781701-5977-533d-6178-d8b5536a82bd@intel.com>
[...]
>>>
>>> I should point out that retrying CMD13 will clear the error bits in the
>>> status so there is no point retrying when checking for the SWITCH_ERROR bit.
>>> i.e. we need a version of __switch_send_status() that sets retries to zero.
>>
>> Are you really sure about this?
>
> I don't think I have ever tested it. I was going on what the spec says:
> "1) Error bit. Signals an error condition was detected by the device. These
> bits are cleared as soon as the response (reporting the error) is sent out."
Couldn't have been more clear than that.
So when doing CMD13 polling, it's seems like we should be validating
the SWITCH_ERROR bit for each successful CMD13 response, as otherwise
we may end up loosing that information.
[...]
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 8/9] mmc: core: Allow CMD13 polling when switch to HS400 mode
From: Ulf Hansson @ 2016-11-18 12:59 UTC (permalink / raw)
To: Adrian Hunter
Cc: linux-mmc, Jaehoon Chung, Linus Walleij, Chaotian Jing,
Stephen Boyd, Michael Walle, Yong Mao, Shawn Lin, Ziyuan Xu
In-Reply-To: <5b69bb3b-ede3-753c-e261-89e51ca0e43d@intel.com>
On 18 November 2016 at 13:02, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 16/11/16 12:51, Ulf Hansson wrote:
>> In cases when the mmc host doesn't support HW busy detection, polling for
>> busy by using CMD13 is beneficial. The reasons have already been explained
>> in earlier change logs.
>>
>> Moreover, when polling with CMD13 during bus timing changes, we should
>> retry instead of fail when we get CRC errors.
>>
>> Switching from HS200 to HS400 and reverse includes several steps, where
>> each step changes the bus speed timing. Let's improve the behaviour during
>> these sequences, by allowing CMD13 polling for each of the step. Let's also
>> make sure the CMD13 polling becomes retried, while receiving a CRC error.
>
> I don't know we need to try to get polling for HS400. The support of HS400
> suggests a relatively sophisticated host controller which really should
> support busy waiting as well.
That's a reasonable argument.
Moreover, I do also think that argument stands for a controller
supporting HS200. So I have no problem dropping the CMD13 polling
support for that bus speed mode as well, if people wants that.
>
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>> drivers/mmc/core/mmc.c | 87 +++++++++++++++-----------------------------------
>> 1 file changed, 26 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 0b26383..24b9e72 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1094,8 +1094,7 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
>> static int mmc_select_hs400(struct mmc_card *card)
>> {
>> struct mmc_host *host = card->host;
>> - unsigned int max_dtr;
>> - int err = 0;
>> + int err;
>> u8 val;
>>
>> /*
>> @@ -1105,34 +1104,26 @@ static int mmc_select_hs400(struct mmc_card *card)
>> host->ios.bus_width == MMC_BUS_WIDTH_8))
>> return 0;
>>
>> - /* Switch card to HS mode */
>> - val = EXT_CSD_TIMING_HS;
>> - err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> - EXT_CSD_HS_TIMING, val,
>> - card->ext_csd.generic_cmd6_time, 0,
>> - true, false, true);
>> + /*
>> + * Switch card to HS mode.
>> + * First reduce to the HS frequency as CMD13 are sent to poll for busy
>> + * and/or to validate the switch status.
>> + */
>> + mmc_set_clock(host, card->ext_csd.hs_max_dtr);
>
> That was moved by commit 649c6059d2371fef886a8f967e21416204723d63
> ("mmc: mmc: Fix HS switch failure in mmc_select_hs400()") i.e. if you put it
> back I would expect the gru board problem to reappear.
Correct, thanks for pointing it out!
According to below comments, it's clear that the JEDEC spec lacks a
good description of the HS200/400 switch behaviour.
>
>> + err = mmc_select_hs(card);
>> if (err) {
>> pr_err("%s: switch to high-speed from hs200 failed, err:%d\n",
>> mmc_hostname(host), err);
>> return err;
>> }
>>
>> - /* Set host controller to HS timing */
>> - mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
>> -
>> - /* Reduce frequency to HS frequency */
>> - max_dtr = card->ext_csd.hs_max_dtr;
>> - mmc_set_clock(host, max_dtr);
>> -
>> - err = mmc_switch_status(card);
>> - if (err)
>> - goto out_err;
>> -
>> - /* Switch card to DDR */
>> - err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> - EXT_CSD_BUS_WIDTH,
>> - EXT_CSD_DDR_BUS_WIDTH_8,
>> - card->ext_csd.generic_cmd6_time);
>> + /* Switch card to HS DDR */
>> + err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> + EXT_CSD_BUS_WIDTH,
>> + EXT_CSD_DDR_BUS_WIDTH_8,
>> + card->ext_csd.generic_cmd6_time,
>> + MMC_TIMING_MMC_DDR52,
>> + true, true, true);
>> if (err) {
>> pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
>> mmc_hostname(host), err);
>> @@ -1144,28 +1135,17 @@ static int mmc_select_hs400(struct mmc_card *card)
>> card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> EXT_CSD_HS_TIMING, val,
>> - card->ext_csd.generic_cmd6_time, 0,
>> - true, false, true);
>> + card->ext_csd.generic_cmd6_time,
>> + MMC_TIMING_MMC_HS400,
>> + true, true, true);
>
> This is not exactly right. Tuning has been done at the HS400 operating
> frequency. That is why we set the bus speed before sending any more
> commands. i.e. mmc_switch_status(card) is below mmc_set_bus_speed(card)
Right, I see!
I had another look at the JEDEC spec for HS400 mode switch:
---
6) Perform the Tuning Process at the HS400 target operating frequency,
NOTE Tuning process in HS200 mode is required to synchronize the
command response on the CMD line to CLK for HS400 operation.
---
This confirms your point. Although on the other hand by looking at the
HS400 switch flow chart, it says that checking the switch status shall
be done *before* bumping the clock rate. :-)
Anyway, I will move back to the behaviour where CMD13 polling is not
allowed for HS400, as that seems like the only thing we can do!
Kind regards
Uffe
^ permalink raw reply
* Re: arasan,sdhci.txt "compatibility" DT binding
From: Mason @ 2016-11-18 12:42 UTC (permalink / raw)
To: Rameshwar Sahu
Cc: Linux ARM, linux-mmc, Soren Brinkmann, Michal Simek, Shawn Lin,
Michal Simek, Anton Vorontsov, Xiaobo Xie, Suman Tripathi,
Linus Walleij, Maxime Ripard, Arnd Bergmann, Rob Herring,
Adrian Hunter, Zach Brown, Ulf Hansson, Douglas Anderson,
Heiko Stuebner, Jisheng Zhang, Suneel Garapati <sunee>
In-Reply-To: <CAFd313x6MJeO90_+rxditdPn762g7TqZc0_D6ojGTKmtGJKjPA@mail.gmail.com>
On 18/11/2016 11:49, Rameshwar Sahu wrote:
> Mason wrote:
>
>> Suman/APM added "arasan,sdhci-4.9a" in 308f3f8d8112
>> @Suman, @Rameshwar: what specific IP block does your SoC embed?
>> What does 4.9a refer to? The documentation revision number?
>
> We have Arasan SD3.0/ SDIO3.0/ eMMC4.41 AHB Host Controller IP
> embedded in our SoC and 4.9a is documentation revision number which
> was given by Arasan.
> FYI this documentation date was May, 2012.
Hello Ram,
Thanks for the information.
Xilinx is using "SD2.0/SDIO2.0/MMC3.31 AHB Host Controller"
APM is using "SD3.0/SDIO3.0/eMMC4.41 AHB Host Controller"
Sigma is using "SD3.0/SDIO3.0/eMMC4.4 AHB Host Controller"
Rockchip is using ? (Shawn could you specify exactly which IP you use?)
And it looks like Arasan is using different revision numbers for
every documentation they have.
The "SD3.0/SDIO3.0/eMMC4.4 AHB Host Controller" doc I have is
at rev number 6.0, dated 2010-02-19.
Regards.
^ permalink raw reply
* Re: [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode
From: Adrian Hunter @ 2016-11-18 12:32 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Jaehoon Chung, Linus Walleij, Chaotian Jing,
Stephen Boyd, Michael Walle, Yong Mao, Shawn Lin
In-Reply-To: <CAPDyKFoAGeMqdUaniohshXDULpetNBxj-Tt6+oDzrWR6u9wcQQ@mail.gmail.com>
On 18/11/16 14:20, Ulf Hansson wrote:
> On 18 November 2016 at 10:30, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 17/11/16 17:02, Ulf Hansson wrote:
>>> On 17 November 2016 at 11:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 16/11/16 12:51, Ulf Hansson wrote:
>>>>> In cases when the mmc host doesn't support HW busy detection, polling for
>>>>> busy by using CMD13 is beneficial. The reasons have already been explained
>>>>> in earlier change logs.
>>>>>
>>>>> To allow polling with CMD13, let's provide MMC_TIMING_MMC_HS200 as the
>>>>> timing parameter to __mmc_switch(), which makes sure the mmc host and the
>>>>> mmc card operates at the same bus timing during the polling.
>>>>
>>>> I have reports of cases where CMD13 always gives CRC errors after switch
>>>> to HS200. Currently we are assuming the low frequency should mean that
>>>> won't happen, but it does in some cases. That is not entirely surprising
>>>> since HS200 needs tuning at the final operating frequency.
>>>
>>> >From a logical point of view and if tuning is needed also for the CMD
>>> line, this somehow make sense.
>>>
>>> However, this is *not* how the JEDEC spec describes the HS200 switch
>>> sequence. It is clearly stated that the host should validate the CM6
>>> status via sending a CMD13 command, *before* performing tuning.
>>
>> I agree, it seems not to be following spec.
>>
>>>
>>> Could it be that the observations about the CRC errors, is related to
>>> a controller/driver issue and not a card issue?
>>
>> I don't know what causes the problem (and I have a sneaking suspicion that
>> if vendors configured / designed their boards correctly, it wouldn't
>> happen). However, while some cards have better signal characteristics than
>> others, tuning is a host controller issue - the card doesn't care.
>>
>>>
>>>>
>>>> What I would like to do for hosts that support busy waiting or DAT0 polling
>>>> (i.e. MMC_CAP_WAIT_WHILE_BUSY or host->ops->card_busy), is to ignore CRC
>>>> errors from the CMD13 that checks the switch status. The main reason for
>>>> doing that is that we really expect the switch to succeed and, given HS200
>>>> tuning requirement, the CRC error is not a reliable means of determining
>>>> that it hasn't.
>>>
>>> Hmm. So what you are saying is that CMD13 polling for HS200 doesn't
>>> work, as tuning is needed.
>>
>> I would assume that vendors integrate a working combination of eMMC and host
>> controller, so if polling is the only option, then we could assume it will work.
>>
>>>
>>> So, to me that means we need to fall-back to use the generic CMD6
>>> timeout instead (when HW busy detection isn't supported).
>>
>> Or, in the ignore_crc/retry_err_crc case, return -EILSEQ instead -ETIMEOUT,
>> and catch and ignore the error in the calling code. Then you get polling if
>> it works, otherwise getting CRC errors until timeout.
>>
>>>
>>>>
>>>> With the existing code I would just change the err check:
>>>>
>>>>
>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>> index 3268fcd3378d..c8862c58b60b 100644
>>>> --- a/drivers/mmc/core/mmc.c
>>>> +++ b/drivers/mmc/core/mmc.c
>>>> @@ -1387,6 +1387,13 @@ static int mmc_select_hs200(struct mmc_card *card)
>>>>
>>>> err = mmc_switch_status(card);
>>>> /*
>>>> + * For HS200, CRC errors are not a reliable way to know the
>>>> + * switch failed. If there really is a problem, we would expect
>>>> + * tuning will fail and the result ends up the same.
>>>> + */
>>>> + if (err == -EILSEQ)
>>>> + err = 0;
>>>> + /*
>>>
>>> I don't think ignoring CRC errors is reliable when verifying the CMD6
>>> status. My point is that we must not parse the status, in case of CRC
>>> errors as it can't be trusted.
>>
>> I agree, but mmc_switch_status() doesn't look at the response if there is an
>> error.
>
> Correct, it's only during CMD13 polling when CRC was ignored.
>
>>
>>>
>>> So, then we might as well just ignore validating the CMD6 status
>>> altogether, but instead always move on to the tuning and hope that it
>>> succeeds.
>>
>> That is a possibility, but it seemed to me that is was worth checking for
>> all the users where it does work. i.e if CMD13 does not give a CRC error
>> then validate the response, and if CMD13 does give a CRC error then ignore
>> the response and keep going anyway.
>
> Okay, let me think about this.
>
>>
>>>
>>> I think the CMD21 (tuning) should set the ILLEGAL COMMAND if HS200
>>> mode isn't enabled, so we could check that. Anyway, we should fail
>>> with the tuning if the earlier HS200 switch also failed. Don't you
>>> think?
>>
>> Yes CMD21 is an illegal command if the mode is not HS200. The card should
>> set ILLEGAL_COMMAND but also not respond i.e there will be a timeout error.
>> That could cause a long delay before tuning finally fails. The only way to
>> mitigate that would be to make ignoring the CRC error a host-specific option
>> (e.g. MMC_CAP_... flag). Arguably, if the switch fails, the mode is broken
>> and should not have been allowed in the first place.
>
> Not sure why there should be a long delay?
>
> If the CMD21 fails with a timeout, it's like any other command that
> fails with a timeout, right?
Ah, right. I was thinking of the data timeout, but yes the command timeout
will kick in first of course.
>
> So why should this one take longer to report for the host compared to others?
>
>>
>>>
>>>> * mmc_select_timing() assumes timing has not changed if
>>>> * it is a switch error.
>>>> */
>>>>
>>>>
>>>> Then to support polling:
>>>>
>>>>
>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>> index c8862c58b60b..66d8d57ae2fb 100644
>>>> --- a/drivers/mmc/core/mmc.c
>>>> +++ b/drivers/mmc/core/mmc.c
>>>> @@ -1352,6 +1352,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>>>> {
>>>> struct mmc_host *host = card->host;
>>>> unsigned int old_timing, old_signal_voltage;
>>>> + bool send_status;
>>>> int err = -EINVAL;
>>>> u8 val;
>>>>
>>>> @@ -1373,18 +1374,20 @@ static int mmc_select_hs200(struct mmc_card *card)
>>>> * switch to HS200 mode if bus width is set successfully.
>>>> */
>>>> err = mmc_select_bus_width(card);
>>>> - if (err > 0) {
>>>> - val = EXT_CSD_TIMING_HS200 |
>>>> - card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>>>> - err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>>> - EXT_CSD_HS_TIMING, val,
>>>> - card->ext_csd.generic_cmd6_time, 0,
>>>> - true, false, true);
>>>> - if (err)
>>>> - goto err;
>>>> - old_timing = host->ios.timing;
>>>> - mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>>>> + if (err <= 0)
>>>> + goto err;
>>>> +
>>>> + send_status = !(host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
>>>> + !host->ops->card_busy;
>>>> + old_timing = host->ios.timing;
>>>> +
>>>> + val = EXT_CSD_TIMING_HS200 |
>>>> + card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>>>> + err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, val,
>>>> + card->ext_csd.generic_cmd6_time,
>>>> + MMC_TIMING_MMC_HS200, true, send_status, true);
>>>>
>>>> + if (!err && !send_status) {
>>>> err = mmc_switch_status(card);
>>>> /*
>>>> * For HS200, CRC errors are not a reliable way to know the
>>>>
>>>>
>>>>
>>>> Thoughts?
>>>
>>> Well, I think the main problem is that if we have cards that returns
>>> CRC errors even after the HS200 switch, then we can't use polling, as
>>> we can't trust to parse the CMD6 status.
>>
>> As I wrote above, if there is no option but polling then we could expect it
>> to work. And if CMD13 does not give a CRC error then we can validate the
>> response, only ignoring it if there is a CRC error.
>>
>> I should point out that retrying CMD13 will clear the error bits in the
>> status so there is no point retrying when checking for the SWITCH_ERROR bit.
>> i.e. we need a version of __switch_send_status() that sets retries to zero.
>
> Are you really sure about this?
I don't think I have ever tested it. I was going on what the spec says:
"1) Error bit. Signals an error condition was detected by the device. These
bits are cleared as soon as the response (reporting the error) is sent out."
>
> I thought the switch status remained present in the device, but got
> cleared first when a new CMD6 command is being sent (or a reset of
> course), that would make more sense to me. :-)
> Anyway, this would mean that old CMD13 polling method was broken even
> in this sense.
>
> Okay, some more tests seems to be needed here. I will do some local
> hacks to explore this.
>
> Kind regards
> Uffe
>
^ permalink raw reply
* [PATCH 3/3] mmc: block: move packed command struct init
From: Linus Walleij @ 2016-11-18 12:36 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson; +Cc: Chunyan Zhang, Baolin Wang, Linus Walleij
In-Reply-To: <1479472576-2687-1-git-send-email-linus.walleij@linaro.org>
By moving the mmc_packed_init() and mmc_packed_clean() into the
only file in the kernel where they are used, we save two exported
functions and can staticize those to the block.c file.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/mmc/card/block.c | 43 +++++++++++++++++++++++++++++++++++++++++++
drivers/mmc/card/queue.c | 43 -------------------------------------------
drivers/mmc/card/queue.h | 3 ---
3 files changed, 43 insertions(+), 46 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index eec4986a0175..ea9de876a846 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1432,6 +1432,49 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
return MMC_BLK_SUCCESS;
}
+static int mmc_packed_init(struct mmc_queue *mq, struct mmc_card *card)
+{
+ struct mmc_queue_req *mqrq_cur = &mq->mqrq[0];
+ struct mmc_queue_req *mqrq_prev = &mq->mqrq[1];
+ int ret = 0;
+
+
+ mqrq_cur->packed = kzalloc(sizeof(struct mmc_packed), GFP_KERNEL);
+ if (!mqrq_cur->packed) {
+ pr_warn("%s: unable to allocate packed cmd for mqrq_cur\n",
+ mmc_card_name(card));
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ mqrq_prev->packed = kzalloc(sizeof(struct mmc_packed), GFP_KERNEL);
+ if (!mqrq_prev->packed) {
+ pr_warn("%s: unable to allocate packed cmd for mqrq_prev\n",
+ mmc_card_name(card));
+ kfree(mqrq_cur->packed);
+ mqrq_cur->packed = NULL;
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ INIT_LIST_HEAD(&mqrq_cur->packed->list);
+ INIT_LIST_HEAD(&mqrq_prev->packed->list);
+
+out:
+ return ret;
+}
+
+static void mmc_packed_clean(struct mmc_queue *mq)
+{
+ struct mmc_queue_req *mqrq_cur = &mq->mqrq[0];
+ struct mmc_queue_req *mqrq_prev = &mq->mqrq[1];
+
+ kfree(mqrq_cur->packed);
+ mqrq_cur->packed = NULL;
+ kfree(mqrq_prev->packed);
+ mqrq_prev->packed = NULL;
+}
+
static enum mmc_blk_status mmc_blk_packed_err_check(struct mmc_card *card,
struct mmc_async_req *areq)
{
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index 8037f73a109a..3f6a2463ab30 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -362,49 +362,6 @@ void mmc_cleanup_queue(struct mmc_queue *mq)
}
EXPORT_SYMBOL(mmc_cleanup_queue);
-int mmc_packed_init(struct mmc_queue *mq, struct mmc_card *card)
-{
- struct mmc_queue_req *mqrq_cur = &mq->mqrq[0];
- struct mmc_queue_req *mqrq_prev = &mq->mqrq[1];
- int ret = 0;
-
-
- mqrq_cur->packed = kzalloc(sizeof(struct mmc_packed), GFP_KERNEL);
- if (!mqrq_cur->packed) {
- pr_warn("%s: unable to allocate packed cmd for mqrq_cur\n",
- mmc_card_name(card));
- ret = -ENOMEM;
- goto out;
- }
-
- mqrq_prev->packed = kzalloc(sizeof(struct mmc_packed), GFP_KERNEL);
- if (!mqrq_prev->packed) {
- pr_warn("%s: unable to allocate packed cmd for mqrq_prev\n",
- mmc_card_name(card));
- kfree(mqrq_cur->packed);
- mqrq_cur->packed = NULL;
- ret = -ENOMEM;
- goto out;
- }
-
- INIT_LIST_HEAD(&mqrq_cur->packed->list);
- INIT_LIST_HEAD(&mqrq_prev->packed->list);
-
-out:
- return ret;
-}
-
-void mmc_packed_clean(struct mmc_queue *mq)
-{
- struct mmc_queue_req *mqrq_cur = &mq->mqrq[0];
- struct mmc_queue_req *mqrq_prev = &mq->mqrq[1];
-
- kfree(mqrq_cur->packed);
- mqrq_cur->packed = NULL;
- kfree(mqrq_prev->packed);
- mqrq_prev->packed = NULL;
-}
-
/**
* mmc_queue_suspend - suspend a MMC request queue
* @mq: MMC queue to suspend
diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
index 6ccadf88ef27..ae96eb223990 100644
--- a/drivers/mmc/card/queue.h
+++ b/drivers/mmc/card/queue.h
@@ -76,9 +76,6 @@ extern unsigned int mmc_queue_map_sg(struct mmc_queue *,
extern void mmc_queue_bounce_pre(struct mmc_queue_req *);
extern void mmc_queue_bounce_post(struct mmc_queue_req *);
-extern int mmc_packed_init(struct mmc_queue *, struct mmc_card *);
-extern void mmc_packed_clean(struct mmc_queue *);
-
extern int mmc_access_rpmb(struct mmc_queue *);
#endif
--
2.7.4
^ permalink raw reply related
* [PATCH 2/3] mmc: block: rename data to blkdata
From: Linus Walleij @ 2016-11-18 12:36 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson; +Cc: Chunyan Zhang, Baolin Wang, Linus Walleij
In-Reply-To: <1479472576-2687-1-git-send-email-linus.walleij@linaro.org>
The struct mmc_blk_request contains an opaque void *data that
is actually only used to store a pointer to a per-request
struct mmc_blk_data. This is confusing, so rename the member
to blkdata and forward-declare the block.c local struct.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/mmc/card/block.c | 20 ++++++++++----------
drivers/mmc/card/queue.h | 3 ++-
2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 56209d32ca5f..eec4986a0175 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1153,7 +1153,7 @@ static inline void mmc_blk_reset_success(struct mmc_blk_data *md, int type)
int mmc_access_rpmb(struct mmc_queue *mq)
{
- struct mmc_blk_data *md = mq->data;
+ struct mmc_blk_data *md = mq->blkdata;
/*
* If this is a RPMB partition access, return ture
*/
@@ -1165,7 +1165,7 @@ int mmc_access_rpmb(struct mmc_queue *mq)
static int mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
{
- struct mmc_blk_data *md = mq->data;
+ struct mmc_blk_data *md = mq->blkdata;
struct mmc_card *card = md->queue.card;
unsigned int from, nr, arg;
int err = 0, type = MMC_BLK_DISCARD;
@@ -1209,7 +1209,7 @@ static int mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
static int mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq,
struct request *req)
{
- struct mmc_blk_data *md = mq->data;
+ struct mmc_blk_data *md = mq->blkdata;
struct mmc_card *card = md->queue.card;
unsigned int from, nr, arg;
int err = 0, type = MMC_BLK_SECDISCARD;
@@ -1275,7 +1275,7 @@ static int mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq,
static int mmc_blk_issue_flush(struct mmc_queue *mq, struct request *req)
{
- struct mmc_blk_data *md = mq->data;
+ struct mmc_blk_data *md = mq->blkdata;
struct mmc_card *card = md->queue.card;
int ret = 0;
@@ -1489,7 +1489,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
u32 readcmd, writecmd;
struct mmc_blk_request *brq = &mqrq->brq;
struct request *req = mqrq->req;
- struct mmc_blk_data *md = mq->data;
+ struct mmc_blk_data *md = mq->blkdata;
bool do_data_tag;
/*
@@ -1662,7 +1662,7 @@ static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request *req)
struct request_queue *q = mq->queue;
struct mmc_card *card = mq->card;
struct request *cur = req, *next = NULL;
- struct mmc_blk_data *md = mq->data;
+ struct mmc_blk_data *md = mq->blkdata;
struct mmc_queue_req *mqrq = mq->mqrq_cur;
bool en_rel_wr = card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN;
unsigned int req_sectors = 0, phys_segments = 0;
@@ -1783,7 +1783,7 @@ static void mmc_blk_packed_hdr_wrq_prep(struct mmc_queue_req *mqrq,
struct mmc_blk_request *brq = &mqrq->brq;
struct request *req = mqrq->req;
struct request *prq;
- struct mmc_blk_data *md = mq->data;
+ struct mmc_blk_data *md = mq->blkdata;
struct mmc_packed *packed = mqrq->packed;
bool do_rel_wr, do_data_tag;
u32 *packed_cmd_hdr;
@@ -1954,7 +1954,7 @@ static void mmc_blk_revert_packed_req(struct mmc_queue *mq,
static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
{
- struct mmc_blk_data *md = mq->data;
+ struct mmc_blk_data *md = mq->blkdata;
struct mmc_card *card = md->queue.card;
struct mmc_blk_request *brq = &mq->mqrq_cur->brq;
int ret = 1, disable_multi = 0, retry = 0, type, retune_retry_done = 0;
@@ -2147,7 +2147,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
{
int ret;
- struct mmc_blk_data *md = mq->data;
+ struct mmc_blk_data *md = mq->blkdata;
struct mmc_card *card = md->queue.card;
struct mmc_host *host = card->host;
unsigned long flags;
@@ -2265,7 +2265,7 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
if (ret)
goto err_putdisk;
- md->queue.data = md;
+ md->queue.blkdata = md;
md->disk->major = MMC_BLOCK_MAJOR;
md->disk->first_minor = devidx * perdev_minors;
diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
index 3c15a75bae86..6ccadf88ef27 100644
--- a/drivers/mmc/card/queue.h
+++ b/drivers/mmc/card/queue.h
@@ -11,6 +11,7 @@ static inline bool mmc_req_is_special(struct request *req)
struct request;
struct task_struct;
+struct mmc_blk_data;
struct mmc_blk_request {
struct mmc_request mrq;
@@ -57,7 +58,7 @@ struct mmc_queue {
unsigned int flags;
#define MMC_QUEUE_SUSPENDED (1 << 0)
#define MMC_QUEUE_NEW_REQUEST (1 << 1)
- void *data;
+ struct mmc_blk_data *blkdata;
struct request_queue *queue;
struct mmc_queue_req mqrq[2];
struct mmc_queue_req *mqrq_cur;
--
2.7.4
^ permalink raw reply related
* [PATCH 1/3] mmc: block: use mmc_req_is_special()
From: Linus Walleij @ 2016-11-18 12:36 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson; +Cc: Chunyan Zhang, Baolin Wang, Linus Walleij
In-Reply-To: <1479472576-2687-1-git-send-email-linus.walleij@linaro.org>
Instead of open coding the check for the same thing that
the helper checks: use the helper.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/mmc/card/block.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 4c397b9684e5..56209d32ca5f 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1735,9 +1735,7 @@ static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request *req)
!IS_ALIGNED(blk_rq_sectors(next), 8))
break;
- if (req_op(next) == REQ_OP_DISCARD ||
- req_op(next) == REQ_OP_SECURE_ERASE ||
- req_op(next) == REQ_OP_FLUSH)
+ if (mmc_req_is_special(next))
break;
if (rq_data_dir(cur) != rq_data_dir(next))
--
2.7.4
^ permalink raw reply related
* [PATCH 0/3] More MMC block core cleanup
From: Linus Walleij @ 2016-11-18 12:36 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson; +Cc: Chunyan Zhang, Baolin Wang, Linus Walleij
The more I dig the more I find. These are pretty straight-forward
and should be possible to merge right off.
Linus Walleij (3):
mmc: block: use mmc_req_is_special()
mmc: block: rename data to blkdata
mmc: block: move packed command struct init
drivers/mmc/card/block.c | 67 ++++++++++++++++++++++++++++++++++++++----------
drivers/mmc/card/queue.c | 43 -------------------------------
drivers/mmc/card/queue.h | 6 ++---
3 files changed, 56 insertions(+), 60 deletions(-)
--
2.7.4
^ permalink raw reply
* Re: [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode
From: Ulf Hansson @ 2016-11-18 12:20 UTC (permalink / raw)
To: Adrian Hunter
Cc: linux-mmc, Jaehoon Chung, Linus Walleij, Chaotian Jing,
Stephen Boyd, Michael Walle, Yong Mao, Shawn Lin
In-Reply-To: <2a2f1f8c-871c-164e-d4fe-abc0658df90f@intel.com>
On 18 November 2016 at 10:30, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 17/11/16 17:02, Ulf Hansson wrote:
>> On 17 November 2016 at 11:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 16/11/16 12:51, Ulf Hansson wrote:
>>>> In cases when the mmc host doesn't support HW busy detection, polling for
>>>> busy by using CMD13 is beneficial. The reasons have already been explained
>>>> in earlier change logs.
>>>>
>>>> To allow polling with CMD13, let's provide MMC_TIMING_MMC_HS200 as the
>>>> timing parameter to __mmc_switch(), which makes sure the mmc host and the
>>>> mmc card operates at the same bus timing during the polling.
>>>
>>> I have reports of cases where CMD13 always gives CRC errors after switch
>>> to HS200. Currently we are assuming the low frequency should mean that
>>> won't happen, but it does in some cases. That is not entirely surprising
>>> since HS200 needs tuning at the final operating frequency.
>>
>>>From a logical point of view and if tuning is needed also for the CMD
>> line, this somehow make sense.
>>
>> However, this is *not* how the JEDEC spec describes the HS200 switch
>> sequence. It is clearly stated that the host should validate the CM6
>> status via sending a CMD13 command, *before* performing tuning.
>
> I agree, it seems not to be following spec.
>
>>
>> Could it be that the observations about the CRC errors, is related to
>> a controller/driver issue and not a card issue?
>
> I don't know what causes the problem (and I have a sneaking suspicion that
> if vendors configured / designed their boards correctly, it wouldn't
> happen). However, while some cards have better signal characteristics than
> others, tuning is a host controller issue - the card doesn't care.
>
>>
>>>
>>> What I would like to do for hosts that support busy waiting or DAT0 polling
>>> (i.e. MMC_CAP_WAIT_WHILE_BUSY or host->ops->card_busy), is to ignore CRC
>>> errors from the CMD13 that checks the switch status. The main reason for
>>> doing that is that we really expect the switch to succeed and, given HS200
>>> tuning requirement, the CRC error is not a reliable means of determining
>>> that it hasn't.
>>
>> Hmm. So what you are saying is that CMD13 polling for HS200 doesn't
>> work, as tuning is needed.
>
> I would assume that vendors integrate a working combination of eMMC and host
> controller, so if polling is the only option, then we could assume it will work.
>
>>
>> So, to me that means we need to fall-back to use the generic CMD6
>> timeout instead (when HW busy detection isn't supported).
>
> Or, in the ignore_crc/retry_err_crc case, return -EILSEQ instead -ETIMEOUT,
> and catch and ignore the error in the calling code. Then you get polling if
> it works, otherwise getting CRC errors until timeout.
>
>>
>>>
>>> With the existing code I would just change the err check:
>>>
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index 3268fcd3378d..c8862c58b60b 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1387,6 +1387,13 @@ static int mmc_select_hs200(struct mmc_card *card)
>>>
>>> err = mmc_switch_status(card);
>>> /*
>>> + * For HS200, CRC errors are not a reliable way to know the
>>> + * switch failed. If there really is a problem, we would expect
>>> + * tuning will fail and the result ends up the same.
>>> + */
>>> + if (err == -EILSEQ)
>>> + err = 0;
>>> + /*
>>
>> I don't think ignoring CRC errors is reliable when verifying the CMD6
>> status. My point is that we must not parse the status, in case of CRC
>> errors as it can't be trusted.
>
> I agree, but mmc_switch_status() doesn't look at the response if there is an
> error.
Correct, it's only during CMD13 polling when CRC was ignored.
>
>>
>> So, then we might as well just ignore validating the CMD6 status
>> altogether, but instead always move on to the tuning and hope that it
>> succeeds.
>
> That is a possibility, but it seemed to me that is was worth checking for
> all the users where it does work. i.e if CMD13 does not give a CRC error
> then validate the response, and if CMD13 does give a CRC error then ignore
> the response and keep going anyway.
Okay, let me think about this.
>
>>
>> I think the CMD21 (tuning) should set the ILLEGAL COMMAND if HS200
>> mode isn't enabled, so we could check that. Anyway, we should fail
>> with the tuning if the earlier HS200 switch also failed. Don't you
>> think?
>
> Yes CMD21 is an illegal command if the mode is not HS200. The card should
> set ILLEGAL_COMMAND but also not respond i.e there will be a timeout error.
> That could cause a long delay before tuning finally fails. The only way to
> mitigate that would be to make ignoring the CRC error a host-specific option
> (e.g. MMC_CAP_... flag). Arguably, if the switch fails, the mode is broken
> and should not have been allowed in the first place.
Not sure why there should be a long delay?
If the CMD21 fails with a timeout, it's like any other command that
fails with a timeout, right?
So why should this one take longer to report for the host compared to others?
>
>>
>>> * mmc_select_timing() assumes timing has not changed if
>>> * it is a switch error.
>>> */
>>>
>>>
>>> Then to support polling:
>>>
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index c8862c58b60b..66d8d57ae2fb 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1352,6 +1352,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>>> {
>>> struct mmc_host *host = card->host;
>>> unsigned int old_timing, old_signal_voltage;
>>> + bool send_status;
>>> int err = -EINVAL;
>>> u8 val;
>>>
>>> @@ -1373,18 +1374,20 @@ static int mmc_select_hs200(struct mmc_card *card)
>>> * switch to HS200 mode if bus width is set successfully.
>>> */
>>> err = mmc_select_bus_width(card);
>>> - if (err > 0) {
>>> - val = EXT_CSD_TIMING_HS200 |
>>> - card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>>> - err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>> - EXT_CSD_HS_TIMING, val,
>>> - card->ext_csd.generic_cmd6_time, 0,
>>> - true, false, true);
>>> - if (err)
>>> - goto err;
>>> - old_timing = host->ios.timing;
>>> - mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>>> + if (err <= 0)
>>> + goto err;
>>> +
>>> + send_status = !(host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
>>> + !host->ops->card_busy;
>>> + old_timing = host->ios.timing;
>>> +
>>> + val = EXT_CSD_TIMING_HS200 |
>>> + card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>>> + err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, val,
>>> + card->ext_csd.generic_cmd6_time,
>>> + MMC_TIMING_MMC_HS200, true, send_status, true);
>>>
>>> + if (!err && !send_status) {
>>> err = mmc_switch_status(card);
>>> /*
>>> * For HS200, CRC errors are not a reliable way to know the
>>>
>>>
>>>
>>> Thoughts?
>>
>> Well, I think the main problem is that if we have cards that returns
>> CRC errors even after the HS200 switch, then we can't use polling, as
>> we can't trust to parse the CMD6 status.
>
> As I wrote above, if there is no option but polling then we could expect it
> to work. And if CMD13 does not give a CRC error then we can validate the
> response, only ignoring it if there is a CRC error.
>
> I should point out that retrying CMD13 will clear the error bits in the
> status so there is no point retrying when checking for the SWITCH_ERROR bit.
> i.e. we need a version of __switch_send_status() that sets retries to zero.
Are you really sure about this?
I thought the switch status remained present in the device, but got
cleared first when a new CMD6 command is being sent (or a reset of
course), that would make more sense to me. :-)
Anyway, this would mean that old CMD13 polling method was broken even
in this sense.
Okay, some more tests seems to be needed here. I will do some local
hacks to explore this.
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 8/9] mmc: core: Allow CMD13 polling when switch to HS400 mode
From: Adrian Hunter @ 2016-11-18 12:02 UTC (permalink / raw)
To: Ulf Hansson, linux-mmc
Cc: Jaehoon Chung, Linus Walleij, Chaotian Jing, Stephen Boyd,
Michael Walle, Yong Mao, Shawn Lin, Ziyuan Xu
In-Reply-To: <1479293481-20186-9-git-send-email-ulf.hansson@linaro.org>
On 16/11/16 12:51, Ulf Hansson wrote:
> In cases when the mmc host doesn't support HW busy detection, polling for
> busy by using CMD13 is beneficial. The reasons have already been explained
> in earlier change logs.
>
> Moreover, when polling with CMD13 during bus timing changes, we should
> retry instead of fail when we get CRC errors.
>
> Switching from HS200 to HS400 and reverse includes several steps, where
> each step changes the bus speed timing. Let's improve the behaviour during
> these sequences, by allowing CMD13 polling for each of the step. Let's also
> make sure the CMD13 polling becomes retried, while receiving a CRC error.
I don't know we need to try to get polling for HS400. The support of HS400
suggests a relatively sophisticated host controller which really should
support busy waiting as well.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/mmc/core/mmc.c | 87 +++++++++++++++-----------------------------------
> 1 file changed, 26 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 0b26383..24b9e72 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1094,8 +1094,7 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
> static int mmc_select_hs400(struct mmc_card *card)
> {
> struct mmc_host *host = card->host;
> - unsigned int max_dtr;
> - int err = 0;
> + int err;
> u8 val;
>
> /*
> @@ -1105,34 +1104,26 @@ static int mmc_select_hs400(struct mmc_card *card)
> host->ios.bus_width == MMC_BUS_WIDTH_8))
> return 0;
>
> - /* Switch card to HS mode */
> - val = EXT_CSD_TIMING_HS;
> - err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> - EXT_CSD_HS_TIMING, val,
> - card->ext_csd.generic_cmd6_time, 0,
> - true, false, true);
> + /*
> + * Switch card to HS mode.
> + * First reduce to the HS frequency as CMD13 are sent to poll for busy
> + * and/or to validate the switch status.
> + */
> + mmc_set_clock(host, card->ext_csd.hs_max_dtr);
That was moved by commit 649c6059d2371fef886a8f967e21416204723d63
("mmc: mmc: Fix HS switch failure in mmc_select_hs400()") i.e. if you put it
back I would expect the gru board problem to reappear.
> + err = mmc_select_hs(card);
> if (err) {
> pr_err("%s: switch to high-speed from hs200 failed, err:%d\n",
> mmc_hostname(host), err);
> return err;
> }
>
> - /* Set host controller to HS timing */
> - mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
> -
> - /* Reduce frequency to HS frequency */
> - max_dtr = card->ext_csd.hs_max_dtr;
> - mmc_set_clock(host, max_dtr);
> -
> - err = mmc_switch_status(card);
> - if (err)
> - goto out_err;
> -
> - /* Switch card to DDR */
> - err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> - EXT_CSD_BUS_WIDTH,
> - EXT_CSD_DDR_BUS_WIDTH_8,
> - card->ext_csd.generic_cmd6_time);
> + /* Switch card to HS DDR */
> + err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> + EXT_CSD_BUS_WIDTH,
> + EXT_CSD_DDR_BUS_WIDTH_8,
> + card->ext_csd.generic_cmd6_time,
> + MMC_TIMING_MMC_DDR52,
> + true, true, true);
> if (err) {
> pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
> mmc_hostname(host), err);
> @@ -1144,28 +1135,17 @@ static int mmc_select_hs400(struct mmc_card *card)
> card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> EXT_CSD_HS_TIMING, val,
> - card->ext_csd.generic_cmd6_time, 0,
> - true, false, true);
> + card->ext_csd.generic_cmd6_time,
> + MMC_TIMING_MMC_HS400,
> + true, true, true);
This is not exactly right. Tuning has been done at the HS400 operating
frequency. That is why we set the bus speed before sending any more
commands. i.e. mmc_switch_status(card) is below mmc_set_bus_speed(card)
> if (err) {
> pr_err("%s: switch to hs400 failed, err:%d\n",
> mmc_hostname(host), err);
> return err;
> }
>
> - /* Set host controller to HS400 timing and frequency */
> - mmc_set_timing(host, MMC_TIMING_MMC_HS400);
> mmc_set_bus_speed(card);
> -
> - err = mmc_switch_status(card);
> - if (err)
> - goto out_err;
> -
> return 0;
> -
> -out_err:
> - pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
> - __func__, err);
> - return err;
> }
>
> int mmc_hs200_to_hs400(struct mmc_card *card)
> @@ -1187,27 +1167,17 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
> /* Switch HS400 to HS DDR */
> val = EXT_CSD_TIMING_HS;
> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
> - val, card->ext_csd.generic_cmd6_time, 0,
> - true, false, true);
> - if (err)
> - goto out_err;
> -
> - mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
> -
> - err = mmc_switch_status(card);
> + val, card->ext_csd.generic_cmd6_time,
> + MMC_TIMING_MMC_DDR52,
> + true, true, true);
> if (err)
> goto out_err;
>
> /* Switch HS DDR to HS */
> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
> EXT_CSD_BUS_WIDTH_8, card->ext_csd.generic_cmd6_time,
> - 0, true, false, true);
> - if (err)
> - goto out_err;
> -
> - mmc_set_timing(host, MMC_TIMING_MMC_HS);
> -
> - err = mmc_switch_status(card);
> + MMC_TIMING_MMC_HS,
> + true, true, true);
> if (err)
> goto out_err;
>
> @@ -1215,14 +1185,9 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
> val = EXT_CSD_TIMING_HS200 |
> card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
> - val, card->ext_csd.generic_cmd6_time, 0,
> - true, false, true);
> - if (err)
> - goto out_err;
> -
> - mmc_set_timing(host, MMC_TIMING_MMC_HS200);
> -
> - err = mmc_switch_status(card);
> + val, card->ext_csd.generic_cmd6_time,
> + MMC_TIMING_MMC_HS200,
> + true, true, true);
> if (err)
> goto out_err;
>
>
^ permalink raw reply
* Re: [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode
From: Ulf Hansson @ 2016-11-18 11:45 UTC (permalink / raw)
To: Shawn Lin
Cc: linux-mmc, Jaehoon Chung, Adrian Hunter, Linus Walleij,
Chaotian Jing, Stephen Boyd, Michael Walle, Yong Mao
In-Reply-To: <847df7a0-6a00-a3ac-01f5-bdd1bffcc185@rock-chips.com>
Hi Shawn,
On 18 November 2016 at 09:05, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> Hi Ulf,
>
>
> 在 2016/11/16 18:51, Ulf Hansson 写道:
>>
>> In cases when the mmc host doesn't support HW busy detection, polling for
>> busy by using CMD13 is beneficial. The reasons have already been explained
>> in earlier change logs.
>>
>> To allow polling with CMD13, let's provide MMC_TIMING_MMC_HS200 as the
>> timing parameter to __mmc_switch(), which makes sure the mmc host and the
>> mmc card operates at the same bus timing during the polling.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>> drivers/mmc/core/mmc.c | 21 +++++----------------
>> 1 file changed, 5 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 3268fcd..0b26383 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1351,7 +1351,7 @@ static void mmc_select_driver_type(struct mmc_card
>> *card)
>> static int mmc_select_hs200(struct mmc_card *card)
>> {
>> struct mmc_host *host = card->host;
>> - unsigned int old_timing, old_signal_voltage;
>> + unsigned int old_signal_voltage;
>> int err = -EINVAL;
>> u8 val;
>>
>> @@ -1378,22 +1378,11 @@ static int mmc_select_hs200(struct mmc_card *card)
>> card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> EXT_CSD_HS_TIMING, val,
>> - card->ext_csd.generic_cmd6_time, 0,
>> - true, false, true);
>> - if (err)
>> - goto err;
>> - old_timing = host->ios.timing;
>> - mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>> -
>> - err = mmc_switch_status(card);
>> - /*
>> - * mmc_select_timing() assumes timing has not changed if
>> - * it is a switch error.
>> - */
>> - if (err == -EBADMSG)
>> - mmc_set_timing(host, old_timing);
>> + card->ext_csd.generic_cmd6_time,
>> + MMC_TIMING_MMC_HS200,
>> + true, true, true);
>
>
> I was finding a failure from the test last night after applying these
> patches and using HS200 only.
>
> It seems like the controller(sdhci-of-arasan,5.1) continuously generate
> response timeout for checking CMD13.. Per TRM, response timeout could
> means the device didn't ack the CMD13 or the controller was failing to
> latch the response..
Did you make any changes to the sdchi driver while testing this?
What I am wondering is whether you tested this with an implemented
->card_busy() host ops or not, as sdhci has a default implementation
of it.
BTW, some sdhci hosts have lately shown problem [1] with sdhci's
default ->card_busy() host ops.
>
> The eMMC part number is KLMBG2JENB-B041. I will use two boards to check
> if it was indeed related to these patches.
>
>> }
>> -err:
>> +
>> if (err) {
>> /* fall back to the old signal voltage, if fails report
>> error */
>> if (__mmc_set_signal_voltage(host, old_signal_voltage))
>>
>
>
> --
> Best Regards
> Shawn Lin
>
Thanks for helping out with testing etc, I really appreciate it!
Kind regards
Uffe
[1]
https://patchwork.kernel.org/patch/9429299/
^ permalink raw reply
* Re: [PATCHv3 0/9] mmc: dw_mmc: clean the codes for dwmmc controller
From: Jaehoon Chung @ 2016-11-18 11:17 UTC (permalink / raw)
To: linux-mmc; +Cc: ulf.hansson, shawn.lin, adrian.hunter
In-Reply-To: <20161117074041.8641-1-jh80.chung@samsung.com>
Hi,
On 11/17/2016 04:40 PM, Jaehoon Chung wrote:
> This patchset is modified the some minor fixing and cleaning codes.
>
> * Major changes
> - Remove the unnecessary codes and use_stop_abort() by default.
> - Remove the obsoleted property "supports-highspeed"
> - Deprecated the "clock-freq-min-max" property. Instead, use "max-frequency"
> - Minimum clock value is set to 100K by default.
I have applied this patchset on my dwmmc repository.
If there are some issues, let me know, plz.
Best Regards,
Jaehoon Chung
>
> Change in v3:
> - Use the dwmmc private cookie enum.
> - Add the reviewed-by and acked-by tags.
>
> Change in v2:
> - Applied patches relevant to dt files
> - Use "deprecated" instead of removing about "clock-freq-min-max"
> - Added Heiko's Tested-by tag
>
> Jaehoon Chung (9):
> mmc: dw_mmc: display the real register value on debugfs
> mmc: dw_mmc: fix the debug message for checking card's present
> mmc: dw_mmc: change the DW_MCI_FREQ_MIN from 400K to 100K
> mmc: dw_mmc: use the hold register when send stop command
> mmc: dw_mmc: call the dw_mci_prep_stop_abort() by default
> mmc: dw_mmc: use the cookie's enum values for post/pre_req()
> mmc: dw_mmc: remove the unnecessary mmc_data structure
> mmc: dw_mmc: The "clock-freq-min-max" property was deprecated
> Documentation: synopsys-dw-mshc: remove the unused properties
>
> .../devicetree/bindings/mmc/synopsys-dw-mshc.txt | 8 +-
> drivers/mmc/host/dw_mmc.c | 88 +++++++++++-----------
> include/linux/mmc/dw_mmc.h | 6 ++
> 3 files changed, 50 insertions(+), 52 deletions(-)
>
^ permalink raw reply
* [PATCHv2] mmc: dw_mmc: fix the error handling for dma operation
From: Jaehoon Chung @ 2016-11-18 11:16 UTC (permalink / raw)
To: linux-mmc; +Cc: ulf.hansson, shawn.lin, Jaehoon Chung
When dma->start is failed,then it has to fall back to PIO mode
for current transfer.
But Host controller was already set to bits relevant to DMA operation.
If needs to use the PIO mode, Host controller has to stop the DMA
operation. (It's more stable than now.)
When it occurred error, it's not running any request.
Fixes: 3fc7eaef44db ("mmc: dw_mmc: Add external dma interface support")
Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changelog on V2
- Change the Fiexes Commit Id
drivers/mmc/host/dw_mmc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 50a674b..df478ae 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1058,6 +1058,7 @@ static int dw_mci_submit_data_dma(struct dw_mci *host, struct mmc_data *data)
spin_unlock_irqrestore(&host->irq_lock, irqflags);
if (host->dma_ops->start(host, sg_len)) {
+ host->dma_ops->stop(host);
/* We can't do DMA, try PIO for this one */
dev_dbg(host->dev,
"%s: fall back to PIO mode for current transfer\n",
--
2.10.1
^ permalink raw reply related
* Re: arasan,sdhci.txt "compatibility" DT binding
From: Rameshwar Sahu @ 2016-11-18 10:49 UTC (permalink / raw)
To: Mason
Cc: Linux ARM, linux-mmc, Soren Brinkmann, Michal Simek, Shawn Lin,
Michal Simek, Anton Vorontsov, Xiaobo Xie, Suman Tripathi,
Linus Walleij, Maxime Ripard, Arnd Bergmann, Rob Herring,
Adrian Hunter, Zach Brown, Ulf Hansson, Douglas Anderson,
Heiko Stuebner, Jisheng Zhang, Suneel Garapati <sunee>
In-Reply-To: <582ED9E0.5090506@free.fr>
Hi Mason,
Suman/APM added "arasan,sdhci-4.9a" in 308f3f8d8112
@Suman, @Rameshwar: what specific IP block does your SoC embed?
What does 4.9a refer to? The documentation revision number?
We have Arasan SD3.0/ SDIO3.0/ eMMC4.41 AHB Host Controller IP
embedded in our SoC and 4.9a is documentation revision number which
was given by arasan.
FYI this documentation date was May, 2012.
Thanks,
Ram
On Fri, Nov 18, 2016 at 4:07 PM, Mason <slash.tmp@free.fr> wrote:
> On 03/02/2016 16:33, Mason wrote:
>> On 03/02/2016 16:21, Sören Brinkmann wrote:
>>> On Wed, 2016-02-03 at 10:58:24 +0100, Michal Simek wrote:
>>>> On 3.2.2016 09:31, Mason wrote:
>>>>> On 03/02/2016 08:20, Michal Simek wrote:
>>>>>
>>>>>> On 3.2.2016 03:33, Shawn Lin wrote:
>>>>>>> + Michal, Sören Brinkmann
>>>>>>>
>>>>>>> On 2016/2/2 17:49, Mason wrote:
>>>>>>>> Hello everyone,
>>>>>>>>
>>>>>>>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt states:
>>>>>>>>
>>>>>>>> Required Properties:
>>>>>>>> - compatible: Compatibility string. Must be 'arasan,sdhci-8.9a' or
>>>>>>>> 'arasan,sdhci-4.9a' or 'arasan,sdhci-5.1'
>>>>>>>>
>>>>>>>> What do 8.9a, 4.9a, and 5.1 refer to?
>>>>>>>>
>>>>>>>
>>>>>>> Good question.
>>>>>>>
>>>>>>> Michal told me that 8.9a and 4.9a came from Xilinx
>>>>>>> databook which define their available arasan controller to be version
>>>>>>> 4.9a and 8.9a.
>>>>>>
>>>>>> Our version is coming from here.
>>>>>> http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
>>>>>> page 28
>>>>>>
>>>>>> Datasheets from 2012 and 2010 doesn't look too recent and I expect that
>>>>>> Arasan did a lot of work from that time that's why I am not surprised
>>>>>> that you are not able to see that versions.
>>>>>
>>>>> Hello Michal,
>>>>>
>>>>> I'm even more confused.
>>>>>
>>>>> Arasan's 2010-02-19 data sheet is titled
>>>>> SD3.0 / SDIO3.0 / eMMC4.4 AHB Host Controller
>>>>>
>>>>> Page 28 of the Xilinx data sheet mentions
>>>>> SD2.0 / SDIO2.0 / MMC3.31 AHB Host Controller
>>>>> Version 8.9A_apr02nd_2010
>>>>>
>>>>> It does not make sense that Arasan would support SD3.0 in February,
>>>>> then go back to SD 2.0 in April.
>>>>>
>>>>> I do note eMMC4.4 vs MMC3.31 => perhaps these are two *different*
>>>>> IP blocks?
>>>>>
>>>>> Do your data sheets come with revision history?
>>>>
>>>> I don't have datasheet for this IP in my hand that's why I can't check it.
>>>> But I can't see any problem with it. Our zynq SoC supports SD2.0 and it
>>>> was requirement at that time. Bugs can happen. Arasan fixed it and
>>>> create new version.
>>>> At the same time can have 3.0 versions but vendor is just decide not to
>>>> use it for whatever reason.
>>>>
>>>> That's why timing of features and versions can upgrade any time and
>>>> unfortunately bugs happen.
>>>
>>> We have several Arasan data sheets here. The document names are:
>>> "SD2.0 / SDIO2.0 / MMC3.31 AHB Userguide" and the revisions are 9.2a,
>>> 4.4a and 5.4a. I have the feeling that the document revisions have
>>> been mistaken as the IP revision. I cannot find any other indicator
>>> for the IP revision though. Does the IP have a way to discover its
>>> revision?
>>
>> To summarize, it looks to me like
>> 4.9a and 8.9a are documentation revision numbers for this IP:
>> "SD2.0 / SDIO2.0 / MMC3.31 AHB Host Controller"
>>
>> and 5.1 seems to be the eMMC standard, so one of these IP:
>> http://arasan.com/products/emmc51/sd3-emmc-5-1-host/
>> http://arasan.com/products/emmc51/emmc-5-1-sd-4-1-host/
>>
>> Whereas my board is using still another IP:
>> "SD3.0 / SDIO3.0 / eMMC4.4 AHB Host Controller"
>>
>>
>> If that is correct, then I should be able to use the 8.9a
>> compatible string, and perhaps my hardware will work in
>> slightly degraded performance mode, from not using the
>> newest protocol gizmos available.
>
> Resurrecting this thread after a chat with Michal on IRC.
>
> The driver now supports a few more compatible strings.
> Tracing the history...
>
> Soren/Xilinx defined "arasan,sdhci-8.9a" in e3ec3a3d11ad
> They have the "SD2.0/SDIO2.0/MMC3.31 AHB Host Controller" version
> (8.9a might be a document revision number, dated 2010-04-02)
>
> Suman/APM added "arasan,sdhci-4.9a" in 308f3f8d8112
> @Suman, @Rameshwar: what specific IP block does your SoC embed?
> What does 4.9a refer to? The documentation revision number?
>
> Shawn/Rockchip added "arasan,sdhci-5.1" in da795ec26e25
> This seems to be for an *actual* eMMC 5.1 version
> (instead of a documentation version) as mentioned in
> the commit message.
>
> Douglas added "rockchip,rk3399-sdhci-5.1"
> and made several other improvements.
>
> I have reached out to Arasan support. Hopefully they can also
> help clear up the confusion, assuming they care about the
> Linux driver.
>
> Regards.
>
^ permalink raw reply
* Re: arasan,sdhci.txt "compatibility" DT binding
From: Mason @ 2016-11-18 10:37 UTC (permalink / raw)
To: Linux ARM, linux-mmc
Cc: Soren Brinkmann, Michal Simek, Shawn Lin, Michal Simek,
Anton Vorontsov, Xiaobo Xie, Suman Tripathi, Linus Walleij,
Maxime Ripard, Arnd Bergmann, Rob Herring, Adrian Hunter,
Zach Brown, Ulf Hansson, Douglas Anderson, Heiko Stuebner,
Jisheng Zhang, Rameshwar Prasad Sahu, Suneel Garapati,
Russell King
In-Reply-To: <56B21DDB.6050708@free.fr>
On 03/02/2016 16:33, Mason wrote:
> On 03/02/2016 16:21, Sören Brinkmann wrote:
>> On Wed, 2016-02-03 at 10:58:24 +0100, Michal Simek wrote:
>>> On 3.2.2016 09:31, Mason wrote:
>>>> On 03/02/2016 08:20, Michal Simek wrote:
>>>>
>>>>> On 3.2.2016 03:33, Shawn Lin wrote:
>>>>>> + Michal, Sören Brinkmann
>>>>>>
>>>>>> On 2016/2/2 17:49, Mason wrote:
>>>>>>> Hello everyone,
>>>>>>>
>>>>>>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt states:
>>>>>>>
>>>>>>> Required Properties:
>>>>>>> - compatible: Compatibility string. Must be 'arasan,sdhci-8.9a' or
>>>>>>> 'arasan,sdhci-4.9a' or 'arasan,sdhci-5.1'
>>>>>>>
>>>>>>> What do 8.9a, 4.9a, and 5.1 refer to?
>>>>>>>
>>>>>>
>>>>>> Good question.
>>>>>>
>>>>>> Michal told me that 8.9a and 4.9a came from Xilinx
>>>>>> databook which define their available arasan controller to be version
>>>>>> 4.9a and 8.9a.
>>>>>
>>>>> Our version is coming from here.
>>>>> http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
>>>>> page 28
>>>>>
>>>>> Datasheets from 2012 and 2010 doesn't look too recent and I expect that
>>>>> Arasan did a lot of work from that time that's why I am not surprised
>>>>> that you are not able to see that versions.
>>>>
>>>> Hello Michal,
>>>>
>>>> I'm even more confused.
>>>>
>>>> Arasan's 2010-02-19 data sheet is titled
>>>> SD3.0 / SDIO3.0 / eMMC4.4 AHB Host Controller
>>>>
>>>> Page 28 of the Xilinx data sheet mentions
>>>> SD2.0 / SDIO2.0 / MMC3.31 AHB Host Controller
>>>> Version 8.9A_apr02nd_2010
>>>>
>>>> It does not make sense that Arasan would support SD3.0 in February,
>>>> then go back to SD 2.0 in April.
>>>>
>>>> I do note eMMC4.4 vs MMC3.31 => perhaps these are two *different*
>>>> IP blocks?
>>>>
>>>> Do your data sheets come with revision history?
>>>
>>> I don't have datasheet for this IP in my hand that's why I can't check it.
>>> But I can't see any problem with it. Our zynq SoC supports SD2.0 and it
>>> was requirement at that time. Bugs can happen. Arasan fixed it and
>>> create new version.
>>> At the same time can have 3.0 versions but vendor is just decide not to
>>> use it for whatever reason.
>>>
>>> That's why timing of features and versions can upgrade any time and
>>> unfortunately bugs happen.
>>
>> We have several Arasan data sheets here. The document names are:
>> "SD2.0 / SDIO2.0 / MMC3.31 AHB Userguide" and the revisions are 9.2a,
>> 4.4a and 5.4a. I have the feeling that the document revisions have
>> been mistaken as the IP revision. I cannot find any other indicator
>> for the IP revision though. Does the IP have a way to discover its
>> revision?
>
> To summarize, it looks to me like
> 4.9a and 8.9a are documentation revision numbers for this IP:
> "SD2.0 / SDIO2.0 / MMC3.31 AHB Host Controller"
>
> and 5.1 seems to be the eMMC standard, so one of these IP:
> http://arasan.com/products/emmc51/sd3-emmc-5-1-host/
> http://arasan.com/products/emmc51/emmc-5-1-sd-4-1-host/
>
> Whereas my board is using still another IP:
> "SD3.0 / SDIO3.0 / eMMC4.4 AHB Host Controller"
>
>
> If that is correct, then I should be able to use the 8.9a
> compatible string, and perhaps my hardware will work in
> slightly degraded performance mode, from not using the
> newest protocol gizmos available.
Resurrecting this thread after a chat with Michal on IRC.
The driver now supports a few more compatible strings.
Tracing the history...
Soren/Xilinx defined "arasan,sdhci-8.9a" in e3ec3a3d11ad
They have the "SD2.0/SDIO2.0/MMC3.31 AHB Host Controller" version
(8.9a might be a document revision number, dated 2010-04-02)
Suman/APM added "arasan,sdhci-4.9a" in 308f3f8d8112
@Suman, @Rameshwar: what specific IP block does your SoC embed?
What does 4.9a refer to? The documentation revision number?
Shawn/Rockchip added "arasan,sdhci-5.1" in da795ec26e25
This seems to be for an *actual* eMMC 5.1 version
(instead of a documentation version) as mentioned in
the commit message.
Douglas added "rockchip,rk3399-sdhci-5.1"
and made several other improvements.
I have reached out to Arasan support. Hopefully they can also
help clear up the confusion, assuming they care about the
Linux driver.
Regards.
^ permalink raw reply
* Re: [PATCH v2] mmc: Hynix: add QUIRK_NOTIFY_POWEROFF_ON_SLEEP
From: Thierry Escande @ 2016-11-18 9:58 UTC (permalink / raw)
To: Shawn Lin, Ulf Hansson; +Cc: linux-mmc, bhthompson
In-Reply-To: <504193bf-4d6c-32d1-4639-5feadc2f3b75@rock-chips.com>
Hi Shawn,
On 18/11/2016 05:27, Shawn Lin wrote:
> On 2016/11/17 18:59, Thierry Escande wrote:
>> From: zhaojohn <john.zhao@intel.com>
>>
>> Hynix eMMC devices sometimes take 50% longer to resume from sleep. This
>> occurs on Braswell based chromebook with acpi sdhci controller.
>>
>> Based on a recommendation from Hynix engineers, this patch sends a
>> Power-Off Notification using mmc_poweroff_notify() before going to S3 to
>> have a resume time consistently within spec. No voltage regulator are
>> being cut through this notification but merely tells the eMMC firmware
>> that we're about to do so and get it to behave better on resume.
>>
>> Signed-off-by: zhaojohn <john.zhao@intel.com>
>> Signed-off-by: Arindam Roy <arindam.roy@intel.com>
>> Tested-by: Freddy Paul <freddy.paul@intel.com>
>> Reviewed-by: Icarus W Sparry <icarus.w.sparry@intel.com>
>> Reviewed-by: Marc Herbert <marc.herbert@intel.com>
>> Reviewed-by: Eric Caruso <ejcaruso@chromium.org>
>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>> Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
>
> You shouldn't carry on these tags from your local tree.
>
>> ---
>>
>> Changes in v2:
>> - Add a few more details in the commit message
>>
>> drivers/mmc/card/block.c | 8 ++++++++
>> drivers/mmc/core/mmc.c | 8 +++++++-
>> include/linux/mmc/card.h | 2 ++
>> 3 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index 709a872..3db5344 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -2573,6 +2573,14 @@ static const struct mmc_fixup blk_fixups[] =
>> MMC_FIXUP("V10016", CID_MANFID_KINGSTON, CID_OEMID_ANY,
>> add_quirk_mmc,
>> MMC_QUIRK_TRIM_BROKEN),
>>
>> + /*
>> + * Hynix eMMC devices sometimes take 50% longer to resume from
>> sleep.
>> + * Based on a recommendation from Hynix, send a Power-Off
>> Notification
>> + * before going to S3 to restore a resume time consistently
>> within spec.
>> + */
>
> I don't have time to check this although I have handful of Hynix eMMCs
> on my test farm.. But it doesn't seem correct to me that *ALL* of the
> Hynix eMMCs have the same problems and they won't be fixed?
Sure. I can link the quirk to the cid name (HBG4e, which seems to be a
very commonly used one) and the oemid as well.
Regards,
Thierry
>
>> + MMC_FIXUP(CID_NAME_ANY, CID_MANFID_HYNIX, CID_OEMID_ANY,
>> add_quirk_mmc,
>> + MMC_QUIRK_NOTIFY_POWEROFF_ON_SLEEP),
>> +
>> END_FIXUP
>> };
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index df19777..774d478 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1941,8 +1941,14 @@ static int _mmc_suspend(struct mmc_host *host,
>> bool is_suspend)
>> if (mmc_can_poweroff_notify(host->card) &&
>> ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
>> err = mmc_poweroff_notify(host->card, notify_type);
>> - else if (mmc_can_sleep(host->card))
>> + else if (mmc_can_sleep(host->card)) {
>> + if (host->card->quirks & MMC_QUIRK_NOTIFY_POWEROFF_ON_SLEEP) {
>> + err = mmc_poweroff_notify(host->card, notify_type);
>> + if (err)
>> + goto out;
>> + }
>> err = mmc_sleep(host);
>> + }
>> else if (!mmc_host_is_spi(host))
>> err = mmc_deselect_cards(host);
>>
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index 73fad83..e4940f4 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -281,6 +281,8 @@ struct mmc_card {
>> #define MMC_QUIRK_BROKEN_IRQ_POLLING (1<<11) /* Polling
>> SDIO_CCCR_INTx could create a fake interrupt */
>> #define MMC_QUIRK_TRIM_BROKEN (1<<12) /* Skip trim */
>> #define MMC_QUIRK_BROKEN_HPI (1<<13) /* Disable broken HPI
>> support */
>> +#define MMC_QUIRK_NOTIFY_POWEROFF_ON_SLEEP \
>> + (1<<14) /* Poweroff notification*/
>>
>>
>> unsigned int erase_size; /* erase size in sectors */
>>
>
>
^ permalink raw reply
* Re: [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode
From: Adrian Hunter @ 2016-11-18 9:30 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Jaehoon Chung, Linus Walleij, Chaotian Jing,
Stephen Boyd, Michael Walle, Yong Mao, Shawn Lin
In-Reply-To: <CAPDyKFrDP_K_muRArcNrMGBeTLB_XWawcMc9yb82k4cdd8z0eQ@mail.gmail.com>
On 17/11/16 17:02, Ulf Hansson wrote:
> On 17 November 2016 at 11:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 16/11/16 12:51, Ulf Hansson wrote:
>>> In cases when the mmc host doesn't support HW busy detection, polling for
>>> busy by using CMD13 is beneficial. The reasons have already been explained
>>> in earlier change logs.
>>>
>>> To allow polling with CMD13, let's provide MMC_TIMING_MMC_HS200 as the
>>> timing parameter to __mmc_switch(), which makes sure the mmc host and the
>>> mmc card operates at the same bus timing during the polling.
>>
>> I have reports of cases where CMD13 always gives CRC errors after switch
>> to HS200. Currently we are assuming the low frequency should mean that
>> won't happen, but it does in some cases. That is not entirely surprising
>> since HS200 needs tuning at the final operating frequency.
>
>>From a logical point of view and if tuning is needed also for the CMD
> line, this somehow make sense.
>
> However, this is *not* how the JEDEC spec describes the HS200 switch
> sequence. It is clearly stated that the host should validate the CM6
> status via sending a CMD13 command, *before* performing tuning.
I agree, it seems not to be following spec.
>
> Could it be that the observations about the CRC errors, is related to
> a controller/driver issue and not a card issue?
I don't know what causes the problem (and I have a sneaking suspicion that
if vendors configured / designed their boards correctly, it wouldn't
happen). However, while some cards have better signal characteristics than
others, tuning is a host controller issue - the card doesn't care.
>
>>
>> What I would like to do for hosts that support busy waiting or DAT0 polling
>> (i.e. MMC_CAP_WAIT_WHILE_BUSY or host->ops->card_busy), is to ignore CRC
>> errors from the CMD13 that checks the switch status. The main reason for
>> doing that is that we really expect the switch to succeed and, given HS200
>> tuning requirement, the CRC error is not a reliable means of determining
>> that it hasn't.
>
> Hmm. So what you are saying is that CMD13 polling for HS200 doesn't
> work, as tuning is needed.
I would assume that vendors integrate a working combination of eMMC and host
controller, so if polling is the only option, then we could assume it will work.
>
> So, to me that means we need to fall-back to use the generic CMD6
> timeout instead (when HW busy detection isn't supported).
Or, in the ignore_crc/retry_err_crc case, return -EILSEQ instead -ETIMEOUT,
and catch and ignore the error in the calling code. Then you get polling if
it works, otherwise getting CRC errors until timeout.
>
>>
>> With the existing code I would just change the err check:
>>
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 3268fcd3378d..c8862c58b60b 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1387,6 +1387,13 @@ static int mmc_select_hs200(struct mmc_card *card)
>>
>> err = mmc_switch_status(card);
>> /*
>> + * For HS200, CRC errors are not a reliable way to know the
>> + * switch failed. If there really is a problem, we would expect
>> + * tuning will fail and the result ends up the same.
>> + */
>> + if (err == -EILSEQ)
>> + err = 0;
>> + /*
>
> I don't think ignoring CRC errors is reliable when verifying the CMD6
> status. My point is that we must not parse the status, in case of CRC
> errors as it can't be trusted.
I agree, but mmc_switch_status() doesn't look at the response if there is an
error.
>
> So, then we might as well just ignore validating the CMD6 status
> altogether, but instead always move on to the tuning and hope that it
> succeeds.
That is a possibility, but it seemed to me that is was worth checking for
all the users where it does work. i.e if CMD13 does not give a CRC error
then validate the response, and if CMD13 does give a CRC error then ignore
the response and keep going anyway.
>
> I think the CMD21 (tuning) should set the ILLEGAL COMMAND if HS200
> mode isn't enabled, so we could check that. Anyway, we should fail
> with the tuning if the earlier HS200 switch also failed. Don't you
> think?
Yes CMD21 is an illegal command if the mode is not HS200. The card should
set ILLEGAL_COMMAND but also not respond i.e there will be a timeout error.
That could cause a long delay before tuning finally fails. The only way to
mitigate that would be to make ignoring the CRC error a host-specific option
(e.g. MMC_CAP_... flag). Arguably, if the switch fails, the mode is broken
and should not have been allowed in the first place.
>
>> * mmc_select_timing() assumes timing has not changed if
>> * it is a switch error.
>> */
>>
>>
>> Then to support polling:
>>
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index c8862c58b60b..66d8d57ae2fb 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1352,6 +1352,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>> {
>> struct mmc_host *host = card->host;
>> unsigned int old_timing, old_signal_voltage;
>> + bool send_status;
>> int err = -EINVAL;
>> u8 val;
>>
>> @@ -1373,18 +1374,20 @@ static int mmc_select_hs200(struct mmc_card *card)
>> * switch to HS200 mode if bus width is set successfully.
>> */
>> err = mmc_select_bus_width(card);
>> - if (err > 0) {
>> - val = EXT_CSD_TIMING_HS200 |
>> - card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>> - err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> - EXT_CSD_HS_TIMING, val,
>> - card->ext_csd.generic_cmd6_time, 0,
>> - true, false, true);
>> - if (err)
>> - goto err;
>> - old_timing = host->ios.timing;
>> - mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>> + if (err <= 0)
>> + goto err;
>> +
>> + send_status = !(host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
>> + !host->ops->card_busy;
>> + old_timing = host->ios.timing;
>> +
>> + val = EXT_CSD_TIMING_HS200 |
>> + card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>> + err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, val,
>> + card->ext_csd.generic_cmd6_time,
>> + MMC_TIMING_MMC_HS200, true, send_status, true);
>>
>> + if (!err && !send_status) {
>> err = mmc_switch_status(card);
>> /*
>> * For HS200, CRC errors are not a reliable way to know the
>>
>>
>>
>> Thoughts?
>
> Well, I think the main problem is that if we have cards that returns
> CRC errors even after the HS200 switch, then we can't use polling, as
> we can't trust to parse the CMD6 status.
As I wrote above, if there is no option but polling then we could expect it
to work. And if CMD13 does not give a CRC error then we can validate the
response, only ignoring it if there is a CRC error.
I should point out that retrying CMD13 will clear the error bits in the
status so there is no point retrying when checking for the SWITCH_ERROR bit.
i.e. we need a version of __switch_send_status() that sets retries to zero.
^ permalink raw reply
* Re: [PATCH] ARM: dts: sunxi: Explicitly enable pull-ups for MMC pins
From: Linus Walleij @ 2016-11-18 9:18 UTC (permalink / raw)
To: klaus.goger-SN7IsUiht6C/RdPyistoZJqQE7yCjDx5, Ulf Hansson,
linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Chen-Yu Tsai, Maxime Ripard,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-sunxi
In-Reply-To: <ea02ec937a12d6ca2a2e0d8bbea5c484-SN7IsUiht6C/RdPyistoZJqQE7yCjDx5@public.gmane.org>
[Notice this reply has little to do with the patch in question: I think
it should be applied. I just want to involve some MMC/SD people here]
On Thu, Nov 17, 2016 at 9:57 PM, <klaus.goger-SN7IsUiht6C/RdPyistoZJqQE7yCjDx5@public.gmane.org> wrote:
> On 2016-11-17 10:34, Chen-Yu Tsai wrote:
>>
>> Given that MMC starts in open-drain mode and the pull-ups are required,
>> it's best to enable it for all the pin settings.
>
> It's even more complicated than that with MMC. It starts in open-drain mode
> for
> CMD during initialization but changes to push-pull afterwards. The card has
> internal pull-ups to prevent bus floating during setup and will disable them
> after switching to 4bit mode (or 8bit for eMMC when available).
> But even after switching to push-pull drivers there are states the bus would
> float and pull ups have to ensure a high level on the bus.
>
> See JESD84-A441 chapter 7.15 ff as reference.
>
> The difference between the P-bit and Z-bit is that a P-bit is actively
> driven
> to HIGH by the card respectively host output driver, while Z-bit is driven
> to
> (respectively kept) HIGH by the pull-up resistors Rcmd respectively Rdat.
>
> Enabling the pull ups on the CPU would be the right choice considering that
> most boards will not have external pull-ups. Even if they would have one,
> adding the internal in parallel would work in almost all cases and the
> increase in power consumption would be negligible.
I guess you are referring to software-controlled pull up on the pad ring of
the SoC. Nominally that should be handled by pin control like in this
patch.
It is not clear from context to me which of the lines need to be handled
like this? Just the data lines? DAT0 would be the critical line to pull up
in that case, since the others will not be used until bus switch anyways
right?
But what about CMD?
I assume CLK should always be push-pull?
Also: if the pins have an explicit Open Drain setting, should that
be used? I guess so?
Overall I think this construction is pretty common.
We essentially have two classes of connections:
- Those connecting the eMMC or even MMC/SD card directly to
the SoC. No pull-ups on the board. Here it makes sense to have:
1. A pin control default state with open drain and pull-ups
enabled for those lines
and it then needs
2. A second "4/8bit mode" that will switch these
pins to push-pull mode and turn off the pull-ups.
If the OD+pull-up state is the default we should probably
standardize a default name for the state when we kick in 4/8bit
mode from the IOS operation.
- Those connection the MMC/SD on an external slot through a
levelshifter/EMI filter. In that case I guess it is pretty much
dependent on the levelshifter or EMI thing how the lines out
of the SoC should be configured, and usually it is static so
you do not need to worry about it after boot configuration
of pins. (Mostly no bias, push-pull I think.)
I highly suspect a whole bunch of SoC drivers are not getting
this business right today. Not even in out-of-tree vendor kernels.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode
From: Shawn Lin @ 2016-11-18 8:05 UTC (permalink / raw)
To: Ulf Hansson, linux-mmc
Cc: shawn.lin, Jaehoon Chung, Adrian Hunter, Linus Walleij,
Chaotian Jing, Stephen Boyd, Michael Walle, Yong Mao
In-Reply-To: <1479293481-20186-8-git-send-email-ulf.hansson@linaro.org>
Hi Ulf,
在 2016/11/16 18:51, Ulf Hansson 写道:
> In cases when the mmc host doesn't support HW busy detection, polling for
> busy by using CMD13 is beneficial. The reasons have already been explained
> in earlier change logs.
>
> To allow polling with CMD13, let's provide MMC_TIMING_MMC_HS200 as the
> timing parameter to __mmc_switch(), which makes sure the mmc host and the
> mmc card operates at the same bus timing during the polling.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/mmc/core/mmc.c | 21 +++++----------------
> 1 file changed, 5 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 3268fcd..0b26383 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1351,7 +1351,7 @@ static void mmc_select_driver_type(struct mmc_card *card)
> static int mmc_select_hs200(struct mmc_card *card)
> {
> struct mmc_host *host = card->host;
> - unsigned int old_timing, old_signal_voltage;
> + unsigned int old_signal_voltage;
> int err = -EINVAL;
> u8 val;
>
> @@ -1378,22 +1378,11 @@ static int mmc_select_hs200(struct mmc_card *card)
> card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> EXT_CSD_HS_TIMING, val,
> - card->ext_csd.generic_cmd6_time, 0,
> - true, false, true);
> - if (err)
> - goto err;
> - old_timing = host->ios.timing;
> - mmc_set_timing(host, MMC_TIMING_MMC_HS200);
> -
> - err = mmc_switch_status(card);
> - /*
> - * mmc_select_timing() assumes timing has not changed if
> - * it is a switch error.
> - */
> - if (err == -EBADMSG)
> - mmc_set_timing(host, old_timing);
> + card->ext_csd.generic_cmd6_time,
> + MMC_TIMING_MMC_HS200,
> + true, true, true);
I was finding a failure from the test last night after applying these
patches and using HS200 only.
It seems like the controller(sdhci-of-arasan,5.1) continuously generate
response timeout for checking CMD13.. Per TRM, response timeout could
means the device didn't ack the CMD13 or the controller was failing to
latch the response..
The eMMC part number is KLMBG2JENB-B041. I will use two boards to check
if it was indeed related to these patches.
> }
> -err:
> +
> if (err) {
> /* fall back to the old signal voltage, if fails report error */
> if (__mmc_set_signal_voltage(host, old_signal_voltage))
>
--
Best Regards
Shawn Lin
^ permalink raw reply
* Re: [PATCH] mmc: dw_mmc: fix the error handling for dma operation
From: Jaehoon Chung @ 2016-11-18 7:15 UTC (permalink / raw)
To: Shawn Lin, linux-mmc; +Cc: ulf.hansson
In-Reply-To: <bbb9dcda-ead5-0a52-4c9a-3b5ff078ad45@samsung.com>
Hi Shawn,
On 11/17/2016 05:25 PM, Jaehoon Chung wrote:
> On 11/17/2016 05:08 PM, Shawn Lin wrote:
>> Hi Jaehoon,
>>
>> 在 2016/11/17 15:53, Jaehoon Chung 写道:
>>> When OWN bit of dma descriptor is not cleared, then it returns -EINVAL.
>>> Then it has to fall back to PIO mode for current transfer.
>>>
>>> Host controller was already set to bits relevant to DMA operation.
>>> If needs to use the PIO mode, Host controller has to stop the DMA
>>> operation. (It's more stable than now.)
>>>
>>
>> It looks good to me, but
>>
>>> When it occurred error, it's not running any request.
>>>
>>> Fixes: 3b2a067b98b4 ("mmc: dw_mmc: avoid race condition of cpu and IDMAC")
>>>
>>
>> I think the real fixes tag should indicate the another commit,
>> 3fc7eaef44dbcbcd60 ("mmc: dw_mmc: Add external dma interface support")
>
> You're right.
> But my main problem is "checking OWN bit". Thanks for noticing this commit. :)
> Some time..OWN bit is never released..
I think your the suggested commit is more correct than me.
After modifying fixes commit ID, then will apply. Thanks!
Best Regards,
Jaehoon Chung
>
> Best Regards,
> Jaehoon Chung
>
>>
>> otherwise,
>>
>> Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>> ---
>>> drivers/mmc/host/dw_mmc.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 9341b18..080003b 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -1058,6 +1058,7 @@ static int dw_mci_submit_data_dma(struct dw_mci *host, struct mmc_data *data)
>>> spin_unlock_irqrestore(&host->irq_lock, irqflags);
>>>
>>> if (host->dma_ops->start(host, sg_len)) {
>>> + host->dma_ops->stop(host);
>>> /* We can't do DMA, try PIO for this one */
>>> dev_dbg(host->dev,
>>> "%s: fall back to PIO mode for current transfer\n",
>>>
>>
>>
>
> --
> 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
>
>
>
^ permalink raw reply
* Re: [PATCH v6 0/5] Add intial support to DW MMC host on ZTE SoC
From: Jaehoon Chung @ 2016-11-18 6:52 UTC (permalink / raw)
To: Jun Nie, shawn.guo, xie.baoyou
Cc: ulf.hansson, jason.liu, chen.chaokai, lai.binz, linux-mmc
In-Reply-To: <1479450555-19047-1-git-send-email-jun.nie@linaro.org>
Hi Jun,
On 11/18/2016 03:29 PM, Jun Nie wrote:
> Add intial support to DW MMC host on ZTE SoC. It include platform
> specific wrapper driver and workarounds for fifo quirk.
>
> Patches are prepared based on latest dw mmc runtime change:
> https://github.com/jh80chung/dw-mmc.git for-ulf
Applied on my dwmmc repository.
I have applied your patches, after modifying some conflict and subject prefix as "dw_mmc*".
And added Shawn's reviewed-by tags. Thanks!
Best Regards,
Jaehoon Chung
>
> Changes vs version 5:
> - Add clock delay lock status check to save CPU cycle in timing tuning CMD.
>
> Changes vs version 4:
> - Fix missing empty dts compatible element in the end of compatible array.
>
> Changes vs version 3:
> - Fix brace error in document.
>
> Changes vs version 2:
> - Change dt property fifo-addr to data-addr and fifo-watermark-quirk to
> fifo-watermark-aligned.
> - Polish ZX MMC driver on minor coding style issues.
>
> Changes vs version 1:
> - Change fifo-addr-override to fifo-addr and remove its workaround tag in comments.
> - Remove ZX DW MMC driver reset cap in driver, which can be added in dt nodes.
>
> Jun Nie (5):
> mmc: dt-bindings: add ZTE ZX296718 MMC bindings
> mmc: zx: Initial support for ZX mmc controller
> Documentation: synopsys-dw-mshc: add binding for fifo quirks
> mmc: dw: Add fifo address property
> mmc: dw: Add fifo watermark alignment property
>
> .../devicetree/bindings/mmc/synopsys-dw-mshc.txt | 13 ++
> .../devicetree/bindings/mmc/zx-dw-mshc.txt | 34 +++
> drivers/mmc/host/Kconfig | 9 +
> drivers/mmc/host/Makefile | 1 +
> drivers/mmc/host/dw_mmc-zx.c | 242 +++++++++++++++++++++
> drivers/mmc/host/dw_mmc-zx.h | 31 +++
> drivers/mmc/host/dw_mmc.c | 17 +-
> include/linux/mmc/dw_mmc.h | 5 +
> 8 files changed, 349 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/mmc/zx-dw-mshc.txt
> create mode 100644 drivers/mmc/host/dw_mmc-zx.c
> create mode 100644 drivers/mmc/host/dw_mmc-zx.h
>
^ permalink raw reply
* Re: [PATCH v5 0/5] Add intial support to DW MMC host on ZTE SoC
From: Jun Nie @ 2016-11-18 6:34 UTC (permalink / raw)
To: Jaehoon Chung; +Cc: Shawn Guo, xie.baoyou, Ulf Hansson, Jason Liu, linux-mmc
In-Reply-To: <20eca791-015f-1d9f-d24b-213276db08db@samsung.com>
2016-11-18 10:16 GMT+08:00 Jaehoon Chung <jh80.chung@samsung.com>:
> Hi Jun,
>
> On 11/08/2016 10:24 AM, Jun Nie wrote:
>> Add intial support to DW MMC host on ZTE SoC. It include platform
>> specific wrapper driver and workarounds for fifo quirk.
>
> If you send the patch after modifying Shawn's comment, I will apply these patchset on my repository.
> I don't know but i guess you want to apply these patches on v4.10.
>
> Best Regards,
> Jaehoon Chung
>
Thanks for you guys' review. Could you please help add your review tag
to V6 patches as I already send out and do not want to send twice to
confuse everyone. Thank you!
Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>
Jun
>>
>> Patches are prepared based on latest dw mmc runtime change:
>> https://github.com/jh80chung/dw-mmc.git for-ulf
>>
>> Changes vs version 4:
>> - Fix missing empty dts compatible element in the end of compatible array.
>>
>> Changes vs version 3:
>> - Fix brace error in document.
>>
>> Changes vs version 2:
>> - Change dt property fifo-addr to data-addr and fifo-watermark-quirk to
>> fifo-watermark-aligned.
>> - Polish ZX MMC driver on minor coding style issues.
>>
>> Changes vs version 1:
>> - Change fifo-addr-override to fifo-addr and remove its workaround tag in comments.
>> - Remove ZX DW MMC driver reset cap in driver, which can be added in dt nodes.
>>
>> Jun Nie (5):
>> mmc: dt-bindings: add ZTE ZX296718 MMC bindings
>> mmc: zx: Initial support for ZX mmc controller
>> Documentation: synopsys-dw-mshc: add binding for fifo quirks
>> mmc: dw: Add fifo address property
>> mmc: dw: Add fifo watermark alignment property
>>
>> .../devicetree/bindings/mmc/synopsys-dw-mshc.txt | 13 ++
>> .../devicetree/bindings/mmc/zx-dw-mshc.txt | 34 +++
>> drivers/mmc/host/Kconfig | 9 +
>> drivers/mmc/host/Makefile | 1 +
>> drivers/mmc/host/dw_mmc-zx.c | 242 +++++++++++++++++++++
>> drivers/mmc/host/dw_mmc-zx.h | 31 +++
>> drivers/mmc/host/dw_mmc.c | 17 +-
>> include/linux/mmc/dw_mmc.h | 5 +
>> 8 files changed, 349 insertions(+), 3 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/mmc/zx-dw-mshc.txt
>> create mode 100644 drivers/mmc/host/dw_mmc-zx.c
>> create mode 100644 drivers/mmc/host/dw_mmc-zx.h
>>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox