[U-Boot] [PATCH 2/3] fix: s5p_gpio: call: dev_get_addr() instead of fdtdec_get_addr()

[Simon Glass]

On 25 September 2015 at 09:48, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 09/25/2015 02:36 AM, Przemyslaw Marczak wrote:
>>
>> Hello Stephen,
>>
>> On 09/24/2015 07:29 PM, Stephen Warren wrote:
>>>
>>> On 09/24/2015 09:29 AM, Przemyslaw Marczak wrote:
>>>>
>>>> After rework in lib/fdtdec.c, the function fdtdec_get_addr()
>>>> doesn't work for nodes with #size-cells property, set to 0.
>>>>
>>>> To get GPIO's 'reg' property, the code should use one of:
>>>> fdtdec_get_addr_size_auto_no/parent() function.
>>>>
>>>> Fortunately dm core provides a function to get the property.
>>>>
>>>> This commit reworks function gpio_exynos_bind(), to properly
>>>> use dev_get_addr() for GPIO device.
>>>>
>>>> This prevents setting a wrong base register for Exynos GPIOs.
>>>
>>>
>>> Migrating everything to dev_get_addr() is the correct long-term fix, so
>>> this patch,
>>>
>>> Acked-by: Stephen Warren <swarren at nvidia.com>
>>>
>>> ... although I'd have liked to see a smaller diff that didn't both
>>> re-order all the code /and/ call a different function, but I suppose
>>> that's not possible given the need to pass the device object to
>>> dev_get_addr(). You could have used fdtdec_get_addr_size_auto_parent()
>>> directly.
>>
>>
>> Yes, it's not a single line diff, but the driver supports driver-model,
>> so it's natural that it should use driver model API if can, instead of
>> fdtdec API.
>>
>> This approach makes things easier to test and catch mistakes in the
>> future.
>>
>>>
>>>
>>> I think it'd be good to fix fdtdec_get_addr_size() to have the same
>>> semantics that it previously did. There might be other code in U-Boot
>>> that's affected by the same issue, and fixing fdtdec_get_addr_size()
>>> would make sure that all got fixed too. Are you willing to send that
>>> patch too?
>>>
>>> Essentially, fdtdec_get_addr_size() used to assume:
>>>
>>> #address-cells == sizeof(fdt_addr_t)
>>> if sizep == NULL:
>>>      #size-cells == 0
>>> else:
>>>      #size-cells == sizeof(fdt_addr_t)
>>>
>>> However, it now assumes:
>>>
>>> #address-cells == sizeof(fdt_addr_t)
>>> #size-cells == sizeof(fdt_addr_t)
>>>
>>> Let's just add that condition back by doing something like the following
>>> in fdtdec_get_addr_size():
>>>
>>> u32 ns;
>>>
>>> if (sizep)
>>>      ns = sizeof(fdt_size_t) / sizeof(fdt32_t);
>>> else
>>>      ns = 0;
>>>
>>> ... and replacing the ns parameter that's passed to
>>> fdtdec_get_addr_size_fixed() with that variable, rather than hard-coding
>>> it.
>>
>>
>> Sorry, currently I have some other things to do, and I wouldn't prefer
>> fixing this without proper testing. Such core things should be tested in
>> sandbox by couple of unit tests.
>
>
> OK, I'll take a stab at it.
>
>> This seem to be okay, but is still wrong.
>>
>> We should always call fdtdec_get_addr_size_fixed() with arguments, which
>> fits to the dtb, instead of hardcoded values.
>>
>> So, only the implementation of function
>>
>> fdtdec_get_addr_size_auto_parent()
>>
>> seem to be correct.
>>
>> It check the real #address-cells and #size-cells.
>
>
> Right. All "client" code should be migrated to call function which look at
> #address-cells and #size-cells. That's what
> fdtdec_get_addr_size_auto_parent(), fdtdec_get_addr_size_auto_noparent(),
> and dev_get_addr() do.
>
> However, there is code in U-Boot which (incorrectly) used fdtdec_get_addr()
> to parse properties other than reg. Those properties aren't affected by
> #address-cells and #size-cells. Hence, the hard-coding of na and ns inside
> fdtdec_get_addr_size() is required to support those use-case. Hopefully once
> everything that parses reg is migrated to the functions that look at
> #address-cells and #size-cells, fdtdec_get_addr_size() can be renamed to
> make it obvious it shouldn't be used for parsing reg.
>
>> If this is slow, then maybe we need some cache with nodes, its
>> parents/childs and its size/addr cells to be checked only once?
>
>
> Hopefully all (or almost all) use-cases can use dev_get_addr(). There's no
> slowness there, since there's no searching of the DT to find the parent;
> it's already known directly.

Tested on snow:
Tested-by: Simon Glass <sjg at chromium.org>

Acked-by: Simon Glass <sjg at chromium.org>