Re: [4/5] powerpc: PA Semi PWRficient platform support

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

Overall, looks good, some bits  tho:

> +static int pa_pxp_read_config(struct pci_bus *bus, unsigned int devfn,
> +			      int offset, int len, u32 *val)
> +{
> +	struct pci_controller *hose;
> +	unsigned long addr;

 .../...

You don't seem to encode support for extended config space (offsets >
256). If it's PCIe, you should have it (flush the 4 top bits of the
offste in the 4 top bits of the cfg address in general). In any case,
you should check the passed-in offset and error out if it is above your
max offset (256 or 4096 depending if you implement extended config space
support) or the kernel might get confused.

> +	of_prop = alloc_bootmem(sizeof(struct property) +
> +				sizeof(hose->global_number));
> +	if (of_prop) {
> +		memset(of_prop, 0, sizeof(struct property));
> +		of_prop->name = "linux,pci-domain";
> +		of_prop->length = sizeof(hose->global_number);
> +		of_prop->value = (unsigned char *)&of_prop[1];
> +		memcpy(of_prop->value, &hose->global_number, sizeof(hose->global_number));
> +		prom_add_property(dev, of_prop);
> +	}

Just kill that... nobody cares about that property.

> +	if (device_is_compatible(dev, "pa-pxp"))
> +		setup_pa_pxp(hose);

And if not ? You have a non-initialized hose without config space access
methods ? Hrm... 

> +	printk(KERN_INFO "Found PA-PXP PCI host bridge. Firmware bus number: %d->%d\n",
> +		hose->first_busno, hose->last_busno);
> +
> +	/* Interpret the "ranges" property */
> +	/* This also maps the I/O region and sets isa_io/mem_base */
> +	pci_process_bridge_OF_ranges(hose, dev, 1);
> +	pci_setup_phb_io(hose, 1);
> +
> +	return 0;
> +}
> +
> +
> +void __init pas_pcibios_fixup(void)
> +{
> +	struct pci_dev *dev = NULL;
> +
> +	for_each_pci_dev(dev)
> +		pci_read_irq_line(dev);
> +}
> +
> +static void __init pas_fixup_phb_resources(void)
> +{
> +	struct pci_controller *hose, *tmp;
> +
> +	list_for_each_entry_safe(hose, tmp, &hose_list, list_node) {
> +		unsigned long offset = (unsigned long)hose->io_base_virt - pci_io_base;
> +		hose->io_resource.start += offset;
> +		hose->io_resource.end += offset;
> +		printk(KERN_INFO "PCI Host %d, io start: %lx; io end: %lx\n",
> +		       hose->global_number,
> +		       hose->io_resource.start, hose->io_resource.end);
> +	}
> +}

There is no generic code for the above ? (I have to double check). If
not, we shall fix that ;)

.../...

> +#ifdef CONFIG_SMP
> +struct smp_ops_t pas_smp_ops = {
> +	.probe		= smp_mpic_probe,
> +	.message_pass	= smp_mpic_message_pass,
> +	.kick_cpu	= smp_generic_kick_cpu,
> +	.setup_cpu	= smp_mpic_setup_cpu,
> +	.give_timebase	= smp_generic_give_timebase,
> +	.take_timebase	= smp_generic_take_timebase,
> +};

You don't have a HW timebase sync facility ? Is this the board or the
processor doesn't have a pin to freeze the timebase ? Either way, that
is bad ! Time to use the cluebat !

> +#endif /* CONFIG_SMP */
> +
> +void __init pas_setup_arch(void)
> +{
> +	/* init to some ~sane value until calibrate_delay() runs */
> +	loops_per_jiffy = 50000000;

That value still makes any sense with the new tb based delays ? I know
we still have it here or there, I'm wondering tho...