Re: [PATCH 3/3] ARM: Add support for IXP4xx CPU and for Goramo Multilink router platform.

[Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>]

> +
> +/* offsets from start of flash ROM = 0x50000000 */
> +#define CFG_ETH0_ADDRESS     0x40 /* 6 bytes */
                           ^^^^^
whitespace please use tab for indent
> +#define CFG_ETH1_ADDRESS     0x46 /* 6 bytes */
> +#define CFG_REV              0x4C /* u32 */
> +#define CFG_SDRAM_SIZE       0x50 /* u32 */
> +#define CFG_SDRAM_CONF       0x54 /* u32 */
> +#define CFG_SDRAM_MODE       0x58 /* u32 */
> +#define CFG_SDRAM_REFRESH    0x5C /* u32 */
> +
> +#define CFG_HW_BITS          0x60 /* u32 */
> +#define  CFG_HW_USB_PORTS    0x00000007 /* 0 = no NEC chip, 1-5 = ports # */
> +#define  CFG_HW_HAS_PCI_SLOT 0x00000008
> +#define  CFG_HW_HAS_ETH0     0x00000010
> +#define  CFG_HW_HAS_ETH1     0x00000020
> +#define  CFG_HW_HAS_HSS0     0x00000040
> +#define  CFG_HW_HAS_HSS1     0x00000080
> +#define  CFG_HW_HAS_UART0    0x00000100
> +#define  CFG_HW_HAS_UART1    0x00000200
> +#define  CFG_HW_HAS_EEPROM   0x00000400
> +
> +#define ETH_ALEN             6
IIR we have a macro for it
> +
> +#define BAREBOX_START        0x00000
> +#define BAREBOX_LENGTH       0x34000
> +#define NPE_A_START          (BAREBOX_START + BAREBOX_LENGTH)
> +#define NPE_A_LENGTH         0x05000
> +#define NPE_B_START          (NPE_A_START + NPE_A_LENGTH)
> +#define NPE_B_LENGTH         0x03000
> +#define NPE_C_START          (NPE_B_START + NPE_B_LENGTH)
> +#define NPE_C_LENGTH         0x04000
> +#define NPE_ENV0_START       (NPE_C_START + NPE_C_LENGTH)
> +#define NPE_ENV0_LENGTH      0x20000
I prefer we use a fs to store it so we can share it Linux
with a cramfs at least
> +
> +static struct device_d cfi_dev = {
> +	.name     = "cfi_flash",
> +	.map_base = IXP4XX_EXP_BASE(0),
> +	.size     = 16 * 1024 * 1024,
> +};
> +
> +static struct eth_plat_info eth_pinfo[2] = {
> +	{
> +		.regs     = IXP4XX_EthB_BASE,
> +		.npe      = 1,
> +		.phy      = 0,
> +		.rxq      = 20,
> +		.txreadyq = 29,
> +	}, {
> +		.regs     = IXP4XX_EthC_BASE,
> +		.npe      = 2,
> +		.phy      = 1,
> +		.rxq      = 21,
> +		.txreadyq = 30,
> +	}
> +};
> +
> +
> +	sdram_dev.size = __raw_readl(IXP4XX_EXP_BASE(0) + CFG_SDRAM_SIZE);
how about check the data before register
and if not good use the minimum memory size for the hardware design
> +	i = register_device(&sdram_dev);
> +	if (i)
> +		goto out;
> +
> +	armlinux_add_dram(&sdram_dev);
> +	armlinux_set_bootparams((void *)(0x00000100));
> +	armlinux_set_architecture(MACH_TYPE_GORAMO_MLR);
> +
> +out:
> +	return i;
> +}

 +
> +static inline void qmgr_put_entry(unsigned int queue, u32 val)
> +{
> +#if DEBUG_QMGR
> +	BUG_ON(!qmgr_queue_descs[queue]); /* not yet requested */
> +
> +	fprintf(stderr, "Queue %s(%i) put %X\n",
> +		qmgr_queue_descs[queue], queue, val);
please use debug() instead of fprintf so no need of ifdef if no BUG_ON
and it will be good to be able to enable the DEBUG via Kconfig

please avoid the #if 0
if no need remove it or use a CONFIG

othersize looks fine

I will test it on the nslu2 later

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox