Re: [patch 2/7] xenon: add platform support

[Arnd Bergmann <arnd@arndb.de>]

> +#define PRIO_IPI_4       0x08
> +#define PRIO_IPI_3       0x10
> +#define PRIO_SMM         0x14
> +#define PRIO_SFCX        0x18
> +#define PRIO_SATA_HDD    0x20
> +#define PRIO_SATA_CDROM  0x24
> +#define PRIO_OHCI_0      0x2c
> +#define PRIO_EHCI_0      0x30
> +#define PRIO_OHCI_1      0x34
> +#define PRIO_EHCI_1      0x38
> +#define PRIO_XMA         0x40
> +#define PRIO_AUDIO       0x44
> +#define PRIO_ENET        0x4C
> +#define PRIO_XPS         0x54
> +#define PRIO_GRAPHICS    0x58
> +#define PRIO_PROFILER    0x60
> +#define PRIO_BIU         0x64
> +#define PRIO_IOC         0x68
> +#define PRIO_FSB         0x6c
> +#define PRIO_IPI_2       0x70
> +#define PRIO_CLOCK       0x74
> +#define PRIO_IPI_1       0x78

These should probably all come from the device tree
instead of compile-time constants.

> +void __init xenon_iic_init_IRQ(void)
> +{
> +	int i;
> +	struct device_node *dn;
> +
> +	printk("XENON init IRQ\n");

needs a printk level, KERN_DEBUG or KERN_INFO.

> +			/* search for our interrupt controller inside the device tree */
> +	for (dn = NULL;
> +	     (dn = of_find_node_by_name(dn,"interrupt-controller")) != NULL;) {
> +		if (!device_is_compatible(dn,
> +				     "xenon"))
> +			continue;
> +
> +		irq_set_virq_count(0x80);
> +		iic_base = ioremap_nocache(0x20000050000, 0x10000);

This should come from the 'reg' property of the above node.


> +void xenon_cause_IPI(int target, int msg)
> +{
> +	int ipi_prio;
> +
> +	ipi_prio = ipi_to_prio(msg);
> +
> +	__raw_writeq( (0x10000<<target) | ipi_prio, iic_base + 0x10 +
> hard_smp_processor_id() * 0x1000); +}
> +

You seem to have a line wrapping problem and might need to fix your
mail client.
Also, don't use __raw_writeq here. In order to access on-chip data,
use out_be64().

> +	hose->ops = &xenon_pci_ops;
> +	hose->cfg_addr = ioremap(0xd0000000, 0x1000000);
> +
The addresses should come from the device tree, don't hardcode
them in the source.

> +#define DEBUG
> +

really?


> +void __init xenon_pci_init(void);
> +#ifdef CONFIG_SMP
> +extern void smp_init_xenon(void);
> +#endif

move these declarations to a header

> +static void xenon_show_cpuinfo(struct seq_file *m)
> +{
> +	struct device_node *root;
> +	const char *model = "";
> +
> +	root = of_find_node_by_path("/");
> +	if (root)
> +		model = get_property(root, "model", NULL);
> +	seq_printf(m, "machine\t\t: CHRP %s\n", model);
> +	of_node_put(root);
> +}

CHRP???

> +static void __init xenon_pcibios_fixup(void)
> +{
> +	struct pci_dev *dev = NULL;
> +
> +	for_each_pci_dev(dev)
> +		pci_read_irq_line(dev);
> +}

I guess you copied that from cell? You should not need it.

> +	if (ROOT_DEV == 0) {
> +		printk("No ramdisk, default root is /dev/hda2\n");
> +		ROOT_DEV = Root_HDA2;
> +	}

kill this, it's unnecessary (and probably wrong in your case)

> +static int __init xenon_probe(void)
> +{
> +       hpte_init_native();
> +
> +       return 1;
> +}

This definitely needs to check the device tree whether it's running
on the right platform, otherwise you can not build a multiplatform
kernel.

> +void __init xenon_hpte_init(unsigned long htab_size);
> +
move this to a header file


> +#include <asm/rtas.h>

rtas???

> +#ifdef DEBUG
> +#define DBG(fmt...) printk(fmt)
> +#else
> +#define DBG(fmt...)
> +#endif
> +

Don't define your own macros like this, use the pr_debug()
one from kernel.h.

> +void smp_init_xenon(void);
> +
> +extern void xenon_request_IPIs(void);
> +extern void xenon_init_irq_on_cpu(int cpu);
> +
> +extern void xenon_cause_IPI(int target, int msg);

extern declarations should go to header files, not into
the implementation.


> +	/* Mark threads which are still spinning in hold loops. */
> +	if (cpu_has_feature(CPU_FTR_SMT)) {
> +		for_each_present_cpu(i) {
> +			if (i % 2 == 0)
> +				/*
> +				 * Even-numbered logical cpus correspond to
> +				 * primary threads.
> +				 */
> +				cpu_set(i, of_spin_map);
> +		}
> +	} else {
> +		of_spin_map = cpu_present_map;
> +	}

Yes, we wrote code like that for CBE, but that doesn't mean it's good
enough to copy it ;-)

Relying on the cpu number to mean something specific is bad, and
we're trying to get rid of that in other places, so you should
not introduce it again here.

> +#define CPU_FTRS_XENON ((CPU_FTR_SPLIT_ID_CACHE | CPU_FTR_USE_TB | \
> +	    CPU_FTR_HPTE_TABLE | CPU_FTR_PPCAS_ARCH_V2 | \
> +	    CPU_FTR_ALTIVEC_COMP | CPU_FTR_MMCRA | CPU_FTR_SMT | \
> +	    CPU_FTR_CTRL )&~CPU_FTR_16M_PAGE)
> +// | CPU_FTR_PAUSE_ZERO | CPU_FTR_CI_LARGE_PAGE  /* we need to setup large

If the CPU doesn't do 16M pages, you need to make sure that CPU_FTRS_ALWAYS
is adapted properly.