[RFC 3/6] dtoc: extend dtoc to use struct driver_info when linking nodes

[Walter Lozano <walter.lozano@collabora.com>]

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