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

Mon Nov 3 12:58:24 CET 2008

Hi,

thanks all for the comments. I will try to fix these.

On Sat, 1 Nov 2008 13:57:25 +0100, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj at jcrosoft.com> wrote:

> On 15:30 Fri 31 Oct     , Juergen Schoew wrote:
> > Hi U-Boot mailling list,
> > 
> > This patchset adds a new ARM board with the NXP PNX8181 cpu to u-boot.
> > The PNX8181 is an ARM926ej with an internal DSP and a baseband processor
> > (used for DECT). The chip also features dual ethernet, digital to analog
> > interface, spi, i2c and other SOC peripherals. 
> > 
> 
> From here
> > The patch is against u-boot commit
> > 055b12f2ffd7c34eea7e983a0588b24f2e69e0e3 (Date: Sun Oct 19 21:54:30 2008
> > +0200) but should apply to newer commits as well, because the code is
> > mostly seperated.
> > 
> > If you have any comments please email to me.
> > 
> > Is is possible to include that patch in the new version of u-boot?
> > 
> > Regards
> > 
> > Jürgen Schöw
> > 
> > --
> > Dipl.-Ing. Jürgen Schöw, emlix GmbH, http://www.emlix.com,
> > mailto:js at emlix.com Fon +49 551 30664-0, Fax -11, Bahnhofsallee 1b, 37081
> > Göttingen, Germany Geschäftsführung: Dr. Uwe Kracke, Dr. Cord Seele,
> > Ust-IdNr.: DE 205 198 055 Sitz der Gesellschaft: Göttingen, Amtsgericht
> > Göttingen HR B 3160
> > 
> > emlix - your embedded linux partner
> to here is supposed to be after ---
> > -----
> > 
> > Signed-off-by: Jürgen Schöw <js at emlix.com>
> > Signed-off-by: Sebastian Hess <sh at emlix.com>
> > Signed-off-by: Matthias Mwenzel <nxp at mazzoo.de>
> > Signed-off-by: Dirk Hörner <dirk.hoerner at dspg.com>
> > Signed-off-by: Andreas Weißel <andreas.weissel at dspg.com>
> > 
> and sob before

OK. I hope to remember when posting the next cycle.

> first a question
> who build this board?

These boards have been build by NXP Semiconductors GmbH, Nuremberg, Germany
and are now build by DSPG Technologies GmbH, Nuremberg, Germany.

> > +++ b/Makefile
> > @@ -2683,6 +2683,13 @@ voiceblue_config:	unconfig
> >  	@$(MKCONFIG) $(@:_config=) arm arm925t voiceblue
> >  
> >  #########################################################################
> > +## NXP PNX8181 "firetux"
> > +#########################################################################
> > +
> > +firetux_config:unconfig
> > +	@$(MKCONFIG) $(@:_config=) arm arm926ejs firetux # manufacturer
> > SOC
>                                                          ^^^^^^^^^^^^^^^^^^
> please remove

OK

> > +++ b/board/firetux/Makefile
> > +
> > +#ifdef CONFIG_CMD_NAND
> > +COBJS	+= nand.o
> > +#endif
> please use COBJS-$(CONFIG_CMD_NAND)

Correct.

> > +++ b/board/firetux/config.mk
> > +
> > +# SDRAM
> > +TEXT_BASE = 0x20780000
> > +# mobile pSRAM
> > +#TEXT_BASE = 0x90700000
> > +
> could you use a condition

Yes will change.

> > +PLATFORM_CPPFLAGS += -fPIC -fPIE -fno-jump-tables # -msingle-pic-base
> > +
> > +ifneq ($(OBJTREE),$(SRCTREE))
> > +# We are building u-boot in a separate directory, use generated
> > +# .lds script from OBJTREE directory.
> > +LDSCRIPT := $(OBJTREE)/board/$(BOARDDIR)/u-boot.lds
> > +endif
> > diff --git a/board/firetux/ethernet.c b/board/firetux/ethernet.c
> please move to drivers/net/

I'm not sure how many SOC will use this IP-Core for ethernet. I think it will
not be too much, but I can move it to drivers/net/.
Should I remove all board specific routines and leave them in the
board/firetux/ directory? (Just to make this driver hardware not depending
on a special board?) What do you suggest?

> > new file mode 100644
> > index 0000000..866d578
> > --- /dev/null
> > +++ b/board/firetux/ethernet.c
> > +
> > +extern unsigned int boardrevision;
> > +uint16_t ETN1_MADR_PHY_ADDR, ETN2_MADR_PHY_ADDR;
> please do not use uppercase for var name

Will rework.

> > +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)))
> why static? it should be strange to assume it's static when we read the code

Because these structures are private to the driver.

> please use tab instead of space

Opps, I thought that I found all.

> > +/* which interface to be currently work on           */
> > +/* default can be set by environment variable ethact */
> > +static int firetux_eth = 0;
> > +
> > +/* in the code we use the following descriptors which are either */
> > +/* set to the etn1 or the etn2 descriptors, depending on ethact  */
> > +ALIGN8 rx_descriptor_t *etn_rxdescriptor = etn1_rxdescriptor;
> > +ALIGN8 rx_status_t     *etn_rxstatus     = etn1_rxstatus;
> > +ALIGN8 tx_descriptor_t *etn_txdescriptor = etn1_txdescriptor;
> > +ALIGN8 tx_status_t     *etn_txstatus     = etn1_txstatus;
> > +
> > +/* also the base address is switched for etn1 and etn2,          */
> > +/* except for the MII registers etn1_m* which are only available */
> > +/* on etn1 */
> please use this style of comment
> /*
>  *
>  */

Will change.

> > +uint32_t etn_base = ETN1;
> > +/* we first try Vega Platform III-a settings */
> > +uint16_t firetux_phy_addr = 0x0100;
> > +
> is it not better to do all of this init in an init function?
> 
> and why not use a struct to save all parameter?

Good point.

> > +
> > +static void firetux_set_ethact(int act, int hardwarerevision)
> > +{
> > +	switch (hardwarerevision) {
> > +	/* EZ_MCP, Vega_pnx8181_basestation Platform III-a */
> > +	case 1:
> > +	case 2:
> > +		ETN1_MADR_PHY_ADDR = 0x00000100;
> > +		ETN2_MADR_PHY_ADDR = 0x00000200;
> please use macro instead

OK.

> > +		ETN1_MADR_PHY_ADDR = 0x00001E00;
> > +		ETN2_MADR_PHY_ADDR = 0x00001D00;
> > +		break;
> > +	}
> > +	if (act) {
> > +		etn_rxdescriptor = etn2_rxdescriptor;
> > +		etn_rxstatus     = etn2_rxstatus;
> > +		etn_txdescriptor = etn2_txdescriptor;
> > +		etn_txstatus     = etn2_txstatus;
> > +		etn_base         = ETN2;
> > +		firetux_phy_addr = (uint16_t) ETN2_MADR_PHY_ADDR;
> > +	} else {
> > +		etn_rxdescriptor = etn1_rxdescriptor;
> > +		etn_rxstatus     = etn1_rxstatus;
> > +		etn_txdescriptor = etn1_txdescriptor;
> > +		etn_txstatus     = etn1_txstatus;
> > +		etn_base         = ETN1;
> > +		firetux_phy_addr = (uint16_t) ETN1_MADR_PHY_ADDR;
> > +	}
> > +}
> > +
> > +static void firetux_reset_phy(int hardwareversion)
> > +{
> > +	switch (hardwareversion) {
> > +	case 1:
> > +	case 2:
> > +	case 3:
> please use readx and writex

OK.

> it will be better to create a gpiolib support
> > +		/* set GPIOa12 direction to output */
> > +		*(vu_long *)(PNX8181_GPIOA_DR) = (((*(vu_long *)
> > +				(PNX8181_GPIOA_DR)) & 0xffffefff) |
> > 0x00001000);
> > +		/* ETH_RESET_N */
> > +		*(vu_long *)(PNX8181_GPIOA_OR) = ((*(vu_long *)
> > +				(PNX8181_GPIOA_OR)) & 0xffffefff);
> > +		udelay(256000);
> > +		*(vu_long *)(PNX8181_GPIOA_OR) = (((*(vu_long *)
> > +				(PNX8181_GPIOA_OR)) & 0xffffefff) |
> > 0x00001000);
> > +		udelay(100000);
> why 100ms of delay?

You are correct. It must be at least 100us. But we had at the beginning much
trouble with the phy and a long external reset had helped a lot to make this
thing stable.

> > +		/* ETH_RESET_N */
> > +		*(vu_long *) (PNX8181_GPIOA_OR) = ((*(vu_long *)
> > +					(PNX8181_GPIOA_OR)) &
> > 0xfffffff7);
> > +		udelay(256000);
> why 256ms of delay?

Same as above. Will lower it.

> > +		*(vu_long *) (PNX8181_GPIOA_OR) = (((*(vu_long *)
> > +				(PNX8181_GPIOA_OR)) & 0xfffffff7) |
> > 0x00000008);
> > +		udelay(100000);
> why 100ms of delay?

Same as above. Will lower it.

> > +	*(vu_long *) CGU_PER2CON = (15<<9) | (62<<3) | 3;
> please ad space between "<<"

Will fix.

> > +static int firetux_eth_init_rxdescriptor(void)
> > +{
> > +	*(vu_long *) (etn_base + ETN_RXDESCRIPTOR) = (vu_long)
> > etn_rxdescriptor;
> > +	*(vu_long *) (etn_base + ETN_RXSTATUS)     = (vu_long)
> > etn_rxstatus;
> > +	*(vu_long *) (etn_base + ETN_RXCONSUMEINDEX) = 0x00000000;
> > +	*(vu_long *) (etn_base + ETN_RXDESCRIPTORNUMBER) =
> > +						ETN_RX_DESCRIPTOR_NUMBER
> > - 1; +
> > +	/* allocate rx-buffers, but only once, we're called multiple
> > times! */
> > +	static void *rxbuf = 0;
> > +	if (!rxbuf)
> > +		rxbuf = malloc(MAX_ETH_FRAME_SIZE *
> > ETN_RX_DESCRIPTOR_NUMBER);
> > +	if (!rxbuf) {
> > +		puts("ERROR: couldn't allocate rx buffers!\n");
> > +		return -1;
> > +	}
> > +
> > +	int i;
> please declare var at begining

OK.

> > +static void PHY_write(uint16_t a, uint16_t d)
> please no uppercase in function name

OK.

> > +#ifdef ET_DEBUG
> > +		printf("### PHY_write(%2.d, 0x%4.4x) success after %d
> > cycles "
> > +			"[eth=%d, phy_addr=%x]\n", a, d, i, firetux_eth,
> > +
> > firetux_phy_addr);
> 		please use debug()

OK.

> > +static uint16_t PHY_read(uint16_t a)
> > +{
> > +	uint32_t status;
> > +	int i = 0;
> > +
> > +	a &= 0x001f; /* 5 bit PHY register address */
> > +	*(vu_long *) (ETN1 + ETN_MADR) = firetux_phy_addr | a;
> > +	*(vu_long *) (ETN1 + ETN_MCMD) = 0x00000001;
> > +
> > +	/* poll for done */
> > +	while ((status = ((*(vu_long *)
> > +			    (ETN1 + ETN_MIND))) & 0x7) && i < 1000000)
> > +		i++;
> > +
> > +	uint16_t d = *(vu_long *) (ETN1 + ETN_MRDD);
> please declare var at the beggening

OK.

> Their is too much fix to do to.

Puh. Well, that is absolutely coorect.

> Please fix first.

OK.

-- 
Dipl.-Ing. Jürgen Schöw, emlix GmbH, http://www.emlix.com, mailto:js at emlix.com
Fon +49 551 30664-0, Fax -11, Bahnhofsallee 1b, 37081 Göttingen, Germany
Geschäftsführung: Dr. Uwe Kracke, Dr. Cord Seele, Ust-IdNr.: DE 205 198 055
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160

emlix - your embedded linux partner
-------------- 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/20081103/b32960e2/attachment-0001.pgp 