Comments

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(+)

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html