Re: [PATCH v2] mmc: mmci: manage MMC_PM_KEEP_POWER per variant config

[Yann Gautier]

On 3/15/22 10:07, Ulf Hansson wrote:
> On Mon, 14 Mar 2022 at 17:52, Yann Gautier <yann.gautier@xxxxxxxxxxx> wrote:
> > On 3/14/22 17:18, Ulf Hansson wrote:
> > > On Mon, 14 Mar 2022 at 15:34, Yann Gautier <yann.gautier@xxxxxxxxxxx> wrote:
> > > > On 3/14/22 14:03, Ulf Hansson wrote:
> > > > > On Mon, 14 Mar 2022 at 13:56, Yann Gautier <yann.gautier@xxxxxxxxxxx> wrote:
> > > > > > Add a disable_keep_power field in variant_data struct. The
> > > > > > MMC_PM_KEEP_POWER flag will be enabled if disable_keep_power is not set.
> > > > > > It is only set to true for stm32_sdmmc variants.
> > > > > >
> > > > > > The issue was seen on STM32MP157C-DK2 board, which embeds a wifi chip.
> > > > > > It doesn't correctly support low power on this board. The Wifi chip
> > > > > > awaits an always-on regulator, but it was connected to v3v3 which is off
> > > > > > in low-power sequence. MMC_PM_KEEP_POWER should then be disabled.
> > > > >
> > > > > Just to make sure I get this correct.
> > > > >
> > > > > Why can't the regulator stay on during system suspend? The point is,
> > > > > we don't need an always on regulator to cope with this.
> > > > >
> > > > > Kind regards
> > > > > Uffe
> > > >
> > > > Hi Ulf,
> > > >
> > > > This v3v3 regulator powers most of the devices on this board. So we need
> > > > to switch it off to gain power in suspend mode.
> > >
> > > I see. Thanks for sharing that information.
> > >
> > > The MMC_PM_KEEP_POWER flag is there to describe what is supported by
> > > the platform/host. It doesn't mean that the card *must* stay powered
> > > on during system suspend. Instead that depends on whether system
> > > wakeup for the SDIO/WiFi is supported too - and if that is enabled by
> > > userspace. If not, the regulator will be turned off for the SDIO card
> > > during system suspend.
> > >
> > > Assuming the regulator is implemented as a proper regulator and can
> > > remain on during system suspend, the right thing would be to keep the
> > > MMC_PM_KEEP_POWER flag around.
> > >
> > > Kind regards
> > > Uffe
> >
> > OK, but in the wifi driver we use on this platform (brcmfmac), the
> > suspend/resume functions (brcmf_ops_sdio_suspend/brcmf_ops_sdio_resume)
> > use the flag to check regu was off, and then call probe function during
> > resume, to re-init Wifi chip and reload its firmware.
>
> I had a closer look at the brcmfmac driver, thanks for the pointers.
>
> In my opinion, I think we should change the brcmfmac driver, so it
> decides to power off the SDIO card, unless the WiFi chip is configured
> to serve us with system wakeups.
>
> I can send a patch for brcmfmac that we can try, unless you want to
> send it yourself?

Hi Ulf,
If you already have an idea of the patch in the brcmfmac driver, can you 
propose something?
We'll be able to test it at our side.

Thanks,
Yann

> > Best regards,
> > Yann
>
> Kind regards
> Uffe
> >>
> > > > Yann
> > > >
> > > > > > The flag can still be enabled through DT property:
> > > > > > keep-power-in-suspend.
> > > > > >
> > > > > > Signed-off-by: Yann Gautier <yann.gautier@xxxxxxxxxxx>
> > > > > > ---
> > > > > > Update in v2:
> > > > > > Reword commit message to better explain the issue.
> > > > > >
> > > > > > Resend the patch alone. It was previoulsy in a series [1] for which the
> > > > > > other patches will be reworked.
> > > > > >
> > > > > > [1] https://lore.kernel.org/lkml/20220304135134.47827-1-yann.gautier@xxxxxxxxxxx/
> > > > > >
> > > > > >     drivers/mmc/host/mmci.c | 5 ++++-
> > > > > >     drivers/mmc/host/mmci.h | 1 +
> > > > > >     2 files changed, 5 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> > > > > > index 45b8608c935c..0e2f2f5d6a52 100644
> > > > > > --- a/drivers/mmc/host/mmci.c
> > > > > > +++ b/drivers/mmc/host/mmci.c
> > > > > > @@ -274,6 +274,7 @@ static struct variant_data variant_stm32_sdmmc = {
> > > > > >            .busy_detect            = true,
> > > > > >            .busy_detect_flag       = MCI_STM32_BUSYD0,
> > > > > >            .busy_detect_mask       = MCI_STM32_BUSYD0ENDMASK,
> > > > > > +       .disable_keep_power     = true,
> > > > > >            .init                   = sdmmc_variant_init,
> > > > > >     };
> > > > > >
> > > > > > @@ -301,6 +302,7 @@ static struct variant_data variant_stm32_sdmmcv2 = {
> > > > > >            .busy_detect            = true,
> > > > > >            .busy_detect_flag       = MCI_STM32_BUSYD0,
> > > > > >            .busy_detect_mask       = MCI_STM32_BUSYD0ENDMASK,
> > > > > > +       .disable_keep_power     = true,
> > > > > >            .init                   = sdmmc_variant_init,
> > > > > >     };
> > > > > >
> > > > > > @@ -2172,7 +2174,8 @@ static int mmci_probe(struct amba_device *dev,
> > > > > >            host->stop_abort.flags = MMC_RSP_R1B | MMC_CMD_AC;
> > > > > >
> > > > > >            /* We support these PM capabilities. */
> > > > > > -       mmc->pm_caps |= MMC_PM_KEEP_POWER;
> > > > > > +       if (!variant->disable_keep_power)
> > > > > > +               mmc->pm_caps |= MMC_PM_KEEP_POWER;
> > > > > >
> > > > > >            /*
> > > > > >             * We can do SGIO
> > > > > > diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> > > > > > index e1a9b96a3396..2cad1ef9766a 100644
> > > > > > --- a/drivers/mmc/host/mmci.h
> > > > > > +++ b/drivers/mmc/host/mmci.h
> > > > > > @@ -361,6 +361,7 @@ struct variant_data {
> > > > > >            u32                     opendrain;
> > > > > >            u8                      dma_lli:1;
> > > > > >            u32                     stm32_idmabsize_mask;
> > > > > > +       u8                      disable_keep_power:1;
> > > > > >            void (*init)(struct mmci_host *host);
> > > > > >     };
> > > > > >
> > > > > > --
> > > > > > 2.25.1