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

[Przemyslaw Marczak]

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