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

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:

quoted

Hi Lucas

First of all many thanks for the quick reply, really appreciated

quoted

-----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;

quoted

quoted

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:

quoted

Hi Lucas

I have rewritten the patch to take into account multiple

controllers.

quoted

quoted

quoted

As you can see now there is a static var in dw_pcie_host_init()

that

quoted

quoted

tracks

quoted

the bus numbers used.

This is wrong. The DT specifies the valid bus number range. You can

not

quoted

quoted

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

quoted

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).

quoted

In general I agree with you but if you look at all the current

drivers

quoted

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

quoted

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.

quoted

If that is the case in order to fix this in the way you say I would

need

quoted

to assign "bus-range" for all the PCIe drivers with multiple

controllers:

quoted

in this case I would split the default range evenly (that is, if we

have

quoted

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

quoted

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

quoted

quoted

It is perfectly valid to have a bus range of 0x00-0x10 assigned to

one

quoted

quoted

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.

quoted

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

quoted

quoted

bus range

quoted

assignment.

No, please lets get rid of this assignment altogether. The glue

drivers

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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/  |