[RFC 0/7] mx6cuboxi: enable OF_PLATDATA with MMC support

[RFC 0/7] mx6cuboxi: enable OF_PLATDATA with MMC support

[Walter Lozano]

The SPL in iMX6 boards is restricted to 68 KB as this is the free available
space in OCRAM for most revisions. In this context, adding OF_CONTROL and DM
increases the SPL size which could make it difficult to add specific features
required for custom scenarios.

These patches aim to take advantage of OF_PLATADATA in order to reduce the SPL
size in this scenario, by parsing DT data to generate platdata structures,
and thus removing the overhead caused by DT and related libraries.

This series is focused in MMC driver, which is used for boot in boards such as
Cubox-i. Also, in order to support CD, the OF_PLATDATA support is also
implemented on GPIO driver.

To make possible to link the CD information found in platdata with a GPIO,
a new API is suggested, to find/get a device based on its platdata. This
new API was discussed in [1] but the lack of context made the discussion
not to progress. With this series, the general idea should be clearer,
so a better solution could be discussed.

Finally, in order to make use of these new features, enable OF_PLATADATA for
Cubox-i board, for which OF_CONTROL support is being discussed in [2].

[1] https://patchwork.ozlabs.org/patch/1249198/
[2] https://patchwork.ozlabs.org/project/uboot/list/?series=163738

Walter Lozano (7):
  mmc: fsl_esdhc_imx: add OF_PLATDATA support
  mmc: fsl_esdhc_imx: add ofdata_to_platdata support
  dtoc: update dtb_platdata to support cd-gpio
  dm: uclass: add functions to get device by platdata
  gpio: mxc_gpio: add OF_PLATDATA support
  mmc: fsl_esdhc_imx: add CD support when OF_PLATDATA is enabled
  mx6cuboxi: enable OF_PLATDATA

 configs/mx6cuboxi_defconfig  |   2 +
 drivers/core/device.c        |  19 +++++++
 drivers/core/uclass.c        |  34 ++++++++++++
 drivers/gpio/mxc_gpio.c      |  27 ++++++++-
 drivers/mmc/fsl_esdhc_imx.c  | 105 ++++++++++++++++++++++++++++++-----
 include/dm/device.h          |  11 ++++
 include/dm/uclass-internal.h |  15 +++++
 include/dm/uclass.h          |  15 +++++
 tools/dtoc/dtb_platdata.py   |   9 ++-
 9 files changed, 218 insertions(+), 19 deletions(-)

-- 
2.20.1

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

[Walter Lozano]

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)
 	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)
+U_BOOT_DRIVER(fsl_usdhc) = {
+	.name	= "fsl_imx6q_usdhc",
+	.id	= UCLASS_MMC,
+	.ops	= &fsl_esdhc_ops,
+#if CONFIG_IS_ENABLED(BLK)
+	.bind	= fsl_esdhc_bind,
+#endif
+	.probe	= fsl_esdhc_probe,
+	.platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat),
+	.priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv),
+};
+#endif
 #endif
-- 
2.20.1

[RFC 2/7] mmc: fsl_esdhc_imx: add ofdata_to_platdata support

[Walter Lozano]

Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
 drivers/mmc/fsl_esdhc_imx.c | 71 ++++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 29 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
index 761a4b46e9..049a1b6ea8 100644
--- a/drivers/mmc/fsl_esdhc_imx.c
+++ b/drivers/mmc/fsl_esdhc_imx.c
@@ -1379,41 +1379,20 @@ __weak void init_clk_usdhc(u32 index)
 {
 }
 
-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);
+static int fsl_esdhc_ofdata_to_platdata(struct udevice *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);
+	struct fsl_esdhc_priv *priv = dev_get_priv(dev);
 #if CONFIG_IS_ENABLED(DM_REGULATOR)
 	struct udevice *vqmmc_dev;
+	int ret;
 #endif
+	const void *fdt = gd->fdt_blob;
+	int node = dev_of_offset(dev);
+
+	fdt_addr_t addr;
 	unsigned int val;
-	struct mmc *mmc;
-#if !CONFIG_IS_ENABLED(BLK)
-	struct blk_desc *bdesc;
-#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;
@@ -1483,8 +1462,40 @@ static int fsl_esdhc_probe(struct udevice *dev)
 	}
 #endif
 #endif
+	return 0;
+}
+
+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);
+	struct esdhc_soc_data *data =
+		(struct esdhc_soc_data *)dev_get_driver_data(dev);
+	struct mmc *mmc;
+#if !CONFIG_IS_ENABLED(BLK)
+	struct blk_desc *bdesc;
+#endif
+	int ret;
+
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
+	struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat;
+	unsigned int val;
+
+	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;
+#endif
+
 	if (data)
 		priv->flags = data->flags;
+
 	/*
 	 * TODO:
 	 * Because lack of clk driver, if SDHC clk is not enabled,
@@ -1664,6 +1675,7 @@ U_BOOT_DRIVER(fsl_esdhc) = {
 	.name	= "fsl-esdhc-mmc",
 	.id	= UCLASS_MMC,
 	.of_match = fsl_esdhc_ids,
+	.ofdata_to_platdata = fsl_esdhc_ofdata_to_platdata,
 	.ops	= &fsl_esdhc_ops,
 #if CONFIG_IS_ENABLED(BLK)
 	.bind	= fsl_esdhc_bind,
@@ -1677,6 +1689,7 @@ U_BOOT_DRIVER(fsl_esdhc) = {
 U_BOOT_DRIVER(fsl_usdhc) = {
 	.name	= "fsl_imx6q_usdhc",
 	.id	= UCLASS_MMC,
+	.ofdata_to_platdata = fsl_esdhc_ofdata_to_platdata,
 	.ops	= &fsl_esdhc_ops,
 #if CONFIG_IS_ENABLED(BLK)
 	.bind	= fsl_esdhc_bind,
-- 
2.20.1

[RFC 3/7] dtoc: update dtb_platdata to support cd-gpio

[Walter Lozano]

Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
 tools/dtoc/dtb_platdata.py | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
index 037e82c8bb..c52da7925e 100644
--- a/tools/dtoc/dtb_platdata.py
+++ b/tools/dtoc/dtb_platdata.py
@@ -211,7 +211,7 @@ class DtbPlatdata(object):
         Return:
             Number of argument cells is this is a phandle, else None
         """
-        if prop.name in ['clocks']:
+        if prop.name in ['clocks', 'cd-gpios']:
             if not isinstance(prop.value, list):
                 prop.value = [prop.value]
             val = prop.value
@@ -231,8 +231,11 @@ class DtbPlatdata(object):
                 if not target:
                     raise ValueError("Cannot parse '%s' in node '%s'" %
                                      (prop.name, node_name))
-                prop_name = '#clock-cells'
-                cells = target.props.get(prop_name)
+                cells = None
+                for prop_name in ['#clock-cells', '#gpio-cells']:
+                    cells = target.props.get(prop_name)
+                    if cells:
+                        break
                 if not cells:
                     raise ValueError("Node '%s' has no '%s' property" %
                             (target.name, prop_name))
-- 
2.20.1

[RFC 4/7] dm: uclass: add functions to get device by platdata

[Walter Lozano]

When OF_PLATDATA is enabled DT information is parsed and platdata
structures are populated. In this context the links between DT nodes are
represented as pointers to platdata structures, and there is no clear way
to access to the device which owns the structure.

This patch implements a set of functions:

- device_find_by_platdata
- uclass_find_device_by_platdata

to access to the device.

Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
 drivers/core/device.c        | 19 +++++++++++++++++++
 drivers/core/uclass.c        | 34 ++++++++++++++++++++++++++++++++++
 include/dm/device.h          | 11 +++++++++++
 include/dm/uclass-internal.h | 15 +++++++++++++++
 include/dm/uclass.h          | 15 +++++++++++++++
 5 files changed, 94 insertions(+)

diff --git a/drivers/core/device.c b/drivers/core/device.c
index 89ea820d48..54a3a8d870 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -591,6 +591,25 @@ static int device_find_by_ofnode(ofnode node, struct udevice **devp)
 }
 #endif
 
+
+int device_find_by_platdata(void *platdata, struct udevice **devp)
+{
+	struct uclass *uc;
+	struct udevice *dev;
+	int ret;
+
+	list_for_each_entry(uc, &gd->uclass_root, sibling_node) {
+		ret = uclass_find_device_by_platdata(uc->uc_drv->id, platdata,
+						   &dev);
+		if (!ret || dev) {
+			*devp = dev;
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+
 int device_get_child(const struct udevice *parent, int index,
 		     struct udevice **devp)
 {
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
index 58b19a4210..7b0ae5b122 100644
--- a/drivers/core/uclass.c
+++ b/drivers/core/uclass.c
@@ -271,6 +271,29 @@ int uclass_find_device_by_name(enum uclass_id id, const char *name,
 	return -ENODEV;
 }
 
+int uclass_find_device_by_platdata(enum uclass_id id, void * platdata, struct udevice **devp)
+{
+	struct uclass *uc;
+	struct udevice *dev;
+	int ret;
+
+	*devp = NULL;
+	ret = uclass_get(id, &uc);
+	if (ret)
+		return ret;
+	if (list_empty(&uc->dev_head))
+		return -ENODEV;
+
+	uclass_foreach_dev(dev, uc) {
+		if (dev->platdata == platdata) {
+			*devp = dev;
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+
 int uclass_find_next_free_req_seq(enum uclass_id id)
 {
 	struct uclass *uc;
@@ -466,6 +489,17 @@ int uclass_get_device_by_name(enum uclass_id id, const char *name,
 	return uclass_get_device_tail(dev, ret, devp);
 }
 
+int uclass_get_device_by_platdata(enum uclass_id id, void * platdata,
+			      struct udevice **devp)
+{
+	struct udevice *dev;
+	int ret;
+
+	*devp = NULL;
+	ret = uclass_find_device_by_platdata(id, platdata, &dev);
+	return uclass_get_device_tail(dev, ret, devp);
+}
+
 int uclass_get_device_by_seq(enum uclass_id id, int seq, struct udevice **devp)
 {
 	struct udevice *dev;
diff --git a/include/dm/device.h b/include/dm/device.h
index ab806d0b7e..6282376789 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -396,6 +396,17 @@ enum uclass_id device_get_uclass_id(const struct udevice *dev);
  */
 const char *dev_get_uclass_name(const struct udevice *dev);
 
+/**
+ * device_find_by_platdata() - return the device by its platdata
+ *
+ * Returns the device which onws the platdata structure pointed.
+ *
+ * @platdata:	Struct platdata to use for search
+ * @devp:	Returns pointer to device
+ * @return  error code
+ */
+int device_find_by_platdata(void *platdata, struct udevice **devp);
+
 /**
  * device_get_child() - Get the child of a device by index
  *
diff --git a/include/dm/uclass-internal.h b/include/dm/uclass-internal.h
index 6e3f15c2b0..aeff1ec127 100644
--- a/include/dm/uclass-internal.h
+++ b/include/dm/uclass-internal.h
@@ -100,6 +100,21 @@ int uclass_find_next_device(struct udevice **devp);
 int uclass_find_device_by_name(enum uclass_id id, const char *name,
 			       struct udevice **devp);
 
+/**
+ * uclass_find_device_by_platdata() - Find uclass device based on ID and platdata
+ *
+ * This searches for a device with the exactly given platada.
+ *
+ * The device is NOT probed, it is merely returned.
+ *
+ * @id: ID to look up
+ * @platdata: pointer to struct platdata of a device to find
+ * @devp: Returns pointer to device
+ * @return 0 if OK, -ve on error
+ */
+int uclass_find_device_by_platdata(enum uclass_id id, void *platdata,
+			       struct udevice **devp);
+
 /**
  * uclass_find_device_by_seq() - Find uclass device based on ID and sequence
  *
diff --git a/include/dm/uclass.h b/include/dm/uclass.h
index 70fca79b44..8429b28289 100644
--- a/include/dm/uclass.h
+++ b/include/dm/uclass.h
@@ -167,6 +167,21 @@ int uclass_get_device(enum uclass_id id, int index, struct udevice **devp);
 int uclass_get_device_by_name(enum uclass_id id, const char *name,
 			      struct udevice **devp);
 
+/**
+ * uclass_get_device_by_platdata() - Get a uclass device by its platdata
+ *
+ * This searches the devices in the uclass for one with the exactly given platdata.
+ *
+ * The device is probed to activate it ready for use.
+ *
+ * @id: ID to look up
+ * @platdata: pointer to struct platdata of a device to get
+ * @devp: Returns pointer to device (the first one with the name)
+ * @return 0 if OK, -ve on error
+ */
+int uclass_get_device_by_platdata(enum uclass_id id, void *platdata,
+			      struct udevice **devp);
+
 /**
  * uclass_get_device_by_seq() - Get a uclass device based on an ID and sequence
  *
-- 
2.20.1

[RFC 5/7] gpio: mxc_gpio: add OF_PLATDATA support

[Walter Lozano]

Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
 drivers/gpio/mxc_gpio.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
index c924e52f07..ba63c0b76a 100644
--- a/drivers/gpio/mxc_gpio.c
+++ b/drivers/gpio/mxc_gpio.c
@@ -13,6 +13,8 @@
 #include <asm/arch/imx-regs.h>
 #include <asm/gpio.h>
 #include <asm/io.h>
+#include <dt-structs.h>
+#include <mapmem.h>
 
 enum mxc_gpio_direction {
 	MXC_GPIO_DIRECTION_IN,
@@ -22,6 +24,10 @@ enum mxc_gpio_direction {
 #define GPIO_PER_BANK			32
 
 struct mxc_gpio_plat {
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
+	/* Put this first since driver model will copy the data here */
+	struct dtd_fsl_imx6q_gpio dtplat;
+#endif
 	int bank_index;
 	struct gpio_regs *regs;
 };
@@ -303,8 +309,16 @@ static int mxc_gpio_bind(struct udevice *dev)
 	 * is statically initialized in U_BOOT_DEVICES.Here
 	 * will return.
 	 */
-	if (plat)
+
+	if (plat) {
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
+		struct dtd_fsl_imx6q_gpio *dtplat = &plat->dtplat;
+
+		plat->regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]);
+		plat->bank_index = dev->req_seq;
+#endif
 		return 0;
+	}
 
 	addr = devfdt_get_addr(dev);
 	if (addr == FDT_ADDR_T_NONE)
@@ -347,6 +361,17 @@ U_BOOT_DRIVER(gpio_mxc) = {
 	.bind	= mxc_gpio_bind,
 };
 
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
+U_BOOT_DRIVER(fsl_imx6q_gpio) = {
+	.name	= "fsl_imx6q_gpio",
+	.id	= UCLASS_GPIO,
+	.ops	= &gpio_mxc_ops,
+	.probe	= mxc_gpio_probe,
+	.priv_auto_alloc_size = sizeof(struct mxc_bank_info),
+	.bind	= mxc_gpio_bind,
+};
+#endif
+
 #if !CONFIG_IS_ENABLED(OF_CONTROL)
 static const struct mxc_gpio_plat mxc_plat[] = {
 	{ 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
-- 
2.20.1

[RFC 6/7] mmc: fsl_esdhc_imx: add CD support when OF_PLATDATA is enabled

[Walter Lozano]

Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
 drivers/mmc/fsl_esdhc_imx.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
index 049a1b6ea8..a3a9e5ff96 100644
--- a/drivers/mmc/fsl_esdhc_imx.c
+++ b/drivers/mmc/fsl_esdhc_imx.c
@@ -31,6 +31,7 @@
 #include <dm/pinctrl.h>
 #include <dt-structs.h>
 #include <mapmem.h>
+#include <dm/ofnode.h>
 
 #if !CONFIG_IS_ENABLED(BLK)
 #include "mmc_private.h"
@@ -1490,7 +1491,30 @@ static int fsl_esdhc_probe(struct udevice *dev)
 		priv->bus_width = 4;
 	else
 		priv->bus_width = 1;
-	priv->non_removable = 1;
+
+	if (dtplat->non_removable)
+		priv->non_removable = 1;
+	else
+		priv->non_removable = 0;
+
+#if CONFIG_IS_ENABLED(DM_GPIO)
+	if (!priv->non_removable) {
+		struct udevice *gpiodev;
+
+		ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void *)dtplat->cd_gpios->node, &gpiodev);
+
+		if (ret)
+			return ret;
+
+		ret = gpio_dev_request_index(gpiodev, gpiodev->name, "cd-gpios",
+					     dtplat->cd_gpios->arg[0], GPIOD_IS_IN,
+					     dtplat->cd_gpios->arg[1], &priv->cd_gpio);
+
+		if (ret)
+			return ret;
+
+	}
+#endif
 #endif
 
 	if (data)
-- 
2.20.1

[RFC 7/7] mx6cuboxi: enable OF_PLATDATA

[Walter Lozano]

Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
 configs/mx6cuboxi_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/configs/mx6cuboxi_defconfig b/configs/mx6cuboxi_defconfig
index 7ea79b9064..90aac8a284 100644
--- a/configs/mx6cuboxi_defconfig
+++ b/configs/mx6cuboxi_defconfig
@@ -42,6 +42,7 @@ CONFIG_SPL_OF_CONTROL=y
 CONFIG_DEFAULT_DEVICE_TREE="imx6dl-hummingboard2-emmc-som-v15"
 CONFIG_OF_LIST="imx6dl-hummingboard2-emmc-som-v15 imx6q-hummingboard2-emmc-som-v15"
 CONFIG_MULTI_DTB_FIT=y
+CONFIG_SPL_OF_PLATDATA=y
 CONFIG_ENV_IS_IN_MMC=y
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG=y
@@ -64,3 +65,4 @@ CONFIG_USB_KEYBOARD=y
 CONFIG_VIDEO_IPUV3=y
 CONFIG_VIDEO=y
 # CONFIG_VIDEO_SW_CURSOR is not set
+# CONFIG_SPL_OF_LIBFDT is not set
-- 
2.20.1

[RFC 0/7] mx6cuboxi: enable OF_PLATDATA with MMC support

[Baruch Siach]

Hi Walter,

On Mon, Mar 30 2020, Walter Lozano wrote:
> The SPL in iMX6 boards is restricted to 68 KB as this is the free available
> space in OCRAM for most revisions. In this context, adding OF_CONTROL and DM
> increases the SPL size which could make it difficult to add specific features
> required for custom scenarios.
>
> These patches aim to take advantage of OF_PLATADATA in order to reduce the SPL
> size in this scenario, by parsing DT data to generate platdata structures,
> and thus removing the overhead caused by DT and related libraries.
>
> This series is focused in MMC driver, which is used for boot in boards such as
> Cubox-i. Also, in order to support CD, the OF_PLATDATA support is also
> implemented on GPIO driver.
>
> To make possible to link the CD information found in platdata with a GPIO,
> a new API is suggested, to find/get a device based on its platdata. This
> new API was discussed in [1] but the lack of context made the discussion
> not to progress. With this series, the general idea should be clearer,
> so a better solution could be discussed.
>
> Finally, in order to make use of these new features, enable OF_PLATADATA for
> Cubox-i board, for which OF_CONTROL support is being discussed in [2].

What is the net SPL size reduction of OF_PLATDATA in the Cubox-i case?

Thanks,
baruch

> [1] https://patchwork.ozlabs.org/patch/1249198/
> [2] https://patchwork.ozlabs.org/project/uboot/list/?series=163738
>
> Walter Lozano (7):
>   mmc: fsl_esdhc_imx: add OF_PLATDATA support
>   mmc: fsl_esdhc_imx: add ofdata_to_platdata support
>   dtoc: update dtb_platdata to support cd-gpio
>   dm: uclass: add functions to get device by platdata
>   gpio: mxc_gpio: add OF_PLATDATA support
>   mmc: fsl_esdhc_imx: add CD support when OF_PLATDATA is enabled
>   mx6cuboxi: enable OF_PLATDATA
>
>  configs/mx6cuboxi_defconfig  |   2 +
>  drivers/core/device.c        |  19 +++++++
>  drivers/core/uclass.c        |  34 ++++++++++++
>  drivers/gpio/mxc_gpio.c      |  27 ++++++++-
>  drivers/mmc/fsl_esdhc_imx.c  | 105 ++++++++++++++++++++++++++++++-----
>  include/dm/device.h          |  11 ++++
>  include/dm/uclass-internal.h |  15 +++++
>  include/dm/uclass.h          |  15 +++++
>  tools/dtoc/dtb_platdata.py   |   9 ++-
>  9 files changed, 218 insertions(+), 19 deletions(-)


--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

[RFC 0/7] mx6cuboxi: enable OF_PLATDATA with MMC support

[Walter Lozano]

Hi Baruch,

On 30/3/20 00:54, Baruch Siach wrote:
> Hi Walter,
>
> On Mon, Mar 30 2020, Walter Lozano wrote:
>> The SPL in iMX6 boards is restricted to 68 KB as this is the free available
>> space in OCRAM for most revisions. In this context, adding OF_CONTROL and DM
>> increases the SPL size which could make it difficult to add specific features
>> required for custom scenarios.
>>
>> These patches aim to take advantage of OF_PLATADATA in order to reduce the SPL
>> size in this scenario, by parsing DT data to generate platdata structures,
>> and thus removing the overhead caused by DT and related libraries.
>>
>> This series is focused in MMC driver, which is used for boot in boards such as
>> Cubox-i. Also, in order to support CD, the OF_PLATDATA support is also
>> implemented on GPIO driver.
>>
>> To make possible to link the CD information found in platdata with a GPIO,
>> a new API is suggested, to find/get a device based on its platdata. This
>> new API was discussed in [1] but the lack of context made the discussion
>> not to progress. With this series, the general idea should be clearer,
>> so a better solution could be discussed.
>>
>> Finally, in order to make use of these new features, enable OF_PLATADATA for
>> Cubox-i board, for which OF_CONTROL support is being discussed in [2].
> What is the net SPL size reduction of OF_PLATDATA in the Cubox-i case?

For the Cubox-i defconfig the reduction is 6 KB. Please take into 
account that this tries to reduce the impact of enabling OF_CONTROL and 
DM in SPL which increased the size by 12 KB, as described in [1]

Regards,

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=163738


>
> Thanks,
> baruch
>
>> [1] https://patchwork.ozlabs.org/patch/1249198/
>> [2] https://patchwork.ozlabs.org/project/uboot/list/?series=163738
>>
>> Walter Lozano (7):
>>    mmc: fsl_esdhc_imx: add OF_PLATDATA support
>>    mmc: fsl_esdhc_imx: add ofdata_to_platdata support
>>    dtoc: update dtb_platdata to support cd-gpio
>>    dm: uclass: add functions to get device by platdata
>>    gpio: mxc_gpio: add OF_PLATDATA support
>>    mmc: fsl_esdhc_imx: add CD support when OF_PLATDATA is enabled
>>    mx6cuboxi: enable OF_PLATDATA
>>
>>   configs/mx6cuboxi_defconfig  |   2 +
>>   drivers/core/device.c        |  19 +++++++
>>   drivers/core/uclass.c        |  34 ++++++++++++
>>   drivers/gpio/mxc_gpio.c      |  27 ++++++++-
>>   drivers/mmc/fsl_esdhc_imx.c  | 105 ++++++++++++++++++++++++++++++-----
>>   include/dm/device.h          |  11 ++++
>>   include/dm/uclass-internal.h |  15 +++++
>>   include/dm/uclass.h          |  15 +++++
>>   tools/dtoc/dtb_platdata.py   |   9 ++-
>>   9 files changed, 218 insertions(+), 19 deletions(-)
>
> --
>       http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>     - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

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

[Simon Glass]

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?

>         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.

> +U_BOOT_DRIVER(fsl_usdhc) = {
> +       .name   = "fsl_imx6q_usdhc",
> +       .id     = UCLASS_MMC,
> +       .ops    = &fsl_esdhc_ops,
> +#if CONFIG_IS_ENABLED(BLK)
> +       .bind   = fsl_esdhc_bind,
> +#endif
> +       .probe  = fsl_esdhc_probe,
> +       .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat),
> +       .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv),
> +};
> +#endif
>  #endif
> --
> 2.20.1
>

Regards,
Simon

[RFC 2/7] mmc: fsl_esdhc_imx: add ofdata_to_platdata support

[Simon Glass]

Hi Walter

On Sun, 29 Mar 2020 at 21:32, Walter Lozano <walter.lozano@collabora.com> wrote:
>

All of these commits need a commit message please.

> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
>  drivers/mmc/fsl_esdhc_imx.c | 71 ++++++++++++++++++++++---------------
>  1 file changed, 42 insertions(+), 29 deletions(-)
>

Reviewed-by: SImon Glass <sjg@chromium.org>

> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> index 761a4b46e9..049a1b6ea8 100644
> --- a/drivers/mmc/fsl_esdhc_imx.c
> +++ b/drivers/mmc/fsl_esdhc_imx.c
> @@ -1379,41 +1379,20 @@ __weak void init_clk_usdhc(u32 index)
>  {
>  }
>
> -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);
> +static int fsl_esdhc_ofdata_to_platdata(struct udevice *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);
> +       struct fsl_esdhc_priv *priv = dev_get_priv(dev);
>  #if CONFIG_IS_ENABLED(DM_REGULATOR)
>         struct udevice *vqmmc_dev;
> +       int ret;
>  #endif
> +       const void *fdt = gd->fdt_blob;
> +       int node = dev_of_offset(dev);
> +
> +       fdt_addr_t addr;
>         unsigned int val;
> -       struct mmc *mmc;
> -#if !CONFIG_IS_ENABLED(BLK)
> -       struct blk_desc *bdesc;
> -#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;
> @@ -1483,8 +1462,40 @@ static int fsl_esdhc_probe(struct udevice *dev)
>         }
>  #endif
>  #endif
> +       return 0;
> +}
> +
> +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);
> +       struct esdhc_soc_data *data =
> +               (struct esdhc_soc_data *)dev_get_driver_data(dev);
> +       struct mmc *mmc;
> +#if !CONFIG_IS_ENABLED(BLK)

Should not need to support !BLK now. The migration date has passed.

> +       struct blk_desc *bdesc;
> +#endif
> +       int ret;
> +
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> +       struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat;
> +       unsigned int val;
> +
> +       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;
> +#endif
> +
>         if (data)
>                 priv->flags = data->flags;
> +
>         /*
>          * TODO:
>          * Because lack of clk driver, if SDHC clk is not enabled,
> @@ -1664,6 +1675,7 @@ U_BOOT_DRIVER(fsl_esdhc) = {
>         .name   = "fsl-esdhc-mmc",
>         .id     = UCLASS_MMC,
>         .of_match = fsl_esdhc_ids,
> +       .ofdata_to_platdata = fsl_esdhc_ofdata_to_platdata,
>         .ops    = &fsl_esdhc_ops,
>  #if CONFIG_IS_ENABLED(BLK)
>         .bind   = fsl_esdhc_bind,
> @@ -1677,6 +1689,7 @@ U_BOOT_DRIVER(fsl_esdhc) = {
>  U_BOOT_DRIVER(fsl_usdhc) = {
>         .name   = "fsl_imx6q_usdhc",
>         .id     = UCLASS_MMC,
> +       .ofdata_to_platdata = fsl_esdhc_ofdata_to_platdata,
>         .ops    = &fsl_esdhc_ops,
>  #if CONFIG_IS_ENABLED(BLK)
>         .bind   = fsl_esdhc_bind,
> --
> 2.20.1
>

Regards,
Simon

[RFC 3/7] dtoc: update dtb_platdata to support cd-gpio

[Simon Glass]

On Sun, 29 Mar 2020 at 21:32, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
>  tools/dtoc/dtb_platdata.py | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

This looks OK, but please add a test.


>
> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
> index 037e82c8bb..c52da7925e 100644
> --- a/tools/dtoc/dtb_platdata.py
> +++ b/tools/dtoc/dtb_platdata.py
> @@ -211,7 +211,7 @@ class DtbPlatdata(object):
>          Return:
>              Number of argument cells is this is a phandle, else None
>          """
> -        if prop.name in ['clocks']:
> +        if prop.name in ['clocks', 'cd-gpios']:
>              if not isinstance(prop.value, list):
>                  prop.value = [prop.value]
>              val = prop.value
> @@ -231,8 +231,11 @@ class DtbPlatdata(object):
>                  if not target:
>                      raise ValueError("Cannot parse '%s' in node '%s'" %
>                                       (prop.name, node_name))
> -                prop_name = '#clock-cells'
> -                cells = target.props.get(prop_name)
> +                cells = None
> +                for prop_name in ['#clock-cells', '#gpio-cells']:
> +                    cells = target.props.get(prop_name)
> +                    if cells:
> +                        break
>                  if not cells:
>                      raise ValueError("Node '%s' has no '%s' property" %
>                              (target.name, prop_name))
> --
> 2.20.1
>

[RFC 5/7] gpio: mxc_gpio: add OF_PLATDATA support

[Simon Glass]

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/gpio/mxc_gpio.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
> index c924e52f07..ba63c0b76a 100644
> --- a/drivers/gpio/mxc_gpio.c
> +++ b/drivers/gpio/mxc_gpio.c
> @@ -13,6 +13,8 @@
>  #include <asm/arch/imx-regs.h>
>  #include <asm/gpio.h>
>  #include <asm/io.h>
> +#include <dt-structs.h>
> +#include <mapmem.h>
>
>  enum mxc_gpio_direction {
>         MXC_GPIO_DIRECTION_IN,
> @@ -22,6 +24,10 @@ enum mxc_gpio_direction {
>  #define GPIO_PER_BANK                  32
>
>  struct mxc_gpio_plat {
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> +       /* Put this first since driver model will copy the data here */
> +       struct dtd_fsl_imx6q_gpio dtplat;
> +#endif
>         int bank_index;
>         struct gpio_regs *regs;
>  };
> @@ -303,8 +309,16 @@ static int mxc_gpio_bind(struct udevice *dev)
>          * is statically initialized in U_BOOT_DEVICES.Here
>          * will return.
>          */
> -       if (plat)
> +
> +       if (plat) {
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> +               struct dtd_fsl_imx6q_gpio *dtplat = &plat->dtplat;
> +
> +               plat->regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]);
> +               plat->bank_index = dev->req_seq;
> +#endif
>                 return 0;
> +       }
>
>         addr = devfdt_get_addr(dev);
>         if (addr == FDT_ADDR_T_NONE)
> @@ -347,6 +361,17 @@ U_BOOT_DRIVER(gpio_mxc) = {
>         .bind   = mxc_gpio_bind,
>  };
>
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> +U_BOOT_DRIVER(fsl_imx6q_gpio) = {

Please drop this and find a way to use the existing U_BOOT_DRIVER() declaration.

> +       .name   = "fsl_imx6q_gpio",
> +       .id     = UCLASS_GPIO,
> +       .ops    = &gpio_mxc_ops,
> +       .probe  = mxc_gpio_probe,
> +       .priv_auto_alloc_size = sizeof(struct mxc_bank_info),
> +       .bind   = mxc_gpio_bind,
> +};
> +#endif
> +
>  #if !CONFIG_IS_ENABLED(OF_CONTROL)
>  static const struct mxc_gpio_plat mxc_plat[] = {
>         { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
> --
> 2.20.1
>

Regards,
Simon

[RFC 7/7] mx6cuboxi: enable OF_PLATDATA

[Simon Glass]

On Sun, 29 Mar 2020 at 21:32, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
>  configs/mx6cuboxi_defconfig | 2 ++
>  1 file changed, 2 insertions(+)
>

Reviewed-by: SImon Glass <sjg@chromium.org>

(with commit msg)

[RFC 3/7] dtoc: update dtb_platdata to support cd-gpio

[Walter Lozano]

Hi Simon,

On 6/4/20 00:42, Simon Glass wrote:
> On Sun, 29 Mar 2020 at 21:32, Walter Lozano<walter.lozano@collabora.com>  wrote:
>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com>
>> ---
>>   tools/dtoc/dtb_platdata.py | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
> This looks OK, but please add a test.


Noted. Thank you.


>> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
>> index 037e82c8bb..c52da7925e 100644
>> --- a/tools/dtoc/dtb_platdata.py
>> +++ b/tools/dtoc/dtb_platdata.py
>> @@ -211,7 +211,7 @@ class DtbPlatdata(object):
>>           Return:
>>               Number of argument cells is this is a phandle, else None
>>           """
>> -        if prop.name in ['clocks']:
>> +        if prop.name in ['clocks', 'cd-gpios']:
>>               if not isinstance(prop.value, list):
>>                   prop.value = [prop.value]
>>               val = prop.value
>> @@ -231,8 +231,11 @@ class DtbPlatdata(object):
>>                   if not target:
>>                       raise ValueError("Cannot parse '%s' in node '%s'" %
>>                                        (prop.name, node_name))
>> -                prop_name = '#clock-cells'
>> -                cells = target.props.get(prop_name)
>> +                cells = None
>> +                for prop_name in ['#clock-cells', '#gpio-cells']:
>> +                    cells = target.props.get(prop_name)
>> +                    if cells:
>> +                        break
>>                   if not cells:
>>                       raise ValueError("Node '%s' has no '%s' property" %
>>                               (target.name, prop_name))
>> --
>> 2.20.1
>>

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

[Walter Lozano]

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?

>> +U_BOOT_DRIVER(fsl_usdhc) = {
>> +       .name   = "fsl_imx6q_usdhc",
>> +       .id     = UCLASS_MMC,
>> +       .ops    = &fsl_esdhc_ops,
>> +#if CONFIG_IS_ENABLED(BLK)
>> +       .bind   = fsl_esdhc_bind,
>> +#endif
>> +       .probe  = fsl_esdhc_probe,
>> +       .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat),
>> +       .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv),
>> +};
>> +#endif
>>   #endif
>> --
>> 2.20.1
>>
> Regards,
> Simon

[RFC 2/7] mmc: fsl_esdhc_imx: add ofdata_to_platdata support

[Walter Lozano]

Hi Simon,

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:
> All of these commits need a commit message please.

Thank you for pointing it.

>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com>
>> ---
>>   drivers/mmc/fsl_esdhc_imx.c | 71 ++++++++++++++++++++++---------------
>>   1 file changed, 42 insertions(+), 29 deletions(-)
>>
> Reviewed-by: SImon Glass<sjg@chromium.org>
>
>> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
>> index 761a4b46e9..049a1b6ea8 100644
>> --- a/drivers/mmc/fsl_esdhc_imx.c
>> +++ b/drivers/mmc/fsl_esdhc_imx.c
>> @@ -1379,41 +1379,20 @@ __weak void init_clk_usdhc(u32 index)
>>   {
>>   }
>>
>> -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);
>> +static int fsl_esdhc_ofdata_to_platdata(struct udevice *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);
>> +       struct fsl_esdhc_priv *priv = dev_get_priv(dev);
>>   #if CONFIG_IS_ENABLED(DM_REGULATOR)
>>          struct udevice *vqmmc_dev;
>> +       int ret;
>>   #endif
>> +       const void *fdt = gd->fdt_blob;
>> +       int node = dev_of_offset(dev);
>> +
>> +       fdt_addr_t addr;
>>          unsigned int val;
>> -       struct mmc *mmc;
>> -#if !CONFIG_IS_ENABLED(BLK)
>> -       struct blk_desc *bdesc;
>> -#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;
>> @@ -1483,8 +1462,40 @@ static int fsl_esdhc_probe(struct udevice *dev)
>>          }
>>   #endif
>>   #endif
>> +       return 0;
>> +}
>> +
>> +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);
>> +       struct esdhc_soc_data *data =
>> +               (struct esdhc_soc_data *)dev_get_driver_data(dev);
>> +       struct mmc *mmc;
>> +#if !CONFIG_IS_ENABLED(BLK)
> Should not need to support !BLK now. The migration date has passed.

Thanks.

>> +       struct blk_desc *bdesc;
>> +#endif
>> +       int ret;
>> +
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> +       struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat;
>> +       unsigned int val;
>> +
>> +       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;
>> +#endif
>> +
>>          if (data)
>>                  priv->flags = data->flags;
>> +
>>          /*
>>           * TODO:
>>           * Because lack of clk driver, if SDHC clk is not enabled,
>> @@ -1664,6 +1675,7 @@ U_BOOT_DRIVER(fsl_esdhc) = {
>>          .name   = "fsl-esdhc-mmc",
>>          .id     = UCLASS_MMC,
>>          .of_match = fsl_esdhc_ids,
>> +       .ofdata_to_platdata = fsl_esdhc_ofdata_to_platdata,
>>          .ops    = &fsl_esdhc_ops,
>>   #if CONFIG_IS_ENABLED(BLK)
>>          .bind   = fsl_esdhc_bind,
>> @@ -1677,6 +1689,7 @@ U_BOOT_DRIVER(fsl_esdhc) = {
>>   U_BOOT_DRIVER(fsl_usdhc) = {
>>          .name   = "fsl_imx6q_usdhc",
>>          .id     = UCLASS_MMC,
>> +       .ofdata_to_platdata = fsl_esdhc_ofdata_to_platdata,
>>          .ops    = &fsl_esdhc_ops,
>>   #if CONFIG_IS_ENABLED(BLK)
>>          .bind   = fsl_esdhc_bind,
>> --
>> 2.20.1
>>
> Regards,
> Simon

[RFC 5/7] gpio: mxc_gpio: add OF_PLATDATA support

[Walter Lozano]

Hi Simon,

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/gpio/mxc_gpio.c | 27 ++++++++++++++++++++++++++-
>>   1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
>> index c924e52f07..ba63c0b76a 100644
>> --- a/drivers/gpio/mxc_gpio.c
>> +++ b/drivers/gpio/mxc_gpio.c
>> @@ -13,6 +13,8 @@
>>   #include <asm/arch/imx-regs.h>
>>   #include <asm/gpio.h>
>>   #include <asm/io.h>
>> +#include <dt-structs.h>
>> +#include <mapmem.h>
>>
>>   enum mxc_gpio_direction {
>>          MXC_GPIO_DIRECTION_IN,
>> @@ -22,6 +24,10 @@ enum mxc_gpio_direction {
>>   #define GPIO_PER_BANK                  32
>>
>>   struct mxc_gpio_plat {
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> +       /* Put this first since driver model will copy the data here */
>> +       struct dtd_fsl_imx6q_gpio dtplat;
>> +#endif
>>          int bank_index;
>>          struct gpio_regs *regs;
>>   };
>> @@ -303,8 +309,16 @@ static int mxc_gpio_bind(struct udevice *dev)
>>           * is statically initialized in U_BOOT_DEVICES.Here
>>           * will return.
>>           */
>> -       if (plat)
>> +
>> +       if (plat) {
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> +               struct dtd_fsl_imx6q_gpio *dtplat = &plat->dtplat;
>> +
>> +               plat->regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]);
>> +               plat->bank_index = dev->req_seq;
>> +#endif
>>                  return 0;
>> +       }
>>
>>          addr = devfdt_get_addr(dev);
>>          if (addr == FDT_ADDR_T_NONE)
>> @@ -347,6 +361,17 @@ U_BOOT_DRIVER(gpio_mxc) = {
>>          .bind   = mxc_gpio_bind,
>>   };
>>
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> +U_BOOT_DRIVER(fsl_imx6q_gpio) = {
> Please drop this and find a way to use the existing U_BOOT_DRIVER() declaration.

Thanks for pointing it. This discussion already began in a previous 
patch from this series, so probably the best way to accomplish this will 
be discussed there.

>> +       .name   = "fsl_imx6q_gpio",
>> +       .id     = UCLASS_GPIO,
>> +       .ops    = &gpio_mxc_ops,
>> +       .probe  = mxc_gpio_probe,
>> +       .priv_auto_alloc_size = sizeof(struct mxc_bank_info),
>> +       .bind   = mxc_gpio_bind,
>> +};
>> +#endif
>> +
>>   #if !CONFIG_IS_ENABLED(OF_CONTROL)
>>   static const struct mxc_gpio_plat mxc_plat[] = {
>>          { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
>> --
>> 2.20.1
>>
> Regards,
> Simon

[RFC 7/7] mx6cuboxi: enable OF_PLATDATA

[Walter Lozano]

Hi Simon,

On 6/4/20 00:43, Simon Glass wrote:
> On Sun, 29 Mar 2020 at 21:32, Walter Lozano<walter.lozano@collabora.com>  wrote:
>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com>
>> ---
>>   configs/mx6cuboxi_defconfig | 2 ++
>>   1 file changed, 2 insertions(+)
>>
> Reviewed-by: SImon Glass<sjg@chromium.org>
>
> (with commit msg)


Thank you.

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

[Simon Glass]

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?

Regards,
SImon

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

[Walter Lozano]

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.

> Regards,
> SImon

Regards,

Walter

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

[Simon Glass]

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.

Regards,
Simon

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

[Walter Lozano]

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

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

[Simon Glass]

Hi Walter,

On Thu, 9 Apr 2020 at 13:44, Walter Lozano <walter.lozano@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@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.

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

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

[Walter Lozano]

On 9/4/20 16:50, Simon Glass wrote:
> Hi Walter,
>
> On Thu, 9 Apr 2020 at 13:44, Walter Lozano <walter.lozano@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@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.
> 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.

OK, Iet's do it step by step, I hope to schedule some time for this 
series next week.


> Regards,
> SImon

Regards,

Walter

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

[Walter Lozano]

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@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@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.
> 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


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.

Regards,

Walter

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

[Simon Glass]

Hi Walter,

On Fri, 17 Apr 2020 at 14:05, Walter Lozano <walter.lozano@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@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@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.
> > 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