Hi Adrian, On 04/28/2016 03:39 PM, 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@xxxxxxxxx> 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 > >> >> 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". > > We have to move forward *and* avoid potential breakage. > > In this case it seems me that the risk of breakage outweighs the value of > prettier code. > > 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: maybe.. this question should not be related with this discussion.. But i have one question..sdhci_do_start_signal_voltage_switch() returned 0 or EAGAIN, when IS_ERR(mmc->supply.vqmmc) is ture. It there any problem? I'm also checking on core side. but just wondering this. (Because i'm fixing dwmmc controller for this.) Best Regards, Jaehoon Chung > > 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)); > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
