PCI:Add SPEAr13xx PCie support

From: Pratyush Anand <[email protected]>

PCIe RC drivers are initialized with subsys_initcall. Few PCIe drivers like SPEAr13xx needs phy drivers to be initialized.

Therefore initialize phy core driver with subsys_initcall to avoid calling of phy_get before phy_class is created.

Signed-off-by Pratyush Anand <[email protected]>

Cc Mohit Kumar <[email protected]>

Cc Kishon Vijay Abraham I <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc [email protected]

Cc [email protected]

---
 drivers/phy/phy-core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/phy/phy-core.c +1 -1

index 03cf8fb..fa73101 100644
@@ -685,7 +685,7 @@ static int __init phy_core_init(void)
 
 	return 0;
 }
-module_init(phy_core_init);
+subsys_initcall(phy_core_init);
 
 static void __exit phy_core_exit(void)
 {

Hi,

Quoted 4 lines · Mohit Kumar

From: Pratyush Anand <[email protected]>

PCIe RC drivers are initialized with subsys_initcall. Few PCIe drivers like SPEAr13xx needs phy drivers to be initialized.

Instead change PCIe RC drivers to module init. Phy drivers should be loaded very early otherwise. (Hint: drivers/Makefile).

Thanks
Kishon

Quoted 28 lines

Therefore initialize phy core driver with subsys_initcall to avoid calling of phy_get before phy_class is created.

Signed-off-by Pratyush Anand <[email protected]>

Cc Mohit Kumar <[email protected]>

Cc Kishon Vijay Abraham I <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc [email protected]

Cc [email protected]

---
 drivers/phy/phy-core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/phy/phy-core.c +1 -1

index 03cf8fb..fa73101 100644
@@ -685,7 +685,7 @@ static int __init phy_core_init(void)
 
 	return 0;
 }
-module_init(phy_core_init);
+subsys_initcall(phy_core_init);
 
 static void __exit phy_core_exit(void)
 {

Quoted 5 lines · Kishon Vijay Abraham I

Hi,

Quoted 2 lines · Mohit Kumar

From: Pratyush Anand <[email protected]>

Quoted 5 lines

Quoted 2 lines

PCIe RC drivers are initialized with subsys_initcall. Few PCIe drivers like SPEAr13xx needs phy drivers to be initialized.

Instead change PCIe RC drivers to module init. Phy drivers should be loaded very early otherwise. (Hint: drivers/Makefile).

I think PCIe RC driver can not be made module init. Bjorn can comment better.

All PCIe card drivers are initialized with module init. RC driver must have been initialized before any card driver initialization. Currently, card drivers does not have deferred probe concept, so I am not sure if keeping RC driver as module init will work always.

By the way, is there any side effect of loading phy driver very early?

Quoted 5 lines · Kishon Vijay Abraham I

Hi,

Quoted 2 lines · Mohit Kumar

From: Pratyush Anand <[email protected]>

Quoted 5 lines

Quoted 2 lines

PCIe RC drivers are initialized with subsys_initcall. Few PCIe drivers like SPEAr13xx needs phy drivers to be initialized.

Instead change PCIe RC drivers to module init. Phy drivers should be loaded very early otherwise. (Hint: drivers/Makefile).

From hint, you mean that if makefile has pci entry (and hence RC

I think PCIe RC driver can not be made module init. Bjorn can comment better.

All PCIe card drivers are initialized with module init. RC driver must have been initialized before any card driver initialization. Currently, card drivers does not have deferred probe concept, so I am not sure if keeping RC driver as module init will work always.

By the way, is there any side effect of loading phy driver very early?

Regards
Pratyush

Quoted 32 lines

Thanks
Kishon

Quoted 28 lines

Therefore initialize phy core driver with subsys_initcall to avoid calling of phy_get before phy_class is created.

Signed-off-by Pratyush Anand <[email protected]>

Cc Mohit Kumar <[email protected]>

Cc Kishon Vijay Abraham I <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc [email protected]

Cc [email protected]

---
 drivers/phy/phy-core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/phy/phy-core.c +1 -1

index 03cf8fb..fa73101 100644
@@ -685,7 +685,7 @@ static int __init phy_core_init(void)
 
 	return 0;
 }
-module_init(phy_core_init);
+subsys_initcall(phy_core_init);
 
 static void __exit phy_core_exit(void)
 {

Quoted 9 lines · Pratyush ANAND

Quoted 8 lines · Kishon Vijay Abraham I

Hi,

Quoted 4 lines · Mohit Kumar

From: Pratyush Anand <[email protected]>

PCIe RC drivers are initialized with subsys_initcall. Few PCIe drivers like SPEAr13xx needs phy drivers to be initialized.

Quoted 2 lines

Quoted 2 lines

Instead change PCIe RC drivers to module init. Phy drivers should be loaded very early otherwise. (Hint: drivers/Makefile).

From hint, you mean that if makefile has pci entry (and hence RC driver entry) before card drivers entry, then it insures that rc driver's probe is called before card driver's probe?

I think, yes.
And if yes, then what you say is acceptable :)

Regards
Pratyush

Quoted 45 lines

I think PCIe RC driver can not be made module init. Bjorn can comment better.

All PCIe card drivers are initialized with module init. RC driver must have been initialized before any card driver initialization. Currently, card drivers does not have deferred probe concept, so I am not sure if keeping RC driver as module init will work always.

By the way, is there any side effect of loading phy driver very early?

Regards
Pratyush

Quoted 32 lines

Thanks
Kishon

Quoted 28 lines

Therefore initialize phy core driver with subsys_initcall to avoid calling of phy_get before phy_class is created.

Signed-off-by Pratyush Anand <[email protected]>

Cc Mohit Kumar <[email protected]>

Cc Kishon Vijay Abraham I <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc [email protected]

Cc [email protected]

---
 drivers/phy/phy-core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/phy/phy-core.c +1 -1

index 03cf8fb..fa73101 100644
@@ -685,7 +685,7 @@ static int __init phy_core_init(void)
 
 	return 0;
 }
-module_init(phy_core_init);
+subsys_initcall(phy_core_init);
 
 static void __exit phy_core_exit(void)
 {

Quoted 5 lines · Kishon Vijay Abraham I

Hi,

Quoted 2 lines · Mohit Kumar

From: Pratyush Anand <[email protected]>

Quoted 5 lines

Quoted 2 lines

PCIe RC drivers are initialized with subsys_initcall. Few PCIe drivers like SPEAr13xx needs phy drivers to be initialized.

Instead change PCIe RC drivers to module init. Phy drivers should be loaded very early otherwise. (Hint: drivers/Makefile).

I think PCIe RC driver can not be made module init. Bjorn can comment better.

Enable EABI, OEABI, VFP and NFS configs in default configuration file for SPEAr13xx.

From: Pratyush Anand <[email protected]>

Few platform does only phy specific work in platform callbacks. It is better to use standard phy callbacks for such platform in stead of platform callbacks.

This patch does not break any platform callbacks. It adds phy plugins on top of that.

It has been assumed that no platform will need both phy plugins as well as platform callbacks.

Signed-off-by Pratyush Anand <[email protected]>

Cc Mohit Kumar <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc [email protected]

Cc [email protected]

Documentation/devicetree/bindings/phy/spear13xx-miphy.txt new file +6 -1

+		ahci0: ahci at b1000000 {
 			compatible = "snps,spear-ahci";
 			reg = <0xb1000000 0x10000>;
 			interrupts = <0 68 0x4>;
+			phys = <&miphy0 0 0>;
+			phy-names = "ahci-phy";
 			status = "disabled";
 		};
 
-		ahci at b1800000 {
+		ahci1: ahci at b1800000 {
 			compatible = "snps,spear-ahci";
 			reg = <0xb1800000 0x10000>;
 			interrupts = <0 69 0x4>;
+			phys = <&miphy1 0 1>;
+			phy-names = "ahci-phy";
 			status = "disabled";
 		};

Documentation/devicetree/bindings/phy/spear13xx-miphy.txt new file +9 -0

+static int spear1340_sata_miphy_init(struct spear13xx_phy_priv *phypriv)
+{
+	regmap_update_bits(phypriv->misc, SPEAR1340_PCIE_SATA_CFG,
+			SPEAR1340_PCIE_SATA_CFG_MASK, SPEAR1340_SATA_CFG_VAL);
+	regmap_update_bits(phypriv->misc, SPEAR1340_PCIE_MIPHY_CFG,
+			SPEAR1340_PCIE_MIPHY_CFG_MASK,
+			SPEAR1340_PCIE_SATA_MIPHY_CFG_SATA_25M_CRYSTAL_CLK);
+	/* Switch on sata power domain */
+	regmap_update_bits(phypriv->misc, SPEAR1340_PCM_CFG,

arch/arm/boot/dts/spear1310.dtsi +6 -0

@@ -80,6 +80,57 @@
 			status = "disabled";
 		};
 
+		pcie0: pcie@b1000000 {
+			compatible = "st,spear1340-pcie", "snps,dw-pcie";
+			reg = <0xb1000000 0x4000>;
+			interrupts = <0 68 0x4>;
+			pcie_is_gen1 = <0>;
+			num-lanes = <1>;

drivers/phy/phy-spear13xx-sata-pcie.c +11 -0

+static int spear1310_pcie_miphy_init(struct spear13xx_phy_priv *phypriv)
+{
+	u32 mask, val;
+
+	regmap_update_bits(phypriv->misc, SPEAR1310_PCIE_MIPHY_CFG_1,
+			SPEAR1310_PCIE_SATA_MIPHY_CFG_PCIE_MASK,
+			SPEAR1310_PCIE_SATA_MIPHY_CFG_PCIE);
+
+	switch (phypriv->id) {
+	case 0:
+		mask = SPEAR1310_PCIE_CFG_MASK(0);

Quoted 6 lines

arch/arm/boot/dts/spear1310.dtsi +6 -0

+             pcie0: pcie@b1000000 {
+                     compatible = "st,spear1340-pcie", "snps,dw-pcie";
+                     reg = <0xb1000000 0x4000>;
+                     interrupts = <0 68 0x4>;
+                     pcie_is_gen1 = <0>;
+                     num-lanes = <1>;

arch/arm/boot/dts/spear1310.dtsi line 98

Quoted 10 lines

arch/arm/boot/dts/spear1310.dtsi +10 -0

+                     phys = <&miphy0 1 0>;
+                     phy-names = "pcie-phy";
+                     #address-cells = <3>;
+                     #size-cells = <2>;
+                     device_type = "pci";
+                     ranges = <0x00000800 0 0x80000000 0x80000000 0 0x00020000   /* configuration space */
+                             0x81000000 0 0   0x80020000 0 0x00010000   /* downstream I/O */
+                             0x82000000 0 0x80030000 0xc0030000 0 0x0ffd0000>; /* non-prefetchable memory */
+                     status = "disabled";
+             };

Shouldn't there be more than one interrupt? Normally each root port has four legacy IRQs, in order to support bridge devices.

Quoted 6 lines · Pratyush Anand

Quoted 5 lines · Kishon Vijay Abraham I

Hi,

Quoted 2 lines · Mohit Kumar

From: Pratyush Anand <[email protected]>

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

PCIe RC drivers are initialized with subsys_initcall. Few PCIe drivers like SPEAr13xx needs phy drivers to be initialized.

Instead change PCIe RC drivers to module init. Phy drivers should be loaded very early otherwise. (Hint: drivers/Makefile).

I think PCIe RC driver can not be made module init. Bjorn can comment better.

I don't think there is any problem here: the PCI devices will only appear after the PCIe root bus has been probed. All drivers using the regular pci_driver framework should work fine even if they are loaded before the device is found. There are a handful of drivers using 'pci_get_device' rather than pci_register_driver, and those will break. As far as I can tell, those drivers are all x86 specific, and you should not worry about them.

Having the PHY driver get initialized after the PCI root driver should also work, but it requires correct handling of -EPROBE_DEFER: if phy_get

returns this error, the PCI driver must silently return the same error from its probe() function so it will get called again at a later time (after some other devices have been probed successfully).

Arnd

Quoted 12 lines · Mohit Kumar

From: Pratyush Anand <[email protected]>

Few platform does only phy specific work in platform callbacks. It is better to use standard phy callbacks for such platform in stead of platform callbacks.

This patch does not break any platform callbacks. It adds phy plugins on top of that.

It has been assumed that no platform will need both phy plugins as well as platform callbacks.

Quoted 6 lines

Signed-off-by Pratyush Anand <[email protected]>

Cc Mohit Kumar <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc [email protected]

Cc [email protected]

This patch looks good to me, but as I mentioned, there has been some related work recently. Please see the patch from Roger Quadros posted at http://lkml.org/lkml/2014/1/8/142

One of the two should be enough.

Arnd

Documentation/devicetree/bindings/phy/spear13xx-miphy.txt line 2

Quoted 9 lines · Mohit Kumar

Documentation/devicetree/bindings/phy/spear13xx-miphy.txt new file +2 -0

new file mode 100644
index 0000000..208b37d
@@ -0,0 +1,8 @@
+Required properties:
+- compatible : should be "st,spear1340-sata-pcie-phy".

Just for confirmation: This phy is by design only capable of driving sata or pcie, but nothing else if reused in a different SoC, right?

If the phy is actually more generic than that, I'd suggest changing the name, otherwise it's ok.

Documentation/devicetree/bindings/phy/spear13xx-miphy.txt line 8

Quoted 7 lines

Documentation/devicetree/bindings/phy/spear13xx-miphy.txt new file +6 -0

+- reg : offset and length of the PHY register set.
+- misc: phandle for the syscon node to access misc registers
+- #phy-cells : from the generic PHY bindings, must be 2.
+	- 1st arg: phandle to the phy node.
+	- 2nd arg: 0 if phy (in 1st arg) is to be used for sata else 1.
+	- 3rd arg: Instance id of the phy (in 1st arg).

I would count "arg" differently: There are three cells, and the first cell is the phandle, while the second and third cells contain the first and second argument.

The third cell seems redundant, more on that below.

Quoted 19 lines

Documentation/devicetree/bindings/phy/spear13xx-miphy.txt new file +6 -1

+		ahci0: ahci at b1000000 {
 			compatible = "snps,spear-ahci";
 			reg = <0xb1000000 0x10000>;
 			interrupts = <0 68 0x4>;
+			phys = <&miphy0 0 0>;
+			phy-names = "ahci-phy";
 			status = "disabled";
 		};
 
-		ahci at b1800000 {
+		ahci1: ahci at b1800000 {
 			compatible = "snps,spear-ahci";
 			reg = <0xb1800000 0x10000>;
 			interrupts = <0 69 0x4>;
+			phys = <&miphy1 0 1>;
+			phy-names = "ahci-phy";
 			status = "disabled";
 		};

Documentation/devicetree/bindings/phy/spear13xx-miphy.txt

Quoted 9 lines

Documentation/devicetree/bindings/phy/spear13xx-miphy.txt new file +3 -1

-		ahci at b4000000 {
+		ahci2: ahci at b4000000 {
 			compatible = "snps,spear-ahci";
 			reg = <0xb4000000 0x10000>;
 			interrupts = <0 70 0x4>;
+			phys = <&miphy2 0 2>;
+			phy-names = "ahci-phy";
 			status = "disabled";
 		};

In each case, the number of the phy 'miphyX' is identical to the third cell, and I suspect this is by design. In the driver, the 'id' field is set in the xlate function, but I could not find any place where it actually gets used, so unless you know that it's needed, I'd suggest simply removing it.

Even if you need it, it may be better to have the instance encoded in the phy node itself, since it's a property of the phy hardware (e.g. if you have to pass the number into a generic register that is global to all phys.

Alternatively, you could have a different representation, where you have a single DT device node representing all three PHYs, with "reg = <0xeb800000 0xc000>;" In that case, all sata devices would point to the same phy node and pass the instance id so the phy driver can operated the correct register set.

Quoted 17 lines

Documentation/devicetree/bindings/phy/spear13xx-miphy.txt new file +16 -0

+static int spear1340_sata_miphy_init(struct spear13xx_phy_priv *phypriv)
+{
+	regmap_update_bits(phypriv->misc, SPEAR1340_PCIE_SATA_CFG,
+			SPEAR1340_PCIE_SATA_CFG_MASK, SPEAR1340_SATA_CFG_VAL);
+	regmap_update_bits(phypriv->misc, SPEAR1340_PCIE_MIPHY_CFG,
+			SPEAR1340_PCIE_MIPHY_CFG_MASK,
+			SPEAR1340_PCIE_SATA_MIPHY_CFG_SATA_25M_CRYSTAL_CLK);
+	/* Switch on sata power domain */
+	regmap_update_bits(phypriv->misc, SPEAR1340_PCM_CFG,
+			SPEAR1340_PCM_CFG_SATA_POWER_EN,
+			SPEAR1340_PCM_CFG_SATA_POWER_EN);
+	msleep(20);
+	/* Disable PCIE SATA Controller reset */
+	regmap_update_bits(phypriv->misc, SPEAR1340_PERIP1_SW_RST,
+			SPEAR1340_PERIP1_SW_RST_SATA, 0);
+	msleep(20);

Documentation/devicetree/bindings/phy/spear13xx-miphy.txt

Quoted 3 lines

Documentation/devicetree/bindings/phy/spear13xx-miphy.txt new file +3 -0

+
+	return 0;
+}

I guess some of the parts above can eventually get moved into other drivers (reset controller, power domains) that get called directly by the SATA driver (e.g. though reset_device()). Since that won't impact the PHY binding, it seems fine to leave it here for now.

Arnd

Quoted 10 lines · Mohit Kumar

arch/arm/boot/dts/spear1310.dtsi +6 -0

@@ -80,6 +80,57 @@
 			status = "disabled";
 		};
 
+		pcie0: pcie@b1000000 {
+			compatible = "st,spear1340-pcie", "snps,dw-pcie";
+			reg = <0xb1000000 0x4000>;
+			interrupts = <0 68 0x4>;
+			pcie_is_gen1 = <0>;
+			num-lanes = <1>;

arch/arm/boot/dts/spear1310.dtsi line 98

Quoted 10 lines

arch/arm/boot/dts/spear1310.dtsi +10 -0

+			phys = <&miphy0 1 0>;
+			phy-names = "pcie-phy";
+			#address-cells = <3>;
+			#size-cells = <2>;
+			device_type = "pci";
+			ranges = <0x00000800 0 0x80000000 0x80000000 0 0x00020000   /* configuration space */
+				0x81000000 0 0	 0x80020000 0 0x00010000   /* downstream I/O */
+				0x82000000 0 0x80030000 0xc0030000 0 0x0ffd0000>; /* non-prefetchable memory */
+			status = "disabled";
+		};

Shouldn't there be more than one interrupt? Normally each root port has four legacy IRQs, in order to support bridge devices.

drivers/pci/host/pcie-spear13xx.c line 326

Quoted 5 lines

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

+	spear13xx_pcie->phy = devm_phy_get(dev, "pcie-phy");
+	if (IS_ERR(spear13xx_pcie->phy)) {
+		dev_err(dev, "Could not get PHY\n");
+		return -EPROBE_DEFER;
+	}

I think you should only return -EPROBE_DEFER if you got that error from the PHY layer. If there is some other problem with getting the PHY, you want that returned as a fatal error here and not retry the PCIe probe function.

drivers/phy/phy-spear13xx-sata-pcie.c line 73

Quoted 9 lines

drivers/phy/phy-spear13xx-sata-pcie.c +0 -0

index 6adfa64..5eabf51 100644
@@ -71,6 +71,80 @@
 	#define SPEAR1340_PCIE_SATA_MIPHY_CFG_PCIE \
 			(SPEAR1340_MIPHY_OSC_BYPASS_EXT | \
 			SPEAR1340_MIPHY_PLL_RATIO_TOP(25))

Please split this out into a separate patch.

Quoted 17 lines

drivers/phy/phy-spear13xx-sata-pcie.c +16 -0

+static int spear1310_pcie_miphy_init(struct spear13xx_phy_priv *phypriv)
+{
+	u32 mask, val;
+
+	regmap_update_bits(phypriv->misc, SPEAR1310_PCIE_MIPHY_CFG_1,
+			SPEAR1310_PCIE_SATA_MIPHY_CFG_PCIE_MASK,
+			SPEAR1310_PCIE_SATA_MIPHY_CFG_PCIE);
+
+	switch (phypriv->id) {
+	case 0:
+		mask = SPEAR1310_PCIE_CFG_MASK(0);
+		val = SPEAR1310_PCIE_CFG_VAL(0);
+		break;
+	case 1:
+		mask = SPEAR1310_PCIE_CFG_MASK(1);
+		val = SPEAR1310_PCIE_CFG_VAL(1);

drivers/phy/phy-spear13xx-sata-pcie.c static int sata_miphy_resume(struct spear13xx_phy_priv *phypriv) · line 272

Quoted 5 lines

drivers/phy/phy-spear13xx-sata-pcie.c +5 -0

+		break;
+	case 2:
+		mask = SPEAR1310_PCIE_CFG_MASK(2);
+		val = SPEAR1310_PCIE_CFG_VAL(2);
+		break;

Ah, so this is what the ID gets used for. I would indeed encode this in the phy node, unlike the configuration of whether it's used for PCIe or SATA.

drivers/phy/phy-spear13xx-sata-pcie.c static int sata_miphy_resume(struct spear13xx_phy_priv *phypriv) · line 317

Quoted 10 lines

drivers/phy/phy-spear13xx-sata-pcie.c +9 -0

+static int pcie_miphy_init(struct spear13xx_phy_priv *phypriv)
+{
+	if (of_machine_is_compatible("st,spear1340"))
+		return spear1340_pcie_miphy_init(phypriv);
+	else if (of_machine_is_compatible("st,spear1310"))
+		return spear1310_pcie_miphy_init(phypriv);
+	else
+		return -EINVAL;
+}

You should never check global properties such as the machine compatible value to make local decisions. You have two options here: Either use two different compatible strings for the phy node, or encode the difference in another property. If the only difference between spear1310 and spear1340 is the location of the register within the "misc" syscon space, a good represenation would be to put the register offset next to the syscon phandle in the same property. That way it could transparently work for future SoCs.

Arnd

Quoted 17 lines · Arnd Bergmann

Quoted 16 lines

arch/arm/boot/dts/spear1310.dtsi +16 -0

+             pcie0: pcie@b1000000 {
+                     compatible = "st,spear1340-pcie", "snps,dw-pcie";
+                     reg = <0xb1000000 0x4000>;
+                     interrupts = <0 68 0x4>;
+                     pcie_is_gen1 = <0>;
+                     num-lanes = <1>;
+                     phys = <&miphy0 1 0>;
+                     phy-names = "pcie-phy";
+                     #address-cells = <3>;
+                     #size-cells = <2>;
+                     device_type = "pci";
+                     ranges = <0x00000800 0 0x80000000 0x80000000 0 0x00020000   /* configuration space */
+                             0x81000000 0 0   0x80020000 0 0x00010000   /* downstream I/O */
+                             0x82000000 0 0x80030000 0xc0030000 0 0x0ffd0000>; /* non-prefetchable memory */
+                     status = "disabled";
+             };

Quoted 3 lines

Shouldn't there be more than one interrupt? Normally each root port has four legacy IRQs, in order to support bridge devices.

Sorry, my mistake: I was thinking of the interrupt map for legacy IRQs. The interrupt here is used only for the integrated MSI controller, right?

That seems fine from the DT bindings perspective but raises two other questions:

1. Are you not lacking an interrupt-map property to enable legacy IntA IRQs?

2. If the MSI controller is integrated in the pcie host controller, does that maintain the PCIe ordering guarantees between inbound DMA and MSI, or is it possible that the <0 68 0x4> IRQ gets raised at the CPU before the the DMA transfer becomes visible to the CPU in main memory?

Arnd

-----Original Message-----
From: Arnd Bergmann [mailto:arnd at arndb.de]
Sent: Thursday, January 30, 2014 6:32 PM
To: Mohit KUMAR DCG
Cc: Pratyush ANAND; spear-devel; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH V3 3/8] SPEAr13xx: defconfig: Update

Quoted 2 lines · Mohit Kumar

Enable EABI, OEABI, VFP and NFS configs in default configuration file for SPEAr13xx.

Are you sure about OABI_COMPAT? That seems unusual.

Also, please add all the options you need to multi_v7_defconfig and ensure that this configuration works with your hardware as well.

Quoted 2 lines

Having the PHY driver get initialized after the PCI root driver should also work, but it requires correct handling of -EPROBE_DEFER: if phy_get

I had issue with phy-core driver getting initialized after pcie rc driver. I found a kernel crash, as devm_phy_get was called before phy_class was created. I think this too need to be fixed, we should not see a crash.

Anyway, I will keep spear phy and rc driver both with module_init and -EPROBE_DEFER implementation.

Quoted 14 lines · Arnd Bergmann

Documentation/devicetree/bindings/phy/spear13xx-miphy.txt line 2

Quoted 9 lines · Mohit Kumar

Documentation/devicetree/bindings/phy/spear13xx-miphy.txt new file +2 -0

new file mode 100644
index 0000000..208b37d
@@ -0,0 +1,8 @@
+Required properties:
+- compatible : should be "st,spear1340-sata-pcie-phy".

Just for confirmation: This phy is by design only capable of driving sata or pcie, but nothing else if reused in a different SoC, right?

Quoted 2 lines

If the phy is actually more generic than that, I'd suggest changing the name, otherwise it's ok.

OK, we will give a generic name as it can be used for sata/pcie/usb3.0. Although, phy register definition may vary from version to version, but that can be managed,as and when support of new soc is added.

Quoted 4 lines · Arnd Bergmann

On Thursday 30 January 2014, Mohit Kumar wrote:

Ah, so this is what the ID gets used for. I would indeed encode this in the phy node, unlike the configuration of whether it's used for PCIe or SATA.

ok.. ll use "phy_id = <n>;" in phy node.

Quoted 10 lines

drivers/phy/phy-spear13xx-sata-pcie.c static int sata_miphy_resume(struct spear13xx_phy_priv *phypriv) · line 317

Quoted 9 lines

drivers/phy/phy-spear13xx-sata-pcie.c +9 -0

+static int pcie_miphy_init(struct spear13xx_phy_priv *phypriv)
+{
+	if (of_machine_is_compatible("st,spear1340"))
+		return spear1340_pcie_miphy_init(phypriv);
+	else if (of_machine_is_compatible("st,spear1310"))
+		return spear1310_pcie_miphy_init(phypriv);
+	else
+		return -EINVAL;
+}

Quoted 8 lines

You should never check global properties such as the machine compatible value to make local decisions. You have two options here: Either use two different compatible strings for the phy node, or encode the difference in another property. If the only difference between spear1310 and spear1340 is the location of the register within the "misc" syscon space, a good represenation would be to put the register offset next to the syscon phandle in the same property. That way it could transparently work for future SoCs.

Currently, there is only difference in misc syscon space. But, as I said few phy register definition might also change in different soc.

Ok.. I ll go with first option, ie different compatible string for different socs. It seems most logical also.

Quoted 5 lines

That seems fine from the DT bindings perspective but raises two other questions:

1. Are you not lacking an interrupt-map property to enable legacy IntA IRQs?

As current pcie-designeware driver is not supporting legacy IntA IRQs, so we left it.

Quoted 5 lines

2. If the MSI controller is integrated in the pcie host controller, does that maintain the PCIe ordering guarantees between inbound DMA and MSI, or is it possible that the <0 68 0x4> IRQ gets raised at the CPU before the the DMA transfer becomes visible to the CPU in main memory?

If the system does not guarantee it, then won't be it a bug in the hardware?
In our case, there is no separate interrupt for DMA completion. (I do not know if other system does have DMA interrupt). We have only one interrupt and when it is received SW will look into main memory (MSI address) for MSI data. If DMA transfer is yet not complete, then SW ll read junk data and which will be a bug.

We have never seen any erroneous behaviour with MSI interrupt.

Quoted 6 lines · Pratyush Anand

Quoted 5 lines

That seems fine from the DT bindings perspective but raises two other questions:

1. Are you not lacking an interrupt-map property to enable legacy IntA IRQs?

Quoted 2 lines

As current pcie-designeware driver is not supporting legacy IntA IRQs, so we left it.

Hmm, that sounds hard to believe. Doesn't that exclude 90% of the add-on cards? I noticed that imx also doesn't have it, but exynos does.

Can you check the data sheet again? Maybe the IntA IRQs are not mapped to

host (GIC) IRQs but instead get handled internally in the MSI controller? IIRC, PCIe INTa IRQs are implemented as MSI on the bus, but normally get turned into physical IRQ lines by the root complex. If the RC contains the MSI controller itself, that may have a special register for the LSI.

Quoted 15 lines

Quoted 6 lines

2. If the MSI controller is integrated in the pcie host controller, does that maintain the PCIe ordering guarantees between inbound DMA and MSI, or is it possible that the <0 68 0x4> IRQ gets raised at the CPU before the the DMA transfer becomes visible to the CPU in main memory?

If the system does not guarantee it, then won't be it a bug in the hardware?
In our case, there is no separate interrupt for DMA completion. (I do not know if other system does have DMA interrupt). We have only one interrupt and when it is received SW will look into main memory (MSI address) for MSI data. If DMA transfer is yet not complete, then SW ll read junk data and which will be a bug.

Quoted 1 line

We have never seen any erroneous behaviour with MSI interrupt.

There should not be a separate interrupt for DMA, the typical behavior of a PCIe adapter (SCSI, ethernet, ...) is that it sends an MSI after data has arrived from an external interface and gets submitted as a bus-master DMA into main memory. The actual data transfer may have the 'relaxed ordering' bit set on the PCIe transaction, but the MSI message (which is essentially a 4-byte DMA) will not, which means that all buses are required to only forward the MSI after the DMA is completed. If the PCIe host is located on a bus that is not directly connected to the memory controller, the RC may have seen the DMA complete and signalled the IRC to the CPU while the data transfer is still in progress on its way to the actual memory.

That kind of problem is extremely hard to debug and will only occur in rare cases of bus congestion.

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

Quoted 7 lines · Arnd Bergmann

Quoted 6 lines · Pratyush Anand

Quoted 5 lines

That seems fine from the DT bindings perspective but raises two other questions:

1. Are you not lacking an interrupt-map property to enable legacy IntA IRQs?

Quoted 7 lines

Quoted 2 lines

As current pcie-designeware driver is not supporting legacy IntA IRQs, so we left it.

Hmm, that sounds hard to believe. Doesn't that exclude 90% of the add-on cards? I noticed that imx also doesn't have it, but exynos does.

Can you check the data sheet again? Maybe the IntA IRQs are not mapped to

I did not say that designware controller does not supoort it. It does support. But pcie-designware driver is not supporting it as of now. I think, a common irq_chip for intx handling need to be created in designware driver.

I am not sure even if exynos does define dt bindings for irq, are they able to use it?
Jingoo can comment.

Quoted 6 lines

host (GIC) IRQs but instead get handled internally in the MSI controller? IIRC, PCIe INTa IRQs are implemented as MSI on the bus, but normally get turned into physical IRQ lines by the root complex. If the RC contains the MSI controller itself, that may have a special register for the LSI.

I agree that in PCIe Intx is also handled as message only. As far as designware controller is concerned, it has different intx and msi controller. Now, a SoC can have two different irq line or can have a single irq line at gic for MSI and INTx. In SPEAr13xx we have a single IRQ line for both MSI and INTx.

Once this patch set gets into mainline, may be someone of us will work to add irq chip for INTx in common designware driver, and also its wrapper in SPEAr13xx pcie driver. At that time we will add interrupt-map property to enable legacy IntA.

Quoted 29 lines

Quoted 15 lines

Quoted 6 lines

2. If the MSI controller is integrated in the pcie host controller, does that maintain the PCIe ordering guarantees between inbound DMA and MSI, or is it possible that the <0 68 0x4> IRQ gets raised at the CPU before the the DMA transfer becomes visible to the CPU in main memory?

If the system does not guarantee it, then won't be it a bug in the hardware?
In our case, there is no separate interrupt for DMA completion. (I do not know if other system does have DMA interrupt). We have only one interrupt and when it is received SW will look into main memory (MSI address) for MSI data. If DMA transfer is yet not complete, then SW ll read junk data and which will be a bug.

Quoted 1 line

We have never seen any erroneous behaviour with MSI interrupt.

There should not be a separate interrupt for DMA, the typical behavior of a PCIe adapter (SCSI, ethernet, ...) is that it sends an MSI after data has arrived from an external interface and gets submitted as a bus-master DMA into main memory. The actual data transfer may have the 'relaxed ordering' bit set on the PCIe transaction, but the MSI message (which is essentially a 4-byte DMA) will not, which means that all buses are required to only forward the MSI after the DMA is completed. If the PCIe host is located on a bus that is not directly connected to the memory controller, the RC may have seen the DMA complete and signalled the IRC to the CPU while the data transfer is still in progress on its way to the actual memory.

Quoted 2 lines

That kind of problem is extremely hard to debug and will only occur in rare cases of bus congestion.

Agreed, But there is anything special for software to do here?

Thanks a lot for your review.

Regards
Pratyush

Quoted 3 lines

Arnd
--

Add Mohit Kumar as maintainer for ST SPEAr13xx PCIe driver.

Signed-off-by Mohit Kumar <[email protected]>

Signed-off-by Pratyush Anand <[email protected]>

Cc Jingoo Han <[email protected]>

Cc [email protected]

---
 MAINTAINERS |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

MAINTAINERS +6 -1

index 8285ed4..fd03da6 100644
@@ -6462,6 +6462,12 @@ L:	[email protected]
 S:	Maintained
 F:	drivers/pci/host/pci-exynos.c
 
+PCIE DRIVER FOR ST SPEAR13XX
+M:	Mohit Kumar <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	drivers/pci/host/pcie-spear13xx.c
+
 PCMCIA SUBSYSTEM
 P:	Linux PCMCIA Team
 L:	[email protected]
--

1.7.0.1