Re: [PATCH 24/32] pci: PCIe driver for Marvell Armada 370/XP systems

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday 12 February 2013, Thomas Petazzoni wrote:
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> new file mode 100644
> index 0000000..3ad563f
> --- /dev/null
> +++ b/drivers/pci/host/Makefile
> @@ -0,0 +1,4 @@
> +obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
> +CFLAGS_pci-mvebu.o += \
> +	-I$(srctree)/arch/arm/plat-orion/include \
> +	-I$(srctree)/arch/arm/mach-mvebu/include

This does not seem like a good idea to me. We should not include
architecture specific directories from a driver directory.

What are the header files you need here? 

> +/*
> + * This product ID is registered by Marvell, and used when the Marvell
> + * SoC is not the root complex, but an endpoint on the PCIe bus. It is
> + * therefore safe to re-use this PCI ID for our emulated PCI-to-PCI
> + * bridge.
> + */
> +#define MARVELL_EMULATED_PCI_PCI_BRIDGE_ID 0x7846

Just a side note: What happens if you have two of these systems and connect
them over PCIe, putting one of them into host mode and the other into
endpoint mode?

> +static void mvebu_pcie_setup_io_window(struct mvebu_pcie_port *port,
> +				       int enable)
> +{
> +	unsigned long iobase, iolimit;
> +
> +	if (port->bridge.iolimit < port->bridge.iobase)
> +		return;
> +
> +	iolimit = 0xFFF | ((port->bridge.iolimit & 0xF0) << 8) |
> +		(port->bridge.iolimitupper << 16);
> +	iobase = ((port->bridge.iobase & 0xF0) << 8) |
> +		(port->bridge.iobaseupper << 16);

I don't understand this code with the masks and shifts. Could you
add a comment here for readers like me?

> +
> +/*
> + * Initialize the configuration space of the PCI-to-PCI bridge
> + * associated with the given PCIe interface.
> + */
> +static void mvebu_sw_pci_bridge_init(struct mvebu_pcie_port *port)
> +{

As mentioned, I'm still skeptical of the sw_pci_bridge approach,
so I'm not commenting on the details of your implementations
(they seem fine on a first look though)

> +	/* Get the I/O and memory ranges from DT */
> +	while ((range = of_pci_process_ranges(np, &res, range)) != NULL) {
> +		if (resource_type(&res) == IORESOURCE_IO) {
> +			memcpy(&pcie->io, &res, sizeof(res));
> +			memcpy(&pcie->realio, &res, sizeof(res));
> +			pcie->io.name = "I/O";
> +			pcie->realio.start &= 0xFFFFF;
> +			pcie->realio.end   &= 0xFFFFF;
> +		}

The bit masking seems fishy here. What exactly are you doing,
does this just assume you have a 1MB window at most?

Maybe something like

	pcie->realio.start = 0;
	pcie->realio.end = pcie->io.end - pcie->io.start;

I suppose you also need to fix up pcie->io to be in IORESOURCE_MEM
space instead of IORESOURCE_IO, or fix the of_pci_process_ranges
function to return it in a different way.

> +static int mvebu_pcie_init(void)
> +{
> +	return platform_driver_probe(&mvebu_pcie_driver,
> +				     mvebu_pcie_probe);
> +}
> +
> +subsys_initcall(mvebu_pcie_init);

You don't have to do it, but I wonder if this could be a module
with unload support instead.

	Arnd