adding PCI support to AXS10x

This patch adds PCI support to ARC and updates drivers/pci Makefile enabling the ARC arch to use the generic PCI setup functions.

Signed-off-by Joao Pinto <jpinto at synopsys.com>

This patch adds a new driver that will be the reference platform driver for all PCI RC IP Protoyping Kits based on ARC SDP.
This patch is composed by:

(quoted diff) +0 -2

-MAINTAINERS file was updated to include the new driver
-Documentation/devicetree/bindings/pci was updated to include the new

driver documentation
-New driver called pcie-synopsys

Signed-off-by Joao Pinto <jpinto at synopsys.com>

This patch adds a new driver that will be the reference platform driver for all PCI RC IP Protoyping Kits based on ARC SDP.
This patch is composed by:

(quoted diff) +0 -2

-MAINTAINERS file was updated to include the new driver
-Documentation/devicetree/bindings/pci was updated to include the new

driver documentation
-New driver called pcie-synopsys

Signed-off-by Joao Pinto <jpinto at synopsys.com>

+- reg: base address and length of the pcie controller registers.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions.
+- interrupts: one interrupt source for MSI interrupts, followed by interrupt

Documentation/devicetree/bindings/pci/pcie-synopsys.txt new file +3 -0

+	source for hardware related interrupts.
+- #interrupt-cells: set to <1>
+- num-lanes: set to <1>;

drivers/pci/host/pcie-synopsys.c line 87

Quoted 13 lines · On Monday 01 February 2016 18:07:45 Joao Pinto wrote:

drivers/pci/host/pcie-synopsys.c new file +13 -0

+static void synopsys_pcie_establish_link(struct pcie_port *pp)
+{
+	int retries = 0;
+
+	/* check if the link is up or not */
+	for (retries = 0; retries < 10; retries++) {
+		if (dw_pcie_link_up(pp)) {
+			dev_info(pp->dev, "Link up\n");
+			return;
+		}
+		mdelay(100);
+	}
+}

That mdelay() needs to be an msleep(). You should never waste a whole second worth of CPU time in a driver.

Quoted 17 lines · Arnd Bergmann

drivers/pci/host/pcie-synopsys.c line 87

Quoted 13 lines · On Monday 01 February 2016 18:07:45 Joao Pinto wrote:

drivers/pci/host/pcie-synopsys.c new file +13 -0

+static void synopsys_pcie_establish_link(struct pcie_port *pp)
+{
+   int retries = 0;
+
+   /* check if the link is up or not */
+   for (retries = 0; retries < 10; retries++) {
+           if (dw_pcie_link_up(pp)) {
+                   dev_info(pp->dev, "Link up\n");
+                   return;
+           }
+           mdelay(100);
+   }
+}

That mdelay() needs to be an msleep(). You should never waste a whole second worth of CPU time in a driver.

There are six other copies of this code, and two of them (exynos and spear) have the same problem.

Quoted 9 lines · On Monday 01 February 2016 18:07:45 Joao Pinto wrote:

This patch adds a new driver that will be the reference platform driver for all PCI RC IP Protoyping Kits based on ARC SDP.
This patch is composed by:

(quoted diff) +0 -2

-MAINTAINERS file was updated to include the new driver
-Documentation/devicetree/bindings/pci was updated to include the new

driver documentation
-New driver called pcie-synopsys

Quoted 1 line

Signed-off-by Joao Pinto <[email protected]>

I must have completely missed all the earlier submissions and just happened to see this one now that it was merged.

I have a couple of comments, maybe you can send a follow up patch to address them:

Documentation/devicetree/bindings/pci/pcie-synopsys.txt line 8

Quoted 7 lines

+----------------------------------

Documentation/devicetree/bindings/pci/pcie-synopsys.txt new file +6 -0

+
+This is the reference platform driver to be used in the Synopsys PCI Root
+Complex IP Prototyping Kit.
+
+Required properties:
+- compatible: set to "snps,pcie-synopsys";

Please also include the exact version of the designware PCIe part that is used in here, such as "snps,dw-pcie-1.234". I'm sure you can find out the version.

"snps,pcie-synopsys" by itself is about the worst possible compatible string because it says nothing about the specific hardware. Half the machines we support all have a designware PCIe host that was made by synopsys. Please use the name of the SoC for the most specific string.

Quoted 9 lines

+- reg: base address and length of the pcie controller registers.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions.
+- interrupts: one interrupt source for MSI interrupts, followed by interrupt

Documentation/devicetree/bindings/pci/pcie-synopsys.txt new file +3 -0

+	source for hardware related interrupts.
+- #interrupt-cells: set to <1>
+- num-lanes: set to <1>;

Documentation/devicetree/bindings/pci/pcie-synopsys.txt line 27

Quoted 10 lines

Documentation/devicetree/bindings/pci/pcie-synopsys.txt new file +10 -0

+
+Example configuration:
+
+	pcie: pcie@0xdffff000 {
+		compatible = "snps,pcie-synopsys";
+		reg = <0xdffff000 0x1000>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		ranges = <0x00000800 0 0xd0000000 0xd0000000 0 0x00002000>,

This line looks misplaced here and does not match the documentation. It's probably a leftover from an earlier version that had the config space in the ranges, so you need to remove that.

drivers/pci/host/pcie-synopsys.c line 53

Quoted 5 lines

drivers/pci/host/pcie-synopsys.c new file +5 -0

+/* This handler was created for future work */
+static irqreturn_t synopsys_pcie_irq_handler(int irq, void *arg)
+{
+	return IRQ_NONE;
+}

It's not harmful, but we generally don't add code "for future work", better remove it for now.

drivers/pci/host/pcie-synopsys.c line 87

Quoted 13 lines

drivers/pci/host/pcie-synopsys.c new file +13 -0

+static void synopsys_pcie_establish_link(struct pcie_port *pp)
+{
+	int retries = 0;
+
+	/* check if the link is up or not */
+	for (retries = 0; retries < 10; retries++) {
+		if (dw_pcie_link_up(pp)) {
+			dev_info(pp->dev, "Link up\n");
+			return;
+		}
+		mdelay(100);
+	}
+}

That mdelay() needs to be an msleep(). You should never waste a whole second worth of CPU time in a driver.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Joao & Arnd,

Quoted 10 lines · Arnd Bergmann

Quoted 9 lines · On Monday 01 February 2016 18:07:45 Joao Pinto wrote:

This patch adds a new driver that will be the reference platform driver for all PCI RC IP Protoyping Kits based on ARC SDP.
This patch is composed by:

(quoted diff) +0 -2

-MAINTAINERS file was updated to include the new driver
-Documentation/devicetree/bindings/pci was updated to include the new

driver documentation
-New driver called pcie-synopsys

Quoted 7 lines

Quoted 1 line

Signed-off-by Joao Pinto <[email protected]>

I must have completely missed all the earlier submissions and just happened to see this one now that it was merged.

I have a couple of comments, maybe you can send a follow up patch to address them:

These seem pretty easy to address, so I pulled these patches out of for-linus temporarily while we fix them.

Joao, you can just fix the mdelay() in your driver; don't worry about fixing other drivers with the same problem or consolidating them.

Bjorn

Quoted 73 lines

Documentation/devicetree/bindings/pci/pcie-synopsys.txt line 8

Quoted 7 lines

+----------------------------------

Documentation/devicetree/bindings/pci/pcie-synopsys.txt new file +6 -0

+
+This is the reference platform driver to be used in the Synopsys PCI Root
+Complex IP Prototyping Kit.
+
+Required properties:
+- compatible: set to "snps,pcie-synopsys";

Please also include the exact version of the designware PCIe part that is used in here, such as "snps,dw-pcie-1.234". I'm sure you can find out the version.

"snps,pcie-synopsys" by itself is about the worst possible compatible string because it says nothing about the specific hardware. Half the machines we support all have a designware PCIe host that was made by synopsys. Please use the name of the SoC for the most specific string.

Quoted 9 lines

+- reg: base address and length of the pcie controller registers.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions.
+- interrupts: one interrupt source for MSI interrupts, followed by interrupt

Documentation/devicetree/bindings/pci/pcie-synopsys.txt new file +3 -0

+	source for hardware related interrupts.
+- #interrupt-cells: set to <1>
+- num-lanes: set to <1>;

Documentation/devicetree/bindings/pci/pcie-synopsys.txt line 27

Quoted 10 lines

Documentation/devicetree/bindings/pci/pcie-synopsys.txt new file +10 -0

+
+Example configuration:
+
+	pcie: pcie@0xdffff000 {
+		compatible = "snps,pcie-synopsys";
+		reg = <0xdffff000 0x1000>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		ranges = <0x00000800 0 0xd0000000 0xd0000000 0 0x00002000>,

This line looks misplaced here and does not match the documentation. It's probably a leftover from an earlier version that had the config space in the ranges, so you need to remove that.

drivers/pci/host/pcie-synopsys.c line 53

Quoted 5 lines

drivers/pci/host/pcie-synopsys.c new file +5 -0

+/* This handler was created for future work */
+static irqreturn_t synopsys_pcie_irq_handler(int irq, void *arg)
+{
+	return IRQ_NONE;
+}

It's not harmful, but we generally don't add code "for future work", better remove it for now.

drivers/pci/host/pcie-synopsys.c line 87

Quoted 13 lines

drivers/pci/host/pcie-synopsys.c new file +13 -0

+static void synopsys_pcie_establish_link(struct pcie_port *pp)
+{
+	int retries = 0;
+
+	/* check if the link is up or not */
+	for (retries = 0; retries < 10; retries++) {
+		if (dw_pcie_link_up(pp)) {
+			dev_info(pp->dev, "Link up\n");
+			return;
+		}
+		mdelay(100);
+	}
+}

That mdelay() needs to be an msleep(). You should never waste a whole second worth of CPU time in a driver.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Bjorn,

Are these tasks enough?

- replace mdelay() for msleep()
- remove synopsys_pcie_irq_handler()

- replace "snps,pcie-synopsys" for "snps,pcie-synopsys-ipk"?

- rename the driver to pcie-synopsys-ipk?

- update the devicetree documentation referring that the ranges also include the config space

Quoted 102 lines

Hi Joao & Arnd,

Quoted 10 lines · Arnd Bergmann

Quoted 9 lines · On Monday 01 February 2016 18:07:45 Joao Pinto wrote:

This patch adds a new driver that will be the reference platform driver for all PCI RC IP Protoyping Kits based on ARC SDP.
This patch is composed by:

(quoted diff) +0 -2

-MAINTAINERS file was updated to include the new driver
-Documentation/devicetree/bindings/pci was updated to include the new

driver documentation
-New driver called pcie-synopsys

Quoted 7 lines

Quoted 1 line

Signed-off-by Joao Pinto <jpinto at synopsys.com>

I must have completely missed all the earlier submissions and just happened to see this one now that it was merged.

I have a couple of comments, maybe you can send a follow up patch to address them:

These seem pretty easy to address, so I pulled these patches out of for-linus temporarily while we fix them.

Joao, you can just fix the mdelay() in your driver; don't worry about fixing other drivers with the same problem or consolidating them.

Bjorn

Quoted 73 lines

Documentation/devicetree/bindings/pci/pcie-synopsys.txt line 8

Quoted 7 lines

+----------------------------------

Documentation/devicetree/bindings/pci/pcie-synopsys.txt new file +6 -0

+
+This is the reference platform driver to be used in the Synopsys PCI Root
+Complex IP Prototyping Kit.
+
+Required properties:
+- compatible: set to "snps,pcie-synopsys";

Please also include the exact version of the designware PCIe part that is used in here, such as "snps,dw-pcie-1.234". I'm sure you can find out the version.

"snps,pcie-synopsys" by itself is about the worst possible compatible string because it says nothing about the specific hardware. Half the machines we support all have a designware PCIe host that was made by synopsys. Please use the name of the SoC for the most specific string.

Quoted 9 lines

+- reg: base address and length of the pcie controller registers.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions.
+- interrupts: one interrupt source for MSI interrupts, followed by interrupt

Documentation/devicetree/bindings/pci/pcie-synopsys.txt new file +3 -0

+	source for hardware related interrupts.
+- #interrupt-cells: set to <1>
+- num-lanes: set to <1>;

Documentation/devicetree/bindings/pci/pcie-synopsys.txt line 27

Quoted 10 lines

Documentation/devicetree/bindings/pci/pcie-synopsys.txt new file +10 -0

+
+Example configuration:
+
+	pcie: pcie at 0xdffff000 {
+		compatible = "snps,pcie-synopsys";
+		reg = <0xdffff000 0x1000>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		ranges = <0x00000800 0 0xd0000000 0xd0000000 0 0x00002000>,

This line looks misplaced here and does not match the documentation. It's probably a leftover from an earlier version that had the config space in the ranges, so you need to remove that.

drivers/pci/host/pcie-synopsys.c line 53

Quoted 5 lines

drivers/pci/host/pcie-synopsys.c new file +5 -0

+/* This handler was created for future work */
+static irqreturn_t synopsys_pcie_irq_handler(int irq, void *arg)
+{
+	return IRQ_NONE;
+}

It's not harmful, but we generally don't add code "for future work", better remove it for now.

drivers/pci/host/pcie-synopsys.c line 87

Quoted 13 lines

drivers/pci/host/pcie-synopsys.c new file +13 -0

+static void synopsys_pcie_establish_link(struct pcie_port *pp)
+{
+	int retries = 0;
+
+	/* check if the link is up or not */
+	for (retries = 0; retries < 10; retries++) {
+		if (dw_pcie_link_up(pp)) {
+			dev_info(pp->dev, "Link up\n");
+			return;
+		}
+		mdelay(100);
+	}
+}

That mdelay() needs to be an msleep(). You should never waste a whole second worth of CPU time in a driver.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 1 line · Joao Pinto

- replace "snps,pcie-synopsys" for "snps,pcie-synopsys-ipk"?

This is a question for Arnd.

Quoted 1 line

- rename the driver to pcie-synopsys-ipk?

It doesn't seem necessary to me to include both "synopsys" and "ipk" in the filename and the driver name. Take a look at what the existing drivers do, and do something similar.

Quoted 2 lines

- update the devicetree documentation referring that the ranges also include the config space

Another one for Arnd.

Quoted 11 lines · On Wednesday 03 February 2016 12:38:44 Bjorn Helgaas wrote:

Quoted 1 line · Joao Pinto

- replace "snps,pcie-synopsys" for "snps,pcie-synopsys-ipk"?

This is a question for Arnd.

Quoted 1 line

- rename the driver to pcie-synopsys-ipk?

It doesn't seem necessary to me to include both "synopsys" and "ipk" in the filename and the driver name. Take a look at what the existing drivers do, and do something similar.

The "synopsys" can go away, it's already in the vendor field of the string. "ipk" is still a bit unspecific, I was hoping to see a specific chip and/or version of the PCIe part. Something like

	compatible = "snps,ipk2040-pcie", "snps,ipk-pcie", "snps,dw-pcie-1.23", "snps,dw-pcie";

which would indicate that there is a chip called "ipk2040" in a family called "ipk", and this includes the designware pcie implementation in version 1.23.

Quoted 4 lines

Quoted 2 lines

- update the devicetree documentation referring that the ranges also include the config space

Another one for Arnd.

This one is wrong, the ranges should *not* include the config space, and if they currently do, you must change the driver. The generic dw-pcie driver still accepts the config space in the ranges for backwards compatibility with some of the earlier front-ends that mistakenly did this, but new driver should not do the same, and we should probably add some code in the common driver to prevent it for front-ends other than the ones we have to keep compatibility with.

Arnd

Quoted 4 lines

The "synopsys" can go away, it's already in the vendor field of the string. "ipk" is still a bit unspecific, I was hoping to see a specific chip and/or version of the PCIe part. Something like

Quoted 5 lines

      compatible = "snps,ipk2040-pcie", "snps,ipk-pcie", "snps,dw-pcie-1.23", "snps,dw-pcie";

which would indicate that there is a chip called "ipk2040" in a family called "ipk", and this includes the designware pcie implementation in version 1.23.

"snps,dw-pcie" seems a good idea!

Quoted 6 lines · Joao Pinto

Hi Bjorn,

Are these tasks enough?

- replace mdelay() for msleep()
- remove synopsys_pcie_irq_handler()

Above are fine with me.

Hi Bjorn,

Quoted 9 lines · Bjorn Helgaas

Quoted 6 lines · Joao Pinto

Hi Bjorn,

Are these tasks enough?

- replace mdelay() for msleep()
- remove synopsys_pcie_irq_handler()

Above are fine with me.

Ok, I'll to that.

Thanks.

Hi Bjorn and Arnd,

Removing the irq_handler causes the irq request to fail because in request_threaded_irq() both handler and thread_fn are NULL.

What's the typical procedure for this?

This patch set has the goal to add suppport for DesignWare PCIe RC in ARC AXS10x. It includes the necessary tweaks to the ARC architecture, necessary tweaks to the PCI subsystem and a new driver (pcie-synopsys). This new driver will be used extensively in the PCIe RC Prototyping Kit.

The patches were produced against Bjorn Helgaas' repository. It was properly tested in an IP Prototyping Kit.

Joao Pinto (2):
PCI support added to ARC
add new platform driver for PCI RC

 .../devicetree/bindings/pci/pcie-synopsys.txt      |  33 +++
 MAINTAINERS                                        |   7 +
 arch/arc/Kconfig                                   |  23 ++
 arch/arc/include/asm/dma.h                         |   5 +
 arch/arc/include/asm/io.h                          |   9 +
 arch/arc/include/asm/pci.h                         |  31 +++
 arch/arc/kernel/Makefile                           |   1 +
 arch/arc/kernel/pcibios.c                          |  23 ++
 arch/arc/plat-axs10x/Kconfig                       |   1 +
 drivers/pci/Makefile                               |   1 +

 drivers/pci/host/Kconfig                           |   8 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pcie-synopsys.c                   | 257 +++++++++++++++++++++
 13 files changed, 400 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-synopsys.txt
 create mode 100644 arch/arc/include/asm/pci.h
 create mode 100644 arch/arc/kernel/pcibios.c
 create mode 100644 drivers/pci/host/pcie-synopsys.c

Quoted 22 lines · Joao Pinto

This patch set has the goal to add suppport for DesignWare PCIe RC in ARC AXS10x. It includes the necessary tweaks to the ARC architecture, necessary tweaks to the PCI subsystem and a new driver (pcie-synopsys). This new driver will be used extensively in the PCIe RC Prototyping Kit.

The patches were produced against Bjorn Helgaas' repository. It was properly tested in an IP Prototyping Kit.

Joao Pinto (2):
PCI support added to ARC
add new platform driver for PCI RC

 .../devicetree/bindings/pci/pcie-synopsys.txt      |  33 +++
 MAINTAINERS                                        |   7 +
 arch/arc/Kconfig                                   |  23 ++
 arch/arc/include/asm/dma.h                         |   5 +
 arch/arc/include/asm/io.h                          |   9 +
 arch/arc/include/asm/pci.h                         |  31 +++
 arch/arc/kernel/Makefile                           |   1 +
 arch/arc/kernel/pcibios.c                          |  23 ++
 arch/arc/plat-axs10x/Kconfig                       |   1 +
 drivers/pci/Makefile                               |   1 +

Quoted 8 lines

 drivers/pci/host/Kconfig                           |   8 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pcie-synopsys.c                   | 257 +++++++++++++++++++++
 13 files changed, 400 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-synopsys.txt
 create mode 100644 arch/arc/include/asm/pci.h
 create mode 100644 arch/arc/kernel/pcibios.c
 create mode 100644 drivers/pci/host/pcie-synopsys.c

I applied these to pci/host-synopsys, and I plan to merge them for v4.5.

Bjorn

Hi Bjorn,
Thanks. Could you please send me the git URL for me to give it a try?

Joao

Quoted 36 lines · Bjorn Helgaas

Quoted 22 lines · Joao Pinto

This patch set has the goal to add suppport for DesignWare PCIe RC in ARC AXS10x. It includes the necessary tweaks to the ARC architecture, necessary tweaks to the PCI subsystem and a new driver (pcie-synopsys). This new driver will be used extensively in the PCIe RC Prototyping Kit.

The patches were produced against Bjorn Helgaas' repository. It was properly tested in an IP Prototyping Kit.

Joao Pinto (2):
PCI support added to ARC
add new platform driver for PCI RC

 .../devicetree/bindings/pci/pcie-synopsys.txt      |  33 +++
 MAINTAINERS                                        |   7 +
 arch/arc/Kconfig                                   |  23 ++
 arch/arc/include/asm/dma.h                         |   5 +
 arch/arc/include/asm/io.h                          |   9 +
 arch/arc/include/asm/pci.h                         |  31 +++
 arch/arc/kernel/Makefile                           |   1 +
 arch/arc/kernel/pcibios.c                          |  23 ++
 arch/arc/plat-axs10x/Kconfig                       |   1 +
 drivers/pci/Makefile                               |   1 +

Quoted 8 lines

 drivers/pci/host/Kconfig                           |   8 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pcie-synopsys.c                   | 257 +++++++++++++++++++++
 13 files changed, 400 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-synopsys.txt
 create mode 100644 arch/arc/include/asm/pci.h
 create mode 100644 arch/arc/kernel/pcibios.c
 create mode 100644 drivers/pci/host/pcie-synopsys.c

I applied these to pci/host-synopsys, and I plan to merge them for v4.5.

Bjorn