Hi Przemyslaw, Stephen, On 13 January 2016 at 04:10, Przemyslaw Marczak <p.marc...@samsung.com> 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot