Hi Przemyslaw, On 12 January 2016 at 07:22, Przemyslaw Marczak <[email protected]> wrote: > Hello Simon, > > > On 01/12/2016 02:57 PM, Simon Glass wrote: >> >> Hi Przemyslaw, >> >> On 12 January 2016 at 03:25, Przemyslaw Marczak <[email protected]> >> 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, so the above case is possible. The 'reg' for the parent >>> bus can represent MMIO (depends on what its parent defines) and the child is >>> non-MMIO. >>> >>> You won't allow to use dev_get_addr() for other than MMIO addresses. >>> Ok, I have no more arguments and no more time. >>> >>> 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()" >> >> >> I'm sorry this has been so difficult. Thank you for digging into it. >> >> I'm going to take this patch as is unless there is an alternative >> patch on the table. Stephen please let me know if you'd like to write >> something. One idea seems to be a new function (like >> dev_get_addr_local()) which avoids the address translation. But >> Przemyslaw has put enough energy into this I think. >> >> Regards, >> Simon >> >> > > I think, that we don't need such function. > > Stephen has right with the universal dev_get_addr() - it should be used only > for MMIO addresses. > > And also any universal function for getting the reg value is useless, for > some specific reasons, which Stephen mentioned. > > I'm going to send another patch soon, which I think (again) should close the > issue at all. Changing GPIO driver is not required, it will be enough when I > fix the device-tree files (SoCxxx-pinctrl-uboot.dts). > > We don't need to look at kernel, since we have two different drivers and > also the kernel doesn't use the GPIO's regs (addresses are hardcoded). > So fixing device-tree is a good choose. It's really only few lines per file. >
OK sounds good, thanks. Regards, Simon _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
