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

[Kornel Dulęba]

Hi Vladimir,

On Thu, May 13, 2021 at 8:31 PM Vladimir Oltean <vladimir.oltean at nxp.com> wrote:
>
> Hi Marcin,
>
> On Thu, May 13, 2021 at 07:54:15PM +0200, Marcin Wojtas wrote:
> > Hi Vladimir,
> >
> > czw., 13 maj 2021 o 16:19 Vladimir Oltean <vladimir.oltean at nxp.com> napisał(a):
> > >
> > > 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 at 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 at 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 at 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 at 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 at nxp.com>
> > > >
> > > > Thanks, Claudiu.
> > > >
> > > > Kornel,
> > > >
> > > > Do we need a Fixes tag for this patch?
> > > >
> > > > Shawn
> > >
> > > Reviewed-by: Vladimir Oltean <vladimir.oltean at 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.
> >
> > ...in Linux. Please note Linux device trees can be used as-is by other
> > projects. Regardless my opinion on how that's unfortunate, FreeBSD
> > does additional ranges check before performing EA and fails. Since the
> > current DT description is imo broken and the change is transparent for
> > Linux, it would be great to get this change merged into tree in case
> > there are are no objections.
>
> Just for my curiosity, can you please link me to the extra FreeBSD checks?

FreeBSD parses values from "ranges" and uses "rman" API to store them.
Now "rman", or Resource manager is used in the FreeBSD kernel to
manage memory regions.
In particular it checks if any two regions inserted into the same
"manager" overlap.
If they do it is treated as a fatal error, which in our case causes
the PCI driver to fail to probe.
code: https://github.com/freebsd/freebsd-src/blob/main/sys/dev/pci/pci_host_generic.c#L148.

>
> Anyway, I'm not sure what is more "broken", to have a "ranges" property
> when no address translation takes place, or for that "ranges" property
> to be set to a confusing "child address space" value. That's not to say
> I have an objection against Shawn merging the patch.
>
> My main point was slightly different though, the "ranges" property is
> currently mandatory, although in this case it provides no information
> which cannot be retrieved directly from the config space. Properties
> that have no other use except to be pedantic are, well, useless.
> Maybe we can do something about that too.