[PATCH] ARM: Kirkwood: Support basic hotplug for PCI-E

[PATCH] ARM: Kirkwood: Support basic hotplug for PCI-E

[Jason Gunthorpe]

Unconditionally register the PCI-E bus, even if the link is currently
down. When the link is brought up the bus can be scanned through
/sys/bus/pci/rescan or otherwise. Since the HW has no interrupt for
link up, userspace will have to take care of the timing.

An earlier version of this was contingent on CONFIG_HOTPLUG, but
that is being removed from the kernel.

This also fixes printing the link up/down message to be displayed
on one line (structured logging broke this?)

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 arch/arm/mach-kirkwood/pcie.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

All PCI-E ports are required to support hot plug at the link training
level. Our systems support it electrically, and userspace sequences
everything to work properly. But the PCI-E root port needs to be
registered with the kernel to initiate a rescan via sysfs when things
are ready.

diff --git a/arch/arm/mach-kirkwood/pcie.c b/arch/arm/mach-kirkwood/pcie.c
index 59c97fe..80fd4f2 100644
--- a/arch/arm/mach-kirkwood/pcie.c
+++ b/arch/arm/mach-kirkwood/pcie.c
@@ -244,13 +244,9 @@ static struct hw_pci kirkwood_pci __initdata = {
 
 static void __init add_pcie_port(int index, void __iomem *base)
 {
-	printk(KERN_INFO "Kirkwood PCIe port %d: ", index);
-
-	if (orion_pcie_link_up(base)) {
-		printk(KERN_INFO "link up\n");
-		pcie_port_map[num_pcie_ports++] = index;
-	} else
-		printk(KERN_INFO "link down, ignoring\n");
+	pcie_port_map[num_pcie_ports++] = index;
+	pr_info("Kirkwood PCIe port %d: link %s\n", index,
+		orion_pcie_link_up(base) ? "up" : "down");
 }
 
 void __init kirkwood_pcie_init(unsigned int portmask)
-- 
1.7.5.4

[PATCH] ARM: Kirkwood: Support basic hotplug for PCI-E

[Jason Cooper]

On Wed, Nov 21, 2012 at 11:25:28AM -0700, Jason Gunthorpe wrote:
> Unconditionally register the PCI-E bus, even if the link is currently
> down. 

How does this affect clock gating on boards without PCI-E devices?  Will
the SoC then power this unconditionally?

thx,

Jason.

> When the link is brought up the bus can be scanned through
> /sys/bus/pci/rescan or otherwise. Since the HW has no interrupt for
> link up, userspace will have to take care of the timing.
> 
> An earlier version of this was contingent on CONFIG_HOTPLUG, but
> that is being removed from the kernel.
> 
> This also fixes printing the link up/down message to be displayed
> on one line (structured logging broke this?)
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  arch/arm/mach-kirkwood/pcie.c |   10 +++-------
>  1 files changed, 3 insertions(+), 7 deletions(-)
> 
> All PCI-E ports are required to support hot plug at the link training
> level. Our systems support it electrically, and userspace sequences
> everything to work properly. But the PCI-E root port needs to be
> registered with the kernel to initiate a rescan via sysfs when things
> are ready.
> 
> diff --git a/arch/arm/mach-kirkwood/pcie.c b/arch/arm/mach-kirkwood/pcie.c
> index 59c97fe..80fd4f2 100644
> --- a/arch/arm/mach-kirkwood/pcie.c
> +++ b/arch/arm/mach-kirkwood/pcie.c
> @@ -244,13 +244,9 @@ static struct hw_pci kirkwood_pci __initdata = {
>  
>  static void __init add_pcie_port(int index, void __iomem *base)
>  {
> -	printk(KERN_INFO "Kirkwood PCIe port %d: ", index);
> -
> -	if (orion_pcie_link_up(base)) {
> -		printk(KERN_INFO "link up\n");
> -		pcie_port_map[num_pcie_ports++] = index;
> -	} else
> -		printk(KERN_INFO "link down, ignoring\n");
> +	pcie_port_map[num_pcie_ports++] = index;
> +	pr_info("Kirkwood PCIe port %d: link %s\n", index,
> +		orion_pcie_link_up(base) ? "up" : "down");
>  }
>  
>  void __init kirkwood_pcie_init(unsigned int portmask)
> -- 
> 1.7.5.4
> 

[PATCH] ARM: Kirkwood: Support basic hotplug for PCI-E

[Jason Gunthorpe]

On Wed, Nov 21, 2012 at 01:35:27PM -0500, Jason Cooper wrote:
> On Wed, Nov 21, 2012 at 11:25:28AM -0700, Jason Gunthorpe wrote:
> > Unconditionally register the PCI-E bus, even if the link is currently
> > down. 
> 
> How does this affect clock gating on boards without PCI-E devices?  Will
> the SoC then power this unconditionally?

Hmm, interesting question.. This will definitely hold the clock lock
on the PCI-E ports even if they are down at boot, and they will surely
consume more power than if they were switched off.

However, if a board has no possible PCI-E devices, then it shouldn't
be calling kirkwood_pcie_init, which will let things gate.

So the only case is boards that have pluggable PCI-E devices, where no
device is in the plug? That sounds like eval cards, is this a big
deal?

I guess another way to deal with this is to make the stuff in
kirkwood/pcie.c an actual platform driver and hot attach the entire
driver rather than using just using pci/rescan, but that is a fairly
extensive change.. And I worry that powering down the interface in the
interm will require re-initializing all the registers, which linux
doesn't have the code to do at all.

How about fixing this up to have a DT binding and make it a DT
controlled option - that seems like a simple middle ground..

What do you think?

Jason

[PATCH] ARM: Kirkwood: Support basic hotplug for PCI-E

[Jason Cooper]

On Wed, Nov 21, 2012 at 11:47:54AM -0700, Jason Gunthorpe wrote:
> On Wed, Nov 21, 2012 at 01:35:27PM -0500, Jason Cooper wrote:
> > On Wed, Nov 21, 2012 at 11:25:28AM -0700, Jason Gunthorpe wrote:
> > > Unconditionally register the PCI-E bus, even if the link is currently
> > > down. 
> > 
> > How does this affect clock gating on boards without PCI-E devices?  Will
> > the SoC then power this unconditionally?
> 
> Hmm, interesting question.. This will definitely hold the clock lock
> on the PCI-E ports even if they are down at boot, and they will surely
> consume more power than if they were switched off.
> 
> However, if a board has no possible PCI-E devices, then it shouldn't
> be calling kirkwood_pcie_init, which will let things gate.

Ok, that's the good short-term answer.

> I guess another way to deal with this is to make the stuff in
> kirkwood/pcie.c an actual platform driver and hot attach the entire
> driver rather than using just using pci/rescan, but that is a fairly
> extensive change.. And I worry that powering down the interface in the
> interm will require re-initializing all the registers, which linux
> doesn't have the code to do at all.

The goal is to remove everything from mach-kirkwood/, mach-dove/, etc.
which means pcie will eventually become a real driver under drivers/.
We just aren't there yet.

I'll pull this in as is.  Thanks for the patch!

thx,

Jason.

[PATCH] ARM: Kirkwood: Support basic hotplug for PCI-E

[Andrew Lunn]

On Wed, Nov 21, 2012 at 01:35:27PM -0500, Jason Cooper wrote:
> On Wed, Nov 21, 2012 at 11:25:28AM -0700, Jason Gunthorpe wrote:
> > Unconditionally register the PCI-E bus, even if the link is currently
> > down. 
> 
> How does this affect clock gating on boards without PCI-E devices?  Will
> the SoC then power this unconditionally?

Hi Jason and Jason

I think it does.

The output from /debug/clk will tell.

I think it may also cause problems with udev persistent rules. The IDs
will change for devices which are on the second controller and the
first controller was previously not registered. I've not idea if this
is just a theoretical problem, or a real problem.

   Andrew

> 
> thx,
> 
> Jason.
> 
> > When the link is brought up the bus can be scanned through
> > /sys/bus/pci/rescan or otherwise. Since the HW has no interrupt for
> > link up, userspace will have to take care of the timing.
> > 
> > An earlier version of this was contingent on CONFIG_HOTPLUG, but
> > that is being removed from the kernel.
> > 
> > This also fixes printing the link up/down message to be displayed
> > on one line (structured logging broke this?)
> > 
> > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > ---
> >  arch/arm/mach-kirkwood/pcie.c |   10 +++-------
> >  1 files changed, 3 insertions(+), 7 deletions(-)
> > 
> > All PCI-E ports are required to support hot plug at the link training
> > level. Our systems support it electrically, and userspace sequences
> > everything to work properly. But the PCI-E root port needs to be
> > registered with the kernel to initiate a rescan via sysfs when things
> > are ready.
> > 
> > diff --git a/arch/arm/mach-kirkwood/pcie.c b/arch/arm/mach-kirkwood/pcie.c
> > index 59c97fe..80fd4f2 100644
> > --- a/arch/arm/mach-kirkwood/pcie.c
> > +++ b/arch/arm/mach-kirkwood/pcie.c
> > @@ -244,13 +244,9 @@ static struct hw_pci kirkwood_pci __initdata = {
> >  
> >  static void __init add_pcie_port(int index, void __iomem *base)
> >  {
> > -	printk(KERN_INFO "Kirkwood PCIe port %d: ", index);
> > -
> > -	if (orion_pcie_link_up(base)) {
> > -		printk(KERN_INFO "link up\n");
> > -		pcie_port_map[num_pcie_ports++] = index;
> > -	} else
> > -		printk(KERN_INFO "link down, ignoring\n");
> > +	pcie_port_map[num_pcie_ports++] = index;
> > +	pr_info("Kirkwood PCIe port %d: link %s\n", index,
> > +		orion_pcie_link_up(base) ? "up" : "down");
> >  }
> >  
> >  void __init kirkwood_pcie_init(unsigned int portmask)
> > -- 
> > 1.7.5.4
> > 

[PATCH] ARM: Kirkwood: Support basic hotplug for PCI-E

[Jason Gunthorpe]

On Wed, Nov 21, 2012 at 08:02:30PM +0100, Andrew Lunn wrote:

> I think it may also cause problems with udev persistent rules. The IDs
> will change for devices which are on the second controller and the
> first controller was previously not registered. I've not idea if this
> is just a theoretical problem, or a real problem.

Most udev persistance rules I've seen are based on something more
permanent, like a MAC address or whatnot. The PCI-E bus number is
usually not used because it is historically not stable.

If a system is pluggable then the bus number already changes based on
what ports are plugged, so in a sense this patch makes things a bit
better by providing stable bus numbers.

When the PCI-E driver is integrated with DT it can get a busnr
resource range unique to each port and then the bus numbers will be
stable in almost all cases.

Jason

[PATCH] ARM: Kirkwood: Support basic hotplug for PCI-E

[Jason Cooper]

On Wed, Nov 21, 2012 at 11:25:28AM -0700, Jason Gunthorpe wrote:
> Unconditionally register the PCI-E bus, even if the link is currently
> down. When the link is brought up the bus can be scanned through
> /sys/bus/pci/rescan or otherwise. Since the HW has no interrupt for
> link up, userspace will have to take care of the timing.
> 
> An earlier version of this was contingent on CONFIG_HOTPLUG, but
> that is being removed from the kernel.
> 
> This also fixes printing the link up/down message to be displayed
> on one line (structured logging broke this?)
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  arch/arm/mach-kirkwood/pcie.c |   10 +++-------
>  1 files changed, 3 insertions(+), 7 deletions(-)

Applied to mvebu/drivers.

thx,

Jason.