Re: [PATCH 2/2] pci: fsl: rework PCI driver compatible with Layerscape

2013-09-30 Thread Scott Wood 
On Mon, 2013-09-30 at 09:52 +0800, Lian Minghuan-b31939 wrote:
 Hi Timur,
 
 Thanks for your comments.
 
 How about PCI_FSL_COMMON?

Why not just have one symbol, which is used in the makefile in both
drivers/pci and arch/whatever?

-Scott



___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/2] pci: fsl: rework PCI driver compatible with Layerscape

2013-09-30 Thread Scott Wood 
On Sun, 2013-09-29 at 19:51 +0800, Lian Minghuan-b31939 wrote:
  +/*
  + * The fsl_arch_* functions are arch hooks. Those functions are
  + * implemented as weak symbols so that they can be overridden by
  + * architecture specific code if needed.
  + */
  +
  +/* Return PCI64 DMA offset */
  +u64 fsl_arch_pci64_dma_offset(void);
  Is this always guaranteed to exist?
 [Minghuan] Yes. I define a __weak implementation in pci-fsl.c

I meant is it guaranteed that such a mapping exists?  The API doesn't
give a way to say no.

Also you should describe what PCI64 DMA offset means.

  +/* Register PCI/PCIe controller to architecture system */
  +int __weak fsl_arch_pci_sys_register(struct fsl_pci *pci);
  +
  +/* Remove PCI/PCIe controller from architecture system */
  +void __weak fsl_arch_pci_sys_remove(struct fsl_pci *pci);
  Why do these need to be weak?  Won't there be exactly one implementation
  per supported arch?
 [Minghuan] I added __weak for compiling kernel when selecting pci-fsl 
 module but
 there is no related arch pci implementation.

The module should not be buildable on arches that don't have support.

-Scott



___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/2] pci: fsl: rework PCI driver compatible with Layerscape

2013-09-30 Thread Timur Tabi 
On Sun, Sep 29, 2013 at 8:52 PM, Lian Minghuan-b31939
b31...@freescale.com wrote:

 How about PCI_FSL_COMMON?

That's not any better.  The problem is that you have two symbols for
generic/common FSL PCI support.  Adding the word common does not
help.  You need to make the symbols distinct.  Either that, or merge
them into one symbol.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/2] pci: fsl: rework PCI driver compatible with Layerscape

2013-09-29 Thread Lian Minghuan-b31939 

Hi Scott,

please see my comments inline.

On 09/28/2013 12:54 AM, Scott Wood wrote:

On Wed, 2013-09-18 at 19:02 +0800, Minghuan Lian wrote:

@@ -592,6 +719,7 @@ int fsl_pci_mcheck_exception(struct pt_regs *regs)
  #if defined(CONFIG_FSL_SOC_BOOKE) || defined(CONFIG_PPC_86xx)
  
  struct device_node *fsl_pci_primary;

+extern const struct of_device_id fsl_pci_ids[];

Externs go in headers.

[Minghuan] ok.



-static int fsl_pci_probe(struct platform_device *pdev)
+static int __init fsl_pci_probe(struct platform_device *pdev)
  {
int ret;
-   struct device_node *node;
+   struct fsl_pci *pci;
+
+   if (!of_device_is_available(pdev-dev.of_node)) {
+   dev_warn(pdev-dev, disabled\n);
+   return -ENODEV;
+   }

This should be dev_dbg().

[Minghuan] ok.

-#ifdef CONFIG_PM
-static int fsl_pci_resume(struct device *dev)
+static int __exit fsl_pci_remove(struct platform_device *pdev)

Why __exit?  What happens if someone unbinds the PCI controller via
sysfs?
  

[Minghuan] Sorry. should remove __exit

+/*
+ * Structure of a PCI controller (host bridge)
+ */
+struct fsl_pci {
+   struct list_head node;
+   int is_pcie;

bool is_pcie;

[Minghuan] ok.

+/* Return link status 0- link, 1- no link */
+int fsl_pci_check_link(struct fsl_pci *pci);

bool

[Minghuan] ok.

+
+/*
+ * The fsl_arch_* functions are arch hooks. Those functions are
+ * implemented as weak symbols so that they can be overridden by
+ * architecture specific code if needed.
+ */
+
+/* Return PCI64 DMA offset */
+u64 fsl_arch_pci64_dma_offset(void);

Is this always guaranteed to exist?

[Minghuan] Yes. I define a __weak implementation in pci-fsl.c

+/* Register PCI/PCIe controller to architecture system */
+int __weak fsl_arch_pci_sys_register(struct fsl_pci *pci);
+
+/* Remove PCI/PCIe controller from architecture system */
+void __weak fsl_arch_pci_sys_remove(struct fsl_pci *pci);

Why do these need to be weak?  Won't there be exactly one implementation
per supported arch?
[Minghuan] I added __weak for compiling kernel when selecting pci-fsl 
module but

there is no related arch pci implementation.
I can remove the __weak, and use
depends on FSL_SOC_BOOKE || PPC_86xx in Kconfig to make sure there is 
one implementation

of supported arch.

-Scott





___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/2] pci: fsl: rework PCI driver compatible with Layerscape

2013-09-29 Thread Timur Tabi 
On Wed, Sep 18, 2013 at 6:02 AM, Minghuan Lian
minghuan.l...@freescale.com wrote:

 diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
 index 38f3b7e..6fd6348 100644
 --- a/arch/powerpc/Kconfig
 +++ b/arch/powerpc/Kconfig
 @@ -690,6 +690,7 @@ config FSL_SOC

  config FSL_PCI
 bool
 +   select PCI_FSL if FSL_SOC_BOOKE || PPC_86xx

I think having two config options, one called FSL_PCI and the other
PCI_FSL, is very confusing.  Surely, you can pick better names than
that?
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/2] pci: fsl: rework PCI driver compatible with Layerscape

2013-09-29 Thread Lian Minghuan-b31939 

Hi Timur,

Thanks for your comments.

How about PCI_FSL_COMMON?

Thanks,
Minghuan

On 09/30/2013 07:56 AM, Timur Tabi wrote:

On Wed, Sep 18, 2013 at 6:02 AM, Minghuan Lian
minghuan.l...@freescale.com wrote:


diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 38f3b7e..6fd6348 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -690,6 +690,7 @@ config FSL_SOC

  config FSL_PCI
 bool
+   select PCI_FSL if FSL_SOC_BOOKE || PPC_86xx

I think having two config options, one called FSL_PCI and the other
PCI_FSL, is very confusing.  Surely, you can pick better names than
that?




___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/2] pci: fsl: rework PCI driver compatible with Layerscape

2013-09-27 Thread Lian Minghuan-b31939 

Hi All,

Can anyone comment on my code or help to pick up?

Thanks,
Minghuan


On 09/18/2013 07:02 PM, Minghuan Lian wrote:

The Freescale's Layerscape series processors will use the same PCI
controller but change cores from PowerPC to ARM. This patch is to
rework FSL PCI driver to support PowerPC and ARM simultaneously.
PowerPC uses structure pci_controller to describe PCI controller,
but arm uses structure hw_pci and pci_sys_data. They also have
different architecture implementation and initialization flow.
The architecture-dependent driver will bridge the gap, get the
settings from the common driver and initialize the corresponding
structure and call the related interface to register PCI controller.
The common driver pci-fsl.c removes all the architecture-specific
code and provides structure fsl_pci to store all the controller
settings and the common functionalities that include reading/writing
PCI configuration space, parsing dts node and getting the MEM/IO and
bus number ranges, setting ATMU and check link status.

Signed-off-by: Minghuan Lian minghuan.l...@freescale.com
---
Based on upstream master
Based on the discussion of RFC version here
http://patchwork.ozlabs.org/patch/274488/
The function has been tested on MPC8315ERDB MPC8572DS P5020DS P3041DS
and T4240QDS boards

  arch/powerpc/Kconfig  |   1 +
  arch/powerpc/sysdev/fsl_pci.c | 147 +-
  drivers/edac/mpc85xx_edac.c   |  16 +-
  drivers/pci/host/Kconfig  |   7 +
  drivers/pci/host/Makefile |   1 +
  drivers/pci/host/pci-fsl.c| 653 +++---
  include/linux/fsl/pci.h   |  69 +
  7 files changed, 653 insertions(+), 241 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 38f3b7e..6fd6348 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -690,6 +690,7 @@ config FSL_SOC
  
  config FSL_PCI

bool
+   select PCI_FSL if FSL_SOC_BOOKE || PPC_86xx
select PPC_INDIRECT_PCI
select PCI_QUIRKS
  
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c

index a189ff0..1413257 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -62,7 +62,11 @@ static void quirk_fsl_pcie_header(struct pci_dev *dev)
  #if defined(CONFIG_FSL_SOC_BOOKE) || defined(CONFIG_PPC_86xx)
  
  #define MAX_PHYS_ADDR_BITS	40

-static u64 pci64_dma_offset = 1ull  MAX_PHYS_ADDR_BITS;
+
+u64 fsl_arch_pci64_dma_offset(void)
+{
+   return 1ull  MAX_PHYS_ADDR_BITS;
+}
  
  static int fsl_pci_dma_set_mask(struct device *dev, u64 dma_mask)

  {
@@ -77,17 +81,43 @@ static int fsl_pci_dma_set_mask(struct device *dev, u64 
dma_mask)
if ((dev-bus == pci_bus_type) 
dma_mask = DMA_BIT_MASK(MAX_PHYS_ADDR_BITS)) {
set_dma_ops(dev, dma_direct_ops);
-   set_dma_offset(dev, pci64_dma_offset);
+   set_dma_offset(dev, fsl_arch_pci64_dma_offset());
}
  
  	*dev-dma_mask = dma_mask;

return 0;
  }
  
+struct fsl_pci *fsl_arch_sys_to_pci(void *sys)

+{
+   struct pci_controller *hose = sys;
+   struct fsl_pci *pci = hose-private_data;
+
+   /* Update the first bus number */
+   if (pci-first_busno != hose-first_busno)
+   pci-first_busno = hose-first_busno;
+
+   return pci;
+}
+
+struct pci_bus *fsl_arch_fake_pci_bus(struct fsl_pci *pci, int busnr)
+{
+   static struct pci_bus bus;
+   static struct pci_controller hose;
+
+   bus.number = busnr;
+   bus.sysdata = hose;
+   hose.private_data = pci;
+   bus.ops = pci-ops;
+
+   return bus;
+}
+
  void fsl_pcibios_fixup_bus(struct pci_bus *bus)
  {
struct pci_controller *hose = pci_bus_to_host(bus);
-   int i, is_pcie = 0, no_link;
+   int i, is_pcie, no_link;
+   struct fsl_pci *pci = fsl_arch_sys_to_pci(hose);
  
  	/* The root complex bridge comes up with bogus resources,

 * we copy the PHB ones in.
@@ -97,9 +127,8 @@ void fsl_pcibios_fixup_bus(struct pci_bus *bus)
 * tricky.
 */
  
-	if (fsl_pcie_bus_fixup)

-   is_pcie = early_find_capability(hose, 0, 0, PCI_CAP_ID_EXP);
-   no_link = !!(hose-indirect_type  PPC_INDIRECT_TYPE_NO_PCIE_LINK);
+   is_pcie = pci-is_pcie;
+   no_link = fsl_pci_check_link(pci);
  
  	if (bus-parent == hose-bus  (is_pcie || no_link)) {

for (i = 0; i  PCI_BRIDGE_RESOURCE_NUM; ++i) {
@@ -121,6 +150,94 @@ void fsl_pcibios_fixup_bus(struct pci_bus *bus)
}
  }
  
+int fsl_arch_pci_exclude_device(struct fsl_pci *pci, u8 bus, u8 devfn)

+{
+   struct pci_controller *hose = pci-sys;
+
+   if (!hose)
+   return PCIBIOS_SUCCESSFUL;
+
+   if (ppc_md.pci_exclude_device)
+   if (ppc_md.pci_exclude_device(hose, bus, devfn))
+   return PCIBIOS_DEVICE_NOT_FOUND;
+
+   return PCIBIOS_SUCCESSFUL;
+}
+
+int fsl_arch_pci_sys_register(struct fsl_pci *pci)
+{
+   struct

Re: [PATCH 2/2] pci: fsl: rework PCI driver compatible with Layerscape

2013-09-27 Thread Scott Wood 
On Fri, 2013-09-27 at 19:29 +0800, Lian Minghuan-b31939 wrote:
 Hi All,
 
 Can anyone comment on my code or help to pick up?

Please break it up into multiple patches, each with one logical change
that is individually explained.  It will be much easier to review that
way.

-Scott



___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/2] pci: fsl: rework PCI driver compatible with Layerscape

2013-09-27 Thread Scott Wood 
On Wed, 2013-09-18 at 19:02 +0800, Minghuan Lian wrote:
 @@ -592,6 +719,7 @@ int fsl_pci_mcheck_exception(struct pt_regs *regs)
  #if defined(CONFIG_FSL_SOC_BOOKE) || defined(CONFIG_PPC_86xx)
  
  struct device_node *fsl_pci_primary;
 +extern const struct of_device_id fsl_pci_ids[];

Externs go in headers.

 -static int fsl_pci_probe(struct platform_device *pdev)
 +static int __init fsl_pci_probe(struct platform_device *pdev)
  {
   int ret;
 - struct device_node *node;
 + struct fsl_pci *pci;
 +
 + if (!of_device_is_available(pdev-dev.of_node)) {
 + dev_warn(pdev-dev, disabled\n);
 + return -ENODEV;
 + }

This should be dev_dbg().

 -#ifdef CONFIG_PM
 -static int fsl_pci_resume(struct device *dev)
 +static int __exit fsl_pci_remove(struct platform_device *pdev)

Why __exit?  What happens if someone unbinds the PCI controller via
sysfs?
 
 +/*
 + * Structure of a PCI controller (host bridge)
 + */
 +struct fsl_pci {
 + struct list_head node;
 + int is_pcie;

bool is_pcie;

 +/* Return link status 0- link, 1- no link */
 +int fsl_pci_check_link(struct fsl_pci *pci);

bool

 +
 +/*
 + * The fsl_arch_* functions are arch hooks. Those functions are
 + * implemented as weak symbols so that they can be overridden by
 + * architecture specific code if needed.
 + */
 +
 +/* Return PCI64 DMA offset */
 +u64 fsl_arch_pci64_dma_offset(void);

Is this always guaranteed to exist?

 +/* Register PCI/PCIe controller to architecture system */
 +int __weak fsl_arch_pci_sys_register(struct fsl_pci *pci);
 +
 +/* Remove PCI/PCIe controller from architecture system */
 +void __weak fsl_arch_pci_sys_remove(struct fsl_pci *pci);

Why do these need to be weak?  Won't there be exactly one implementation
per supported arch?

-Scott



___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev