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

[Stefano Babic]

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
=====================================================================