2018-01-17 2:16 GMT+09:00 Marek Vasut <marek.va...@gmail.com>: > Factor out the regulator handling into set_ios and add support for > selecting pin configuration based on the voltage to support UHS modes. > > Signed-off-by: Marek Vasut <marek.vasut+rene...@gmail.com> > Cc: Jaehoon Chung <jh80.ch...@samsung.com> > Cc: Masahiro Yamada <yamada.masah...@socionext.com> > --- > V2: Protect vqmmc_dev access in uniphier_sd_set_pins() with an ifdef > just like everywhere else > --- > drivers/mmc/uniphier-sd.c | 35 ++++++++++++++++++++++++++--------- > 1 file changed, 26 insertions(+), 9 deletions(-) > > diff --git a/drivers/mmc/uniphier-sd.c b/drivers/mmc/uniphier-sd.c > index 552e37d852..e6c610a22a 100644 > --- a/drivers/mmc/uniphier-sd.c > +++ b/drivers/mmc/uniphier-sd.c > @@ -10,6 +10,7 @@ > #include <fdtdec.h> > #include <mmc.h> > #include <dm.h> > +#include <dm/pinctrl.h> > #include <linux/compat.h> > #include <linux/dma-direction.h> > #include <linux/io.h> > @@ -134,6 +135,9 @@ struct uniphier_sd_priv { > #define UNIPHIER_SD_CAP_DMA_INTERNAL BIT(1) /* have internal DMA engine */ > #define UNIPHIER_SD_CAP_DIV1024 BIT(2) /* divisor 1024 is > available */ > #define UNIPHIER_SD_CAP_64BIT BIT(3) /* Controller is 64bit */ > +#ifdef CONFIG_DM_REGULATOR > + struct udevice *vqmmc_dev; > +#endif > }; > > static u64 uniphier_sd_readq(struct uniphier_sd_priv *priv, unsigned int reg) > @@ -676,6 +680,26 @@ static void uniphier_sd_set_clk_rate(struct > uniphier_sd_priv *priv, > udelay(1000); > } > > +static void uniphier_sd_set_pins(struct udevice *dev) > +{ > + struct uniphier_sd_priv *priv = dev_get_priv(dev); > + struct mmc *mmc = mmc_get_mmc_dev(dev);
This gives me a new warning for my board, where CONFIG_DM_REGULATOR is disabled. drivers/mmc/uniphier-sd.c:685:27: warning: unused variable ‘priv’ [-Wunused-variable] struct uniphier_sd_priv *priv = dev_get_priv(dev); ^~~~ Is it reasonable to surround the whole this function by #if CONFIG_IS_ENABLED(MMC_IO_VOLTAGE) ? > +#ifdef CONFIG_DM_REGULATOR > + if (priv->vqmmc_dev) { > + if (mmc->signal_voltage == MMC_SIGNAL_VOLTAGE_180) > + regulator_set_value(priv->vqmmc_dev, 1800000); > + else > + regulator_set_value(priv->vqmmc_dev, 3300000); > + } > +#endif > + > + if (mmc->signal_voltage == MMC_SIGNAL_VOLTAGE_180) > + pinctrl_select_state(dev, "state_uhs"); > + else > + pinctrl_select_state(dev, "default"); > +} I am not sure about this code. If MMC_SIGNAL_VOLTAGE_180 is set, is it always UHS? eMMC also can do 1.8V signaling. > static int uniphier_sd_set_ios(struct udevice *dev) > { > struct uniphier_sd_priv *priv = dev_get_priv(dev); > @@ -690,6 +714,7 @@ static int uniphier_sd_set_ios(struct udevice *dev) > return ret; > uniphier_sd_set_ddr_mode(priv, mmc); > uniphier_sd_set_clk_rate(priv, mmc); > + uniphier_sd_set_pins(dev); > > return 0; > } > @@ -757,9 +782,6 @@ static int uniphier_sd_probe(struct udevice *dev) > fdt_addr_t base; > struct clk clk; > int ret; > -#ifdef CONFIG_DM_REGULATOR > - struct udevice *vqmmc_dev; > -#endif > > base = devfdt_get_addr(dev); > if (base == FDT_ADDR_T_NONE) > @@ -770,12 +792,7 @@ static int uniphier_sd_probe(struct udevice *dev) > return -ENOMEM; > > #ifdef CONFIG_DM_REGULATOR > - ret = device_get_supply_regulator(dev, "vqmmc-supply", &vqmmc_dev); > - if (!ret) { > - /* Set the regulator to 3.3V until we support 1.8V modes */ > - regulator_set_value(vqmmc_dev, 3300000); > - regulator_set_enable(vqmmc_dev, true); > - } > + ret = device_get_supply_regulator(dev, "vqmmc-supply", > &priv->vqmmc_dev); > #endif > > ret = clk_get_by_index(dev, 0, &clk); The return value from device_get_supply_regulator() is overwritten by the following clk_get_by_index(). Shouldn't it be checked? -- Best Regards Masahiro Yamada _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot