From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Tue, 02 Feb 2016 09:55:50 +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> <5698CCF4.2090805@samsung.com> <56991FC9.6030005@wwwdotorg.org> Message-ID: <56B06F16.4080106@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/29/2016 07:23 PM, Simon Glass wrote: > Hi Przymyslaw, > > On 15 January 2016 at 09:35, Stephen Warren wrote: >> On 01/15/2016 03:41 AM, Przemyslaw Marczak wrote: >>> >>> 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. >> >> >> There should only be one definition of DT bindings. That is, both U-Boot and >> Linux must use the same bindings and hence interpret the DT in the same way. >> That's the entire point of DT. >> >> Preferably both Linux and U-Boot will use the exact same DT content. There >> may be some differences, e.g. if U-Boot doesn't support a particular >> driver/feature, then the nodes/properties that enable that feature can be >> omitted from the U-Boot DT since they won't be used. However, where the same >> node/property exists in both places, it should be identical between both. >> >> Prior to proposing any DT changes for U-Boot, the best approach is to get >> them into the Linux kernel DTs so that they get widespread review against >> the binding definitions and so that everyone using DT approves the changes. > > What would you like to do here? > > Regards, > Simon > > I will send a proper patch for the Kernel and probably U-Boot, before the end of this week. Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com