* Re: [PATCH] fdt: Try to read #address-cells/size-cells from parent
[not found] ` <56C349E8.9030808-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
@ 2016-03-13 1:54 ` Simon Glass
2016-03-14 21:10 ` Michal Simek
0 siblings, 1 reply; 6+ messages in thread
From: Simon Glass @ 2016-03-13 1:54 UTC (permalink / raw)
To: Michal Simek; +Cc: U-Boot Mailing List, Devicetree Compiler
Hi Michal,
On 16 February 2016 at 09:10, Michal Simek <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> 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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> 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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>>
>>>> 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().
>
> Thanks,
> Michal
>
Regards,
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fdt: Try to read #address-cells/size-cells from parent
2016-03-13 1:54 ` [PATCH] fdt: Try to read #address-cells/size-cells from parent Simon Glass
@ 2016-03-14 21:10 ` Michal Simek
[not found] ` <56E728E2.5050905-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: Michal Simek @ 2016-03-14 21:10 UTC (permalink / raw)
To: Simon Glass, Michal Simek; +Cc: U-Boot Mailing List, Devicetree Compiler
On 13.3.2016 02:54, Simon Glass wrote:
> Hi Michal,
>
> On 16 February 2016 at 09:10, Michal Simek <michal.simek@xilinx.com> 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@xilinx.com> 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@xilinx.com>
>>>>> ---
>>>>>
>>>>> 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.
Thanks,
Michal
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fdt: Try to read #address-cells/size-cells from parent
[not found] ` <56E728E2.5050905-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
@ 2016-03-15 0:27 ` David Gibson
[not found] ` <20160315002743.GC15272-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: David Gibson @ 2016-03-15 0:27 UTC (permalink / raw)
To: Michal Simek; +Cc: Simon Glass, U-Boot Mailing List, Devicetree Compiler
[-- Attachment #1: Type: text/plain, Size: 5392 bytes --]
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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> 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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> 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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >>>>> ---
> >>>>>
> >>>>> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fdt: Try to read #address-cells/size-cells from parent
[not found] ` <20160315002743.GC15272-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org>
@ 2016-03-16 16:18 ` Michal Simek
[not found] ` <56E98751.4060007-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: Michal Simek @ 2016-03-16 16:18 UTC (permalink / raw)
To: David Gibson, Michal Simek
Cc: Simon Glass, U-Boot Mailing List, Devicetree Compiler
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 <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> 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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> 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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>>>>>>> ---
>>>>>>>
>>>>>>> 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.
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);
address_cells = fdt_address_cells(parent, nodeoffset);
size_cells = fdt_size_cells(parent, nodeoffset);
> * #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.
>
ok. And I expect that this is in spec.
Definitely thank you for your input it was very helpful.
Thanks,
Michal
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fdt: Try to read #address-cells/size-cells from parent
[not found] ` <56E98751.4060007-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
@ 2016-03-16 22:47 ` David Gibson
2016-03-16 23:21 ` Michal Simek
0 siblings, 1 reply; 6+ messages in thread
From: David Gibson @ 2016-03-16 22:47 UTC (permalink / raw)
To: Michal Simek; +Cc: Simon Glass, U-Boot Mailing List, Devicetree Compiler
[-- Attachment #1: Type: text/plain, Size: 6574 bytes --]
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 <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> 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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> 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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> 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.
>
> 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);
> address_cells = fdt_address_cells(parent, nodeoffset);
> size_cells = fdt_size_cells(parent, nodeoffset);
That's correct. One way to look at it that #address-cells and
#size-cells are properties of the bus anchored at this node, rather
than properties of the node itself.
> > * #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.
> >
>
> ok. And I expect that this is in spec.
Yes. Actually relying on the default values is discouraged, of
course: you should include #address-cells and #size-cells for any node
with children.
> Definitely thank you for your input it was very helpful.
>
> Thanks,
> Michal
>
--
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fdt: Try to read #address-cells/size-cells from parent
2016-03-16 22:47 ` David Gibson
@ 2016-03-16 23:21 ` Michal Simek
0 siblings, 0 replies; 6+ messages in thread
From: Michal Simek @ 2016-03-16 23:21 UTC (permalink / raw)
To: David Gibson, Michal Simek; +Cc: U-Boot Mailing List, Devicetree Compiler
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 <michal.simek@xilinx.com> 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@xilinx.com> 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@xilinx.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> 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.
>>
>> 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);
>> address_cells = fdt_address_cells(parent, nodeoffset);
>> size_cells = fdt_size_cells(parent, nodeoffset);
>
> That's correct. One way to look at it that #address-cells and
> #size-cells are properties of the bus anchored at this node, rather
> than properties of the node itself.
>
>>> * #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.
>>>
>>
>> ok. And I expect that this is in spec.
>
> Yes. Actually relying on the default values is discouraged, of
> course: you should include #address-cells and #size-cells for any node
> with children.
Good thanks. I will test it when I have direct access to the board but I
can't see any problem if this is in spec.
Thanks,
Michal
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-03-16 23:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <5b7c7b1b2f6a81b2f97f1496558b9d03a59d0288.1455105881.git.michal.simek@xilinx.com>
[not found] ` <56C1A148.70905@xilinx.com>
[not found] ` <CAPnjgZ3kv2gdzmktbqBApnxnH_pm3ZmXcZCgd2dpE8VhxbZ-hg@mail.gmail.com>
[not found] ` <56C349E8.9030808@xilinx.com>
[not found] ` <56C349E8.9030808-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
2016-03-13 1:54 ` [PATCH] fdt: Try to read #address-cells/size-cells from parent Simon Glass
2016-03-14 21:10 ` Michal Simek
[not found] ` <56E728E2.5050905-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
2016-03-15 0:27 ` David Gibson
[not found] ` <20160315002743.GC15272-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org>
2016-03-16 16:18 ` Michal Simek
[not found] ` <56E98751.4060007-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
2016-03-16 22:47 ` David Gibson
2016-03-16 23:21 ` Michal Simek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).