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] ?
> 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
