Hi Walter, On Thu, 9 Apr 2020 at 13:44, Walter Lozano <walter.loz...@collabora.com> wrote: > > Hi Simon, > > On 9/4/20 16:36, Simon Glass wrote: > > Hi Walter, > > > > On Thu, 9 Apr 2020 at 13:07, Walter Lozano <walter.loz...@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.loz...@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.loz...@collabora.com> wrote: > >>>>>> Signed-off-by: Walter Lozano<walter.loz...@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.
OK sounds good. I should mention that the dtoc tool should be upstreamed to dtc. I was thinking of sending something very simple to start. Regards, SImon