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

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: Thu, 23 Apr 2020 10:12:03 +0200	[thread overview]
Message-ID: <96725b28-fa19-54f0-7ba7-2043098a24a5@st.com> (raw)
In-Reply-To: <CAPDyKFpri4VBnH9nbqUa4L=3o_h+fSZ052v7AG_9MhJX2gKgCQ@mail.gmail.com>

Le 4/22/20 à 6:03 PM, Ulf Hansson a écrit :
> On Wed, 22 Apr 2020 at 15:40, Ludovic BARRE <ludovic.barre@st.com> wrote:
>>
>> 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.
> 
> Thanks for the detailed information, much appreciated!
> 
>>
>>>>
>>>> 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).
> 
> Yep, thanks!
> 
>>
>>>>>
>>>>> 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.
> 
> So, it seems like you need to use the ->set_ios() callback, to
> re-configure the controller correctly.
> 
> Just tell if you need more help to make that work, otherwise I am here
> to review your patches.
> 
> In regards to $subject patch, I have applied it for next, thanks!

Thanks for your review.
Have a nice day.

> 
> Kind regards
> Uffe
> 