2012/12/14 Kevin Liu <[email protected]>:
> 2012/12/14 Ulf Hansson <[email protected]>:
>> On 14 December 2012 11:53, Kevin Liu <[email protected]> wrote:
>>> 1. if host does NOT implement signal voltage setting function,
>>> should return error.
>>> 2. add the check for data signal before voltage change according
>>> to the spec. If host does NOT detect a low signal level, the host
>>> should abort the voltage switch sequence. (phisical layer spec 4.2.4.2(4))
>>> 3. if voltage change failed then no need to restore the clock
>>> before cycle power the card.
>>> 4. call mmc_power_cycle here since it's a part of voltage switch.
>>> besides -EAGAIN, any other error returned should also cycle power the card.
>>>
>>> Signed-off-by: Kevin Liu <[email protected]>
>>> ---
>>> drivers/mmc/core/core.c | 71
>>> ++++++++++++++++++++++++++++------------------
>>> 1 files changed, 43 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index d1aa8ab..da93186 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -1226,6 +1226,9 @@ int mmc_set_signal_voltage(struct mmc_host *host, int
>>> signal_voltage, bool cmd11
>>>
>>> BUG_ON(!host);
>>>
>>> + if (!host->ops->start_signal_voltage_switch)
>>> + return -EPERM;
>>> +
>>
>> No, this is not OK.
>> For example, eMMC might default use 1.8V as I/O voltage, you don't
>> want to force host drivers implementing this function to make this
>> work.
>>
> I agree.
> But then how can we make sure emmc's voltage?
> Only depend on the voltage by default?
> I will update the patch and move this judgement withine cmd11.
>
>>> /*
>>> * Send CMD11 only if the request is to switch the card to
>>> * 1.8V signalling.
>>> @@ -1245,47 +1248,59 @@ int mmc_set_signal_voltage(struct mmc_host *host,
>>> int signal_voltage, bool cmd11
>>>
>>> host->ios.signal_voltage = signal_voltage;
>>>
>>> - if (host->ops->start_signal_voltage_switch) {
>>> - u32 clock;
>>> + u32 clock;
>>> +
>>> + mmc_host_clk_hold(host);
>>> +
>>> + if (cmd11) {
>>> + if (!host->ops->card_busy)
>>> + pr_warning("%s: cannot verify signal voltage
>>> switch\n",
>>> + mmc_hostname(host));
>>>
>>> - mmc_host_clk_hold(host);
>>> /*
>>> * During a signal voltage level switch, the clock must be
>>> gated
>>> * for a certain period of time according to the SD spec
>>> */
>>> - if (cmd11) {
>>> - clock = host->ios.clock;
>>> - host->ios.clock = 0;
>>> - mmc_set_ios(host);
>>> - }
>>> + clock = host->ios.clock;
>>> + host->ios.clock = 0;
>>> + mmc_set_ios(host);
>>>
>>> - err = host->ops->start_signal_voltage_switch(host,
>>> &host->ios);
>>> + if (host->ops->card_busy && !host->ops->card_busy(host)) {
>>> + err = -EAGAIN;
>>> + } else {
>>
>> Great that you spotted this! We should check busy before we do the
>> actual voltage switch.
>>
>> Although, I think it would make sense to do this before the clock is
>> set to 0. Especially important for those host controllers that do
>> support busy-detection, a qualified guess would be that those also
>> could require the clock to be on to detect this. Or what do you think?
>> From spec point of view it shall be safe to do it before setting clk
>> to 0.
>>
> I considered this when I draft this patch.
> My only concern is whether DAT level is stable at that point.
> According to the spec, the card driver DAT[3:0] to low at the NEXT
> clock of the end bit for R1 response after completing the R1 response.
> I'm not sure whether the status is stable if we query the DAT level
> just after cmd11.
>
How about adding a 1ms delay between cmd11 response and calling card_busy?
And we can do this before the clock is set to 0.
As spec said the timing
> I agree with you that it will be safer to keep clock on when detecting.
>
>>>
>>> - if (err && cmd11) {
>>> - host->ios.clock = clock;
>>> - mmc_set_ios(host);
>>> - } else if (cmd11) {
>>> - /* Keep clock gated for at least 5 ms */
>>> - mmc_delay(5);
>>> - host->ios.clock = clock;
>>> - mmc_set_ios(host);
>>> + err = host->ops->start_signal_voltage_switch(host,
>>> &host->ios);
>>>
>>> - /* Wait for at least 1 ms according to spec */
>>> - mmc_delay(1);
>>> + if (!err) {
>>> + /* Keep clock gated for at least 5 ms */
>>> + mmc_delay(5);
>>> + host->ios.clock = clock;
>>> + mmc_set_ios(host);
>>>
>>> - /*
>>> - * Failure to switch is indicated by the card
>>> holding
>>> - * dat[0:3] low
>>> - */
>>> - if (!host->ops->card_busy)
>>> - pr_warning("%s: cannot verify signal
>>> voltage switch\n",
>>> + /* Wait for at least 1 ms according to spec
>>> */
>>> + mmc_delay(1);
>>> +
>>> + /*
>>> + * Failure to switch is indicated by the
>>> card holding
>>> + * dat[0:3] low
>>> + */
>>> + if (host->ops->card_busy &&
>>> host->ops->card_busy(host))
>>> + err = -EAGAIN;
>>> + }
>>> + }
>>> + if (err) {
>>> + /* Power cycle card */
>>> + pr_debug("%s: Signal voltage switch failed, "
>>> + "power cycling card \n",
>>> mmc_hostname(host));
>>> - else if (host->ops->card_busy(host))
>>> - err = -EAGAIN;
>>> + mmc_power_cycle(host);
>>> }
>>> - mmc_host_clk_release(host);
>>> + } else {
>>> + err = host->ops->start_signal_voltage_switch(host,
>>> &host->ios);
>>> }
>>>
>>> + mmc_host_clk_release(host);
>>> +
>>> return err;
>>> }
>>>
>>> --
>>> 1.7.0.4
>>>
>>
>> Kind regards
>> Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
