| Message ID | 1452166849-24461-1-git-send-email-p.marczak@samsung.com |
|---|---|
| State | Rejected |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Przemyslaw, > 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' > > The fix: > The proper result by this commit, is achieved by skipping > the condition check: 'size-cells > 0', when the parent doesn't > provide the 'ranges' property and allows to return 1:1 address > translation, if caller want's just the reg property's value. > > No additional argument is needed, the 'ranges' property existence > decides, what type of translation is done when calling dev_get_addr(). > And this should be, what the compatible driver expects. > > Now, by this commit the function __of_translate_address() can be used > for the both reg property use-cases: > > Case 1: (ranges) > ---------------- > some-bus { > address-cells = <1>; > size-cells = <1>; > ranges = <0x0 0x10000000 0x1000>; > reg = <0x10000000 0x1000>; > > child1 { > reg = <0xa00 0x100>; > }; > > child2 { > reg = <0xb00 0x100>; > }; > }; > > Return values (CONFIG_OF_TRANSLATE=y): > - dev_get_addr(some-bus) - retrurns: 0x10000000 - correct > - dev_get_addr(child1) - retrurns: 0x10000a00 - correct > - dev_get_addr(child2) - retrurns: 0x10000b00 - correct > This works as previous - this commit have no impact on this case. > > Case 2: (no ranges - e.g. I2C bus childs) - fixed > ------------------------------------------------- > I2C-bus { > address-cells = <1>; > size-cells = <0>; > reg = <0x10000000>; > > chip1 { > reg = <0xa00>; > }; > > chip2 { > reg = <0xb00>; > }; > }; > > Return values (CONFIG_OF_TRANSLATE=y): > - dev_get_addr(I2C-bus) - retrurns: 0x10000000 - correct > - dev_get_addr(chip1) - retrurns: 0xa00 - correct > - dev_get_addr(chip2) - retrurns: 0xb00 - correct > > This is fixed, since the previously returned value for chip1 and chip2 > was: 0xffffffff, which means: 'FDT_ADDR_T_NONE'. > > Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com> > Cc: Stefan Roese <sr@denx.de> > Cc: Tom Rini <trini@konsulko.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Stephen Warren <swarren@nvidia.com> > Cc: Stephen Warren <swarren@wwwdotorg.org> > --- > common/fdt_support.c | 25 +++++++++++++++++-------- > 1 file changed, 17 insertions(+), 8 deletions(-) > > diff --git a/common/fdt_support.c b/common/fdt_support.c > index 66464db..1f472ca 100644 > --- a/common/fdt_support.c > +++ b/common/fdt_support.c > @@ -952,8 +952,9 @@ void fdt_del_node_and_alias(void *blob, const > char *alias) /* Max address size we deal with */ > #define OF_MAX_ADDR_CELLS 4 > #define OF_BAD_ADDR FDT_ADDR_T_NONE > -#define OF_CHECK_COUNTS(na, ns) ((na) > 0 && (na) <= > OF_MAX_ADDR_CELLS && \ > - (ns) > 0) > +#define OF_CHECK_COUNTS(na, ns, skip_ns) \ > + ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \ > + (skip_ns || (ns) > 0)) > > /* Debug utility */ > #ifdef DEBUG > @@ -1104,11 +1105,12 @@ static int of_translate_one(void * blob, int > parent, struct of_bus *bus, static u64 __of_translate_address(void > *blob, int node_offset, const fdt32_t *in_addr, const char *rprop) > { > - int parent; > - struct of_bus *bus, *pbus; > + const fdt32_t *pranges; > fdt32_t addr[OF_MAX_ADDR_CELLS]; > - int na, ns, pna, pns; > + int na, ns, pna, pns, parent; > + struct of_bus *bus, *pbus; > u64 result = OF_BAD_ADDR; > + bool skip_ns_check; > > debug("OF: ** translation for device %s **\n", > fdt_get_name(blob, node_offset, NULL)); > @@ -1119,9 +1121,16 @@ static u64 __of_translate_address(void *blob, > int node_offset, const fdt32_t *in goto bail; > bus = &of_busses[0]; > > - /* Cound address cells & copy address locally */ > + /* Chek 'ns' only if parent provides 'ranges' property */ > + pranges = fdt_getprop(blob, parent, rprop, NULL); > + if (pranges) > + skip_ns_check = false; > + else > + skip_ns_check = true; > + > + /* Count address cells & copy address locally */ > bus->count_cells(blob, parent, &na, &ns); > - if (!OF_CHECK_COUNTS(na, ns)) { > + if (!OF_CHECK_COUNTS(na, ns, skip_ns_check)) { > printf("%s: Bad cell count for %s\n", __FUNCTION__, > fdt_get_name(blob, node_offset, NULL)); > goto bail; > @@ -1148,7 +1157,7 @@ static u64 __of_translate_address(void *blob, > int node_offset, const fdt32_t *in /* Get new parent bus and counts */ > pbus = &of_busses[0]; > pbus->count_cells(blob, parent, &pna, &pns); > - if (!OF_CHECK_COUNTS(pna, pns)) { > + if (!OF_CHECK_COUNTS(pna, pns, skip_ns_check)) { > printf("%s: Bad cell count for %s\n", > __FUNCTION__, fdt_get_name(blob, node_offset, NULL)); > break; Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
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.
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>; }; }; code: dev_get_reg(child) - returns '0xa00' ---------------------- I would like to distinguish few things: 1. This patch just adds the support for the above second case, which is sensible and possible from specification point of view. 2. How it will be used by the code - is another thing. 3. If some driver's code can just use of dev_get_addr() for it's case, then why shouldn't it? Since it, knows how to interpret the returned value in its own specified way - and also it's described by the proper binding file, what the reg represents - then I don't threat this as issue. And here the example may be: - s5p_gpio.c - exynos GPIO driver 4. If I update the commit message with a general naming convention (parent/children) instead of using I2C suggestion - will be that patch acceptable for you? Best regards,
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.
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()" Best regards,
Hi Przemyslaw, On 12 January 2016 at 03:25, Przemyslaw Marczak <p.marczak@samsung.com> 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
Hello Simon, On 01/12/2016 02:57 PM, Simon Glass wrote: > Hi Przemyslaw, > > On 12 January 2016 at 03:25, Przemyslaw Marczak <p.marczak@samsung.com> 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. Best regards,
Hi Przemyslaw, On 12 January 2016 at 07:22, Przemyslaw Marczak <p.marczak@samsung.com> wrote: > Hello Simon, > > > On 01/12/2016 02:57 PM, Simon Glass wrote: >> >> Hi Przemyslaw, >> >> On 12 January 2016 at 03:25, Przemyslaw Marczak <p.marczak@samsung.com> >> 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
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. > 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. > 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.
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,
Hi Przemyslaw, Stephen, On 13 January 2016 at 04:10, Przemyslaw Marczak <p.marczak@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
Hello Simon, On 01/14/2016 06:17 PM, Simon Glass wrote: > Hi Przemyslaw, Stephen, > > On 13 January 2016 at 04:10, Przemyslaw Marczak <p.marczak@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 > > 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,
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 >> <p.marczak@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 >> >> > > 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.
Hi Przymyslaw, On 15 January 2016 at 09:35, Stephen Warren <swarren@wwwdotorg.org> 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 >>> <p.marczak@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 >>> >>> >> >> 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
Hello, On 01/29/2016 07:23 PM, Simon Glass wrote: > Hi Przymyslaw, > > On 15 January 2016 at 09:35, Stephen Warren <swarren@wwwdotorg.org> 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 >>>> <p.marczak@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 >>>> >>>> >>> >>> 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,
diff --git a/common/fdt_support.c b/common/fdt_support.c index 66464db..1f472ca 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -952,8 +952,9 @@ void fdt_del_node_and_alias(void *blob, const char *alias) /* Max address size we deal with */ #define OF_MAX_ADDR_CELLS 4 #define OF_BAD_ADDR FDT_ADDR_T_NONE -#define OF_CHECK_COUNTS(na, ns) ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \ - (ns) > 0) +#define OF_CHECK_COUNTS(na, ns, skip_ns) \ + ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \ + (skip_ns || (ns) > 0)) /* Debug utility */ #ifdef DEBUG @@ -1104,11 +1105,12 @@ static int of_translate_one(void * blob, int parent, struct of_bus *bus, static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in_addr, const char *rprop) { - int parent; - struct of_bus *bus, *pbus; + const fdt32_t *pranges; fdt32_t addr[OF_MAX_ADDR_CELLS]; - int na, ns, pna, pns; + int na, ns, pna, pns, parent; + struct of_bus *bus, *pbus; u64 result = OF_BAD_ADDR; + bool skip_ns_check; debug("OF: ** translation for device %s **\n", fdt_get_name(blob, node_offset, NULL)); @@ -1119,9 +1121,16 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in goto bail; bus = &of_busses[0]; - /* Cound address cells & copy address locally */ + /* Chek 'ns' only if parent provides 'ranges' property */ + pranges = fdt_getprop(blob, parent, rprop, NULL); + if (pranges) + skip_ns_check = false; + else + skip_ns_check = true; + + /* Count address cells & copy address locally */ bus->count_cells(blob, parent, &na, &ns); - if (!OF_CHECK_COUNTS(na, ns)) { + if (!OF_CHECK_COUNTS(na, ns, skip_ns_check)) { printf("%s: Bad cell count for %s\n", __FUNCTION__, fdt_get_name(blob, node_offset, NULL)); goto bail; @@ -1148,7 +1157,7 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in /* Get new parent bus and counts */ pbus = &of_busses[0]; pbus->count_cells(blob, parent, &pna, &pns); - if (!OF_CHECK_COUNTS(pna, pns)) { + if (!OF_CHECK_COUNTS(pna, pns, skip_ns_check)) { printf("%s: Bad cell count for %s\n", __FUNCTION__, fdt_get_name(blob, node_offset, NULL)); break;
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' The fix: The proper result by this commit, is achieved by skipping the condition check: 'size-cells > 0', when the parent doesn't provide the 'ranges' property and allows to return 1:1 address translation, if caller want's just the reg property's value. No additional argument is needed, the 'ranges' property existence decides, what type of translation is done when calling dev_get_addr(). And this should be, what the compatible driver expects. Now, by this commit the function __of_translate_address() can be used for the both reg property use-cases: Case 1: (ranges) ---------------- some-bus { address-cells = <1>; size-cells = <1>; ranges = <0x0 0x10000000 0x1000>; reg = <0x10000000 0x1000>; child1 { reg = <0xa00 0x100>; }; child2 { reg = <0xb00 0x100>; }; }; Return values (CONFIG_OF_TRANSLATE=y): - dev_get_addr(some-bus) - retrurns: 0x10000000 - correct - dev_get_addr(child1) - retrurns: 0x10000a00 - correct - dev_get_addr(child2) - retrurns: 0x10000b00 - correct This works as previous - this commit have no impact on this case. Case 2: (no ranges - e.g. I2C bus childs) - fixed ------------------------------------------------- I2C-bus { address-cells = <1>; size-cells = <0>; reg = <0x10000000>; chip1 { reg = <0xa00>; }; chip2 { reg = <0xb00>; }; }; Return values (CONFIG_OF_TRANSLATE=y): - dev_get_addr(I2C-bus) - retrurns: 0x10000000 - correct - dev_get_addr(chip1) - retrurns: 0xa00 - correct - dev_get_addr(chip2) - retrurns: 0xb00 - correct This is fixed, since the previously returned value for chip1 and chip2 was: 0xffffffff, which means: 'FDT_ADDR_T_NONE'. Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com> Cc: Stefan Roese <sr@denx.de> Cc: Tom Rini <trini@konsulko.com> Cc: Simon Glass <sjg@chromium.org> Cc: Stephen Warren <swarren@nvidia.com> Cc: Stephen Warren <swarren@wwwdotorg.org> --- common/fdt_support.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)