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

[Kornel Dulęba]

Hi Shawn,

On Fri, May 14, 2021 at 10:25 AM Kornel Dulęba <mindal@xxxxxxxxxxxx> wrote:
>
> Hi Vladimir,
>
> On Thu, May 13, 2021 at 8:31 PM Vladimir Oltean <vladimir.oltean@xxxxxxx> 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@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?
>
> 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.

Do you have any more remarks regarding this patch?