Re: sdhci-omap: issues with PM features since 5.16

[Romain Naour <romain.naour@smile.fr>]

Le 02/02/2024 à 05:36, Tony Lindgren a écrit :
> * Romain Naour <romain.naour@smile.fr> [240201 09:04]:
>> Le 31/01/2024 à 11:30, Tony Lindgren a écrit :
>>> * Romain Naour <romain.naour@smile.fr> [240130 11:20]:
>>>> Le 29/01/2024 à 18:42, Romain Naour a écrit :
>>>>> Le 29/01/2024 à 12:17, Tony Lindgren a écrit :
>>>>>> So I'm still guessing your issue is with emmc not getting reinitialized
>>>>>> properly as there's no mmc-pwrseq-emmc configured. Can you give it a
>>>>>> try? See am5729-beagleboneai.dts for an example.
>>>>
>>>> I can't add such mmc-pwrseq-emmc on the custom board, there is no gpio available
>>>> to reset the emmc device.
>>>>
>>>> To resume:
>>>> - the emmc doesn't work with mmc-hs200-1_8v mode with PM runtime enabled
>>>> - the emmc works with mmc-hs200-1_8v mode without PM runtime (patch series reverted)
>>>> - the emmc works with mmc-ddr-1_8v mode with PM runtime enabled
>>>>
>>>> AFAIU the hs200 mode requires some pin iodelay tuning (SDHCI_OMAP_REQUIRE_IODELAY)
>>>> is sdhci_omap_runtime_{suspend,resume} needs to take care of that?
> 
> On PM runtime resume, sdhci_omap_runtime_resume() gets called and calls
> sdhci_runtime_resume_host(), and calls mmc->ops->set_ios().
> 
> Then sdhci_omap_set_ios() calls sdhci_omap_set_timing() to set the iodelay.
> Maybe add some printk to sdhci_omap_set_timing() to verify the right modes
> get set on PM runtime resume?
> 
>>>> The mmc2 clock seems idle when mmc-hs200-1_8v and PM runtime are used.
>>>>
>>>> omapconf dump prcm l3init
>>>>
>>>> (mmc2 clock idle)
>>>> | CM_L3INIT_MMC2_CLKCTRL           | 0x4A009330   | 0x01070000 |
>>>>
>>>> (mmc2 clock running)
>>>> | CM_L3INIT_MMC2_CLKCTRL           | 0x4A009330   | 0x01040002 |
>>>>
>>>> Thoughts?
> 
> For the clocks above, that is as expected. The clocks get idled when the
> MMC controller is idle.
> 
>>> OK so if the emmc reset gpio is not available, seems we should do something
>>> like the following patch to not set MMC_CAP_POWER_OFF_CARD unless the
>>> cap-power-off-card devicetree property is set.
>>>
>>> Care to give it a try and see if it helps?
>>
>> Same problem without MMC_CAP_POWER_OFF_CARD flag (even by removing
>> MMC_CAP_AGGRESSIVE_PM too).
>>
>> I did some test with mmc capabilities mask but no progress so far.
> 
> OK. So this issue seems to be related to the PM runtime resume not
> restoring something properly as you suggested earlier.

Adding your PM reply with the mailing list in Cc:

Le 06/02/2024 à 09:25, Tony Lindgren a écrit :
> * Tony Lindgren <tony@atomide.com> [240202 10:30]:
[...]
>
> When you get a chance, maybe give the following debug patch a try.
> I'm mostly seeing value of 2 and sometimes 0, but that could be
> for a different mmc controller instance as I just used pr_info.
> So you may need to tweak the debug patch to use dev_dbg to leave
> out other controllers.
>
> #define MMC_POWER_OFF           0
> #define MMC_POWER_UP            1
> #define MMC_POWER_ON            2
> #define MMC_POWER_UNDEFINED     3
>
> So on MMC_POWER_OFF, in sdhci_runtime_resume_host() the flag
> host->reinit_uhs = true does not get set, and maybe with hs200
> that causes the failure?

With the debug line added, I don't see any MMC_POWER_OFF for the emmc but only
MMC_POWER_ON lines:

XXX sdhci_runtime_resume_host: mmc->ios.power_mode: 2

>
> If you're seeing MMC_POWER_OFF values, maybe also try changing
> sdhci_omap_runtime_resume() to call sdhci_runtime_resume_host(host, 1)
> and see if that helps as requesting a soft reset causes sdhci_init() to
> set host->reinit_uhs = true.. That change feels like a workaround
> though.

I tried anyway with soft reset, the cache flush error is gone but now I have
this dump in dmesg each time the emmc is reset:

[ 3978.852783] mmc1: Reset 0x6 never completed.
[ 3978.852783] mmc1: sdhci: ============ SDHCI REGISTER DUMP ===========
[ 3978.852783] mmc1: sdhci: Sys addr:  0x00000000 | Version:  0x00003302
[ 3978.852813] mmc1: sdhci: Blk size:  0x00000000 | Blk cnt:  0x00000000
[ 3978.852813] mmc1: sdhci: Argument:  0x00000000 | Trn mode: 0x00000000
[ 3978.852813] mmc1: sdhci: Present:   0x01f00000 | Host ctl: 0x00000000
[ 3978.852813] mmc1: sdhci: Power:     0x00000000 | Blk gap:  0x00000000
[ 3978.852813] mmc1: sdhci: Wake-up:   0x00000000 | Clock:    0x00000000
[ 3978.852844] mmc1: sdhci: Timeout:   0x00000000 | Int stat: 0x00000000
[ 3978.852844] mmc1: sdhci: Int enab:  0x00000000 | Sig enab: 0x00000000
[ 3978.852844] mmc1: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
[ 3978.852844] mmc1: sdhci: Caps:      0x24e90080 | Caps_1:   0x00000f77
[ 3978.852844] mmc1: sdhci: Cmd:       0x00000000 | Max curr: 0x00000000
[ 3978.852874] mmc1: sdhci: Resp[0]:   0x00000000 | Resp[1]:  0x00000000
[ 3978.852874] mmc1: sdhci: Resp[2]:   0x00000000 | Resp[3]:  0x00000000
[ 3978.852874] mmc1: sdhci: Host ctl2: 0x00000000
[ 3978.852874] mmc1: sdhci: ADMA Err:  0x00000000 | ADMA Ptr: 0x00000000
[ 3978.852874] mmc1: sdhci: ============================================

With sdhci soft reset enabled, there are some MMC_POWER_OFF in dmesg

[ 3978.852905] XXX sdhci_runtime_resume_host: mmc->ios.power_mode: 0
[ 3982.217590] XXX sdhci_runtime_resume_host: mmc->ios.power_mode: 2

The iodelay pin setting is still applied after the emmc is reset:

# omapconf dump 0x4A00348c 0x4A0034ac
|----------------------------|
| Address (hex) | Data (hex) |
|----------------------------|
| 0x4A00348C    | 0x00070101 |
| 0x4A003490    | 0x00070101 |
| 0x4A003494    | 0x00070101 |
| 0x4A003498    | 0x00070101 |
| 0x4A00349C    | 0x00060101 |
| 0x4A0034A0    | 0x00070101 |
| 0x4A0034A4    | 0x00070101 |
| 0x4A0034A8    | 0x00070101 |
| 0x4A0034AC    | 0x00070101 |
|----------------------------|

>
> Regards,
>
> Tony
>
> 8< ------
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3848,6 +3848,7 @@ int sdhci_runtime_resume_host(struct sdhci_host *host,
int soft_reset)
>  	}
>
>  	sdhci_init(host, soft_reset);
> +	pr_info("XXX %s: mmc->ios.power_mode: %i\n", __func__, mmc->ios.power_mode);
>
>  	if (mmc->ios.power_mode != MMC_POWER_UNDEFINED &&
>  	    mmc->ios.power_mode != MMC_POWER_OFF) {


Best regards,
Romain


> 
> Regards,
> 
> Tony