On 11 April 2014 01:31, Tim Kryger <tim.kryger@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
