| Message ID | 20180116171703.14361-2-marek.vasut+renesas@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Masahiro Yamada |
| Headers | show |
| Series | [U-Boot,1/7] mmc: uniphier-sd: Use mmc_of_parse() | expand |
2018-01-17 2:16 GMT+09:00 Marek Vasut <marek.vasut@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+renesas@gmail.com> > Cc: Jaehoon Chung <jh80.chung@samsung.com> > Cc: Masahiro Yamada <yamada.masahiro@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?
On 01/25/2018 11:23 AM, Masahiro Yamada wrote: > 2018-01-17 2:16 GMT+09:00 Marek Vasut <marek.vasut@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+renesas@gmail.com> >> Cc: Jaehoon Chung <jh80.chung@samsung.com> >> Cc: Masahiro Yamada <yamada.masahiro@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) ? No, rather the pinconf bit should be protected by ifdef CONFIG_PINCONF and there should be a __maybe_unused prepended to the mmc variable. >> +#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. Probably not all of them, so I can add a switch and set the pinconf to UHS only for UHS modes. >> 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? The regulator is optional, so no, it should be discarded. I'll drop the ret =.
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); + +#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"); +} + 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);
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+renesas@gmail.com> Cc: Jaehoon Chung <jh80.chung@samsung.com> Cc: Masahiro Yamada <yamada.masahiro@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(-)