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()?
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