On Mon, Mar 05, 2018 at 10:41:27AM -0600, Rob Herring wrote:
> Child nodes with the same unit-address (and different node names) are
> either an error or just bad DT design. Typical errors are the unit-address
> is just wrong (i.e. doesn't match reg value) or multiple children using the
> same overlapping area. Overlapping regions are considered an error in new
> bindings, but do exist in some existing trees. This check should flag
> most but not all of those errors. Finding all cases would require doing
> address translations and creating a full map of address spaces.
>
> Mixing more than one address/number space at a level is bad design. It only
> works if both spaces can use the same #address-cells and #size-cells sizes.
> It also complicates parsing have a mixture of types of child nodes. The
> best practice in this case is adding child container nodes for each
> address/number space.
Or in some cases encoding both sub-address-spaces into a single value
with extra bits and/or cells (similar to the way standard DT PCI
addressing works). Combined encoding is your only option if you want
to map parts of both sub address spaces into the parent with 'ranges'.
>
> Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
Looks good, except for a couple of nits noted below.
> ---
> This throws 90K warnings for arm32! This is largely from i.MX pinctrl
> and OMAP clock bindings which both have a large number of nodes
> following the same pattern. Other arches are in the 10s of warnings.
>
> Rob
>
> checks.c | 34 ++++++++++++++++++++++++++++++++++
> tests/run_tests.sh | 1 +
> tests/unit-addr-unique.dts | 14 ++++++++++++++
> 3 files changed, 49 insertions(+)
> create mode 100644 tests/unit-addr-unique.dts
>
> diff --git a/checks.c b/checks.c
> index c07ba4da9e36..fef3dea37b9d 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -1018,6 +1018,39 @@ static void check_avoid_unnecessary_addr_size(struct check *c, struct dt_info *d
> }
> WARNING(avoid_unnecessary_addr_size, check_avoid_unnecessary_addr_size, NULL, &avoid_default_addr_size);
>
> +static void check_unique_unit_address(struct check *c, struct dt_info *dti,
> + struct node *node)
> +{
> + struct node *childa;
> +
> + if (node->addr_cells < 0 || node->size_cells < 0)
> + return;
> +
> + if (get_property(node, "ranges") || !node->children)
What's the rationale for excluding children of nodes with 'ranges'?
> + return;
> +
> + for_each_child(node, childa) {
> + struct node *childb;
> + const char *addr_a = get_unitname(childa);
> +
> + if (!strlen(addr_a))
> + continue;
> +
> + for_each_child(node, childb) {
> + const char *addr_b = get_unitname(childb);
> + if (childa == childb)
> + break;
> +
> + if (!strlen(addr_a))
You want strlen(addr_b) here, no?
> + continue;
> +
> + if (streq(addr_a, addr_b))
> + FAIL(c, dti, childb, "duplicate unit-address (also used in node %s)", childa->fullpath);
> + }
> + }
> +}
> +WARNING(unique_unit_address, check_unique_unit_address, NULL, &avoid_default_addr_size);
> +
> static void check_obsolete_chosen_interrupt_controller(struct check *c,
> struct dt_info *dti,
> struct node *node)
> @@ -1391,6 +1424,7 @@ static struct check *check_table[] = {
>
> &avoid_default_addr_size,
> &avoid_unnecessary_addr_size,
> + &unique_unit_address,
> &obsolete_chosen_interrupt_controller,
> &chosen_node_is_root, &chosen_node_bootargs, &chosen_node_stdout_path,
>
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 0d30edfc0bc4..eebc38558e1a 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -571,6 +571,7 @@ dtc_tests () {
> check_tests unit-addr-without-reg.dts unit_address_vs_reg
> check_tests unit-addr-leading-0x.dts unit_address_format
> check_tests unit-addr-leading-0s.dts unit_address_format
> + check_tests unit-addr-unique.dts unique_unit_address
> check_tests bad-phandle-cells.dts interrupts_extended_property
> check_tests bad-gpio.dts gpios_property
> run_sh_test dtc-checkfails.sh deprecated_gpio_property -- -Wdeprecated_gpio_property -I dts -O dtb bad-gpio.dts
> diff --git a/tests/unit-addr-unique.dts b/tests/unit-addr-unique.dts
> new file mode 100644
> index 000000000000..7cc650b400c5
> --- /dev/null
> +++ b/tests/unit-addr-unique.dts
> @@ -0,0 +1,14 @@
> +/dts-v1/;
> +
> +/ {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + foo@1 {
> + reg = <1>;
> + };
> +
> + bar@1 {
> + reg = <1>;
> + };
> +};
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
