[PATCH 0/5] PCI: st: provide support for dw pcie

[PATCH 0/5] PCI: st: provide support for dw pcie

[Gabriel FERNANDEZ]

This patch-set introduces a STMicroelectronics PCIe controler.
It's based on designware PCIe driver.

Gabriel Fernandez (5):
  ARM: STi: Kconfig update for PCIe support
  PCI: st: Add Device Tree bindings for sti pcie
  PCI: st: Provide support for the sti PCIe controller
  PCI: designware: Add setup bus-related to pcie_host_ops
  PCI: st: disable IO support

 Documentation/devicetree/bindings/pci/st-pcie.txt |  53 ++
 arch/arm/mach-sti/Kconfig                         |   2 +
 drivers/pci/host/Kconfig                          |   5 +
 drivers/pci/host/Makefile                         |   1 +
 drivers/pci/host/pci-st.c                         | 736 ++++++++++++++++++++++
 drivers/pci/host/pcie-designware.c                |   3 +
 drivers/pci/host/pcie-designware.h                |   1 +
 7 files changed, 801 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/st-pcie.txt
 create mode 100644 drivers/pci/host/pci-st.c

-- 
1.9.1

[PATCH 1/5] ARM: STi: Kconfig update for PCIe support

[Gabriel FERNANDEZ]

Update Kconfig:
- MIGHT_HAVE_PCI
- PCI_DOMAINS

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 arch/arm/mach-sti/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-sti/Kconfig b/arch/arm/mach-sti/Kconfig
index 8825bc9..d1e563c 100644
--- a/arch/arm/mach-sti/Kconfig
+++ b/arch/arm/mach-sti/Kconfig
@@ -8,6 +8,8 @@ menuconfig ARCH_STI
 	select ARCH_HAS_RESET_CONTROLLER
 	select HAVE_ARM_SCU if SMP
 	select ARCH_REQUIRE_GPIOLIB
+	select PCI_DOMAINS if PCI
+	select MIGHT_HAVE_PCI
 	select ARM_ERRATA_754322
 	select ARM_ERRATA_764369 if SMP
 	select ARM_ERRATA_775420
-- 
1.9.1

[PATCH 2/5] PCI: st: Add Device Tree bindings for sti pcie

[Gabriel FERNANDEZ]

sti pcie is built around a Synopsis Designware PCIe IP.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
---
 Documentation/devicetree/bindings/pci/st-pcie.txt | 53 +++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/st-pcie.txt

diff --git a/Documentation/devicetree/bindings/pci/st-pcie.txt b/Documentation/devicetree/bindings/pci/st-pcie.txt
new file mode 100644
index 0000000..bd3488f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/st-pcie.txt
@@ -0,0 +1,53 @@
+STMicroelectronics STi PCIe controller
+
+This PCIe host controller is based on the Synopsis Designware PCIe IP
+and thus inherits all the common properties defined in designware-pcie.txt.
+
+Required properties:
+ - compatible: "st,stih407-pcie"
+ - reg: base address and length of the pcie controller, mem-window address
+   and length available to the controller.
+ - interrupts: A list of interrupt outputs of the controller.
+ - interrupt-names: Must include the following entries:
+   "msi": STi interrupt that is asserted when an MSI is received
+ - st,syscfg : should be a phandle of the syscfg node. Also contains syscfg
+   offset for IP configuration.
+ - resets, reset-names: the power-down and soft-reset lines of PCIe IP.
+   Associated names must be "powerdown" and "softreset".
+ - phys, phy-names: the phandle for the PHY device.
+   Associated name must be "pcie_phy"
+
+Optional properties:
+ - reset-gpio: a GPIO spec to define which pin is connected to the bus reset.
+
+Example:
+
+pcie0: pcie@9b00000 {
+	compatible = "st,stih407-pcie", "snps,dw-pcie";
+	device_type = "pci";
+	reg = <0x09b00000 0x4000>,	/* dbi cntrl registers */
+	      <0x2fff0000 0x00010000>,	/* configuration space */
+	      <0x40000000 0x80000000>;	/* lmi mem window */
+	reg-names = "dbi", "config", "mem-window";
+	st,syscfg = <&syscfg_core 0xd8 0xe0>;
+	#address-cells = <3>;
+	#size-cells = <2>;
+	ranges = <0x00000800 0 0x2fff0000 0x2fff0000 0 0x00010000   /* configuration space */
+		  0x82000000 0 0x20000000 0x20000000 0 0x0FFF0000>; /* non-prefetchable memory */
+	num-lanes = <1>;
+	interrupts = <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>;
+	interrupt-names = "msi";
+	#interrupt-cells = <1>;
+	interrupt-map-mask = <0 0 0 7>;
+	interrupt-map = <0 0 0 1 &intc GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>, /* INT A */
+			<0 0 0 2 &intc GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>, /* INT B */
+			<0 0 0 3 &intc GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>, /* INT C */
+			<0 0 0 4 &intc GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>; /* INT D */
+
+	resets = <&powerdown STIH407_PCIE0_POWERDOWN>,
+		 <&softreset STIH407_PCIE0_SOFTRESET>;
+	reset-names = "powerdown",
+		      "softreset";
+	phys = <&phy_port0 PHY_TYPE_PCIE>;
+	phy-names = "pcie_phy";
+};
-- 
1.9.1

Re: [PATCH 2/5] PCI: st: Add Device Tree bindings for sti pcie

[Arnd Bergmann]

On Wednesday 17 December 2014 11:34:43 Gabriel FERNANDEZ wrote:
> sti pcie is built around a Synopsis Designware PCIe IP.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
> ---
>  Documentation/devicetree/bindings/pci/st-pcie.txt | 53 +++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/st-pcie.txt
> 
> diff --git a/Documentation/devicetree/bindings/pci/st-pcie.txt b/Documentation/devicetree/bindings/pci/st-pcie.txt
> new file mode 100644
> index 0000000..bd3488f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/st-pcie.txt
> @@ -0,0 +1,53 @@
> +STMicroelectronics STi PCIe controller
> +
> +This PCIe host controller is based on the Synopsis Designware PCIe IP
> +and thus inherits all the common properties defined in designware-pcie.txt.
> +
> +Required properties:
> + - compatible: "st,stih407-pcie"
> + - reg: base address and length of the pcie controller, mem-window address
> +   and length available to the controller.
> + - interrupts: A list of interrupt outputs of the controller.
> + - interrupt-names: Must include the following entries:
> +   "msi": STi interrupt that is asserted when an MSI is received

You should specify that only one interrupt may be present.
If you extend the binding, you will need to describe what the
second one is as well.

> + - st,syscfg : should be a phandle of the syscfg node. Also contains syscfg
> +   offset for IP configuration.
> + - resets, reset-names: the power-down and soft-reset lines of PCIe IP.
> +   Associated names must be "powerdown" and "softreset".
> + - phys, phy-names: the phandle for the PHY device.
> +   Associated name must be "pcie_phy"

Names should not have underscores in them in general. "pcie-phy" would
be fine, but just "pcie" seems good enough, unless there are existing
drivers that have established a specific naming.


	Arnd

Re: [PATCH 2/5] PCI: st: Add Device Tree bindings for sti pcie

[Gabriel Fernandez]

[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]

Hi Arnd,

​Thanks for reviewing​

On 17 December 2014 at 23:01, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wednesday 17 December 2014 11:34:43 Gabriel FERNANDEZ wrote:
> > sti pcie is built around a Synopsis Designware PCIe IP.
> >
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/pci/st-pcie.txt | 53
> +++++++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pci/st-pcie.txt
> >
> > diff --git a/Documentation/devicetree/bindings/pci/st-pcie.txt
> b/Documentation/devicetree/bindings/pci/st-pcie.txt
> > new file mode 100644
> > index 0000000..bd3488f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/st-pcie.txt
> > @@ -0,0 +1,53 @@
> > +STMicroelectronics STi PCIe controller
> > +
> > +This PCIe host controller is based on the Synopsis Designware PCIe IP
> > +and thus inherits all the common properties defined in
> designware-pcie.txt.
> > +
> > +Required properties:
> > + - compatible: "st,stih407-pcie"
> > + - reg: base address and length of the pcie controller, mem-window
> address
> > +   and length available to the controller.
> > + - interrupts: A list of interrupt outputs of the controller.
> > + - interrupt-names: Must include the following entries:
> > +   "msi": STi interrupt that is asserted when an MSI is received
>
> You should specify that only one interrupt may be present.
> If you extend the binding, you will need to describe what the
> second one is as well.
>

​ok, i will change into:
​
​"​
 - interrupts: A list of interrupt outputs of the controller. Must contain
an
  entry for each entry in the interrupt-names property.
​"​




> > + - st,syscfg : should be a phandle of the syscfg node. Also contains
> syscfg
> > +   offset for IP configuration.
> > + - resets, reset-names: the power-down and soft-reset lines of PCIe IP.
> > +   Associated names must be "powerdown" and "softreset".
> > + - phys, phy-names: the phandle for the PHY device.
> > +   Associated name must be "pcie_phy"
>
> Names should not have underscores in them in general. "pcie-phy" would
> be fine, but just "pcie" seems good enough, unless there are existing
> drivers that have established a specific naming.
>

​

​just "pcie" is fine for me


BR

Gabriel
​

>
>
>         Arnd
>
>

[-- Attachment #2: Type: text/html, Size: 4674 bytes --]

Re: [PATCH 2/5] PCI: st: Add Device Tree bindings for sti pcie

[Gabriel Fernandez]

Hi Arnd,

Thanks for reviewing

On 17 December 2014 at 23:01, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 17 December 2014 11:34:43 Gabriel FERNANDEZ wrote:
>> sti pcie is built around a Synopsis Designware PCIe IP.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/pci/st-pcie.txt | 53 +++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pci/st-pcie.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pci/st-pcie.txt b/Documentation/devicetree/bindings/pci/st-pcie.txt
>> new file mode 100644
>> index 0000000..bd3488f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/st-pcie.txt
>> @@ -0,0 +1,53 @@
>> +STMicroelectronics STi PCIe controller
>> +
>> +This PCIe host controller is based on the Synopsis Designware PCIe IP
>> +and thus inherits all the common properties defined in designware-pcie.txt.
>> +
>> +Required properties:
>> + - compatible: "st,stih407-pcie"
>> + - reg: base address and length of the pcie controller, mem-window address
>> +   and length available to the controller.
>> + - interrupts: A list of interrupt outputs of the controller.
>> + - interrupt-names: Must include the following entries:
>> +   "msi": STi interrupt that is asserted when an MSI is received
>
> You should specify that only one interrupt may be present.
> If you extend the binding, you will need to describe what the
> second one is as well.
>

ok, i will change into:
"
 - interrupts: A list of interrupt outputs of the controller. Must contain an
  entry for each entry in the interrupt-names property.

>> + - st,syscfg : should be a phandle of the syscfg node. Also contains syscfg
>> +   offset for IP configuration.
>> + - resets, reset-names: the power-down and soft-reset lines of PCIe IP.
>> +   Associated names must be "powerdown" and "softreset".
>> + - phys, phy-names: the phandle for the PHY device.
>> +   Associated name must be "pcie_phy"
>
> Names should not have underscores in them in general. "pcie-phy" would
> be fine, but just "pcie" seems good enough, unless there are existing
> drivers that have established a specific naming.
>

just "pcie" is fine for me


BR

Gabriel

>
>         Arnd
>

Re: [PATCH 2/5] PCI: st: Add Device Tree bindings for sti pcie

[Pratyush Anand]

On Wednesday 17 December 2014 04:04 PM, Gabriel FERNANDEZ wrote:
> sti pcie is built around a Synopsis Designware PCIe IP.
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
> ---
>   Documentation/devicetree/bindings/pci/st-pcie.txt | 53 +++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/pci/st-pcie.txt
>
> diff --git a/Documentation/devicetree/bindings/pci/st-pcie.txt b/Documentation/devicetree/bindings/pci/st-pcie.txt
> new file mode 100644
> index 0000000..bd3488f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/st-pcie.txt
> @@ -0,0 +1,53 @@
> +STMicroelectronics STi PCIe controller
> +
> +This PCIe host controller is based on the Synopsis Designware PCIe IP
> +and thus inherits all the common properties defined in designware-pcie.txt.
> +
> +Required properties:
> + - compatible: "st,stih407-pcie"
> + - reg: base address and length of the pcie controller, mem-window address
> +   and length available to the controller.
> + - interrupts: A list of interrupt outputs of the controller.
> + - interrupt-names: Must include the following entries:
> +   "msi": STi interrupt that is asserted when an MSI is received
> + - st,syscfg : should be a phandle of the syscfg node. Also contains syscfg
> +   offset for IP configuration.
> + - resets, reset-names: the power-down and soft-reset lines of PCIe IP.
> +   Associated names must be "powerdown" and "softreset".
> + - phys, phy-names: the phandle for the PHY device.
> +   Associated name must be "pcie_phy"
> +
> +Optional properties:
> + - reset-gpio: a GPIO spec to define which pin is connected to the bus reset.
> +
> +Example:
> +
> +pcie0: pcie@9b00000 {
> +	compatible = "st,stih407-pcie", "snps,dw-pcie";
> +	device_type = "pci";
> +	reg = <0x09b00000 0x4000>,	/* dbi cntrl registers */
> +	      <0x2fff0000 0x00010000>,	/* configuration space */
> +	      <0x40000000 0x80000000>;	/* lmi mem window */
> +	reg-names = "dbi", "config", "mem-window";
> +	st,syscfg = <&syscfg_core 0xd8 0xe0>;
> +	#address-cells = <3>;
> +	#size-cells = <2>;
> +	ranges = <0x00000800 0 0x2fff0000 0x2fff0000 0 0x00010000   /* configuration space */

Pass it through reg property.
See: arch/arm/boot/dts/spear1310.dtsi or any other dw pcie's dtsi.


@Jingoo, Mohit: I would suggest following changes so that no upcoming 
platform can add it through ranges.

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index 4fc03b7f1cee..f21570847d08 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -143,8 +143,7 @@
                         #address-cells = <3>;
                         #size-cells = <2>;
                         device_type = "pci";
-                       ranges = <0x00000800 0 0x01f00000 0x01f00000 0 
0x00080000 /* configuration space */
-                                 0x81000000 0 0          0x01f80000 0 
0x00010000 /* downstream I/O */
+                       ranges = 0x81000000 0 0          0x01f80000 0 
0x00010000 /* downstream I/O */
                                   0x82000000 0 0x01000000 0x01000000 0 
0x00f00000>; /* non-prefetchable memory */
                         num-lanes = <1>;
                         interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
index 7a24fee1e7ae..72593a77455e 100644
--- a/arch/arm/boot/dts/imx6sx.dtsi
+++ b/arch/arm/boot/dts/imx6sx.dtsi
@@ -1197,14 +1197,14 @@

                 pcie: pcie@0x08000000 {
                         compatible = "fsl,imx6sx-pcie", "snps,dw-pcie";
-                       reg = <0x08ffc000 0x4000>; /* DBI */
+                       reg = <0x08ffc000 0x4000>, /* DBI */
+                               <0x08f00000 0x80000>;
+                       reg-names = "dbi", "config";
                         #address-cells = <3>;
                         #size-cells = <2>;
                         device_type = "pci";
-                                 /* configuration space */
-                       ranges = <0x00000800 0 0x08f00000 0x08f00000 0 
0x00080000
                                   /* downstream I/O */
-                                 0x81000000 0 0          0x08f80000 0 
0x00010000
+                       ranges =  0x81000000 0 0          0x08f80000 0 
0x00010000
                                   /* non-prefetchable memory */
                                   0x82000000 0 0x08000000 0x08000000 0 
0x00f00000>;
                         num-lanes = <1>;
diff --git a/drivers/pci/host/pcie-designware.c 
b/drivers/pci/host/pcie-designware.c
index df781cdf13c1..0b22b42e1ff9 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -409,19 +409,6 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
                         pp->mem_mod_base = of_read_number(parser.range -
                                                           parser.np + 
na, ns);
                 }
-               if (restype == 0) {
-                       of_pci_range_to_resource(&range, np, &pp->cfg);
-                       pp->cfg0_size = resource_size(&pp->cfg)/2;
-                       pp->cfg1_size = resource_size(&pp->cfg)/2;
-                       pp->cfg0_base = pp->cfg.start;
-                       pp->cfg1_base = pp->cfg.start + pp->cfg0_size;
-
-                       /* Find the untranslated configuration space 
address */
-                       pp->cfg0_mod_base = of_read_number(parser.range -
-                                                          parser.np + 
na, ns);
-                       pp->cfg1_mod_base = pp->cfg0_mod_base +
-                                           pp->cfg0_size;
-               }
         }

         ret = of_pci_parse_bus_range(np, &pp->busn);


Regards
Pratyush

> +		  0x82000000 0 0x20000000 0x20000000 0 0x0FFF0000>; /* non-prefetchable memory */
> +	num-lanes = <1>;
> +	interrupts = <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>;
> +	interrupt-names = "msi";
> +	#interrupt-cells = <1>;
> +	interrupt-map-mask = <0 0 0 7>;
> +	interrupt-map = <0 0 0 1 &intc GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>, /* INT A */
> +			<0 0 0 2 &intc GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>, /* INT B */
> +			<0 0 0 3 &intc GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>, /* INT C */
> +			<0 0 0 4 &intc GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>; /* INT D */
> +
> +	resets = <&powerdown STIH407_PCIE0_POWERDOWN>,
> +		 <&softreset STIH407_PCIE0_SOFTRESET>;
> +	reset-names = "powerdown",
> +		      "softreset";
> +	phys = <&phy_port0 PHY_TYPE_PCIE>;
> +	phy-names = "pcie_phy";
> +};
>

[PATCH 3/5] PCI: st: Provide support for the sti PCIe controller

[Gabriel FERNANDEZ]

sti pcie is built around a Synopsis Designware PCIe IP.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
---
 drivers/pci/host/Kconfig  |   5 +
 drivers/pci/host/Makefile |   1 +
 drivers/pci/host/pci-st.c | 713 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 719 insertions(+)
 create mode 100644 drivers/pci/host/pci-st.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index c4b6568..999d2b9 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -102,4 +102,9 @@ config PCI_LAYERSCAPE
 	help
 	  Say Y here if you want PCIe controller support on Layerscape SoCs.
 
+config PCI_ST
+	bool "ST STiH41x PCIe controller"
+	depends on ARCH_STI
+	select PCIE_DW
+
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 44c2699..ca14829 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
 obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
 obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
 obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
+obj-$(CONFIG_PCI_ST) += pci-st.o
diff --git a/drivers/pci/host/pci-st.c b/drivers/pci/host/pci-st.c
new file mode 100644
index 0000000..bd3d32d
--- /dev/null
+++ b/drivers/pci/host/pci-st.c
@@ -0,0 +1,713 @@
+/*
+ * Copyright (C) 2014 STMicroelectronics
+ *
+ * STMicroelectronics PCI express Driver for sti SoCs.
+ * ST PCIe IPs are built around a Synopsys IP Core.
+ *
+ * Author: Fabrice Gasnier <fabrice.gasnier@st.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2, as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/of_gpio.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/delay.h>
+#include <linux/reset.h>
+#include <linux/phy/phy.h>
+
+#include "pcie-designware.h"
+
+#define TRANSLATION_CONTROL		0x900
+/* Controls if area is inclusive or exclusive */
+#define RC_PASS_ADDR_RANGE		BIT(1)
+
+/* Base of area reserved for config accesses. Fixed size of 64K. */
+#define CFG_BASE_ADDRESS		0x92c
+#define CFG_REGION_SIZE			65536
+
+/* First 4K of config space has this BDF (bus,device,function) */
+#define FUNC0_BDF_NUM			0x930
+
+/* Mem regions */
+#define IN0_MEM_ADDR_START		0x964
+#define IN0_MEM_ADDR_LIMIT		0x968
+#define IN1_MEM_ADDR_START		0x974
+#define IN1_MEM_ADDR_LIMIT		0x978
+
+/* This actually contains the LTSSM state machine state */
+#define PORT_LOGIC_DEBUG_REG_0		0x728
+
+/* LTSSM state machine values	*/
+#define DEBUG_REG_0_LTSSM_MASK		0x1f
+#define S_DETECT_QUIET			0x00
+#define S_DETECT_ACT			0x01
+#define S_POLL_ACTIVE			0x02
+#define S_POLL_COMPLIANCE		0x03
+#define S_POLL_CONFIG			0x04
+#define S_PRE_DETECT_QUIET		0x05
+#define S_DETECT_WAIT			0x06
+#define S_CFG_LINKWD_START		0x07
+#define S_CFG_LINKWD_ACEPT		0x08
+#define S_CFG_LANENUM_WAIT		0x09
+#define S_CFG_LANENUM_ACEPT		0x0A
+#define S_CFG_COMPLETE			0x0B
+#define S_CFG_IDLE			0x0C
+#define S_RCVRY_LOCK			0x0D
+#define S_RCVRY_SPEED			0x0E
+#define S_RCVRY_RCVRCFG			0x0F
+#define S_RCVRY_IDLE			0x10
+#define S_L0				0x11
+#define S_L0S				0x12
+#define S_L123_SEND_EIDLE		0x13
+#define S_L1_IDLE			0x14
+#define S_L2_IDLE			0x15
+#define S_L2_WAKE			0x16
+#define S_DISABLED_ENTRY		0x17
+#define S_DISABLED_IDLE			0x18
+#define S_DISABLED			0x19
+#define S_LPBK_ENTRY			0x1A
+#define S_LPBK_ACTIVE			0x1B
+#define S_LPBK_EXIT			0x1C
+#define S_LPBK_EXIT_TIMEOUT		0x1D
+#define S_HOT_RESET_ENTRY		0x1E
+#define S_HOT_RESET			0x1F
+
+/* syscfg bits */
+#define PCIE_SYS_INT			BIT(5)
+#define PCIE_APP_REQ_RETRY_EN		BIT(3)
+#define PCIE_APP_LTSSM_ENABLE		BIT(2)
+#define PCIE_APP_INIT_RST		BIT(1)
+#define PCIE_DEVICE_TYPE		BIT(0)
+#define PCIE_DEFAULT_VAL		PCIE_DEVICE_TYPE
+
+/* Time to wait between testing the link in msecs (hardware poll interval) */
+#define LINK_LOOP_DELAY_MS 1
+/* Total amount of time to wait for the link to come up in msecs */
+#define LINK_WAIT_MS 120
+#define LINK_LOOP_COUNT (LINK_WAIT_MS / LINK_LOOP_DELAY_MS)
+
+/* st,syscfg offsets */
+#define SYSCFG0_REG	1
+#define SYSCFG1_REG	2
+
+#define to_st_pcie(x)	container_of(x, struct st_pcie, pp)
+
+/**
+ * struct st_pcie_ops - SOC dependent data
+ * @init: reference to controller power & reset init routine
+ * @enable_ltssm: reference to controller link enable routine
+ * @disable_ltssm:  reference to controller link disable routine
+ * @phy_auto: flag when phy automatically configured
+ */
+struct st_pcie_ops {
+	int (*init)(struct pcie_port *pp);
+	int (*enable_ltssm)(struct pcie_port *pp);
+	int (*disable_ltssm)(struct pcie_port *pp);
+	bool phy_auto;
+};
+
+/**
+ * struct st_pcie - private data of the controller
+ * @pp: designware pcie port
+ * @syscfg0: PCIe configuration 0 register, regmap offset
+ * @syscfg1: PCIe configuration 1 register, regmap offset
+ * @phy: associated pcie phy
+ * @config_area: PCIe configuration space
+ * @lmi: memory made available to the controller
+ * @data: SOC dependent data
+ * @regmap: Syscfg registers bank in which PCIe port is configured
+ * @pwr: power control
+ * @rst: reset control
+ * @reset_gpio: optional reset gpio
+ * @config_window_start: start address of 64K config space area
+ */
+struct st_pcie {
+	struct pcie_port pp;
+	int syscfg0;
+	int syscfg1;
+	struct phy *phy;
+	void __iomem *config_area;
+	struct resource	*lmi;
+	const struct st_pcie_ops *data;
+	struct regmap *regmap;
+	struct reset_control *pwr;
+	struct reset_control *rst;
+	int reset_gpio;
+	phys_addr_t config_window_start;
+};
+
+/*
+ * Function to test if the link is in an operational state or not. We must
+ * ensure the link is operational before we try to do a configuration access.
+ */
+static int st_pcie_link_up(struct pcie_port *pp)
+{
+	u32 status;
+	int link_up;
+	int count = 0;
+
+	/*
+	 * We have to be careful here. This is used in config read/write,
+	 * The higher levels switch off interrupts, so you cannot use
+	 * jiffies to do a timeout, or reschedule
+	 */
+	do {
+		/*
+		 * What about L2? I think software intervention is
+		 * required to get it out of L2, so in effect the link
+		 * is down. Requires more work when/if we implement power
+		 * management
+		 */
+		status = readl_relaxed(pp->dbi_base + PORT_LOGIC_DEBUG_REG_0);
+		status &= DEBUG_REG_0_LTSSM_MASK;
+
+		link_up = (status == S_L0) || (status == S_L0S) ||
+			  (status == S_L1_IDLE);
+
+		/*
+		 * It can take some considerable time for the link to actually
+		 * come up, caused by the PLLs. Experiments indicate it takes
+		 * about 8ms to actually bring the link up, but this can vary
+		 * considerably according to the specification. This code should
+		 * allow sufficient time
+		 */
+		if (!link_up)
+			mdelay(LINK_LOOP_DELAY_MS);
+
+	} while (!link_up && ++count < LINK_LOOP_COUNT);
+
+	return link_up;
+}
+
+/*
+ * On ARM platforms, we actually get a bus error returned when the PCIe IP
+ * returns a UR or CRS instead of an OK.
+ */
+static int st_pcie_abort_handler(unsigned long addr, unsigned int fsr,
+				 struct pt_regs *regs)
+{
+	return 0;
+}
+
+/*
+ * The PCI express core IP expects the following arrangement on it's address
+ * bus (slv_haddr) when driving config cycles.
+ * bus_number		[31:24]
+ * dev_number		[23:19]
+ * func_number		[18:16]
+ * unused		[15:12]
+ * ext_reg_number	[11:8]
+ * reg_number		[7:2]
+ *
+ * Bits [15:12] are unused.
+ *
+ * In the glue logic there is a 64K region of address space that can be
+ * written/read to generate config cycles. The base address of this is
+ * controlled by CFG_BASE_ADDRESS. There are 8 16 bit registers called
+ * FUNC0_BDF_NUM to FUNC8_BDF_NUM. These split the bottom half of the 64K
+ * window into 8 regions at 4K boundaries. These control the bus,device and
+ * function number you are trying to talk to.
+ *
+ * The decision on whether to generate a type 0 or type 1 access is controlled
+ * by bits 15:12 of the address you write to.  If they are zero, then a type 0
+ * is generated, if anything else it will be a type 1. Thus the bottom 4K
+ * region controlled by FUNC0_BDF_NUM can only generate type 0, all the others
+ * can only generate type 1.
+ *
+ * We only use FUNC0_BDF_NUM and FUNC1_BDF_NUM. Which one you use is selected
+ * by bit 12 of the address you write to. The selected register is then used
+ * for the top 16 bits of the slv_haddr to form the bus/dev/func, bit 15:12 are
+ * wired to zero, and bits 11:2 form the address of the register you want to
+ * read in config space.
+ *
+ * We always write FUNC0_BDF_NUM as a 32 bit write. So if we want type 1
+ * accesses we have to shift by 16 so in effect we are writing to FUNC1_BDF_NUM
+ */
+static inline u32 bdf_num(int bus, int devfn, int is_root_bus)
+{
+	return ((bus << 8) | devfn) << (is_root_bus ? 0 : 16);
+}
+
+static inline unsigned config_addr(int where, int is_root_bus)
+{
+	return (where & ~3) | (!is_root_bus << 12);
+}
+
+static int st_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
+				 unsigned int devfn, int where, int size,
+				 u32 *val)
+{
+	u32 data;
+	u32 bdf;
+	struct st_pcie *pcie = to_st_pcie(pp);
+	int is_root_bus = pci_is_root_bus(bus);
+	int retry_count = 0;
+	int ret;
+	void __iomem *addr;
+
+	/*
+	 * Prerequisite
+	 * PCI express devices will respond to all config type 0 cycles, since
+	 * they are point to point links. Thus to avoid probing for multiple
+	 * devices on the root, dw-pcie already check for us if it is on the
+	 * root bus / other slots. Also, dw-pcie checks for the link being up
+	 * as we will hang if we issue a config request and the link is down.
+	 * A switch will reject requests for slots it knows do not exist.
+	 */
+	bdf = bdf_num(bus->number, devfn, is_root_bus);
+	addr = pcie->config_area + config_addr(where,
+			bus->parent->number == pp->root_bus_nr);
+retry:
+	/* Set the config packet devfn */
+	writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
+	readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
+
+	ret = dw_pcie_cfg_read(addr, where, size, &data);
+
+	/*
+	 * This is intended to help with when we are probing the bus. The
+	 * problem is that the wrapper logic doesn't have any way to
+	 * interrogate if the configuration request failed or not.
+	 * On the ARM we actually get a real bus error.
+	 *
+	 * Unfortunately this means it is impossible to tell the difference
+	 * between when a device doesn't exist (the switch will return a UR
+	 * completion) or the device does exist but isn't yet ready to accept
+	 * configuration requests (the device will return a CRS completion)
+	 *
+	 * The result of this is that we will miss devices when probing.
+	 *
+	 * So if we are trying to read the dev/vendor id on devfn 0 and we
+	 * appear to get zero back, then we retry the request.  We know that
+	 * zero can never be a valid device/vendor id. The specification says
+	 * we must retry for up to a second before we decide the device is
+	 * dead. If we are still dead then we assume there is nothing there and
+	 * return ~0
+	 *
+	 * The downside of this is that we incur a delay of 1s for every pci
+	 * express link that doesn't have a device connected.
+	 */
+	if (((where & ~3) == 0) && devfn == 0 && (data == 0 || data == ~0)) {
+		if (retry_count++ < 1000) {
+			mdelay(1);
+			goto retry;
+		} else {
+			*val = ~0;
+			return PCIBIOS_DEVICE_NOT_FOUND;
+		}
+	}
+
+	*val = data;
+	return ret;
+}
+
+static int st_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
+				 unsigned int devfn, int where, int size,
+				 u32 val)
+{
+	u32 bdf;
+	struct st_pcie *pcie = to_st_pcie(pp);
+	int is_root_bus = pci_is_root_bus(bus);
+	void __iomem *addr;
+
+	bdf = bdf_num(bus->number, devfn, is_root_bus);
+	addr = pcie->config_area + config_addr(where,
+			bus->parent->number == pp->root_bus_nr);
+
+	/* Set the config packet devfn */
+	writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
+	readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
+
+	return dw_pcie_cfg_write(addr, where, size, val);
+}
+
+static void st_pcie_board_reset(struct pcie_port *pp)
+{
+	struct st_pcie *pcie = to_st_pcie(pp);
+
+	if (!gpio_is_valid(pcie->reset_gpio))
+		return;
+
+	if (gpio_direction_output(pcie->reset_gpio, 0)) {
+		dev_err(pp->dev, "Cannot set PERST# (gpio %u) to output\n",
+			pcie->reset_gpio);
+		return;
+	}
+
+	/* From PCIe spec */
+	usleep_range(1000, 2000);
+	gpio_direction_output(pcie->reset_gpio, 1);
+
+	/*
+	 * PCIe specification states that you should not issue any config
+	 * requests until 100ms after asserting reset, so we enforce that here
+	 */
+	usleep_range(100000, 150000);
+}
+
+static void st_pcie_hw_setup(struct pcie_port *pp)
+{
+	struct st_pcie *pcie = to_st_pcie(pp);
+
+	dw_pcie_setup_rc(pp);
+
+	/* Set up the config window to the top of the PCI address space */
+	writel_relaxed(pcie->config_window_start,
+		       pp->dbi_base + CFG_BASE_ADDRESS);
+
+	/*
+	 * Open up memory to the PCI controller. We could do slightly
+	 * better than this and exclude the kernel text segment and bss etc.
+	 * They are base/limit registers so can be of arbitrary alignment
+	 * presumably
+	 */
+	writel_relaxed(pcie->lmi->start, pp->dbi_base + IN0_MEM_ADDR_START);
+	writel_relaxed(pcie->lmi->end, pp->dbi_base + IN0_MEM_ADDR_LIMIT);
+
+	/* Disable the 2nd region */
+	writel_relaxed(~0, pp->dbi_base + IN1_MEM_ADDR_START);
+	writel_relaxed(0, pp->dbi_base + IN1_MEM_ADDR_LIMIT);
+
+	writel_relaxed(RC_PASS_ADDR_RANGE, pp->dbi_base + TRANSLATION_CONTROL);
+
+	/* Now assert the board level reset to the other PCIe device */
+	st_pcie_board_reset(pp);
+}
+
+static irqreturn_t st_pcie_msi_irq_handler(int irq, void *arg)
+{
+	struct pcie_port *pp = arg;
+
+	return dw_handle_msi_irq(pp);
+}
+
+static void st_msi_init_one(struct pcie_port *pp)
+{
+	struct st_pcie *pcie = to_st_pcie(pp);
+
+	/*
+	 * Set the magic address the hardware responds to. This has to be in
+	 * the range the PCI controller can write to.
+	 */
+	dw_pcie_msi_init(pp);
+
+	if ((virt_to_phys((void *)pp->msi_data) < pcie->lmi->start) ||
+	    (virt_to_phys((void *)pp->msi_data) > pcie->lmi->end))
+		dev_err(pp->dev, "MSI addr miss-configured\n");
+}
+
+static void st_pcie_host_init(struct pcie_port *pp)
+{
+	struct st_pcie *pcie = to_st_pcie(pp);
+	int err;
+
+	/*
+	 * We have to initialise the PCIe cell on some hardware before we can
+	 * talk to the phy
+	 */
+	err = pcie->data->init(pp);
+	if (err)
+		return;
+
+	err = pcie->data->disable_ltssm(pp);
+	if (err) {
+		dev_err(pp->dev, "disable ltssm failed, %d\n", err);
+		return;
+	}
+
+	if (!pcie->data->phy_auto) {
+		/* Now init the associated miphy */
+		err = phy_init(pcie->phy);
+		if (err < 0) {
+			dev_err(pp->dev, "Cannot init PHY: %d\n", err);
+			return;
+		}
+	}
+
+	/* Now do all the register poking */
+	st_pcie_hw_setup(pp);
+
+	/* Re-enable the link */
+	err = pcie->data->enable_ltssm(pp);
+	if (err)
+		dev_err(pp->dev, "enable ltssm failed, %d\n", err);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		st_msi_init_one(pp);
+}
+
+static struct pcie_host_ops st_pcie_host_ops = {
+	.rd_other_conf = st_pcie_rd_other_conf,
+	.wr_other_conf = st_pcie_wr_other_conf,
+	.link_up = st_pcie_link_up,
+	.host_init = st_pcie_host_init,
+};
+
+static int st_pcie_init(struct pcie_port *pp)
+{
+	struct st_pcie *pcie = to_st_pcie(pp);
+	int ret;
+
+	ret = reset_control_deassert(pcie->pwr);
+	if (ret) {
+		dev_err(pp->dev, "unable to bring out of powerdown\n");
+		return ret;
+	}
+
+	ret = reset_control_deassert(pcie->rst);
+	if (ret) {
+		dev_err(pp->dev, "unable to bring out of softreset\n");
+		return ret;
+	}
+
+	/* Set device type : Root Complex */
+	ret = regmap_write(pcie->regmap, pcie->syscfg0, PCIE_DEVICE_TYPE);
+	if (ret < 0) {
+		dev_err(pp->dev, "unable to set device type\n");
+		return ret;
+	}
+
+	usleep_range(1000, 2000);
+	return ret;
+}
+
+/* STiH407 */
+static int stih407_pcie_enable_ltssm(struct pcie_port *pp)
+{
+	struct st_pcie *pcie = to_st_pcie(pp);
+
+	if (!pcie->syscfg1)
+		return -EINVAL;
+
+	return regmap_update_bits(pcie->regmap, pcie->syscfg1,
+				  PCIE_APP_LTSSM_ENABLE, PCIE_APP_LTSSM_ENABLE);
+}
+
+static int stih407_pcie_disable_ltssm(struct pcie_port *pp)
+{
+	struct st_pcie *pcie = to_st_pcie(pp);
+
+	if (!pcie->syscfg1)
+		return -EINVAL;
+
+	return regmap_update_bits(pcie->regmap, pcie->syscfg1,
+				  PCIE_APP_LTSSM_ENABLE, 0);
+}
+
+static struct st_pcie_ops stih407_pcie_ops = {
+	.init = st_pcie_init,
+	.enable_ltssm = stih407_pcie_enable_ltssm,
+	.disable_ltssm = stih407_pcie_disable_ltssm,
+};
+
+static const struct of_device_id st_pcie_of_match[] = {
+	{ .compatible = "st,stih407-pcie", .data = (void *)&stih407_pcie_ops},
+	{ },
+};
+
+#ifdef CONFIG_PM
+static int st_pcie_suspend(struct device *pcie_dev)
+{
+	struct st_pcie *pcie = dev_get_drvdata(pcie_dev);
+
+	/* To guarantee a real phy initialization on resume */
+	if (!pcie->data->phy_auto)
+		phy_exit(pcie->phy);
+
+	return 0;
+}
+
+static int st_pcie_resume(struct device *pcie_dev)
+{
+	struct st_pcie *pcie = dev_get_drvdata(pcie_dev);
+
+	st_pcie_host_init(&pcie->pp);
+
+	return 0;
+}
+#endif
+
+const struct dev_pm_ops st_pcie_pm_ops = {
+	.suspend_noirq = st_pcie_suspend,
+	.resume_noirq = st_pcie_resume,
+};
+
+static int __init st_pcie_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct st_pcie *pcie;
+	struct device_node *np = pdev->dev.of_node;
+	struct pcie_port *pp;
+	int ret;
+
+	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	memset(pcie, 0, sizeof(*pcie));
+
+	pp = &pcie->pp;
+	pp->dev = &pdev->dev;
+
+	/* mem regions */
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
+	pp->dbi_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pp->dbi_base))
+		return PTR_ERR(pp->dbi_base);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
+	if (!res)
+		return -ENXIO;
+
+	/* Check that this has sensible values */
+	if ((resource_size(res) != CFG_REGION_SIZE) ||
+	    (res->start & (CFG_REGION_SIZE - 1))) {
+		dev_err(&pdev->dev, "Invalid config space properties\n");
+		return -EINVAL;
+	}
+	pcie->config_window_start = res->start;
+
+	pcie->config_area = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pcie->config_area))
+		return PTR_ERR(pcie->config_area);
+
+	pcie->lmi = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						 "mem-window");
+	if (!pcie->lmi)
+		return -ENXIO;
+
+	/* regmap registers for PCIe IP configuration */
+	pcie->regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
+	if (IS_ERR(pcie->regmap)) {
+		dev_err(&pdev->dev, "No syscfg phandle specified\n");
+		return PTR_ERR(pcie->regmap);
+	}
+
+	ret = of_property_read_u32_index(np, "st,syscfg", SYSCFG0_REG,
+					 &pcie->syscfg0);
+	if (ret) {
+		dev_err(&pdev->dev, "can't get syscfg0 offset (%d)\n", ret);
+		return ret;
+	}
+
+	ret = of_property_read_u32_index(np, "st,syscfg", SYSCFG1_REG,
+					 &pcie->syscfg1);
+	if (ret) {
+		dev_err(&pdev->dev, "can't get syscfg1 offset (%d)\n", ret);
+		return ret;
+	}
+
+	/* retrieve data opts from compatible */
+	pcie->data = of_match_node(st_pcie_of_match, np)->data;
+	if (!pcie->data) {
+		dev_err(&pdev->dev, "compatible data not provided\n");
+		return -EINVAL;
+	}
+
+	/* powerdown / resets */
+	pcie->pwr = devm_reset_control_get(&pdev->dev, "powerdown");
+	if (IS_ERR(pcie->pwr)) {
+		dev_err(&pdev->dev, "powerdown reset control not defined\n");
+		return -EINVAL;
+	}
+
+	pcie->rst = devm_reset_control_get(&pdev->dev, "softreset");
+	if (IS_ERR(pcie->rst)) {
+		dev_err(&pdev->dev, "Soft reset control not defined\n");
+		return -EINVAL;
+	}
+
+	/* phy */
+	if (!pcie->data->phy_auto) {
+		pcie->phy = devm_phy_get(&pdev->dev, "pcie_phy");
+		if (IS_ERR(pcie->phy)) {
+			dev_err(&pdev->dev, "no PHY configured\n");
+			return PTR_ERR(pcie->phy);
+		}
+	}
+
+	/* Claim the GPIO for PRST# if available */
+	pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
+	if (!gpio_is_valid(pcie->reset_gpio))
+		dev_dbg(&pdev->dev, "No reset-gpio configured\n");
+	else {
+		ret = devm_gpio_request(&pdev->dev,
+					pcie->reset_gpio,
+					"PCIe reset");
+		if (ret) {
+			dev_err(&pdev->dev, "Cannot request reset-gpio %d\n",
+				pcie->reset_gpio);
+			return ret;
+		}
+	}
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		pp->msi_irq = platform_get_irq_byname(pdev, "msi");
+		if (pp->msi_irq <= 0) {
+			dev_err(&pdev->dev, "failed to get MSI irq\n");
+			return -ENODEV;
+		}
+
+		ret = devm_request_irq(&pdev->dev, pp->msi_irq,
+				       st_pcie_msi_irq_handler,
+				       IRQF_SHARED, "st-pcie-msi", pp);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to request MSI irq\n");
+			return -ENODEV;
+		}
+	}
+
+	if (IS_ENABLED(CONFIG_ARM)) {
+		/*
+		 * We have to hook the abort handler so that we can intercept
+		 * bus errors when doing config read/write that return UR,
+		 * which is flagged up as a bus error
+		 */
+		hook_fault_code(16+6, st_pcie_abort_handler, SIGBUS, 0,
+				"imprecise external abort");
+	}
+
+	pp->root_bus_nr = -1;
+	pp->ops = &st_pcie_host_ops;
+
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to initialize host\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, pcie);
+
+	dev_info(&pdev->dev, "Initialized\n");
+	return 0;
+}
+
+MODULE_DEVICE_TABLE(of, st_pcie_of_match);
+
+static struct platform_driver st_pcie_driver = {
+	.driver = {
+		.name = "st-pcie",
+		.of_match_table = st_pcie_of_match,
+		.pm = &st_pcie_pm_ops,
+	},
+};
+
+/* ST PCIe driver does not allow module unload */
+static int __init pcie_init(void)
+{
+	return platform_driver_probe(&st_pcie_driver, st_pcie_probe);
+}
+device_initcall(pcie_init);
+
+MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
+MODULE_DESCRIPTION("PCI express Driver for ST SoCs");
+MODULE_LICENSE("GPLv2");
-- 
1.9.1

Re: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller

[Arnd Bergmann]

On Wednesday 17 December 2014 11:34:44 Gabriel FERNANDEZ wrote:
> sti pcie is built around a Synopsis Designware PCIe IP.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
> ---
>  drivers/pci/host/Kconfig  |   5 +
>  drivers/pci/host/Makefile |   1 +
>  drivers/pci/host/pci-st.c | 713 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 719 insertions(+)
>  create mode 100644 drivers/pci/host/pci-st.c
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index c4b6568..999d2b9 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -102,4 +102,9 @@ config PCI_LAYERSCAPE
>  	help
>  	  Say Y here if you want PCIe controller support on Layerscape SoCs.
>  
> +config PCI_ST
> +	bool "ST STiH41x PCIe controller"
> +	depends on ARCH_STI
> +	select PCIE_DW

I'd use 'depends on ARCH_STI || (ARM && COMPILE_TEST)' to enable
building this on other platforms for test purposes.

> +
> +#define to_st_pcie(x)	container_of(x, struct st_pcie, pp)
> +
> +/**
> + * struct st_pcie_ops - SOC dependent data
> + * @init: reference to controller power & reset init routine
> + * @enable_ltssm: reference to controller link enable routine
> + * @disable_ltssm:  reference to controller link disable routine
> + * @phy_auto: flag when phy automatically configured
> + */
> +struct st_pcie_ops {
> +	int (*init)(struct pcie_port *pp);
> +	int (*enable_ltssm)(struct pcie_port *pp);
> +	int (*disable_ltssm)(struct pcie_port *pp);
> +	bool phy_auto;
> +};

It would be better not to invent another level of indirection. Try
turning this around so you have a driver that binds to the specific
SoC compatible string for the PCIe port while calling into a common
library module for things that are shared.

> +/*
> + * On ARM platforms, we actually get a bus error returned when the PCIe IP
> + * returns a UR or CRS instead of an OK.
> + */
> +static int st_pcie_abort_handler(unsigned long addr, unsigned int fsr,
> +				 struct pt_regs *regs)
> +{
> +	return 0;
> +}

You should check that it's actually PCI that caused the abort. Don't
just ignore a hard error condition.

Usually there are registers in the PCI core that let you identify what
happened.

> +static int st_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> +				 unsigned int devfn, int where, int size,
> +				 u32 *val)
> +{
> +	u32 data;
> +	u32 bdf;
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +	int is_root_bus = pci_is_root_bus(bus);
> +	int retry_count = 0;
> +	int ret;
> +	void __iomem *addr;
> +
> +	/*
> +	 * Prerequisite
> +	 * PCI express devices will respond to all config type 0 cycles, since
> +	 * they are point to point links. Thus to avoid probing for multiple
> +	 * devices on the root, dw-pcie already check for us if it is on the
> +	 * root bus / other slots. Also, dw-pcie checks for the link being up
> +	 * as we will hang if we issue a config request and the link is down.
> +	 * A switch will reject requests for slots it knows do not exist.
> +	 */
> +	bdf = bdf_num(bus->number, devfn, is_root_bus);
> +	addr = pcie->config_area + config_addr(where,
> +			bus->parent->number == pp->root_bus_nr);
> +retry:
> +	/* Set the config packet devfn */
> +	writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
> +	readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
> +
> +	ret = dw_pcie_cfg_read(addr, where, size, &data);
> +
> +	/*
> +	 * This is intended to help with when we are probing the bus. The
> +	 * problem is that the wrapper logic doesn't have any way to
> +	 * interrogate if the configuration request failed or not.
> +	 * On the ARM we actually get a real bus error.
> +	 *
> +	 * Unfortunately this means it is impossible to tell the difference
> +	 * between when a device doesn't exist (the switch will return a UR
> +	 * completion) or the device does exist but isn't yet ready to accept
> +	 * configuration requests (the device will return a CRS completion)
> +	 *
> +	 * The result of this is that we will miss devices when probing.
> +	 *
> +	 * So if we are trying to read the dev/vendor id on devfn 0 and we
> +	 * appear to get zero back, then we retry the request.  We know that
> +	 * zero can never be a valid device/vendor id. The specification says
> +	 * we must retry for up to a second before we decide the device is
> +	 * dead. If we are still dead then we assume there is nothing there and
> +	 * return ~0
> +	 *
> +	 * The downside of this is that we incur a delay of 1s for every pci
> +	 * express link that doesn't have a device connected.
> +	 */
> +	if (((where & ~3) == 0) && devfn == 0 && (data == 0 || data == ~0)) {
> +		if (retry_count++ < 1000) {
> +			mdelay(1);
> +			goto retry;
> +		} else {
> +			*val = ~0;
> +			return PCIBIOS_DEVICE_NOT_FOUND;
> +		}
> +	}
> +
> +	*val = data;
> +	return ret;
> +}

A busy-loop is extremely nasty. If this is only during the initial bus
scan, could you use an msleep instead?

Also, it sounds like the error you get is actually the fault that you
are catching above. If this is correct, then use the fault handler to
communicate this to the probe function.

> +
> +static void st_pcie_board_reset(struct pcie_port *pp)
> +{
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +
> +	if (!gpio_is_valid(pcie->reset_gpio))
> +		return;
> +
> +	if (gpio_direction_output(pcie->reset_gpio, 0)) {
> +		dev_err(pp->dev, "Cannot set PERST# (gpio %u) to output\n",
> +			pcie->reset_gpio);
> +		return;
> +	}
> +
> +	/* From PCIe spec */
> +	usleep_range(1000, 2000);
> +	gpio_direction_output(pcie->reset_gpio, 1);
> +
> +	/*
> +	 * PCIe specification states that you should not issue any config
> +	 * requests until 100ms after asserting reset, so we enforce that here
> +	 */
> +	usleep_range(100000, 150000);
> +}

This seems hardly performance critical, just use msleep(2) and
msleep(100) instead of the usleep_range().

> +static void st_msi_init_one(struct pcie_port *pp)
> +{
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +
> +	/*
> +	 * Set the magic address the hardware responds to. This has to be in
> +	 * the range the PCI controller can write to.
> +	 */
> +	dw_pcie_msi_init(pp);
> +
> +	if ((virt_to_phys((void *)pp->msi_data) < pcie->lmi->start) ||
> +	    (virt_to_phys((void *)pp->msi_data) > pcie->lmi->end))
> +		dev_err(pp->dev, "MSI addr miss-configured\n");
> +}

Why do you call virt_to_phys() here? Isn't msi_data a physical address?

> +static int __init st_pcie_probe(struct platform_device *pdev)

I'd suggest removing the __init here, as discussed in the review for
the qualcomm driver.

	Arnd

Re: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller

[Gabriel Fernandez]

[-- Attachment #1: Type: text/plain, Size: 8574 bytes --]

Hi Arnd,

On 17 December 2014 at 23:14, Arnd Bergmann <arnd@arndb.de> wrote:

> On Wednesday 17 December 2014 11:34:44 Gabriel FERNANDEZ wrote:
> > sti pcie is built around a Synopsis Designware PCIe IP.
> >
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
> > ---
> >  drivers/pci/host/Kconfig  |   5 +
> >  drivers/pci/host/Makefile |   1 +
> >  drivers/pci/host/pci-st.c | 713
> ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 719 insertions(+)
> >  create mode 100644 drivers/pci/host/pci-st.c
> >
> > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> > index c4b6568..999d2b9 100644
> > --- a/drivers/pci/host/Kconfig
> > +++ b/drivers/pci/host/Kconfig
> > @@ -102,4 +102,9 @@ config PCI_LAYERSCAPE
> >       help
> >         Say Y here if you want PCIe controller support on Layerscape
> SoCs.
> >
> > +config PCI_ST
> > +     bool "ST STiH41x PCIe controller"
> > +     depends on ARCH_STI
> > +     select PCIE_DW
>
> I'd use 'depends on ARCH_STI || (ARM && COMPILE_TEST)' to enable
> building this on other platforms for test purposes.
>
> ​ok
​


> > +
> > +#define to_st_pcie(x)        container_of(x, struct st_pcie, pp)
> > +
> > +/**
> > + * struct st_pcie_ops - SOC dependent data
> > + * @init: reference to controller power & reset init routine
> > + * @enable_ltssm: reference to controller link enable routine
> > + * @disable_ltssm:  reference to controller link disable routine
> > + * @phy_auto: flag when phy automatically configured
> > + */
> > +struct st_pcie_ops {
> > +     int (*init)(struct pcie_port *pp);
> > +     int (*enable_ltssm)(struct pcie_port *pp);
> > +     int (*disable_ltssm)(struct pcie_port *pp);
> > +     bool phy_auto;
> > +};
>
> It would be better not to invent another level of indirection. Try
> turning this around so you have a driver that binds to the specific
> SoC compatible string for the PCIe port while calling into a common
> library module for things that are shared.
>
​ok, i will fix it.​



>
> > +/*
> > + * On ARM platforms, we actually get a bus error returned when the PCIe
> IP
> > + * returns a UR or CRS instead of an OK.
> > + */
> > +static int st_pcie_abort_
> ​​
> handler(unsigned long addr, unsigned int fsr,
> > +                              struct pt_regs *regs)
> > +{
> > +     return 0;
> > +}
>
> You should check that it's actually PCI that caused the abort. Don't
> just ignore a hard error condition.
>
> Usually there are registers in the PCI core that let you identify what
> happened.
>

​
We return 0 because abort handler is not activated during boot.

​


>
> > +static int
> ​​
> st_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> > +                              unsigned int devfn, int where, int size,
> > +                              u32 *val)
> > +{
> > +     u32 data;
> > +     u32 bdf;
> > +     struct st_pcie *pcie = to_st_pcie(pp);
> > +     int is_root_bus = pci_is_root_bus(bus);
> > +     int retry_count = 0;
> > +     int ret;
> > +     void __iomem *addr;
> > +
> > +     /*
> > +      * Prerequisite
> > +      * PCI express devices will respond to all config type 0 cycles,
> since
> > +      * they are point to point links. Thus to avoid probing for
> multiple
> > +      * devices on the root, dw-pcie already check for us if it is on
> the
> > +      * root bus / other slots. Also, dw-pcie checks for the link being
> up
> > +      * as we will hang if we issue a config request and the link is
> down.
> > +      * A switch will reject requests for slots it knows do not exist.
> > +      */
> > +     bdf = bdf_num(bus->number, devfn, is_root_bus);
> > +     addr = pcie->config_area + config_addr(where,
> > +                     bus->parent->number == pp->root_bus_nr);
> > +retry:
> > +     /* Set the config packet devfn */
> > +     writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
> > +     readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
> > +
> > +     ret = dw_pcie_cfg_read(addr, where, size, &data);
> > +
> > +     /*
> > +      * This is intended to help with when we are probing the bus. The
> > +      * problem is that the wrapper logic doesn't have any way to
> > +      * interrogate if the configuration request failed or not.
> > +      * On the ARM we actually get a real bus error.
> > +      *
> > +      * Unfortunately this means it is impossible to tell the difference
> > +      * between when a device doesn't exist (the switch will return a UR
> > +      * completion) or the device does exist but isn't yet ready to
> accept
> > +      * configuration requests (the device will return a CRS completion)
> > +      *
> > +      * The result of this is that we will miss devices when probing.
> > +      *
> > +      * So if we are trying to read the dev/vendor id on devfn 0 and we
> > +      * appear to get zero back, then we retry the request.  We know
> that
> > +      * zero can never be a valid device/vendor id. The specification
> says
> > +      * we must retry for up to a second before we decide the device is
> > +      * dead. If we are still dead then we assume there is nothing
> there and
> > +      * return ~0
> > +      *
> > +      * The downside of this is that we incur a delay of 1s for every
> pci
> > +      * express link that doesn't have a device connected.
> > +      */
> > +     if (((where & ~3) == 0) && devfn == 0 && (data == 0 || data ==
> ~0)) {
> > +             if (retry_count++ < 1000) {
> > +                     mdelay(1);
> > +                     goto retry;
> > +             } else {
> > +                     *val = ~0;
> > +                     return PCIBIOS_DEVICE_NOT_FOUND;
> > +             }
> > +     }
> > +
> > +     *val = data;
> > +     return ret;
> > +}
>
> A busy-loop is extremely nasty. If this is only during the initial bus
> scan, could you use an msleep instead?
>
> ​yes it's during the initial bus scan.
But we can't use msleep because we are under raw_spin_lock_irqsave()
see PCI_OP_READ() macro in drivers/pci/access.c
​


> Also, it sounds like the error you get is actually the fault that you
> are catching above. If this is correct, then use the fault handler to
> communicate this to the probe function.
>

​Same as above the handler is not activated during the boot and initial bus
scan.​



>
> > +
> > +static void st_pcie_board_reset(struct pcie_port *pp)
> > +{
> > +     struct st_pcie *pcie = to_st_pcie(pp);
> > +
> > +     if (!gpio_is_valid(pcie->reset_gpio))
> > +             return;
> > +
> > +     if (gpio_direction_output(pcie->reset_gpio, 0)) {
> > +             dev_err(pp->dev, "Cannot set PERST# (gpio %u) to output\n",
> > +                     pcie->reset_gpio);
> > +             return;
> > +     }
> > +
> > +     /* From PCIe spec */
> > +     usleep_range(1000, 2000);
> > +     gpio_direction_output(pcie->reset_gpio, 1);
> > +
> > +     /*
> > +      * PCIe specification states that you should not issue any config
> > +      * requests until 100ms after asserting reset, so we enforce that
> here
> > +      */
> > +     usleep_range(100000, 150000);
> > +}
>
> This seems hardly performance critical, just use msleep(2) and
> msleep(100) instead of the usleep_range().
>

​ok i will fix it.​



>
> > +static void st_msi_init_one(struct pcie_port *pp)
> > +{
> > +     struct st_pcie *pcie = to_st_pcie(pp);
> > +
> > +     /*
> > +      * Set the magic address the hardware responds to. This has to be
> in
> > +      * the range the PCI controller can write to.
> > +      */
> > +     dw_pcie_msi_init(pp);
> > +
> > +     if ((virt_to_phys((void *)pp->msi_data) < pcie->lmi->start) ||
> > +         (virt_to_phys((void *)pp->msi_data) > pcie->lmi->end))
> > +             dev_err(pp->dev, "MSI addr miss-configured\n");
> > +}
>
> Why do you call virt_to_phys() here? Isn't
> ​​
> msi_data a physical address?
>
​?

​
msi_data is a virtual address, it's obtained through a   __get_free_pages()
function in dw_pcie_msi_init() procedure.
​​


> > +static int __init st_pcie_probe(struct platform_device *pdev)
>
> I'd suggest removing the __init here, as discussed in the review for
> the qualcomm driver.
>
> ​ok
​


>         Arnd
>

​BR

Gabriel.​

[-- Attachment #2: Type: text/html, Size: 15235 bytes --]

Re: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller

[Arnd Bergmann]

On Monday 19 January 2015 13:37:33 Gabriel Fernandez wrote:
> On 17 December 2014 at 23:14, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 17 December 2014 11:34:44 Gabriel FERNANDEZ wrote:
> > > +/*
> > > + * On ARM platforms, we actually get a bus error returned when the PCIe
> > IP
> > > + * returns a UR or CRS instead of an OK.
> > > + */
> > > +static int st_pcie_abort_
> > ​​
> > handler(unsigned long addr, unsigned int fsr,
> > > +                              struct pt_regs *regs)
> > > +{
> > > +     return 0;
> > > +}
> >
> > You should check that it's actually PCI that caused the abort. Don't
> > just ignore a hard error condition.
> >
> > Usually there are registers in the PCI core that let you identify what
> > happened.
> >
> 
> ​
> We return 0 because abort handler is not activated during boot.
> 

Can you just remove the handler then? We should never have exception
handlers that unconditionally return 0.

> > > +      * we must retry for up to a second before we decide the device is
> > > +      * dead. If we are still dead then we assume there is nothing
> > there and
> > > +      * return ~0
> > > +      *
> > > +      * The downside of this is that we incur a delay of 1s for every
> > pci
> > > +      * express link that doesn't have a device connected.
> > > +      */
> > > +     if (((where & ~3) == 0) && devfn == 0 && (data == 0 || data ==
> > ~0)) {
> > > +             if (retry_count++ < 1000) {
> > > +                     mdelay(1);
> > > +                     goto retry;
> > > +             } else {
> > > +                     *val = ~0;
> > > +                     return PCIBIOS_DEVICE_NOT_FOUND;
> > > +             }
> > > +     }
> > > +
> > > +     *val = data;
> > > +     return ret;
> > > +}
> >
> > A busy-loop is extremely nasty. If this is only during the initial bus
> > scan, could you use an msleep instead?
> >
> > ​yes it's during the initial bus scan.
> But we can't use msleep because we are under raw_spin_lock_irqsave()
> see PCI_OP_READ() macro in drivers/pci/access.c

Ah, I see. Better use a loop with 'time_before()' and a much shorter
delay then. Even a single mdelay(1) with irqs disabled can be annoying,
so try to make the time as short as possible.

> > Also, it sounds like the error you get is actually the fault that you
> > are catching above. If this is correct, then use the fault handler to
> > communicate this to the probe function.
> >
> 
> ​Same as above the handler is not activated during the boot and initial bus
> scan.​

Maybe you could enable the handler during boot to catch this case, and
then disable it later?

> >
> > > +static void st_msi_init_one(struct pcie_port *pp)
> > > +{
> > > +     struct st_pcie *pcie = to_st_pcie(pp);
> > > +
> > > +     /*
> > > +      * Set the magic address the hardware responds to. This has to be
> > in
> > > +      * the range the PCI controller can write to.
> > > +      */
> > > +     dw_pcie_msi_init(pp);
> > > +
> > > +     if ((virt_to_phys((void *)pp->msi_data) < pcie->lmi->start) ||
> > > +         (virt_to_phys((void *)pp->msi_data) > pcie->lmi->end))
> > > +             dev_err(pp->dev, "MSI addr miss-configured\n");
> > > +}
> >
> > Why do you call virt_to_phys() here? Isn't
> > ​​
> > msi_data a physical address?
> >
> ​?
> 
> ​
> msi_data is a virtual address, it's obtained through a   __get_free_pages()
> function in dw_pcie_msi_init() procedure.

I guess you need dma_map_single() then, or use dma_alloc_coherent instead
of __get_free_pages(). There is no guarantee that the page you allocate
there is actually visible to the PCI host at the same address that the CPU
uses, so you need to map from a CPU address to a DMA address that the PCI
host bridge uses.

	Arnd

Re: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller

[Gabriel Fernandez]

On 19 January 2015 at 14:49, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 19 January 2015 13:37:33 Gabriel Fernandez wrote:
>> On 17 December 2014 at 23:14, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Wednesday 17 December 2014 11:34:44 Gabriel FERNANDEZ wrote:
>> > > +/*
>> > > + * On ARM platforms, we actually get a bus error returned when the PCIe
>> > IP
>> > > + * returns a UR or CRS instead of an OK.
>> > > + */
>> > > +static int st_pcie_abort_
>> >
>> > handler(unsigned long addr, unsigned int fsr,
>> > > +                              struct pt_regs *regs)
>> > > +{
>> > > +     return 0;
>> > > +}
>> >
>> > You should check that it's actually PCI that caused the abort. Don't
>> > just ignore a hard error condition.
>> >
>> > Usually there are registers in the PCI core that let you identify what
>> > happened.
>> >
>>
>>
>> We return 0 because abort handler is not activated during boot.
>>
>
> Can you just remove the handler then? We should never have exception
> handlers that unconditionally return 0.
>

Ah sorry, we need the handler because we can received aborts from
user-land after the boot.

I have 2 solutions, the first to simplify we can only return 0.
The second is to manage handler during boot. Then i need for that a
new patch from Fabrice
https://lkml.org/lkml/2014/2/7/631

>> > > +      * we must retry for up to a second before we decide the device is
>> > > +      * dead. If we are still dead then we assume there is nothing
>> > there and
>> > > +      * return ~0
>> > > +      *
>> > > +      * The downside of this is that we incur a delay of 1s for every
>> > pci
>> > > +      * express link that doesn't have a device connected.
>> > > +      */
>> > > +     if (((where & ~3) == 0) && devfn == 0 && (data == 0 || data ==
>> > ~0)) {
>> > > +             if (retry_count++ < 1000) {
>> > > +                     mdelay(1);
>> > > +                     goto retry;
>> > > +             } else {
>> > > +                     *val = ~0;
>> > > +                     return PCIBIOS_DEVICE_NOT_FOUND;
>> > > +             }
>> > > +     }
>> > > +
>> > > +     *val = data;
>> > > +     return ret;
>> > > +}
>> >
>> > A busy-loop is extremely nasty. If this is only during the initial bus
>> > scan, could you use an msleep instead?
>> >
>> > yes it's during the initial bus scan.
>> But we can't use msleep because we are under raw_spin_lock_irqsave()
>> see PCI_OP_READ() macro in drivers/pci/access.c
>
> Ah, I see. Better use a loop with 'time_before()' and a much shorter
> delay then. Even a single mdelay(1) with irqs disabled can be annoying,
> so try to make the time as short as possible.
>
>> > Also, it sounds like the error you get is actually the fault that you
>> > are catching above. If this is correct, then use the fault handler to
>> > communicate this to the probe function.
>> >
>>
>> Same as above the handler is not activated during the boot and initial bus
>> scan.
>
> Maybe you could enable the handler during boot to catch this case, and
> then disable it later?
>
>> >
>> > > +static void st_msi_init_one(struct pcie_port *pp)
>> > > +{
>> > > +     struct st_pcie *pcie = to_st_pcie(pp);
>> > > +
>> > > +     /*
>> > > +      * Set the magic address the hardware responds to. This has to be
>> > in
>> > > +      * the range the PCI controller can write to.
>> > > +      */
>> > > +     dw_pcie_msi_init(pp);
>> > > +
>> > > +     if ((virt_to_phys((void *)pp->msi_data) < pcie->lmi->start) ||
>> > > +         (virt_to_phys((void *)pp->msi_data) > pcie->lmi->end))
>> > > +             dev_err(pp->dev, "MSI addr miss-configured\n");
>> > > +}
>> >
>> > Why do you call virt_to_phys() here? Isn't
>> >
>> > msi_data a physical address?
>> >
>> ?
>>
>>
>> msi_data is a virtual address, it's obtained through a   __get_free_pages()
>> function in dw_pcie_msi_init() procedure.
>
> I guess you need dma_map_single() then, or use dma_alloc_coherent instead
> of __get_free_pages(). There is no guarantee that the page you allocate
> there is actually visible to the PCI host at the same address that the CPU
> uses, so you need to map from a CPU address to a DMA address that the PCI
> host bridge uses.
>
>         Arnd

This is only to check the msi magic address given to ip, we never read
or write in this area.
this code is only a check

BR

Gabriel

Re: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller

[Arnd Bergmann]

On Wednesday 21 January 2015 16:47:36 Gabriel Fernandez wrote:
> On 19 January 2015 at 14:49, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > On Monday 19 January 2015 13:37:33 Gabriel Fernandez wrote:
> >> On 17 December 2014 at 23:14, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> >> > On Wednesday 17 December 2014 11:34:44 Gabriel FERNANDEZ wrote:
> >> > > +/*
> >> > > + * On ARM platforms, we actually get a bus error returned when the PCIe
> >> > IP
> >> > > + * returns a UR or CRS instead of an OK.
> >> > > + */
> >> > > +static int st_pcie_abort_
> >> >
> >> > handler(unsigned long addr, unsigned int fsr,
> >> > > +                              struct pt_regs *regs)
> >> > > +{
> >> > > +     return 0;
> >> > > +}
> >> >
> >> > You should check that it's actually PCI that caused the abort. Don't
> >> > just ignore a hard error condition.
> >> >
> >> > Usually there are registers in the PCI core that let you identify what
> >> > happened.
> >> >
> >>
> >>
> >> We return 0 because abort handler is not activated during boot.
> >>
> >
> > Can you just remove the handler then? We should never have exception
> > handlers that unconditionally return 0.
> >
> 
> Ah sorry, we need the handler because we can received aborts from
> user-land after the boot.
> 
> I have 2 solutions, the first to simplify we can only return 0.
> The second is to manage handler during boot. Then i need for that a
> new patch from Fabrice
> https://lkml.org/lkml/2014/2/7/631

I still don't get it. What is causing the abort? Is that something
user space does, or is it just a condition that gets stuck the pcie
device after probing that gets delivered once as soon as the aborts
are enabled?

> >>
> >> msi_data is a virtual address, it's obtained through a   __get_free_pages()
> >> function in dw_pcie_msi_init() procedure.
> >
> > I guess you need dma_map_single() then, or use dma_alloc_coherent instead
> > of __get_free_pages(). There is no guarantee that the page you allocate
> > there is actually visible to the PCI host at the same address that the CPU
> > uses, so you need to map from a CPU address to a DMA address that the PCI
> > host bridge uses.
> >
> >         Arnd
> 
> This is only to check the msi magic address given to ip, we never read
> or write in this area.
> this code is only a check

But doesn't the hardware access the pointer?

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

Re: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller

[Lucas Stach]

Am Mittwoch, den 21.01.2015, 20:35 +0100 schrieb Arnd Bergmann:
> On Wednesday 21 January 2015 16:47:36 Gabriel Fernandez wrote:
> > On 19 January 2015 at 14:49, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > > On Monday 19 January 2015 13:37:33 Gabriel Fernandez wrote:
> > >> On 17 December 2014 at 23:14, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > >> > On Wednesday 17 December 2014 11:34:44 Gabriel FERNANDEZ wrote:
> > >> > > +/*
> > >> > > + * On ARM platforms, we actually get a bus error returned when the PCIe
> > >> > IP
> > >> > > + * returns a UR or CRS instead of an OK.
> > >> > > + */
> > >> > > +static int st_pcie_abort_
> > >> >
> > >> > handler(unsigned long addr, unsigned int fsr,
> > >> > > +                              struct pt_regs *regs)
> > >> > > +{
> > >> > > +     return 0;
> > >> > > +}
> > >> >
> > >> > You should check that it's actually PCI that caused the abort. Don't
> > >> > just ignore a hard error condition.
> > >> >
> > >> > Usually there are registers in the PCI core that let you identify what
> > >> > happened.
> > >> >
> > >>
> > >>
> > >> We return 0 because abort handler is not activated during boot.
> > >>
> > >
> > > Can you just remove the handler then? We should never have exception
> > > handlers that unconditionally return 0.
> > >
> > 
> > Ah sorry, we need the handler because we can received aborts from
> > user-land after the boot.
> > 
> > I have 2 solutions, the first to simplify we can only return 0.
> > The second is to manage handler during boot. Then i need for that a
> > new patch from Fabrice
> > https://lkml.org/lkml/2014/2/7/631
> 
> I still don't get it. What is causing the abort? Is that something
> user space does, or is it just a condition that gets stuck the pcie
> device after probing that gets delivered once as soon as the aborts
> are enabled?
> 
The abort is caused by the kernel trying to access a non-existent device
while probing the bus. It isn't delivered to the ARM core, as it is an
imprecise external abort which are masked during boot.
Once pid1 is started and we do the first schedule imprecise aborts get
unmasked and pid1 will be hit by the stale abort.

The above patch does the right thing by unmasking imprecise abort early
during boot, so they can get handled once they are hit.

The abort handling for the DW PCIe core right now is a complete
disaster. Hooking the fault code means we possibly overwrite any other
handler for the same abort. Also as the configuration space is mapped as
device memory the abort actually happens on the *next* instruction after
the read that caused it. Improving this is something I had on my to-do
list for quite some time now. 

Regards,
Lucas


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller

[Gabriel Fernandez]

Hi Arnd,

On 17 December 2014 at 23:14, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 17 December 2014 11:34:44 Gabriel FERNANDEZ wrote:
>> sti pcie is built around a Synopsis Designware PCIe IP.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
>> ---
>>  drivers/pci/host/Kconfig  |   5 +
>>  drivers/pci/host/Makefile |   1 +
>>  drivers/pci/host/pci-st.c | 713 ++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 719 insertions(+)
>>  create mode 100644 drivers/pci/host/pci-st.c
>>
>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index c4b6568..999d2b9 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -102,4 +102,9 @@ config PCI_LAYERSCAPE
>>       help
>>         Say Y here if you want PCIe controller support on Layerscape SoCs.
>>
>> +config PCI_ST
>> +     bool "ST STiH41x PCIe controller"
>> +     depends on ARCH_STI
>> +     select PCIE_DW
>
> I'd use 'depends on ARCH_STI || (ARM && COMPILE_TEST)' to enable
> building this on other platforms for test purposes.
>
ok

>> +
>> +#define to_st_pcie(x)        container_of(x, struct st_pcie, pp)
>> +
>> +/**
>> + * struct st_pcie_ops - SOC dependent data
>> + * @init: reference to controller power & reset init routine
>> + * @enable_ltssm: reference to controller link enable routine
>> + * @disable_ltssm:  reference to controller link disable routine
>> + * @phy_auto: flag when phy automatically configured
>> + */
>> +struct st_pcie_ops {
>> +     int (*init)(struct pcie_port *pp);
>> +     int (*enable_ltssm)(struct pcie_port *pp);
>> +     int (*disable_ltssm)(struct pcie_port *pp);
>> +     bool phy_auto;
>> +};
>
ok, i will fix it.

> It would be better not to invent another level of indirection. Try
> turning this around so you have a driver that binds to the specific
> SoC compatible string for the PCIe port while calling into a common
> library module for things that are shared.
>
>> +/*
>> + * On ARM platforms, we actually get a bus error returned when the PCIe IP
>> + * returns a UR or CRS instead of an OK.
>> + */
>> +static int st_pcie_abort_handler(unsigned long addr, unsigned int fsr,
>> +                              struct pt_regs *regs)
>> +{
>> +     return 0;
>> +}
>
> You should check that it's actually PCI that caused the abort. Don't
> just ignore a hard error condition.
>
> Usually there are registers in the PCI core that let you identify what
> happened.
>
We return 0 because abort handler is not activated during boot.

>> +static int st_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>> +                              unsigned int devfn, int where, int size,
>> +                              u32 *val)
>> +{
>> +     u32 data;
>> +     u32 bdf;
>> +     struct st_pcie *pcie = to_st_pcie(pp);
>> +     int is_root_bus = pci_is_root_bus(bus);
>> +     int retry_count = 0;
>> +     int ret;
>> +     void __iomem *addr;
>> +
>> +     /*
>> +      * Prerequisite
>> +      * PCI express devices will respond to all config type 0 cycles, since
>> +      * they are point to point links. Thus to avoid probing for multiple
>> +      * devices on the root, dw-pcie already check for us if it is on the
>> +      * root bus / other slots. Also, dw-pcie checks for the link being up
>> +      * as we will hang if we issue a config request and the link is down.
>> +      * A switch will reject requests for slots it knows do not exist.
>> +      */
>> +     bdf = bdf_num(bus->number, devfn, is_root_bus);
>> +     addr = pcie->config_area + config_addr(where,
>> +                     bus->parent->number == pp->root_bus_nr);
>> +retry:
>> +     /* Set the config packet devfn */
>> +     writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
>> +     readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
>> +
>> +     ret = dw_pcie_cfg_read(addr, where, size, &data);
>> +
>> +     /*
>> +      * This is intended to help with when we are probing the bus. The
>> +      * problem is that the wrapper logic doesn't have any way to
>> +      * interrogate if the configuration request failed or not.
>> +      * On the ARM we actually get a real bus error.
>> +      *
>> +      * Unfortunately this means it is impossible to tell the difference
>> +      * between when a device doesn't exist (the switch will return a UR
>> +      * completion) or the device does exist but isn't yet ready to accept
>> +      * configuration requests (the device will return a CRS completion)
>> +      *
>> +      * The result of this is that we will miss devices when probing.
>> +      *
>> +      * So if we are trying to read the dev/vendor id on devfn 0 and we
>> +      * appear to get zero back, then we retry the request.  We know that
>> +      * zero can never be a valid device/vendor id. The specification says
>> +      * we must retry for up to a second before we decide the device is
>> +      * dead. If we are still dead then we assume there is nothing there and
>> +      * return ~0
>> +      *
>> +      * The downside of this is that we incur a delay of 1s for every pci
>> +      * express link that doesn't have a device connected.
>> +      */
>> +     if (((where & ~3) == 0) && devfn == 0 && (data == 0 || data == ~0)) {
>> +             if (retry_count++ < 1000) {
>> +                     mdelay(1);
>> +                     goto retry;
>> +             } else {
>> +                     *val = ~0;
>> +                     return PCIBIOS_DEVICE_NOT_FOUND;
>> +             }
>> +     }
>> +
>> +     *val = data;
>> +     return ret;
>> +}
>
> A busy-loop is extremely nasty. If this is only during the initial bus
> scan, could you use an msleep instead?
>
yes it's during the initial bus scan.
But we can't use msleep because we are under raw_spin_lock_irqsave()
see PCI_OP_READ() macro in drivers/pci/access.c

> Also, it sounds like the error you get is actually the fault that you
> are catching above. If this is correct, then use the fault handler to
> communicate this to the probe function.
>
Same as above the handler is not activated during the boot and initial bus scan.


>> +
>> +static void st_pcie_board_reset(struct pcie_port *pp)
>> +{
>> +     struct st_pcie *pcie = to_st_pcie(pp);
>> +
>> +     if (!gpio_is_valid(pcie->reset_gpio))
>> +             return;
>> +
>> +     if (gpio_direction_output(pcie->reset_gpio, 0)) {
>> +             dev_err(pp->dev, "Cannot set PERST# (gpio %u) to output\n",
>> +                     pcie->reset_gpio);
>> +             return;
>> +     }
>> +
>> +     /* From PCIe spec */
>> +     usleep_range(1000, 2000);
>> +     gpio_direction_output(pcie->reset_gpio, 1);
>> +
>> +     /*
>> +      * PCIe specification states that you should not issue any config
>> +      * requests until 100ms after asserting reset, so we enforce that here
>> +      */
>> +     usleep_range(100000, 150000);
>> +}
>
> This seems hardly performance critical, just use msleep(2) and
> msleep(100) instead of the usleep_range().
>

ok i will fix it.


>> +static void st_msi_init_one(struct pcie_port *pp)
>> +{
>> +     struct st_pcie *pcie = to_st_pcie(pp);
>> +
>> +     /*
>> +      * Set the magic address the hardware responds to. This has to be in
>> +      * the range the PCI controller can write to.
>> +      */
>> +     dw_pcie_msi_init(pp);
>> +
>> +     if ((virt_to_phys((void *)pp->msi_data) < pcie->lmi->start) ||
>> +         (virt_to_phys((void *)pp->msi_data) > pcie->lmi->end))
>> +             dev_err(pp->dev, "MSI addr miss-configured\n");
>> +}
>
> Why do you call virt_to_phys() here? Isn't msi_data a physical address?
>
msi_data is a virtual address, it's obtained through a
__get_free_pages() function in dw_pcie_msi_init() procedure.

>> +static int __init st_pcie_probe(struct platform_device *pdev)
>
> I'd suggest removing the __init here, as discussed in the review for
> the qualcomm driver.
>
ok

>         Arnd

BR

Gabriel.

Re: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller

[Jingoo Han]

On Wednesday, December 17, 2014 7:35 PM, Gabriel FERNANDEZ wrote:
> 
> sti pcie is built around a Synopsis Designware PCIe IP.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/pci/host/Kconfig  |   5 +
>  drivers/pci/host/Makefile |   1 +
>  drivers/pci/host/pci-st.c | 713 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 719 insertions(+)
>  create mode 100644 drivers/pci/host/pci-st.c
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index c4b6568..999d2b9 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -102,4 +102,9 @@ config PCI_LAYERSCAPE
>  	help
>  	  Say Y here if you want PCIe controller support on Layerscape SoCs.
> 
> +config PCI_ST
> +	bool "ST STiH41x PCIe controller"
> +	depends on ARCH_STI
> +	select PCIE_DW
> +
>  endmenu
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 44c2699..ca14829 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
>  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
>  obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
>  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
> +obj-$(CONFIG_PCI_ST) += pci-st.o
> diff --git a/drivers/pci/host/pci-st.c b/drivers/pci/host/pci-st.c
> new file mode 100644
> index 0000000..bd3d32d
> --- /dev/null
> +++ b/drivers/pci/host/pci-st.c
> @@ -0,0 +1,713 @@
> +/*
> + * Copyright (C) 2014 STMicroelectronics
> + *
> + * STMicroelectronics PCI express Driver for sti SoCs.
> + * ST PCIe IPs are built around a Synopsys IP Core.
> + *
> + * Author: Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2, as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_address.h>
> +#include <linux/of_pci.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_gpio.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/delay.h>
> +#include <linux/reset.h>
> +#include <linux/phy/phy.h>

Please, re-order these headers alphabetically.
It enhances the readability.

> +
> +#include "pcie-designware.h"
> +
> +#define TRANSLATION_CONTROL		0x900
> +/* Controls if area is inclusive or exclusive */
> +#define RC_PASS_ADDR_RANGE		BIT(1)
> +
> +/* Base of area reserved for config accesses. Fixed size of 64K. */
> +#define CFG_BASE_ADDRESS		0x92c
> +#define CFG_REGION_SIZE			65536
> +
> +/* First 4K of config space has this BDF (bus,device,function) */
> +#define FUNC0_BDF_NUM			0x930
> +
> +/* Mem regions */
> +#define IN0_MEM_ADDR_START		0x964
> +#define IN0_MEM_ADDR_LIMIT		0x968
> +#define IN1_MEM_ADDR_START		0x974
> +#define IN1_MEM_ADDR_LIMIT		0x978
> +
> +/* This actually contains the LTSSM state machine state */
> +#define PORT_LOGIC_DEBUG_REG_0		0x728
> +
> +/* LTSSM state machine values	*/
> +#define DEBUG_REG_0_LTSSM_MASK		0x1f
> +#define S_DETECT_QUIET			0x00
> +#define S_DETECT_ACT			0x01
> +#define S_POLL_ACTIVE			0x02
> +#define S_POLL_COMPLIANCE		0x03
> +#define S_POLL_CONFIG			0x04
> +#define S_PRE_DETECT_QUIET		0x05
> +#define S_DETECT_WAIT			0x06
> +#define S_CFG_LINKWD_START		0x07
> +#define S_CFG_LINKWD_ACEPT		0x08
> +#define S_CFG_LANENUM_WAIT		0x09
> +#define S_CFG_LANENUM_ACEPT		0x0A
> +#define S_CFG_COMPLETE			0x0B
> +#define S_CFG_IDLE			0x0C
> +#define S_RCVRY_LOCK			0x0D
> +#define S_RCVRY_SPEED			0x0E
> +#define S_RCVRY_RCVRCFG			0x0F
> +#define S_RCVRY_IDLE			0x10
> +#define S_L0				0x11
> +#define S_L0S				0x12
> +#define S_L123_SEND_EIDLE		0x13
> +#define S_L1_IDLE			0x14
> +#define S_L2_IDLE			0x15
> +#define S_L2_WAKE			0x16
> +#define S_DISABLED_ENTRY		0x17
> +#define S_DISABLED_IDLE			0x18
> +#define S_DISABLED			0x19
> +#define S_LPBK_ENTRY			0x1A
> +#define S_LPBK_ACTIVE			0x1B
> +#define S_LPBK_EXIT			0x1C
> +#define S_LPBK_EXIT_TIMEOUT		0x1D
> +#define S_HOT_RESET_ENTRY		0x1E
> +#define S_HOT_RESET			0x1F
> +
> +/* syscfg bits */
> +#define PCIE_SYS_INT			BIT(5)
> +#define PCIE_APP_REQ_RETRY_EN		BIT(3)
> +#define PCIE_APP_LTSSM_ENABLE		BIT(2)
> +#define PCIE_APP_INIT_RST		BIT(1)
> +#define PCIE_DEVICE_TYPE		BIT(0)
> +#define PCIE_DEFAULT_VAL		PCIE_DEVICE_TYPE
> +
> +/* Time to wait between testing the link in msecs (hardware poll interval) */
> +#define LINK_LOOP_DELAY_MS 1
> +/* Total amount of time to wait for the link to come up in msecs */
> +#define LINK_WAIT_MS 120
> +#define LINK_LOOP_COUNT (LINK_WAIT_MS / LINK_LOOP_DELAY_MS)
> +
> +/* st,syscfg offsets */
> +#define SYSCFG0_REG	1
> +#define SYSCFG1_REG	2
> +
> +#define to_st_pcie(x)	container_of(x, struct st_pcie, pp)
> +
> +/**
> + * struct st_pcie_ops - SOC dependent data
> + * @init: reference to controller power & reset init routine
> + * @enable_ltssm: reference to controller link enable routine
> + * @disable_ltssm:  reference to controller link disable routine
> + * @phy_auto: flag when phy automatically configured
> + */
> +struct st_pcie_ops {
> +	int (*init)(struct pcie_port *pp);
> +	int (*enable_ltssm)(struct pcie_port *pp);
> +	int (*disable_ltssm)(struct pcie_port *pp);
> +	bool phy_auto;
> +};
> +
> +/**
> + * struct st_pcie - private data of the controller
> + * @pp: designware pcie port
> + * @syscfg0: PCIe configuration 0 register, regmap offset
> + * @syscfg1: PCIe configuration 1 register, regmap offset
> + * @phy: associated pcie phy
> + * @config_area: PCIe configuration space
> + * @lmi: memory made available to the controller
> + * @data: SOC dependent data
> + * @regmap: Syscfg registers bank in which PCIe port is configured
> + * @pwr: power control
> + * @rst: reset control
> + * @reset_gpio: optional reset gpio
> + * @config_window_start: start address of 64K config space area
> + */
> +struct st_pcie {
> +	struct pcie_port pp;
> +	int syscfg0;
> +	int syscfg1;
> +	struct phy *phy;
> +	void __iomem *config_area;
> +	struct resource	*lmi;
> +	const struct st_pcie_ops *data;
> +	struct regmap *regmap;
> +	struct reset_control *pwr;
> +	struct reset_control *rst;
> +	int reset_gpio;
> +	phys_addr_t config_window_start;
> +};
> +
> +/*
> + * Function to test if the link is in an operational state or not. We must
> + * ensure the link is operational before we try to do a configuration access.
> + */
> +static int st_pcie_link_up(struct pcie_port *pp)
> +{
> +	u32 status;
> +	int link_up;
> +	int count = 0;
> +
> +	/*
> +	 * We have to be careful here. This is used in config read/write,
> +	 * The higher levels switch off interrupts, so you cannot use
> +	 * jiffies to do a timeout, or reschedule
> +	 */
> +	do {
> +		/*
> +		 * What about L2? I think software intervention is
> +		 * required to get it out of L2, so in effect the link
> +		 * is down. Requires more work when/if we implement power
> +		 * management
> +		 */
> +		status = readl_relaxed(pp->dbi_base + PORT_LOGIC_DEBUG_REG_0);
> +		status &= DEBUG_REG_0_LTSSM_MASK;
> +
> +		link_up = (status == S_L0) || (status == S_L0S) ||
> +			  (status == S_L1_IDLE);
> +
> +		/*
> +		 * It can take some considerable time for the link to actually
> +		 * come up, caused by the PLLs. Experiments indicate it takes
> +		 * about 8ms to actually bring the link up, but this can vary
> +		 * considerably according to the specification. This code should
> +		 * allow sufficient time
> +		 */
> +		if (!link_up)
> +			mdelay(LINK_LOOP_DELAY_MS);
> +
> +	} while (!link_up && ++count < LINK_LOOP_COUNT);
> +
> +	return link_up;
> +}
> +
> +/*
> + * On ARM platforms, we actually get a bus error returned when the PCIe IP
> + * returns a UR or CRS instead of an OK.
> + */
> +static int st_pcie_abort_handler(unsigned long addr, unsigned int fsr,
> +				 struct pt_regs *regs)
> +{
> +	return 0;
> +}
> +
> +/*
> + * The PCI express core IP expects the following arrangement on it's address
> + * bus (slv_haddr) when driving config cycles.
> + * bus_number		[31:24]
> + * dev_number		[23:19]
> + * func_number		[18:16]
> + * unused		[15:12]
> + * ext_reg_number	[11:8]
> + * reg_number		[7:2]
> + *
> + * Bits [15:12] are unused.
> + *
> + * In the glue logic there is a 64K region of address space that can be
> + * written/read to generate config cycles. The base address of this is
> + * controlled by CFG_BASE_ADDRESS. There are 8 16 bit registers called
> + * FUNC0_BDF_NUM to FUNC8_BDF_NUM. These split the bottom half of the 64K
> + * window into 8 regions at 4K boundaries. These control the bus,device and
> + * function number you are trying to talk to.
> + *
> + * The decision on whether to generate a type 0 or type 1 access is controlled
> + * by bits 15:12 of the address you write to.  If they are zero, then a type 0
> + * is generated, if anything else it will be a type 1. Thus the bottom 4K
> + * region controlled by FUNC0_BDF_NUM can only generate type 0, all the others
> + * can only generate type 1.
> + *
> + * We only use FUNC0_BDF_NUM and FUNC1_BDF_NUM. Which one you use is selected
> + * by bit 12 of the address you write to. The selected register is then used
> + * for the top 16 bits of the slv_haddr to form the bus/dev/func, bit 15:12 are
> + * wired to zero, and bits 11:2 form the address of the register you want to
> + * read in config space.
> + *
> + * We always write FUNC0_BDF_NUM as a 32 bit write. So if we want type 1
> + * accesses we have to shift by 16 so in effect we are writing to FUNC1_BDF_NUM
> + */
> +static inline u32 bdf_num(int bus, int devfn, int is_root_bus)
> +{
> +	return ((bus << 8) | devfn) << (is_root_bus ? 0 : 16);
> +}
> +
> +static inline unsigned config_addr(int where, int is_root_bus)
> +{
> +	return (where & ~3) | (!is_root_bus << 12);
> +}
> +
> +static int st_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> +				 unsigned int devfn, int where, int size,
> +				 u32 *val)
> +{
> +	u32 data;
> +	u32 bdf;
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +	int is_root_bus = pci_is_root_bus(bus);
> +	int retry_count = 0;
> +	int ret;
> +	void __iomem *addr;
> +
> +	/*
> +	 * Prerequisite
> +	 * PCI express devices will respond to all config type 0 cycles, since
> +	 * they are point to point links. Thus to avoid probing for multiple
> +	 * devices on the root, dw-pcie already check for us if it is on the
> +	 * root bus / other slots. Also, dw-pcie checks for the link being up
> +	 * as we will hang if we issue a config request and the link is down.
> +	 * A switch will reject requests for slots it knows do not exist.
> +	 */
> +	bdf = bdf_num(bus->number, devfn, is_root_bus);
> +	addr = pcie->config_area + config_addr(where,
> +			bus->parent->number == pp->root_bus_nr);
> +retry:
> +	/* Set the config packet devfn */
> +	writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
> +	readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
> +
> +	ret = dw_pcie_cfg_read(addr, where, size, &data);
> +
> +	/*
> +	 * This is intended to help with when we are probing the bus. The
> +	 * problem is that the wrapper logic doesn't have any way to
> +	 * interrogate if the configuration request failed or not.
> +	 * On the ARM we actually get a real bus error.
> +	 *
> +	 * Unfortunately this means it is impossible to tell the difference
> +	 * between when a device doesn't exist (the switch will return a UR
> +	 * completion) or the device does exist but isn't yet ready to accept
> +	 * configuration requests (the device will return a CRS completion)
> +	 *
> +	 * The result of this is that we will miss devices when probing.
> +	 *
> +	 * So if we are trying to read the dev/vendor id on devfn 0 and we
> +	 * appear to get zero back, then we retry the request.  We know that
> +	 * zero can never be a valid device/vendor id. The specification says
> +	 * we must retry for up to a second before we decide the device is
> +	 * dead. If we are still dead then we assume there is nothing there and
> +	 * return ~0
> +	 *
> +	 * The downside of this is that we incur a delay of 1s for every pci
> +	 * express link that doesn't have a device connected.
> +	 */
> +	if (((where & ~3) == 0) && devfn == 0 && (data == 0 || data == ~0)) {
> +		if (retry_count++ < 1000) {
> +			mdelay(1);
> +			goto retry;
> +		} else {
> +			*val = ~0;
> +			return PCIBIOS_DEVICE_NOT_FOUND;
> +		}
> +	}
> +
> +	*val = data;
> +	return ret;
> +}
> +
> +static int st_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> +				 unsigned int devfn, int where, int size,
> +				 u32 val)
> +{
> +	u32 bdf;
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +	int is_root_bus = pci_is_root_bus(bus);
> +	void __iomem *addr;
> +
> +	bdf = bdf_num(bus->number, devfn, is_root_bus);
> +	addr = pcie->config_area + config_addr(where,
> +			bus->parent->number == pp->root_bus_nr);
> +
> +	/* Set the config packet devfn */
> +	writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
> +	readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
> +
> +	return dw_pcie_cfg_write(addr, where, size, val);
> +}
> +
> +static void st_pcie_board_reset(struct pcie_port *pp)
> +{
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +
> +	if (!gpio_is_valid(pcie->reset_gpio))
> +		return;
> +
> +	if (gpio_direction_output(pcie->reset_gpio, 0)) {
> +		dev_err(pp->dev, "Cannot set PERST# (gpio %u) to output\n",
> +			pcie->reset_gpio);
> +		return;
> +	}
> +
> +	/* From PCIe spec */
> +	usleep_range(1000, 2000);
> +	gpio_direction_output(pcie->reset_gpio, 1);
> +
> +	/*
> +	 * PCIe specification states that you should not issue any config
> +	 * requests until 100ms after asserting reset, so we enforce that here
> +	 */
> +	usleep_range(100000, 150000);
> +}
> +
> +static void st_pcie_hw_setup(struct pcie_port *pp)
> +{
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +
> +	dw_pcie_setup_rc(pp);
> +
> +	/* Set up the config window to the top of the PCI address space */
> +	writel_relaxed(pcie->config_window_start,
> +		       pp->dbi_base + CFG_BASE_ADDRESS);
> +
> +	/*
> +	 * Open up memory to the PCI controller. We could do slightly
> +	 * better than this and exclude the kernel text segment and bss etc.
> +	 * They are base/limit registers so can be of arbitrary alignment
> +	 * presumably
> +	 */
> +	writel_relaxed(pcie->lmi->start, pp->dbi_base + IN0_MEM_ADDR_START);
> +	writel_relaxed(pcie->lmi->end, pp->dbi_base + IN0_MEM_ADDR_LIMIT);
> +
> +	/* Disable the 2nd region */
> +	writel_relaxed(~0, pp->dbi_base + IN1_MEM_ADDR_START);
> +	writel_relaxed(0, pp->dbi_base + IN1_MEM_ADDR_LIMIT);
> +
> +	writel_relaxed(RC_PASS_ADDR_RANGE, pp->dbi_base + TRANSLATION_CONTROL);
> +
> +	/* Now assert the board level reset to the other PCIe device */
> +	st_pcie_board_reset(pp);
> +}
> +
> +static irqreturn_t st_pcie_msi_irq_handler(int irq, void *arg)
> +{
> +	struct pcie_port *pp = arg;
> +
> +	return dw_handle_msi_irq(pp);
> +}
> +
> +static void st_msi_init_one(struct pcie_port *pp)
> +{
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +
> +	/*
> +	 * Set the magic address the hardware responds to. This has to be in
> +	 * the range the PCI controller can write to.
> +	 */
> +	dw_pcie_msi_init(pp);
> +
> +	if ((virt_to_phys((void *)pp->msi_data) < pcie->lmi->start) ||
> +	    (virt_to_phys((void *)pp->msi_data) > pcie->lmi->end))
> +		dev_err(pp->dev, "MSI addr miss-configured\n");
> +}
> +
> +static void st_pcie_host_init(struct pcie_port *pp)
> +{
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +	int err;
> +
> +	/*
> +	 * We have to initialise the PCIe cell on some hardware before we can
> +	 * talk to the phy
> +	 */
> +	err = pcie->data->init(pp);
> +	if (err)
> +		return;
> +
> +	err = pcie->data->disable_ltssm(pp);
> +	if (err) {
> +		dev_err(pp->dev, "disable ltssm failed, %d\n", err);
> +		return;
> +	}
> +
> +	if (!pcie->data->phy_auto) {
> +		/* Now init the associated miphy */
> +		err = phy_init(pcie->phy);
> +		if (err < 0) {
> +			dev_err(pp->dev, "Cannot init PHY: %d\n", err);
> +			return;
> +		}
> +	}
> +
> +	/* Now do all the register poking */
> +	st_pcie_hw_setup(pp);
> +
> +	/* Re-enable the link */
> +	err = pcie->data->enable_ltssm(pp);
> +	if (err)
> +		dev_err(pp->dev, "enable ltssm failed, %d\n", err);
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		st_msi_init_one(pp);
> +}
> +
> +static struct pcie_host_ops st_pcie_host_ops = {
> +	.rd_other_conf = st_pcie_rd_other_conf,
> +	.wr_other_conf = st_pcie_wr_other_conf,
> +	.link_up = st_pcie_link_up,
> +	.host_init = st_pcie_host_init,
> +};
> +
> +static int st_pcie_init(struct pcie_port *pp)
> +{
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +	int ret;
> +
> +	ret = reset_control_deassert(pcie->pwr);
> +	if (ret) {
> +		dev_err(pp->dev, "unable to bring out of powerdown\n");
> +		return ret;
> +	}
> +
> +	ret = reset_control_deassert(pcie->rst);
> +	if (ret) {
> +		dev_err(pp->dev, "unable to bring out of softreset\n");
> +		return ret;
> +	}
> +
> +	/* Set device type : Root Complex */
> +	ret = regmap_write(pcie->regmap, pcie->syscfg0, PCIE_DEVICE_TYPE);
> +	if (ret < 0) {
> +		dev_err(pp->dev, "unable to set device type\n");
> +		return ret;
> +	}
> +
> +	usleep_range(1000, 2000);
> +	return ret;
> +}
> +
> +/* STiH407 */
> +static int stih407_pcie_enable_ltssm(struct pcie_port *pp)
> +{
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +
> +	if (!pcie->syscfg1)
> +		return -EINVAL;
> +
> +	return regmap_update_bits(pcie->regmap, pcie->syscfg1,
> +				  PCIE_APP_LTSSM_ENABLE, PCIE_APP_LTSSM_ENABLE);
> +}
> +
> +static int stih407_pcie_disable_ltssm(struct pcie_port *pp)
> +{
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +
> +	if (!pcie->syscfg1)
> +		return -EINVAL;
> +
> +	return regmap_update_bits(pcie->regmap, pcie->syscfg1,
> +				  PCIE_APP_LTSSM_ENABLE, 0);
> +}
> +
> +static struct st_pcie_ops stih407_pcie_ops = {
> +	.init = st_pcie_init,
> +	.enable_ltssm = stih407_pcie_enable_ltssm,
> +	.disable_ltssm = stih407_pcie_disable_ltssm,
> +};
> +
> +static const struct of_device_id st_pcie_of_match[] = {
> +	{ .compatible = "st,stih407-pcie", .data = (void *)&stih407_pcie_ops},
> +	{ },
> +};
> +
> +#ifdef CONFIG_PM
> +static int st_pcie_suspend(struct device *pcie_dev)
> +{
> +	struct st_pcie *pcie = dev_get_drvdata(pcie_dev);
> +
> +	/* To guarantee a real phy initialization on resume */
> +	if (!pcie->data->phy_auto)
> +		phy_exit(pcie->phy);
> +
> +	return 0;
> +}
> +
> +static int st_pcie_resume(struct device *pcie_dev)
> +{
> +	struct st_pcie *pcie = dev_get_drvdata(pcie_dev);
> +
> +	st_pcie_host_init(&pcie->pp);
> +
> +	return 0;
> +}
> +#endif
> +
> +const struct dev_pm_ops st_pcie_pm_ops = {
> +	.suspend_noirq = st_pcie_suspend,
> +	.resume_noirq = st_pcie_resume,
> +};
> +
> +static int __init st_pcie_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct st_pcie *pcie;
> +	struct device_node *np = pdev->dev.of_node;
> +	struct pcie_port *pp;
> +	int ret;
> +
> +	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
> +	if (!pcie)
> +		return -ENOMEM;
> +
> +	memset(pcie, 0, sizeof(*pcie));
> +
> +	pp = &pcie->pp;
> +	pp->dev = &pdev->dev;
> +
> +	/* mem regions */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> +	pp->dbi_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(pp->dbi_base))
> +		return PTR_ERR(pp->dbi_base);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
> +	if (!res)
> +		return -ENXIO;
> +
> +	/* Check that this has sensible values */
> +	if ((resource_size(res) != CFG_REGION_SIZE) ||
> +	    (res->start & (CFG_REGION_SIZE - 1))) {
> +		dev_err(&pdev->dev, "Invalid config space properties\n");
> +		return -EINVAL;
> +	}
> +	pcie->config_window_start = res->start;
> +
> +	pcie->config_area = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(pcie->config_area))
> +		return PTR_ERR(pcie->config_area);
> +
> +	pcie->lmi = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +						 "mem-window");
> +	if (!pcie->lmi)
> +		return -ENXIO;
> +
> +	/* regmap registers for PCIe IP configuration */
> +	pcie->regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
> +	if (IS_ERR(pcie->regmap)) {
> +		dev_err(&pdev->dev, "No syscfg phandle specified\n");
> +		return PTR_ERR(pcie->regmap);
> +	}
> +
> +	ret = of_property_read_u32_index(np, "st,syscfg", SYSCFG0_REG,
> +					 &pcie->syscfg0);
> +	if (ret) {
> +		dev_err(&pdev->dev, "can't get syscfg0 offset (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32_index(np, "st,syscfg", SYSCFG1_REG,
> +					 &pcie->syscfg1);
> +	if (ret) {
> +		dev_err(&pdev->dev, "can't get syscfg1 offset (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	/* retrieve data opts from compatible */
> +	pcie->data = of_match_node(st_pcie_of_match, np)->data;
> +	if (!pcie->data) {
> +		dev_err(&pdev->dev, "compatible data not provided\n");
> +		return -EINVAL;
> +	}
> +
> +	/* powerdown / resets */
> +	pcie->pwr = devm_reset_control_get(&pdev->dev, "powerdown");
> +	if (IS_ERR(pcie->pwr)) {
> +		dev_err(&pdev->dev, "powerdown reset control not defined\n");
> +		return -EINVAL;

Please return PTR_ERR(pcie->pwr) instead of '-EINVAL'.

> +	}
> +
> +	pcie->rst = devm_reset_control_get(&pdev->dev, "softreset");
> +	if (IS_ERR(pcie->rst)) {
> +		dev_err(&pdev->dev, "Soft reset control not defined\n");
> +		return -EINVAL;

Please return PTR_ERR(pcie->pwr) instead of '-EINVAL'.

> +	}
> +
> +	/* phy */
> +	if (!pcie->data->phy_auto) {
> +		pcie->phy = devm_phy_get(&pdev->dev, "pcie_phy");
> +		if (IS_ERR(pcie->phy)) {
> +			dev_err(&pdev->dev, "no PHY configured\n");
> +			return PTR_ERR(pcie->phy);
> +		}
> +	}
> +
> +	/* Claim the GPIO for PRST# if available */
> +	pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> +	if (!gpio_is_valid(pcie->reset_gpio))
> +		dev_dbg(&pdev->dev, "No reset-gpio configured\n");
> +	else {
> +		ret = devm_gpio_request(&pdev->dev,
> +					pcie->reset_gpio,
> +					"PCIe reset");
> +		if (ret) {
> +			dev_err(&pdev->dev, "Cannot request reset-gpio %d\n",
> +				pcie->reset_gpio);
> +			return ret;
> +		}
> +	}
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +		pp->msi_irq = platform_get_irq_byname(pdev, "msi");
> +		if (pp->msi_irq <= 0) {
> +			dev_err(&pdev->dev, "failed to get MSI irq\n");
> +			return -ENODEV;
> +		}
> +
> +		ret = devm_request_irq(&pdev->dev, pp->msi_irq,
> +				       st_pcie_msi_irq_handler,
> +				       IRQF_SHARED, "st-pcie-msi", pp);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to request MSI irq\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	if (IS_ENABLED(CONFIG_ARM)) {
> +		/*
> +		 * We have to hook the abort handler so that we can intercept
> +		 * bus errors when doing config read/write that return UR,
> +		 * which is flagged up as a bus error
> +		 */
> +		hook_fault_code(16+6, st_pcie_abort_handler, SIGBUS, 0,
> +				"imprecise external abort");
> +	}
> +
> +	pp->root_bus_nr = -1;
> +	pp->ops = &st_pcie_host_ops;
> +
> +	ret = dw_pcie_host_init(pp);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to initialize host\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, pcie);
> +
> +	dev_info(&pdev->dev, "Initialized\n");
> +	return 0;
> +}
> +
> +MODULE_DEVICE_TABLE(of, st_pcie_of_match);
> +
> +static struct platform_driver st_pcie_driver = {
> +	.driver = {
> +		.name = "st-pcie",
> +		.of_match_table = st_pcie_of_match,
> +		.pm = &st_pcie_pm_ops,
> +	},
> +};
> +
> +/* ST PCIe driver does not allow module unload */
> +static int __init pcie_init(void)
> +{
> +	return platform_driver_probe(&st_pcie_driver, st_pcie_probe);
> +}
> +device_initcall(pcie_init);
> +
> +MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>");
> +MODULE_DESCRIPTION("PCI express Driver for ST SoCs");
> +MODULE_LICENSE("GPLv2");

Please add one space between 'GPL' and 'v2'.

s/"GPLv2"/"GPL v2"

According to the license_is_gpl_compatible(), the MODULE_LICENSE() string
for GPL v2 is "GPL v2" not "GPLv2".

Best regards,
Jingoo Han

> --
> 1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller

[Gabriel Fernandez]

[-- Attachment #1: Type: text/plain, Size: 30273 bytes --]

​Hi Jingoo,

Thanks for reviewing​

On 18 December 2014 at 07:03, Jingoo Han <jg1.han@samsung.com> wrote:

> On Wednesday, December 17, 2014 7:35 PM, Gabriel FERNANDEZ wrote:
> >
> > sti pcie is built around a Synopsis Designware PCIe IP.
> >
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
> > ---
> >  drivers/pci/host/Kconfig  |   5 +
> >  drivers/pci/host/Makefile |   1 +
> >  drivers/pci/host/pci-st.c | 713
> ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 719 insertions(+)
> >  create mode 100644 drivers/pci/host/pci-st.c
> >
> > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> > index c4b6568..999d2b9 100644
> > --- a/drivers/pci/host/Kconfig
> > +++ b/drivers/pci/host/Kconfig
> > @@ -102,4 +102,9 @@ config PCI_LAYERSCAPE
> >       help
> >         Say Y here if you want PCIe controller support on Layerscape
> SoCs.
> >
> > +config PCI_ST
> > +     bool "ST STiH41x PCIe controller"
> > +     depends on ARCH_STI
> > +     select PCIE_DW
> > +
> >  endmenu
> > diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> > index 44c2699..ca14829 100644
> > --- a/drivers/pci/host/Makefile
> > +++ b/drivers/pci/host/Makefile
> > @@ -12,3 +12,4 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o
> pci-keystone.o
> >  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
> >  obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
> >  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
> > +obj-$(CONFIG_PCI_ST) += pci-st.o
> > diff --git a/drivers/pci/host/pci-st.c b/drivers/pci/host/pci-st.c
> > new file mode 100644
> > index 0000000..bd3d32d
> > --- /dev/null
> > +++ b/drivers/pci/host/pci-st.c
> > @@ -0,0 +1,713 @@
> > +/*
> > + * Copyright (C) 2014 STMicroelectronics
> > + *
> > + * STMicroelectronics PCI express Driver for sti SoCs.
> > + * ST PCIe IPs are built around a Synopsys IP Core.
> > + *
> > + * Author: Fabrice Gasnier <fabrice.gasnier@st.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2, as
> > + * published by the Free Software Foundation.
> > + *
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_pci.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/delay.h>
> > +#include <linux/reset.h>
> > +#include <linux/phy/phy.h>
>
> Please, re-order these headers alphabetically.
> It enhances the readability.
>

​sure
​


>
> > +
> > +#include "pcie-designware.h"
> > +
> > +#define TRANSLATION_CONTROL          0x900
> > +/* Controls if area is inclusive or exclusive */
> > +#define RC_PASS_ADDR_RANGE           BIT(1)
> > +
> > +/* Base of area reserved for config accesses. Fixed size of 64K. */
> > +#define CFG_BASE_ADDRESS             0x92c
> > +#define CFG_REGION_SIZE                      65536
> > +
> > +/* First 4K of config space has this BDF (bus,device,function) */
> > +#define FUNC0_BDF_NUM                        0x930
> > +
> > +/* Mem regions */
> > +#define IN0_MEM_ADDR_START           0x964
> > +#define IN0_MEM_ADDR_LIMIT           0x968
> > +#define IN1_MEM_ADDR_START           0x974
> > +#define IN1_MEM_ADDR_LIMIT           0x978
> > +
> > +/* This actually contains the LTSSM state machine state */
> > +#define PORT_LOGIC_DEBUG_REG_0               0x728
> > +
> > +/* LTSSM state machine values        */
> > +#define DEBUG_REG_0_LTSSM_MASK               0x1f
> > +#define S_DETECT_QUIET                       0x00
> > +#define S_DETECT_ACT                 0x01
> > +#define S_POLL_ACTIVE                        0x02
> > +#define S_POLL_COMPLIANCE            0x03
> > +#define S_POLL_CONFIG                        0x04
> > +#define S_PRE_DETECT_QUIET           0x05
> > +#define S_DETECT_WAIT                        0x06
> > +#define S_CFG_LINKWD_START           0x07
> > +#define S_CFG_LINKWD_ACEPT           0x08
> > +#define S_CFG_LANENUM_WAIT           0x09
> > +#define S_CFG_LANENUM_ACEPT          0x0A
> > +#define S_CFG_COMPLETE                       0x0B
> > +#define S_CFG_IDLE                   0x0C
> > +#define S_RCVRY_LOCK                 0x0D
> > +#define S_RCVRY_SPEED                        0x0E
> > +#define S_RCVRY_RCVRCFG                      0x0F
> > +#define S_RCVRY_IDLE                 0x10
> > +#define S_L0                         0x11
> > +#define S_L0S                                0x12
> > +#define S_L123_SEND_EIDLE            0x13
> > +#define S_L1_IDLE                    0x14
> > +#define S_L2_IDLE                    0x15
> > +#define S_L2_WAKE                    0x16
> > +#define S_DISABLED_ENTRY             0x17
> > +#define S_DISABLED_IDLE                      0x18
> > +#define S_DISABLED                   0x19
> > +#define S_LPBK_ENTRY                 0x1A
> > +#define S_LPBK_ACTIVE                        0x1B
> > +#define S_LPBK_EXIT                  0x1C
> > +#define S_LPBK_EXIT_TIMEOUT          0x1D
> > +#define S_HOT_RESET_ENTRY            0x1E
> > +#define S_HOT_RESET                  0x1F
> > +
> > +/* syscfg bits */
> > +#define PCIE_SYS_INT                 BIT(5)
> > +#define PCIE_APP_REQ_RETRY_EN                BIT(3)
> > +#define PCIE_APP_LTSSM_ENABLE                BIT(2)
> > +#define PCIE_APP_INIT_RST            BIT(1)
> > +#define PCIE_DEVICE_TYPE             BIT(0)
> > +#define PCIE_DEFAULT_VAL             PCIE_DEVICE_TYPE
> > +
> > +/* Time to wait between testing the link in msecs (hardware poll
> interval) */
> > +#define LINK_LOOP_DELAY_MS 1
> > +/* Total amount of time to wait for the link to come up in msecs */
> > +#define LINK_WAIT_MS 120
> > +#define LINK_LOOP_COUNT (LINK_WAIT_MS / LINK_LOOP_DELAY_MS)
> > +
> > +/* st,syscfg offsets */
> > +#define SYSCFG0_REG  1
> > +#define SYSCFG1_REG  2
> > +
> > +#define to_st_pcie(x)        container_of(x, struct st_pcie, pp)
> > +
> > +/**
> > + * struct st_pcie_ops - SOC dependent data
> > + * @init: reference to controller power & reset init routine
> > + * @enable_ltssm: reference to controller link enable routine
> > + * @disable_ltssm:  reference to controller link disable routine
> > + * @phy_auto: flag when phy automatically configured
> > + */
> > +struct st_pcie_ops {
> > +     int (*init)(struct pcie_port *pp);
> > +     int (*enable_ltssm)(struct pcie_port *pp);
> > +     int (*disable_ltssm)(struct pcie_port *pp);
> > +     bool phy_auto;
> > +};
> > +
> > +/**
> > + * struct st_pcie - private data of the controller
> > + * @pp: designware pcie port
> > + * @syscfg0: PCIe configuration 0 register, regmap offset
> > + * @syscfg1: PCIe configuration 1 register, regmap offset
> > + * @phy: associated pcie phy
> > + * @config_area: PCIe configuration space
> > + * @lmi: memory made available to the controller
> > + * @data: SOC dependent data
> > + * @regmap: Syscfg registers bank in which PCIe port is configured
> > + * @pwr: power control
> > + * @rst: reset control
> > + * @reset_gpio: optional reset gpio
> > + * @config_window_start: start address of 64K config space area
> > + */
> > +struct st_pcie {
> > +     struct pcie_port pp;
> > +     int syscfg0;
> > +     int syscfg1;
> > +     struct phy *phy;
> > +     void __iomem *config_area;
> > +     struct resource *lmi;
> > +     const struct st_pcie_ops *data;
> > +     struct regmap *regmap;
> > +     struct reset_control *pwr;
> > +     struct reset_control *rst;
> > +     int reset_gpio;
> > +     phys_addr_t config_window_start;
> > +};
> > +
> > +/*
> > + * Function to test if the link is in an operational state or not. We
> must
> > + * ensure the link is operational before we try to do a configuration
> access.
> > + */
> > +static int st_pcie_link_up(struct pcie_port *pp)
> > +{
> > +     u32 status;
> > +     int link_up;
> > +     int count = 0;
> > +
> > +     /*
> > +      * We have to be careful here. This is used in config read/write,
> > +      * The higher levels switch off interrupts, so you cannot use
> > +      * jiffies to do a timeout, or reschedule
> > +      */
> > +     do {
> > +             /*
> > +              * What about L2? I think software intervention is
> > +              * required to get it out of L2, so in effect the link
> > +              * is down. Requires more work when/if we implement power
> > +              * management
> > +              */
> > +             status = readl_relaxed(pp->dbi_base +
> PORT_LOGIC_DEBUG_REG_0);
> > +             status &= DEBUG_REG_0_LTSSM_MASK;
> > +
> > +             link_up = (status == S_L0) || (status == S_L0S) ||
> > +                       (status == S_L1_IDLE);
> > +
> > +             /*
> > +              * It can take some considerable time for the link to
> actually
> > +              * come up, caused by the PLLs. Experiments indicate it
> takes
> > +              * about 8ms to actually bring the link up, but this can
> vary
> > +              * considerably according to the specification. This code
> should
> > +              * allow sufficient time
> > +              */
> > +             if (!link_up)
> > +                     mdelay(LINK_LOOP_DELAY_MS);
> > +
> > +     } while (!link_up && ++count < LINK_LOOP_COUNT);
> > +
> > +     return link_up;
> > +}
> > +
> > +/*
> > + * On ARM platforms, we actually get a bus error returned when the PCIe
> IP
> > + * returns a UR or CRS instead of an OK.
> > + */
> > +static int st_pcie_abort_handler(unsigned long addr, unsigned int fsr,
> > +                              struct pt_regs *regs)
> > +{
> > +     return 0;
> > +}
> > +
> > +/*
> > + * The PCI express core IP expects the following arrangement on it's
> address
> > + * bus (slv_haddr) when driving config cycles.
> > + * bus_number                [31:24]
> > + * dev_number                [23:19]
> > + * func_number               [18:16]
> > + * unused            [15:12]
> > + * ext_reg_number    [11:8]
> > + * reg_number                [7:2]
> > + *
> > + * Bits [15:12] are unused.
> > + *
> > + * In the glue logic there is a 64K region of address space that can be
> > + * written/read to generate config cycles. The base address of this is
> > + * controlled by CFG_BASE_ADDRESS. There are 8 16 bit registers called
> > + * FUNC0_BDF_NUM to FUNC8_BDF_NUM. These split the bottom half of the
> 64K
> > + * window into 8 regions at 4K boundaries. These control the bus,device
> and
> > + * function number you are trying to talk to.
> > + *
> > + * The decision on whether to generate a type 0 or type 1 access is
> controlled
> > + * by bits 15:12 of the address you write to.  If they are zero, then a
> type 0
> > + * is generated, if anything else it will be a type 1. Thus the bottom
> 4K
> > + * region controlled by FUNC0_BDF_NUM can only generate type 0, all the
> others
> > + * can only generate type 1.
> > + *
> > + * We only use FUNC0_BDF_NUM and FUNC1_BDF_NUM. Which one you use is
> selected
> > + * by bit 12 of the address you write to. The selected register is then
> used
> > + * for the top 16 bits of the slv_haddr to form the bus/dev/func, bit
> 15:12 are
> > + * wired to zero, and bits 11:2 form the address of the register you
> want to
> > + * read in config space.
> > + *
> > + * We always write FUNC0_BDF_NUM as a 32 bit write. So if we want type 1
> > + * accesses we have to shift by 16 so in effect we are writing to
> FUNC1_BDF_NUM
> > + */
> > +static inline u32 bdf_num(int bus, int devfn, int is_root_bus)
> > +{
> > +     return ((bus << 8) | devfn) << (is_root_bus ? 0 : 16);
> > +}
> > +
> > +static inline unsigned config_addr(int where, int is_root_bus)
> > +{
> > +     return (where & ~3) | (!is_root_bus << 12);
> > +}
> > +
> > +static int st_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus
> *bus,
> > +                              unsigned int devfn, int where, int size,
> > +                              u32 *val)
> > +{
> > +     u32 data;
> > +     u32 bdf;
> > +     struct st_pcie *pcie = to_st_pcie(pp);
> > +     int is_root_bus = pci_is_root_bus(bus);
> > +     int retry_count = 0;
> > +     int ret;
> > +     void __iomem *addr;
> > +
> > +     /*
> > +      * Prerequisite
> > +      * PCI express devices will respond to all config type 0 cycles,
> since
> > +      * they are point to point links. Thus to avoid probing for
> multiple
> > +      * devices on the root, dw-pcie already check for us if it is on
> the
> > +      * root bus / other slots. Also, dw-pcie checks for the link being
> up
> > +      * as we will hang if we issue a config request and the link is
> down.
> > +      * A switch will reject requests for slots it knows do not exist.
> > +      */
> > +     bdf = bdf_num(bus->number, devfn, is_root_bus);
> > +     addr = pcie->config_area + config_addr(where,
> > +                     bus->parent->number == pp->root_bus_nr);
> > +retry:
> > +     /* Set the config packet devfn */
> > +     writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
> > +     readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
> > +
> > +     ret = dw_pcie_cfg_read(addr, where, size, &data);
> > +
> > +     /*
> > +      * This is intended to help with when we are probing the bus. The
> > +      * problem is that the wrapper logic doesn't have any way to
> > +      * interrogate if the configuration request failed or not.
> > +      * On the ARM we actually get a real bus error.
> > +      *
> > +      * Unfortunately this means it is impossible to tell the difference
> > +      * between when a device doesn't exist (the switch will return a UR
> > +      * completion) or the device does exist but isn't yet ready to
> accept
> > +      * configuration requests (the device will return a CRS completion)
> > +      *
> > +      * The result of this is that we will miss devices when probing.
> > +      *
> > +      * So if we are trying to read the dev/vendor id on devfn 0 and we
> > +      * appear to get zero back, then we retry the request.  We know
> that
> > +      * zero can never be a valid device/vendor id. The specification
> says
> > +      * we must retry for up to a second before we decide the device is
> > +      * dead. If we are still dead then we assume there is nothing
> there and
> > +      * return ~0
> > +      *
> > +      * The downside of this is that we incur a delay of 1s for every
> pci
> > +      * express link that doesn't have a device connected.
> > +      */
> > +     if (((where & ~3) == 0) && devfn == 0 && (data == 0 || data ==
> ~0)) {
> > +             if (retry_count++ < 1000) {
> > +                     mdelay(1);
> > +                     goto retry;
> > +             } else {
> > +                     *val = ~0;
> > +                     return PCIBIOS_DEVICE_NOT_FOUND;
> > +             }
> > +     }
> > +
> > +     *val = data;
> > +     return ret;
> > +}
> > +
> > +static int st_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus
> *bus,
> > +                              unsigned int devfn, int where, int size,
> > +                              u32 val)
> > +{
> > +     u32 bdf;
> > +     struct st_pcie *pcie = to_st_pcie(pp);
> > +     int is_root_bus = pci_is_root_bus(bus);
> > +     void __iomem *addr;
> > +
> > +     bdf = bdf_num(bus->number, devfn, is_root_bus);
> > +     addr = pcie->config_area + config_addr(where,
> > +                     bus->parent->number == pp->root_bus_nr);
> > +
> > +     /* Set the config packet devfn */
> > +     writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
> > +     readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
> > +
> > +     return dw_pcie_cfg_write(addr, where, size, val);
> > +}
> > +
> > +static void st_pcie_board_reset(struct pcie_port *pp)
> > +{
> > +     struct st_pcie *pcie = to_st_pcie(pp);
> > +
> > +     if (!gpio_is_valid(pcie->reset_gpio))
> > +             return;
> > +
> > +     if (gpio_direction_output(pcie->reset_gpio, 0)) {
> > +             dev_err(pp->dev, "Cannot set PERST# (gpio %u) to output\n",
> > +                     pcie->reset_gpio);
> > +             return;
> > +     }
> > +
> > +     /* From PCIe spec */
> > +     usleep_range(1000, 2000);
> > +     gpio_direction_output(pcie->reset_gpio, 1);
> > +
> > +     /*
> > +      * PCIe specification states that you should not issue any config
> > +      * requests until 100ms after asserting reset, so we enforce that
> here
> > +      */
> > +     usleep_range(100000, 150000);
> > +}
> > +
> > +static void st_pcie_hw_setup(struct pcie_port *pp)
> > +{
> > +     struct st_pcie *pcie = to_st_pcie(pp);
> > +
> > +     dw_pcie_setup_rc(pp);
> > +
> > +     /* Set up the config window to the top of the PCI address space */
> > +     writel_relaxed(pcie->config_window_start,
> > +                    pp->dbi_base + CFG_BASE_ADDRESS);
> > +
> > +     /*
> > +      * Open up memory to the PCI controller. We could do slightly
> > +      * better than this and exclude the kernel text segment and bss
> etc.
> > +      * They are base/limit registers so can be of arbitrary alignment
> > +      * presumably
> > +      */
> > +     writel_relaxed(pcie->lmi->start, pp->dbi_base +
> IN0_MEM_ADDR_START);
> > +     writel_relaxed(pcie->lmi->end, pp->dbi_base + IN0_MEM_ADDR_LIMIT);
> > +
> > +     /* Disable the 2nd region */
> > +     writel_relaxed(~0, pp->dbi_base + IN1_MEM_ADDR_START);
> > +     writel_relaxed(0, pp->dbi_base + IN1_MEM_ADDR_LIMIT);
> > +
> > +     writel_relaxed(RC_PASS_ADDR_RANGE, pp->dbi_base +
> TRANSLATION_CONTROL);
> > +
> > +     /* Now assert the board level reset to the other PCIe device */
> > +     st_pcie_board_reset(pp);
> > +}
> > +
> > +static irqreturn_t st_pcie_msi_irq_handler(int irq, void *arg)
> > +{
> > +     struct pcie_port *pp = arg;
> > +
> > +     return dw_handle_msi_irq(pp);
> > +}
> > +
> > +static void st_msi_init_one(struct pcie_port *pp)
> > +{
> > +     struct st_pcie *pcie = to_st_pcie(pp);
> > +
> > +     /*
> > +      * Set the magic address the hardware responds to. This has to be
> in
> > +      * the range the PCI controller can write to.
> > +      */
> > +     dw_pcie_msi_init(pp);
> > +
> > +     if ((virt_to_phys((void *)pp->msi_data) < pcie->lmi->start) ||
> > +         (virt_to_phys((void *)pp->msi_data) > pcie->lmi->end))
> > +             dev_err(pp->dev, "MSI addr miss-configured\n");
> > +}
> > +
> > +static void st_pcie_host_init(struct pcie_port *pp)
> > +{
> > +     struct st_pcie *pcie = to_st_pcie(pp);
> > +     int err;
> > +
> > +     /*
> > +      * We have to initialise the PCIe cell on some hardware before we
> can
> > +      * talk to the phy
> > +      */
> > +     err = pcie->data->init(pp);
> > +     if (err)
> > +             return;
> > +
> > +     err = pcie->data->disable_ltssm(pp);
> > +     if (err) {
> > +             dev_err(pp->dev, "disable ltssm failed, %d\n", err);
> > +             return;
> > +     }
> > +
> > +     if (!pcie->data->phy_auto) {
> > +             /* Now init the associated miphy */
> > +             err = phy_init(pcie->phy);
> > +             if (err < 0) {
> > +                     dev_err(pp->dev, "Cannot init PHY: %d\n", err);
> > +                     return;
> > +             }
> > +     }
> > +
> > +     /* Now do all the register poking */
> > +     st_pcie_hw_setup(pp);
> > +
> > +     /* Re-enable the link */
> > +     err = pcie->data->enable_ltssm(pp);
> > +     if (err)
> > +             dev_err(pp->dev, "enable ltssm failed, %d\n", err);
> > +
> > +     if (IS_ENABLED(CONFIG_PCI_MSI))
> > +             st_msi_init_one(pp);
> > +}
> > +
> > +static struct pcie_host_ops st_pcie_host_ops = {
> > +     .rd_other_conf = st_pcie_rd_other_conf,
> > +     .wr_other_conf = st_pcie_wr_other_conf,
> > +     .link_up = st_pcie_link_up,
> > +     .host_init = st_pcie_host_init,
> > +};
> > +
> > +static int st_pcie_init(struct pcie_port *pp)
> > +{
> > +     struct st_pcie *pcie = to_st_pcie(pp);
> > +     int ret;
> > +
> > +     ret = reset_control_deassert(pcie->pwr);
> > +     if (ret) {
> > +             dev_err(pp->dev, "unable to bring out of powerdown\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = reset_control_deassert(pcie->rst);
> > +     if (ret) {
> > +             dev_err(pp->dev, "unable to bring out of softreset\n");
> > +             return ret;
> > +     }
> > +
> > +     /* Set device type : Root Complex */
> > +     ret = regmap_write(pcie->regmap, pcie->syscfg0, PCIE_DEVICE_TYPE);
> > +     if (ret < 0) {
> > +             dev_err(pp->dev, "unable to set device type\n");
> > +             return ret;
> > +     }
> > +
> > +     usleep_range(1000, 2000);
> > +     return ret;
> > +}
> > +
> > +/* STiH407 */
> > +static int stih407_pcie_enable_ltssm(struct pcie_port *pp)
> > +{
> > +     struct st_pcie *pcie = to_st_pcie(pp);
> > +
> > +     if (!pcie->syscfg1)
> > +             return -EINVAL;
> > +
> > +     return regmap_update_bits(pcie->regmap, pcie->syscfg1,
> > +                               PCIE_APP_LTSSM_ENABLE,
> PCIE_APP_LTSSM_ENABLE);
> > +}
> > +
> > +static int stih407_pcie_disable_ltssm(struct pcie_port *pp)
> > +{
> > +     struct st_pcie *pcie = to_st_pcie(pp);
> > +
> > +     if (!pcie->syscfg1)
> > +             return -EINVAL;
> > +
> > +     return regmap_update_bits(pcie->regmap, pcie->syscfg1,
> > +                               PCIE_APP_LTSSM_ENABLE, 0);
> > +}
> > +
> > +static struct st_pcie_ops stih407_pcie_ops = {
> > +     .init = st_pcie_init,
> > +     .enable_ltssm = stih407_pcie_enable_ltssm,
> > +     .disable_ltssm = stih407_pcie_disable_ltssm,
> > +};
> > +
> > +static const struct of_device_id st_pcie_of_match[] = {
> > +     { .compatible = "st,stih407-pcie", .data = (void
> *)&stih407_pcie_ops},
> > +     { },
> > +};
> > +
> > +#ifdef CONFIG_PM
> > +static int st_pcie_suspend(struct device *pcie_dev)
> > +{
> > +     struct st_pcie *pcie = dev_get_drvdata(pcie_dev);
> > +
> > +     /* To guarantee a real phy initialization on resume */
> > +     if (!pcie->data->phy_auto)
> > +             phy_exit(pcie->phy);
> > +
> > +     return 0;
> > +}
> > +
> > +static int st_pcie_resume(struct device *pcie_dev)
> > +{
> > +     struct st_pcie *pcie = dev_get_drvdata(pcie_dev);
> > +
> > +     st_pcie_host_init(&pcie->pp);
> > +
> > +     return 0;
> > +}
> > +#endif
> > +
> > +const struct dev_pm_ops st_pcie_pm_ops = {
> > +     .suspend_noirq = st_pcie_suspend,
> > +     .resume_noirq = st_pcie_resume,
> > +};
> > +
> > +static int __init st_pcie_probe(struct platform_device *pdev)
> > +{
> > +     struct resource *res;
> > +     struct st_pcie *pcie;
> > +     struct device_node *np = pdev->dev.of_node;
> > +     struct pcie_port *pp;
> > +     int ret;
> > +
> > +     pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
> > +     if (!pcie)
> > +             return -ENOMEM;
> > +
> > +     memset(pcie, 0, sizeof(*pcie));
> > +
> > +     pp = &pcie->pp;
> > +     pp->dev = &pdev->dev;
> > +
> > +     /* mem regions */
> > +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> > +     pp->dbi_base = devm_ioremap_resource(&pdev->dev, res);
> > +     if (IS_ERR(pp->dbi_base))
> > +             return PTR_ERR(pp->dbi_base);
> > +
> > +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
> > +     if (!res)
> > +             return -ENXIO;
> > +
> > +     /* Check that this has sensible values */
> > +     if ((resource_size(res) != CFG_REGION_SIZE) ||
> > +         (res->start & (CFG_REGION_SIZE - 1))) {
> > +             dev_err(&pdev->dev, "Invalid config space properties\n");
> > +             return -EINVAL;
> > +     }
> > +     pcie->config_window_start = res->start;
> > +
> > +     pcie->config_area = devm_ioremap_resource(&pdev->dev, res);
> > +     if (IS_ERR(pcie->config_area))
> > +             return PTR_ERR(pcie->config_area);
> > +
> > +     pcie->lmi = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > +                                              "mem-window");
> > +     if (!pcie->lmi)
> > +             return -ENXIO;
> > +
> > +     /* regmap registers for PCIe IP configuration */
> > +     pcie->regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
> > +     if (IS_ERR(pcie->regmap)) {
> > +             dev_err(&pdev->dev, "No syscfg phandle specified\n");
> > +             return PTR_ERR(pcie->regmap);
> > +     }
> > +
> > +     ret = of_property_read_u32_index(np, "st,syscfg", SYSCFG0_REG,
> > +                                      &pcie->syscfg0);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "can't get syscfg0 offset (%d)\n",
> ret);
> > +             return ret;
> > +     }
> > +
> > +     ret = of_property_read_u32_index(np, "st,syscfg", SYSCFG1_REG,
> > +                                      &pcie->syscfg1);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "can't get syscfg1 offset (%d)\n",
> ret);
> > +             return ret;
> > +     }
> > +
> > +     /* retrieve data opts from compatible */
> > +     pcie->data = of_match_node(st_pcie_of_match, np)->data;
> > +     if (!pcie->data) {
> > +             dev_err(&pdev->dev, "compatible data not provided\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     /* powerdown / resets */
> > +     pcie->pwr = devm_reset_control_get(&pdev->dev, "powerdown");
> > +     if (IS_ERR(pcie->pwr)) {
> > +             dev_err(&pdev->dev, "powerdown reset control not
> defined\n");
> > +             return -EINVAL;
>
> Please return PTR_ERR(pcie->pwr) instead of '-EINVAL'.
>

​ok
​


>
> > +     }
> > +
> > +     pcie->rst = devm_reset_control_get(&pdev->dev, "softreset");
> > +     if (IS_ERR(pcie->rst)) {
> > +             dev_err(&pdev->dev, "Soft reset control not defined\n");
> > +             return -EINVAL;
>
> Please return PTR_ERR(pcie->pwr) instead of '-EINVAL'.
>

​ok
​


>
> > +     }
> > +
> > +     /* phy */
> > +     if (!pcie->data->phy_auto) {
> > +             pcie->phy = devm_phy_get(&pdev->dev, "pcie_phy");
> > +             if (IS_ERR(pcie->phy)) {
> > +                     dev_err(&pdev->dev, "no PHY configured\n");
> > +                     return PTR_ERR(pcie->phy);
> > +             }
> > +     }
> > +
> > +     /* Claim the GPIO for PRST# if available */
> > +     pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> > +     if (!gpio_is_valid(pcie->reset_gpio))
> > +             dev_dbg(&pdev->dev, "No reset-gpio configured\n");
> > +     else {
> > +             ret = devm_gpio_request(&pdev->dev,
> > +                                     pcie->reset_gpio,
> > +                                     "PCIe reset");
> > +             if (ret) {
> > +                     dev_err(&pdev->dev, "Cannot request reset-gpio
> %d\n",
> > +                             pcie->reset_gpio);
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > +             pp->msi_irq = platform_get_irq_byname(pdev, "msi");
> > +             if (pp->msi_irq <= 0) {
> > +                     dev_err(&pdev->dev, "failed to get MSI irq\n");
> > +                     return -ENODEV;
> > +             }
> > +
> > +             ret = devm_request_irq(&pdev->dev, pp->msi_irq,
> > +                                    st_pcie_msi_irq_handler,
> > +                                    IRQF_SHARED, "st-pcie-msi", pp);
> > +             if (ret) {
> > +                     dev_err(&pdev->dev, "failed to request MSI irq\n");
> > +                     return -ENODEV;
> > +             }
> > +     }
> > +
> > +     if (IS_ENABLED(CONFIG_ARM)) {
> > +             /*
> > +              * We have to hook the abort handler so that we can
> intercept
> > +              * bus errors when doing config read/write that return UR,
> > +              * which is flagged up as a bus error
> > +              */
> > +             hook_fault_code(16+6, st_pcie_abort_handler, SIGBUS, 0,
> > +                             "imprecise external abort");
> > +     }
> > +
> > +     pp->root_bus_nr = -1;
> > +     pp->ops = &st_pcie_host_ops;
> > +
> > +     ret = dw_pcie_host_init(pp);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "failed to initialize host\n");
> > +             return ret;
> > +     }
> > +
> > +     platform_set_drvdata(pdev, pcie);
> > +
> > +     dev_info(&pdev->dev, "Initialized\n");
> > +     return 0;
> > +}
> > +
> > +MODULE_DEVICE_TABLE(of, st_pcie_of_match);
> > +
> > +static struct platform_driver st_pcie_driver = {
> > +     .driver = {
> > +             .name = "st-pcie",
> > +             .of_match_table = st_pcie_of_match,
> > +             .pm = &st_pcie_pm_ops,
> > +     },
> > +};
> > +
> > +/* ST PCIe driver does not allow module unload */
> > +static int __init pcie_init(void)
> > +{
> > +     return platform_driver_probe(&st_pcie_driver, st_pcie_probe);
> > +}
> > +device_initcall(pcie_init);
> > +
> > +MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
> > +MODULE_DESCRIPTION("PCI express Driver for ST SoCs");
> > +MODULE_LICENSE("GPLv2");
>
> Please add one space between 'GPL' and 'v2'.
>
> s/"GPLv2"/"GPL v2"
>
> According to the license_is_gpl_compatible(), the MODULE_LICENSE() string
> for GPL v2 is "GPL v2" not "GPLv2".
>
>
​Ok


Best regards,
> Jingoo Han
>
> > --
> > 1.9.1
>

​BR

Gabriel​

[-- Attachment #2: Type: text/html, Size: 40328 bytes --]

Re: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller

[Gabriel Fernandez]

Hi Jingoo,

Thanks for reviewing

On 18 December 2014 at 07:03, Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> On Wednesday, December 17, 2014 7:35 PM, Gabriel FERNANDEZ wrote:
>>
>> sti pcie is built around a Synopsis Designware PCIe IP.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>
>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  drivers/pci/host/Kconfig  |   5 +
>>  drivers/pci/host/Makefile |   1 +
>>  drivers/pci/host/pci-st.c | 713 ++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 719 insertions(+)
>>  create mode 100644 drivers/pci/host/pci-st.c
>>
>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index c4b6568..999d2b9 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -102,4 +102,9 @@ config PCI_LAYERSCAPE
>>       help
>>         Say Y here if you want PCIe controller support on Layerscape SoCs.
>>
>> +config PCI_ST
>> +     bool "ST STiH41x PCIe controller"
>> +     depends on ARCH_STI
>> +     select PCIE_DW
>> +
>>  endmenu
>> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
>> index 44c2699..ca14829 100644
>> --- a/drivers/pci/host/Makefile
>> +++ b/drivers/pci/host/Makefile
>> @@ -12,3 +12,4 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
>>  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
>>  obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
>>  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
>> +obj-$(CONFIG_PCI_ST) += pci-st.o
>> diff --git a/drivers/pci/host/pci-st.c b/drivers/pci/host/pci-st.c
>> new file mode 100644
>> index 0000000..bd3d32d
>> --- /dev/null
>> +++ b/drivers/pci/host/pci-st.c
>> @@ -0,0 +1,713 @@
>> +/*
>> + * Copyright (C) 2014 STMicroelectronics
>> + *
>> + * STMicroelectronics PCI express Driver for sti SoCs.
>> + * ST PCIe IPs are built around a Synopsys IP Core.
>> + *
>> + * Author: Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2, as
>> + * published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_pci.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/delay.h>
>> +#include <linux/reset.h>
>> +#include <linux/phy/phy.h>
>
> Please, re-order these headers alphabetically.
> It enhances the readability.
>

sure

>> +
>> +#include "pcie-designware.h"
>> +
>> +#define TRANSLATION_CONTROL          0x900
>> +/* Controls if area is inclusive or exclusive */
>> +#define RC_PASS_ADDR_RANGE           BIT(1)
>> +
>> +/* Base of area reserved for config accesses. Fixed size of 64K. */
>> +#define CFG_BASE_ADDRESS             0x92c
>> +#define CFG_REGION_SIZE                      65536
>> +
>> +/* First 4K of config space has this BDF (bus,device,function) */
>> +#define FUNC0_BDF_NUM                        0x930
>> +
>> +/* Mem regions */
>> +#define IN0_MEM_ADDR_START           0x964
>> +#define IN0_MEM_ADDR_LIMIT           0x968
>> +#define IN1_MEM_ADDR_START           0x974
>> +#define IN1_MEM_ADDR_LIMIT           0x978
>> +
>> +/* This actually contains the LTSSM state machine state */
>> +#define PORT_LOGIC_DEBUG_REG_0               0x728
>> +
>> +/* LTSSM state machine values        */
>> +#define DEBUG_REG_0_LTSSM_MASK               0x1f
>> +#define S_DETECT_QUIET                       0x00
>> +#define S_DETECT_ACT                 0x01
>> +#define S_POLL_ACTIVE                        0x02
>> +#define S_POLL_COMPLIANCE            0x03
>> +#define S_POLL_CONFIG                        0x04
>> +#define S_PRE_DETECT_QUIET           0x05
>> +#define S_DETECT_WAIT                        0x06
>> +#define S_CFG_LINKWD_START           0x07
>> +#define S_CFG_LINKWD_ACEPT           0x08
>> +#define S_CFG_LANENUM_WAIT           0x09
>> +#define S_CFG_LANENUM_ACEPT          0x0A
>> +#define S_CFG_COMPLETE                       0x0B
>> +#define S_CFG_IDLE                   0x0C
>> +#define S_RCVRY_LOCK                 0x0D
>> +#define S_RCVRY_SPEED                        0x0E
>> +#define S_RCVRY_RCVRCFG                      0x0F
>> +#define S_RCVRY_IDLE                 0x10
>> +#define S_L0                         0x11
>> +#define S_L0S                                0x12
>> +#define S_L123_SEND_EIDLE            0x13
>> +#define S_L1_IDLE                    0x14
>> +#define S_L2_IDLE                    0x15
>> +#define S_L2_WAKE                    0x16
>> +#define S_DISABLED_ENTRY             0x17
>> +#define S_DISABLED_IDLE                      0x18
>> +#define S_DISABLED                   0x19
>> +#define S_LPBK_ENTRY                 0x1A
>> +#define S_LPBK_ACTIVE                        0x1B
>> +#define S_LPBK_EXIT                  0x1C
>> +#define S_LPBK_EXIT_TIMEOUT          0x1D
>> +#define S_HOT_RESET_ENTRY            0x1E
>> +#define S_HOT_RESET                  0x1F
>> +
>> +/* syscfg bits */
>> +#define PCIE_SYS_INT                 BIT(5)
>> +#define PCIE_APP_REQ_RETRY_EN                BIT(3)
>> +#define PCIE_APP_LTSSM_ENABLE                BIT(2)
>> +#define PCIE_APP_INIT_RST            BIT(1)
>> +#define PCIE_DEVICE_TYPE             BIT(0)
>> +#define PCIE_DEFAULT_VAL             PCIE_DEVICE_TYPE
>> +
>> +/* Time to wait between testing the link in msecs (hardware poll interval) */
>> +#define LINK_LOOP_DELAY_MS 1
>> +/* Total amount of time to wait for the link to come up in msecs */
>> +#define LINK_WAIT_MS 120
>> +#define LINK_LOOP_COUNT (LINK_WAIT_MS / LINK_LOOP_DELAY_MS)
>> +
>> +/* st,syscfg offsets */
>> +#define SYSCFG0_REG  1
>> +#define SYSCFG1_REG  2
>> +
>> +#define to_st_pcie(x)        container_of(x, struct st_pcie, pp)
>> +
>> +/**
>> + * struct st_pcie_ops - SOC dependent data
>> + * @init: reference to controller power & reset init routine
>> + * @enable_ltssm: reference to controller link enable routine
>> + * @disable_ltssm:  reference to controller link disable routine
>> + * @phy_auto: flag when phy automatically configured
>> + */
>> +struct st_pcie_ops {
>> +     int (*init)(struct pcie_port *pp);
>> +     int (*enable_ltssm)(struct pcie_port *pp);
>> +     int (*disable_ltssm)(struct pcie_port *pp);
>> +     bool phy_auto;
>> +};
>> +
>> +/**
>> + * struct st_pcie - private data of the controller
>> + * @pp: designware pcie port
>> + * @syscfg0: PCIe configuration 0 register, regmap offset
>> + * @syscfg1: PCIe configuration 1 register, regmap offset
>> + * @phy: associated pcie phy
>> + * @config_area: PCIe configuration space
>> + * @lmi: memory made available to the controller
>> + * @data: SOC dependent data
>> + * @regmap: Syscfg registers bank in which PCIe port is configured
>> + * @pwr: power control
>> + * @rst: reset control
>> + * @reset_gpio: optional reset gpio
>> + * @config_window_start: start address of 64K config space area
>> + */
>> +struct st_pcie {
>> +     struct pcie_port pp;
>> +     int syscfg0;
>> +     int syscfg1;
>> +     struct phy *phy;
>> +     void __iomem *config_area;
>> +     struct resource *lmi;
>> +     const struct st_pcie_ops *data;
>> +     struct regmap *regmap;
>> +     struct reset_control *pwr;
>> +     struct reset_control *rst;
>> +     int reset_gpio;
>> +     phys_addr_t config_window_start;
>> +};
>> +
>> +/*
>> + * Function to test if the link is in an operational state or not. We must
>> + * ensure the link is operational before we try to do a configuration access.
>> + */
>> +static int st_pcie_link_up(struct pcie_port *pp)
>> +{
>> +     u32 status;
>> +     int link_up;
>> +     int count = 0;
>> +
>> +     /*
>> +      * We have to be careful here. This is used in config read/write,
>> +      * The higher levels switch off interrupts, so you cannot use
>> +      * jiffies to do a timeout, or reschedule
>> +      */
>> +     do {
>> +             /*
>> +              * What about L2? I think software intervention is
>> +              * required to get it out of L2, so in effect the link
>> +              * is down. Requires more work when/if we implement power
>> +              * management
>> +              */
>> +             status = readl_relaxed(pp->dbi_base + PORT_LOGIC_DEBUG_REG_0);
>> +             status &= DEBUG_REG_0_LTSSM_MASK;
>> +
>> +             link_up = (status == S_L0) || (status == S_L0S) ||
>> +                       (status == S_L1_IDLE);
>> +
>> +             /*
>> +              * It can take some considerable time for the link to actually
>> +              * come up, caused by the PLLs. Experiments indicate it takes
>> +              * about 8ms to actually bring the link up, but this can vary
>> +              * considerably according to the specification. This code should
>> +              * allow sufficient time
>> +              */
>> +             if (!link_up)
>> +                     mdelay(LINK_LOOP_DELAY_MS);
>> +
>> +     } while (!link_up && ++count < LINK_LOOP_COUNT);
>> +
>> +     return link_up;
>> +}
>> +
>> +/*
>> + * On ARM platforms, we actually get a bus error returned when the PCIe IP
>> + * returns a UR or CRS instead of an OK.
>> + */
>> +static int st_pcie_abort_handler(unsigned long addr, unsigned int fsr,
>> +                              struct pt_regs *regs)
>> +{
>> +     return 0;
>> +}
>> +
>> +/*
>> + * The PCI express core IP expects the following arrangement on it's address
>> + * bus (slv_haddr) when driving config cycles.
>> + * bus_number                [31:24]
>> + * dev_number                [23:19]
>> + * func_number               [18:16]
>> + * unused            [15:12]
>> + * ext_reg_number    [11:8]
>> + * reg_number                [7:2]
>> + *
>> + * Bits [15:12] are unused.
>> + *
>> + * In the glue logic there is a 64K region of address space that can be
>> + * written/read to generate config cycles. The base address of this is
>> + * controlled by CFG_BASE_ADDRESS. There are 8 16 bit registers called
>> + * FUNC0_BDF_NUM to FUNC8_BDF_NUM. These split the bottom half of the 64K
>> + * window into 8 regions at 4K boundaries. These control the bus,device and
>> + * function number you are trying to talk to.
>> + *
>> + * The decision on whether to generate a type 0 or type 1 access is controlled
>> + * by bits 15:12 of the address you write to.  If they are zero, then a type 0
>> + * is generated, if anything else it will be a type 1. Thus the bottom 4K
>> + * region controlled by FUNC0_BDF_NUM can only generate type 0, all the others
>> + * can only generate type 1.
>> + *
>> + * We only use FUNC0_BDF_NUM and FUNC1_BDF_NUM. Which one you use is selected
>> + * by bit 12 of the address you write to. The selected register is then used
>> + * for the top 16 bits of the slv_haddr to form the bus/dev/func, bit 15:12 are
>> + * wired to zero, and bits 11:2 form the address of the register you want to
>> + * read in config space.
>> + *
>> + * We always write FUNC0_BDF_NUM as a 32 bit write. So if we want type 1
>> + * accesses we have to shift by 16 so in effect we are writing to FUNC1_BDF_NUM
>> + */
>> +static inline u32 bdf_num(int bus, int devfn, int is_root_bus)
>> +{
>> +     return ((bus << 8) | devfn) << (is_root_bus ? 0 : 16);
>> +}
>> +
>> +static inline unsigned config_addr(int where, int is_root_bus)
>> +{
>> +     return (where & ~3) | (!is_root_bus << 12);
>> +}
>> +
>> +static int st_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>> +                              unsigned int devfn, int where, int size,
>> +                              u32 *val)
>> +{
>> +     u32 data;
>> +     u32 bdf;
>> +     struct st_pcie *pcie = to_st_pcie(pp);
>> +     int is_root_bus = pci_is_root_bus(bus);
>> +     int retry_count = 0;
>> +     int ret;
>> +     void __iomem *addr;
>> +
>> +     /*
>> +      * Prerequisite
>> +      * PCI express devices will respond to all config type 0 cycles, since
>> +      * they are point to point links. Thus to avoid probing for multiple
>> +      * devices on the root, dw-pcie already check for us if it is on the
>> +      * root bus / other slots. Also, dw-pcie checks for the link being up
>> +      * as we will hang if we issue a config request and the link is down.
>> +      * A switch will reject requests for slots it knows do not exist.
>> +      */
>> +     bdf = bdf_num(bus->number, devfn, is_root_bus);
>> +     addr = pcie->config_area + config_addr(where,
>> +                     bus->parent->number == pp->root_bus_nr);
>> +retry:
>> +     /* Set the config packet devfn */
>> +     writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
>> +     readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
>> +
>> +     ret = dw_pcie_cfg_read(addr, where, size, &data);
>> +
>> +     /*
>> +      * This is intended to help with when we are probing the bus. The
>> +      * problem is that the wrapper logic doesn't have any way to
>> +      * interrogate if the configuration request failed or not.
>> +      * On the ARM we actually get a real bus error.
>> +      *
>> +      * Unfortunately this means it is impossible to tell the difference
>> +      * between when a device doesn't exist (the switch will return a UR
>> +      * completion) or the device does exist but isn't yet ready to accept
>> +      * configuration requests (the device will return a CRS completion)
>> +      *
>> +      * The result of this is that we will miss devices when probing.
>> +      *
>> +      * So if we are trying to read the dev/vendor id on devfn 0 and we
>> +      * appear to get zero back, then we retry the request.  We know that
>> +      * zero can never be a valid device/vendor id. The specification says
>> +      * we must retry for up to a second before we decide the device is
>> +      * dead. If we are still dead then we assume there is nothing there and
>> +      * return ~0
>> +      *
>> +      * The downside of this is that we incur a delay of 1s for every pci
>> +      * express link that doesn't have a device connected.
>> +      */
>> +     if (((where & ~3) == 0) && devfn == 0 && (data == 0 || data == ~0)) {
>> +             if (retry_count++ < 1000) {
>> +                     mdelay(1);
>> +                     goto retry;
>> +             } else {
>> +                     *val = ~0;
>> +                     return PCIBIOS_DEVICE_NOT_FOUND;
>> +             }
>> +     }
>> +
>> +     *val = data;
>> +     return ret;
>> +}
>> +
>> +static int st_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>> +                              unsigned int devfn, int where, int size,
>> +                              u32 val)
>> +{
>> +     u32 bdf;
>> +     struct st_pcie *pcie = to_st_pcie(pp);
>> +     int is_root_bus = pci_is_root_bus(bus);
>> +     void __iomem *addr;
>> +
>> +     bdf = bdf_num(bus->number, devfn, is_root_bus);
>> +     addr = pcie->config_area + config_addr(where,
>> +                     bus->parent->number == pp->root_bus_nr);
>> +
>> +     /* Set the config packet devfn */
>> +     writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
>> +     readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
>> +
>> +     return dw_pcie_cfg_write(addr, where, size, val);
>> +}
>> +
>> +static void st_pcie_board_reset(struct pcie_port *pp)
>> +{
>> +     struct st_pcie *pcie = to_st_pcie(pp);
>> +
>> +     if (!gpio_is_valid(pcie->reset_gpio))
>> +             return;
>> +
>> +     if (gpio_direction_output(pcie->reset_gpio, 0)) {
>> +             dev_err(pp->dev, "Cannot set PERST# (gpio %u) to output\n",
>> +                     pcie->reset_gpio);
>> +             return;
>> +     }
>> +
>> +     /* From PCIe spec */
>> +     usleep_range(1000, 2000);
>> +     gpio_direction_output(pcie->reset_gpio, 1);
>> +
>> +     /*
>> +      * PCIe specification states that you should not issue any config
>> +      * requests until 100ms after asserting reset, so we enforce that here
>> +      */
>> +     usleep_range(100000, 150000);
>> +}
>> +
>> +static void st_pcie_hw_setup(struct pcie_port *pp)
>> +{
>> +     struct st_pcie *pcie = to_st_pcie(pp);
>> +
>> +     dw_pcie_setup_rc(pp);
>> +
>> +     /* Set up the config window to the top of the PCI address space */
>> +     writel_relaxed(pcie->config_window_start,
>> +                    pp->dbi_base + CFG_BASE_ADDRESS);
>> +
>> +     /*
>> +      * Open up memory to the PCI controller. We could do slightly
>> +      * better than this and exclude the kernel text segment and bss etc.
>> +      * They are base/limit registers so can be of arbitrary alignment
>> +      * presumably
>> +      */
>> +     writel_relaxed(pcie->lmi->start, pp->dbi_base + IN0_MEM_ADDR_START);
>> +     writel_relaxed(pcie->lmi->end, pp->dbi_base + IN0_MEM_ADDR_LIMIT);
>> +
>> +     /* Disable the 2nd region */
>> +     writel_relaxed(~0, pp->dbi_base + IN1_MEM_ADDR_START);
>> +     writel_relaxed(0, pp->dbi_base + IN1_MEM_ADDR_LIMIT);
>> +
>> +     writel_relaxed(RC_PASS_ADDR_RANGE, pp->dbi_base + TRANSLATION_CONTROL);
>> +
>> +     /* Now assert the board level reset to the other PCIe device */
>> +     st_pcie_board_reset(pp);
>> +}
>> +
>> +static irqreturn_t st_pcie_msi_irq_handler(int irq, void *arg)
>> +{
>> +     struct pcie_port *pp = arg;
>> +
>> +     return dw_handle_msi_irq(pp);
>> +}
>> +
>> +static void st_msi_init_one(struct pcie_port *pp)
>> +{
>> +     struct st_pcie *pcie = to_st_pcie(pp);
>> +
>> +     /*
>> +      * Set the magic address the hardware responds to. This has to be in
>> +      * the range the PCI controller can write to.
>> +      */
>> +     dw_pcie_msi_init(pp);
>> +
>> +     if ((virt_to_phys((void *)pp->msi_data) < pcie->lmi->start) ||
>> +         (virt_to_phys((void *)pp->msi_data) > pcie->lmi->end))
>> +             dev_err(pp->dev, "MSI addr miss-configured\n");
>> +}
>> +
>> +static void st_pcie_host_init(struct pcie_port *pp)
>> +{
>> +     struct st_pcie *pcie = to_st_pcie(pp);
>> +     int err;
>> +
>> +     /*
>> +      * We have to initialise the PCIe cell on some hardware before we can
>> +      * talk to the phy
>> +      */
>> +     err = pcie->data->init(pp);
>> +     if (err)
>> +             return;
>> +
>> +     err = pcie->data->disable_ltssm(pp);
>> +     if (err) {
>> +             dev_err(pp->dev, "disable ltssm failed, %d\n", err);
>> +             return;
>> +     }
>> +
>> +     if (!pcie->data->phy_auto) {
>> +             /* Now init the associated miphy */
>> +             err = phy_init(pcie->phy);
>> +             if (err < 0) {
>> +                     dev_err(pp->dev, "Cannot init PHY: %d\n", err);
>> +                     return;
>> +             }
>> +     }
>> +
>> +     /* Now do all the register poking */
>> +     st_pcie_hw_setup(pp);
>> +
>> +     /* Re-enable the link */
>> +     err = pcie->data->enable_ltssm(pp);
>> +     if (err)
>> +             dev_err(pp->dev, "enable ltssm failed, %d\n", err);
>> +
>> +     if (IS_ENABLED(CONFIG_PCI_MSI))
>> +             st_msi_init_one(pp);
>> +}
>> +
>> +static struct pcie_host_ops st_pcie_host_ops = {
>> +     .rd_other_conf = st_pcie_rd_other_conf,
>> +     .wr_other_conf = st_pcie_wr_other_conf,
>> +     .link_up = st_pcie_link_up,
>> +     .host_init = st_pcie_host_init,
>> +};
>> +
>> +static int st_pcie_init(struct pcie_port *pp)
>> +{
>> +     struct st_pcie *pcie = to_st_pcie(pp);
>> +     int ret;
>> +
>> +     ret = reset_control_deassert(pcie->pwr);
>> +     if (ret) {
>> +             dev_err(pp->dev, "unable to bring out of powerdown\n");
>> +             return ret;
>> +     }
>> +
>> +     ret = reset_control_deassert(pcie->rst);
>> +     if (ret) {
>> +             dev_err(pp->dev, "unable to bring out of softreset\n");
>> +             return ret;
>> +     }
>> +
>> +     /* Set device type : Root Complex */
>> +     ret = regmap_write(pcie->regmap, pcie->syscfg0, PCIE_DEVICE_TYPE);
>> +     if (ret < 0) {
>> +             dev_err(pp->dev, "unable to set device type\n");
>> +             return ret;
>> +     }
>> +
>> +     usleep_range(1000, 2000);
>> +     return ret;
>> +}
>> +
>> +/* STiH407 */
>> +static int stih407_pcie_enable_ltssm(struct pcie_port *pp)
>> +{
>> +     struct st_pcie *pcie = to_st_pcie(pp);
>> +
>> +     if (!pcie->syscfg1)
>> +             return -EINVAL;
>> +
>> +     return regmap_update_bits(pcie->regmap, pcie->syscfg1,
>> +                               PCIE_APP_LTSSM_ENABLE, PCIE_APP_LTSSM_ENABLE);
>> +}
>> +
>> +static int stih407_pcie_disable_ltssm(struct pcie_port *pp)
>> +{
>> +     struct st_pcie *pcie = to_st_pcie(pp);
>> +
>> +     if (!pcie->syscfg1)
>> +             return -EINVAL;
>> +
>> +     return regmap_update_bits(pcie->regmap, pcie->syscfg1,
>> +                               PCIE_APP_LTSSM_ENABLE, 0);
>> +}
>> +
>> +static struct st_pcie_ops stih407_pcie_ops = {
>> +     .init = st_pcie_init,
>> +     .enable_ltssm = stih407_pcie_enable_ltssm,
>> +     .disable_ltssm = stih407_pcie_disable_ltssm,
>> +};
>> +
>> +static const struct of_device_id st_pcie_of_match[] = {
>> +     { .compatible = "st,stih407-pcie", .data = (void *)&stih407_pcie_ops},
>> +     { },
>> +};
>> +
>> +#ifdef CONFIG_PM
>> +static int st_pcie_suspend(struct device *pcie_dev)
>> +{
>> +     struct st_pcie *pcie = dev_get_drvdata(pcie_dev);
>> +
>> +     /* To guarantee a real phy initialization on resume */
>> +     if (!pcie->data->phy_auto)
>> +             phy_exit(pcie->phy);
>> +
>> +     return 0;
>> +}
>> +
>> +static int st_pcie_resume(struct device *pcie_dev)
>> +{
>> +     struct st_pcie *pcie = dev_get_drvdata(pcie_dev);
>> +
>> +     st_pcie_host_init(&pcie->pp);
>> +
>> +     return 0;
>> +}
>> +#endif
>> +
>> +const struct dev_pm_ops st_pcie_pm_ops = {
>> +     .suspend_noirq = st_pcie_suspend,
>> +     .resume_noirq = st_pcie_resume,
>> +};
>> +
>> +static int __init st_pcie_probe(struct platform_device *pdev)
>> +{
>> +     struct resource *res;
>> +     struct st_pcie *pcie;
>> +     struct device_node *np = pdev->dev.of_node;
>> +     struct pcie_port *pp;
>> +     int ret;
>> +
>> +     pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
>> +     if (!pcie)
>> +             return -ENOMEM;
>> +
>> +     memset(pcie, 0, sizeof(*pcie));
>> +
>> +     pp = &pcie->pp;
>> +     pp->dev = &pdev->dev;
>> +
>> +     /* mem regions */
>> +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
>> +     pp->dbi_base = devm_ioremap_resource(&pdev->dev, res);
>> +     if (IS_ERR(pp->dbi_base))
>> +             return PTR_ERR(pp->dbi_base);
>> +
>> +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
>> +     if (!res)
>> +             return -ENXIO;
>> +
>> +     /* Check that this has sensible values */
>> +     if ((resource_size(res) != CFG_REGION_SIZE) ||
>> +         (res->start & (CFG_REGION_SIZE - 1))) {
>> +             dev_err(&pdev->dev, "Invalid config space properties\n");
>> +             return -EINVAL;
>> +     }
>> +     pcie->config_window_start = res->start;
>> +
>> +     pcie->config_area = devm_ioremap_resource(&pdev->dev, res);
>> +     if (IS_ERR(pcie->config_area))
>> +             return PTR_ERR(pcie->config_area);
>> +
>> +     pcie->lmi = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> +                                              "mem-window");
>> +     if (!pcie->lmi)
>> +             return -ENXIO;
>> +
>> +     /* regmap registers for PCIe IP configuration */
>> +     pcie->regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
>> +     if (IS_ERR(pcie->regmap)) {
>> +             dev_err(&pdev->dev, "No syscfg phandle specified\n");
>> +             return PTR_ERR(pcie->regmap);
>> +     }
>> +
>> +     ret = of_property_read_u32_index(np, "st,syscfg", SYSCFG0_REG,
>> +                                      &pcie->syscfg0);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "can't get syscfg0 offset (%d)\n", ret);
>> +             return ret;
>> +     }
>> +
>> +     ret = of_property_read_u32_index(np, "st,syscfg", SYSCFG1_REG,
>> +                                      &pcie->syscfg1);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "can't get syscfg1 offset (%d)\n", ret);
>> +             return ret;
>> +     }
>> +
>> +     /* retrieve data opts from compatible */
>> +     pcie->data = of_match_node(st_pcie_of_match, np)->data;
>> +     if (!pcie->data) {
>> +             dev_err(&pdev->dev, "compatible data not provided\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* powerdown / resets */
>> +     pcie->pwr = devm_reset_control_get(&pdev->dev, "powerdown");
>> +     if (IS_ERR(pcie->pwr)) {
>> +             dev_err(&pdev->dev, "powerdown reset control not defined\n");
>> +             return -EINVAL;
>
> Please return PTR_ERR(pcie->pwr) instead of '-EINVAL'.
>
ok

>> +     }
>> +
>> +     pcie->rst = devm_reset_control_get(&pdev->dev, "softreset");
>> +     if (IS_ERR(pcie->rst)) {
>> +             dev_err(&pdev->dev, "Soft reset control not defined\n");
>> +             return -EINVAL;
>
> Please return PTR_ERR(pcie->pwr) instead of '-EINVAL'.
>
ok

>> +     }
>> +
>> +     /* phy */
>> +     if (!pcie->data->phy_auto) {
>> +             pcie->phy = devm_phy_get(&pdev->dev, "pcie_phy");
>> +             if (IS_ERR(pcie->phy)) {
>> +                     dev_err(&pdev->dev, "no PHY configured\n");
>> +                     return PTR_ERR(pcie->phy);
>> +             }
>> +     }
>> +
>> +     /* Claim the GPIO for PRST# if available */
>> +     pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
>> +     if (!gpio_is_valid(pcie->reset_gpio))
>> +             dev_dbg(&pdev->dev, "No reset-gpio configured\n");
>> +     else {
>> +             ret = devm_gpio_request(&pdev->dev,
>> +                                     pcie->reset_gpio,
>> +                                     "PCIe reset");
>> +             if (ret) {
>> +                     dev_err(&pdev->dev, "Cannot request reset-gpio %d\n",
>> +                             pcie->reset_gpio);
>> +                     return ret;
>> +             }
>> +     }
>> +
>> +     if (IS_ENABLED(CONFIG_PCI_MSI)) {
>> +             pp->msi_irq = platform_get_irq_byname(pdev, "msi");
>> +             if (pp->msi_irq <= 0) {
>> +                     dev_err(&pdev->dev, "failed to get MSI irq\n");
>> +                     return -ENODEV;
>> +             }
>> +
>> +             ret = devm_request_irq(&pdev->dev, pp->msi_irq,
>> +                                    st_pcie_msi_irq_handler,
>> +                                    IRQF_SHARED, "st-pcie-msi", pp);
>> +             if (ret) {
>> +                     dev_err(&pdev->dev, "failed to request MSI irq\n");
>> +                     return -ENODEV;
>> +             }
>> +     }
>> +
>> +     if (IS_ENABLED(CONFIG_ARM)) {
>> +             /*
>> +              * We have to hook the abort handler so that we can intercept
>> +              * bus errors when doing config read/write that return UR,
>> +              * which is flagged up as a bus error
>> +              */
>> +             hook_fault_code(16+6, st_pcie_abort_handler, SIGBUS, 0,
>> +                             "imprecise external abort");
>> +     }
>> +
>> +     pp->root_bus_nr = -1;
>> +     pp->ops = &st_pcie_host_ops;
>> +
>> +     ret = dw_pcie_host_init(pp);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "failed to initialize host\n");
>> +             return ret;
>> +     }
>> +
>> +     platform_set_drvdata(pdev, pcie);
>> +
>> +     dev_info(&pdev->dev, "Initialized\n");
>> +     return 0;
>> +}
>> +
>> +MODULE_DEVICE_TABLE(of, st_pcie_of_match);
>> +
>> +static struct platform_driver st_pcie_driver = {
>> +     .driver = {
>> +             .name = "st-pcie",
>> +             .of_match_table = st_pcie_of_match,
>> +             .pm = &st_pcie_pm_ops,
>> +     },
>> +};
>> +
>> +/* ST PCIe driver does not allow module unload */
>> +static int __init pcie_init(void)
>> +{
>> +     return platform_driver_probe(&st_pcie_driver, st_pcie_probe);
>> +}
>> +device_initcall(pcie_init);
>> +
>> +MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>");
>> +MODULE_DESCRIPTION("PCI express Driver for ST SoCs");
>> +MODULE_LICENSE("GPLv2");
>
> Please add one space between 'GPL' and 'v2'.
>
> s/"GPLv2"/"GPL v2"
>
> According to the license_is_gpl_compatible(), the MODULE_LICENSE() string
> for GPL v2 is "GPL v2" not "GPLv2".
>

ok


> Best regards,
> Jingoo Han
>
>> --
>> 1.9.1
>

BR

Gabriel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller

[Pratyush Anand]

On Wednesday 17 December 2014 04:04 PM, Gabriel FERNANDEZ wrote:
> sti pcie is built around a Synopsis Designware PCIe IP.
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
> ---
>   drivers/pci/host/Kconfig  |   5 +
>   drivers/pci/host/Makefile |   1 +
>   drivers/pci/host/pci-st.c | 713 ++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 719 insertions(+)
>   create mode 100644 drivers/pci/host/pci-st.c
>

[...]

> +static int st_pcie_abort_handler(unsigned long addr, unsigned int fsr,
> +				 struct pt_regs *regs)
> +{
> +	return 0;
> +}
> +

You should have modification here to populate registers having cfg read 
values by 0xFFFFFFFF.

I would suggest to have a look here:
drivers/pci/host/pci-keystone.c:keystone_pcie_fault

> +/*
> + * The PCI express core IP expects the following arrangement on it's address
> + * bus (slv_haddr) when driving config cycles.
> + * bus_number		[31:24]
> + * dev_number		[23:19]
> + * func_number		[18:16]
> + * unused		[15:12]
> + * ext_reg_number	[11:8]
> + * reg_number		[7:2]
> + *
> + * Bits [15:12] are unused.
> + *
> + * In the glue logic there is a 64K region of address space that can be
> + * written/read to generate config cycles. The base address of this is
> + * controlled by CFG_BASE_ADDRESS. There are 8 16 bit registers called
> + * FUNC0_BDF_NUM to FUNC8_BDF_NUM. These split the bottom half of the 64K
> + * window into 8 regions at 4K boundaries. These control the bus,device and
> + * function number you are trying to talk to.
> + *
> + * The decision on whether to generate a type 0 or type 1 access is controlled
> + * by bits 15:12 of the address you write to.  If they are zero, then a type 0
> + * is generated, if anything else it will be a type 1. Thus the bottom 4K
> + * region controlled by FUNC0_BDF_NUM can only generate type 0, all the others
> + * can only generate type 1.
> + *
> + * We only use FUNC0_BDF_NUM and FUNC1_BDF_NUM. Which one you use is selected
> + * by bit 12 of the address you write to. The selected register is then used
> + * for the top 16 bits of the slv_haddr to form the bus/dev/func, bit 15:12 are
> + * wired to zero, and bits 11:2 form the address of the register you want to
> + * read in config space.
> + *
> + * We always write FUNC0_BDF_NUM as a 32 bit write. So if we want type 1
> + * accesses we have to shift by 16 so in effect we are writing to FUNC1_BDF_NUM
> + */
> +static inline u32 bdf_num(int bus, int devfn, int is_root_bus)
> +{
> +	return ((bus << 8) | devfn) << (is_root_bus ? 0 : 16);
> +}
> +
> +static inline unsigned config_addr(int where, int is_root_bus)
> +{
> +	return (where & ~3) | (!is_root_bus << 12);
> +}
> +
> +static int st_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> +				 unsigned int devfn, int where, int size,
> +				 u32 *val)
> +{
> +	u32 data;
> +	u32 bdf;
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +	int is_root_bus = pci_is_root_bus(bus);
> +	int retry_count = 0;
> +	int ret;
> +	void __iomem *addr;
> +
> +	/*
> +	 * Prerequisite
> +	 * PCI express devices will respond to all config type 0 cycles, since
> +	 * they are point to point links. Thus to avoid probing for multiple
> +	 * devices on the root, dw-pcie already check for us if it is on the
> +	 * root bus / other slots. Also, dw-pcie checks for the link being up
> +	 * as we will hang if we issue a config request and the link is down.
> +	 * A switch will reject requests for slots it knows do not exist.
> +	 */
> +	bdf = bdf_num(bus->number, devfn, is_root_bus);
> +	addr = pcie->config_area + config_addr(where,
> +			bus->parent->number == pp->root_bus_nr);
> +retry:
> +	/* Set the config packet devfn */
> +	writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
> +	readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
> +
> +	ret = dw_pcie_cfg_read(addr, where, size, &data);
> +
> +	/*
> +	 * This is intended to help with when we are probing the bus. The
> +	 * problem is that the wrapper logic doesn't have any way to
> +	 * interrogate if the configuration request failed or not.
> +	 * On the ARM we actually get a real bus error.
> +	 *
> +	 * Unfortunately this means it is impossible to tell the difference
> +	 * between when a device doesn't exist (the switch will return a UR
> +	 * completion) or the device does exist but isn't yet ready to accept
> +	 * configuration requests (the device will return a CRS completion)
> +	 *
> +	 * The result of this is that we will miss devices when probing.
> +	 *
> +	 * So if we are trying to read the dev/vendor id on devfn 0 and we
> +	 * appear to get zero back, then we retry the request.  We know that
> +	 * zero can never be a valid device/vendor id. The specification says
> +	 * we must retry for up to a second before we decide the device is

Not sure if retry has to be part of software. This might already be done 
by hardware.

> +	 * dead. If we are still dead then we assume there is nothing there and
> +	 * return ~0
> +	 *
> +	 * The downside of this is that we incur a delay of 1s for every pci
> +	 * express link that doesn't have a device connected.
> +	 */
> +	if (((where & ~3) == 0) && devfn == 0 && (data == 0 || data == ~0)) {
> +		if (retry_count++ < 1000) {
> +			mdelay(1);
> +			goto retry;
> +		} else {
> +			*val = ~0;
> +			return PCIBIOS_DEVICE_NOT_FOUND;
> +		}
> +	}

Have you found a situation with any of the card when your retry_count > 
0 and  < 1000 at this point. If not then, I think modifying abort 
handler will solve your issue.

> +
> +	*val = data;
> +	return ret;
> +}
> +

~Pratyush

Re: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller

[Bjorn Helgaas]

On Wed, Dec 17, 2014 at 11:34:44AM +0100, Gabriel FERNANDEZ wrote:
> sti pcie is built around a Synopsis Designware PCIe IP.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
> ---
>  drivers/pci/host/Kconfig  |   5 +
>  drivers/pci/host/Makefile |   1 +
>  drivers/pci/host/pci-st.c | 713 ++++++++++++++++++++++++++++++++++++++++++++++

Hi Gabriel,

Can you add a MAINTAINERS update so I know who should ack changes to this
driver?

>  3 files changed, 719 insertions(+)
>  create mode 100644 drivers/pci/host/pci-st.c
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index c4b6568..999d2b9 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -102,4 +102,9 @@ config PCI_LAYERSCAPE
>  	help
>  	  Say Y here if you want PCIe controller support on Layerscape SoCs.
>  
> +config PCI_ST
> +	bool "ST STiH41x PCIe controller"
> +	depends on ARCH_STI
> +	select PCIE_DW

Please add help text here.

> +static int st_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> +				 unsigned int devfn, int where, int size,
> +				 u32 *val)
> +{
> +	u32 data;
> +	u32 bdf;
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +	int is_root_bus = pci_is_root_bus(bus);
> +	int retry_count = 0;
> +	int ret;
> +	void __iomem *addr;
> +
> +	/*
> +	 * Prerequisite
> +	 * PCI express devices will respond to all config type 0 cycles, since
> +	 * they are point to point links. Thus to avoid probing for multiple
> +	 * devices on the root, dw-pcie already check for us if it is on the
> +	 * root bus / other slots. Also, dw-pcie checks for the link being up
> +	 * as we will hang if we issue a config request and the link is down.
> +	 * A switch will reject requests for slots it knows do not exist.
> +	 */
> +	bdf = bdf_num(bus->number, devfn, is_root_bus);
> +	addr = pcie->config_area + config_addr(where,
> +			bus->parent->number == pp->root_bus_nr);
> +retry:
> +	/* Set the config packet devfn */
> +	writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
> +	readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
> +
> +	ret = dw_pcie_cfg_read(addr, where, size, &data);
> +
> +	/*
> +	 * This is intended to help with when we are probing the bus. The
> +	 * problem is that the wrapper logic doesn't have any way to
> +	 * interrogate if the configuration request failed or not.
> +	 * On the ARM we actually get a real bus error.
> +	 *
> +	 * Unfortunately this means it is impossible to tell the difference
> +	 * between when a device doesn't exist (the switch will return a UR
> +	 * completion) or the device does exist but isn't yet ready to accept
> +	 * configuration requests (the device will return a CRS completion)

We do have CRS support in the Linux PCI core, so I guess this comment means
that the ST host bridge doesn't handle CRS correctly?

> +	 *
> +	 * The result of this is that we will miss devices when probing.
> +	 *
> +	 * So if we are trying to read the dev/vendor id on devfn 0 and we
> +	 * appear to get zero back, then we retry the request.  We know that
> +	 * zero can never be a valid device/vendor id. The specification says
> +	 * we must retry for up to a second before we decide the device is
> +	 * dead. If we are still dead then we assume there is nothing there and
> +	 * return ~0
> +	 *
> +	 * The downside of this is that we incur a delay of 1s for every pci
> +	 * express link that doesn't have a device connected.

That sounds pretty bad and I assume is a consequence of CRS handling being
broken in hardware.

> +	 */
> +	if (((where & ~3) == 0) && devfn == 0 && (data == 0 || data == ~0)) {
> +		if (retry_count++ < 1000) {
> +			mdelay(1);
> +			goto retry;
> +		} else {
> +			*val = ~0;
> +			return PCIBIOS_DEVICE_NOT_FOUND;
> +		}
> +	}
> +
> +	*val = data;
> +	return ret;
> +}

> +MODULE_LICENSE("GPLv2");

See license_is_gpl_compatible().  This string needs to be "GPL v2", not
"GPLv2" to avoid tainting the kernel.

Bjorn

Re: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller

[Gabriel Fernandez]

Hi Bjorn Helgaas,

On 12 January 2015 at 19:43, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Dec 17, 2014 at 11:34:44AM +0100, Gabriel FERNANDEZ wrote:
>> sti pcie is built around a Synopsis Designware PCIe IP.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
>> ---
>>  drivers/pci/host/Kconfig  |   5 +
>>  drivers/pci/host/Makefile |   1 +
>>  drivers/pci/host/pci-st.c | 713 ++++++++++++++++++++++++++++++++++++++++++++++
>
> Hi Gabriel,
>
> Can you add a MAINTAINERS update so I know who should ack changes to this
> driver?

yes no problem

>
>>  3 files changed, 719 insertions(+)
>>  create mode 100644 drivers/pci/host/pci-st.c
>>
>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index c4b6568..999d2b9 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -102,4 +102,9 @@ config PCI_LAYERSCAPE
>>       help
>>         Say Y here if you want PCIe controller support on Layerscape SoCs.
>>
>> +config PCI_ST
>> +     bool "ST STiH41x PCIe controller"
>> +     depends on ARCH_STI
>> +     select PCIE_DW
>
> Please add help text here.
>

okay

>> +static int st_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>> +                              unsigned int devfn, int where, int size,
>> +                              u32 *val)
>> +{
>> +     u32 data;
>> +     u32 bdf;
>> +     struct st_pcie *pcie = to_st_pcie(pp);
>> +     int is_root_bus = pci_is_root_bus(bus);
>> +     int retry_count = 0;
>> +     int ret;
>> +     void __iomem *addr;
>> +
>> +     /*
>> +      * Prerequisite
>> +      * PCI express devices will respond to all config type 0 cycles, since
>> +      * they are point to point links. Thus to avoid probing for multiple
>> +      * devices on the root, dw-pcie already check for us if it is on the
>> +      * root bus / other slots. Also, dw-pcie checks for the link being up
>> +      * as we will hang if we issue a config request and the link is down.
>> +      * A switch will reject requests for slots it knows do not exist.
>> +      */
>> +     bdf = bdf_num(bus->number, devfn, is_root_bus);
>> +     addr = pcie->config_area + config_addr(where,
>> +                     bus->parent->number == pp->root_bus_nr);
>> +retry:
>> +     /* Set the config packet devfn */
>> +     writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
>> +     readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
>> +
>> +     ret = dw_pcie_cfg_read(addr, where, size, &data);
>> +
>> +     /*
>> +      * This is intended to help with when we are probing the bus. The
>> +      * problem is that the wrapper logic doesn't have any way to
>> +      * interrogate if the configuration request failed or not.
>> +      * On the ARM we actually get a real bus error.
>> +      *
>> +      * Unfortunately this means it is impossible to tell the difference
>> +      * between when a device doesn't exist (the switch will return a UR
>> +      * completion) or the device does exist but isn't yet ready to accept
>> +      * configuration requests (the device will return a CRS completion)
>
> We do have CRS support in the Linux PCI core, so I guess this comment means
> that the ST host bridge doesn't handle CRS correctly?
>

yes, it is.

>> +      *
>> +      * The result of this is that we will miss devices when probing.
>> +      *
>> +      * So if we are trying to read the dev/vendor id on devfn 0 and we
>> +      * appear to get zero back, then we retry the request.  We know that
>> +      * zero can never be a valid device/vendor id. The specification says
>> +      * we must retry for up to a second before we decide the device is
>> +      * dead. If we are still dead then we assume there is nothing there and
>> +      * return ~0
>> +      *
>> +      * The downside of this is that we incur a delay of 1s for every pci
>> +      * express link that doesn't have a device connected.
>
> That sounds pretty bad and I assume is a consequence of CRS handling being
> broken in hardware.
>
>> +      */
>> +     if (((where & ~3) == 0) && devfn == 0 && (data == 0 || data == ~0)) {
>> +             if (retry_count++ < 1000) {
>> +                     mdelay(1);
>> +                     goto retry;
>> +             } else {
>> +                     *val = ~0;
>> +                     return PCIBIOS_DEVICE_NOT_FOUND;
>> +             }
>> +     }
>> +
>> +     *val = data;
>> +     return ret;
>> +}
>
>> +MODULE_LICENSE("GPLv2");
>
> See license_is_gpl_compatible().  This string needs to be "GPL v2", not
> "GPLv2" to avoid tainting the kernel.
>

okay

> Bjorn


Thanks for reviewing

BR

Gabriel

[PATCH 4/5] PCI: designware: Add setup bus-related to pcie_host_ops

[Gabriel FERNANDEZ]

ST sti SoCs PCIe IPs are built around DesignWare IP Core.
But in these SoCs PCIe IP doesn't support IO.

To support this, add setup_bus() to pcie_host_ops.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
---
 drivers/pci/host/pcie-designware.c | 3 +++
 drivers/pci/host/pcie-designware.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index df781cd..98e19bc 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -719,6 +719,9 @@ static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
 	pci_add_resource_offset(&sys->resources, &pp->mem, sys->mem_offset);
 	pci_add_resource(&sys->resources, &pp->busn);
 
+	if (pp->ops->setup_bus)
+		pp->ops->setup_bus(pp, sys);
+
 	return 1;
 }
 
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index d0bbd27..5c13de7 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -73,6 +73,7 @@ struct pcie_host_ops {
 	u32 (*get_msi_addr)(struct pcie_port *pp);
 	u32 (*get_msi_data)(struct pcie_port *pp, int pos);
 	void (*scan_bus)(struct pcie_port *pp);
+	void (*setup_bus)(struct pcie_port *pp, struct pci_sys_data *sys);
 	int (*msi_host_init)(struct pcie_port *pp, struct msi_controller *chip);
 };
 
-- 
1.9.1

Re: [PATCH 4/5] PCI: designware: Add setup bus-related to pcie_host_ops

[Arnd Bergmann]

On Wednesday 17 December 2014 11:34:45 Gabriel FERNANDEZ wrote:
> ST sti SoCs PCIe IPs are built around DesignWare IP Core.
> But in these SoCs PCIe IP doesn't support IO.
> 
> To support this, add setup_bus() to pcie_host_ops.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>

The dw-pcie driver should be able to tell whether the device has
an I/O space or not, and do the right thing based on that. Don't
add an implementation specific callback for that.

	Arnd

Re: [PATCH 4/5] PCI: designware: Add setup bus-related to pcie_host_ops

[Jingoo Han]

On Thursday, December 18, 2014 7:16 AM, Arnd Bergmann wrote:
> On Wednesday 17 December 2014 11:34:45 Gabriel FERNANDEZ wrote:
> > ST sti SoCs PCIe IPs are built around DesignWare IP Core.
> > But in these SoCs PCIe IP doesn't support IO.

Hi Gabriel,

I cannot understand how ST sti SoCs PCIe IP does not support I/O.
As far as I know, it cannot be selected by the 'parameter'.
Then, H/W engineers dropped out the I/O control logic?

> >
> > To support this, add setup_bus() to pcie_host_ops.


> >
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>
> > Signed-off-by: Gabriel Fernandez <gabriel.fernandez-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> The dw-pcie driver should be able to tell whether the device has
> an I/O space or not, and do the right thing based on that. Don't
> add an implementation specific callback for that.

I agree with Arnd's opinion.

In addition, I have one more question.
Then, if a device that requires I/O region is connected to
PCIe slot of ST sti SoCs PCIe, what will happen?
It just prints error messages?

Best regards,
Jingoo Han

> 
> 	Arnd

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] PCI: designware: Add setup bus-related to pcie_host_ops

[Gabriel Fernandez]

[-- Attachment #1: Type: text/plain, Size: 1504 bytes --]

Hi Arnd, Jingoo,

On 18 December 2014 at 05:58, Jingoo Han <jg1.han@samsung.com> wrote:

> On Thursday, December 18, 2014 7:16 AM, Arnd Bergmann wrote:
> > On Wednesday 17 December 2014 11:34:45 Gabriel FERNANDEZ wrote:
> > > ST sti SoCs PCIe IPs are built around DesignWare IP Core.
> > > But in these SoCs PCIe IP doesn't support IO.
>
> Hi Gabriel,
>
> I cannot understand how ST sti SoCs PCIe IP does not support I/O.
> As far as I know, it cannot be selected by the 'parameter'.
> Then, H/W engineers dropped out the I/O control logic?
>
> > >
> > > To support this, add setup_bus() to pcie_host_ops.
>
>
> > >
> > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> > > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
> >
> > The dw-pcie driver should be able to tell whether the device has
> > an I/O space or not, and do the right thing based on that. Don't
> > add an implementation specific callback for that.
>
> I agree with Arnd's opinion.
>
> In addition, I have one more question.
> Then, if a device that requires I/O region is connected to
> PCIe slot of ST sti SoCs PCIe, what will happen?
> It just prints error messages?
>
> Best regards,
> Jingoo Han
>
> >
> >       Arnd
>
>
Arnd i
n other post mention to add an empty I/O space to workaround lack of I/O
port access.
Is it the right thing to do ?
​
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/299623.html


​Best regards
​​

[-- Attachment #2: Type: text/html, Size: 3202 bytes --]

Re: [PATCH 4/5] PCI: designware: Add setup bus-related to pcie_host_ops

[Arnd Bergmann]

On Monday 19 January 2015 13:38:53 Gabriel Fernandez wrote:
> 
> On 18 December 2014 at 05:58, Jingoo Han <jg1.han@samsung.com> wrote:
> 
> > On Thursday, December 18, 2014 7:16 AM, Arnd Bergmann wrote:
> > > On Wednesday 17 December 2014 11:34:45 Gabriel FERNANDEZ wrote:
> > > > ST sti SoCs PCIe IPs are built around DesignWare IP Core.
> > > > But in these SoCs PCIe IP doesn't support IO.
> > I cannot understand how ST sti SoCs PCIe IP does not support I/O.
> > As far as I know, it cannot be selected by the 'parameter'.
> > Then, H/W engineers dropped out the I/O control logic?
> >
> > > >
> > > > To support this, add setup_bus() to pcie_host_ops.
> >
> >
> > > >
> > > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> > > > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
> > >
> > > The dw-pcie driver should be able to tell whether the device has
> > > an I/O space or not, and do the right thing based on that. Don't
> > > add an implementation specific callback for that.
> >
> > I agree with Arnd's opinion.
> >
> > In addition, I have one more question.
> > Then, if a device that requires I/O region is connected to
> > PCIe slot of ST sti SoCs PCIe, what will happen?
> > It just prints error messages?
> >
> >
> Arnd i
> n other post mention to add an empty I/O space to workaround lack of I/O
> port access.
> Is it the right thing to do ?
> ​
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/299623.html
> 

Good question. Given the latest development, I'd feel tempted to
try moving the entire pcie-designware driver to use the generic probing
that bypassses pci_common_init_dev(). Rob Herring posted a patch
for the versatile PCI driver, you could try doing the same thing
here, see http://permalink.gmane.org/gmane.linux.kernel.pci/30346.

I think that would completely avoid this problem, and also help everyone
that wants to use this driver on arm64.

	Arnd

Re: [PATCH 4/5] PCI: designware: Add setup bus-related to pcie_host_ops

[Lorenzo Pieralisi]

On Mon, Jan 19, 2015 at 01:54:26PM +0000, Arnd Bergmann wrote:
> On Monday 19 January 2015 13:38:53 Gabriel Fernandez wrote:
> > 
> > On 18 December 2014 at 05:58, Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> > 
> > > On Thursday, December 18, 2014 7:16 AM, Arnd Bergmann wrote:
> > > > On Wednesday 17 December 2014 11:34:45 Gabriel FERNANDEZ wrote:
> > > > > ST sti SoCs PCIe IPs are built around DesignWare IP Core.
> > > > > But in these SoCs PCIe IP doesn't support IO.
> > > I cannot understand how ST sti SoCs PCIe IP does not support I/O.
> > > As far as I know, it cannot be selected by the 'parameter'.
> > > Then, H/W engineers dropped out the I/O control logic?
> > >
> > > > >
> > > > > To support this, add setup_bus() to pcie_host_ops.
> > >
> > >
> > > > >
> > > > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>
> > > > > Signed-off-by: Gabriel Fernandez <gabriel.fernandez-QSEj5FYQhm5QFI55V6+gNQ@public.gmane.orgg>
> > > >
> > > > The dw-pcie driver should be able to tell whether the device has
> > > > an I/O space or not, and do the right thing based on that. Don't
> > > > add an implementation specific callback for that.
> > >
> > > I agree with Arnd's opinion.
> > >
> > > In addition, I have one more question.
> > > Then, if a device that requires I/O region is connected to
> > > PCIe slot of ST sti SoCs PCIe, what will happen?
> > > It just prints error messages?
> > >
> > >
> > Arnd i
> > n other post mention to add an empty I/O space to workaround lack of I/O
> > port access.
> > Is it the right thing to do ?
> > ​
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/299623.html
> > 
> 
> Good question. Given the latest development, I'd feel tempted to
> try moving the entire pcie-designware driver to use the generic probing
> that bypassses pci_common_init_dev(). Rob Herring posted a patch
> for the versatile PCI driver, you could try doing the same thing
> here, see http://permalink.gmane.org/gmane.linux.kernel.pci/30346.
> 
> I think that would completely avoid this problem, and also help everyone
> that wants to use this driver on arm64.

I was aiming at that, I started by converting the ranges parsing to the new
API, with its own unfortunate quirks (ie untranslated CPU addresses,
that apparently caused deafening silence on the lists :)):

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/314008.html

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] PCI: designware: Add setup bus-related to pcie_host_ops

[Gabriel Fernandez]

Hi Arnd, Jingoo,

On 18 December 2014 at 05:58, Jingoo Han <jg1.han@samsung.com> wrote:
> On Thursday, December 18, 2014 7:16 AM, Arnd Bergmann wrote:
>> On Wednesday 17 December 2014 11:34:45 Gabriel FERNANDEZ wrote:
>> > ST sti SoCs PCIe IPs are built around DesignWare IP Core.
>> > But in these SoCs PCIe IP doesn't support IO.
>
> Hi Gabriel,
>
> I cannot understand how ST sti SoCs PCIe IP does not support I/O.
> As far as I know, it cannot be selected by the 'parameter'.
> Then, H/W engineers dropped out the I/O control logic?
>
>> >
>> > To support this, add setup_bus() to pcie_host_ops.
>
>
>> >
>> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
>>
>> The dw-pcie driver should be able to tell whether the device has
>> an I/O space or not, and do the right thing based on that. Don't
>> add an implementation specific callback for that.
>
> I agree with Arnd's opinion.
>
> In addition, I have one more question.
> Then, if a device that requires I/O region is connected to
> PCIe slot of ST sti SoCs PCIe, what will happen?
> It just prints error messages?
>
> Best regards,
> Jingoo Han
>
>>
>>       Arnd
>

Arnd in other post mention to add an empty I/O space to workaround
lack of I/O port access.
Is it the right thing to do ?
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/299623.html

Best regards

[PATCH 5/5] PCI: st: disable IO support

[Gabriel FERNANDEZ]

sti SoCs PCIe IPs are built around DesignWare IP Core.
But in these SoCs, PCIe IP doesn't support IO.
By default, when no IO space is provided, a default one is assigned.

Add an empty IO resource to the bus, and disable IO by default.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 drivers/pci/host/pci-st.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/pci/host/pci-st.c b/drivers/pci/host/pci-st.c
index bd3d32d..c0d3895 100644
--- a/drivers/pci/host/pci-st.c
+++ b/drivers/pci/host/pci-st.c
@@ -357,9 +357,15 @@ static void st_pcie_board_reset(struct pcie_port *pp)
 static void st_pcie_hw_setup(struct pcie_port *pp)
 {
 	struct st_pcie *pcie = to_st_pcie(pp);
+	u32 val;
 
 	dw_pcie_setup_rc(pp);
 
+	/* Disable IO support */
+	val = readl_relaxed(pp->dbi_base + PCI_COMMAND);
+	val &= ~PCI_COMMAND_IO;
+	writel_relaxed(val, pp->dbi_base + PCI_COMMAND);
+
 	/* Set up the config window to the top of the PCI address space */
 	writel_relaxed(pcie->config_window_start,
 		       pp->dbi_base + CFG_BASE_ADDRESS);
@@ -445,11 +451,28 @@ static void st_pcie_host_init(struct pcie_port *pp)
 		st_msi_init_one(pp);
 }
 
+static void st_pcie_setup_bus(struct pcie_port *pp, struct pci_sys_data *sys)
+{
+	struct resource *res;
+
+	/* This PCIe controller does not support IO, set an empty one. */
+	res = devm_kzalloc(pp->dev, sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return;
+
+	res->start = 0;
+	res->end = 0;
+	res->name = "PCIe empty IO space";
+	res->flags = IORESOURCE_IO;
+	pci_add_resource(&sys->resources, res);
+}
+
 static struct pcie_host_ops st_pcie_host_ops = {
 	.rd_other_conf = st_pcie_rd_other_conf,
 	.wr_other_conf = st_pcie_wr_other_conf,
 	.link_up = st_pcie_link_up,
 	.host_init = st_pcie_host_init,
+	.setup_bus = st_pcie_setup_bus,
 };
 
 static int st_pcie_init(struct pcie_port *pp)
-- 
1.9.1

Re: [PATCH 5/5] PCI: st: disable IO support

[One Thousand Gnomes]

On Wed, 17 Dec 2014 11:34:46 +0100
Gabriel FERNANDEZ <gabriel.fernandez@st.com> wrote:

> sti SoCs PCIe IPs are built around DesignWare IP Core.
> But in these SoCs, PCIe IP doesn't support IO.
> By default, when no IO space is provided, a default one is assigned.
> 
> Add an empty IO resource to the bus, and disable IO by default.

As a point of PCI pedantry I don't think this is quite sufficient. PCI
has a weird corner case where I/O resources are implied rather than
allocated.

For IDE/SATA you may need to something like

	if (class == PCI_CLASS_STORAGE_IDE) {
		u8 progif;
		pci_read_config_byte(dev, PCI_CLASS_PROG, &progif);
		progif |= 5;
		pci_write_config_byte(dev, PCI_CLASS_PROG, &progif);
	}

so that any adapter is kicked out of legacy mode and doesn't get implied
I/O resources and interrupts. I don't know if that case matters for your
usage.

Re: [PATCH 5/5] PCI: st: disable IO support

[Gabriel Fernandez]

Hi,

Yes, we don't really care about this corner case.
Thanks for your reviewing.

BR

Gabriel

On 17 December 2014 at 15:01, One Thousand Gnomes
<gnomes@lxorguk.ukuu.org.uk> wrote:
> On Wed, 17 Dec 2014 11:34:46 +0100
> Gabriel FERNANDEZ <gabriel.fernandez@st.com> wrote:
>
>> sti SoCs PCIe IPs are built around DesignWare IP Core.
>> But in these SoCs, PCIe IP doesn't support IO.
>> By default, when no IO space is provided, a default one is assigned.
>>
>> Add an empty IO resource to the bus, and disable IO by default.
>
> As a point of PCI pedantry I don't think this is quite sufficient. PCI
> has a weird corner case where I/O resources are implied rather than
> allocated.
>
> For IDE/SATA you may need to something like
>
>         if (class == PCI_CLASS_STORAGE_IDE) {
>                 u8 progif;
>                 pci_read_config_byte(dev, PCI_CLASS_PROG, &progif);
>                 progif |= 5;
>                 pci_write_config_byte(dev, PCI_CLASS_PROG, &progif);
>         }
>
> so that any adapter is kicked out of legacy mode and doesn't get implied
> I/O resources and interrupts. I don't know if that case matters for your
> usage.
>
>