Re: [PATCH] mmc: mmci_sdmmc: fix power on issue due to pwr_reg initialization

[Ludovic BARRE]

hi Ulf

Le 4/21/20 Ã 11:38 AM, Ulf Hansson a ÃcritÂ:
> On Tue, 21 Apr 2020 at 11:25, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> > On Mon, 20 Apr 2020 at 18:18, Ludovic Barre <ludovic.barre@xxxxxx> wrote:
> > > This patch fix a power-on issue, and avoid to retry the power sequence.
> > >
> > > In power off sequence: sdmmc must set pwr_reg in "power-cycle" state
> > > (value 0x2), to prevent the card from being supplied through the signal
> > > lines (all the lines are driven low).
> > >
> > > In power on sequence: when the power is stable, sdmmc must set pwr_reg
> > > in "power-off" state (value 0x0) to drive all signal to high before to
> > > set "power-on".
> >
> > Just a question to gain further understanding.
> >
> > Let's assume that the controller is a power-on state, because it's
> > been initialized by the boot loader. When the mmc core then starts the
> > power-on sequence (not doing a power-off first), would $subject patch
> > then cause the
> > MMCIPOWER to remain as is, or is it going to be overwritten?

On sdmmc controller, the PWRCTRL[1:0] field of MMCIPOWER register allow
to manage sd lines and has a specific bahavior.

PWRCTRL value:
 - 0x0: After reset, Reset: the SDMMC is disabled and the clock to the
        Card is stopped, SDMMC_D[7:0], and SDMMC_CMD are HiZ and
        SDMMC_CK is driven low.
        When written 00, power-off: the SDMMC is disabled and the clock
        to the card is stopped, SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK
        are driven high.

 - 0x2: Power-cycle, the SDMMC is disabled and the clock to the card is
        stopped, SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK are driven low.

 - 0x3: Power-on: the card is clocked, The first 74 SDMMC_CK cycles the
        SDMMC is still disabled. After the 74 cycles the SDMMC is
        enabled and the SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK are
        controlled according the SDMMC operation.
        **Any further write will be ignored, PWRCTRL value
        will keep 0x3**. when the SDMMC is ON (0x3) only a reset could
        change pwrctrl value and the state of sdmmc lines.

So if the lines are already "ON", the power-on sequence (decribed in
commit message) not overwrite the pwctrl field and not disturb the sdmmc 
lines.

> > I am a little worried that we may start to rely on boot loader
> > conditions, which isn't really what we want either...

We not depend of boot loader conditions.

This patch simply allows to drive high the sd lines before to set
"power-on" value (no effect if already power ON).

> > > To avoid writing the same value to the power register several times, this
> > > register is cached by the pwr_reg variable. At probe pwr_reg is initialized
> > > to 0 by kzalloc of mmc_alloc_host.
> > >
> > > Like pwr_reg value is 0 at probing, the power on sequence fail because
> > > the "power-off" state is not writes (value 0x0) and the lines
> > > remain drive to low.
> > >
> > > This patch initializes "pwr_reg" variable with power register value.
> > > This it done in sdmmc variant init to not disturb default mmci behavior.
> > >
> > > Signed-off-by: Ludovic Barre <ludovic.barre@xxxxxx>
> >
> > Besides the comment, the code and the approach seems reasonable to me.
>
> Another related question. I just realized why you probably haven't set
> .pwrreg_nopower for the variant_stm32_sdmmc and variant_stm32_sdmmcv2.
>
> I guess it's because you need a slightly different way to restore the
> context of MMCIPOWER register at ->runtime_resume(), rather than just
> re-writing it with the saved register values. Is this something that
> you are looking into as well?

Yes exactly, the sequence is slightly different. I can't write 0 on 
mmci_runtime_suspend, and can't just re-writing the saved register.

Regards
Ludo

> [...]
>
> Kind regards
> Uffe