diff mbox

[04/23] mmc: sdhci: re-factor sdhci_start_signal_voltage()

Message ID 5721B03A.5030006@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Adrian Hunter April 28, 2016, 6:39 a.m. UTC
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@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:




--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jaehoon Chung April 28, 2016, 7:15 a.m. UTC | #1
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@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@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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Hunter April 28, 2016, 7:44 a.m. UTC | #2
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@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?

> 
> 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@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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jaehoon Chung April 28, 2016, 8:30 a.m. UTC | #3
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@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.)

I don't know sdhci is working fine or not..just wondering. :)

I'm going to analyze the sequence and other thing..so i may miss something.

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@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@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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dong Aisheng April 28, 2016, 1:14 p.m. UTC | #4
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@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
> 

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?

> 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 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?

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Hunter April 28, 2016, 1:36 p.m. UTC | #5
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@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
>>
> 
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dong Aisheng April 28, 2016, 2:09 p.m. UTC | #6
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@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@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@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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dong Aisheng April 28, 2016, 2:28 p.m. UTC | #7
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@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
> >>
> > 
> > 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?
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?

And actually the change is not made arbitrarily, i already list
all possibilities based on my knowledge. People can raise more
if any.
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.

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jaehoon Chung April 28, 2016, 11:06 p.m. UTC | #8
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@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()

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@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@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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Hunter April 29, 2016, 7:32 a.m. UTC | #9
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@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
>>>>
>>>
>>> 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.

> 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?

> 
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dong Aisheng April 29, 2016, 7:57 a.m. UTC | #10
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@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
> >>>>
> >>>
> >>> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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