HI Scott,

please see my comments inline.

On 01/04/2014 06:33 AM, Scott Wood wrote:

On Wed, Oct 23, 2013 at 06:41:25PM +0800, Minghuan Lian wrote:

quoted

The patch adds PCI indirect read/write functions. The main code
is ported from arch/powerpc/sysdev/indirect_pci.c. We use general
IO API iowrite32be/ioread32be instead of out_be32/in_be32, and
use structure fsl_Pci instead of PowerPC's pci_controller.
The patch also provides fsl_pcie_check_link() to check PCI link.
The weak function fsl_arch_pci_exclude_device() is provided to
call ppc_md.pci_exclude_device() for PowerPC architecture.

Signed-off-by: Minghuan Lian <redacted>

---
change log:
v1-v3:
Derived from http://patchwork.ozlabs.org/patch/278965/

Based on upstream master.
Based on the discussion of RFC version here
http://patchwork.ozlabs.org/patch/274487/

  drivers/pci/host/pci-fsl-common.c | 169 ++++++++++++++++++++++++++++++++------
  include/linux/fsl/pci-common.h    |   6 ++
  2 files changed, 151 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/host/pci-fsl-common.c b/drivers/pci/host/pci-fsl-common.c
index 69d338b..8bc9a64 100644
--- a/drivers/pci/host/pci-fsl-common.c
+++ b/drivers/pci/host/pci-fsl-common.c

@@ -35,52 +35,173 @@
  #include <sysdev/fsl_soc.h>
  #include <sysdev/fsl_pci.h>
  
-static int fsl_pcie_check_link(struct pci_controller *hose)
+/* Indirect type */
+#define INDIRECT_TYPE_EXT_REG			0x00000002
+#define INDIRECT_TYPE_SURPRESS_PRIMARY_BUS	0x00000004
+#define INDIRECT_TYPE_NO_PCIE_LINK		0x00000008
+#define INDIRECT_TYPE_BIG_ENDIAN		0x00000010
+#define INDIRECT_TYPE_FSL_CFG_REG_LINK		0x00000040

Why are these here rather than in the header, given that you have
indirect_type in the struct in the header?

[Minghuan] It's better to define the type in the header file. I will fix it.

quoted

+int __weak fsl_arch_pci_exclude_device(struct fsl_pci *pci, u8 bus, u8 devfn)
+{
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int fsl_pci_read_config(struct fsl_pci *pci, int bus, int devfn,
+				int offset, int len, u32 *val)
+{
+	u32 bus_no, reg, data;
+
+	if (pci->indirect_type & INDIRECT_TYPE_NO_PCIE_LINK) {
+		if (bus != pci->first_busno)
+			return PCIBIOS_DEVICE_NOT_FOUND;
+		if (devfn != 0)
+			return PCIBIOS_DEVICE_NOT_FOUND;
+	}

A lot of this seems duplicated from arch/powerpc/sysdev/indirect_pci.c.

How generally applicable is that file to non-PPC implementations?  At a
minimum I see a similar file in arch/microblaze.  It should probably
eventually be moved to common code, rather than duplicated again.  A
prerequisite for that would be making common the dependencies it has on
the rest of what is currently arch PCI infrastructure; until then, it's
probably better to just have the common fsl-pci code know how to
interface with the appropriate PPC/ARM code rather than trying to copy
the infrastructure as well.

[Minghuan] Yes, This is a duplicate except it uses struct fsl_pci. But 
it is hard to be move to common code.
because every indirect read/write functions use different PCI controller 
structure which is very basic structure and ARM has no this structure.
If we can not establish a unified pci controller structure, we can only 
abstract out a simple structure which includes indirect access related 
fields,
and need a callback function to get the pointer like this: 
((powerpc/microblaze/mips/ pci_controller 
*)(pci_bus->sysdata))->indirect_struct.
Should we provide the common code for indirect access API or wait for 
the common PCI controller structure?

-Scott