Re: [PATCH] arm64: dts: fsl-ls1028a: Correct ECAM PCIE window ranges

From: Vladimir Oltean
Date: Thu May 13 2021 - 10:20:44 EST

Next message: Tony Krowiak: "Re: [PATCH v2] s390/vfio-ap: fix memory leak in mdev remove callback"
Previous message: Greg KH: "Re: [PATCH] tty: serial: 8250: serial_cs: Fix a memory leak in error handling path"
In reply to: Shawn Guo: "Re: [PATCH] arm64: dts: fsl-ls1028a: Correct ECAM PCIE window ranges"
Next in thread: Marcin Wojtas: "Re: [PATCH] arm64: dts: fsl-ls1028a: Correct ECAM PCIE window ranges"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
> > >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@xxxxxxxxxxxx>
> > >
> > >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@xxxxxxx>
>
> Thanks, Claudiu.
>
> Kornel,
>
> Do we need a Fixes tag for this patch?
>
> Shawn

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

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@xxxxxxxxx/

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 -> 0x0000000000
+pci-host-generic 1f0000000.pcie: MEM 0x01f8000000..0x01f815ffff -> 0x01f8000000
pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: tmp_res start 0x01f8000000 end 0x01f815ffff
-pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pci_addr 0x0000000000 => offset 0x01f8000000
-pci-host-generic 1f0000000.pcie: MEM 0x01f8160000..0x01f81cffff -> 0x0000000000
+pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pci_addr 0x01f8000000 => offset 0x0000000000
+pci-host-generic 1f0000000.pcie: MEM 0x01f8160000..0x01f81cffff -> 0x01f8160000
pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: tmp_res start 0x01f8160000 end 0x01f81cffff
-pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pci_addr 0x0000000000 => offset 0x01f8160000
-pci-host-generic 1f0000000.pcie: MEM 0x01f81d0000..0x01f81effff -> 0x0000000000
+pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pci_addr 0x01f8160000 => offset 0x0000000000
+pci-host-generic 1f0000000.pcie: MEM 0x01f81d0000..0x01f81effff -> 0x01f81d0000
pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: tmp_res start 0x01f81d0000 end 0x01f81effff
-pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pci_addr 0x0000000000 => offset 0x01f81d0000
-pci-host-generic 1f0000000.pcie: MEM 0x01f81f0000..0x01f820ffff -> 0x0000000000
+pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pci_addr 0x01f81d0000 => offset 0x0000000000
+pci-host-generic 1f0000000.pcie: MEM 0x01f81f0000..0x01f820ffff -> 0x01f81f0000
pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: tmp_res start 0x01f81f0000 end 0x01f820ffff
-pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pci_addr 0x0000000000 => offset 0x01f81f0000
-pci-host-generic 1f0000000.pcie: MEM 0x01f8210000..0x01f822ffff -> 0x0000000000
+pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pci_addr 0x01f81f0000 => offset 0x0000000000
+pci-host-generic 1f0000000.pcie: MEM 0x01f8210000..0x01f822ffff -> 0x01f8210000
pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: tmp_res start 0x01f8210000 end 0x01f822ffff
-pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pci_addr 0x0000000000 => offset 0x01f8210000
-pci-host-generic 1f0000000.pcie: MEM 0x01f8230000..0x01f824ffff -> 0x0000000000
+pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pci_addr 0x01f8210000 => offset 0x0000000000
+pci-host-generic 1f0000000.pcie: MEM 0x01f8230000..0x01f824ffff -> 0x01f8230000
pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: tmp_res start 0x01f8230000 end 0x01f824ffff
-pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pci_addr 0x0000000000 => offset 0x01f8230000
-pci-host-generic 1f0000000.pcie: MEM 0x01fc000000..0x01fc3fffff -> 0x0000000000
+pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pci_addr 0x01f8230000 => offset 0x0000000000
+pci-host-generic 1f0000000.pcie: MEM 0x01fc000000..0x01fc3fffff -> 0x01fc000000
pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: tmp_res start 0x01fc000000 end 0x01fc3fffff
-pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pci_addr 0x0000000000 => offset 0x01fc000000
+pci-host-generic 1f0000000.pcie: devm_of_pci_get_host_bridge_resources: pci_addr 0x01fc000000 => 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 address [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 address [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 address [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 address [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 Enhanced Allocation, properties 0x0)
pci 0000:00:00.0: BAR 2: [mem 0x1f8160000-0x1f816ffff 64bit pref] (from Enhanced 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=4
Entry 0: Enable+ Writable- EntrySize=3
BAR Equivalent Indicator: BAR 0
PrimaryProperties: memory space, non-prefetchable
SecondaryProperties: entry unavailable for use, PrimaryProperties should be used
Base: 1f8000000
MaxOffset: 0003ffff
Entry 1: Enable+ Writable- EntrySize=3
BAR Equivalent Indicator: BAR 2
PrimaryProperties: memory space, prefetchable
SecondaryProperties: memory space, non-prefetchable
Base: 1f8160000
MaxOffset: 0000ffff
Entry 2: Enable+ Writable- EntrySize=3
BAR Equivalent Indicator: VF-BAR 0
PrimaryProperties: VF memory space, non-prefetchable
SecondaryProperties: entry unavailable for use, PrimaryProperties should be used
Base: 1f81d0000
MaxOffset: 0000ffff
Entry 3: Enable+ Writable- EntrySize=3
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 = <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

Next message: Tony Krowiak: "Re: [PATCH v2] s390/vfio-ap: fix memory leak in mdev remove callback"
Previous message: Greg KH: "Re: [PATCH] tty: serial: 8250: serial_cs: Fix a memory leak in error handling path"
In reply to: Shawn Guo: "Re: [PATCH] arm64: dts: fsl-ls1028a: Correct ECAM PCIE window ranges"
Next in thread: Marcin Wojtas: "Re: [PATCH] arm64: dts: fsl-ls1028a: Correct ECAM PCIE window ranges"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]