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

[Stephen Warren]

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.