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

[Simon Glass]

Hi Przemyslaw,

On 5 November 2015 at 23:47, Stefan Roese <sr at denx.de> wrote:
> On 06.11.2015 04:16, Simon Glass wrote:
>>
>> 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.
>
>
> Unfortunately no. I just stumbled over this problem with the
> translation of the "complex" ranges on the MVEBU platform. And
> noticed that we already have this functionality to translate
> the addresses the "right way".
>
> I'm wondering how this problem with those GPIOs is handled in
> the kernel? I assume that it is working correctly there, right?
> Przemyslaw, could you perhaps check this and see, why its
> working there? And change / fix it in U-Boot accordingly?

Let's pick up this patch for now as a bug-fix. We can deal with this
problem after the release.

Regards,
Simon