From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Fri, 25 Sep 2015 09:48:21 -0600 Subject: [U-Boot] [PATCH 2/3] fix: s5p_gpio: call: dev_get_addr() instead of fdtdec_get_addr() In-Reply-To: <56050777.4080901@samsung.com> References: <1443108590-16871-1-git-send-email-p.marczak@samsung.com> <1443108590-16871-3-git-send-email-p.marczak@samsung.com> <56043307.9070606@wwwdotorg.org> <56050777.4080901@samsung.com> Message-ID: <56056CC5.4010908@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> >> ... 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.