Re: [PATCH v3] mmc: core: Add a card quirk for non-hw busy detection

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

On 8/17/21 12:37 AM, Linus Walleij wrote:
> On Mon, Aug 16, 2021 at 4:03 PM Yann Gautier <yann.gautier@foss.st.com> wrote:
> 
>> I was just testing your patch on top of mmc/next.
>> Whereas mmc/next is fine, with your patch I fail to pass MMC test 5
>> (Multi-block write).
>> I've got this error on STM32MP157C-EV1 board:
>> [  108.956218] mmc0: Starting tests of card mmc0:aaaa...
>> [  108.959862] mmc0: Test case 5. Multi-block write...
>> [  108.995615] mmc0: Warning: Host did not wait for busy state to end.
>> [  109.000483] mmc0: Result: ERROR (-110)
>> Then nothing more happens.
>>
>> The test was done on an SD-card Sandisk Extreme Pro SDXC UHS-I mark 3,
>> in DDR50 mode.
>>
>> I'll try to add more traces to see what happens.
> 

Hi Linus

> What I think happens is:
> - You are using the MMCI driver (correct?)
Yes

> - My patch augments the driver to not use busydetect until we have
>    determined that the card can do it (after reading extcsd etc)
> - Before this patch, the MMCI would unconditionally use HW
>    busy detect on any card.
I finally found the problem.
The assignment of host->card is done at the end of mmc_sd_init_card().
But mmci_set_max_busy_timeout() is called in mmc_sd_init_card() before 
that, and card is then NULL at that time. This let me a 
mmc->max_busy_timeout = 0. And this value is no more updated.
mmci_start_command() will then have a unexpected behavior with that 0 value.

Maybe we should not use mmci_use_busy_detect() in 
mmci_set_max_busy_timeout()?

If I use this patch on top of yours (reverting the 
mmci_set_max_busy_timeout() change), all the mmc tests pass on the 
SD-card I was testing:

@@ -1741,11 +1741,11 @@ static void mmci_request(struct mmc_host *mmc, 
struct mmc_request *mrq)
  static void mmci_set_max_busy_timeout(struct mmc_host *mmc)
  {
  	struct mmci_host *host = mmc_priv(mmc);
  	u32 max_busy_timeout = 0;

-	if (!mmci_use_busy_detect(host))
+	if (!host->variant->busy_detect)
  		return;

  	if (host->variant->busy_timeout && mmc->actual_clock)
  		max_busy_timeout = ~0UL / (mmc->actual_clock / MSEC_PER_SEC);


> 
> Either we have managed to wire the MMCI driver so that it doesn't
> work without HW busy detect anymore, you can easily test this
> by doing this:
> 
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 3765e2f4ad98..3a35f65491c8 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -270,10 +270,10 @@ static struct variant_data variant_stm32_sdmmc = {
>          .datactrl_any_blocksz   = true,
>          .datactrl_mask_sdio     = MCI_DPSM_ST_SDIOEN,
>          .stm32_idmabsize_mask   = GENMASK(12, 5),
> -       .busy_timeout           = true,
> -       .busy_detect            = true,
> -       .busy_detect_flag       = MCI_STM32_BUSYD0,
> -       .busy_detect_mask       = MCI_STM32_BUSYD0ENDMASK,
> +       //.busy_timeout         = true,
> +       //.busy_detect          = true,
> +       //.busy_detect_flag     = MCI_STM32_BUSYD0,
> +       //.busy_detect_mask     = MCI_STM32_BUSYD0ENDMASK,
>          .init                   = sdmmc_variant_init,
>   };
> 
> @@ -297,10 +297,10 @@ static struct variant_data variant_stm32_sdmmcv2 = {
>          .datactrl_mask_sdio     = MCI_DPSM_ST_SDIOEN,
>          .stm32_idmabsize_mask   = GENMASK(16, 5),
>          .dma_lli                = true,
> -       .busy_timeout           = true,
> -       .busy_detect            = true,
> -       .busy_detect_flag       = MCI_STM32_BUSYD0,
> -       .busy_detect_mask       = MCI_STM32_BUSYD0ENDMASK,
> +       //.busy_timeout         = true,
> +       //.busy_detect          = true,
> +       //.busy_detect_flag     = MCI_STM32_BUSYD0,
> +       //.busy_detect_mask     = MCI_STM32_BUSYD0ENDMASK,

This was working, but disabling HW busy detection is not really what we 
want.

>          .init                   = sdmmc_variant_init,
> 
> Or else there is a card that cannot work without busy detect which
> I find unlikely.
> 
> Yours,
> Linus Walleij >


I have the same kind of issues with the eMMC on the STM32MP157C-EV1 
board. But here it fails at boot when trying to enable HPI, in mmc_switch().


I then updated the patch like this:
@@ -357,7 +357,7 @@ static bool mmci_use_busy_detect(struct mmci_host *host)

         /* We don't allow this until we know that the card can handle it */
         if (!card)
-               return false;
+               return true;


And it then works for all my use-cases, but I suppose that's not what 
you wanted to do.

So I guess we need to have the mmc_card structure, to determine if we 
have the quirk, but not from the mmc_host. Through some new callback?


Best regards,
Yann