On 25 September 2015 at 09:48, Stephen Warren <swar...@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 <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.
Tested on snow: Tested-by: Simon Glass <s...@chromium.org> Acked-by: Simon Glass <s...@chromium.org> _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot