On 28/04/16 10:15, Jaehoon Chung wrote: > 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? Not that I am aware of. > > I'm also checking on core side. but just wondering this. > (Because i'm fixing dwmmc controller for this.) What is the problem? > > 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
