| Message ID | 20200331155254.100952-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: mmci: Only call .post_sig_volt_switch if voltage switch happened | expand |
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 >
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 ? [...]
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
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.
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)
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(-)