[RFC 1/7] mmc: fsl_esdhc_imx: add OF_PLATDATA support

[Walter Lozano]

Hi Simon,

Thank you for taking the time to review this series.

On 6/4/20 00:42, Simon Glass wrote:
> Hi Walter,
>
> On Sun, 29 Mar 2020 at 21:32, Walter Lozano<walter.lozano at collabora.com>  wrote:
>> Signed-off-by: Walter Lozano<walter.lozano at collabora.com>
>> ---
>>   drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++----
>>   1 file changed, 42 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
>> index 4900498e9b..761a4b46e9 100644
>> --- a/drivers/mmc/fsl_esdhc_imx.c
>> +++ b/drivers/mmc/fsl_esdhc_imx.c
>> @@ -29,6 +29,8 @@
>>   #include <dm.h>
>>   #include <asm-generic/gpio.h>
>>   #include <dm/pinctrl.h>
>> +#include <dt-structs.h>
>> +#include <mapmem.h>
>>
>>   #if !CONFIG_IS_ENABLED(BLK)
>>   #include "mmc_private.h"
>> @@ -98,6 +100,11 @@ struct fsl_esdhc {
>>   };
>>
>>   struct fsl_esdhc_plat {
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> +       /* Put this first since driver model will copy the data here */
>> +       struct dtd_fsl_imx6q_usdhc dtplat;
>> +#endif
>> +
>>          struct mmc_config cfg;
>>          struct mmc mmc;
>>   };
>> @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev)
>>          struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
>>          struct fsl_esdhc_plat *plat = dev_get_platdata(dev);
>>          struct fsl_esdhc_priv *priv = dev_get_priv(dev);
>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA)
>>          const void *fdt = gd->fdt_blob;
>>          int node = dev_of_offset(dev);
>> +       fdt_addr_t addr;
>> +#else
>> +       struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat;
>> +#endif
>>          struct esdhc_soc_data *data =
>>                  (struct esdhc_soc_data *)dev_get_driver_data(dev);
>>   #if CONFIG_IS_ENABLED(DM_REGULATOR)
>>          struct udevice *vqmmc_dev;
>>   #endif
>> -       fdt_addr_t addr;
>>          unsigned int val;
>>          struct mmc *mmc;
>>   #if !CONFIG_IS_ENABLED(BLK)
>> @@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev)
>>   #endif
>>          int ret;
>>
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> +       priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]);
>> +       val = plat->dtplat.bus_width;
>> +       if (val == 8)
>> +               priv->bus_width = 8;
>> +       else if (val == 4)
>> +               priv->bus_width = 4;
>> +       else
>> +               priv->bus_width = 1;
>> +       priv->non_removable = 1;
>> +#else
>>          addr = dev_read_addr(dev);
>>          if (addr == FDT_ADDR_T_NONE)
>>                  return -EINVAL;
>>          priv->esdhc_regs = (struct fsl_esdhc *)addr;
>>          priv->dev = dev;
>>          priv->mode = -1;
>> -       if (data)
>> -               priv->flags = data->flags;
>>
>>          val = dev_read_u32_default(dev, "bus-width", -1);
>>          if (val == 8)
>> @@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev)
>>                          priv->vs18_enable = 1;
>>          }
>>   #endif
>> -
>> +#endif
>> +       if (data)
>> +               priv->flags = data->flags;
>>          /*
>>           * TODO:
>>           * Because lack of clk driver, if SDHC clk is not enabled,
>> @@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev)
>>                  return ret;
>>          }
>>
>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA)
> Maybe can use if() for this one?

Thank you for the suggestion

>>          ret = mmc_of_parse(dev, &plat->cfg);
>>          if (ret)
>>                  return ret;
>> +#endif
>>
>>          mmc = &plat->mmc;
>>          mmc->cfg = &plat->cfg;
>> @@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = {
>>          .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat),
>>          .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv),
>>   };
>> +
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> Don't you already have a U_BOOT_DRIVER declaration?
>
> You may need to change the name of your existing driver though (see
> of-platdata docs).
>
> So if it is because of that, please add a comment.

I have my doubts regarding this issue. As I see, this driver is used by 
many different DT with different compatible strings, so I'm thinking in 
trying to find a more generic approach. Would it be useful to have a 
more elaborated solution searching for the compatible string when 
matching drivers with device?

>> +U_BOOT_DRIVER(fsl_usdhc) = {
>> +       .name   = "fsl_imx6q_usdhc",
>> +       .id     = UCLASS_MMC,
>> +       .ops    = &fsl_esdhc_ops,
>> +#if CONFIG_IS_ENABLED(BLK)
>> +       .bind   = fsl_esdhc_bind,
>> +#endif
>> +       .probe  = fsl_esdhc_probe,
>> +       .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat),
>> +       .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv),
>> +};
>> +#endif
>>   #endif
>> --
>> 2.20.1
>>
> Regards,
> Simon