* [RFC 0/6] improve OF_PLATDATA support
@ 2020-05-13 20:13 Walter Lozano
2020-05-13 20:13 ` [RFC 1/6] dtoc: add support to scan drivers Walter Lozano
` (6 more replies)
0 siblings, 7 replies; 22+ messages in thread
From: Walter Lozano @ 2020-05-13 20:13 UTC (permalink / raw)
To: u-boot
When using OF_PLATDATA dtbs are converted to C structs in order to save
space as we can remove both dtbs and libraries from TPL/SPL binaries.
This patchset tries to improve its support by overcoming some limitations
in the current implementation
First, the support for scan and check for valid driver/aliases is added
in order to generate U_BOOT_DEVICE entries with valid driver names.
Secondly, the way information about linked noded (phandle) is generated
in C structs is improved in order to make it easier to get a device
associated to its data.
Lastly the the suport for the property cd-gpios is added, which is used to
configure the card detection gpio on MMC is added.
This implementation is based in discussion in [1] and [2]
[1] https://patchwork.ozlabs.org/patch/1249198/
[2] https://patchwork.ozlabs.org/project/uboot/list/?series=167495&state=*
Walter Lozano (6):
dtoc: add support to scan drivers
core: extend struct driver_info to point to device
dtoc: extend dtoc to use struct driver_info when linking nodes
dtoc: update tests to match new platdata
dtoc: update dtb_platdata to support cd-gpios
dtoc add test for cd-gpios
drivers/clk/clk-uclass.c | 8 +-
drivers/core/device.c | 25 ++-
drivers/core/root.c | 6 +-
drivers/misc/irq-uclass.c | 4 +-
drivers/mmc/ftsdc010_mci.c | 2 +-
drivers/mmc/rockchip_dw_mmc.c | 2 +-
drivers/mmc/rockchip_sdhci.c | 2 +-
drivers/ram/rockchip/sdram_rk3399.c | 2 +-
drivers/spi/rk_spi.c | 2 +-
include/clk.h | 2 +-
include/dm/device-internal.h | 2 +-
include/dm/device.h | 19 +++
include/dm/platdata.h | 6 +
tools/dtoc/dtb_platdata.py | 83 +++++++--
tools/dtoc/dtoc_test_phandle_cd_gpios.dts | 42 +++++
tools/dtoc/test_dtoc.py | 197 +++++++++++++++++-----
16 files changed, 332 insertions(+), 72 deletions(-)
create mode 100644 tools/dtoc/dtoc_test_phandle_cd_gpios.dts
--
2.20.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 1/6] dtoc: add support to scan drivers
2020-05-13 20:13 [RFC 0/6] improve OF_PLATDATA support Walter Lozano
@ 2020-05-13 20:13 ` Walter Lozano
2020-05-20 3:07 ` Simon Glass
2020-05-13 20:13 ` [RFC 2/6] core: extend struct driver_info to point to device Walter Lozano
` (5 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Walter Lozano @ 2020-05-13 20:13 UTC (permalink / raw)
To: u-boot
Currently dtoc scans dtbs to convert them to struct platdata and
to generate U_BOOT_DEVICE entries. These entries need to be filled
with the driver name, but at this moment the information used is the
compatible name present in the dtb. This causes that only nodes with
a compatible name that matches a driver name generate a working
entry.
In order to improve this behaviour, this patch adds to dtoc the
capability of scan drivers source code to generate a list of valid driver
names. This allows to rise a warning in the case that an U_BOOT_DEVICE
entry will try to use a name not valid.
Additionally, in order to add more flexibility to the solution, adds the
U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but allows an
easy way to declare driver name aliases. Thanks to this, dtoc can look
for the driver name based on its alias when it populates the U_BOOT_DEVICE entry.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
include/dm/device.h | 6 +++++
tools/dtoc/dtb_platdata.py | 53 +++++++++++++++++++++++++++++++++++---
2 files changed, 55 insertions(+), 4 deletions(-)
diff --git a/include/dm/device.h b/include/dm/device.h
index 975eec5d0e..d5b3e732c3 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -282,6 +282,12 @@ struct driver {
#define DM_GET_DRIVER(__name) \
ll_entry_get(struct driver, __name, driver)
+/* Declare a macro to state a alias for a driver name. This macro will
+ * produce no code but its information will be parsed by tools like
+ * dtoc
+ */
+#define U_BOOT_DRIVER_ALIAS(__name, __alias)
+
/**
* dev_get_platdata() - Get the platform data for a device
*
diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
index ecfe0624d1..3f899a8bac 100644
--- a/tools/dtoc/dtb_platdata.py
+++ b/tools/dtoc/dtb_platdata.py
@@ -14,6 +14,8 @@ static data.
import collections
import copy
import sys
+import os
+import re
from dtoc import fdt
from dtoc import fdt_util
@@ -149,6 +151,20 @@ class DtbPlatdata(object):
self._outfile = None
self._lines = []
self._aliases = {}
+ self._drivers = []
+ self._driver_aliases = {}
+
+ def get_normalized_compat_name(self, node):
+ compat_c, aliases_c = get_compat_name(node)
+ if compat_c not in self._drivers:
+ try: # pragma: no cover
+ compat_c_old = compat_c
+ compat_c = self._driver_aliases[compat_c]
+ aliases_c = [compat_c_old] + aliases
+ except:
+ print('WARNING: the driver %s was not found in the driver list' % (compat_c))
+
+ return compat_c, aliases_c
def setup_output(self, fname):
"""Set up the output destination
@@ -243,6 +259,34 @@ class DtbPlatdata(object):
return PhandleInfo(max_args, args)
return None
+ def scan_driver(self, fn):
+ f = open(fn)
+
+ b = f.read()
+
+ drivers = re.findall('U_BOOT_DRIVER\((.*)\)', b)
+
+ for d in drivers:
+ self._drivers.append(d)
+
+ driver_aliases = re.findall('U_BOOT_DRIVER_ALIAS\(\s*(\w+)\s*,\s*(\w+)\s*\)', b)
+
+ for a in driver_aliases: # pragma: no cover
+ try:
+ self._driver_aliases[a[1]] = a[0]
+ except:
+ pass
+
+ def scan_drivers(self):
+ """Scan the driver folders to build a list of driver names and possible
+ aliases
+ """
+ for (dirpath, dirnames, filenames) in os.walk('./'):
+ for fn in filenames:
+ if not fn.endswith('.c'):
+ continue
+ self.scan_driver(dirpath + '/' + fn)
+
def scan_dtb(self):
"""Scan the device tree to obtain a tree of nodes and properties
@@ -353,7 +397,7 @@ class DtbPlatdata(object):
"""
structs = {}
for node in self._valid_nodes:
- node_name, _ = get_compat_name(node)
+ node_name, _ = self.get_normalized_compat_name(node)
fields = {}
# Get a list of all the valid properties in this node.
@@ -377,14 +421,14 @@ class DtbPlatdata(object):
upto = 0
for node in self._valid_nodes:
- node_name, _ = get_compat_name(node)
+ node_name, _ = self.get_normalized_compat_name(node)
struct = structs[node_name]
for name, prop in node.props.items():
if name not in PROP_IGNORE_LIST and name[0] != '#':
prop.Widen(struct[name])
upto += 1
- struct_name, aliases = get_compat_name(node)
+ struct_name, aliases = self.get_normalized_compat_name(node)
for alias in aliases:
self._aliases[alias] = struct_name
@@ -461,7 +505,7 @@ class DtbPlatdata(object):
Args:
node: node to output
"""
- struct_name, _ = get_compat_name(node)
+ struct_name, _ = self.get_normalized_compat_name(node)
var_name = conv_name_to_c(node.name)
self.buf('static const struct %s%s %s%s = {\n' %
(STRUCT_PREFIX, struct_name, VAL_PREFIX, var_name))
@@ -562,6 +606,7 @@ def run_steps(args, dtb_file, include_disabled, output):
raise ValueError('Please specify a command: struct, platdata')
plat = DtbPlatdata(dtb_file, include_disabled)
+ plat.scan_drivers()
plat.scan_dtb()
plat.scan_tree()
plat.scan_reg_sizes()
--
2.20.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC 2/6] core: extend struct driver_info to point to device
2020-05-13 20:13 [RFC 0/6] improve OF_PLATDATA support Walter Lozano
2020-05-13 20:13 ` [RFC 1/6] dtoc: add support to scan drivers Walter Lozano
@ 2020-05-13 20:13 ` Walter Lozano
2020-05-20 3:07 ` Simon Glass
2020-05-13 20:13 ` [RFC 3/6] dtoc: extend dtoc to use struct driver_info when linking nodes Walter Lozano
` (4 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Walter Lozano @ 2020-05-13 20:13 UTC (permalink / raw)
To: u-boot
Currently when creating an U_BOOT_DEVICE entry a struct driver_info
is declared, which contains the data needed to instantiate the device.
However, the actual device is created at runtime and there is no proper
way to get the device based on its struct driver_info.
This patch extends struct driver_info adding a pointer to udevice which
is populated during the bind process, allowing to generate a set of
functions to get the device based on its struct driver_info.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
drivers/core/device.c | 25 +++++++++++++++++++++++--
drivers/core/root.c | 6 +++++-
include/dm/device-internal.h | 2 +-
include/dm/device.h | 13 +++++++++++++
include/dm/platdata.h | 6 ++++++
5 files changed, 48 insertions(+), 4 deletions(-)
diff --git a/drivers/core/device.c b/drivers/core/device.c
index 0157bb1fe0..2476f170a5 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -246,10 +246,11 @@ int device_bind_ofnode(struct udevice *parent, const struct driver *drv,
}
int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
- const struct driver_info *info, struct udevice **devp)
+ struct driver_info *info, struct udevice **devp)
{
struct driver *drv;
uint platdata_size = 0;
+ int ret = 0;
drv = lists_driver_lookup_name(info->name);
if (!drv)
@@ -260,9 +261,19 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
#if CONFIG_IS_ENABLED(OF_PLATDATA)
platdata_size = info->platdata_size;
#endif
- return device_bind_common(parent, drv, info->name,
+ ret = device_bind_common(parent, drv, info->name,
(void *)info->platdata, 0, ofnode_null(), platdata_size,
devp);
+
+ if (ret)
+ return ret;
+
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
+ info->dev = *devp;
+#endif
+
+ return ret;
+
}
static void *alloc_priv(int size, uint flags)
@@ -727,6 +738,16 @@ int device_get_global_by_ofnode(ofnode ofnode, struct udevice **devp)
return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
}
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
+int device_get_by_driver_info(struct driver_info * info, struct udevice **devp)
+{
+ struct udevice *dev;
+
+ dev = info->dev;
+ return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
+}
+#endif
+
int device_find_first_child(const struct udevice *parent, struct udevice **devp)
{
if (list_empty(&parent->child_head)) {
diff --git a/drivers/core/root.c b/drivers/core/root.c
index 14df16c280..8f47a6b356 100644
--- a/drivers/core/root.c
+++ b/drivers/core/root.c
@@ -25,7 +25,7 @@
DECLARE_GLOBAL_DATA_PTR;
-static const struct driver_info root_info = {
+static struct driver_info root_info = {
.name = "root_driver",
};
@@ -346,6 +346,10 @@ int dm_init_and_scan(bool pre_reloc_only)
{
int ret;
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
+ populate_phandle_data();
+#endif
+
ret = dm_init(IS_ENABLED(CONFIG_OF_LIVE));
if (ret) {
debug("dm_init() failed: %d\n", ret);
diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h
index 294d6c1810..5145fb4e14 100644
--- a/include/dm/device-internal.h
+++ b/include/dm/device-internal.h
@@ -81,7 +81,7 @@ int device_bind_with_driver_data(struct udevice *parent,
* @return 0 if OK, -ve on error
*/
int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
- const struct driver_info *info, struct udevice **devp);
+ struct driver_info *info, struct udevice **devp);
/**
* device_ofdata_to_platdata() - Read platform data for a device
diff --git a/include/dm/device.h b/include/dm/device.h
index d5b3e732c3..590c1f99ce 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -537,6 +537,19 @@ int device_find_global_by_ofnode(ofnode node, struct udevice **devp);
*/
int device_get_global_by_ofnode(ofnode node, struct udevice **devp);
+/**
+ * device_get_by_driver_info() - Get a device based on driver_info
+ *
+ * Locates a device by its struct driver_info.
+ *
+ * The device is probed to activate it ready for use.
+ *
+ * @info: Struct driver_info
+ * @devp: Returns pointer to device if found, otherwise this is set to NULL
+ * @return 0 if OK, -ve on error
+ */
+int device_get_by_driver_info(struct driver_info * info, struct udevice **devp);
+
/**
* device_find_first_child() - Find the first child of a device
*
diff --git a/include/dm/platdata.h b/include/dm/platdata.h
index c972fa6936..17eef7c7a3 100644
--- a/include/dm/platdata.h
+++ b/include/dm/platdata.h
@@ -28,6 +28,7 @@ struct driver_info {
const void *platdata;
#if CONFIG_IS_ENABLED(OF_PLATDATA)
uint platdata_size;
+ struct udevice *dev;
#endif
};
@@ -43,4 +44,9 @@ struct driver_info {
#define U_BOOT_DEVICES(__name) \
ll_entry_declare_list(struct driver_info, __name, driver_info)
+/* Get a pointer to a given driver */
+#define DM_GET_DEVICE(__name) \
+ ll_entry_get(struct driver_info, __name, driver_info)
+
+void populate_phandle_data(void);
#endif
--
2.20.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC 3/6] dtoc: extend dtoc to use struct driver_info when linking nodes
2020-05-13 20:13 [RFC 0/6] improve OF_PLATDATA support Walter Lozano
2020-05-13 20:13 ` [RFC 1/6] dtoc: add support to scan drivers Walter Lozano
2020-05-13 20:13 ` [RFC 2/6] core: extend struct driver_info to point to device Walter Lozano
@ 2020-05-13 20:13 ` Walter Lozano
2020-05-20 3:07 ` Simon Glass
2020-05-13 20:13 ` [RFC 4/6] dtoc: update tests to match new platdata Walter Lozano
` (3 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Walter Lozano @ 2020-05-13 20:13 UTC (permalink / raw)
To: u-boot
In the current implementation, when dtoc parses a dtb to generate a struct
platdata it converts the information related to linked nodes as pointers
to struct platdata of destination nodes. By doing this, it makes
difficult to get pointer to udevices created based on these
information.
This patch extends dtoc to use struct driver_info when populating
information about linked nodes, which makes it easier to later get
the devices created. In this context, reimplement functions like
clk_get_by_index_platdata() which made use of the previous approach.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
drivers/clk/clk-uclass.c | 8 +++-----
drivers/misc/irq-uclass.c | 4 ++--
drivers/mmc/ftsdc010_mci.c | 2 +-
drivers/mmc/rockchip_dw_mmc.c | 2 +-
drivers/mmc/rockchip_sdhci.c | 2 +-
drivers/ram/rockchip/sdram_rk3399.c | 2 +-
drivers/spi/rk_spi.c | 2 +-
include/clk.h | 2 +-
tools/dtoc/dtb_platdata.py | 17 ++++++++++++++---
9 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index 71878474eb..f24008fe00 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -25,14 +25,12 @@ static inline const struct clk_ops *clk_dev_ops(struct udevice *dev)
#if CONFIG_IS_ENABLED(OF_CONTROL)
# if CONFIG_IS_ENABLED(OF_PLATDATA)
-int clk_get_by_index_platdata(struct udevice *dev, int index,
- struct phandle_1_arg *cells, struct clk *clk)
+int clk_get_by_driver_info(struct udevice *dev, struct phandle_1_arg *cells,
+ struct clk *clk)
{
int ret;
- if (index != 0)
- return -ENOSYS;
- ret = uclass_get_device(UCLASS_CLK, 0, &clk->dev);
+ ret = device_get_by_driver_info((struct driver_info*) cells[0].node, &clk->dev);
if (ret)
return ret;
clk->id = cells[0].arg[0];
diff --git a/drivers/misc/irq-uclass.c b/drivers/misc/irq-uclass.c
index 61aa10e465..8eaebe6419 100644
--- a/drivers/misc/irq-uclass.c
+++ b/drivers/misc/irq-uclass.c
@@ -63,14 +63,14 @@ int irq_read_and_clear(struct irq *irq)
}
#if CONFIG_IS_ENABLED(OF_PLATDATA)
-int irq_get_by_index_platdata(struct udevice *dev, int index,
+int irq_get_by_driver_info(struct udevice *dev,
struct phandle_1_arg *cells, struct irq *irq)
{
int ret;
if (index != 0)
return -ENOSYS;
- ret = uclass_get_device(UCLASS_IRQ, 0, &irq->dev);
+ ret = device_get_by_driver_info(cells[0].node, &irq->dev);
if (ret)
return ret;
irq->id = cells[0].arg[0];
diff --git a/drivers/mmc/ftsdc010_mci.c b/drivers/mmc/ftsdc010_mci.c
index 9c15eb36d6..efa92d48be 100644
--- a/drivers/mmc/ftsdc010_mci.c
+++ b/drivers/mmc/ftsdc010_mci.c
@@ -437,7 +437,7 @@ static int ftsdc010_mmc_probe(struct udevice *dev)
chip->priv = dev;
chip->dev_index = 1;
memcpy(priv->minmax, dtplat->clock_freq_min_max, sizeof(priv->minmax));
- ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->clk);
+ ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->clk);
if (ret < 0)
return ret;
#endif
diff --git a/drivers/mmc/rockchip_dw_mmc.c b/drivers/mmc/rockchip_dw_mmc.c
index a0e1be8794..7b4479543c 100644
--- a/drivers/mmc/rockchip_dw_mmc.c
+++ b/drivers/mmc/rockchip_dw_mmc.c
@@ -120,7 +120,7 @@ static int rockchip_dwmmc_probe(struct udevice *dev)
priv->minmax[0] = 400000; /* 400 kHz */
priv->minmax[1] = dtplat->max_frequency;
- ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->clk);
+ ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->clk);
if (ret < 0)
return ret;
#else
diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
index b440996b26..b073f1a08d 100644
--- a/drivers/mmc/rockchip_sdhci.c
+++ b/drivers/mmc/rockchip_sdhci.c
@@ -46,7 +46,7 @@ static int arasan_sdhci_probe(struct udevice *dev)
host->name = dev->name;
host->ioaddr = map_sysmem(dtplat->reg[0], dtplat->reg[1]);
max_frequency = dtplat->max_frequency;
- ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk);
+ ret = clk_get_by_driver_info(dev, dtplat->clocks, &clk);
#else
max_frequency = dev_read_u32_default(dev, "max-frequency", 0);
ret = clk_get_by_index(dev, 0, &clk);
diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c
index d69ef01d08..87ec25f893 100644
--- a/drivers/ram/rockchip/sdram_rk3399.c
+++ b/drivers/ram/rockchip/sdram_rk3399.c
@@ -3125,7 +3125,7 @@ static int rk3399_dmc_init(struct udevice *dev)
priv->cic, priv->pmugrf, priv->pmusgrf, priv->pmucru, priv->pmu);
#if CONFIG_IS_ENABLED(OF_PLATDATA)
- ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->ddr_clk);
+ ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->ddr_clk);
#else
ret = clk_get_by_index(dev, 0, &priv->ddr_clk);
#endif
diff --git a/drivers/spi/rk_spi.c b/drivers/spi/rk_spi.c
index 95eeb8307a..bd0337272e 100644
--- a/drivers/spi/rk_spi.c
+++ b/drivers/spi/rk_spi.c
@@ -181,7 +181,7 @@ static int conv_of_platdata(struct udevice *dev)
plat->base = dtplat->reg[0];
plat->frequency = 20000000;
- ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->clk);
+ ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->clk);
if (ret < 0)
return ret;
dev->req_seq = 0;
diff --git a/include/clk.h b/include/clk.h
index c6a2713f62..3be379de03 100644
--- a/include/clk.h
+++ b/include/clk.h
@@ -89,7 +89,7 @@ struct clk_bulk {
#if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(CLK)
struct phandle_1_arg;
-int clk_get_by_index_platdata(struct udevice *dev, int index,
+int clk_get_by_driver_info(struct udevice *dev,
struct phandle_1_arg *cells, struct clk *clk);
/**
diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
index 3f899a8bac..d30e2af2ec 100644
--- a/tools/dtoc/dtb_platdata.py
+++ b/tools/dtoc/dtb_platdata.py
@@ -153,6 +153,7 @@ class DtbPlatdata(object):
self._aliases = {}
self._drivers = []
self._driver_aliases = {}
+ self._links = []
def get_normalized_compat_name(self, node):
compat_c, aliases_c = get_compat_name(node)
@@ -507,7 +508,7 @@ class DtbPlatdata(object):
"""
struct_name, _ = self.get_normalized_compat_name(node)
var_name = conv_name_to_c(node.name)
- self.buf('static const struct %s%s %s%s = {\n' %
+ self.buf('static struct %s%s %s%s = {\n' %
(STRUCT_PREFIX, struct_name, VAL_PREFIX, var_name))
for pname in sorted(node.props):
prop = node.props[pname]
@@ -526,6 +527,7 @@ class DtbPlatdata(object):
if info:
# Process the list as pairs of (phandle, id)
pos = 0
+ item = 0
for args in info.args:
phandle_cell = prop.value[pos]
phandle = fdt_util.fdt32_to_cpu(phandle_cell)
@@ -535,8 +537,10 @@ class DtbPlatdata(object):
for i in range(args):
arg_values.append(str(fdt_util.fdt32_to_cpu(prop.value[pos + 1 + i])))
pos += 1 + args
- vals.append('\t{&%s%s, {%s}}' % (VAL_PREFIX, name,
- ', '.join(arg_values)))
+ vals.append('\t{NULL, {%s}}' % (', '.join(arg_values)))
+ phandle_entry = '%s%s.%s[%d].node = DM_GET_DEVICE(%s)' % (VAL_PREFIX, var_name, member_name, item, name)
+ self._links.append(phandle_entry)
+ item += 1
for val in vals:
self.buf('\n\t\t%s,' % val)
else:
@@ -559,6 +563,7 @@ class DtbPlatdata(object):
self.buf('\t.name\t\t= "%s",\n' % struct_name)
self.buf('\t.platdata\t= &%s%s,\n' % (VAL_PREFIX, var_name))
self.buf('\t.platdata_size\t= sizeof(%s%s),\n' % (VAL_PREFIX, var_name))
+ self.buf('\t.dev\t\t= NULL,\n')
self.buf('};\n')
self.buf('\n')
@@ -592,6 +597,12 @@ class DtbPlatdata(object):
self.output_node(node)
nodes_to_output.remove(node)
+ self.buf('void populate_phandle_data(void) {\n')
+ for l in self._links:
+ self.buf(('\t%s;\n' % l))
+ self.buf('}\n')
+
+ self.out(''.join(self.get_buf()))
def run_steps(args, dtb_file, include_disabled, output):
"""Run all the steps of the dtoc tool
--
2.20.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC 4/6] dtoc: update tests to match new platdata
2020-05-13 20:13 [RFC 0/6] improve OF_PLATDATA support Walter Lozano
` (2 preceding siblings ...)
2020-05-13 20:13 ` [RFC 3/6] dtoc: extend dtoc to use struct driver_info when linking nodes Walter Lozano
@ 2020-05-13 20:13 ` Walter Lozano
2020-05-20 3:07 ` Simon Glass
2020-05-13 20:13 ` [RFC 5/6] dtoc: update dtb_platdata to support cd-gpios Walter Lozano
` (2 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Walter Lozano @ 2020-05-13 20:13 UTC (permalink / raw)
To: u-boot
After using a new approach to link nodes when OF_PLATDATA is enabled
the test cases need to be update.
This patch updates the tests based on this new implementation.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
tools/dtoc/test_dtoc.py | 123 ++++++++++++++++++++++++++--------------
1 file changed, 81 insertions(+), 42 deletions(-)
diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py
index 8498e8303c..7e8947d4af 100755
--- a/tools/dtoc/test_dtoc.py
+++ b/tools/dtoc/test_dtoc.py
@@ -47,7 +47,9 @@ C_HEADER = '''/*
#include <dt-structs.h>
'''
-
+C_EMPTY_POPULATE_PHANDLE_DATA = '''void populate_phandle_data(void) {
+}
+'''
def get_dtb_file(dts_fname, capture_stderr=False):
"""Compile a .dts file to a .dtb
@@ -162,7 +164,7 @@ class TestDtoc(unittest.TestCase):
dtb_platdata.run_steps(['platdata'], dtb_file, False, output)
with open(output) as infile:
lines = infile.read().splitlines()
- self.assertEqual(C_HEADER.splitlines() + [''], lines)
+ self.assertEqual(C_HEADER.splitlines() + [''] + C_EMPTY_POPULATE_PHANDLE_DATA.splitlines(), lines)
def test_simple(self):
"""Test output from some simple nodes with various types of data"""
@@ -197,7 +199,7 @@ struct dtd_sandbox_spl_test_2 {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_sandbox_spl_test dtv_spl_test = {
+static struct dtd_sandbox_spl_test dtv_spl_test = {
\t.boolval\t\t= true,
\t.bytearray\t\t= {0x6, 0x0, 0x0},
\t.byteval\t\t= 0x5,
@@ -213,9 +215,10 @@ U_BOOT_DEVICE(spl_test) = {
\t.name\t\t= "sandbox_spl_test",
\t.platdata\t= &dtv_spl_test,
\t.platdata_size\t= sizeof(dtv_spl_test),
+\t.dev\t\t= NULL,
};
-static const struct dtd_sandbox_spl_test dtv_spl_test2 = {
+static struct dtd_sandbox_spl_test dtv_spl_test2 = {
\t.bytearray\t\t= {0x1, 0x23, 0x34},
\t.byteval\t\t= 0x8,
\t.intarray\t\t= {0x5, 0x0, 0x0, 0x0},
@@ -229,34 +232,38 @@ U_BOOT_DEVICE(spl_test2) = {
\t.name\t\t= "sandbox_spl_test",
\t.platdata\t= &dtv_spl_test2,
\t.platdata_size\t= sizeof(dtv_spl_test2),
+\t.dev\t\t= NULL,
};
-static const struct dtd_sandbox_spl_test dtv_spl_test3 = {
+static struct dtd_sandbox_spl_test dtv_spl_test3 = {
\t.stringarray\t\t= {"one", "", ""},
};
U_BOOT_DEVICE(spl_test3) = {
\t.name\t\t= "sandbox_spl_test",
\t.platdata\t= &dtv_spl_test3,
\t.platdata_size\t= sizeof(dtv_spl_test3),
+\t.dev\t\t= NULL,
};
-static const struct dtd_sandbox_spl_test_2 dtv_spl_test4 = {
+static struct dtd_sandbox_spl_test_2 dtv_spl_test4 = {
};
U_BOOT_DEVICE(spl_test4) = {
\t.name\t\t= "sandbox_spl_test_2",
\t.platdata\t= &dtv_spl_test4,
\t.platdata_size\t= sizeof(dtv_spl_test4),
+\t.dev\t\t= NULL,
};
-static const struct dtd_sandbox_i2c_test dtv_i2c_at_0 = {
+static struct dtd_sandbox_i2c_test dtv_i2c_at_0 = {
};
U_BOOT_DEVICE(i2c_at_0) = {
\t.name\t\t= "sandbox_i2c_test",
\t.platdata\t= &dtv_i2c_at_0,
\t.platdata_size\t= sizeof(dtv_i2c_at_0),
+\t.dev\t\t= NULL,
};
-static const struct dtd_sandbox_pmic_test dtv_pmic_at_9 = {
+static struct dtd_sandbox_pmic_test dtv_pmic_at_9 = {
\t.low_power\t\t= true,
\t.reg\t\t\t= {0x9, 0x0},
};
@@ -264,9 +271,10 @@ U_BOOT_DEVICE(pmic_at_9) = {
\t.name\t\t= "sandbox_pmic_test",
\t.platdata\t= &dtv_pmic_at_9,
\t.platdata_size\t= sizeof(dtv_pmic_at_9),
+\t.dev\t\t= NULL,
};
-''', data)
+''' + C_EMPTY_POPULATE_PHANDLE_DATA, data)
def test_phandle(self):
"""Test output from a node containing a phandle reference"""
@@ -288,56 +296,68 @@ struct dtd_target {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_target dtv_phandle_target = {
+static struct dtd_target dtv_phandle_target = {
\t.intval\t\t\t= 0x0,
};
U_BOOT_DEVICE(phandle_target) = {
\t.name\t\t= "target",
\t.platdata\t= &dtv_phandle_target,
\t.platdata_size\t= sizeof(dtv_phandle_target),
+\t.dev\t\t= NULL,
};
-static const struct dtd_target dtv_phandle2_target = {
+static struct dtd_target dtv_phandle2_target = {
\t.intval\t\t\t= 0x1,
};
U_BOOT_DEVICE(phandle2_target) = {
\t.name\t\t= "target",
\t.platdata\t= &dtv_phandle2_target,
\t.platdata_size\t= sizeof(dtv_phandle2_target),
+\t.dev\t\t= NULL,
};
-static const struct dtd_target dtv_phandle3_target = {
+static struct dtd_target dtv_phandle3_target = {
\t.intval\t\t\t= 0x2,
};
U_BOOT_DEVICE(phandle3_target) = {
\t.name\t\t= "target",
\t.platdata\t= &dtv_phandle3_target,
\t.platdata_size\t= sizeof(dtv_phandle3_target),
+\t.dev\t\t= NULL,
};
-static const struct dtd_source dtv_phandle_source = {
+static struct dtd_source dtv_phandle_source = {
\t.clocks\t\t\t= {
-\t\t\t{&dtv_phandle_target, {}},
-\t\t\t{&dtv_phandle2_target, {11}},
-\t\t\t{&dtv_phandle3_target, {12, 13}},
-\t\t\t{&dtv_phandle_target, {}},},
+\t\t\t{NULL, {}},
+\t\t\t{NULL, {11}},
+\t\t\t{NULL, {12, 13}},
+\t\t\t{NULL, {}},},
};
U_BOOT_DEVICE(phandle_source) = {
\t.name\t\t= "source",
\t.platdata\t= &dtv_phandle_source,
\t.platdata_size\t= sizeof(dtv_phandle_source),
+\t.dev\t\t= NULL,
};
-static const struct dtd_source dtv_phandle_source2 = {
+static struct dtd_source dtv_phandle_source2 = {
\t.clocks\t\t\t= {
-\t\t\t{&dtv_phandle_target, {}},},
+\t\t\t{NULL, {}},},
};
U_BOOT_DEVICE(phandle_source2) = {
\t.name\t\t= "source",
\t.platdata\t= &dtv_phandle_source2,
\t.platdata_size\t= sizeof(dtv_phandle_source2),
+\t.dev\t\t= NULL,
};
+void populate_phandle_data(void) {
+\tdtv_phandle_source.clocks[0].node = DM_GET_DEVICE(phandle_target);
+\tdtv_phandle_source.clocks[1].node = DM_GET_DEVICE(phandle2_target);
+\tdtv_phandle_source.clocks[2].node = DM_GET_DEVICE(phandle3_target);
+\tdtv_phandle_source.clocks[3].node = DM_GET_DEVICE(phandle_target);
+\tdtv_phandle_source2.clocks[0].node = DM_GET_DEVICE(phandle_target);
+}
''', data)
def test_phandle_single(self):
@@ -364,24 +384,29 @@ struct dtd_target {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_target dtv_phandle_target = {
+static struct dtd_target dtv_phandle_target = {
};
U_BOOT_DEVICE(phandle_target) = {
\t.name\t\t= "target",
\t.platdata\t= &dtv_phandle_target,
\t.platdata_size\t= sizeof(dtv_phandle_target),
+\t.dev\t\t= NULL,
};
-static const struct dtd_source dtv_phandle_source2 = {
+static struct dtd_source dtv_phandle_source2 = {
\t.clocks\t\t\t= {
-\t\t\t{&dtv_phandle_target, {}},},
+\t\t\t{NULL, {}},},
};
U_BOOT_DEVICE(phandle_source2) = {
\t.name\t\t= "source",
\t.platdata\t= &dtv_phandle_source2,
\t.platdata_size\t= sizeof(dtv_phandle_source2),
+\t.dev\t\t= NULL,
};
+void populate_phandle_data(void) {
+\tdtv_phandle_source2.clocks[0].node = DM_GET_DEVICE(phandle_target);
+}
''', data)
def test_phandle_bad(self):
@@ -423,16 +448,17 @@ struct dtd_compat1 {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_compat1 dtv_spl_test = {
+static struct dtd_compat1 dtv_spl_test = {
\t.intval\t\t\t= 0x1,
};
U_BOOT_DEVICE(spl_test) = {
\t.name\t\t= "compat1",
\t.platdata\t= &dtv_spl_test,
\t.platdata_size\t= sizeof(dtv_spl_test),
+\t.dev\t\t= NULL,
};
-''', data)
+''' + C_EMPTY_POPULATE_PHANDLE_DATA, data)
def test_addresses64(self):
"""Test output from a node with a 'reg' property with na=2, ns=2"""
@@ -457,34 +483,37 @@ struct dtd_test3 {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_test1 dtv_test1 = {
+static struct dtd_test1 dtv_test1 = {
\t.reg\t\t\t= {0x1234, 0x5678},
};
U_BOOT_DEVICE(test1) = {
\t.name\t\t= "test1",
\t.platdata\t= &dtv_test1,
\t.platdata_size\t= sizeof(dtv_test1),
+\t.dev\t\t= NULL,
};
-static const struct dtd_test2 dtv_test2 = {
+static struct dtd_test2 dtv_test2 = {
\t.reg\t\t\t= {0x1234567890123456, 0x9876543210987654},
};
U_BOOT_DEVICE(test2) = {
\t.name\t\t= "test2",
\t.platdata\t= &dtv_test2,
\t.platdata_size\t= sizeof(dtv_test2),
+\t.dev\t\t= NULL,
};
-static const struct dtd_test3 dtv_test3 = {
+static struct dtd_test3 dtv_test3 = {
\t.reg\t\t\t= {0x1234567890123456, 0x9876543210987654, 0x2, 0x3},
};
U_BOOT_DEVICE(test3) = {
\t.name\t\t= "test3",
\t.platdata\t= &dtv_test3,
\t.platdata_size\t= sizeof(dtv_test3),
+\t.dev\t\t= NULL,
};
-''', data)
+''' + C_EMPTY_POPULATE_PHANDLE_DATA, data)
def test_addresses32(self):
"""Test output from a node with a 'reg' property with na=1, ns=1"""
@@ -506,25 +535,27 @@ struct dtd_test2 {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_test1 dtv_test1 = {
+static struct dtd_test1 dtv_test1 = {
\t.reg\t\t\t= {0x1234, 0x5678},
};
U_BOOT_DEVICE(test1) = {
\t.name\t\t= "test1",
\t.platdata\t= &dtv_test1,
\t.platdata_size\t= sizeof(dtv_test1),
+\t.dev\t\t= NULL,
};
-static const struct dtd_test2 dtv_test2 = {
+static struct dtd_test2 dtv_test2 = {
\t.reg\t\t\t= {0x12345678, 0x98765432, 0x2, 0x3},
};
U_BOOT_DEVICE(test2) = {
\t.name\t\t= "test2",
\t.platdata\t= &dtv_test2,
\t.platdata_size\t= sizeof(dtv_test2),
+\t.dev\t\t= NULL,
};
-''', data)
+''' + C_EMPTY_POPULATE_PHANDLE_DATA, data)
def test_addresses64_32(self):
"""Test output from a node with a 'reg' property with na=2, ns=1"""
@@ -549,34 +580,37 @@ struct dtd_test3 {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_test1 dtv_test1 = {
+static struct dtd_test1 dtv_test1 = {
\t.reg\t\t\t= {0x123400000000, 0x5678},
};
U_BOOT_DEVICE(test1) = {
\t.name\t\t= "test1",
\t.platdata\t= &dtv_test1,
\t.platdata_size\t= sizeof(dtv_test1),
+\t.dev\t\t= NULL,
};
-static const struct dtd_test2 dtv_test2 = {
+static struct dtd_test2 dtv_test2 = {
\t.reg\t\t\t= {0x1234567890123456, 0x98765432},
};
U_BOOT_DEVICE(test2) = {
\t.name\t\t= "test2",
\t.platdata\t= &dtv_test2,
\t.platdata_size\t= sizeof(dtv_test2),
+\t.dev\t\t= NULL,
};
-static const struct dtd_test3 dtv_test3 = {
+static struct dtd_test3 dtv_test3 = {
\t.reg\t\t\t= {0x1234567890123456, 0x98765432, 0x2, 0x3},
};
U_BOOT_DEVICE(test3) = {
\t.name\t\t= "test3",
\t.platdata\t= &dtv_test3,
\t.platdata_size\t= sizeof(dtv_test3),
+\t.dev\t\t= NULL,
};
-''', data)
+''' + C_EMPTY_POPULATE_PHANDLE_DATA, data)
def test_addresses32_64(self):
"""Test output from a node with a 'reg' property with na=1, ns=2"""
@@ -601,34 +635,37 @@ struct dtd_test3 {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_test1 dtv_test1 = {
+static struct dtd_test1 dtv_test1 = {
\t.reg\t\t\t= {0x1234, 0x567800000000},
};
U_BOOT_DEVICE(test1) = {
\t.name\t\t= "test1",
\t.platdata\t= &dtv_test1,
\t.platdata_size\t= sizeof(dtv_test1),
+\t.dev\t\t= NULL,
};
-static const struct dtd_test2 dtv_test2 = {
+static struct dtd_test2 dtv_test2 = {
\t.reg\t\t\t= {0x12345678, 0x9876543210987654},
};
U_BOOT_DEVICE(test2) = {
\t.name\t\t= "test2",
\t.platdata\t= &dtv_test2,
\t.platdata_size\t= sizeof(dtv_test2),
+\t.dev\t\t= NULL,
};
-static const struct dtd_test3 dtv_test3 = {
+static struct dtd_test3 dtv_test3 = {
\t.reg\t\t\t= {0x12345678, 0x9876543210987654, 0x2, 0x3},
};
U_BOOT_DEVICE(test3) = {
\t.name\t\t= "test3",
\t.platdata\t= &dtv_test3,
\t.platdata_size\t= sizeof(dtv_test3),
+\t.dev\t\t= NULL,
};
-''', data)
+''' + C_EMPTY_POPULATE_PHANDLE_DATA, data)
def test_bad_reg(self):
"""Test that a reg property with an invalid type generates an error"""
@@ -668,25 +705,27 @@ struct dtd_sandbox_spl_test {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_sandbox_spl_test dtv_spl_test = {
+static struct dtd_sandbox_spl_test dtv_spl_test = {
\t.intval\t\t\t= 0x1,
};
U_BOOT_DEVICE(spl_test) = {
\t.name\t\t= "sandbox_spl_test",
\t.platdata\t= &dtv_spl_test,
\t.platdata_size\t= sizeof(dtv_spl_test),
+\t.dev\t\t= NULL,
};
-static const struct dtd_sandbox_spl_test dtv_spl_test2 = {
+static struct dtd_sandbox_spl_test dtv_spl_test2 = {
\t.intarray\t\t= 0x5,
};
U_BOOT_DEVICE(spl_test2) = {
\t.name\t\t= "sandbox_spl_test",
\t.platdata\t= &dtv_spl_test2,
\t.platdata_size\t= sizeof(dtv_spl_test2),
+\t.dev\t\t= NULL,
};
-''', data)
+''' + C_EMPTY_POPULATE_PHANDLE_DATA, data)
def testStdout(self):
"""Test output to stdout"""
--
2.20.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC 5/6] dtoc: update dtb_platdata to support cd-gpios
2020-05-13 20:13 [RFC 0/6] improve OF_PLATDATA support Walter Lozano
` (3 preceding siblings ...)
2020-05-13 20:13 ` [RFC 4/6] dtoc: update tests to match new platdata Walter Lozano
@ 2020-05-13 20:13 ` Walter Lozano
2020-05-20 3:07 ` Simon Glass
2020-05-13 20:13 ` [RFC 6/6] dtoc add test for cd-gpios Walter Lozano
2020-05-20 3:07 ` [RFC 0/6] improve OF_PLATDATA support Simon Glass
6 siblings, 1 reply; 22+ messages in thread
From: Walter Lozano @ 2020-05-13 20:13 UTC (permalink / raw)
To: u-boot
Currently dtoc does not support the property cd-gpios used to declare
the gpios for card detect in mmc.
This patch adds support to cd-gpios property.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
tools/dtoc/dtb_platdata.py | 13 ++++++++-----
tools/dtoc/test_dtoc.py | 2 +-
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
index d30e2af2ec..71a734b408 100644
--- a/tools/dtoc/dtb_platdata.py
+++ b/tools/dtoc/dtb_platdata.py
@@ -228,7 +228,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
@@ -248,11 +248,14 @@ 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))
+ raise ValueError("Node '%s' has no cells property" %
+ (target.name))
num_args = fdt_util.fdt32_to_cpu(cells.value)
max_args = max(max_args, num_args)
args.append(num_args)
diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py
index 7e8947d4af..814988c374 100755
--- a/tools/dtoc/test_dtoc.py
+++ b/tools/dtoc/test_dtoc.py
@@ -426,7 +426,7 @@ void populate_phandle_data(void) {
output = tools.GetOutputFilename('output')
with self.assertRaises(ValueError) as e:
dtb_platdata.run_steps(['struct'], dtb_file, False, output)
- self.assertIn("Node 'phandle-target' has no '#clock-cells' property",
+ self.assertIn("Node 'phandle-target' has no cells property",
str(e.exception))
def test_aliases(self):
--
2.20.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC 6/6] dtoc add test for cd-gpios
2020-05-13 20:13 [RFC 0/6] improve OF_PLATDATA support Walter Lozano
` (4 preceding siblings ...)
2020-05-13 20:13 ` [RFC 5/6] dtoc: update dtb_platdata to support cd-gpios Walter Lozano
@ 2020-05-13 20:13 ` Walter Lozano
2020-05-20 3:07 ` Simon Glass
2020-05-20 3:07 ` [RFC 0/6] improve OF_PLATDATA support Simon Glass
6 siblings, 1 reply; 22+ messages in thread
From: Walter Lozano @ 2020-05-13 20:13 UTC (permalink / raw)
To: u-boot
Add a test for dtoc taking into account the cd-gpios property.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
tools/dtoc/dtoc_test_phandle_cd_gpios.dts | 42 +++++++++++++
tools/dtoc/test_dtoc.py | 72 +++++++++++++++++++++++
2 files changed, 114 insertions(+)
create mode 100644 tools/dtoc/dtoc_test_phandle_cd_gpios.dts
diff --git a/tools/dtoc/dtoc_test_phandle_cd_gpios.dts b/tools/dtoc/dtoc_test_phandle_cd_gpios.dts
new file mode 100644
index 0000000000..1029ce03b8
--- /dev/null
+++ b/tools/dtoc/dtoc_test_phandle_cd_gpios.dts
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test device tree file for dtoc
+ *
+ * Copyright 2017 Google, Inc
+ */
+
+ /dts-v1/;
+
+/ {
+ phandle: phandle-target {
+ u-boot,dm-pre-reloc;
+ compatible = "target";
+ intval = <0>;
+ #gpio-cells = <0>;
+ };
+
+ phandle_1: phandle2-target {
+ u-boot,dm-pre-reloc;
+ compatible = "target";
+ intval = <1>;
+ #gpio-cells = <1>;
+ };
+ phandle_2: phandle3-target {
+ u-boot,dm-pre-reloc;
+ compatible = "target";
+ intval = <2>;
+ #gpio-cells = <2>;
+ };
+
+ phandle-source {
+ u-boot,dm-pre-reloc;
+ compatible = "source";
+ cd-gpios = <&phandle &phandle_1 11 &phandle_2 12 13 &phandle>;
+ };
+
+ phandle-source2 {
+ u-boot,dm-pre-reloc;
+ compatible = "source";
+ cd-gpios = <&phandle>;
+ };
+};
diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py
index 814988c374..f3d5e77880 100755
--- a/tools/dtoc/test_dtoc.py
+++ b/tools/dtoc/test_dtoc.py
@@ -407,6 +407,78 @@ U_BOOT_DEVICE(phandle_source2) = {
void populate_phandle_data(void) {
\tdtv_phandle_source2.clocks[0].node = DM_GET_DEVICE(phandle_target);
}
+''', data)
+
+ def test_phandle_cd_gpio(self):
+ """Test that phandle targets are generated when unsing cd-gpios"""
+ dtb_file = get_dtb_file('dtoc_test_phandle_cd_gpios.dts')
+ output = tools.GetOutputFilename('output')
+ dtb_platdata.run_steps(['platdata'], dtb_file, False, output)
+ with open(output) as infile:
+ data = infile.read()
+ self._CheckStrings(C_HEADER + '''
+static struct dtd_target dtv_phandle_target = {
+\t.intval\t\t\t= 0x0,
+};
+U_BOOT_DEVICE(phandle_target) = {
+\t.name\t\t= "target",
+\t.platdata\t= &dtv_phandle_target,
+\t.platdata_size\t= sizeof(dtv_phandle_target),
+\t.dev\t\t= NULL,
+};
+
+static struct dtd_target dtv_phandle2_target = {
+\t.intval\t\t\t= 0x1,
+};
+U_BOOT_DEVICE(phandle2_target) = {
+\t.name\t\t= "target",
+\t.platdata\t= &dtv_phandle2_target,
+\t.platdata_size\t= sizeof(dtv_phandle2_target),
+\t.dev\t\t= NULL,
+};
+
+static struct dtd_target dtv_phandle3_target = {
+\t.intval\t\t\t= 0x2,
+};
+U_BOOT_DEVICE(phandle3_target) = {
+\t.name\t\t= "target",
+\t.platdata\t= &dtv_phandle3_target,
+\t.platdata_size\t= sizeof(dtv_phandle3_target),
+\t.dev\t\t= NULL,
+};
+
+static struct dtd_source dtv_phandle_source = {
+\t.cd_gpios\t\t= {
+\t\t\t{NULL, {}},
+\t\t\t{NULL, {11}},
+\t\t\t{NULL, {12, 13}},
+\t\t\t{NULL, {}},},
+};
+U_BOOT_DEVICE(phandle_source) = {
+\t.name\t\t= "source",
+\t.platdata\t= &dtv_phandle_source,
+\t.platdata_size\t= sizeof(dtv_phandle_source),
+\t.dev\t\t= NULL,
+};
+
+static struct dtd_source dtv_phandle_source2 = {
+\t.cd_gpios\t\t= {
+\t\t\t{NULL, {}},},
+};
+U_BOOT_DEVICE(phandle_source2) = {
+\t.name\t\t= "source",
+\t.platdata\t= &dtv_phandle_source2,
+\t.platdata_size\t= sizeof(dtv_phandle_source2),
+\t.dev\t\t= NULL,
+};
+
+void populate_phandle_data(void) {
+\tdtv_phandle_source.cd_gpios[0].node = DM_GET_DEVICE(phandle_target);
+\tdtv_phandle_source.cd_gpios[1].node = DM_GET_DEVICE(phandle2_target);
+\tdtv_phandle_source.cd_gpios[2].node = DM_GET_DEVICE(phandle3_target);
+\tdtv_phandle_source.cd_gpios[3].node = DM_GET_DEVICE(phandle_target);
+\tdtv_phandle_source2.cd_gpios[0].node = DM_GET_DEVICE(phandle_target);
+}
''', data)
def test_phandle_bad(self):
--
2.20.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC 1/6] dtoc: add support to scan drivers
2020-05-13 20:13 ` [RFC 1/6] dtoc: add support to scan drivers Walter Lozano
@ 2020-05-20 3:07 ` Simon Glass
2020-05-21 13:15 ` Walter Lozano
0 siblings, 1 reply; 22+ messages in thread
From: Simon Glass @ 2020-05-20 3:07 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Wed, 13 May 2020 at 14:13, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Currently dtoc scans dtbs to convert them to struct platdata and
> to generate U_BOOT_DEVICE entries. These entries need to be filled
> with the driver name, but at this moment the information used is the
> compatible name present in the dtb. This causes that only nodes with
> a compatible name that matches a driver name generate a working
> entry.
>
> In order to improve this behaviour, this patch adds to dtoc the
> capability of scan drivers source code to generate a list of valid driver
> names. This allows to rise a warning in the case that an U_BOOT_DEVICE
> entry will try to use a name not valid.
>
> Additionally, in order to add more flexibility to the solution, adds the
> U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but allows an
> easy way to declare driver name aliases. Thanks to this, dtoc can look
> for the driver name based on its alias when it populates the U_BOOT_DEVICE entry.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> include/dm/device.h | 6 +++++
> tools/dtoc/dtb_platdata.py | 53 +++++++++++++++++++++++++++++++++++---
> 2 files changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/include/dm/device.h b/include/dm/device.h
> index 975eec5d0e..d5b3e732c3 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -282,6 +282,12 @@ struct driver {
> #define DM_GET_DRIVER(__name) \
> ll_entry_get(struct driver, __name, driver)
>
> +/* Declare a macro to state a alias for a driver name. This macro will
/*
* Declare ...
> + * produce no code but its information will be parsed by tools like
> + * dtoc
> + */
> +#define U_BOOT_DRIVER_ALIAS(__name, __alias)
> +
> /**
> * dev_get_platdata() - Get the platform data for a device
> *
> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
> index ecfe0624d1..3f899a8bac 100644
> --- a/tools/dtoc/dtb_platdata.py
> +++ b/tools/dtoc/dtb_platdata.py
> @@ -14,6 +14,8 @@ static data.
> import collections
> import copy
> import sys
> +import os
> +import re
please keep sorted
>
> from dtoc import fdt
> from dtoc import fdt_util
> @@ -149,6 +151,20 @@ class DtbPlatdata(object):
> self._outfile = None
> self._lines = []
> self._aliases = {}
> + self._drivers = []
> + self._driver_aliases = {}
Please update comment above for what these new things are for.
> +
> + def get_normalized_compat_name(self, node):
Needs function comment (also for those below).
> + compat_c, aliases_c = get_compat_name(node)
> + if compat_c not in self._drivers:
> + try: # pragma: no cover
Instead of an exception can you use .get() and check for None?
> + compat_c_old = compat_c
> + compat_c = self._driver_aliases[compat_c]
> + aliases_c = [compat_c_old] + aliases
> + except:
> + print('WARNING: the driver %s was not found in the driver list' % (compat_c))
> +
> + return compat_c, aliases_c
>
> def setup_output(self, fname):
> """Set up the output destination
> @@ -243,6 +259,34 @@ class DtbPlatdata(object):
> return PhandleInfo(max_args, args)
> return None
>
> + def scan_driver(self, fn):
> + f = open(fn)
Can you avoid single-char var names? Also how about either using
tools.ReadFile() or:
with open(fname) as fd:
...rest of function
> +
> + b = f.read()
> +
> + drivers = re.findall('U_BOOT_DRIVER\((.*)\)', b)
> +
> + for d in drivers:
> + self._drivers.append(d)
> +
> + driver_aliases = re.findall('U_BOOT_DRIVER_ALIAS\(\s*(\w+)\s*,\s*(\w+)\s*\)', b)
Can you add a comment before this with an example of the line you are
parsing? It helps make sense of the regex.
> +
> + for a in driver_aliases: # pragma: no cover
> + try:
> + self._driver_aliases[a[1]] = a[0]
> + except:
> + pass
Again please avoid exceptions.
> +
> + def scan_drivers(self):
> + """Scan the driver folders to build a list of driver names and possible
> + aliases
> + """
> + for (dirpath, dirnames, filenames) in os.walk('./'):
> + for fn in filenames:
> + if not fn.endswith('.c'):
> + continue
> + self.scan_driver(dirpath + '/' + fn)
> +
> def scan_dtb(self):
> """Scan the device tree to obtain a tree of nodes and properties
>
> @@ -353,7 +397,7 @@ class DtbPlatdata(object):
> """
> structs = {}
> for node in self._valid_nodes:
> - node_name, _ = get_compat_name(node)
> + node_name, _ = self.get_normalized_compat_name(node)
> fields = {}
I wonder how long this scan takes? Should we cache the results somewhere?
>
> # Get a list of all the valid properties in this node.
> @@ -377,14 +421,14 @@ class DtbPlatdata(object):
>
> upto = 0
> for node in self._valid_nodes:
> - node_name, _ = get_compat_name(node)
> + node_name, _ = self.get_normalized_compat_name(node)
> struct = structs[node_name]
> for name, prop in node.props.items():
> if name not in PROP_IGNORE_LIST and name[0] != '#':
> prop.Widen(struct[name])
> upto += 1
>
> - struct_name, aliases = get_compat_name(node)
> + struct_name, aliases = self.get_normalized_compat_name(node)
> for alias in aliases:
> self._aliases[alias] = struct_name
>
> @@ -461,7 +505,7 @@ class DtbPlatdata(object):
> Args:
> node: node to output
> """
> - struct_name, _ = get_compat_name(node)
> + struct_name, _ = self.get_normalized_compat_name(node)
> var_name = conv_name_to_c(node.name)
> self.buf('static const struct %s%s %s%s = {\n' %
> (STRUCT_PREFIX, struct_name, VAL_PREFIX, var_name))
> @@ -562,6 +606,7 @@ def run_steps(args, dtb_file, include_disabled, output):
> raise ValueError('Please specify a command: struct, platdata')
>
> plat = DtbPlatdata(dtb_file, include_disabled)
> + plat.scan_drivers()
> plat.scan_dtb()
> plat.scan_tree()
> plat.scan_reg_sizes()
> --
> 2.20.1
>
Regards,
SImon
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 2/6] core: extend struct driver_info to point to device
2020-05-13 20:13 ` [RFC 2/6] core: extend struct driver_info to point to device Walter Lozano
@ 2020-05-20 3:07 ` Simon Glass
2020-05-21 13:15 ` Walter Lozano
0 siblings, 1 reply; 22+ messages in thread
From: Simon Glass @ 2020-05-20 3:07 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Wed, 13 May 2020 at 14:14, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Currently when creating an U_BOOT_DEVICE entry a struct driver_info
> is declared, which contains the data needed to instantiate the device.
> However, the actual device is created at runtime and there is no proper
> way to get the device based on its struct driver_info.
>
> This patch extends struct driver_info adding a pointer to udevice which
> is populated during the bind process, allowing to generate a set of
> functions to get the device based on its struct driver_info.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> drivers/core/device.c | 25 +++++++++++++++++++++++--
> drivers/core/root.c | 6 +++++-
> include/dm/device-internal.h | 2 +-
> include/dm/device.h | 13 +++++++++++++
> include/dm/platdata.h | 6 ++++++
> 5 files changed, 48 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index 0157bb1fe0..2476f170a5 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -246,10 +246,11 @@ int device_bind_ofnode(struct udevice *parent, const struct driver *drv,
> }
>
> int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
> - const struct driver_info *info, struct udevice **devp)
> + struct driver_info *info, struct udevice **devp)
> {
> struct driver *drv;
> uint platdata_size = 0;
> + int ret = 0;
>
> drv = lists_driver_lookup_name(info->name);
> if (!drv)
> @@ -260,9 +261,19 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
> #if CONFIG_IS_ENABLED(OF_PLATDATA)
> platdata_size = info->platdata_size;
> #endif
> - return device_bind_common(parent, drv, info->name,
> + ret = device_bind_common(parent, drv, info->name,
> (void *)info->platdata, 0, ofnode_null(), platdata_size,
> devp);
> +
drop blank line
> + if (ret)
> + return ret;
> +
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> + info->dev = *devp;
> +#endif
> +
> + return ret;
> +
and here
> }
>
> static void *alloc_priv(int size, uint flags)
> @@ -727,6 +738,16 @@ int device_get_global_by_ofnode(ofnode ofnode, struct udevice **devp)
> return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
> }
>
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> +int device_get_by_driver_info(struct driver_info * info, struct udevice **devp)
> +{
> + struct udevice *dev;
> +
> + dev = info->dev;
blank line before return:-)
> + return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
> +}
> +#endif
> +
> int device_find_first_child(const struct udevice *parent, struct udevice **devp)
> {
> if (list_empty(&parent->child_head)) {
> diff --git a/drivers/core/root.c b/drivers/core/root.c
> index 14df16c280..8f47a6b356 100644
> --- a/drivers/core/root.c
> +++ b/drivers/core/root.c
> @@ -25,7 +25,7 @@
>
> DECLARE_GLOBAL_DATA_PTR;
>
> -static const struct driver_info root_info = {
> +static struct driver_info root_info = {
> .name = "root_driver",
> };
>
> @@ -346,6 +346,10 @@ int dm_init_and_scan(bool pre_reloc_only)
> {
> int ret;
>
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
This one can use if() I think
> + populate_phandle_data();
> +#endif
> +
> ret = dm_init(IS_ENABLED(CONFIG_OF_LIVE));
> if (ret) {
> debug("dm_init() failed: %d\n", ret);
> diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h
> index 294d6c1810..5145fb4e14 100644
> --- a/include/dm/device-internal.h
> +++ b/include/dm/device-internal.h
> @@ -81,7 +81,7 @@ int device_bind_with_driver_data(struct udevice *parent,
> * @return 0 if OK, -ve on error
> */
> int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
> - const struct driver_info *info, struct udevice **devp);
> + struct driver_info *info, struct udevice **devp);
That change should really be a separate patch I think.
>
> /**
> * device_ofdata_to_platdata() - Read platform data for a device
> diff --git a/include/dm/device.h b/include/dm/device.h
> index d5b3e732c3..590c1f99ce 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -537,6 +537,19 @@ int device_find_global_by_ofnode(ofnode node, struct udevice **devp);
> */
> int device_get_global_by_ofnode(ofnode node, struct udevice **devp);
>
> +/**
> + * device_get_by_driver_info() - Get a device based on driver_info
> + *
> + * Locates a device by its struct driver_info.
> + *
> + * The device is probed to activate it ready for use.
> + *
> + * @info: Struct driver_info
> + * @devp: Returns pointer to device if found, otherwise this is set to NULL
> + * @return 0 if OK, -ve on error
> + */
> +int device_get_by_driver_info(struct driver_info * info, struct udevice **devp);
> +
> /**
> * device_find_first_child() - Find the first child of a device
> *
> diff --git a/include/dm/platdata.h b/include/dm/platdata.h
> index c972fa6936..17eef7c7a3 100644
> --- a/include/dm/platdata.h
> +++ b/include/dm/platdata.h
> @@ -28,6 +28,7 @@ struct driver_info {
> const void *platdata;
> #if CONFIG_IS_ENABLED(OF_PLATDATA)
> uint platdata_size;
> + struct udevice *dev;
Please also update comment for this struct
> #endif
> };
>
> @@ -43,4 +44,9 @@ struct driver_info {
> #define U_BOOT_DEVICES(__name) \
> ll_entry_declare_list(struct driver_info, __name, driver_info)
>
> +/* Get a pointer to a given driver */
> +#define DM_GET_DEVICE(__name) \
> + ll_entry_get(struct driver_info, __name, driver_info)
> +
> +void populate_phandle_data(void);
Function comment. Also can this ever fail?
> #endif
> --
> 2.20.1
>
Regards,
SImon
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 0/6] improve OF_PLATDATA support
2020-05-13 20:13 [RFC 0/6] improve OF_PLATDATA support Walter Lozano
` (5 preceding siblings ...)
2020-05-13 20:13 ` [RFC 6/6] dtoc add test for cd-gpios Walter Lozano
@ 2020-05-20 3:07 ` Simon Glass
2020-05-21 13:15 ` Walter Lozano
6 siblings, 1 reply; 22+ messages in thread
From: Simon Glass @ 2020-05-20 3:07 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Wed, 13 May 2020 at 14:13, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> When using OF_PLATDATA dtbs are converted to C structs in order to save
> space as we can remove both dtbs and libraries from TPL/SPL binaries.
>
> This patchset tries to improve its support by overcoming some limitations
> in the current implementation
>
> First, the support for scan and check for valid driver/aliases is added
> in order to generate U_BOOT_DEVICE entries with valid driver names.
>
> Secondly, the way information about linked noded (phandle) is generated
> in C structs is improved in order to make it easier to get a device
> associated to its data.
>
> Lastly the the suport for the property cd-gpios is added, which is used to
> configure the card detection gpio on MMC is added.
>
> This implementation is based in discussion in [1] and [2]
>
> [1] https://patchwork.ozlabs.org/patch/1249198/
> [2] https://patchwork.ozlabs.org/project/uboot/list/?series=167495&state=*
>
> Walter Lozano (6):
> dtoc: add support to scan drivers
> core: extend struct driver_info to point to device
> dtoc: extend dtoc to use struct driver_info when linking nodes
> dtoc: update tests to match new platdata
> dtoc: update dtb_platdata to support cd-gpios
> dtoc add test for cd-gpios
>
> drivers/clk/clk-uclass.c | 8 +-
> drivers/core/device.c | 25 ++-
> drivers/core/root.c | 6 +-
> drivers/misc/irq-uclass.c | 4 +-
> drivers/mmc/ftsdc010_mci.c | 2 +-
> drivers/mmc/rockchip_dw_mmc.c | 2 +-
> drivers/mmc/rockchip_sdhci.c | 2 +-
> drivers/ram/rockchip/sdram_rk3399.c | 2 +-
> drivers/spi/rk_spi.c | 2 +-
> include/clk.h | 2 +-
> include/dm/device-internal.h | 2 +-
> include/dm/device.h | 19 +++
> include/dm/platdata.h | 6 +
> tools/dtoc/dtb_platdata.py | 83 +++++++--
> tools/dtoc/dtoc_test_phandle_cd_gpios.dts | 42 +++++
> tools/dtoc/test_dtoc.py | 197 +++++++++++++++++-----
> 16 files changed, 332 insertions(+), 72 deletions(-)
> create mode 100644 tools/dtoc/dtoc_test_phandle_cd_gpios.dts
>
> --
> 2.20.1
>
This looks really nice. I think you can take off the RFC. Also run
through patman/checkpatch.
Regards,
Simon
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 3/6] dtoc: extend dtoc to use struct driver_info when linking nodes
2020-05-13 20:13 ` [RFC 3/6] dtoc: extend dtoc to use struct driver_info when linking nodes Walter Lozano
@ 2020-05-20 3:07 ` Simon Glass
2020-05-21 13:16 ` Walter Lozano
0 siblings, 1 reply; 22+ messages in thread
From: Simon Glass @ 2020-05-20 3:07 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Wed, 13 May 2020 at 14:14, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> In the current implementation, when dtoc parses a dtb to generate a struct
> platdata it converts the information related to linked nodes as pointers
> to struct platdata of destination nodes. By doing this, it makes
> difficult to get pointer to udevices created based on these
> information.
>
> This patch extends dtoc to use struct driver_info when populating
> information about linked nodes, which makes it easier to later get
> the devices created. In this context, reimplement functions like
> clk_get_by_index_platdata() which made use of the previous approach.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> drivers/clk/clk-uclass.c | 8 +++-----
> drivers/misc/irq-uclass.c | 4 ++--
> drivers/mmc/ftsdc010_mci.c | 2 +-
> drivers/mmc/rockchip_dw_mmc.c | 2 +-
> drivers/mmc/rockchip_sdhci.c | 2 +-
> drivers/ram/rockchip/sdram_rk3399.c | 2 +-
> drivers/spi/rk_spi.c | 2 +-
> include/clk.h | 2 +-
> tools/dtoc/dtb_platdata.py | 17 ++++++++++++++---
> 9 files changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index 71878474eb..f24008fe00 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -25,14 +25,12 @@ static inline const struct clk_ops *clk_dev_ops(struct udevice *dev)
>
> #if CONFIG_IS_ENABLED(OF_CONTROL)
> # if CONFIG_IS_ENABLED(OF_PLATDATA)
> -int clk_get_by_index_platdata(struct udevice *dev, int index,
> - struct phandle_1_arg *cells, struct clk *clk)
> +int clk_get_by_driver_info(struct udevice *dev, struct phandle_1_arg *cells,
> + struct clk *clk)
> {
> int ret;
>
> - if (index != 0)
> - return -ENOSYS;
But it looks like you only support index 0?
> - ret = uclass_get_device(UCLASS_CLK, 0, &clk->dev);
> + ret = device_get_by_driver_info((struct driver_info*) cells[0].node, &clk->dev);
Or do you mean [index] here instead of [0] ?
> if (ret)
> return ret;
> clk->id = cells[0].arg[0];
> diff --git a/drivers/misc/irq-uclass.c b/drivers/misc/irq-uclass.c
> index 61aa10e465..8eaebe6419 100644
> --- a/drivers/misc/irq-uclass.c
> +++ b/drivers/misc/irq-uclass.c
> @@ -63,14 +63,14 @@ int irq_read_and_clear(struct irq *irq)
> }
>
> #if CONFIG_IS_ENABLED(OF_PLATDATA)
> -int irq_get_by_index_platdata(struct udevice *dev, int index,
> +int irq_get_by_driver_info(struct udevice *dev,
> struct phandle_1_arg *cells, struct irq *irq)
> {
> int ret;
>
> if (index != 0)
> return -ENOSYS;
> - ret = uclass_get_device(UCLASS_IRQ, 0, &irq->dev);
> + ret = device_get_by_driver_info(cells[0].node, &irq->dev);
> if (ret)
> return ret;
> irq->id = cells[0].arg[0];
> diff --git a/drivers/mmc/ftsdc010_mci.c b/drivers/mmc/ftsdc010_mci.c
> index 9c15eb36d6..efa92d48be 100644
> --- a/drivers/mmc/ftsdc010_mci.c
> +++ b/drivers/mmc/ftsdc010_mci.c
> @@ -437,7 +437,7 @@ static int ftsdc010_mmc_probe(struct udevice *dev)
> chip->priv = dev;
> chip->dev_index = 1;
> memcpy(priv->minmax, dtplat->clock_freq_min_max, sizeof(priv->minmax));
> - ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->clk);
> + ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->clk);
> if (ret < 0)
> return ret;
> #endif
> diff --git a/drivers/mmc/rockchip_dw_mmc.c b/drivers/mmc/rockchip_dw_mmc.c
> index a0e1be8794..7b4479543c 100644
> --- a/drivers/mmc/rockchip_dw_mmc.c
> +++ b/drivers/mmc/rockchip_dw_mmc.c
> @@ -120,7 +120,7 @@ static int rockchip_dwmmc_probe(struct udevice *dev)
> priv->minmax[0] = 400000; /* 400 kHz */
> priv->minmax[1] = dtplat->max_frequency;
>
> - ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->clk);
> + ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->clk);
> if (ret < 0)
> return ret;
> #else
> diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
> index b440996b26..b073f1a08d 100644
> --- a/drivers/mmc/rockchip_sdhci.c
> +++ b/drivers/mmc/rockchip_sdhci.c
> @@ -46,7 +46,7 @@ static int arasan_sdhci_probe(struct udevice *dev)
> host->name = dev->name;
> host->ioaddr = map_sysmem(dtplat->reg[0], dtplat->reg[1]);
> max_frequency = dtplat->max_frequency;
> - ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk);
> + ret = clk_get_by_driver_info(dev, dtplat->clocks, &clk);
> #else
> max_frequency = dev_read_u32_default(dev, "max-frequency", 0);
> ret = clk_get_by_index(dev, 0, &clk);
> diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c
> index d69ef01d08..87ec25f893 100644
> --- a/drivers/ram/rockchip/sdram_rk3399.c
> +++ b/drivers/ram/rockchip/sdram_rk3399.c
> @@ -3125,7 +3125,7 @@ static int rk3399_dmc_init(struct udevice *dev)
> priv->cic, priv->pmugrf, priv->pmusgrf, priv->pmucru, priv->pmu);
>
> #if CONFIG_IS_ENABLED(OF_PLATDATA)
> - ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->ddr_clk);
> + ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->ddr_clk);
> #else
> ret = clk_get_by_index(dev, 0, &priv->ddr_clk);
> #endif
> diff --git a/drivers/spi/rk_spi.c b/drivers/spi/rk_spi.c
> index 95eeb8307a..bd0337272e 100644
> --- a/drivers/spi/rk_spi.c
> +++ b/drivers/spi/rk_spi.c
> @@ -181,7 +181,7 @@ static int conv_of_platdata(struct udevice *dev)
>
> plat->base = dtplat->reg[0];
> plat->frequency = 20000000;
> - ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->clk);
> + ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->clk);
> if (ret < 0)
> return ret;
> dev->req_seq = 0;
> diff --git a/include/clk.h b/include/clk.h
> index c6a2713f62..3be379de03 100644
> --- a/include/clk.h
> +++ b/include/clk.h
> @@ -89,7 +89,7 @@ struct clk_bulk {
>
> #if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(CLK)
> struct phandle_1_arg;
> -int clk_get_by_index_platdata(struct udevice *dev, int index,
> +int clk_get_by_driver_info(struct udevice *dev,
> struct phandle_1_arg *cells, struct clk *clk);
>
> /**
> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
> index 3f899a8bac..d30e2af2ec 100644
> --- a/tools/dtoc/dtb_platdata.py
> +++ b/tools/dtoc/dtb_platdata.py
Can you put the dtoc part of this change in a separate patch?
> @@ -153,6 +153,7 @@ class DtbPlatdata(object):
> self._aliases = {}
> self._drivers = []
> self._driver_aliases = {}
> + self._links = []
>
> def get_normalized_compat_name(self, node):
> compat_c, aliases_c = get_compat_name(node)
> @@ -507,7 +508,7 @@ class DtbPlatdata(object):
> """
> struct_name, _ = self.get_normalized_compat_name(node)
> var_name = conv_name_to_c(node.name)
> - self.buf('static const struct %s%s %s%s = {\n' %
> + self.buf('static struct %s%s %s%s = {\n' %
> (STRUCT_PREFIX, struct_name, VAL_PREFIX, var_name))
> for pname in sorted(node.props):
> prop = node.props[pname]
> @@ -526,6 +527,7 @@ class DtbPlatdata(object):
> if info:
> # Process the list as pairs of (phandle, id)
> pos = 0
> + item = 0
> for args in info.args:
> phandle_cell = prop.value[pos]
> phandle = fdt_util.fdt32_to_cpu(phandle_cell)
> @@ -535,8 +537,10 @@ class DtbPlatdata(object):
> for i in range(args):
> arg_values.append(str(fdt_util.fdt32_to_cpu(prop.value[pos + 1 + i])))
> pos += 1 + args
> - vals.append('\t{&%s%s, {%s}}' % (VAL_PREFIX, name,
> - ', '.join(arg_values)))
> + vals.append('\t{NULL, {%s}}' % (', '.join(arg_values)))
> + phandle_entry = '%s%s.%s[%d].node = DM_GET_DEVICE(%s)' % (VAL_PREFIX, var_name, member_name, item, name)
> + self._links.append(phandle_entry)
> + item += 1
> for val in vals:
> self.buf('\n\t\t%s,' % val)
> else:
> @@ -559,6 +563,7 @@ class DtbPlatdata(object):
> self.buf('\t.name\t\t= "%s",\n' % struct_name)
> self.buf('\t.platdata\t= &%s%s,\n' % (VAL_PREFIX, var_name))
> self.buf('\t.platdata_size\t= sizeof(%s%s),\n' % (VAL_PREFIX, var_name))
> + self.buf('\t.dev\t\t= NULL,\n')
I suggest emitting a little comment here saying that it is filled in
by (function) at runtime.
> self.buf('};\n')
> self.buf('\n')
>
> @@ -592,6 +597,12 @@ class DtbPlatdata(object):
> self.output_node(node)
> nodes_to_output.remove(node)
>
> + self.buf('void populate_phandle_data(void) {\n')
> + for l in self._links:
> + self.buf(('\t%s;\n' % l))
> + self.buf('}\n')
Can you add a comment with an example line that is output? Or maybe
use a dict with a key and value for each piece (dmc_at_xxx.clocks[0]
as key and clock_controler_at_... as value) so you can have the string
formatting done here. It is a just a bit unclear what is being written
here.
> +
> + self.out(''.join(self.get_buf()))
>
> def run_steps(args, dtb_file, include_disabled, output):
> """Run all the steps of the dtoc tool
> --
> 2.20.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 4/6] dtoc: update tests to match new platdata
2020-05-13 20:13 ` [RFC 4/6] dtoc: update tests to match new platdata Walter Lozano
@ 2020-05-20 3:07 ` Simon Glass
2020-05-21 13:16 ` Walter Lozano
0 siblings, 1 reply; 22+ messages in thread
From: Simon Glass @ 2020-05-20 3:07 UTC (permalink / raw)
To: u-boot
On Wed, 13 May 2020 at 14:14, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> After using a new approach to link nodes when OF_PLATDATA is enabled
> the test cases need to be update.
>
> This patch updates the tests based on this new implementation.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> tools/dtoc/test_dtoc.py | 123 ++++++++++++++++++++++++++--------------
> 1 file changed, 81 insertions(+), 42 deletions(-)
>
Reviewed-by: Simon Glass <sjg@chromium.org>
BTW, setting .dev = NULL is not necessary. You could drop it perhaps?
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 5/6] dtoc: update dtb_platdata to support cd-gpios
2020-05-13 20:13 ` [RFC 5/6] dtoc: update dtb_platdata to support cd-gpios Walter Lozano
@ 2020-05-20 3:07 ` Simon Glass
0 siblings, 0 replies; 22+ messages in thread
From: Simon Glass @ 2020-05-20 3:07 UTC (permalink / raw)
To: u-boot
On Wed, 13 May 2020 at 14:14, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Currently dtoc does not support the property cd-gpios used to declare
> the gpios for card detect in mmc.
>
> This patch adds support to cd-gpios property.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> tools/dtoc/dtb_platdata.py | 13 ++++++++-----
> tools/dtoc/test_dtoc.py | 2 +-
> 2 files changed, 9 insertions(+), 6 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 6/6] dtoc add test for cd-gpios
2020-05-13 20:13 ` [RFC 6/6] dtoc add test for cd-gpios Walter Lozano
@ 2020-05-20 3:07 ` Simon Glass
2020-05-21 13:16 ` Walter Lozano
0 siblings, 1 reply; 22+ messages in thread
From: Simon Glass @ 2020-05-20 3:07 UTC (permalink / raw)
To: u-boot
On Wed, 13 May 2020 at 14:14, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Add a test for dtoc taking into account the cd-gpios property.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> tools/dtoc/dtoc_test_phandle_cd_gpios.dts | 42 +++++++++++++
> tools/dtoc/test_dtoc.py | 72 +++++++++++++++++++++++
> 2 files changed, 114 insertions(+)
> create mode 100644 tools/dtoc/dtoc_test_phandle_cd_gpios.dts
>
Reviewed-by: Simon Glass <sjg@chromium.org>
A few more things - please check sandbox_spl, and also updates the docs.
- SImon
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 0/6] improve OF_PLATDATA support
2020-05-20 3:07 ` [RFC 0/6] improve OF_PLATDATA support Simon Glass
@ 2020-05-21 13:15 ` Walter Lozano
0 siblings, 0 replies; 22+ messages in thread
From: Walter Lozano @ 2020-05-21 13:15 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 20/5/20 00:07, Simon Glass wrote:
> Hi Walter,
>
> On Wed, 13 May 2020 at 14:13, Walter Lozano<walter.lozano@collabora.com> wrote:
>> When using OF_PLATDATA dtbs are converted to C structs in order to save
>> space as we can remove both dtbs and libraries from TPL/SPL binaries.
>>
>> This patchset tries to improve its support by overcoming some limitations
>> in the current implementation
>>
>> First, the support for scan and check for valid driver/aliases is added
>> in order to generate U_BOOT_DEVICE entries with valid driver names.
>>
>> Secondly, the way information about linked noded (phandle) is generated
>> in C structs is improved in order to make it easier to get a device
>> associated to its data.
>>
>> Lastly the the suport for the property cd-gpios is added, which is used to
>> configure the card detection gpio on MMC is added.
>>
>> This implementation is based in discussion in [1] and [2]
>>
>> [1]https://patchwork.ozlabs.org/patch/1249198/
>> [2]https://patchwork.ozlabs.org/project/uboot/list/?series=167495&state=*
>>
>> Walter Lozano (6):
>> dtoc: add support to scan drivers
>> core: extend struct driver_info to point to device
>> dtoc: extend dtoc to use struct driver_info when linking nodes
>> dtoc: update tests to match new platdata
>> dtoc: update dtb_platdata to support cd-gpios
>> dtoc add test for cd-gpios
>>
>> drivers/clk/clk-uclass.c | 8 +-
>> drivers/core/device.c | 25 ++-
>> drivers/core/root.c | 6 +-
>> drivers/misc/irq-uclass.c | 4 +-
>> drivers/mmc/ftsdc010_mci.c | 2 +-
>> drivers/mmc/rockchip_dw_mmc.c | 2 +-
>> drivers/mmc/rockchip_sdhci.c | 2 +-
>> drivers/ram/rockchip/sdram_rk3399.c | 2 +-
>> drivers/spi/rk_spi.c | 2 +-
>> include/clk.h | 2 +-
>> include/dm/device-internal.h | 2 +-
>> include/dm/device.h | 19 +++
>> include/dm/platdata.h | 6 +
>> tools/dtoc/dtb_platdata.py | 83 +++++++--
>> tools/dtoc/dtoc_test_phandle_cd_gpios.dts | 42 +++++
>> tools/dtoc/test_dtoc.py | 197 +++++++++++++++++-----
>> 16 files changed, 332 insertions(+), 72 deletions(-)
>> create mode 100644 tools/dtoc/dtoc_test_phandle_cd_gpios.dts
>>
>> --
>> 2.20.1
>>
> This looks really nice. I think you can take off the RFC. Also run
> through patman/checkpatch.
>
Thanks for the deep review and comments.
I thought initially to send a kind of quick RFC in order to check if we
agree in the general idea, and polish the strange things that this
series might introduce, as it will change some things I'm not able to
test. I hope we can come to a clean patch series soon.
Thanks,
Walter
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 1/6] dtoc: add support to scan drivers
2020-05-20 3:07 ` Simon Glass
@ 2020-05-21 13:15 ` Walter Lozano
2020-05-22 23:13 ` Simon Glass
0 siblings, 1 reply; 22+ messages in thread
From: Walter Lozano @ 2020-05-21 13:15 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 20/5/20 00:07, Simon Glass wrote:
> Hi Walter,
>
> On Wed, 13 May 2020 at 14:13, Walter Lozano<walter.lozano@collabora.com> wrote:
>> Currently dtoc scans dtbs to convert them to struct platdata and
>> to generate U_BOOT_DEVICE entries. These entries need to be filled
>> with the driver name, but at this moment the information used is the
>> compatible name present in the dtb. This causes that only nodes with
>> a compatible name that matches a driver name generate a working
>> entry.
>>
>> In order to improve this behaviour, this patch adds to dtoc the
>> capability of scan drivers source code to generate a list of valid driver
>> names. This allows to rise a warning in the case that an U_BOOT_DEVICE
>> entry will try to use a name not valid.
>>
>> Additionally, in order to add more flexibility to the solution, adds the
>> U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but allows an
>> easy way to declare driver name aliases. Thanks to this, dtoc can look
>> for the driver name based on its alias when it populates the U_BOOT_DEVICE entry.
>>
>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com>
>> ---
>> include/dm/device.h | 6 +++++
>> tools/dtoc/dtb_platdata.py | 53 +++++++++++++++++++++++++++++++++++---
>> 2 files changed, 55 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/dm/device.h b/include/dm/device.h
>> index 975eec5d0e..d5b3e732c3 100644
>> --- a/include/dm/device.h
>> +++ b/include/dm/device.h
>> @@ -282,6 +282,12 @@ struct driver {
>> #define DM_GET_DRIVER(__name) \
>> ll_entry_get(struct driver, __name, driver)
>>
>> +/* Declare a macro to state a alias for a driver name. This macro will
> /*
> * Declare ...
>
Sure.
>> + * produce no code but its information will be parsed by tools like
>> + * dtoc
>> + */
>> +#define U_BOOT_DRIVER_ALIAS(__name, __alias)
>> +
>> /**
>> * dev_get_platdata() - Get the platform data for a device
>> *
>> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
>> index ecfe0624d1..3f899a8bac 100644
>> --- a/tools/dtoc/dtb_platdata.py
>> +++ b/tools/dtoc/dtb_platdata.py
>> @@ -14,6 +14,8 @@ static data.
>> import collections
>> import copy
>> import sys
>> +import os
>> +import re
> please keep sorted
Sure
>> from dtoc import fdt
>> from dtoc import fdt_util
>> @@ -149,6 +151,20 @@ class DtbPlatdata(object):
>> self._outfile = None
>> self._lines = []
>> self._aliases = {}
>> + self._drivers = []
>> + self._driver_aliases = {}
> Please update comment above for what these new things are for.
OK.
>> +
>> + def get_normalized_compat_name(self, node):
> Needs function comment (also for those below).
Sure, no problem.
>> + compat_c, aliases_c = get_compat_name(node)
>> + if compat_c not in self._drivers:
>> + try: # pragma: no cover
> Instead of an exception can you use .get() and check for None?
Sure.
>> + compat_c_old = compat_c
>> + compat_c = self._driver_aliases[compat_c]
>> + aliases_c = [compat_c_old] + aliases
>> + except:
>> + print('WARNING: the driver %s was not found in the driver list' % (compat_c))
This is something I would like to check with you. As you can see, a
warning is raised when a driver name is not found, which I think is the
right thing to do at this point. However, this warning will be trigger
but almost all the tests as the drivers names are no valid (there is no
U_BOOT_DRIVER). How do you think is best to fix this?
I see different possibilities here
- To add specific driver names to self._drivers when running a test
- To change driver names on tests to match a valid one
Maybe you have a more clean way to deal with this.
>> +
>> + return compat_c, aliases_c
>>
>> def setup_output(self, fname):
>> """Set up the output destination
>> @@ -243,6 +259,34 @@ class DtbPlatdata(object):
>> return PhandleInfo(max_args, args)
>> return None
>>
>> + def scan_driver(self, fn):
>> + f = open(fn)
> Can you avoid single-char var names? Also how about either using
> tools.ReadFile() or:
>
> with open(fname) as fd:
> ...rest of function
Agree.
>> +
>> + b = f.read()
>> +
>> + drivers = re.findall('U_BOOT_DRIVER\((.*)\)', b)
>> +
>> + for d in drivers:
>> + self._drivers.append(d)
>> +
>> + driver_aliases = re.findall('U_BOOT_DRIVER_ALIAS\(\s*(\w+)\s*,\s*(\w+)\s*\)', b)
> Can you add a comment before this with an example of the line you are
> parsing? It helps make sense of the regex.
No problem.
>> +
>> + for a in driver_aliases: # pragma: no cover
>> + try:
>> + self._driver_aliases[a[1]] = a[0]
>> + except:
>> + pass
> Again please avoid exceptions.
No problem at all.
>> +
>> + def scan_drivers(self):
>> + """Scan the driver folders to build a list of driver names and possible
>> + aliases
>> + """
>> + for (dirpath, dirnames, filenames) in os.walk('./'):
>> + for fn in filenames:
>> + if not fn.endswith('.c'):
>> + continue
>> + self.scan_driver(dirpath + '/' + fn)
>> +
>> def scan_dtb(self):
>> """Scan the device tree to obtain a tree of nodes and properties
>>
>> @@ -353,7 +397,7 @@ class DtbPlatdata(object):
>> """
>> structs = {}
>> for node in self._valid_nodes:
>> - node_name, _ = get_compat_name(node)
>> + node_name, _ = self.get_normalized_compat_name(node)
>> fields = {}
> I wonder how long this scan takes? Should we cache the results somewhere?
Probably you are worried because this task will be repeated every time
we compile U-boot, and it could be a potential issue when testing
several different configs. However, at least in my setup the scan takes
less than 1 sec, which I think is promising. Also I think that this scan
is only perform for those configs which enable OF_PLATDATA, which are
15, should not cause much overhead.
What do you think?
>> # Get a list of all the valid properties in this node.
>> @@ -377,14 +421,14 @@ class DtbPlatdata(object):
>>
>> upto = 0
>> for node in self._valid_nodes:
>> - node_name, _ = get_compat_name(node)
>> + node_name, _ = self.get_normalized_compat_name(node)
>> struct = structs[node_name]
>> for name, prop in node.props.items():
>> if name not in PROP_IGNORE_LIST and name[0] != '#':
>> prop.Widen(struct[name])
>> upto += 1
>>
>> - struct_name, aliases = get_compat_name(node)
>> + struct_name, aliases = self.get_normalized_compat_name(node)
>> for alias in aliases:
>> self._aliases[alias] = struct_name
>>
>> @@ -461,7 +505,7 @@ class DtbPlatdata(object):
>> Args:
>> node: node to output
>> """
>> - struct_name, _ = get_compat_name(node)
>> + struct_name, _ = self.get_normalized_compat_name(node)
>> var_name = conv_name_to_c(node.name)
>> self.buf('static const struct %s%s %s%s = {\n' %
>> (STRUCT_PREFIX, struct_name, VAL_PREFIX, var_name))
>> @@ -562,6 +606,7 @@ def run_steps(args, dtb_file, include_disabled, output):
>> raise ValueError('Please specify a command: struct, platdata')
>>
>> plat = DtbPlatdata(dtb_file, include_disabled)
>> + plat.scan_drivers()
>> plat.scan_dtb()
>> plat.scan_tree()
>> plat.scan_reg_sizes()
>> --
>> 2.20.1
>>
Regards,
Walter
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 2/6] core: extend struct driver_info to point to device
2020-05-20 3:07 ` Simon Glass
@ 2020-05-21 13:15 ` Walter Lozano
2020-05-22 23:13 ` Simon Glass
0 siblings, 1 reply; 22+ messages in thread
From: Walter Lozano @ 2020-05-21 13:15 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 20/5/20 00:07, Simon Glass wrote:
> Hi Walter,
>
> On Wed, 13 May 2020 at 14:14, Walter Lozano<walter.lozano@collabora.com> wrote:
>> Currently when creating an U_BOOT_DEVICE entry a struct driver_info
>> is declared, which contains the data needed to instantiate the device.
>> However, the actual device is created at runtime and there is no proper
>> way to get the device based on its struct driver_info.
>>
>> This patch extends struct driver_info adding a pointer to udevice which
>> is populated during the bind process, allowing to generate a set of
>> functions to get the device based on its struct driver_info.
>>
>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com>
>> ---
>> drivers/core/device.c | 25 +++++++++++++++++++++++--
>> drivers/core/root.c | 6 +++++-
>> include/dm/device-internal.h | 2 +-
>> include/dm/device.h | 13 +++++++++++++
>> include/dm/platdata.h | 6 ++++++
>> 5 files changed, 48 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>> index 0157bb1fe0..2476f170a5 100644
>> --- a/drivers/core/device.c
>> +++ b/drivers/core/device.c
>> @@ -246,10 +246,11 @@ int device_bind_ofnode(struct udevice *parent, const struct driver *drv,
>> }
>>
>> int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
>> - const struct driver_info *info, struct udevice **devp)
>> + struct driver_info *info, struct udevice **devp)
>> {
>> struct driver *drv;
>> uint platdata_size = 0;
>> + int ret = 0;
>>
>> drv = lists_driver_lookup_name(info->name);
>> if (!drv)
>> @@ -260,9 +261,19 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
>> #if CONFIG_IS_ENABLED(OF_PLATDATA)
>> platdata_size = info->platdata_size;
>> #endif
>> - return device_bind_common(parent, drv, info->name,
>> + ret = device_bind_common(parent, drv, info->name,
>> (void *)info->platdata, 0, ofnode_null(), platdata_size,
>> devp);
>> +
> drop blank line
Sure.
>> + if (ret)
>> + return ret;
>> +
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> + info->dev = *devp;
>> +#endif
>> +
>> + return ret;
>> +
> and here
Sure.
>> }
>>
>> static void *alloc_priv(int size, uint flags)
>> @@ -727,6 +738,16 @@ int device_get_global_by_ofnode(ofnode ofnode, struct udevice **devp)
>> return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
>> }
>>
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> +int device_get_by_driver_info(struct driver_info * info, struct udevice **devp)
>> +{
>> + struct udevice *dev;
>> +
>> + dev = info->dev;
> blank line before return:-)
Sure, thanks!
>> + return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
>> +}
>> +#endif
>> +
>> int device_find_first_child(const struct udevice *parent, struct udevice **devp)
>> {
>> if (list_empty(&parent->child_head)) {
>> diff --git a/drivers/core/root.c b/drivers/core/root.c
>> index 14df16c280..8f47a6b356 100644
>> --- a/drivers/core/root.c
>> +++ b/drivers/core/root.c
>> @@ -25,7 +25,7 @@
>>
>> DECLARE_GLOBAL_DATA_PTR;
>>
>> -static const struct driver_info root_info = {
>> +static struct driver_info root_info = {
>> .name = "root_driver",
>> };
>>
>> @@ -346,6 +346,10 @@ int dm_init_and_scan(bool pre_reloc_only)
>> {
>> int ret;
>>
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> This one can use if() I think
I think that as populate_phandle_data() is only defined when OF_PLATDATA
is enabled (as it is created by dtoc), this will trigger and link error
on those board which don't use this config. Is my understanding correct?
>> + populate_phandle_data();
>> +#endif
>> +
>> ret = dm_init(IS_ENABLED(CONFIG_OF_LIVE));
>> if (ret) {
>> debug("dm_init() failed: %d\n", ret);
>> diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h
>> index 294d6c1810..5145fb4e14 100644
>> --- a/include/dm/device-internal.h
>> +++ b/include/dm/device-internal.h
>> @@ -81,7 +81,7 @@ int device_bind_with_driver_data(struct udevice *parent,
>> * @return 0 if OK, -ve on error
>> */
>> int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
>> - const struct driver_info *info, struct udevice **devp);
>> + struct driver_info *info, struct udevice **devp);
> That change should really be a separate patch I think.
If we move this change to a separate patch, this will fail to compile as
struct driver_info needs to be updated by device_bind_by_name to fill
info->dev. I can move both things to a different patch, in that case.
Please advice.
>> /**
>> * device_ofdata_to_platdata() - Read platform data for a device
>> diff --git a/include/dm/device.h b/include/dm/device.h
>> index d5b3e732c3..590c1f99ce 100644
>> --- a/include/dm/device.h
>> +++ b/include/dm/device.h
>> @@ -537,6 +537,19 @@ int device_find_global_by_ofnode(ofnode node, struct udevice **devp);
>> */
>> int device_get_global_by_ofnode(ofnode node, struct udevice **devp);
>>
>> +/**
>> + * device_get_by_driver_info() - Get a device based on driver_info
>> + *
>> + * Locates a device by its struct driver_info.
>> + *
>> + * The device is probed to activate it ready for use.
>> + *
>> + * @info: Struct driver_info
>> + * @devp: Returns pointer to device if found, otherwise this is set to NULL
>> + * @return 0 if OK, -ve on error
>> + */
>> +int device_get_by_driver_info(struct driver_info * info, struct udevice **devp);
>> +
>> /**
>> * device_find_first_child() - Find the first child of a device
>> *
>> diff --git a/include/dm/platdata.h b/include/dm/platdata.h
>> index c972fa6936..17eef7c7a3 100644
>> --- a/include/dm/platdata.h
>> +++ b/include/dm/platdata.h
>> @@ -28,6 +28,7 @@ struct driver_info {
>> const void *platdata;
>> #if CONFIG_IS_ENABLED(OF_PLATDATA)
>> uint platdata_size;
>> + struct udevice *dev;
> Please also update comment for this struct
Sure
>> #endif
>> };
>>
>> @@ -43,4 +44,9 @@ struct driver_info {
>> #define U_BOOT_DEVICES(__name) \
>> ll_entry_declare_list(struct driver_info, __name, driver_info)
>>
>> +/* Get a pointer to a given driver */
>> +#define DM_GET_DEVICE(__name) \
>> + ll_entry_get(struct driver_info, __name, driver_info)
>> +
>> +void populate_phandle_data(void);
> Function comment. Also can this ever fail?
It shouldn't. It basically does something like this
void populate_phandle_data(void) {
dtv_dmc_at_20020000.clocks[0].node = DM_GET_DEVICE(clock_controller_at_20000000);
dtv_dmc_at_20020000.clocks[1].node = DM_GET_DEVICE(clock_controller_at_20000000);
dtv_serial_at_20064000.clocks[0].node = DM_GET_DEVICE(clock_controller_at_20000000);
dtv_serial_at_20064000.clocks[1].node = DM_GET_DEVICE(clock_controller_at_20000000);
}
as we discuss previously, it is all about to be able to use DM_GET_DEVICE.
>> #endif
>> --
>> 2.20.1
>>
Regards,
Walter
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 3/6] dtoc: extend dtoc to use struct driver_info when linking nodes
2020-05-20 3:07 ` Simon Glass
@ 2020-05-21 13:16 ` Walter Lozano
0 siblings, 0 replies; 22+ messages in thread
From: Walter Lozano @ 2020-05-21 13:16 UTC (permalink / raw)
To: u-boot
On 20/5/20 00:07, Simon Glass wrote:
> Hi Walter,
>
> On Wed, 13 May 2020 at 14:14, Walter Lozano<walter.lozano@collabora.com> wrote:
>> In the current implementation, when dtoc parses a dtb to generate a struct
>> platdata it converts the information related to linked nodes as pointers
>> to struct platdata of destination nodes. By doing this, it makes
>> difficult to get pointer to udevices created based on these
>> information.
>>
>> This patch extends dtoc to use struct driver_info when populating
>> information about linked nodes, which makes it easier to later get
>> the devices created. In this context, reimplement functions like
>> clk_get_by_index_platdata() which made use of the previous approach.
>>
>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com>
>> ---
>> drivers/clk/clk-uclass.c | 8 +++-----
>> drivers/misc/irq-uclass.c | 4 ++--
>> drivers/mmc/ftsdc010_mci.c | 2 +-
>> drivers/mmc/rockchip_dw_mmc.c | 2 +-
>> drivers/mmc/rockchip_sdhci.c | 2 +-
>> drivers/ram/rockchip/sdram_rk3399.c | 2 +-
>> drivers/spi/rk_spi.c | 2 +-
>> include/clk.h | 2 +-
>> tools/dtoc/dtb_platdata.py | 17 ++++++++++++++---
>> 9 files changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
>> index 71878474eb..f24008fe00 100644
>> --- a/drivers/clk/clk-uclass.c
>> +++ b/drivers/clk/clk-uclass.c
>> @@ -25,14 +25,12 @@ static inline const struct clk_ops *clk_dev_ops(struct udevice *dev)
>>
>> #if CONFIG_IS_ENABLED(OF_CONTROL)
>> # if CONFIG_IS_ENABLED(OF_PLATDATA)
>> -int clk_get_by_index_platdata(struct udevice *dev, int index,
>> - struct phandle_1_arg *cells, struct clk *clk)
>> +int clk_get_by_driver_info(struct udevice *dev, struct phandle_1_arg *cells,
>> + struct clk *clk)
>> {
>> int ret;
>>
>> - if (index != 0)
>> - return -ENOSYS;
> But it looks like you only support index 0?
>
>> - ret = uclass_get_device(UCLASS_CLK, 0, &clk->dev);
>> + ret = device_get_by_driver_info((struct driver_info*) cells[0].node, &clk->dev);
> Or do you mean [index] here instead of [0] ?
In my understanding, this new function clk_get_by_driver_info() which
somehow replaces clk_get_by_index_platdata() doesn't require an index at
all. The function device_get_by_driver_info will return the device
associated with cells->node, which basically is a struct driver_info
which holds a struct udevice *dev.
In the case that cells contains more than one struct phandle_1_arg, this
function should be called as many times as required to fill the proper
struct udevice dev.
>> if (ret)
>> return ret;
>> clk->id = cells[0].arg[0];
>> diff --git a/drivers/misc/irq-uclass.c b/drivers/misc/irq-uclass.c
>> index 61aa10e465..8eaebe6419 100644
>> --- a/drivers/misc/irq-uclass.c
>> +++ b/drivers/misc/irq-uclass.c
>> @@ -63,14 +63,14 @@ int irq_read_and_clear(struct irq *irq)
>> }
>>
>> #if CONFIG_IS_ENABLED(OF_PLATDATA)
>> -int irq_get_by_index_platdata(struct udevice *dev, int index,
>> +int irq_get_by_driver_info(struct udevice *dev,
>> struct phandle_1_arg *cells, struct irq *irq)
>> {
>> int ret;
>>
>> if (index != 0)
>> return -ENOSYS;
>> - ret = uclass_get_device(UCLASS_IRQ, 0, &irq->dev);
>> + ret = device_get_by_driver_info(cells[0].node, &irq->dev);
>> if (ret)
>> return ret;
>> irq->id = cells[0].arg[0];
>> diff --git a/drivers/mmc/ftsdc010_mci.c b/drivers/mmc/ftsdc010_mci.c
>> index 9c15eb36d6..efa92d48be 100644
>> --- a/drivers/mmc/ftsdc010_mci.c
>> +++ b/drivers/mmc/ftsdc010_mci.c
>> @@ -437,7 +437,7 @@ static int ftsdc010_mmc_probe(struct udevice *dev)
>> chip->priv = dev;
>> chip->dev_index = 1;
>> memcpy(priv->minmax, dtplat->clock_freq_min_max, sizeof(priv->minmax));
>> - ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->clk);
>> + ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->clk);
>> if (ret < 0)
>> return ret;
>> #endif
>> diff --git a/drivers/mmc/rockchip_dw_mmc.c b/drivers/mmc/rockchip_dw_mmc.c
>> index a0e1be8794..7b4479543c 100644
>> --- a/drivers/mmc/rockchip_dw_mmc.c
>> +++ b/drivers/mmc/rockchip_dw_mmc.c
>> @@ -120,7 +120,7 @@ static int rockchip_dwmmc_probe(struct udevice *dev)
>> priv->minmax[0] = 400000; /* 400 kHz */
>> priv->minmax[1] = dtplat->max_frequency;
>>
>> - ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->clk);
>> + ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->clk);
>> if (ret < 0)
>> return ret;
>> #else
>> diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
>> index b440996b26..b073f1a08d 100644
>> --- a/drivers/mmc/rockchip_sdhci.c
>> +++ b/drivers/mmc/rockchip_sdhci.c
>> @@ -46,7 +46,7 @@ static int arasan_sdhci_probe(struct udevice *dev)
>> host->name = dev->name;
>> host->ioaddr = map_sysmem(dtplat->reg[0], dtplat->reg[1]);
>> max_frequency = dtplat->max_frequency;
>> - ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk);
>> + ret = clk_get_by_driver_info(dev, dtplat->clocks, &clk);
>> #else
>> max_frequency = dev_read_u32_default(dev, "max-frequency", 0);
>> ret = clk_get_by_index(dev, 0, &clk);
>> diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c
>> index d69ef01d08..87ec25f893 100644
>> --- a/drivers/ram/rockchip/sdram_rk3399.c
>> +++ b/drivers/ram/rockchip/sdram_rk3399.c
>> @@ -3125,7 +3125,7 @@ static int rk3399_dmc_init(struct udevice *dev)
>> priv->cic, priv->pmugrf, priv->pmusgrf, priv->pmucru, priv->pmu);
>>
>> #if CONFIG_IS_ENABLED(OF_PLATDATA)
>> - ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->ddr_clk);
>> + ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->ddr_clk);
>> #else
>> ret = clk_get_by_index(dev, 0, &priv->ddr_clk);
>> #endif
>> diff --git a/drivers/spi/rk_spi.c b/drivers/spi/rk_spi.c
>> index 95eeb8307a..bd0337272e 100644
>> --- a/drivers/spi/rk_spi.c
>> +++ b/drivers/spi/rk_spi.c
>> @@ -181,7 +181,7 @@ static int conv_of_platdata(struct udevice *dev)
>>
>> plat->base = dtplat->reg[0];
>> plat->frequency = 20000000;
>> - ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->clk);
>> + ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->clk);
>> if (ret < 0)
>> return ret;
>> dev->req_seq = 0;
>> diff --git a/include/clk.h b/include/clk.h
>> index c6a2713f62..3be379de03 100644
>> --- a/include/clk.h
>> +++ b/include/clk.h
>> @@ -89,7 +89,7 @@ struct clk_bulk {
>>
>> #if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(CLK)
>> struct phandle_1_arg;
>> -int clk_get_by_index_platdata(struct udevice *dev, int index,
>> +int clk_get_by_driver_info(struct udevice *dev,
>> struct phandle_1_arg *cells, struct clk *clk);
>>
>> /**
>> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
>> index 3f899a8bac..d30e2af2ec 100644
>> --- a/tools/dtoc/dtb_platdata.py
>> +++ b/tools/dtoc/dtb_platdata.py
> Can you put the dtoc part of this change in a separate patch?
The issue is that it will cause an error at run time as the struct
driver_info will not have the correct values for struct udevice *dev,
which is populated by populate_phandle_data().
>> @@ -153,6 +153,7 @@ class DtbPlatdata(object):
>> self._aliases = {}
>> self._drivers = []
>> self._driver_aliases = {}
>> + self._links = []
>>
>> def get_normalized_compat_name(self, node):
>> compat_c, aliases_c = get_compat_name(node)
>> @@ -507,7 +508,7 @@ class DtbPlatdata(object):
>> """
>> struct_name, _ = self.get_normalized_compat_name(node)
>> var_name = conv_name_to_c(node.name)
>> - self.buf('static const struct %s%s %s%s = {\n' %
>> + self.buf('static struct %s%s %s%s = {\n' %
>> (STRUCT_PREFIX, struct_name, VAL_PREFIX, var_name))
>> for pname in sorted(node.props):
>> prop = node.props[pname]
>> @@ -526,6 +527,7 @@ class DtbPlatdata(object):
>> if info:
>> # Process the list as pairs of (phandle, id)
>> pos = 0
>> + item = 0
>> for args in info.args:
>> phandle_cell = prop.value[pos]
>> phandle = fdt_util.fdt32_to_cpu(phandle_cell)
>> @@ -535,8 +537,10 @@ class DtbPlatdata(object):
>> for i in range(args):
>> arg_values.append(str(fdt_util.fdt32_to_cpu(prop.value[pos + 1 + i])))
>> pos += 1 + args
>> - vals.append('\t{&%s%s, {%s}}' % (VAL_PREFIX, name,
>> - ', '.join(arg_values)))
>> + vals.append('\t{NULL, {%s}}' % (', '.join(arg_values)))
>> + phandle_entry = '%s%s.%s[%d].node = DM_GET_DEVICE(%s)' % (VAL_PREFIX, var_name, member_name, item, name)
>> + self._links.append(phandle_entry)
>> + item += 1
>> for val in vals:
>> self.buf('\n\t\t%s,' % val)
>> else:
>> @@ -559,6 +563,7 @@ class DtbPlatdata(object):
>> self.buf('\t.name\t\t= "%s",\n' % struct_name)
>> self.buf('\t.platdata\t= &%s%s,\n' % (VAL_PREFIX, var_name))
>> self.buf('\t.platdata_size\t= sizeof(%s%s),\n' % (VAL_PREFIX, var_name))
>> + self.buf('\t.dev\t\t= NULL,\n')
> I suggest emitting a little comment here saying that it is filled in
> by (function) at runtime.
Sure.
>> self.buf('};\n')
>> self.buf('\n')
>>
>> @@ -592,6 +597,12 @@ class DtbPlatdata(object):
>> self.output_node(node)
>> nodes_to_output.remove(node)
>>
>> + self.buf('void populate_phandle_data(void) {\n')
>> + for l in self._links:
>> + self.buf(('\t%s;\n' % l))
>> + self.buf('}\n')
> Can you add a comment with an example line that is output? Or maybe
> use a dict with a key and value for each piece (dmc_at_xxx.clocks[0]
> as key and clock_controler_at_... as value) so you can have the string
> formatting done here. It is a just a bit unclear what is being written
> here.
Yes, I totally agree, there should be a more clear way to do this. I was
thinking about it for a while and I initially thought that doing all the
expansions in one place will make it easier to read but I had many doubts.
Thanks for pointing it.
>> +
>> + self.out(''.join(self.get_buf()))
>>
>> def run_steps(args, dtb_file, include_disabled, output):
>> """Run all the steps of the dtoc tool
>> --
>> 2.20.1
>>
Regards,
Walter
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 4/6] dtoc: update tests to match new platdata
2020-05-20 3:07 ` Simon Glass
@ 2020-05-21 13:16 ` Walter Lozano
0 siblings, 0 replies; 22+ messages in thread
From: Walter Lozano @ 2020-05-21 13:16 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 20/5/20 00:07, Simon Glass wrote:
> On Wed, 13 May 2020 at 14:14, Walter Lozano<walter.lozano@collabora.com> wrote:
>> After using a new approach to link nodes when OF_PLATDATA is enabled
>> the test cases need to be update.
>>
>> This patch updates the tests based on this new implementation.
>>
>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com>
>> ---
>> tools/dtoc/test_dtoc.py | 123 ++++++++++++++++++++++++++--------------
>> 1 file changed, 81 insertions(+), 42 deletions(-)
>>
> Reviewed-by: Simon Glass<sjg@chromium.org>
>
> BTW, setting .dev = NULL is not necessary. You could drop it perhaps?
Sure, thanks!
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 6/6] dtoc add test for cd-gpios
2020-05-20 3:07 ` Simon Glass
@ 2020-05-21 13:16 ` Walter Lozano
0 siblings, 0 replies; 22+ messages in thread
From: Walter Lozano @ 2020-05-21 13:16 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 20/5/20 00:07, Simon Glass wrote:
> On Wed, 13 May 2020 at 14:14, Walter Lozano<walter.lozano@collabora.com> wrote:
>> Add a test for dtoc taking into account the cd-gpios property.
>>
>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com>
>> ---
>> tools/dtoc/dtoc_test_phandle_cd_gpios.dts | 42 +++++++++++++
>> tools/dtoc/test_dtoc.py | 72 +++++++++++++++++++++++
>> 2 files changed, 114 insertions(+)
>> create mode 100644 tools/dtoc/dtoc_test_phandle_cd_gpios.dts
>>
> Reviewed-by: Simon Glass<sjg@chromium.org>
>
> A few more things - please check sandbox_spl, and also updates the docs.
>
>
Sure. Thanks for all the time you spent reviewing my work.
Regards,
Walter
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 1/6] dtoc: add support to scan drivers
2020-05-21 13:15 ` Walter Lozano
@ 2020-05-22 23:13 ` Simon Glass
0 siblings, 0 replies; 22+ messages in thread
From: Simon Glass @ 2020-05-22 23:13 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Thu, 21 May 2020 at 07:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Hi Simon,
>
> On 20/5/20 00:07, Simon Glass wrote:
> > Hi Walter,
> >
> > On Wed, 13 May 2020 at 14:13, Walter Lozano<walter.lozano@collabora.com> wrote:
> >> Currently dtoc scans dtbs to convert them to struct platdata and
> >> to generate U_BOOT_DEVICE entries. These entries need to be filled
> >> with the driver name, but at this moment the information used is the
> >> compatible name present in the dtb. This causes that only nodes with
> >> a compatible name that matches a driver name generate a working
> >> entry.
> >>
> >> In order to improve this behaviour, this patch adds to dtoc the
> >> capability of scan drivers source code to generate a list of valid driver
> >> names. This allows to rise a warning in the case that an U_BOOT_DEVICE
> >> entry will try to use a name not valid.
> >>
> >> Additionally, in order to add more flexibility to the solution, adds the
> >> U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but allows an
> >> easy way to declare driver name aliases. Thanks to this, dtoc can look
> >> for the driver name based on its alias when it populates the U_BOOT_DEVICE entry.
> >>
> >> Signed-off-by: Walter Lozano<walter.lozano@collabora.com>
> >> ---
> >> include/dm/device.h | 6 +++++
> >> tools/dtoc/dtb_platdata.py | 53 +++++++++++++++++++++++++++++++++++---
> >> 2 files changed, 55 insertions(+), 4 deletions(-)
> >>
[..]
> >> +
> >> + def get_normalized_compat_name(self, node):
> > Needs function comment (also for those below).
> Sure, no problem.
> >> + compat_c, aliases_c = get_compat_name(node)
> >> + if compat_c not in self._drivers:
> >> + try: # pragma: no cover
> > Instead of an exception can you use .get() and check for None?
> Sure.
> >> + compat_c_old = compat_c
> >> + compat_c = self._driver_aliases[compat_c]
> >> + aliases_c = [compat_c_old] + aliases
> >> + except:
> >> + print('WARNING: the driver %s was not found in the driver list' % (compat_c))
>
> This is something I would like to check with you. As you can see, a
> warning is raised when a driver name is not found, which I think is the
> right thing to do at this point. However, this warning will be trigger
> but almost all the tests as the drivers names are no valid (there is no
> U_BOOT_DRIVER). How do you think is best to fix this?
>
> I see different possibilities here
>
> - To add specific driver names to self._drivers when running a test
>
> - To change driver names on tests to match a valid one
>
> Maybe you have a more clean way to deal with this.
You could add a new method 'set_test_mode()' dtb_platdata.py
You could add a flag to dtoc to disable the warning.
You could capture the output with test_util.capture_sys_output()
[..]
> >> @@ -353,7 +397,7 @@ class DtbPlatdata(object):
> >> """
> >> structs = {}
> >> for node in self._valid_nodes:
> >> - node_name, _ = get_compat_name(node)
> >> + node_name, _ = self.get_normalized_compat_name(node)
> >> fields = {}
> > I wonder how long this scan takes? Should we cache the results somewhere?
>
> Probably you are worried because this task will be repeated every time
> we compile U-boot, and it could be a potential issue when testing
> several different configs. However, at least in my setup the scan takes
> less than 1 sec, which I think is promising. Also I think that this scan
> is only perform for those configs which enable OF_PLATDATA, which are
> 15, should not cause much overhead.
>
> What do you think?
I think that is OK for now.
[..]
Regards,
Simon
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 2/6] core: extend struct driver_info to point to device
2020-05-21 13:15 ` Walter Lozano
@ 2020-05-22 23:13 ` Simon Glass
0 siblings, 0 replies; 22+ messages in thread
From: Simon Glass @ 2020-05-22 23:13 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Thu, 21 May 2020 at 07:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Hi Simon,
>
> On 20/5/20 00:07, Simon Glass wrote:
> > Hi Walter,
> >
> > On Wed, 13 May 2020 at 14:14, Walter Lozano<walter.lozano@collabora.com> wrote:
> >> Currently when creating an U_BOOT_DEVICE entry a struct driver_info
> >> is declared, which contains the data needed to instantiate the device.
> >> However, the actual device is created at runtime and there is no proper
> >> way to get the device based on its struct driver_info.
> >>
> >> This patch extends struct driver_info adding a pointer to udevice which
> >> is populated during the bind process, allowing to generate a set of
> >> functions to get the device based on its struct driver_info.
> >>
> >> Signed-off-by: Walter Lozano<walter.lozano@collabora.com>
> >> ---
> >> drivers/core/device.c | 25 +++++++++++++++++++++++--
> >> drivers/core/root.c | 6 +++++-
> >> include/dm/device-internal.h | 2 +-
> >> include/dm/device.h | 13 +++++++++++++
> >> include/dm/platdata.h | 6 ++++++
> >> 5 files changed, 48 insertions(+), 4 deletions(-)
> >>
[..]
> >> @@ -346,6 +346,10 @@ int dm_init_and_scan(bool pre_reloc_only)
> >> {
> >> int ret;
> >>
> >> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> > This one can use if() I think
>
> I think that as populate_phandle_data() is only defined when OF_PLATDATA
> is enabled (as it is created by dtoc), this will trigger and link error
> on those board which don't use this config. Is my understanding correct?
Hopefully not. The linker should drop code that is not called, so it
won't become an issue.
>
> >> + populate_phandle_data();
> >> +#endif
> >> +
> >> ret = dm_init(IS_ENABLED(CONFIG_OF_LIVE));
> >> if (ret) {
> >> debug("dm_init() failed: %d\n", ret);
> >> diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h
> >> index 294d6c1810..5145fb4e14 100644
> >> --- a/include/dm/device-internal.h
> >> +++ b/include/dm/device-internal.h
> >> @@ -81,7 +81,7 @@ int device_bind_with_driver_data(struct udevice *parent,
> >> * @return 0 if OK, -ve on error
> >> */
> >> int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
> >> - const struct driver_info *info, struct udevice **devp);
> >> + struct driver_info *info, struct udevice **devp);
> > That change should really be a separate patch I think.
>
> If we move this change to a separate patch, this will fail to compile as
> struct driver_info needs to be updated by device_bind_by_name to fill
> info->dev. I can move both things to a different patch, in that case.
>
> Please advice.
Yes I just mean that here you are taking away the 'const' and I think
that little bit should be in its own patch. So take away the const,
but do nothing else.
[..]
Regards.
Simon
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2020-05-22 23:13 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-13 20:13 [RFC 0/6] improve OF_PLATDATA support Walter Lozano
2020-05-13 20:13 ` [RFC 1/6] dtoc: add support to scan drivers Walter Lozano
2020-05-20 3:07 ` Simon Glass
2020-05-21 13:15 ` Walter Lozano
2020-05-22 23:13 ` Simon Glass
2020-05-13 20:13 ` [RFC 2/6] core: extend struct driver_info to point to device Walter Lozano
2020-05-20 3:07 ` Simon Glass
2020-05-21 13:15 ` Walter Lozano
2020-05-22 23:13 ` Simon Glass
2020-05-13 20:13 ` [RFC 3/6] dtoc: extend dtoc to use struct driver_info when linking nodes Walter Lozano
2020-05-20 3:07 ` Simon Glass
2020-05-21 13:16 ` Walter Lozano
2020-05-13 20:13 ` [RFC 4/6] dtoc: update tests to match new platdata Walter Lozano
2020-05-20 3:07 ` Simon Glass
2020-05-21 13:16 ` Walter Lozano
2020-05-13 20:13 ` [RFC 5/6] dtoc: update dtb_platdata to support cd-gpios Walter Lozano
2020-05-20 3:07 ` Simon Glass
2020-05-13 20:13 ` [RFC 6/6] dtoc add test for cd-gpios Walter Lozano
2020-05-20 3:07 ` Simon Glass
2020-05-21 13:16 ` Walter Lozano
2020-05-20 3:07 ` [RFC 0/6] improve OF_PLATDATA support Simon Glass
2020-05-21 13:15 ` Walter Lozano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.