Issue with an SD-card switching to high speed

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():

         }

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()


What's your opinion?



Best regards,
Yann

On 10/19/22 17:50, Yann Gautier 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);
>                  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()
> 
> 
> What's your opinion?
> 
> 
> 
> Best regards,
> Yann

Gentle ping.

Should I push the patch, and have the discussion based on that?


Thanks,
Yann

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

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