[RFC 1/3] checks: Add infrastructure for setting bus type of nodes

[RFC 1/3] checks: Add infrastructure for setting bus type of nodes

[Rob Herring]

In preparation to support bus specific checks, add the necessary
infrastructure to determine the bus type for nodes. Initially, PCI and
simple bus are supported.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
David,

Hopefully this matches what you had in mind for bus checks.

Rob

 checks.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h    | 11 +++++++++
 2 files changed, 90 insertions(+)

diff --git a/checks.c b/checks.c
index 386f956..48e926e 100644
--- a/checks.c
+++ b/checks.c
@@ -527,6 +527,84 @@ static void fixup_path_references(struct check *c, struct node *dt,
 ERROR(path_references, NULL, NULL, fixup_path_references, NULL,
       &duplicate_node_names);
 
+static bool is_pci_bridge(struct node *node)
+{
+	struct property *prop;
+
+	if (!node)
+		return false;
+
+	prop = get_property(node, "device_type");
+	if (!prop)
+		return false;
+
+	if (strcmp(prop->val.val, "pci") == 0)
+		return true;
+
+	return false;
+}
+
+struct bus_type pci_bus_type = {
+        .expected_addr_cells = 3,
+        .expected_size_cells = 2,
+        .is_type = is_pci_bridge,
+};
+
+static bool is_simple_bridge(struct node *node)
+{
+	struct property *prop;
+	int len = 0;
+
+	if (!node)
+		return false;
+
+	/* root node is special case defaulting to simple-bus */
+	if (!node->parent)
+		return true;
+
+	prop = get_property(node, "compatible");
+	if (!prop)
+		return false;
+
+	do {
+		const char *str = prop->val.val;
+
+		if (strcmp(str, "simple-bus") == 0)
+			return true;
+		len += strlen(str) + 1;
+		str += len;
+	} while (len < prop->val.len);
+
+	return false;
+}
+
+struct bus_type simple_bus_type = {
+	.expected_addr_cells = -1, /* For don't care */
+	.expected_size_cells = -1,
+	.is_type = is_simple_bridge,
+};
+
+struct bus_type *bus_types[] = {
+	&pci_bus_type,
+	&simple_bus_type,
+	NULL
+};
+
+static void fixup_bus_type(struct check *c, struct node *dt,
+				  struct node *node)
+{
+	struct bus_type **bus;
+
+	for (bus = bus_types; *bus != NULL; bus++) {
+		if (!(*bus)->is_type(node))
+			continue;
+
+		node->bus_type = *bus;
+		break;
+	}
+}
+ERROR(bus_type, NULL, fixup_bus_type, NULL, NULL);
+
 /*
  * Semantic checks
  */
@@ -685,6 +763,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,
diff --git a/dtc.h b/dtc.h
index 56212c8..c1a1fe9 100644
--- a/dtc.h
+++ b/dtc.h
@@ -132,6 +132,16 @@ struct label {
 	struct label *next;
 };
 
+struct check;
+struct node;
+
+struct bus_type {
+        int expected_addr_cells;
+        int expected_size_cells;
+        bool (*is_type)(struct node *node);
+        void (*check_unit_addr)(struct check *c, struct node *dt, struct node *node);
+};
+
 struct property {
 	bool deleted;
 	char *name;
@@ -158,6 +168,7 @@ struct node {
 	int addr_cells, size_cells;
 
 	struct label *labels;
+	struct bus_type *bus_type;
 };
 
 #define for_each_label_withdel(l0, l) \
-- 
2.5.0

[RFC 2/3] checks: Add unit-address checks for simple-bus and default

[Rob Herring]

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c                                    | 87 +++++++++++++++++++++++++++--
 tests/run_tests.sh                          |  4 ++
 tests/unit-addr-leading-0s.dts              | 10 ++++
 tests/unit-addr-leading-0x.dts              | 10 ++++
 tests/unit-addr-simple-bus-comma.dts        | 18 ++++++
 tests/unit-addr-simple-bus-reg-mismatch.dts | 18 ++++++
 6 files changed, 142 insertions(+), 5 deletions(-)
 create mode 100644 tests/unit-addr-leading-0s.dts
 create mode 100644 tests/unit-addr-leading-0x.dts
 create mode 100644 tests/unit-addr-simple-bus-comma.dts
 create mode 100644 tests/unit-addr-simple-bus-reg-mismatch.dts

diff --git a/checks.c b/checks.c
index 48e926e..82a7f38 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 { \
@@ -578,12 +583,88 @@ static bool is_simple_bridge(struct node *node)
 	return false;
 }
 
+static void default_unit_addr(struct check *c, struct node *dt, struct node *node)
+{
+	const char *unitname = get_unitname(node);
+
+	if (strstr(unitname, "0x") == unitname) {
+		FAIL(c, "Node %s unit address 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 address should not have leading 0s",
+		     node->fullpath);
+}
+
+static void simple_bus_unit_addr(struct check *c, struct node *dt, struct node *node)
+{
+	const char *unitname = get_unitname(node);
+	struct property *prop;
+	uint64_t unitaddr, regaddr = 0;
+	int n, addr_cells;
+	cell_t *cell;
+
+	default_unit_addr(c, dt, node);
+
+	n = strspn(unitname, DIGITS "abcedf");
+	if (n != strlen(unitname))
+		FAIL(c, "Node %s unit address should have only lower case hex digits",
+		     node->fullpath);
+
+	unitaddr = strtoll(unitname, NULL, 16);
+
+	prop = get_property(node, "reg");
+	if (!prop) {
+		prop = get_property(node, "ranges");
+		if (!prop || !prop->val.len)
+			return;
+
+		cell = (cell_t *)prop->val.val;
+		cell += node_addr_cells(node);
+	} else
+		cell = (cell_t *)prop->val.val;
+
+	addr_cells = node_addr_cells(node->parent);
+	while (addr_cells--)
+		regaddr = (regaddr << 32) | fdt32_to_cpu(*cell++);
+
+	if (regaddr != unitaddr)
+		FAIL(c, "Node %s unit address does not match reg address (%zx != %zx)",
+		     node->fullpath, regaddr, unitaddr);
+}
+
 struct bus_type simple_bus_type = {
 	.expected_addr_cells = -1, /* For don't care */
 	.expected_size_cells = -1,
 	.is_type = is_simple_bridge,
+	.check_unit_addr = simple_bus_unit_addr,
+};
+
+struct bus_type default_bus_type = {
+	.expected_addr_cells = -1, /* For don't care */
+	.expected_size_cells = -1,
+	.check_unit_addr = default_unit_addr,
 };
 
+static void check_unit_address_format(struct check *c, struct node *dt,
+				      struct node *node)
+{
+	struct bus_type *bt;
+
+	if (!node->parent)
+		return;
+
+	bt = node->parent->bus_type;
+	if (!bt)
+		bt = &default_bus_type;
+
+	if (bt->check_unit_addr)
+		bt->check_unit_addr(c, dt, node);
+}
+NODE_WARNING(unit_address_format, NULL);
+
 struct bus_type *bus_types[] = {
 	&pci_bus_type,
 	&simple_bus_type,
@@ -635,11 +716,6 @@ static void fixup_addr_size_cells(struct check *c, struct node *dt,
 WARNING(addr_size_cells, NULL, fixup_addr_size_cells, NULL, 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 node *dt,
 			     struct node *node)
 {
@@ -771,6 +847,7 @@ static struct check *check_table[] = {
 	&addr_size_cells, &reg_format, &ranges_format,
 
 	&unit_address_vs_reg,
+	&unit_address_format,
 
 	&avoid_default_addr_size,
 	&obsolete_chosen_interrupt_controller,
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 7eb9b3d..4adc704 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -447,6 +447,10 @@ 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_format
+    check_tests unit-addr-leading-0s.dts unit_address_format
+    check_tests unit-addr-simple-bus-comma.dts unit_address_format
+    check_tests unit-addr-simple-bus-reg-mismatch.dts unit_address_format
     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 0000000..7c8e2ce
--- /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 0000000..7ed7254
--- /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>;
+	};
+};
diff --git a/tests/unit-addr-simple-bus-comma.dts b/tests/unit-addr-simple-bus-comma.dts
new file mode 100644
index 0000000..ea6f769
--- /dev/null
+++ b/tests/unit-addr-simple-bus-comma.dts
@@ -0,0 +1,18 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	bus@10000000 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "simple-bus";
+		ranges = <0x0 0x10000000 0x10000>;
+
+		node@0,1000 {
+			reg = <0x1000 1>;
+		};
+	};
+
+};
diff --git a/tests/unit-addr-simple-bus-reg-mismatch.dts b/tests/unit-addr-simple-bus-reg-mismatch.dts
new file mode 100644
index 0000000..2823377
--- /dev/null
+++ b/tests/unit-addr-simple-bus-reg-mismatch.dts
@@ -0,0 +1,18 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	bus@10000000 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "simple-bus";
+		ranges = <0x0 0x10000000 0x10000>;
+
+		node@100 {
+			reg = <0x1000 1>;
+		};
+	};
+
+};
-- 
2.5.0

Re: [RFC 2/3] checks: Add unit-address checks for simple-bus and default

[David Gibson]

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

On Wed, Mar 23, 2016 at 07:40:20PM -0500, Rob Herring wrote:
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Minor nit: before doing these tests, we should probably add a check
which ensures that any bus bridge node *has* a #address-cells and
#size-cells value.

> ---
>  checks.c                                    | 87 +++++++++++++++++++++++++++--
>  tests/run_tests.sh                          |  4 ++
>  tests/unit-addr-leading-0s.dts              | 10 ++++
>  tests/unit-addr-leading-0x.dts              | 10 ++++
>  tests/unit-addr-simple-bus-comma.dts        | 18 ++++++
>  tests/unit-addr-simple-bus-reg-mismatch.dts | 18 ++++++
>  6 files changed, 142 insertions(+), 5 deletions(-)
>  create mode 100644 tests/unit-addr-leading-0s.dts
>  create mode 100644 tests/unit-addr-leading-0x.dts
>  create mode 100644 tests/unit-addr-simple-bus-comma.dts
>  create mode 100644 tests/unit-addr-simple-bus-reg-mismatch.dts
> 
> diff --git a/checks.c b/checks.c
> index 48e926e..82a7f38 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 { \
> @@ -578,12 +583,88 @@ static bool is_simple_bridge(struct node *node)
>  	return false;
>  }
>  
> +static void default_unit_addr(struct check *c, struct node *dt, struct node *node)
> +{
> +	const char *unitname = get_unitname(node);
> +
> +	if (strstr(unitname, "0x") == unitname) {
> +		FAIL(c, "Node %s unit address 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 address should not have leading 0s",
> +		     node->fullpath);

Explicitly checking various aspects of the format seems a bit weird to
me.  Why not just generate the expected address from 'reg' and
strcmp()?

> +}
> +
> +static void simple_bus_unit_addr(struct check *c, struct node *dt, struct node *node)
> +{
> +	const char *unitname = get_unitname(node);
> +	struct property *prop;
> +	uint64_t unitaddr, regaddr = 0;
> +	int n, addr_cells;
> +	cell_t *cell;
> +
> +	default_unit_addr(c, dt, node);
> +
> +	n = strspn(unitname, DIGITS "abcedf");
> +	if (n != strlen(unitname))
> +		FAIL(c, "Node %s unit address should have only lower case hex digits",
> +		     node->fullpath);
> +
> +	unitaddr = strtoll(unitname, NULL, 16);
> +
> +	prop = get_property(node, "reg");
> +	if (!prop) {
> +		prop = get_property(node, "ranges");
> +		if (!prop || !prop->val.len)
> +			return;
> +
> +		cell = (cell_t *)prop->val.val;
> +		cell += node_addr_cells(node);
> +	} else
> +		cell = (cell_t *)prop->val.val;
> +
> +	addr_cells = node_addr_cells(node->parent);
> +	while (addr_cells--)
> +		regaddr = (regaddr << 32) | fdt32_to_cpu(*cell++);
> +
> +	if (regaddr != unitaddr)
> +		FAIL(c, "Node %s unit address does not match reg address (%zx != %zx)",
> +		     node->fullpath, regaddr, unitaddr);

Again, parsing the unit address and comparing back to reg seems
backwards to me.

> +}
> +
>  struct bus_type simple_bus_type = {
>  	.expected_addr_cells = -1, /* For don't care */
>  	.expected_size_cells = -1,
>  	.is_type = is_simple_bridge,
> +	.check_unit_addr = simple_bus_unit_addr,
> +};
> +
> +struct bus_type default_bus_type = {
> +	.expected_addr_cells = -1, /* For don't care */
> +	.expected_size_cells = -1,
> +	.check_unit_addr = default_unit_addr,
>  };
>  
> +static void check_unit_address_format(struct check *c, struct node *dt,
> +				      struct node *node)
> +{
> +	struct bus_type *bt;
> +
> +	if (!node->parent)
> +		return;
> +
> +	bt = node->parent->bus_type;
> +	if (!bt)
> +		bt = &default_bus_type;
> +
> +	if (bt->check_unit_addr)
> +		bt->check_unit_addr(c, dt, node);
> +}
> +NODE_WARNING(unit_address_format, NULL);

I'm not entirely convinced with the idea of the default unit address
checker.  I'm more inclined towards only checking when we have a known
bus type, then trying to expand those known bus types as much as we can.

>  struct bus_type *bus_types[] = {
>  	&pci_bus_type,
>  	&simple_bus_type,
> @@ -635,11 +716,6 @@ static void fixup_addr_size_cells(struct check *c, struct node *dt,
>  WARNING(addr_size_cells, NULL, fixup_addr_size_cells, NULL, 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 node *dt,
>  			     struct node *node)
>  {
> @@ -771,6 +847,7 @@ static struct check *check_table[] = {
>  	&addr_size_cells, &reg_format, &ranges_format,
>  
>  	&unit_address_vs_reg,
> +	&unit_address_format,
>  
>  	&avoid_default_addr_size,
>  	&obsolete_chosen_interrupt_controller,
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 7eb9b3d..4adc704 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -447,6 +447,10 @@ 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_format
> +    check_tests unit-addr-leading-0s.dts unit_address_format
> +    check_tests unit-addr-simple-bus-comma.dts unit_address_format
> +    check_tests unit-addr-simple-bus-reg-mismatch.dts unit_address_format
>      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 0000000..7c8e2ce
> --- /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 0000000..7ed7254
> --- /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>;
> +	};
> +};
> diff --git a/tests/unit-addr-simple-bus-comma.dts b/tests/unit-addr-simple-bus-comma.dts
> new file mode 100644
> index 0000000..ea6f769
> --- /dev/null
> +++ b/tests/unit-addr-simple-bus-comma.dts
> @@ -0,0 +1,18 @@
> +/dts-v1/;
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	bus@10000000 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "simple-bus";
> +		ranges = <0x0 0x10000000 0x10000>;
> +
> +		node@0,1000 {
> +			reg = <0x1000 1>;
> +		};
> +	};
> +
> +};
> diff --git a/tests/unit-addr-simple-bus-reg-mismatch.dts b/tests/unit-addr-simple-bus-reg-mismatch.dts
> new file mode 100644
> index 0000000..2823377
> --- /dev/null
> +++ b/tests/unit-addr-simple-bus-reg-mismatch.dts
> @@ -0,0 +1,18 @@
> +/dts-v1/;
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	bus@10000000 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "simple-bus";
> +		ranges = <0x0 0x10000000 0x10000>;
> +
> +		node@100 {
> +			reg = <0x1000 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 2/3] checks: Add unit-address checks for simple-bus and default

[Rob Herring]

On Thu, Mar 31, 2016 at 12:29 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Wed, Mar 23, 2016 at 07:40:20PM -0500, Rob Herring wrote:
>> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
> Minor nit: before doing these tests, we should probably add a check
> which ensures that any bus bridge node *has* a #address-cells and
> #size-cells value.

I'll check, but I thought we already had that check because any bridge
node has reg or ranges.

>
>> ---
>>  checks.c                                    | 87 +++++++++++++++++++++++++++--
>>  tests/run_tests.sh                          |  4 ++
>>  tests/unit-addr-leading-0s.dts              | 10 ++++
>>  tests/unit-addr-leading-0x.dts              | 10 ++++
>>  tests/unit-addr-simple-bus-comma.dts        | 18 ++++++
>>  tests/unit-addr-simple-bus-reg-mismatch.dts | 18 ++++++
>>  6 files changed, 142 insertions(+), 5 deletions(-)
>>  create mode 100644 tests/unit-addr-leading-0s.dts
>>  create mode 100644 tests/unit-addr-leading-0x.dts
>>  create mode 100644 tests/unit-addr-simple-bus-comma.dts
>>  create mode 100644 tests/unit-addr-simple-bus-reg-mismatch.dts
>>
>> diff --git a/checks.c b/checks.c
>> index 48e926e..82a7f38 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 { \
>> @@ -578,12 +583,88 @@ static bool is_simple_bridge(struct node *node)
>>       return false;
>>  }
>>
>> +static void default_unit_addr(struct check *c, struct node *dt, struct node *node)
>> +{
>> +     const char *unitname = get_unitname(node);
>> +
>> +     if (strstr(unitname, "0x") == unitname) {
>> +             FAIL(c, "Node %s unit address 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 address should not have leading 0s",
>> +                  node->fullpath);
>
> Explicitly checking various aspects of the format seems a bit weird to
> me.  Why not just generate the expected address from 'reg' and
> strcmp()?

Because for the default check, I'm only testing these aspects. I found
some cases running this thru the kernel tree dts files that the full
simple-bus check is too strict. For example, we want to warn on
"@0x002,4", but not "@2,4" or "@2blah".

>> +static void simple_bus_unit_addr(struct check *c, struct node *dt, struct node *node)
>> +{
>> +     const char *unitname = get_unitname(node);
>> +     struct property *prop;
>> +     uint64_t unitaddr, regaddr = 0;
>> +     int n, addr_cells;
>> +     cell_t *cell;
>> +
>> +     default_unit_addr(c, dt, node);
>> +
>> +     n = strspn(unitname, DIGITS "abcedf");
>> +     if (n != strlen(unitname))
>> +             FAIL(c, "Node %s unit address should have only lower case hex digits",
>> +                  node->fullpath);
>> +
>> +     unitaddr = strtoll(unitname, NULL, 16);
>> +
>> +     prop = get_property(node, "reg");
>> +     if (!prop) {
>> +             prop = get_property(node, "ranges");
>> +             if (!prop || !prop->val.len)
>> +                     return;
>> +
>> +             cell = (cell_t *)prop->val.val;
>> +             cell += node_addr_cells(node);
>> +     } else
>> +             cell = (cell_t *)prop->val.val;
>> +
>> +     addr_cells = node_addr_cells(node->parent);
>> +     while (addr_cells--)
>> +             regaddr = (regaddr << 32) | fdt32_to_cpu(*cell++);
>> +
>> +     if (regaddr != unitaddr)
>> +             FAIL(c, "Node %s unit address does not match reg address (%zx != %zx)",
>> +                  node->fullpath, regaddr, unitaddr);
>
> Again, parsing the unit address and comparing back to reg seems
> backwards to me.

I agree here. And then I don't need simple-bus to inherit the default checks.

>> +}
>> +
>>  struct bus_type simple_bus_type = {
>>       .expected_addr_cells = -1, /* For don't care */
>>       .expected_size_cells = -1,
>>       .is_type = is_simple_bridge,
>> +     .check_unit_addr = simple_bus_unit_addr,
>> +};
>> +
>> +struct bus_type default_bus_type = {
>> +     .expected_addr_cells = -1, /* For don't care */
>> +     .expected_size_cells = -1,
>> +     .check_unit_addr = default_unit_addr,
>>  };
>>
>> +static void check_unit_address_format(struct check *c, struct node *dt,
>> +                                   struct node *node)
>> +{
>> +     struct bus_type *bt;
>> +
>> +     if (!node->parent)
>> +             return;
>> +
>> +     bt = node->parent->bus_type;
>> +     if (!bt)
>> +             bt = &default_bus_type;
>> +
>> +     if (bt->check_unit_addr)
>> +             bt->check_unit_addr(c, dt, node);
>> +}
>> +NODE_WARNING(unit_address_format, NULL);
>
> I'm not entirely convinced with the idea of the default unit address
> checker.  I'm more inclined towards only checking when we have a known
> bus type, then trying to expand those known bus types as much as we can.

We've been thru this. The default check is pretty minimal. If we could
come up with determining bus types of I2C and SPI, then maybe. We
could look at controller node names, but then if the node names are
wrong, we'd need to detect that. With SPI the child nodes generally
have SPI specific properties. With I2C, we don't have anything else to
key off of.

Rob

Re: [RFC 2/3] checks: Add unit-address checks for simple-bus and default

[David Gibson]

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

On Thu, Mar 31, 2016 at 11:18:25AM -0500, Rob Herring wrote:
> On Thu, Mar 31, 2016 at 12:29 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Wed, Mar 23, 2016 at 07:40:20PM -0500, Rob Herring wrote:
> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >
> > Minor nit: before doing these tests, we should probably add a check
> > which ensures that any bus bridge node *has* a #address-cells and
> > #size-cells value.
> 
> I'll check, but I thought we already had that check because any bridge
> node has reg or ranges.
> 
> >
> >> ---
> >>  checks.c                                    | 87 +++++++++++++++++++++++++++--
> >>  tests/run_tests.sh                          |  4 ++
> >>  tests/unit-addr-leading-0s.dts              | 10 ++++
> >>  tests/unit-addr-leading-0x.dts              | 10 ++++
> >>  tests/unit-addr-simple-bus-comma.dts        | 18 ++++++
> >>  tests/unit-addr-simple-bus-reg-mismatch.dts | 18 ++++++
> >>  6 files changed, 142 insertions(+), 5 deletions(-)
> >>  create mode 100644 tests/unit-addr-leading-0s.dts
> >>  create mode 100644 tests/unit-addr-leading-0x.dts
> >>  create mode 100644 tests/unit-addr-simple-bus-comma.dts
> >>  create mode 100644 tests/unit-addr-simple-bus-reg-mismatch.dts
> >>
> >> diff --git a/checks.c b/checks.c
> >> index 48e926e..82a7f38 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 { \
> >> @@ -578,12 +583,88 @@ static bool is_simple_bridge(struct node *node)
> >>       return false;
> >>  }
> >>
> >> +static void default_unit_addr(struct check *c, struct node *dt, struct node *node)
> >> +{
> >> +     const char *unitname = get_unitname(node);
> >> +
> >> +     if (strstr(unitname, "0x") == unitname) {
> >> +             FAIL(c, "Node %s unit address 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 address should not have leading 0s",
> >> +                  node->fullpath);
> >
> > Explicitly checking various aspects of the format seems a bit weird to
> > me.  Why not just generate the expected address from 'reg' and
> > strcmp()?
> 
> Because for the default check, I'm only testing these aspects. I found
> some cases running this thru the kernel tree dts files that the full
> simple-bus check is too strict. For example, we want to warn on
> "@0x002,4", but not "@2,4" or "@2blah".

Ok.  Thinking about it, I think this might work a bit better separated
(mostly) from the bus type stuff.  Basically treat it as a "common
unit name problems" test, that will skip itself if a bus type is set
(which will allow more thorough testing of the unit name).

> >> +static void simple_bus_unit_addr(struct check *c, struct node *dt, struct node *node)
> >> +{
> >> +     const char *unitname = get_unitname(node);
> >> +     struct property *prop;
> >> +     uint64_t unitaddr, regaddr = 0;
> >> +     int n, addr_cells;
> >> +     cell_t *cell;
> >> +
> >> +     default_unit_addr(c, dt, node);
> >> +
> >> +     n = strspn(unitname, DIGITS "abcedf");
> >> +     if (n != strlen(unitname))
> >> +             FAIL(c, "Node %s unit address should have only lower case hex digits",
> >> +                  node->fullpath);
> >> +
> >> +     unitaddr = strtoll(unitname, NULL, 16);
> >> +
> >> +     prop = get_property(node, "reg");
> >> +     if (!prop) {
> >> +             prop = get_property(node, "ranges");
> >> +             if (!prop || !prop->val.len)
> >> +                     return;
> >> +
> >> +             cell = (cell_t *)prop->val.val;
> >> +             cell += node_addr_cells(node);
> >> +     } else
> >> +             cell = (cell_t *)prop->val.val;
> >> +
> >> +     addr_cells = node_addr_cells(node->parent);
> >> +     while (addr_cells--)
> >> +             regaddr = (regaddr << 32) | fdt32_to_cpu(*cell++);
> >> +
> >> +     if (regaddr != unitaddr)
> >> +             FAIL(c, "Node %s unit address does not match reg address (%zx != %zx)",
> >> +                  node->fullpath, regaddr, unitaddr);
> >
> > Again, parsing the unit address and comparing back to reg seems
> > backwards to me.
> 
> I agree here. And then I don't need simple-bus to inherit the default checks.

Yes, I think that makes sense.

> >> +}
> >> +
> >>  struct bus_type simple_bus_type = {
> >>       .expected_addr_cells = -1, /* For don't care */
> >>       .expected_size_cells = -1,
> >>       .is_type = is_simple_bridge,
> >> +     .check_unit_addr = simple_bus_unit_addr,
> >> +};
> >> +
> >> +struct bus_type default_bus_type = {
> >> +     .expected_addr_cells = -1, /* For don't care */
> >> +     .expected_size_cells = -1,
> >> +     .check_unit_addr = default_unit_addr,
> >>  };
> >>
> >> +static void check_unit_address_format(struct check *c, struct node *dt,
> >> +                                   struct node *node)
> >> +{
> >> +     struct bus_type *bt;
> >> +
> >> +     if (!node->parent)
> >> +             return;
> >> +
> >> +     bt = node->parent->bus_type;
> >> +     if (!bt)
> >> +             bt = &default_bus_type;
> >> +
> >> +     if (bt->check_unit_addr)
> >> +             bt->check_unit_addr(c, dt, node);
> >> +}
> >> +NODE_WARNING(unit_address_format, NULL);
> >
> > I'm not entirely convinced with the idea of the default unit address
> > checker.  I'm more inclined towards only checking when we have a known
> > bus type, then trying to expand those known bus types as much as we can.
> 
> We've been thru this. The default check is pretty minimal. If we could
> come up with determining bus types of I2C and SPI, then maybe. We
> could look at controller node names, but then if the node names are
> wrong, we'd need to detect that. With SPI the child nodes generally
> have SPI specific properties. With I2C, we don't have anything else to
> key off of.

Ok.  As above, I think I'd be more comfortable with this check as a
"common mistakes" warning than a "default bus" checker.

It's a small distinction, but it's a question of being presented as
something with authoritative knowledge of what a unit address should
look like, versus something looking for specific common problems.

-- 
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: [RFC 2/3] checks: Add unit-address checks for simple-bus and default

[Rob Herring]

On Thu, Mar 31, 2016 at 9:27 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Thu, Mar 31, 2016 at 11:18:25AM -0500, Rob Herring wrote:
>> On Thu, Mar 31, 2016 at 12:29 AM, David Gibson
>> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Wed, Mar 23, 2016 at 07:40:20PM -0500, Rob Herring wrote:
>> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> >
>> > Minor nit: before doing these tests, we should probably add a check
>> > which ensures that any bus bridge node *has* a #address-cells and
>> > #size-cells value.
>>
>> I'll check, but I thought we already had that check because any bridge
>> node has reg or ranges.
>>
>> >
>> >> ---
>> >>  checks.c                                    | 87 +++++++++++++++++++++++++++--
>> >>  tests/run_tests.sh                          |  4 ++
>> >>  tests/unit-addr-leading-0s.dts              | 10 ++++
>> >>  tests/unit-addr-leading-0x.dts              | 10 ++++
>> >>  tests/unit-addr-simple-bus-comma.dts        | 18 ++++++
>> >>  tests/unit-addr-simple-bus-reg-mismatch.dts | 18 ++++++
>> >>  6 files changed, 142 insertions(+), 5 deletions(-)
>> >>  create mode 100644 tests/unit-addr-leading-0s.dts
>> >>  create mode 100644 tests/unit-addr-leading-0x.dts
>> >>  create mode 100644 tests/unit-addr-simple-bus-comma.dts
>> >>  create mode 100644 tests/unit-addr-simple-bus-reg-mismatch.dts
>> >>
>> >> diff --git a/checks.c b/checks.c
>> >> index 48e926e..82a7f38 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 { \
>> >> @@ -578,12 +583,88 @@ static bool is_simple_bridge(struct node *node)
>> >>       return false;
>> >>  }
>> >>
>> >> +static void default_unit_addr(struct check *c, struct node *dt, struct node *node)
>> >> +{
>> >> +     const char *unitname = get_unitname(node);
>> >> +
>> >> +     if (strstr(unitname, "0x") == unitname) {
>> >> +             FAIL(c, "Node %s unit address 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 address should not have leading 0s",
>> >> +                  node->fullpath);
>> >
>> > Explicitly checking various aspects of the format seems a bit weird to
>> > me.  Why not just generate the expected address from 'reg' and
>> > strcmp()?
>>
>> Because for the default check, I'm only testing these aspects. I found
>> some cases running this thru the kernel tree dts files that the full
>> simple-bus check is too strict. For example, we want to warn on
>> "@0x002,4", but not "@2,4" or "@2blah".
>
> Ok.  Thinking about it, I think this might work a bit better separated
> (mostly) from the bus type stuff.  Basically treat it as a "common
> unit name problems" test, that will skip itself if a bus type is set
> (which will allow more thorough testing of the unit name).

Ha! That's pretty much back to my original patch, but with the
addition of skipping the test if the bus type is known.

Rob

Re: [RFC 2/3] checks: Add unit-address checks for simple-bus and default

[David Gibson]

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

On Fri, Apr 01, 2016 at 01:50:18PM -0500, Rob Herring wrote:
> On Thu, Mar 31, 2016 at 9:27 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Thu, Mar 31, 2016 at 11:18:25AM -0500, Rob Herring wrote:
> >> On Thu, Mar 31, 2016 at 12:29 AM, David Gibson
> >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >> > On Wed, Mar 23, 2016 at 07:40:20PM -0500, Rob Herring wrote:
> >> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> >
> >> > Minor nit: before doing these tests, we should probably add a check
> >> > which ensures that any bus bridge node *has* a #address-cells and
> >> > #size-cells value.
> >>
> >> I'll check, but I thought we already had that check because any bridge
> >> node has reg or ranges.
> >>
> >> >
> >> >> ---
> >> >>  checks.c                                    | 87 +++++++++++++++++++++++++++--
> >> >>  tests/run_tests.sh                          |  4 ++
> >> >>  tests/unit-addr-leading-0s.dts              | 10 ++++
> >> >>  tests/unit-addr-leading-0x.dts              | 10 ++++
> >> >>  tests/unit-addr-simple-bus-comma.dts        | 18 ++++++
> >> >>  tests/unit-addr-simple-bus-reg-mismatch.dts | 18 ++++++
> >> >>  6 files changed, 142 insertions(+), 5 deletions(-)
> >> >>  create mode 100644 tests/unit-addr-leading-0s.dts
> >> >>  create mode 100644 tests/unit-addr-leading-0x.dts
> >> >>  create mode 100644 tests/unit-addr-simple-bus-comma.dts
> >> >>  create mode 100644 tests/unit-addr-simple-bus-reg-mismatch.dts
> >> >>
> >> >> diff --git a/checks.c b/checks.c
> >> >> index 48e926e..82a7f38 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 { \
> >> >> @@ -578,12 +583,88 @@ static bool is_simple_bridge(struct node *node)
> >> >>       return false;
> >> >>  }
> >> >>
> >> >> +static void default_unit_addr(struct check *c, struct node *dt, struct node *node)
> >> >> +{
> >> >> +     const char *unitname = get_unitname(node);
> >> >> +
> >> >> +     if (strstr(unitname, "0x") == unitname) {
> >> >> +             FAIL(c, "Node %s unit address 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 address should not have leading 0s",
> >> >> +                  node->fullpath);
> >> >
> >> > Explicitly checking various aspects of the format seems a bit weird to
> >> > me.  Why not just generate the expected address from 'reg' and
> >> > strcmp()?
> >>
> >> Because for the default check, I'm only testing these aspects. I found
> >> some cases running this thru the kernel tree dts files that the full
> >> simple-bus check is too strict. For example, we want to warn on
> >> "@0x002,4", but not "@2,4" or "@2blah".
> >
> > Ok.  Thinking about it, I think this might work a bit better separated
> > (mostly) from the bus type stuff.  Basically treat it as a "common
> > unit name problems" test, that will skip itself if a bus type is set
> > (which will allow more thorough testing of the unit name).
> 
> Ha! That's pretty much back to my original patch, but with the
> addition of skipping the test if the bus type is known.

Heh, yeah, I guess it is.  Sorry.

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

[RFC 3/3] checks: Add unit-address checks for PCI buses

[Rob Herring]

PCI 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. Add checks that the unit
address matches this form.

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

diff --git a/checks.c b/checks.c
index 82a7f38..1d0fcfb 100644
--- a/checks.c
+++ b/checks.c
@@ -549,10 +549,30 @@ static bool is_pci_bridge(struct node *node)
 	return false;
 }
 
+static void pci_unit_addr(struct check *c, struct node *dt, struct node *node)
+{
+	const char *unitname = get_unitname(node);
+	unsigned int dev, func;
+	int ret;
+
+	ret = sscanf(unitname, "%2x,%1x", &dev, &func);
+	if (ret >= 1) {
+		if (dev > 0x1f)
+			FAIL(c, "Node %s PCI device number out of range",
+			     node->fullpath);
+	}
+	if (ret == 2) {
+		if (func > 7)
+			FAIL(c, "Node %s PCI function number out of range",
+			     node->fullpath);
+	}
+}
+
 struct bus_type pci_bus_type = {
         .expected_addr_cells = 3,
         .expected_size_cells = 2,
         .is_type = is_pci_bridge,
+        .check_unit_addr = pci_unit_addr,
 };
 
 static bool is_simple_bridge(struct node *node)
-- 
2.5.0

Re: [RFC 3/3] checks: Add unit-address checks for PCI buses

[David Gibson]

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

On Wed, Mar 23, 2016 at 07:40:21PM -0500, Rob Herring wrote:
> PCI 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. Add checks that the unit
> address matches this form.

Hmm.. we ought to be able to do a bit better than this, again by
constructing the expected unit address from reg.  While we're at it,
it probably makes sense to have that dependent on a check which makes
sure the first entry in a pci device's 'reg' is the config address.

Hmm.. I wonder if what might make sense is to have two whole
(possible) sublists of tests within the bus_type structure - one, a
list of checks to apply to bridge nodes and the other a list of checks
to apply to bus device nodes.

> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  checks.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/checks.c b/checks.c
> index 82a7f38..1d0fcfb 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -549,10 +549,30 @@ static bool is_pci_bridge(struct node *node)
>  	return false;
>  }
>  
> +static void pci_unit_addr(struct check *c, struct node *dt, struct node *node)
> +{
> +	const char *unitname = get_unitname(node);
> +	unsigned int dev, func;
> +	int ret;
> +
> +	ret = sscanf(unitname, "%2x,%1x", &dev, &func);
> +	if (ret >= 1) {
> +		if (dev > 0x1f)
> +			FAIL(c, "Node %s PCI device number out of range",
> +			     node->fullpath);
> +	}
> +	if (ret == 2) {
> +		if (func > 7)
> +			FAIL(c, "Node %s PCI function number out of range",
> +			     node->fullpath);
> +	}
> +}
> +
>  struct bus_type pci_bus_type = {
>          .expected_addr_cells = 3,
>          .expected_size_cells = 2,
>          .is_type = is_pci_bridge,
> +        .check_unit_addr = pci_unit_addr,
>  };
>  
>  static bool is_simple_bridge(struct node *node)

-- 
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: [RFC 3/3] checks: Add unit-address checks for PCI buses

[Rob Herring]

On Thu, Mar 31, 2016 at 12:32 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Wed, Mar 23, 2016 at 07:40:21PM -0500, Rob Herring wrote:
>> PCI 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. Add checks that the unit
>> address matches this form.
>
> Hmm.. we ought to be able to do a bit better than this, again by
> constructing the expected unit address from reg.

If I do that, I can't check the dev and function values are within range.

> While we're at it,
> it probably makes sense to have that dependent on a check which makes
> sure the first entry in a pci device's 'reg' is the config address.

You mean just check the first cell masked with 0x1f000000 is 0, right?

Really, I'm not so worried about checking PCI devices. DT's with PCI
devices described are rare. All I really want to check first are
leading 0s, leading 0x and uppercase hex. That's 90% of what I find in
reviews and am tired of commenting on. I do think bus specific checks
are useful, but don't think we need to fill them all in right now.

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: [RFC 3/3] checks: Add unit-address checks for PCI buses

[David Gibson]

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

On Fri, Apr 01, 2016 at 02:52:44PM -0500, Rob Herring wrote:
> On Thu, Mar 31, 2016 at 12:32 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Wed, Mar 23, 2016 at 07:40:21PM -0500, Rob Herring wrote:
> >> PCI 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. Add checks that the unit
> >> address matches this form.
> >
> > Hmm.. we ought to be able to do a bit better than this, again by
> > constructing the expected unit address from reg.
> 
> If I do that, I can't check the dev and function values are within range.

Not from here, but that seems an odd place to do it anyway.  Instead
I'd suggest checking the dev and function values in a verifier for
'reg' values on the bus, then check the unit address afterwards.

> > While we're at it,
> > it probably makes sense to have that dependent on a check which makes
> > sure the first entry in a pci device's 'reg' is the config address.
> 
> You mean just check the first cell masked with 0x1f000000 is 0, right?

I guess, I'd have to look at the PCI reg encoding again.

> Really, I'm not so worried about checking PCI devices. DT's with PCI
> devices described are rare. All I really want to check first are
> leading 0s, leading 0x and uppercase hex. That's 90% of what I find in
> reviews and am tired of commenting on. I do think bus specific checks
> are useful, but don't think we need to fill them all in right now.

Ok.  How about we leave the PCI checked function as just /* TODO */
for now then.

-- 
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: [RFC 1/3] checks: Add infrastructure for setting bus type of nodes

[David Gibson]

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

On Wed, Mar 23, 2016 at 07:40:19PM -0500, Rob Herring wrote:
> In preparation to support bus specific checks, add the necessary
> infrastructure to determine the bus type for nodes. Initially, PCI and
> simple bus are supported.
> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> David,

Sorry it's taken me a while to look at this.  I've been a mixture of
busy and sick :/

> Hopefully this matches what you had in mind for bus checks.

It's getting there.

> 
> Rob
> 
>  checks.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  dtc.h    | 11 +++++++++
>  2 files changed, 90 insertions(+)
> 
> diff --git a/checks.c b/checks.c
> index 386f956..48e926e 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -527,6 +527,84 @@ static void fixup_path_references(struct check *c, struct node *dt,
>  ERROR(path_references, NULL, NULL, fixup_path_references, NULL,
>        &duplicate_node_names);
>  
> +static bool is_pci_bridge(struct node *node)
> +{
> +	struct property *prop;
> +
> +	if (!node)
> +		return false;
> +
> +	prop = get_property(node, "device_type");
> +	if (!prop)
> +		return false;
> +
> +	if (strcmp(prop->val.val, "pci") == 0)
> +		return true;
> +
> +	return false;
> +}

So, I don't love using device_type here, since that's generally
discouraged in modern flat trees, but I don't know of a better way to
detect a pci bridge, so I guess it's ok.  

> +struct bus_type pci_bus_type = {
> +        .expected_addr_cells = 3,
> +        .expected_size_cells = 2,

I'm a bit torn here.  Part of me wants to suggest a 'check_bridge'
function which handles this and can also make more subtle checks, but
then just the expected cells values will handle nearly all real cases
more succinctly.

> +        .is_type = is_pci_bridge,

'is_type' isn't clear whether it's checking a bus bridge or a bus
device, so it needs a rename.  'bridge_is_type' maybe?

> +};
> +
> +static bool is_simple_bridge(struct node *node)
> +{
> +	struct property *prop;
> +	int len = 0;
> +
> +	if (!node)
> +		return false;
> +
> +	/* root node is special case defaulting to simple-bus */
> +	if (!node->parent)
> +		return true;
> +
> +	prop = get_property(node, "compatible");
> +	if (!prop)
> +		return false;
> +
> +	do {
> +		const char *str = prop->val.val;
> +
> +		if (strcmp(str, "simple-bus") == 0)
> +			return true;
> +		len += strlen(str) + 1;
> +		str += len;
> +	} while (len < prop->val.len);

Definitely want a helper function in livetree.c to do this check for a
compatible value.

> +	return false;
> +}
> +
> +struct bus_type simple_bus_type = {
> +	.expected_addr_cells = -1, /* For don't care */
> +	.expected_size_cells = -1,
> +	.is_type = is_simple_bridge,
> +};
> +
> +struct bus_type *bus_types[] = {
> +	&pci_bus_type,
> +	&simple_bus_type,
> +	NULL
> +};
> +
> +static void fixup_bus_type(struct check *c, struct node *dt,
> +				  struct node *node)
> +{
> +	struct bus_type **bus;
> +
> +	for (bus = bus_types; *bus != NULL; bus++) {
> +		if (!(*bus)->is_type(node))
> +			continue;
> +
> +		node->bus_type = *bus;
> +		break;
> +	}
> +}
> +ERROR(bus_type, NULL, fixup_bus_type, NULL, NULL);
> +
>  /*
>   * Semantic checks
>   */
> @@ -685,6 +763,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,
> diff --git a/dtc.h b/dtc.h
> index 56212c8..c1a1fe9 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -132,6 +132,16 @@ struct label {
>  	struct label *next;
>  };
>  
> +struct check;
> +struct node;
> +
> +struct bus_type {
> +        int expected_addr_cells;
> +        int expected_size_cells;
> +        bool (*is_type)(struct node *node);
> +        void (*check_unit_addr)(struct check *c, struct node *dt, struct node *node);
> +};
> +
>  struct property {
>  	bool deleted;
>  	char *name;
> @@ -158,6 +168,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: [RFC 1/3] checks: Add infrastructure for setting bus type of nodes

[Rob Herring]

On Thu, Mar 31, 2016 at 12:22 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Wed, Mar 23, 2016 at 07:40:19PM -0500, Rob Herring wrote:
>> In preparation to support bus specific checks, add the necessary
>> infrastructure to determine the bus type for nodes. Initially, PCI and
>> simple bus are supported.
>>
>> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> ---
>> David,
>
> Sorry it's taken me a while to look at this.  I've been a mixture of
> busy and sick :/

No problem.

[...]

>> +static bool is_pci_bridge(struct node *node)
>> +{
>> +     struct property *prop;
>> +
>> +     if (!node)
>> +             return false;
>> +
>> +     prop = get_property(node, "device_type");
>> +     if (!prop)
>> +             return false;
>> +
>> +     if (strcmp(prop->val.val, "pci") == 0)
>> +             return true;
>> +
>> +     return false;
>> +}
>
> So, I don't love using device_type here, since that's generally
> discouraged in modern flat trees, but I don't know of a better way to
> detect a pci bridge, so I guess it's ok.

True, but pci, cpu, and memory remain as accepted uses. We'd have to
define a "pci-bridge" or "pci-bus" compatible to replace it.

>> +struct bus_type pci_bus_type = {
>> +        .expected_addr_cells = 3,
>> +        .expected_size_cells = 2,
>
> I'm a bit torn here.  Part of me wants to suggest a 'check_bridge'
> function which handles this and can also make more subtle checks, but
> then just the expected cells values will handle nearly all real cases
> more succinctly.

I left them as you had them, but I'm not so sure these are all that
useful. It works for PCI as the sizes are fixed, but then we could
just check against fixed values. For simple-bus, we need more
flexibility because the size could be 1 or 2. For other cases like I2C
or SPI buses, we know the sizes, but we can't really detect those
buses.

Rob

Re: [RFC 1/3] checks: Add infrastructure for setting bus type of nodes

[David Gibson]

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

On Thu, Mar 31, 2016 at 10:17:46AM -0500, Rob Herring wrote:
> On Thu, Mar 31, 2016 at 12:22 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Wed, Mar 23, 2016 at 07:40:19PM -0500, Rob Herring wrote:
> >> In preparation to support bus specific checks, add the necessary
> >> infrastructure to determine the bus type for nodes. Initially, PCI and
> >> simple bus are supported.
> >>
> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> ---
> >> David,
> >
> > Sorry it's taken me a while to look at this.  I've been a mixture of
> > busy and sick :/
> 
> No problem.
> 
> [...]
> 
> >> +static bool is_pci_bridge(struct node *node)
> >> +{
> >> +     struct property *prop;
> >> +
> >> +     if (!node)
> >> +             return false;
> >> +
> >> +     prop = get_property(node, "device_type");
> >> +     if (!prop)
> >> +             return false;
> >> +
> >> +     if (strcmp(prop->val.val, "pci") == 0)
> >> +             return true;
> >> +
> >> +     return false;
> >> +}
> >
> > So, I don't love using device_type here, since that's generally
> > discouraged in modern flat trees, but I don't know of a better way to
> > detect a pci bridge, so I guess it's ok.
> 
> True, but pci, cpu, and memory remain as accepted uses. We'd have to
> define a "pci-bridge" or "pci-bus" compatible to replace it.

Right, that was the conclusion I was coming around to.

> >> +struct bus_type pci_bus_type = {
> >> +        .expected_addr_cells = 3,
> >> +        .expected_size_cells = 2,
> >
> > I'm a bit torn here.  Part of me wants to suggest a 'check_bridge'
> > function which handles this and can also make more subtle checks, but
> > then just the expected cells values will handle nearly all real cases
> > more succinctly.
> 
> I left them as you had them, but I'm not so sure these are all that
> useful. It works for PCI as the sizes are fixed, but then we could
> just check against fixed values. For simple-bus, we need more
> flexibility because the size could be 1 or 2. For other cases like I2C
> or SPI buses, we know the sizes, but we can't really detect those
> buses.

Hm, ok.  Well how about instead of the expected cells values, we have
two callbacks:  check_bridge() and check_device().   The
check_bridge() callback can check the cells parameters along with
anything else that makes sense.

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