[RFC 3/6] dtoc: extend dtoc to use struct driver_info when linking nodes
Walter Lozano
walter.lozano at collabora.com
Thu May 21 15:16:04 CEST 2020
On 20/5/20 00:07, Simon Glass wrote:
> Hi Walter,
>
> On Wed, 13 May 2020 at 14:14, Walter Lozano<walter.lozano at 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 at 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
More information about the U-Boot
mailing list