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

[Stephen Warren]

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.