[PATCH 0/4] Marvell PCIe driver improvements

[PATCH 0/4] Marvell PCIe driver improvements

[Thomas Petazzoni]

Bjorn, Jason, Andrew, Gregory,

You'll find in this patch series 4 small patches that make various
improvements to the Marvell PCIe driver.

I'd like those improvements to be kept separated from the original
PCIe driver itself: while the PCIe driver has been around and reviewed
for a long time, those improvements are newer. And I clearly do not
want the PCIe driver to miss 3.11 because of any problem that could be
found in those additional improvements.

The main improvement being brought here is that the PCI-to-PCI bridge
logic is fixed/extended to properly support physical PCIe bridges that
are connected on a PCIe interface of a Marvell board. Without this
improvement, only the devices connected directly to the PCIe
interfaces of the board are properly enumerated. Any device that would
sit beyond a physical bridge is not visible.

The other improvements are more minor, and the patch description
should be sufficient to understand what's going on.

Those patches have been tested on both Armada 370/XP and Kirkwood.

Bjorn, with your Acked-by, could the Marvell maintainers include those
patches in their branch, merged through arm-soc? They already have the
Marvell PCIe driver itself, so I believe it makes sense to merge those
improvements through the same path.

Jason, those patches have been prepared on top of my marvell-pcie-v10
branch, I hope that's ok for you. If you want me to rebase them on
some other branch in which you have integrated the PCIe driver, don't
hesitate to tell me to do so. That said, since those patches are only
touching the driver itself and no other file, they should not cause
any conflict with other changes.

Thanks,

Thomas

Thomas Petazzoni (4):
  pci: mvebu: no longer fake the slot location of downstream devices
  pci: mvebu: allow the enumeration of devices beyond physical bridges
  pci: mvebu: emulate an empty capability list
  pci: mvebu: fix the emulation of the status register

 drivers/pci/host/pci-mvebu.c |   65 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 7 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] pci: mvebu: no longer fake the slot location of downstream devices

[Thomas Petazzoni]

By default, the Marvell hardware, for each PCIe interface, exhibits
the following devices:

 * On slot 0, a "Marvell Memory controller", identical on all PCIe
   interfaces, and which isn't useful when the Marvell SoC is the PCIe
   root complex (i.e, the normal case when we run Linux on the Marvell
   SoC).

 * On slot 1, the real PCIe card connected into the PCIe slot of the
   board.

So, what the Marvell PCIe driver was doing in its PCI-to-PCI bridge
emulation is that when the Linux PCI core was trying to access the
device in slot 0, we were in fact forwarding the configuration
transaction to the device in slot 1. For all other slots, we were
telling the Linux PCI core that there was no device connected.

However, new versions of bootloaders from Marvell change the default
PCIe configuration, and make the real device appear in slot 0, and the
"Marvell Memory controller" in slot 1.

Therefore, this commit modifies the Marvell PCIe driver to adjust the
PCIe hardware configuration to make sure that this behavior (real
device in slot 0, "Marvell Memory controller" in slot 1) is the one
we'll see regardless of what the bootloader has done. It allows to
remove the little hack that was forwarding configuration transactions
on slot 0 to slot 1, which is nice.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/pci-mvebu.c |   19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index ad1c46b..0bc21b0 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -51,6 +51,7 @@
 #define  PCIE_CTRL_X1_MODE		0x0001
 #define PCIE_STAT_OFF		0x1a04
 #define  PCIE_STAT_BUS                  0xff00
+#define  PCIE_STAT_DEV                  0x1f0000
 #define  PCIE_STAT_LINK_DOWN		BIT(0)
 #define PCIE_DEBUG_CTRL         0x1a60
 #define  PCIE_DEBUG_SOFT_RESET		BIT(20)
@@ -148,6 +149,16 @@ static void mvebu_pcie_set_local_bus_nr(struct mvebu_pcie_port *port, int nr)
 	writel(stat, port->base + PCIE_STAT_OFF);
 }
 
+static void mvebu_pcie_set_local_dev_nr(struct mvebu_pcie_port *port, int nr)
+{
+	u32 stat;
+
+	stat = readl(port->base + PCIE_STAT_OFF);
+	stat &= ~PCIE_STAT_DEV;
+	stat |= nr << 16;
+	writel(stat, port->base + PCIE_STAT_OFF);
+}
+
 /*
  * Setup PCIE BARs and Address Decode Wins:
  * BAR[0,2] -> disabled, BAR[1] -> covers all DRAM banks
@@ -572,8 +583,7 @@ static int mvebu_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 
 	/* Access the real PCIe interface */
 	spin_lock_irqsave(&port->conf_lock, flags);
-	ret = mvebu_pcie_hw_wr_conf(port, bus,
-				    PCI_DEVFN(1, PCI_FUNC(devfn)),
+	ret = mvebu_pcie_hw_wr_conf(port, bus, devfn,
 				    where, size, val);
 	spin_unlock_irqrestore(&port->conf_lock, flags);
 
@@ -606,8 +616,7 @@ static int mvebu_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 
 	/* Access the real PCIe interface */
 	spin_lock_irqsave(&port->conf_lock, flags);
-	ret = mvebu_pcie_hw_rd_conf(port, bus,
-				    PCI_DEVFN(1, PCI_FUNC(devfn)),
+	ret = mvebu_pcie_hw_rd_conf(port, bus, devfn,
 				    where, size, val);
 	spin_unlock_irqrestore(&port->conf_lock, flags);
 
@@ -817,6 +826,8 @@ static int __init mvebu_pcie_probe(struct platform_device *pdev)
 			continue;
 		}
 
+		mvebu_pcie_set_local_dev_nr(port, 1);
+
 		if (mvebu_pcie_link_up(port)) {
 			port->haslink = 1;
 			dev_info(&pdev->dev, "PCIe%d.%d: link up\n",
-- 
1.7.9.5

[PATCH 2/4] pci: mvebu: allow the enumeration of devices beyond physical bridges

[Thomas Petazzoni]

Until now, the Marvell PCIe driver was only allowing the enumeration
of the devices in the secondary bus of the emulated PCI-to-PCI
bridge. This works fine when a PCIe device is directly connected into
a PCIe slot of the Marvell board.

However, when the device connected in the PCIe slot is a physical PCIe
bridge, beyond which a real PCIe device is connected, it no longer
worked, as the driver was preventing the Linux PCI core from seeing
such devices.

This commit fixes that by ensuring that configuration transactions on
subordinate busses are properly forwarded on the right PCIe interface.

Thanks to this patch, a PCIe card beyond a PCIe bridge, itself beyond
the emulated PCI-to-PCI bridge is properly detected, with the
following layout:

-[0000:00]-+-01.0-[01]----00.0
           +-09.0-[02-07]----00.0-[03-07]--+-01.0-[04]--
           |                               +-05.0-[05]--
           |                               +-07.0-[06]--
           |                               \-09.0-[07]----00.0
           \-0a.0-[08]----00.0

Where the PCIe interface that sits beyond the emulated PCI-to-PCI
bridge at 09.0 allows to access the secondary bus 02, on which there
is a PCIe bridge that allows to access the 3 to 7 busses, that are
subordinates to this bridge. And on one of this bus (bus 7), there is
one real PCIe device connected.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/pci-mvebu.c |   31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 0bc21b0..c21ca84 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -554,7 +554,8 @@ mvebu_pcie_find_port(struct mvebu_pcie *pcie, struct pci_bus *bus,
 		if (bus->number == 0 && port->devfn == devfn)
 			return port;
 		if (bus->number != 0 &&
-		    port->bridge.secondary_bus == bus->number)
+		    bus->number >= port->bridge.secondary_bus &&
+		    bus->number <= port->bridge.subordinate_bus)
 			return port;
 	}
 
@@ -578,7 +579,18 @@ static int mvebu_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	if (bus->number == 0)
 		return mvebu_sw_pci_bridge_write(port, where, size, val);
 
-	if (!port->haslink || PCI_SLOT(devfn) != 0)
+	if (!port->haslink)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	/*
+	 * On the secondary bus, we don't want to expose any other
+	 * device that the device physically connected in the PCIe
+	 * slot, visible in slot 0. In slot 1, there's a special
+	 * Marvell device that only makes sense when the Armada is
+	 * used as a PCIe endpoint.
+	 */
+	if (bus->number == port->bridge.secondary_bus &&
+	    PCI_SLOT(devfn) != 0)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	/* Access the real PCIe interface */
@@ -609,7 +621,20 @@ static int mvebu_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 	if (bus->number == 0)
 		return mvebu_sw_pci_bridge_read(port, where, size, val);
 
-	if (!port->haslink || PCI_SLOT(devfn) != 0) {
+	if (!port->haslink) {
+		*val = 0xffffffff;
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	}
+
+	/*
+	 * On the secondary bus, we don't want to expose any other
+	 * device that the device physically connected in the PCIe
+	 * slot, visible in slot 0. In slot 1, there's a special
+	 * Marvell device that only makes sense when the Armada is
+	 * used as a PCIe endpoint.
+	 */
+	if (bus->number == port->bridge.secondary_bus &&
+	    PCI_SLOT(devfn) != 0) {
 		*val = 0xffffffff;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
-- 
1.7.9.5

Re: [PATCH 2/4] pci: mvebu: allow the enumeration of devices beyond physical bridges

[Bjorn Helgaas]

On Wed, May 22, 2013 at 7:12 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Until now, the Marvell PCIe driver was only allowing the enumeration
> of the devices in the secondary bus of the emulated PCI-to-PCI
> bridge. This works fine when a PCIe device is directly connected into
> a PCIe slot of the Marvell board.
>
> However, when the device connected in the PCIe slot is a physical PCIe
> bridge, beyond which a real PCIe device is connected, it no longer
> worked, as the driver was preventing the Linux PCI core from seeing
> such devices.
>
> This commit fixes that by ensuring that configuration transactions on
> subordinate busses are properly forwarded on the right PCIe interface.
>
> Thanks to this patch, a PCIe card beyond a PCIe bridge, itself beyond
> the emulated PCI-to-PCI bridge is properly detected, with the
> following layout:
>
> -[0000:00]-+-01.0-[01]----00.0
>            +-09.0-[02-07]----00.0-[03-07]--+-01.0-[04]--
>            |                               +-05.0-[05]--
>            |                               +-07.0-[06]--
>            |                               \-09.0-[07]----00.0
>            \-0a.0-[08]----00.0
>
> Where the PCIe interface that sits beyond the emulated PCI-to-PCI
> bridge at 09.0 allows to access the secondary bus 02, on which there
> is a PCIe bridge that allows to access the 3 to 7 busses, that are
> subordinates to this bridge. And on one of this bus (bus 7), there is
> one real PCIe device connected.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/pci/host/pci-mvebu.c |   31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index 0bc21b0..c21ca84 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -554,7 +554,8 @@ mvebu_pcie_find_port(struct mvebu_pcie *pcie, struct pci_bus *bus,
>                 if (bus->number == 0 && port->devfn == devfn)
>                         return port;
>                 if (bus->number != 0 &&
> -                   port->bridge.secondary_bus == bus->number)
> +                   bus->number >= port->bridge.secondary_bus &&
> +                   bus->number <= port->bridge.subordinate_bus)
>                         return port;
>         }
>
> @@ -578,7 +579,18 @@ static int mvebu_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
>         if (bus->number == 0)
>                 return mvebu_sw_pci_bridge_write(port, where, size, val);
>
> -       if (!port->haslink || PCI_SLOT(devfn) != 0)
> +       if (!port->haslink)
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +       /*
> +        * On the secondary bus, we don't want to expose any other
> +        * device that the device physically connected in the PCIe

s/that/than/ (also below)

> +        * slot, visible in slot 0. In slot 1, there's a special
> +        * Marvell device that only makes sense when the Armada is
> +        * used as a PCIe endpoint.
> +        */
> +       if (bus->number == port->bridge.secondary_bus &&
> +           PCI_SLOT(devfn) != 0)
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>
>         /* Access the real PCIe interface */
> @@ -609,7 +621,20 @@ static int mvebu_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
>         if (bus->number == 0)
>                 return mvebu_sw_pci_bridge_read(port, where, size, val);
>
> -       if (!port->haslink || PCI_SLOT(devfn) != 0) {
> +       if (!port->haslink) {
> +               *val = 0xffffffff;
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +       }
> +
> +       /*
> +        * On the secondary bus, we don't want to expose any other
> +        * device that the device physically connected in the PCIe
> +        * slot, visible in slot 0. In slot 1, there's a special
> +        * Marvell device that only makes sense when the Armada is
> +        * used as a PCIe endpoint.
> +        */
> +       if (bus->number == port->bridge.secondary_bus &&
> +           PCI_SLOT(devfn) != 0) {
>                 *val = 0xffffffff;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
> --
> 1.7.9.5
>
> --
> 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

[PATCH 3/4] pci: mvebu: emulate an empty capability list

[Thomas Petazzoni]

Currently, our PCI-to-PCI bridge emulation doesn't emulate a proper
capability list, which leads 'lspci -v' to show:

  Capabilities: [fc] <chain broken>

In order to fix this, this commit improve the PCI-to-PCI bridge
emulation to emulate an empty capability list. It might be later
extended to expose things like the PCI Express Capability header, but
an empty capability list is sufficient for now.

lspci -v now shows the much nicer:

  Capabilities: [40] #00 [0000]

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/pci-mvebu.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index c21ca84..c887598 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -440,6 +440,16 @@ static int mvebu_sw_pci_bridge_read(struct mvebu_pcie_port *port,
 		*value = 0;
 		break;
 
+	case PCI_CAPABILITY_LIST:
+		/* Offset of the capability list */
+		*value = 0x40;
+		break;
+
+	case 0x40:
+		/* We have no element in our capability list */
+		*value = 0;
+		break;
+
 	default:
 		*value = 0xffffffff;
 		return PCIBIOS_BAD_REGISTER_NUMBER;
-- 
1.7.9.5

Re: [PATCH 3/4] pci: mvebu: emulate an empty capability list

[Bjorn Helgaas]

On Wed, May 22, 2013 at 7:12 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Currently, our PCI-to-PCI bridge emulation doesn't emulate a proper
> capability list, which leads 'lspci -v' to show:
>
>   Capabilities: [fc] <chain broken>
>
> In order to fix this, this commit improve the PCI-to-PCI bridge

s/improve/improves/

> emulation to emulate an empty capability list. It might be later
> extended to expose things like the PCI Express Capability header, but
> an empty capability list is sufficient for now.
>
> lspci -v now shows the much nicer:
>
>   Capabilities: [40] #00 [0000]

It'd be even better if you could make the Capabilities List bit in the
Device Status register be zero.  Then lspci wouldn't even try to read
the Capabilities Pointer at 0x34, and you wouldn't have to deal with
reads of 0x40.

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/pci/host/pci-mvebu.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index c21ca84..c887598 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -440,6 +440,16 @@ static int mvebu_sw_pci_bridge_read(struct mvebu_pcie_port *port,
>                 *value = 0;
>                 break;
>
> +       case PCI_CAPABILITY_LIST:
> +               /* Offset of the capability list */
> +               *value = 0x40;
> +               break;
> +
> +       case 0x40:
> +               /* We have no element in our capability list */
> +               *value = 0;
> +               break;
> +
>         default:
>                 *value = 0xffffffff;
>                 return PCIBIOS_BAD_REGISTER_NUMBER;
> --
> 1.7.9.5
>
> --
> 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

Re: [PATCH 3/4] pci: mvebu: emulate an empty capability list

[Thomas Petazzoni]

Dear Bjorn Helgaas,

On Wed, 22 May 2013 08:39:03 -0600, Bjorn Helgaas wrote:

> > emulation to emulate an empty capability list. It might be later
> > extended to expose things like the PCI Express Capability header, but
> > an empty capability list is sufficient for now.
> >
> > lspci -v now shows the much nicer:
> >
> >   Capabilities: [40] #00 [0000]
> 
> It'd be even better if you could make the Capabilities List bit in the
> Device Status register be zero.  Then lspci wouldn't even try to read
> the Capabilities Pointer at 0x34, and you wouldn't have to deal with
> reads of 0x40.

The Device Status register is emulated with an initial value of zero.
Then, whenever the Linux PCI core writes into it, those writes are
preserved. So to me, it looks like the Linux PCI core might be setting
this Capabilities List bit, and later re-reads it and finds it to be
set to 1.

Should this bit be emulated as a read-only bit (it's not made explicit
for this particular bit in the PCI-to-PCI bridge specification that I
have in front of me) ? Or even the entire Device Status register ?

Thanks for your comments!

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 4/4] pci: mvebu: fix the emulation of the status register

[Thomas Petazzoni]

In a PCI configuration header, the 'devsel' bits of the status
register are read-only, and indicate the timing of the secondary
interface. Currently, we implement them as read/write, so when the
Linux PCI core writes all 1's to this register, it gets 11b as the
'devsel' value, which is reserved.

This commit fixes the PCI-to-PCI bridge emulation of the Marvell PCIe
driver to ensure those bits remain set to 00b, which indicate a fast
devsel decoding.

This allows to fix the lspci -v output from:

  Flags: bus master, 66MHz, user-definable features, ?? devsel, latency 0

to:

  Flags: bus master, 66MHz, user-definable features, fast devsel, latency 0

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/pci-mvebu.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index c887598..d730bf4 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -490,6 +490,11 @@ static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port,
 	case PCI_COMMAND:
 		bridge->command = value & 0xffff;
 		bridge->status = value >> 16;
+		/*
+		 * The devsel bits are read-only, and we want to keep
+		 * them set to 0
+		 */
+		bridge->status &= ~PCI_STATUS_DEVSEL_MASK;
 		break;
 
 	case PCI_BASE_ADDRESS_0 ... PCI_BASE_ADDRESS_1:
-- 
1.7.9.5

Re: [PATCH 4/4] pci: mvebu: fix the emulation of the status register

[Bjorn Helgaas]

On Wed, May 22, 2013 at 7:12 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> In a PCI configuration header, the 'devsel' bits of the status
> register are read-only, and indicate the timing of the secondary
> interface. Currently, we implement them as read/write, so when the
> Linux PCI core writes all 1's to this register, it gets 11b as the
> 'devsel' value, which is reserved.

I suppose the problem is that pci_scan_bridge() writes 0xffff to the
PCI_STATUS register?

PCI_STATUS is documented as containing either RO or RW1C
(write-1-to-clear status) bits, so it makes sense to write all ones to
clear all status bits.  But there are other RO bits that I think you
should mask out, too: Interrupt Status, Capabilities List, etc.

Maybe if you fix this, it will also fix the Capability List issue you
saw in patch 3/4.

> This commit fixes the PCI-to-PCI bridge emulation of the Marvell PCIe
> driver to ensure those bits remain set to 00b, which indicate a fast
> devsel decoding.
>
> This allows to fix the lspci -v output from:
>
>   Flags: bus master, 66MHz, user-definable features, ?? devsel, latency 0
>
> to:
>
>   Flags: bus master, 66MHz, user-definable features, fast devsel, latency 0
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/pci/host/pci-mvebu.c |    5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index c887598..d730bf4 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -490,6 +490,11 @@ static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port,
>         case PCI_COMMAND:
>                 bridge->command = value & 0xffff;
>                 bridge->status = value >> 16;
> +               /*
> +                * The devsel bits are read-only, and we want to keep
> +                * them set to 0
> +                */
> +               bridge->status &= ~PCI_STATUS_DEVSEL_MASK;
>                 break;
>
>         case PCI_BASE_ADDRESS_0 ... PCI_BASE_ADDRESS_1:
> --
> 1.7.9.5
>
> --
> 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

Re: [PATCH 0/4] Marvell PCIe driver improvements

[Jason Cooper]

Thomas,

On Wed, May 22, 2013 at 03:12:34PM +0200, Thomas Petazzoni wrote:
> Bjorn, Jason, Andrew, Gregory,
> 
> You'll find in this patch series 4 small patches that make various
> improvements to the Marvell PCIe driver.
> 
> I'd like those improvements to be kept separated from the original
> PCIe driver itself: while the PCIe driver has been around and reviewed
> for a long time, those improvements are newer. And I clearly do not
> want the PCIe driver to miss 3.11 because of any problem that could be
> found in those additional improvements.

Agreed.

For Bjorn and others:

The original patch series Thomas is referring to missed the merge window
for v3.10 due to a build failure.  It is currently sitting in our mvebu
repo.  The first three patches, "of/pci ..." are in mvebu/of_pci.  The
rest of the series is on top of that in the branch mvebu/pcie.

I've sent pull requests for both branches to arm-soc.  There are two
other arm SoC pcie efforts which are depending on mvebu/of_pci.  Hence
why I did a separate branch.

Both branches are also merged into mvebu/for-next, and have survived
linux-next for three days or so.  But, it's early, so we'll see how it
goes. :-)

> Bjorn, with your Acked-by, could the Marvell maintainers include those
> patches in their branch, merged through arm-soc? They already have the
> Marvell PCIe driver itself, so I believe it makes sense to merge those
> improvements through the same path.

I agree, please see above regarding arm-soc dependencies.

> Jason, those patches have been prepared on top of my marvell-pcie-v10
> branch, I hope that's ok for you. If you want me to rebase them on
> some other branch in which you have integrated the PCIe driver, don't
> hesitate to tell me to do so. That said, since those patches are only
> touching the driver itself and no other file, they should not cause
> any conflict with other changes.

Should be fine, I'll let you know.

thx,

Jason.

Re: [PATCH 0/4] Marvell PCIe driver improvements

[Gregory CLEMENT]

On 05/22/2013 03:43 PM, Jason Cooper wrote:
> Thomas,
> 
> On Wed, May 22, 2013 at 03:12:34PM +0200, Thomas Petazzoni wrote:
>> Bjorn, Jason, Andrew, Gregory,
>>
>> You'll find in this patch series 4 small patches that make various
>> improvements to the Marvell PCIe driver.
>>
>> I'd like those improvements to be kept separated from the original
>> PCIe driver itself: while the PCIe driver has been around and reviewed
>> for a long time, those improvements are newer. And I clearly do not
>> want the PCIe driver to miss 3.11 because of any problem that could be
>> found in those additional improvements.
> 
> Agreed.

All the changes are located in one single file drivers/pci/host/pci-mvebu.c.
So as soon as they will be acked by Bjron, I think we can make a PR for them.
As nothing else depend of it, even if for a reason or another it is dropped,
there will be no reason to drop anything else.

> 
> For Bjorn and others:
> 
> The original patch series Thomas is referring to missed the merge window
> for v3.10 due to a build failure.  It is currently sitting in our mvebu
> repo.  The first three patches, "of/pci ..." are in mvebu/of_pci.  The
> rest of the series is on top of that in the branch mvebu/pcie.
> 
> I've sent pull requests for both branches to arm-soc.  There are two
> other arm SoC pcie efforts which are depending on mvebu/of_pci.  Hence
> why I did a separate branch.
> 
> Both branches are also merged into mvebu/for-next, and have survived
> linux-next for three days or so.  But, it's early, so we'll see how it
> goes. :-)
> 
>> Bjorn, with your Acked-by, could the Marvell maintainers include those
>> patches in their branch, merged through arm-soc? They already have the
>> Marvell PCIe driver itself, so I believe it makes sense to merge those
>> improvements through the same path.
> 
> I agree, please see above regarding arm-soc dependencies.
> 
>> Jason, those patches have been prepared on top of my marvell-pcie-v10
>> branch, I hope that's ok for you. If you want me to rebase them on
>> some other branch in which you have integrated the PCIe driver, don't
>> hesitate to tell me to do so. That said, since those patches are only
>> touching the driver itself and no other file, they should not cause
>> any conflict with other changes.
> 
> Should be fine, I'll let you know.
> 
> thx,
> 
> Jason.
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 0/4] Marvell PCIe driver improvements

[Jason Cooper]

On Wed, May 22, 2013 at 05:09:46PM +0200, Gregory CLEMENT wrote:
> On 05/22/2013 03:43 PM, Jason Cooper wrote:
> > Thomas,
> > 
> > On Wed, May 22, 2013 at 03:12:34PM +0200, Thomas Petazzoni wrote:
> >> Bjorn, Jason, Andrew, Gregory,
> >>
> >> You'll find in this patch series 4 small patches that make various
> >> improvements to the Marvell PCIe driver.
> >>
> >> I'd like those improvements to be kept separated from the original
> >> PCIe driver itself: while the PCIe driver has been around and reviewed
> >> for a long time, those improvements are newer. And I clearly do not
> >> want the PCIe driver to miss 3.11 because of any problem that could be
> >> found in those additional improvements.
> > 
> > Agreed.
> 
> All the changes are located in one single file drivers/pci/host/pci-mvebu.c.
> So as soon as they will be acked by Bjron, I think we can make a PR for them.
> As nothing else depend of it, even if for a reason or another it is dropped,
> there will be no reason to drop anything else.

Right, which is why we'll put it in a separate branch.  Say,
mvebu/pcie_bridge.  This way if /pcie_bridge has to be dropped (say an
API that it used is changed), /pcie would remain.  Which is the main
concern.

Had I known enough last time around, I would have done the /of_pci |
/pcie split.  /pcie would have been dropped, and /of_pci would have made
it in.  As well as LinusW's work which depends upon it (and now Arnd
might have something using it as well).

thx,

Jason.

Re: [PATCH 0/4] Marvell PCIe driver improvements

[Willy Tarreau]

Hi Thomas,

On Wed, May 22, 2013 at 03:12:34PM +0200, Thomas Petazzoni wrote:
> Bjorn, Jason, Andrew, Gregory,
> 
> You'll find in this patch series 4 small patches that make various
> improvements to the Marvell PCIe driver.
(...)

FWIW, these patches applied on top of your pcie-v10 tree have made
my mini-pcie dual-nic appear for the first time on the mirabox. Till
now I never knew if it was a hardware or software issue since the NIC
does not appear in u-boot nor lspci, whatever the kernel versions,
including the original Marvell one's. The NIC has two functions (one
per controller) and no bridge, so it may be a side effect of your
improvements (or maybe you fixed a bug).

Anyway for me it's a great improvement !

I've not tried it on the OpenBlocks AX3 yet (it was not detected
there previously). But I have good hopes.

For reference, the NIC is Jetway's ADMPEIDLA (intel i350-AM2).

Best regards,
Willy

Re: [PATCH 0/4] Marvell PCIe driver improvements

[Thomas Petazzoni]

Dear Willy Tarreau,

On Thu, 23 May 2013 02:13:53 +0200, Willy Tarreau wrote:

> > You'll find in this patch series 4 small patches that make various
> > improvements to the Marvell PCIe driver.
> (...)
> 
> FWIW, these patches applied on top of your pcie-v10 tree have made
> my mini-pcie dual-nic appear for the first time on the mirabox. Till
> now I never knew if it was a hardware or software issue since the NIC
> does not appear in u-boot nor lspci, whatever the kernel versions,
> including the original Marvell one's. The NIC has two functions (one
> per controller) and no bridge, so it may be a side effect of your
> improvements (or maybe you fixed a bug).

Hum, interesting. Can you show the output of lspci -v and lspci -t
(which a pciutils version of lspci, not Busybox one) ? I'm interested
in seeing the layout of the PCIe bus with such a device, to get a
better understanding.

> Anyway for me it's a great improvement !
> 
> I've not tried it on the OpenBlocks AX3 yet (it was not detected
> there previously). But I have good hopes.
> 
> For reference, the NIC is Jetway's ADMPEIDLA (intel i350-AM2).

Should I be taking this as a formal Tested-by from you? :-)

Thanks!

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 0/4] Marvell PCIe driver improvements

[Willy Tarreau]

On Thu, May 23, 2013 at 08:35:09AM +0200, Thomas Petazzoni wrote:
> > FWIW, these patches applied on top of your pcie-v10 tree have made
> > my mini-pcie dual-nic appear for the first time on the mirabox. Till
> > now I never knew if it was a hardware or software issue since the NIC
> > does not appear in u-boot nor lspci, whatever the kernel versions,
> > including the original Marvell one's. The NIC has two functions (one
> > per controller) and no bridge, so it may be a side effect of your
> > improvements (or maybe you fixed a bug).
> 
> Hum, interesting. Can you show the output of lspci -v and lspci -t
> (which a pciutils version of lspci, not Busybox one) ? I'm interested
> in seeing the layout of the PCIe bus with such a device, to get a
> better understanding.

(send privately in another thread).

> > Anyway for me it's a great improvement !
> > 
> > I've not tried it on the OpenBlocks AX3 yet (it was not detected
> > there previously). But I have good hopes.
> > 
> > For reference, the NIC is Jetway's ADMPEIDLA (intel i350-AM2).
> 
> Should I be taking this as a formal Tested-by from you? :-)

Yes, obviously, feel free to add this one :

  Tested-by: Willy Tarreau <w@1wt.eu>

Willy

Re: [PATCH 0/4] Marvell PCIe driver improvements

[Jason Gunthorpe]

On Thu, May 23, 2013 at 02:13:53AM +0200, Willy Tarreau wrote:
> Hi Thomas,
> 
> On Wed, May 22, 2013 at 03:12:34PM +0200, Thomas Petazzoni wrote:
> > Bjorn, Jason, Andrew, Gregory,
> > 
> > You'll find in this patch series 4 small patches that make various
> > improvements to the Marvell PCIe driver.
> (...)
> 
> FWIW, these patches applied on top of your pcie-v10 tree have made
> my mini-pcie dual-nic appear for the first time on the mirabox. Till
> now I never knew if it was a hardware or software issue since the NIC
> does not appear in u-boot nor lspci, whatever the kernel versions,
> including the original Marvell one's. The NIC has two functions (one
> per controller) and no bridge, so it may be a side effect of your
> improvements (or maybe you fixed a bug).

There was some talk earlier that some buggy chips required their
device number to be 0. I'm guessing Thomas's change to ensure that
always happens by moving the device number of the internal device is
the root fix here?

Regards,
Jason

Re: [PATCH 0/4] Marvell PCIe driver improvements

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Thu, 23 May 2013 10:46:54 -0600, Jason Gunthorpe wrote:

> > FWIW, these patches applied on top of your pcie-v10 tree have made
> > my mini-pcie dual-nic appear for the first time on the mirabox. Till
> > now I never knew if it was a hardware or software issue since the NIC
> > does not appear in u-boot nor lspci, whatever the kernel versions,
> > including the original Marvell one's. The NIC has two functions (one
> > per controller) and no bridge, so it may be a side effect of your
> > improvements (or maybe you fixed a bug).
> 
> There was some talk earlier that some buggy chips required their
> device number to be 0. I'm guessing Thomas's change to ensure that
> always happens by moving the device number of the internal device is
> the root fix here?

You're correct. Off-list, I've sent to Willy a patch that applies on
top of those additional "bridge-related" fixes, but that reverts back
the slot number of the real PCIe device to be 1 instead of 0. And in
this case, the PCIe device that Willy is using is not detected.

So indeed, there are some PCIe devices who really want to receive
"configuration transactions" as slot 0.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com