[U-Boot] [PATCH] Add support for AX88183 ethernet chip

[stefano babic]

Am 29.01.2011 03:03, schrieb Joe Xue:

Hi Joe,

> Signed-off-by: Joe Xue <lgxue at hotmail.com>
> 	modified:   README
> 	modified:   drivers/net/Makefile
> 	new file:   drivers/net/ax88183.c
> 	new file:   drivers/net/ax88183.h
> 	modified:   include/netdev.h

You do not need to add this stuff in the commit message. They are
already reported under the "---" line and can be easy find with git
commands. Instead, add here which chip is supported (you do not mention
it is an Asix chip in the commit id), how do you tested, and some
general info you can find useful for any reader who finds your driver
and what to know something more. Is ax88183 the right name for the chip
? I cannot find it on the Asix's website.

> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index fd9d0b4..393d5f3 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -84,6 +84,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

The list is not *alfabetically* sorted, as it is before your patch. You
have to put your driver after the ax88180 driver.

> +
> +/*
> + * AX88783 has two ethernet ports, this driver uses port 0 in u-boot
> + */

Is there a way to make this configurable ? I mean, the driver supports
always one port, but which one could be configurable in the bard
configuration file.

> +
> +#include <common.h>
> +#include <command.h>
> +#include <net.h>
> +#include <linux/mii.h>
> +#include <miiphy.h>
> +#include <config.h>
> +#include <asm/io.h>
> +#include "ax88183.h"
> +
> +static int ax88180_phy_initial(struct eth_device *dev)
> +{
> +	int i;
> +	unsigned int tmp;
> +	struct ax88183_reg * reg = (struct ax88183_reg *)dev->iobase;
> +
> +	/* reset chip */
> +	tmp = readl(&reg->cr);
> +	writel((tmp & ~CR_CHIP_RESET), &reg->cr);
> +	udelay(1000);
> +
> +	writel((tmp | CR_CHIP_RESET), &reg->cr);
> +
> +	/* phy init */
> +	tmp = readl(&reg->pcr);
> +	tmp |= PCR_PHY0_RESET_CLEAR;
> +
> +	writel(tmp, &reg->pcr);
> +	udelay(100000);

This is a very long time (100mSec). Why do we need ?

> +
> +	tmp = readl(&reg->pollcr);
> +	tmp &= POLLCR_PORT0_PHYID_MASK;
> +	tmp |= POLLCR_PORT0_PHYID(0x10);
> +	writel(tmp, &reg->pollcr);
> +
> +	/* write MII mode */
> +	tmp = readl(&reg->miicr) & 0xFF;
> +	tmp &= (~MIICR_PORT0_MII_CLK_GEN);
> +	tmp &= (~MIICR_PORT0_PHY_RMII);
> +	writel(tmp, &reg->miicr);
> +
> +	/* set LED mode */
> +	tmp = readl(&reg->ledcr);
> +	tmp |= LEDCR_PORT_LED_ON(0) | LEDCR_LED0(PHY_LED_RX | PHY_LED_TX);
> +	tmp |= LEDCR_PORT_LED_ON(1) | LEDCR_LED1(PHY_LED_LINK);
> +	writel(tmp, &reg->ledcr);
> +
> +	/* set auto polling */
> +	tmp = readl(&reg->pollcr);
> +	tmp |= (POLLCR_PORT0_AUTO_POOLING);
> +	writel(tmp, &reg->pollcr);
> +
> +	/* set link speed */
> +	for (i = 0; i < 2; i++) {
> +		int reg_num = i*8+16;
> +		tmp = MDCR_READ | MDCR_PHY_ID(0x10) | MDCR_PHY_REG(reg_num);
> +		writel(tmp, &reg->mdcr);
> +		while (1) {
> +			tmp = readl(&reg->mdcr);
> +			if (tmp & MDCR_VALID)
> +				break;
> +		}
> +
> +		tmp = readl(&reg->mdcr) & MDCR_VALUE_MASK;
> +		tmp = tmp | 0x1000 | MDCR_WRITE | \
> +			MDCR_PHY_ID(0x10) | MDCR_PHY_REG(reg_num);
> +		writel(tmp, &reg->mdcr);
> +		while (1) {
> +			tmp = readl(&reg->mdcr);
> +			if (tmp & MDCR_VALID)

No timeout here ? If it does not work, system hangs without printing

> +	while (1) {
> +		tmp = readl(&reg->mdcr);
> +		if (tmp & MDCR_VALID)

Ditto. It should be fixed globally.


> +static int ax88183_init(struct eth_device *dev, bd_t * bd)
> +{
> +	unsigned int tmp;
> +	struct ax88183_reg *reg = (struct ax88183_reg *)dev->iobase;
> +
> +	/* disable interrupt */
> +
> +	writel(IMSR_MASK_ALL, &reg->imsr);
> +	/* set mac address*/
> +	unsigned char mactmp[4];
> +	unsigned int * mac = (unsigned int *)mactmp;

Why are these variabled defined here and not at the beginning of the
funtion ?

> +static void ax88183_halt(struct eth_device *dev)
> +{
> +	;

This is not needed, and should be dropped. However, you should disable
the device in this function.

> +/* Send a data block via Ethernet. */
> +static int
> +ax88183_send(struct eth_device *dev, volatile void *packet, int length)
> +{
> +	unsigned int pkt_header, tmp, i;
> +	unsigned char *buf = (unsigned char *)packet;
> +	struct ax88183_reg * reg = (struct ax88183_reg *)dev->iobase;
> +	tmp = length;
> +	tmp = (~tmp << 16) | tmp;
> +	pkt_header = ((tmp & 0xff000000) >> 24) | \
> +				 ((tmp & 0x00ff0000) >> 8) | \
> +				 ((tmp & 0x0000ff00) << 8) | \
> +				 ((tmp & 0x000000ff) << 24);

It seems to me you this are due to adapt the endianess. In this case you
should use acceesors (cpu_to_le32, cpu_to_be32).

> +struct ax88183_reg {
> +	unsigned int cr;        /* 0x0000 */
> +	unsigned int pcr;       /* 0x0004 */

I think the offset as comment is not very useful, I suggest to remove it.

> +#define P0MAC0 0x0230
> +
> +#define P0MAC1 0x0234

These two defines seem to me obsolete and should be removed.

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