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@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.
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.
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?
Best regards,