From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Fri, 15 Jan 2016 11:41:56 +0100 Subject: [U-Boot] [PATCH] fdt: __of_translate_address(): check parent's 'ranges' before translate In-Reply-To: References: <1452166849-24461-1-git-send-email-p.marczak@samsung.com> <568EAD8B.6090909@wwwdotorg.org> <56939045.5070806@samsung.com> <5693DC9F.2090100@wwwdotorg.org> <5694D4B4.8040709@samsung.com> <56952D23.7010109@wwwdotorg.org> <56963091.7050508@samsung.com> Message-ID: <5698CCF4.2090805@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 Simon, On 01/14/2016 06:17 PM, Simon Glass wrote: > Hi Przemyslaw, Stephen, > > On 13 January 2016 at 04:10, Przemyslaw Marczak wrote: >> Hello Stephen, >> >> >> On 01/12/2016 05:43 PM, Stephen Warren wrote: >>> >>> On 01/12/2016 03:25 AM, Przemyslaw Marczak wrote: >>>> >>>> Hello Stephen, >>>> >>>> On 01/11/2016 05:47 PM, Stephen Warren wrote: >>>>> >>>>> On 01/11/2016 04:21 AM, Przemyslaw Marczak wrote: >>>>>> >>>>>> Hello Stephen, >>>>>> >>>>>> On 01/07/2016 07:25 PM, Stephen Warren wrote: >>>>>>> >>>>>>> On 01/07/2016 04:40 AM, Przemyslaw Marczak wrote: >>>>>>>> >>>>>>>> The present implementation of __of_translate_address() taken >>>>>>>> from the Linux, is designed for translate bus/child address >>>>>>>> mappings by using 'ranges' property - and it doesn't allow >>>>>>>> for checking an address for a device's node with zero size-cells. >>>>>>>> >>>>>>>> The 'size-cells > 0' is required for bus/child address mapping, >>>>>>>> but is not required for non-memory mapped address, e.g.: I2C chip. >>>>>>>> Then when we need only raw 'reg' property's value. >>>>>>>> >>>>>>>> Since the I2C device address goes to a single-cell reg property, >>>>>>>> support for that case is welcome, but currently calling >>>>>>>> dev_get_addr() >>>>>>>> for I2C device will return 'FDT_ADDR_T_NONE', and print the warning: >>>>>>>> >>>>>>>> warning: >>>>>>>> __of_translate_address: Bad cell count for 'some-dev' >>>>>>> >>>>>>> >>>>>>> This patch takes the wrong approach. >>>>>>> >>>>>>> It simply doesn't make sense to /attempt/ to translate an I2C address >>>>>>> into an MMIO address space. It's a nonsensical operation; no such >>>>>>> translation is possible under any circumstances because I2C and MMIO >>>>>>> addresses mean completely different things and simply can't be >>>>>>> translated to each-other. >>>>>>> >>>>>>> Rather than making this nonsensical operation succeed in a way that >>>>>>> gives the desired no-op result, the nonsensical operation simply >>>>>>> shouldn't be performed in the first place. >>>>>>> >>>>>>> >>>>>> >>>>>> Okay, the example with I2C may be little confusing - I could use some >>>>>> general naming convention. However, this patch updates FDT-related code >>>>>> only. >>>>>> >>>>>> In one of your previous e-mails, you well argued that we shouldn't use >>>>>> dev_get_reg() for some buses, since they have a different 'reg' >>>>>> meaning. >>>>>> >>>>>> You are right, using dev_get_addr() as universal function may be >>>>>> nonsensical. >>>>>> >>>>>> Please note, that the present implementation of function: >>>>>> '__of_translate_address()' - allows for 1:1 translation, but only if >>>>>> '#size-cells' exists. So the below case is possible: >>>>>> >>>>>> ---------------------- >>>>>> parent { >>>>>> address-cells = <1>; >>>>>> size-cells = <1>; >>>>>> reg = <0x10000000 0x1000>; >>>>>> >>>>>> child { >>>>>> reg = <0xa00 0x100>; >>>>>> }; >>>>>> }; >>>>>> >>>>>> dev_get_reg(child) - will return '0xa00' >>>>>> ---------------------- >>>>>> >>>>>> If we don't need the address length, we can define: >>>>>> ---------------------- >>>>>> parent { >>>>>> address-cells = <1>; >>>>>> size-cells = <0>; >>>>>> reg = <0x10000000 0x1000>; >>>>>> >>>>>> child { >>>>>> reg = <0xa00>; >>>>>> }; >>>>>> }; >>>>> >>>>> >>>>> This case won't ever appear in a correctly written DT where reg >>>>> represents an MMIO address; MMIO addresses always have sizes, and hence >>>>> can't have size-cells=0. Hence, translating through a DT structures like >>>>> that is an error case, and shouldn't work. >>>> >>>> >>>> As we found out, the 'reg' property can represent not only MMIO, but may >>>> have other meaning, >>> >>> >>> Of course. >>> >>>> so the above case is possible. >>> >>> >>> Yes and no. >>> >>> That DT snippet is certainly possible. >>> >>> However, that's irrelevant to whether address translation should be >>> attempted across that boundary. *That* is not legal and should not be >>> attempted. >>> >> >> Going through your suggestions I took your side. >> You are on Cc in the new patchset. >> >>> > The 'reg' for the >>>> >>>> parent bus can represent MMIO (depends on what its parent defines) and >>>> the child is non-MMIO. >>> >>> >>> Correct. >>> >>>> You won't allow to use dev_get_addr() for other than MMIO addresses. >>>> Ok, I have no more arguments and no more time. >>> >>> >>> "You" is incorrect. This has absolutely nothing to do with me, but >>> rather the rule is imposed by the semantics of device tree. >>> >>> Also, I never said that dev_get_addr() must not be used for non-MMIO >>> addresses. In fact, I offered a suggestion to make it work correctly. >>> What I actually stated is that address translation must not be attempted >>> across boundaries between address spaces, since it is semantically >>> non-sensical. >>> >> >> Ok, please don't take it personally:), it was just how I understood your >> opinion. >> >> As you know the specification is not so clean, I thought, that checking the >> existence of "ranges" in parent node - is enough to provide proper >> "translation" (or rather choosing the root address space), when size-cells >> == 0. However, checking this condition is probably not enough, but you >> didn't provide a device-tree example to give it some light. >> >> Also maybe the translation is a bad word here, since we know that it's not >> MMIO translatable address. >> >> For me, this patch is okay. >> If I call it for I2C chip and it returns the chip address in I2C address >> space - then I can assume, that this is correct. >> >> Since, at present I2C subsystem takes the 'reg' as property's value, it >> looks that there should be no difference when using modified dev_get_reg(). >> >> However the main reason for this change was not I2C code update, but fixing >> Exynos GPIO driver which uses DTB in a quite different way than the others. >> >> So, I don't need to put the pressure for applying an improvement like this >> one - because it can be fixed in a more proper way. >> >>>> My issue can be also fixed by removing dev_get_addr() call from Exynos >>>> GPIO driver - so I will do this and within this change, will also revert >>>> the commit: >>>> "fdt: fix address cell count checking in fdt_translate_address()" >>> >>> >>> That sounds fine. It'd be better to introduce some code into the I2C >>> subsystem to handle this, but the approach you mention should work in >>> practice. >>> >>> >> >> So finally, as you can see at the new patches: >> >> http://patchwork.ozlabs.org/patch/566584/ >> http://patchwork.ozlabs.org/patch/566587/ >> >> I made other quick fix. This should be extended by ranges to be proper in >> 100%, but Linux don't use it for this platform and I don't see the reason >> for adding it to U-Boot. > > You could presumably add it to Linux also. > > Thank you both for figuring this out. > > Regards, > Simon > > The commit updates files, which exists in U-Boot only. Moreover, the problematic reg properties are not used by Linux's Exynos GPIO driver - because all required addresses are hardcoded in the driver. So I don't see the reason for doing it there. Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com