On 01/25/2018 11:23 AM, Masahiro Yamada wrote:
> 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) ?
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 =.
--
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
