[U-Boot] [PATCH] fdt: fix address cell count checking in fdt_translate_address()

[Simon Glass]

Hi,

On 3 November 2015 at 02:57, Przemyslaw Marczak <p.marczak at samsung.com> wrote:
> Hello All,
>
>
> On 10/29/2015 06:15 PM, Simon Glass wrote:
>>
>> Hi Stefan,
>>
>> On 28 October 2015 at 08:37, Przemyslaw Marczak <p.marczak at samsung.com>
>> wrote:
>>>
>>> Commit: dm: core: Enable optional use of fdt_translate_address()
>>>
>>> Enables use of this function as default, but after this it's not
>>> possible to get dev address for the case in which: '#size-cells == 0'
>>>
>>> This causes errors when getting address for some GPIOs, for which
>>> the '#size-cells' is set to 0.
>>>
>>> Example error:
>>> '__of_translate_address: Bad cell count for gpx0'
>>>
>>> Allowing for that case by modifying the macro 'OF_CHECK_COUNTS',
>>> (called from )__of_translate_address(), fixes the issue.
>>>
>>> Now, this macro doesn't check, that '#size-cells' is greater than 0.
>>>
>>> This is possible from the specification point of view, but I'm not sure
>>> that it doesn't introduce a regression for other configs.
>>>
>>> Please test and share the results.
>>>
>>> Tested-on: Odroid U3, Odroid X2, Odroid XU3, Sandbox.
>>>
>>> Signed-off-by: Przemyslaw Marczak <p.marczak at samsung.com>
>>> Cc: Masahiro Yamada <yamada.masahiro at socionext.com>
>>> Cc: Lukasz Majewski <l.majewski at samsung.com>
>>> Cc: Jaehoon Chung <jh80.chung at samsung.com>
>>> Cc: Stefan Roese <sr at denx.de>
>>> Cc: Simon Glass <sjg at chromium.org>
>>> Cc: Bin Meng <bmeng.cn at gmail.com>
>>> Cc: Marek Vasut <marex at denx.de>
>>> ---
>>>   common/fdt_support.c | 7 +++----
>>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>> index f86365e..5f808cc 100644
>>> --- a/common/fdt_support.c
>>> +++ b/common/fdt_support.c
>>> @@ -946,8 +946,7 @@ void fdt_del_node_and_alias(void *blob, const char
>>> *alias)
>>>   /* Max address size we deal with */
>>>   #define OF_MAX_ADDR_CELLS      4
>>>   #define OF_BAD_ADDR    ((u64)-1)
>>> -#define OF_CHECK_COUNTS(na, ns)        ((na) > 0 && (na) <=
>>> OF_MAX_ADDR_CELLS && \
>>> -                       (ns) > 0)
>>> +#define OF_CHECK_COUNTS(na)    ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
>>>
>>>   /* Debug utility */
>>>   #ifdef DEBUG
>>> @@ -1115,7 +1114,7 @@ static u64 __of_translate_address(void *blob, int
>>> node_offset, const fdt32_t *in
>>>
>>>          /* Cound address cells & copy address locally */
>>>          bus->count_cells(blob, parent, &na, &ns);
>>> -       if (!OF_CHECK_COUNTS(na, ns)) {
>>> +       if (!OF_CHECK_COUNTS(na)) {
>>
>>
>> This seems to conflict with the comment at the top of this function:
>>
>>   * Note: We consider that crossing any level with #size-cells == 0 to
>> mean
>>   * that translation is impossible (that is we are not dealing with a
>> value
>>   * that can be mapped to a cpu physical address). This is not really
>> specified
>>   * that way, but this is traditionally the way IBM at least do things
>>
>> What should we do here?
>>
>
> Is that commit acceptable? I would like send V2 with removing the above
> comment.

That's what I am worried about. Presumably the comment is accurate
today and this check has some value. I was hoping Stefan might know.

>
> Best regards,
> --
> Przemyslaw Marczak
> Samsung R&D Institute Poland
> Samsung Electronics
> p.marczak at samsung.com
>
>
>>>                  printf("%s: Bad cell count for %s\n", __FUNCTION__,
>>>                         fdt_get_name(blob, node_offset, NULL));
>>>                  goto bail;
>>> @@ -1142,7 +1141,7 @@ static u64 __of_translate_address(void *blob, int
>>> node_offset, const fdt32_t *in
>>>                  /* Get new parent bus and counts */
>>>                  pbus = &of_busses[0];
>>>                  pbus->count_cells(blob, parent, &pna, &pns);
>>> -               if (!OF_CHECK_COUNTS(pna, pns)) {
>>> +               if (!OF_CHECK_COUNTS(pna)) {
>>>                          printf("%s: Bad cell count for %s\n",
>>> __FUNCTION__,
>>>                                  fdt_get_name(blob, node_offset, NULL));
>>>                          break;
>>> --
>>> 1.9.1
>>>
>>
>> Regards,
>> Simon
>>
>

Regards,
Simon