[v2,4/8] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch
diff mbox

Message ID 1443622064-14362-5-git-send-email-heiko@sntech.de
State New
Headers show

Commit Message

Heiko Stuebner Sept. 30, 2015, 2:07 p.m. UTC
From: Douglas Anderson <dianders@chromium.org>

We've introduced a new helper in the MMC core:
mmc_regulator_set_vqmmc().  Let's use this in dw_mmc.  Using this new
helper has some advantages:

1. We get the mmc_regulator_set_vqmmc() behavior of trying to match
   VQMMC and VMMC when the signal voltage is 3.3V.  This ensures max
   compatibility.

2. We get rid of a few more warnings when probing unsupported
   voltages.

3. We get rid of some non-dw_mmc specific code in dw_mmc.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/mmc/host/dw_mmc.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

Comments

Jaehoon Chung Oct. 8, 2015, 1:15 a.m. UTC | #1
Hi, Heiko.

Acked-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung


On 09/30/2015 11:07 PM, Heiko Stuebner wrote:
> From: Douglas Anderson <dianders@chromium.org>
> 
> We've introduced a new helper in the MMC core:
> mmc_regulator_set_vqmmc().  Let's use this in dw_mmc.  Using this new
> helper has some advantages:
> 
> 1. We get the mmc_regulator_set_vqmmc() behavior of trying to match
>    VQMMC and VMMC when the signal voltage is 3.3V.  This ensures max
>    compatibility.
> 
> 2. We get rid of a few more warnings when probing unsupported
>    voltages.
> 
> 3. We get rid of some non-dw_mmc specific code in dw_mmc.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/mmc/host/dw_mmc.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index fcbf552..b1b7e7f 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1279,7 +1279,6 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
>  	const struct dw_mci_drv_data *drv_data = host->drv_data;
>  	u32 uhs;
>  	u32 v18 = SDMMC_UHS_18V << slot->id;
> -	int min_uv, max_uv;
>  	int ret;
>  
>  	if (drv_data && drv_data->switch_voltage)
> @@ -1291,22 +1290,18 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
>  	 * does no harm but you need to set the regulator directly.  Try both.
>  	 */
>  	uhs = mci_readl(host, UHS_REG);
> -	if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
> -		min_uv = 2700000;
> -		max_uv = 3600000;
> +	if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330)
>  		uhs &= ~v18;
> -	} else {
> -		min_uv = 1700000;
> -		max_uv = 1950000;
> +	else
>  		uhs |= v18;
> -	}
> +
>  	if (!IS_ERR(mmc->supply.vqmmc)) {
> -		ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
> +		ret = mmc_regulator_set_vqmmc(mmc, ios);
>  
>  		if (ret) {
>  			dev_dbg(&mmc->class_dev,
> -					 "Regulator set error %d: %d - %d\n",
> -					 ret, min_uv, max_uv);
> +					 "Regulator set error %d - %s V\n",
> +					 ret, uhs & v18 ? "1.8" : "3.3");
>  			return ret;
>  		}
>  	}
>
Kevin Hilman Oct. 9, 2015, 7:06 p.m. UTC | #2
On Wed, Sep 30, 2015 at 4:07 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> From: Douglas Anderson <dianders@chromium.org>
>
> We've introduced a new helper in the MMC core:
> mmc_regulator_set_vqmmc().  Let's use this in dw_mmc.  Using this new
> helper has some advantages:
>
> 1. We get the mmc_regulator_set_vqmmc() behavior of trying to match
>    VQMMC and VMMC when the signal voltage is 3.3V.  This ensures max
>    compatibility.
>
> 2. We get rid of a few more warnings when probing unsupported
>    voltages.
>
> 3. We get rid of some non-dw_mmc specific code in dw_mmc.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>

kernelci.org found a new failure in linux-next on
exynos5800-peach-pi[1] and it was bisected down to this patch, which
has hit linux-next in the form of commit fba2e23128d0 (mmc: dw_mmc:
Use mmc_regulator_set_vqmmc in start_signal_voltage_switch).  Based on
a quick glance at the boot log[2], it looks to be a bad pointer
dereference in regulator_get_voltage().

I confirmed that reverting this patch on top of next-20151009 gets my
peach-pi booting again.

Note that the boot failure is only on exynos_defconfig, not multi_v7_defconfig.

Kevin


[1] http://kernelci.org/boot/exynos5800-peach-pi/job/next/kernel/next-20151009/defconfig/exynos_defconfig/lab/lab-khilman/?_id=56177d3659b514a50c6c3267

[2] http://storage.kernelci.org/next/next-20151009/arm-exynos_defconfig/lab-khilman/boot-exynos5800-peach-pi.html


> ---
>  drivers/mmc/host/dw_mmc.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index fcbf552..b1b7e7f 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1279,7 +1279,6 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
>         const struct dw_mci_drv_data *drv_data = host->drv_data;
>         u32 uhs;
>         u32 v18 = SDMMC_UHS_18V << slot->id;
> -       int min_uv, max_uv;
>         int ret;
>
>         if (drv_data && drv_data->switch_voltage)
> @@ -1291,22 +1290,18 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
>          * does no harm but you need to set the regulator directly.  Try both.
>          */
>         uhs = mci_readl(host, UHS_REG);
> -       if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
> -               min_uv = 2700000;
> -               max_uv = 3600000;
> +       if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330)
>                 uhs &= ~v18;
> -       } else {
> -               min_uv = 1700000;
> -               max_uv = 1950000;
> +       else
>                 uhs |= v18;
> -       }
> +
>         if (!IS_ERR(mmc->supply.vqmmc)) {
> -               ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
> +               ret = mmc_regulator_set_vqmmc(mmc, ios);
>
>                 if (ret) {
>                         dev_dbg(&mmc->class_dev,
> -                                        "Regulator set error %d: %d - %d\n",
> -                                        ret, min_uv, max_uv);
> +                                        "Regulator set error %d - %s V\n",
> +                                        ret, uhs & v18 ? "1.8" : "3.3");
>                         return ret;
>                 }
>         }
> --
> 2.5.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Douglas Anderson Oct. 9, 2015, 8:12 p.m. UTC | #3
Kevin,

On Fri, Oct 9, 2015 at 12:06 PM, Kevin Hilman <khilman@kernel.org> wrote:
> On Wed, Sep 30, 2015 at 4:07 PM, Heiko Stuebner <heiko@sntech.de> wrote:
>> From: Douglas Anderson <dianders@chromium.org>
>>
>> We've introduced a new helper in the MMC core:
>> mmc_regulator_set_vqmmc().  Let's use this in dw_mmc.  Using this new
>> helper has some advantages:
>>
>> 1. We get the mmc_regulator_set_vqmmc() behavior of trying to match
>>    VQMMC and VMMC when the signal voltage is 3.3V.  This ensures max
>>    compatibility.
>>
>> 2. We get rid of a few more warnings when probing unsupported
>>    voltages.
>>
>> 3. We get rid of some non-dw_mmc specific code in dw_mmc.
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>
> kernelci.org found a new failure in linux-next on
> exynos5800-peach-pi[1] and it was bisected down to this patch, which
> has hit linux-next in the form of commit fba2e23128d0 (mmc: dw_mmc:
> Use mmc_regulator_set_vqmmc in start_signal_voltage_switch).  Based on
> a quick glance at the boot log[2], it looks to be a bad pointer
> dereference in regulator_get_voltage().
>
> I confirmed that reverting this patch on top of next-20151009 gets my
> peach-pi booting again.
>
> Note that the boot failure is only on exynos_defconfig, not multi_v7_defconfig.

Right.  See <http://www.spinics.net/lists/arm-kernel/msg450802.html>
for discussion.  Ulf responded to the cover letter rather than the
individual patch.

-Doug
Kevin Hilman Oct. 9, 2015, 8:17 p.m. UTC | #4
On Fri, Oct 9, 2015 at 10:12 PM, Doug Anderson <dianders@chromium.org> wrote:
> Kevin,
>
> On Fri, Oct 9, 2015 at 12:06 PM, Kevin Hilman <khilman@kernel.org> wrote:
>> On Wed, Sep 30, 2015 at 4:07 PM, Heiko Stuebner <heiko@sntech.de> wrote:
>>> From: Douglas Anderson <dianders@chromium.org>
>>>
>>> We've introduced a new helper in the MMC core:
>>> mmc_regulator_set_vqmmc().  Let's use this in dw_mmc.  Using this new
>>> helper has some advantages:
>>>
>>> 1. We get the mmc_regulator_set_vqmmc() behavior of trying to match
>>>    VQMMC and VMMC when the signal voltage is 3.3V.  This ensures max
>>>    compatibility.
>>>
>>> 2. We get rid of a few more warnings when probing unsupported
>>>    voltages.
>>>
>>> 3. We get rid of some non-dw_mmc specific code in dw_mmc.
>>>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>>
>> kernelci.org found a new failure in linux-next on
>> exynos5800-peach-pi[1] and it was bisected down to this patch, which
>> has hit linux-next in the form of commit fba2e23128d0 (mmc: dw_mmc:
>> Use mmc_regulator_set_vqmmc in start_signal_voltage_switch).  Based on
>> a quick glance at the boot log[2], it looks to be a bad pointer
>> dereference in regulator_get_voltage().
>>
>> I confirmed that reverting this patch on top of next-20151009 gets my
>> peach-pi booting again.
>>
>> Note that the boot failure is only on exynos_defconfig, not multi_v7_defconfig.
>
> Right.  See <http://www.spinics.net/lists/arm-kernel/msg450802.html>
> for discussion.  Ulf responded to the cover letter rather than the
> individual patch.

Ah, I hadn't noticed that.  Thanks.

I guess it should be dropped by tomorrow's linux-next then.  I'll keep
an eye on it.

Kevin

Patch
diff mbox

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index fcbf552..b1b7e7f 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1279,7 +1279,6 @@  static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
 	const struct dw_mci_drv_data *drv_data = host->drv_data;
 	u32 uhs;
 	u32 v18 = SDMMC_UHS_18V << slot->id;
-	int min_uv, max_uv;
 	int ret;
 
 	if (drv_data && drv_data->switch_voltage)
@@ -1291,22 +1290,18 @@  static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
 	 * does no harm but you need to set the regulator directly.  Try both.
 	 */
 	uhs = mci_readl(host, UHS_REG);
-	if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
-		min_uv = 2700000;
-		max_uv = 3600000;
+	if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330)
 		uhs &= ~v18;
-	} else {
-		min_uv = 1700000;
-		max_uv = 1950000;
+	else
 		uhs |= v18;
-	}
+
 	if (!IS_ERR(mmc->supply.vqmmc)) {
-		ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
+		ret = mmc_regulator_set_vqmmc(mmc, ios);
 
 		if (ret) {
 			dev_dbg(&mmc->class_dev,
-					 "Regulator set error %d: %d - %d\n",
-					 ret, min_uv, max_uv);
+					 "Regulator set error %d - %s V\n",
+					 ret, uhs & v18 ? "1.8" : "3.3");
 			return ret;
 		}
 	}