On Wed, 19 Oct 2022 at 17:50, Yann Gautier <yann.gautier@xxxxxxxxxxx> 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
