[RFC 1/7] mmc: fsl_esdhc_imx: add OF_PLATDATA support
Simon Glass
sjg at chromium.org
Tue Apr 21 19:44:58 CEST 2020
Hi Walter,
On Fri, 17 Apr 2020 at 14:05, Walter Lozano <walter.lozano at collabora.com> wrote:
>
> Hi Simon,
>
> On 9/4/20 16:50, Simon Glass wrote:
> > Hi Walter,
> >
> > On Thu, 9 Apr 2020 at 13:44, Walter Lozano <walter.lozano at 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.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.
> >>
> >> 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.
>
> I have been thinking in your suggestions and the main goal behind all
> these. With that in mind I think that the best approach we can use is to
> move as much complexity as we can to dtoc, which will give us
>
> - Reduction in TPL/SPL image
>
> - Warnings at compile time
Good ideas.
>
>
> So I was thinking in add the support for aliases to doc as
>
> - Add U_BOOT_DRIVER_ALIAS(name, new_name) in drivers which generate no
> code for U-Boot
>
> - Parse U_BOOT_DRIVER(name) to build a list of drivers in dtoc
>
> - Parse U_BOOT_DRIVER_ALIAS(name, new_name) to populate the list of
> drivers with aliases in dtoc
>
> - When parsing dts file, look for the proper driver name based on the
> list created, is no one is found raise an error
>
>
> I think the parser could be simple, something like (this is only a draft
> to show the idea)
>
> #!/usr/bin/python
> import os
> import re
>
> class UBootDriverList:
>
> def parse_u_boot_driver(self, fn):
> f = open(fn)
>
> b = f.read()
>
> drivers = re.findall('U_BOOT_DRIVER\((.*)\)', b)
>
> for d in drivers:
> self.driver_list.append(d)
>
> def parse_u_boot_drivers(self):
> self.driver_list = []
> for (dirpath, dirnames, filenames) in os.walk('./'):
> for fn in filenames:
> if not fn.endswith('.c'):
> continue
> self.parse_u_boot_driver(dirpath + '/' + fn)
>
> def print_list(self):
> for d in self.driver_list:
> print d
>
> driver_list = UBootDriverList()
> driver_list.parse_u_boot_drivers()
> driver_list.print_list()
>
> What do you think?
>
> If this make sense we can try to go also in this way for other improvements.
LGTM sounds nice.
Regards,
Simon
More information about the U-Boot
mailing list