[RFT PATCH v2 02/42] drivers: pci: host: iproc: Convert link check to raw PCI config accessors

Oza Oza oza.oza at broadcom.com
Mon Jun 12 10:40:03 PDT 2017


On Mon, Jun 12, 2017 at 9:43 PM, Lorenzo Pieralisi
<lorenzo.pieralisi at arm.com> wrote:
> On Sun, Jun 11, 2017 at 09:42:34AM +0530, Oza Oza wrote:
>> On Thu, Jun 8, 2017 at 10:06 PM, Lorenzo Pieralisi
>> <lorenzo.pieralisi at arm.com> wrote:
>> > [dropped rock-chips maintainers, email bounces]
>> >
>> > On Thu, Jun 08, 2017 at 08:56:05AM -0700, Ray Jui wrote:
>> >> Hi Lorenzo,
>> >>
>> >> Thanks, I'll try my best to find time to test this along 15/42 and
>> >> 33/42 patches. Hopefully I can get to that some time next week.
>> >>
>> >> I have not yet reviewed these patches in details. Do they have
>> >> dependency on other patches to the generic framework code you
>> >> changed?
>> >>
>> >> If so, is there a repo I can pull them?
>> >
>> > I added it in the cover letter but anyway here it is:
>> >
>> > git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git pci/pci-fixup-irqs-removal-v2
>>
>> Hi Lorenzo,
>>
>> I picked up your changes, and tested on iproc based SOC,
>> and ran basic fio data transfer.. and it looks okay.
>
> Thank you. If you could also check it is all OK from a legacy
> IRQ allocation (ie same as before applying series) I'd be grateful.
>

I disabled msi and it should default to legacy IRQ, looks like there
is a problem, if I missed any change !

before applying series:  cat /proc/interrupts
377:        168          0          0          0          0          0
         0          0     dummy   1 Edge      nvme0q0, nvme0q1

after applying series:
root at bcm958742k:~# dmesg | grep nvme
[    3.855466] nvme 0000:01:00.0: assign irq: got 0
[    3.855469] nvme 0000:01:00.0: assigning IRQ 00
[    3.855515] nvme nvme0: pci function 0000:01:00.0
[    4.270850] nvme 0000:01:00.0: enabling device (0000 -> 0002)
[    4.276787] nvme 0000:01:00.0: enabling bus mastering
[    4.276817] nvme nvme0: Removing after probe failure status: -22

here is my git status of your series, let me know if I am missing any
change with respect to legacy IRQ ?

 modified:   arch/arm/include/asm/mach/pci.h
        modified:   arch/arm/kernel/bios32.c
        modified:   arch/arm/mach-dove/pcie.c
        modified:   arch/arm/mach-iop13xx/pci.c
        modified:   arch/arm/mach-iop13xx/pci.h
        modified:   arch/arm/mach-mv78xx0/pcie.c
        modified:   arch/arm/mach-orion5x/common.h
        modified:   arch/arm/mach-orion5x/pci.c
        modified:   arch/arm64/kernel/pci.c
        modified:   drivers/of/of_pci_irq.c
        modified:   drivers/pci/Makefile
        modified:   drivers/pci/host/pci-aardvark.c
        modified:   drivers/pci/host/pci-ftpci100.c
        modified:   drivers/pci/host/pci-host-common.c
        modified:   drivers/pci/host/pci-tegra.c
        modified:   drivers/pci/host/pci-versatile.c
        modified:   drivers/pci/host/pci-xgene.c
        modified:   drivers/pci/host/pcie-altera.c
        modified:   drivers/pci/host/pcie-iproc-bcma.c
        modified:   drivers/pci/host/pcie-iproc-platform.c
        modified:   drivers/pci/host/pcie-iproc.c
        modified:   drivers/pci/host/pcie-iproc.h
        modified:   drivers/pci/host/pcie-rcar.c
        modified:   drivers/pci/host/pcie-rockchip.c
        modified:   drivers/pci/host/pcie-xilinx-nwl.c
        modified:   drivers/pci/host/pcie-xilinx.c
        modified:   drivers/pci/pci-driver.c
        modified:   drivers/pci/probe.c
        modified:   drivers/pci/setup-irq.c
        modified:   include/linux/pci.h

> Thanks a lot,
> Lorenzo
>
>> Hi Bjorn,
>>
>> please help to review following changes, to avoid merge conflicts
>> since Lorenzo's changes in iproc driver is touching basic config APIs.
>>
>> [PATCH v3 1/2] PCI: iproc: Retry request when CRS returned from EP
>> [PATCH v3 2/2] PCI: iproc: add device shutdown for PCI RC
>>
>> Regards,
>> Oza.
>>
>>
>>
>> >
>> > Thanks for testing it, please let me know.
>> >
>> > Thank you !
>> > Lorenzo
>> >
>> >>
>> >> Thanks,
>> >>
>> >> Ray
>> >>
>> >>
>> >> On 6/8/2017 7:13 AM, Lorenzo Pieralisi wrote:
>> >> >Current iproc driver host bridge controller driver requires struct
>> >> >pci_bus to be created in order to carry out PCI link checks with standard
>> >> >PCI config space accessors.
>> >> >
>> >> >This struct pci_bus dependency is fictitious and burdens the driver
>> >> >with unneeded constraints (eg to use separate APIs to create and scan
>> >> >the root bus).
>> >> >
>> >> >Add PCI raw config space accessors to PCIe iproc driver and remove the
>> >> >fictitious struct pci_bus dependency.
>> >> >
>> >> >Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi at arm.com>
>> >> >Cc: Scott Branden <sbranden at broadcom.com>
>> >> >Cc: Ray Jui <rjui at broadcom.com>
>> >> >Cc: Jon Mason <jonmason at broadcom.com>
>> >> >---
>> >> >  drivers/pci/host/pcie-iproc.c | 94 ++++++++++++++++++++++++++++++++++---------
>> >> >  1 file changed, 74 insertions(+), 20 deletions(-)
>> >> >
>> >> >diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>> >> >index 0f39bd2..e48b8e2 100644
>> >> >--- a/drivers/pci/host/pcie-iproc.c
>> >> >+++ b/drivers/pci/host/pcie-iproc.c
>> >> >@@ -452,14 +452,13 @@ static inline void iproc_pcie_apb_err_disable(struct pci_bus *bus,
>> >> >   * Note access to the configuration registers are protected at the higher layer
>> >> >   * by 'pci_lock' in drivers/pci/access.c
>> >> >   */
>> >> >-static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
>> >> >+static void __iomem *iproc_pcie_map_cfg_bus(struct iproc_pcie *pcie,
>> >> >+                                        int busno,
>> >> >                                         unsigned int devfn,
>> >> >                                         int where)
>> >> >  {
>> >> >-    struct iproc_pcie *pcie = iproc_data(bus);
>> >> >     unsigned slot = PCI_SLOT(devfn);
>> >> >     unsigned fn = PCI_FUNC(devfn);
>> >> >-    unsigned busno = bus->number;
>> >> >     u32 val;
>> >> >     u16 offset;
>> >> >@@ -499,6 +498,58 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
>> >> >             return (pcie->base + offset);
>> >> >  }
>> >> >+static void __iomem *iproc_pcie_bus_map_cfg_bus(struct pci_bus *bus,
>> >> >+                                            unsigned int devfn,
>> >> >+                                            int where)
>> >> >+{
>> >> >+    return iproc_pcie_map_cfg_bus(iproc_data(bus), bus->number, devfn,
>> >> >+                                  where);
>> >> >+}
>> >> >+
>> >> >+static int iproc_pci_raw_config_read32(struct iproc_pcie *pcie,
>> >> >+                                   unsigned int devfn, int where,
>> >> >+                                   int size, u32 *val)
>> >> >+{
>> >> >+    void __iomem *addr;
>> >> >+
>> >> >+    addr = iproc_pcie_map_cfg_bus(pcie, 0, devfn, where & ~0x3);
>> >> >+    if (!addr) {
>> >> >+            *val = ~0;
>> >> >+            return PCIBIOS_DEVICE_NOT_FOUND;
>> >> >+    }
>> >> >+
>> >> >+    *val = readl(addr);
>> >> >+
>> >> >+    if (size <= 2)
>> >> >+            *val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
>> >> >+
>> >> >+    return PCIBIOS_SUCCESSFUL;
>> >> >+}
>> >> >+
>> >> >+static int iproc_pci_raw_config_write32(struct iproc_pcie *pcie,
>> >> >+                                    unsigned int devfn, int where,
>> >> >+                                    int size, u32 val)
>> >> >+{
>> >> >+    void __iomem *addr;
>> >> >+    u32 mask, tmp;
>> >> >+
>> >> >+    addr = iproc_pcie_map_cfg_bus(pcie, 0, devfn, where & ~0x3);
>> >> >+    if (!addr)
>> >> >+            return PCIBIOS_DEVICE_NOT_FOUND;
>> >> >+
>> >> >+    if (size == 4) {
>> >> >+            writel(val, addr);
>> >> >+            return PCIBIOS_SUCCESSFUL;
>> >> >+    }
>> >> >+
>> >> >+    mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8));
>> >> >+    tmp = readl(addr) & mask;
>> >> >+    tmp |= val << ((where & 0x3) * 8);
>> >> >+    writel(tmp, addr);
>> >> >+
>> >> >+    return PCIBIOS_SUCCESSFUL;
>> >> >+}
>> >> >+
>> >> >  static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>> >> >                                 int where, int size, u32 *val)
>> >> >  {
>> >> >@@ -524,7 +575,7 @@ static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int devfn,
>> >> >  }
>> >> >  static struct pci_ops iproc_pcie_ops = {
>> >> >-    .map_bus = iproc_pcie_map_cfg_bus,
>> >> >+    .map_bus = iproc_pcie_bus_map_cfg_bus,
>> >> >     .read = iproc_pcie_config_read32,
>> >> >     .write = iproc_pcie_config_write32,
>> >> >  };
>> >> >@@ -556,12 +607,11 @@ static void iproc_pcie_reset(struct iproc_pcie *pcie)
>> >> >     msleep(100);
>> >> >  }
>> >> >-static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
>> >> >+static int iproc_pcie_check_link(struct iproc_pcie *pcie)
>> >> >  {
>> >> >     struct device *dev = pcie->dev;
>> >> >-    u8 hdr_type;
>> >> >-    u32 link_ctrl, class, val;
>> >> >-    u16 pos = PCI_EXP_CAP, link_status;
>> >> >+    u32 hdr_type, link_ctrl, link_status, class, val;
>> >> >+    u16 pos = PCI_EXP_CAP;
>> >> >     bool link_is_active = false;
>> >> >     /*
>> >> >@@ -578,7 +628,7 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
>> >> >     }
>> >> >     /* make sure we are not in EP mode */
>> >> >-    pci_bus_read_config_byte(bus, 0, PCI_HEADER_TYPE, &hdr_type);
>> >> >+    iproc_pci_raw_config_read32(pcie,  0, PCI_HEADER_TYPE, 1, &hdr_type);
>> >> >     if ((hdr_type & 0x7f) != PCI_HEADER_TYPE_BRIDGE) {
>> >> >             dev_err(dev, "in EP mode, hdr=%#02x\n", hdr_type);
>> >> >             return -EFAULT;
>> >> >@@ -588,13 +638,16 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
>> >> >  #define PCI_BRIDGE_CTRL_REG_OFFSET 0x43c
>> >> >  #define PCI_CLASS_BRIDGE_MASK      0xffff00
>> >> >  #define PCI_CLASS_BRIDGE_SHIFT     8
>> >> >-    pci_bus_read_config_dword(bus, 0, PCI_BRIDGE_CTRL_REG_OFFSET, &class);
>> >> >+    iproc_pci_raw_config_read32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
>> >> >+                                4, &class);
>> >> >     class &= ~PCI_CLASS_BRIDGE_MASK;
>> >> >     class |= (PCI_CLASS_BRIDGE_PCI << PCI_CLASS_BRIDGE_SHIFT);
>> >> >-    pci_bus_write_config_dword(bus, 0, PCI_BRIDGE_CTRL_REG_OFFSET, class);
>> >> >+    iproc_pci_raw_config_write32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
>> >> >+                                 4, class);
>> >> >     /* check link status to see if link is active */
>> >> >-    pci_bus_read_config_word(bus, 0, pos + PCI_EXP_LNKSTA, &link_status);
>> >> >+    iproc_pci_raw_config_read32(pcie, 0, pos + PCI_EXP_LNKSTA,
>> >> >+                                2, &link_status);
>> >> >     if (link_status & PCI_EXP_LNKSTA_NLW)
>> >> >             link_is_active = true;
>> >> >@@ -603,20 +656,21 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
>> >> >  #define PCI_TARGET_LINK_SPEED_MASK    0xf
>> >> >  #define PCI_TARGET_LINK_SPEED_GEN2    0x2
>> >> >  #define PCI_TARGET_LINK_SPEED_GEN1    0x1
>> >> >-            pci_bus_read_config_dword(bus, 0,
>> >> >-                                      pos + PCI_EXP_LNKCTL2,
>> >> >+            iproc_pci_raw_config_read32(pcie, 0,
>> >> >+                                      pos + PCI_EXP_LNKCTL2, 4,
>> >> >                                       &link_ctrl);
>> >> >             if ((link_ctrl & PCI_TARGET_LINK_SPEED_MASK) ==
>> >> >                 PCI_TARGET_LINK_SPEED_GEN2) {
>> >> >                     link_ctrl &= ~PCI_TARGET_LINK_SPEED_MASK;
>> >> >                     link_ctrl |= PCI_TARGET_LINK_SPEED_GEN1;
>> >> >-                    pci_bus_write_config_dword(bus, 0,
>> >> >-                                       pos + PCI_EXP_LNKCTL2,
>> >> >-                                       link_ctrl);
>> >> >+                    iproc_pci_raw_config_write32(pcie, 0,
>> >> >+                                            pos + PCI_EXP_LNKCTL2,
>> >> >+                                            4, link_ctrl);
>> >> >                     msleep(100);
>> >> >-                    pci_bus_read_config_word(bus, 0, pos + PCI_EXP_LNKSTA,
>> >> >-                                             &link_status);
>> >> >+                    iproc_pci_raw_config_read32(pcie, 0,
>> >> >+                                            pos + PCI_EXP_LNKSTA,
>> >> >+                                            2, &link_status);
>> >> >                     if (link_status & PCI_EXP_LNKSTA_NLW)
>> >> >                             link_is_active = true;
>> >> >             }
>> >> >@@ -1260,7 +1314,7 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>> >> >     }
>> >> >     pcie->root_bus = bus;
>> >> >-    ret = iproc_pcie_check_link(pcie, bus);
>> >> >+    ret = iproc_pcie_check_link(pcie);
>> >> >     if (ret) {
>> >> >             dev_err(dev, "no PCIe EP device detected\n");
>> >> >             goto err_rm_root_bus;
>> >>
>> >
>> > _______________________________________________
>> > linux-arm-kernel mailing list
>> > linux-arm-kernel at lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



More information about the linux-arm-kernel mailing list