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;
MIME-Version: 1.0
References: <20210407123438.224551-1-mindal@semihalf.com> <20210511030658.GG3425@dragon>
 <AM0PR04MB67542D30A9424D455DB3ADD496539@AM0PR04MB6754.eurprd04.prod.outlook.com>
 <20210513021214.GJ3425@dragon> <20210513141921.i7sfmekbcw2m7vxd@skbuf>
 <CAPv3WKfnWFjfZw39avZBEyUpEsH2f=NCs8VfjeR+wzk4qV3GmA@mail.gmail.com> <20210513183102.6dflgb4v2oekdlq5@skbuf>
In-Reply-To: <20210513183102.6dflgb4v2oekdlq5@skbuf>
From:   =?UTF-8?Q?Kornel_Dul=C4=99ba?= <mindal@semihalf.com>
Date:   Fri, 14 May 2021 10:25:07 +0200
Message-ID: <CAKpxNiyxDi66CXW4Z_exC=uy6mNbfhdBG5czgqNZqxmszegB0A@mail.gmail.com>
Subject: Re: [PATCH] arm64: dts: fsl-ls1028a: Correct ECAM PCIE window ranges
To:     Vladimir Oltean <vladimir.oltean@nxp.com>
Cc:     Marcin Wojtas <mw@semihalf.com>, Shawn Guo <shawnguo@kernel.org>,
        Claudiu Manoil <claudiu.manoil@nxp.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>,
        "tn@semihalf.com" <tn@semihalf.com>,
        "upstream@semihalf.com" <upstream@semihalf.com>,
        Alexandru Marginean <alexandru.marginean@nxp.com>,
        Bjorn Helgaas <bhelgaas@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Vladimir,

On Thu, May 13, 2021 at 8:31 PM Vladimir Oltean <vladimir.oltean@nxp.com> w=
rote:
>
> 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@nxp.com> nap=
isa=C5=82(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@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 doe=
s 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..0x01f815f=
fff -> 0x0000000000
> > > > > pci-host-generic 1f0000000.pcie:      MEM 0x01f8160000..0x01f81cf=
fff -> 0x0000000000
> > > > >
> > > > > to:
> > > > >
> > > > > pci-host-generic 1f0000000.pcie: host bridge /soc/pcie@1f0000000 =
ranges:
> > > > > pci-host-generic 1f0000000.pcie:      MEM 0x01f8000000..0x01f815f=
fff -> 0x01f8000000
> > > > > pci-host-generic 1f0000000.pcie:      MEM 0x01f8160000..0x01f81cf=
fff -> 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 Fixe=
s:
> > > tag or not, probably not.
> > >
> > > Bjorn Helgaas did point out before that "The fact that all these wind=
ows
> > > map to PCI bus address 0 looks broken", so there's that:
> > >
> > > https://patchwork.kernel.org/project/linux-pci/cover/20201129230743.3=
006978-1-kw@linux.com/
> > >
> > > And while it does look "broken", with the Enhanced Allocation capabil=
ity
> > > 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.