[RFC 0/7] mx6cuboxi: enable OF_PLATDATA with MMC support
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(-)
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
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;elsepriv->bus_width = 1;priv->non_removable = 1;+#else addr = dev_read_addr(dev); if (addr == FDT_ADDR_T_NONE) return -EINVAL; priv->esdhc_regs = (struct fsl_esdhc *)addr; priv->dev = dev; priv->mode = -1;
if (data)priv->flags = data->flags; val = dev_read_u32_default(dev, "bus-width", -1); if (val == 8)@@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) priv->vs18_enable = 1; } #endif
+#endif
if (data)priv->flags = data->flags; /* * TODO: * Because lack of clk driver, if SDHC clk is not enabled,@@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) return ret; }
+#if !CONFIG_IS_ENABLED(OF_PLATDATA)
Maybe can use if() for this one?
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
Hi Simon,
Thank you for taking the time to review this series.
On 6/4/20 00:42, Simon Glass wrote:
Hi Walter,
On Sun, 29 Mar 2020 at 21:32, Walter Lozanowalter.lozano@collabora.com wrote:
Signed-off-by: Walter Lozanowalter.lozano@collabora.com
drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index 4900498e9b..761a4b46e9 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -29,6 +29,8 @@ #include <dm.h> #include <asm-generic/gpio.h> #include <dm/pinctrl.h> +#include <dt-structs.h> +#include <mapmem.h>
#if !CONFIG_IS_ENABLED(BLK) #include "mmc_private.h" @@ -98,6 +100,11 @@ struct fsl_esdhc { };
struct fsl_esdhc_plat { +#if CONFIG_IS_ENABLED(OF_PLATDATA)
/* Put this first since driver model will copy the data here */struct dtd_fsl_imx6q_usdhc dtplat;+#endif
struct mmc_config cfg; struct mmc mmc;@@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); struct fsl_esdhc_plat *plat = dev_get_platdata(dev); struct fsl_esdhc_priv *priv = dev_get_priv(dev); +#if !CONFIG_IS_ENABLED(OF_PLATDATA) const void *fdt = gd->fdt_blob; int node = dev_of_offset(dev);
fdt_addr_t addr;+#else
struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat;+#endif struct esdhc_soc_data *data = (struct esdhc_soc_data *)dev_get_driver_data(dev); #if CONFIG_IS_ENABLED(DM_REGULATOR) struct udevice *vqmmc_dev; #endif
fdt_addr_t addr; unsigned int val; struct mmc *mmc;@@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) #endif int ret;
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]);val = plat->dtplat.bus_width;if (val == 8)priv->bus_width = 8;else if (val == 4)priv->bus_width = 4;elsepriv->bus_width = 1;priv->non_removable = 1;+#else addr = dev_read_addr(dev); if (addr == FDT_ADDR_T_NONE) return -EINVAL; priv->esdhc_regs = (struct fsl_esdhc *)addr; priv->dev = dev; priv->mode = -1;
if (data)priv->flags = data->flags; val = dev_read_u32_default(dev, "bus-width", -1); if (val == 8)@@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) priv->vs18_enable = 1; } #endif
+#endif
if (data)priv->flags = data->flags; /* * TODO: * Because lack of clk driver, if SDHC clk is not enabled,@@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) return ret; }
+#if !CONFIG_IS_ENABLED(OF_PLATDATA)
Maybe can use if() for this one?
Thank you for the suggestion
ret = mmc_of_parse(dev, &plat->cfg); if (ret) return ret;+#endif
mmc = &plat->mmc; mmc->cfg = &plat->cfg;@@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), };
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
Don't you already have a U_BOOT_DRIVER declaration?
You may need to change the name of your existing driver though (see of-platdata docs).
So if it is because of that, please add a comment.
I have my doubts regarding this issue. As I see, this driver is used by many different DT with different compatible strings, so I'm thinking in trying to find a more generic approach. Would it be useful to have a more elaborated solution searching for the compatible string when matching drivers with device?
+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
Hi Walter,
On Tue, 7 Apr 2020 at 14:05, Walter Lozano walter.lozano@collabora.com wrote:
Hi Simon,
Thank you for taking the time to review this series.
On 6/4/20 00:42, Simon Glass wrote:
Hi Walter,
On Sun, 29 Mar 2020 at 21:32, Walter Lozanowalter.lozano@collabora.com wrote:
Signed-off-by: Walter Lozanowalter.lozano@collabora.com
drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index 4900498e9b..761a4b46e9 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -29,6 +29,8 @@ #include <dm.h> #include <asm-generic/gpio.h> #include <dm/pinctrl.h> +#include <dt-structs.h> +#include <mapmem.h>
#if !CONFIG_IS_ENABLED(BLK) #include "mmc_private.h" @@ -98,6 +100,11 @@ struct fsl_esdhc { };
struct fsl_esdhc_plat { +#if CONFIG_IS_ENABLED(OF_PLATDATA)
/* Put this first since driver model will copy the data here */struct dtd_fsl_imx6q_usdhc dtplat;+#endif
struct mmc_config cfg; struct mmc mmc;@@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); struct fsl_esdhc_plat *plat = dev_get_platdata(dev); struct fsl_esdhc_priv *priv = dev_get_priv(dev); +#if !CONFIG_IS_ENABLED(OF_PLATDATA) const void *fdt = gd->fdt_blob; int node = dev_of_offset(dev);
fdt_addr_t addr;+#else
struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat;+#endif struct esdhc_soc_data *data = (struct esdhc_soc_data *)dev_get_driver_data(dev); #if CONFIG_IS_ENABLED(DM_REGULATOR) struct udevice *vqmmc_dev; #endif
fdt_addr_t addr; unsigned int val; struct mmc *mmc;@@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) #endif int ret;
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]);val = plat->dtplat.bus_width;if (val == 8)priv->bus_width = 8;else if (val == 4)priv->bus_width = 4;elsepriv->bus_width = 1;priv->non_removable = 1;+#else addr = dev_read_addr(dev); if (addr == FDT_ADDR_T_NONE) return -EINVAL; priv->esdhc_regs = (struct fsl_esdhc *)addr; priv->dev = dev; priv->mode = -1;
if (data)priv->flags = data->flags; val = dev_read_u32_default(dev, "bus-width", -1); if (val == 8)@@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) priv->vs18_enable = 1; } #endif
+#endif
if (data)priv->flags = data->flags; /* * TODO: * Because lack of clk driver, if SDHC clk is not enabled,@@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) return ret; }
+#if !CONFIG_IS_ENABLED(OF_PLATDATA)
Maybe can use if() for this one?
Thank you for the suggestion
ret = mmc_of_parse(dev, &plat->cfg); if (ret) return ret;+#endif
mmc = &plat->mmc; mmc->cfg = &plat->cfg;@@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), };
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
Don't you already have a U_BOOT_DRIVER declaration?
You may need to change the name of your existing driver though (see of-platdata docs).
So if it is because of that, please add a comment.
I have my doubts regarding this issue. As I see, this driver is used by many different DT with different compatible strings, so I'm thinking in trying to find a more generic approach. Would it be useful to have a more elaborated solution searching for the compatible string when matching drivers with device?
Yes I think so.
Actually searching for a string is not great anyway. I wonder if we can use the linker-list idea in the previous email somehow?
Regards, SImon
Hi Simon,
On 8/4/20 00:14, Simon Glass wrote:
Hi Walter,
On Tue, 7 Apr 2020 at 14:05, Walter Lozano walter.lozano@collabora.com wrote:
Hi Simon,
Thank you for taking the time to review this series.
On 6/4/20 00:42, Simon Glass wrote:
Hi Walter,
On Sun, 29 Mar 2020 at 21:32, Walter Lozanowalter.lozano@collabora.com wrote:
Signed-off-by: Walter Lozanowalter.lozano@collabora.com
drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index 4900498e9b..761a4b46e9 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -29,6 +29,8 @@ #include <dm.h> #include <asm-generic/gpio.h> #include <dm/pinctrl.h> +#include <dt-structs.h> +#include <mapmem.h>
#if !CONFIG_IS_ENABLED(BLK) #include "mmc_private.h" @@ -98,6 +100,11 @@ struct fsl_esdhc { };
struct fsl_esdhc_plat { +#if CONFIG_IS_ENABLED(OF_PLATDATA)
/* Put this first since driver model will copy the data here */struct dtd_fsl_imx6q_usdhc dtplat;+#endif
struct mmc_config cfg; struct mmc mmc;@@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); struct fsl_esdhc_plat *plat = dev_get_platdata(dev); struct fsl_esdhc_priv *priv = dev_get_priv(dev); +#if !CONFIG_IS_ENABLED(OF_PLATDATA) const void *fdt = gd->fdt_blob; int node = dev_of_offset(dev);
fdt_addr_t addr;+#else
struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat;+#endif struct esdhc_soc_data *data = (struct esdhc_soc_data *)dev_get_driver_data(dev); #if CONFIG_IS_ENABLED(DM_REGULATOR) struct udevice *vqmmc_dev; #endif
fdt_addr_t addr; unsigned int val; struct mmc *mmc;@@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) #endif int ret;
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]);val = plat->dtplat.bus_width;if (val == 8)priv->bus_width = 8;else if (val == 4)priv->bus_width = 4;elsepriv->bus_width = 1;priv->non_removable = 1;+#else addr = dev_read_addr(dev); if (addr == FDT_ADDR_T_NONE) return -EINVAL; priv->esdhc_regs = (struct fsl_esdhc *)addr; priv->dev = dev; priv->mode = -1;
if (data)priv->flags = data->flags; val = dev_read_u32_default(dev, "bus-width", -1); if (val == 8)@@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) priv->vs18_enable = 1; } #endif
+#endif
if (data)priv->flags = data->flags; /* * TODO: * Because lack of clk driver, if SDHC clk is not enabled,@@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) return ret; }
+#if !CONFIG_IS_ENABLED(OF_PLATDATA)
Maybe can use if() for this one?
Thank you for the suggestion
ret = mmc_of_parse(dev, &plat->cfg); if (ret) return ret;+#endif
mmc = &plat->mmc; mmc->cfg = &plat->cfg;@@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), };
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
Don't you already have a U_BOOT_DRIVER declaration?
You may need to change the name of your existing driver though (see of-platdata docs).
So if it is because of that, please add a comment.
I have my doubts regarding this issue. As I see, this driver is used by many different DT with different compatible strings, so I'm thinking in trying to find a more generic approach. Would it be useful to have a more elaborated solution searching for the compatible string when matching drivers with device?
Yes I think so.
Actually searching for a string is not great anyway. I wonder if we can use the linker-list idea in the previous email somehow?
I'm sure I' understand the idea you try to share with me. Sorry, I understand that one limitation of the current implementation of OF_PLATDATA is the fact that the U_BOOT_DRIVER name should match the one used as first entry in DT. As in particular this driver has several compatible
{ .compatible = "fsl,imx53-esdhc", }, { .compatible = "fsl,imx6ul-usdhc", }, { .compatible = "fsl,imx6sx-usdhc", }, { .compatible = "fsl,imx6sl-usdhc", }, { .compatible = "fsl,imx6q-usdhc", }, { .compatible = "fsl,imx7d-usdhc", .data = (ulong)&usdhc_imx7d_data,}, { .compatible = "fsl,imx7ulp-usdhc", }, { .compatible = "fsl,imx8qm-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imx8mm-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imx8mn-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imx8mq-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imxrt-usdhc", },
and in order to create a general solution, we need to search for the compatible string instead of matching for driver name.
Could you please elaborate a bit more your suggestion in order to understand your approach.
Thanks in advance.
Regards, SImon
Regards,
Walter
Hi Walter,
On Thu, 9 Apr 2020 at 13:07, Walter Lozano walter.lozano@collabora.com wrote:
Hi Simon,
On 8/4/20 00:14, Simon Glass wrote:
Hi Walter,
On Tue, 7 Apr 2020 at 14:05, Walter Lozano walter.lozano@collabora.com wrote:
Hi Simon,
Thank you for taking the time to review this series.
On 6/4/20 00:42, Simon Glass wrote:
Hi Walter,
On Sun, 29 Mar 2020 at 21:32, Walter Lozanowalter.lozano@collabora.com wrote:
Signed-off-by: Walter Lozanowalter.lozano@collabora.com
drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index 4900498e9b..761a4b46e9 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -29,6 +29,8 @@ #include <dm.h> #include <asm-generic/gpio.h> #include <dm/pinctrl.h> +#include <dt-structs.h> +#include <mapmem.h>
#if !CONFIG_IS_ENABLED(BLK) #include "mmc_private.h" @@ -98,6 +100,11 @@ struct fsl_esdhc { };
struct fsl_esdhc_plat { +#if CONFIG_IS_ENABLED(OF_PLATDATA)
/* Put this first since driver model will copy the data here */struct dtd_fsl_imx6q_usdhc dtplat;+#endif
struct mmc_config cfg; struct mmc mmc;@@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); struct fsl_esdhc_plat *plat = dev_get_platdata(dev); struct fsl_esdhc_priv *priv = dev_get_priv(dev); +#if !CONFIG_IS_ENABLED(OF_PLATDATA) const void *fdt = gd->fdt_blob; int node = dev_of_offset(dev);
fdt_addr_t addr;+#else
struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat;+#endif struct esdhc_soc_data *data = (struct esdhc_soc_data *)dev_get_driver_data(dev); #if CONFIG_IS_ENABLED(DM_REGULATOR) struct udevice *vqmmc_dev; #endif
fdt_addr_t addr; unsigned int val; struct mmc *mmc;@@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) #endif int ret;
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]);val = plat->dtplat.bus_width;if (val == 8)priv->bus_width = 8;else if (val == 4)priv->bus_width = 4;elsepriv->bus_width = 1;priv->non_removable = 1;+#else addr = dev_read_addr(dev); if (addr == FDT_ADDR_T_NONE) return -EINVAL; priv->esdhc_regs = (struct fsl_esdhc *)addr; priv->dev = dev; priv->mode = -1;
if (data)priv->flags = data->flags; val = dev_read_u32_default(dev, "bus-width", -1); if (val == 8)@@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) priv->vs18_enable = 1; } #endif
+#endif
if (data)priv->flags = data->flags; /* * TODO: * Because lack of clk driver, if SDHC clk is not enabled,@@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) return ret; }
+#if !CONFIG_IS_ENABLED(OF_PLATDATA)
Maybe can use if() for this one?
Thank you for the suggestion
ret = mmc_of_parse(dev, &plat->cfg); if (ret) return ret;+#endif
mmc = &plat->mmc; mmc->cfg = &plat->cfg;@@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), };
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
Don't you already have a U_BOOT_DRIVER declaration?
You may need to change the name of your existing driver though (see of-platdata docs).
So if it is because of that, please add a comment.
I have my doubts regarding this issue. As I see, this driver is used by many different DT with different compatible strings, so I'm thinking in trying to find a more generic approach. Would it be useful to have a more elaborated solution searching for the compatible string when matching drivers with device?
Yes I think so.
Actually searching for a string is not great anyway. I wonder if we can use the linker-list idea in the previous email somehow?
I'm sure I' understand the idea you try to share with me. Sorry, I understand that one limitation of the current implementation of OF_PLATDATA is the fact that the U_BOOT_DRIVER name should match the one used as first entry in DT. As in particular this driver has several compatible
{ .compatible = "fsl,imx53-esdhc", }, { .compatible = "fsl,imx6ul-usdhc", }, { .compatible = "fsl,imx6sx-usdhc", }, { .compatible = "fsl,imx6sl-usdhc", }, { .compatible = "fsl,imx6q-usdhc", }, { .compatible = "fsl,imx7d-usdhc", .data =(ulong)&usdhc_imx7d_data,}, { .compatible = "fsl,imx7ulp-usdhc", }, { .compatible = "fsl,imx8qm-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imx8mm-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imx8mn-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imx8mq-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imxrt-usdhc", },
and in order to create a general solution, we need to search for the compatible string instead of matching for driver name.
Could you please elaborate a bit more your suggestion in order to understand your approach.
Thanks in advance.
I am wondering if we can use the DM_GET_DRIVER() macro somehow in dt_platdata.c. At present we emit a string, and that string matches the driver name, so we should be able to. That will give a compile error if something is wrong, much better than the current behaviour of not being able to bind the driver at runtime.
This is just to improve the current impl, not to do what you are asking here.
For what you want, our current approach is to use the first compatible string to find the driver name. Since all compatible strings point to the same driver, perhaps that is good enough? We should at least understand the limitations before going further.
The main one I am aware of is that you need to duplicate the U_BOOT_DRIVER()xxx for each compatible string. To fix that we could add:
U_BOOT_DRIVER_ALIAS(xxx, new_name)
which creates a linker list symbol that points to the original driver, perhaps using ll_entry_get(). That should be easy enough I think. Then whichever symbol you use you will get the same driver since all the symbols point to it.
Unfortunately the .data field is not available with of_platdata. That could be added to the dtd_... struct automatically by dtoc, I suspect. However that requires some clever parsing of the C code...
As you can tell I would really like to avoid string comparisons and tables of compatible strings in the image itself. It adds overheade.
Regards, Simon
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 Lozanowalter.lozano@collabora.com wrote:
Signed-off-by: Walter Lozanowalter.lozano@collabora.com
drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-)diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index 4900498e9b..761a4b46e9 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -29,6 +29,8 @@ #include <dm.h> #include <asm-generic/gpio.h> #include <dm/pinctrl.h> +#include <dt-structs.h> +#include <mapmem.h>
#if !CONFIG_IS_ENABLED(BLK) #include "mmc_private.h"@@ -98,6 +100,11 @@ struct fsl_esdhc { };
struct fsl_esdhc_plat {+#if CONFIG_IS_ENABLED(OF_PLATDATA)
/* Put this first since driver model will copy the data here */struct dtd_fsl_imx6q_usdhc dtplat;+#endif
struct mmc_config cfg; struct mmc mmc;@@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); struct fsl_esdhc_plat *plat = dev_get_platdata(dev); struct fsl_esdhc_priv *priv = dev_get_priv(dev); +#if !CONFIG_IS_ENABLED(OF_PLATDATA) const void *fdt = gd->fdt_blob; int node = dev_of_offset(dev);
fdt_addr_t addr;+#else
struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat;+#endif struct esdhc_soc_data *data = (struct esdhc_soc_data *)dev_get_driver_data(dev); #if CONFIG_IS_ENABLED(DM_REGULATOR) struct udevice *vqmmc_dev; #endif
fdt_addr_t addr; unsigned int val; struct mmc *mmc;@@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) #endif int ret;
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]);val = plat->dtplat.bus_width;if (val == 8)priv->bus_width = 8;else if (val == 4)priv->bus_width = 4;elsepriv->bus_width = 1;priv->non_removable = 1;+#else addr = dev_read_addr(dev); if (addr == FDT_ADDR_T_NONE) return -EINVAL; priv->esdhc_regs = (struct fsl_esdhc *)addr; priv->dev = dev; priv->mode = -1;
if (data)priv->flags = data->flags; val = dev_read_u32_default(dev, "bus-width", -1); if (val == 8)@@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) priv->vs18_enable = 1; } #endif
+#endif
if (data)priv->flags = data->flags; /* * TODO: * Because lack of clk driver, if SDHC clk is not enabled,@@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) return ret; }
+#if !CONFIG_IS_ENABLED(OF_PLATDATA)
Maybe can use if() for this one?
Thank you for the suggestion
ret = mmc_of_parse(dev, &plat->cfg); if (ret) return ret;+#endif
mmc = &plat->mmc; mmc->cfg = &plat->cfg;@@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), };
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
Don't you already have a U_BOOT_DRIVER declaration?
You may need to change the name of your existing driver though (see of-platdata docs).
So if it is because of that, please add a comment.
I have my doubts regarding this issue. As I see, this driver is used by many different DT with different compatible strings, so I'm thinking in trying to find a more generic approach. Would it be useful to have a more elaborated solution searching for the compatible string when matching drivers with device?
Yes I think so.
Actually searching for a string is not great anyway. I wonder if we can use the linker-list idea in the previous email somehow?
I'm sure I' understand the idea you try to share with me. Sorry, I understand that one limitation of the current implementation of OF_PLATDATA is the fact that the U_BOOT_DRIVER name should match the one used as first entry in DT. As in particular this driver has several compatible
{ .compatible = "fsl,imx53-esdhc", }, { .compatible = "fsl,imx6ul-usdhc", }, { .compatible = "fsl,imx6sx-usdhc", }, { .compatible = "fsl,imx6sl-usdhc", }, { .compatible = "fsl,imx6q-usdhc", }, { .compatible = "fsl,imx7d-usdhc", .data =(ulong)&usdhc_imx7d_data,}, { .compatible = "fsl,imx7ulp-usdhc", }, { .compatible = "fsl,imx8qm-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imx8mm-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imx8mn-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imx8mq-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imxrt-usdhc", },
and in order to create a general solution, we need to search for the compatible string instead of matching for driver name.
Could you please elaborate a bit more your suggestion in order to understand your approach.
Thanks in advance.
I am wondering if we can use the DM_GET_DRIVER() macro somehow in dt_platdata.c. At present we emit a string, and that string matches the driver name, so we should be able to. That will give a compile error if something is wrong, much better than the current behaviour of not being able to bind the driver at runtime.
This is just to improve the current impl, not to do what you are asking here.
For what you want, our current approach is to use the first compatible string to find the driver name. Since all compatible strings point to the same driver, perhaps that is good enough? We should at least understand the limitations before going further.
The main one I am aware of is that you need to duplicate the U_BOOT_DRIVER()xxx for each compatible string. To fix that we could add:
U_BOOT_DRIVER_ALIAS(xxx, new_name)
which creates a linker list symbol that points to the original driver, perhaps using ll_entry_get(). That should be easy enough I think. Then whichever symbol you use you will get the same driver since all the symbols point to it.
Unfortunately the .data field is not available with of_platdata. That could be added to the dtd_... struct automatically by dtoc, I suspect. However that requires some clever parsing of the C code...
As you can tell I would really like to avoid string comparisons and tables of compatible strings in the image itself. It adds overheade.
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
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 Lozanowalter.lozano@collabora.com wrote: > Signed-off-by: Walter Lozanowalter.lozano@collabora.com > --- > drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- > 1 file changed, 42 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c > index 4900498e9b..761a4b46e9 100644 > --- a/drivers/mmc/fsl_esdhc_imx.c > +++ b/drivers/mmc/fsl_esdhc_imx.c > @@ -29,6 +29,8 @@ > #include <dm.h> > #include <asm-generic/gpio.h> > #include <dm/pinctrl.h> > +#include <dt-structs.h> > +#include <mapmem.h> > > #if !CONFIG_IS_ENABLED(BLK) > #include "mmc_private.h" > @@ -98,6 +100,11 @@ struct fsl_esdhc { > }; > > struct fsl_esdhc_plat { > +#if CONFIG_IS_ENABLED(OF_PLATDATA) > + /* Put this first since driver model will copy the data here */ > + struct dtd_fsl_imx6q_usdhc dtplat; > +#endif > + > struct mmc_config cfg; > struct mmc mmc; > }; > @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) > struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); > struct fsl_esdhc_plat *plat = dev_get_platdata(dev); > struct fsl_esdhc_priv *priv = dev_get_priv(dev); > +#if !CONFIG_IS_ENABLED(OF_PLATDATA) > const void *fdt = gd->fdt_blob; > int node = dev_of_offset(dev); > + fdt_addr_t addr; > +#else > + struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat; > +#endif > struct esdhc_soc_data *data = > (struct esdhc_soc_data *)dev_get_driver_data(dev); > #if CONFIG_IS_ENABLED(DM_REGULATOR) > struct udevice *vqmmc_dev; > #endif > - fdt_addr_t addr; > unsigned int val; > struct mmc *mmc; > #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
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 Lozanowalter.lozano@collabora.com wrote: >> Signed-off-by: Walter Lozanowalter.lozano@collabora.com >> --- >> drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- >> 1 file changed, 42 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c >> index 4900498e9b..761a4b46e9 100644 >> --- a/drivers/mmc/fsl_esdhc_imx.c >> +++ b/drivers/mmc/fsl_esdhc_imx.c >> @@ -29,6 +29,8 @@ >> #include <dm.h> >> #include <asm-generic/gpio.h> >> #include <dm/pinctrl.h> >> +#include <dt-structs.h> >> +#include <mapmem.h> >> >> #if !CONFIG_IS_ENABLED(BLK) >> #include "mmc_private.h" >> @@ -98,6 +100,11 @@ struct fsl_esdhc { >> }; >> >> struct fsl_esdhc_plat { >> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >> + /* Put this first since driver model will copy the data here */ >> + struct dtd_fsl_imx6q_usdhc dtplat; >> +#endif >> + >> struct mmc_config cfg; >> struct mmc mmc; >> }; >> @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) >> struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); >> struct fsl_esdhc_plat *plat = dev_get_platdata(dev); >> struct fsl_esdhc_priv *priv = dev_get_priv(dev); >> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) >> const void *fdt = gd->fdt_blob; >> int node = dev_of_offset(dev); >> + fdt_addr_t addr; >> +#else >> + struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat; >> +#endif >> struct esdhc_soc_data *data = >> (struct esdhc_soc_data *)dev_get_driver_data(dev); >> #if CONFIG_IS_ENABLED(DM_REGULATOR) >> struct udevice *vqmmc_dev; >> #endif >> - fdt_addr_t addr; >> unsigned int val; >> struct mmc *mmc; >> #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
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 Lozanowalter.lozano@collabora.com wrote: >> Signed-off-by: Walter Lozanowalter.lozano@collabora.com >> --- >> drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- >> 1 file changed, 42 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c >> index 4900498e9b..761a4b46e9 100644 >> --- a/drivers/mmc/fsl_esdhc_imx.c >> +++ b/drivers/mmc/fsl_esdhc_imx.c >> @@ -29,6 +29,8 @@ >> #include <dm.h> >> #include <asm-generic/gpio.h> >> #include <dm/pinctrl.h> >> +#include <dt-structs.h> >> +#include <mapmem.h> >> >> #if !CONFIG_IS_ENABLED(BLK) >> #include "mmc_private.h" >> @@ -98,6 +100,11 @@ struct fsl_esdhc { >> }; >> >> struct fsl_esdhc_plat { >> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >> + /* Put this first since driver model will copy the data here */ >> + struct dtd_fsl_imx6q_usdhc dtplat; >> +#endif >> + >> struct mmc_config cfg; >> struct mmc mmc; >> }; >> @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) >> struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); >> struct fsl_esdhc_plat *plat = dev_get_platdata(dev); >> struct fsl_esdhc_priv *priv = dev_get_priv(dev); >> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) >> const void *fdt = gd->fdt_blob; >> int node = dev_of_offset(dev); >> + fdt_addr_t addr; >> +#else >> + struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat; >> +#endif >> struct esdhc_soc_data *data = >> (struct esdhc_soc_data *)dev_get_driver_data(dev); >> #if CONFIG_IS_ENABLED(DM_REGULATOR) >> struct udevice *vqmmc_dev; >> #endif >> - fdt_addr_t addr; >> unsigned int val; >> struct mmc *mmc; >> #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
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 Lozanowalter.lozano@collabora.com wrote: >>> Signed-off-by: Walter Lozanowalter.lozano@collabora.com >>> --- >>> drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- >>> 1 file changed, 42 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c >>> index 4900498e9b..761a4b46e9 100644 >>> --- a/drivers/mmc/fsl_esdhc_imx.c >>> +++ b/drivers/mmc/fsl_esdhc_imx.c >>> @@ -29,6 +29,8 @@ >>> #include <dm.h> >>> #include <asm-generic/gpio.h> >>> #include <dm/pinctrl.h> >>> +#include <dt-structs.h> >>> +#include <mapmem.h> >>> >>> #if !CONFIG_IS_ENABLED(BLK) >>> #include "mmc_private.h" >>> @@ -98,6 +100,11 @@ struct fsl_esdhc { >>> }; >>> >>> struct fsl_esdhc_plat { >>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >>> + /* Put this first since driver model will copy the data here */ >>> + struct dtd_fsl_imx6q_usdhc dtplat; >>> +#endif >>> + >>> struct mmc_config cfg; >>> struct mmc mmc; >>> }; >>> @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) >>> struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); >>> struct fsl_esdhc_plat *plat = dev_get_platdata(dev); >>> struct fsl_esdhc_priv *priv = dev_get_priv(dev); >>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) >>> const void *fdt = gd->fdt_blob; >>> int node = dev_of_offset(dev); >>> + fdt_addr_t addr; >>> +#else >>> + struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat; >>> +#endif >>> struct esdhc_soc_data *data = >>> (struct esdhc_soc_data *)dev_get_driver_data(dev); >>> #if CONFIG_IS_ENABLED(DM_REGULATOR) >>> struct udevice *vqmmc_dev; >>> #endif >>> - fdt_addr_t addr; >>> unsigned int val; >>> struct mmc *mmc; >>> #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 ddriver_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
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,
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;elsepriv->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;elsepriv->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
Hi Simon,
On 6/4/20 00:42, Simon Glass wrote:
Hi Walter
On Sun, 29 Mar 2020 at 21:32, Walter Lozanowalter.lozano@collabora.com wrote: All of these commits need a commit message please.
Thank you for pointing it.
Signed-off-by: Walter Lozanowalter.lozano@collabora.com
drivers/mmc/fsl_esdhc_imx.c | 71 ++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 29 deletions(-)
Reviewed-by: SImon Glasssjg@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);int ret;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;elsepriv->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;elsepriv->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,@@ -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,-- 2.20.1
Regards, Simon
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))
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 = Nonefor 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
Hi Simon,
On 6/4/20 00:42, Simon Glass wrote:
On Sun, 29 Mar 2020 at 21:32, Walter Lozanowalter.lozano@collabora.com wrote:
Signed-off-by: Walter Lozanowalter.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 = Nonefor 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
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 *
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 },
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
Hi Simon,
On 6/4/20 00:42, Simon Glass wrote:
Hi Walter,
On Sun, 29 Mar 2020 at 21:32, Walter Lozanowalter.lozano@collabora.com wrote:
Signed-off-by: Walter Lozanowalter.lozano@collabora.com
drivers/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
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)
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
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)
Hi Simon,
On 6/4/20 00:43, Simon Glass wrote:
On Sun, 29 Mar 2020 at 21:32, Walter Lozanowalter.lozano@collabora.com wrote:
Signed-off-by: Walter Lozanowalter.lozano@collabora.com
configs/mx6cuboxi_defconfig | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: SImon Glasssjg@chromium.org
(with commit msg)
Thank you.
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@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
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@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
participants (3)
-
Baruch Siach -
Simon Glass -
Walter Lozano