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 <[email protected]>
... 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,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
[email protected]
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
