[kvm-unit-tests PATCH 0/2] devicetree: use correct #address/#size cells

[kvm-unit-tests PATCH 0/2] devicetree: use correct #address/#size cells

[Andrew Jones]

I recently noticed that when using the default bus translation (pbus)
we'd always use the root nodes #address/#size cells, which may not be
correct.

Andrew Jones (2):
  devicetree: search up tree in dt_get_nr_cells
  devicetree: use node #address/size-cells

 lib/devicetree.c | 42 ++++++++++++++++++++++++++++++------------
 lib/devicetree.h | 13 +++++--------
 2 files changed, 35 insertions(+), 20 deletions(-)

-- 
2.4.11

[kvm-unit-tests PATCH 1/2] devicetree: search up tree in dt_get_nr_cells

[Andrew Jones]

Search up the tree until we find #address-cells/#size-cells.
Also only assign outputs if both address and size are found.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 lib/devicetree.c | 17 +++++++++++++----
 lib/devicetree.h |  3 ++-
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/lib/devicetree.c b/lib/devicetree.c
index a5c7f7c69ddfd..d3751e2b7e7f9 100644
--- a/lib/devicetree.c
+++ b/lib/devicetree.c
@@ -24,21 +24,30 @@ int dt_get_nr_cells(int fdtnode, u32 *nr_address_cells, u32 *nr_size_cells)
 {
 	const struct fdt_property *prop;
 	u32 *nr_cells;
-	int len;
+	int len, nac, nsc;
+
+	while (1) {
+		prop = fdt_get_property(fdt, fdtnode, "#address-cells", &len);
+		if (prop != NULL || len != -FDT_ERR_NOTFOUND)
+			break;
+		fdtnode = fdt_parent_offset(fdt, fdtnode);
+	}
 
-	prop = fdt_get_property(fdt, fdtnode, "#address-cells", &len);
 	if (prop == NULL)
 		return len;
 
 	nr_cells = (u32 *)prop->data;
-	*nr_address_cells = fdt32_to_cpu(*nr_cells);
+	nac = fdt32_to_cpu(*nr_cells);
 
 	prop = fdt_get_property(fdt, fdtnode, "#size-cells", &len);
 	if (prop == NULL)
 		return len;
 
 	nr_cells = (u32 *)prop->data;
-	*nr_size_cells = fdt32_to_cpu(*nr_cells);
+	nsc = fdt32_to_cpu(*nr_cells);
+
+	*nr_address_cells = nac;
+	*nr_size_cells = nsc;
 
 	return 0;
 }
diff --git a/lib/devicetree.h b/lib/devicetree.h
index c8c86eeae28b6..09f2509409615 100644
--- a/lib/devicetree.h
+++ b/lib/devicetree.h
@@ -168,7 +168,8 @@ extern int dt_pbus_get_base_compatible(const char *compatible,
 
 /*
  * dt_get_nr_cells sets @nr_address_cells and @nr_size_cells to the
- * #address-cells and #size-cells properties of @fdtnode
+ * #address-cells and #size-cells properties of @fdtnode, potentially
+ * searching up the tree to find them in a parent node.
  * returns
  *  - zero on success
  *  - a negative FDT_ERR_* value on failure
-- 
2.4.11

Re: [kvm-unit-tests PATCH 1/2] devicetree: search up tree in dt_get_nr_cells

[Thomas Huth]

On 10.05.2016 19:32, Andrew Jones wrote:
> Search up the tree until we find #address-cells/#size-cells.
> Also only assign outputs if both address and size are found.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  lib/devicetree.c | 17 +++++++++++++----
>  lib/devicetree.h |  3 ++-
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/devicetree.c b/lib/devicetree.c
> index a5c7f7c69ddfd..d3751e2b7e7f9 100644
> --- a/lib/devicetree.c
> +++ b/lib/devicetree.c
> @@ -24,21 +24,30 @@ int dt_get_nr_cells(int fdtnode, u32 *nr_address_cells, u32 *nr_size_cells)
>  {
>  	const struct fdt_property *prop;
>  	u32 *nr_cells;
> -	int len;
> +	int len, nac, nsc;
> +
> +	while (1) {
> +		prop = fdt_get_property(fdt, fdtnode, "#address-cells", &len);
> +		if (prop != NULL || len != -FDT_ERR_NOTFOUND)
> +			break;
> +		fdtnode = fdt_parent_offset(fdt, fdtnode);
> +	}

Why do you need this search? ePAPR clearly states:

"The #address-cells and #size-cells properties are not inherited from
ancestors in the device tree. They shall be explicitly defined."

So as far as I can see, it should always be enough to look up the
properties in the parent of the node, no need for a recursion here?

 Thomas

Re: [kvm-unit-tests PATCH 1/2] devicetree: search up tree in dt_get_nr_cells

[Andrew Jones]

On Wed, May 11, 2016 at 09:02:39AM +0200, Thomas Huth wrote:
> On 10.05.2016 19:32, Andrew Jones wrote:
> > Search up the tree until we find #address-cells/#size-cells.
> > Also only assign outputs if both address and size are found.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  lib/devicetree.c | 17 +++++++++++++----
> >  lib/devicetree.h |  3 ++-
> >  2 files changed, 15 insertions(+), 5 deletions(-)
> > 
> > diff --git a/lib/devicetree.c b/lib/devicetree.c
> > index a5c7f7c69ddfd..d3751e2b7e7f9 100644
> > --- a/lib/devicetree.c
> > +++ b/lib/devicetree.c
> > @@ -24,21 +24,30 @@ int dt_get_nr_cells(int fdtnode, u32 *nr_address_cells, u32 *nr_size_cells)
> >  {
> >  	const struct fdt_property *prop;
> >  	u32 *nr_cells;
> > -	int len;
> > +	int len, nac, nsc;
> > +
> > +	while (1) {
> > +		prop = fdt_get_property(fdt, fdtnode, "#address-cells", &len);
> > +		if (prop != NULL || len != -FDT_ERR_NOTFOUND)
> > +			break;
> > +		fdtnode = fdt_parent_offset(fdt, fdtnode);
> > +	}
> 
> Why do you need this search? ePAPR clearly states:
> 
> "The #address-cells and #size-cells properties are not inherited from
> ancestors in the device tree. They shall be explicitly defined."

You are right.

My skimming of Documentation/devicetree/booting-without-of.txt was
sloppy and I missed basically the same quote

"Note that the parent's parent definitions of #address-cells and
#size-cells are not inherited so every node with children must
specify them."

> 
> So as far as I can see, it should always be enough to look up the
> properties in the parent of the node, no need for a recursion here?

So, yes, I should look in the current node, and then the parent, and
then just assert.

Thanks,
drew

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

Re: [kvm-unit-tests PATCH 1/2] devicetree: search up tree in dt_get_nr_cells

[Thomas Huth]

On 11.05.2016 11:04, Andrew Jones wrote:
> On Wed, May 11, 2016 at 09:02:39AM +0200, Thomas Huth wrote:
>> On 10.05.2016 19:32, Andrew Jones wrote:
>>> Search up the tree until we find #address-cells/#size-cells.
>>> Also only assign outputs if both address and size are found.
>>>
>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>> ---
>>>  lib/devicetree.c | 17 +++++++++++++----
>>>  lib/devicetree.h |  3 ++-
>>>  2 files changed, 15 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/lib/devicetree.c b/lib/devicetree.c
>>> index a5c7f7c69ddfd..d3751e2b7e7f9 100644
>>> --- a/lib/devicetree.c
>>> +++ b/lib/devicetree.c
>>> @@ -24,21 +24,30 @@ int dt_get_nr_cells(int fdtnode, u32 *nr_address_cells, u32 *nr_size_cells)
>>>  {
>>>  	const struct fdt_property *prop;
>>>  	u32 *nr_cells;
>>> -	int len;
>>> +	int len, nac, nsc;
>>> +
>>> +	while (1) {
>>> +		prop = fdt_get_property(fdt, fdtnode, "#address-cells", &len);
>>> +		if (prop != NULL || len != -FDT_ERR_NOTFOUND)
>>> +			break;
>>> +		fdtnode = fdt_parent_offset(fdt, fdtnode);
>>> +	}
>>
>> Why do you need this search? ePAPR clearly states:
>>
>> "The #address-cells and #size-cells properties are not inherited from
>> ancestors in the device tree. They shall be explicitly defined."
> 
> You are right.
> 
> My skimming of Documentation/devicetree/booting-without-of.txt was
> sloppy and I missed basically the same quote
> 
> "Note that the parent's parent definitions of #address-cells and
> #size-cells are not inherited so every node with children must
> specify them."
> 
>>
>> So as far as I can see, it should always be enough to look up the
>> properties in the parent of the node, no need for a recursion here?
> 
> So, yes, I should look in the current node, and then the parent, and
> then just assert.

AFAIK the #address-cells and #size-cells properties of the current node
only apply to the _children_ of the current node, but not to itself. I
think you always have to look at the parent to find out the correct
value for the current node.

 Thomas

Re: [kvm-unit-tests PATCH 1/2] devicetree: search up tree in dt_get_nr_cells

[Andrew Jones]

On Wed, May 11, 2016 at 11:28:36AM +0200, Thomas Huth wrote:
> On 11.05.2016 11:04, Andrew Jones wrote:
> > On Wed, May 11, 2016 at 09:02:39AM +0200, Thomas Huth wrote:
> >> On 10.05.2016 19:32, Andrew Jones wrote:
> >>> Search up the tree until we find #address-cells/#size-cells.
> >>> Also only assign outputs if both address and size are found.
> >>>
> >>> Signed-off-by: Andrew Jones <drjones@redhat.com>
> >>> ---
> >>>  lib/devicetree.c | 17 +++++++++++++----
> >>>  lib/devicetree.h |  3 ++-
> >>>  2 files changed, 15 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/lib/devicetree.c b/lib/devicetree.c
> >>> index a5c7f7c69ddfd..d3751e2b7e7f9 100644
> >>> --- a/lib/devicetree.c
> >>> +++ b/lib/devicetree.c
> >>> @@ -24,21 +24,30 @@ int dt_get_nr_cells(int fdtnode, u32 *nr_address_cells, u32 *nr_size_cells)
> >>>  {
> >>>  	const struct fdt_property *prop;
> >>>  	u32 *nr_cells;
> >>> -	int len;
> >>> +	int len, nac, nsc;
> >>> +
> >>> +	while (1) {
> >>> +		prop = fdt_get_property(fdt, fdtnode, "#address-cells", &len);
> >>> +		if (prop != NULL || len != -FDT_ERR_NOTFOUND)
> >>> +			break;
> >>> +		fdtnode = fdt_parent_offset(fdt, fdtnode);
> >>> +	}
> >>
> >> Why do you need this search? ePAPR clearly states:
> >>
> >> "The #address-cells and #size-cells properties are not inherited from
> >> ancestors in the device tree. They shall be explicitly defined."
> > 
> > You are right.
> > 
> > My skimming of Documentation/devicetree/booting-without-of.txt was
> > sloppy and I missed basically the same quote
> > 
> > "Note that the parent's parent definitions of #address-cells and
> > #size-cells are not inherited so every node with children must
> > specify them."
> > 
> >>
> >> So as far as I can see, it should always be enough to look up the
> >> properties in the parent of the node, no need for a recursion here?
> > 
> > So, yes, I should look in the current node, and then the parent, and
> > then just assert.
> 
> AFAIK the #address-cells and #size-cells properties of the current node
> only apply to the _children_ of the current node, but not to itself. I
> think you always have to look at the parent to find out the correct
> value for the current node.

Thanks for this clarification. I'll rework this API to allow fetching
a node's *-cells proprieties (when you know the node you're asking
about is the parent, and you just want get this information) and for
asking for a child-node's *-cells when doing translation (which means
getting it from the parent).

drew

[kvm-unit-tests PATCH 2/2] devicetree: use node #address/size-cells

[Andrew Jones]

Don't assume all pbus (cpu physical bus) translations will use the same
number of address and size cells as are defined in the root node. Also,
drop the caching of the address/size cells in the bus structure, as that
was a bad idea, and unused anyway.

(I was tempted to completely remove dt_pbus_translate_node, which still
 uses the root node's address/size cells, but I kept it, as it offers a
 nice shortcut for when we are sure we can use the root node's cells.)

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 lib/devicetree.c | 25 +++++++++++++++++--------
 lib/devicetree.h | 10 +++-------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/lib/devicetree.c b/lib/devicetree.c
index d3751e2b7e7f9..3d31f694b0d77 100644
--- a/lib/devicetree.c
+++ b/lib/devicetree.c
@@ -87,13 +87,14 @@ int dt_get_reg(int fdtnode, int regidx, struct dt_reg *reg)
 	return 0;
 }
 
-int dt_pbus_translate_node(int fdtnode, int regidx,
-			   struct dt_pbus_reg *pbus_reg)
+static int __dt_pbus_translate_node(int fdtnode, int regidx,
+				    struct dt_pbus_reg *pbus_reg,
+				    u32 nr_address_cells, u32 nr_size_cells)
 {
 	struct dt_reg raw_reg;
 	int ret;
 
-	dt_reg_init(&raw_reg, root_nr_address_cells, root_nr_size_cells);
+	dt_reg_init(&raw_reg, nr_address_cells, nr_size_cells);
 
 	ret = dt_get_reg(fdtnode, regidx, &raw_reg);
 	if (ret < 0)
@@ -107,10 +108,22 @@ int dt_pbus_translate_node(int fdtnode, int regidx,
 	return 0;
 }
 
+int dt_pbus_translate_node(int fdtnode, int regidx,
+			   struct dt_pbus_reg *pbus_reg)
+{
+	return __dt_pbus_translate_node(fdtnode, regidx, pbus_reg,
+			root_nr_address_cells, root_nr_size_cells);
+}
+
 int dt_pbus_translate(const struct dt_device *dev, int regidx,
 		      void *reg)
 {
-	return dt_pbus_translate_node(dev->fdtnode, regidx, reg);
+	u32 nac, nsc;
+	int ret = dt_get_nr_cells(dev->fdtnode, &nac, &nsc);
+
+	if (ret < 0)
+		return ret;
+	return __dt_pbus_translate_node(dev->fdtnode, regidx, reg, nac, nsc);
 }
 
 int dt_bus_match_any(const struct dt_device *dev __unused, int fdtnode)
@@ -278,7 +291,6 @@ int dt_get_default_console_node(void)
 
 int dt_init(const void *fdt_ptr)
 {
-	struct dt_bus *defbus = (struct dt_bus *)&dt_default_bus;
 	int root, ret;
 
 	ret = fdt_check_header(fdt_ptr);
@@ -295,8 +307,5 @@ int dt_init(const void *fdt_ptr)
 	if (ret < 0)
 		return ret;
 
-	defbus->nr_address_cells = root_nr_address_cells;
-	defbus->nr_size_cells = root_nr_size_cells;
-
 	return 0;
 }
diff --git a/lib/devicetree.h b/lib/devicetree.h
index 09f2509409615..dc2f11b6e9b36 100644
--- a/lib/devicetree.h
+++ b/lib/devicetree.h
@@ -66,9 +66,6 @@ struct dt_bus {
 	 *  - a negative FDT_ERR_* value on failure
 	 */
 	int (*translate)(const struct dt_device *dev, int regidx, void *reg);
-
-	/* the bus #address-cells and #size-cells properties */
-	u32 nr_address_cells, nr_size_cells;
 };
 
 /* dt_bus_match_any matches any fdt node, i.e. it always returns true */
@@ -92,7 +89,7 @@ static inline dt_pbus_addr_t dt_pbus_read_cells(u32 nr_cells, u32 *cells)
 
 /*
  * dt_pbus_translate translates device node regs for the
- * processor bus using the root node's #address-cells and
+ * processor bus using the node's #address-cells and
  * #size-cells and dt_pbus_read_cells()
  * returns
  *  - zero on success
@@ -103,7 +100,8 @@ extern int dt_pbus_translate(const struct dt_device *dev, int regidx,
 
 /*
  * dt_pbus_translate_node is the same as dt_pbus_translate but
- * operates on an fdt node instead of a dt_device
+ * operates on an fdt node, instead of a dt_device, and uses the
+ * root node's #address-cells and #size-cells.
  */
 extern int dt_pbus_translate_node(int fdtnode, int regidx,
 				  struct dt_pbus_reg *reg);
@@ -125,8 +123,6 @@ static inline int dt_pbus_get_base(const struct dt_device *dev,
  * dt_bus_init_defaults initializes @bus with
  *  match		<- dt_bus_match_any
  *  translate		<- dt_pbus_translate
- *  nr_address_cells	<- #address-cells of the root node
- *  nr_size_cells	<- #size-cells of the root node
  */
 extern void dt_bus_init_defaults(struct dt_bus *bus);
 
-- 
2.4.11

Re: [kvm-unit-tests PATCH 2/2] devicetree: use node #address/size-cells

[Thomas Huth]

On 10.05.2016 19:32, Andrew Jones wrote:
> Don't assume all pbus (cpu physical bus) translations will use the same
> number of address and size cells as are defined in the root node.

That's a good idea, of course.

> Also,
> drop the caching of the address/size cells in the bus structure, as that
> was a bad idea, and unused anyway.

Maybe do it in a separate patch?

> (I was tempted to completely remove dt_pbus_translate_node, which still
>  uses the root node's address/size cells, but I kept it, as it offers a
>  nice shortcut for when we are sure we can use the root node's cells.)

Sounds like this function could cause quite a bit of confusion in the
future... maybe it would be better to remove it so that no one can use
it in the wrong way anymore?

 Thomas

Re: [kvm-unit-tests PATCH 2/2] devicetree: use node #address/size-cells

[Andrew Jones]

On Wed, May 11, 2016 at 09:12:50AM +0200, Thomas Huth wrote:
> On 10.05.2016 19:32, Andrew Jones wrote:
> > Don't assume all pbus (cpu physical bus) translations will use the same
> > number of address and size cells as are defined in the root node.
> 
> That's a good idea, of course.
> 
> > Also,
> > drop the caching of the address/size cells in the bus structure, as that
> > was a bad idea, and unused anyway.
> 
> Maybe do it in a separate patch?

OK

> 
> > (I was tempted to completely remove dt_pbus_translate_node, which still
> >  uses the root node's address/size cells, but I kept it, as it offers a
> >  nice shortcut for when we are sure we can use the root node's cells.)
> 
> Sounds like this function could cause quite a bit of confusion in the
> future... maybe it would be better to remove it so that no one can use
> it in the wrong way anymore?

Agreed, and on 2nd (or is it 3rd) thought, I can keep it, but there's no
good reason to use the root node's cells. It can work just like before
this patch, that is, be the equivalent of dt_pbus_translate, but operate
on the node instead of the device.

Thanks,
drew

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