Re: commit e38c0a1f breaks powerpc boards with uli1575 chip

Re: commit e38c0a1f breaks powerpc boards with uli1575 chip

[Rob Herring]

[fixed DT maillist address]

On 12/17/2013 01:35 AM, Nikita Yushchenko wrote:
> Hi
> 
> While trying to make freescale p2020ds and  mpc8572ds boards working with mainline kernel, I faced that commit 
> e38c0a1f (Handle #address-cells > 2 specially) breaks things with these boards.

Good to see this broke in v3.7 and is just now found...

> 
> Both these boards have uli1575 chip.
> Corresponding part in device tree is something like
> 
>                 uli1575@0 {
>                         reg = <0x0 0x0 0x0 0x0 0x0>;
>                         #size-cells = <2>;
>                         #address-cells = <3>;
>                         ranges = <0x2000000 0x0 0x80000000
>                                   0x2000000 0x0 0x80000000
>                                   0x0 0x20000000
> 
>                                   0x1000000 0x0 0x0
>                                   0x1000000 0x0 0x0
>                                   0x0 0x10000>;
>                         isa@1e {
> ...
> 
> I.e. it has #address-cells = <3>
> 
> 
> With commit e38c0a1f reverted, devices under uli1575 are registered correctly, e.g. for rtc
> 
> OF: ** translation for device /pcie@ffe09000/pcie@0/uli1575@0/isa@1e/rtc@70 **
> OF: bus is isa (na=2, ns=1) on /pcie@ffe09000/pcie@0/uli1575@0/isa@1e
> OF: translating address: 00000001 00000070
> OF: parent bus is default (na=3, ns=2) on /pcie@ffe09000/pcie@0/uli1575@0
> OF: walking ranges...
> OF: ISA map, cp=0, s=1000, da=70
> OF: parent translation for: 01000000 00000000 00000000
> OF: with offset: 70
> OF: one level translation: 00000000 00000000 00000070
> OF: parent bus is pci (na=3, ns=2) on /pcie@ffe09000/pcie@0
> OF: walking ranges...
> OF: default map, cp=a0000000, s=20000000, da=70
> OF: default map, cp=0, s=10000, da=70
> OF: parent translation for: 01000000 00000000 00000000
> OF: with offset: 70
> OF: one level translation: 01000000 00000000 00000070
> OF: parent bus is pci (na=3, ns=2) on /pcie@ffe09000
> OF: walking ranges...
> OF: PCI map, cp=0, s=10000, da=70
> OF: parent translation for: 01000000 00000000 00000000
> OF: with offset: 70
> OF: one level translation: 01000000 00000000 00000070
> OF: parent bus is default (na=2, ns=2) on /
> OF: walking ranges...
> OF: PCI map, cp=0, s=10000, da=70
> OF: parent translation for: 00000000 ffc10000
> OF: with offset: 70
> OF: one level translation: 00000000 ffc10070
> OF: reached root node
> 
> With commit e38c0a1f in place, address translation fails:
> 
> OF: ** translation for device /pcie@ffe09000/pcie@0/uli1575@0/isa@1e/rtc@70 **
> OF: bus is isa (na=2, ns=1) on /pcie@ffe09000/pcie@0/uli1575@0/isa@1e
> OF: translating address: 00000001 00000070
> OF: parent bus is default (na=3, ns=2) on /pcie@ffe09000/pcie@0/uli1575@0
> OF: walking ranges...
> OF: ISA map, cp=0, s=1000, da=70
> OF: parent translation for: 01000000 00000000 00000000
> OF: with offset: 70
> OF: one level translation: 00000000 00000000 00000070
> OF: parent bus is pci (na=3, ns=2) on /pcie@ffe09000/pcie@0
> OF: walking ranges...
> OF: default map, cp=a0000000, s=20000000, da=70
> OF: default map, cp=0, s=10000, da=70
> OF: not found !
> 
> Either e38c0a1f should be reverted, or uli1575 (and perhaps other similar devices) have to be described in device 
> trees differently.

Reverting would break Tegra PCIe, but you should not have to change the
DT either. So we need a solution.

Is this something like this sufficient to fix it?

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 4b9317b..378aebd 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -74,7 +74,7 @@ static u64 of_bus_default_map(__be32 *addr, const
__be32 *range,
         * mapping doesn't specify a physical address. Rather, the address
         * specifies an identifier that must match exactly.
         */
-       if (na > 2 && memcmp(range, addr, na * 4) != 0)
+       if (na > 2 && memcmp(range, addr, (na - 2) * 4) != 0)
                return OF_BAD_ADDR;

        if (da < cp || da >= (cp + s))

Re: commit e38c0a1f breaks powerpc boards with uli1575 chip

[Nikita Yushchenko]

> Reverting would break Tegra PCIe, but you should not have to change the
> DT either. So we need a solution.
>
> Is this something like this sufficient to fix it?
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 4b9317b..378aebd 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -74,7 +74,7 @@ static u64 of_bus_default_map(__be32 *addr, const
> __be32 *range,
>          * mapping doesn't specify a physical address. Rather, the
> address * specifies an identifier that must match exactly.
>          */
> -       if (na > 2 && memcmp(range, addr, na * 4) != 0)
> +       if (na > 2 && memcmp(range, addr, (na - 2) * 4) != 0)
>                 return OF_BAD_ADDR;
>
>         if (da < cp || da >= (cp + s))


No, this does not help.

I've dumped the actual content of 'range' and 'addr' at the failure point 
(i.e. ar point that returns error with e38c0a1f but passes without 
e38c0a1f ):

OF: default map, cp=0, s=10000, da=70
range:  01 00 00 00 00 00 00 00 00 00 00 00
 addr:  00 00 00 00 00 00 00 00 00 00 00 70

Nikita

Re: commit e38c0a1f breaks powerpc boards with uli1575 chip

[Benjamin Herrenschmidt]

On Thu, 2013-12-19 at 08:42 +0400, Nikita Yushchenko wrote:
> No, this does not help.
> 
> I've dumped the actual content of 'range' and 'addr' at the failure
> point 
> (i.e. ar point that returns error with e38c0a1f but passes without 
> e38c0a1f ):
> 
> OF: default map, cp=0, s=10000, da=70
> range:  01 00 00 00 00 00 00 00 00 00 00 00
>  addr:  00 00 00 00 00 00 00 00 00 00 00 70

Something that has a #address-cells larger than 2, or more generally,
an address field that contains more than a single number, must have
a specific translation backend, like we have for PCI.

This is a bit annoying but originates from the original OFW stuff on
which this stuff is based where the bus node would provide the methods
for translation.

Cheers,
Ben.
 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: commit e38c0a1f breaks powerpc boards with uli1575 chip

[Scott Wood]

On Mon, 2013-12-30 at 14:13 +1100, Benjamin Herrenschmidt wrote:
> On Thu, 2013-12-19 at 08:42 +0400, Nikita Yushchenko wrote:
> > No, this does not help.
> > 
> > I've dumped the actual content of 'range' and 'addr' at the failure
> > point 
> > (i.e. ar point that returns error with e38c0a1f but passes without 
> > e38c0a1f ):
> > 
> > OF: default map, cp=0, s=10000, da=70
> > range:  01 00 00 00 00 00 00 00 00 00 00 00
> >  addr:  00 00 00 00 00 00 00 00 00 00 00 70
> 
> Something that has a #address-cells larger than 2, or more generally,
> an address field that contains more than a single number, must have
> a specific translation backend, like we have for PCI.
> 
> This is a bit annoying but originates from the original OFW stuff on
> which this stuff is based where the bus node would provide the methods
> for translation.

I can maybe see that for PCI which has a special encoding, but why is it
always needed?  E.g. if Freescale localbus had a 64-bit offset instead
of 32-bit, the child nodes would have 3 address cells, but
straightforward use of ranges would bring it down to 2 for the final
physical address.  Existing localbus nodes already have "an address
field that contains more than a single number"; it's just a simple
enough encoding that it works to treat it as if it were a single large
number.

-Scott

Re: commit e38c0a1f breaks powerpc boards with uli1575 chip

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4008 bytes --]

On Wed, Dec 18, 2013 at 12:40:21PM -0600, Rob Herring wrote:
> [fixed DT maillist address]
> 
> On 12/17/2013 01:35 AM, Nikita Yushchenko wrote:
> > Hi
> > 
> > While trying to make freescale p2020ds and  mpc8572ds boards working with mainline kernel, I faced that commit 
> > e38c0a1f (Handle #address-cells > 2 specially) breaks things with these boards.
> 
> Good to see this broke in v3.7 and is just now found...
> 
> > 
> > Both these boards have uli1575 chip.
> > Corresponding part in device tree is something like
> > 
> >                 uli1575@0 {
> >                         reg = <0x0 0x0 0x0 0x0 0x0>;
> >                         #size-cells = <2>;
> >                         #address-cells = <3>;
> >                         ranges = <0x2000000 0x0 0x80000000
> >                                   0x2000000 0x0 0x80000000
> >                                   0x0 0x20000000
> > 
> >                                   0x1000000 0x0 0x0
> >                                   0x1000000 0x0 0x0
> >                                   0x0 0x10000>;
> >                         isa@1e {
> > ...
> > 
> > I.e. it has #address-cells = <3>
> > 
> > 
> > With commit e38c0a1f reverted, devices under uli1575 are registered correctly, e.g. for rtc
> > 
> > OF: ** translation for device /pcie@ffe09000/pcie@0/uli1575@0/isa@1e/rtc@70 **
> > OF: bus is isa (na=2, ns=1) on /pcie@ffe09000/pcie@0/uli1575@0/isa@1e
> > OF: translating address: 00000001 00000070
> > OF: parent bus is default (na=3, ns=2) on /pcie@ffe09000/pcie@0/uli1575@0
> > OF: walking ranges...
> > OF: ISA map, cp=0, s=1000, da=70
> > OF: parent translation for: 01000000 00000000 00000000
> > OF: with offset: 70
> > OF: one level translation: 00000000 00000000 00000070
> > OF: parent bus is pci (na=3, ns=2) on /pcie@ffe09000/pcie@0
> > OF: walking ranges...
> > OF: default map, cp=a0000000, s=20000000, da=70
> > OF: default map, cp=0, s=10000, da=70
> > OF: parent translation for: 01000000 00000000 00000000
> > OF: with offset: 70
> > OF: one level translation: 01000000 00000000 00000070
> > OF: parent bus is pci (na=3, ns=2) on /pcie@ffe09000
> > OF: walking ranges...
> > OF: PCI map, cp=0, s=10000, da=70
> > OF: parent translation for: 01000000 00000000 00000000
> > OF: with offset: 70
> > OF: one level translation: 01000000 00000000 00000070
> > OF: parent bus is default (na=2, ns=2) on /
> > OF: walking ranges...
> > OF: PCI map, cp=0, s=10000, da=70
> > OF: parent translation for: 00000000 ffc10000
> > OF: with offset: 70
> > OF: one level translation: 00000000 ffc10070
> > OF: reached root node
> > 
> > With commit e38c0a1f in place, address translation fails:
> > 
> > OF: ** translation for device /pcie@ffe09000/pcie@0/uli1575@0/isa@1e/rtc@70 **
> > OF: bus is isa (na=2, ns=1) on /pcie@ffe09000/pcie@0/uli1575@0/isa@1e
> > OF: translating address: 00000001 00000070
> > OF: parent bus is default (na=3, ns=2) on /pcie@ffe09000/pcie@0/uli1575@0
> > OF: walking ranges...
> > OF: ISA map, cp=0, s=1000, da=70
> > OF: parent translation for: 01000000 00000000 00000000
> > OF: with offset: 70
> > OF: one level translation: 00000000 00000000 00000070
> > OF: parent bus is pci (na=3, ns=2) on /pcie@ffe09000/pcie@0
> > OF: walking ranges...
> > OF: default map, cp=a0000000, s=20000000, da=70
> > OF: default map, cp=0, s=10000, da=70
> > OF: not found !
> > 
> > Either e38c0a1f should be reverted, or uli1575 (and perhaps other similar devices) have to be described in device 
> > trees differently.
> 
> Reverting would break Tegra PCIe, but you should not have to change the
> DT either. So we need a solution.

We ended up merging a different address representation for Tegra PCIe
and I've confirmed that reverting this commit doesn't cause any obvious
regressions. I think all other drivers in drivers/pci/host ended up
copying what we did on Tegra, so I wouldn't expect any other breakage
either.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]