Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     Shawn Guo <shawnguo@kernel.org>
CC:     Claudiu Manoil <claudiu.manoil@nxp.com>,
        Kornel Duleba <mindal@semihalf.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Leo Li <leoyang.li@nxp.com>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "mw@semihalf.com" <mw@semihalf.com>,
        "tn@semihalf.com" <tn@semihalf.com>,
        "upstream@semihalf.com" <upstream@semihalf.com>,
        Alexandru Marginean <alexandru.marginean@nxp.com>,
        Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: [PATCH] arm64: dts: fsl-ls1028a: Correct ECAM PCIE window ranges
Thread-Topic: [PATCH] arm64: dts: fsl-ls1028a: Correct ECAM PCIE window ranges
Thread-Index: AQHXRhK6wSxUwtoqZkenOXHlDRJ4r6reBtQAgAKnz4CAAMsmgA==
Date:   Thu, 13 May 2021 14:19:22 +0000
Message-ID: <20210513141921.i7sfmekbcw2m7vxd@skbuf>
References: <20210407123438.224551-1-mindal@semihalf.com>
 <20210511030658.GG3425@dragon>
 <AM0PR04MB67542D30A9424D455DB3ADD496539@AM0PR04MB6754.eurprd04.prod.outlook.com>
 <20210513021214.GJ3425@dragon>
In-Reply-To: <20210513021214.GJ3425@dragon>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <06E0CD60FBF9004FB6018489E1C93131@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: d0a18c19-a8b0-4287-4aa6-08d9161a1b01
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 May 2021 14:19:22.6176
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: cyxfGwId7QvQKnlLHPv4gXawqhMs2Bv2Uj2Lh7fc3KrpKMpYp7obQStgopG+1H+4PukRTqDgsfOxlMS4Iq5TwA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0402MB2861
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, May 13, 2021 at 10:12:15AM +0800, Shawn Guo wrote:
> On Tue, May 11, 2021 at 09:48:22AM +0000, Claudiu Manoil wrote:
> > >-----Original Message-----
> > >From: Shawn Guo <shawnguo@kernel.org>
> > >Sent: Tuesday, May 11, 2021 6:07 AM
> > [...]
> > >Subject: Re: [PATCH] arm64: dts: fsl-ls1028a: Correct ECAM PCIE window
> > >ranges
> > >
> > >+ Claudiu
> > >
> > >On Wed, Apr 07, 2021 at 02:34:38PM +0200, Kornel Duleba wrote:
> > >> Currently all PCIE windows point to bus address 0x0, which does not =
match
> > >> the values obtained from hardware during EA.
> > >> Replace those values with CPU addresses, since in reality we
> > >> have a 1:1 mapping between the two.
> > >>
> > >> Signed-off-by: Kornel Duleba <mindal@semihalf.com>
> > >
> > >Claudiu,
> > >
> > >Do you have any comment on this?
> > >
> >
> > Well, probing is still working with this change, I've just tested it.
> >
> > PCI listing at boot time changes from:
> >
> > pci-host-generic 1f0000000.pcie: host bridge /soc/pcie@1f0000000 ranges=
:
> > pci-host-generic 1f0000000.pcie:      MEM 0x01f8000000..0x01f815ffff ->=
 0x0000000000
> > pci-host-generic 1f0000000.pcie:      MEM 0x01f8160000..0x01f81cffff ->=
 0x0000000000
> >
> > to:
> >
> > pci-host-generic 1f0000000.pcie: host bridge /soc/pcie@1f0000000 ranges=
:
> > pci-host-generic 1f0000000.pcie:      MEM 0x01f8000000..0x01f815ffff ->=
 0x01f8000000
> > pci-host-generic 1f0000000.pcie:      MEM 0x01f8160000..0x01f81cffff ->=
 0x01f8160000
> >
> > and looks reasonable.
> > Adding Vladimir and Alex just in case.
> >
> > Acked-by: Claudiu Manoil <claudiu.manoil@nxp.com>
>
> Thanks, Claudiu.
>
> Kornel,
>
> Do we need a Fixes tag for this patch?
>
> Shawn

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

I am not sure whether "incorrect data that is unused" deserves a Fixes:
tag or not, probably not.

Bjorn Helgaas did point out before that "The fact that all these windows
map to PCI bus address 0 looks broken", so there's that:

https://patchwork.kernel.org/project/linux-pci/cover/20201129230743.3006978=
-1-kw@linux.com/

And while it does look "broken", with the Enhanced Allocation capability
and the pci-host-ecam-generic driver, there is no address translation
taking place, so no inbound/outbound windows are configured, so the
range.pci_addr calculated in devm_of_pci_get_host_bridge_resources() is
not used for anything except for printing.

FWIW here's a more complete image of what changes with Kornel's patch
("-" is before, "+" is after) - again all is limited to the dmesg output.

 pci-host-generic 1f0000000.pcie: host bridge /soc/pcie@1f0000000 ranges:
 pci-host-generic 1f0000000.pcie: Parsing ranges property...
-pci-host-generic 1f0000000.pcie:      MEM 0x01f8000000..0x01f815ffff -> 0x=
0000000000
+pci-host-generic 1f0000000.pcie:      MEM 0x01f8000000..0x01f815ffff -> 0x=
01f8000000
 pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: tm=
p_res start 0x01f8000000 end 0x01f815ffff
-pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pc=
i_addr 0x0000000000 =3D> offset 0x01f8000000
-pci-host-generic 1f0000000.pcie:      MEM 0x01f8160000..0x01f81cffff -> 0x=
0000000000
+pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pc=
i_addr 0x01f8000000 =3D> offset 0x0000000000
+pci-host-generic 1f0000000.pcie:      MEM 0x01f8160000..0x01f81cffff -> 0x=
01f8160000
 pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: tm=
p_res start 0x01f8160000 end 0x01f81cffff
-pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pc=
i_addr 0x0000000000 =3D> offset 0x01f8160000
-pci-host-generic 1f0000000.pcie:      MEM 0x01f81d0000..0x01f81effff -> 0x=
0000000000
+pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pc=
i_addr 0x01f8160000 =3D> offset 0x0000000000
+pci-host-generic 1f0000000.pcie:      MEM 0x01f81d0000..0x01f81effff -> 0x=
01f81d0000
 pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: tm=
p_res start 0x01f81d0000 end 0x01f81effff
-pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pc=
i_addr 0x0000000000 =3D> offset 0x01f81d0000
-pci-host-generic 1f0000000.pcie:      MEM 0x01f81f0000..0x01f820ffff -> 0x=
0000000000
+pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pc=
i_addr 0x01f81d0000 =3D> offset 0x0000000000
+pci-host-generic 1f0000000.pcie:      MEM 0x01f81f0000..0x01f820ffff -> 0x=
01f81f0000
 pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: tm=
p_res start 0x01f81f0000 end 0x01f820ffff
-pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pc=
i_addr 0x0000000000 =3D> offset 0x01f81f0000
-pci-host-generic 1f0000000.pcie:      MEM 0x01f8210000..0x01f822ffff -> 0x=
0000000000
+pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pc=
i_addr 0x01f81f0000 =3D> offset 0x0000000000
+pci-host-generic 1f0000000.pcie:      MEM 0x01f8210000..0x01f822ffff -> 0x=
01f8210000
 pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: tm=
p_res start 0x01f8210000 end 0x01f822ffff
-pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pc=
i_addr 0x0000000000 =3D> offset 0x01f8210000
-pci-host-generic 1f0000000.pcie:      MEM 0x01f8230000..0x01f824ffff -> 0x=
0000000000
+pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pc=
i_addr 0x01f8210000 =3D> offset 0x0000000000
+pci-host-generic 1f0000000.pcie:      MEM 0x01f8230000..0x01f824ffff -> 0x=
01f8230000
 pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: tm=
p_res start 0x01f8230000 end 0x01f824ffff
-pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pc=
i_addr 0x0000000000 =3D> offset 0x01f8230000
-pci-host-generic 1f0000000.pcie:      MEM 0x01fc000000..0x01fc3fffff -> 0x=
0000000000
+pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pc=
i_addr 0x01f8230000 =3D> offset 0x0000000000
+pci-host-generic 1f0000000.pcie:      MEM 0x01fc000000..0x01fc3fffff -> 0x=
01fc000000
 pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: tm=
p_res start 0x01fc000000 end 0x01fc3fffff
-pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pc=
i_addr 0x0000000000 =3D> offset 0x01fc000000
+pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pc=
i_addr 0x01fc000000 =3D> offset 0x0000000000
 pci-host-generic 1f0000000.pcie: ECAM at [mem 0x1f0000000-0x1f00fffff] for=
 [bus 00]
 pci-host-generic 1f0000000.pcie: PCI host bridge to bus 0000:00
 pci_bus 0000:00: root bus resource [bus 00]
-pci_bus 0000:00: root bus resource [mem 0x1f8000000-0x1f815ffff] (bus addr=
ess [0x00000000-0x0015ffff])
-pci_bus 0000:00: root bus resource [mem 0x1f8160000-0x1f81cffff pref] (bus=
 address [0x00000000-0x0006ffff])
-pci_bus 0000:00: root bus resource [mem 0x1f81d0000-0x1f81effff] (bus addr=
ess [0x00000000-0x0001ffff])
-pci_bus 0000:00: root bus resource [mem 0x1f81f0000-0x1f820ffff pref] (bus=
 address [0x00000000-0x0001ffff])
-pci_bus 0000:00: root bus resource [mem 0x1f8210000-0x1f822ffff] (bus addr=
ess [0x00000000-0x0001ffff])
-pci_bus 0000:00: root bus resource [mem 0x1f8230000-0x1f824ffff pref] (bus=
 address [0x00000000-0x0001ffff])
-pci_bus 0000:00: root bus resource [mem 0x1fc000000-0x1fc3fffff] (bus addr=
ess [0x00000000-0x003fffff])
+pci_bus 0000:00: root bus resource [mem 0x1f8000000-0x1f815ffff]
+pci_bus 0000:00: root bus resource [mem 0x1f8160000-0x1f81cffff pref]
+pci_bus 0000:00: root bus resource [mem 0x1f81d0000-0x1f81effff]
+pci_bus 0000:00: root bus resource [mem 0x1f81f0000-0x1f820ffff pref]
+pci_bus 0000:00: root bus resource [mem 0x1f8210000-0x1f822ffff]
+pci_bus 0000:00: root bus resource [mem 0x1f8230000-0x1f824ffff pref]
+pci_bus 0000:00: root bus resource [mem 0x1fc000000-0x1fc3fffff]
 pci 0000:00:00.0: [1957:e100] type 00 class 0x020001
 pci 0000:00:00.0: BAR 0: [mem 0x1f8000000-0x1f803ffff 64bit] (from Enhance=
d Allocation, properties 0x0)
 pci 0000:00:00.0: BAR 2: [mem 0x1f8160000-0x1f816ffff 64bit pref] (from En=
hanced Allocation, properties 0x1)

My understanding might be wrong, but it should be possible for the PCIe
host bridge driver to initialize some of its resources by enumerating
the functions which have the EA capability, and not require the device
tree writer to add a "ranges" entry for them at all. Then this discussion
would be moot - that resource would have no way to be incorrect.

$ lspci -vvv
0000:00:00.0 Ethernet controller: Freescale Semiconductor Inc Device e100 (=
rev 01) (prog-if 01)
	Subsystem: Freescale Semiconductor Inc Device e100
(...)
	Capabilities: [9c] Enhanced Allocation (EA): NumEntries=3D4
		Entry 0: Enable+ Writable- EntrySize=3D3
			 BAR Equivalent Indicator: BAR 0
			 PrimaryProperties: memory space, non-prefetchable
			 SecondaryProperties: entry unavailable for use, PrimaryProperties shoul=
d be used
			 Base: 1f8000000
			 MaxOffset: 0003ffff
		Entry 1: Enable+ Writable- EntrySize=3D3
			 BAR Equivalent Indicator: BAR 2
			 PrimaryProperties: memory space, prefetchable
			 SecondaryProperties: memory space, non-prefetchable
			 Base: 1f8160000
			 MaxOffset: 0000ffff
		Entry 2: Enable+ Writable- EntrySize=3D3
			 BAR Equivalent Indicator: VF-BAR 0
			 PrimaryProperties: VF memory space, non-prefetchable
			 SecondaryProperties: entry unavailable for use, PrimaryProperties shoul=
d be used
			 Base: 1f81d0000
			 MaxOffset: 0000ffff
		Entry 3: Enable+ Writable- EntrySize=3D3
			 BAR Equivalent Indicator: VF-BAR 2
			 PrimaryProperties: VF memory space, prefetchable
			 SecondaryProperties: VF memory space, prefetchable
			 Base: 1f81f0000
			 MaxOffset: 0000ffff

This information, which is already present in the hardware, needs to be
duplicated here (now I do see that the 'ranges' property declares them
larger than they really are, too):

				  /* PF0-6 BAR0 - non-prefetchable memory */
			ranges =3D <0x82000000 0x0 0x00000000  0x1 0xf8000000  0x0 0x160000
				  /* PF0-6 BAR2 - prefetchable memory */
				  0xc2000000 0x0 0x00000000  0x1 0xf8160000  0x0 0x070000
				  /* PF0: VF0-1 BAR0 - non-prefetchable memory */
				  0x82000000 0x0 0x00000000  0x1 0xf81d0000  0x0 0x020000
				  /* PF0: VF0-1 BAR2 - prefetchable memory */
				  0xc2000000 0x0 0x00000000  0x1 0xf81f0000  0x0 0x020000=