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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 11 Dec 2015 18:06:17 +0100
From:	Ludovic Desroches <ludovic.desroches@...el.com>
To:	Ulf Hansson <ulf.hansson@...aro.org>
CC:	Jisheng Zhang <jszhang@...vell.com>,
	Ludovic Desroches <ludovic.desroches@...el.com>,
	linux-mmc <linux-mmc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2] mmc: sdhci: restore behavior when setting VDD via
 external regulator

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@...vell.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@...vell.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/