Re: [PATCH V2 1/2] mmc: sdhci: Request regulators before reading capabilities

From: Adrian Hunter
Date: Thu Jul 21 2016 - 02:44:09 EST


On 20/07/16 16:04, Adrian Hunter wrote:
> On 20/07/16 16:02, Adrian Hunter wrote:
>> On 12/07/16 16:53, Jon Hunter wrote:
>>> The capabilities of the SDHCI host controller are read early during the
>>> SDHCI host initialisation in sdhci_setup_host() and before any
>>> regulators for the host have been requested. This means that if the host
>>> supports some high-speed modes (according to its capabilities register),
>>> but the board cannot because the appropriate voltage regulator is not
>>> available, then the host cannot easily override the capabilities that
>>> are supported.
>>>
>>> To allow a SDHCI host controller to determine if it can support high
>>> speed modes via the presense of the MMC regulators, request the
>>
>> Try not to confuse High Speed mode with UHS modes.
>>
>> presense -> presence
>>
>>> regulators before reading the capabilites of the host controller. This
>>
>> capabilites -> capabilities
>>
>>> will allow the SDHCI host to use the 'reset' callback to take the
>>> appropriate action (set flags, configure registers, etc) before the
>>> capabilities register(s) are read.
>>>
>>> Please note that some SDHCI hosts, such as the Tegra SDHCI host, has
>>> the ability to mask bits in the capabilities register to prevent
>>> certain capabilities from being advertised.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>
>>> ---
>>>
>>> Although this is described as a "V2" this patch has been added since
>>> the original patch was posted.
>>>
>>> If the below is deemed not appropriate, then another solution I was
>>> thinking of is to allow the SDHCI host to call 'mmc_regulator_get_supply'
>>> before calling sdhci_setup_host() and then in sdhci_setup_host() check
>>> if any regulators are already present before calling
>>> mmc_regulator_get_supply().
>>
>> And we can still do that later if we need to.
>>
>>>
>>> drivers/mmc/host/sdhci.c | 16 +++++++++++-----
>>> 1 file changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 2ee8bfa77116..628c4b3558c0 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -3021,6 +3021,17 @@ int sdhci_setup_host(struct sdhci_host *host)
>>>
>>> mmc = host->mmc;
>>>
>>> + /*
>>> + * If there are external regulators, get them. Note
>>> + * this must be done early before resetting the host
>>> + * and reading the capabilities so that the host can
>>> + * take the appropriate action if regulators are not
>>> + * available.
>
> Also you could spread this comment out to 80 columns.
>
>>> + */
>>> + ret = mmc_regulator_get_supply(mmc);
>>> + if (ret == -EPROBE_DEFER)
>>> + return ret;
>>> +
>>> sdhci_read_caps(host);
>>>
>>> override_timeout_clk = host->timeout_clk;
>>> @@ -3253,11 +3264,6 @@ int sdhci_setup_host(struct sdhci_host *host)
>>> mmc_gpio_get_cd(host->mmc) < 0)
>>> mmc->caps |= MMC_CAP_NEEDS_POLL;
>>>
>>> - /* If there are external regulators, get them */
>>> - ret = mmc_regulator_get_supply(mmc);
>>> - if (ret == -EPROBE_DEFER)
>>> - goto undma;
>>
>> All goto's before this now need to goto unreg.

Er, actually they don't!

So apart from the cosmetics above:

Acked-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>

>>
>>> -
>>> /* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
>>> if (!IS_ERR(mmc->supply.vqmmc)) {
>>> ret = regulator_enable(mmc->supply.vqmmc);
>>>
>>
>>
>
>