On 20/5/20 00:07, Simon Glass wrote:
Hi Walter, On Wed, 13 May 2020 at 14:14, Walter Lozano<walter.loz...@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.loz...@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.
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().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.pyCan 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.
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
