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

Tue Jan 12 11:49:14 CET 2016

Hello Simon,

On 01/11/2016 05:59 PM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 8 January 2016 at 05:01, Przemyslaw Marczak <p.marczak at samsung.com> wrote:
>> Hello Simon,
>>
>>
>> On 01/07/2016 08:24 PM, Simon Glass wrote:
>>>
>>> +Stephen
>>>
>>> On 4 January 2016 at 17:59, Simon Glass <sjg at chromium.org> wrote:
>>>>
>>>> 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.
>>>
>>>
>>> Applied to u-boot-dm/master.
>>>
>>> I'll post a revert after the release. It seems like you and Stephen
>>> are making good progress.
>>>
>>> - Simon
>>>
>>>
>>
>> Why so fast with this one?
>>
>> I think, that more proper for a temporary fix is my latest patch with
>> #size-cells count checking only if ranges found in the parent node.
>>
>> I will continue the discussion with Stephen.
>
> The release is scheduled for today, so we had to do something to fix
> the breakage.
>
> Once you have a full solution figured out we can revert this patch and
> apply what you come up with.
>
> Regards,
> Simon
>
>

Ok. It's hard to convince Stephen to accept such change, so I will send 
a patch with another solution - just bring back fdtdec_get_addr() for 
Exynos GPIO driver. And will revert this one within the patchset.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com