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

[Przemyslaw Marczak <p.marczak@samsung.com>]

Hello,

On 01/04/2016 09:06 PM, Stephen Warren wrote:
> On 12/29/2015 01:47 AM, Przemyslaw Marczak wrote:
>> Hello Stephen,
>>
>> On 12/16/2015 08:07 PM, Stephen Warren wrote:
>>> On 12/16/2015 11:53 AM, 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.
>>>
>>> Ah, I guess the problem is caused by the following code in
>>> __of_translate_address():
>>>
>>>      bus->count_cells(blob, parent, &na, &ns);
>>>      if (!OF_CHECK_COUNTS(na, ns)) {
>>>          printf("%s: Bad cell count for %s\n", __FUNCTION__,
>>>
>>
>> Yes, and this is what my previous patch 'fixes'.
>>
>> [1] https://patchwork.ozlabs.org/patch/537372/
>>
>> However Linux makes the translate in the same way.
>>
>>> That's because the function assumes it's called for MMIO addresses.
>>> However, reg values for I2C devices aren't MMIO addresses, so those
>>> assumptions don't apply. So, there is an argument for introducing some
>>> new functionality. I'm not sure that a whole new function is the correct
>>> way to go though. Rather, the existing translation functions should be
>>> enhanced to know the location of root of the address space that contains
>>> the address that's being translated. Then, translation can stop there.
>>
>> This is okay but then, all device tree blobs should be defined in a
>> proper way.
>
> Well, why shouldn't that be true? There are rules for how DTs must be
> constructed. Nobody should expect DTs that violate those rules to work
> in any particular way.
>
>> The problem is, that there are some additions and various assumptions in
>> the drivers, e.g. the exynos gpio driver (s5p_gpio.c) is checking the
>> reg's property value for each bank. But the driver in Linux hardcodes
>> those values, however for both cases this is wrong, because the gpio
>> regs could be mapped with ranges.
>
> It sounds like there are many bugs to fix:-)
>

Unfortunately... :(

>> Even that issues above, I would prefer introduce a function or modify
>> the existing one to allow keeping this as it is.
>
> Adding an extra function sounds OK, although I stand by my comment that
> the caller should pass in a parameter indicating the root of the address
> space, so that both #address-cells and #size-cells can be checked all
> the way up the chain, and #size-cells should only be allowed to be 0 at
> the root of the translation, not at any intermediate point.
>
>>> Something like skipping the check on ns in the above code if parent ==
>>> addr_space_root_offset, and also terminating the for (;;) loop in that
>>> function under a similar condition.
>>>
>>> This would allow for translation to occur for buses other than the CPU's
>>> root MMIO space, yet not attempt to translate across known address space
>>> boundaries (i.e. where address translation is known to be impossible).
>>
>> To achieve this functionality, it should be enough to take my first
>> patch [1]. And then if no "ranges" is defined, then we have 1:1
>> translation.
>
> I don't think so; that patch removes all checks on #size-cells rather
> than only removing/ignoring the check at the root of the address space.
>
>> I think, that it is safe, but then we will have a different assumptions,
>> than in the Linux - is it acceptable?
>
> Both Linux and U-Boot should conform to the DT specification. So, if
> there's a difference between the two, there's likely a bug.
>
>

According to your comments with the new parameter, I think that we don't 
need this. As Simon wrote in one of his reply:

  "How would the caller know this root?".

What about adding a check that the parent defines "ranges" property and 
if not - then don't check the #size-cells?

So with this simple check, we could fix all present issues, beside the 
later fdt/drivers fixing.

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