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

[Walter Lozano <walter.lozano@collabora.com>]

Hi Simon,

On 9/4/20 16:36, Simon Glass wrote:
> Hi Walter,
>
> On Thu, 9 Apr 2020 at 13:07, Walter Lozano <walter.lozano@collabora.com> wrote:
>> 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 Lozano<walter.lozano@collabora.com>  wrote:
>>>>>> Signed-off-by: Walter Lozano<walter.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.
> I am wondering if we can use the DM_GET_DRIVER() macro somehow in
> dt_platdata.c. At present we emit a string, and that string matches
> the driver name, so we should be able to. That will give a compile
> error if something is wrong, much better than the current behaviour of
> not being able to bind the driver at runtime.
>
> This is just to improve the current impl, not to do what you are asking here.
>
> For what you want, our current approach is to use the first compatible
> string to find the driver name. Since all compatible strings point to
> the same driver, perhaps that is good enough? We should at least
> understand the limitations before going further.
>
> The main one I am aware of is that you need to duplicate the
> U_BOOT_DRIVER()xxx for each compatible string. To fix that we could
> add:
>
> U_BOOT_DRIVER_ALIAS(xxx, new_name)
>
> which creates a linker list symbol that points to the original driver,
> perhaps using ll_entry_get(). That should be easy enough I think. Then
> whichever symbol you use you will get the same driver since all the
> symbols point to it.
>
> Unfortunately the .data field is not available with of_platdata. That
> could be added to the dtd_... struct automatically by dtoc, I suspect.
> However that requires some clever parsing of the C code...
>
> As you can tell I would really like to avoid string comparisons and
> tables of compatible strings in the image itself. It adds overheade.


Thanks for taking the time to elaborate your idea, now is clear. I 
totally agree with you, the whole idea behind it to reduce the image 
size, so we need to work to avoid any kind of table of strings.


I will investigate you approach and come back to you.


Regards,

Walter