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 <[email protected]>
---
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;
+}
+
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);
--
1.7.9.5
On 11 April 2014 01:31, Tim Kryger <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Fri, Apr 11, 2014 at 1:15 AM, Ulf Hansson <[email protected]> wrote:
> On 11 April 2014 01:31, Tim Kryger <[email protected]> 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
On Fri, Apr 11, 2014 at 1:15 AM, Ulf Hansson <[email protected]> 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
On 15 April 2014 19:09, Tim Kryger <[email protected]> wrote:
> On Fri, Apr 11, 2014 at 1:15 AM, Ulf Hansson <[email protected]> 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
On Wed, Apr 16, 2014 at 12:20 AM, Ulf Hansson <[email protected]> wrote:
> On 15 April 2014 19:09, Tim Kryger <[email protected]> wrote:
>> On Fri, Apr 11, 2014 at 1:15 AM, Ulf Hansson <[email protected]> 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