[PATCH v2] mmc: sdhci: restore behavior when setting VDD via external regulator

[PATCH v2] mmc: sdhci: restore behavior when setting VDD via external regulator

[Jisheng Zhang]

After commit 52221610dd84 ("mmc: sdhci: Improve external VDD regulator
support"), for the VDD is supplied via external regulators, we ignore
the code to convert a VDD voltage request into one of the standard
SDHCI voltage levels, then program it in the SDHCI_POWER_CONTROL. This
brings two issues:

1. SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk isn't handled properly any
more.

2. What's more, once SDHCI_POWER_ON bit is set, some controllers such
as the sdhci-pxav3 used in marvell berlin SoCs require the voltage
levels programming in the SDHCI_POWER_CONTROL register, even the VDD
is supplied by external regulator. So the host in marvell berlin SoCs
still works fine after the commit. However, commit 3cbc6123a93d ("mmc:
sdhci: Set SDHCI_POWER_ON with external vmmc") sets the SDHCI_POWER_ON
bit, this would make the host in marvell berlin SoCs won't work any
more with external vmmc.

This patch restores the behavior when setting VDD through external
regulator by moving the call of mmc_regulator_set_ocr() to the end
of sdhci_set_power() function.

After this patch, the sdcard on Marvell Berlin SoC boards work again.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
Fixes: 52221610dd84 ("mmc: sdhci: Improve external VDD ...")
---
Since v1:
 - add more details about why the sdhci-pxav3 used in marvell berlin
   SoCs need this patch.

 drivers/mmc/host/sdhci.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index b48565e..616aa90 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1274,19 +1274,6 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
 	struct mmc_host *mmc = host->mmc;
 	u8 pwr = 0;
 
-	if (!IS_ERR(mmc->supply.vmmc)) {
-		spin_unlock_irq(&host->lock);
-		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
-		spin_lock_irq(&host->lock);
-
-		if (mode != MMC_POWER_OFF)
-			sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
-		else
-			sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
-
-		return;
-	}
-
 	if (mode != MMC_POWER_OFF) {
 		switch (1 << vdd) {
 		case MMC_VDD_165_195:
@@ -1345,6 +1332,12 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
 		if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
 			mdelay(10);
 	}
+
+	if (!IS_ERR(mmc->supply.vmmc)) {
+		spin_unlock_irq(&host->lock);
+		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
+		spin_lock_irq(&host->lock);
+	}
 }
 
 /*****************************************************************************\
-- 
2.6.3

Re: [PATCH v2] mmc: sdhci: restore behavior when setting VDD via external regulator

[Ulf Hansson]

+ Ludovic (We had some discussions around this code recently as well)

On 11 December 2015 at 14:36, Jisheng Zhang <jszhang@marvell.com> wrote:
> After commit 52221610dd84 ("mmc: sdhci: Improve external VDD regulator
> support"), for the VDD is supplied via external regulators, we ignore
> the code to convert a VDD voltage request into one of the standard
> SDHCI voltage levels, then program it in the SDHCI_POWER_CONTROL. This
> brings two issues:
>
> 1. SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk isn't handled properly any
> more.
>
> 2. What's more, once SDHCI_POWER_ON bit is set, some controllers such
> as the sdhci-pxav3 used in marvell berlin SoCs require the voltage
> levels programming in the SDHCI_POWER_CONTROL register, even the VDD
> is supplied by external regulator. So the host in marvell berlin SoCs
> still works fine after the commit. However, commit 3cbc6123a93d ("mmc:
> sdhci: Set SDHCI_POWER_ON with external vmmc") sets the SDHCI_POWER_ON
> bit, this would make the host in marvell berlin SoCs won't work any
> more with external vmmc.
>
> This patch restores the behavior when setting VDD through external
> regulator by moving the call of mmc_regulator_set_ocr() to the end
> of sdhci_set_power() function.
>
> After this patch, the sdcard on Marvell Berlin SoC boards work again.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> Fixes: 52221610dd84 ("mmc: sdhci: Improve external VDD ...")
> ---
> Since v1:
>  - add more details about why the sdhci-pxav3 used in marvell berlin
>    SoCs need this patch.
>
>  drivers/mmc/host/sdhci.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index b48565e..616aa90 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1274,19 +1274,6 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
>         struct mmc_host *mmc = host->mmc;
>         u8 pwr = 0;
>
> -       if (!IS_ERR(mmc->supply.vmmc)) {
> -               spin_unlock_irq(&host->lock);
> -               mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> -               spin_lock_irq(&host->lock);
> -
> -               if (mode != MMC_POWER_OFF)
> -                       sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
> -               else
> -                       sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> -
> -               return;
> -       }
> -
>         if (mode != MMC_POWER_OFF) {
>                 switch (1 << vdd) {
>                 case MMC_VDD_165_195:
> @@ -1345,6 +1332,12 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
>                 if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
>                         mdelay(10);
>         }
> +
> +       if (!IS_ERR(mmc->supply.vmmc)) {
> +               spin_unlock_irq(&host->lock);
> +               mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> +               spin_lock_irq(&host->lock);
> +       }
>  }
>
>  /*****************************************************************************\
> --
> 2.6.3
>

My concern with this patch is that it might fix the problem for your
SDHCI variant, but will break it for others.
I guess we can give it try, unless or until someone reports a problem.

Although, I would like to get Ludovic's input on this change, before I
decide to do anything.

Kind regards
Uffe

RE: [PATCH v2] mmc: sdhci: restore behavior when setting VDD via external regulator

[Jisheng Zhang]

Dear Ulf, Ludovic,

Sorry for top post. I'm under webmail for I have no proper email client now.

OOPS, I should ask mmc maintainers and experts for help early. I spent two
days to debug the issue then found the two problematic commits.

PS: It seems other sdhci-pxav3 users never complained, it's a bit strange ;)

Thanks for reviewing this patch,
Jisheng

________________________________________
From: Ulf Hansson
Sent: Friday, December 11, 2015 22:48
To: Jisheng Zhang; Ludovic Desroches
Cc: linux-mmc; linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2] mmc: sdhci: restore behavior when setting VDD via external regulator

+ Ludovic (We had some discussions around this code recently as well)

On 11 December 2015 at 14:36, Jisheng Zhang <jszhang@marvell.com> wrote:
> After commit 52221610dd84 ("mmc: sdhci: Improve external VDD regulator
> support"), for the VDD is supplied via external regulators, we ignore
> the code to convert a VDD voltage request into one of the standard
> SDHCI voltage levels, then program it in the SDHCI_POWER_CONTROL. This
> brings two issues:
>
> 1. SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk isn't handled properly any
> more.
>
> 2. What's more, once SDHCI_POWER_ON bit is set, some controllers such
> as the sdhci-pxav3 used in marvell berlin SoCs require the voltage
> levels programming in the SDHCI_POWER_CONTROL register, even the VDD
> is supplied by external regulator. So the host in marvell berlin SoCs
> still works fine after the commit. However, commit 3cbc6123a93d ("mmc:
> sdhci: Set SDHCI_POWER_ON with external vmmc") sets the SDHCI_POWER_ON
> bit, this would make the host in marvell berlin SoCs won't work any
> more with external vmmc.
>
> This patch restores the behavior when setting VDD through external
> regulator by moving the call of mmc_regulator_set_ocr() to the end
> of sdhci_set_power() function.
>
> After this patch, the sdcard on Marvell Berlin SoC boards work again.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> Fixes: 52221610dd84 ("mmc: sdhci: Improve external VDD ...")
> ---
> Since v1:
>  - add more details about why the sdhci-pxav3 used in marvell berlin
>    SoCs need this patch.
>
>  drivers/mmc/host/sdhci.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index b48565e..616aa90 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1274,19 +1274,6 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
>         struct mmc_host *mmc = host->mmc;
>         u8 pwr = 0;
>
> -       if (!IS_ERR(mmc->supply.vmmc)) {
> -               spin_unlock_irq(&host->lock);
> -               mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> -               spin_lock_irq(&host->lock);
> -
> -               if (mode != MMC_POWER_OFF)
> -                       sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
> -               else
> -                       sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> -
> -               return;
> -       }
> -
>         if (mode != MMC_POWER_OFF) {
>                 switch (1 << vdd) {
>                 case MMC_VDD_165_195:
> @@ -1345,6 +1332,12 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
>                 if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
>                         mdelay(10);
>         }
> +
> +       if (!IS_ERR(mmc->supply.vmmc)) {
> +               spin_unlock_irq(&host->lock);
> +               mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> +               spin_lock_irq(&host->lock);
> +       }
>  }
>
>  /*****************************************************************************\
> --
> 2.6.3
>

My concern with this patch is that it might fix the problem for your
SDHCI variant, but will break it for others.
I guess we can give it try, unless or until someone reports a problem.

Although, I would like to get Ludovic's input on this change, before I
decide to do anything.

Kind regards
Uffe

Re: [PATCH v2] mmc: sdhci: restore behavior when setting VDD via external regulator

[Ludovic Desroches]

On Fri, Dec 11, 2015 at 03:48:04PM +0100, Ulf Hansson wrote:
> + Ludovic (We had some discussions around this code recently as well)
> 

Thanks Ulf.

> On 11 December 2015 at 14:36, Jisheng Zhang <jszhang@marvell.com> wrote:
> > After commit 52221610dd84 ("mmc: sdhci: Improve external VDD regulator
> > support"), for the VDD is supplied via external regulators, we ignore
> > the code to convert a VDD voltage request into one of the standard
> > SDHCI voltage levels, then program it in the SDHCI_POWER_CONTROL. This
> > brings two issues:
> >
> > 1. SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk isn't handled properly any
> > more.
> >
> > 2. What's more, once SDHCI_POWER_ON bit is set, some controllers such
> > as the sdhci-pxav3 used in marvell berlin SoCs require the voltage
> > levels programming in the SDHCI_POWER_CONTROL register, even the VDD
> > is supplied by external regulator.So the host in marvell berlin SoCs
> > still works fine after the commit.

I am not sure to understand this part. You explain that the controller
in berlin SoC requireis the voltage level programming even if there is an
external regulator for VDD. I agree this part, I am in the same
situation with atmel controller. It is not smart to rely on the voltage
level if we have an external regulator but it follows the sdhci specs.

That I don't understand is that you say it still works fine after this
commit... If you need to set the voltage level in the
SDHCI_POWER_CONTROL register, it is broken by this commit if you declare
an external regulator.

> > However, commit 3cbc6123a93d ("mmc:
> > sdhci: Set SDHCI_POWER_ON with external vmmc") sets the SDHCI_POWER_ON
> > bit, this would make the host in marvell berlin SoCs won't work any
> > more with external vmmc.
> >
> > This patch restores the behavior when setting VDD through external
> > regulator by moving the call of mmc_regulator_set_ocr() to the end
> > of sdhci_set_power() function.
> >
> > After this patch, the sdcard on Marvell Berlin SoC boards work again.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > Fixes: 52221610dd84 ("mmc: sdhci: Improve external VDD ...")
> > ---
> > Since v1:
> >  - add more details about why the sdhci-pxav3 used in marvell berlin
> >    SoCs need this patch.
> >
> >  drivers/mmc/host/sdhci.c | 19 ++++++-------------
> >  1 file changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index b48565e..616aa90 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1274,19 +1274,6 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> >         struct mmc_host *mmc = host->mmc;
> >         u8 pwr = 0;
> >
> > -       if (!IS_ERR(mmc->supply.vmmc)) {
> > -               spin_unlock_irq(&host->lock);
> > -               mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> > -               spin_lock_irq(&host->lock);
> > -
> > -               if (mode != MMC_POWER_OFF)
> > -                       sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
> > -               else
> > -                       sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> > -
> > -               return;
> > -       }
> > -
> >         if (mode != MMC_POWER_OFF) {
> >                 switch (1 << vdd) {
> >                 case MMC_VDD_165_195:
> > @@ -1345,6 +1332,12 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> >                 if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
> >                         mdelay(10);
> >         }
> > +
> > +       if (!IS_ERR(mmc->supply.vmmc)) {
> > +               spin_unlock_irq(&host->lock);
> > +               mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> > +               spin_lock_irq(&host->lock);
> > +       }
> >  }
> >
> >  /*****************************************************************************\
> > --
> > 2.6.3
> >
> 
> My concern with this patch is that it might fix the problem for your
> SDHCI variant, but will break it for others.
> I guess we can give it try, unless or until someone reports a problem.
> 
> Although, I would like to get Ludovic's input on this change, before I
> decide to do anything.
>

I would be pleased to get this patch since it would solve one of my
issues.

Concerning the risk to take this patch. I would say one part of this
patch is safe, the other one maybe not.

Reading the log of commit 52221610dd84, it is not a bug fix. It was done
in this way because it seemed logical to not set the voltage level in
the SDHCI_POWER_CONTROL if we have an external regulator.

Moving mmc_regulator_set_ocr at the end could cause issue since it
changes the sequence order: the regulator is configured after the
SDHCI_POWER_CONTROL register.

Regards

Ludovic

Re: [PATCH v2] mmc: sdhci: restore behavior when setting VDD via external regulator

[Jisheng Zhang]

Dear Ludovic,

On Fri, 11 Dec 2015 18:06:17 +0100 Ludovic Desroches wrote:

> On Fri, Dec 11, 2015 at 03:48:04PM +0100, Ulf Hansson wrote:
> > + Ludovic (We had some discussions around this code recently as well)
> >   
> 
> Thanks Ulf.
> 
> > On 11 December 2015 at 14:36, Jisheng Zhang <jszhang@marvell.com> wrote:  
> > > After commit 52221610dd84 ("mmc: sdhci: Improve external VDD regulator
> > > support"), for the VDD is supplied via external regulators, we ignore
> > > the code to convert a VDD voltage request into one of the standard
> > > SDHCI voltage levels, then program it in the SDHCI_POWER_CONTROL. This
> > > brings two issues:
> > >
> > > 1. SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk isn't handled properly any
> > > more.
> > >
> > > 2. What's more, once SDHCI_POWER_ON bit is set, some controllers such
> > > as the sdhci-pxav3 used in marvell berlin SoCs require the voltage
> > > levels programming in the SDHCI_POWER_CONTROL register, even the VDD
> > > is supplied by external regulator.So the host in marvell berlin SoCs
> > > still works fine after the commit.  
> 
> I am not sure to understand this part. You explain that the controller
> in berlin SoC requireis the voltage level programming even if there is an
> external regulator for VDD. I agree this part, I am in the same

plus one more condition ;) -- "once SDHCI_POWER_ON bit is set", that's to say
either not touching SDHCI_POWER_CONTROL register at all or setting SDHCI_POWER_ON
bit and voltage level at the same time is fine, but the sdhci-pxav3 in berlin
case can't work if we set SDHCI_POWER_ON but don't program the voltage level
, unfortunately this is true after commit 3cbc6123a93d ("mmc: sdhci: Set
SDHCI_POWER_ON with external vmmc")

> situation with atmel controller. It is not smart to rely on the voltage
> level if we have an external regulator but it follows the sdhci specs.
> 
> That I don't understand is that you say it still works fine after this
> commit... If you need to set the voltage level in the
> SDHCI_POWER_CONTROL register, it is broken by this commit if you declare
> an external regulator.

See above, commit 52221610dd84 doesn't break the host controller, it still
works fine after commit 52221610dd84 but the combination of 52221610dd84 and
3cbc6123a93d do break the host controller.

> 
> > > However, commit 3cbc6123a93d ("mmc:
> > > sdhci: Set SDHCI_POWER_ON with external vmmc") sets the SDHCI_POWER_ON
> > > bit, this would make the host in marvell berlin SoCs won't work any
> > > more with external vmmc.
> > >
> > > This patch restores the behavior when setting VDD through external
> > > regulator by moving the call of mmc_regulator_set_ocr() to the end
> > > of sdhci_set_power() function.
> > >
> > > After this patch, the sdcard on Marvell Berlin SoC boards work again.
> > >
> > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > > Fixes: 52221610dd84 ("mmc: sdhci: Improve external VDD ...")
> > > ---
> > > Since v1:
> > >  - add more details about why the sdhci-pxav3 used in marvell berlin
> > >    SoCs need this patch.
> > >
> > >  drivers/mmc/host/sdhci.c | 19 ++++++-------------
> > >  1 file changed, 6 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > > index b48565e..616aa90 100644
> > > --- a/drivers/mmc/host/sdhci.c
> > > +++ b/drivers/mmc/host/sdhci.c
> > > @@ -1274,19 +1274,6 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> > >         struct mmc_host *mmc = host->mmc;
> > >         u8 pwr = 0;
> > >
> > > -       if (!IS_ERR(mmc->supply.vmmc)) {
> > > -               spin_unlock_irq(&host->lock);
> > > -               mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> > > -               spin_lock_irq(&host->lock);
> > > -
> > > -               if (mode != MMC_POWER_OFF)
> > > -                       sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
> > > -               else
> > > -                       sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> > > -
> > > -               return;
> > > -       }
> > > -
> > >         if (mode != MMC_POWER_OFF) {
> > >                 switch (1 << vdd) {
> > >                 case MMC_VDD_165_195:
> > > @@ -1345,6 +1332,12 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> > >                 if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
> > >                         mdelay(10);
> > >         }
> > > +
> > > +       if (!IS_ERR(mmc->supply.vmmc)) {
> > > +               spin_unlock_irq(&host->lock);
> > > +               mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> > > +               spin_lock_irq(&host->lock);
> > > +       }
> > >  }
> > >
> > >  /*****************************************************************************\
> > > --
> > > 2.6.3
> > >  
> > 
> > My concern with this patch is that it might fix the problem for your
> > SDHCI variant, but will break it for others.
> > I guess we can give it try, unless or until someone reports a problem.
> > 
> > Although, I would like to get Ludovic's input on this change, before I
> > decide to do anything.
> >  
> 
> I would be pleased to get this patch since it would solve one of my
> issues.
> 
> Concerning the risk to take this patch. I would say one part of this
> patch is safe, the other one maybe not.
> 
> Reading the log of commit 52221610dd84, it is not a bug fix. It was done
> in this way because it seemed logical to not set the voltage level in
> the SDHCI_POWER_CONTROL if we have an external regulator.
> 
> Moving mmc_regulator_set_ocr at the end could cause issue since it
> changes the sequence order: the regulator is configured after the
> SDHCI_POWER_CONTROL register.

hmmm, this sequence order is the same as the one before commit 52221610dd84.
IOW, the patch restores the old sequence order: the regulator is configured
after the SDHCI_POWER_CONTROL register.

Thanks for reviewing,
Jisheng

Re: [PATCH v2] mmc: sdhci: restore behavior when setting VDD via external regulator

[Ludovic Desroches]

Hi Ulf, Jisheng,

On Fri, Dec 11, 2015 at 03:48:04PM +0100, Ulf Hansson wrote:
> + Ludovic (We had some discussions around this code recently as well)
> 
> On 11 December 2015 at 14:36, Jisheng Zhang <jszhang@marvell.com> wrote:
> > After commit 52221610dd84 ("mmc: sdhci: Improve external VDD regulator
> > support"), for the VDD is supplied via external regulators, we ignore
> > the code to convert a VDD voltage request into one of the standard
> > SDHCI voltage levels, then program it in the SDHCI_POWER_CONTROL. This
> > brings two issues:
> >
> > 1. SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk isn't handled properly any
> > more.
> >
> > 2. What's more, once SDHCI_POWER_ON bit is set, some controllers such
> > as the sdhci-pxav3 used in marvell berlin SoCs require the voltage
> > levels programming in the SDHCI_POWER_CONTROL register, even the VDD
> > is supplied by external regulator. So the host in marvell berlin SoCs
> > still works fine after the commit. However, commit 3cbc6123a93d ("mmc:
> > sdhci: Set SDHCI_POWER_ON with external vmmc") sets the SDHCI_POWER_ON
> > bit, this would make the host in marvell berlin SoCs won't work any
> > more with external vmmc.
> >
> > This patch restores the behavior when setting VDD through external
> > regulator by moving the call of mmc_regulator_set_ocr() to the end
> > of sdhci_set_power() function.
> >
> > After this patch, the sdcard on Marvell Berlin SoC boards work again.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > Fixes: 52221610dd84 ("mmc: sdhci: Improve external VDD ...")

Reviewed-by: Ludovic Desroches <ludovic.desroches@atmel.com>
Tested-by: Ludovic Desroches <ludovic.desroches@atmel.com>

Even if the patch sounds good for me, I wanted to test it. As planned,
with this patch, I can describe my vcc regulator without breaking the
behavior of my sdhci controller.


Regards

Ludovic

> > ---
> > Since v1:
> >  - add more details about why the sdhci-pxav3 used in marvell berlin
> >    SoCs need this patch.
> >
> >  drivers/mmc/host/sdhci.c | 19 ++++++-------------
> >  1 file changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index b48565e..616aa90 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1274,19 +1274,6 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> >         struct mmc_host *mmc = host->mmc;
> >         u8 pwr = 0;
> >
> > -       if (!IS_ERR(mmc->supply.vmmc)) {
> > -               spin_unlock_irq(&host->lock);
> > -               mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> > -               spin_lock_irq(&host->lock);
> > -
> > -               if (mode != MMC_POWER_OFF)
> > -                       sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL);
> > -               else
> > -                       sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> > -
> > -               return;
> > -       }
> > -
> >         if (mode != MMC_POWER_OFF) {
> >                 switch (1 << vdd) {
> >                 case MMC_VDD_165_195:
> > @@ -1345,6 +1332,12 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
> >                 if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
> >                         mdelay(10);
> >         }
> > +
> > +       if (!IS_ERR(mmc->supply.vmmc)) {
> > +               spin_unlock_irq(&host->lock);
> > +               mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
> > +               spin_lock_irq(&host->lock);
> > +       }
> >  }
> >
> >  /*****************************************************************************\
> > --
> > 2.6.3
> >
> 
> My concern with this patch is that it might fix the problem for your
> SDHCI variant, but will break it for others.
> I guess we can give it try, unless or until someone reports a problem.
> 
> Although, I would like to get Ludovic's input on this change, before I
> decide to do anything.
> 
> Kind regards
> Uffe

Re: [PATCH v2] mmc: sdhci: restore behavior when setting VDD via external regulator

[Ulf Hansson]

On 18 December 2015 at 09:11, Ludovic Desroches
<ludovic.desroches@atmel.com> wrote:
> Hi Ulf, Jisheng,
>
> On Fri, Dec 11, 2015 at 03:48:04PM +0100, Ulf Hansson wrote:
>> + Ludovic (We had some discussions around this code recently as well)
>>
>> On 11 December 2015 at 14:36, Jisheng Zhang <jszhang@marvell.com> wrote:
>> > After commit 52221610dd84 ("mmc: sdhci: Improve external VDD regulator
>> > support"), for the VDD is supplied via external regulators, we ignore
>> > the code to convert a VDD voltage request into one of the standard
>> > SDHCI voltage levels, then program it in the SDHCI_POWER_CONTROL. This
>> > brings two issues:
>> >
>> > 1. SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk isn't handled properly any
>> > more.
>> >
>> > 2. What's more, once SDHCI_POWER_ON bit is set, some controllers such
>> > as the sdhci-pxav3 used in marvell berlin SoCs require the voltage
>> > levels programming in the SDHCI_POWER_CONTROL register, even the VDD
>> > is supplied by external regulator. So the host in marvell berlin SoCs
>> > still works fine after the commit. However, commit 3cbc6123a93d ("mmc:
>> > sdhci: Set SDHCI_POWER_ON with external vmmc") sets the SDHCI_POWER_ON
>> > bit, this would make the host in marvell berlin SoCs won't work any
>> > more with external vmmc.
>> >
>> > This patch restores the behavior when setting VDD through external
>> > regulator by moving the call of mmc_regulator_set_ocr() to the end
>> > of sdhci_set_power() function.
>> >
>> > After this patch, the sdcard on Marvell Berlin SoC boards work again.
>> >
>> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>> > Fixes: 52221610dd84 ("mmc: sdhci: Improve external VDD ...")
>
> Reviewed-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> Tested-by: Ludovic Desroches <ludovic.desroches@atmel.com>
>
> Even if the patch sounds good for me, I wanted to test it. As planned,
> with this patch, I can describe my vcc regulator without breaking the
> behavior of my sdhci controller.
>

Okay! According to the reasoning and testing, this change seems like a
good idea!

I have now queued the v2 for next.

Thanks!

Kind regards
Uffe

Re: [PATCH v2] mmc: sdhci: restore behavior when setting VDD via external regulator

[Adrian Hunter]

On 18/12/15 11:55, Ulf Hansson wrote:
> On 18 December 2015 at 09:11, Ludovic Desroches
> <ludovic.desroches@atmel.com> wrote:
>> Hi Ulf, Jisheng,
>>
>> On Fri, Dec 11, 2015 at 03:48:04PM +0100, Ulf Hansson wrote:
>>> + Ludovic (We had some discussions around this code recently as well)
>>>
>>> On 11 December 2015 at 14:36, Jisheng Zhang <jszhang@marvell.com> wrote:
>>>> After commit 52221610dd84 ("mmc: sdhci: Improve external VDD regulator
>>>> support"), for the VDD is supplied via external regulators, we ignore
>>>> the code to convert a VDD voltage request into one of the standard
>>>> SDHCI voltage levels, then program it in the SDHCI_POWER_CONTROL. This
>>>> brings two issues:
>>>>
>>>> 1. SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk isn't handled properly any
>>>> more.

For the record, the way it was working made more sense to me i.e. if you
have control of an external regulator then you know the power is not
disrupted by runtime suspend.

Also AFAIK Intel is the only user of SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON, so I
am surprised you listed it as an issue that you have.