[PATCH 04/23] mmc: sdhci: re-factor sdhci_start_signal_voltage()
Dong Aisheng
dongas86 at gmail.com
Thu Apr 28 07:09:29 PDT 2016
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 at intel.com> 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()
> 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.
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 at vger.kernel.org
> >>> 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 at vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> >
>
More information about the linux-arm-kernel
mailing list