From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Tue, 05 Jan 2016 16:38:24 +0100 Subject: [U-Boot] [PATCH 0/3] dm: add dev_get_reg() for getting device node's reg In-Reply-To: <568AD0C6.8070605@wwwdotorg.org> 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> Message-ID: <568BE370.1050203@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/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?". What about adding a check that the parent defines "ranges" property and if not - then don't check the #size-cells? So with this simple check, we could fix all present issues, beside the later fdt/drivers fixing. Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com