PCI: layerscape: remove ls_pcie_establish_link()

PATCH New Minghuan Lian 3 participants 22 messages Oct 16, 2015 – Nov 3, 2015 PCI

Share & tools

Minghuan Lian

author

2015-10-16 07:18 UTC #0 lore raw

PATCH v4 1/6 PCI: layerscape: remove ls_pcie_establish_link()

ls_pcie_establish_link() does not do any real operation, except to wait for the linkup establishment. In fact, this is not necessary. Moreover, each PCIe controller not inserted device will increase the Linux startup time about 200ms.

Signed-off-by Minghuan Lian <[email protected]>

---
Change log
v4: split from [PATCH v3] PCI: layerscape: Add PCIe support for LS1043a and LS2080a

 drivers/pci/host/pci-layerscape.c | 16 ----------------
 1 file changed, 16 deletions(-)

drivers/pci/host/pci-layerscape.c +0 ~~-16~~

index b2328ea1..6dd44a0 100644
@@ -11,7 +11,6 @@
  */
 
 #include <linux/kernel.h>
-#include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of_pci.h>

@@ -62,27 +61,12 @@ static int ls_pcie_link_up(struct pcie_port *pp)
 	return 1;
 }
 
-static int ls_pcie_establish_link(struct pcie_port *pp)
-{
-	unsigned int retries;
-
-	for (retries = 0; retries < 200; retries++) {
-		if (dw_pcie_link_up(pp))
-			return 0;
-		usleep_range(100, 1000);
-	}
-
-	dev_err(pp->dev, "phy link never came up\n");
-	return -EINVAL;
-}
-
 static void ls_pcie_host_init(struct pcie_port *pp)
 {
 	struct ls_pcie *pcie = to_ls_pcie(pp);
 	u32 val;
 
 	dw_pcie_setup_rc(pp);
-	ls_pcie_establish_link(pp);
 
 	/*
 	 * LS1021A Workaround for internal TKT228622

Signature · 1 line

1.9.1

Minghuan Lian

author

2015-10-16 07:18 UTC #1 lore raw

PATCH v4 2/6 PCI: layerscape: check PCIe controller work mode

Layerscape PCIe controller supports root complex (RC) and endpoint (EP) mode. The mode can be set by RCW. The patch will check it. If not in RC mode, the driver directly returns -ENODEV.

Signed-off-by Minghuan Lian <[email protected]>

---
Change log
v4: split from [PATCH v3] PCI: layerscape: Add PCIe support for LS1043a and LS2080a

 drivers/pci/host/pci-layerscape.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

drivers/pci/host/pci-layerscape.c +13 -0

index 6dd44a0..5eabe92 100644
@@ -47,6 +47,16 @@ struct ls_pcie {
 
 #define to_ls_pcie(x)	container_of(x, struct ls_pcie, pp)
 
+static bool ls_pcie_is_bridge(struct ls_pcie *pcie)
+{
+	u32 header_type;
+
+	header_type = ioread8(pcie->dbi + PCI_HEADER_TYPE);
+	header_type &= 0x7f;
+
+	return header_type == PCI_HEADER_TYPE_BRIDGE;
+}
+
 static int ls_pcie_link_up(struct pcie_port *pp)
 {
 	u32 state;

@@ -135,6 +145,9 @@ static int __init ls_pcie_probe(struct platform_device *pdev)
 		return ret;
 	pcie->index = index[1];
 
+	if (!ls_pcie_is_bridge(pcie))
+		return -ENODEV;
+
 	ret = ls_add_pcie_port(pcie);
 	if (ret < 0)
 		return ret;

Signature · 1 line

1.9.1

Minghuan Lian

author

2015-10-16 07:18 UTC #2 lore raw

PATCH v4 3/6 PCI: layerscape: factor out SCFG related function

In reply to Minghuan Lian

For LS1021a PCIe controller, some status registers are located in SCFG. This is different to other kind of Layerscape. The patch moves SCFG related code to ls1021_pcie_host_init() and rename ls_pcie_link_up to ls1021_pcie_link_up because LTSSM status is also in SCFG.

Signed-off-by Minghuan Lian <[email protected]>

---
Change log
v4: split from [PATCH v3] PCI: layerscape: Add PCIe support for LS1043a and LS2080a

 drivers/pci/host/pci-layerscape.c | 72 +++++++++++++++++++++++++--------------
 1 file changed, 46 insertions(+), 26 deletions(-)

drivers/pci/host/pci-layerscape.c +46 ~~-26~~

index 5eabe92..89b7eb8 100644
@@ -34,6 +34,10 @@
 /* Symbol Timer Register and Filter Mask Register 1 */
 #define PCIE_STRFMR1 0x71c
 
+struct ls_pcie_drvdata {
+	struct pcie_host_ops *ops;
+};
+
 struct ls_pcie {
 	struct list_head node;
 	struct device *dev;

@@ -41,6 +45,7 @@ struct ls_pcie {
 	void __iomem *dbi;
 	struct regmap *scfg;
 	struct pcie_port pp;
+	const struct ls_pcie_drvdata *drvdata;
 	int index;
 	int msi_irq;
 };

@@ -57,11 +62,14 @@ static bool ls_pcie_is_bridge(struct ls_pcie *pcie)
 	return header_type == PCI_HEADER_TYPE_BRIDGE;
 }
 
-static int ls_pcie_link_up(struct pcie_port *pp)
+static int ls1021_pcie_link_up(struct pcie_port *pp)
 {
 	u32 state;
 	struct ls_pcie *pcie = to_ls_pcie(pp);
 
+	if (!pcie->scfg)
+		return 0;
+
 	regmap_read(pcie->scfg, SCFG_PEXMSCPORTSR(pcie->index), &state);
 	state = (state >> LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;

@@ -71,10 +79,25 @@ static int ls_pcie_link_up(struct pcie_port *pp)
 	return 1;
 }
 
-static void ls_pcie_host_init(struct pcie_port *pp)
+static void ls1021_pcie_host_init(struct pcie_port *pp)
 {
 	struct ls_pcie *pcie = to_ls_pcie(pp);
-	u32 val;
+	u32 val, index[2];
+
+	pcie->scfg = syscon_regmap_lookup_by_phandle(pp->dev->of_node,
+						     "fsl,pcie-scfg");
+	if (IS_ERR(pcie->scfg)) {
+		dev_err(pp->dev, "No syscfg phandle specified\n");
+		pcie->scfg = NULL;
+		return;
+	}
+
+	if (of_property_read_u32_array(pp->dev->of_node,
+				       "fsl,pcie-scfg", index, 2)) {
+		pcie->scfg = NULL;
+		return;
+	}
+	pcie->index = index[1];
 
 	dw_pcie_setup_rc(pp);

@@ -87,11 +110,21 @@ static void ls_pcie_host_init(struct pcie_port *pp)
 	iowrite32(val, pcie->dbi + PCIE_STRFMR1);
 }
 
-static struct pcie_host_ops ls_pcie_host_ops = {
-	.link_up = ls_pcie_link_up,
-	.host_init = ls_pcie_host_init,
+static struct pcie_host_ops ls1021_pcie_host_ops = {
+	.link_up = ls1021_pcie_link_up,
+	.host_init = ls1021_pcie_host_init,
+};
+
+static struct ls_pcie_drvdata ls1021_drvdata = {
+	.ops = &ls1021_pcie_host_ops,
 };
 
+static const struct of_device_id ls_pcie_of_match[] = {
+	{ .compatible = "fsl,ls1021a-pcie", .data = &ls1021_drvdata },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ls_pcie_of_match);
+
 static int ls_add_pcie_port(struct ls_pcie *pcie)
 {
 	struct pcie_port *pp;

@@ -101,7 +134,7 @@ static int ls_add_pcie_port(struct ls_pcie *pcie)
 	pp->dev = pcie->dev;
 	pp->dbi_base = pcie->dbi;
 	pp->root_bus_nr = -1;
-	pp->ops = &ls_pcie_host_ops;
+	pp->ops = pcie->drvdata->ops;
 
 	ret = dw_pcie_host_init(pp);
 	if (ret) {

@@ -114,11 +147,15 @@ static int ls_add_pcie_port(struct ls_pcie *pcie)
 
 static int __init ls_pcie_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *match;
 	struct ls_pcie *pcie;
 	struct resource *dbi_base;
-	u32 index[2];
 	int ret;
 
+	match = of_match_device(ls_pcie_of_match, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+
 	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
 	if (!pcie)
 		return -ENOMEM;

@@ -132,18 +169,7 @@ static int __init ls_pcie_probe(struct platform_device *pdev)
 		return PTR_ERR(pcie->dbi);
 	}
 
-	pcie->scfg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
-						     "fsl,pcie-scfg");
-	if (IS_ERR(pcie->scfg)) {
-		dev_err(&pdev->dev, "No syscfg phandle specified\n");
-		return PTR_ERR(pcie->scfg);
-	}
-
-	ret = of_property_read_u32_array(pdev->dev.of_node,
-					 "fsl,pcie-scfg", index, 2);
-	if (ret)
-		return ret;
-	pcie->index = index[1];
+	pcie->drvdata = match->data;
 
 	if (!ls_pcie_is_bridge(pcie))
 		return -ENODEV;

@@ -157,12 +183,6 @@ static int __init ls_pcie_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id ls_pcie_of_match[] = {
-	{ .compatible = "fsl,ls1021a-pcie" },
-	{ },
-};
-MODULE_DEVICE_TABLE(of, ls_pcie_of_match);
-
 static struct platform_driver ls_pcie_driver = {
 	.driver = {
 		.name = "layerscape-pcie",

Signature · 1 line

1.9.1

Minghuan Lian

author

2015-10-16 07:18 UTC #3 lore raw

PATCH v4 4/6 PCI: layerscape: update ls_add_pcie_port()

In reply to Minghuan Lian

To keep consistent with the other DesignWare-based host drivers, The patch updates ls_add_pcie_port().

Signed-off-by Minghuan Lian <[email protected]>

---
Change log
v4: split from [PATCH v3] PCI: layerscape: Add PCIe support for LS1043a and LS2080a

 drivers/pci/host/pci-layerscape.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

drivers/pci/host/pci-layerscape.c +5 -8

index 89b7eb8..891e504 100644
@@ -125,15 +125,14 @@ static const struct of_device_id ls_pcie_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, ls_pcie_of_match);
 
-static int ls_add_pcie_port(struct ls_pcie *pcie)
+static int __init
+ls_add_pcie_port(struct pcie_port *pp, struct platform_device *pdev)
 {
-	struct pcie_port *pp;
 	int ret;
+	struct ls_pcie *pcie = to_ls_pcie(pp);
 
-	pp = &pcie->pp;
-	pp->dev = pcie->dev;
+	pp->dev = &pdev->dev;
 	pp->dbi_base = pcie->dbi;
-	pp->root_bus_nr = -1;
 	pp->ops = pcie->drvdata->ops;
 
 	ret = dw_pcie_host_init(pp);

@@ -160,8 +159,6 @@ static int __init ls_pcie_probe(struct platform_device *pdev)
 	if (!pcie)
 		return -ENOMEM;
 
-	pcie->dev = &pdev->dev;
-
 	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
 	pcie->dbi = devm_ioremap_resource(&pdev->dev, dbi_base);
 	if (IS_ERR(pcie->dbi)) {

@@ -174,7 +171,7 @@ static int __init ls_pcie_probe(struct platform_device *pdev)
 	if (!ls_pcie_is_bridge(pcie))
 		return -ENODEV;
 
-	ret = ls_add_pcie_port(pcie);
+	ret = ls_add_pcie_port(&pcie->pp, pdev);
 	if (ret < 0)
 		return ret;

Signature · 1 line

1.9.1

Minghuan Lian

author

2015-10-16 07:18 UTC #4 lore raw

PATCH v4 5/6 PCI: layerscape: add PCIe support for LS1043a and LS2080a

In reply to Minghuan Lian

Both LS1043a and LS2080a are based on ARMv8 64-bit architecture and have similar PCIe implementation. LUT is added to controller. The patch removes the necessary fields from struct ls_pcie.

Signed-off-by Minghuan Lian <[email protected]>

---
This patch is based on v4.3-rc4 and [PATCH v10 3/6]
PCI: designware: Add ARM64 support.

change log
v4:
1. split to 6 patches.
2. use ARCH_LAYERSCAPE instead of ARM64

v3:
1. Use 8 or 16 bit access function to simplify code
2. Add ls_add_pcie_port in accordance with other DesignWare-based drivers

v2:
1. Rename ls2085a to ls2080a
2. Add ls_pcie_msi_host_init()

 drivers/pci/host/Kconfig          |  2 +-
 drivers/pci/host/pci-layerscape.c | 72 +++++++++++++++++++++++++++++++++++----
 2 files changed, 67 insertions(+), 7 deletions(-)

drivers/pci/host/Kconfig +1 -1

index ae873be..8eb09ea 100644
@@ -105,7 +105,7 @@ config PCI_XGENE_MSI
 
 config PCI_LAYERSCAPE
 	bool "Freescale Layerscape PCIe controller"
-	depends on OF && ARM
+	depends on OF && (ARM || ARCH_LAYERSCAPE)
 	select PCIE_DW
 	select MFD_SYSCON
 	help

drivers/pci/host/pci-layerscape.c +66 -6

118 lines not on this page · raw message

Signature · 1 line

1.9.1

Bjorn Helgaas

2015-10-21 21:36 UTC #5 lore raw

PATCH v4 5/6 PCI: layerscape: add PCIe support for LS1043a and LS2080a

Quoted 56 lines · Minghuan Lian

Both LS1043a and LS2080a are based on ARMv8 64-bit architecture and have similar PCIe implementation. LUT is added to controller. The patch removes the necessary fields from struct ls_pcie.
Signed-off-by Minghuan Lian <[email protected]>
---
This patch is based on v4.3-rc4 and [PATCH v10 3/6]
PCI: designware: Add ARM64 support.
change log
v4:
1. split to 6 patches.
2. use ARCH_LAYERSCAPE instead of ARM64
v3:
1. Use 8 or 16 bit access function to simplify code
2. Add ls_add_pcie_port in accordance with other DesignWare-based drivers
v2:
1. Rename ls2085a to ls2080a
2. Add ls_pcie_msi_host_init()
 drivers/pci/host/Kconfig          |  2 +-
 drivers/pci/host/pci-layerscape.c | 72 +++++++++++++++++++++++++++++++++++----
 2 files changed, 67 insertions(+), 7 deletions(-)
drivers/pci/host/Kconfig +1 -1
index ae873be..8eb09ea 100644
@@ -105,7 +105,7 @@ config PCI_XGENE_MSI
 
 config PCI_LAYERSCAPE
 	bool "Freescale Layerscape PCIe controller"
-	depends on OF && ARM
+	depends on OF && (ARM || ARCH_LAYERSCAPE)
 	select PCIE_DW
 	select MFD_SYSCON
 	help
drivers/pci/host/pci-layerscape.c +6 -2
index 891e504..c53692a 100644
@@ -31,23 +31,26 @@
 #define LTSSM_STATE_MASK	0x3f
 #define LTSSM_PCIE_L0		0x11 /* L0 state */
 
-/* Symbol Timer Register and Filter Mask Register 1 */
-#define PCIE_STRFMR1 0x71c
+/* PEX Internal Configuration Registers */
+#define PCIE_STRFMR1		0x71c /* Symbol Timer & Filter Mask Register1 */
+#define PCIE_DBI_RO_WR_EN	0x8bc /* DBI Read-Only Write Enable Register */
+
+/* PEX LUT registers */
+#define PCIE_LUT_DBG		0x7FC /* PEX LUT Debug Register */
 

drivers/pci/host/pci-layerscape.c line 44

Quoted 10 lines

drivers/pci/host/pci-layerscape.c +2 -3

 struct ls_pcie_drvdata {
+	u32 lut_offset;
+	u32 ltssm_shift;
 	struct pcie_host_ops *ops;
 };
 
 struct ls_pcie {
-	struct list_head node;
-	struct device *dev;
-	struct pci_bus *bus;

These fields look unused and unrelated to LS1043a and LS2080a. I split their removal into a separate patch.

Quoted 106 lines

drivers/pci/host/pci-layerscape.c +58 -1

 	void __iomem *dbi;
+	void __iomem *lut;
 	struct regmap *scfg;
 	struct pcie_port pp;
 	const struct ls_pcie_drvdata *drvdata;
 	int index;
-	int msi_irq;
 };
 
 #define to_ls_pcie(x)	container_of(x, struct ls_pcie, pp)

@@ -62,6 +65,18 @@ static bool ls_pcie_is_bridge(struct ls_pcie *pcie)
 	return header_type == PCI_HEADER_TYPE_BRIDGE;
 }
 
+/* Clear multi-function bit */
+static void ls_pcie_clear_multifunction(struct ls_pcie *pcie)
+{
+	iowrite8(PCI_HEADER_TYPE_BRIDGE, pcie->dbi + PCI_HEADER_TYPE);
+}
+
+/* Fix class value */
+static void ls_pcie_fix_class(struct ls_pcie *pcie)
+{
+	iowrite16(PCI_CLASS_BRIDGE_PCI, pcie->dbi + PCI_CLASS_DEVICE);
+}
+
 static int ls1021_pcie_link_up(struct pcie_port *pp)
 {
 	u32 state;

@@ -110,17 +125,61 @@ static void ls1021_pcie_host_init(struct pcie_port *pp)
 	iowrite32(val, pcie->dbi + PCIE_STRFMR1);
 }
 
+static int ls_pcie_link_up(struct pcie_port *pp)
+{
+	struct ls_pcie *pcie = to_ls_pcie(pp);
+	u32 state;
+
+	state = (ioread32(pcie->lut + PCIE_LUT_DBG) >>
+		 pcie->drvdata->ltssm_shift) &
+		 LTSSM_STATE_MASK;
+
+	if (state < LTSSM_PCIE_L0)
+		return 0;
+
+	return 1;
+}
+
+static void ls_pcie_host_init(struct pcie_port *pp)
+{
+	struct ls_pcie *pcie = to_ls_pcie(pp);
+
+	iowrite32(1, pcie->dbi + PCIE_DBI_RO_WR_EN);
+	ls_pcie_fix_class(pcie);
+	ls_pcie_clear_multifunction(pcie);
+	iowrite32(0, pcie->dbi + PCIE_DBI_RO_WR_EN);
+}
+
 static struct pcie_host_ops ls1021_pcie_host_ops = {
 	.link_up = ls1021_pcie_link_up,
 	.host_init = ls1021_pcie_host_init,
 };
 
+static struct pcie_host_ops ls_pcie_host_ops = {
+	.link_up = ls_pcie_link_up,
+	.host_init = ls_pcie_host_init,
+};
+
 static struct ls_pcie_drvdata ls1021_drvdata = {
 	.ops = &ls1021_pcie_host_ops,
 };
 
+static struct ls_pcie_drvdata ls1043_drvdata = {
+	.lut_offset = 0x10000,
+	.ltssm_shift = 24,
+	.ops = &ls_pcie_host_ops,
+};
+
+static struct ls_pcie_drvdata ls2080_drvdata = {
+	.lut_offset = 0x80000,
+	.ltssm_shift = 0,
+	.ops = &ls_pcie_host_ops,
+};
+
 static const struct of_device_id ls_pcie_of_match[] = {
 	{ .compatible = "fsl,ls1021a-pcie", .data = &ls1021_drvdata },
+	{ .compatible = "fsl,ls1043a-pcie", .data = &ls1043_drvdata },
+	{ .compatible = "fsl,ls2080a-pcie", .data = &ls2080_drvdata },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, ls_pcie_of_match);

@@ -167,6 +226,7 @@ static int __init ls_pcie_probe(struct platform_device *pdev)
 	}
 
 	pcie->drvdata = match->data;
+	pcie->lut = pcie->dbi + pcie->drvdata->lut_offset;
 
 	if (!ls_pcie_is_bridge(pcie))
 		return -ENODEV;

Signature · 6 lines

1.9.1

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

Li Yang

2015-10-22 17:38 UTC #6 lore raw

PATCH v4 5/6 PCI: layerscape: add PCIe support for LS1043a and LS2080a

Quoted 9 lines · Bjorn Helgaas

Quoted 8 lines · Minghuan Lian
Both LS1043a and LS2080a are based on ARMv8 64-bit architecture and have similar PCIe implementation. LUT is added to controller. The patch removes the necessary fields from struct ls_pcie.
Signed-off-by Minghuan Lian <[email protected]>
---
This patch is based on v4.3-rc4 and [PATCH v10 3/6]
PCI: designware: Add ARM64 support.

Hi Bjorn,

Are you also applying this patch
https://patchwork.ozlabs.org/patch/531184/ in this merge windows? Because the new hardware can only work with the generic ARM64 support added in the designware pcie driver.

Regards,
Leo

Bjorn Helgaas

2015-10-22 18:08 UTC #7 lore raw

PATCH v4 5/6 PCI: layerscape: add PCIe support for LS1043a and LS2080a

Hi Leo,

Quoted 11 lines · Li Yang

Quoted 9 lines · Bjorn Helgaas
Quoted 8 lines · Minghuan Lian
Both LS1043a and LS2080a are based on ARMv8 64-bit architecture and have similar PCIe implementation. LUT is added to controller. The patch removes the necessary fields from struct ls_pcie.
Signed-off-by Minghuan Lian <[email protected]>
---
This patch is based on v4.3-rc4 and [PATCH v10 3/6]
PCI: designware: Add ARM64 support.

Quoted 6 lines

Hi Bjorn,
Are you also applying this patch
https://patchwork.ozlabs.org/patch/531184/ in this merge windows? Because the new hardware can only work with the generic ARM64 support added in the designware pcie driver.

I'm reviewing that patch right now, and I do hope to apply it for v4.4.

There was no cover letter for this series (Minghuan's Layerscape v4 series), and I didn't see any mention of dependencies, so I applied it on top of v4.3-rc1. Is that the wrong thing to do? If it depends on the Zhou's v11 series, I can hold the Layerscape branch until after that.

Bjorn

Li Yang

2015-10-22 19:17 UTC #8 lore raw

PATCH v4 5/6 PCI: layerscape: add PCIe support for LS1043a and LS2080a

Quoted 21 lines · Bjorn Helgaas

Hi Leo,
Quoted 11 lines · Li Yang
Quoted 9 lines · Bjorn Helgaas
Quoted 8 lines · Minghuan Lian
Both LS1043a and LS2080a are based on ARMv8 64-bit architecture and have similar PCIe implementation. LUT is added to controller. The patch removes the necessary fields from struct ls_pcie.
Signed-off-by Minghuan Lian <[email protected]>
---
This patch is based on v4.3-rc4 and [PATCH v10 3/6]
PCI: designware: Add ARM64 support.
Quoted 6 lines
Hi Bjorn,
Are you also applying this patch
https://patchwork.ozlabs.org/patch/531184/ in this merge windows? Because the new hardware can only work with the generic ARM64 support added in the designware pcie driver.

Quoted 8 lines

I'm reviewing that patch right now, and I do hope to apply it for v4.4.
There was no cover letter for this series (Minghuan's Layerscape v4 series), and I didn't see any mention of dependencies, so I applied it on top of v4.3-rc1. Is that the wrong thing to do? If it depends on the Zhou's v11 series, I can hold the Layerscape branch until after that.

I don't know if this patch will break anything without Zhou's patch. But the new hardware mentioned in the description will not be working without the dependent patch. We should hold this patch until after the generic change is merged. Minghuan, are there any bug fixing patch in the series that need to be merged earlier?

Regards,
Leo

Lian M.H.

author

2015-10-23 06:02 UTC #9 lore raw

PATCH v4 5/6 PCI: layerscape: add PCIe support for LS1043a and LS2080a

Hi Bjorn and Leo,

Quoted 45 lines

-----Original Message-----
From: pku.leo at gmail.com [mailto:pku.leo at gmail.com] On Behalf Of Li Yang
Sent: Friday, October 23, 2015 3:18 AM
To: Bjorn Helgaas <[email protected]>
Cc: Arnd Bergmann <[email protected]>; linux-pci at vger.kernel.org; Jingoo Han <[email protected]>; Hu Mingkai-B21284 <[email protected]>; Zang Roy-R61911 <[email protected]>; Yoder Stuart-B08248 <[email protected]>; Lian Minghuan-B31939 <[email protected]>; Zhou Wang <[email protected]>; Bjorn Helgaas <[email protected]>; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH v4 5/6] PCI: layerscape: add PCIe support for LS1043a and LS2080a
Quoted 4 lines · Bjorn Helgaas
Hi Leo,
Quoted 1 line · Li Yang
On Wed, Oct 21, 2015 at 4:36 PM, Bjorn Helgaas <[email protected]>
Quoted 17 lines · wrote:
Quoted 10 lines
Quoted 9 lines · Minghuan Lian
Quoted 8 lines
Both LS1043a and LS2080a are based on ARMv8 64-bit architecture and have similar PCIe implementation. LUT is added to controller. The patch removes the necessary fields from struct ls_pcie.
Signed-off-by Minghuan Lian <[email protected]>
---
This patch is based on v4.3-rc4 and [PATCH v10 3/6]
PCI: designware: Add ARM64 support.
Quoted 6 lines
Hi Bjorn,
Are you also applying this patch
https://patchwork.ozlabs.org/patch/531184/ in this merge windows? Because the new hardware can only work with the generic ARM64 support added in the designware pcie driver.
Quoted 8 lines
I'm reviewing that patch right now, and I do hope to apply it for v4.4.
There was no cover letter for this series (Minghuan's Layerscape v4 series), and I didn't see any mention of dependencies, so I applied it on top of v4.3-rc1. Is that the wrong thing to do? If it depends on the Zhou's v11 series, I can hold the Layerscape branch until after that.

Quoted 6 lines

I don't know if this patch will break anything without Zhou's patch. But the new hardware mentioned in the description will not be working without the dependent patch. We should hold this patch until after the generic change is merged. Minghuan, are there any bug fixing patch in the series that need to be merged earlier?

[Lian Minghuan-B31939] [PATCH v4 5/6] PCI: layerscape: add PCIe support for LS1043a and LS2080a] depended on Zhou's patch. I have described this in this patch. Actually, without zhou's patch, pci_layerscape.c can also be compiled and works well for LS1021A arm32 architecture, just do not support arm64 architecture.

Thank Bjorn very much, all the PCIe patches have been applied except MSI related patches.

Thanks,
Minghuan

Quoted 2 lines

Regards,
Leo

Li Yang

2015-11-02 21:08 UTC #10 lore raw

PATCH v4 5/6 PCI: layerscape: add PCIe support for LS1043a and LS2080a

In reply to Bjorn Helgaas

Quoted 21 lines · Bjorn Helgaas

Hi Leo,
Quoted 11 lines · Li Yang
Quoted 9 lines · Bjorn Helgaas
Quoted 8 lines · Minghuan Lian
Both LS1043a and LS2080a are based on ARMv8 64-bit architecture and have similar PCIe implementation. LUT is added to controller. The patch removes the necessary fields from struct ls_pcie.
Signed-off-by Minghuan Lian <[email protected]>
---
This patch is based on v4.3-rc4 and [PATCH v10 3/6]
PCI: designware: Add ARM64 support.
Quoted 6 lines
Hi Bjorn,
Are you also applying this patch
https://patchwork.ozlabs.org/patch/531184/ in this merge windows? Because the new hardware can only work with the generic ARM64 support added in the designware pcie driver.

Quoted 8 lines

I'm reviewing that patch right now, and I do hope to apply it for v4.4.
There was no cover letter for this series (Minghuan's Layerscape v4 series), and I didn't see any mention of dependencies, so I applied it on top of v4.3-rc1. Is that the wrong thing to do? If it depends on the Zhou's v11 series, I can hold the Layerscape branch until after that.

I noticed that you have merged Zhou's latest patch. Probably you can also merge the Layerscape branch now.

Regards,
Leo

Bjorn Helgaas

2015-11-02 21:36 UTC #11 lore raw

PATCH v4 5/6 PCI: layerscape: add PCIe support for LS1043a and LS2080a

Quoted 25 lines · Li Yang

Quoted 24 lines · Bjorn Helgaas
Hi Leo,
Quoted 11 lines · Li Yang
Quoted 9 lines · Bjorn Helgaas
Quoted 8 lines · Minghuan Lian
Both LS1043a and LS2080a are based on ARMv8 64-bit architecture and have similar PCIe implementation. LUT is added to controller. The patch removes the necessary fields from struct ls_pcie.
Signed-off-by Minghuan Lian <[email protected]>
---
This patch is based on v4.3-rc4 and [PATCH v10 3/6]
PCI: designware: Add ARM64 support.
Quoted 6 lines
Hi Bjorn,
Are you also applying this patch
https://patchwork.ozlabs.org/patch/531184/ in this merge windows? Because the new hardware can only work with the generic ARM64 support added in the designware pcie driver.
I'm reviewing that patch right now, and I do hope to apply it for v4.4.

Quoted 8 lines

Quoted 5 lines
There was no cover letter for this series (Minghuan's Layerscape v4 series), and I didn't see any mention of dependencies, so I applied it on top of v4.3-rc1. Is that the wrong thing to do? If it depends on the Zhou's v11 series, I can hold the Layerscape branch until after that.
I noticed that you have merged Zhou's latest patch. Probably you can also merge the Layerscape branch now.

That's my intention.

Bjorn

Bjorn Helgaas

2015-10-22 15:47 UTC #12 lore raw

PATCH v4 5/6 PCI: layerscape: add PCIe support for LS1043a and LS2080a

In reply to Minghuan Lian

[+cc Fabio, Lucas]

Hi Minghuan,

Quoted 44 lines · Minghuan Lian

Both LS1043a and LS2080a are based on ARMv8 64-bit architecture and have similar PCIe implementation. LUT is added to controller. The patch removes the necessary fields from struct ls_pcie.
Signed-off-by Minghuan Lian <[email protected]>
---
This patch is based on v4.3-rc4 and [PATCH v10 3/6]
PCI: designware: Add ARM64 support.
change log
v4:
1. split to 6 patches.
2. use ARCH_LAYERSCAPE instead of ARM64
v3:
1. Use 8 or 16 bit access function to simplify code
2. Add ls_add_pcie_port in accordance with other DesignWare-based drivers
v2:
1. Rename ls2085a to ls2080a
2. Add ls_pcie_msi_host_init()
 drivers/pci/host/Kconfig          |  2 +-
 drivers/pci/host/pci-layerscape.c | 72 +++++++++++++++++++++++++++++++++++----
 2 files changed, 67 insertions(+), 7 deletions(-)
drivers/pci/host/Kconfig +1 -1
index ae873be..8eb09ea 100644
@@ -105,7 +105,7 @@ config PCI_XGENE_MSI
 
 config PCI_LAYERSCAPE
 	bool "Freescale Layerscape PCIe controller"
-	depends on OF && ARM
+	depends on OF && (ARM || ARCH_LAYERSCAPE)
 	select PCIE_DW
 	select MFD_SYSCON
 	help
drivers/pci/host/pci-layerscape.c +0 -0
index 891e504..c53692a 100644

drivers/pci/host/pci-layerscape.c line 31

Quoted 1 line

drivers/pci/host/pci-layerscape.c +0 -0
@@ -31,23 +31,26 @@
 #define LTSSM_STATE_MASK	0x3f

BTW, not related to *this* patch, but does LTSSM_STATE_MASK really need to be 0x3f (6 bits), or could it be 0x1f (5 bits)?

I'd like to include Layerscape in the LTSSM_STATE_MASK cleanup done by Fabio:

https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/host-designware&id=4788fe6ebf4594c9a95b620cbff05147c8504823

I don't have specs for any of these devices, so I don't know if this is really something that can vary between the different DesignWare-based devices, or if they all should use a mask of 0x1f.

Bjorn

Lian M.H.

author

2015-10-23 05:48 UTC #13 lore raw

PATCH v4 5/6 PCI: layerscape: add PCIe support for LS1043a and LS2080a

Hi Bjorn,

Our LTSSM mask is 0x3f because it includes the following states:
0x20 S_RCVRY_EQ0
0x21 S_RCVRY_EQ1
0x22 S_RCVRY_EQ2
0x23 S_RCVRY_EQ3

And I checked DesignWare Cores PCI Express Controller Databook v4.21a and found the following descriptor:
[5:0]: smlh_ltssm_state: LTSSM current state. Encoding is defined in workspace/src/include/cxpl_defs.vh

So could we use the mask 0x3f for all SoCs?

Anyway, LTSSM_PCIE_L0 in pci-layerscape.c can be replaced by LTSSM_STATE_L0 defined in pcie-designware.h.

Thanks,
Minghuan

Quoted 82 lines

-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas at kernel.org]
Sent: Thursday, October 22, 2015 11:47 PM
To: Lian Minghuan-B31939 <[email protected]>
Cc: linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Zang Roy-R61911 <[email protected]>; Hu Mingkai-B21284 <[email protected]>; Yoder Stuart-B08248 <[email protected]>; Li Yang-Leo-R58472 <[email protected]>; Arnd Bergmann <[email protected]>; Bjorn Helgaas <[email protected]>; Jingoo Han <[email protected]>; Zhou Wang <[email protected]>; Estevam Fabio-R49496 <[email protected]>; Lucas Stach <[email protected]>
Subject: Re: [PATCH v4 5/6] PCI: layerscape: add PCIe support for LS1043a and LS2080a
[+cc Fabio, Lucas]
Hi Minghuan,
Quoted 46 lines · Minghuan Lian
Both LS1043a and LS2080a are based on ARMv8 64-bit architecture and have similar PCIe implementation. LUT is added to controller. The patch removes the necessary fields from struct ls_pcie.
Signed-off-by Minghuan Lian <[email protected]>
---
This patch is based on v4.3-rc4 and [PATCH v10 3/6]
PCI: designware: Add ARM64 support.
change log
v4:
1. split to 6 patches.
2. use ARCH_LAYERSCAPE instead of ARM64
v3:
1. Use 8 or 16 bit access function to simplify code 2. Add ls_add_pcie_port in accordance with other DesignWare-based drivers
v2:
1. Rename ls2085a to ls2080a
2. Add ls_pcie_msi_host_init()
 drivers/pci/host/Kconfig          |  2 +-
 drivers/pci/host/pci-layerscape.c | 72
+++++++++++++++++++++++++++++++++++----
 2 files changed, 67 insertions(+), 7 deletions(-)
drivers/pci/host/Kconfig index renamed from drivers/pci/host/Kconfig +0 -0
ae873be..8eb09ea 100644
drivers/pci/host/Kconfig +1 -1
@@ -105,7 +105,7 @@ config PCI_XGENE_MSI
 
 config PCI_LAYERSCAPE
 	bool "Freescale Layerscape PCIe controller"
-	depends on OF && ARM
+	depends on OF && (ARM || ARCH_LAYERSCAPE)
 	select PCIE_DW
 	select MFD_SYSCON
 	help
drivers/pci/host/pci-layerscape.c +0 -0
index 891e504..c53692a 100644
drivers/pci/host/pci-layerscape.c line 31
Quoted 1 line
drivers/pci/host/pci-layerscape.c +0 -0
@@ -31,23 +31,26 @@
 #define LTSSM_STATE_MASK	0x3f
BTW, not related to *this* patch, but does LTSSM_STATE_MASK really need to be 0x3f (6 bits), or could it be 0x1f (5 bits)?
I'd like to include Layerscape in the LTSSM_STATE_MASK cleanup done by Fabio:
https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/host
-designware&id=4788fe6ebf4594c9a95b620cbff05147c8504823
I don't have specs for any of these devices, so I don't know if this is really something that can vary between the different DesignWare-based devices, or if they all should use a mask of 0x1f.
Bjorn

Minghuan Lian

author

2015-10-16 07:18 UTC #14 lore raw

PATCH v4 6/6 PCI: layerscape: add ls_pcie_msi_host_init

In reply to Minghuan Lian

Layerscape PCIe has its own MSI implementation. The patch registers ls_pcie_msi_host_init() to avoid using Designware's MSI.

Signed-off-by Minghuan Lian <[email protected]>

---
Change log
v4: split from [PATCH v3] PCI: layerscape: Add PCIe support for LS1043a and LS2080a

 drivers/pci/host/pci-layerscape.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

drivers/pci/host/pci-layerscape.c +17 -0

index c53692a..8fac6c8 100644
@@ -150,14 +150,31 @@ static void ls_pcie_host_init(struct pcie_port *pp)
 	iowrite32(0, pcie->dbi + PCIE_DBI_RO_WR_EN);
 }
 
+static int ls_pcie_msi_host_init(struct pcie_port *pp,
+				 struct msi_controller *chip)
+{
+	struct device_node *msi_node;
+	struct device_node *np = pp->dev->of_node;
+
+	msi_node = of_parse_phandle(np, "msi-parent", 0);
+	if (!msi_node) {
+		dev_err(pp->dev, "failed to find msi-parent\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static struct pcie_host_ops ls1021_pcie_host_ops = {
 	.link_up = ls1021_pcie_link_up,
 	.host_init = ls1021_pcie_host_init,
+	.msi_host_init = ls_pcie_msi_host_init,
 };
 
 static struct pcie_host_ops ls_pcie_host_ops = {
 	.link_up = ls_pcie_link_up,
 	.host_init = ls_pcie_host_init,
+	.msi_host_init = ls_pcie_msi_host_init,
 };
 
 static struct ls_pcie_drvdata ls1021_drvdata = {

Signature · 1 line

1.9.1

Bjorn Helgaas

2015-10-21 21:34 UTC #15 lore raw

PATCH v4 6/6 PCI: layerscape: add ls_pcie_msi_host_init

Hi Minghuan,

Quoted 22 lines · Minghuan Lian

Layerscape PCIe has its own MSI implementation. The patch registers ls_pcie_msi_host_init() to avoid using Designware's MSI.
Signed-off-by Minghuan Lian <[email protected]>
---
Change log
v4: split from [PATCH v3] PCI: layerscape: Add PCIe support for LS1043a and LS2080a
 drivers/pci/host/pci-layerscape.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
drivers/pci/host/pci-layerscape.c +3 -0
index c53692a..8fac6c8 100644
@@ -150,14 +150,31 @@ static void ls_pcie_host_init(struct pcie_port *pp)
 	iowrite32(0, pcie->dbi + PCIE_DBI_RO_WR_EN);
 }
 
+static int ls_pcie_msi_host_init(struct pcie_port *pp,
+				 struct msi_controller *chip)
+{

drivers/pci/host/pci-layerscape.c static void ls_pcie_host_init(struct pcie_port *pp) · line 165

Quoted 10 lines

drivers/pci/host/pci-layerscape.c +10 -0

+	struct device_node *msi_node;
+	struct device_node *np = pp->dev->of_node;
+
+	msi_node = of_parse_phandle(np, "msi-parent", 0);
+	if (!msi_node) {
+		dev_err(pp->dev, "failed to find msi-parent\n");
+		return -EINVAL;
+	}
+
+	return 0;

I don't see how this can be right. I think it's OK if you want to enforce the presence of "msi-parent", but the other implementations of .msi_host_init (ks_dw_pcie_msi_host_init() and the default implementation in dw_pcie_host_init()) both set pp->irq_domain and call irq_create_mapping().

You don't do either of those, so I don't see how MSIs can work, because I assume the generic DesignWare code will depend on pp->irq_domain. If you're planning to add more Layerscape-specific MSI support later, I think you should wait and include this patch with that work.

Quoted 22 lines

drivers/pci/host/pci-layerscape.c +4 -0

+}
+
 static struct pcie_host_ops ls1021_pcie_host_ops = {
 	.link_up = ls1021_pcie_link_up,
 	.host_init = ls1021_pcie_host_init,
+	.msi_host_init = ls_pcie_msi_host_init,
 };
 
 static struct pcie_host_ops ls_pcie_host_ops = {
 	.link_up = ls_pcie_link_up,
 	.host_init = ls_pcie_host_init,
+	.msi_host_init = ls_pcie_msi_host_init,
 };
 
 static struct ls_pcie_drvdata ls1021_drvdata = {

Signature · 6 lines

1.9.1

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

Lian M.H.

author

2015-10-22 03:03 UTC #16 lore raw

PATCH v4 6/6 PCI: layerscape: add ls_pcie_msi_host_init

Hi Bjorn,

I thank you very much to help review and help revise my patches.

Regarding MSI, both LS1021a and LS1043a use SCFG to implement it. I have submitted patch:
https://patchwork.kernel.org/patch/7411131/
https://patchwork.kernel.org/patch/7411141/

While ls2085a use ITS for it, we just re-use the ITS driver.

I notice some platform MSI driver files were placed in pci/host folder not irqchip. If it is ok, I would like to change driver folder and re-submitted the MSI patch.

Thanks,
Minghaun

Quoted 81 lines

-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas at kernel.org]
Sent: Thursday, October 22, 2015 5:34 AM
To: Lian Minghuan-B31939 <[email protected]>
Cc: linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Zang Roy-R61911 <[email protected]>; Hu Mingkai-B21284 <[email protected]>; Yoder Stuart-B08248 <[email protected]>; Li Yang-Leo-R58472 <[email protected]>; Arnd Bergmann <[email protected]>; Bjorn Helgaas <[email protected]>; Jingoo Han <[email protected]>; Zhou Wang <[email protected]>
Subject: Re: [PATCH v4 6/6] PCI: layerscape: add ls_pcie_msi_host_init
Hi Minghuan,
Quoted 23 lines · Minghuan Lian
Layerscape PCIe has its own MSI implementation. The patch registers ls_pcie_msi_host_init() to avoid using Designware's MSI.
Signed-off-by Minghuan Lian <[email protected]>
---
Change log
v4: split from [PATCH v3] PCI: layerscape: Add PCIe support for LS1043a and LS2080a
 drivers/pci/host/pci-layerscape.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
drivers/pci/host/pci-layerscape.c +3 -0
index c53692a..8fac6c8 100644
@@ -150,14 +150,31 @@ static void ls_pcie_host_init(struct pcie_port *pp)
 	iowrite32(0, pcie->dbi + PCIE_DBI_RO_WR_EN);  }
 
+static int ls_pcie_msi_host_init(struct pcie_port *pp,
+				 struct msi_controller *chip)
+{
drivers/pci/host/pci-layerscape.c static void ls_pcie_host_init(struct pcie_port *pp) · line 164
Quoted 10 lines
drivers/pci/host/pci-layerscape.c +10 -0
+	struct device_node *msi_node;
+	struct device_node *np = pp->dev->of_node;
+
+	msi_node = of_parse_phandle(np, "msi-parent", 0);
+	if (!msi_node) {
+		dev_err(pp->dev, "failed to find msi-parent\n");
+		return -EINVAL;
+	}
+
+	return 0;
I don't see how this can be right. I think it's OK if you want to enforce the presence of "msi-parent", but the other implementations of .msi_host_init (ks_dw_pcie_msi_host_init() and the default implementation in dw_pcie_host_init()) both set pp->irq_domain and call irq_create_mapping().
You don't do either of those, so I don't see how MSIs can work, because I assume the generic DesignWare code will depend on pp->irq_domain. If you're planning to add more Layerscape-specific MSI support later, I think you should wait and include this patch with that work.
Quoted 22 lines
drivers/pci/host/pci-layerscape.c +4 -1
+}
+
 static struct pcie_host_ops ls1021_pcie_host_ops = {
 	.link_up = ls1021_pcie_link_up,
 	.host_init = ls1021_pcie_host_init,
+	.msi_host_init = ls_pcie_msi_host_init,
 };
 
 static struct pcie_host_ops ls_pcie_host_ops = {
 	.link_up = ls_pcie_link_up,
 	.host_init = ls_pcie_host_init,
+	.msi_host_init = ls_pcie_msi_host_init,
 };
 
 static struct ls_pcie_drvdata ls1021_drvdata = {
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas

2015-10-22 16:21 UTC #17 lore raw

PATCH v4 6/6 PCI: layerscape: add ls_pcie_msi_host_init

In reply to Bjorn Helgaas

[+cc Thomas for MSI driver file placement question + PCI MSI driver structure]

Hi Minghuan,

Your reply was base64-encoded and thus rejected by the vger mailing lists. I mentioned this before
(http://lkml.kernel.org/r/20151011191027.GA29221 at localhost). You might
want to fix your mail strategy, because it's really hard to carry on a conversation if nobody can hear your side :)

But I'll include your response here again by hand.

Quoted 52 lines · Minghuan

Quoted 42 lines · Bjorn Helgaas
Hi Minghuan,
Quoted 22 lines · Minghuan Lian
Layerscape PCIe has its own MSI implementation. The patch registers ls_pcie_msi_host_init() to avoid using Designware's MSI.
Signed-off-by Minghuan Lian <[email protected]>
---
Change log
v4: split from [PATCH v3] PCI: layerscape: Add PCIe support for LS1043a and LS2080a
 drivers/pci/host/pci-layerscape.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
drivers/pci/host/pci-layerscape.c +3 -0
index c53692a..8fac6c8 100644
@@ -150,14 +150,31 @@ static void ls_pcie_host_init(struct pcie_port *pp)
 	iowrite32(0, pcie->dbi + PCIE_DBI_RO_WR_EN);
 }
 
+static int ls_pcie_msi_host_init(struct pcie_port *pp,
+				 struct msi_controller *chip)
+{
drivers/pci/host/pci-layerscape.c static void ls_pcie_host_init(struct pcie_port *pp) · line 165
Quoted 10 lines
drivers/pci/host/pci-layerscape.c +10 -0
+	struct device_node *msi_node;
+	struct device_node *np = pp->dev->of_node;
+
+	msi_node = of_parse_phandle(np, "msi-parent", 0);
+	if (!msi_node) {
+		dev_err(pp->dev, "failed to find msi-parent\n");
+		return -EINVAL;
+	}
+
+	return 0;
I don't see how this can be right. I think it's OK if you want to enforce the presence of "msi-parent", but the other implementations of .msi_host_init (ks_dw_pcie_msi_host_init() and the default implementation in dw_pcie_host_init()) both set pp->irq_domain and call irq_create_mapping().
Quoted 4 lines
You don't do either of those, so I don't see how MSIs can work, because I assume the generic DesignWare code will depend on pp->irq_domain. If you're planning to add more Layerscape-specific MSI support later, I think you should wait and include this patch with that work.
Regarding MSI, both LS1021a and LS1043a use SCFG to implement it. I have submitted patch:
https://patchwork.kernel.org/patch/7411131/
https://patchwork.kernel.org/patch/7411141/

Quoted 5 lines

While ls2085a use ITS for it, we just re-use the ITS driver.
I notice some platform MSI driver files were placed in pci/host folder not irqchip. If it is ok, I would like to change driver folder and re-submitted the MSI patch.

I suppose you're referring to drivers/pci/host/pci-xgene-msi.c. That file doesn't really have much PCI stuff in it. It does call pci_msi_create_irq_domain(), but that's really the only PCI interface or data structure it uses. So I don't know if drivers/pci/host or drivers/irqchip is the best place for it and for your irq-ls-scfg-msi.c.

The connection between pci-xgene-msi.c and pci-xgene.c is not very clear to me, and that's sort of what I'm complaining about here. You're overriding a default MSI initialization method. Usually that means you do the same thing as the default method, but in a different way. You aren't doing the same thing at all, which makes the code hard to review.

Maybe a comment about how the MSI controller gets connected to devices below this host bridge would be enough.

Bjorn

Lian M.H.

author

2015-10-23 03:39 UTC #18 lore raw

PATCH v4 6/6 PCI: layerscape: add ls_pcie_msi_host_init

Hi Bjorn,

I am sorry, I just change to outlook as email client instead of thunderbird.
And I changed email type to plain text but forgot to check code type. Now, I have updated the options and hope it is ok.

Quoted 94 lines

-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas at kernel.org]
Sent: Friday, October 23, 2015 12:22 AM
To: Lian Minghuan-B31939 <[email protected]>
Cc: linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Zang Roy-R61911 <[email protected]>; Hu Mingkai-B21284 <[email protected]>; Yoder Stuart-B08248 <[email protected]>; Li Yang-Leo-R58472 <[email protected]>; Arnd Bergmann <[email protected]>; Bjorn Helgaas <[email protected]>; Jingoo Han <[email protected]>; Zhou Wang <[email protected]>; Thomas Gleixner <[email protected]>
Subject: Re: [PATCH v4 6/6] PCI: layerscape: add ls_pcie_msi_host_init
[+cc Thomas for MSI driver file placement question + PCI MSI driver structure]
Hi Minghuan,
Your reply was base64-encoded and thus rejected by the vger mailing lists. I mentioned this before
(http://lkml.kernel.org/r/20151011191027.GA29221 at localhost). You might
want to fix your mail strategy, because it's really hard to carry on a conversation if nobody can hear your side :)
But I'll include your response here again by hand.
Quoted 17 lines · Minghuan
Quoted 16 lines · Bjorn Helgaas
Hi Minghuan,
Quoted 13 lines · Minghuan Lian
Layerscape PCIe has its own MSI implementation. The patch registers
ls_pcie_msi_host_init() to avoid using Designware's MSI.
Signed-off-by Minghuan Lian <[email protected]>
---
Change log
v4: split from [PATCH v3] PCI: layerscape: Add PCIe support for LS1043a and LS2080a
 drivers/pci/host/pci-layerscape.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
Quoted 6 lines
Quoted 6 lines
drivers/pci/host/pci-layerscape.c static void ls_pcie_host_init(struct pcie_port
Quoted 6 lines
drivers/pci/host/pci-layerscape.c +0 -0
index c53692a..8fac6c8 100644
@@ -150,14 +150,31 @@ static void ls_pcie_host_init(struct pcie_port
*pp)
Quoted 21 lines
Quoted 21 lines
Quoted 4 lines
drivers/pci/host/pci-layerscape.c +2 -0
 	iowrite32(0, pcie->dbi + PCIE_DBI_RO_WR_EN);  }
 
+static int ls_pcie_msi_host_init(struct pcie_port *pp,
+				 struct msi_controller *chip) {
drivers/pci/host/pci-layerscape.c static void ls_pcie_host_init(struct pcie_port *pp) · line 165
Quoted 10 lines
drivers/pci/host/pci-layerscape.c +10 -0
+	struct device_node *msi_node;
+	struct device_node *np = pp->dev->of_node;
+
+	msi_node = of_parse_phandle(np, "msi-parent", 0);
+	if (!msi_node) {
+		dev_err(pp->dev, "failed to find msi-parent\n");
+		return -EINVAL;
+	}
+
+	return 0;
I don't see how this can be right. I think it's OK if you want to enforce the presence of "msi-parent", but the other implementations of .msi_host_init (ks_dw_pcie_msi_host_init() and the default implementation in dw_pcie_host_init()) both set pp->irq_domain and call irq_create_mapping().
Quoted 4 lines
Quoted 4 lines
You don't do either of those, so I don't see how MSIs can work, because I assume the generic DesignWare code will depend on pp->irq_domain. If you're planning to add more Layerscape-specific MSI support later, I think you should wait and include this patch with that
work.
Quoted 1 line
Regarding MSI, both LS1021a and LS1043a use SCFG to implement it. I have
submitted patch:
Quoted 8 lines
https://patchwork.kernel.org/patch/7411131/
https://patchwork.kernel.org/patch/7411141/
While ls2085a use ITS for it, we just re-use the ITS driver.
I notice some platform MSI driver files were placed in pci/host folder not irqchip. If it is ok, I would like to change driver folder and re-submitted the MSI patch.
I suppose you're referring to drivers/pci/host/pci-xgene-msi.c.
That file doesn't really have much PCI stuff in it. It does call pci_msi_create_irq_domain(), but that's really the only PCI interface or data structure it uses. So I don't know if drivers/pci/host or drivers/irqchip is the best place for it and for your irq-ls-scfg-msi.c.

Quoted 9 lines

The connection between pci-xgene-msi.c and pci-xgene.c is not very clear to me, and that's sort of what I'm complaining about here.
You're overriding a default MSI initialization method. Usually that means you do the same thing as the default method, but in a different way. You aren't doing the same thing at all, which makes the code hard to review.
Maybe a comment about how the MSI controller gets connected to devices below this host bridge would be enough.

[Lian Minghuan-B31939]
1. We need to define callback function msi_host_init() to avoid the running desginware MSI related code
Because our PCIe controller do not enable this feature.
2. we do not need to do anything such as bus->msi= ls_scfg_msi
Because with the latest code, when PCIe controller device is created of_msi_configure() will be called and MSI domain pointed by 'msi-parent' will be bound to the device. pci_msi_get_domain(struct pci_dev *dev) -> dev_get_msi_domain(&dev->dev) will return this MSI domain.
3. I will add a comment in the next version.

Thanks,
Minghuan

Quoted 1 line

Bjorn

Bjorn Helgaas

2015-11-02 21:06 UTC #19 lore raw

PATCH v4 6/6 PCI: layerscape: add ls_pcie_msi_host_init

In reply to Bjorn Helgaas

Your reply was *still* base64-encoded and thus rejected by the vger mailing lists.

Quoted 81 lines · Minghuan

Quoted 73 lines · Bjorn Helgaas
Quoted 52 lines · Minghuan
Quoted 42 lines · Bjorn Helgaas
Hi Minghuan,
Quoted 22 lines · Minghuan Lian
Layerscape PCIe has its own MSI implementation. The patch registers ls_pcie_msi_host_init() to avoid using Designware's MSI.
Signed-off-by Minghuan Lian <[email protected]>
---
Change log
v4: split from [PATCH v3] PCI: layerscape: Add PCIe support for LS1043a and LS2080a
 drivers/pci/host/pci-layerscape.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
drivers/pci/host/pci-layerscape.c +3 -0
index c53692a..8fac6c8 100644
@@ -150,14 +150,31 @@ static void ls_pcie_host_init(struct pcie_port *pp)
 	iowrite32(0, pcie->dbi + PCIE_DBI_RO_WR_EN);
 }
 
+static int ls_pcie_msi_host_init(struct pcie_port *pp,
+				 struct msi_controller *chip)
+{
drivers/pci/host/pci-layerscape.c static void ls_pcie_host_init(struct pcie_port *pp) · line 165
Quoted 10 lines
drivers/pci/host/pci-layerscape.c +10 -0
+	struct device_node *msi_node;
+	struct device_node *np = pp->dev->of_node;
+
+	msi_node = of_parse_phandle(np, "msi-parent", 0);
+	if (!msi_node) {
+		dev_err(pp->dev, "failed to find msi-parent\n");
+		return -EINVAL;
+	}
+
+	return 0;
I don't see how this can be right. I think it's OK if you want to enforce the presence of "msi-parent", but the other implementations of .msi_host_init (ks_dw_pcie_msi_host_init() and the default implementation in dw_pcie_host_init()) both set pp->irq_domain and call irq_create_mapping().
Quoted 4 lines
You don't do either of those, so I don't see how MSIs can work, because I assume the generic DesignWare code will depend on pp->irq_domain. If you're planning to add more Layerscape-specific MSI support later, I think you should wait and include this patch with that work.
Regarding MSI, both LS1021a and LS1043a use SCFG to implement it. I have submitted patch:
https://patchwork.kernel.org/patch/7411131/
https://patchwork.kernel.org/patch/7411141/
Quoted 5 lines
While ls2085a use ITS for it, we just re-use the ITS driver.
I notice some platform MSI driver files were placed in pci/host folder not irqchip. If it is ok, I would like to change driver folder and re-submitted the MSI patch.
I suppose you're referring to drivers/pci/host/pci-xgene-msi.c. That file doesn't really have much PCI stuff in it. It does call pci_msi_create_irq_domain(), but that's really the only PCI interface or data structure it uses. So I don't know if drivers/pci/host or drivers/irqchip is the best place for it and for your irq-ls-scfg-msi.c.
The connection between pci-xgene-msi.c and pci-xgene.c is not very clear to me, and that's sort of what I'm complaining about here. You're overriding a default MSI initialization method. Usually that means you do the same thing as the default method, but in a different way. You aren't doing the same thing at all, which makes the code hard to review.
Quoted 2 lines
Maybe a comment about how the MSI controller gets connected to devices below this host bridge would be enough.
1. We need to define callback function msi_host_init() to avoid the running desginware MSI related code Because our PCIe controller do not enable this feature.

Quoted 8 lines

2. we do not need to do anything such as bus->msi= ls_scfg_msi Because with the latest code, when PCIe controller device is created of_msi_configure() will be called and MSI domain pointed by 'msi-parent' will be bound to the device. pci_msi_get_domain(struct pci_dev *dev) -> dev_get_msi_domain(&dev->dev) will return this MSI domain.
3. I will add a comment in the next version.

Don't bother, I added a comment and applied the patch like this:

commit 90cbdf8412d206f65ece3dcc53230e673da569c6

Author: Minghuan Lian <[email protected]>
Date:   Fri Oct 16 15:19:20 2015 +0800

PCI: layerscape: Add ls_pcie_msi_host_init()

Layerscape PCIe has its own MSI implementation.

[bhelgaas: add comment]

Signed-off-by Minghuan Lian <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

drivers/pci/host/pci-layerscape.c +23 -0

index 6a9fa8a..1677890 100644
@@ -150,14 +150,37 @@ static void ls_pcie_host_init(struct pcie_port *pp)
 	iowrite32(0, pcie->dbi + PCIE_DBI_RO_WR_EN);
 }
 
+static int ls_pcie_msi_host_init(struct pcie_port *pp,
+				 struct msi_controller *chip)
+{
+	struct device_node *msi_node;
+	struct device_node *np = pp->dev->of_node;
+
+	/*
+	 * The MSI domain is set by the generic of_msi_configure().  This
+	 * .msi_host_init() function keeps us from doing the default MSI
+	 * domain setup in dw_pcie_host_init() and also enforces the
+	 * requirement that "msi-parent" exists.
+	 */
+	msi_node = of_parse_phandle(np, "msi-parent", 0);
+	if (!msi_node) {
+		dev_err(pp->dev, "failed to find msi-parent\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static struct pcie_host_ops ls1021_pcie_host_ops = {
 	.link_up = ls1021_pcie_link_up,
 	.host_init = ls1021_pcie_host_init,
+	.msi_host_init = ls_pcie_msi_host_init,
 };
 
 static struct pcie_host_ops ls_pcie_host_ops = {
 	.link_up = ls_pcie_link_up,
 	.host_init = ls_pcie_host_init,
+	.msi_host_init = ls_pcie_msi_host_init,
 };
 
 static struct ls_pcie_drvdata ls1021_drvdata = {

Lian M.H.

author

2015-11-03 06:08 UTC #20 lore raw

PATCH v4 6/6 PCI: layerscape: add ls_pcie_msi_host_init

Hi Bjorn,

I am so sorry about text code setting. Now, I carefully checked the mail configuration again, hope it is ok. I greatly appreciate your review and comments.

Thanks,
Minghuan

Quoted 169 lines

-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas at kernel.org]
Sent: Tuesday, November 03, 2015 5:07 AM
To: Lian Minghuan-B31939 <[email protected]>
Cc: linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Zang Roy-R61911 <[email protected]>; Hu Mingkai-B21284 <[email protected]>; Yoder Stuart-B08248 <[email protected]>; Li Yang-Leo-R58472 <[email protected]>; Arnd Bergmann <[email protected]>; Bjorn Helgaas <[email protected]>; Jingoo Han <[email protected]>; Zhou Wang <[email protected]>; Thomas Gleixner <[email protected]>
Subject: Re: [PATCH v4 6/6] PCI: layerscape: add ls_pcie_msi_host_init
Your reply was *still* base64-encoded and thus rejected by the vger mailing lists.
Quoted 19 lines · Minghuan
Quoted 18 lines · Bjorn Helgaas
Quoted 17 lines · Minghuan
Quoted 16 lines · Bjorn Helgaas
Hi Minghuan,
Quoted 13 lines · Minghuan Lian
Layerscape PCIe has its own MSI implementation. The patch registers
ls_pcie_msi_host_init() to avoid using Designware's MSI.
Signed-off-by Minghuan Lian <[email protected]>
---
Change log
v4: split from [PATCH v3] PCI: layerscape: Add PCIe support for LS1043a and LS2080a
 drivers/pci/host/pci-layerscape.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/pci/host/pci-layerscape.c static void ls_pcie_host_init(struct
Quoted 6 lines
drivers/pci/host/pci-layerscape.c +0 -0
index c53692a..8fac6c8 100644
@@ -150,14 +150,31 @@ static void ls_pcie_host_init(struct
pcie_port *pp)
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Quoted 4 lines
drivers/pci/host/pci-layerscape.c +2 -0
 	iowrite32(0, pcie->dbi + PCIE_DBI_RO_WR_EN);  }
 
+static int ls_pcie_msi_host_init(struct pcie_port *pp,
+				 struct msi_controller *chip) {
drivers/pci/host/pci-layerscape.c static void ls_pcie_host_init(struct pcie_port *pp) · line 171
Quoted 10 lines
drivers/pci/host/pci-layerscape.c +10 -0
+	struct device_node *msi_node;
+	struct device_node *np = pp->dev->of_node;
+
+	msi_node = of_parse_phandle(np, "msi-parent", 0);
+	if (!msi_node) {
+		dev_err(pp->dev, "failed to find msi-parent\n");
+		return -EINVAL;
+	}
+
+	return 0;
I don't see how this can be right. I think it's OK if you want to enforce the presence of "msi-parent", but the other implementations of .msi_host_init (ks_dw_pcie_msi_host_init() and the default implementation in dw_pcie_host_init()) both set pp->irq_domain and call irq_create_mapping().
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
You don't do either of those, so I don't see how MSIs can work, because I assume the generic DesignWare code will depend on pp->irq_domain. If you're planning to add more Layerscape-specific MSI support later, I think you should wait and
include this patch with that work.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Regarding MSI, both LS1021a and LS1043a use SCFG to implement it. I
have submitted patch:
Quoted 30 lines
Quoted 23 lines
Quoted 8 lines
https://patchwork.kernel.org/patch/7411131/
https://patchwork.kernel.org/patch/7411141/
While ls2085a use ITS for it, we just re-use the ITS driver.
I notice some platform MSI driver files were placed in pci/host folder not irqchip. If it is ok, I would like to change driver folder and re-submitted the MSI patch.
I suppose you're referring to drivers/pci/host/pci-xgene-msi.c. That file doesn't really have much PCI stuff in it. It does call pci_msi_create_irq_domain(), but that's really the only PCI interface or data structure it uses. So I don't know if drivers/pci/host or drivers/irqchip is the best place for it and for your irq-ls-scfg-msi.c.
The connection between pci-xgene-msi.c and pci-xgene.c is not very clear to me, and that's sort of what I'm complaining about here. You're overriding a default MSI initialization method. Usually that means you do the same thing as the default method, but in a different way. You aren't doing the same thing at all, which makes the code hard to review.
Quoted 2 lines
Maybe a comment about how the MSI controller gets connected to devices below this host bridge would be enough.
1. We need to define callback function msi_host_init() to avoid the running desginware MSI related code Because our PCIe controller do not enable this feature.
Quoted 8 lines
2. we do not need to do anything such as bus->msi= ls_scfg_msi Because with the latest code, when PCIe controller device is created of_msi_configure() will be called and MSI domain pointed by 'msi-parent' will be bound to the device. pci_msi_get_domain(struct pci_dev *dev) -> dev_get_msi_domain(&dev->dev) will return this MSI domain.
3. I will add a comment in the next version.
Don't bother, I added a comment and applied the patch like this:
commit 90cbdf8412d206f65ece3dcc53230e673da569c6
Author: Minghuan Lian <[email protected]>
Date:   Fri Oct 16 15:19:20 2015 +0800
PCI: layerscape: Add ls_pcie_msi_host_init()
Layerscape PCIe has its own MSI implementation.
Register ls_pcie_msi_host_init() to avoid using DesignWare's MSI.
[bhelgaas: add comment]
Signed-off-by Minghuan Lian <[email protected]>
Signed-off-by Bjorn Helgaas <[email protected]>
drivers/pci/host/pci-layerscape.c +23 -0
index 6a9fa8a..1677890 100644
@@ -150,14 +150,37 @@ static void ls_pcie_host_init(struct pcie_port *pp)
 	iowrite32(0, pcie->dbi + PCIE_DBI_RO_WR_EN);  }
 
+static int ls_pcie_msi_host_init(struct pcie_port *pp,
+				 struct msi_controller *chip)
+{
+	struct device_node *msi_node;
+	struct device_node *np = pp->dev->of_node;
+
+	/*
+	 * The MSI domain is set by the generic of_msi_configure().  This
+	 * .msi_host_init() function keeps us from doing the default MSI
+	 * domain setup in dw_pcie_host_init() and also enforces the
+	 * requirement that "msi-parent" exists.
+	 */
+	msi_node = of_parse_phandle(np, "msi-parent", 0);
+	if (!msi_node) {
+		dev_err(pp->dev, "failed to find msi-parent\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static struct pcie_host_ops ls1021_pcie_host_ops = {
 	.link_up = ls1021_pcie_link_up,
 	.host_init = ls1021_pcie_host_init,
+	.msi_host_init = ls_pcie_msi_host_init,
 };
 
 static struct pcie_host_ops ls_pcie_host_ops = {
 	.link_up = ls_pcie_link_up,
 	.host_init = ls_pcie_host_init,
+	.msi_host_init = ls_pcie_msi_host_init,
 };
 
 static struct ls_pcie_drvdata ls1021_drvdata = {

Bjorn Helgaas

Applied

2015-10-21 21:40 UTC #21 lore raw

PATCH v4 1/6 PCI: layerscape: remove ls_pcie_establish_link()

In reply to Minghuan Lian

Quoted 6 lines · Minghuan Lian

ls_pcie_establish_link() does not do any real operation, except to wait for the linkup establishment. In fact, this is not necessary. Moreover, each PCIe controller not inserted device will increase the Linux startup time about 200ms.
Signed-off-by Minghuan Lian <[email protected]>

It'd be handy if there were a [0/6] cover letter where I could reply with this update about the whole series.

I applied patches 1-5 to pci/host-layerscape for v4.4, thanks!

I split patch 5 (adding LS1043a and LS2080a support) into one patch that removes the unused struct fields and a second patch that actually adds the LS1043a and LS2080a support.

I haven't applied patch 6 because it doesn't make sense to me (more details in the response to it). I'll be happy to apply it if you explain what I'm missing.

Bjorn

Quoted 54 lines

---
Change log
v4: split from [PATCH v3] PCI: layerscape: Add PCIe support for LS1043a and LS2080a

 drivers/pci/host/pci-layerscape.c | 16 ----------------
 1 file changed, 16 deletions(-)

drivers/pci/host/pci-layerscape.c +0 ~~-16~~

index b2328ea1..6dd44a0 100644
@@ -11,7 +11,6 @@
  */
 
 #include <linux/kernel.h>
-#include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of_pci.h>

@@ -62,27 +61,12 @@ static int ls_pcie_link_up(struct pcie_port *pp)
 	return 1;
 }
 
-static int ls_pcie_establish_link(struct pcie_port *pp)
-{
-	unsigned int retries;
-
-	for (retries = 0; retries < 200; retries++) {
-		if (dw_pcie_link_up(pp))
-			return 0;
-		usleep_range(100, 1000);
-	}
-
-	dev_err(pp->dev, "phy link never came up\n");
-	return -EINVAL;
-}
-
 static void ls_pcie_host_init(struct pcie_port *pp)
 {
 	struct ls_pcie *pcie = to_ls_pcie(pp);
 	u32 val;
 
 	dw_pcie_setup_rc(pp);
-	ls_pcie_establish_link(pp);
 
 	/*
 	 * LS1021A Workaround for internal TKT228622

Signature · 6 lines

1.9.1

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