From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Tue, 05 Jan 2016 16:37:40 +0100 Subject: [U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg In-Reply-To: <568ACFDE.5080306@wwwdotorg.org> References: <1450197123-1822-1-git-send-email-p.marczak@samsung.com> <5671B33E.5070701@wwwdotorg.org> <5682489B.2050408@samsung.com> <568ACFDE.5080306@wwwdotorg.org> Message-ID: <568BE344.9010003@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello, On 01/04/2016 09:02 PM, Stephen Warren wrote: > On 12/29/2015 01:47 AM, Przemyslaw Marczak wrote: >> Hello Stephen, >> >> On 12/16/2015 07:53 PM, Stephen Warren wrote: >>> On 12/15/2015 09:32 AM, Przemyslaw Marczak wrote: >>>> commit: dm: core: Enable optional use of fdt_translate_address() >>>> >>>> enables device's bus/child address translation method, depending >>>> on bus 'ranges' property and including child 'reg' property. >>>> This change makes impossible to decode the 'reg' for node with >>>> '#size-cells' equal to 0. >>>> >>>> Such case is possible by the specification and is also used in U-Boot, >>>> e.g. by I2C uclass or S5P GPIO - the last one is broken at present. >>> >>> Can you please explain the problem you're seeing in more detail? Without >>> any context, my initial reaction is that this is simply a bug somewhere. >>> That bug should be fixed, rather than introducing new APIs to hide the >>> problem. >>> >> >> Some time ago I send a patch with such fix: >> >> [1] https://patchwork.ozlabs.org/patch/537372/ >> >> Sorry, I didn't add you to the 'CC' list. >> >> However. I checked this in linux, and the code is the same, the >> size-cells == 0 is not supported also in Linux. > > The discussion there does indicate that removing the check on > #size-cells would be incorrect. > Ok, this probably would be good if we assume that dts is always well written, so this is not acceptable. >> So to prevent breaking some consistency in parsing fdt between U-boot >> and Linux, I sent the patch which adds dev_get_reg(). And it seem to be >> useful at least for I2C and Exynos GPIO driver. > > OK; as I mentioned in my other reply, some form of new function or new > parameter does seem reasonable here. > > ... At this point I can say, that the device-tree files and some compatible drivers are using wrong assumptions. I think, that adding the new function is not needed, and also that we don't need any new parameter to the function dev_get_reg(), because the right way is to fix the fdt. For the Exynos GPIO issue, we can use two cases: - define proper ranges - move #size-cells=0 to #size-cells=1 and extend the reg property by it's size (actually not too much to do) This will fix the Exynos boot issue. >>> Looking at arch/arm/dts/exynos4412-trats2.dts, I see the following: >>> >>> >>> i2c at 138d0000 { >>> samsung,i2c-sda-delay = <100>; >>> samsung,i2c-slave-addr = <0x10>; >>> samsung,i2c-max-bus-freq = <100000>; >>> status = "okay"; >>> >>> max77686_pmic at 09 { >>> compatible = "maxim,max77686"; >>> interrupts = <7 0>; >>> reg = <0x09 0 0>; >>> >>> Is that the node you're having problems with? If so, I believe this may >>> simply be due to invalid DT content. In exynos4.dtsi, that i2c node is >>> defined as: >>> >>> i2c at 138d0000 { >>> #address-cells = <1>; >>> #size-cells = <0>; >>> >>> Thus, any reg property in a child of that node must only contain a >>> single cell (the sum of #address-cells and #size-cells in the parent). >>> Does fixing the DT so it's valid solve your issue at all? >> >> Nice hit above! However we don't use DM API yet for the above example, >> so probably this is why it is still working - currently, the driver uses >> fdtdec_get_int(), for getting this value. >> >> But for test, after switching it to use of sequence: fdt_getprop() -> >> fdt_translate_address(), then I can see the warning: >> >> ---- cut ---- >> _of_translate_address: Bad cell count for max77686_pmic at 09 >> ---- cut ---- >> >> And for the above issue - applying patch [1] - allows return the right >> device address: 0x9 - without FDT modifying. >> >> Now, I checked, why the above example compiles by dtc with no warning. >> It looks, that dtc ignores some child's reg cells-count combination: > > dtc doesn't check that the length of the reg property is *equal* to the > sum of #address-cells and #size-cells, but rather that the length is a > *multiple* of that value. This is because the reg property can contain > multiple addresses. > >> ---- case 1 ----- >> parent { >> #address-cells = <1>; >> #size-cells = <0>; >> child { >> reg = <0x9>; >> }; >> }; >> This is ok! > > This is "1 * (1 + 0)". > >> ---- case 2 ----- >> parent { >> #address-cells = <1>; >> #size-cells = <0>; >> child { >> reg = <0x9 0 0>; >> }; >> }; >> This is ok: (the 2nd and 3rd child's cells are ignored by dtc) > > The extra cells aren't ignored; the length is "3 * (1 + 0)". >> ---- case 3 ----- >> parent { >> #address-cells = <1>; >> #size-cells = <1>; >> child { >> reg = <0x9 0 0>; >> }; >> }; >> >> This is wrong! dtc warning: >> Warning (reg_format): "reg" property in /i2c at 138d0000/max77686_pmic has >> invalid length (12 bytes) (#address-cells == 1, #size-cells == 1) > > Yes, this is "1.5 * (1 + 1)", yet the "1.5" isn't an integer, hence the > warning is triggered. > > Also thank you for the explanation. Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com