Add support for ASIX's AX88783 ethernet chip

PATCH New Joe Xue 3 participants 5 messages Jan 31, 2011 – Feb 4, 2011 Network drivers

Share & tools

Joe Xue

author

2011-01-31 17:42 UTC #0 lore raw

PATCH 1/1 Add support for ASIX's AX88783 ethernet chip

for more information about this chip, please check:
http://www.asix.com.tw/products.php?op=pItemdetail&PItemID=98;65;86&PLine=65

Signed-off-by Joe Xue <[email protected]>

---
 README               |    7 +++++++
 drivers/net/Makefile |    1 +
 include/netdev.h     |    1 +
 3 files changed, 9 insertions(+), 0 deletions(-)

README +7 -0

index 755d17c..c9657d8 100644
@@ -892,6 +892,13 @@ The following options need to be configured:
 			automatically converts one 32 bit word to two 16 bit
 			words you may also try CONFIG_SMC911X_32_BIT.
 
+		CONFIG_DRIVER_AX88783
+		Support for ASIX's AX88783 chip
+
+			CONFIG_AX88783_BASE
+			Define this to hold the physical address
+			of the device (I/O space)
+
 - USB Support:
 		At the moment only the UHCI host controller is
 		supported (PIP405, MIP405, MPC5200); define

drivers/net/Makefile +1 -0

index fd9d0b4..7cd6e2c 100644
@@ -30,6 +30,7 @@ COBJS-$(CONFIG_PPC4xx_EMAC) += 4xx_enet.o
 COBJS-$(CONFIG_ALTERA_TSE) += altera_tse.o
 COBJS-$(CONFIG_DRIVER_AT91EMAC) += at91_emac.o
 COBJS-$(CONFIG_DRIVER_AX88180) += ax88180.o
+COBJS-$(CONFIG_DRIVER_AX88783) += ax88783.o
 COBJS-$(CONFIG_BCM570x) += bcm570x.o
 COBJS-$(CONFIG_BCM570x) += bcm570x_autoneg.o
 COBJS-$(CONFIG_BCM570x) += 5701rls.o

drivers/net/ax88783.c new file +297 -0

297 lines not on this page · raw message

drivers/net/ax88783.h new file +100 -0

100 lines not on this page · raw message

include/netdev.h +1 -0

index 1a542e8..fddc64d 100644
@@ -44,6 +44,7 @@ int cpu_eth_init(bd_t *bis);
 int altera_tse_initialize(u8 dev_num, int mac_base,
 			  int sgdma_rx_base, int sgdma_tx_base);
 int ax88180_initialize(bd_t *bis);
+int ax88783_initialize(bd_t *bis);
 int au1x00_enet_initialize(bd_t*);
 int at91emac_register(bd_t *bis, unsigned long iobase);
 int bfin_EMAC_initialize(bd_t *bis);

Signature · 1 line

1.7.0.4

Stefano Babic

2011-02-02 18:02 UTC #1 lore raw

PATCH 1/1 Add support for ASIX's AX88783 ethernet chip

Quoted 5 lines · Joe Xue

for more information about this chip, please check:
http://www.asix.com.tw/products.php?op=pItemdetail&PItemID=98;65;86&PLine=65
Signed-off-by Joe Xue <[email protected]>

Please add a version number to your patch to make easier tracking which is your last version.

Do not forget to add always the net Maintainer to CC (Wolfgang Denk), I added him now.

drivers/net/ax88783.c line 2

Quoted 5 lines

drivers/net/ax88783.c new file +2 -0
@@ -0,0 +1,297 @@
+/*
+ *

You should drop this line

drivers/net/ax88783.c line 4

Quoted 2 lines

drivers/net/ax88783.c new file +2 -0
+
+static int ax88183_phy_initial(struct eth_device *dev)

You forget to replace the name of the function. It has still ax88183_

drivers/net/ax88783.c line 52

Quoted 6 lines

drivers/net/ax88783.c new file +6 -0

+	/* phy init */
+	tmp = readl(&reg->pcr);
+	tmp |= PCR_PHY0_RESET_CLEAR;
+
+	writel(tmp, &reg->pcr);
+	udelay(100000);

you already explained why you need such a long delay. It is not bad to add your explanation as comment here, so everyone knows your answer.

Quoted 7 lines

drivers/net/ax88783.c new file +7 -0

+static void ax88783_halt(struct eth_device *dev)
+{
+	unsigned int tmp;
+	struct ax88783_reg *reg = (struct ax88783_reg *)dev->iobase;
+	tmp = readl(&reg->pcr);
+	writel((tmp | PCR_LOOP_BACK), &reg->pcr);
+}

Joe XUE

author

2011-02-04 14:47 UTC #2 lore raw

PATCH 1/1 Add support for ASIX's AX88783 ethernet chip

Than you Stefano.

Quoted 7 lines

Date: Wed, 2 Feb 2011 19:02:11 +0100
From: sbabic at denx.de
To: lgxue at hotmail.com
CC: u-boot at lists.denx.de; wd at denx.de
Subject: Re: [U-Boot] [PATCH] Add support for ASIX's AX88783 ethernet chip
On 01/31/2011 06:42 PM, Joe Xue wrote:

Quoted 9 lines

Quoted 5 lines
for more information about this chip, please check:
http://www.asix.com.tw/products.php?op=pItemdetail&PItemID=98;65;86&PLine=65
Signed-off-by Joe Xue <[email protected]>
Please add a version number to your patch to make easier tracking which is your last version.

Will add it.

Quoted 3 lines

Do not forget to add always the net Maintainer to CC (Wolfgang Denk), I added him now.

Not exactly understand your meaning. You mean I should add wd as maintainer to my code or just add him in mail.

Quoted 8 lines

drivers/net/ax88783.c line 2
Quoted 5 lines
drivers/net/ax88783.c new file +2 -0
@@ -0,0 +1,297 @@
+/*
+ *
You should drop this line

will drop it.

Quoted 5 lines

drivers/net/ax88783.c line 4
Quoted 2 lines
drivers/net/ax88783.c new file +2 -0
+
+static int ax88183_phy_initial(struct eth_device *dev)
You forget to replace the name of the function. It has still ax88183_

will change, sorry for it.

Quoted 10 lines

drivers/net/ax88783.c line 52
Quoted 6 lines
drivers/net/ax88783.c new file +6 -0
+	/* phy init */
+	tmp = readl(&reg->pcr);
+	tmp |= PCR_PHY0_RESET_CLEAR;
+
+	writel(tmp, &reg->pcr);
+	udelay(100000);
you already explained why you need such a long delay. It is not bad to add your explanation as comment here, so everyone knows your answer.

will do.

Quoted 11 lines

drivers/net/ax88783.c line 186
Quoted 7 lines
drivers/net/ax88783.c new file +7 -0
+static void ax88783_halt(struct eth_device *dev)
+{
+	unsigned int tmp;
+	struct ax88783_reg *reg = (struct ax88783_reg *)dev->iobase;
+	tmp = readl(&reg->pcr);
+	writel((tmp | PCR_LOOP_BACK), &reg->pcr);
+}
From the name it seems you set the controller in loopback, instead of disabling it. Is it correct ?

mmn. I just make it can't receive the data outside.The other way is make it into sleep mode.

Quoted 4 lines

drivers/net/ax88783.c new file +4 -0

+
+	res = eth_getenv_enetaddr("ethaddr", dev->enetaddr);
+	if (!res) {
+		puts("Please set your MAC address!");

Quoted 9 lines

drivers/net/ax88783.c line 286
Quoted 3 lines
drivers/net/ax88783.c new file +3 -0
+		free(dev);
+		return 0;
+	}
This is wrong. A network driver should not call directly eth_getenv_enetaddr, and you do not need. If you want to check the mac addrsss, use is_valid_ether_addr() inside ax88783_init() before copying the mac to the hardware.

I refer to the newest net driver patch ftgmac100.c, it uses this function to getmac address from environmental setting. and I checked the code eth_getenv_enetaddralso call the is_valid_ether_addr(). Anyway, will change it according to your advice.

Quoted 5 lines

drivers/net/ax88783.c line 288
Quoted 2 lines
drivers/net/ax88783.c new file +2 -0
+
+	res = ax88183_phy_initial(dev);
Name must be changed.

will change.

Quoted 12 lines

drivers/net/ax88783.h line 2
Quoted 8 lines
drivers/net/ax88783.h new file +2 -0
new file mode 100644
index 0000000..09ac9ed
@@ -0,0 +1,100 @@
+/*
+ *
As explained, all headers start with copyright on the second line. Drop this line.

Yes, thank you for your patience :-)

Quoted 2 lines

Best regards,
Stefano Babic

Best wishes,
Joe

Quoted 7 lines

Signature · 6 lines

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

Stefano Babic

2011-02-04 15:23 UTC #3 lore raw

PATCH 1/1 Add support for ASIX's AX88783 ethernet chip

Quoted 7 lines · Joe XUE

Quoted 3 lines
Do not forget to add always the net Maintainer to CC (Wolfgang Denk), I added him now.
Not exactly understand your meaning. You mean I should add wd as maintainer to my code or
just add him in mail.

Only add him in mail. He is the maintainer for net code.

Quoted 6 lines

Quoted 3 lines
From the name it seems you set the controller in loopback, instead of disabling it. Is it correct ?
mmn. I just make it can't receive the data outside. The other way is make it into sleep mode.

I do not know your chip, I cannot help. However, this function is thought to put the controller in a not operative state. It is a good law to leave your hardware disable and let the kernel to reinitialize it again when your system boots.

Quoted 5 lines

I refer to the newest net driver patch ftgmac100.c, it uses this function to get
mac address from environmental setting. and I checked the code eth_getenv_enetaddr
also call the is_valid_ether_addr().

I checked this driver, too.IMHO it is wrong, and I do not know why in ftgmac100_set_mac_from_env() is called getenv instead of using directly the values in dev->enetaddr. Anyway, I missed my chance to send my comments ;-)

You can check in other drivers in drivers/net, most of them do not call getenv, because the net subsystem is responsible to do this.

Best regards,
Stefano Babic

Signature · 5 lines

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

Wolfgang Denk

2011-02-04 15:59 UTC #4 lore raw

PATCH 1/1 Add support for ASIX's AX88783 ethernet chip

In reply to Joe XUE

Dear Joe XUE,

Quoted 4 lines · In message <[email protected]> you wrote:

Quoted 4 lines
Quoted 3 lines
Signed-off-by Joe Xue <[email protected]>

Quoted 8 lines

Quoted 3 lines
Please add a version number to your patch to make easier tracking which is your last version.
Will add it.
Quoted 3 lines
Do not forget to add always the net Maintainer to CC (Wolfgang Denk), I added him now.
Not exactly understand your meaning. You mean I should add wd as maintainer to my code or just add him in mail.

Only email, please.

An easy way to not forget this is to add a "Cc:" line below your
Signed-off-by: line - then "git send-email" will automatically generate the needed Cc:s

Quoted 11 lines

Quoted 11 lines
drivers/net/ax88783.c line 186
Quoted 7 lines
drivers/net/ax88783.c new file +7 -0
+static void ax88783_halt(struct eth_device *dev)
+{
+	unsigned int tmp;
+	struct ax88783_reg *reg = (struct ax88783_reg *)dev->iobase;
+	tmp = readl(&reg->pcr);
+	writel((tmp | PCR_LOOP_BACK), &reg->pcr);
+}
From the name it seems you set the controller in loopback, instead of disabling it. Is it correct ?

Quoted 1 line

mmn. I just make it can't receive the data outside.The other way is make it into sleep mode.

Please put it in sleep mode, then.

Best regards,

Wolfgang Denk

Signature · 5 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Most legends have their basis in facts.
	-- Kirk, "And The Children Shall Lead", stardate 5029.5