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

[Dong Aisheng <dongas86@gmail.com>]

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@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.

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?

> 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.

Regards
Dong Aisheng

> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html