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

[Simon Glass]

Hi Stephen,

On 5 January 2016 at 10:12, Stephen Warren <swarren at 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()?

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