On Mon, Mar 14, 2016 at 10:10:58PM +0100, Michal Simek wrote:
> On 13.3.2016 02:54, Simon Glass wrote:
> > Hi Michal,
> >
> > On 16 February 2016 at 09:10, Michal Simek <michal.simek@xxxxxxxxxx> wrote:
> >> Hi Simon,
> >>
> >> On 16.2.2016 17:00, Simon Glass wrote:
> >>> Hi Michal,
> >>>
> >>> On 15 February 2016 at 02:58, Michal Simek <michal.simek@xxxxxxxxxx> wrote:
> >>>> Hi Simon,
> >>>>
> >>>> On 10.2.2016 13:04, Michal Simek wrote:
> >>>>> Read #address-cells and #size-cells from parent if they are not present in
> >>>>> current node.
> >>>>>
> >>>>> Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
> >>>>> ---
> >>>>>
> >>>>> I have code which read information about memory for zynqmp but memory
> >>>>> node most of the time doesn't contain #address/size-cells which are
> >>>>> present in parent node.
> >>>>> That's why let's try to read it from parent.
> >>>>>
> >>>>> Also I think that we shouldn't return 2 if property is not found because
> >>>>> it has side effect on 32bit systems with #address/size-cells = <1>;
> >>>>>
> >>>>> ---
> >>>>> lib/libfdt/fdt_addresses.c | 18 ++++++++++++++----
> >>>>> 1 file changed, 14 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/lib/libfdt/fdt_addresses.c b/lib/libfdt/fdt_addresses.c
> >>>>> index 76054d98e5fd..b164d0988079 100644
> >>>>> --- a/lib/libfdt/fdt_addresses.c
> >>>>> +++ b/lib/libfdt/fdt_addresses.c
> >>>>> @@ -19,10 +19,15 @@ int fdt_address_cells(const void *fdt, int nodeoffset)
> >>>>> const fdt32_t *ac;
> >>>>> int val;
> >>>>> int len;
> >>>>> + int parent;
> >>>>>
> >>>>> ac = fdt_getprop(fdt, nodeoffset, "#address-cells", &len);
> >>>>> - if (!ac)
> >>>>> - return 2;
> >>>>> + if (!ac) {
> >>>>> + parent = fdt_parent_offset(fdt, nodeoffset);
> >>>>> + ac = fdt_getprop(fdt, parent, "#address-cells", &len);
> >>>>> + if (!ac)
> >>>>> + return 2;
> >>>>> + }
> >>>>>
> >>>>> if (len != sizeof(*ac))
> >>>>> return -FDT_ERR_BADNCELLS;
> >>>>> @@ -39,10 +44,15 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
> >>>>> const fdt32_t *sc;
> >>>>> int val;
> >>>>> int len;
> >>>>> + int parent;
> >>>>>
> >>>>> sc = fdt_getprop(fdt, nodeoffset, "#size-cells", &len);
> >>>>> - if (!sc)
> >>>>> - return 2;
> >>>>> + if (!sc) {
> >>>>> + parent = fdt_parent_offset(fdt, nodeoffset);
> >>>>> + sc = fdt_getprop(fdt, parent, "#size-cells", &len);
> >>>>> + if (!sc)
> >>>>> + return 2;
> >>>>> + }
> >>>>>
> >>>>> if (len != sizeof(*sc))
> >>>>> return -FDT_ERR_BADNCELLS;
> >>>>>
> >>>>
> >>>> Simon: Any comment?
> >>>
> >>> It seems risky to change the behaviour here. Also fdt_parent_offset() is slow.
> >>>
> >>> Can you point me to the binding / example DT that you are trying to parse?
> >>
> >> Look at dram_init(), etc.
> >> https://github.com/Xilinx/u-boot-xlnx/blob/master/board/xilinx/zynqmp/zynqmp.c
> >>
> >> fdt_get_reg() is calling fdt_size_cells()
> >>
> >>
> >> And this is DTS fragment.
> >> #address-cells = <2>;
> >> #size-cells = <1>;
> >>
> >> memory {
> >> device_type = "memory";
> >> reg = <0x0 0x0 0x80000000>, <0x8 0x00000000 0x80000000>;
> >> };
> >>
> >> Code is in memory node I need to work with and asking for size-cells.
> >> Current code returns 2 instead of error and the rest of code just works
> >> with size = 2 which is incorrect for this setup.
> >>
> >> I have already changed size-cells = 2 in our repo because I need to
> >> support for more than 4GB memory anyway but this should point to the
> >> problem in that generic functions.
> >
> > I think this should go in a higher-level function. I very much doubt
> > that this patch would be accepted upstream.
> >
> > Can you find the caller and make it call this function again (for the
> > parent) when no nothing is found on the first call? Hopefully this
> > caller will have access to the parent node and will not need to call
> > fdt_parent_offset().
>
> The funny part is that nothing is found means return 2. If this returns
> something <0 then there is not a problem to try it with parents.
I don't have the full context of this thread, so it's a bit hard to be
sure, but this doesn't look right from what I can see. Two things to
remember here:
* #address-cells and #size-cells describe the format of addresses
for children of this node, not this node itself. So if you're
looking to parse 'reg' for this node, you *always* need to look at
the parent, not just as a fallback.
* #address-cells and #size-cells are *not* inherited. If they're
missing in a node, then the format for its children's addresses is
2 cell addresses and 2 cell sizes, it is *not* correct to look at
the next parent up for these properties.
--
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
Attachment:
signature.asc
Description: PGP signature
