[RFC 1/7] mmc: fsl_esdhc_imx: add OF_PLATDATA support
Simon Glass
sjg at chromium.org
Thu Apr 9 21:36:32 CEST 2020
Hi Walter,
On Thu, 9 Apr 2020 at 13:07, Walter Lozano <walter.lozano at 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 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.
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.
Regards,
Simon
More information about the U-Boot
mailing list