[U-Boot] [patch V2] U-Boot Firetux board support

[Jürgen Schöw]

Hi Jean-Christophe PLAGNIOL-VILLARD,

sorry to answer late. I've been on the CELF Conference last week and
being busy this week delays so much.

Am Thu, 6 Nov 2008 21:53:29 +0100
schrieb Jean-Christophe PLAGNIOL-VILLARD <plagnioj at jcrosoft.com>:

> On 12:48 Wed 05 Nov     , Juergen Schoew wrote:
> > Hi U-Boot mailling list,
>
> First some general comments
> 
> As you anwser it's a NXP board so please create a vendor directory

OK, I use DSPG.

> please use tab instead of space for code alignement and indentation
> please a blank line when you write block of code and comment to make
> it more readable
> please and whitespace before and after "<<" & co

OK, I try to find every position.
 
> in the code their is a lot of hard code value please use Macro or
> specify why you can not do it

Will try to find suitable places.

> please only one blank line consecutive

OK.

> > diff --git a/board/firetux/Makefile b/board/firetux/Makefile
> > new file mode 100644
> > index 0000000..bf1afec
> > --- /dev/null
> > +++ b/board/firetux/Makefile
> > +include $(TOPDIR)/config.mk
> > +
> > +LIB	= $(obj)lib$(BOARD).a
> > +
> > +COBJS-y	+= firetux.o ethernet.o
> as ask by Ben and I move this to drivers/net

OK, I read it more as "you may" and not "you have to". Sorry for the
misinterpretation. I try to find some spare time to do this jobs.
Hopefully this can work on the next weekend.

> > diff --git a/board/firetux/ethernet.c b/board/firetux/ethernet.c
> > new file mode 100644
> > index 0000000..a4c1bc2
> > --- /dev/null
> > +++ b/board/firetux/ethernet.c
> > +
> > +extern unsigned int boardrevision;
> > +
> > +int firetux_miiphy_initialize(bd_t *bis);
> > +int mii_discover_phy(void);
> > +int mii_negotiate_phy(void);
> > +
> > +#define ALIGN8	static __attribute__ ((aligned(8)))
> > +#define ALIGN4	static __attribute__ ((aligned(4)))
> IMHO this make more sense
> #define align8 __attribute__ ((aligned(8))

OK, changed.

> > +	}
> > +	if (act) {
> > +		etn_rxdescriptor = etn2_rxdescriptor;
> > +		etn_rxstatus     = etn2_rxstatus;
>                             ^^^^^
> please use tab instead of space on all

Yes.

> > +	/* get mode from environment */
> > +	mode = getenv("phymode");
> > +	if (mode  != NULL) {
> please use if (mode)
> > +		if (0 == strcmp(mode, "auto")) {
> please invert the condition an so on
> 		if (strcmp(mode, "auto")) == 0) {

No problem.

> > +void firetux_gpio_init_A(void)
> > +{
> > +	/* select TXD2 for GPIOa11 and select RXD2 for GPIOa1 */
> > +	writel(((readl((void *)(PNX8181_SCON_BASE +
> > PNX8181_SYSMUX0))
> > +						& 0xff3ffff3) |
> > 0x00400004),
> > +				(void *)(PNX8181_SCON_BASE +
> > PNX8181_SYSMUX0));
> > +	/* select ETN for GPIOc0-16 */
> > +	writel(readl((void *)(PNX8181_SCON_BASE + PNX8181_SYSMUX5))
> > +								&
> > 0xfffffffc,
> > +				(void *)(PNX8181_SCON_BASE +
> > PNX8181_SYSMUX5));
> > +	writel(readl((void *)(PNX8181_SCON_BASE + PNX8181_SYSMUX4))
> > +								&
> > 0x30000000,
> > +				(void *)(PNX8181_SCON_BASE +
> > PNX8181_SYSMUX4)); +
> > +	/* use clk26m and fract-n for UART2 */
> > +	writew(((1 << 7) | (1 << 1)),
> > +			(void *)(PNX8181_UART2_BASE +
> > PNX8181_UART_FDIV_CTRL));
> > +	writew(0x5F37, (void *)(PNX8181_UART2_BASE +
> > PNX8181_UART_FDIV_M));
> > +	writew(0x32C8, (void *)(PNX8181_UART2_BASE +
> > PNX8181_UART_FDIV_N)); +}
> > +
> > 
> IMHO please use deferent function for each hardware revision
> it will allow to make it more readable and to reduce the u-boot when
> compiling it for specific revision

The different revisions are very similair. I can split the routines
into different functions, but I doubt that it make sense to split these
into different files. The overhead is IMHO too much.

> > +void firetux_gpio_init_B(void)
> > +{
> > +	if (boardrevision < 2)		/* EZ_MCP has no
> > leds or keys */
> > +		return;
> > +
> > +	/* set GPIOA10 and GPIOA2 for key input */
> > +	writel(readl((void *)(PNX8181_SCON_BASE + PNX8181_SYSMUX0))
> > +								&
> > 0xffcfffcf,
> > +				(void *)(PNX8181_SCON_BASE +
> > PNX8181_SYSMUX0));
> > +	writel(readl((void *)PNX8181_GPIOA_PAD_LOW) & 0xffcfffcf,
> > +						(void
> > *)PNX8181_GPIOA_PAD_LOW);
> > +	writel(readl((void *)PNX8181_GPIOA_DIRECTION) & ~((1 << 2)
> > | (1 << 10)),
> > +					(void
> > *)PNX8181_GPIOA_DIRECTION); +
> > +	/* set GPIOA9 and GPIOA0 for LED output */
> > +	if (boardrevision > 2) {
> > +		writel(readl((void *)(PNX8181_SCON_BASE +
> > PNX8181_SYSMUX0))
> > +								&
> > 0xfff3fffc,
> > +				(void *)(PNX8181_SCON_BASE +
> > PNX8181_SYSMUX0));
> > +		writel(readl((void *)PNX8181_GPIOA_DIRECTION)
> > +							| ((1 <<
> > 9) | (1 << 0)),
> > +				(void *)PNX8181_GPIOA_DIRECTION);
> > +		writel(readl((void *)PNX8181_GPIOA_OUTPUT)
> > +							| ((1 <<
> > 9) | (1 << 0)),
> > +				(void *)PNX8181_GPIOA_OUTPUT);
> > +	} else {    /* Baseboard IIIa */
> > +		writel(readl((void *)(PNX8181_SCON_BASE +
> > PNX8181_SYSMUX0))
> > +								&
> > 0xfff3ff3f,
> > +				(void *)(PNX8181_SCON_BASE +
> > PNX8181_SYSMUX0));
> > +		writel(readl((void *)(PNX8181_SCON_BASE +
> > PNX8181_SYSMUX1))
> > +								&
> > 0x3ffffffc,
> > +				(void *)(PNX8181_SCON_BASE +
> > PNX8181_SYSMUX1));
> > +		writel(readl((void *)PNX8181_GPIOA_DIRECTION)
> > +				| ((1 << 9) | (1 << 31) | (1 <<
> > 16) | (1 << 3)),
> > +				(void *)PNX8181_GPIOA_DIRECTION);
> > +		writel(readl((void *)PNX8181_GPIOA_OUTPUT)
> > +				| ((1 << 9) | (1 << 31) | (1 <<
> > 16) | (1 << 3)),
> > +				(void *)PNX8181_GPIOA_OUTPUT);
> > +	}
> > +}
> > +
> if possible split it in multiple file to avoid ifdef in the code at
> maximun note we will move in the futur to Kconfig to manage it
> and it's the same for the asm

This is normally one function, which was broken into two parts, because
I had to check the board revision in between. But for the move of the
network driver I have to split this part out of the network code and
can do the hardware check in a extra function and merge this one.


> > +void firetux_irq_disable(void)
> > +{
> > +	int i;
> > +
> > +	writel(31, (void *)PNX8181_INTC_PRIOMASK_IRQ);
> > +	writel(31, (void *)PNX8181_INTC_PRIOMASK_FIQ);
> > +
> > +	for (i = 1; i < 67; i++)
> > +		PNX8181_DISABLEIRQ(i);
> > +
> > +	writel(readl((void *)PNX8181_CGU_GATESC) & 0xfdd7becd,
> > +						(void
> > *)PNX8181_CGU_GATESC); +}
> > +
>  +	}
> > +
> > +	return ret;
> > +}
> > +
> > +
> please create a led.c

OK.

> > +/*
> > + * timer without interrupts
> > + */
> > +
> > +
> > +/*
> > + * U-Boot expects a 32 bit timer, running at CONFIG_SYS_HZ
> > + * Keep total timer count to avoid losing decrements < div_timer
> > + */
> > +
> is your timer is board specific or soc specific??

You are right. This is more SOC specific. I'll move it.


> > diff --git a/drivers/i2c/pnx8181_i2c.c b/drivers/i2c/pnx8181_i2c.c
> > new file mode 100644
> > index 0000000..9979ebe
> > --- /dev/null
> > +++ b/drivers/i2c/pnx8181_i2c.c

Oops, I missed to rework this code last time.

> > +
> > +#if defined(CONFIG_CMD_I2C) && defined(CONFIG_FIRETUX)
> please move to Makefile

OK, done.

> > +void i2c_init(int speed, int slaveadd)
> > +{
> > +	unsigned long timeout;
> > +
> > +	SCON_SYSMUX1_REG &= SCON_GPIOA27_MUX_FIELD &
> > SCON_GPIOA28_MUX_FIELD;
> > +	SCON_SYSMUX1_REG |= SCON_GPIOA27_SCL | SCON_GPIOA28_SDA;
> > +	CGU_GATESC_REG |= CGU_I2CEN;
> > +	if (speed == 400000) {
> ?????

Only tested so far with 100KHz, need to calculate the needed divisors.

> > +	} else {
> > +		speed = 100000;
> > +		/* here the registers are set in order to have a
> > 100KHz clk
> > +		 * for a pclk1 of 23Mhz */
> > +		I2C_CLKHI_REG   = I2C_CLKDIV_FIELD  | 0x50;
> > +		I2C_CLKLO_REG   = I2C_CLKDIV_FIELD  | 0x96;
> > +		I2C_HOLDDAT_REG = 0x10;
> > +	}
> > +	I2C_ADR_REG = slaveadd & ~I2C_SADDR_FIELD;
> > +	SOFT_I2C_RESET;
> > +	timeout = 0;
> > +	while ((I2C_ACTIVE & I2C_STS_REG) == I2C_ACTIVE) {
> > +		if (timeout > I2C_TIMEOUT)
> > +			break;
> > +		timeout++;
> > +		udelay(I2C_DELAY);
> > +	}
> > +	DPRINT("%s: speed: %d\n", __func__, speed);
> please use debug()

Done.

> > diff --git a/include/configs/firetux.h b/include/configs/firetux.h
> > new file mode 100644
> > index 0000000..efa27af
> > --- /dev/null
> > +++ b/include/configs/firetux.h
> > +/* we can have nand _or_ nor flash */
> > +/* #define CONFIG_NANDFLASH		1 */
> > +
> > +/* #define CONFIG_SHOW_BOOT_PROGRESS	1 */
> please no dead code

Sorry, I use this code for testing the release and trying to find
problematic areas. This way I only need to change the config and do not
need to write the code for every release cycle. So I prefer to let it
there.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20081113/f92b0d18/attachment-0001.pgp