Re: [02/12,v3] pci: fsl: add structure fsl_pci

2014-01-07 Thread Scott Wood
On Mon, 2014-01-06 at 14:10 +0800, Lian Minghuan-b31939 wrote:
 On 01/04/2014 06:19 AM, Scott Wood wrote:
  I don't like the extent to which this duplicates (not moves) PPC's struct
  pci_controller.  Also this leaves some fields like indirect_type
  unexplained (PPC_INDIRECT_TYPE_xxx is only in the PPC header).
 
  Does the arch-independent part of the driver really need all this?  Given
  how closely this tracks the PPC code, how would this work on ARM?
 [Minghuan] I added the duplicate fields because PPC's struct 
 pci_controller need them.

I think a better approach would be to create a cleanly architected
arch-independent driver.  Share what you reasonably can with the current
fsl_pci.c, but not to the extent of propagating PPCisms that don't match
up with what we ultimately want to see in generic code, or copying
things that ought to be controller-independent infrastructure into
controller-specific code.

See these threads:
http://www.spinics.net/lists/linux-pci/msg25769.html
https://lkml.org/lkml/2013/5/4/103

 The following is for ARM, I will submit them after verification:
[snip]
 +static int fsl_pcie_register(struct fsl_pcie *pcie)
 +{
 +pcie-controller = fsl_hw_pcie.nr_controllers;
 +fsl_hw_pcie.nr_controllers = 1;
 +fsl_hw_pcie.private_data = (void **)pcie;

I believe this should be:
fsl_hw_pcie.private_data = pcie;

 +pci_common_init(fsl_hw_pcie);
 +pci_assign_unassigned_resources();
 +#ifdef CONFIG_PCI_DOMAINS
 +fsl_hw_pcie.domain++;
 +#endif

What serializes that non-atomic increment?

-Scott


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


Re: [02/12,v3] pci: fsl: add structure fsl_pci

2014-01-05 Thread Lian Minghuan-b31939

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

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

PowerPC uses structure pci_controller to describe PCI controller,
but ARM uses structure pci_sys_data. In order to support PowerPC
and ARM simultaneously, the patch adds a structure fsl_pci that
contains most of the members of the pci_controller and pci_sys_data.
Meanwhile, it defines a interface fsl_arch_sys_to_pci() which should
be implemented in architecture-specific PCI controller driver to
convert pci_controller or pci_sys_data to fsl_pci.

Signed-off-by: Minghuan Lian minghuan.l...@freescale.com

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

  include/linux/fsl/pci-common.h | 41 +
  1 file changed, 41 insertions(+)

diff --git a/include/linux/fsl/pci-common.h b/include/linux/fsl/pci-common.h
index 5e4f683..e56a040 100644
--- a/include/linux/fsl/pci-common.h
+++ b/include/linux/fsl/pci-common.h
@@ -102,5 +102,46 @@ struct ccsr_pci {
  
  };
  
+/*

+ * Structure of a PCI controller (host bridge)
+ */
+struct fsl_pci {
+   struct list_head node;
+   bool is_pcie;
+   struct device_node *dn;
+   struct device *dev;
+
+   int first_busno;
+   int last_busno;
+   int self_busno;
+   struct resource busn;
+
+   struct pci_ops *ops;
+   struct ccsr_pci __iomem *regs;
+
+   u32 indirect_type;
+
+   struct resource io_resource;
+   resource_size_t io_base_phys;
+   resource_size_t pci_io_size;
+
+   struct resource mem_resources[3];
+   resource_size_t mem_offset[3];
+
+   int global_number;  /* PCI domain number */
+
+   resource_size_t dma_window_base_cur;
+   resource_size_t dma_window_size;
+
+   void *sys;
+};

I don't like the extent to which this duplicates (not moves) PPC's struct
pci_controller.  Also this leaves some fields like indirect_type
unexplained (PPC_INDIRECT_TYPE_xxx is only in the PPC header).

Does the arch-independent part of the driver really need all this?  Given
how closely this tracks the PPC code, how would this work on ARM?
[Minghuan] I added the duplicate fields because PPC's struct 
pci_controller need them.
The common PCI driver gets the related information and pass to PowerPC 
driver.
And I do hope PowerPC driver to parse dts or access controller to get 
the information again.

please see the following code for PowerPC:
int fsl_arch_pci_sys_register(struct fsl_pci *pci)
+{
+struct pci_controller *hose;

+hose = pcibios_alloc_controller(pci-dn);
+
+hose-private_data = pci;
+hose-parent = pci-dev;
+hose-first_busno = pci-first_busno;
+hose-last_busno = pci-last_busno;
+hose-ops = pci-ops;
+
+hose-io_base_virt = ioremap(pci-io_base_phys + 
pci-io_resource.start,

+ pci-pci_io_size);
+hose-pci_io_size = pci-io_resource.start + pci-pci_io_size;
+hose-io_base_phys = pci-io_base_phys;
+hose-io_resource = pci-io_resource;
+
+memcpy(hose-mem_offset, pci-mem_offset, sizeof(hose-mem_offset));
+memcpy(hose-mem_resources, pci-mem_resources,
+sizeof(hose-mem_resources));
+hose-dma_window_base_cur = pci-dma_window_base_cur;
+hose-dma_window_size = pci-dma_window_size;
+pci-sys = hose;
+
+return 0;
+}



The following is for ARM, I will submit them after verification:

+
+static inline struct fsl_pcie *sys_to_pcie(struct pci_sys_data *sys)
+{
+return sys-private_data;
+}
+
+static int fsl_pcie_setup(int nr, struct pci_sys_data *sys)
+{
+struct fsl_pcie *pcie;
+
+pcie = sys_to_pcie(sys);
+
+if (!pcie)
+return 0;
+
+pcie-sys = sys;
+
+sys-io_offset = pcie-io_base_phys;
+pci_ioremap_io(sys-io_offset, pcie-io_resource.start);
+pci_add_resource_offset(sys-resources, pcie-io_resource,
+sys-io_offset);
+
+sys-mem_offset = pcie-mem_offset;
+pci_add_resource_offset(sys-resources, pcie-mem_resource,
+sys-mem_offset);
+
+return 1;
+}
+
+static struct pci_bus *
+fsl_pcie_scan_bus(int nr, struct pci_sys_data *sys)
+{
+struct pci_bus *bus;
+struct fsl_pcie *pcie = sys_to_pcie(sys);
+
+bus = pci_create_root_bus(pcie-dev, sys-busnr,
+  pcie-ops, sys, sys-resources);
+if (!bus)
+return NULL;
+
+pci_scan_child_bus(bus);
+
+return bus;
+}
+
+static int fsl_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
+{
+struct of_irq oirq;
+int ret;
+
+ret = of_irq_map_pci(dev, oirq);
+if (ret)
+return ret;
+
+return irq_create_of_mapping(oirq.controller, oirq.specifier,
+ oirq.size);
+}
+
+static struct hw_pci fsl_hw_pcie = {
+.ops= fsl_indirect_pci_ops;
+.setup= fsl_pcie_setup,
+.scan= fsl_pcie_scan_bus,
+.map_irq= 

Re: [02/12,v3] pci: fsl: add structure fsl_pci

2014-01-03 Thread Scott Wood
On Wed, Oct 23, 2013 at 06:41:24PM +0800, Minghuan Lian wrote:
 PowerPC uses structure pci_controller to describe PCI controller,
 but ARM uses structure pci_sys_data. In order to support PowerPC
 and ARM simultaneously, the patch adds a structure fsl_pci that
 contains most of the members of the pci_controller and pci_sys_data.
 Meanwhile, it defines a interface fsl_arch_sys_to_pci() which should
 be implemented in architecture-specific PCI controller driver to
 convert pci_controller or pci_sys_data to fsl_pci.
 
 Signed-off-by: Minghuan Lian minghuan.l...@freescale.com
 
 ---
 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/
 
  include/linux/fsl/pci-common.h | 41 +
  1 file changed, 41 insertions(+)
 
 diff --git a/include/linux/fsl/pci-common.h b/include/linux/fsl/pci-common.h
 index 5e4f683..e56a040 100644
 --- a/include/linux/fsl/pci-common.h
 +++ b/include/linux/fsl/pci-common.h
 @@ -102,5 +102,46 @@ struct ccsr_pci {
  
  };
  
 +/*
 + * Structure of a PCI controller (host bridge)
 + */
 +struct fsl_pci {
 + struct list_head node;
 + bool is_pcie;
 + struct device_node *dn;
 + struct device *dev;
 +
 + int first_busno;
 + int last_busno;
 + int self_busno;
 + struct resource busn;
 +
 + struct pci_ops *ops;
 + struct ccsr_pci __iomem *regs;
 +
 + u32 indirect_type;
 +
 + struct resource io_resource;
 + resource_size_t io_base_phys;
 + resource_size_t pci_io_size;
 +
 + struct resource mem_resources[3];
 + resource_size_t mem_offset[3];
 +
 + int global_number;  /* PCI domain number */
 +
 + resource_size_t dma_window_base_cur;
 + resource_size_t dma_window_size;
 +
 + void *sys;
 +};

I don't like the extent to which this duplicates (not moves) PPC's struct
pci_controller.  Also this leaves some fields like indirect_type
unexplained (PPC_INDIRECT_TYPE_xxx is only in the PPC header).

Does the arch-independent part of the driver really need all this?  Given
how closely this tracks the PPC code, how would this work on ARM?

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