Re: [U-Boot] [PATCH] fdt: Try to read #address-cells/size-cells from parent

[David Gibson]

On Wed, Mar 16, 2016 at 05:18:25PM +0100, Michal Simek wrote:
> Hi David,
> 
> On 15.3.2016 01:27, David Gibson wrote:
> > 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  
> >>> wrote:
>  Hi Simon,
> 
>  On 16.2.2016 17:00, Simon Glass wrote:
> > Hi Michal,
> >
> > On 15 February 2016 at 02:58, Michal Simek  
> > 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 
> >>> ---
> >>>
> >>> 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", );
> >>> - if (!ac)
> >>> - return 2;
> >>> + if (!ac) {
> >>> + parent = fdt_parent_offset(fdt, nodeoffset);
> >>> + ac = fdt_getprop(fdt, parent, "#address-cells", );
> >>> + 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", );
> >>> - if (!sc)
> >>> - return 2;
> >>> + if (!sc) {
> >>> + parent = fdt_parent_offset(fdt, nodeoffset);
> >>> + sc = fdt_getprop(fdt, parent, "#size-cells", );
> >>> + 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 0x8000>, <0x8 0x 0x8000>;
>  };
> 
>  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 

Re: [U-Boot] [PATCH] fdt: Try to read #address-cells/size-cells from parent

[Michal Simek]

On 16.3.2016 23:47, David Gibson wrote:
> On Wed, Mar 16, 2016 at 05:18:25PM +0100, Michal Simek wrote:
>> Hi David,
>>
>> On 15.3.2016 01:27, David Gibson wrote:
>>> 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  
> wrote:
>> Hi Simon,
>>
>> On 16.2.2016 17:00, Simon Glass wrote:
>>> Hi Michal,
>>>
>>> On 15 February 2016 at 02:58, Michal Simek  
>>> 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 
> ---
>
> 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", );
> - if (!ac)
> - return 2;
> + if (!ac) {
> + parent = fdt_parent_offset(fdt, nodeoffset);
> + ac = fdt_getprop(fdt, parent, "#address-cells", );
> + 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", );
> - if (!sc)
> - return 2;
> + if (!sc) {
> + parent = fdt_parent_offset(fdt, nodeoffset);
> + sc = fdt_getprop(fdt, parent, "#size-cells", );
> + 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 0x8000>, <0x8 0x 0x8000>;
>> };
>>
>> 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 

Re: [U-Boot] [PATCH] fdt: Try to read #address-cells/size-cells from parent

[Michal Simek]

Hi David,

On 15.3.2016 01:27, David Gibson wrote:
> 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  wrote:
 Hi Simon,

 On 16.2.2016 17:00, Simon Glass wrote:
> Hi Michal,
>
> On 15 February 2016 at 02:58, Michal Simek  
> 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 
>>> ---
>>>
>>> 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", );
>>> - if (!ac)
>>> - return 2;
>>> + if (!ac) {
>>> + parent = fdt_parent_offset(fdt, nodeoffset);
>>> + ac = fdt_getprop(fdt, parent, "#address-cells", );
>>> + 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", );
>>> - if (!sc)
>>> - return 2;
>>> + if (!sc) {
>>> + parent = fdt_parent_offset(fdt, nodeoffset);
>>> + sc = fdt_getprop(fdt, parent, "#size-cells", );
>>> + 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 0x8000>, <0x8 0x 0x8000>;
 };

 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.

ok that means that I should fix my code to find parent of current node
and then read address and size cells.

fdt - actual memory node
parent = fdt_parent_offset(fdt, nodeoffset);

Re: [U-Boot] [PATCH] fdt: Try to read #address-cells/size-cells from parent

[David Gibson]

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  wrote:
> >> Hi Simon,
> >>
> >> On 16.2.2016 17:00, Simon Glass wrote:
> >>> Hi Michal,
> >>>
> >>> On 15 February 2016 at 02:58, Michal Simek  
> >>> 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 
> > ---
> >
> > 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", );
> > - if (!ac)
> > - return 2;
> > + if (!ac) {
> > + parent = fdt_parent_offset(fdt, nodeoffset);
> > + ac = fdt_getprop(fdt, parent, "#address-cells", );
> > + 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", );
> > - if (!sc)
> > - return 2;
> > + if (!sc) {
> > + parent = fdt_parent_offset(fdt, nodeoffset);
> > + sc = fdt_getprop(fdt, parent, "#size-cells", );
> > + 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 0x8000>, <0x8 0x 0x8000>;
> >> };
> >>
> >> 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.

Re: [U-Boot] [PATCH] fdt: Try to read #address-cells/size-cells from parent

[Michal Simek]

On 13.3.2016 02:54, Simon Glass wrote:
> Hi Michal,
> 
> On 16 February 2016 at 09:10, Michal Simek  wrote:
>> Hi Simon,
>>
>> On 16.2.2016 17:00, Simon Glass wrote:
>>> Hi Michal,
>>>
>>> On 15 February 2016 at 02:58, Michal Simek  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 
> ---
>
> 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", );
> - if (!ac)
> - return 2;
> + if (!ac) {
> + parent = fdt_parent_offset(fdt, nodeoffset);
> + ac = fdt_getprop(fdt, parent, "#address-cells", );
> + 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", );
> - if (!sc)
> - return 2;
> + if (!sc) {
> + parent = fdt_parent_offset(fdt, nodeoffset);
> + sc = fdt_getprop(fdt, parent, "#size-cells", );
> + 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 0x8000>, <0x8 0x 0x8000>;
>> };
>>
>> 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.

Thanks,
Michal

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] fdt: Try to read #address-cells/size-cells from parent

[Simon Glass]

Hi Michal,

On 16 February 2016 at 09:10, Michal Simek  wrote:
> Hi Simon,
>
> On 16.2.2016 17:00, Simon Glass wrote:
>> Hi Michal,
>>
>> On 15 February 2016 at 02:58, Michal Simek  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 
 ---

 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", );
 - if (!ac)
 - return 2;
 + if (!ac) {
 + parent = fdt_parent_offset(fdt, nodeoffset);
 + ac = fdt_getprop(fdt, parent, "#address-cells", );
 + 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", );
 - if (!sc)
 - return 2;
 + if (!sc) {
 + parent = fdt_parent_offset(fdt, nodeoffset);
 + sc = fdt_getprop(fdt, parent, "#size-cells", );
 + 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 0x8000>, <0x8 0x 0x8000>;
> };
>
> 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().

>
> Thanks,
> Michal
>

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] fdt: Try to read #address-cells/size-cells from parent

[Michal Simek]

Hi Simon,

On 16.2.2016 17:00, Simon Glass wrote:
> Hi Michal,
> 
> On 15 February 2016 at 02:58, Michal Simek  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 
>>> ---
>>>
>>> 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", );
>>> - if (!ac)
>>> - return 2;
>>> + if (!ac) {
>>> + parent = fdt_parent_offset(fdt, nodeoffset);
>>> + ac = fdt_getprop(fdt, parent, "#address-cells", );
>>> + 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", );
>>> - if (!sc)
>>> - return 2;
>>> + if (!sc) {
>>> + parent = fdt_parent_offset(fdt, nodeoffset);
>>> + sc = fdt_getprop(fdt, parent, "#size-cells", );
>>> + 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 0x8000>, <0x8 0x 0x8000>;
};

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.

Thanks,
Michal

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] fdt: Try to read #address-cells/size-cells from parent

[Simon Glass]

Hi Michal,

On 15 February 2016 at 02:58, Michal Simek  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 
>> ---
>>
>> 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", );
>> - if (!ac)
>> - return 2;
>> + if (!ac) {
>> + parent = fdt_parent_offset(fdt, nodeoffset);
>> + ac = fdt_getprop(fdt, parent, "#address-cells", );
>> + 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", );
>> - if (!sc)
>> - return 2;
>> + if (!sc) {
>> + parent = fdt_parent_offset(fdt, nodeoffset);
>> + sc = fdt_getprop(fdt, parent, "#size-cells", );
>> + 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?

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] fdt: Try to read #address-cells/size-cells from parent

[Michal Simek]

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 
> ---
> 
> 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", );
> - if (!ac)
> - return 2;
> + if (!ac) {
> + parent = fdt_parent_offset(fdt, nodeoffset);
> + ac = fdt_getprop(fdt, parent, "#address-cells", );
> + 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", );
> - if (!sc)
> - return 2;
> + if (!sc) {
> + parent = fdt_parent_offset(fdt, nodeoffset);
> + sc = fdt_getprop(fdt, parent, "#size-cells", );
> + if (!sc)
> + return 2;
> + }
>  
>   if (len != sizeof(*sc))
>   return -FDT_ERR_BADNCELLS;
> 

Simon: Any comment?

Thanks,
Michal
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] fdt: Try to read #address-cells/size-cells from parent

[Michal Simek]

Read #address-cells and #size-cells from parent if they are not present in
current node.

Signed-off-by: Michal Simek 
---

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", );
-   if (!ac)
-   return 2;
+   if (!ac) {
+   parent = fdt_parent_offset(fdt, nodeoffset);
+   ac = fdt_getprop(fdt, parent, "#address-cells", );
+   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", );
-   if (!sc)
-   return 2;
+   if (!sc) {
+   parent = fdt_parent_offset(fdt, nodeoffset);
+   sc = fdt_getprop(fdt, parent, "#size-cells", );
+   if (!sc)
+   return 2;
+   }
 
if (len != sizeof(*sc))
return -FDT_ERR_BADNCELLS;
-- 
1.9.1

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot