Dear Stephen Warren, On Mon, 28 Jan 2013 15:21:45 -0700, Stephen Warren wrote: > > +Mandatory properties: > > +- compatible: must be "marvell,armada-370-xp-pcie" > > +- status: either "disabled" or "okay" > > status is a standard DT property; I certainly wouldn't expect its > presence to be mandatory (there's a defined default), nor would I expect > each device's binding to redefine this property. Ok. > > +- marvell,pcie-port: the physical PCIe port number > > Should the standardized cell-index property be used here instead? Or, > perhaps that property is deprecated/discouraged... The problem is that I need two identifiers, the pcie-port and pcie-lane, and it would be strange to have one referenced as cell-index, and the other one as marvell,pcie-lane, no? Unless of course we can put two numbers in the cell-index property, but a quick grep in Documentation/devicetree/bindings/ seems to indicate that all users of cell-index use it with a single identifier. Just tell me what to do here, I don't have a strong opinion on this. > > diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile > > > +obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o > > +ccflags-$(CONFIG_PCI_MVEBU) += \ > > + -I$(srctree)/arch/arm/plat-orion/include \ > > + -I$(srctree)/arch/arm/mach-mvebu/include > > That seems a little dangerous w.r.t. multi-platform zImage. Can the > required headers be moved out to somewhere more public to avoid this? Why is this dangerous for multi-platform zImage? For this specific driver only, some SoC-specific headers are used. I don't think it prevents another PCI driver (such as the Tegra one) from being built into the same kernel image, no? Also, this is kind of a temporary solution, because I can't fix all the problems in just the PCI patch series without making it horribly large. The reason why we need those include paths at the moment are: * For plat-orion/include, because of pcie.h that provides functions common to all Marvell SoCs regarding PCI. Ultimately, all the Marvell SoCs that use this common code should be migrated over to the DT-capable PCI driver that is submitted through this patch series. This will take a bit of time, and is too complex to do in one shot, together with the introduction of the driver itself. So ultimately, all the code in plat-orion/pcie.c will migrate into drivers/pci/host/pci-mvebu.c, and this plat-orion/include/plat/pcie.h file should disappear, removing the need for this special header path. * For mach-mvebu/include, because of the addr-map.h header that provides functions related to address decoding windows. This is also likely to evolve quite significantly when we'll make the PCI driver being used by the other Marvell SoC families (Kirkwood, Dove, Orion5x), and when this work is done, we can think of having a public header in include/linux that exposes the address decoding APIs, once it has stabilized a bit across the different Marvell SoC families. So, the bottom line is: yes I know those include paths are not nice, but I don't think they prevent multiplatform builds, and they are a temporary solution until we convert more Marvell SoC families to this new PCI driver. > > +/* > > + * Those are the product IDs used for the emulated PCI Host bridge and > > + * emulated PCI-to-PCI bridges. They are temporary until we get > > + * official IDs assigned. > > + */ > > +#define MARVELL_EMULATED_HOST_BRIDGE_ID 4141 > > +#define MARVELL_EMULATED_PCI_PCI_BRIDGE_ID 4242 > > I assume that means we can't merge this driver yet. The cover letter > mentioned a desire to merge this for 3.9; there's not much time to get > official IDs assigned, then. I am working on getting real IDs assigned. For now, I'd like to work on getting all other issues fixed, and have this only problem remaining. > > > +static int mvebu_pcie_init(void) > > +{ > > + return platform_driver_probe(&mvebu_pcie_driver, > > + mvebu_pcie_probe); > > +} > > + > > +subsys_initcall(mvebu_pcie_init); > > Why isn't that just platform_driver_register()? I didn't test recently, but with my first version of the patch set, having an initialization as late as module_init() was too late. Some PCI fixup code was being executed *before* we get the opportunity of initializing the PCI driver, and it was crashing the kernel. I can provide more details if you want. > > +MODULE_LICENSE("GPL"); > > "GPL v2". Sure. Thomas -- Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html