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