[dtc PATCH V2] Warn on node name unit-address presence/absence mismatch

[dtc PATCH V2] Warn on node name unit-address presence/absence mismatch

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

ePAPR 1.1 section 2.2.1.1 "Node Name Requirements" specifies that any
node that has a reg property must include a unit address in its name
with value matching the first entry in its reg property. Conversely, if
a node does not have a reg property, the node name must not include a
unit address.

Implement a check for this. The code doesn't validate the format of the
unit address; ePAPR implies this may vary from (containing bus) binding
to binding, so doing so would be much more complex.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
v2: Implement the new checks separately, rather than as part of existing
    checkes. downgrade from errors to warnings. Add tests.

Question: Do I need to make a special exception for the /memory node? I
assume we do want to fix that too, so the answer is no. That will require
removing the /memory node from skeleton.dtsi in the kernel though.
---
 checks.c                        | 22 ++++++++++++++++++++--
 tests/reg-without-unit-addr.dts | 10 ++++++++++
 tests/run_tests.sh              |  2 ++
 tests/unit-addr-without-reg.dts |  9 +++++++++
 4 files changed, 41 insertions(+), 2 deletions(-)
 create mode 100644 tests/reg-without-unit-addr.dts
 create mode 100644 tests/unit-addr-without-reg.dts

diff --git a/checks.c b/checks.c
index ee96a25..8d2ccc5 100644
--- a/checks.c
+++ b/checks.c
@@ -293,6 +293,24 @@ static void check_node_name_format(struct check *c, struct node *dt,
 }
 NODE_ERROR(node_name_format, NULL, &node_name_chars);
 
+static void check_unit_address_vs_reg(struct check *c, struct node *dt,
+			     struct node *node)
+{
+	const char *unitname = get_unitname(node);
+	struct property *prop = get_property(node, "reg");
+
+	if (prop) {
+		if (!unitname[0])
+			FAIL(c, "Node %s has a reg property, but no unit name",
+			    node->fullpath);
+	} else {
+		if (unitname[0])
+			FAIL(c, "Node %s has a unit name, but no reg property",
+			    node->fullpath);
+	}
+}
+NODE_WARNING(unit_address_vs_reg, NULL);
+
 static void check_property_name_chars(struct check *c, struct node *dt,
 				      struct node *node, struct property *prop)
 {
@@ -653,8 +671,8 @@ TREE_WARNING(obsolete_chosen_interrupt_controller, NULL);
 
 static struct check *check_table[] = {
 	&duplicate_node_names, &duplicate_property_names,
-	&node_name_chars, &node_name_format, &property_name_chars,
-	&name_is_string, &name_properties,
+	&node_name_chars, &node_name_format, &unit_address_vs_reg,
+	&property_name_chars, &name_is_string, &name_properties,
 
 	&duplicate_label,
 
diff --git a/tests/reg-without-unit-addr.dts b/tests/reg-without-unit-addr.dts
new file mode 100644
index 0000000..aaf8af7
--- /dev/null
+++ b/tests/reg-without-unit-addr.dts
@@ -0,0 +1,10 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	node {
+		reg = <0 1>;
+	};
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index c0a136b..8a95bec 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -400,6 +400,8 @@ dtc_tests () {
     check_tests reg-ranges-root.dts reg_format ranges_format
     check_tests default-addr-size.dts avoid_default_addr_size
     check_tests obsolete-chosen-interrupt-controller.dts obsolete_chosen_interrupt_controller
+    check_tests reg-without-unit-addr.dts unit_address_vs_reg
+    check_tests unit-addr-without-reg.dts unit_address_vs_reg
     run_sh_test dtc-checkfails.sh node_name_chars -- -I dtb -O dtb bad_node_char.dtb
     run_sh_test dtc-checkfails.sh node_name_format -- -I dtb -O dtb bad_node_format.dtb
     run_sh_test dtc-checkfails.sh prop_name_chars -- -I dtb -O dtb bad_prop_char.dtb
diff --git a/tests/unit-addr-without-reg.dts b/tests/unit-addr-without-reg.dts
new file mode 100644
index 0000000..ac786eb
--- /dev/null
+++ b/tests/unit-addr-without-reg.dts
@@ -0,0 +1,9 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	node@1 {
+	};
+};
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [dtc PATCH V2] Warn on node name unit-address presence/absence mismatch

[Kumar Gala]

On Sep 19, 2013, at 12:54 PM, Stephen Warren wrote:

> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> ePAPR 1.1 section 2.2.1.1 "Node Name Requirements" specifies that any
> node that has a reg property must include a unit address in its name
> with value matching the first entry in its reg property. Conversely, if
> a node does not have a reg property, the node name must not include a
> unit address.
> 
> Implement a check for this. The code doesn't validate the format of the
> unit address; ePAPR implies this may vary from (containing bus) binding
> to binding, so doing so would be much more complex.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> v2: Implement the new checks separately, rather than as part of existing
>    checkes. downgrade from errors to warnings. Add tests.
> 
> Question: Do I need to make a special exception for the /memory node? I
> assume we do want to fix that too, so the answer is no. That will require
> removing the /memory node from skeleton.dtsi in the kernel though.
> ---
> checks.c                        | 22 ++++++++++++++++++++--
> tests/reg-without-unit-addr.dts | 10 ++++++++++
> tests/run_tests.sh              |  2 ++
> tests/unit-addr-without-reg.dts |  9 +++++++++
> 4 files changed, 41 insertions(+), 2 deletions(-)
> create mode 100644 tests/reg-without-unit-addr.dts
> create mode 100644 tests/unit-addr-without-reg.dts

What about the case of no reg but a ranges?

This pattern shows up on a lot (if not all) the PPC dts:

arch/powerpc/boot/dts/mpc8544ds.dts:

        board_soc: soc: soc8544@e0000000 {
                ranges = <0x0 0x0 0xe0000000 0x100000>;
        };

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [dtc PATCH V2] Warn on node name unit-address presence/absence mismatch

[Stephen Warren]

On 09/26/2013 12:21 PM, Kumar Gala wrote:
> 
> On Sep 19, 2013, at 12:54 PM, Stephen Warren wrote:
> 
>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> ePAPR 1.1 section 2.2.1.1 "Node Name Requirements" specifies that any
>> node that has a reg property must include a unit address in its name
>> with value matching the first entry in its reg property. Conversely, if
>> a node does not have a reg property, the node name must not include a
>> unit address.
>>
>> Implement a check for this. The code doesn't validate the format of the
>> unit address; ePAPR implies this may vary from (containing bus) binding
>> to binding, so doing so would be much more complex.

> What about the case of no reg but a ranges?
> 
> This pattern shows up on a lot (if not all) the PPC dts:
> 
> arch/powerpc/boot/dts/mpc8544ds.dts:
> 
>         board_soc: soc: soc8544@e0000000 {
>                 ranges = <0x0 0x0 0xe0000000 0x100000>;

Well, ePAPR seems pretty specific that unit address and reg are related,
but says nothing about ranges in the section on node naming, nor about
node naming in the section about ranges.

I'd claim that the existing PPC trees are nonconforming, and should be
fixed too:-)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [dtc PATCH V2] Warn on node name unit-address presence/absence mismatch

[Benjamin Herrenschmidt]

On Thu, 2013-09-26 at 17:12 -0600, Stephen Warren wrote:
> Well, ePAPR seems pretty specific that unit address and reg are
> related,
> but says nothing about ranges in the section on node naming, nor about
> node naming in the section about ranges.
> 
> I'd claim that the existing PPC trees are nonconforming, and should be
> fixed too:-)

This is tricky, we should probably fix ePAPR here.

If you have a "soc" bus covering a given range of addresses which it
forwards to its children devices but doesn't have per-se its own
registers in that area, then it wouldn't have a "reg" property. I would
thus argue that in the absence of a "reg" property, if a "ranges" one is
present, the "parent address" entry in there is an acceptable substitute
for the "reg" property as far as unit addresses are concerned.

Also don't forget that in real OFW land, the unit address is something
that's somewhat bus specific ... for example, PCI uses "dev,fn" rather
than the full 96-bit number of the "reg" entry :-)

Another option which would more strictly conform to ePAPR and in fact to
of1275 would be to require such bus nodes to have a "reg" property with
the address value set to the beginning of the range and the size value
set to 0 :-)

Cheers,
Ben


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [dtc PATCH V2] Warn on node name unit-address presence/absence mismatch

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

On Fri, Sep 27, 2013 at 11:30:38AM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2013-09-26 at 17:12 -0600, Stephen Warren wrote:
> > Well, ePAPR seems pretty specific that unit address and reg are
> > related,
> > but says nothing about ranges in the section on node naming, nor about
> > node naming in the section about ranges.
> > 
> > I'd claim that the existing PPC trees are nonconforming, and should be
> > fixed too:-)
> 
> This is tricky, we should probably fix ePAPR here.
> 
> If you have a "soc" bus covering a given range of addresses which it
> forwards to its children devices but doesn't have per-se its own
> registers in that area, then it wouldn't have a "reg" property. I would
> thus argue that in the absence of a "reg" property, if a "ranges" one is
> present, the "parent address" entry in there is an acceptable substitute
> for the "reg" property as far as unit addresses are concerned.

So, that's been accepted practice in fdt world for a while; I think
ePAPR already permits that, in fact.

> Also don't forget that in real OFW land, the unit address is something
> that's somewhat bus specific ... for example, PCI uses "dev,fn" rather
> than the full 96-bit number of the "reg" entry :-)
> 
> Another option which would more strictly conform to ePAPR and in fact to
> of1275 would be to require such bus nodes to have a "reg" property with
> the address value set to the beginning of the range and the size value
> set to 0 :-)
> 

-- 
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 #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [dtc PATCH V2] Warn on node name unit-address presence/absence mismatch

[Kumar Gala]

On Sep 27, 2013, at 12:17 AM, David Gibson wrote:

> On Fri, Sep 27, 2013 at 11:30:38AM +1000, Benjamin Herrenschmidt wrote:
>> On Thu, 2013-09-26 at 17:12 -0600, Stephen Warren wrote:
>>> Well, ePAPR seems pretty specific that unit address and reg are
>>> related,
>>> but says nothing about ranges in the section on node naming, nor about
>>> node naming in the section about ranges.
>>> 
>>> I'd claim that the existing PPC trees are nonconforming, and should be
>>> fixed too:-)
>> 
>> This is tricky, we should probably fix ePAPR here.
>> 
>> If you have a "soc" bus covering a given range of addresses which it
>> forwards to its children devices but doesn't have per-se its own
>> registers in that area, then it wouldn't have a "reg" property. I would
>> thus argue that in the absence of a "reg" property, if a "ranges" one is
>> present, the "parent address" entry in there is an acceptable substitute
>> for the "reg" property as far as unit addresses are concerned.
> 
> So, that's been accepted practice in fdt world for a while; I think
> ePAPR already permits that, in fact.

Are you saying that the bus binding would cover this case or something else?

>> Also don't forget that in real OFW land, the unit address is something
>> that's somewhat bus specific ... for example, PCI uses "dev,fn" rather
>> than the full 96-bit number of the "reg" entry :-)
>> 
>> Another option which would more strictly conform to ePAPR and in fact to
>> of1275 would be to require such bus nodes to have a "reg" property with
>> the address value set to the beginning of the range and the size value
>> set to 0 :-)

Uugh, that's a bit ugly.  I wonder what breaks if we had reg w/size 0.

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [dtc PATCH V2] Warn on node name unit-address presence/absence mismatch

[Kumar Gala]

On Sep 26, 2013, at 8:30 PM, Benjamin Herrenschmidt wrote:

> On Thu, 2013-09-26 at 17:12 -0600, Stephen Warren wrote:
>> Well, ePAPR seems pretty specific that unit address and reg are
>> related,
>> but says nothing about ranges in the section on node naming, nor about
>> node naming in the section about ranges.
>> 
>> I'd claim that the existing PPC trees are nonconforming, and should be
>> fixed too:-)
> 
> This is tricky, we should probably fix ePAPR here.

I'll poke Stuart to see what's going w/updating ePAPR.

> If you have a "soc" bus covering a given range of addresses which it
> forwards to its children devices but doesn't have per-se its own
> registers in that area, then it wouldn't have a "reg" property. I would
> thus argue that in the absence of a "reg" property, if a "ranges" one is
> present, the "parent address" entry in there is an acceptable substitute
> for the "reg" property as far as unit addresses are concerned.

Either we update the section in general about 'ranges' or at least update the simple-bus binding to state that rules about the node name.

> Also don't forget that in real OFW land, the unit address is something
> that's somewhat bus specific ... for example, PCI uses "dev,fn" rather
> than the full 96-bit number of the "reg" entry :-)
> 
> Another option which would more strictly conform to ePAPR and in fact to
> of1275 would be to require such bus nodes to have a "reg" property with
> the address value set to the beginning of the range and the size value
> set to 0 :-)

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [dtc PATCH V2] Warn on node name unit-address presence/absence mismatch

[Stephen Warren]

On 09/27/2013 09:39 AM, Kumar Gala wrote:
> 
> On Sep 26, 2013, at 8:30 PM, Benjamin Herrenschmidt wrote:
> 
>> On Thu, 2013-09-26 at 17:12 -0600, Stephen Warren wrote:
>>> Well, ePAPR seems pretty specific that unit address and reg are
>>> related,
>>> but says nothing about ranges in the section on node naming, nor about
>>> node naming in the section about ranges.
>>>
>>> I'd claim that the existing PPC trees are nonconforming, and should be
>>> fixed too:-)
>>
>> This is tricky, we should probably fix ePAPR here.
> 
> I'll poke Stuart to see what's going w/updating ePAPR.
> 
>> If you have a "soc" bus covering a given range of addresses which it
>> forwards to its children devices but doesn't have per-se its own
>> registers in that area, then it wouldn't have a "reg" property. I would
>> thus argue that in the absence of a "reg" property, if a "ranges" one is
>> present, the "parent address" entry in there is an acceptable substitute
>> for the "reg" property as far as unit addresses are concerned.
> 
> Either we update the section in general about 'ranges' or at least update the simple-bus binding to state that rules about the node name.

I think you'd need to update section 2.2.1.1 which specifies the node
name rather than any other section.

The wording in 2.2.1.1 certainly states that buses can impose specific
rules on the value/format of the unit address in the node name, but I'm
not convinced it allows buses to override the rules that control the
presence/absence of the unit address.

In other words, I would simply replace:

The unit-address must match the first address specified in the reg
property of the node

with:

The unit-address must match the first address specified in the reg
property of the node. If the node has no reg property, the unit-address
must match the first address specified in the ranges property of the node.

and:

If the node has no reg property,

with:

If the node has no reg property or ranges property,
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html