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

[Walter Lozano]

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 at 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 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?
> 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