On Mon, Mar 5, 2018 at 6:18 PM, David Gibson
<david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> 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'?
Nothing really. Left over from copying
check_avoid_unnecessary_addr_size. It doesn't make sense here.
>
>> + 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?
Ah yes, good catch.
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
