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".
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 <[email protected]>
---
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
On Mon, 20 Apr 2020 at 18:18, Ludovic Barre <[email protected]> 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 <[email protected]>
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
>
On Tue, 21 Apr 2020 at 11:25, Ulf Hansson <[email protected]> wrote:
>
> On Mon, 20 Apr 2020 at 18:18, Ludovic Barre <[email protected]> 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 <[email protected]>
>
> 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?
[...]
Kind regards
Uffe
hi Ulf
Le 4/21/20 à 11:38 AM, Ulf Hansson a écrit :
> On Tue, 21 Apr 2020 at 11:25, Ulf Hansson <[email protected]> wrote:
>>
>> On Mon, 20 Apr 2020 at 18:18, Ludovic Barre <[email protected]> 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 <[email protected]>
>>
>> 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
>
On Wed, 22 Apr 2020 at 15:40, Ludovic BARRE <[email protected]> wrote:
>
> hi Ulf
>
> Le 4/21/20 à 11:38 AM, Ulf Hansson a écrit :
> > On Tue, 21 Apr 2020 at 11:25, Ulf Hansson <[email protected]> wrote:
> >>
> >> On Mon, 20 Apr 2020 at 18:18, Ludovic Barre <[email protected]> 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 <[email protected]>
> >>
> >> 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!
Kind regards
Uffe
Le 4/22/20 à 6:03 PM, Ulf Hansson a écrit :
> On Wed, 22 Apr 2020 at 15:40, Ludovic BARRE <[email protected]> wrote:
>>
>> hi Ulf
>>
>> Le 4/21/20 à 11:38 AM, Ulf Hansson a écrit :
>>> On Tue, 21 Apr 2020 at 11:25, Ulf Hansson <[email protected]> wrote:
>>>>
>>>> On Mon, 20 Apr 2020 at 18:18, Ludovic Barre <[email protected]> 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 <[email protected]>
>>>>
>>>> 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
>