From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Wed, 13 Jan 2016 12:10:09 +0100 Subject: [U-Boot] [PATCH] fdt: __of_translate_address(): check parent's 'ranges' before translate In-Reply-To: <56952D23.7010109@wwwdotorg.org> 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> Message-ID: <56963091.7050508@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 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. Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com