Re: [PATCH 1/4] Add support for 750CL Holly board

[Arnd Bergmann <arnd@arndb.de>]

On Friday 04 May 2007, Josh Boyer wrote:
> +
> +#undef DEBUG
> +#ifdef DEBUG
> +#define DBG(fmt...) do { printk(fmt); } while(0)
> +#else
> +#define DBG(fmt...) do { } while(0)
> +#endif

Please replace this with the generic pr_debug()

> +#ifndef CONFIG_PCI
> +pci_dram_offset = PPC750GXCL_TSI_PCI_MEM_OFFSET;
> +#endif

This looks like the compile would expect a type.

> +extern int tsi108_setup_pci(struct device_node *dev);
> +extern void tsi108_pci_int_init(struct device_node *node);
> +extern void tsi108_irq_cascade(unsigned int irq, struct irq_desc *desc);

please move any extern declarations into a header file that
is included by both the file that defines and uses them

> +static void holly_remap_bridge(void)
> +{
> +	u32 lut_val, lut_addr = 0x900, misc_cfg;
> +	int i;
> +
> +	printk(KERN_ERR "Remapping PCI bridge\n");

Is it an error to remap the bridge? If not, KERN_INFO would be more
appropriate ;-)

> +void holly_show_cpuinfo(struct seq_file *m)
> +{
> +	seq_printf(m, "vendor\t\t: IBM\n");
> +	seq_printf(m, "machine\t\t: PPC750 GX/CL\n");
> +}

If it's an IBM product, it should come with a product code like 123-4567,
which fits in here, instead of just listing the CPU.

> +static int ppc750_machine_check_exception(struct pt_regs *regs)
> +{
> +	extern void tsi108_clear_pci_cfg_error(void);

move declaration to header file.

> +	const struct exception_table_entry *entry;
> +
> +	/* Are we prepared to handle this fault */
> +	if ((entry = search_exception_tables(regs->nip)) != NULL) {
> +		tsi108_clear_pci_cfg_error();
> +		regs->msr |= MSR_RI;
> +		regs->nip = entry->fixup;
> +		return 1;
> +	}
> +	return 0;
> +}

Are you sure that you can use the generic exception table mechanism
like this? I can't see why it doesn't work, but it's something I haven't
seen anyone do like this.

> --- linux-2.6.orig/drivers/net/tsi108_eth.h
> +++ linux-2.6/drivers/net/tsi108_eth.h
> @@ -49,7 +49,11 @@
>   */
>  #define PHY_MV88E	1	/* Marvel 88Exxxx PHY */
>  #define PHY_BCM54XX	2	/* Broardcom BCM54xx PHY */
> +#if defined(CONFIG_HOLLY)
> +#define TSI108_PHY_TYPE PHY_BCM54XX
> +#else
>  #define TSI108_PHY_TYPE	PHY_MV88E
> +#endif
>  

this breaks multiplatform setups.

> --- linux-2.6.orig/include/asm-powerpc/tsi108.h
> +++ linux-2.6/include/asm-powerpc/tsi108.h
> @@ -68,7 +68,11 @@
>  #define TSI108_PB_ERRCS_ES		(1 << 1)
>  #define TSI108_PB_ISR_PBS_RD_ERR	(1 << 8)
>  
> +#ifdef CONFIG_HOLLY
> +#define TSI108_PCI_CFG_BASE_PHYS	(0x7c000000)
> +#else
>  #define TSI108_PCI_CFG_BASE_PHYS	(0xfb000000)
> +#endif
>  #define TSI108_PCI_CFG_SIZE		(0x01000000)
>  /* Global variables */
> 

Same here.

	Arnd <><