by Aapo Vienamo

[permalink] [raw]

Subject: Re: [PATCH v2 03/10] mmc: tegra: Reconfigure pad voltages during voltage switching

On Tue, 31 Jul 2018 11:15:41 +0200
Stefan Agner <[email protected]> wrote:

> On 27.07.2018 10:44, Aapo Vienamo wrote:
> > On Thu, 26 Jul 2018 15:33:11 +0200
> > Stefan Agner <[email protected]> wrote:
> >
> >> On 26.07.2018 14:19, Aapo Vienamo wrote:
> >> > Parse the pinctrl state and nvidia,only-1-8-v properties from the device
> >> > tree and implement pad voltage state reconfiguration in the mmc
> >> > start_signal_voltage_switch() callback. Validate the pinctrl and
> >> > regulator configuration before unmasking UHS modes. Add
> >> > NVQUIRK_NEEDS_PAD_CONTROL and add set it for Tegra210 and Tegra186.
> >> >
> >> > The pad configuration is done in the mmc callback because the order of
> >> > pad reconfiguration and sdhci voltage switch depend on the voltage to
> >> > which the transition occurs.
> >> >
> >> > Signed-off-by: Aapo Vienamo <[email protected]>
> >> > ---
> >> > drivers/mmc/host/sdhci-tegra.c | 140 +++++++++++++++++++++++++++++++++++++----
> >> > 1 file changed, 127 insertions(+), 13 deletions(-)
> >> >
> >> > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> >> > index ddf00166..9587365 100644
> >> > --- a/drivers/mmc/host/sdhci-tegra.c
> >> > +++ b/drivers/mmc/host/sdhci-tegra.c
> >> > @@ -21,6 +21,7 @@
> >> > #include <linux/io.h>
> >> > #include <linux/of.h>
> >> > #include <linux/of_device.h>
> >> > +#include <linux/pinctrl/consumer.h>
> >> > #include <linux/reset.h>
> >> > #include <linux/mmc/card.h>
> >> > #include <linux/mmc/host.h>
> >> > @@ -55,6 +56,7 @@
> >> > #define NVQUIRK_ENABLE_SDR104 BIT(4)
> >> > #define NVQUIRK_ENABLE_DDR50 BIT(5)
> >> > #define NVQUIRK_HAS_PADCALIB BIT(6)
> >> > +#define NVQUIRK_NEEDS_PAD_CONTROL BIT(7)
> >> >
> >> > struct sdhci_tegra_soc_data {
> >> > const struct sdhci_pltfm_data *pdata;
> >> > @@ -66,8 +68,13 @@ struct sdhci_tegra {
> >> > struct gpio_desc *power_gpio;
> >> > bool ddr_signaling;
> >> > bool pad_calib_required;
> >> > + bool pad_control_available;
> >> > + bool only_1v8;
> >> >
> >> > struct reset_control *rst;
> >> > + struct pinctrl *pinctrl_sdmmc;
> >> > + struct pinctrl_state *pinctrl_state_3v3;
> >> > + struct pinctrl_state *pinctrl_state_1v8;
> >> > };
> >> >
> >> > static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg)
> >> > @@ -138,12 +145,47 @@ static unsigned int tegra_sdhci_get_ro(struct
> >> > sdhci_host *host)
> >> > return mmc_gpio_get_ro(host->mmc);
> >> > }
> >> >
> >> > +static bool tegra_sdhci_is_uhs_valid(struct sdhci_host *host)
> >> > +{
> >> > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> > + struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
> >> > + const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data;
> >> > +
> >> > + /*
> >> > + * The 1.8 V only host controllers don't need to have configurable
> >> > + * regulators and pad voltages. In this case the UHS modes can be
> >> > + * enabled regardless.
> >> > + */
> >> > + if (tegra_host->only_1v8)
> >> > + return true;
> >>
> >> Hm, SD always needs 3.3V capabilities, not?
> >
> > Yes, the controllers used for eMMCs should have the only_1v8 property
> > set and they exit the function here. With controllers used for SD cards,
> > the regulator and pad configurations are verified later in the function.
> >
>
> By returning true you ask the controller to enable UHS modes. However,
> if you have 1.8V only, there should be no UHS modes advertised (since SD
> cards require 1.8V).
>
> That assumes that UHS modes are a SD card thing only (which I think
> strictly speaking should be the case).
>
> However, at least on Tegra 3 it seems that enabling SD 3.0 is also
> required for higher eMMC speeds:
> https://patchwork.kernel.org/patch/10521147/
>
> So UHS is probably not so much about SD card UHS modes but more about
> higher speed modes in general...

True.

> >> So if the controller is 1.8V only, then only eMMC modes are allowed.
> >>
> >> You can use MMC_CAP2_HSX00_1_8V in caps2.
> >
> > I can't quite follow you, how this should be used here?
> >
>
> Similar to
>
> if (tegra_host->soc_data->nvquirks & NVQUIRK_ENABLE_DDR50)
>
> host->mmc->caps |= MMC_CAP_1_8V_DDR
>
> You can enable higher speed eMMC modes without advertising SD card
> modes.

Makes sense. Thanks for clearing that up.

> >> So far the Tegra SDHCI driver did not use device tree to indicate modes,
> >> but maybe we should start doing that. In this case you can use
> >> mmc-hs200-1_8v/mmc-hs400-1_8v to indicate higher eMMC speed modes.
> >>
> >> > +
> >> > + /*
> >> > + * If the board does not define a regulator for the SDHCI IO voltage,
> >> > + * then don't advertise support for UHS modes even if the device
> >> > + * supports it because the IO voltage cannot be configured.
> >> > + */
> >> > + if (IS_ERR(host->mmc->supply.vqmmc))
> >> > + return false;
> >>
> >> The stack should already check that and mask caps1 accordingly (see
> >> sdhci_setup_host).
> >
> > True, the logic seems to be duplicated here. Although also
> > tegra_sdhci_reset() needs to be change so that the bits masked off by
> > sdhci_setup_host() aren't set if this check is removed.
> >
>
> sdhci_setup_host() calls reset before doing initialization (e.g. in
> __sdhci_read_caps).
>
> Checking just for vqmmc presence is bogus IMHO. It does not say anything
> about the exact capabilities of that regulator.

Yes, by itself it doesn't make much sense. It looks like the regulator
voltage checking code from sdhci_setup_host() has to be duplicated to
the tegra driver because the regulator checks are done after the reset
callback. The sdhci-tegra driver needs to make sure that the faster
signaling modes are enabled only if pinctrl properties are present on SD
controllers with variable voltage regulators.

> I think we should do something like this:
>
> if (there is a host->mmc->supply.vqmmc) {
> if (host->mmc->supply.vqmmc can do 1.8V (or/and also 1.2V?)) {
> /*
> * Enable SDHCI spec v3.00 support
> * This is required for SD UHS modes as well as higher eMMC speeds
> */
> if (soc_data->nvquirks & NVQUIRK_ENABLE_SDHCI_SPEC_300)
> misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDHCI_SPEC_300;
>
> if (host->mmc->supply.vqmmc can do 3.3V) {
> /*
> * Proper SD voltage switching is possible, advertise SD UHS
> * modes as supported by host
> */
> if (soc_data->nvquirks & NVQUIRK_ENABLE_SDR50)
> misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDR50;
> if (soc_data->nvquirks & NVQUIRK_ENABLE_DDR50)
> misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_DDR50;
> if (soc_data->nvquirks & NVQUIRK_ENABLE_SDR104)
> misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDR104;
> if (soc_data->nvquirks & SDHCI_MISC_CTRL_ENABLE_SDR50)
> clk_ctrl |= SDHCI_CLOCK_CTRL_SDR50_TUNING_OVERRIDE;
> }
> }
> }
>
>
>
>
> >> > +
> >> > + /*
> >> > + * Later SoC generations require software pad voltage configuration.
> >> > + * The UHS modes should only be enabled if the pad configuration states
> >> > + * are available on platforms where they are required in order to switch
> >> > + * the signaling voltage.
> >> > + */
> >> > + if (soc_data->nvquirks & NVQUIRK_NEEDS_PAD_CONTROL)
> >> > + return tegra_host->pad_control_available;
> >> > +
> >> > + return false;
> >> > +}
> >> > +
> >> > static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask)
> >> > {
> >> > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> > struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
> >> > const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data;
> >> > u32 misc_ctrl, clk_ctrl;
> >> > + bool uhs_valid;
> >> >
> >> > sdhci_reset(host, mask);
> >> >
> >> > @@ -160,13 +202,8 @@ static void tegra_sdhci_reset(struct sdhci_host
> >> > *host, u8 mask)
> >> >
> >> > clk_ctrl &= ~SDHCI_CLOCK_CTRL_SPI_MODE_CLKEN_OVERRIDE;
> >> >
> >> > - /*
> >> > - * If the board does not define a regulator for the SDHCI
> >> > - * IO voltage, then don't advertise support for UHS modes
> >> > - * even if the device supports it because the IO voltage
> >> > - * cannot be configured.
> >> > - */
> >> > - if (!IS_ERR(host->mmc->supply.vqmmc)) {
> >> > + uhs_valid = tegra_sdhci_is_uhs_valid(host);
> >> > + if (uhs_valid) {
> >> > /* Erratum: Enable SDHCI spec v3.00 support */
> >> > if (soc_data->nvquirks & NVQUIRK_ENABLE_SDHCI_SPEC_300)
> >> > misc_ctrl |= SDHCI_MISC_CTRL_ENABLE_SDHCI_SPEC_300;
> >> > @@ -286,14 +323,80 @@ static int tegra_sdhci_execute_tuning(struct
> >> > sdhci_host *host, u32 opcode)
> >> > return mmc_send_tuning(host->mmc, opcode, NULL);
> >> > }
> >> >
> >> > -static void tegra_sdhci_voltage_switch(struct sdhci_host *host)
> >> > +static int tegra_sdhci_set_padctrl(struct sdhci_host *host, int voltage)
> >> > {
> >> > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> > struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
> >> > - const struct sdhci_tegra_soc_data *soc_data = tegra_host->soc_data;
> >> > + int ret;
> >> > +
> >> > + if (!tegra_host->pad_control_available)
> >> > + return 0;
> >> > +
> >> > + if (voltage == MMC_SIGNAL_VOLTAGE_180) {
> >> > + ret = pinctrl_select_state(tegra_host->pinctrl_sdmmc,
> >> > + tegra_host->pinctrl_state_1v8);
> >> > + if (ret < 0)
> >> > + dev_err(mmc_dev(host->mmc),
> >> > + "setting 1.8V failed, ret: %d\n", ret);
> >> > + } else {
> >> > + ret = pinctrl_select_state(tegra_host->pinctrl_sdmmc,
> >> > + tegra_host->pinctrl_state_3v3);
> >> > + if (ret < 0)
> >> > + dev_err(mmc_dev(host->mmc),
> >> > + "setting 3.3V failed, ret: %d\n", ret);
> >> > + }
> >> >
> >> > - if (soc_data->nvquirks & NVQUIRK_HAS_PADCALIB)
> >> > - tegra_host->pad_calib_required = true;
>
> Is this really not needed anymore?

Good catch. That probably happened accidentally during a rebase.

> >> > + return ret;
> >> > +}
> >> > +
> >> > +static int sdhci_tegra_start_signal_voltage_switch(struct mmc_host *mmc,
> >> > + struct mmc_ios *ios)
> >> > +{
> >> > + struct sdhci_host *host = mmc_priv(mmc);
> >> > + int ret = 0;
> >> > +
> >> > + if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
> >> > + ret = tegra_sdhci_set_padctrl(host, ios->signal_voltage);
> >> > + if (ret < 0)
> >> > + return ret;
> >> > + ret = sdhci_start_signal_voltage_switch(mmc, ios);
> >> > + } else if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
> >> > + ret = sdhci_start_signal_voltage_switch(mmc, ios);
> >> > + if (ret < 0)
> >> > + return ret;
> >> > + ret = tegra_sdhci_set_padctrl(host, ios->signal_voltage);
> >>
> >> So depending on voltage you first set the pads setting and then do the
> >> signaling voltage switch. Is this necessary? Why?
> >
> > This is done to enforce the constraint of keeping the signaling voltage
> > always less or equal to the pad voltage. Driving 3.3 V into a pad that
> > has been configured to 1.8 V will damage it. This is stated in the
> > Tegra210 and Tegra186 TRMs in SDMMC Initialization Sequece sections of
> > controllers supporting voltage switching.
> >
> > Here when switching from 3.3 V to 1.8 V the regulator is first
> > configured to 1.8 V by sdhci_start_signal_voltage_switch() and after
> > that the pad is configured to 1.8 V. When going in the other direction
> > the pad needs to be first configured to 3.3 V before the regulator
> > voltage can be raised.
>
> Ok makes sense. Can you add a comment, e.g. /* Order of operation
> matters, it make sure to keep signaling voltage below pad voltage */
>
> >
> >> > + }
> >> > +
> >> > + return ret;
> >> > +}
> >> > +
> >> > +static void tegra_sdhci_init_pinctrl_info(struct device *dev,
> >> > + struct sdhci_tegra *tegra_host)
> >> > +{
> >> > + tegra_host->pinctrl_sdmmc = devm_pinctrl_get(dev);
> >> > + if (IS_ERR(tegra_host->pinctrl_sdmmc)) {
> >> > + dev_dbg(dev, "No pinctrl info, err: %ld\n",
> >> > + PTR_ERR(tegra_host->pinctrl_sdmmc));
> >> > + return;
> >> > + }
> >> > +
> >> > + tegra_host->pinctrl_state_3v3 =
> >> > + pinctrl_lookup_state(tegra_host->pinctrl_sdmmc, "sdmmc-3v3");
> >> > + if (IS_ERR(tegra_host->pinctrl_state_3v3)) {
> >> > + dev_err(dev, "Missing 3.3V pad state, err: %ld\n",
> >> > + PTR_ERR(tegra_host->pinctrl_state_3v3));
> >> > + return;
> >> > + }
> >> > +
> >> > + tegra_host->pinctrl_state_1v8 =
> >> > + pinctrl_lookup_state(tegra_host->pinctrl_sdmmc, "sdmmc-1v8");
> >> > + if (IS_ERR(tegra_host->pinctrl_state_1v8)) {
> >> > + dev_err(dev, "Missing 1.8V pad state, err: %ld\n",
> >> > + PTR_ERR(tegra_host->pinctrl_state_3v3));
> >>
> >> Is missing pad states considered as error? If yes, we should return a
> >> error code and make the probe function fail.
> >>
> >> If it is ok to run it without the states, then this should be dev_warn
> >> or dev_info.
> >
> > I don't think failing probe due to missing pinctrl entries can be done
> > because it would break backwards compatibility with pre-existing dtbs.
> > Also SDHCI controllers with fixed signaling voltage don't support pad
> > voltage reconfiguration and therefore the pinctrl properties won't be
> > there either. In the case of fixed voltage SDHCI controllers the
> > "nvidia,only-1-8-v" property is used to signal that pad reconfiguration
> > doesn't need to be performed.
>
> Ok, if they are not mandatory, just use dev_warn(...).
>
> Btw,
>
> + if (soc_data->nvquirks & NVQUIRK_NEEDS_PAD_CONTROL) {
> + host->mmc_host_ops.start_signal_voltage_switch =
> + sdhci_tegra_start_signal_voltage_switch;
> + tegra_sdhci_init_pinctrl_info(&pdev->dev, tegra_host);
> + }
> +
>
> Can we make tegra_sdhci_init_pinctrl_info return a boolean and do the
> tegra_host->pad_control_available assignment here? I think this would be
> cleaner.
>
> It also won't assign host->mmc_host_ops.start_signal_voltage_switch if
> not needed.
>
> if (soc_data->nvquirks & NVQUIRK_NEEDS_PAD_CONTROL &&
> tegra_sdhci_init_pinctrl_info(&pdev->dev, tegra_host)) {
> host->mmc_host_ops.start_signal_voltage_switch =
> sdhci_tegra_start_signal_voltage_switch;
> tegra_host->pad_control_available = true;
> }

-Aapo