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

From: Zhou Wang <wangzhou1@hisilicon.com>
Date: 2015-08-24 06:26:07
Also in: linux-arm-kernel, linux-pci

On 2015/8/21 22:19, Gabriele Paoloni wrote:

Hi Liviu

Many Thanks for reviewing

quoted

-----Original Message-----
From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
owner@vger.kernel.org] On Behalf Of Liviu Dudau
Sent: Friday, August 21, 2015 2:43 PM
To: Gabriele Paoloni
Cc: Lucas Stach; Wangzhou (B); Bjorn Helgaas; jingoohan1@gmail.com;
Pratyush Anand; Arnd Bergmann; linux@arm.linux.org.uk;
thomas.petazzoni@free-electrons.com; Lorenzo Pieralisi; James Morse;
jason@lakedaemon.net; robh@kernel.org; linux-pci@vger.kernel.org;
linux-arm-kernel@lists.infradead.org; devicetree@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

On Thu, Aug 20, 2015 at 12:10:24PM +0100, Gabriele Paoloni wrote:

quoted

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.

quoted

However I have a question...see inline below

quoted

-----Original Message-----
From: Lucas Stach [mailto:l.stach@pengutronix.de]
Sent: Thursday, August 20, 2015 9:27 AM
To: Gabriele Paoloni
Cc: Wangzhou (B); Bjorn Helgaas; jingoohan1@gmail.com; Pratyush

Anand;

quoted

Arnd Bergmann; linux@arm.linux.org.uk; thomas.petazzoni@free-
electrons.com; lorenzo.pieralisi@arm.com; james.morse@arm.com;
Liviu.Dudau@arm.com; jason@lakedaemon.net; robh@kernel.org; linux-
pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
devicetree@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@pengutronix.de]
Sent: Wednesday, August 19, 2015 4:37 PM
To: Gabriele Paoloni
Cc: Wangzhou (B); Bjorn Helgaas; jingoohan1@gmail.com; Pratyush

Anand;

quoted

Arnd Bergmann; linux@arm.linux.org.uk; thomas.petazzoni@free-
electrons.com; lorenzo.pieralisi@arm.com; james.morse@arm.com;
Liviu.Dudau@arm.com; jason@lakedaemon.net; robh@kernel.org;

linux-

quoted

pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
devicetree@vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo;
qiuzhenfa; liudongdong (C); qiujiang; xuwei (O); Liguozhu

(Kenneth)

quoted

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

As you can see now there is a static var in

dw_pcie_host_init()

quoted

that

quoted

tracks

quoted

the bus numbers used.

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

can

quoted

not

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

quoted

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

quoted

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

quoted

which assigns the next free bus number, which may still lay within

the

quoted

current instances bus range.

Hi Gabriele,

quoted

Mmmm here I have done a mistake: in the current designware the number

of hw

quoted

controllers is always 1
http://lxr.free-electrons.com/source/drivers/pci/host/pcie-

designware.c#L510

quoted

So the loop in bios32.c does not work on multiple PCIe ports...

Bios32.c is broken for multiple PCIe hosts for multiple reasons, this
is just
one of them. Hence the effort to get rid of it for maintained drivers.

As you can see we're pushing hard for this :)

How about we pass pp->busn->start to pci_create_root_bus as root bus number?
We get pp->busn->start from resource list(res) whose elements come from
of_pci_get_host_bridge_resources.

If there is a bus-range item in dts, root bus number will get from it.
Oppositely, root bus number will be default value(0x0).

Thanks,
Zhou

quoted

However your comment about PCI_DOMAINS enlightened my mind and now I

see

quoted

that when CONFIG_PCI_DOMAINS is defined we have the domains numbers
(tracked by __domain_nr).

CONFIG_PCI_DOMAINS in itself doesn't do much without adding code to
your driver.
You could request a new domain for each controller with a special
property to
disable that behaviour in the dts, or you could enable
CONFIG_PCI_DOMAINS_GENERIC
which will give you a new domain automatically.

So far I see that for ARM and ARM64 CONFIG_PCI_DOMAINS_GENERIC defaults to 
the same value as CONFIG_PCI_DOMAINS (see arch/arm/Kconfig, arch/arm64/Kconfig). 
So I think this is how current designware based drivers get multiple PCIe 
controller to work (they just enable CONFIG_PCI_DOMAINS)...

quoted

So (correct me if I am wrong) if we have 2 PCIe ports that do not

specify

quoted

the "bus-range" property, both root ports will enumerate starting

from

quoted

root_bus_nr = 0 and everything will still work ok.

Correct.

Best regards,
Liviu

quoted

So if my assumption is correct, I do not see why the orginal patch

from Wang

quoted

Zhou is wrong.
The only point can be that assigning pp->root_bus_nr = 0 is not

required

quoted

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.

quoted

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

quoted

you must define the bus range property in the DTB if you want to

enable

quoted

multiple instances of one controller and support a kernel without

PCI

quoted

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

quoted

it's an optional property and it is well defined how things behave

if

quoted

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

quoted

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

quoted

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

quoted

about

quoted

touching other vendors DTBs....

You don't need to touch any of the current DTBs, as they are fine

with

quoted

the default bus range behavior. You need to keep the specified

behavior

quoted

of the bus range property with the new code.

Regards,
Lucas

quoted

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

to

quoted

one

quoted

instance and 0x50-0xff to the next instance. Additional with

PCIe

quoted

hotplug you may not use the full range of the bus numbers on

one

quoted

instance at the first scan, but only later populate more buses

when

quoted

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

bus range

quoted

assignment.

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

drivers

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

quoted

bus

quoted

number from the DT, it is handled the same way for all the DW

based

quoted

PCIe
drivers. The bindings specifies that if the bus range property

is

quoted

missing the range is 0x00-0xff, so you can default to 0 as the

root

quoted

bus

quoted

number in that case.

Also I would think this conversion warrants a patch on its own

and

quoted

should not be mixed in the ARM64 support patch.

Regards,
Lucas

--
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

--
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help