Hi Stephen, On 5 January 2016 at 10:12, Stephen Warren <swar...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot