[PATCH 11/19] powerpc: Supporting PCI bus and base of I/O for Celleb

[Benjamin Herrenschmidt]

On Thu, 2006-12-14 at 11:38 +0900, Ishizaki Kou wrote:
> This patch includes support for pci buses, base of Celleb specific
> devices, and etc. It works on of_platform bus.

There is still a few serious issues unfortunately with that PCI code...

> +static int celleb_epci_subordinate_read_config(struct pci_controller *hose,
> +					       int bus, unsigned int devfn,
> +					       int where, int size, u32 *val)
> +{
> +	unsigned long addr;
> +
> +	if (!hose->cfg_data)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	if (where == PCI_INTERRUPT_LINE) {
> +		struct celleb_epci_private *private = hose->private_data;
> +		if (private == NULL)
> +			return PCIBIOS_DEVICE_NOT_FOUND;
> +		*val = irq_create_mapping(NULL, private->epci_ext_irq);
> +		return PCIBIOS_SUCCESSFUL;
> +	} else if (where == PCI_INTERRUPT_PIN) {
> +		*val = 1;
> +		return PCIBIOS_SUCCESSFUL;
> +	}

Why the above ? You have 3 copies of the code here that does the IRQ
mapping inside of read_config, and I don't see this making any sense.

The IRQ mapping should be in the device-tree, in the form of an
interrupt-map property, or at -worse- as a fixup pass to setup
pci_dev->irq in ppc_md, not some weird code in the config space. Nobody
actually cares what is in PCI_INTERRUPT_LINE of a PCI device in fact.
 
> +	clear_and_disable_master_abort_interrupt(hose);
> +
> +	/* address for PCI configuration access */
> +	addr = (unsigned long)hose->cfg_data +
> +		(((bus & 0xff) << 16)
> +		 | ((devfn & 0xff) << 8)
> +		 | (where & 0xff)
> +		 | 0x01000000);
> +
> +	switch (size) {
> +	case 1:
> +		*val = in_8((u8 *)addr);
> +		break;
> +	case 2:
> +		*val = in_le16((u16 *)addr);
> +		break;
> +	case 4:
> +		*val = in_le32((u32 *)addr);
> +		break;
> +	default:
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	}
> +
> +	return celleb_epci_check_abort(hose, 0);
> +}

The above code is duplicated several times (+/- endian) and I don't see
why. You definitely do not need special functions that are 90% duplicate
to handle wether you are doing a type 0 or type 1 config access :-) Best
is you look at how other platforms do it, it's mostly a single if in the
first few lines of the function to calculate the cfg address.

The host bridge case must stay separate due to different endian, though
it should still shrink the size of the code significantly by moving the
IRQ bits elsewhere.

> +int __devinit celleb_setup_epci(struct device_node *node,
> +				struct pci_controller *hose)
> +{
> +	const unsigned long *li, *li2;
> +	unsigned int rlen;
> +	struct celleb_epci_private *private;
> +
> +
> +	pr_debug("PCI: celleb_setup_epci()\n");
> +
> +	li = get_property(node, "toshiba,reg", &rlen);

Why that ? Why not use a normal "reg" property with proper ranges etc...
in the device tree, and then here, use the standard
of_address_to_resource() or similar ?

We are getting critical short on time for the 2.6.20 merge window
unfortunately. I hope we might still be able to get in, though I can't
guarantee it at this point.

Cheers,
Ben.