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

[Przemyslaw Marczak]

Hello,

On 01/05/2016 06:08 PM, Stephen Warren wrote:
> 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.
>
>

Agree, then I think my new patch should be suitable for the both cases:

https://patchwork.ozlabs.org/patch/564246/

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