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 <[EMAIL PROTECTED]> 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:[EMAIL PROTECTED] 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 <[EMAIL PROTECTED]> > > Signed-off-by: Sebastian Hess <[EMAIL PROTECTED]> > > Signed-off-by: Matthias Mwenzel <[EMAIL PROTECTED]> > > Signed-off-by: Dirk Hörner <[EMAIL PROTECTED]> > > Signed-off-by: Andreas Weißel <[EMAIL PROTECTED]> > > > 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:[EMAIL PROTECTED] 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
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
