| Message ID | 1397172708-19735-1-git-send-email-tim.kryger@linaro.org |
|---|---|
| State | New |
| Headers | show |
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/
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/
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
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
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
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);
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(-)