[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/06/2016 01:24 AM, Simon Glass wrote:
> Hi Stephen,
>
> On 5 January 2016 at 10:12, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 01/05/2016 08:38 AM, Przemyslaw Marczak wrote:
>>>
>>> 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?".
>>
>>
>> This is a facet of the hardware.
>>
>> The root of the MMIO address space is the root of the DT.
>>
>> The root of any other kind of address space is the IO controller that
>> "hosts" the address space, i.e. the I2C or SPI controller.
>>
>> Every device knows semantically what its address represents, and hence can
>> trivially determine the root node of the address space.
>>
>> Device driver writers shouldn't have to care about this, so likely some form
>> of helper function should be provided by I2C/SPI/... subsystems to hide
>> these details. IIRC (although I haven't looked in a while) this is exactly
>> how/why the Linux kernel avoids this kind of issue; the I2C/SPI/...
>> subsystem, handles parsing of reg properties before any device-specific
>> driver is invoked.
>
> OK, then I suppose we could do this in a uclass pre_probe() method.
> But this would be different from how every current driver works (it is
> the driver's responsibility to call dev_get_addr()).
>
> I'm not super-keen on dev_get_reg() as it adds confusion - why is one
> reg dealth with differently from another. Perhaps just a different
> name, like dev_get_bus_addr()?
>

But there is only one "reg" property per device and I think, that its 
use-case is actually clearly defined in device-tree specification:
- if parent provides 'ranges' - try to map the address
- if no ranges - then return the reg value only

> Re Linux, can someone trace through the of_address_to_resource() call
> and see what it actually does? It seems to call of_translate_address()
> but presumably does not suffer from this problem. So maybe I am
> missing something. The S3C I2C driver calls platform_get_resource()
> which is presumably set up by a call to of_address_to_resource()?
>
> There is way too much code there to bring into U-Boot, but we can try
> to do similar things.
>
> Let's solve this in a general way for the next release.
>
> Regards,
> Simon
>
>

Please review my new patch:

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

If the dev_get_reg() will return what we want with the above patch,
then it could be used in each uclass pre_probe() method.

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