| Message ID | 1451862280-15245-1-git-send-email-sjg@chromium.org |
|---|---|
| State | Rejected |
| Delegated to: | Simon Glass |
| Headers | show |
Hello Simon, On 01/04/2016 12:04 AM, Simon Glass wrote: > It is common for I2C and SPI buses to have a single-cell address and a size > of 0. These produce a warning at present. For example on snow: > > __of_translate_address: Bad cell count for gpc4 > __of_translate_address: Bad cell count for gpx0 > __of_translate_address: Bad cell count for gpv2 > __of_translate_address: Bad cell count for gpv4 > > One of the nodes in question looks like this in part: > > pinctrl_2: pinctrl@10d10000 { > #address-cells = <1>; > #size-cells = <0>; > gpv2: gpv2 { > reg = <0x060>; > }; > gpv4: gpv4 { > reg = <0xc0>; > }; > }; > > This is clearly valid so it looks like the conversion to use > fdt_translate_address() in dev_get_addr() is not currently a good move. > > Przemyslaw Marczak sent three patches to resolve this for exynos boards: > > https://patchwork.ozlabs.org/patch/557008/ > https://patchwork.ozlabs.org/patch/557010/ > https://patchwork.ozlabs.org/patch/557009/ > > But this involves creating a new function, and everyone will need to know > when to use which one. Also the problem may affect other boards. > > Instead we can relax the contraint that there must be at least one size > cell. This fixes the problem on snow and should not affect anything else, > since the error check should not fire on normal device trees anyway. > > After the release we will have more time to come up with a better solution, > if one exists. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > common/fdt_support.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/common/fdt_support.c b/common/fdt_support.c > index 66464db..43c5fa8 100644 > --- a/common/fdt_support.c > +++ b/common/fdt_support.c > @@ -952,8 +952,7 @@ 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) ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS) > > /* Debug utility */ > #ifdef DEBUG > The patch with this fix was send by me some time ago: https://patchwork.ozlabs.org/patch/537372/ It actually do the same, what your patch does. And my next three patches (adding dev_get_reg()), which you mentioned, are another way to fix the issue. So I think, we should choose between those two ways. However the first one breaks the consistency with the kernel: drivers/of/address.c: __of_translate_address() Best regards,
On 01/03/2016 04:04 PM, Simon Glass wrote: > It is common for I2C and SPI buses to have a single-cell address and a size > of 0. These produce a warning at present. For example on snow: > > __of_translate_address: Bad cell count for gpc4 > __of_translate_address: Bad cell count for gpx0 > __of_translate_address: Bad cell count for gpv2 > __of_translate_address: Bad cell count for gpv4 > > One of the nodes in question looks like this in part: > > pinctrl_2: pinctrl@10d10000 { > #address-cells = <1>; > #size-cells = <0>; > gpv2: gpv2 { > reg = <0x060>; > }; > gpv4: gpv4 { > reg = <0xc0>; > }; > }; > > This is clearly valid so it looks like the conversion to use > fdt_translate_address() in dev_get_addr() is not currently a good move. To disable that, why not simply turn off CONFIG_OF_TRANSLATE on the affected platforms? That's precisely why that config option was introduced when the call to fdt_translate_address() was added to dev_get_addr()? That would prevent this patch from affecting platforms that don't trigger this issue, this leaving the valid check in place. > Przemyslaw Marczak sent three patches to resolve this for exynos boards: > > https://patchwork.ozlabs.org/patch/557008/ > https://patchwork.ozlabs.org/patch/557010/ > https://patchwork.ozlabs.org/patch/557009/ > > But this involves creating a new function, and everyone will need to know > when to use which one. Also the problem may affect other boards. I suggest adding an extra parameter to dev_get_addr() (or whatever calls it) that indicates the root of the address space. The check on #size-cells should be skipped for that one node (or level of translation) but enabled for all other levels. This way, there would be no need for anyone to choose between functions; there'd only be one. Most cases (i.e. translation of MMIO addresses) would simply pass 0 as the extra parameter (for the root node), but in special cases where it's known translation is not expected to reach the root MMIO space (e.g. I2C, SPI controllers), the controller node would be passed in.
Hi Stephen, On 4 January 2016 at 13:15, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 01/03/2016 04:04 PM, Simon Glass wrote: >> >> It is common for I2C and SPI buses to have a single-cell address and a >> size >> of 0. These produce a warning at present. For example on snow: >> >> __of_translate_address: Bad cell count for gpc4 >> __of_translate_address: Bad cell count for gpx0 >> __of_translate_address: Bad cell count for gpv2 >> __of_translate_address: Bad cell count for gpv4 >> >> One of the nodes in question looks like this in part: >> >> pinctrl_2: pinctrl@10d10000 { >> #address-cells = <1>; >> #size-cells = <0>; >> gpv2: gpv2 { >> reg = <0x060>; >> }; >> gpv4: gpv4 { >> reg = <0xc0>; >> }; >> }; >> >> This is clearly valid so it looks like the conversion to use >> fdt_translate_address() in dev_get_addr() is not currently a good move. > > > To disable that, why not simply turn off CONFIG_OF_TRANSLATE on the affected > platforms? That's precisely why that config option was introduced when the > call to fdt_translate_address() was added to dev_get_addr()? > > That would prevent this patch from affecting platforms that don't trigger > this issue, this leaving the valid check in place. But since this breaks normal behaviour we don't know what platforms are affected. We have made CONFIG_OF_TRANSLATE the default. So this approach doesn't seem (in effect) any better than Przemyslaw's newer series, below. > >> Przemyslaw Marczak sent three patches to resolve this for exynos boards: >> >> https://patchwork.ozlabs.org/patch/557008/ >> https://patchwork.ozlabs.org/patch/557010/ >> https://patchwork.ozlabs.org/patch/557009/ >> >> But this involves creating a new function, and everyone will need to know >> when to use which one. Also the problem may affect other boards. > > > I suggest adding an extra parameter to dev_get_addr() (or whatever calls it) > that indicates the root of the address space. The check on #size-cells > should be skipped for that one node (or level of translation) but enabled > for all other levels. This way, there would be no need for anyone to choose > between functions; there'd only be one. Most cases (i.e. translation of MMIO > addresses) would simply pass 0 as the extra parameter (for the root node), > but in special cases where it's known translation is not expected to reach > the root MMIO space (e.g. I2C, SPI controllers), the controller node would > be passed in. How would the caller know this root? It sounds plausible, but I do want to avoid complex rules. I think you are saying that buses that use their own address mechanism (i.e. not MMIO) must do something special. The current dev_get_addr() is really simple. Regards, Simon
Hello, On 01/05/2016 02:00 AM, Simon Glass wrote: > Hi Stephen, > > On 4 January 2016 at 13:15, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 01/03/2016 04:04 PM, Simon Glass wrote: >>> >>> It is common for I2C and SPI buses to have a single-cell address and a >>> size >>> of 0. These produce a warning at present. For example on snow: >>> >>> __of_translate_address: Bad cell count for gpc4 >>> __of_translate_address: Bad cell count for gpx0 >>> __of_translate_address: Bad cell count for gpv2 >>> __of_translate_address: Bad cell count for gpv4 >>> >>> One of the nodes in question looks like this in part: >>> >>> pinctrl_2: pinctrl@10d10000 { >>> #address-cells = <1>; >>> #size-cells = <0>; >>> gpv2: gpv2 { >>> reg = <0x060>; >>> }; >>> gpv4: gpv4 { >>> reg = <0xc0>; >>> }; >>> }; >>> >>> This is clearly valid so it looks like the conversion to use >>> fdt_translate_address() in dev_get_addr() is not currently a good move. >> >> >> To disable that, why not simply turn off CONFIG_OF_TRANSLATE on the affected >> platforms? That's precisely why that config option was introduced when the >> call to fdt_translate_address() was added to dev_get_addr()? >> >> That would prevent this patch from affecting platforms that don't trigger >> this issue, this leaving the valid check in place. > > But since this breaks normal behaviour we don't know what platforms > are affected. We have made CONFIG_OF_TRANSLATE the default. So this > approach doesn't seem (in effect) any better than Przemyslaw's newer > series, below. > >> >>> Przemyslaw Marczak sent three patches to resolve this for exynos boards: >>> >>> https://patchwork.ozlabs.org/patch/557008/ >>> https://patchwork.ozlabs.org/patch/557010/ >>> https://patchwork.ozlabs.org/patch/557009/ >>> >>> But this involves creating a new function, and everyone will need to know >>> when to use which one. Also the problem may affect other boards. >> >> >> I suggest adding an extra parameter to dev_get_addr() (or whatever calls it) >> that indicates the root of the address space. The check on #size-cells >> should be skipped for that one node (or level of translation) but enabled >> for all other levels. This way, there would be no need for anyone to choose >> between functions; there'd only be one. Most cases (i.e. translation of MMIO >> addresses) would simply pass 0 as the extra parameter (for the root node), >> but in special cases where it's known translation is not expected to reach >> the root MMIO space (e.g. I2C, SPI controllers), the controller node would >> be passed in. > > How would the caller know this root? It sounds plausible, but I do > want to avoid complex rules. I think you are saying that buses that > use their own address mechanism (i.e. not MMIO) must do something > special. The current dev_get_addr() is really simple. > > Regards, > Simon > > Simon, Stephen As a summary of the issue, please tell me your opinion about this: Since the __of_translate_address() is called always with the "ranges" as an argument, it looks reasonable to check it at the function beginning, that the "ranges" property exists in the parent node. If not exists - then don't check the size-cells count and this should fix the problem with additional argument. This is simple and correct from specification point of view - which says ranges and #size-cells property's - are not required [ePAPR v1.1]. Best regards,
On 01/04/2016 06:00 PM, Simon Glass wrote: > Hi Stephen, > > On 4 January 2016 at 13:15, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 01/03/2016 04:04 PM, Simon Glass wrote: >>> >>> It is common for I2C and SPI buses to have a single-cell address and a >>> size >>> of 0. These produce a warning at present. For example on snow: >>> >>> __of_translate_address: Bad cell count for gpc4 >>> __of_translate_address: Bad cell count for gpx0 >>> __of_translate_address: Bad cell count for gpv2 >>> __of_translate_address: Bad cell count for gpv4 >>> >>> One of the nodes in question looks like this in part: >>> >>> pinctrl_2: pinctrl@10d10000 { >>> #address-cells = <1>; >>> #size-cells = <0>; >>> gpv2: gpv2 { >>> reg = <0x060>; >>> }; >>> gpv4: gpv4 { >>> reg = <0xc0>; >>> }; >>> }; >>> >>> This is clearly valid so it looks like the conversion to use >>> fdt_translate_address() in dev_get_addr() is not currently a good move. >> >> >> To disable that, why not simply turn off CONFIG_OF_TRANSLATE on the affected >> platforms? That's precisely why that config option was introduced when the >> call to fdt_translate_address() was added to dev_get_addr()? >> >> That would prevent this patch from affecting platforms that don't trigger >> this issue, this leaving the valid check in place. > > But since this breaks normal behaviour we don't know what platforms > are affected. We have made CONFIG_OF_TRANSLATE the default. So this > approach doesn't seem (in effect) any better than Przemyslaw's newer > series, below. It'd be better since it'd isolate the disabling of the feature only to those platforms that need it to work around bugs. Still, if you're absolutely certain that this change will be reverted as soon as the release is made, that is probably OK. >>> Przemyslaw Marczak sent three patches to resolve this for exynos boards: >>> >>> https://patchwork.ozlabs.org/patch/557008/ >>> https://patchwork.ozlabs.org/patch/557010/ >>> https://patchwork.ozlabs.org/patch/557009/ >>> >>> But this involves creating a new function, and everyone will need to know >>> when to use which one. Also the problem may affect other boards. >> >> >> I suggest adding an extra parameter to dev_get_addr() (or whatever calls it) >> that indicates the root of the address space. The check on #size-cells >> should be skipped for that one node (or level of translation) but enabled >> for all other levels. This way, there would be no need for anyone to choose >> between functions; there'd only be one. Most cases (i.e. translation of MMIO >> addresses) would simply pass 0 as the extra parameter (for the root node), >> but in special cases where it's known translation is not expected to reach >> the root MMIO space (e.g. I2C, SPI controllers), the controller node would >> be passed in. > > How would the caller know this root? I appear to have answered this already in response to one of Przemyslaw's emails. > It sounds plausible, but I do > want to avoid complex rules. I think you are saying that buses that > use their own address mechanism (i.e. not MMIO) must do something > special. The current dev_get_addr() is really simple. dev_get_addr() is simple yes, but also incorrect (to use in certain contexts, as currently implemented). DT has special cases. Code that parses DT has to deal with them. To paraphrase some famous quote: Things should be as simple as possible, but no simpler.
On 01/05/2016 08:47 AM, Przemyslaw Marczak wrote: > Hello, > > On 01/05/2016 02:00 AM, Simon Glass wrote: >> Hi Stephen, >> >> On 4 January 2016 at 13:15, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> On 01/03/2016 04:04 PM, Simon Glass wrote: >>>> >>>> It is common for I2C and SPI buses to have a single-cell address and a >>>> size >>>> of 0. These produce a warning at present. For example on snow: >>>> >>>> __of_translate_address: Bad cell count for gpc4 >>>> __of_translate_address: Bad cell count for gpx0 >>>> __of_translate_address: Bad cell count for gpv2 >>>> __of_translate_address: Bad cell count for gpv4 >>>> >>>> One of the nodes in question looks like this in part: >>>> >>>> pinctrl_2: pinctrl@10d10000 { >>>> #address-cells = <1>; >>>> #size-cells = <0>; >>>> gpv2: gpv2 { >>>> reg = <0x060>; >>>> }; >>>> gpv4: gpv4 { >>>> reg = <0xc0>; >>>> }; >>>> }; >>>> >>>> This is clearly valid so it looks like the conversion to use >>>> fdt_translate_address() in dev_get_addr() is not currently a good move. >>> >>> >>> To disable that, why not simply turn off CONFIG_OF_TRANSLATE on the >>> affected >>> platforms? That's precisely why that config option was introduced >>> when the >>> call to fdt_translate_address() was added to dev_get_addr()? >>> >>> That would prevent this patch from affecting platforms that don't >>> trigger >>> this issue, this leaving the valid check in place. >> >> But since this breaks normal behaviour we don't know what platforms >> are affected. We have made CONFIG_OF_TRANSLATE the default. So this >> approach doesn't seem (in effect) any better than Przemyslaw's newer >> series, below. >> >>> >>>> Przemyslaw Marczak sent three patches to resolve this for exynos >>>> boards: >>>> >>>> https://patchwork.ozlabs.org/patch/557008/ >>>> https://patchwork.ozlabs.org/patch/557010/ >>>> https://patchwork.ozlabs.org/patch/557009/ >>>> >>>> But this involves creating a new function, and everyone will need to >>>> know >>>> when to use which one. Also the problem may affect other boards. >>> >>> >>> I suggest adding an extra parameter to dev_get_addr() (or whatever >>> calls it) >>> that indicates the root of the address space. The check on #size-cells >>> should be skipped for that one node (or level of translation) but >>> enabled >>> for all other levels. This way, there would be no need for anyone to >>> choose >>> between functions; there'd only be one. Most cases (i.e. translation >>> of MMIO >>> addresses) would simply pass 0 as the extra parameter (for the root >>> node), >>> but in special cases where it's known translation is not expected to >>> reach >>> the root MMIO space (e.g. I2C, SPI controllers), the controller node >>> would >>> be passed in. >> >> How would the caller know this root? It sounds plausible, but I do >> want to avoid complex rules. I think you are saying that buses that >> use their own address mechanism (i.e. not MMIO) must do something >> special. The current dev_get_addr() is really simple. > > Simon, Stephen > > As a summary of the issue, please tell me your opinion about this: > > Since the __of_translate_address() is called always with the "ranges" as > an argument, it looks reasonable to check it at the function beginning, > that the "ranges" property exists in the parent node. > > If not exists - then don't check the size-cells count and this should > fix the problem with additional argument. > This is simple and correct from specification point of view - which says > ranges and #size-cells property's - are not required [ePAPR v1.1]. It /might/ indeed be reasonable to skip the check on #size-cells if there is no ranges property; a missing ranges property is already treated as a 1:1 translation as described by the comment at the top of of_translate_one(). Still, there are likely some edge-cases w.r.t. the "length" fields in the ranges property that would need to be thought through in more detail before I'd say for certain that this change does absolutely make sense. Either way though, making that change wouldn't solve the problem at all. It's simply not possible to translate an I2C device address beyond the root of the I2C address space; there is no equivalent of an I2C device's address in MMIO address space. The root of the problem is that the code is attempting to perform an I2C->MMIO address translation. If we prevent that (by capping any such translation at the root of the I2C address space for example), the problem is solved at that point. Once that's solved, the issue of translating across a boundary with #size-cells=0 will never appear, and hence does not need to be solved.
Hello, On 01/05/2016 06:26 PM, Stephen Warren wrote: > On 01/05/2016 08:47 AM, Przemyslaw Marczak wrote: >> Hello, >> >> On 01/05/2016 02:00 AM, Simon Glass wrote: >>> Hi Stephen, >>> >>> On 4 January 2016 at 13:15, Stephen Warren <swarren@wwwdotorg.org> >>> wrote: >>>> On 01/03/2016 04:04 PM, Simon Glass wrote: >>>>> >>>>> It is common for I2C and SPI buses to have a single-cell address and a >>>>> size >>>>> of 0. These produce a warning at present. For example on snow: >>>>> >>>>> __of_translate_address: Bad cell count for gpc4 >>>>> __of_translate_address: Bad cell count for gpx0 >>>>> __of_translate_address: Bad cell count for gpv2 >>>>> __of_translate_address: Bad cell count for gpv4 >>>>> >>>>> One of the nodes in question looks like this in part: >>>>> >>>>> pinctrl_2: pinctrl@10d10000 { >>>>> #address-cells = <1>; >>>>> #size-cells = <0>; >>>>> gpv2: gpv2 { >>>>> reg = <0x060>; >>>>> }; >>>>> gpv4: gpv4 { >>>>> reg = <0xc0>; >>>>> }; >>>>> }; >>>>> >>>>> This is clearly valid so it looks like the conversion to use >>>>> fdt_translate_address() in dev_get_addr() is not currently a good >>>>> move. >>>> >>>> >>>> To disable that, why not simply turn off CONFIG_OF_TRANSLATE on the >>>> affected >>>> platforms? That's precisely why that config option was introduced >>>> when the >>>> call to fdt_translate_address() was added to dev_get_addr()? >>>> >>>> That would prevent this patch from affecting platforms that don't >>>> trigger >>>> this issue, this leaving the valid check in place. >>> >>> But since this breaks normal behaviour we don't know what platforms >>> are affected. We have made CONFIG_OF_TRANSLATE the default. So this >>> approach doesn't seem (in effect) any better than Przemyslaw's newer >>> series, below. >>> >>>> >>>>> Przemyslaw Marczak sent three patches to resolve this for exynos >>>>> boards: >>>>> >>>>> https://patchwork.ozlabs.org/patch/557008/ >>>>> https://patchwork.ozlabs.org/patch/557010/ >>>>> https://patchwork.ozlabs.org/patch/557009/ >>>>> >>>>> But this involves creating a new function, and everyone will need to >>>>> know >>>>> when to use which one. Also the problem may affect other boards. >>>> >>>> >>>> I suggest adding an extra parameter to dev_get_addr() (or whatever >>>> calls it) >>>> that indicates the root of the address space. The check on #size-cells >>>> should be skipped for that one node (or level of translation) but >>>> enabled >>>> for all other levels. This way, there would be no need for anyone to >>>> choose >>>> between functions; there'd only be one. Most cases (i.e. translation >>>> of MMIO >>>> addresses) would simply pass 0 as the extra parameter (for the root >>>> node), >>>> but in special cases where it's known translation is not expected to >>>> reach >>>> the root MMIO space (e.g. I2C, SPI controllers), the controller node >>>> would >>>> be passed in. >>> >>> How would the caller know this root? It sounds plausible, but I do >>> want to avoid complex rules. I think you are saying that buses that >>> use their own address mechanism (i.e. not MMIO) must do something >>> special. The current dev_get_addr() is really simple. >> >> Simon, Stephen >> >> As a summary of the issue, please tell me your opinion about this: >> >> Since the __of_translate_address() is called always with the "ranges" as >> an argument, it looks reasonable to check it at the function beginning, >> that the "ranges" property exists in the parent node. >> >> If not exists - then don't check the size-cells count and this should >> fix the problem with additional argument. >> This is simple and correct from specification point of view - which says >> ranges and #size-cells property's - are not required [ePAPR v1.1]. > > It /might/ indeed be reasonable to skip the check on #size-cells if > there is no ranges property; a missing ranges property is already > treated as a 1:1 translation as described by the comment at the top of > of_translate_one(). Still, there are likely some edge-cases w.r.t. the > "length" fields in the ranges property that would need to be thought > through in more detail before I'd say for certain that this change does > absolutely make sense. > > Either way though, making that change wouldn't solve the problem at all. > > It's simply not possible to translate an I2C device address beyond the > root of the I2C address space; there is no equivalent of an I2C device's > address in MMIO address space. The root of the problem is that the code > is attempting to perform an I2C->MMIO address translation. If we prevent > that (by capping any such translation at the root of the I2C address > space for example), the problem is solved at that point. Once that's > solved, the issue of translating across a boundary with #size-cells=0 > will never appear, and hence does not need to be solved. > > The problem with I2C and I2C->MMIO address translation is solved by my new patch. The specification is clear for such case, if reg can't be mapped to it's parent address space, then don't use 'ranges' and this can be easy solved by additional check. Please review my new patch: https://patchwork.ozlabs.org/patch/564246/ Best regards,
On 01/07/2016 04:43 AM, Przemyslaw Marczak wrote: > Hello, > > On 01/05/2016 06:26 PM, Stephen Warren wrote: >> On 01/05/2016 08:47 AM, Przemyslaw Marczak wrote: >>> Hello, >>> >>> On 01/05/2016 02:00 AM, Simon Glass wrote: >>>> Hi Stephen, >>>> >>>> On 4 January 2016 at 13:15, Stephen Warren <swarren@wwwdotorg.org> >>>> wrote: >>>>> On 01/03/2016 04:04 PM, Simon Glass wrote: >>>>>> >>>>>> It is common for I2C and SPI buses to have a single-cell address >>>>>> and a >>>>>> size >>>>>> of 0. These produce a warning at present. For example on snow: >>>>>> >>>>>> __of_translate_address: Bad cell count for gpc4 >>>>>> __of_translate_address: Bad cell count for gpx0 >>>>>> __of_translate_address: Bad cell count for gpv2 >>>>>> __of_translate_address: Bad cell count for gpv4 >>>>>> >>>>>> One of the nodes in question looks like this in part: >>>>>> >>>>>> pinctrl_2: pinctrl@10d10000 { >>>>>> #address-cells = <1>; >>>>>> #size-cells = <0>; >>>>>> gpv2: gpv2 { >>>>>> reg = <0x060>; >>>>>> }; >>>>>> gpv4: gpv4 { >>>>>> reg = <0xc0>; >>>>>> }; >>>>>> }; >>>>>> >>>>>> This is clearly valid so it looks like the conversion to use >>>>>> fdt_translate_address() in dev_get_addr() is not currently a good >>>>>> move. >>>>> >>>>> >>>>> To disable that, why not simply turn off CONFIG_OF_TRANSLATE on the >>>>> affected >>>>> platforms? That's precisely why that config option was introduced >>>>> when the >>>>> call to fdt_translate_address() was added to dev_get_addr()? >>>>> >>>>> That would prevent this patch from affecting platforms that don't >>>>> trigger >>>>> this issue, this leaving the valid check in place. >>>> >>>> But since this breaks normal behaviour we don't know what platforms >>>> are affected. We have made CONFIG_OF_TRANSLATE the default. So this >>>> approach doesn't seem (in effect) any better than Przemyslaw's newer >>>> series, below. >>>> >>>>> >>>>>> Przemyslaw Marczak sent three patches to resolve this for exynos >>>>>> boards: >>>>>> >>>>>> https://patchwork.ozlabs.org/patch/557008/ >>>>>> https://patchwork.ozlabs.org/patch/557010/ >>>>>> https://patchwork.ozlabs.org/patch/557009/ >>>>>> >>>>>> But this involves creating a new function, and everyone will need to >>>>>> know >>>>>> when to use which one. Also the problem may affect other boards. >>>>> >>>>> >>>>> I suggest adding an extra parameter to dev_get_addr() (or whatever >>>>> calls it) >>>>> that indicates the root of the address space. The check on #size-cells >>>>> should be skipped for that one node (or level of translation) but >>>>> enabled >>>>> for all other levels. This way, there would be no need for anyone to >>>>> choose >>>>> between functions; there'd only be one. Most cases (i.e. translation >>>>> of MMIO >>>>> addresses) would simply pass 0 as the extra parameter (for the root >>>>> node), >>>>> but in special cases where it's known translation is not expected to >>>>> reach >>>>> the root MMIO space (e.g. I2C, SPI controllers), the controller node >>>>> would >>>>> be passed in. >>>> >>>> How would the caller know this root? It sounds plausible, but I do >>>> want to avoid complex rules. I think you are saying that buses that >>>> use their own address mechanism (i.e. not MMIO) must do something >>>> special. The current dev_get_addr() is really simple. >>> >>> Simon, Stephen >>> >>> As a summary of the issue, please tell me your opinion about this: >>> >>> Since the __of_translate_address() is called always with the "ranges" as >>> an argument, it looks reasonable to check it at the function beginning, >>> that the "ranges" property exists in the parent node. >>> >>> If not exists - then don't check the size-cells count and this should >>> fix the problem with additional argument. >>> This is simple and correct from specification point of view - which says >>> ranges and #size-cells property's - are not required [ePAPR v1.1]. >> >> It /might/ indeed be reasonable to skip the check on #size-cells if >> there is no ranges property; a missing ranges property is already >> treated as a 1:1 translation as described by the comment at the top of >> of_translate_one(). Still, there are likely some edge-cases w.r.t. the >> "length" fields in the ranges property that would need to be thought >> through in more detail before I'd say for certain that this change does >> absolutely make sense. >> >> Either way though, making that change wouldn't solve the problem at all. >> >> It's simply not possible to translate an I2C device address beyond the >> root of the I2C address space; there is no equivalent of an I2C device's >> address in MMIO address space. The root of the problem is that the code >> is attempting to perform an I2C->MMIO address translation. If we prevent >> that (by capping any such translation at the root of the I2C address >> space for example), the problem is solved at that point. Once that's >> solved, the issue of translating across a boundary with #size-cells=0 >> will never appear, and hence does not need to be solved. >> >> > > The problem with I2C and I2C->MMIO address translation is solved by my > new patch. The specification is clear for such case, if reg can't be > mapped to it's parent address space, then don't use 'ranges' and this > can be easy solved by additional check. Logic dictates that such a case (I2C->MMIO) doesn't make sense, so the specification doesn't cover it at all; see my upcoming comments on that patch.
diff --git a/common/fdt_support.c b/common/fdt_support.c index 66464db..43c5fa8 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -952,8 +952,7 @@ 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) ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS) /* Debug utility */ #ifdef DEBUG
It is common for I2C and SPI buses to have a single-cell address and a size of 0. These produce a warning at present. For example on snow: __of_translate_address: Bad cell count for gpc4 __of_translate_address: Bad cell count for gpx0 __of_translate_address: Bad cell count for gpv2 __of_translate_address: Bad cell count for gpv4 One of the nodes in question looks like this in part: pinctrl_2: pinctrl@10d10000 { #address-cells = <1>; #size-cells = <0>; gpv2: gpv2 { reg = <0x060>; }; gpv4: gpv4 { reg = <0xc0>; }; }; This is clearly valid so it looks like the conversion to use fdt_translate_address() in dev_get_addr() is not currently a good move. Przemyslaw Marczak sent three patches to resolve this for exynos boards: https://patchwork.ozlabs.org/patch/557008/ https://patchwork.ozlabs.org/patch/557010/ https://patchwork.ozlabs.org/patch/557009/ But this involves creating a new function, and everyone will need to know when to use which one. Also the problem may affect other boards. Instead we can relax the contraint that there must be at least one size cell. This fixes the problem on snow and should not affect anything else, since the error check should not fire on normal device trees anyway. After the release we will have more time to come up with a better solution, if one exists. Signed-off-by: Simon Glass <sjg@chromium.org> --- common/fdt_support.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)