[PATCH 0/5] dtc unit-address and character set checks

[PATCH 0/5] dtc unit-address and character set checks

[Rob Herring]

This is reviving a series of checks I started on a while back. The 
motivation for this series is to add checks for trivial issues often 
found in binding reviews.

The first 2 patches restrict the character set for node and property 
names to what's the recommended practice. These are quite noisy when 
enabled mainly due to '_', but that's most often what needs fixing.

The 3rd patch checks for leading 0s and '0x' on unit addresses.

The last 2 patches add infrastructure for setting the bus type of nodes 
and bus specific checks for those nodes. Initially, PCI is the only 
supported bus type. Unlike the RFC, there's no default/simple bus.

Rob

Rob Herring (5):
  checks: Add Warning for stricter property name character checking
  checks: Add Warning for stricter node name character checking
  checks: Warn on node name unit-addresses with '0x' or leading 0s
  checks: Add infrastructure for setting bus type of nodes
  checks: Add bus checks for PCI buses

 checks.c                       | 199 +++++++++++++++++++++++++++++++++++++++--
 dtc.h                          |  11 +++
 tests/run_tests.sh             |   2 +
 tests/unit-addr-leading-0s.dts |  10 +++
 tests/unit-addr-leading-0x.dts |  10 +++
 5 files changed, 227 insertions(+), 5 deletions(-)
 create mode 100644 tests/unit-addr-leading-0s.dts
 create mode 100644 tests/unit-addr-leading-0x.dts

-- 
2.10.1

[PATCH 1/5] checks: Add Warning for stricter property name character checking

[Rob Herring]

While '?', '.', '+', '*', and '_' are considered valid characters their
use is discouraged in recommended practices. '#' is also only
recommended to be used at the beginning of property names.

Testing this found one typo error with '.' used instead of ','. The
rest of the warnings were all from underscores.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/checks.c b/checks.c
index 3d18e45374c8..a0d4a9d968d7 100644
--- a/checks.c
+++ b/checks.c
@@ -239,6 +239,7 @@ ERROR(duplicate_property_names, check_duplicate_property_names, NULL);
 #define UPPERCASE	"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
 #define DIGITS		"0123456789"
 #define PROPNODECHARS	LOWERCASE UPPERCASE DIGITS ",._+*#?-"
+#define PROPNODECHARSSTRICT	LOWERCASE UPPERCASE DIGITS ",-"
 
 static void check_node_name_chars(struct check *c, struct dt_info *dti,
 				  struct node *node)
@@ -299,6 +300,38 @@ static void check_property_name_chars(struct check *c, struct dt_info *dti,
 }
 ERROR(property_name_chars, check_property_name_chars, PROPNODECHARS);
 
+static void check_property_name_chars_strict(struct check *c,
+					     struct dt_info *dti,
+					     struct node *node)
+{
+	struct property *prop;
+
+	for_each_property(node, prop) {
+		const char *name = prop->name;
+		int n = strspn(name, c->data);
+
+		if (n == strlen(prop->name))
+			continue;
+
+		/* Certain names are whitelisted */
+		if (strcmp(name, "device_type") == 0)
+			continue;
+
+		/*
+		 * # is only allowed at the beginning of property names not counting
+		 * the vendor prefix.
+		 */
+		if (name[n] == '#' && ((n == 0) || (name[n-1] == ','))) {
+			name += n + 1;
+			n = strspn(name, c->data);
+		}
+		if (n < strlen(name))
+			FAIL(c, "Character '%c' not recommended in property name \"%s\", node %s",
+			     name[n], prop->name, node->fullpath);
+	}
+}
+WARNING(property_name_chars_strict, check_property_name_chars_strict, PROPNODECHARSSTRICT);
+
 #define DESCLABEL_FMT	"%s%s%s%s%s"
 #define DESCLABEL_ARGS(node,prop,mark)		\
 	((mark) ? "value of " : ""),		\
@@ -703,6 +736,8 @@ static struct check *check_table[] = {
 	&address_cells_is_cell, &size_cells_is_cell, &interrupt_cells_is_cell,
 	&device_type_is_string, &model_is_string, &status_is_string,
 
+	&property_name_chars_strict,
+
 	&addr_size_cells, &reg_format, &ranges_format,
 
 	&unit_address_vs_reg,
-- 
2.10.1

[PATCH 2/5] checks: Add Warning for stricter node name character checking

[Rob Herring]

While '#', '?', '.', '+', '*', and '_' are considered valid characters,
their use is discouraged in recommended practices.

Testing this found a few cases of '.'. The majority of the warnings were
all from underscores.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/checks.c b/checks.c
index a0d4a9d968d7..0c78d69316bc 100644
--- a/checks.c
+++ b/checks.c
@@ -252,6 +252,17 @@ static void check_node_name_chars(struct check *c, struct dt_info *dti,
 }
 ERROR(node_name_chars, check_node_name_chars, PROPNODECHARS "@");
 
+static void check_node_name_chars_strict(struct check *c, struct dt_info *dti,
+					 struct node *node)
+{
+	int n = strspn(node->name, c->data);
+
+	if (n < node->basenamelen)
+		FAIL(c, "Character '%c' not recommended in node %s",
+		     node->name[n], node->fullpath);
+}
+WARNING(node_name_chars_strict, check_node_name_chars_strict, PROPNODECHARSSTRICT);
+
 static void check_node_name_format(struct check *c, struct dt_info *dti,
 				   struct node *node)
 {
@@ -737,6 +748,7 @@ static struct check *check_table[] = {
 	&device_type_is_string, &model_is_string, &status_is_string,
 
 	&property_name_chars_strict,
+	&node_name_chars_strict,
 
 	&addr_size_cells, &reg_format, &ranges_format,
 
-- 
2.10.1

--
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

[PATCH 3/5] checks: Warn on node name unit-addresses with '0x' or leading 0s

[Rob Herring]

Node name unit-addresses should never begin with 0x or leading 0s
regardless of whether they have a bus specific address (i.e. one with
commas) or not. Add warnings to check for these cases.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c                       | 10 ++++++++++
 tests/run_tests.sh             |  2 ++
 tests/unit-addr-leading-0s.dts | 10 ++++++++++
 tests/unit-addr-leading-0x.dts | 10 ++++++++++
 4 files changed, 32 insertions(+)
 create mode 100644 tests/unit-addr-leading-0s.dts
 create mode 100644 tests/unit-addr-leading-0x.dts

diff --git a/checks.c b/checks.c
index 0c78d69316bc..8e310d69ca3d 100644
--- a/checks.c
+++ b/checks.c
@@ -288,6 +288,16 @@ static void check_unit_address_vs_reg(struct check *c, struct dt_info *dti,
 		if (!unitname[0])
 			FAIL(c, "Node %s has a reg or ranges property, but no unit name",
 			    node->fullpath);
+
+		if (!strncmp(unitname, "0x", 2)) {
+			FAIL(c, "Node %s unit name should not have leading \"0x\"",
+			    node->fullpath);
+			/* skip over 0x for next test */
+			unitname += 2;
+		}
+		if (unitname[0] == '0' && isxdigit(unitname[1]))
+			FAIL(c, "Node %s unit name should not have leading 0s",
+			    node->fullpath);
 	} else {
 		if (unitname[0])
 			FAIL(c, "Node %s has a unit name, but no reg property",
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 157dbaea7600..2c06666a9e23 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -539,6 +539,8 @@ dtc_tests () {
     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
+    check_tests unit-addr-leading-0x.dts unit_address_vs_reg
+    check_tests unit-addr-leading-0s.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-leading-0s.dts b/tests/unit-addr-leading-0s.dts
new file mode 100644
index 000000000000..7c8e2cebbc84
--- /dev/null
+++ b/tests/unit-addr-leading-0s.dts
@@ -0,0 +1,10 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	node@001 {
+		reg = <1 0>;
+	};
+};
diff --git a/tests/unit-addr-leading-0x.dts b/tests/unit-addr-leading-0x.dts
new file mode 100644
index 000000000000..7ed7254e8dc2
--- /dev/null
+++ b/tests/unit-addr-leading-0x.dts
@@ -0,0 +1,10 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	node@0x1 {
+		reg = <1 0>;
+	};
+};
-- 
2.10.1

--
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

[PATCH 4/5] checks: Add infrastructure for setting bus type of nodes

[Rob Herring]

In preparation to support bus specific checks, add the necessary
infrastructure to determine and set the bus type for nodes.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h    | 11 +++++++++++
 2 files changed, 62 insertions(+)

diff --git a/checks.c b/checks.c
index 8e310d69ca3d..5ef63a6a4317 100644
--- a/checks.c
+++ b/checks.c
@@ -587,6 +587,53 @@ static void fixup_path_references(struct check *c, struct dt_info *dti,
 }
 ERROR(path_references, fixup_path_references, NULL, &duplicate_node_names);
 
+struct bus_type *bus_types[] = {
+	NULL
+};
+
+static void fixup_bus_type(struct check *c, struct dt_info *dti,
+				  struct node *node)
+{
+	struct bus_type **bus;
+
+	for (bus = bus_types; *bus != NULL; bus++) {
+		if (!(*bus)->bridge_is_type(node))
+			continue;
+
+		node->bus_type = *bus;
+		break;
+	}
+}
+ERROR(bus_type, fixup_bus_type, NULL);
+
+static void check_bus_bridge(struct check *c, struct dt_info *dti,
+			     struct node *node)
+{
+	struct bus_type *bt;
+
+	if (!node->bus_type)
+		return;
+
+	bt = node->bus_type;
+	if (bt->check_bridge)
+		bt->check_bridge(c, dti, node);
+}
+WARNING(bus_bridge, check_bus_bridge, NULL);
+
+static void check_bus_device(struct check *c, struct dt_info *dti,
+			     struct node *node)
+{
+	struct bus_type *bt;
+
+	if (!node->parent || !node->parent->bus_type)
+		return;
+
+	bt = node->parent->bus_type;
+	if (bt->check_device)
+		bt->check_device(c, dti, node);
+}
+WARNING(bus_device, check_bus_device, NULL);
+
 /*
  * Semantic checks
  */
@@ -753,6 +800,7 @@ static struct check *check_table[] = {
 
 	&explicit_phandles,
 	&phandle_references, &path_references,
+	&bus_type,
 
 	&address_cells_is_cell, &size_cells_is_cell, &interrupt_cells_is_cell,
 	&device_type_is_string, &model_is_string, &status_is_string,
@@ -764,6 +812,9 @@ static struct check *check_table[] = {
 
 	&unit_address_vs_reg,
 
+	&bus_bridge,
+	&bus_device,
+
 	&avoid_default_addr_size,
 	&obsolete_chosen_interrupt_controller,
 
diff --git a/dtc.h b/dtc.h
index c6f125c68ba8..f27397cd7622 100644
--- a/dtc.h
+++ b/dtc.h
@@ -136,6 +136,16 @@ struct label {
 	struct label *next;
 };
 
+struct check;
+struct node;
+struct dt_info;
+
+struct bus_type {
+        bool (*bridge_is_type)(struct node *node);
+        void (*check_bridge)(struct check *c, struct dt_info *dti, struct node *node);
+        void (*check_device)(struct check *c, struct dt_info *dti, struct node *node);
+};
+
 struct property {
 	bool deleted;
 	char *name;
@@ -162,6 +172,7 @@ struct node {
 	int addr_cells, size_cells;
 
 	struct label *labels;
+	struct bus_type *bus_type;
 };
 
 #define for_each_label_withdel(l0, l) \
-- 
2.10.1

[PATCH 5/5] checks: Add bus checks for PCI buses

[Rob Herring]

Add PCI bridge and device node checks. We identify PCI bridges with
'device_type = "pci"' as only PCI bridges should set that property. For
bridges, we do a secondary check that the bridge has a ranges property
in cases of nodes incorrectly setting 'device_type = "pci"'.

For devices, the primary check is the reg property and the unit address.
Device unit addresses are in the form DD or DD,F where DD is the
device 0-0x1f and F is the function 0-7.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 86 insertions(+), 5 deletions(-)

diff --git a/checks.c b/checks.c
index 5ef63a6a4317..f5fcd625a19b 100644
--- a/checks.c
+++ b/checks.c
@@ -20,6 +20,11 @@
 
 #include "dtc.h"
 
+#define node_addr_cells(n) \
+	(((n)->addr_cells == -1) ? 2 : (n)->addr_cells)
+#define node_size_cells(n) \
+	(((n)->size_cells == -1) ? 1 : (n)->size_cells)
+
 #ifdef TRACE_CHECKS
 #define TRACE(c, ...) \
 	do { \
@@ -587,7 +592,88 @@ static void fixup_path_references(struct check *c, struct dt_info *dti,
 }
 ERROR(path_references, fixup_path_references, NULL, &duplicate_node_names);
 
+static bool is_pci_bridge(struct node *node)
+{
+	struct property *prop;
+
+	prop = get_property(node, "device_type");
+	if (!prop)
+		return false;
+
+	if (strcmp(prop->val.val, "pci") == 0)
+		return true;
+
+	return false;
+}
+
+static void pci_check_bridge(struct check *c, struct dt_info *dti, struct node *node)
+{
+	struct property *prop;
+
+	prop = get_property(node, "ranges");
+	if (!prop) {
+		FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)",
+			     node->fullpath);
+		return;
+	}
+
+	if (node_addr_cells(node) != 3)
+		FAIL(c, "Node %s incorrect #address-cells for PCI bridge",
+			     node->fullpath);
+	if (node_size_cells(node) != 2)
+		FAIL(c, "Node %s incorrect #size-cells for PCI bridge",
+			     node->fullpath);
+}
+
+static void pci_check_device(struct check *c, struct dt_info *dti, struct node *node)
+{
+	struct property *prop;
+	const char *unitname = get_unitname(node);
+	char unit_addr[5];
+	unsigned int dev, func, reg;
+
+	prop = get_property(node, "reg");
+	if (!prop)
+		return;
+
+	reg = fdt32_to_cpu(*((cell_t *)prop->val.val));
+
+	dev = (reg & 0xf800) >> 11;
+	func = (reg & 0x700) >> 8;
+
+	if (reg & 0xff000000)
+		FAIL(c, "Node %s PCI reg address is not configuration space",
+			     node->fullpath);
+
+	if (dev > 0x1f)
+		FAIL(c, "Node %s PCI device number out of range",
+			     node->fullpath);
+	if (func > 7)
+		FAIL(c, "Node %s PCI function number out of range",
+		     node->fullpath);
+
+	if (func == 0) {
+		snprintf(unit_addr, sizeof(unit_addr), "%x", dev);
+		if (!strcmp(unitname, unit_addr))
+			return;
+	}
+
+	snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func);
+	if (!strcmp(unitname, unit_addr))
+		return;
+
+	FAIL(c, "Node %s PCI unit address format error, expected \"%s\"",
+	     node->fullpath, unit_addr);
+}
+
+struct bus_type pci_bus_type = {
+        .bridge_is_type = is_pci_bridge,
+        .check_bridge = pci_check_bridge,
+        .check_device = pci_check_device,
+};
+
 struct bus_type *bus_types[] = {
+	&pci_bus_type,
 	NULL
 };
 
@@ -664,11 +750,6 @@ static void fixup_addr_size_cells(struct check *c, struct dt_info *dti,
 WARNING(addr_size_cells, fixup_addr_size_cells, NULL,
 	&address_cells_is_cell, &size_cells_is_cell);
 
-#define node_addr_cells(n) \
-	(((n)->addr_cells == -1) ? 2 : (n)->addr_cells)
-#define node_size_cells(n) \
-	(((n)->size_cells == -1) ? 1 : (n)->size_cells)
-
 static void check_reg_format(struct check *c, struct dt_info *dti,
 			     struct node *node)
 {
-- 
2.10.1

--
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: [PATCH 5/5] checks: Add bus checks for PCI buses

[Stephen Boyd]

Quoting Rob Herring (2017-01-24 09:45:34)
> @@ -587,7 +592,88 @@ static void fixup_path_references(struct check *c, struct dt_info *dti,
>  }
>  ERROR(path_references, fixup_path_references, NULL, &duplicate_node_names);
>  
> +static bool is_pci_bridge(struct node *node)
> +{
> +       struct property *prop;
> +
> +       prop = get_property(node, "device_type");
> +       if (!prop)
> +               return false;
> +
> +       if (strcmp(prop->val.val, "pci") == 0)
> +               return true;
> +
> +       return false;

This could be simplified?

	return strcmp(prop->val.val, "pci") == 0;

> +}
> +
[...]
> +
> +struct bus_type pci_bus_type = {

static? const?

> +        .bridge_is_type = is_pci_bridge,
> +        .check_bridge = pci_check_bridge,
> +        .check_device = pci_check_device,
> +};
> +
>  struct bus_type *bus_types[] = {

static? const?

> +       &pci_bus_type,
>         NULL
>  };
>  

Re: [PATCH 5/5] checks: Add bus checks for PCI buses

[Rob Herring]

On Wed, Jan 25, 2017 at 4:37 PM, Stephen Boyd <stephen.boyd-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Quoting Rob Herring (2017-01-24 09:45:34)
>> @@ -587,7 +592,88 @@ static void fixup_path_references(struct check *c, struct dt_info *dti,
>>  }
>>  ERROR(path_references, fixup_path_references, NULL, &duplicate_node_names);
>>
>> +static bool is_pci_bridge(struct node *node)
>> +{
>> +       struct property *prop;
>> +
>> +       prop = get_property(node, "device_type");
>> +       if (!prop)
>> +               return false;
>> +
>> +       if (strcmp(prop->val.val, "pci") == 0)
>> +               return true;
>> +
>> +       return false;
>
> This could be simplified?
>
>         return strcmp(prop->val.val, "pci") == 0;

Duh, OC!

>
>> +}
>> +
> [...]
>> +
>> +struct bus_type pci_bus_type = {
>
> static? const?
>
>> +        .bridge_is_type = is_pci_bridge,
>> +        .check_bridge = pci_check_bridge,
>> +        .check_device = pci_check_device,
>> +};
>> +
>>  struct bus_type *bus_types[] = {
>
> static? const?

Yes. Fixed these 2 and everywhere else.

Rob

Re: [PATCH 3/5] checks: Warn on node name unit-addresses with '0x' or leading 0s

[David Gibson]

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

On Tue, Jan 24, 2017 at 11:45:32AM -0600, Rob Herring wrote:
> Node name unit-addresses should never begin with 0x or leading 0s
> regardless of whether they have a bus specific address (i.e. one with
> commas) or not. Add warnings to check for these cases.
> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

I'd prefer to see this check split out on its own rather than folded
into unit_address_vs_reg.  Strictly speaking you could have a bus
binding which does specify unit addresses starting with leading 0 or
0x, although it's pretty unlikely.  Splitting this out will make it
easier to override this when we start having cases where we can really
check the unit address is correct versus the bus binding.

> ---
>  checks.c                       | 10 ++++++++++
>  tests/run_tests.sh             |  2 ++
>  tests/unit-addr-leading-0s.dts | 10 ++++++++++
>  tests/unit-addr-leading-0x.dts | 10 ++++++++++
>  4 files changed, 32 insertions(+)
>  create mode 100644 tests/unit-addr-leading-0s.dts
>  create mode 100644 tests/unit-addr-leading-0x.dts
> 
> diff --git a/checks.c b/checks.c
> index 0c78d69316bc..8e310d69ca3d 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -288,6 +288,16 @@ static void check_unit_address_vs_reg(struct check *c, struct dt_info *dti,
>  		if (!unitname[0])
>  			FAIL(c, "Node %s has a reg or ranges property, but no unit name",
>  			    node->fullpath);
> +
> +		if (!strncmp(unitname, "0x", 2)) {
> +			FAIL(c, "Node %s unit name should not have leading \"0x\"",
> +			    node->fullpath);
> +			/* skip over 0x for next test */
> +			unitname += 2;
> +		}
> +		if (unitname[0] == '0' && isxdigit(unitname[1]))
> +			FAIL(c, "Node %s unit name should not have leading 0s",
> +			    node->fullpath);
>  	} else {
>  		if (unitname[0])
>  			FAIL(c, "Node %s has a unit name, but no reg property",
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 157dbaea7600..2c06666a9e23 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -539,6 +539,8 @@ dtc_tests () {
>      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
> +    check_tests unit-addr-leading-0x.dts unit_address_vs_reg
> +    check_tests unit-addr-leading-0s.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-leading-0s.dts b/tests/unit-addr-leading-0s.dts
> new file mode 100644
> index 000000000000..7c8e2cebbc84
> --- /dev/null
> +++ b/tests/unit-addr-leading-0s.dts
> @@ -0,0 +1,10 @@
> +/dts-v1/;
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	node@001 {
> +		reg = <1 0>;
> +	};
> +};
> diff --git a/tests/unit-addr-leading-0x.dts b/tests/unit-addr-leading-0x.dts
> new file mode 100644
> index 000000000000..7ed7254e8dc2
> --- /dev/null
> +++ b/tests/unit-addr-leading-0x.dts
> @@ -0,0 +1,10 @@
> +/dts-v1/;
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	node@0x1 {
> +		reg = <1 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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/5] checks: Add infrastructure for setting bus type of nodes

[David Gibson]

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

On Tue, Jan 24, 2017 at 11:45:33AM -0600, Rob Herring wrote:
> In preparation to support bus specific checks, add the necessary
> infrastructure to determine and set the bus type for nodes.
> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  checks.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  dtc.h    | 11 +++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/checks.c b/checks.c
> index 8e310d69ca3d..5ef63a6a4317 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -587,6 +587,53 @@ static void fixup_path_references(struct check *c, struct dt_info *dti,
>  }
>  ERROR(path_references, fixup_path_references, NULL, &duplicate_node_names);
>  
> +struct bus_type *bus_types[] = {
> +	NULL
> +};
> +
> +static void fixup_bus_type(struct check *c, struct dt_info *dti,
> +				  struct node *node)
> +{
> +	struct bus_type **bus;
> +
> +	for (bus = bus_types; *bus != NULL; bus++) {

I think this would be clearer using ARRAY_SIZE() rather than the **
and NULL terminator.

> +		if (!(*bus)->bridge_is_type(node))
> +			continue;
> +
> +		node->bus_type = *bus;
> +		break;
> +	}
> +}
> +ERROR(bus_type, fixup_bus_type, NULL);
> +
> +static void check_bus_bridge(struct check *c, struct dt_info *dti,
> +			     struct node *node)
> +{
> +	struct bus_type *bt;
> +
> +	if (!node->bus_type)
> +		return;
> +
> +	bt = node->bus_type;
> +	if (bt->check_bridge)
> +		bt->check_bridge(c, dti, node);
> +}
> +WARNING(bus_bridge, check_bus_bridge, NULL);

Hrm.  So, we have a double multiplex here, and I'm not sure that it's
necessary.  First the table of checks themselves, then the table of
bus types.  Could we eliminate that simply by having each bus type
implement a check function which tests for that bus type then,
performans any checks for the bridge then sets the bus_type field.

It would mean that if there are multiple bus-specific things we want
to check about the bridge, we could put them into separate checks.
That might be clearer in some cases, and it means our existing
messages can show something informative via the check name which
failed, rather than just saying "(bus_bridge)" for anything wrong with
any bus bridge.

We could still use the bus_type field for "semi-generic" checks like
validating unit names that are the same in outline between different
buses, but differ in details.

> +static void check_bus_device(struct check *c, struct dt_info *dti,
> +			     struct node *node)
> +{
> +	struct bus_type *bt;
> +
> +	if (!node->parent || !node->parent->bus_type)
> +		return;
> +
> +	bt = node->parent->bus_type;
> +	if (bt->check_device)
> +		bt->check_device(c, dti, node);
> +}
> +WARNING(bus_device, check_bus_device, NULL);

Removing the double indirection would be a bit fiddlier for the bus
devices, but the same advantages apply.

>  /*
>   * Semantic checks
>   */
> @@ -753,6 +800,7 @@ static struct check *check_table[] = {
>  
>  	&explicit_phandles,
>  	&phandle_references, &path_references,
> +	&bus_type,
>  
>  	&address_cells_is_cell, &size_cells_is_cell, &interrupt_cells_is_cell,
>  	&device_type_is_string, &model_is_string, &status_is_string,
> @@ -764,6 +812,9 @@ static struct check *check_table[] = {
>  
>  	&unit_address_vs_reg,
>  
> +	&bus_bridge,
> +	&bus_device,
> +
>  	&avoid_default_addr_size,
>  	&obsolete_chosen_interrupt_controller,
>  
> diff --git a/dtc.h b/dtc.h
> index c6f125c68ba8..f27397cd7622 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -136,6 +136,16 @@ struct label {
>  	struct label *next;
>  };
>  
> +struct check;
> +struct node;
> +struct dt_info;
> +
> +struct bus_type {
> +        bool (*bridge_is_type)(struct node *node);
> +        void (*check_bridge)(struct check *c, struct dt_info *dti, struct node *node);
> +        void (*check_device)(struct check *c, struct dt_info *dti, struct node *node);
> +};
> +
>  struct property {
>  	bool deleted;
>  	char *name;
> @@ -162,6 +172,7 @@ struct node {
>  	int addr_cells, size_cells;
>  
>  	struct label *labels;
> +	struct bus_type *bus_type;
>  };
>  
>  #define for_each_label_withdel(l0, l) \

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/5] checks: Add Warning for stricter property name character checking

[David Gibson]

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

On Tue, Jan 24, 2017 at 11:45:30AM -0600, Rob Herring wrote:
> While '?', '.', '+', '*', and '_' are considered valid characters their
> use is discouraged in recommended practices. '#' is also only
> recommended to be used at the beginning of property names.

Hrm.  I'm not totally convinced about this.  PAPR definitely specifies
a few properties that include '#' not at the beginning.  I'm not sure
that IEEE1275 doesn't as well (it's not exactly easy to grep for a
single character).

I'm not sure about '?' either.  I didn't positively identify any
properties including it, but there are certainly OF configuration
variables specified by PAPR including '?', and I think those will be
exposed in the device tree as properties.

I'm not 100% sure about '*' either, I have a vague memory of some spec
that requires it, but I'm not certain.

AFAIK, excluding '.', '+' and '_' should be ok, except for device_type
which you already have special exception for.

> Testing this found one typo error with '.' used instead of ','. The
> rest of the warnings were all from underscores.
> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  checks.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/checks.c b/checks.c
> index 3d18e45374c8..a0d4a9d968d7 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -239,6 +239,7 @@ ERROR(duplicate_property_names, check_duplicate_property_names, NULL);
>  #define UPPERCASE	"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
>  #define DIGITS		"0123456789"
>  #define PROPNODECHARS	LOWERCASE UPPERCASE DIGITS ",._+*#?-"
> +#define PROPNODECHARSSTRICT	LOWERCASE UPPERCASE DIGITS ",-"
>  
>  static void check_node_name_chars(struct check *c, struct dt_info *dti,
>  				  struct node *node)
> @@ -299,6 +300,38 @@ static void check_property_name_chars(struct check *c, struct dt_info *dti,
>  }
>  ERROR(property_name_chars, check_property_name_chars, PROPNODECHARS);
>  
> +static void check_property_name_chars_strict(struct check *c,
> +					     struct dt_info *dti,
> +					     struct node *node)
> +{
> +	struct property *prop;
> +
> +	for_each_property(node, prop) {
> +		const char *name = prop->name;
> +		int n = strspn(name, c->data);
> +
> +		if (n == strlen(prop->name))
> +			continue;
> +
> +		/* Certain names are whitelisted */
> +		if (strcmp(name, "device_type") == 0)
> +			continue;
> +
> +		/*
> +		 * # is only allowed at the beginning of property names not counting
> +		 * the vendor prefix.
> +		 */
> +		if (name[n] == '#' && ((n == 0) || (name[n-1] == ','))) {
> +			name += n + 1;
> +			n = strspn(name, c->data);
> +		}
> +		if (n < strlen(name))
> +			FAIL(c, "Character '%c' not recommended in property name \"%s\", node %s",
> +			     name[n], prop->name, node->fullpath);
> +	}
> +}
> +WARNING(property_name_chars_strict, check_property_name_chars_strict, PROPNODECHARSSTRICT);
> +
>  #define DESCLABEL_FMT	"%s%s%s%s%s"
>  #define DESCLABEL_ARGS(node,prop,mark)		\
>  	((mark) ? "value of " : ""),		\
> @@ -703,6 +736,8 @@ static struct check *check_table[] = {
>  	&address_cells_is_cell, &size_cells_is_cell, &interrupt_cells_is_cell,
>  	&device_type_is_string, &model_is_string, &status_is_string,
>  
> +	&property_name_chars_strict,
> +
>  	&addr_size_cells, &reg_format, &ranges_format,
>  
>  	&unit_address_vs_reg,

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/5] checks: Add Warning for stricter node name character checking

[David Gibson]

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

On Tue, Jan 24, 2017 at 11:45:31AM -0600, Rob Herring wrote:
> While '#', '?', '.', '+', '*', and '_' are considered valid characters,
> their use is discouraged in recommended practices.
> 
> Testing this found a few cases of '.'. The majority of the warnings were
> all from underscores.

Hmm.  The Opal firmware on POWER8 machines uses both '.' and '#' in
node names in some places.  So I'm not terribly convinced this is a
good idea.

> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  checks.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/checks.c b/checks.c
> index a0d4a9d968d7..0c78d69316bc 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -252,6 +252,17 @@ static void check_node_name_chars(struct check *c, struct dt_info *dti,
>  }
>  ERROR(node_name_chars, check_node_name_chars, PROPNODECHARS "@");
>  
> +static void check_node_name_chars_strict(struct check *c, struct dt_info *dti,
> +					 struct node *node)
> +{
> +	int n = strspn(node->name, c->data);
> +
> +	if (n < node->basenamelen)
> +		FAIL(c, "Character '%c' not recommended in node %s",
> +		     node->name[n], node->fullpath);
> +}
> +WARNING(node_name_chars_strict, check_node_name_chars_strict, PROPNODECHARSSTRICT);
> +
>  static void check_node_name_format(struct check *c, struct dt_info *dti,
>  				   struct node *node)
>  {
> @@ -737,6 +748,7 @@ static struct check *check_table[] = {
>  	&device_type_is_string, &model_is_string, &status_is_string,
>  
>  	&property_name_chars_strict,
> +	&node_name_chars_strict,
>  
>  	&addr_size_cells, &reg_format, &ranges_format,
>  

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/5] checks: Add Warning for stricter node name character checking

[Rob Herring]

On Mon, Jan 30, 2017 at 9:14 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Tue, Jan 24, 2017 at 11:45:31AM -0600, Rob Herring wrote:
>> While '#', '?', '.', '+', '*', and '_' are considered valid characters,
>> their use is discouraged in recommended practices.
>>
>> Testing this found a few cases of '.'. The majority of the warnings were
>> all from underscores.
>
> Hmm.  The Opal firmware on POWER8 machines uses both '.' and '#' in
> node names in some places.  So I'm not terribly convinced this is a
> good idea.

When would it be using a flat tree and dtc? I know the DT is more
dynamic on these systems, but it would be nice if a dts was published
as reference.

These checks are more something I want to discourage new uses of, not
necessarily fix existing cases. I've thought about making the warnings
be levels rather than true/false. Or perhaps something like -Wall.
Then these checks can be off by default. The kernel build is doing
that already for the unit address checks.

Rob
--
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: [PATCH 4/5] checks: Add infrastructure for setting bus type of nodes

[Rob Herring]

On Tue, Jan 31, 2017 at 11:26:34AM +1100, David Gibson wrote:
> On Tue, Jan 24, 2017 at 11:45:33AM -0600, Rob Herring wrote:
> > In preparation to support bus specific checks, add the necessary
> > infrastructure to determine and set the bus type for nodes.
> > 
> > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> >  checks.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  dtc.h    | 11 +++++++++++
> >  2 files changed, 62 insertions(+)

[...]

> > +static void check_bus_bridge(struct check *c, struct dt_info *dti,
> > +			     struct node *node)
> > +{
> > +	struct bus_type *bt;
> > +
> > +	if (!node->bus_type)
> > +		return;
> > +
> > +	bt = node->bus_type;
> > +	if (bt->check_bridge)
> > +		bt->check_bridge(c, dti, node);
> > +}
> > +WARNING(bus_bridge, check_bus_bridge, NULL);
> 
> Hrm.  So, we have a double multiplex here, and I'm not sure that it's
> necessary.  First the table of checks themselves, then the table of
> bus types.  Could we eliminate that simply by having each bus type
> implement a check function which tests for that bus type then,
> performans any checks for the bridge then sets the bus_type field.
> 
> It would mean that if there are multiple bus-specific things we want
> to check about the bridge, we could put them into separate checks.
> That might be clearer in some cases, and it means our existing
> messages can show something informative via the check name which
> failed, rather than just saying "(bus_bridge)" for anything wrong with
> any bus bridge.
> 
> We could still use the bus_type field for "semi-generic" checks like
> validating unit names that are the same in outline between different
> buses, but differ in details.

Something like this what you have in mind? If we have additional checks 
to add, we can just use the existing prerq functionality.

8<--------------------------------------------------------------------------
>From 5cea821e6078f84eaae5af317651a4a696d1f0f7 Mon Sep 17 00:00:00 2001
From: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Date: Wed, 1 Feb 2017 15:43:32 -0600
Subject: [PATCH v2] checks: Add bus checks for PCI buses

Add PCI bridge and device node checks. We identify PCI bridges with
'device_type = "pci"' as only PCI bridges should set that property. For
bridges, check that ranges is present and #address-cells and

For devices, the primary check is the reg property and the unit address.
Device unit addresses are in the form DD or DD,F where DD is the
device 0-0x1f and F is the function 0-7.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h    |  4 +++-
 2 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/checks.c b/checks.c
index 225ace39c698..b727c49d457c 100644
--- a/checks.c
+++ b/checks.c
@@ -702,6 +702,77 @@ static void check_ranges_format(struct check *c, struct dt_info *dti,
 }
 WARNING(ranges_format, check_ranges_format, NULL, &addr_size_cells);
 
+static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node)
+{
+	struct property *prop;
+
+	prop = get_property(node, "device_type");
+	if (!prop || strcmp(prop->val.val, "pci"))
+		return;
+
+	node->bus_type = PCI_BUS_TYPE;
+
+	prop = get_property(node, "ranges");
+	if (!prop) {
+		FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)",
+			     node->fullpath);
+		return;
+	}
+
+	if (node_addr_cells(node) != 3)
+		FAIL(c, "Node %s incorrect #address-cells for PCI bridge",
+			     node->fullpath);
+	if (node_size_cells(node) != 2)
+		FAIL(c, "Node %s incorrect #size-cells for PCI bridge",
+			     node->fullpath);
+}
+WARNING(pci_bridge, check_pci_bridge, NULL, &device_type_is_string,&addr_size_cells);
+
+static void check_pci_device(struct check *c, struct dt_info *dti, struct node *node)
+{
+	struct property *prop;
+	const char *unitname = get_unitname(node);
+	char unit_addr[5];
+	unsigned int dev, func, reg;
+
+	if (!node->parent || (node->parent->bus_type != PCI_BUS_TYPE))
+		return;
+
+	prop = get_property(node, "reg");
+	if (!prop)
+		return;
+
+	reg = fdt32_to_cpu(*((cell_t *)prop->val.val));
+
+	dev = (reg & 0xf800) >> 11;
+	func = (reg & 0x700) >> 8;
+
+	if (reg & 0xff000000)
+		FAIL(c, "Node %s PCI reg address is not configuration space",
+			     node->fullpath);
+
+	if (dev > 0x1f)
+		FAIL(c, "Node %s PCI device number out of range",
+			     node->fullpath);
+	if (func > 7)
+		FAIL(c, "Node %s PCI function number out of range",
+		     node->fullpath);
+
+	if (func == 0) {
+		snprintf(unit_addr, sizeof(unit_addr), "%x", dev);
+		if (!strcmp(unitname, unit_addr))
+			return;
+	}
+
+	snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func);
+	if (!strcmp(unitname, unit_addr))
+		return;
+
+	FAIL(c, "Node %s PCI unit address format error, expected \"%s\"",
+	     node->fullpath, unit_addr);
+}
+WARNING(pci_device, check_pci_device, NULL, &reg_format);
+
 /*
  * Style checks
  */
@@ -775,6 +846,9 @@ static struct check *check_table[] = {
 	&unit_address_vs_reg,
 	&unit_address_format,
 
+	&pci_bridge,
+	&pci_device,
+
 	&avoid_default_addr_size,
 	&obsolete_chosen_interrupt_controller,
 
diff --git a/dtc.h b/dtc.h
index c6f125c68ba8..03c249c65101 100644
--- a/dtc.h
+++ b/dtc.h
@@ -146,6 +146,8 @@ struct property {
 	struct label *labels;
 };
 
+#define PCI_BUS_TYPE	1
+
 struct node {
 	bool deleted;
 	char *name;
@@ -159,7 +161,7 @@ struct node {
 	int basenamelen;
 
 	cell_t phandle;
-	int addr_cells, size_cells;
+	int addr_cells, size_cells, bus_type;
 
 	struct label *labels;
 };
-- 
2.10.1

Re: [PATCH 4/5] checks: Add infrastructure for setting bus type of nodes

[Rob Herring]

On Tue, Jan 31, 2017 at 11:26:34AM +1100, David Gibson wrote:
> On Tue, Jan 24, 2017 at 11:45:33AM -0600, Rob Herring wrote:
> > In preparation to support bus specific checks, add the necessary
> > infrastructure to determine and set the bus type for nodes.
> > 
> > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> >  checks.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  dtc.h    | 11 +++++++++++
> >  2 files changed, 62 insertions(+)

[...]

> > +static void check_bus_bridge(struct check *c, struct dt_info *dti,
> > +			     struct node *node)
> > +{
> > +	struct bus_type *bt;
> > +
> > +	if (!node->bus_type)
> > +		return;
> > +
> > +	bt = node->bus_type;
> > +	if (bt->check_bridge)
> > +		bt->check_bridge(c, dti, node);
> > +}
> > +WARNING(bus_bridge, check_bus_bridge, NULL);
> 
> Hrm.  So, we have a double multiplex here, and I'm not sure that it's
> necessary.  First the table of checks themselves, then the table of
> bus types.  Could we eliminate that simply by having each bus type
> implement a check function which tests for that bus type then,
> performans any checks for the bridge then sets the bus_type field.
> 
> It would mean that if there are multiple bus-specific things we want
> to check about the bridge, we could put them into separate checks.
> That might be clearer in some cases, and it means our existing
> messages can show something informative via the check name which
> failed, rather than just saying "(bus_bridge)" for anything wrong with
> any bus bridge.
> 
> We could still use the bus_type field for "semi-generic" checks like
> validating unit names that are the same in outline between different
> buses, but differ in details.

Something like this what you have in mind? If we have additional checks 
to add, we can just use the existing prerq functionality.

8<--------------------------------------------------------------------------
From 5cea821e6078f84eaae5af317651a4a696d1f0f7 Mon Sep 17 00:00:00 2001
From: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Date: Wed, 1 Feb 2017 15:43:32 -0600
Subject: [PATCH v2] checks: Add bus checks for PCI buses

Add PCI bridge and device node checks. We identify PCI bridges with
'device_type = "pci"' as only PCI bridges should set that property. For
bridges, check that ranges is present and #address-cells and

For devices, the primary check is the reg property and the unit address.
Device unit addresses are in the form DD or DD,F where DD is the
device 0-0x1f and F is the function 0-7.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h    |  4 +++-
 2 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/checks.c b/checks.c
index 225ace39c698..b727c49d457c 100644
--- a/checks.c
+++ b/checks.c
@@ -702,6 +702,77 @@ static void check_ranges_format(struct check *c, struct dt_info *dti,
 }
 WARNING(ranges_format, check_ranges_format, NULL, &addr_size_cells);
 
+static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node)
+{
+	struct property *prop;
+
+	prop = get_property(node, "device_type");
+	if (!prop || strcmp(prop->val.val, "pci"))
+		return;
+
+	node->bus_type = PCI_BUS_TYPE;
+
+	prop = get_property(node, "ranges");
+	if (!prop) {
+		FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)",
+			     node->fullpath);
+		return;
+	}
+
+	if (node_addr_cells(node) != 3)
+		FAIL(c, "Node %s incorrect #address-cells for PCI bridge",
+			     node->fullpath);
+	if (node_size_cells(node) != 2)
+		FAIL(c, "Node %s incorrect #size-cells for PCI bridge",
+			     node->fullpath);
+}
+WARNING(pci_bridge, check_pci_bridge, NULL, &device_type_is_string,&addr_size_cells);
+
+static void check_pci_device(struct check *c, struct dt_info *dti, struct node *node)
+{
+	struct property *prop;
+	const char *unitname = get_unitname(node);
+	char unit_addr[5];
+	unsigned int dev, func, reg;
+
+	if (!node->parent || (node->parent->bus_type != PCI_BUS_TYPE))
+		return;
+
+	prop = get_property(node, "reg");
+	if (!prop)
+		return;
+
+	reg = fdt32_to_cpu(*((cell_t *)prop->val.val));
+
+	dev = (reg & 0xf800) >> 11;
+	func = (reg & 0x700) >> 8;
+
+	if (reg & 0xff000000)
+		FAIL(c, "Node %s PCI reg address is not configuration space",
+			     node->fullpath);
+
+	if (dev > 0x1f)
+		FAIL(c, "Node %s PCI device number out of range",
+			     node->fullpath);
+	if (func > 7)
+		FAIL(c, "Node %s PCI function number out of range",
+		     node->fullpath);
+
+	if (func == 0) {
+		snprintf(unit_addr, sizeof(unit_addr), "%x", dev);
+		if (!strcmp(unitname, unit_addr))
+			return;
+	}
+
+	snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func);
+	if (!strcmp(unitname, unit_addr))
+		return;
+
+	FAIL(c, "Node %s PCI unit address format error, expected \"%s\"",
+	     node->fullpath, unit_addr);
+}
+WARNING(pci_device, check_pci_device, NULL, &reg_format);
+
 /*
  * Style checks
  */
@@ -775,6 +846,9 @@ static struct check *check_table[] = {
 	&unit_address_vs_reg,
 	&unit_address_format,
 
+	&pci_bridge,
+	&pci_device,
+
 	&avoid_default_addr_size,
 	&obsolete_chosen_interrupt_controller,
 
diff --git a/dtc.h b/dtc.h
index c6f125c68ba8..03c249c65101 100644
--- a/dtc.h
+++ b/dtc.h
@@ -146,6 +146,8 @@ struct property {
 	struct label *labels;
 };
 
+#define PCI_BUS_TYPE	1
+
 struct node {
 	bool deleted;
 	char *name;
@@ -159,7 +161,7 @@ struct node {
 	int basenamelen;
 
 	cell_t phandle;
-	int addr_cells, size_cells;
+	int addr_cells, size_cells, bus_type;
 
 	struct label *labels;
 };
-- 
2.10.1

Re: [PATCH 4/5] checks: Add infrastructure for setting bus type of nodes

[David Gibson]

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

On Wed, Feb 01, 2017 at 03:54:31PM -0600, Rob Herring wrote:
> On Tue, Jan 31, 2017 at 11:26:34AM +1100, David Gibson wrote:
> > On Tue, Jan 24, 2017 at 11:45:33AM -0600, Rob Herring wrote:
> > > In preparation to support bus specific checks, add the necessary
> > > infrastructure to determine and set the bus type for nodes.
> > > 
> > > Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > ---
> > >  checks.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  dtc.h    | 11 +++++++++++
> > >  2 files changed, 62 insertions(+)
> 
> [...]
> 
> > > +static void check_bus_bridge(struct check *c, struct dt_info *dti,
> > > +			     struct node *node)
> > > +{
> > > +	struct bus_type *bt;
> > > +
> > > +	if (!node->bus_type)
> > > +		return;
> > > +
> > > +	bt = node->bus_type;
> > > +	if (bt->check_bridge)
> > > +		bt->check_bridge(c, dti, node);
> > > +}
> > > +WARNING(bus_bridge, check_bus_bridge, NULL);
> > 
> > Hrm.  So, we have a double multiplex here, and I'm not sure that it's
> > necessary.  First the table of checks themselves, then the table of
> > bus types.  Could we eliminate that simply by having each bus type
> > implement a check function which tests for that bus type then,
> > performans any checks for the bridge then sets the bus_type field.
> > 
> > It would mean that if there are multiple bus-specific things we want
> > to check about the bridge, we could put them into separate checks.
> > That might be clearer in some cases, and it means our existing
> > messages can show something informative via the check name which
> > failed, rather than just saying "(bus_bridge)" for anything wrong with
> > any bus bridge.
> > 
> > We could still use the bus_type field for "semi-generic" checks like
> > validating unit names that are the same in outline between different
> > buses, but differ in details.
> 
> Something like this what you have in mind? If we have additional checks 
> to add, we can just use the existing prerq functionality.

This looks like the right basic idea, yes.

I'd still suggest an actual structure for the bus type variable,
rather than just an integer / enum.  We may not have use for method
pointers in there now, but I think we could well benefit from them in
future.

> 
> 8<--------------------------------------------------------------------------
> >From 5cea821e6078f84eaae5af317651a4a696d1f0f7 Mon Sep 17 00:00:00 2001
> From: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Date: Wed, 1 Feb 2017 15:43:32 -0600
> Subject: [PATCH v2] checks: Add bus checks for PCI buses
> 
> Add PCI bridge and device node checks. We identify PCI bridges with
> 'device_type = "pci"' as only PCI bridges should set that property. For
> bridges, check that ranges is present and #address-cells and
> 
> For devices, the primary check is the reg property and the unit address.
> Device unit addresses are in the form DD or DD,F where DD is the
> device 0-0x1f and F is the function 0-7.
> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  checks.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  dtc.h    |  4 +++-
>  2 files changed, 77 insertions(+), 1 deletion(-)
> 
> diff --git a/checks.c b/checks.c
> index 225ace39c698..b727c49d457c 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -702,6 +702,77 @@ static void check_ranges_format(struct check *c, struct dt_info *dti,
>  }
>  WARNING(ranges_format, check_ranges_format, NULL, &addr_size_cells);
>  
> +static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node)
> +{
> +	struct property *prop;
> +
> +	prop = get_property(node, "device_type");
> +	if (!prop || strcmp(prop->val.val, "pci"))
> +		return;
> +
> +	node->bus_type = PCI_BUS_TYPE;
> +
> +	prop = get_property(node, "ranges");
> +	if (!prop) {
> +		FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)",
> +			     node->fullpath);
> +		return;
> +	}
> +
> +	if (node_addr_cells(node) != 3)
> +		FAIL(c, "Node %s incorrect #address-cells for PCI bridge",
> +			     node->fullpath);
> +	if (node_size_cells(node) != 2)
> +		FAIL(c, "Node %s incorrect #size-cells for PCI bridge",
> +			     node->fullpath);
> +}
> +WARNING(pci_bridge, check_pci_bridge, NULL, &device_type_is_string,&addr_size_cells);
> +
> +static void check_pci_device(struct check *c, struct dt_info *dti, struct node *node)
> +{
> +	struct property *prop;
> +	const char *unitname = get_unitname(node);
> +	char unit_addr[5];
> +	unsigned int dev, func, reg;
> +
> +	if (!node->parent || (node->parent->bus_type != PCI_BUS_TYPE))
> +		return;
> +
> +	prop = get_property(node, "reg");
> +	if (!prop)
> +		return;
> +
> +	reg = fdt32_to_cpu(*((cell_t *)prop->val.val));
> +
> +	dev = (reg & 0xf800) >> 11;
> +	func = (reg & 0x700) >> 8;
> +
> +	if (reg & 0xff000000)
> +		FAIL(c, "Node %s PCI reg address is not configuration space",
> +			     node->fullpath);
> +
> +	if (dev > 0x1f)
> +		FAIL(c, "Node %s PCI device number out of range",
> +			     node->fullpath);
> +	if (func > 7)
> +		FAIL(c, "Node %s PCI function number out of range",
> +		     node->fullpath);
> +
> +	if (func == 0) {
> +		snprintf(unit_addr, sizeof(unit_addr), "%x", dev);
> +		if (!strcmp(unitname, unit_addr))
> +			return;
> +	}
> +
> +	snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func);
> +	if (!strcmp(unitname, unit_addr))
> +		return;
> +
> +	FAIL(c, "Node %s PCI unit address format error, expected \"%s\"",
> +	     node->fullpath, unit_addr);
> +}
> +WARNING(pci_device, check_pci_device, NULL, &reg_format);
> +
>  /*
>   * Style checks
>   */
> @@ -775,6 +846,9 @@ static struct check *check_table[] = {
>  	&unit_address_vs_reg,
>  	&unit_address_format,
>  
> +	&pci_bridge,
> +	&pci_device,
> +
>  	&avoid_default_addr_size,
>  	&obsolete_chosen_interrupt_controller,
>  
> diff --git a/dtc.h b/dtc.h
> index c6f125c68ba8..03c249c65101 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -146,6 +146,8 @@ struct property {
>  	struct label *labels;
>  };
>  
> +#define PCI_BUS_TYPE	1
> +
>  struct node {
>  	bool deleted;
>  	char *name;
> @@ -159,7 +161,7 @@ struct node {
>  	int basenamelen;
>  
>  	cell_t phandle;
> -	int addr_cells, size_cells;
> +	int addr_cells, size_cells, bus_type;
>  
>  	struct label *labels;
>  };

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]