From: Ludovic BARRE <ludovic.barre@st.com>
To: Ulf Hansson <ulf.hansson@linaro.org>
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>
Subject: Re: [PATCH] mmc: mmci_sdmmc: fix power on issue due to pwr_reg initialization
Date: Wed, 22 Apr 2020 15:40:05 +0200 [thread overview]
Message-ID: <1d9cefd1-aaed-1eb5-92f2-b1f45b4da2ac@st.com> (raw)
In-Reply-To: <CAPDyKFrHcoVd=GKPB70gOFE8STOnTJrJbcZzE_DEgFWh1Vhszg@mail.gmail.com>
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@linaro.org> wrote:
>>
>> 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?
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@st.com>
>>
>> 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
>
next prev parent reply other threads:[~2020-04-22 13:40 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-20 16:18 [PATCH] mmc: mmci_sdmmc: fix power on issue due to pwr_reg initialization Ludovic Barre
2020-04-21 9:25 ` Ulf Hansson
2020-04-21 9:38 ` Ulf Hansson
2020-04-22 13:40 ` Ludovic BARRE [this message]
2020-04-22 16:03 ` Ulf Hansson
2020-04-23 8:12 ` Ludovic BARRE
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1d9cefd1-aaed-1eb5-92f2-b1f45b4da2ac@st.com \
--to=ludovic.barre@st.com \
--cc=alexandre.torgue@st.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=linux-stm32@st-md-mailman.stormreply.com \
--cc=mcoquelin.stm32@gmail.com \
--cc=robh+dt@kernel.org \
--cc=srinivas.kandagatla@linaro.org \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).