DT: augment tree and use it to re-order macros #22612

mbolivar · 2020-02-07T20:29:10Z

This reorders the DT macros so they're easier to read.

I've tried to be careful about bisectability by making the output of this PR all the way up to the commit with shortlog scripts: gen_defines: re-work write_bus() with augmented nodes match the existing implementation exactly (other than the order in which macros are generated, and some additional comments). This carries forward some behavior I think is questionable, which I've left FIXME comments about.

I've run sanitycheck --cmake-only -T samples and wrote a script to compare the macros before this PR and up to that commit, without any observed differences.

This new way of doing things is much more explicit about which macros are generated in the code, which should hopefully be more readable and make it easier to see things that could be removed.

mbolivar · 2020-02-07T20:35:06Z

Example output for /soc/spi@10014000/flash@0 when building hello_world for hifive1_revb:

/* BASE_ADDRESS and SIZE macros from the "reg" property */
#define DT_SIFIVE_SPI0_10014000_JEDEC_SPI_NOR_0_BASE_ADDRESS_0 0x0
#define DT_SIFIVE_SPI0_10014000_JEDEC_SPI_NOR_0_SIZE_0 0x0
#define DT_SIFIVE_SPI0_10014000_JEDEC_SPI_NOR_0_BASE_ADDRESS DT_SIFIVE_SPI0_10014000_JEDEC_SPI_NOR_0_BASE_ADDRESS_0
#define DT_SIFIVE_SPI0_10014000_JEDEC_SPI_NOR_0_SIZE DT_SIFIVE_SPI0_10014000_JEDEC_SPI_NOR_0_SIZE_0
#define DT_INST_0_ISSI_IS25LP128_BASE_ADDRESS_0     DT_SIFIVE_SPI0_10014000_JEDEC_SPI_NOR_0_BASE_ADDRESS_0
#define DT_INST_0_ISSI_IS25LP128_SIZE_0             DT_SIFIVE_SPI0_10014000_JEDEC_SPI_NOR_0_SIZE_0
#define DT_INST_0_ISSI_IS25LP128_BASE_ADDRESS       DT_SIFIVE_SPI0_10014000_JEDEC_SPI_NOR_0_BASE_ADDRESS_0
#define DT_INST_0_ISSI_IS25LP128_SIZE               DT_SIFIVE_SPI0_10014000_JEDEC_SPI_NOR_0_SIZE_0
#define DT_INST_0_JEDEC_SPI_NOR_BASE_ADDRESS_0      DT_SIFIVE_SPI0_10014000_JEDEC_SPI_NOR_0_BASE_ADDRESS_0
#define DT_INST_0_JEDEC_SPI_NOR_SIZE_0              DT_SIFIVE_SPI0_10014000_JEDEC_SPI_NOR_0_SIZE_0
#define DT_INST_0_JEDEC_SPI_NOR_BASE_ADDRESS        DT_SIFIVE_SPI0_10014000_JEDEC_SPI_NOR_0_BASE_ADDRESS_0
#define DT_INST_0_JEDEC_SPI_NOR_SIZE                DT_SIFIVE_SPI0_10014000_JEDEC_SPI_NOR_0_SIZE_0

And for /soc/interrupt-controller@c000000:

/* BASE_ADDRESS and SIZE macros from the "reg" property */
#define DT_SIFIVE_PLIC_1_0_0_C000000_BASE_ADDRESS_0 0xc000000
#define DT_SIFIVE_PLIC_1_0_0_C000000_SIZE_0         0x2000
#define DT_SIFIVE_PLIC_1_0_0_C000000_PRIO_BASE_ADDRESS DT_SIFIVE_PLIC_1_0_0_C000000_BASE_ADDRESS_0
#define DT_SIFIVE_PLIC_1_0_0_C000000_PRIO_SIZE      DT_SIFIVE_PLIC_1_0_0_C000000_SIZE_0
#define DT_SIFIVE_PLIC_1_0_0_C000000_BASE_ADDRESS_1 0xc002000
#define DT_SIFIVE_PLIC_1_0_0_C000000_SIZE_1         0x1fe000
#define DT_SIFIVE_PLIC_1_0_0_C000000_IRQ_EN_BASE_ADDRESS DT_SIFIVE_PLIC_1_0_0_C000000_BASE_ADDRESS_1
#define DT_SIFIVE_PLIC_1_0_0_C000000_IRQ_EN_SIZE    DT_SIFIVE_PLIC_1_0_0_C000000_SIZE_1
#define DT_SIFIVE_PLIC_1_0_0_C000000_BASE_ADDRESS_2 0xc200000
#define DT_SIFIVE_PLIC_1_0_0_C000000_SIZE_2         0x2000000
#define DT_SIFIVE_PLIC_1_0_0_C000000_REG_BASE_ADDRESS DT_SIFIVE_PLIC_1_0_0_C000000_BASE_ADDRESS_2
#define DT_SIFIVE_PLIC_1_0_0_C000000_REG_SIZE       DT_SIFIVE_PLIC_1_0_0_C000000_SIZE_2
#define DT_INST_0_SIFIVE_PLIC_1_0_0_BASE_ADDRESS_0  DT_SIFIVE_PLIC_1_0_0_C000000_BASE_ADDRESS_0
#define DT_INST_0_SIFIVE_PLIC_1_0_0_SIZE_0          DT_SIFIVE_PLIC_1_0_0_C000000_SIZE_0
#define DT_INST_0_SIFIVE_PLIC_1_0_0_PRIO_BASE_ADDRESS DT_SIFIVE_PLIC_1_0_0_C000000_BASE_ADDRESS_0
#define DT_INST_0_SIFIVE_PLIC_1_0_0_PRIO_SIZE       DT_SIFIVE_PLIC_1_0_0_C000000_SIZE_0
#define DT_INST_0_SIFIVE_PLIC_1_0_0_BASE_ADDRESS_1  DT_SIFIVE_PLIC_1_0_0_C000000_BASE_ADDRESS_1
#define DT_INST_0_SIFIVE_PLIC_1_0_0_SIZE_1          DT_SIFIVE_PLIC_1_0_0_C000000_SIZE_1
#define DT_INST_0_SIFIVE_PLIC_1_0_0_IRQ_EN_BASE_ADDRESS DT_SIFIVE_PLIC_1_0_0_C000000_BASE_ADDRESS_1
#define DT_INST_0_SIFIVE_PLIC_1_0_0_IRQ_EN_SIZE     DT_SIFIVE_PLIC_1_0_0_C000000_SIZE_1
#define DT_INST_0_SIFIVE_PLIC_1_0_0_BASE_ADDRESS_2  DT_SIFIVE_PLIC_1_0_0_C000000_BASE_ADDRESS_2
#define DT_INST_0_SIFIVE_PLIC_1_0_0_SIZE_2          DT_SIFIVE_PLIC_1_0_0_C000000_SIZE_2
#define DT_INST_0_SIFIVE_PLIC_1_0_0_REG_BASE_ADDRESS DT_SIFIVE_PLIC_1_0_0_C000000_BASE_ADDRESS_2
#define DT_INST_0_SIFIVE_PLIC_1_0_0_REG_SIZE        DT_SIFIVE_PLIC_1_0_0_C000000_SIZE_2

mbolivar · 2020-02-07T20:36:00Z

@pabigot -- this also fixes an issue you mentioned in dev-review where you have to special-case code depending on whether there is only one reg or more than one. Now, the BASE_ADDRESS_0 and BASE_ADDRESS defines are both emitted.

EDIT: for bisectability, this is no longer true.

scripts/dts/gen_defines.py

ulfalizer · 2020-02-07T23:02:42Z

scripts/dts/gen_defines.py

+    #   compatible, plus information from its unit address (or its
+    #   parent's unit address) or its name, and/or its bus. No example
+    #   given since this is a complex and questionably useful form of
+    #   generating an identifier.


Wonder if these could be calculated as needed instead, e.g. with a function that takes a node and returns the path identifier. Maybe the z_ could be dropped then too, if the functions are in gen_defines.py.

I realize that efficiency is not necessarily a goal for this script, but one of the benefits of augmenting the tree is that you can reuse the results of node.edt.compat2enabled[compat].index(node) by stashing it in z_inst_idents instead of repeating it each time node_instance_aliases() is called.

Since your comments so far seem to be about specifics rather than the PR in general, I'll fold in your suggestions and keep working on it. When it's done, node_instance_aliases() will be removed entirely.

ulfalizer · 2020-02-07T23:06:20Z

scripts/dts/gen_defines.py

+        # the primary macros are saved.
+
+        for reg_i, reg in enumerate(node.regs):
+            idx = f"_{reg_i}"


I'd remove this variable and pass f"_{reg_i}" directly to write_reg().

ulfalizer · 2020-02-07T23:12:33Z

scripts/dts/gen_defines.py

+
+        for reg_i, reg in enumerate(node.regs):
+            idx = f"_{reg_i}"
+            name = f"{str2ident(reg.name)}_" if reg.name else ""


Wonder if this is redundant. name is only used within an if reg.name below, so it seems it'd never be "" if it's used.

Maybe f"{str2ident(reg.name)}_" could be passed directly to write_reg() instead.

ulfalizer · 2020-02-07T23:53:05Z

scripts/dts/gen_defines.py

+            if reg.name:
+                write_reg(ident, reg, addr, size, name, "")
+
+    def write_reg(ident, reg, addr_val, size_val, prefix, suffix):


Could get rid of some variables so that you don't have to scan ahead to check if size_val and addr_val are used elsewhere:

def write_reg(ident, reg, addr, size, prefix, suffix): if addr is None: addr = hex(reg.addr) if size is None: size = reg.size ...

Looks like addr_val and size_val mean "override address/size from reg with these values, if not None." Wonder if that could be handled in some other way. Bit cryptic.

ulfalizer · 2020-02-08T00:00:58Z

scripts/dts/gen_defines.py

-        if reg.size:
-            write_reg(reg, "SIZE", reg.size)
+    primary_addrs = {}
+    primary_sizes = {}


This is a bit confusing to me. Does "primary" mean "path" here too? Noticed that comment above. I'd spell it out in that case.

A nice naming convention is for dicts is x2y btw, like index2addr.

I'm deliberately being vague about which of the node identifiers is the "primary" identifier. Right now it's the <COMPAT>_<UNIT_ADDR> style one. Later on it will be the path-based one. Let me see if I can make this clearer. Thanks.

ulfalizer · 2020-02-08T00:12:34Z

scripts/dts/gen_defines.py

+            idx = f"_{reg_i}"
+            name = f"{str2ident(reg.name)}_" if reg.name else ""
+            addr = primary_addrs.get(reg_i)
+            size = primary_sizes.get(reg_i)


Maybe this could be refactored to remove the addr = None ... if addr is None: indirection:

if reg_i not in primary_addrs: primary_addrs[reg_i] = prim_addr primary_sizes[reg_i] = prim_size reg = primary_addrs[reg_i] addr = primary_sizes[reg_i] write_reg(...)

Could maybe merge primary_addrs and primary_sizes too, since they're always accessed together, and maybe use a list, since you never get any "holes" in the indices (though dict is fine if it turns out clearer).

mbolivar · 2020-02-13T06:54:55Z

@pabigot @galak @ulfalizer please revisit.

I've finished the work redoing things in terms of the new augmented tree and made other changes in response to review, then added the new namespaces.

Some follow up work should be done after this to address the FIXME comments mentioned in the PR description.

zephyrbot · 2020-02-13T06:58:11Z

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

galak · 2020-02-13T08:43:26Z

rebased on master to pull out the doc change that is now merged in master.

scripts/dts/gen_defines.py

galak · 2020-02-13T09:40:15Z

Add a commit to deal with generation for phandle(s) - based on PR #22770

galak · 2020-02-19T02:50:33Z

scripts/dts/gen_defines.py

+                blank_before=False)
+    for ident in node.z_inst_idents:
+        write_existence_flags_for_ident(node, ident)
+    for ident in node.z_label_idents:


Would prefer if we did NOT generate DT_NODELABEL_FOO 1 and only generated the DT_EXISTS_NODELABEL_FOO 1

galak · 2020-02-19T02:51:13Z

@mbolivar pushed an update to fix a commit message that mentioned NODEORD.

galak · 2020-02-19T02:54:26Z

Might be good to rebase on 22922 and update the docs for DT_PATH, DT_NODELABEL and DT_EXISTS_.

mbolivar · 2020-02-20T02:33:54Z

Might be good to rebase on 22922 and update the docs for DT_PATH, DT_NODELABEL and DT_EXISTS_.

I'm going to rebase on top of #22966 instead, once I can remove DNM from that.

mbolivar · 2020-02-20T18:19:04Z

Re-worked to remove all the commits which change the generated macros.

Add additional attributes to each edtlib.Node we process, before calling into the write_foo() routines. This includes the identifier returned by node_ident(), which is used as the primary identifier for the node, as well as lists for instance and aliases nodes, and a catchall list that contains all the other identifiers in addition to the primary one. Use this information in a new for_each_ident(node, ident) def that will be put to use in the various write_foo() routines. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

Group the macros together by namespace rather than putting all the BASE_ADDRESS macros together and all the SIZE macros together. E.g., all the DT_INST_<x> namespace macros for each node now appear consecutively. Add a comment making it clear that this output comes from "regs", since "BASE_ADDRESS" and "SIZE" are not property names. Other than the order in which they appear and comments, the output before and after this patch should be exactly the same. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

Use augmented nodes to print macros grouped by namespace. Other than the order in which they appear and comments, the output before and after this patch should be exactly the same. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

Mirror the change already done to write_regs(). Other than the order in which they appear and comments, the output before and after this patch should be exactly the same. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

This is similar to the work already done for regs. Other than the order in which they appear and comments, the output before and after this patch should be exactly the same. We're intentionally leaving some of the helpers in module scope here to use some of the subroutines elsewhere later on when reworking write_spi_dev(). Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

This uses augmented nodes in the same way as done previously. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

This too is an attempt to reimplement the previous behavior exactly, modulo the order in which things are defined. This is the last function which is calling into the previous implementation's out_node and node_*_alias() functions, so these can be removed now. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

Write the same results explicitly in its only remaining caller. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

Pad node identifiers to 60 characters. This results in better alignment in practice than the current value of 40, which is a bit low. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

This corresponds to the attribute by the same name in dtlib.Node. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

Add a function takes a 'label' and returns "y" if we find an "enabled" node that has a 'nodelabel' of 'label' in the EDT otherwise we return "n" Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

changes made

mbolivar · 2020-02-20T18:51:41Z

Retested macros are the same between e7be2e1 and 1be14e9.

galak · 2020-02-20T20:04:17Z

Merging by hand since the scancode failure is a bug in scancode action.

mbolivar requested review from galak and ulfalizer as code owners February 7, 2020 20:29

mbolivar requested a review from pabigot February 7, 2020 20:34

mbolivar force-pushed the dt-augment-and-regs branch from 4ed4861 to 947dabe Compare February 7, 2020 20:38

galak reviewed Feb 7, 2020

View reviewed changes

scripts/dts/gen_defines.py Outdated Show resolved Hide resolved

mbolivar force-pushed the dt-augment-and-regs branch from 947dabe to ad57576 Compare February 7, 2020 22:19

ulfalizer reviewed Feb 7, 2020

View reviewed changes

ulfalizer reviewed Feb 8, 2020

View reviewed changes

galak added the area: Devicetree Tooling PR modifies or adds a Device Tree tooling label Feb 8, 2020

ulfalizer reviewed Feb 8, 2020

View reviewed changes

mbolivar changed the title ~~DT: augment tree and use it to print regs~~ DT: augment tree and use it to add NODEORD, PATH, and NODELABEL namespaces Feb 13, 2020

mbolivar force-pushed the dt-augment-and-regs branch from ad57576 to b79da28 Compare February 13, 2020 06:46

mbolivar requested review from nashif and SebastianBoe as code owners February 13, 2020 06:46

zephyrbot added area: Devicetree area: Build System labels Feb 13, 2020

galak force-pushed the dt-augment-and-regs branch from b79da28 to a7b7256 Compare February 13, 2020 08:43

galak reviewed Feb 13, 2020

View reviewed changes

scripts/dts/gen_defines.py Show resolved Hide resolved

galak reviewed Feb 13, 2020

View reviewed changes

scripts/dts/gen_defines.py Outdated Show resolved Hide resolved

galak reviewed Feb 13, 2020

View reviewed changes

scripts/dts/gen_defines.py Outdated Show resolved Hide resolved

galak force-pushed the dt-augment-and-regs branch from a7b7256 to df0c227 Compare February 13, 2020 09:37

mbolivar removed request for jfischer-no and nashif February 18, 2020 19:00

galak force-pushed the dt-augment-and-regs branch from 50f9836 to 07bb757 Compare February 19, 2020 02:49

galak reviewed Feb 19, 2020

View reviewed changes

galak mentioned this pull request Feb 19, 2020

devicetree Arduino bindings do not support identification of bus controllers #20330

Closed

mbolivar force-pushed the dt-augment-and-regs branch from 07bb757 to 7219fb6 Compare February 20, 2020 18:18

mbolivar changed the title ~~DT: augment tree and use it to add DT_PATH and DT_NODELABEL namespaces~~ DT: augment tree and use it to re-order macros Feb 20, 2020

galak changed the base branch from master to topic-devicetree February 20, 2020 18:21

galak approved these changes Feb 20, 2020

View reviewed changes

mbolivar-nordic and others added 11 commits February 20, 2020 10:28

scripts: gen_defines: re-work write_spi_dev() with augmented nodes

3b07bff

This uses augmented nodes in the same way as done previously. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

scripts: gen_defines: remove out()'s aliases kwarg

4d89721

Write the same results explicitly in its only remaining caller. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

scripts: gen_defines: tweak out_define() whitespace

a577661

Pad node identifiers to 60 characters. This results in better alignment in practice than the current value of 40, which is a bit low. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

edtlib: add 'labels' attribute to Node class

7fa1c82

This corresponds to the attribute by the same name in dtlib.Node. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

kconfig: Add dt_nodelabel_enabled function

1be14e9

Add a function takes a 'label' and returns "y" if we find an "enabled" node that has a 'nodelabel' of 'label' in the EDT otherwise we return "n" Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

galak force-pushed the dt-augment-and-regs branch from 3370d7e to 1f5436d Compare February 20, 2020 18:42

mbolivar force-pushed the dt-augment-and-regs branch from 1f5436d to 1be14e9 Compare February 20, 2020 18:50

galak merged commit f516d06 into zephyrproject-rtos:topic-devicetree Feb 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DT: augment tree and use it to re-order macros #22612

DT: augment tree and use it to re-order macros #22612

mbolivar commented Feb 7, 2020 •

edited

Loading

mbolivar commented Feb 7, 2020

mbolivar commented Feb 7, 2020 •

edited

Loading

ulfalizer Feb 7, 2020

mbolivar Feb 8, 2020

ulfalizer Feb 7, 2020

ulfalizer Feb 7, 2020

ulfalizer Feb 7, 2020 •

edited

Loading

ulfalizer Feb 8, 2020

mbolivar Feb 8, 2020

ulfalizer Feb 8, 2020 •

edited

Loading

mbolivar commented Feb 13, 2020

zephyrbot commented Feb 13, 2020 •

edited

Loading

galak commented Feb 13, 2020

galak commented Feb 13, 2020

galak Feb 19, 2020

galak commented Feb 19, 2020

galak commented Feb 19, 2020

mbolivar commented Feb 20, 2020

mbolivar commented Feb 20, 2020

mbolivar commented Feb 20, 2020

galak commented Feb 20, 2020

DT: augment tree and use it to re-order macros #22612

DT: augment tree and use it to re-order macros #22612

Conversation

mbolivar commented Feb 7, 2020 • edited Loading

mbolivar commented Feb 7, 2020

mbolivar commented Feb 7, 2020 • edited Loading

ulfalizer Feb 7, 2020

Choose a reason for hiding this comment

mbolivar Feb 8, 2020

Choose a reason for hiding this comment

ulfalizer Feb 7, 2020

Choose a reason for hiding this comment

ulfalizer Feb 7, 2020

Choose a reason for hiding this comment

ulfalizer Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

ulfalizer Feb 8, 2020

Choose a reason for hiding this comment

mbolivar Feb 8, 2020

Choose a reason for hiding this comment

ulfalizer Feb 8, 2020 • edited Loading

Choose a reason for hiding this comment

mbolivar commented Feb 13, 2020

zephyrbot commented Feb 13, 2020 • edited Loading

galak commented Feb 13, 2020

galak commented Feb 13, 2020

galak Feb 19, 2020

Choose a reason for hiding this comment

galak commented Feb 19, 2020

galak commented Feb 19, 2020

mbolivar commented Feb 20, 2020

mbolivar commented Feb 20, 2020

mbolivar commented Feb 20, 2020

galak commented Feb 20, 2020

mbolivar commented Feb 7, 2020 •

edited

Loading

mbolivar commented Feb 7, 2020 •

edited

Loading

ulfalizer Feb 7, 2020 •

edited

Loading

ulfalizer Feb 8, 2020 •

edited

Loading

zephyrbot commented Feb 13, 2020 •

edited

Loading