From mboxrd@z Thu Jan 1 00:00:00 1970 From: Walter Lozano Date: Thu, 21 May 2020 10:16:04 -0300 Subject: [RFC 3/6] dtoc: extend dtoc to use struct driver_info when linking nodes In-Reply-To: References: <20200513201345.26769-1-walter.lozano@collabora.com> <20200513201345.26769-4-walter.lozano@collabora.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 20/5/20 00:07, Simon Glass wrote: > Hi Walter, > > On Wed, 13 May 2020 at 14:14, Walter Lozano 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 >> --- >> 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