[U-Boot] [PATCH] Driver for AX88183 ethernet chip

[Joe XUE]

Hi Stefano,
Thank you for your advice.It has been changed according to you suggestion and passed the checking of checkpatch.pl (two warnings , one is asm/io.h one is struct eth_device's member function send has a volatile parameter).
Although AX88183 has the similar name with AX88180, they are very different.
It works well on my board.I have submitted it again.
Best wishes.
Joe

> Date: Fri, 28 Jan 2011 09:02:38 +0100
> From: sbabic at denx.de
> To: lgxue at hotmail.com
> CC: u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] Driver for AX88183 ethernet chip
> 
> On 01/27/2011 07:55 PM, root wrote:
> > From: Joe Xue <lgxue at hotmail.com>
> 
> You have to set you Signe-off-by in your patch. Please add also
> a commit message to explain what your patch is doing, and on which board
> you tested this driver. And set the net custodian as CC, this helps to
> get faster review.
> 
> > --- a/drivers/net/Makefile
> > +++ b/drivers/net/Makefile
> > @@ -83,6 +83,7 @@ COBJS-$(CONFIG_TSI108_ETH) += tsi108_eth.o
> >  COBJS-$(CONFIG_ULI526X) += uli526x.o
> >  COBJS-$(CONFIG_VSC7385_ENET) += vsc7385.o
> >  COBJS-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o
> > +COBJS-$(CONFIG_DRIVER_AX88183) += ax88183.o
> 
> Please maintain the list sorted.
> 
> > @@ -0,0 +1,265 @@
> > +
> > +/*
> > + * (C) Copyright 2010  Joe Xue <lgxue at hotmail.com>
> > + *
> > + * This file is released under the terms of GPL v2 and any later version.
> > + * See the file COPYING in the root directory of the source tree for details.
> > +*/
> 
> The header is not enough. Please add coherent header with license, as
> you can see for other drivers. This must be fixed globally.
> 
> > +
> > +/*
> > + * This driver works on 32bit mode
> > + * AX88783 has two ethernet ports, we only use port 0 in u-boot
> > + */
> 
> I suggest you add this comment to the README file, too,
> 
> > +#include <common.h>
> > +#include <command.h>
> > +#include <net.h>
> > +#include <linux/mii.h>
> > +#include <miiphy.h>
> > +#include <config.h>
> > +#include "ax88183.h"
> > +
> > +static int ax88183_init(bd_t *bd)
> > +{
> > +    volatile unsigned int tmp;
> > +    int i;
> > +
> > +    /* reset chip */
> > +    tmp = CR;
> > +    CR = (tmp & ~CR_CHIP_RESET);
> 
> u-boot does not not accept anymore to write/read into registers using
> offsets and volatile variables. Please define an
> appropriate structure for your chip, and use read/write accessors to
> work with registers. This must be fixed globally in all your code.
> 
> I do not know this chip. However, u-boot has already a ax88180 driver.
> Would be not possible to modify this driver to make it suitable for your
> chip, too ?
> 
> Other remarks: it seems to me your driver does not support CONFIG_MULTI,
> as it should do. And it must call eth_register() to be inserted as
> device at runtime. Please check with other network drivers.
> 
> > +    udelay(1000);
> > +    CR = (tmp | CR_CHIP_RESET);
> > +    
> > +    /* disable interrupt */
> > +    IMSR = IMSR_MASK_ALL;
> > +
> > +    /* set mac address*/
> > +    unsigned char mactmp[4];
> > +    unsigned int * mac = (unsigned int *)mactmp;
> > +    mactmp[0] = bd->bi_enetaddr[5];
> > +    mactmp[1] = bd->bi_enetaddr[4];
> > +    mactmp[2] = bd->bi_enetaddr[3];
> > +    mactmp[3] = bd->bi_enetaddr[2];
> 
> This is not correct anymore. You do not have to set the mac from the bd.
> Instead of that, you have to provide in your initialization function a
> struct eth_device that should be passed to eth_register().
> > +
> > +int eth_init (bd_t * bd)
> > +{
> > +
> > +	char *s, *e;
> > +	int i;
> > +
> > +	s = getenv ("ethaddr");
> 
> The driver must not call getenv explicitely.
> 
> > +#if 0
> > +	printf ("\tHW MAC address:  "
> > +		"%02X:%02X:%02X:%02X:%02X:%02X\n",
> > +		bd->bi_enetaddr[0], bd->bi_enetaddr[1],
> > +		bd->bi_enetaddr[2], bd->bi_enetaddr[3],
> > +		bd->bi_enetaddr[4], bd->bi_enetaddr[5] );
> > +#endif
> 
> do not add dead code. Instead of that, you can use the debug() function
> to trace.
> 
> 
> > +/*
> > + *-------------------------------------------------------------------------
> > + * Registers
> > + *-------------------------------------------------------------------------
> > + */
> > +
> > +#define PCR  (*(volatile unsigned int *)(CONFIG_AX88183_BASE + 0x0004))
> 
> Replace all defines with a C structure describing your chip.
> 
> Best regards,
> Stefano Babic
> 
> -- 
> =====================================================================
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
> =====================================================================