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

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

Hello Simon,

On 01/12/2016 02:59 PM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 12 January 2016 at 03:49, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> 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@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@chromium.org> wrote:
>>>>>>
>>>>>>
>>>>>> Hi Przemyslaw,
>>>>>>
>>>>>> On 5 November 2015 at 23:47, Stefan Roese <sr@denx.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 06.11.2015 04:16, Simon Glass wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 3 November 2015 at 02:57, Przemyslaw Marczak
>>>>>>>> <p.marczak@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@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.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>
> Please hold off on that. I'll accept your other patch but let's see if
> Stephen wants to write something first. Using fdtdec_get_addr()
> doesn't make sense although I fully understand your frustration. Let's
> give it a week.
>
> Regards,
> Simon
>
>

Ah, too late:)

I have prepared patch, it's simple and doesn't touch the code - only 
device-tree. And this would be a proper approach, according to Stephen's 
comments.

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