From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Mon, 4 Jan 2016 13:02:38 -0700 Subject: [U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg In-Reply-To: <5682489B.2050408@samsung.com> References: <1450197123-1822-1-git-send-email-p.marczak@samsung.com> <5671B33E.5070701@wwwdotorg.org> <5682489B.2050408@samsung.com> Message-ID: <568ACFDE.5080306@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 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. > 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. ... >> 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.