On 14 December 2012 14:40, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> On 14 December 2012 11:53, Kevin Liu <kliu5@xxxxxxxxxxx> 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 <kliu5@xxxxxxxxxxx>
>> ---
>> 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.
>
>> /*
>> * 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.
>
>>
>> - 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);
I definitely agree, that this should be done here!
Although, this will break the SDIO case, so we need to fix that in the
sdio.c as well.
>> }
>> - 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
Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
