[PATCH v2] PCI: layerscape: Add PCIe support for LS1043a and LS2080a

The patch adds PCIe support for LS1043a and LS2080a.

Signed-off-by: Minghuan Lian <redacted>
---
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(-)

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index ae873be..38fe8a8 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig

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

diff --git a/drivers/pci/host/pci-layerscape.c b/drivers/pci/host/pci-layerscape.c
index b2328ea1..cdb8737 100644
--- a/drivers/pci/host/pci-layerscape.c
+++ b/drivers/pci/host/pci-layerscape.c

@@ -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)
+{
+	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)
+{
+	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,
+				 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 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,
-					 "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);
+	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",

-- 
1.9.1

Hi Minghuan,

On Thu, Sep 17, 2015 at 05:13:39PM +0800, Minghuan Lian wrote:

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?

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.

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

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?

+
+	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,
-					 "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.

+	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",
-- 
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

[+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 :)

Minghuan wrote:

On Wed, Oct 07, 2015 at 12:57:25PM -0500, Bjorn Helgaas wrote:

quoted

Hi Minghuan,

On Thu, Sep 17, 2015 at 05:13:39PM +0800, Minghuan Lian wrote:

quoted

The patch adds PCIe support for LS1043a and LS2080a.

Signed-off-by: Minghuan Lian <redacted>
---
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(-)

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index ae873be..38fe8a8 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig

@@ -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?

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.

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

quoted

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

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?

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

quoted

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

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

On Sun, Oct 11, 2015 at 12:10 PM, Bjorn Helgaas [off-list ref] wrote:

[+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 :)

Minghuan wrote:

quoted

On Wed, Oct 07, 2015 at 12:57:25PM -0500, Bjorn Helgaas wrote:

quoted

Hi Minghuan,

On Thu, Sep 17, 2015 at 05:13:39PM +0800, Minghuan Lian wrote:

quoted

The patch adds PCIe support for LS1043a and LS2080a.

Signed-off-by: Minghuan Lian <redacted>
---
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(-)

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index ae873be..38fe8a8 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig

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

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.

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

quoted

quoted

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

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

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.

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

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

quoted

quoted

quoted

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

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

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

-----Original Message-----
From: Duc Dang [mailto:dhdang at apm.com]
Sent: Monday, October 12, 2015 9:48 AM
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Lian Minghuan-B31939 <redacted>;
linux-pci at vger.kernel.org; linux-arm [off-list ref];
Zang Roy-R61911 [off-list ref]; Hu Mingkai-B21284
[off-list ref]; Yoder Stuart-B08248
[off-list ref]; Li Yang-Leo-R58472 [off-list ref];
Arnd Bergmann [off-list ref]; Bjorn Helgaas [off-list ref];
Jingoo Han [off-list ref]; Zhou Wang
[off-list ref]; Thomas Petazzoni
[off-list ref]; Jason Cooper
[off-list ref]; Tanmay Inamdar [off-list ref]
Subject: Re: [PATCH v2] PCI: layerscape: Add PCIe support for LS1043a and
LS2080a

On Sun, Oct 11, 2015 at 12:10 PM, Bjorn Helgaas [off-list ref] wrote:

quoted

[+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 :)

Minghuan wrote:

quoted

On Wed, Oct 07, 2015 at 12:57:25PM -0500, Bjorn Helgaas wrote:

quoted

Hi Minghuan,

On Thu, Sep 17, 2015 at 05:13:39PM +0800, Minghuan Lian wrote:

quoted

The patch adds PCIe support for LS1043a and LS2080a.

Signed-off-by: Minghuan Lian <redacted>
---
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(-)

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index ae873be..38fe8a8 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig

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

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.

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

quoted

quoted

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

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

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.

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

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

quoted

quoted

quoted

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

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

Bjorn,

On Sun, 11 Oct 2015 14:10:27 -0500, Bjorn Helgaas wrote:

quoted

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

On Wednesday 07 October 2015 12:57:25 Bjorn Helgaas wrote:

quoted

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index ae873be..38fe8a8 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig

@@ -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.

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.

On Mon, Oct 12, 2015 at 02:36:29PM +0200, Arnd Bergmann wrote:

On Wednesday 07 October 2015 12:57:25 Bjorn Helgaas wrote:

quoted

quoted

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index ae873be..38fe8a8 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig

@@ -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.

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

On Sun, Oct 11, 2015 at 7:53 PM, Lian M.H. [off-list ref] wrote:

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.

Thanks,
Minghuan

quoted

-----Original Message-----
From: Duc Dang [mailto:dhdang at apm.com]
Sent: Monday, October 12, 2015 9:48 AM
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Lian Minghuan-B31939 <redacted>;
linux-pci at vger.kernel.org; linux-arm [off-list ref];
Zang Roy-R61911 [off-list ref]; Hu Mingkai-B21284
[off-list ref]; Yoder Stuart-B08248
[off-list ref]; Li Yang-Leo-R58472 [off-list ref];
Arnd Bergmann [off-list ref]; Bjorn Helgaas [off-list ref];
Jingoo Han [off-list ref]; Zhou Wang
[off-list ref]; Thomas Petazzoni
[off-list ref]; Jason Cooper
[off-list ref]; Tanmay Inamdar [off-list ref]
Subject: Re: [PATCH v2] PCI: layerscape: Add PCIe support for LS1043a and
LS2080a

On Sun, Oct 11, 2015 at 12:10 PM, Bjorn Helgaas [off-list ref] wrote:

quoted

[+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 :)

Minghuan wrote:

quoted

On Wed, Oct 07, 2015 at 12:57:25PM -0500, Bjorn Helgaas wrote:

quoted

Hi Minghuan,

On Thu, Sep 17, 2015 at 05:13:39PM +0800, Minghuan Lian wrote:

quoted

The patch adds PCIe support for LS1043a and LS2080a.

Signed-off-by: Minghuan Lian <redacted>
---
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(-)

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index ae873be..38fe8a8 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig

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

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.

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

quoted

quoted

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

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

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.

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

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

quoted

quoted

quoted

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

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

Hi Bjorn,

Thanks for your comments
I will split the patch.

Thanks,
Minghuan

-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas at kernel.org]
Sent: Monday, October 12, 2015 11:27 PM
To: Arnd Bergmann <arnd@arndb.de>
Cc: Lian Minghuan-B31939 <redacted>;
linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Zang
Roy-R61911 [off-list ref]; Hu Mingkai-B21284
[off-list ref]; Yoder Stuart-B08248
[off-list ref]; Li Yang-Leo-R58472 [off-list ref];
Bjorn Helgaas [off-list ref]; Jingoo Han [off-list ref];
Zhou Wang [off-list ref]
Subject: Re: [PATCH v2] PCI: layerscape: Add PCIe support for LS1043a and
LS2080a

On Mon, Oct 12, 2015 at 02:36:29PM +0200, Arnd Bergmann wrote:

quoted

On Wednesday 07 October 2015 12:57:25 Bjorn Helgaas wrote:

quoted

quoted

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index ae873be..38fe8a8 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig

@@ -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.

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