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

[Olof Johansson]

On Wed, 06 Sep 2006 07:31:49 +1000 Benjamin Herrenschmidt <benh at kernel.crashing.org> wrote:

> Overall, looks good, some bits  tho:

Thanks! See below.


> 
> > +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.

I'll take a look at this, thanks for pointing it out.

> > +	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.

Done

> > +	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... 

True. Just a leftover from forking off the maple setup code.

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

Didn't see anything, but then I didn't look that long. I'll follow up with a 
separate patch if it doesn't exist, since other platforms can use it too.

>
> > +#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 !

See comment to Arnd in another email: We have hardware support for it, just 
not including it at this time.

> 
> > +#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...

I haven't tried doing without it, I'll take a look.


-Olof