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

[Vladimir Oltean]

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@xxxxxxx> 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@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.
> 
> ...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?

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.