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 <swar...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
