[U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg

[Przemyslaw Marczak]

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.

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.

>> For this purpose this patch set introduces new core function:
>>   fdt_addr_t dev_get_reg(struct udevice *dev)
>> which returns the 'reg' value in the same way as previously
>> dev_get_addr().
>>
>> This fixes s5p gpio driver and booting issue on few Exynos based boards:
>> - Trats2
>> - Odroid U3/X2
>
> 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:

---- case 1 -----
parent {
	#address-cells = <1>;
	#size-cells = <0>;
	child {
		reg = <0x9>;
	};
};
This is ok!

---- 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)

---- 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)



Now I don't have a time for checking it in dtc code, however we can 
check in U-Boot, that the dtb is not coherent?

So I can compile my default dts - no dtc warnings, and next in U-Boot I 
can see:

------------ check i2c node --------------------
Trats2 # fdt list /i2c at 138d0000
i2c at 138d0000 {
         #address-cells = <0x00000001>;
         #size-cells = <0x00000000>;
         compatible = "samsung,s3c2440-i2c";
         reg = <0x138d0000 0x00000100>;
         interrupts = <0x00000007 0x0000003f 0x00000000>;
         samsung,i2c-sda-delay = <0x00000064>;
         samsung,i2c-slave-addr = <0x00000010>;
         samsung,i2c-max-bus-freq = <0x000186a0>;
         status = "okay";
         max77686_pmic at 09 {
         };
};

------------ check pmic node --------------------
Trats2 # fdt list /i2c at 138d0000/max77686_pmic at 09
max77686_pmic at 09 {
         compatible = "maxim,max77686";
         interrupts = <0x00000007 0x00000000>;
         reg = <0x00000009 0x00000000 0x00000000>;
         #clock-cells = <0x00000001>;
         voltage-regulators {
         };
};

So, the parent defines the summarized cells count as 1, and the child 
node can exceed it, because it provides 3 cells.

However it looks that we are still safe, since the code doesn't try 
exceed the cells count, defined by the parent.

What do you think about this?

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com