[PATCH v7 3/6] PCI: designware: Add ARM64 support

Thu Aug 20 04:10:24 PDT 2015

Hi Lucas

Again many thanks for explaining and for your time.

I got your point now and I have dug a bit better in the PCI_DOMAINS code.

However I have a question...see inline below

> -----Original Message-----
> From: Lucas Stach [mailto:l.stach at pengutronix.de]
> Sent: Thursday, August 20, 2015 9:27 AM
> To: Gabriele Paoloni
> Cc: Wangzhou (B); Bjorn Helgaas; jingoohan1 at gmail.com; Pratyush Anand;
> Arnd Bergmann; linux at arm.linux.org.uk; thomas.petazzoni at free-
> electrons.com; lorenzo.pieralisi at arm.com; james.morse at arm.com;
> Liviu.Dudau at arm.com; jason at lakedaemon.net; robh at kernel.org; linux-
> pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> devicetree at vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo;
> qiuzhenfa; liudongdong (C); qiujiang; xuwei (O); Liguozhu (Kenneth)
> Subject: Re: [PATCH v7 3/6] PCI: designware: Add ARM64 support
> 
> Hi Gab,
> 
> Am Mittwoch, den 19.08.2015, 16:34 +0000 schrieb Gabriele Paoloni:
> > Hi Lucas
> >
> > First of all many thanks for the quick reply, really appreciated
> >
> > > -----Original Message-----
> > > From: Lucas Stach [mailto:l.stach at pengutronix.de]
> > > Sent: Wednesday, August 19, 2015 4:37 PM
> > > To: Gabriele Paoloni
> > > Cc: Wangzhou (B); Bjorn Helgaas; jingoohan1 at gmail.com; Pratyush
> Anand;
> > > Arnd Bergmann; linux at arm.linux.org.uk; thomas.petazzoni at free-
> > > electrons.com; lorenzo.pieralisi at arm.com; james.morse at arm.com;
> > > Liviu.Dudau at arm.com; jason at lakedaemon.net; robh at kernel.org; linux-
> > > pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> > > devicetree at vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo;
> > > qiuzhenfa; liudongdong (C); qiujiang; xuwei (O); Liguozhu (Kenneth)
> > > Subject: Re: [PATCH v7 3/6] PCI: designware: Add ARM64 support
> > >
> > > Hi Gab,
> > >
> > > Am Mittwoch, den 19.08.2015, 15:16 +0000 schrieb Gabriele Paoloni:
> > > > Hi Lucas
> > > >
> > > > I have rewritten the patch to take into account multiple
> controllers.
> > > >
> > > > As you can see now there is a static var in dw_pcie_host_init()
> that
> > > tracks
> > > > the bus numbers used.
> > >
> > > This is wrong. The DT specifies the valid bus number range. You can
> not
> > > just assign the next free bus number to the root bus.
> >
> > I think this is what is being done in
> > http://lxr.free-electrons.com/source/arch/arm/kernel/bios32.c#L495
> > and currently designware assigns the root bus number in
> > http://lxr.free-electrons.com/source/drivers/pci/host/pcie-
> designware.c#L730
> >
> You found the right spot. It works a bit different than I remember, but
> is less broken than your current code. :)
> 
> It actually assigns the next instance a root bus number behind the
> current instances bus range. Please note the difference to your code
> which assigns the next free bus number, which may still lay within the
> current instances bus range.

Mmmm here I have done a mistake: in the current designware the number of hw
controllers is always 1
http://lxr.free-electrons.com/source/drivers/pci/host/pcie-designware.c#L510
So the loop in bios32.c does not work on multiple PCIe ports...
However your comment about PCI_DOMAINS enlightened my mind and now I see
that when CONFIG_PCI_DOMAINS is defined we have the domains numbers
(tracked by __domain_nr).
So (correct me if I am wrong) if we have 2 PCIe ports that do not specify
the "bus-range" property, both root ports will enumerate starting from 
root_bus_nr = 0 and everything will still work ok.

So if my assumption is correct, I do not see why the orginal patch from Wang 
Zhou is wrong. 
The only point can be that assigning pp->root_bus_nr = 0 is not required 
as pp memory is kzalloc'ed (an therefore init to zero).

> 
> >
> > In general I agree with you but if you look at all the current
> drivers
> > based on designware none of them define the "bus-range" dtb property.
> > Therefore doing as you say would break the current driver when we
> have
> > multiple controllers...am I right?
> >
> The current _code_ works fine for multiple controllers. It's just that
> you must define the bus range property in the DTB if you want to enable
> multiple instances of one controller and support a kernel without PCI
> domains support. As all current implementations have only a single
> instance of the controller per SoC, or depend on PCI domain support,
> it's totally fine for them to not define the bus ranges property, as
> it's an optional property and it is well defined how things behave if
> it
> is absent. We absolutely need to keep that specified behavior.
> 
> > If that is the case in order to fix this in the way you say I would
> need
> > to assign "bus-range" for all the PCIe drivers with multiple
> controllers:
> > in this case I would split the default range evenly (that is, if we
> have
> > two controllers I would define "bus-range"  0-127 and 128-255)
> >
> > If you think this solution is ok I can go for it. My only doubt was
> about
> > touching other vendors DTBs....
> >
> You don't need to touch any of the current DTBs, as they are fine with
> the default bus range behavior. You need to keep the specified behavior
> of the bus range property with the new code.
> 
> Regards,
> Lucas
> >
> > >
> > > It is perfectly valid to have a bus range of 0x00-0x10 assigned to
> one
> > > instance and 0x50-0xff to the next instance. Additional with PCIe
> > > hotplug you may not use the full range of the bus numbers on one
> > > instance at the first scan, but only later populate more buses when
> > > more
> > > bridges are added to the tree.
> > >
> > > > Drivers that do not specify the bus range in the DTB set pp-
> > > >root_bus_nr = DW_ROOT_NR_UNDEFINED.
> > > > Designware will check if the flag is set and will use the
> automatic
> > > bus range
> > > > assignment.
> > >
> > > No, please lets get rid of this assignment altogether. The glue
> drivers
> > > have no business in assigning the bus range. Please remove the
> > > pp->root_bus_nr assignment from all the glue drivers.
> > >
> > > "bus range" is a generic DW PCIe property, so just parse the root
> bus
> > > number from the DT, it is handled the same way for all the DW based
> > > PCIe
> > > drivers. The bindings specifies that if the bus range property is
> > > missing the range is 0x00-0xff, so you can default to 0 as the root
> bus
> > > number in that case.
> > >
> > > Also I would think this conversion warrants a patch on its own and
> > > should not be mixed in the ARM64 support patch.
> > >
> > > Regards,
> > > Lucas
> > >
> 
> --
> Pengutronix e.K.             | Lucas Stach                 |
> Industrial Linux Solutions   | http://www.pengutronix.de/  |