[U-Boot] [PATCH V2 2/7] mmc: uniphier-sd: Properly handle pin voltage configuration

[Marek Vasut <marek.vasut@gmail.com>]

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 =.

-- 
Best regards,
Marek Vasut