[PATCH 3/3] 82xx: SBCPQ2 board platform support

[Arnd Bergmann]

On Monday 16 July 2007, Mark Zhan wrote:
> This patch addes the powerpc support to Wind River SBC PowerQUICCII 82xx board.

Hi Mark!

I've got lots of small comments about your code, but mostly it comes
down to one problem: your new platform code is not able to coexist
with other platforms because you hardcode information.

> +static struct resource m48t59_resources[] = {
> +	{
> +		.start	= SBCPQ2_RTC_BASE,
> +		.end	= SBCPQ2_RTC_BASE + SBCPQ2_RTC_SIZE - 1,
> +		.flags	= IORESOURCE_MEM,
> +	}, {
> +		.start	= SBCPQ2_M48T59_IRQ,
> +		.end	= SBCPQ2_M48T59_IRQ,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +	{ },
> +};

This is the kind of information that belongs into the device tree,
not hardcoded into the board code.

> +/**
> + * sbcpq2_pdev_init - Register the platform device for sbcpq2 board
> + */
> +static int __init sbcpq2_platdev_init(void)
> +{
> +	struct irq_desc *desc = irq_desc + SBCPQ2_M48T59_IRQ;

same for the interrupt number. Worse, this looks broken
because the descriptor array describes virtual interrupt
numbers, while SBCPQ2_M48T59_IRQ must be a physical number.
These are often the same, but there is no guarantee.

In order to get a virtual interrupt number for a given device,
you need to call irq_of_parse_and_map().

> +	/* Install a dummy irq chip for M48T59 RTC irq */
> +	if (desc->chip == &no_irq_chip)
> +		set_irq_handler(SBCPQ2_M48T59_IRQ, desc->handle_irq);
> +
> +	/* Register all platform devices for sbcpq2 */
> +	platform_add_devices(sbcpq2_devices, ARRAY_SIZE(sbcpq2_devices));
> +	return 0;
> +}
> +arch_initcall(sbcpq2_platdev_init);


> +/*
> + * For SBCPQ2 board, the interrupt of M48T59 RTC chip
> + * will generate a machine check exception. We use a
> + * fake irq to give the platform machine_check_exception() hook
> + * a chance to call the driver ISR. If IRQ_HANDLED is returned,
> + * then we will survive from the machine check exception.
> + */
> +static int sbcpq2_mach_check(struct pt_regs *regs)
> +{
> +	int recover = 0;
> +	struct irq_desc *desc = irq_desc + SBCPQ2_M48T59_IRQ;
> +
> +	struct irqaction *action = desc->action;
> +
> +	while (action && (action->dev_id != &m48t59_rtc))
> +		action = action->next;
> +
> +	/* Try to call m48t59 RTC driver ISR */
> +	if (action && action->handler)
> +		recover = action->handler(SBCPQ2_M48T59_IRQ, &m48t59_rtc);
> +
> +	return recover;
> +}

What you do here looks really scary, but maybe I'm just misunderstanding
it completely. Why don't you just register your rtc handler function
as the machine check handler instead of going through various indirections?

> +static void __init sbcpq2_init_IRQ(void)
> +{
> +	struct device_node *np;
> +	struct resource res;
> +
> +	np = of_find_compatible_node(NULL, "cpm-pic", "CPM2");
> +	if (np == NULL) {
> +		printk(KERN_ERR "PIC init: can not find cpm-pic node\n");
> +		return;
> +	}

This looks like your device tree is wrong. Shouldn't the interrupt
controller have device_type="interrupt-controller" and a specific
compatible property instead of having the name in the device_type?

> +static void __init sbcpq2_setup_arch(void)
> +{
> +	struct device_node *np;
> +	volatile memctl_cpm2_t *mc;

not volatile, but __iomem!

> +	unsigned char * eeprom_base;
> +	int i = 0;
> +
> +#ifdef CONFIG_CPM2
> +	cpm2_reset();
> +#endif
> +
> +	/*
> +	 * Make sure that we have the right CS# setting
> +	 */
> +	mc = &cpm2_immr->im_memctl;
> +
> +	/* Boot Flash is the on-board flash */
> +	mc->memc_br0 = (SBCPQ2_BOOT_FLASH_BASE & 0xFFFF8000) | 0x0801;
> +	mc->memc_or0 = 0xFFE00896;

consequently, this needs to use out_be32 or similar.
Where does SBCPQ2_BOOT_FLASH_BASE come from? Shouldn't that be set
up by the boot loader to match the device tree?

> +		model = (char *)of_get_property(np, "model", NULL);
> +		if (!model)
> +			continue;

The cast is not needed here.
> +
> +		id = of_get_property(np, "device-id", NULL);
> +		if (!id)
> +			continue;
> +
> +		macaddr = (unsigned char *)of_get_mac_address(np);
> +		if (!macaddr)
> +			continue;

or here.

> +		if (strstr(model, "FCC"))
> +			eeprom_ofs = SBCPQ2_FCC1_MACADDR_OFS;
> +		else if (strstr(model, "SCC"))
> +			eeprom_ofs = SBCPQ2_SCC1_MACADDR_OFS;
> +		eeprom_ofs += ((*id) - 1) * 6;

of_device_is_compatible()


> +		for (j = 0; j < 6; j++)
> +			*(macaddr + j) = *(eeprom_base + eeprom_ofs + j);

in_8().

> +	}
> +	iounmap(eeprom_base);
> +}
> +
> +/*
> + * Called very early, device-tree isn't unflattened
> + */
> +static int __init sbcpq2_probe(void)
> +{
> +	/* We always match for now, eventually we should look at
> +	 * the flat dev tree to ensure this is the board we are
> +	 * supposed to run on
> +	 */
> +	return 1;
> +}

Don't write why the code is wrong -- just fix it.

> +/* For our show_cpuinfo hooks. */
> +#define CPUINFO_VENDOR		"Wind River"
> +#define CPUINFO_MACHINE		"SBC PowerQUICCII 82xx"

Not in a header file please.

> +/*
> + * Wind River SBC PowerQUICCII 82xx Physical Memory Map (CS0 for OnBoard Flash)
> + *
> + *   0x00000000 - 0x07FFFFFF	CS2, 128 MB DIMM SDRAM
> + *   0x08000000 - 0x0FFFFFFF	CS3, 128 MB DIMM SDRAM
> + *   0x12000000 - 0x12100000	CS8, ATM
> + *   0x20000000 - 0x20FFFFFF	CS4, 16 MB Local Bus SDRAM
> + *   0x21000000 - 0x21001FFF	CS7, Control EPLD
> + *   0x22000000 - 0x22001FFF	CS5, 8KB EEPROM
> + *   0x22002000 - 0x22003FFF	CS5, visionPORT
> + *   0x22004000 - 0x22005FFF	CS5, User Switches
> + *   0x22006000 - 0x22007FFF	CS5, STATUS
> + *   0x22008000 - 0x22009FFF	CS5, i8259 interrupt controller
> + *   0x2200A000 - 0x2200BFFF	CS5, LED (Seven Segment Display)
> + *   0x80000000 - 0x80001FFF	CS11, RTC
> + *   0xE0000000 - 0xE3FFFFFF	CS6, 64 MB DIMM Flash
> + *   0xE4000000 - 0xE7FFFFFF	CS1, 64 MB DIMM Flash
> + *   0xFE000000 - 0xFFFFFFFF	CS0, 2 MB Boot Flash
> + *   0xF0000000 - 0xF0020000	MPC82xx Internal Registers Space
> + */
> +#define SBCPQ2_SDRAM_BASE		0x00000000
> +#define SBCPQ2_SDRAM_SIZE		0x10000000
> +
> +#define SBCPQ2_LOCAL_SDRAM_BASE		0x20000000
> +#define SBCPQ2_LOCAL_SDRAM_SIZE		0x1000000
> +
> +#define SBCPQ2_EPLD_BASE		0x21000000
> +#define SBCPQ2_EPLD_SIZE		0x2000
> +
> +#define SBCPQ2_EEPROM_BASE		0x22000000
> +#define SBCPQ2_EEPROM_SIZE		0x2000
> +
> +/* User Switches SW5 */
> +#define SBCPQ2_USER_SW_BASE		0x22004000
> +#define SBCPQ2_USER_SW_SIZE		0x2000
> +
> +#define SBCPQ2_STATUS_BASE		0x22006000
> +#define SBCPQ2_STATUS_SIZE		0x2000
> +
> +#define SBCPQ2_I8259_BASE		0x22008000
> +#define SBCPQ2_I8259_SIZE		0x2000
> +
> +/* Seven Segment Display LED D46 */
> +#define SBCPQ2_LED_BASE			0x2200A000
> +#define SBCPQ2_LED_SIZE			0x2000
> +
> +#define SBCPQ2_RTC_BASE			0x80000000
> +#define SBCPQ2_RTC_SIZE			0x2000
> +
> +#define SBCPQ2_BOOT_FLASH_BASE		0xFE000000
> +#define SBCPQ2_BOOT_FLASH_SIZE		0x00200000
> +
> +#define SBCPQ2_DIMM_FLASH_BASE		0xE0000000
> +#define SBCPQ2_DIMM_FLASH_SIZE		0x04000000
> +
> +#define CPM_MAP_ADDR			0xF0000000
> +#define CPM_IRQ_OFFSET			0

All this is in the device tree already, so don't duplicate it here.

> +/*
> + * The offset of ethernet MAC addr within EEPROM
> + */
> +#define SBCPQ2_FCC1_MACADDR_OFS		0x60
> +#define SBCPQ2_FCC2_MACADDR_OFS		0x66
> +#define SBCPQ2_FCC3_MACADDR_OFS		0x72
> +#define SBCPQ2_SCC1_MACADDR_OFS		0x78

Likewise, the mac address is in the device tree, so no need
to tell the kernel how to read it.

> +/*
> + * The following IRQs are routed to i8259 PIC.
> + *
> + * NOTE: i8259 PIC is cascaded to SIU_INT_IRQ6 of CPM2 interrupt controller
> + */
> +#define SBCPQ2_PC_IRQA		(NR_SIU_INTS+0)
> +#define SBCPQ2_PC_IRQB		(NR_SIU_INTS+1)
> +#define SBCPQ2_MPC185_IRQ	(NR_SIU_INTS+2)
> +#define SBCPQ2_ATM_IRQ		(NR_SIU_INTS+3)
> +#define SBCPQ2_PIRQA		(NR_SIU_INTS+4)
> +#define SBCPQ2_PIRQB		(NR_SIU_INTS+5)
> +#define SBCPQ2_PIRQC		(NR_SIU_INTS+6)
> +#define SBCPQ2_PIRQD		(NR_SIU_INTS+7)

Again, these are in the device tree, so don't put them here.

> +/* cpm serial driver works with constants below */
> +#define SIU_INT_SMC1		((uint)0x04+CPM_IRQ_OFFSET)
> +#define SIU_INT_SMC2		((uint)0x05+CPM_IRQ_OFFSET)
> +#define SIU_INT_SCC1		((uint)0x28+CPM_IRQ_OFFSET)
> +#define SIU_INT_SCC2		((uint)0x29+CPM_IRQ_OFFSET)
> +#define SIU_INT_SCC3		((uint)0x2a+CPM_IRQ_OFFSET)
> +#define SIU_INT_SCC4		((uint)0x2b+CPM_IRQ_OFFSET)

What are these for? If you need them in the device driver, just put
them in there, not in a header file. Also, you should make
sure not to pollute the global name space, so they should
be named SBCPQ2_SIU_INT_* to make it clear that they are board
specific.

> +#ifdef CONFIG_SBCPQ2
> +#include <platforms/82xx/sbcpq2.h>
> +#endif

Never put #ifdef around an #include.

> +
>   #ifdef CONFIG_PCI_8260
>   #include <platforms/82xx/m82xx_pci.h>
>   #endif

Kill this #ifdef as well while you're there. If you get name space
conflicts, just rename the symbols to make them unique.

> +/ {
> +	model = "SBCPQ2";
> +	compatible = "mpc82xx";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	linux,phandle = <100>;

Don't put explicit phandles here. If you need a reference, do it
symbolically.

	Arnd <><