[PATCH v2 0/7] PCI: aardvark: improve compatibility with PCI devices

[PATCH v2 0/7] PCI: aardvark: improve compatibility with PCI devices

[Thomas Petazzoni]

Hello,

This patch series brings a number of fixes to the pci-aardvark driver
that allows a much larger number of PCIe devices to be used.

It fixes kernel bug
https://bugzilla.kernel.org/show_bug.cgi?id=196339, and has been
tested with an IGB NIC and a Marvell SATA PCIe card, which were not
working previously.

They should be taken for the 4.14 cycle, as they are all bug fixes.

Changes since v1:
 - Rebased on top of v4.14-rc2, in order to resolve a minor conflict
   with another patch merged in the mean time.

Thanks a lot!

Thomas

Evan Wang (1):
  PCI: aardvark: fix PCIe max read request size setting

Thomas Petazzoni (1):
  PCI: aardvark: define IRQ related hooks in pci_host_bridge

Victor Gu (5):
  PCI: aardvark: fix logic in PCI configuration read/write functions
  PCI: aardvark: set PIO_ADDR_LS correctly in advk_pcie_rd_conf()
  PCI: aardvark: set host and device to the same MAX payload size
  PCI: aardvark: use isr1 instead of isr0 interrupt in legacy irq mode
  PCI: aardvark: disable LOS state by default

 drivers/pci/host/pci-aardvark.c | 116 +++++++++++++++++++++++++++++++---------
 1 file changed, 92 insertions(+), 24 deletions(-)

-- 
2.13.5

[PATCH v2 1/7] PCI: aardvark: fix logic in PCI configuration read/write functions

[Thomas Petazzoni]

From: Victor Gu <xigu@marvell.com>

The PCI configuration space read/write functions were special casing
the situation where PCI_SLOT(devfn) != 0, and returned
PCIBIOS_DEVICE_NOT_FOUND in this case.

However, will this is what is intended for the root bus, it is not
intended for the child busses, as it prevents discovering devices with
PCI_SLOT(x) != 0. Therefore, we return PCIBIOS_DEVICE_NOT_FOUND only
if we're on the root bus.

Fixes: 8c39d710363c1 ("PCI: aardvark: Add Aardvark PCI host controller driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Victor Gu <xigu@marvell.com>
Reviewed-by: Wilson Ding <dingwei@marvell.com>
Reviewed-by: Nadav Haklai <nadavh@marvell.com>
[Thomas: tweak commit log.]
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/pci-aardvark.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
index 89f4e3d072d7..da2881ba7737 100644
--- a/drivers/pci/host/pci-aardvark.c
+++ b/drivers/pci/host/pci-aardvark.c
@@ -440,7 +440,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 	u32 reg;
 	int ret;
 
-	if (PCI_SLOT(devfn) != 0) {
+	if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0)) {
 		*val = 0xffffffff;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
@@ -494,7 +494,7 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	int offset;
 	int ret;
 
-	if (PCI_SLOT(devfn) != 0)
+	if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0))
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	if (where % size)
-- 
2.13.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/7] PCI: aardvark: fix logic in PCI configuration read/write functions

[Bjorn Helgaas]

On Thu, Sep 28, 2017 at 02:58:32PM +0200, Thomas Petazzoni wrote:
> From: Victor Gu <xigu@marvell.com>
> 
> The PCI configuration space read/write functions were special casing
> the situation where PCI_SLOT(devfn) != 0, and returned
> PCIBIOS_DEVICE_NOT_FOUND in this case.
> 
> However, will this is what is intended for the root bus, it is not
> intended for the child busses, as it prevents discovering devices with
> PCI_SLOT(x) != 0. Therefore, we return PCIBIOS_DEVICE_NOT_FOUND only
> if we're on the root bus.
> 
> Fixes: 8c39d710363c1 ("PCI: aardvark: Add Aardvark PCI host controller driver")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Victor Gu <xigu@marvell.com>
> Reviewed-by: Wilson Ding <dingwei@marvell.com>
> Reviewed-by: Nadav Haklai <nadavh@marvell.com>
> [Thomas: tweak commit log.]
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/pci/host/pci-aardvark.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
> index 89f4e3d072d7..da2881ba7737 100644
> --- a/drivers/pci/host/pci-aardvark.c
> +++ b/drivers/pci/host/pci-aardvark.c
> @@ -440,7 +440,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>  	u32 reg;
>  	int ret;
>  
> -	if (PCI_SLOT(devfn) != 0) {
> +	if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0)) {

I'm fine with this, but please take a look at these:

  8e7ca8ca5fd8 PCI: xilinx: Relax device number checking to allow SR-IOV
  e18934b5e9c7 PCI: designware: Relax device number checking to allow SR-IOV
  d99e30b7936a PCI: altera: Relax device number checking to allow SR-IOV

and make sure that reasoning doesn't apply here, too.

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8e7ca8ca5fd8
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e18934b5e9c7
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d99e30b7936a

>  		*val = 0xffffffff;
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  	}
> @@ -494,7 +494,7 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
>  	int offset;
>  	int ret;
>  
> -	if (PCI_SLOT(devfn) != 0)
> +	if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0))
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  
>  	if (where % size)
> -- 
> 2.13.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/7] PCI: aardvark: fix logic in PCI configuration read/write functions

[Thomas Petazzoni]

Hello Bjorn,

Again, reviving this very old thread :-)

On Thu, 5 Oct 2017 12:23:30 -0500, Bjorn Helgaas wrote:

> > -	if (PCI_SLOT(devfn) != 0) {
> > +	if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0)) {  
> 
> I'm fine with this, but please take a look at these:
> 
>   8e7ca8ca5fd8 PCI: xilinx: Relax device number checking to allow SR-IOV
>   e18934b5e9c7 PCI: designware: Relax device number checking to allow SR-IOV
>   d99e30b7936a PCI: altera: Relax device number checking to allow SR-IOV
> 
> and make sure that reasoning doesn't apply here, too.
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8e7ca8ca5fd8
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e18934b5e9c7
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d99e30b7936a

The original code for xilinx/designware/altera was doing:

 	if (bus->number == port->root_busno && devfn > 0)
 		return false;
 
	if (bus->primary == port->root_busno && devfn > 0)
		return false;

I.e, it was checking both if bus->number *and* bus->primary were equal
to port->root_busno.

The commit you points removed the check on bus->primary, keeping the
check on bus->number.

Your patch for the Aadvark driver only adds a check on bus->number, i.e
exactly what the xilinx/designware/altera code is still doing today:

Altera:

        /* access only one slot on each root port */
        if (bus->number == pcie->root_bus_nr && dev > 0)
                return false;

Designware:

        /* access only one slot on each root port */
        if (bus->number == pp->root_bus_nr && dev > 0)
                return 0;

Xilinx:

        /* Only one device down on each root port */
        if (bus->number == port->root_busno && devfn > 0)
                return false;

Aardvark (with our patch):

        if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0)) {
                *val = 0xffffffff;
                return PCIBIOS_DEVICE_NOT_FOUND;
        }

So we're doing exactly the same thing.

Do you agree ?

Best regards,

Thomas Petazzoni
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 1/7] PCI: aardvark: fix logic in PCI configuration read/write functions

[Bjorn Helgaas]

[+cc Lorenzo, since he takes care of this now]

On Tue, Jan 09, 2018 at 05:49:18PM +0100, Thomas Petazzoni wrote:
> Hello Bjorn,
> 
> Again, reviving this very old thread :-)
> 
> On Thu, 5 Oct 2017 12:23:30 -0500, Bjorn Helgaas wrote:
> 
> > > -	if (PCI_SLOT(devfn) != 0) {
> > > +	if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0)) {  
> > 
> > I'm fine with this, but please take a look at these:
> > 
> >   8e7ca8ca5fd8 PCI: xilinx: Relax device number checking to allow SR-IOV
> >   e18934b5e9c7 PCI: designware: Relax device number checking to allow SR-IOV
> >   d99e30b7936a PCI: altera: Relax device number checking to allow SR-IOV
> > 
> > and make sure that reasoning doesn't apply here, too.
> > 
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8e7ca8ca5fd8
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e18934b5e9c7
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d99e30b7936a
> 
> The original code for xilinx/designware/altera was doing:
> 
>  	if (bus->number == port->root_busno && devfn > 0)
>  		return false;
>  
> 	if (bus->primary == port->root_busno && devfn > 0)
> 		return false;
> 
> I.e, it was checking both if bus->number *and* bus->primary were equal
> to port->root_busno.
> 
> The commit you points removed the check on bus->primary, keeping the
> check on bus->number.
> 
> Your patch for the Aadvark driver only adds a check on bus->number, i.e
> exactly what the xilinx/designware/altera code is still doing today:

This is a long time ago and I could have forgotten, but I don't think
this is *my* patch, is it?  

> Altera:
> 
>         /* access only one slot on each root port */
>         if (bus->number == pcie->root_bus_nr && dev > 0)
>                 return false;
> 
> Designware:
> 
>         /* access only one slot on each root port */
>         if (bus->number == pp->root_bus_nr && dev > 0)
>                 return 0;
> 
> Xilinx:
> 
>         /* Only one device down on each root port */
>         if (bus->number == port->root_busno && devfn > 0)
>                 return false;
> 
> Aardvark (with our patch):
> 
>         if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0)) {
>                 *val = 0xffffffff;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
> 
> So we're doing exactly the same thing.
> 
> Do you agree ?

I do agree.  I can't remember what I was thinking when I first
responded.

I *would* suggest making an advk_pcie_valid_device() so your structure
matches the other drivers.  I know it seems trivial when you're mostly
looking at one driver, but it really helps those who pay attention to
all of them.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/7] PCI: aardvark: fix logic in PCI configuration read/write functions

[Mason]

[ NB: CC list trimmed ]

On 28/09/2017 14:58, Thomas Petazzoni wrote:

> From: Victor Gu <xigu@marvell.com>
> 
> The PCI configuration space read/write functions were special casing
> the situation where PCI_SLOT(devfn) != 0, and returned
> PCIBIOS_DEVICE_NOT_FOUND in this case.
> 
> However, will this is what is intended for the root bus, it is not

*while* this is what is intended?

[PATCH v2 2/7] PCI: aardvark: set PIO_ADDR_LS correctly in advk_pcie_rd_conf()

[Thomas Petazzoni]

From: Victor Gu <xigu@marvell.com>

When setting the PIO_ADDR_LS register during a configuration read, we
were properly passing the device number, function number and register
number, but not the bus number, causing issues when reading the
configuration of PCIe devices.

Fixes: 8c39d710363c1 ("PCI: aardvark: Add Aardvark PCI host controller driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Victor Gu <xigu@marvell.com>
Reviewed-by: Wilson Ding <dingwei@marvell.com>
Reviewed-by: Nadav Haklai <nadavh@marvell.com>
[Thomas: tweak commit log.]
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/pci-aardvark.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
index da2881ba7737..af7a9c4a61a4 100644
--- a/drivers/pci/host/pci-aardvark.c
+++ b/drivers/pci/host/pci-aardvark.c
@@ -459,7 +459,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 	advk_writel(pcie, reg, PIO_CTRL);
 
 	/* Program the address registers */
-	reg = PCIE_BDF(devfn) | PCIE_CONF_REG(where);
+	reg = PCIE_CONF_ADDR(bus->number, devfn, where);
 	advk_writel(pcie, reg, PIO_ADDR_LS);
 	advk_writel(pcie, 0, PIO_ADDR_MS);
 
-- 
2.13.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/7] PCI: aardvark: set PIO_ADDR_LS correctly in advk_pcie_rd_conf()

[Bjorn Helgaas]

On Thu, Sep 28, 2017 at 02:58:33PM +0200, Thomas Petazzoni wrote:
> From: Victor Gu <xigu@marvell.com>
> 
> When setting the PIO_ADDR_LS register during a configuration read, we
> were properly passing the device number, function number and register
> number, but not the bus number, causing issues when reading the
> configuration of PCIe devices.
> 
> Fixes: 8c39d710363c1 ("PCI: aardvark: Add Aardvark PCI host controller driver")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Victor Gu <xigu@marvell.com>
> Reviewed-by: Wilson Ding <dingwei@marvell.com>
> Reviewed-by: Nadav Haklai <nadavh@marvell.com>
> [Thomas: tweak commit log.]
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/pci/host/pci-aardvark.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
> index da2881ba7737..af7a9c4a61a4 100644
> --- a/drivers/pci/host/pci-aardvark.c
> +++ b/drivers/pci/host/pci-aardvark.c
> @@ -459,7 +459,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>  	advk_writel(pcie, reg, PIO_CTRL);
>  
>  	/* Program the address registers */
> -	reg = PCIE_BDF(devfn) | PCIE_CONF_REG(where);
> +	reg = PCIE_CONF_ADDR(bus->number, devfn, where);

I think PCIE_BDF() is now unused and should be removed.

>  	advk_writel(pcie, reg, PIO_ADDR_LS);
>  	advk_writel(pcie, 0, PIO_ADDR_MS);
>  
> -- 
> 2.13.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/7] PCI: aardvark: set PIO_ADDR_LS correctly in advk_pcie_rd_conf()

[Thomas Petazzoni]

Hello,

On Thu, 5 Oct 2017 12:25:45 -0500, Bjorn Helgaas wrote:

> > diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
> > index da2881ba7737..af7a9c4a61a4 100644
> > --- a/drivers/pci/host/pci-aardvark.c
> > +++ b/drivers/pci/host/pci-aardvark.c
> > @@ -459,7 +459,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> >  	advk_writel(pcie, reg, PIO_CTRL);
> >  
> >  	/* Program the address registers */
> > -	reg = PCIE_BDF(devfn) | PCIE_CONF_REG(where);
> > +	reg = PCIE_CONF_ADDR(bus->number, devfn, where);  
> 
> I think PCIE_BDF() is now unused and should be removed.

True, I'll fix this in v3.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v2 3/7] PCI: aardvark: set host and device to the same MAX payload size

[Thomas Petazzoni]

From: Victor Gu <xigu@marvell.com>

Since the Aardvark does not implement a PCIe root bus, the Linux PCIe
subsystem will not align the MAX payload size between the host and the
device. This patch ensures that the host and device have the same MAX
payload size, fixing a number of problems with various PCIe devices.

This is part of fixing bug
https://bugzilla.kernel.org/show_bug.cgi?id=196339, this commit was
reported as the user to be important to get a Intel 7260 mini-PCIe
WiFi card working.

Fixes: Fixes: 8c39d710363c1 ("PCI: aardvark: Add Aardvark PCI host controller driver")
Signed-off-by: Victor Gu <xigu@marvell.com>
Reviewed-by: Evan Wang <xswang@marvell.com>
Reviewed-by: Nadav Haklai <nadavh@marvell.com>
[Thomas: tweak commit log.]
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/pci-aardvark.c | 60 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
index af7a9c4a61a4..c8a97bad6c4c 100644
--- a/drivers/pci/host/pci-aardvark.c
+++ b/drivers/pci/host/pci-aardvark.c
@@ -30,8 +30,10 @@
 #define PCIE_CORE_DEV_CTRL_STATS_REG				0xc8
 #define     PCIE_CORE_DEV_CTRL_STATS_RELAX_ORDER_DISABLE	(0 << 4)
 #define     PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ_SHIFT	5
+#define     PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ		0x2
 #define     PCIE_CORE_DEV_CTRL_STATS_SNOOP_DISABLE		(0 << 11)
 #define     PCIE_CORE_DEV_CTRL_STATS_MAX_RD_REQ_SIZE_SHIFT	12
+#define     PCIE_CORE_MPS_UNIT_BYTE				128
 #define PCIE_CORE_LINK_CTRL_STAT_REG				0xd0
 #define     PCIE_CORE_LINK_L0S_ENTRY				BIT(0)
 #define     PCIE_CORE_LINK_TRAINING				BIT(5)
@@ -297,7 +299,8 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 
 	/* Set PCIe Device Control and Status 1 PF0 register */
 	reg = PCIE_CORE_DEV_CTRL_STATS_RELAX_ORDER_DISABLE |
-		(7 << PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ_SHIFT) |
+		(PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ <<
+		 PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ_SHIFT) |
 		PCIE_CORE_DEV_CTRL_STATS_SNOOP_DISABLE |
 		PCIE_CORE_DEV_CTRL_STATS_MAX_RD_REQ_SIZE_SHIFT;
 	advk_writel(pcie, reg, PCIE_CORE_DEV_CTRL_STATS_REG);
@@ -879,6 +882,58 @@ static int advk_pcie_parse_request_of_pci_ranges(struct advk_pcie *pcie)
 	return err;
 }
 
+static int advk_pcie_find_smpss(struct pci_dev *dev, void *data)
+{
+	u8 *smpss = data;
+
+	if (!dev)
+		return 0;
+
+	if (!pci_is_pcie(dev))
+		return 0;
+
+	if (*smpss > dev->pcie_mpss)
+		*smpss = dev->pcie_mpss;
+
+	return 0;
+}
+
+static int advk_pcie_bus_configure_mps(struct pci_dev *dev, void *data)
+{
+	int mps;
+
+	if (!dev)
+		return 0;
+
+	if (!pci_is_pcie(dev))
+		return 0;
+
+	mps = PCIE_CORE_MPS_UNIT_BYTE << *(u8 *)data;
+	pcie_set_mps(dev, mps);
+
+	return 0;
+}
+
+static void advk_pcie_configure_mps(struct pci_bus *bus, struct advk_pcie *pcie)
+{
+	u8 smpss = PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ;
+	u32 reg;
+
+	/* Find the minimal supported MAX payload size */
+	advk_pcie_find_smpss(bus->self, &smpss);
+	pci_walk_bus(bus, advk_pcie_find_smpss, &smpss);
+
+	/* Configure RC MAX payload size */
+	reg = advk_readl(pcie, PCIE_CORE_DEV_CTRL_STATS_REG);
+	reg &= ~PCI_EXP_DEVCTL_PAYLOAD;
+	reg |= smpss << PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ_SHIFT;
+	advk_writel(pcie, reg, PCIE_CORE_DEV_CTRL_STATS_REG);
+
+	/* Configure device MAX payload size */
+	advk_pcie_bus_configure_mps(bus->self, &smpss);
+	pci_walk_bus(bus, advk_pcie_bus_configure_mps, &smpss);
+}
+
 static int advk_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -950,6 +1005,9 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	list_for_each_entry(child, &bus->children, node)
 		pcie_bus_configure_settings(child);
 
+	/* Configure the MAX pay load size */
+	advk_pcie_configure_mps(bus, pcie);
+
 	pci_bus_add_devices(bus);
 	return 0;
 }
-- 
2.13.5

Re: [PATCH v2 3/7] PCI: aardvark: set host and device to the same MAX payload size

[Bjorn Helgaas]

On Thu, Sep 28, 2017 at 02:58:34PM +0200, Thomas Petazzoni wrote:
> From: Victor Gu <xigu@marvell.com>
> 
> Since the Aardvark does not implement a PCIe root bus, 

What exactly do you mean by "does not implement a PCIe root bus"?  I
assume there is still a hierarchy of PCI buses, and I assume the
hierarchy has a top-most ("root") bus.

Maybe there's no Root Port?  There are other systems that don't have
Root Ports, and we've made changes to accommodate that, e.g.,

http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=1b8a6079015f

If we can make the generic code work for whatever the Aardvark
topology is, that would be better than adding Aardvark-specific code
here.

> the Linux PCIe
> subsystem will not align the MAX payload size between the host and the
> device. This patch ensures that the host and device have the same MAX
> payload size, fixing a number of problems with various PCIe devices.
> 
> This is part of fixing bug
> https://bugzilla.kernel.org/show_bug.cgi?id=196339, this commit was
> reported as the user to be important to get a Intel 7260 mini-PCIe
> WiFi card working.
> 
> Fixes: Fixes: 8c39d710363c1 ("PCI: aardvark: Add Aardvark PCI host controller driver")
> Signed-off-by: Victor Gu <xigu@marvell.com>
> Reviewed-by: Evan Wang <xswang@marvell.com>
> Reviewed-by: Nadav Haklai <nadavh@marvell.com>
> [Thomas: tweak commit log.]
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/pci/host/pci-aardvark.c | 60 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
> index af7a9c4a61a4..c8a97bad6c4c 100644
> --- a/drivers/pci/host/pci-aardvark.c
> +++ b/drivers/pci/host/pci-aardvark.c
> @@ -30,8 +30,10 @@
>  #define PCIE_CORE_DEV_CTRL_STATS_REG				0xc8
>  #define     PCIE_CORE_DEV_CTRL_STATS_RELAX_ORDER_DISABLE	(0 << 4)
>  #define     PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ_SHIFT	5
> +#define     PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ		0x2
>  #define     PCIE_CORE_DEV_CTRL_STATS_SNOOP_DISABLE		(0 << 11)
>  #define     PCIE_CORE_DEV_CTRL_STATS_MAX_RD_REQ_SIZE_SHIFT	12
> +#define     PCIE_CORE_MPS_UNIT_BYTE				128
>  #define PCIE_CORE_LINK_CTRL_STAT_REG				0xd0
>  #define     PCIE_CORE_LINK_L0S_ENTRY				BIT(0)
>  #define     PCIE_CORE_LINK_TRAINING				BIT(5)
> @@ -297,7 +299,8 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  
>  	/* Set PCIe Device Control and Status 1 PF0 register */
>  	reg = PCIE_CORE_DEV_CTRL_STATS_RELAX_ORDER_DISABLE |
> -		(7 << PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ_SHIFT) |
> +		(PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ <<
> +		 PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ_SHIFT) |
>  		PCIE_CORE_DEV_CTRL_STATS_SNOOP_DISABLE |
>  		PCIE_CORE_DEV_CTRL_STATS_MAX_RD_REQ_SIZE_SHIFT;
>  	advk_writel(pcie, reg, PCIE_CORE_DEV_CTRL_STATS_REG);
> @@ -879,6 +882,58 @@ static int advk_pcie_parse_request_of_pci_ranges(struct advk_pcie *pcie)
>  	return err;
>  }
>  
> +static int advk_pcie_find_smpss(struct pci_dev *dev, void *data)
> +{
> +	u8 *smpss = data;
> +
> +	if (!dev)
> +		return 0;
> +
> +	if (!pci_is_pcie(dev))
> +		return 0;
> +
> +	if (*smpss > dev->pcie_mpss)
> +		*smpss = dev->pcie_mpss;
> +
> +	return 0;
> +}
> +
> +static int advk_pcie_bus_configure_mps(struct pci_dev *dev, void *data)
> +{
> +	int mps;
> +
> +	if (!dev)
> +		return 0;
> +
> +	if (!pci_is_pcie(dev))
> +		return 0;
> +
> +	mps = PCIE_CORE_MPS_UNIT_BYTE << *(u8 *)data;
> +	pcie_set_mps(dev, mps);
> +
> +	return 0;
> +}
> +
> +static void advk_pcie_configure_mps(struct pci_bus *bus, struct advk_pcie *pcie)
> +{
> +	u8 smpss = PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ;
> +	u32 reg;
> +
> +	/* Find the minimal supported MAX payload size */
> +	advk_pcie_find_smpss(bus->self, &smpss);
> +	pci_walk_bus(bus, advk_pcie_find_smpss, &smpss);
> +
> +	/* Configure RC MAX payload size */
> +	reg = advk_readl(pcie, PCIE_CORE_DEV_CTRL_STATS_REG);
> +	reg &= ~PCI_EXP_DEVCTL_PAYLOAD;
> +	reg |= smpss << PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ_SHIFT;
> +	advk_writel(pcie, reg, PCIE_CORE_DEV_CTRL_STATS_REG);
> +
> +	/* Configure device MAX payload size */
> +	advk_pcie_bus_configure_mps(bus->self, &smpss);
> +	pci_walk_bus(bus, advk_pcie_bus_configure_mps, &smpss);
> +}
> +
>  static int advk_pcie_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -950,6 +1005,9 @@ static int advk_pcie_probe(struct platform_device *pdev)
>  	list_for_each_entry(child, &bus->children, node)
>  		pcie_bus_configure_settings(child);
>  
> +	/* Configure the MAX pay load size */
> +	advk_pcie_configure_mps(bus, pcie);
> +
>  	pci_bus_add_devices(bus);
>  	return 0;
>  }
> -- 
> 2.13.5
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/7] PCI: aardvark: set host and device to the same MAX payload size

[Thomas Petazzoni]

Hello Bjorn,

On Thu, 5 Oct 2017 12:31:02 -0500, Bjorn Helgaas wrote:
> On Thu, Sep 28, 2017 at 02:58:34PM +0200, Thomas Petazzoni wrote:
> > From: Victor Gu <xigu@marvell.com>
> > 
> > Since the Aardvark does not implement a PCIe root bus,   
> 
> What exactly do you mean by "does not implement a PCIe root bus"?  I
> assume there is still a hierarchy of PCI buses, and I assume the
> hierarchy has a top-most ("root") bus.
> 
> Maybe there's no Root Port?  There are other systems that don't have
> Root Ports, and we've made changes to accommodate that, e.g.,
> 
> http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=1b8a6079015f

I'm trying to get back (finally) to this topic. Unfortunately, your
branch has been rebased, and this commit no longer exists. Do you have
an updated pointer about what you suggest to use for systems that don't
have Root Ports ?

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 3/7] PCI: aardvark: set host and device to the same MAX payload size

[Bjorn Helgaas]

[+cc Lorenzo, who maintains this area now]

On Tue, Jan 09, 2018 at 04:39:18PM +0100, Thomas Petazzoni wrote:
> Hello Bjorn,
> 
> On Thu, 5 Oct 2017 12:31:02 -0500, Bjorn Helgaas wrote:
> > On Thu, Sep 28, 2017 at 02:58:34PM +0200, Thomas Petazzoni wrote:
> > > From: Victor Gu <xigu@marvell.com>
> > > 
> > > Since the Aardvark does not implement a PCIe root bus,   
> > 
> > What exactly do you mean by "does not implement a PCIe root bus"?  I
> > assume there is still a hierarchy of PCI buses, and I assume the
> > hierarchy has a top-most ("root") bus.
> > 
> > Maybe there's no Root Port?  There are other systems that don't have
> > Root Ports, and we've made changes to accommodate that, e.g.,
> > 
> > http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=1b8a6079015f
> 
> I'm trying to get back (finally) to this topic. Unfortunately, your
> branch has been rebased, and this commit no longer exists. Do you have
> an updated pointer about what you suggest to use for systems that don't
> have Root Ports ?

Sorry, about that; here's the upstream commit, FWIW:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=ee8bdfb6568d

If the OS sees no Root Port (I haven't seen the full lspci or kernel
enumeration log, so I don't know what the topology actually is), I
assume you probably have some Endpoints that have valid Link
Capabilities, Control, and Status registers.  Those refer to the
downstream end of the Link, and the Root Port would normally have
corresponding registers that refer to the upstream end.

The lack of the Root Port means we can't do any management of those
top-level Links, so no ASPM, no MPS, no link width/speed management,
etc.

I see that advk_pcie_probe() calls pcie_bus_configure_settings() like
all other drivers, and ideally we would try to make that work just
like it does on other platforms.  The code is:

  pci_scan_root_bus_bridge(bridge);
  bus = bridge->bus;
  list_for_each_entry(child, &bus->children, node)
    pcie_bus_configure_settings(child);

This MPS setting is all strictly in the PCIe domain (it's not in the
Aardvark domain and shouldn't have any Aardvark dependencies), so I
would expect the core code to just work, modulo some possible
confusion if it expects to find a Root Port but doesn't.

Can you collect "lspci -vv" output and details about what currently
goes wrong?  Then we'd have something more concrete to talk about.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/7] PCI: aardvark: set host and device to the same MAX payload size

[Thomas Petazzoni]

Hello,

On Tue, 9 Jan 2018 16:14:36 -0600, Bjorn Helgaas wrote:

> > I'm trying to get back (finally) to this topic. Unfortunately, your
> > branch has been rebased, and this commit no longer exists. Do you have
> > an updated pointer about what you suggest to use for systems that don't
> > have Root Ports ?  
> 
> Sorry, about that; here's the upstream commit, FWIW:
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=ee8bdfb6568d

Thanks. I don't see how this commit can fix our problem though, see below.

> If the OS sees no Root Port (I haven't seen the full lspci or kernel
> enumeration log, so I don't know what the topology actually is), I
> assume you probably have some Endpoints that have valid Link
> Capabilities, Control, and Status registers.  Those refer to the
> downstream end of the Link, and the Root Port would normally have
> corresponding registers that refer to the upstream end.
> 
> The lack of the Root Port means we can't do any management of those
> top-level Links, so no ASPM, no MPS, no link width/speed management,
> etc.
> 
> I see that advk_pcie_probe() calls pcie_bus_configure_settings() like
> all other drivers, and ideally we would try to make that work just
> like it does on other platforms.  The code is:
> 
>   pci_scan_root_bus_bridge(bridge);
>   bus = bridge->bus;
>   list_for_each_entry(child, &bus->children, node)
>     pcie_bus_configure_settings(child);
> 
> This MPS setting is all strictly in the PCIe domain (it's not in the
> Aardvark domain and shouldn't have any Aardvark dependencies), so I
> would expect the core code to just work, modulo some possible
> confusion if it expects to find a Root Port but doesn't.
> 
> Can you collect "lspci -vv" output and details about what currently
> goes wrong?  Then we'd have something more concrete to talk about.

With an E1000E PCIe NIC connected, the entire lspci -vvv output is:

# lspci -vv
00:00.0 Ethernet controller: Intel Corporation 82572EI Gigabit Ethernet Controller (Copper) (rev 06)
	Subsystem: Intel Corporation PRO/1000 PT Server Adapter
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 32 bytes
	Interrupt: pin A routed to IRQ 40
	Region 0: Memory at e8000000 (32-bit, non-prefetchable) [size=128K]
	Region 1: Memory at e8020000 (32-bit, non-prefetchable) [size=128K]
	Region 2: I/O ports at 1000 [disabled] [size=32]
	Expansion ROM at e8040000 [disabled] [size=128K]
	Capabilities: [c8] Power Management version 2
		Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=1 PME-
	Capabilities: [d0] MSI: Enable+ Count=1/1 Maskable- 64bit+
		Address: 000000001d1f6f68  Data: 0028
	Capabilities: [e0] Express (v1) Endpoint, MSI 00
		DevCap:	MaxPayload 256 bytes, PhantFunc 0, Latency L0s <512ns, L1 <64us
			ExtTag- AttnBtn- AttnInd- PwrInd- RBE- FLReset- SlotPowerLimit 0.000W
		DevCtl:	Report errors: Correctable+ Non-Fatal+ Fatal+ Unsupported+
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
			MaxPayload 256 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
		LnkCap:	Port #0, Speed 2.5GT/s, Width x1, ASPM L0s, Exit Latency L0s <4us
			ClockPM- Surprise- LLActRep- BwNot- ASPMOptComp-
		LnkCtl:	ASPM Disabled; RCB 64 bytes Disabled- CommClk-
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
	Capabilities: [100 v1] Advanced Error Reporting
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UESvrt:	DLP+ SDES- TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
		CEMsk:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
		AERCap:	First Error Pointer: 00, ECRCGenCap- ECRCGenEn- ECRCChkCap- ECRCChkEn-
			MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
		HeaderLog: 00000000 00000000 00000000 00000000
	Capabilities: [140 v1] Device Serial Number 00-1b-21-ff-ff-c1-c4-fe
	Kernel driver in use: e1000e

I.e, there is no Root Port. Therefore, I don't see how the kernel
can know what is the maximum allowed payload size of the PCIe
controller, nor how to adjust the payload size to use. Same for the L0s
configuration.

This is why we need those changes, one to update the PCIe controller
MPS according to the Maximum Payload Size acceptable by the endpoint,
and one to disable L0s entirely to avoid issues with non-L0s compliant
devices.

Does that make more sense ?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 3/7] PCI: aardvark: set host and device to the same MAX payload size

[Bjorn Helgaas]

On Fri, Jan 12, 2018 at 11:14:48AM +0100, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 9 Jan 2018 16:14:36 -0600, Bjorn Helgaas wrote:
> 
> > > I'm trying to get back (finally) to this topic. Unfortunately, your
> > > branch has been rebased, and this commit no longer exists. Do you have
> > > an updated pointer about what you suggest to use for systems that don't
> > > have Root Ports ?  
> > 
> > Sorry, about that; here's the upstream commit, FWIW:
> > 
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=ee8bdfb6568d
> 
> Thanks. I don't see how this commit can fix our problem though, see below.

Sorry, I didn't mean that commit would fix your problem.  It's just an
example of another case where generic code incorrectly assumed a Root
Port would always be present.

> > If the OS sees no Root Port (I haven't seen the full lspci or kernel
> > enumeration log, so I don't know what the topology actually is), I
> > assume you probably have some Endpoints that have valid Link
> > Capabilities, Control, and Status registers.  Those refer to the
> > downstream end of the Link, and the Root Port would normally have
> > corresponding registers that refer to the upstream end.
> > 
> > The lack of the Root Port means we can't do any management of those
> > top-level Links, so no ASPM, no MPS, no link width/speed management,
> > etc.
> > 
> > I see that advk_pcie_probe() calls pcie_bus_configure_settings() like
> > all other drivers, and ideally we would try to make that work just
> > like it does on other platforms.  The code is:
> > 
> >   pci_scan_root_bus_bridge(bridge);
> >   bus = bridge->bus;
> >   list_for_each_entry(child, &bus->children, node)
> >     pcie_bus_configure_settings(child);
> > 
> > This MPS setting is all strictly in the PCIe domain (it's not in the
> > Aardvark domain and shouldn't have any Aardvark dependencies), so I
> > would expect the core code to just work, modulo some possible
> > confusion if it expects to find a Root Port but doesn't.
> > 
> > Can you collect "lspci -vv" output and details about what currently
> > goes wrong?  Then we'd have something more concrete to talk about.
> 
> With an E1000E PCIe NIC connected, the entire lspci -vvv output is:
> 
> # lspci -vv
> 00:00.0 Ethernet controller: Intel Corporation 82572EI Gigabit Ethernet Controller (Copper) (rev 06)
> 	Subsystem: Intel Corporation PRO/1000 PT Server Adapter
> 	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
> 	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
> 	Latency: 0, Cache Line Size: 32 bytes
> 	Interrupt: pin A routed to IRQ 40
> 	Region 0: Memory at e8000000 (32-bit, non-prefetchable) [size=128K]
> 	Region 1: Memory at e8020000 (32-bit, non-prefetchable) [size=128K]
> 	Region 2: I/O ports at 1000 [disabled] [size=32]
> 	Expansion ROM at e8040000 [disabled] [size=128K]
> 	Capabilities: [c8] Power Management version 2
> 		Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
> 		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=1 PME-
> 	Capabilities: [d0] MSI: Enable+ Count=1/1 Maskable- 64bit+
> 		Address: 000000001d1f6f68  Data: 0028
> 	Capabilities: [e0] Express (v1) Endpoint, MSI 00
> 		DevCap:	MaxPayload 256 bytes, PhantFunc 0, Latency L0s <512ns, L1 <64us
> 			ExtTag- AttnBtn- AttnInd- PwrInd- RBE- FLReset- SlotPowerLimit 0.000W
> 		DevCtl:	Report errors: Correctable+ Non-Fatal+ Fatal+ Unsupported+
> 			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
> 			MaxPayload 256 bytes, MaxReadReq 512 bytes
> 		DevSta:	CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
> 		LnkCap:	Port #0, Speed 2.5GT/s, Width x1, ASPM L0s, Exit Latency L0s <4us
> 			ClockPM- Surprise- LLActRep- BwNot- ASPMOptComp-
> 		LnkCtl:	ASPM Disabled; RCB 64 bytes Disabled- CommClk-
> 			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
> 		LnkSta:	Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
> 	Capabilities: [100 v1] Advanced Error Reporting
> 		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
> 		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
> 		UESvrt:	DLP+ SDES- TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
> 		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
> 		CEMsk:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
> 		AERCap:	First Error Pointer: 00, ECRCGenCap- ECRCGenEn- ECRCChkCap- ECRCChkEn-
> 			MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
> 		HeaderLog: 00000000 00000000 00000000 00000000
> 	Capabilities: [140 v1] Device Serial Number 00-1b-21-ff-ff-c1-c4-fe
> 	Kernel driver in use: e1000e
> 
> I.e, there is no Root Port. Therefore, I don't see how the kernel
> can know what is the maximum allowed payload size of the PCIe
> controller, nor how to adjust the payload size to use. Same for the L0s
> configuration.

The Device Control MPS field defaults to 128 bytes.  Generic software
can only change that default if it knows that every element that might
receive a packet from the device can handle it.  In this case, we have
no information about what the invisible Root Port can handle, so I
would argue that we cannot change MPS.

In the lspci above, MPS is set to 256 bytes.  If that was done by
firmware, it might be safe because it knows things about the Root Port
that Linux doesn't.  But I don't think the Linux PCI core could set it
to 256.

ASPM L0s is similar.  We should only enable L0s if we can tell that
both ends of the link support it.  If there's no Root Port, we don't
have any ASPM capability information for the upstream end of the link,
so we shouldn't enable ASPM at all.

> This is why we need those changes, one to update the PCIe controller
> MPS according to the Maximum Payload Size acceptable by the endpoint,
> and one to disable L0s entirely to avoid issues with non-L0s compliant
> devices.

The generic core code should perform minimal, guaranteed-to-work
configuration using the least information and fewest assumptions
possible.  That may not lead to optimal performance, but it should at
least be functional.  This should work even if there is no Root Port.

Once we have that figured out, then we can worry about whether we can
or should do platform-specific tweaks to improve performance, e.g.,
increase MPS if we know Root Port capabilities implicitly.

I had the impression that these patches were required for correct
functionality, not just to improve performance.  But maybe I
misunderstood?

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/7] PCI: aardvark: set host and device to the same MAX payload size

[Thomas Petazzoni]

Hello Bjorn,

Thanks again for this discussion!

On Fri, 12 Jan 2018 08:40:24 -0600, Bjorn Helgaas wrote:

> Sorry, I didn't mean that commit would fix your problem.  It's just an
> example of another case where generic code incorrectly assumed a Root
> Port would always be present.

OK, understood.

> > I.e, there is no Root Port. Therefore, I don't see how the kernel
> > can know what is the maximum allowed payload size of the PCIe
> > controller, nor how to adjust the payload size to use. Same for the L0s
> > configuration.  
> 
> The Device Control MPS field defaults to 128 bytes.  Generic software
> can only change that default if it knows that every element that might
> receive a packet from the device can handle it.  In this case, we have
> no information about what the invisible Root Port can handle, so I
> would argue that we cannot change MPS.
> 
> In the lspci above, MPS is set to 256 bytes.  If that was done by
> firmware, it might be safe because it knows things about the Root Port
> that Linux doesn't.  But I don't think the Linux PCI core could set it
> to 256.

So you're suspecting that the firmware/bootloader has configured the
MPS on the E1000E device to 256 bytes ?

Isn't it dangerous for the kernel to rely on the firmware/bootloader
configuration ? Indeed, the firmware/bootloader might have configured
MPS to X bytes on the endpoint, but when the kernel boots and
initializes the PCIe controller, its sets the PCIe controller MPS to Y
bytes, with Y > X.

> ASPM L0s is similar.  We should only enable L0s if we can tell that
> both ends of the link support it.  If there's no Root Port, we don't
> have any ASPM capability information for the upstream end of the link,
> so we shouldn't enable ASPM at all.

Well, even without the Root Port, we are able to use the endpoint
configuration space to figure out whether it supports L0s, and adjust
the root complex configuration accordingly. This is what our patch is
doing for MPS, and which could be done similarly for L0s, no?

> 
> > This is why we need those changes, one to update the PCIe controller
> > MPS according to the Maximum Payload Size acceptable by the endpoint,
> > and one to disable L0s entirely to avoid issues with non-L0s compliant
> > devices.  
> 
> The generic core code should perform minimal, guaranteed-to-work
> configuration using the least information and fewest assumptions
> possible.  That may not lead to optimal performance, but it should at
> least be functional.  This should work even if there is no Root Port.
> 
> Once we have that figured out, then we can worry about whether we can
> or should do platform-specific tweaks to improve performance, e.g.,
> increase MPS if we know Root Port capabilities implicitly.
> 
> I had the impression that these patches were required for correct
> functionality, not just to improve performance.  But maybe I
> misunderstood?

I don't myself have the device that wasn't working, and that this patch
got to work, so I can't double check myself. However, indeed, I was
told that without this fix, some devices would not work.

One question: is it valid/working to have the root complex configured
with MPS = 128 bytes but the endpoint configured with MPS = 256 or 512
bytes ? Or should the MPS value be strictly equal on both sides ?

Depending on your answer, there are two options:

 - It is a valid situation to have a root complex MPS lower than the
   endpoint MPS. In this case, we could for now simply unconditionally
   set the MPS to 128 bytes in the root complex, as a fix to get all
   devices working. And then separately, work on improving performance
   by increasing the MPS according to the endpoint capabilities.

 - It is not valid for the root complex MPS to be different than the
   endpoint MPS. In this case, then I don't see how we can do things
   differently than the proposed patch: we have to see what the
   endpoint MPS is, and adjust the root complex MPS accordingly.
   Indeed, the bootloader/firmware might have changed the endpoint MPS
   so that it is no longer the default of 128 bytes.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 3/7] PCI: aardvark: set host and device to the same MAX payload size

[Bjorn Helgaas]

On Fri, Jan 12, 2018 at 04:46:44PM +0100, Thomas Petazzoni wrote:
> On Fri, 12 Jan 2018 08:40:24 -0600, Bjorn Helgaas wrote:

> > The Device Control MPS field defaults to 128 bytes.  Generic software
> > can only change that default if it knows that every element that might
> > receive a packet from the device can handle it.  In this case, we have
> > no information about what the invisible Root Port can handle, so I
> > would argue that we cannot change MPS.
> > 
> > In the lspci above, MPS is set to 256 bytes.  If that was done by
> > firmware, it might be safe because it knows things about the Root Port
> > that Linux doesn't.  But I don't think the Linux PCI core could set it
> > to 256.
> 
> So you're suspecting that the firmware/bootloader has configured the
> MPS on the E1000E device to 256 bytes ?

I didn't word that very well.  It looks like *something* set it to
256, but I don't know what.  It's possible Linux did, but I think that
would be a bug and should be fixed.  We'd have to instrument the code
or analyze it more closely than I have.

> Isn't it dangerous for the kernel to rely on the firmware/bootloader
> configuration ? Indeed, the firmware/bootloader might have configured
> MPS to X bytes on the endpoint, but when the kernel boots and
> initializes the PCIe controller, its sets the PCIe controller MPS to Y
> bytes, with Y > X.
> 
> > ASPM L0s is similar.  We should only enable L0s if we can tell that
> > both ends of the link support it.  If there's no Root Port, we don't
> > have any ASPM capability information for the upstream end of the link,
> > so we shouldn't enable ASPM at all.
> 
> Well, even without the Root Port, we are able to use the endpoint
> configuration space to figure out whether it supports L0s, and adjust
> the root complex configuration accordingly. This is what our patch is
> doing for MPS, and which could be done similarly for L0s, no?

Yes, this is where it would get machine-specific.  I don't know how
that should be structured, or even whether it's really worthwhile.  I
think the core should make it *work* with the least-common-denominator
approach, but I'm not convinced that a lot of effort should be put
into optimizing a topology that doesn't follow the spec.  A driver
could do this outside the core, but I think it would be better to put
the effort into making the topology more standard.

Why exactly *doesn't* Aardvark expose the Root Port?  I assume it does
actually exist, since there is actually a link leading to the slot,
and there has to be *something* at the upstream end of that link.

> > I had the impression that these patches were required for correct
> > functionality, not just to improve performance.  But maybe I
> > misunderstood?
> 
> I don't myself have the device that wasn't working, and that this patch
> got to work, so I can't double check myself. However, indeed, I was
> told that without this fix, some devices would not work.
> 
> One question: is it valid/working to have the root complex configured
> with MPS = 128 bytes but the endpoint configured with MPS = 256 or 512
> bytes ? Or should the MPS value be strictly equal on both sides ?

Per PCIe r4.0, sec 2.2.2, a device cannot transmit a TLP with a
payload larger than its MPS.  A device receiving a TLP with a payload
larger than its MPS must treat the TLP as malformed.

I think that means MPS really should be set the same on both sides so
the device can do both DMA reads and writes safely.

> Depending on your answer, there are two options:
> 
>  - It is a valid situation to have a root complex MPS lower than the
>    endpoint MPS. In this case, we could for now simply unconditionally
>    set the MPS to 128 bytes in the root complex, as a fix to get all
>    devices working. And then separately, work on improving performance
>    by increasing the MPS according to the endpoint capabilities.
> 
>  - It is not valid for the root complex MPS to be different than the
>    endpoint MPS. In this case, then I don't see how we can do things
>    differently than the proposed patch: we have to see what the
>    endpoint MPS is, and adjust the root complex MPS accordingly.
>    Indeed, the bootloader/firmware might have changed the endpoint MPS
>    so that it is no longer the default of 128 bytes.

All devices are guaranteed to support MPS = 128 bytes, so if you set
the Root Port to that in the driver, we should be able to make the PCI
core leave (or set, if necessary) all devices with MPS = 128.

I think we should start with that first, then worry about performance
optimizations separately.  I guess I'm still just shaking my head over
the invisible Root Port mystery.  I think the other cases I know about
are related to virtualization, where I can sort of understand why the
Root Port is missing, but I don't think that's the situation here.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/7] PCI: aardvark: use isr1 instead of isr0 interrupt in legacy irq mode

[Thomas Petazzoni]

From: Victor Gu <xigu@marvell.com>

The Aardvark has two interrupts sets:

 - first set is bit[23:16] of PCIe ISR 0 register(RD0074840h)

 - second set is bit[11:8] of PCIe ISR 1 register(RD0074848h)

Only one set should be used, while another set should be masked.

The second set, ISR1, is more advanced, the Legacy INT_X status bit is
asserted once Assert_INTX message is received, and de-asserted after
Deassert_INTX message is received. Therefore, it matches what the
driver is currently doing in the ->irq_mask() and ->irq_unmask()
functions. The ISR0 requires additional work to deassert the
interrupt, which the driver doesn't do currently.

This commit resolves a number of issues with legacy interrupts.

This is part of fixing bug
https://bugzilla.kernel.org/show_bug.cgi?id=196339, this commit was
reported as the user to be important to get a Intel 7260 mini-PCIe
WiFi card working.

Fixes: 8c39d710363c1 ("PCI: aardvark: Add Aardvark PCI host controller driver")
Signed-off-by: Victor Gu <xigu@marvell.com>
Reviewed-by: Evan Wang <xswang@marvell.com>
Reviewed-by: Nadav Haklai <nadavh@marvell.com>
[Thomas: tweak commit log.]
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/pci-aardvark.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
index c8a97bad6c4c..23e243135ebe 100644
--- a/drivers/pci/host/pci-aardvark.c
+++ b/drivers/pci/host/pci-aardvark.c
@@ -105,7 +105,8 @@
 #define PCIE_ISR1_MASK_REG			(CONTROL_BASE_ADDR + 0x4C)
 #define     PCIE_ISR1_POWER_STATE_CHANGE	BIT(4)
 #define     PCIE_ISR1_FLUSH			BIT(5)
-#define     PCIE_ISR1_ALL_MASK			GENMASK(5, 4)
+#define     PCIE_ISR1_INTX_ASSERT(val)		BIT(8 + (val))
+#define     PCIE_ISR1_ALL_MASK			GENMASK(11, 4)
 #define PCIE_MSI_ADDR_LOW_REG			(CONTROL_BASE_ADDR + 0x50)
 #define PCIE_MSI_ADDR_HIGH_REG			(CONTROL_BASE_ADDR + 0x54)
 #define PCIE_MSI_STATUS_REG			(CONTROL_BASE_ADDR + 0x58)
@@ -615,9 +616,9 @@ static void advk_pcie_irq_mask(struct irq_data *d)
 	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 	u32 mask;
 
-	mask = advk_readl(pcie, PCIE_ISR0_MASK_REG);
-	mask |= PCIE_ISR0_INTX_ASSERT(hwirq);
-	advk_writel(pcie, mask, PCIE_ISR0_MASK_REG);
+	mask = advk_readl(pcie, PCIE_ISR1_MASK_REG);
+	mask |= PCIE_ISR1_INTX_ASSERT(hwirq);
+	advk_writel(pcie, mask, PCIE_ISR1_MASK_REG);
 }
 
 static void advk_pcie_irq_unmask(struct irq_data *d)
@@ -626,9 +627,9 @@ static void advk_pcie_irq_unmask(struct irq_data *d)
 	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 	u32 mask;
 
-	mask = advk_readl(pcie, PCIE_ISR0_MASK_REG);
-	mask &= ~PCIE_ISR0_INTX_ASSERT(hwirq);
-	advk_writel(pcie, mask, PCIE_ISR0_MASK_REG);
+	mask = advk_readl(pcie, PCIE_ISR1_MASK_REG);
+	mask &= ~PCIE_ISR1_INTX_ASSERT(hwirq);
+	advk_writel(pcie, mask, PCIE_ISR1_MASK_REG);
 }
 
 static int advk_pcie_irq_map(struct irq_domain *h,
@@ -771,29 +772,35 @@ static void advk_pcie_handle_msi(struct advk_pcie *pcie)
 
 static void advk_pcie_handle_int(struct advk_pcie *pcie)
 {
-	u32 val, mask, status;
+	u32 isr0_val, isr0_mask, isr0_status;
+	u32 isr1_val, isr1_mask, isr1_status;
 	int i, virq;
 
-	val = advk_readl(pcie, PCIE_ISR0_REG);
-	mask = advk_readl(pcie, PCIE_ISR0_MASK_REG);
-	status = val & ((~mask) & PCIE_ISR0_ALL_MASK);
+	isr0_val = advk_readl(pcie, PCIE_ISR0_REG);
+	isr0_mask = advk_readl(pcie, PCIE_ISR0_MASK_REG);
+	isr0_status = isr0_val & ((~isr0_mask) & PCIE_ISR0_ALL_MASK);
 
-	if (!status) {
-		advk_writel(pcie, val, PCIE_ISR0_REG);
+	isr1_val = advk_readl(pcie, PCIE_ISR1_REG);
+	isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG);
+	isr1_status = isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
+
+	if (!isr0_status && !isr1_status) {
+		advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
+		advk_writel(pcie, isr1_val, PCIE_ISR1_REG);
 		return;
 	}
 
 	/* Process MSI interrupts */
-	if (status & PCIE_ISR0_MSI_INT_PENDING)
+	if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
 		advk_pcie_handle_msi(pcie);
 
 	/* Process legacy interrupts */
 	for (i = 0; i < PCI_NUM_INTX; i++) {
-		if (!(status & PCIE_ISR0_INTX_ASSERT(i)))
+		if (!(isr1_status & PCIE_ISR1_INTX_ASSERT(i)))
 			continue;
 
-		advk_writel(pcie, PCIE_ISR0_INTX_ASSERT(i),
-			    PCIE_ISR0_REG);
+		advk_writel(pcie, PCIE_ISR1_INTX_ASSERT(i),
+			    PCIE_ISR1_REG);
 
 		virq = irq_find_mapping(pcie->irq_domain, i);
 		generic_handle_irq(virq);
-- 
2.13.5

[PATCH v2 5/7] PCI: aardvark: disable LOS state by default

[Thomas Petazzoni]

From: Victor Gu <xigu@marvell.com>

Some PCIe devices do not support LOS, and will cause timeouts if the
root complex forces the LOS state. This patch disables the LOS state
by default.

This is part of fixing bug
https://bugzilla.kernel.org/show_bug.cgi?id=196339, this commit was
reported as the user to be important to get a Intel 7260 mini-PCIe
WiFi card working.

Fixes: 8c39d710363c1 ("PCI: aardvark: Add Aardvark PCI host controller driver")
Signed-off-by: Victor Gu <xigu@marvell.com>
Reviewed-by: Evan Wang <xswang@marvell.com>
Reviewed-by: Nadav Haklai <nadavh@marvell.com>
[Thomas: tweak commit log.]
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/pci-aardvark.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
index 23e243135ebe..a173f31853df 100644
--- a/drivers/pci/host/pci-aardvark.c
+++ b/drivers/pci/host/pci-aardvark.c
@@ -368,8 +368,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 
 	advk_pcie_wait_for_link(pcie);
 
-	reg = PCIE_CORE_LINK_L0S_ENTRY |
-		(1 << PCIE_CORE_LINK_WIDTH_SHIFT);
+	reg = (1 << PCIE_CORE_LINK_WIDTH_SHIFT);
 	advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
 
 	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
-- 
2.13.5

Re: [PATCH v2 5/7] PCI: aardvark: disable LOS state by default

[Bjorn Helgaas]

I assume you mean "ASPM L0s"?

On Thu, Sep 28, 2017 at 02:58:36PM +0200, Thomas Petazzoni wrote:
> From: Victor Gu <xigu@marvell.com>
> 
> Some PCIe devices do not support LOS, and will cause timeouts if the
> root complex forces the LOS state. This patch disables the LOS state
> by default.

Per PCIe r3.1, sec 5.4.1.3, software should not enable L0s in either
direction unless both ends support L0s.

I'm unclear on what the bug is here.  Is the generic ASPM code
incorrectly enabling L0s when one end doesn't support it?  That seems
unlikely, but if so, it should be fixed in the generic ASPM code.

Are both ends advertising L0s support, but there's a hardware erratum
on the Aardvark end that keeps it from working correctly?  If so, we
should say that explicitly and include a reference to a published
hardware erratum.

Something else?

> This is part of fixing bug
> https://bugzilla.kernel.org/show_bug.cgi?id=196339, this commit was
> reported as the user to be important to get a Intel 7260 mini-PCIe
> WiFi card working.

The bugzilla link is a good start, but "reported by the user to be
important" is meaningless by itself.  What we need here is the details
of what's broken and how this fixes it.  Unfortunately the bugzilla
doesn't have those details.

> Fixes: 8c39d710363c1 ("PCI: aardvark: Add Aardvark PCI host controller driver")
> Signed-off-by: Victor Gu <xigu@marvell.com>
> Reviewed-by: Evan Wang <xswang@marvell.com>
> Reviewed-by: Nadav Haklai <nadavh@marvell.com>
> [Thomas: tweak commit log.]
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/pci/host/pci-aardvark.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
> index 23e243135ebe..a173f31853df 100644
> --- a/drivers/pci/host/pci-aardvark.c
> +++ b/drivers/pci/host/pci-aardvark.c
> @@ -368,8 +368,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  
>  	advk_pcie_wait_for_link(pcie);
>  
> -	reg = PCIE_CORE_LINK_L0S_ENTRY |
> -		(1 << PCIE_CORE_LINK_WIDTH_SHIFT);
> +	reg = (1 << PCIE_CORE_LINK_WIDTH_SHIFT);
>  	advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
>  
>  	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
> -- 
> 2.13.5
> 

Re: [PATCH v2 5/7] PCI: aardvark: disable LOS state by default

[Thomas Petazzoni]

Hello,

On Thu, 5 Oct 2017 12:46:07 -0500, Bjorn Helgaas wrote:

> On Thu, Sep 28, 2017 at 02:58:36PM +0200, Thomas Petazzoni wrote:
> > From: Victor Gu <xigu@marvell.com>
> > 
> > Some PCIe devices do not support LOS, and will cause timeouts if the
> > root complex forces the LOS state. This patch disables the LOS state
> > by default.  
> 
> Per PCIe r3.1, sec 5.4.1.3, software should not enable L0s in either
> direction unless both ends support L0s.
> 
> I'm unclear on what the bug is here.  Is the generic ASPM code
> incorrectly enabling L0s when one end doesn't support it?  That seems
> unlikely, but if so, it should be fixed in the generic ASPM code.
> 
> Are both ends advertising L0s support, but there's a hardware erratum
> on the Aardvark end that keeps it from working correctly?  If so, we
> should say that explicitly and include a reference to a published
> hardware erratum.
> 
> Something else?

I'll do some more research on this and get back to you.

> > This is part of fixing bug
> > https://bugzilla.kernel.org/show_bug.cgi?id=196339, this commit was
> > reported as the user to be important to get a Intel 7260 mini-PCIe
> > WiFi card working.  
> 
> The bugzilla link is a good start, but "reported by the user to be
> important" is meaningless by itself.  What we need here is the details
> of what's broken and how this fixes it.  Unfortunately the bugzilla
> doesn't have those details.

Yes, the issue is that the bug report doesn't have much details, and I
don't have the specific PCIe card that was used by the bug reporter.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v2 6/7] PCI: aardvark: fix PCIe max read request size setting

[Thomas Petazzoni]

From: Evan Wang <xswang@marvell.com>

There is an obvious typo issue in the definition of the PCIe maximum
read request size: a bit shift is directly used as a value, while it
should be used to shift the correct value.

This is part of fixing bug
https://bugzilla.kernel.org/show_bug.cgi?id=196339, this commit was
reported as the user to be important to get a Intel 7260 mini-PCIe
WiFi card working.

Fixes: 8c39d710363c1 ("PCI: aardvark: Add Aardvark PCI host controller driver")
Signed-off-by: Evan Wang <xswang@marvell.com>
Reviewed-by: Victor Gu <xigu@marvell.com>
Reviewed-by: Nadav Haklai <nadavh@marvell.com>
[Thomas: tweak commit log.]
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/pci-aardvark.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
index a173f31853df..c1093b023e48 100644
--- a/drivers/pci/host/pci-aardvark.c
+++ b/drivers/pci/host/pci-aardvark.c
@@ -33,6 +33,7 @@
 #define     PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ		0x2
 #define     PCIE_CORE_DEV_CTRL_STATS_SNOOP_DISABLE		(0 << 11)
 #define     PCIE_CORE_DEV_CTRL_STATS_MAX_RD_REQ_SIZE_SHIFT	12
+#define     PCIE_CORE_DEV_CTRL_STATS_MAX_RD_REQ_SZ		0x2
 #define     PCIE_CORE_MPS_UNIT_BYTE				128
 #define PCIE_CORE_LINK_CTRL_STAT_REG				0xd0
 #define     PCIE_CORE_LINK_L0S_ENTRY				BIT(0)
@@ -303,7 +304,8 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 		(PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ <<
 		 PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ_SHIFT) |
 		PCIE_CORE_DEV_CTRL_STATS_SNOOP_DISABLE |
-		PCIE_CORE_DEV_CTRL_STATS_MAX_RD_REQ_SIZE_SHIFT;
+		(PCIE_CORE_DEV_CTRL_STATS_MAX_RD_REQ_SZ <<
+		 PCIE_CORE_DEV_CTRL_STATS_MAX_RD_REQ_SIZE_SHIFT);
 	advk_writel(pcie, reg, PCIE_CORE_DEV_CTRL_STATS_REG);
 
 	/* Program PCIe Control 2 to disable strict ordering */
-- 
2.13.5

[PATCH v2 7/7] PCI: aardvark: define IRQ related hooks in pci_host_bridge

[Thomas Petazzoni]

Commit 769b461fc0c0 ("arm64: PCI: Drop DT IRQ allocation from
pcibios_alloc_irq()") was assuming all PCI host controller drivers had
been converted to use ->map_irq(), but that wasn't the case:
pci-aardvark had not been converted. Due to this, it broke the support
for legacy PCI interrupts when using the pci-aardvark driver (used on
Marvell Armada 3720 platforms).

In order to fix this, we make sure the ->map_irq and ->swizzle_irq
fields of pci_host_bridge are properly filled in.

Fixes: 769b461fc0c0 ("arm64: PCI: Drop DT IRQ allocation from pcibios_alloc_irq()")
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: <stable@vger.kernel.org> # v4.13+
---
 drivers/pci/host/pci-aardvark.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
index c1093b023e48..249a088b463c 100644
--- a/drivers/pci/host/pci-aardvark.c
+++ b/drivers/pci/host/pci-aardvark.c
@@ -998,6 +998,8 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	bridge->sysdata = pcie;
 	bridge->busnr = 0;
 	bridge->ops = &advk_pcie_ops;
+	bridge->map_irq = of_irq_parse_and_map_pci;
+	bridge->swizzle_irq = pci_common_swizzle;
 
 	ret = pci_scan_root_bus_bridge(bridge);
 	if (ret < 0) {
-- 
2.13.5

Re: [PATCH v2 7/7] PCI: aardvark: define IRQ related hooks in pci_host_bridge

[Bjorn Helgaas]

[+cc Lorenzo]

On Thu, Sep 28, 2017 at 02:58:38PM +0200, Thomas Petazzoni wrote:
> Commit 769b461fc0c0 ("arm64: PCI: Drop DT IRQ allocation from
> pcibios_alloc_irq()") was assuming all PCI host controller drivers had
> been converted to use ->map_irq(), but that wasn't the case:
> pci-aardvark had not been converted. Due to this, it broke the support
> for legacy PCI interrupts when using the pci-aardvark driver (used on
> Marvell Armada 3720 platforms).

Lorenzo is pretty thorough, but maybe pci-aardvark.c got overlooked.
Cc'ing him just to make sure there's nothing deeper than that.

> In order to fix this, we make sure the ->map_irq and ->swizzle_irq
> fields of pci_host_bridge are properly filled in.
> 
> Fixes: 769b461fc0c0 ("arm64: PCI: Drop DT IRQ allocation from pcibios_alloc_irq()")
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: <stable@vger.kernel.org> # v4.13+
> ---
>  drivers/pci/host/pci-aardvark.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
> index c1093b023e48..249a088b463c 100644
> --- a/drivers/pci/host/pci-aardvark.c
> +++ b/drivers/pci/host/pci-aardvark.c
> @@ -998,6 +998,8 @@ static int advk_pcie_probe(struct platform_device *pdev)
>  	bridge->sysdata = pcie;
>  	bridge->busnr = 0;
>  	bridge->ops = &advk_pcie_ops;
> +	bridge->map_irq = of_irq_parse_and_map_pci;
> +	bridge->swizzle_irq = pci_common_swizzle;
>  
>  	ret = pci_scan_root_bus_bridge(bridge);
>  	if (ret < 0) {
> -- 
> 2.13.5
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 7/7] PCI: aardvark: define IRQ related hooks in pci_host_bridge

[Thomas Petazzoni]

Hello,

On Thu, 5 Oct 2017 12:55:47 -0500, Bjorn Helgaas wrote:

> On Thu, Sep 28, 2017 at 02:58:38PM +0200, Thomas Petazzoni wrote:
> > Commit 769b461fc0c0 ("arm64: PCI: Drop DT IRQ allocation from
> > pcibios_alloc_irq()") was assuming all PCI host controller drivers had
> > been converted to use ->map_irq(), but that wasn't the case:
> > pci-aardvark had not been converted. Due to this, it broke the support
> > for legacy PCI interrupts when using the pci-aardvark driver (used on
> > Marvell Armada 3720 platforms).  
> 
> Lorenzo is pretty thorough, but maybe pci-aardvark.c got overlooked.
> Cc'ing him just to make sure there's nothing deeper than that.

I talked about this issue with Lorenzo on IRC and then briefly in real
life at Linux Plumbers, and I'm pretty sure he agreed that it was just
due to the Aardvark driver having been overlooked.

But I'll let him confirm.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 0/7] PCI: aardvark: improve compatibility with PCI devices

[Thomas Petazzoni]

Hello Bjorn,

On Thu, 28 Sep 2017 14:58:31 +0200, Thomas Petazzoni wrote:

> This patch series brings a number of fixes to the pci-aardvark driver
> that allows a much larger number of PCIe devices to be used.

I sent the initial version of this patch series almost a month ago, and
it consists of fixes that I would like to have in 4.14.

Is there a specific problem with those patches that explains why they
have been ignored? Or is it just lack of time?

If there is any problem with the patches, please let me know, I am of
course perfectly fine with reworking them as needed.

Thanks a lot,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 0/7] PCI: aardvark: improve compatibility with PCI devices

[Bjorn Helgaas]

[+cc Lorenzo]

On Thu, Oct 05, 2017 at 05:53:10PM +0200, Thomas Petazzoni wrote:
> Hello Bjorn,
> 
> On Thu, 28 Sep 2017 14:58:31 +0200, Thomas Petazzoni wrote:
> 
> > This patch series brings a number of fixes to the pci-aardvark driver
> > that allows a much larger number of PCIe devices to be used.
> 
> I sent the initial version of this patch series almost a month ago, and
> it consists of fixes that I would like to have in 4.14.

The general rule is that after the merge window, I merge fixes to
things we put in during the merge window, as well as important
regression fixes.  Most bug fixes will be queued for the next merge
window.  I'll need some guidance on classifying these.

I think the map_irq/swizzle_irq patch should definitely be in v4.14.
(It looks a lot like these:

  1ee4d93d5037 PCI: xilinx-nwl: Move to struct pci_host_bridge IRQ mapping functions
  5a3dc3c1f694 PCI: rockchip: Move to struct pci_host_bridge IRQ mapping functions
  c62e98bdaa70 PCI: xgene: Move to struct pci_host_bridge IRQ mapping functions
  6ab380957838 PCI: altera: Drop pci_fixup_irqs()
  cf60374de8f6 PCI: versatile: Drop pci_fixup_irqs()
  6982a068aa5f PCI: generic: Drop pci_fixup_irqs()
  f7c2e69b65fe PCI: faraday: Drop pci_fixup_irqs()
  60eca198b1ea PCI: designware: Drop pci_fixup_irqs()
  64bcd00a7ef5 PCI: iproc: Drop pci_fixup_irqs()
  29db991902ec PCI: rcar: Drop pci_fixup_irqs()
  cc2eaaef63df PCI: xilinx: Drop pci_fixup_irqs()
  dd5fcce2a7f9 PCI: tegra: Drop pci_fixup_irqs()

and I'm obsessive enough to use one of those subject lines to tie this
patch together with those.)

Most of the rest look like they've been there since the driver was
first merged, so they would *probably* go in the v4.15 queue.

> Is there a specific problem with those patches that explains why they
> have been ignored? Or is it just lack of time?
> 
> If there is any problem with the patches, please let me know, I am of
> course perfectly fine with reworking them as needed.

Sorry for the delay; mostly just lack of time.  I used to work pretty
strictly first-in, first-out, but the native host bridge drivers
consume a disproportionate share of my time compared with the generic
code that benefits everybody, so I'm trying to figure out how to
prioritize generic changes.  Obviously I need a solution that gives
*some* time to the native drivers.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/7] PCI: aardvark: improve compatibility with PCI devices

[Thomas Petazzoni]

Hello,

On Thu, 5 Oct 2017 13:16:17 -0500, Bjorn Helgaas wrote:

> The general rule is that after the merge window, I merge fixes to
> things we put in during the merge window, as well as important
> regression fixes.  Most bug fixes will be queued for the next merge
> window.  I'll need some guidance on classifying these.
> 
> I think the map_irq/swizzle_irq patch should definitely be in v4.14.
> (It looks a lot like these:
> 
>   1ee4d93d5037 PCI: xilinx-nwl: Move to struct pci_host_bridge IRQ mapping functions
>   5a3dc3c1f694 PCI: rockchip: Move to struct pci_host_bridge IRQ mapping functions
>   c62e98bdaa70 PCI: xgene: Move to struct pci_host_bridge IRQ mapping functions
>   6ab380957838 PCI: altera: Drop pci_fixup_irqs()
>   cf60374de8f6 PCI: versatile: Drop pci_fixup_irqs()
>   6982a068aa5f PCI: generic: Drop pci_fixup_irqs()
>   f7c2e69b65fe PCI: faraday: Drop pci_fixup_irqs()
>   60eca198b1ea PCI: designware: Drop pci_fixup_irqs()
>   64bcd00a7ef5 PCI: iproc: Drop pci_fixup_irqs()
>   29db991902ec PCI: rcar: Drop pci_fixup_irqs()
>   cc2eaaef63df PCI: xilinx: Drop pci_fixup_irqs()
>   dd5fcce2a7f9 PCI: tegra: Drop pci_fixup_irqs()
> 
> and I'm obsessive enough to use one of those subject lines to tie this
> patch together with those.)

Fine, I'll adjust the commit title to be "PCI: aardvark: Move to struct
pci_host_bridge IRQ mapping functions". I also find it nice when commit
titles are very consistent, so I can only agree with your obsessiveness
on this!

> Most of the rest look like they've been there since the driver was
> first merged, so they would *probably* go in the v4.15 queue.

I agree that the other patches do not fix regressions but bugs. So it's
really up to you as to what you consider a "fix". The Aardvark driver
in its current form leaves a lot of PCIe devices unusable, and we get
bug reports about this. But admittedly, such PCIe devices have never
worked with Aardvark.

> Sorry for the delay; mostly just lack of time.  I used to work pretty
> strictly first-in, first-out, but the native host bridge drivers
> consume a disproportionate share of my time compared with the generic
> code that benefits everybody, so I'm trying to figure out how to
> prioritize generic changes.  Obviously I need a solution that gives
> *some* time to the native drivers.

No problem. I do understand that reviewing all of those native drivers
takes a significant amount of time.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 0/7] PCI: aardvark: improve compatibility with PCI devices

[Lorenzo Pieralisi]

On Thu, Oct 05, 2017 at 01:16:17PM -0500, Bjorn Helgaas wrote:
> [+cc Lorenzo]
> 
> On Thu, Oct 05, 2017 at 05:53:10PM +0200, Thomas Petazzoni wrote:
> > Hello Bjorn,
> > 
> > On Thu, 28 Sep 2017 14:58:31 +0200, Thomas Petazzoni wrote:
> > 
> > > This patch series brings a number of fixes to the pci-aardvark driver
> > > that allows a much larger number of PCIe devices to be used.
> > 
> > I sent the initial version of this patch series almost a month ago, and
> > it consists of fixes that I would like to have in 4.14.
> 
> The general rule is that after the merge window, I merge fixes to
> things we put in during the merge window, as well as important
> regression fixes.  Most bug fixes will be queued for the next merge
> window.  I'll need some guidance on classifying these.
> 
> I think the map_irq/swizzle_irq patch should definitely be in v4.14.

Yes it is v4.14 (actually v4.13 - Fixes: tag will cover that) material,
I missed updating this host bridge while patching all ARM host controller
bridges, apologies.

Thanks,
Lorenzo

> (It looks a lot like these:
> 
>   1ee4d93d5037 PCI: xilinx-nwl: Move to struct pci_host_bridge IRQ mapping functions
>   5a3dc3c1f694 PCI: rockchip: Move to struct pci_host_bridge IRQ mapping functions
>   c62e98bdaa70 PCI: xgene: Move to struct pci_host_bridge IRQ mapping functions
>   6ab380957838 PCI: altera: Drop pci_fixup_irqs()
>   cf60374de8f6 PCI: versatile: Drop pci_fixup_irqs()
>   6982a068aa5f PCI: generic: Drop pci_fixup_irqs()
>   f7c2e69b65fe PCI: faraday: Drop pci_fixup_irqs()
>   60eca198b1ea PCI: designware: Drop pci_fixup_irqs()
>   64bcd00a7ef5 PCI: iproc: Drop pci_fixup_irqs()
>   29db991902ec PCI: rcar: Drop pci_fixup_irqs()
>   cc2eaaef63df PCI: xilinx: Drop pci_fixup_irqs()
>   dd5fcce2a7f9 PCI: tegra: Drop pci_fixup_irqs()
> 
> and I'm obsessive enough to use one of those subject lines to tie this
> patch together with those.)
> 
> Most of the rest look like they've been there since the driver was
> first merged, so they would *probably* go in the v4.15 queue.
> 
> > Is there a specific problem with those patches that explains why they
> > have been ignored? Or is it just lack of time?
> > 
> > If there is any problem with the patches, please let me know, I am of
> > course perfectly fine with reworking them as needed.
> 
> Sorry for the delay; mostly just lack of time.  I used to work pretty
> strictly first-in, first-out, but the native host bridge drivers
> consume a disproportionate share of my time compared with the generic
> code that benefits everybody, so I'm trying to figure out how to
> prioritize generic changes.  Obviously I need a solution that gives
> *some* time to the native drivers.
> 
> Bjorn