[U-Boot] [PATCH] fdt: Fix fdtdec_get_addr_size() for 64-bit

[Thierry Reding]

On Sun, Aug 02, 2015 at 03:27:53PM -0600, Simon Glass wrote:
> Hi,
> 
> On 27 July 2015 at 11:13, Simon Glass <sjg at chromium.org> wrote:
> > Hi,
> >
> > On 23 July 2015 at 10:51, Stephen Warren <swarren at wwwdotorg.org> wrote:
> >> From: Thierry Reding <treding at nvidia.com>
> >>
> >> Signed-off-by: Thierry Reding <treding at nvidia.com>
> >> Signed-off-by: Tom Warren <twarren at nvidia.com>
> >> Signed-off-by: Stephen Warren <swarren at nvidia.com>
> >> ---
> >> Simon,
> >>
> >> When Thierry first posted this patch, you responded:
> >>
> >>> > +       parent = fdt_parent_offset(blob, node);
> >>>
> >>> This function is very slow as it must scan the whole tree. Can we
> >>> instead pass in the parent node?
> >>
> >> I don't think that's possible in general. This function is called from
> >> fdtdec_get_addr(), and it's easy to find call sites of that function that
> >> don't have the parent node available. IIRC, the first couple of example I
> >> found scan the DT for a node with a certain compatible value, or look up
> >> nodes via aliases, rather than being called while parsing the DT in a
> >> top-down tree-like fashion, where the parent node is easily available. I
> >> didn't do an exhaustive search after I found a few problematic cases.
> >>
> >>> Also, how about (in addition) a
> >>> version of this function that works for devices? Like:
> >>>
> >>> device_get_addr_size(struct udevice *dev, ...)
> >>>
> >>> so that it can handle this for you.
> >>
> >> That sounds like a separate patch?
> >
> > Yes, but I think we should get it in there so that people don't start
> > using this (wildly inefficient) function when they don't need to. I
> > think by passing in the parent node we force people to think about the
> > cost.
> >
> > Yes the driver model function can be a separate patch, but let's get
> > it in at about the same time. We have dev_get_addr() so could have
> > dev_get_addr_size().
> >
> >>
> >> Equally, I see that struct udevice contains an of_offset field, but no
> >> parent_of_offset or similar. There is a struct udevice *parent though;
> >> is the struct udevice hierarchy guaranteed to 100% match the DT
> >> hierarchy? I know this isn't necessarily guaranteed in Linux's device
> >> model for example.
> >
> > Yes it is 100% guaranteed, so dev->parent->of_offset will do the right thing.
> >
> >>
> >> As such, this patch seems OK to me as-is.
> >> ---
> >>  lib/fdtdec.c | 56 ++++++++++++++++++++++++++++++++++++--------------------
> >>  1 file changed, 36 insertions(+), 20 deletions(-)
> >>
> 
> This patch has been applied. I'm going to post a revert of this patch.
> Please can you take a look at the comments above? In particular this
> function is called from dev_get_addr() which is a core driver model
> function. It needs to be fast - and in fact dev_get_addr() already has
> access to the parent node.

Perhaps this could be fixed by doing passing in the parent as an
optional argument and then do something like this:

	if (parent < 0) {
		parent = fdt_parent_offset(blob, node);
		if (parent < 0) {
			...
		}
	}

In that case callers that have access to the parent node already can
pass it in, but others can simply pass in -1 and have the function do
the lookup.

> Also looking a bit closer this patch does a lot more than 'fix it for
> 64-bit'. A commit message would be useful to explain what problems it
> is fixing, etc.
> 
> Another point is that fdt_addr_t and fdt_size_t are supposed to match
> the address size used in the device tree. Is that not the case with
> Tegra210?

You can't assume that #address-cells and #size-cells will be 2 for all
64-bit platforms. Some may well go with #address-cells = 1 and #size-
cells = 1, and I've seen others do #address-cells = 2 and #size-cells =
1. All of these combinations are perfectly valid.

As such, fdt_addr_t and fdt_size_t make sense only if they are the
maximum that the architecture can support. Even so an address could
technically be larger than that, if it's behind a translated bus, for
example.

So what this does is really fix parsing of address and size cells in the
general case, though it would still fail for values of #address-cells or
#size-cells bigger than 2 (because we don't have a datatype that would
be able to contain such large values).

Note that there's also still a corner case that this doesn't handle. The
DT specification states, if I remember correctly, that #address-cells
and #size-cells are inherited. That means with the current code we will
wrongly parse something like this:

	/ {
		...
		#address-cells = <1>;
		#size-cells = <1>;
		...
		bus at XXXXXXXX {
			...
			device at XXXXXXXX {
				...
				reg = <0xXXXXXXXX 0x1000>;
				...
			};
			...
		};
		...
	};

According to the DT specification the bus at XXXXXXXX node would inherit
#address-cells = <1> and #size-cells = <1> from the root node. However
with libfdt what really happens is that since bus at XXXXXXXX does not have
either property it will default to 2 in both cases. I'm not sure if this
really is a problem. Typically nodes are not nested that deeply, or if
they are then, typically, they explicitly contain #address-cells and
#size-cells properties.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150804/20eeb628/attachment.sig>