| Message ID | 1468331617-22265-1-git-send-email-jonathanh@nvidia.com |
|---|---|
| State | Accepted |
| Headers | show |
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@nvidia.com> > --- > > 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. > + */ > + 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. > - > /* 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); > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@nvidia.com> >> --- >> >> 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. > >> - >> /* 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); >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@nvidia.com> >>> --- >>> >>> 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@intel.com> >> >>> - >>> /* 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); >>> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 July 2016 at 15:53, Jon Hunter <jonathanh@nvidia.com> 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 > regulators before reading the capabilites of the host controller. This > 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@nvidia.com> Thanks, applied for next! I changed the minor things pointed out by Adrian. Kind regards Uffe > --- > > 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(). > > 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. > + */ > + 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; > - > /* 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); > -- > 2.1.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" 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 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. + */ + 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; - /* 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);
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 regulators before reading the capabilites of the host controller. This 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@nvidia.com> --- 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(). drivers/mmc/host/sdhci.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)