From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Date: Tue, 4 Aug 2015 16:26:29 +0200 Subject: [U-Boot] [PATCH] fdt: Fix fdtdec_get_addr_size() for 64-bit In-Reply-To: References: <1437670290-25660-1-git-send-email-swarren@wwwdotorg.org> Message-ID: <20150804142628.GA3812@ulmo.nvidia.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Sun, Aug 02, 2015 at 03:27:53PM -0600, Simon Glass wrote: > Hi, > > On 27 July 2015 at 11:13, Simon Glass wrote: > > Hi, > > > > On 23 July 2015 at 10:51, Stephen Warren wrote: > >> From: Thierry Reding > >> > >> Signed-off-by: Thierry Reding > >> Signed-off-by: Tom Warren > >> Signed-off-by: Stephen Warren > >> --- > >> 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: