From mboxrd@z Thu Jan 1 00:00:00 1970 From: Benjamin Fair Subject: Re: [PATCH] libfdt: add helpers to read address and size from reg Date: Mon, 5 Dec 2016 11:46:31 -0600 Message-ID: References: <1478710712-25010-1-git-send-email-b-fair@ti.com> <20161125105125.GJ12287@umbus.fritz.box> <2f744a7f-5112-c83e-d47a-ce9fef491dde@ti.com> <20161202031256.GB10089@umbus.fritz.box> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20161202031256.GB10089-K0bRW+63XPQe6aEkudXLsA@public.gmane.org> Sender: devicetree-compiler-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Content-Type: text/plain; charset="us-ascii"; format="flowed" To: David Gibson Cc: Jon Loeliger , Nishanth Menon , Rob Herring , devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org On 12/01/2016 09:12 PM, David Gibson wrote: > On Wed, Nov 30, 2016 at 01:35:04PM -0600, Benjamin Fair wrote: >> On 11/25/2016 04:51 AM, David Gibson wrote: >>> On Wed, Nov 09, 2016 at 10:58:32AM -0600, Benjamin Fair wrote: >> [...] >>> >>> I like the concept of a helper to read entries from reg, but there >>> some things about the execution of it I think need some more thought. >>> >> >> Thanks for the review. >> >> [...] >>>> diff --git a/libfdt/fdt_addresses.c b/libfdt/fdt_addresses.c >>>> index eff4dbc..92cbed9 100644 >>>> --- a/libfdt/fdt_addresses.c >>>> +++ b/libfdt/fdt_addresses.c >>>> @@ -55,6 +55,9 @@ >>>> >>>> #include "libfdt_internal.h" >>>> >>>> +#define BYTES_PER_CELL 4 >>>> +#define BITS_PER_CELL 32 >>> >>> You shouldn't need these. BYTES_PER_CELL == sizeof(fdt32_t). >>> >> >> Of course. Thanks. I'll get rid of them. >> >>>> + >>>> int fdt_address_cells(const void *fdt, int nodeoffset) >>>> { >>>> const fdt32_t *ac; >>>> @@ -94,3 +97,62 @@ int fdt_size_cells(const void *fdt, int nodeoffset) >>>> >>>> return val; >>>> } >>>> + >>>> +static uint64_t _fdt_read_cells(const fdt32_t *cells, int n) >>> >>> This is a reasonable helper, but the name is bad. "read_cells" >>> suggests it can read some arbitrary number of cells, but in fact all >>> it can do is read a 32-bit int or a 64-bit int. Plus everything is >>> made up of cells, but more specifically what you're doing here is >>> interpreting several cells as an integer in the usual encoding. >>> >> >> Would "cells_to_integer" be a better name? Or would you recommend something >> else for this? > > cells_to_integer would be ok. Or just fdt_read_integer(). > Thanks for the suggestion. I changed it to fdt_read_integer() >> >>>> +{ >>>> + int i; >>>> + uint64_t res; >>>> + >>>> + /* TODO: Support values larger than 2 cells */ >>> >>> I don't really see any way you could support >2 cells without >>> completely changing the interface. >>> >> >> True. I wanted to have the result be a 128 bit integer, but couldn't find a >> portable way to do so. Is there a better way to go about this? Or is it fine >> to only support at most 2 cells, even though the rest of libfdt supports 4? > > I think just supporting 2 cells is ok - it's useful in enough > practical cases. However, I'd suggest thinking of this in a more > layered way. > > First, create a helper which just locates the relevant pieces of reg > entries, returning addresses and sizes as (fdt32_t *). That's at > least somewhat useful for the >2 cells case, even though you have to > then parse the address/size yourself. In the most common case for > this, PCI (address-cells == 3), you probably want to do that anyway. > This may also be useful for cases which are <= 2 cells, but the > encoding of the address is not just a plain integer. > > You can then polish up fdt_read_integer() a bit and export it. > > Finally you can add another helper which combines these to directly > get you the addr+size as u64 for buses where that's suitable. Make > sure to return an error if #a or #s > 2, of course. > That seems like a good idea. I implemented this for the next revision. >>>> + if (n > 2) >>>> + return -FDT_ERR_BADNCELLS; >>>> + >>>> + res = 0; >>>> + for (i = 0; i < n; i++) { >>>> + res <<= BITS_PER_CELL; >>>> + res |= fdt32_to_cpu(cells[i]); >>>> + } >>>> + >>>> + return res; >>>> +} >>>> + >>>> +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx, >>>> + uintptr_t *addr, size_t *size) >>>> +{ >>>> + int parent; >>>> + int ac, sc, reg_stride; >>>> + int res; >>>> + const fdt32_t *reg; >>>> + >>>> + reg = fdt_getprop(fdt, nodeoffset, "reg", &res); >>>> + if (res < 0) >>>> + return res; >>>> + >>>> + parent = fdt_parent_offset(fdt, nodeoffset); >>>> + if (parent < 0) >>>> + return res; >>> >>> So, fdt_parent_offset() is very expensive, I wouldn't recommend it in >>> a function that's likely to be called a lot like this. Instead I'd >>> suggest a function which takes the parent offset as a parameter, and >>> optionally a wrapper that uses fdt_parent_offset(). >> >> Great idea, I'll do this in the next revision once we have a solution for >> the rest of the comments. >> Would it be OK to pass in the number of address cells and number of size cells directly instead of the parent offset? I was thinking about this in order to minimize repeated work. Plus, the calling function needs to know this information in order to interpret the results anyway. >>> >>>> + >>>> + ac = fdt_address_cells(fdt, parent); >>>> + if (ac < 0) >>>> + return ac; >>>> + >>>> + sc = fdt_size_cells(fdt, parent); >>>> + if (sc < 0) >>>> + return sc; >>>> + >>>> + reg_stride = ac + sc; >>>> + /* >>>> + * res is the number of bytes read and must be an even multiple of the >>>> + * sum of ac and sc. >>>> + */ >>>> + if ((res % (reg_stride * BYTES_PER_CELL)) != 0) >>>> + return -FDT_ERR_BADVALUE; >>>> + >>>> + if (addr) >>>> + *addr = (uintptr_t) _fdt_read_cells(®[reg_stride * idx], ac); >>> >>> I don't think uintptr_t makes sense here. The addresses in the device >>> tree are in whatever bus they're in, and there are a whole stack of >>> reasons that could be unrelated to the pointer size of environment >>> libfdt is running in: >>> - The device may be on a subordinate bus whose addresses need >>> to be translated >>> - Even at the top-level, the reg properties represent >>> *physical* addresses, which may not be the same as virtual >>> addresses in code running on the system >>> - libfdt may be running on a completely different system from the >>> one the device tree in question is aimed at (bootloaders are >>> only one use case for libfdt). >>> >>>> + if (size) >>>> + *size = (size_t) _fdt_read_cells(®[ac + reg_stride * idx], >>>> + sc); >>> >>> Likewise size_t isn't necessarily right here, although I suspect it's >>> less likely to break in practice. >>> >> >> Hmm... Is it fine to use uint64_t for both of these instead then? > > Yes, I think that's reasonable. Or you could even use unsigned long > or unsigned long long - as long as you error whenever #cells is too > big for the size of that type. uint64_t is probably clearer and more > consistent, though. > Thanks, I changed it to use uint64_t -- Benjamin Fair