diff mbox series

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

Message ID 20170124174534.3865-5-robh@kernel.org
State New
Headers show
Series dtc unit-address and character set checks | expand

Commit Message

Rob Herring Jan. 24, 2017, 5:45 p.m. UTC 
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@kernel.org>

---
 checks.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h    | 11 +++++++++++
 2 files changed, 62 insertions(+)

-- 
2.10.1

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

Comments

David Gibson Jan. 31, 2017, 12:26 a.m. UTC | #1 
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@kernel.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
Rob Herring Feb. 1, 2017, 9:54 p.m. UTC | #2 
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@kernel.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@kernel.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@kernel.org>

---
 checks.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h    |  4 +++-
 2 files changed, 77 insertions(+), 1 deletion(-)

-- 
2.10.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.htmldiff --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 Feb. 6, 2017, 2:14 a.m. UTC | #3 
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@kernel.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@kernel.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@kernel.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
diff mbox series

Patch

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