net: axi_ethernet: Add driver to u-boot

drivers/net/xilinx_axi_emac.c new file +8 -0

+static void axi_ethernet_init(struct eth_device *dev)
+{
...
+	u32 timeout = 200;
+	while (timeout &&
+		(!(in_be32(dev->iobase + XAE_IS_OFFSET) & XAE_INT_MGTRDY_MASK)))
+		timeout--;
...
+static void axi_dma_init(struct eth_device *dev)
+{
...

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

+static void setup_mac(struct eth_device *dev)
+{
+	/* Set the MAC address */
+	int val = ((dev->enetaddr[3] << 24) | (dev->enetaddr[2] << 16) |
+		(dev->enetaddr[1] << 8) | (dev->enetaddr[0]));
+	out_be32(dev->iobase + XAE_UAW0_OFFSET, val);
+

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

+	/* Setup the BD. */
+	memset((void *) &rx_bd, 0, sizeof(axidma_bd));
+	rx_bd.next = (u32)&rx_bd;
+	rx_bd.phys = (u32)&RxFrame;

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

+int xilinx_axiemac_initialize(bd_t *bis, int base_addr, int dma_addr)
+{
+	struct eth_device *dev;
+	struct axidma_priv *dma;
+
+	dev = malloc(sizeof(*dev));

Quoted 18 lines · On Monday, February 28, 2011 04:40:33 Michal Simek wrote:

drivers/net/xilinx_axi_emac.c new file +13 -0

+static void axi_ethernet_init(struct eth_device *dev)
+{
...
+	u32 timeout = 200;
+	while (timeout &&
+		(!(in_be32(dev->iobase + XAE_IS_OFFSET) & XAE_INT_MGTRDY_MASK)))
+		timeout--;
...
+static void axi_dma_init(struct eth_device *dev)
+{
...
+	/* At the initialization time, hardware should finish reset quickly */
+	u32 timeout = 500;
+	while (timeout--) {
+		/* Check transmit/receive channel */
+		/* Reset is done when the reset bit is low */

drivers/net/xilinx_axi_emac.c line 337

Quoted 5 lines

drivers/net/xilinx_axi_emac.c new file +5 -0

+		if (!(dma->dmatx->control | dma->dmarx->control)
+						& XAXIDMA_CR_RESET_MASK)
+			break;
+		timeout -= 1;
+	}

shouldnt this be using a timer rather than some constant that will change the actual timeout length based on the speed of the core ?

Quoted 13 lines

drivers/net/xilinx_axi_emac.c new file +12 -0

+static void setup_mac(struct eth_device *dev)
+{
+	/* Set the MAC address */
+	int val = ((dev->enetaddr[3] << 24) | (dev->enetaddr[2] << 16) |
+		(dev->enetaddr[1] << 8) | (dev->enetaddr[0]));
+	out_be32(dev->iobase + XAE_UAW0_OFFSET, val);
+
+	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
+	val |= in_be32(dev->iobase + XAE_UAW1_OFFSET)
+						& ~XAE_UAW1_UNICASTADDR_MASK;
+	out_be32(dev->iobase + XAE_UAW1_OFFSET, val);
+}

drivers/net/xilinx_axi_emac.c line 358

Quoted 5 lines

...

drivers/net/xilinx_axi_emac.c new file +3 -0

+static int axiemac_init(struct eth_device *dev, bd_t * bis)
+{
...
+	setup_mac(dev);

this should be moved to eth_device.write_hwaddr in the initialize function. then the common layers will call setup_mac for you as needed.

Quoted 5 lines

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

+	/* Setup the BD. */
+	memset((void *) &rx_bd, 0, sizeof(axidma_bd));
+	rx_bd.next = (u32)&rx_bd;
+	rx_bd.phys = (u32)&RxFrame;

drivers/net/xilinx_axi_emac.c line 381

Quoted 10 lines

drivers/net/xilinx_axi_emac.c new file +10 -0

+	rx_bd.cntrl = sizeof(RxFrame);
+	/* Flush the last BD so DMA core could see the updates */
+	flush_cache((u32)&rx_bd, sizeof(axidma_bd));
+
+	/* it is necessary to flush RxFrame because if you don't do it
+	 * then cache can contain uninitialized data */
+	flush_cache((u32)&RxFrame, sizeof(RxFrame));
+
+	/* Rx BD is ready - start */
+	dma->dmarx->tail = (u32)&rx_bd;

hmm, shouldnt this be done only in the recv func ?

drivers/net/xilinx_axi_emac.c

Quoted 2 lines

drivers/net/xilinx_axi_emac.c new file +1 -0

+	return 1;

a bunch of these funcs return 1 when i'm pretty sure they should be 0

Quoted 13 lines

drivers/net/xilinx_axi_emac.c new file +12 -0

+int xilinx_axiemac_initialize(bd_t *bis, int base_addr, int dma_addr)
+{
+	struct eth_device *dev;
+	struct axidma_priv *dma;
+
+	dev = malloc(sizeof(*dev));
+	if (dev == NULL)
+		hang();
+
+	memset(dev, 0, sizeof(*dev));
+	sprintf(dev->name, "Xilinx_AxiEmac");
+

drivers/net/xilinx_axi_emac.c line 520

Quoted 4 lines

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

+	dev->iobase = base_addr;
+	dma = dev->priv;
+	dma->dmatx = dma_addr;
+	dma->dmarx = (u32)dma->dmatx + 0x30; /* rx channel offset */

hmm, this is weird. you just memset(dev) to 0, and then used dev->priv without assigning it storage. so you're scribbling on top of address 0 with your dma struct here arent you ?

Quoted 4 lines · Mike Frysinger

Review

Quoted 1 line · On Monday, February 28, 2011 04:40:33 Michal Simek wrote:

(quoted diff) +1 -0

+	return 1;

a bunch of these funcs return 1 when i'm pretty sure they should be 0

init function is called from eth.c:eth_init(). From the code below you see that return can be >=0.

Quoted 12 lines

Quoted 12 lines

drivers/net/xilinx_axi_emac.c new file +12 -0

+int xilinx_axiemac_initialize(bd_t *bis, int base_addr, int dma_addr)
+{
+	struct eth_device *dev;
+	struct axidma_priv *dma;
+
+	dev = malloc(sizeof(*dev));
+	if (dev == NULL)
+		hang();
+
+	memset(dev, 0, sizeof(*dev));
+	sprintf(dev->name, "Xilinx_AxiEmac");
+

Quoted 8 lines

drivers/net/xilinx_axi_emac.c line 520

Quoted 4 lines

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

+	dev->iobase = base_addr;
+	dma = dev->priv;
+	dma->dmatx = dma_addr;
+	dma->dmarx = (u32)dma->dmatx + 0x30; /* rx channel offset */

hmm, this is weird. you just memset(dev) to 0, and then used dev->priv without assigning it storage. so you're scribbling on top of address 0 with your dma struct here arent you ?

dev contains:
iobase - axi emac baseaddr
init, halt, send, recv pointers to functions
and pointer priv
+ others

I need to clear it that's why memset.

dma controller is special IP that's why I use priv for storing pointer to axidma_priv structure which contains two pointers to dma for master to slave and slave to master directions (rx and tx if you like). As I wrote dma controller is different IP that's why there are different addresses. axi_dma has offset between channels which is 0x30.
I used structures for hw access.

Quoted 6 lines · On Monday, February 28, 2011 13:50:55 Michal Simek wrote:

Quoted 3 lines · Mike Frysinger

Review

Quoted 1 line · On Monday, February 28, 2011 04:40:33 Michal Simek wrote:

(quoted diff) +1 -0

+	return 1;

a bunch of these funcs return 1 when i'm pretty sure they should be 0

init function is called from eth.c:eth_init(). From the code below you see that return can be >=0.

funny enough, that func in your patch is returning 0 when it should be 1. it doesnt explain why your recv/send are returning 1 when it should be 0.

the init func is a special beast -- it returns the # of devices registered, not "1 is success".

Quoted 26 lines

Quoted 12 lines

Quoted 12 lines

drivers/net/xilinx_axi_emac.c new file +11 -0

+int xilinx_axiemac_initialize(bd_t *bis, int base_addr, int dma_addr)
+{
+	struct eth_device *dev;
+	struct axidma_priv *dma;
+
+	dev = malloc(sizeof(*dev));
+	if (dev == NULL)
+		hang();
+
+	memset(dev, 0, sizeof(*dev));
+	sprintf(dev->name, "Xilinx_AxiEmac");

Quoted 8 lines

drivers/net/xilinx_axi_emac.c line 520

Quoted 5 lines

drivers/net/xilinx_axi_emac.c new file +5 -0

+
+	dev->iobase = base_addr;
+	dma = dev->priv;
+	dma->dmatx = dma_addr;
+	dma->dmarx = (u32)dma->dmatx + 0x30; /* rx channel offset */

hmm, this is weird. you just memset(dev) to 0, and then used dev->priv without assigning it storage. so you're scribbling on top of address 0 with your dma struct here arent you ?

dev contains:
iobase - axi emac baseaddr
init, halt, send, recv pointers to functions
and pointer priv
+ others

Quoted 8 lines

I need to clear it that's why memset.

dma controller is special IP that's why I use priv for storing pointer to axidma_priv structure which contains two pointers to dma for master to slave and slave to master directions (rx and tx if you like). As I wrote dma controller is different IP that's why there are different addresses. axi_dma has offset between channels which is 0x30.
I used structures for hw access.

ok, but i think you missed my point. let me use this example:
dev->priv = 0; /* the memset */
dma = dev->priv;
dma->dmatx = dma_addr;

you're doing a NULL pointer deref here.

If both functions should return 0 then any code should check it and all others drivers should be fixed.

Quoted 2 lines

the init func is a special beast -- it returns the # of devices registered, not "1 is success".

Where is it written?

ep93xx_eth.c returns also 1.
Anyway if is number of registered devices, "1" should means one registered device. If zero means one registered device then please point me to that documentation.

Quoted 2 lines · On Tuesday, March 01, 2011 03:00:47 Michal Simek wrote:

If both functions should return 0 then any code should check it and all others drivers should be fixed.

i agree, but that doesnt mean new code should knowingly be left broken

Quoted 4 lines

Quoted 3 lines

the init func is a special beast -- it returns the # of devices registered, not "1 is success".

Where is it written?

atm, only the mailing list. i have a local patch that i havent gotten around to pushing up yet for the README.drivers.eth.

Quoted 4 lines

ep93xx_eth.c returns also 1.
Anyway if is number of registered devices, "1" should means one registered device. If zero means one registered device then please point me to that documentation.

the change hasnt been ported to all drivers yet. but new drivers should be doing it as i described.

also, you should change the "hang()" to "return 0" in the init func.

Quoted 5 lines · Mike Frysinger

Quoted 2 lines · On Tuesday, March 01, 2011 03:00:47 Michal Simek wrote:

If both functions should return 0 then any code should check it and all others drivers should be fixed.

i agree, but that doesnt mean new code should knowingly be left broken

I agree that make no sense do not fix it right now.

Quoted 7 lines

Quoted 4 lines

ep93xx_eth.c returns also 1.
Anyway if is number of registered devices, "1" should means one registered device. If zero means one registered device then please point me to that documentation.

the change hasnt been ported to all drivers yet. but new drivers should be doing it as i described.

How does it look like phy lib u-boot support?

Quoted 1 line

also, you should change the "hang()" to "return 0" in the init func.

Are you sure return 0 which should mean success. Anything different from 0 seems to me relevant.

I maintain emaclite driver and none tell me this that's why the process is so slow. I believe if you release that documentation, which you are talking about, then others will clean/test their drivers.

Quoted 6 lines · On Tuesday, March 01, 2011 03:34:38 Michal Simek wrote:

Quoted 4 lines · Mike Frysinger

Quoted 2 lines · On Tuesday, March 01, 2011 03:00:47 Michal Simek wrote:

If both functions should return 0 then any code should check it and all others drivers should be fixed.

i agree, but that doesnt mean new code should knowingly be left broken

I agree that make no sense do not fix it right now.

err, your new driver should return the correct values. what higher levels do or do not check does not matter, and whether other drivers do it correctly does not matter.

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

ep93xx_eth.c returns also 1.
Anyway if is number of registered devices, "1" should means one registered device. If zero means one registered device then please point me to that documentation.

the change hasnt been ported to all drivers yet. but new drivers should be doing it as i described.

How does it look like phy lib u-boot support?

i dont know what you mean ... how is phylib relevant to this ? or are you just asking in general ?

Quoted 4 lines

Quoted 2 lines

also, you should change the "hang()" to "return 0" in the init func.

Are you sure return 0 which should mean success. Anything different from 0 seems to me relevant.

as i said, the initialize function is not returning "success" or "failure". it is returning "# of devices registered". if you cannot register any, you should return 0. having the boot process fail because of network issues doesnt make much sense when u-boot can do quite a lot without the network. including updating itself via other means.

Quoted 1 line

Quoted 1 line

also, you should change the "hang()" to "return 0" in the init func.

Quoted 4 lines

I maintain emaclite driver and none tell me this that's why the process is so slow. I believe if you release that documentation, which you are talking about, then others will clean/test their drivers.

the behavior i describe isnt a decision i made. it was made by the previous net maintainer and agreed upon by others in the discussion.

Quoted 3 lines · Mike Frysinger

Quoted 1 line · On Tuesday, March 01, 2011 03:34:38 Michal Simek wrote:

How does it look like phy lib u-boot support?

Quoted 2 lines

i dont know what you mean ... how is phylib relevant to this ? or are you just asking in general ?

Ben wanted to create general phy lib support and remove all phy specific things from net drivers. I hope you see that connection because there is also phy part.
If phy lib support is in progress (which probably not) then I would change the driver to support it.

Quoted 10 lines

Quoted 4 lines

Quoted 1 line

also, you should change the "hang()" to "return 0" in the init func.

Are you sure return 0 which should mean success. Anything different from 0 seems to me relevant.

as i said, the initialize function is not returning "success" or "failure". it is returning "# of devices registered". if you cannot register any, you should return 0. having the boot process fail because of network issues doesnt make much sense when u-boot can do quite a lot without the network. including updating itself via other means.

Interesting.
1. you talked about hang() in initialize function(not dev->init) and for me xilinx_axiemac_initialize Initialize function is called from board_eth_init which is called from eth_initialize(eth.c)

There is this part of code

	if (board_eth_init != __def_eth_init) {
		if (board_eth_init(bis) < 0)
			printf("Board Net Initialization Failed\n");

If initialization failed the return value is < 0.
That's why hang() should be changed to return -1 and doesn't matter how many device there are.

If you write:

Quoted 1 line

Quoted 1 line

Quoted 1 line

also, you should change the "hang()" to "return 0" in the init func.

(hang is only in xilinx_axiemac_initialize) and should be changed which I agree.

If you propose any change which I should do, I expect that if you are focus on blackfin that you have done that changes in all blackfin eth drivers. For example in bfin_mac.c where hang is also used.

Quoted 6 lines

Quoted 3 lines

I maintain emaclite driver and none tell me this that's why the process is so slow. I believe if you release that documentation, which you are talking about, then others will clean/test their drivers.

the behavior i describe isnt a decision i made. it was made by the previous net maintainer and agreed upon by others in the discussion.

Ok. If that decision was made than I expect that should be written somewhere in doc. I know it is boring to write any documentation but I expect that if any decision was made then is common that general code will be changed.

1. hang() -> return -1

2. driver initialize function (setup dev functions, driver name, priv, etc) return -1 - if initialize failed
return 0 - on success

3. dev->init
return -1 - if init failed
return 0 - on success

(here you are saying should be return # of devices)

4. dev->recv
return -1 - failed
return 0 - packet not received
return >0 - success - packet lenght

5. dev->send
return -1 - failed
return 0 - success

6. dev->write_hwaddr
return -1 - failed
return 0 - success

Quoted 18 lines

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

also, you should change the "hang()" to "return 0" in the init func.

Are you sure return 0 which should mean success. Anything different from 0 seems to me relevant.

as i said, the initialize function is not returning "success" or "failure". it is returning "# of devices registered". if you cannot register any, you should return 0. having the boot process fail because of network issues doesnt make much sense when u-boot can do quite a lot without the network. including updating itself via other means.

Interesting.
1. you talked about hang() in initialize function(not dev->init) and for me xilinx_axiemac_initialize Initialize function is called from board_eth_init which is called from eth_initialize(eth.c)

There is this part of code

	if (board_eth_init != __def_eth_init) {
		if (board_eth_init(bis) < 0)
			printf("Board Net Initialization Failed\n");

Quoted 3 lines

If initialization failed the return value is < 0.
That's why hang() should be changed to return -1 and doesn't matter how many device there are.

this detail isnt currently ironed out, so if you wanted to change the "hang()" into "return -1" or "return 0", that is fine.

Quoted 7 lines

If you write:

Quoted 1 line

Quoted 1 line

Quoted 1 line

also, you should change the "hang()" to "return 0" in the init func.

(hang is only in xilinx_axiemac_initialize) and should be changed which I agree.

Quoted 3 lines

If you propose any change which I should do, I expect that if you are focus on blackfin that you have done that changes in all blackfin eth drivers. For example in bfin_mac.c where hang is also used.

incorrect code in other drivers (including bfin_mac) is not justification for adding incorrect code to new drivers. bfin_mac.c's call to hang() is wrong too in the current code base.

Quoted 10 lines

Quoted 6 lines

Quoted 4 lines

I maintain emaclite driver and none tell me this that's why the process is so slow. I believe if you release that documentation, which you are talking about, then others will clean/test their drivers.

the behavior i describe isnt a decision i made. it was made by the previous net maintainer and agreed upon by others in the discussion.

Ok. If that decision was made than I expect that should be written somewhere in doc. I know it is boring to write any documentation but I expect that if any decision was made then is common that general code will be changed.

obviously that is true, but docs only get written/updated when someone is so inclined to do the work. the existing doc only exists because i felt like writing at the time. ive never been the net maintainer and thus "obligated" to write net documentation. i just got tired of people doing it wrong and no one knowing what should be going on.

Quoted 1 line

1. hang() -> return -1

ok

Quoted 3 lines

2. driver initialize function (setup dev functions, driver name, priv, etc) return -1 - if initialize failed
return 0 - on success

no, "return 1" when the device has successfully registered

Quoted 3 lines

3. dev->init
return -1 - if init failed
return 0 - on success

ok

Quoted 1 line

(here you are saying should be return # of devices)

no, i think you confused "initialize" with "init" in my feedback

Quoted 5 lines

4. dev->recv
return -1 - failed
return 0 - packet not received
return >0 - success - packet lenght

"return 0" is still "success" in the sense that there is nothing to do, but that nuance doesnt matter

Quoted 8 lines

5. dev->send
return -1 - failed
return 0 - success

6. dev->write_hwaddr
return -1 - failed
return 0 - success

ok

Quoted 8 lines · Mike Frysinger

Quoted 3 lines

3. dev->init
return -1 - if init failed
return 0 - on success

ok

Quoted 1 line

(here you are saying should be return # of devices)

Quoted 1 line

no, i think you confused "initialize" with "init" in my feedback

ok. From my point of view is better do initialize only for one device and not to registered all device in the driver. IMHO this functionality should be done by board specific net function in board_eth_init. Not sure if this covers all cases which can happen. But anyway if the driver initialize several drivers with the same type then you are losing flexibility.

To finish this discuss - here is what you think that it is correct. ad 2)
return -1 - if initialize failed
return 0 - never return
return >0 - # of devices

Quoted 6 lines · On Monday, February 28, 2011 13:50:55 Michal Simek wrote:

Quoted 3 lines · Mike Frysinger

Review

Quoted 1 line · On Monday, February 28, 2011 04:40:33 Michal Simek wrote:

(quoted diff) +1 -0

+	return 1;

a bunch of these funcs return 1 when i'm pretty sure they should be 0

init function is called from eth.c:eth_init(). From the code below you see that return can be >=0.

funny enough, that func in your patch is returning 0 when it should be 1. it doesnt explain why your recv/send are returning 1 when it should be 0.

Quoted 10 lines · Mike Frysinger

Quoted 8 lines · On Monday, February 28, 2011 13:50:55 Michal Simek wrote:

Quoted 4 lines · Mike Frysinger

Review

Quoted 1 line · On Monday, February 28, 2011 04:40:33 Michal Simek wrote:

(quoted diff) +1 -0

+	return 1;

a bunch of these funcs return 1 when i'm pretty sure they should be 0

init function is called from eth.c:eth_init(). From the code below you see that return can be >=0.

Quoted 3 lines

funny enough, that func in your patch is returning 0 when it should be 1.
it doesnt explain why your recv/send are returning 1 when it should be
0.

BTW: blackin emac recv functions return 0, -1 and length values. Is it correct?

Quoted 12 lines

drivers/net/xilinx_axi_emac.c new file +12 -0

+static void setup_mac(struct eth_device *dev)
+{
+	/* Set the MAC address */
+	int val = ((dev->enetaddr[3] << 24) | (dev->enetaddr[2] << 16) |
+		(dev->enetaddr[1] << 8) | (dev->enetaddr[0]));
+	out_be32(dev->iobase + XAE_UAW0_OFFSET, val);
+
+	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
+	val |= in_be32(dev->iobase + XAE_UAW1_OFFSET)
+						& ~XAE_UAW1_UNICASTADDR_MASK;
+	out_be32(dev->iobase + XAE_UAW1_OFFSET, val);
+}

drivers/net/xilinx_axi_emac.c line 358

Quoted 5 lines

...

drivers/net/xilinx_axi_emac.c new file +3 -0

+static int axiemac_init(struct eth_device *dev, bd_t * bis)
+{
...
+	setup_mac(dev);

this should be moved to eth_device.write_hwaddr in the initialize function. then the common layers will call setup_mac for you as needed.

Quoted 12 lines

Quoted 12 lines

drivers/net/xilinx_axi_emac.c new file +12 -0

+static void setup_mac(struct eth_device *dev)
+{
+	/* Set the MAC address */
+	int val = ((dev->enetaddr[3] << 24) | (dev->enetaddr[2] << 16) |
+		(dev->enetaddr[1] << 8) | (dev->enetaddr[0]));
+	out_be32(dev->iobase + XAE_UAW0_OFFSET, val);
+
+	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
+	val |= in_be32(dev->iobase + XAE_UAW1_OFFSET)
+						& ~XAE_UAW1_UNICASTADDR_MASK;
+	out_be32(dev->iobase + XAE_UAW1_OFFSET, val);
+}

Quoted 8 lines

drivers/net/xilinx_axi_emac.c line 358

Quoted 5 lines

...

drivers/net/xilinx_axi_emac.c new file +3 -0

+static int axiemac_init(struct eth_device *dev, bd_t * bis)
+{
...
+	setup_mac(dev);

this should be moved to eth_device.write_hwaddr in the initialize function. then the common layers will call setup_mac for you as needed.

I am not going to use write_hwaddr function because axi emac needs some initialization before you can write a mac addr that's why I will keep it in the same location as is.

Quoted 19 lines · On Tuesday, March 01, 2011 03:19:12 Michal Simek wrote:

Quoted 19 lines

Quoted 7 lines

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

+static void setup_mac(struct eth_device *dev)
+{
+	/* Set the MAC address */
+	int val = ((dev->enetaddr[3] << 24) | (dev->enetaddr[2] << 16) |
+		(dev->enetaddr[1] << 8) | (dev->enetaddr[0]));
+	out_be32(dev->iobase + XAE_UAW0_OFFSET, val);
+

drivers/net/xilinx_axi_emac.c line 358

Quoted 10 lines

drivers/net/xilinx_axi_emac.c new file +8 -0

+	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
+	val |= in_be32(dev->iobase + XAE_UAW1_OFFSET)
+						& ~XAE_UAW1_UNICASTADDR_MASK;
+	out_be32(dev->iobase + XAE_UAW1_OFFSET, val);
+}
...
+static int axiemac_init(struct eth_device *dev, bd_t * bis)
+{
...
+	setup_mac(dev);

this should be moved to eth_device.write_hwaddr in the initialize function. then the common layers will call setup_mac for you as needed.

Quoted 3 lines

I am not going to use write_hwaddr function because axi emac needs some initialization before you can write a mac addr that's why I will keep it in the same location as is.

please add explicit comments to the setup_mac() func then so someone else doesnt go changing things without taking this into consideration first

Quoted 26 lines · Mike Frysinger

Quoted 24 lines · On Tuesday, March 01, 2011 03:19:12 Michal Simek wrote:

Quoted 12 lines

Quoted 12 lines

drivers/net/xilinx_axi_emac.c new file +12 -0

+static void setup_mac(struct eth_device *dev)
+{
+	/* Set the MAC address */
+	int val = ((dev->enetaddr[3] << 24) | (dev->enetaddr[2] << 16) |
+		(dev->enetaddr[1] << 8) | (dev->enetaddr[0]));
+	out_be32(dev->iobase + XAE_UAW0_OFFSET, val);
+
+	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
+	val |= in_be32(dev->iobase + XAE_UAW1_OFFSET)
+						& ~XAE_UAW1_UNICASTADDR_MASK;
+	out_be32(dev->iobase + XAE_UAW1_OFFSET, val);
+}

Quoted 8 lines

drivers/net/xilinx_axi_emac.c line 358

Quoted 5 lines

...

drivers/net/xilinx_axi_emac.c new file +3 -0

+static int axiemac_init(struct eth_device *dev, bd_t * bis)
+{
...
+	setup_mac(dev);

this should be moved to eth_device.write_hwaddr in the initialize function. then the common layers will call setup_mac for you as needed.

I am not going to use write_hwaddr function because axi emac needs some initialization before you can write a mac addr that's why I will keep it in the same location as is.

Quoted 2 lines

please add explicit comments to the setup_mac() func then so someone else doesnt go changing things without taking this into consideration first

No problem to write it.

I have experience with several network IPs but I think less amount than you.

But anyway I think it is common that device must be in proper state to be able to setup mac for most of them.
From my simple test I see that dev->write_hwaddr is called before
dev->init which looks weird.