From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Thu, 07 Jan 2016 12:57:16 +0100 Subject: [U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg In-Reply-To: References: <1450197123-1822-1-git-send-email-p.marczak@samsung.com> <5671B33E.5070701@wwwdotorg.org> <5671B68B.5080203@wwwdotorg.org> <568248A1.4080606@samsung.com> <568AD0C6.8070605@wwwdotorg.org> <568BE370.1050203@samsung.com> <568BF987.1090205@wwwdotorg.org> Message-ID: <568E529C.2050503@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello, On 01/06/2016 01:24 AM, Simon Glass wrote: > Hi Stephen, > > On 5 January 2016 at 10:12, Stephen Warren 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