Question about MMC_PM_KEEP_POWER in MMCI driver

Question about MMC_PM_KEEP_POWER in MMCI driver

[Yann Gautier]

Hi,

In drivers/mmc/host/mmci.c, MMC_PM_KEEP_POWER is unconditionally enabled.
This prevents correct low-power sequence on STM32MP157C-DK2 board which 
embeds a Wifi chip brcm,bcm4329-fmac (this wifi part has not yet been 
sent upstream).

This MMC_PM_KEEP_POWER can be taken from DT with the property 
keep-power-in-suspend. This is what is done for other MMC drivers.

I wonder what should be the best solution for this.

1) Remove MMC_PM_KEEP_POWER from the driver, and modify all SoC device 
tree files embedding a arm,pl18x with adding keep-power-in-suspend; 
property (except stm32mp151.dtsi file).
This can be easy to do (~10 files to modify). But that could be more 
board dependent, if an SDIO chip is plugged on this MMC IP.
And the name keep-power-in-suspend can be misleading as it only applies 
to SDIO.

2) Remove MMC_PM_KEEP_POWER from the driver, and modify board DT files 
with the property. This could be a difficult task to find all those 
boards. And this should be applied only for SDIO configs.

3) Just modify the driver to apply this capability for all MMCI chips 
but STM32. This could be done in the dedicated file, in 
sdmmc_variant_init() function. But some boards based on STM32MP15 chip 
might want to keep this capability.

All advice is welcome.


Thanks,
Yann

Re: Question about MMC_PM_KEEP_POWER in MMCI driver

[Ulf Hansson]

On Wed, 25 Aug 2021 at 16:34, Yann Gautier <yann.gautier@foss.st.com> wrote:
>
> Hi,
>
> In drivers/mmc/host/mmci.c, MMC_PM_KEEP_POWER is unconditionally enabled.
> This prevents correct low-power sequence on STM32MP157C-DK2 board which
> embeds a Wifi chip brcm,bcm4329-fmac (this wifi part has not yet been
> sent upstream).

Exactly why doesn't this work with the STM32MP157C-DK2 board?

>
> This MMC_PM_KEEP_POWER can be taken from DT with the property
> keep-power-in-suspend. This is what is done for other MMC drivers.

The DT property is what should have been used for mmci as well.

>
> I wonder what should be the best solution for this.
>
> 1) Remove MMC_PM_KEEP_POWER from the driver, and modify all SoC device
> tree files embedding a arm,pl18x with adding keep-power-in-suspend;
> property (except stm32mp151.dtsi file).
> This can be easy to do (~10 files to modify). But that could be more
> board dependent, if an SDIO chip is plugged on this MMC IP.
> And the name keep-power-in-suspend can be misleading as it only applies
> to SDIO.
>
> 2) Remove MMC_PM_KEEP_POWER from the driver, and modify board DT files
> with the property. This could be a difficult task to find all those
> boards. And this should be applied only for SDIO configs.
>
> 3) Just modify the driver to apply this capability for all MMCI chips
> but STM32. This could be done in the dedicated file, in
> sdmmc_variant_init() function. But some boards based on STM32MP15 chip
> might want to keep this capability.

I would suggest option 3).

As a matter of fact, we also allow MMC_PM_KEEP_POWER to become set
when parsing the DTB via calling mmc_of_parse(). So just changing the
default value (don't set MMC_PM_KEEP_POWER) for the stm32 variant,
would do the trick I think.

Kind regards
Uffe

Re: Question about MMC_PM_KEEP_POWER in MMCI driver

[Yann Gautier]

On 8/30/21 3:43 PM, Ulf Hansson wrote:
> On Wed, 25 Aug 2021 at 16:34, Yann Gautier <yann.gautier@foss.st.com> wrote:
>>
>> Hi,
>>

Hi Linus,

Thanks for the advice.

>> In drivers/mmc/host/mmci.c, MMC_PM_KEEP_POWER is unconditionally enabled.
>> This prevents correct low-power sequence on STM32MP157C-DK2 board which
>> embeds a Wifi chip brcm,bcm4329-fmac (this wifi part has not yet been
>> sent upstream).
> 
> Exactly why doesn't this work with the STM32MP157C-DK2 board?
> 
It is more the opposite.
We had that patch in our downstream for a while, and this was possible 
due to changes in wifi driver.
But I'll check all that precisely, and make some more tests.

>>
>> This MMC_PM_KEEP_POWER can be taken from DT with the property
>> keep-power-in-suspend. This is what is done for other MMC drivers.
> 
> The DT property is what should have been used for mmci as well.
> 
>>
>> I wonder what should be the best solution for this.
>>
>> 1) Remove MMC_PM_KEEP_POWER from the driver, and modify all SoC device
>> tree files embedding a arm,pl18x with adding keep-power-in-suspend;
>> property (except stm32mp151.dtsi file).
>> This can be easy to do (~10 files to modify). But that could be more
>> board dependent, if an SDIO chip is plugged on this MMC IP.
>> And the name keep-power-in-suspend can be misleading as it only applies
>> to SDIO.
>>
>> 2) Remove MMC_PM_KEEP_POWER from the driver, and modify board DT files
>> with the property. This could be a difficult task to find all those
>> boards. And this should be applied only for SDIO configs.
>>
>> 3) Just modify the driver to apply this capability for all MMCI chips
>> but STM32. This could be done in the dedicated file, in
>> sdmmc_variant_init() function. But some boards based on STM32MP15 chip
>> might want to keep this capability.
> 
> I would suggest option 3).
> 
> As a matter of fact, we also allow MMC_PM_KEEP_POWER to become set
> when parsing the DTB via calling mmc_of_parse(). So just changing the
> default value (don't set MMC_PM_KEEP_POWER) for the stm32 variant,
> would do the trick I think.
> 
OK, I'll do that, in mmci.c file, keeping the possibility to have the DT 
property set even for stm32 variant.

> Kind regards
> Uffe
> 
Best regards,
Yann