[PATCH 04/23] mmc: sdhci: re-factor sdhci_start_signal_voltage()
Jaehoon Chung
jh80.chung at samsung.com
Thu Apr 28 00:15:38 PDT 2016
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?
I'm also checking on core side. but just wondering this.
(Because i'm fixing dwmmc controller for this.)
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
>
>
More information about the linux-arm-kernel
mailing list