| Message ID | 20121121182528.GA12715@obsidianresearch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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.
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 > >
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
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.
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)
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.