Re: Issue with an SD-card switching to high speed

[Yann Gautier <yann.gautier@foss.st.com>]

On 10/26/22 16:16, Ulf Hansson wrote:
> On Wed, 19 Oct 2022 at 17:50, Yann Gautier <yann.gautier@foss.st.com> wrote:
>>
>> Hi Ulf (and mailing-list),
>>
>> I've an SD-card on a STM32MP157F-DK2 board that cannot switch to
>> high-speed mode:
>> "mmc0: Problem switching card into high-speed mode!"
>>
>> On this board, it is not possible to switch to UHS modes.
>> And there is no power cycle done in kernel.
>>
>> When checking the differences when I add full-pwr-cycle in DT, I see
>> that the OCR we ask the card is different:
>> 0x300000 (MMC_VDD_32_33 | MMC_VDD_33_34) vs 0x200000 (MMC_VDD_33_34).
>>
>> If I add this missing MMC_VDD_32_33 voltage range (without power cycle),
>> then the card can switch to high-speed.
>>
>> Checking where this is done in the framework, I've seen something that
>> could correct my issue in mmc_select_voltage():
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 368f10405e13..bcd8fa81f78b 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1132,7 +1132,7 @@ u32 mmc_select_voltage(struct mmc_host *host, u32 ocr)
>>                   mmc_power_cycle(host, ocr);
>>           } else {
>>                   bit = fls(ocr) - 1;
>> -               ocr &= 3 << bit;
>> +               ocr &= 3 << (bit - 1);
> 
> To me, this looks like you may be fixing a very old bug. Unless I am
> wrong, it seems like the current code might as well have been:
> 
> ocr &= 1 << bit;
> 
> The upper bit that the '3' is trying to allow to be set, can in fact
> never be set, because we have already done "ocr &= host->ocr_avail" a
> few lines above.
> 
> 
>>                   if (bit != host->ios.vdd)
>>                           dev_warn(mmc_dev(host), "exceeding card's
>> volts\n");
>>           }
>>
>> The ocr given to mmc_select_voltage() is 0x300000.
>> fls(ocr) = 22, bit = 21, 3 << bit = 0x600000.
>> With the &= operator, we then have only 0x200000 and have removed
>> MMC_VDD_32_33 mode.
>> The architecture is an Armv7, I hope that the fls() has the same
>> behavior on other architectures.
>>
>> But as this function is also used for eMMC and SDIO, this could have
>> impacts I've not seen.
>>
>>
>> Maybe the issue is just with this SD-card, that doesn't properly handle
>> the range MMC_VDD_33_34 alone, and it could be out of spec.
>>
>> I then have 3 possibilities:
>> - stop using this type of card if it is out-of-specs
>> - add full-pwr-cycle in this board's DT, but I'll have issues with other
>> boards that really cannot do power cycle
>> - push the proposed patch in mmc_select_voltage()
> 
> Yes, please - so we can discuss it better.
> 
> Also, please try to add a relevant comment in the code too, so it
> becomes a bit more obvious of what goes on.
> 
> Kind regards
> Uffe

Thanks Ulf,

I'll send the patch in the coming days, with the required comments.

Best regards,
Yann