mmc: sdhci: Set ocr_avail directly based on vmmc

Message ID 1397172708-19735-1-git-send-email-tim.kryger@linaro.org
State New
Headers show

Commit Message

Tim Kryger April 10, 2014, 11:31 p.m.
When an external regulator provides VDD, set ocr_avail directly based on
the supported voltage range.  This allows for the use of regulators that
can't provide exactly 1.8v, 3.0v, or 3.3v and ensures that ocr_avil bits
are only set for supported voltage ranges.  Commit cec2e21 had attempted
to relax the range checks but because it relied on setting capabilities
as an intermediate step, ocr_avail could easily get a bit set that the
host couldn't support.

Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
---
 drivers/mmc/host/sdhci.c |  107 +++++++++++++++++++++++++---------------------
 1 file changed, 58 insertions(+), 49 deletions(-)

Comments

Ulf Hansson April 11, 2014, 8:15 a.m. | #1
On 11 April 2014 01:31, Tim Kryger <tim.kryger@linaro.org> wrote:
> When an external regulator provides VDD, set ocr_avail directly based on
> the supported voltage range.  This allows for the use of regulators that
> can't provide exactly 1.8v, 3.0v, or 3.3v and ensures that ocr_avil bits
> are only set for supported voltage ranges.  Commit cec2e21 had attempted
> to relax the range checks but because it relied on setting capabilities
> as an intermediate step, ocr_avail could easily get a bit set that the
> host couldn't support.
>
> Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
> ---
>  drivers/mmc/host/sdhci.c |  107 +++++++++++++++++++++++++---------------------
>  1 file changed, 58 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 9a79fc4..4d56fbe 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2769,6 +2769,29 @@ struct sdhci_host *sdhci_alloc_host(struct device *dev,
>
>  EXPORT_SYMBOL_GPL(sdhci_alloc_host);
>
> +static unsigned int sdhci_get_ocr_avail_from_vmmc(struct sdhci_host *host)
> +{
> +       unsigned int ocr_avail = 0;
> +       struct regulator *vmmc = host->vmmc;
> +
> +       if (regulator_is_supported_voltage(vmmc, 1650000, 1950000) > 0)
> +               ocr_avail |= MMC_VDD_165_195;
> +
> +       if (regulator_is_supported_voltage(vmmc, 2900000, 3000000) > 0)
> +               ocr_avail |= MMC_VDD_29_30;
> +
> +       if (regulator_is_supported_voltage(vmmc, 3000000, 3100000) > 0)
> +               ocr_avail |= MMC_VDD_30_31;
> +
> +       if (regulator_is_supported_voltage(vmmc, 3200000, 3300000) > 0)
> +               ocr_avail |= MMC_VDD_32_33;
> +
> +       if (regulator_is_supported_voltage(vmmc, 3300000, 3400000) > 0)
> +               ocr_avail |= MMC_VDD_33_34;
> +
> +       return ocr_avail;
> +}
> +

There is an API called mmc_regulator_get_ocrmask() for this.

>  int sdhci_add_host(struct sdhci_host *host)
>  {
>         struct mmc_host *mmc;
> @@ -3063,24 +3086,39 @@ int sdhci_add_host(struct sdhci_host *host)
>                 }
>         }
>
> -#ifdef CONFIG_REGULATOR
> -       /*
> -        * Voltage range check makes sense only if regulator reports
> -        * any voltage value.
> -        */
> +       /* If using external regulator, check supported voltage ranges */
>         if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) {
> -               ret = regulator_is_supported_voltage(host->vmmc, 2700000,
> -                       3600000);
> -               if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
> -                       caps[0] &= ~SDHCI_CAN_VDD_330;
> -               if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300)))
> -                       caps[0] &= ~SDHCI_CAN_VDD_300;
> -               ret = regulator_is_supported_voltage(host->vmmc, 1700000,
> -                       1950000);
> -               if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180)))
> -                       caps[0] &= ~SDHCI_CAN_VDD_180;
> -       }
> -#endif /* CONFIG_REGULATOR */
> +               ocr_avail = sdhci_get_ocr_avail_from_vmmc(host);
> +       } else {
> +               if (caps[0] & SDHCI_CAN_VDD_330)
> +                       ocr_avail |= MMC_VDD_32_33 | MMC_VDD_33_34;
> +               if (caps[0] & SDHCI_CAN_VDD_300)
> +                       ocr_avail |= MMC_VDD_29_30 | MMC_VDD_30_31;
> +               if (caps[0] & SDHCI_CAN_VDD_180)
> +                       ocr_avail |= MMC_VDD_165_195;
> +       }
> +
> +       if (host->ocr_mask)
> +               ocr_avail = host->ocr_mask;
> +
> +       mmc->ocr_avail = ocr_avail;
> +       mmc->ocr_avail_sdio = ocr_avail;
> +       if (host->ocr_avail_sdio)
> +               mmc->ocr_avail_sdio &= host->ocr_avail_sdio;
> +       mmc->ocr_avail_sd = ocr_avail;
> +       if (host->ocr_avail_sd)
> +               mmc->ocr_avail_sd &= host->ocr_avail_sd;
> +       else /* normal SD controllers don't support 1.8V */
> +               mmc->ocr_avail_sd &= ~MMC_VDD_165_195;
> +       mmc->ocr_avail_mmc = ocr_avail;
> +       if (host->ocr_avail_mmc)
> +               mmc->ocr_avail_mmc &= host->ocr_avail_mmc;
> +
> +       if (mmc->ocr_avail == 0) {
> +               pr_err("%s: Hardware doesn't report any support voltages.\n",
> +                      mmc_hostname(mmc));
> +               return -ENODEV;
> +       }

I have not fully understand why you have different ocr masks depending
on what card you initialize. Is that really supported by the
controller?

>
>         /*
>          * According to SD Host Controller spec v3.00, if the Host System
> @@ -3106,52 +3144,23 @@ int sdhci_add_host(struct sdhci_host *host)
>                 }
>         }
>
> -       if (caps[0] & SDHCI_CAN_VDD_330) {
> -               ocr_avail |= MMC_VDD_32_33 | MMC_VDD_33_34;
> -
> +       if (ocr_avail & (MMC_VDD_32_33 | MMC_VDD_33_34))
>                 mmc->max_current_330 = ((max_current_caps &
>                                    SDHCI_MAX_CURRENT_330_MASK) >>
>                                    SDHCI_MAX_CURRENT_330_SHIFT) *
>                                    SDHCI_MAX_CURRENT_MULTIPLIER;
> -       }
> -       if (caps[0] & SDHCI_CAN_VDD_300) {
> -               ocr_avail |= MMC_VDD_29_30 | MMC_VDD_30_31;
>
> +       if (ocr_avail & (MMC_VDD_29_30 | MMC_VDD_30_31))
>                 mmc->max_current_300 = ((max_current_caps &
>                                    SDHCI_MAX_CURRENT_300_MASK) >>
>                                    SDHCI_MAX_CURRENT_300_SHIFT) *
>                                    SDHCI_MAX_CURRENT_MULTIPLIER;
> -       }
> -       if (caps[0] & SDHCI_CAN_VDD_180) {
> -               ocr_avail |= MMC_VDD_165_195;
>
> +       if (ocr_avail & MMC_VDD_165_195)
>                 mmc->max_current_180 = ((max_current_caps &
>                                    SDHCI_MAX_CURRENT_180_MASK) >>
>                                    SDHCI_MAX_CURRENT_180_SHIFT) *
>                                    SDHCI_MAX_CURRENT_MULTIPLIER;
> -       }
> -
> -       if (host->ocr_mask)
> -               ocr_avail = host->ocr_mask;
> -
> -       mmc->ocr_avail = ocr_avail;
> -       mmc->ocr_avail_sdio = ocr_avail;
> -       if (host->ocr_avail_sdio)
> -               mmc->ocr_avail_sdio &= host->ocr_avail_sdio;
> -       mmc->ocr_avail_sd = ocr_avail;
> -       if (host->ocr_avail_sd)
> -               mmc->ocr_avail_sd &= host->ocr_avail_sd;
> -       else /* normal SD controllers don't support 1.8V */
> -               mmc->ocr_avail_sd &= ~MMC_VDD_165_195;
> -       mmc->ocr_avail_mmc = ocr_avail;
> -       if (host->ocr_avail_mmc)
> -               mmc->ocr_avail_mmc &= host->ocr_avail_mmc;
> -
> -       if (mmc->ocr_avail == 0) {
> -               pr_err("%s: Hardware doesn't report any "
> -                       "support voltages.\n", mmc_hostname(mmc));
> -               return -ENODEV;
> -       }
>

I have seen some patches around lately touching the code for handling
the regulators (vcc and vccq) in sdhci.

A few times I have suggested to switch to use the
mmc_regulator_get_supply() API to simplify and consolidate code. Could
you please have a look at that?

Kind regards
Ulf Hansson

>         spin_lock_init(&host->lock);
>
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Tim Kryger April 11, 2014, 3:59 p.m. | #2
On Fri, Apr 11, 2014 at 1:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 11 April 2014 01:31, Tim Kryger <tim.kryger@linaro.org> wrote:

>> +static unsigned int sdhci_get_ocr_avail_from_vmmc(struct sdhci_host *host)
>> +{
>> +       unsigned int ocr_avail = 0;
>> +       struct regulator *vmmc = host->vmmc;
>> +
>> +       if (regulator_is_supported_voltage(vmmc, 1650000, 1950000) > 0)
>> +               ocr_avail |= MMC_VDD_165_195;
>> +
>> +       if (regulator_is_supported_voltage(vmmc, 2900000, 3000000) > 0)
>> +               ocr_avail |= MMC_VDD_29_30;
>> +
>> +       if (regulator_is_supported_voltage(vmmc, 3000000, 3100000) > 0)
>> +               ocr_avail |= MMC_VDD_30_31;
>> +
>> +       if (regulator_is_supported_voltage(vmmc, 3200000, 3300000) > 0)
>> +               ocr_avail |= MMC_VDD_32_33;
>> +
>> +       if (regulator_is_supported_voltage(vmmc, 3300000, 3400000) > 0)
>> +               ocr_avail |= MMC_VDD_33_34;
>> +
>> +       return ocr_avail;
>> +}
>> +
>
> There is an API called mmc_regulator_get_ocrmask() for this.

Great.  Thanks for pointing this out.


>> +               ocr_avail = sdhci_get_ocr_avail_from_vmmc(host);
>> +       } else {
>> +               if (caps[0] & SDHCI_CAN_VDD_330)
>> +                       ocr_avail |= MMC_VDD_32_33 | MMC_VDD_33_34;
>> +               if (caps[0] & SDHCI_CAN_VDD_300)
>> +                       ocr_avail |= MMC_VDD_29_30 | MMC_VDD_30_31;
>> +               if (caps[0] & SDHCI_CAN_VDD_180)
>> +                       ocr_avail |= MMC_VDD_165_195;
>> +       }
>> +
>> +       if (host->ocr_mask)
>> +               ocr_avail = host->ocr_mask;
>> +
>> +       mmc->ocr_avail = ocr_avail;
>> +       mmc->ocr_avail_sdio = ocr_avail;
>> +       if (host->ocr_avail_sdio)
>> +               mmc->ocr_avail_sdio &= host->ocr_avail_sdio;
>> +       mmc->ocr_avail_sd = ocr_avail;
>> +       if (host->ocr_avail_sd)
>> +               mmc->ocr_avail_sd &= host->ocr_avail_sd;
>> +       else /* normal SD controllers don't support 1.8V */
>> +               mmc->ocr_avail_sd &= ~MMC_VDD_165_195;
>> +       mmc->ocr_avail_mmc = ocr_avail;
>> +       if (host->ocr_avail_mmc)
>> +               mmc->ocr_avail_mmc &= host->ocr_avail_mmc;
>> +
>> +       if (mmc->ocr_avail == 0) {
>> +               pr_err("%s: Hardware doesn't report any support voltages.\n",
>> +                      mmc_hostname(mmc));
>> +               return -ENODEV;
>> +       }
>
> I have not fully understand why you have different ocr masks depending
> on what card you initialize. Is that really supported by the
> controller?

Would you mind clarifying your question?  I'm not sure what you are
after.  Also this logic is mostly unchanged, I have simply moved it up
earlier in the function to keep all the ocr parts together.

> I have seen some patches around lately touching the code for handling
> the regulators (vcc and vccq) in sdhci.
>
> A few times I have suggested to switch to use the
> mmc_regulator_get_supply() API to simplify and consolidate code. Could
> you please have a look at that?

Sure, I'll take a look.  Thanks for the helpful comments.

-Tim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Tim Kryger April 15, 2014, 5:09 p.m. | #3
On Fri, Apr 11, 2014 at 1:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

> I have seen some patches around lately touching the code for handling
> the regulators (vcc and vccq) in sdhci.

Was it this patch you were thinking of or something else?

http://www.spinics.net/lists/linux-mmc/msg25640.html

> A few times I have suggested to switch to use the
> mmc_regulator_get_supply() API to simplify and consolidate code. Could
> you please have a look at that?

This function will solve my problem but it also suggests that SDHCI
drivers should use the vmmc/vqmmc pointers in the mmc_host struct
rather than the ones in the sdhci_host struct.

Is this your intent?  Do you want to see the regulator pointers in the
sdhci_host struct removed once all drivers are modified to use the
mmc_host ones?

-Tim
--
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
Ulf Hansson April 16, 2014, 7:20 a.m. | #4
On 15 April 2014 19:09, Tim Kryger <tim.kryger@linaro.org> wrote:
> On Fri, Apr 11, 2014 at 1:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
>> I have seen some patches around lately touching the code for handling
>> the regulators (vcc and vccq) in sdhci.
>
> Was it this patch you were thinking of or something else?
>
> http://www.spinics.net/lists/linux-mmc/msg25640.html

Yes, that's one of them. I am not sure I remember correct, but I think
there were patches for converting to devm_* API as well.

>
>> A few times I have suggested to switch to use the
>> mmc_regulator_get_supply() API to simplify and consolidate code. Could
>> you please have a look at that?
>
> This function will solve my problem but it also suggests that SDHCI
> drivers should use the vmmc/vqmmc pointers in the mmc_host struct
> rather than the ones in the sdhci_host struct.
>
> Is this your intent?  Do you want to see the regulator pointers in the
> sdhci_host struct removed once all drivers are modified to use the
> mmc_host ones?

Correct. That will consolidate code!

Then if sdhci has some special needs for regulators, let's first see
if we can adopt the API to handle it, before we decide to put that
code in sdhci driver.

Kind regards
Uffe

>
> -Tim
--
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
Tim Kryger April 18, 2014, 9:23 p.m. | #5
On Wed, Apr 16, 2014 at 12:20 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 15 April 2014 19:09, Tim Kryger <tim.kryger@linaro.org> wrote:
>> On Fri, Apr 11, 2014 at 1:15 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

>>> A few times I have suggested to switch to use the
>>> mmc_regulator_get_supply() API to simplify and consolidate code. Could
>>> you please have a look at that?
>>
>> This function will solve my problem but it also suggests that SDHCI
>> drivers should use the vmmc/vqmmc pointers in the mmc_host struct
>> rather than the ones in the sdhci_host struct.
>>
>> Is this your intent?  Do you want to see the regulator pointers in the
>> sdhci_host struct removed once all drivers are modified to use the
>> mmc_host ones?
>
> Correct. That will consolidate code!
>
> Then if sdhci has some special needs for regulators, let's first see
> if we can adopt the API to handle it, before we decide to put that
> code in sdhci driver.

Sounds good.  Please forget about this patch and consider the new
series instead.

https://lkml.org/lkml/2014/4/17/653
--
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

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9a79fc4..4d56fbe 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2769,6 +2769,29 @@  struct sdhci_host *sdhci_alloc_host(struct device *dev,
 
 EXPORT_SYMBOL_GPL(sdhci_alloc_host);
 
+static unsigned int sdhci_get_ocr_avail_from_vmmc(struct sdhci_host *host)
+{
+	unsigned int ocr_avail = 0;
+	struct regulator *vmmc = host->vmmc;
+
+	if (regulator_is_supported_voltage(vmmc, 1650000, 1950000) > 0)
+		ocr_avail |= MMC_VDD_165_195;
+
+	if (regulator_is_supported_voltage(vmmc, 2900000, 3000000) > 0)
+		ocr_avail |= MMC_VDD_29_30;
+
+	if (regulator_is_supported_voltage(vmmc, 3000000, 3100000) > 0)
+		ocr_avail |= MMC_VDD_30_31;
+
+	if (regulator_is_supported_voltage(vmmc, 3200000, 3300000) > 0)
+		ocr_avail |= MMC_VDD_32_33;
+
+	if (regulator_is_supported_voltage(vmmc, 3300000, 3400000) > 0)
+		ocr_avail |= MMC_VDD_33_34;
+
+	return ocr_avail;
+}
+
 int sdhci_add_host(struct sdhci_host *host)
 {
 	struct mmc_host *mmc;
@@ -3063,24 +3086,39 @@  int sdhci_add_host(struct sdhci_host *host)
 		}
 	}
 
-#ifdef CONFIG_REGULATOR
-	/*
-	 * Voltage range check makes sense only if regulator reports
-	 * any voltage value.
-	 */
+	/* If using external regulator, check supported voltage ranges */
 	if (host->vmmc && regulator_get_voltage(host->vmmc) > 0) {
-		ret = regulator_is_supported_voltage(host->vmmc, 2700000,
-			3600000);
-		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
-			caps[0] &= ~SDHCI_CAN_VDD_330;
-		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300)))
-			caps[0] &= ~SDHCI_CAN_VDD_300;
-		ret = regulator_is_supported_voltage(host->vmmc, 1700000,
-			1950000);
-		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180)))
-			caps[0] &= ~SDHCI_CAN_VDD_180;
-	}
-#endif /* CONFIG_REGULATOR */
+		ocr_avail = sdhci_get_ocr_avail_from_vmmc(host);
+	} else {
+		if (caps[0] & SDHCI_CAN_VDD_330)
+			ocr_avail |= MMC_VDD_32_33 | MMC_VDD_33_34;
+		if (caps[0] & SDHCI_CAN_VDD_300)
+			ocr_avail |= MMC_VDD_29_30 | MMC_VDD_30_31;
+		if (caps[0] & SDHCI_CAN_VDD_180)
+			ocr_avail |= MMC_VDD_165_195;
+	}
+
+	if (host->ocr_mask)
+		ocr_avail = host->ocr_mask;
+
+	mmc->ocr_avail = ocr_avail;
+	mmc->ocr_avail_sdio = ocr_avail;
+	if (host->ocr_avail_sdio)
+		mmc->ocr_avail_sdio &= host->ocr_avail_sdio;
+	mmc->ocr_avail_sd = ocr_avail;
+	if (host->ocr_avail_sd)
+		mmc->ocr_avail_sd &= host->ocr_avail_sd;
+	else /* normal SD controllers don't support 1.8V */
+		mmc->ocr_avail_sd &= ~MMC_VDD_165_195;
+	mmc->ocr_avail_mmc = ocr_avail;
+	if (host->ocr_avail_mmc)
+		mmc->ocr_avail_mmc &= host->ocr_avail_mmc;
+
+	if (mmc->ocr_avail == 0) {
+		pr_err("%s: Hardware doesn't report any support voltages.\n",
+		       mmc_hostname(mmc));
+		return -ENODEV;
+	}
 
 	/*
 	 * According to SD Host Controller spec v3.00, if the Host System
@@ -3106,52 +3144,23 @@  int sdhci_add_host(struct sdhci_host *host)
 		}
 	}
 
-	if (caps[0] & SDHCI_CAN_VDD_330) {
-		ocr_avail |= MMC_VDD_32_33 | MMC_VDD_33_34;
-
+	if (ocr_avail & (MMC_VDD_32_33 | MMC_VDD_33_34))
 		mmc->max_current_330 = ((max_current_caps &
 				   SDHCI_MAX_CURRENT_330_MASK) >>
 				   SDHCI_MAX_CURRENT_330_SHIFT) *
 				   SDHCI_MAX_CURRENT_MULTIPLIER;
-	}
-	if (caps[0] & SDHCI_CAN_VDD_300) {
-		ocr_avail |= MMC_VDD_29_30 | MMC_VDD_30_31;
 
+	if (ocr_avail & (MMC_VDD_29_30 | MMC_VDD_30_31))
 		mmc->max_current_300 = ((max_current_caps &
 				   SDHCI_MAX_CURRENT_300_MASK) >>
 				   SDHCI_MAX_CURRENT_300_SHIFT) *
 				   SDHCI_MAX_CURRENT_MULTIPLIER;
-	}
-	if (caps[0] & SDHCI_CAN_VDD_180) {
-		ocr_avail |= MMC_VDD_165_195;
 
+	if (ocr_avail & MMC_VDD_165_195)
 		mmc->max_current_180 = ((max_current_caps &
 				   SDHCI_MAX_CURRENT_180_MASK) >>
 				   SDHCI_MAX_CURRENT_180_SHIFT) *
 				   SDHCI_MAX_CURRENT_MULTIPLIER;
-	}
-
-	if (host->ocr_mask)
-		ocr_avail = host->ocr_mask;
-
-	mmc->ocr_avail = ocr_avail;
-	mmc->ocr_avail_sdio = ocr_avail;
-	if (host->ocr_avail_sdio)
-		mmc->ocr_avail_sdio &= host->ocr_avail_sdio;
-	mmc->ocr_avail_sd = ocr_avail;
-	if (host->ocr_avail_sd)
-		mmc->ocr_avail_sd &= host->ocr_avail_sd;
-	else /* normal SD controllers don't support 1.8V */
-		mmc->ocr_avail_sd &= ~MMC_VDD_165_195;
-	mmc->ocr_avail_mmc = ocr_avail;
-	if (host->ocr_avail_mmc)
-		mmc->ocr_avail_mmc &= host->ocr_avail_mmc;
-
-	if (mmc->ocr_avail == 0) {
-		pr_err("%s: Hardware doesn't report any "
-			"support voltages.\n", mmc_hostname(mmc));
-		return -ENODEV;
-	}
 
 	spin_lock_init(&host->lock);