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?
