On Fri, Apr 29, 2016 at 10:32:36AM +0300, Adrian Hunter wrote: > On 28/04/16 17:28, Dong Aisheng wrote: > > On Thu, Apr 28, 2016 at 04:36:25PM +0300, Adrian Hunter wrote: > >> 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. > >> > > > > Hmm... If there is no way to know the correct order, how can we > > assume the exist order is correct? > > There is no correct order. This is outside the SDHCI spec. and so belongs > to individual drivers. > > If it mattered we could push the ugly code down onto the drivers, and then > driver maintainers could opt to use the new pretty code. However at the > moment there is a lot more important work, so I would want to avoid that > code churn. > > > And i already pointed out, the exist order is also confused that > > it switch controller first then vqmmc for 3.3v and switch vqmmc > > first, then controller for 1.8v > > > > If we can't sure the exist order is correct, why do we block > > the changing to correctly use mmc_regulator_set_vqmmc() to > > improve the driver stability? > > Not sure what you mean here. I have already showed how we can use > mmc_regulator_set_vqmmc(). > > > > > And actually the change is not made arbitrarily, i already list > > all possibilities based on my knowledge. People can raise more > > if any. > > You are assuming every driver has a maintainer and every maintainer is > following this thread, and understands how it might affect all the > different versions of their hardware. That is extremely unlikely. > Yes, we can't sure it. All we can do on breaking old rules is do it early and test people. Potential break shouldn't block us to going forward IMHO. But i understand at this time you may not want it. > > The target is correct that we make start_signal_voltage_switch() > > order independant. > > It's worth a try even there's a potential very low possibility > > break IMHO. > > And so we disagree. > > Aren't your needs met by changing to use mmc_regulator_set_vqmmc() the way I > suggested? > Well, although i'm not in favor of that way. But as you insist, i will re-cook the patch to fix the issue first. Regards Dong Aisheng > > > > Or else if we can find a better way to switch to > > mmc_regulator_set_vqmmc i would also love to try. > > > > Maybe we need more people's thought on it! > > > > Ulf, > > Would you give some inputs? > > > > Regards > > Dong Aisheng > > > >>> > >>>> 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
