| Message ID | 1382524894-15164-2-git-send-email-Minghuan.Lian@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Scott Wood |
| Headers | show |
On Oct 23, 2013, at 5:41 AM, 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.Lian@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(+) NAK. We discussed this some at the ARM Summit this week and the feeling is we need to move to a common interface between the various ARCHs. - k > > 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; > +}; > + > +/* > + * Convert architecture specific pci controller structure to fsl_pci > + * PowerPC uses structure pci_controller and ARM uses structure pci_sys_data > + * to describe pci controller. > + */ > +extern struct fsl_pci *fsl_arch_sys_to_pci(void *sys); > + > #endif /* __PCI_COMMON_H */ > #endif /* __KERNEL__ */ > -- > 1.8.1.2 >
> -----Original Message----- > From: Kumar Gala [mailto:galak@kernel.crashing.org] > Sent: Wednesday, October 23, 2013 11:11 PM > To: Lian Minghuan-B31939 > Cc: linuxppc-dev@lists.ozlabs.org list; Arnd Bergmann; Zang Roy-R61911; > Wood Scott-B07421; Bjorn Helgaas; linux-pci@vger.kernel.org; linux-arm- > kernel@lists.infradead.org > Subject: Re: [PATCH 02/12][v3] pci: fsl: add structure fsl_pci > > > On Oct 23, 2013, at 5:41 AM, 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.Lian@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(+) > > NAK. > > We discussed this some at the ARM Summit this week and the feeling is we > need to move to a common interface between the various ARCHs. Can you share more about the *common interface*? Any plan to implement the * common interface*? Thanks. Roy
Hi Kumar, please see my comment inline. On 10/24/2013 12:11 PM, Kumar Gala wrote: > On Oct 23, 2013, at 5:41 AM, 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.Lian@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(+) > NAK. > > We discussed this some at the ARM Summit this week and the feeling is we need to move to a common interface between the various ARCHs. [Minghuan] Do you mean we will use the common interface instead of arch/powerpc/kernel/pci-common.c... and arch/arm/kernel/bios32.c? Who will do the code movement and when will the work be completed? The patches just move the common functions of FSL PCI controller operation which can be re-used by PowerPC and ARM. LS1 is coming, I worry about not having enough time to wait for the move is completed. > - k > >> 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; >> +}; >> + >> +/* >> + * Convert architecture specific pci controller structure to fsl_pci >> + * PowerPC uses structure pci_controller and ARM uses structure pci_sys_data >> + * to describe pci controller. >> + */ >> +extern struct fsl_pci *fsl_arch_sys_to_pci(void *sys); >> + >> #endif /* __PCI_COMMON_H */ >> #endif /* __KERNEL__ */ >> -- >> 1.8.1.2 >> >
On Fri, 2013-10-25 at 13:58 +0800, Lian Minghuan-b31939 wrote: > Hi Kumar, > > please see my comment inline. > > On 10/24/2013 12:11 PM, Kumar Gala wrote: > > On Oct 23, 2013, at 5:41 AM, 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.Lian@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(+) > > NAK. > > > > We discussed this some at the ARM Summit this week and the feeling is we need to move to a common interface between the various ARCHs. > [Minghuan] Do you mean we will use the common interface instead of > arch/powerpc/kernel/pci-common.c... > and arch/arm/kernel/bios32.c? Who will do the code movement and when > will the work be completed? The patches just move the common functions > of FSL PCI controller operation which can be re-used by PowerPC and ARM. > LS1 is coming, I worry about not having enough time to wait for the move > is completed. I agree -- it can take quite a while to get from "the feeling is we need to move to a common interface" to actually having something we can use. If and when this unification is achieved, we can drop this extra layer. It's a better interim solution than just duplicating the entire driver and letting them drift apart. -Scott
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.Lian@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
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.Lian@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 = fsl_pcie_map_irq, +}; +static struct pci_bus * +fake_pci_bus(struct fsl_pcie *pcie, int busnr) +{ + static struct pci_bus bus; + static struct pci_sys_data sys; + + bus.number = busnr; + bus.sysdata = &sys; + sys.private_data = pcie; + bus.ops = pcie->ops; + return &bus; +} + +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; + + pci_common_init(&fsl_hw_pcie); + pci_assign_unassigned_resources(); +#ifdef CONFIG_PCI_DOMAINS + fsl_hw_pcie.domain++; +#endif +} > > -Scott
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
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Friday, January 03, 2014 4:19 PM > To: Lian Minghuan-B31939 > Cc: linuxppc-dev@lists.ozlabs.org; linux-pci@vger.kernel.org; Zang Roy- > R61911; Bjorn Helgaas > Subject: Re: [02/12,v3] pci: fsl: add structure fsl_pci > > 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.Lian@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). INDIRECT type is for configuration space access, I do not think it is ppc header specific. It is good to put it to freescale pci common code. Roy
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; +}; + +/* + * Convert architecture specific pci controller structure to fsl_pci + * PowerPC uses structure pci_controller and ARM uses structure pci_sys_data + * to describe pci controller. + */ +extern struct fsl_pci *fsl_arch_sys_to_pci(void *sys); + #endif /* __PCI_COMMON_H */ #endif /* __KERNEL__ */
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.Lian@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(+)