Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200420161831.5043-1-ludovic.barre@st.com>
In-Reply-To: <20200420161831.5043-1-ludovic.barre@st.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Tue, 21 Apr 2020 11:25:33 +0200
Message-ID: <CAPDyKFqC3fdnQ9CMYhS-=5MiCET=r5Az2S5oFoA2v1gdDeGO3w@mail.gmail.com>
Subject: Re: [PATCH] mmc: mmci_sdmmc: fix power on issue due to pwr_reg initialization
To:     Ludovic Barre <ludovic.barre@st.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Srini Kandagatla <srinivas.kandagatla@linaro.org>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        Alexandre Torgue <alexandre.torgue@st.com>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        DTML <devicetree@vger.kernel.org>,
        "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
        linux-stm32@st-md-mailman.stormreply.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 20 Apr 2020 at 18:18, Ludovic Barre <ludovic.barre@st.com> 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?

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

>
> 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@st.com>

Besides the comment, the code and the approach seems reasonable to me.

Kind regards
Uffe

> ---
>
> This patch is the proposal from:
> https://patchwork.kernel.org/patch/11457987/
>
> ---
>  drivers/mmc/host/mmci_stm32_sdmmc.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c
> index d33e62bd6153..14f99d8aa3f0 100644
> --- a/drivers/mmc/host/mmci_stm32_sdmmc.c
> +++ b/drivers/mmc/host/mmci_stm32_sdmmc.c
> @@ -519,6 +519,7 @@ void sdmmc_variant_init(struct mmci_host *host)
>         struct sdmmc_dlyb *dlyb;
>
>         host->ops = &sdmmc_variant_ops;
> +       host->pwr_reg = readl_relaxed(host->base + MMCIPOWER);
>
>         base_dlyb = devm_of_iomap(mmc_dev(host->mmc), np, 1, NULL);
>         if (IS_ERR(base_dlyb))
> --
> 2.17.1
>