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