Hi Scott,

Thanks for your comments, please see my replies inline.

On 08/24/2013 05:45 AM, Scott Wood wrote:

On Mon, 2013-08-19 at 20:23 +0800, Minghuan Lian wrote:

quoted

The Freescale's Layerscape series processors will use ARM cores.
The LS1's PCIe controllers is the same as T4240's. So it's better
the PCIe controller driver can support PowerPC and ARM
simultaneously. This patch is for this purpose. It derives
the common functions from arch/powerpc/sysdev/fsl_pci.c to
drivers/pci/host/pcie-fsl.c and leaves several platform-dependent
functions which should be implemented in platform files.

Signed-off-by: Minghuan Lian<redacted>
---
Based on upstream master 3.11-rc6
The function has been tested  on P5020DS and P3041DS and T4240QDS boards
For mpc83xx and mpc86xx, I only did compile test.

I assume you'll test on these (and older mpc85xx) before this becomes
non-RFC?

[Minghuan] I will try to test on the relevant boards and list them.

quoted

  arch/powerpc/Kconfig          |   1 +
  arch/powerpc/sysdev/fsl_pci.c | 591 ++++++----------------------------
  arch/powerpc/sysdev/fsl_pci.h |  91 ------
  drivers/edac/mpc85xx_edac.c   |  10 -
  drivers/pci/host/Kconfig      |   4 +
  drivers/pci/host/Makefile     |   1 +
  drivers/pci/host/pcie-fsl.c   | 734 ++++++++++++++++++++++++++++++++++++++++++
  include/linux/fsl/fsl-pcie.h  | 176 ++++++++++
  8 files changed, 1008 insertions(+), 600 deletions(-)
  create mode 100644 drivers/pci/host/pcie-fsl.c
  create mode 100644 include/linux/fsl/fsl-pcie.h

Please use -M -C with git format-patch.

Why "pcie" rather than "pci"?  Is non-express not supported by these new
files?

[Minghuan] Using "pci" is more accurate. I selected 'pcie' because the 
new file is
mainly for PCI Express controller, but it does contain two pci 
boards(mpc8610,
mpc8540) support. I will change to 'pci'.

quoted

@@ -259,15 +258,6 @@ int mpc85xx_pci_err_probe(struct platform_device *op)
  
  	/* we only need the error registers */
  	r.start += 0xe00;
-
-	if (!devm_request_mem_region(&op->dev, r.start, resource_size(&r),
-					pdata->name)) {
-		printk(KERN_ERR "%s: Error while requesting mem region\n",
-		       __func__);
-		res = -EBUSY;
-		goto err;
-	}
-
  	pdata->pci_vbase = devm_ioremap(&op->dev, r.start, resource_size(&r));
  	if (!pdata->pci_vbase) {
  		printk(KERN_ERR "%s: Unable to setup PCI err regs\n", __func__);

Could you explain this part?

[Minghuan] The new pci driver used devm_ioremap_resource() to map reg space.
So PCI EDAC driver would encounter an error when calling 
devm_request_mem_region()
because pci device reg space has been assigned to pci driver. And EDAC 
is only to
handler the error, has no reason to request exclusive PCI device reg space.

quoted

diff --git a/drivers/pci/host/pcie-fsl.c b/drivers/pci/host/pcie-fsl.c
new file mode 100644
index 0000000..6e767d4
--- /dev/null
+++ b/drivers/pci/host/pcie-fsl.c

@@ -0,0 +1,734 @@
+/*
+ * 85xx/86xx/LS PCI/PCIE common driver support
+ *
+ * Copyright 2013 Freescale Semiconductor, Inc.
+ *
+ * Moved from arch/power/fsl_pci.c

That's not the right pathname.

[Minghuan] Sorry, I will fix it.

quoted

+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/string.h>
+#include <linux/init.h>
+#include <linux/log2.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/pci_regs.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/types.h>
+#include <linux/memblock.h>
+#include <linux/fsl/fsl-pcie.h>

You don't need an "fsl-" prefix if it's in the "fsl/" directory.

[Minghuan] No, I will remove 'fsl-' prefix.

quoted

+static int fsl_pcie_write_config(struct fsl_pcie *pcie, int bus, int devfn,
+				 int offset, int len, u32 val)
+{
+	void __iomem *cfg_data;
+	u32 bus_no, reg;
+
+	if (pcie->indirect_type & INDIRECT_TYPE_NO_PCIE_LINK) {
+		if (bus != pcie->first_busno)
+			return PCIBIOS_DEVICE_NOT_FOUND;
+		if (devfn != 0)
+			return PCIBIOS_DEVICE_NOT_FOUND;
+	}
+
+	if (fsl_pci_exclude_device(pcie, bus, devfn))
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	bus_no = (bus == pcie->first_busno) ?
+			pcie->self_busno : bus;
+
+	if (pcie->indirect_type & INDIRECT_TYPE_EXT_REG)
+		reg = ((offset & 0xf00) << 16) | (offset & 0xfc);
+	else
+		reg = offset & 0xfc;
+
+	if (pcie->indirect_type & INDIRECT_TYPE_BIG_ENDIAN)
+		out_be32(&pcie->regs->config_addr,
+			 (0x80000000 | (bus_no << 16) | (devfn << 8) | reg));
+	else
+		out_le32(&pcie->regs->config_addr,
+			 (0x80000000 | (bus_no << 16) | (devfn << 8) | reg));

Did you try building this on ARM?  out_be32/le32() is PPC-specific.  Use iowrite32be()/iowrite32().

[Minghuan] Yes. I will change them.

quoted

+ep_mode:
+	dev_info(&pdev->dev, "It works as EP mode\n");

This is a bit casually phrased...  and where did this come from?  This
patch should just be about moving code around and removing PPC
dependencies (ideally even those two would be separate).  If there's
new functionality or other changes it should be a separate patch.

[Minghuan] Sorry, I will continue using "no_bridge"

quoted

+static int __init fsl_pcie_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct fsl_pcie *pcie;
+
+	if (!of_device_is_available(pdev->dev.of_node)) {
+		dev_err(&pdev->dev, "disabled\n");
+		return -ENODEV;
+	}

That's not an error.

[Minghuan] Ok, I will fix it.

-Scott