[PATCH] mmc: mmci: Only call .post_sig_volt_switch if voltage switch happened

[PATCH] mmc: mmci: Only call .post_sig_volt_switch if voltage switch happened

[Marek Vasut]

Call the post voltage switch handler only if the voltage switch actually
happened. It is well possible that the regulator is already set correctly
and no voltage switch happened, so there is no need to take any further
action.

This fixes a real issue on STM32MP1 where, if the eMMC is supplied with
VccQ=1.8 V, the post voltage switch code will spin indefinitelly waiting
for the voltage switch to complete, even though no voltage switch really
happened.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Ludovic Barre <ludovic.barre@st.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-mmc@vger.kernel.org
---
 drivers/mmc/host/mmci.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 647567def612..11c2f417cbe8 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1861,10 +1861,12 @@ static int mmci_get_cd(struct mmc_host *mmc)
 static int mmci_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct mmci_host *host = mmc_priv(mmc);
-	int ret = 0;
+	int old_voltage, new_voltage, ret = 0;
 
 	if (!IS_ERR(mmc->supply.vqmmc)) {
 
+		old_voltage = regulator_get_voltage(mmc->supply.vqmmc);
+
 		switch (ios->signal_voltage) {
 		case MMC_SIGNAL_VOLTAGE_330:
 			ret = regulator_set_voltage(mmc->supply.vqmmc,
@@ -1880,7 +1882,10 @@ static int mmci_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
 			break;
 		}
 
-		if (!ret && host->ops && host->ops->post_sig_volt_switch)
+		new_voltage = regulator_get_voltage(mmc->supply.vqmmc);
+
+		if (!ret && old_voltage != new_voltage &&
+		    host->ops && host->ops->post_sig_volt_switch)
 			ret = host->ops->post_sig_volt_switch(host, ios);
 
 		if (ret)
-- 
2.25.1

Re: [PATCH] mmc: mmci: Only call .post_sig_volt_switch if voltage switch happened

[Ulf Hansson]

On Tue, 31 Mar 2020 at 17:53, Marek Vasut <marex@denx.de> wrote:
>
> Call the post voltage switch handler only if the voltage switch actually
> happened. It is well possible that the regulator is already set correctly
> and no voltage switch happened, so there is no need to take any further
> action.
>
> This fixes a real issue on STM32MP1 where, if the eMMC is supplied with
> VccQ=1.8 V, the post voltage switch code will spin indefinitelly waiting
> for the voltage switch to complete, even though no voltage switch really
> happened.

Whether this is a common problem or not, I think in a first step we
should manage this in the common mmc_regulator_set_vqmmc().

Then on top of that, convert mmci into using the mmc_regulator_set_vqmmc() API.

Can please try this approach instead?

Kind regards
Uffe

>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Ludovic Barre <ludovic.barre@st.com>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> To: linux-mmc@vger.kernel.org
> ---
>  drivers/mmc/host/mmci.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 647567def612..11c2f417cbe8 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -1861,10 +1861,12 @@ static int mmci_get_cd(struct mmc_host *mmc)
>  static int mmci_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
>  {
>         struct mmci_host *host = mmc_priv(mmc);
> -       int ret = 0;
> +       int old_voltage, new_voltage, ret = 0;
>
>         if (!IS_ERR(mmc->supply.vqmmc)) {
>
> +               old_voltage = regulator_get_voltage(mmc->supply.vqmmc);
> +
>                 switch (ios->signal_voltage) {
>                 case MMC_SIGNAL_VOLTAGE_330:
>                         ret = regulator_set_voltage(mmc->supply.vqmmc,
> @@ -1880,7 +1882,10 @@ static int mmci_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
>                         break;
>                 }
>
> -               if (!ret && host->ops && host->ops->post_sig_volt_switch)
> +               new_voltage = regulator_get_voltage(mmc->supply.vqmmc);
> +
> +               if (!ret && old_voltage != new_voltage &&
> +                   host->ops && host->ops->post_sig_volt_switch)
>                         ret = host->ops->post_sig_volt_switch(host, ios);
>
>                 if (ret)
> --
> 2.25.1
>

Re: [PATCH] mmc: mmci: Only call .post_sig_volt_switch if voltage switch happened

[Marek Vasut]

On 3/31/20 8:53 PM, Ulf Hansson wrote:
> On Tue, 31 Mar 2020 at 17:53, Marek Vasut <marex@denx.de> wrote:
>>
>> Call the post voltage switch handler only if the voltage switch actually
>> happened. It is well possible that the regulator is already set correctly
>> and no voltage switch happened, so there is no need to take any further
>> action.
>>
>> This fixes a real issue on STM32MP1 where, if the eMMC is supplied with
>> VccQ=1.8 V, the post voltage switch code will spin indefinitelly waiting
>> for the voltage switch to complete, even though no voltage switch really
>> happened.
> 
> Whether this is a common problem or not, I think in a first step we
> should manage this in the common mmc_regulator_set_vqmmc().

I can pass in a variable which would be set if a voltage switch actually
happened in mmc_regulator_set_vqmmc() OR I can return a code > 0 from
there. Which one do you prefer?

Then I guess we can add something like

if (regulator_get_voltage(...vqmmc) is already in voltage range)
 return 1;

...

and the MMCI driver would do something like

if (mmc_regulator_set_vqmmc(...) > 0)
 host->ops->post_sig_volt_switch(...);

That looks OK I guess ?

> Then on top of that, convert mmci into using the mmc_regulator_set_vqmmc() API.
> 
> Can please try this approach instead?
Sure. Does the above look sane ?

[...]

-- 
Best regards,
Marek Vasut

Re: [PATCH] mmc: mmci: Only call .post_sig_volt_switch if voltage switch happened

[Ulf Hansson]

On Tue, 31 Mar 2020 at 23:01, Marek Vasut <marex@denx.de> wrote:
>
> On 3/31/20 8:53 PM, Ulf Hansson wrote:
> > On Tue, 31 Mar 2020 at 17:53, Marek Vasut <marex@denx.de> wrote:
> >>
> >> Call the post voltage switch handler only if the voltage switch actually
> >> happened. It is well possible that the regulator is already set correctly
> >> and no voltage switch happened, so there is no need to take any further
> >> action.
> >>
> >> This fixes a real issue on STM32MP1 where, if the eMMC is supplied with
> >> VccQ=1.8 V, the post voltage switch code will spin indefinitelly waiting
> >> for the voltage switch to complete, even though no voltage switch really
> >> happened.
> >
> > Whether this is a common problem or not, I think in a first step we
> > should manage this in the common mmc_regulator_set_vqmmc().
>
> I can pass in a variable which would be set if a voltage switch actually
> happened in mmc_regulator_set_vqmmc() OR I can return a code > 0 from
> there. Which one do you prefer?

Return a code > 0.

>
> Then I guess we can add something like
>
> if (regulator_get_voltage(...vqmmc) is already in voltage range)
>  return 1;
>
> ...
>
> and the MMCI driver would do something like
>
> if (mmc_regulator_set_vqmmc(...) > 0)
>  host->ops->post_sig_volt_switch(...);
>
> That looks OK I guess ?
>
> > Then on top of that, convert mmci into using the mmc_regulator_set_vqmmc() API.
> >
> > Can please try this approach instead?
> Sure. Does the above look sane ?

Yes, great!

Kind regards
Uffe

Re: [PATCH] mmc: mmci: Only call .post_sig_volt_switch if voltage switch happened

[Marek Vasut]

On 4/1/20 10:16 AM, Ulf Hansson wrote:
> On Tue, 31 Mar 2020 at 23:01, Marek Vasut <marex@denx.de> wrote:
>>
>> On 3/31/20 8:53 PM, Ulf Hansson wrote:
>>> On Tue, 31 Mar 2020 at 17:53, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> Call the post voltage switch handler only if the voltage switch actually
>>>> happened. It is well possible that the regulator is already set correctly
>>>> and no voltage switch happened, so there is no need to take any further
>>>> action.
>>>>
>>>> This fixes a real issue on STM32MP1 where, if the eMMC is supplied with
>>>> VccQ=1.8 V, the post voltage switch code will spin indefinitelly waiting
>>>> for the voltage switch to complete, even though no voltage switch really
>>>> happened.
>>>
>>> Whether this is a common problem or not, I think in a first step we
>>> should manage this in the common mmc_regulator_set_vqmmc().
>>
>> I can pass in a variable which would be set if a voltage switch actually
>> happened in mmc_regulator_set_vqmmc() OR I can return a code > 0 from
>> there. Which one do you prefer?
> 
> Return a code > 0.
> 
>>
>> Then I guess we can add something like
>>
>> if (regulator_get_voltage(...vqmmc) is already in voltage range)
>>  return 1;
>>
>> ...
>>
>> and the MMCI driver would do something like
>>
>> if (mmc_regulator_set_vqmmc(...) > 0)
>>  host->ops->post_sig_volt_switch(...);
>>
>> That looks OK I guess ?
>>
>>> Then on top of that, convert mmci into using the mmc_regulator_set_vqmmc() API.
>>>
>>> Can please try this approach instead?
>> Sure. Does the above look sane ?
> 
> Yes, great!

The resulting patch doesn't really look all that great, but I sent out a
small series.