On 04/28/2016 11:09 PM, Dong Aisheng wrote: > On Thu, Apr 28, 2016 at 05:30:11PM +0900, Jaehoon Chung wrote: >> On 04/28/2016 04:44 PM, Adrian Hunter wrote: >>> 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? >> >> It should be difference with dwmmc controller. >> if mmc->supply.vqmmc is not assigned, it didn't change the voltage.. >> (I'm not sure that HOST_CONTROL2 register can internally change the voltage..because i didn't have SDHC 3.0 spec.) >> >> __mmc_set_signal_voltage() >> -> host->ops->start_signal_voltage_switch() >> -> host controller just set the bits for v1.8 or v3.3..(if vqmmc didn't use.) >> if return -EAGAIN or 0 , the bits that was set for v1.8 or v3.3 should be maintained. >> >> And do mmc_power_cycle() -> mmc_power_off and mmc_set_initial_state() >> >> But host controller didn't initialize. >> >> That is dwmmc controller's side.. >> >> As my understanding, if voltage switch(UHS-I mode) is failed, it should be set to HS mode. >> but dwmmc controller didn't work this case..so i will fix. >> (Some parts are code bugs in dwmmc controller.) >> > > MMC core will retry HS mode initialization if voltage switch reach 10 > times failure. > See: mmc_sd_get_cid() Yes, i knew. > >> I don't know sdhci is working fine or not..just wondering. :) >> > > I tested sdhci is working fine. > >> I'm going to analyze the sequence and other thing..so i may miss something. >> > > Probably you may need check if the IO voltage is correctly back up > after failure. As i mentioned, this is dwmmc controller's problem..there is some bugs. :) So i'm fixing. Thanks! Best Regards, Jaehoon Chung > > Regards > Dong Aisheng > >> Best Regards, >> Jaehoon Chung >> >>> >>>> >>>> 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 >>> >>> >> > > -- 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
