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

Hi Simon,

On 8/4/20 00:14, Simon Glass wrote:

Hi Walter,

On Tue, 7 Apr 2020 at 14:05, Walter Lozano walter.lozano@collabora.com wrote:

...

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 Lozanowalter.lozano@collabora.com wrote:

...

Signed-off-by: Walter Lozanowalter.lozano@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?

Yes I think so.

Actually searching for a string is not great anyway. I wonder if we can use the linker-list idea in the previous email somehow?

I'm sure I' understand the idea you try to share with me. Sorry, I understand that one limitation of the current implementation of OF_PLATDATA is the fact that the U_BOOT_DRIVER name should match the one used as first entry in DT. As in particular this driver has several compatible

{ .compatible = "fsl,imx53-esdhc", },         { .compatible = "fsl,imx6ul-usdhc", },         { .compatible = "fsl,imx6sx-usdhc", },         { .compatible = "fsl,imx6sl-usdhc", },         { .compatible = "fsl,imx6q-usdhc", },         { .compatible = "fsl,imx7d-usdhc", .data = (ulong)&usdhc_imx7d_data,},         { .compatible = "fsl,imx7ulp-usdhc", },         { .compatible = "fsl,imx8qm-usdhc", .data = (ulong)&usdhc_imx8qm_data,},         { .compatible = "fsl,imx8mm-usdhc", .data = (ulong)&usdhc_imx8qm_data,},         { .compatible = "fsl,imx8mn-usdhc", .data = (ulong)&usdhc_imx8qm_data,},         { .compatible = "fsl,imx8mq-usdhc", .data = (ulong)&usdhc_imx8qm_data,},         { .compatible = "fsl,imxrt-usdhc", },

and in order to create a general solution, we need to search for the compatible string instead of matching for driver name.

Could you please elaborate a bit more your suggestion in order to understand your approach.

Thanks in advance.

Regards, SImon

Regards,

Walter