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

[Przemyslaw Marczak <p.marczak@samsung.com>]

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@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@samsung.com>
>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>> Cc: Jaehoon Chung <jh80.chung@samsung.com>
>> Cc: Stefan Roese <sr@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>> Cc: Marek Vasut <marex@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.

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
>