[PATCH 04/23] mmc: sdhci: re-factor sdhci_start_signal_voltage()

[Dong Aisheng]

On Fri, Apr 29, 2016 at 10:32:36AM +0300, Adrian Hunter wrote:
> On 28/04/16 17:28, Dong Aisheng wrote:
> > On Thu, Apr 28, 2016 at 04:36:25PM +0300, Adrian Hunter wrote:
> >> On 28/04/16 16:14, Dong Aisheng wrote:
> >>> On Thu, Apr 28, 2016 at 09:39:54AM +0300, Adrian Hunter wrote:
> >>>> On 28/04/16 06:09, Dong Aisheng wrote:
> >>>>> On Wed, Apr 27, 2016 at 11:26:52PM +0300, Adrian Hunter wrote:
> >>>>>> On 24/04/2016 12:14 p.m., Dong Aisheng wrote:
> >>>>>>> Hi Adrian,
> >>>>>>>
> >>>>>>> Thanks for the review first.
> >>>>>>>
> >>>>>>> On Fri, Apr 22, 2016 at 7:43 PM, Adrian Hunter <adrian.hunter at intel.com> wrote:
> >>>>>>>> On 15/04/16 20:29, Dong Aisheng wrote:
> >>>>>>>>> Handle host and regulator signal voltage switch separately.
> >>>>>>>>> Move host signal voltage switch code into a separated function
> >>>>>>>>> sdhci_do_signal_voltage_switch() first, the following patches will
> >>>>>>>>> remove the regulator voltage switch code and use the common
> >>>>>>>>> mmc_regulator_set_vqmmc() instead.
> >>>>>>>>
> >>>>>>>> You have changed the order that things are done.
> >>>>>>>
> >>>>>>> Yes, the oder changes a bit that we always do controller voltage switch first.
> >>>>>>> I suppose the order is irrelevant here since i don't recall any
> >>>>>>> requirement from card.
> >>>>>>>
> >>>>>>> Actually the original order is also a bit mass.
> >>>>>>> e.g.
> >>>>>>> For MMC_SIGNAL_VOLTAGE_330, switch controller first, then vqmmc.
> >>>>>>> But for MMC_SIGNAL_VOLTAGE_180, switch vqmmc first, then controller.
> >>>>>>> It looks to us the original one also order irrelevant.
> >>>>>>>
> >>>>>>>> There is no way to know
> >>>>>>>> what that will break, so let's not do that.  What about just changing
> >>>>>>>> regulator_set_voltage() to mmc_regulator_set_vqmmc()?
> >>>>>>>>
> >>>>>>>
> >>>>>>> Currently what i can think out VIO switch using are three cases: (Pls
> >>>>>>> help add if any)
> >>>>>>> 1) Both host IO and card IO use external vqmmc to do switch
> >>>>>>> (e.g eMMC 1.8V DDR/HS200/HS400 mode)
> >>>>>>>
> >>>>>>> eMMC has no IO voltage switch protocol and requirement, so usually
> >>>>>>> board designed
> >>>>>>> using fixed 1.8V for eMMC and host IO.
> >>>>>>> Event it's switchable, it should be done in the first mmc_power_up().
> >>>>>>> Dynamical switch later may cause eMMC unable to work properly.
> >>>>>>> (We have been confirmed about this issue by many eMMC vendors
> >>>>>>> like Micron and Sandisk. I'm not sure if any exceptions in the community
> >>>>>>> still doing VIO dynamical switch for eMMC, if yes, please help share
> >>>>>>> the experience!).
> >>>>>>>
> >>>>>>> Event some people still do dynamical IO switch for eMMC, since eMMC
> >>>>>>> spec has no requirement, so the order should also not care.
> >>>>>>>
> >>>>>>> 2) Host using controller IO switch while card using standard CMD (SD/SDIO3.0)
> >>>>>>>
> >>>>>>> SD/SDIO 3.0 spec defines the standard IO switch process and using it's internal
> >>>>>>> regulator to do card IO voltage switch. It does not use external vqmmc
> >>>>>>> regulator.
> >>>>>>> So order irrelevant too.
> >>>>>>>
> >>>>>>> 3) Host using controller IO switch while card using external vqmmc
> >>>>>>> (special SDIO3.0 or eMMC)
> >>>>>>> I have met some special SDIO3.0 card like Broadcom WiFi which does not follow
> >>>>>>> the spec and using external regulator for card IO voltage.
> >>>>>>> Usually it's required to fix to 1.8v and also not order irrelevant.
> >>>>>>>
> >>>>>>> For eMMC, refer to case 1), it should be fixed to 1.8v at power up.
> >>>>>>>
> >>>>>>> So it looks all cases seems are not order required.
> >>>>>>
> >>>>>> I don't agree that there is any way to know that other host controllers
> >>>>>> are not affected.  I don't want a repeat of sdhci_set_power().
> >>>>>>
> >>>>>
> >>>>> Can you share some more info about sdhci_set_power() issue?
> >>>>> I'd like to see if we are same the issue.
> >>>>
> >>>> Not the same issue, but the same concept.  People changing the code under
> >>>> the impression that their way was correct, and then breaking other people's
> >>>> drivers.  Check the git history and mailing list.
> >>>>
> >>>> 	http://marc.info/?l=linux-mmc&m=145880454106474&w=2
> >>>>
> >>>
> >>> Yes, now i understand your concern.
> >>>
> >>>>>
> >>>>> BTW, IMHO i don't think we should stop keep moving only afraid of potential
> >>>>> break if it's correct way. Because .start_signal_voltage_switch() interface
> >>>>> seems shouldn't be order dependant.
> >>>>> If it is, then it should be fixed and handled in high layer like MMC core
> >>>>> rather than in host driver. Right?
> >>>>
> >>>> The SDHCI spec. does not define how to use external regulators, so there is
> >>>> no "correct way".
> >>>>
> >>>
> >>> The "correct way" i mean here is .start_signal_voltage_switch() shouldn't be
> >>> order dependant, would you agree?
> >>
> >> No.  There is no way to know if the regulator must be switched before or
> >> after the host controller register is changed.
> >>
> > 
> > Hmm... If there is no way to know the correct order, how can we
> > assume the exist order is correct?
> 
> There is no correct order.  This is outside the SDHCI spec. and so belongs
> to individual drivers.
> 
> If it mattered we could push the ugly code down onto the drivers, and then
> driver maintainers could opt to use the new pretty code.  However at the
> moment there is a lot more important work, so I would want to avoid that
> code churn.
> 
> > And i already pointed out, the exist order is also confused that
> > it switch controller first then vqmmc for 3.3v and switch vqmmc
> > first, then controller for 1.8v
> > 
> > If we can't sure the exist order is correct, why do we block
> > the changing to correctly use mmc_regulator_set_vqmmc() to
> > improve the driver stability?
> 
> Not sure what you mean here.  I have already showed how we can use
> mmc_regulator_set_vqmmc().
> 
> > 
> > And actually the change is not made arbitrarily, i already list
> > all possibilities based on my knowledge. People can raise more
> > if any.
> 
> You are assuming every driver has a maintainer and every maintainer is
> following this thread, and understands how it might affect all the
> different versions of their hardware.  That is extremely unlikely.
> 

Yes, we can't sure it.
All we can do on breaking old rules is do it early and test people.
Potential break shouldn't block us to going forward IMHO.
But i understand at this time you may not want it.

> > The target is correct that we make start_signal_voltage_switch()
> > order independant.
> > It's worth a try even there's a potential very low possibility
> > break IMHO.
> 
> And so we disagree.
> 
> Aren't your needs met by changing to use mmc_regulator_set_vqmmc() the way I
> suggested?
> 

Well, although i'm not in favor of that way.
But as you insist, i will re-cook the patch to fix the issue first.

Regards
Dong Aisheng

> > 
> > Or else if we can find a better way to switch to
> > mmc_regulator_set_vqmmc i would also love to try.
> >  
> > Maybe we need more people's thought on it!
> > 
> > Ulf,
> > Would you give some inputs?
> > 
> > Regards
> > Dong Aisheng
> > 
> >>>
> >>>> We have to move forward *and* avoid potential breakage.
> >>>>
> >>>
> >>> If really break happens, fix platform driver, not common SDHCI.
> >>> That's the same thing you done for sdhci_set_power().
> >>
> >> In that case the original behaviour was kept in the common SDHCI code and
> >> the driver had to provide its own way.
> >>
> >>>
> >>>> In this case it seems me that the risk of breakage outweighs the value of
> >>>> prettier code.
> >>>>
> >>>
> >>> Actually my main purpose is patch 6: using mmc_regulator_set_vqmmc()
> >>> which is worth and it does improve the stability and eliminate the
> >>> potential signal issue.
> >>> However it's not the same way as you proposed.
> >>> See below.
> >>>
> >>>> By the way, there are ways to get rid of the ugliness - such as pushing it down
> >>>> into individual drivers.
> >>>>
> >>>>>
> >>>>>> Please instead send a patch for just using mmc_regulator_set_vqmmc()
> >>>>>> in place of regulator_set_voltage().
> >>>>>
> >>>>> Just using mmc_regulator_set_vqmmc() also changes the order which
> >>>>> is the same situation.
> >>>>
> >>>> How so?  It looks like a drop-in replacement to me:
> >>>>
> >>>
> >>> Sorry, i did not get that you want to change like below.
> >>> However, it looks that it does not make too much sense to call
> >>> mmc_regulator_set_vqmmc() for each VOLTAGE type like 3.3v/1.8v/1.2v
> >>> which introduces ugliness because mmc_regulator_set_vqmmc()
> >>> already handles it internally, right?
> >>> Only because we want to keep an "ASSUMED" order as before?
> >>
> >> Yes
> >>
> >>>
> >>> Regards
> >>> Dong Aisheng
> >>>
> >>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >>>> index 94cffa77490a..69b4d48aff87 100644
> >>>> --- a/drivers/mmc/host/sdhci.c
> >>>> +++ b/drivers/mmc/host/sdhci.c
> >>>> @@ -1757,8 +1757,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
> >>>>  		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> >>>>  
> >>>>  		if (!IS_ERR(mmc->supply.vqmmc)) {
> >>>> -			ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
> >>>> -						    3600000);
> >>>> +			ret = mmc_regulator_set_vqmmc(mmc, ios);
> >>>>  			if (ret) {
> >>>>  				pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
> >>>>  					mmc_hostname(mmc));
> >>>> @@ -1779,8 +1778,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
> >>>>  		return -EAGAIN;
> >>>>  	case MMC_SIGNAL_VOLTAGE_180:
> >>>>  		if (!IS_ERR(mmc->supply.vqmmc)) {
> >>>> -			ret = regulator_set_voltage(mmc->supply.vqmmc,
> >>>> -					1700000, 1950000);
> >>>> +			ret = mmc_regulator_set_vqmmc(mmc, ios);
> >>>>  			if (ret) {
> >>>>  				pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
> >>>>  					mmc_hostname(mmc));
> >>>> @@ -1810,8 +1808,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
> >>>>  		return -EAGAIN;
> >>>>  	case MMC_SIGNAL_VOLTAGE_120:
> >>>>  		if (!IS_ERR(mmc->supply.vqmmc)) {
> >>>> -			ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
> >>>> -						    1300000);
> >>>> +			ret = mmc_regulator_set_vqmmc(mmc, ios);
> >>>>  			if (ret) {
> >>>>  				pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
> >>>>  					mmc_hostname(mmc));
> >>>>
> >>>>
> >>>>
> >>>
> >>
> > 
>