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

[Stephen Warren]

On 01/05/2016 08:37 AM, Przemyslaw Marczak wrote:
> 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.

Why not?

Certainly it's reasonable to expect code not to blow up in crashy ways 
in the face of bad DTs. However, I don't think it's reasonable to 
require that code perform exactly the desired function in the face of 
arbitrary bad input data.

Besides, the problem here isn't a bad DT. It's perfectly legal for a DT 
to contain #size-cells=<0>. The problem is bad code, which attempts to 
translate an address beyond the root of the address space that the 
address is within. This is a bug in the code, pure and simple.

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

That may fix/hide the issue, but semantically seems entirely incorrect. 
I2C addresses cannot logically be mapped into the CPU's MMIO address 
space, so adding a ranges property that attempts to do that doesn't make 
sense. I2C addresses don't have a size, so adding one to the DT doesn't 
make sense.