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
