PCI: layerscape: Add PCIe support for LS1043a and LS2080a

PATCH Superseded Minghuan Lian 5 participants 10 messages Sep 17, 2015 – Oct 13, 2015 PCI

lore

Share & tools

lore

Minghuan Lian

author

2015-09-17 09:13 UTC #0 lore raw

PATCH v2 1/1 PCI: layerscape: Add PCIe support for LS1043a and LS2080a

The patch adds PCIe support for LS1043a and LS2080a.

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

---
This patch is based on v4.3-rc1 and [PATCH v9 0/6]
PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05 patchset from Zhou Wang.

change log
v2:
1. rename ls2085a to ls2080a
2. Add ls_pcie_msi_host_init()

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

drivers/pci/host/Kconfig +1 -1

index ae873be..38fe8a8 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 || ARM64)
 	select PCIE_DW
 	select MFD_SYSCON
 	help

drivers/pci/host/pci-layerscape.c +156 ~~-71~~

300 lines not on this page · raw message

Signature · 1 line

1.9.1

Bjorn Helgaas

2015-10-07 17:57 UTC #1 lore raw

PATCH v2 1/1 PCI: layerscape: Add PCIe support for LS1043a and LS2080a

Hi Minghuan,

Quoted 22 lines · Minghuan Lian

The patch adds PCIe support for LS1043a and LS2080a.
Signed-off-by Minghuan Lian <[email protected]>
---
This patch is based on v4.3-rc1 and [PATCH v9 0/6]
PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05 patchset from Zhou Wang.
change log
v2:
1. rename ls2085a to ls2080a
2. Add ls_pcie_msi_host_init()
 drivers/pci/host/Kconfig          |   2 +-
 drivers/pci/host/pci-layerscape.c | 227 ++++++++++++++++++++++++++------------
 2 files changed, 157 insertions(+), 72 deletions(-)
drivers/pci/host/Kconfig +0 -0
index ae873be..38fe8a8 100644

drivers/pci/host/Kconfig config PCI_XGENE_MSI · line 108

Quoted 5 lines

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 || ARM64)

It seems like there are a couple things going on here, and I wonder if you can split them out into separate patches.

1) Making this work on ARM64 as well as on ARM. This may be of interest for other DesignWare-based drivers, so if you split this out, maybe it would be a useful template for converting the other drivers, too.

2) Adding LS1043a and LS2080a. Obviously, this is only of interest to this driver, but maybe it could be separated out into a separate patch?

Quoted 54 lines

drivers/pci/host/Kconfig +0 -0

 	select PCIE_DW
 	select MFD_SYSCON
 	help

drivers/pci/host/pci-layerscape.c +18 ~~-10~~

index b2328ea1..cdb8737 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>

@@ -32,27 +31,68 @@
 #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 */
+
+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;
-	void __iomem *dbi;
-	struct regmap *scfg;
 	struct pcie_port pp;
+	const struct ls_pcie_drvdata *drvdata;
+	void __iomem *regs;
+	void __iomem *lut;
+	struct regmap *scfg;
 	int index;
-	int msi_irq;
 };
 
 #define to_ls_pcie(x)	container_of(x, struct ls_pcie, pp)
 
-static int ls_pcie_link_up(struct pcie_port *pp)
+static bool ls_pcie_is_bridge(struct ls_pcie *pcie)
+{

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

Quoted 10 lines

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

+	u32 header_type;
+
+	header_type = ioread32(pcie->regs + (PCI_HEADER_TYPE & ~0x3));
+	header_type = (header_type >> (8 * (PCI_HEADER_TYPE & 0x3))) & 0x7f;
+
+	return header_type == PCI_HEADER_TYPE_BRIDGE;
+}
+
+/* Clean multi-function bit */
+static void ls_pcie_clean_multifunction(struct ls_pcie *pcie)

This *clears* the multi-function bit. It's not really clear what it means to "clean" a bit :)

Why do you read/write 32 bits at a time? Is there a problem with 8- or 16-bit accesses? If 8- or 16-bit accesses don't work, then we have to worry about the RW1C problem for some registers.

Quoted 107 lines

drivers/pci/host/pci-layerscape.c +66 ~~-18~~

+{
+	u32 val;
+
+	val = ioread32(pcie->regs + (PCI_HEADER_TYPE & ~0x3));
+	val &= ~(1 << 23);
+	iowrite32(val, pcie->regs + (PCI_HEADER_TYPE & ~0x3));
+}
+
+/* Fix class value */
+static void ls_pcie_fix_class(struct ls_pcie *pcie)
+{
+	u32 val;
+
+	val = ioread32(pcie->regs + PCI_CLASS_REVISION);
+	val = (val & 0x0000ffff) | (PCI_CLASS_BRIDGE_PCI << 16);
+	iowrite32(val, pcie->regs + PCI_CLASS_REVISION);
+}
+
+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;

@@ -62,110 +102,155 @@ static int ls_pcie_link_up(struct pcie_port *pp)
 	return 1;
 }
 
-static int ls_pcie_establish_link(struct pcie_port *pp)
+static void ls1021_pcie_host_init(struct pcie_port *pp)
 {
-	unsigned int retries;
+	struct ls_pcie *pcie = to_ls_pcie(pp);
+	u32 val, index[2];
 
-	for (retries = 0; retries < 200; retries++) {
-		if (dw_pcie_link_up(pp))
-			return 0;
-		usleep_range(100, 1000);
+	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];
 
-	dev_err(pp->dev, "phy link never came up\n");
-	return -EINVAL;
+	/*
+	 * LS1021A Workaround for internal TKT228622
+	 * to fix the INTx hang issue
+	 */
+	val = ioread32(pcie->regs + PCIE_STRFMR1);
+	val &= 0xffff;
+	iowrite32(val, pcie->regs + 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);
-	u32 val;
 
-	dw_pcie_setup_rc(pp);
-	ls_pcie_establish_link(pp);
+	iowrite32(1, pcie->regs + PCIE_DBI_RO_WR_EN);
+	ls_pcie_fix_class(pcie);
+	ls_pcie_clean_multifunction(pcie);
+	iowrite32(0, pcie->regs + PCIE_DBI_RO_WR_EN);
+}
 
-	/*
-	 * LS1021A Workaround for internal TKT228622
-	 * to fix the INTx hang issue
-	 */
-	val = ioread32(pcie->dbi + PCIE_STRFMR1);
-	val &= 0xffff;
-	iowrite32(val, pcie->dbi + PCIE_STRFMR1);
 
+static int ls_pcie_msi_host_init(struct pcie_port *pp,

drivers/pci/host/pci-layerscape.c static int ls_pcie_link_up(struct pcie_port *pp) · line 170

Quoted 10 lines

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

+				 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;
+	}

This doesn't actually *do* anything except complain if "msi-parent" is missing. I'm not sure that's worth doing: if we need "msi-parent" somewhere, we should complain there if we don't find it. If we don't need it, why complain if it's missing?

Quoted 101 lines

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

+
+	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 int ls_add_pcie_port(struct ls_pcie *pcie)
-{
-	struct pcie_port *pp;
-	int ret;
+static struct ls_pcie_drvdata ls1021_drvdata = {
+	.ops = &ls1021_pcie_host_ops,
+};
 
-	pp = &pcie->pp;
-	pp->dev = pcie->dev;
-	pp->dbi_base = pcie->dbi;
-	pp->root_bus_nr = -1;
-	pp->ops = &ls_pcie_host_ops;
+static struct ls_pcie_drvdata ls1043_drvdata = {
+	.lut_offset = 0x10000,
+	.ltssm_shift = 24,
+	.ops = &ls_pcie_host_ops,
+};
 
-	ret = dw_pcie_host_init(pp);
-	if (ret) {
-		dev_err(pp->dev, "failed to initialize host\n");
-		return ret;
-	}
+static struct ls_pcie_drvdata ls2080_drvdata = {
+	.lut_offset = 0x80000,
+	.ltssm_shift = 0,
+	.ops = &ls_pcie_host_ops,
+};
 
-	return 0;
-}
+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);
 
 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];
+	struct pcie_port *pp;
+	struct resource *res;
 	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;
 
-	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)) {
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
+	pcie->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pcie->regs)) {
 		dev_err(&pdev->dev, "missing *regs* space\n");
-		return PTR_ERR(pcie->dbi);
+		return PTR_ERR(pcie->regs);
 	}
 
-	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);
-	}
+	pcie->drvdata = match->data;
+	pcie->lut = pcie->regs + pcie->drvdata->lut_offset;
+	pp = &pcie->pp;
+	pp->dev = &pdev->dev;
+	pp->dbi_base = pcie->regs;
+	pp->ops = pcie->drvdata->ops;
 
-	ret = of_property_read_u32_array(pdev->dev.of_node,

drivers/pci/host/pci-layerscape.c static int ls_pcie_link_up(struct pcie_port *pp) · line 243

Quoted 10 lines

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

-					 "fsl,pcie-scfg", index, 2);
-	if (ret)
-		return ret;
-	pcie->index = index[1];
+	if (!ls_pcie_is_bridge(pcie))
+		return -ENODEV;
 
-	ret = ls_add_pcie_port(pcie);
-	if (ret < 0)
+	ret = dw_pcie_host_init(pp);

We have several DesignWare-based drivers (dra7xx, exynos, imx6, ks, ls, spear13xx), and I'd like to keep their structure as similar as possible. For example, they all have basically this structure:

X_pcie_probe

    X_add_pcie_port
      dw_pcie_host_init             # pp->ops->host_init callback
        X_pcie_host_init
          X_pcie_establish_link

This patch removes ls_add_pcie_port() and ls_pcie_establish_link(), so we're diverging a bit. That makes it harder to see the similarities across these drivers, which I think is a loss.

Quoted 26 lines

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

+	if (ret) {
+		dev_err(&pdev->dev, "failed to initialize host\n");
 		return ret;
+	}
 
 	platform_set_drvdata(pdev, pcie);
 
 	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 · 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

Bjorn Helgaas

2015-10-11 19:10 UTC #2 lore raw

PATCH v2 1/1 PCI: layerscape: Add PCIe support for LS1043a and LS2080a

[+cc Thomas, Jason, Tanmay]

Hi Minghuan,

You responded to this message, but your response was rejected by the mailing list, I think because it was not plain text. See
http://vger.kernel.org/majordomo-info.html

I went ahead and reconstructed what your response would have looked like so I could continue the conversation. But please fix your email setup before responding again :)

Quoted 48 lines · Minghuan

Quoted 34 lines · Bjorn Helgaas
Hi Minghuan,
Quoted 22 lines · Minghuan Lian
The patch adds PCIe support for LS1043a and LS2080a.
Signed-off-by Minghuan Lian <[email protected]>
---
This patch is based on v4.3-rc1 and [PATCH v9 0/6]
PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05 patchset from Zhou Wang.
change log
v2:
1. rename ls2085a to ls2080a
2. Add ls_pcie_msi_host_init()
 drivers/pci/host/Kconfig          |   2 +-
 drivers/pci/host/pci-layerscape.c | 227 ++++++++++++++++++++++++++------------
 2 files changed, 157 insertions(+), 72 deletions(-)
drivers/pci/host/Kconfig +0 -0
index ae873be..38fe8a8 100644
drivers/pci/host/Kconfig config PCI_XGENE_MSI · line 108
Quoted 5 lines
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 || ARM64)
It seems like there are a couple things going on here, and I wonder if you can split them out into separate patches.
Quoted 8 lines
1) Making this work on ARM64 as well as on ARM. This may be of interest for other DesignWare-based drivers, so if you split this out, maybe it would be a useful template for converting the other drivers, too.
2) Adding LS1043a and LS2080a. Obviously, this is only of interest to this driver, but maybe it could be separated out into a separate patch?
The patch is based on v4.3-rc1 and [PATCH v9 0/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05 patchset from Zhou Wang which adds arm and arm64 support.

Quoted 6 lines

The original code with the Zhou Wang's patches can support arm64 as well. Because both LS1043a and LS2085 are armv8 (arm64) architecture, the patch updates Kconfig to add 'arm64'.
If splitting two patches, the first patch for arm64 support only includes one line changes of Kconfig. So, I think it is unnecessary.

It's OK if the first patch is very small. The point is that the patch does two orthogonal things, and those two things should be in separate commits. This makes bisection work better, and it reduces the amount of functionality removed if we have to revert something.

Quoted 17 lines

Quoted 8 lines
Quoted 8 lines
drivers/pci/host/Kconfig +8 -0
+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) {
Quoted 8 lines
drivers/pci/host/Kconfig
Quoted 3 lines
drivers/pci/host/Kconfig +3 -0
+		dev_err(pp->dev, "failed to find msi-parent\n");
+		return -EINVAL;
+	}
This doesn't actually *do* anything except complain if "msi-parent" is missing. I'm not sure that's worth doing: if we need "msi-parent" somewhere, we should complain there if we don't find it. If we don't need it, why complain if it's missing?

Quoted 6 lines

driver/of/irq.c void of_msi_configure(struct device *dev, struct device_node *np) will bind "msi-parent" to each device if there is "msi-parent" handler. The PCIe driver do not need to do anything. If we do not check "msi-parent" here, we will have no chance to check it. The common code of 'of' and 'pci' bus driver will not complain, because the msi controller may be found by other way.

Hmm. In mvebu_pcie_msi_enable() and xgene_pcie_msi_enable(), we
also look for "msi-parent". If that fails, mvebu continues silently and xgene complains (but only if CONFIG_PCI_MSI=y).

This seems like three unnecessarily different ways of doing the same thing.

I cc'd the maintainers of those drivers; maybe we can agree on a single strategy.

Quoted 21 lines

Quoted 12 lines
drivers/pci/host/Kconfig
Quoted 6 lines
static int __init ls_pcie_probe(struct platform_device *pdev) {
...
drivers/pci/host/Kconfig +1 -2
-	ret = ls_add_pcie_port(pcie);
-	if (ret < 0)
+	ret = dw_pcie_host_init(pp);
We have several DesignWare-based drivers (dra7xx, exynos, imx6, ks, ls, spear13xx), and I'd like to keep their structure as similar as possible. For example, they all have basically this structure:
X_pcie_probe
Quoted 8 lines
    X_add_pcie_port
      dw_pcie_host_init             # pp->ops->host_init callback
        X_pcie_host_init
          X_pcie_establish_link
This patch removes ls_add_pcie_port() and ls_pcie_establish_link(), so we're diverging a bit. That makes it harder to see the similarities across these drivers, which I think is a loss.

Quoted 5 lines

I will add X_add_pcie_port. But we do not need X_pcie_establish_link because we do not need change/reset phy to establish link, besides, the PCIe controller slot may be not inserted any PCIe device at all. If each controller waits some time for link, the start time will increase a lot. In some scenes, long start-up time is not acceptable.

If we wait for a timeout trying to establish a link when the slot is empty, it sounds like something's wrong. Can't we tell from presence detect whether a card is present, even without trying to bring up the link?

If we truly do not need a piece like X_pcie_establish_link(), I'm OK with entirely omitting it. What I really object to is when we have the same functionality two places with different names or structured two different ways.

Bjorn

Duc Dang

2015-10-12 01:47 UTC #3 lore raw

PATCH v2 1/1 PCI: layerscape: Add PCIe support for LS1043a and LS2080a

Quoted 97 lines · Bjorn Helgaas

[+cc Thomas, Jason, Tanmay]
Hi Minghuan,
You responded to this message, but your response was rejected by the mailing list, I think because it was not plain text. See
http://vger.kernel.org/majordomo-info.html
I went ahead and reconstructed what your response would have looked like so I could continue the conversation. But please fix your email setup before responding again :)
Quoted 48 lines · Minghuan
Quoted 34 lines · Bjorn Helgaas
Hi Minghuan,
Quoted 22 lines · Minghuan Lian
The patch adds PCIe support for LS1043a and LS2080a.
Signed-off-by Minghuan Lian <[email protected]>
---
This patch is based on v4.3-rc1 and [PATCH v9 0/6]
PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05 patchset from Zhou Wang.
change log
v2:
1. rename ls2085a to ls2080a
2. Add ls_pcie_msi_host_init()
 drivers/pci/host/Kconfig          |   2 +-
 drivers/pci/host/pci-layerscape.c | 227 ++++++++++++++++++++++++++------------
 2 files changed, 157 insertions(+), 72 deletions(-)
drivers/pci/host/Kconfig +0 -0
index ae873be..38fe8a8 100644
drivers/pci/host/Kconfig config PCI_XGENE_MSI · line 108
Quoted 5 lines
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 || ARM64)
It seems like there are a couple things going on here, and I wonder if you can split them out into separate patches.
Quoted 8 lines
1) Making this work on ARM64 as well as on ARM. This may be of interest for other DesignWare-based drivers, so if you split this out, maybe it would be a useful template for converting the other drivers, too.
2) Adding LS1043a and LS2080a. Obviously, this is only of interest to this driver, but maybe it could be separated out into a separate patch?
The patch is based on v4.3-rc1 and [PATCH v9 0/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05 patchset from Zhou Wang which adds arm and arm64 support.
Quoted 6 lines
The original code with the Zhou Wang's patches can support arm64 as well. Because both LS1043a and LS2085 are armv8 (arm64) architecture, the patch updates Kconfig to add 'arm64'.
If splitting two patches, the first patch for arm64 support only includes one line changes of Kconfig. So, I think it is unnecessary.
It's OK if the first patch is very small. The point is that the patch does two orthogonal things, and those two things should be in separate commits. This makes bisection work better, and it reduces the amount of functionality removed if we have to revert something.
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
drivers/pci/host/Kconfig +8 -0
+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) {
Quoted 8 lines
drivers/pci/host/Kconfig
Quoted 3 lines
drivers/pci/host/Kconfig +3 -0
+         dev_err(pp->dev, "failed to find msi-parent\n");
+         return -EINVAL;
+ }
This doesn't actually *do* anything except complain if "msi-parent" is missing. I'm not sure that's worth doing: if we need "msi-parent" somewhere, we should complain there if we don't find it. If we don't need it, why complain if it's missing?
Quoted 6 lines
driver/of/irq.c void of_msi_configure(struct device *dev, struct device_node *np) will bind "msi-parent" to each device if there is "msi-parent" handler. The PCIe driver do not need to do anything. If we do not check "msi-parent" here, we will have no chance to check it. The common code of 'of' and 'pci' bus driver will not complain, because the msi controller may be found by other way.

Quoted 6 lines

Hmm. In mvebu_pcie_msi_enable() and xgene_pcie_msi_enable(), we
also look for "msi-parent". If that fails, mvebu continues silently and xgene complains (but only if CONFIG_PCI_MSI=y).
This seems like three unnecessarily different ways of doing the same thing.

For X-Gene MSI, this is the old code where at the time the code was prepared, msi_controller needs to be assigned to a host bridge so that the bridge can support MSI. Until now, with recent changes on MSI irqdomain from Marc and others, we can drop this msi_controller assignment completely as my recent patch that you merged to pci/host-xgene branch:
https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/host-xgene

Quoted 41 lines

I cc'd the maintainers of those drivers; maybe we can agree on a single strategy.
Quoted 21 lines
Quoted 12 lines
Quoted 6 lines
static int __init ls_pcie_probe(struct platform_device *pdev) {
...
- ret = ls_add_pcie_port(pcie);
- if (ret < 0)
+ ret = dw_pcie_host_init(pp);
We have several DesignWare-based drivers (dra7xx, exynos, imx6, ks, ls, spear13xx), and I'd like to keep their structure as similar as possible. For example, they all have basically this structure:
X_pcie_probe
Quoted 8 lines
    X_add_pcie_port
      dw_pcie_host_init             # pp->ops->host_init callback
        X_pcie_host_init
          X_pcie_establish_link
This patch removes ls_add_pcie_port() and ls_pcie_establish_link(), so we're diverging a bit. That makes it harder to see the similarities across these drivers, which I think is a loss.
Quoted 5 lines
I will add X_add_pcie_port. But we do not need X_pcie_establish_link because we do not need change/reset phy to establish link, besides, the PCIe controller slot may be not inserted any PCIe device at all. If each controller waits some time for link, the start time will increase a lot. In some scenes, long start-up time is not acceptable.
If we wait for a timeout trying to establish a link when the slot is empty, it sounds like something's wrong. Can't we tell from presence detect whether a card is present, even without trying to bring up the link?
If we truly do not need a piece like X_pcie_establish_link(), I'm OK with entirely omitting it. What I really object to is when we have the same functionality two places with different names or structured two different ways.

Quoted 5 lines

Bjorn
--
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

Regards,
Duc Dang.

Lian M.H.

author

2015-10-12 02:53 UTC #4 lore raw

PATCH v2 1/1 PCI: layerscape: Add PCIe support for LS1043a and LS2080a

Hi Duc,

I notice your patches removed all the related MSI code and you descriptor said " Remove this unnecessary code. This also avoids a warning message ("failed to enable MSI") during boot ". I have a question.
Do we need a warning when MSI-parent is missing?

Thanks,
Minghuan

Quoted 179 lines

-----Original Message-----
From: Duc Dang [mailto:dhdang at apm.com]
Sent: Monday, October 12, 2015 9:48 AM
To: Bjorn Helgaas <[email protected]>
Cc: Lian Minghuan-B31939 <[email protected]>;
linux-pci at vger.kernel.org; linux-arm <[email protected]>; 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 Petazzoni <[email protected]>; Jason Cooper <[email protected]>; Tanmay Inamdar <[email protected]>
Subject: Re: [PATCH v2] PCI: layerscape: Add PCIe support for LS1043a and LS2080a
Quoted 98 lines · Bjorn Helgaas
[+cc Thomas, Jason, Tanmay]
Hi Minghuan,
You responded to this message, but your response was rejected by the mailing list, I think because it was not plain text. See
http://vger.kernel.org/majordomo-info.html
I went ahead and reconstructed what your response would have looked like so I could continue the conversation. But please fix your email setup before responding again :)
Quoted 50 lines · Minghuan
Quoted 35 lines · Bjorn Helgaas
Hi Minghuan,
Quoted 23 lines · Minghuan Lian
The patch adds PCIe support for LS1043a and LS2080a.
Signed-off-by Minghuan Lian <[email protected]>
---
This patch is based on v4.3-rc1 and [PATCH v9 0/6]
PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05 patchset from Zhou Wang.
change log
v2:
1. rename ls2085a to ls2080a
2. Add ls_pcie_msi_host_init()
 drivers/pci/host/Kconfig          |   2 +-
 drivers/pci/host/pci-layerscape.c | 227
++++++++++++++++++++++++++------------
 2 files changed, 157 insertions(+), 72 deletions(-)
drivers/pci/host/Kconfig +0 -0
index ae873be..38fe8a8 100644
drivers/pci/host/Kconfig config PCI_XGENE_MSI · line 108
Quoted 5 lines
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 || ARM64)
It seems like there are a couple things going on here, and I wonder if you can split them out into separate patches.
Quoted 8 lines
1) Making this work on ARM64 as well as on ARM. This may be of interest for other DesignWare-based drivers, so if you split this out, maybe it would be a useful template for converting the other drivers, too.
2) Adding LS1043a and LS2080a. Obviously, this is only of interest to this driver, but maybe it could be separated out into a separate patch?
The patch is based on v4.3-rc1 and [PATCH v9 0/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05 patchset from Zhou Wang which adds arm and
arm64 support.
Quoted 6 lines
The original code with the Zhou Wang's patches can support arm64 as well. Because both LS1043a and LS2085 are armv8 (arm64) architecture, the patch updates Kconfig to add 'arm64'.
If splitting two patches, the first patch for arm64 support only includes one line changes of Kconfig. So, I think it is unnecessary.
It's OK if the first patch is very small. The point is that the patch does two orthogonal things, and those two things should be in separate commits. This makes bisection work better, and it reduces the amount of functionality removed if we have to revert something.
Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
drivers/pci/host/Kconfig +7 -0
+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) {
Quoted 8 lines
drivers/pci/host/Kconfig
Quoted 3 lines
drivers/pci/host/Kconfig +3 -0
+         dev_err(pp->dev, "failed to find msi-parent\n");
+         return -EINVAL;
+ }
This doesn't actually *do* anything except complain if "msi-parent" is missing. I'm not sure that's worth doing: if we need "msi-parent" somewhere, we should complain there if we don't find it. If we don't need it, why complain if it's missing?
Quoted 6 lines
driver/of/irq.c void of_msi_configure(struct device *dev, struct device_node *np) will bind "msi-parent" to each device if there is "msi-parent" handler. The PCIe driver do not need to do anything. If we do not check "msi-parent" here, we will have no chance to check it. The common code of 'of' and 'pci' bus driver will not complain, because the msi controller may be found by other way.
Quoted 6 lines
Hmm. In mvebu_pcie_msi_enable() and xgene_pcie_msi_enable(), we
also look for "msi-parent". If that fails, mvebu continues silently and xgene complains (but only if CONFIG_PCI_MSI=y).
This seems like three unnecessarily different ways of doing the same thing.
For X-Gene MSI, this is the old code where at the time the code was prepared, msi_controller needs to be assigned to a host bridge so that the bridge can support MSI. Until now, with recent changes on MSI irqdomain from Marc and others, we can drop this msi_controller assignment completely as my recent patch that you merged to pci/host-xgene branch:
https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/host
-xgene
Quoted 40 lines
I cc'd the maintainers of those drivers; maybe we can agree on a single strategy.
Quoted 20 lines
Quoted 11 lines
Quoted 5 lines
static int __init ls_pcie_probe(struct platform_device *pdev) {
...
- ret = ls_add_pcie_port(pcie);
- if (ret < 0)
+ ret = dw_pcie_host_init(pp);
We have several DesignWare-based drivers (dra7xx, exynos, imx6, ks, ls, spear13xx), and I'd like to keep their structure as similar as possible. For example, they all have basically this structure:
X_pcie_probe
Quoted 8 lines
    X_add_pcie_port
      dw_pcie_host_init             # pp->ops->host_init callback
        X_pcie_host_init
          X_pcie_establish_link
This patch removes ls_add_pcie_port() and ls_pcie_establish_link(), so we're diverging a bit. That makes it harder to see the similarities across these drivers, which I think is a loss.
Quoted 5 lines
I will add X_add_pcie_port. But we do not need X_pcie_establish_link because we do not need change/reset phy to establish link, besides, the PCIe controller slot may be not inserted any PCIe device at all. If each controller waits some time for link, the start time will increase a lot. In some scenes, long start-up time is not acceptable.
If we wait for a timeout trying to establish a link when the slot is empty, it sounds like something's wrong. Can't we tell from presence detect whether a card is present, even without trying to bring up the link?
If we truly do not need a piece like X_pcie_establish_link(), I'm OK with entirely omitting it. What I really object to is when we have the same functionality two places with different names or structured two different ways.
Quoted 5 lines
Bjorn
--
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
Regards,
Duc Dang.

Duc Dang

2015-10-12 23:02 UTC #5 lore raw

PATCH v2 1/1 PCI: layerscape: Add PCIe support for LS1043a and LS2080a

Quoted 5 lines · Lian M.H.

Hi Duc,
I notice your patches removed all the related MSI code and you descriptor said " Remove this unnecessary code. This also avoids a warning message ("failed to enable MSI") during boot ". I have a question.
Do we need a warning when MSI-parent is missing?

Hi Lian,

In case of X-Gene PCIe: No. Initially, X-Gene PCIe driver does not support MSI. Later on, MSI support was added with the code to look for msi-parent property. But then thanks to recent works on msi irqdomain, these code is no longer needed to support MSI for X-Gene PCIe, so I removed them.

Quoted 183 lines

Thanks,
Minghuan
Quoted 179 lines
-----Original Message-----
From: Duc Dang [mailto:dhdang at apm.com]
Sent: Monday, October 12, 2015 9:48 AM
To: Bjorn Helgaas <[email protected]>
Cc: Lian Minghuan-B31939 <[email protected]>;
linux-pci at vger.kernel.org; linux-arm <[email protected]>; 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 Petazzoni <[email protected]>; Jason Cooper <[email protected]>; Tanmay Inamdar <[email protected]>
Subject: Re: [PATCH v2] PCI: layerscape: Add PCIe support for LS1043a and LS2080a
Quoted 98 lines · Bjorn Helgaas
[+cc Thomas, Jason, Tanmay]
Hi Minghuan,
You responded to this message, but your response was rejected by the mailing list, I think because it was not plain text. See
http://vger.kernel.org/majordomo-info.html
I went ahead and reconstructed what your response would have looked like so I could continue the conversation. But please fix your email setup before responding again :)
Quoted 50 lines · Minghuan
Quoted 35 lines · Bjorn Helgaas
Hi Minghuan,
Quoted 23 lines · Minghuan Lian
The patch adds PCIe support for LS1043a and LS2080a.
Signed-off-by Minghuan Lian <[email protected]>
---
This patch is based on v4.3-rc1 and [PATCH v9 0/6]
PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05 patchset from Zhou Wang.
change log
v2:
1. rename ls2085a to ls2080a
2. Add ls_pcie_msi_host_init()
 drivers/pci/host/Kconfig          |   2 +-
 drivers/pci/host/pci-layerscape.c | 227
++++++++++++++++++++++++++------------
 2 files changed, 157 insertions(+), 72 deletions(-)
drivers/pci/host/Kconfig +0 -0
index ae873be..38fe8a8 100644
drivers/pci/host/Kconfig config PCI_XGENE_MSI · line 108
Quoted 5 lines
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 || ARM64)
It seems like there are a couple things going on here, and I wonder if you can split them out into separate patches.
Quoted 8 lines
1) Making this work on ARM64 as well as on ARM. This may be of interest for other DesignWare-based drivers, so if you split this out, maybe it would be a useful template for converting the other drivers, too.
2) Adding LS1043a and LS2080a. Obviously, this is only of interest to this driver, but maybe it could be separated out into a separate patch?
The patch is based on v4.3-rc1 and [PATCH v9 0/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05 patchset from Zhou Wang which adds arm and
arm64 support.
Quoted 6 lines
The original code with the Zhou Wang's patches can support arm64 as well. Because both LS1043a and LS2085 are armv8 (arm64) architecture, the patch updates Kconfig to add 'arm64'.
If splitting two patches, the first patch for arm64 support only includes one line changes of Kconfig. So, I think it is unnecessary.
It's OK if the first patch is very small. The point is that the patch does two orthogonal things, and those two things should be in separate commits. This makes bisection work better, and it reduces the amount of functionality removed if we have to revert something.
Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
drivers/pci/host/Kconfig +7 -0
+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) {
Quoted 8 lines
drivers/pci/host/Kconfig
Quoted 3 lines
drivers/pci/host/Kconfig +3 -0
+         dev_err(pp->dev, "failed to find msi-parent\n");
+         return -EINVAL;
+ }
This doesn't actually *do* anything except complain if "msi-parent" is missing. I'm not sure that's worth doing: if we need "msi-parent" somewhere, we should complain there if we don't find it. If we don't need it, why complain if it's missing?
Quoted 6 lines
driver/of/irq.c void of_msi_configure(struct device *dev, struct device_node *np) will bind "msi-parent" to each device if there is "msi-parent" handler. The PCIe driver do not need to do anything. If we do not check "msi-parent" here, we will have no chance to check it. The common code of 'of' and 'pci' bus driver will not complain, because the msi controller may be found by other way.
Quoted 6 lines
Hmm. In mvebu_pcie_msi_enable() and xgene_pcie_msi_enable(), we
also look for "msi-parent". If that fails, mvebu continues silently and xgene complains (but only if CONFIG_PCI_MSI=y).
This seems like three unnecessarily different ways of doing the same thing.
For X-Gene MSI, this is the old code where at the time the code was prepared, msi_controller needs to be assigned to a host bridge so that the bridge can support MSI. Until now, with recent changes on MSI irqdomain from Marc and others, we can drop this msi_controller assignment completely as my recent patch that you merged to pci/host-xgene branch:
https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/host
-xgene
Quoted 40 lines
I cc'd the maintainers of those drivers; maybe we can agree on a single strategy.
Quoted 20 lines
Quoted 11 lines
Quoted 5 lines
static int __init ls_pcie_probe(struct platform_device *pdev) {
...
- ret = ls_add_pcie_port(pcie);
- if (ret < 0)
+ ret = dw_pcie_host_init(pp);
We have several DesignWare-based drivers (dra7xx, exynos, imx6, ks, ls, spear13xx), and I'd like to keep their structure as similar as possible. For example, they all have basically this structure:
X_pcie_probe
Quoted 8 lines
    X_add_pcie_port
      dw_pcie_host_init             # pp->ops->host_init callback
        X_pcie_host_init
          X_pcie_establish_link
This patch removes ls_add_pcie_port() and ls_pcie_establish_link(), so we're diverging a bit. That makes it harder to see the similarities across these drivers, which I think is a loss.
Quoted 5 lines
I will add X_add_pcie_port. But we do not need X_pcie_establish_link because we do not need change/reset phy to establish link, besides, the PCIe controller slot may be not inserted any PCIe device at all. If each controller waits some time for link, the start time will increase a lot. In some scenes, long start-up time is not acceptable.
If we wait for a timeout trying to establish a link when the slot is empty, it sounds like something's wrong. Can't we tell from presence detect whether a card is present, even without trying to bring up the link?
If we truly do not need a piece like X_pcie_establish_link(), I'm OK with entirely omitting it. What I really object to is when we have the same functionality two places with different names or structured two different ways.
Quoted 5 lines
Bjorn
--
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
Regards,
Duc Dang.

Regards.
Duc Dang.

Thomas Petazzoni

2015-10-12 07:15 UTC #6 lore raw

PATCH v2 1/1 PCI: layerscape: Add PCIe support for LS1043a and LS2080a

In reply to Bjorn Helgaas

Bjorn,

Quoted 10 lines · Bjorn Helgaas

Quoted 6 lines
driver/of/irq.c void of_msi_configure(struct device *dev, struct device_node *np) will bind "msi-parent" to each device if there is "msi-parent" handler. The PCIe driver do not need to do anything. If we do not check "msi-parent" here, we will have no chance to check it. The common code of 'of' and 'pci' bus driver will not complain, because the msi controller may be found by other way.
Hmm. In mvebu_pcie_msi_enable() and xgene_pcie_msi_enable(), we
also look for "msi-parent". If that fails, mvebu continues silently and xgene complains (but only if CONFIG_PCI_MSI=y).

I don't really have the context of the discussion here. But the reason why the mvebu pcie driver silently continues if msi-parent is missing is because we initially introduced the PCIe mvebu Device Tree binding without MSI support. When we later added MSI support thanks to the msi-parent property, we wanted to preserve backward compatibility with old DTs that didn't had the msi-parent property. Such DTs would continue to work, albeit without the MSI functionality obviously.

Other drivers that had the MSI functionality from day 1 may want to make such a property mandatory rather than optional.

Best regards,

Thomas

Signature · 3 lines

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Arnd Bergmann

2015-10-12 12:36 UTC #7 lore raw

PATCH v2 1/1 PCI: layerscape: Add PCIe support for LS1043a and LS2080a

In reply to Bjorn Helgaas

Quoted 14 lines · On Wednesday 07 October 2015 12:57:25 Bjorn Helgaas wrote:

drivers/pci/host/Kconfig config PCI_XGENE_MSI · line 108
Quoted 10 lines
drivers/pci/host/Kconfig +1 -1
index ae873be..38fe8a8 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 || ARM64)
It seems like there are a couple things going on here, and I wonder if you can split them out into separate patches.

Quoted 4 lines

1) Making this work on ARM64 as well as on ARM. This may be of interest for other DesignWare-based drivers, so if you split this out, maybe it would be a useful template for converting the other drivers, too.

The Kconfig change apparently made it into linux-next now, but it doesn't actually build on arm64, because the dependency that was mentioned in the cover letter [0] is not there.

Arnd

[0] This patch is based on v4.3-rc1 and [PATCH v9 0/6]
PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05 patchset from Zhou Wang.

Bjorn Helgaas

2015-10-12 15:26 UTC #8 lore raw

PATCH v2 1/1 PCI: layerscape: Add PCIe support for LS1043a and LS2080a

Quoted 20 lines · Arnd Bergmann

Quoted 14 lines · On Wednesday 07 October 2015 12:57:25 Bjorn Helgaas wrote:
drivers/pci/host/Kconfig config PCI_XGENE_MSI · line 108
Quoted 10 lines
drivers/pci/host/Kconfig +1 -1
index ae873be..38fe8a8 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 || ARM64)
It seems like there are a couple things going on here, and I wonder if you can split them out into separate patches.
Quoted 4 lines
1) Making this work on ARM64 as well as on ARM. This may be of interest for other DesignWare-based drivers, so if you split this out, maybe it would be a useful template for converting the other drivers, too.

Quoted 7 lines

The Kconfig change apparently made it into linux-next now, but it doesn't actually build on arm64, because the dependency that was mentioned in the cover letter [0] is not there.
...
[0] This patch is based on v4.3-rc1 and [PATCH v9 0/6]
PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05 patchset from Zhou Wang.

Oops, my fault. I read that as "this patch is derived from, e.g., it a newer version of, the HiSilicon patchset." But that's obviously a silly way to read it. I re-did the merge without the Layerscape patch and repushed it to my "next" branch.

I'll work on Zhou's series, then revisit Minghuan's patch(es). I'm hoping the Layerscape patch can be split up a bit, both to split out the ARM64 config change and to make a cleaner "LS1043a and LS2080a" support patch. The current patch changes several things that are not obviously directly related to LS1043a and LS2080a. It would be ideal to have a series of things like:

- add ARM64 build support (maybe just the Kconfig change)
- make the ls_pcie_probe() changes that are apparently generic across all the LS devices
- add driver data to lc_pcie_of_match[] (this wouldn't change any behavior at all; it would just add the ls1021_drvdata that works for the already-supported devices)
- add ls1032_drvdata and ls2080_drvdata for the new devices
- add the "msi-parent" diagnostic (this seems to be basically new functionality that applies to all devices, not just the new ones)

That way, each individual change would be easier to review.

Bjorn

Lian M.H.

author

2015-10-13 01:37 UTC #9 lore raw

PATCH v2 1/1 PCI: layerscape: Add PCIe support for LS1043a and LS2080a

Hi Bjorn,

Thanks for your comments
I will split the patch.

Thanks,
Minghuan

Quoted 68 lines

-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas at kernel.org]
Sent: Monday, October 12, 2015 11:27 PM
To: Arnd Bergmann <[email protected]>
Cc: Lian Minghuan-B31939 <[email protected]>; 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]>; Bjorn Helgaas <[email protected]>; Jingoo Han <[email protected]>; Zhou Wang <[email protected]>
Subject: Re: [PATCH v2] PCI: layerscape: Add PCIe support for LS1043a and LS2080a
Quoted 20 lines · Arnd Bergmann
Quoted 14 lines · On Wednesday 07 October 2015 12:57:25 Bjorn Helgaas wrote:
drivers/pci/host/Kconfig config PCI_XGENE_MSI · line 108
Quoted 10 lines
drivers/pci/host/Kconfig +1 -1
index ae873be..38fe8a8 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 || ARM64)
It seems like there are a couple things going on here, and I wonder if you can split them out into separate patches.
Quoted 4 lines
1) Making this work on ARM64 as well as on ARM. This may be of interest for other DesignWare-based drivers, so if you split this out, maybe it would be a useful template for converting the other drivers, too.
Quoted 7 lines
The Kconfig change apparently made it into linux-next now, but it doesn't actually build on arm64, because the dependency that was mentioned in the cover letter [0] is not there.
...
[0] This patch is based on v4.3-rc1 and [PATCH v9 0/6]
PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05 patchset from Zhou Wang.
Oops, my fault. I read that as "this patch is derived from, e.g., it a newer version of, the HiSilicon patchset." But that's obviously a silly way to read it. I re-did the merge without the Layerscape patch and repushed it to my "next" branch.
I'll work on Zhou's series, then revisit Minghuan's patch(es). I'm hoping the Layerscape patch can be split up a bit, both to split out the ARM64 config change and to make a cleaner "LS1043a and LS2080a"
support patch. The current patch changes several things that are not obviously directly related to LS1043a and LS2080a. It would be ideal to have a series of things like:
- add ARM64 build support (maybe just the Kconfig change)
- make the ls_pcie_probe() changes that are apparently generic across all the LS devices
- add driver data to lc_pcie_of_match[] (this wouldn't change any behavior at all; it would just add the ls1021_drvdata that works for the already-supported devices)
- add ls1032_drvdata and ls2080_drvdata for the new devices
- add the "msi-parent" diagnostic (this seems to be basically new functionality that applies to all devices, not just the new ones)
That way, each individual change would be easier to review.
Bjorn