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 Lozanowalter.lozano@collabora.com wrote:
Signed-off-by: Walter Lozanowalter.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;@@ -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;elsepriv->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