| Message ID | 1298886033-3350-1-git-send-email-monstr@monstr.eu |
|---|---|
| State | Superseded |
| Headers | show |
On Monday, February 28, 2011 04:40:33 Michal Simek wrote: > --- /dev/null > +++ b/drivers/net/xilinx_axi_emac.c > +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 */ > + 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 ? > +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); > +} > ... > +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. > + /* Setup the BD. */ > + memset((void *) &rx_bd, 0, sizeof(axidma_bd)); > + rx_bd.next = (u32)&rx_bd; > + rx_bd.phys = (u32)&RxFrame; > + 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 ? > + return 1; a bunch of these funcs return 1 when i'm pretty sure they should be 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"); > + > + 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 ? -mike
Mike Frysinger wrote: > On Monday, February 28, 2011 04:40:33 Michal Simek wrote: >> --- /dev/null >> +++ b/drivers/net/xilinx_axi_emac.c >> +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 */ >> + 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 ? it could/should be. > >> +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); >> +} >> ... >> +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. ok. I haven't know about this possibility. Will fix it. > >> + /* Setup the BD. */ >> + memset((void *) &rx_bd, 0, sizeof(axidma_bd)); >> + rx_bd.next = (u32)&rx_bd; >> + rx_bd.phys = (u32)&RxFrame; >> + 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 ? For transmit I decided to setup BD directly in function axiemac_send. For recv func BD must be setup before RX is enable that's why I have done it. I haven't tried to setup BD and enable RX in axiemac_recv function. Anyway axiemac_recv is called after axiemac_send and if I setup BD in that it will take some time to get that packet that's why I think it is better to setup BD and enable RX in init function. > >> + 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. if (eth_current->init(eth_current,bis) >= 0) { eth_current->state = ETH_STATE_ACTIVE; return 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"); >> + >> + 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. Thanks for your comments, Michal
On Monday, February 28, 2011 13:50:55 Michal Simek wrote: > Mike Frysinger wrote: > > On Monday, February 28, 2011 04:40:33 Michal Simek wrote: > >> + 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". > >> +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"); > >> + > >> + 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. 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. -mike
Mike Frysinger wrote: > On Monday, February 28, 2011 13:50:55 Michal Simek wrote: >> Mike Frysinger wrote: >>> On Monday, February 28, 2011 04:40:33 Michal Simek wrote: >>>> + 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. for recv: Doesn't matter what you return because there is no return value checking at all. $ grep -rn "eth_rx" net/ net/eth.c:398:int eth_rx(void) net/eth.c:433: eth_rx(); net/net.c:480: eth_rx(); + I think that doesn't matter what you return. For recv I am returning packet length as for example uli526x, ep93xx_eth and I believe other do. The truth is that in README.drivers.eth is suggested to return 0 for recv. for sending - yes, there should be 0 but why this even work. net/net.c:252: (void) eth_send (NetTxPacket, (pkt - NetTxPacket) + ARP_HDR_SIZE); net/net.c:641: (void) eth_send(pkt, len); net/net.c:686: (void) eth_send(NetTxPacket, (pkt - NetTxPacket) + IP_HDR_SIZE + len); net/net.c:970: (void) eth_send(NetTxPacket, (uchar *)s - NetTxPacket); net/net.c:1452: (void) eth_send((uchar *)et, (pkt - (uchar *)et) + ARP_HDR_SIZE); net/net.c:1484: (void) eth_send(NetArpWaitTxPacket, NetArpWaitTxPacketSize); net/net.c:1618: (void) eth_send((uchar *)et, ETHER_HDR_SIZE + len); + api/api_net.c:100: return eth_send(buf, len); Only api_net checks return values. If both functions should return 0 then any code should check it and all others drivers should be fixed. > > 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. Does any code even work with return value? I expect that if I use bad return values that network driver shouldn't work but it works. > >>>> +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"); >>>> + >>>> + 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. > > 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. you are right. I have to fix it. Thanks, Michal
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 > > 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. > 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. -mike
>> +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); >> +} >> ... >> +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. Michal
On Tuesday, March 01, 2011 03:19:12 Michal Simek wrote: > >> +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); > >> +} > >> ... > >> +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. please add explicit comments to the setup_mac() func then so someone else doesnt go changing things without taking this into consideration first -mike
Mike Frysinger wrote: > 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. > >>> 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. > >> 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? > > 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. you too. int bfin_EMAC_initialize(bd_t *bis) { struct eth_device *dev; dev = malloc(sizeof(*dev)); if (dev == NULL) hang(); 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. Michal
Mike Frysinger wrote: > On Monday, February 28, 2011 13:50:55 Michal Simek wrote: >> Mike Frysinger wrote: >>> On Monday, February 28, 2011 04:40:33 Michal Simek wrote: >>>> + 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. BTW: blackin emac recv functions return 0, -1 and length values. Is it correct? static int bfin_EMAC_recv(struct eth_device *dev) { int length = 0; for (;;) { if ((rxbuf[rxIdx]->StatusWord & RX_COMP) == 0) { length = -1; break; } ... return length; } Michal
Mike Frysinger wrote: > On Tuesday, March 01, 2011 03:19:12 Michal Simek wrote: >>>> +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); >>>> +} >>>> ... >>>> +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. > > 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. Michal
On Tuesday, March 01, 2011 03:34:38 Michal Simek wrote: > Mike Frysinger wrote: > > 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. > >> 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 ? > > 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. > 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. -mike
On Tuesday, March 01, 2011 03:37:30 Michal Simek wrote: > Mike Frysinger wrote: > > On Monday, February 28, 2011 13:50:55 Michal Simek wrote: > >> Mike Frysinger wrote: > >>> On Monday, February 28, 2011 04:40:33 Michal Simek wrote: > >>>> + 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. > > BTW: blackin emac recv functions return 0, -1 and length values. > Is it correct? looking at net/eth.c, -1 is an error return value. add in current docs which says 0 is a success return value. that leaves all other values as currently undocumented. if a driver survey suggested that returning the length was useful info, then perhaps the documentation could be updated. but as you noted, that return value isnt currently checked. so for now, you could change the recv func to return the length rather than 0 and i wouldnt complain, but always returning 1 is wrong. -mike
On Tuesday, March 01, 2011 03:46:16 Michal Simek wrote: > Mike Frysinger wrote: > > On Tuesday, March 01, 2011 03:19:12 Michal Simek wrote: > >>>> +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); > >>>> +} > >>>> ... > >>>> +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. > > > > 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. i think you unduly credit me with more experience than i actually have > 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. i dont think this is weird ... usually the MAC address is simply memory mapped storage, so the state of the EMAC controller itself doesnt matter. you can peek/poke the contents of that MAC address storage at any time. but i dont know what weirdness your controller has in place to make this not work too. -mike
Mike Frysinger wrote: > On Tuesday, March 01, 2011 03:34:38 Michal Simek wrote: >> Mike Frysinger wrote: >>> 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. Comment below. > >>>> 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 ? 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. > >>> 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: >>> 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. > >> 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. The changes can be from top to down. If general eth code checks return values then driver which returns wrong return codes won't work. I am not arguing that my driver shouldn't return correct values!!! That's not my point. If you start arguing that my driver has some faults, I am open to fix it and I really appreciate your comments. I think it is good time to summarize all that changes we discuss: (Please correct it if you think that it is wrong). (Our communication is getting messy a little bit) 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 Thanks, Michal Michal Simek, Ing. (M.Eng) w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/ Microblaze U-BOOT custodian
On Tuesday, March 01, 2011 04:29:21 Michal Simek wrote: > Mike Frysinger wrote: > > On Tuesday, March 01, 2011 03:34:38 Michal Simek wrote: > >> 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 ? > > 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. yes, but that isnt relevant to any of the feedback ive given for this patch > >>> 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. this detail isnt currently ironed out, so if you wanted to change the "hang()" into "return -1" or "return 0", that is fine. > If you write: > >>> 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. 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. > >> 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. > 1. hang() -> return -1 ok > 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 > 3. dev->init > return -1 - if init failed > return 0 - on success ok > (here you are saying should be return # of devices) no, i think you confused "initialize" with "init" in my feedback > 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 > 5. dev->send > return -1 - failed > return 0 - success > > 6. dev->write_hwaddr > return -1 - failed > return 0 - success ok -mike
Mike Frysinger wrote: >>>>> 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. > > this detail isnt currently ironed out, so if you wanted to change the "hang()" > into "return -1" or "return 0", that is fine. ok. > >> If you write: >> >>> 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. > > 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. I am not arguing that I shouldn't fix this driver. I was/am still open to any suggestions which help me to improve microblaze code/drivers. I am just saying that if you know that there is something wrong (even in blackfin part) and it is easy to fix it, then it will be good to fix it. > >>>> 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. > >> 1. hang() -> return -1 > > ok > >> 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 > >> 3. dev->init >> return -1 - if init failed >> return 0 - on success > > ok > >> (here you are saying should be return # of devices) > > 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. I see at least one reason why will be better not to do it: Initialize only one device which can speedup bootup time and maybe security reasons. Number of registered device can be counted through eth_register function or similar If you and others like to return number of devices I will return it that's no problem. But I don't want to loose flexibility which is the reason why FPGAs are great. 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 Is it ok for you? > >> 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 agree. > >> 5. dev->send >> return -1 - failed >> return 0 - success >> >> 6. dev->write_hwaddr >> return -1 - failed >> return 0 - success > > ok ok. great. Michal
On Friday, March 04, 2011 05:09:53 Michal Simek wrote: > Mike Frysinger wrote: > >> 3. dev->init > >> return -1 - if init failed > >> return 0 - on success > > > > ok > > > >> (here you are saying should be return # of devices) > > > > 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. no, that isnt what i meant. the initialize function is allowed to return more than one because a single part might support more than one interface. think of MAC switches or multiple PHYs hooked up to a single MAC. in neither of these cases should the board_eth_init func need to call a driver's initialize function multiple times imo. -mike
On Friday, March 04, 2011 05:09:53 Michal Simek wrote: > 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 oh, and to clarify on the "return 0", i think it's conceivable that if a device might not exist, the initialize function may return 0. whether any driver currently utilizes this is a different issue. -mike
diff --git a/board/xilinx/microblaze-generic/microblaze-generic.c b/board/xilinx/microblaze-generic/microblaze-generic.c index 0102c9e..b38cf3d 100644 --- a/board/xilinx/microblaze-generic/microblaze-generic.c +++ b/board/xilinx/microblaze-generic/microblaze-generic.c @@ -73,6 +73,10 @@ int board_eth_init(bd_t *bis) { int ret = 0; +#ifdef CONFIG_XILINX_AXIEMAC + ret |= xilinx_axiemac_initialize(bis, XILINX_AXIEMAC_BASEADDR, + XILINX_AXIDMA_BASEADDR); +#endif #ifdef CONFIG_XILINX_EMACLITE ret |= xilinx_emaclite_initialize(bis, XILINX_EMACLITE_BASEADDR); #endif diff --git a/drivers/net/Makefile b/drivers/net/Makefile index d720e27..4c77062 100644 --- a/drivers/net/Makefile +++ b/drivers/net/Makefile @@ -83,6 +83,7 @@ COBJS-$(CONFIG_TSEC_ENET) += tsec.o COBJS-$(CONFIG_TSI108_ETH) += tsi108_eth.o COBJS-$(CONFIG_ULI526X) += uli526x.o COBJS-$(CONFIG_VSC7385_ENET) += vsc7385.o +COBJS-$(CONFIG_XILINX_AXIEMAC) += xilinx_axi_emac.o COBJS-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o COBJS-$(CONFIG_XILINX_LL_TEMAC) += xilinx_ll_temac.o diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c new file mode 100644 index 0000000..75ebac7 --- /dev/null +++ b/drivers/net/xilinx_axi_emac.c @@ -0,0 +1,529 @@ +/* + * Copyright (C) 2011 Michal Simek <monstr@monstr.eu> + * Copyright (C) 2011 PetaLogix + * Copyright (C) 2010 Xilinx, Inc. All rights reserved. + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <config.h> +#include <common.h> +#include <net.h> +#include <malloc.h> +#include <asm/io.h> + +/* Axi Ethernet registers offset */ +#define XAE_IS_OFFSET 0x0000000C /* Interrupt status */ +#define XAE_IE_OFFSET 0x00000014 /* Interrupt enable */ +#define XAE_RCW1_OFFSET 0x00000404 /* Rx Configuration Word 1 */ +#define XAE_TC_OFFSET 0x00000408 /* Tx Configuration */ +#define XAE_EMMC_OFFSET 0x00000410 /* EMAC mode configuration */ +#define XAE_MDIO_MC_OFFSET 0x00000500 /* MII Management Config */ +#define XAE_MDIO_MCR_OFFSET 0x00000504 /* MII Management Control */ +#define XAE_MDIO_MRD_OFFSET 0x0000050C /* MII Management Read Data */ + +/* Link setup */ +#define XAE_EMMC_LINKSPEED_MASK 0xC0000000 /* Link speed */ +#define XAE_EMMC_LINKSPD_10 0x00000000 /* Link Speed mask for 10 Mbit */ +#define XAE_EMMC_LINKSPD_100 0x40000000 /* Link Speed mask for 100 Mbit */ +#define XAE_EMMC_LINKSPD_1000 0x80000000 /* Link Speed mask for 1000 Mbit */ + +/* Interrupt Status/Enable/Mask Registers bit definitions */ +#define XAE_INT_RXRJECT_MASK 0x00000008 /* Rx frame rejected */ +#define XAE_INT_MGTRDY_MASK 0x00000080 /* MGT clock Lock */ + +/* Receive Configuration Word 1 (RCW1) Register bit definitions */ +#define XAE_RCW1_RX_MASK 0x10000000 /* Receiver enable */ + +/* Transmitter Configuration (TC) Register bit definitions */ +#define XAE_TC_TX_MASK 0x10000000 /* Transmitter enable */ + +/* + * Station address bits [47:32] + * Station address bits [31:0] are stored in register UAW0 + */ +#define XAE_UAW0_OFFSET 0x00000700 /* Unicast address word 0 */ +#define XAE_UAW1_OFFSET 0x00000704 /* Unicast address word 1 */ +#define XAE_UAW1_UNICASTADDR_MASK 0x0000FFFF + +/* MDIO Management Configuration (MC) Register bit definitions */ +#define XAE_MDIO_MC_MDIOEN_MASK 0x00000040 /* MII management enable*/ + +/* MDIO Management Control Register (MCR) Register bit definitions */ +#define XAE_MDIO_MCR_PHYAD_MASK 0x1F000000 /* Phy Address Mask */ +#define XAE_MDIO_MCR_PHYAD_SHIFT 24 /* Phy Address Shift */ +#define XAE_MDIO_MCR_REGAD_MASK 0x001F0000 /* Reg Address Mask */ +#define XAE_MDIO_MCR_REGAD_SHIFT 16 /* Reg Address Shift */ +#define XAE_MDIO_MCR_OP_READ_MASK 0x00008000 /* Op Code Read Mask */ +#define XAE_MDIO_MCR_OP_WRITE_MASK 0x00004000 /* Op Code Write Mask */ +#define XAE_MDIO_MCR_INITIATE_MASK 0x00000800 /* Ready Mask */ +#define XAE_MDIO_MCR_READY_MASK 0x00000080 /* Ready Mask */ + +#define XAE_MDIO_DIV_DFT 29 /* Default MDIO clock divisor */ + +/* DMA macros */ +/* Bitmasks of XAXIDMA_CR_OFFSET register */ +#define XAXIDMA_CR_RUNSTOP_MASK 0x00000001 /* Start/stop DMA channel */ +#define XAXIDMA_CR_RESET_MASK 0x00000004 /* Reset DMA engine */ + +/* Bitmasks of XAXIDMA_SR_OFFSET register */ +#define XAXIDMA_HALTED_MASK 0x00000001 /* DMA channel halted */ + +/* Bitmask for interrupts */ +#define XAXIDMA_IRQ_IOC_MASK 0x00001000 /* Completion intr */ +#define XAXIDMA_IRQ_DELAY_MASK 0x00002000 /* Delay interrupt */ +#define XAXIDMA_IRQ_ALL_MASK 0x00007000 /* All interrupts */ + +/* Bitmasks of XAXIDMA_BD_CTRL_OFFSET register */ +#define XAXIDMA_BD_CTRL_TXSOF_MASK 0x08000000 /* First tx packet */ +#define XAXIDMA_BD_CTRL_TXEOF_MASK 0x04000000 /* Last tx packet */ + + +#define ENET_MAX_MTU PKTSIZE_ALIGN +#define DMAALIGN 128 + +static u8 RxFrame[ENET_MAX_MTU] __attribute((aligned(DMAALIGN))) ; + +/* reflect dma offsets */ +struct axidma_reg { + u32 control; /* DMACR */ + u32 status; /* DMASR */ + u32 current; /* CURDESC */ + u32 reserved; + u32 tail; /* TAILDESC */ +}; + +/* Private driver structures */ +struct axidma_priv { + struct axidma_reg *dmatx; + struct axidma_reg *dmarx; +}; + +/* BD descriptors */ +typedef struct axidma_bd { + u32 next; /* Next descriptor pointer */ + u32 reserved1; + u32 phys; /* Buffer address */ + u32 reserved2; + u32 reserved3; + u32 reserved4; + u32 cntrl; /* Control */ + u32 status; /* Status */ + u32 app0; + u32 app1; /* TX start << 16 | insert */ + u32 app2; /* TX csum seed */ + u32 app3; + u32 app4; + u32 sw_id_offset; + u32 reserved5; + u32 reserved6; +} axidma_bd __attribute((aligned(DMAALIGN))); + +/* Static BDs - driver uses only one BD */ +static axidma_bd tx_bd; +static axidma_bd rx_bd; + +/* Use MII register 1 (MII status register) to detect PHY */ +#define PHY_DETECT_REG 1 + +/* Mask used to verify certain PHY features (or register contents) + * in the register above: + * 0x1000: 10Mbps full duplex support + * 0x0800: 10Mbps half duplex support + * 0x0008: Auto-negotiation support + */ +#define PHY_DETECT_MASK 0x1808 + +static u16 phy_read(struct eth_device *dev, u32 PhyAddress, u32 RegisterNum) +{ + u32 MdioCtrlReg = 0; + + /* Wait till MDIO interface is ready to accept a new transaction. */ + while (!(in_be32(dev->iobase + XAE_MDIO_MCR_OFFSET) + & XAE_MDIO_MCR_READY_MASK)) + ; + + MdioCtrlReg = ((PhyAddress << XAE_MDIO_MCR_PHYAD_SHIFT) & + XAE_MDIO_MCR_PHYAD_MASK) | + ((RegisterNum << XAE_MDIO_MCR_REGAD_SHIFT) + & XAE_MDIO_MCR_REGAD_MASK) | + XAE_MDIO_MCR_INITIATE_MASK | + XAE_MDIO_MCR_OP_READ_MASK; + + out_be32(dev->iobase + XAE_MDIO_MCR_OFFSET, MdioCtrlReg); + + /* Wait till MDIO transaction is completed. */ + while (!(in_be32(dev->iobase + XAE_MDIO_MCR_OFFSET) + & XAE_MDIO_MCR_READY_MASK)) + ; + + /* Read data */ + return (u16) in_be32(dev->iobase + XAE_MDIO_MRD_OFFSET); +} + +/* setting axi emac and phy to proper setting */ +static void setup_phy(struct eth_device *dev) +{ + int i; + unsigned retries = 100; + u16 PhyReg; + u16 PhyReg2; + int PhyAddr = -1; + u32 emmc_reg; + + debug("detecting phy address\n"); + + /* detect the PHY address */ + for (PhyAddr = 31; PhyAddr >= 0; PhyAddr--) { + PhyReg = phy_read(dev, PhyAddr, PHY_DETECT_REG); + + if ((PhyReg != 0xFFFF) && + ((PhyReg & PHY_DETECT_MASK) == PHY_DETECT_MASK)) { + /* Found a valid PHY address */ + + debug("Found valid phy address, %d\n", PhyReg); + break; + } + } + + debug("waiting for the phy to be up\n"); + + /* wait for link up and autonegotiation completed */ + while (retries-- > 0) { + if ((PhyReg & 0x24) == 0x24) + break; + } + + /* get PHY id */ + PhyReg = phy_read(dev, PhyAddr, 2); + PhyReg2 = phy_read(dev, PhyAddr, 2); + i = (PhyReg << 16) | PhyReg2; + debug("LL_TEMAC: Phy ID 0x%x\n", i); + + /* Marwell 88e1111 id - ml50x, 0x1410141 id - sp605 */ + if ((i == 0x1410cc2) || (i == 0x1410141)) { + debug("Marvell PHY recognized\n"); + + /* Setup the emac for the phy speed */ + emmc_reg = in_be32(dev->iobase + XAE_EMMC_OFFSET); + emmc_reg &= ~XAE_EMMC_LINKSPEED_MASK; + + PhyReg = phy_read(dev, PhyAddr, 17); + + if ((PhyReg & 0x8000) == 0x8000) { + emmc_reg |= XAE_EMMC_LINKSPD_1000; + printf("1000BASE-T\n"); + } else if ((PhyReg & 0x4000) == 0x4000) { + printf("100BASE-T\n"); + emmc_reg |= XAE_EMMC_LINKSPD_100; + } else { + printf("10BASE-T\n"); + emmc_reg |= XAE_EMMC_LINKSPD_10; + } + + /* Write new speed setting out to Axi Ethernet */ + out_be32(dev->iobase + XAE_EMMC_OFFSET, emmc_reg); + + /* + * Setting the operating speed of the MAC needs a delay. There + * doesn't seem to be register to poll, so please consider this + * during your application design. + */ + udelay(1); + } +} + +/* STOP DMA transfers */ +static void axiemac_halt(struct eth_device *dev) +{ + struct axidma_priv *dma = dev->priv; + + if (dev->state) { + /* Stop the hardware */ + dma->dmatx->control &= ~XAXIDMA_CR_RUNSTOP_MASK; + dma->dmarx->control &= ~XAXIDMA_CR_RUNSTOP_MASK; + dev->state = 0; + } + debug("axiemac halted\n"); +} + +static void axi_ethernet_init(struct eth_device *dev) +{ + /* + * Check the status of the MgtRdy bit in the interrupt status + * registers. This must be done to allow the MGT clock to become stable + * for the Sgmii and 1000BaseX PHY interfaces. No other register reads + * will be valid until this bit is valid. + * The bit is always a 1 for all other PHY interfaces. + */ + u32 timeout = 200; + while (timeout && + (!(in_be32(dev->iobase + XAE_IS_OFFSET) & XAE_INT_MGTRDY_MASK))) + timeout--; + + if (!timeout) + printf("%s: Timeout\n", __func__); + + /* Stop the device and reset HW */ + /* Disable interrupts */ + out_be32(dev->iobase + XAE_IE_OFFSET, 0); + + /* Disable the receiver */ + out_be32(dev->iobase + XAE_RCW1_OFFSET, + in_be32(dev->iobase + XAE_RCW1_OFFSET) & ~XAE_RCW1_RX_MASK); + + /* + * Stopping the receiver in mid-packet causes a dropped packet + * indication from HW. Clear it. + */ + /* set the interrupt status register to clear the interrupt */ + out_be32(dev->iobase + XAE_IS_OFFSET, XAE_INT_RXRJECT_MASK); + + /* Setup HW */ + /* Set default MDIO divisor */ + out_be32(dev->iobase + XAE_MDIO_MC_OFFSET, + (u32) XAE_MDIO_DIV_DFT | XAE_MDIO_MC_MDIOEN_MASK); + + debug("XAxiEthernet InitHw: done\n"); +} + +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); +} + +/* Reset DMA engine */ +static void axi_dma_init(struct eth_device *dev) +{ + struct axidma_priv *dma = dev->priv; + + /* Reset the engine so the hardware starts from a known state */ + dma->dmatx->control = XAXIDMA_CR_RESET_MASK; + dma->dmarx->control = XAXIDMA_CR_RESET_MASK; + + /* 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 */ + if (!(dma->dmatx->control | dma->dmarx->control) + & XAXIDMA_CR_RESET_MASK) + break; + timeout -= 1; + } + + if (!timeout) + printf("%s: Timeout\n", __func__); +} + +static int axiemac_init(struct eth_device *dev, bd_t * bis) +{ + struct axidma_priv *dma = dev->priv; + debug("axi emac init started\n"); + + /* + * Initialize AXIDMA engine. AXIDMA engine must be initialized before + * AxiEthernet. During AXIDMA engine initialization, AXIDMA hardware is + * reset, and since AXIDMA reset line is connected to AxiEthernet, this + * would ensure a reset of AxiEthernet. + */ + axi_dma_init(dev); + + /* Initialize AxiEthernet hardware. */ + axi_ethernet_init(dev); + setup_mac(dev); + + /* Start the hardware */ + dma->dmarx->control |= XAXIDMA_CR_RUNSTOP_MASK; + /* Start DMA RX channel. Now it's ready to receive data.*/ + dma->dmarx->current = (u32)&rx_bd; + + /* Disable all RX interrupts before RxBD space setup */ + dma->dmarx->control &= ~(XAXIDMA_IRQ_ALL_MASK & XAXIDMA_IRQ_ALL_MASK); + + /* Setup the BD. */ + memset((void *) &rx_bd, 0, sizeof(axidma_bd)); + rx_bd.next = (u32)&rx_bd; + rx_bd.phys = (u32)&RxFrame; + 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; + + /* enable TX */ + out_be32(dev->iobase + XAE_TC_OFFSET, XAE_TC_TX_MASK); + /* enable RX */ + out_be32(dev->iobase + XAE_RCW1_OFFSET, XAE_RCW1_RX_MASK); + + /* PHY setup */ + setup_phy(dev); + dev->state = 1; + debug("axi emac init complete\n"); + return 1; +} + +static int axiemac_send(struct eth_device *dev, volatile void *ptr, int len) +{ + struct axidma_priv *dma = dev->priv; + + if (len > ENET_MAX_MTU) + len = ENET_MAX_MTU; + + /* Flush packet to main memory to be trasfered by DMA */ + flush_cache(ptr, len); + + /* Setup Tx BD */ + memset((void *) &tx_bd, 0, sizeof(axidma_bd)); + /* At the end of the ring, link the last BD back to the top */ + tx_bd.next = (u32)&tx_bd; + tx_bd.phys = (u32)ptr; + /* save len */ + tx_bd.cntrl = len | XAXIDMA_BD_CTRL_TXSOF_MASK | + XAXIDMA_BD_CTRL_TXEOF_MASK; + + /* Flush the last BD so DMA core could see the updates */ + flush_cache((u32)&tx_bd, sizeof(axidma_bd)); + + if (dma->dmatx->status & XAXIDMA_HALTED_MASK) { + dma->dmatx->current = (u32)&tx_bd; + /* Start the hardware */ + dma->dmatx->control |= XAXIDMA_CR_RUNSTOP_MASK; + } + + /* start transfer */ + dma->dmatx->tail = (u32)&tx_bd; + + /* Wait for transmission to complete */ + debug("axi emac, waiting for tx to be done\n"); + while (!dma->dmatx->status & + (XAXIDMA_IRQ_DELAY_MASK | XAXIDMA_IRQ_IOC_MASK)) + ; + + debug("axi emac send complete\n"); + return 1; +} + +static int IsRxReady(struct eth_device *dev) +{ + u32 status; + struct axidma_priv *dma = dev->priv; + + /* Read pending interrupts */ + status = dma->dmarx->status; + + /* Acknowledge pending interrupts */ + dma->dmarx->status &= XAXIDMA_IRQ_ALL_MASK; + + /* + * If Reception done interrupt is asserted, call RX call back function + * to handle the processed BDs and then raise the according flag. + */ + if ((status & (XAXIDMA_IRQ_DELAY_MASK | XAXIDMA_IRQ_IOC_MASK))) + return 1; + + return 0; +} + +static int axiemac_recv(struct eth_device *dev) +{ + u32 length; + struct axidma_priv *dma = dev->priv; + + /* wait for an incoming packet */ + if (!IsRxReady(dev)) + return 0; + + debug("axi emac, rx data ready\n"); + + /* Disable IRQ for a moment till packet is handled */ + dma->dmarx->control &= ~(XAXIDMA_IRQ_ALL_MASK & XAXIDMA_IRQ_ALL_MASK); + + length = rx_bd.app4 & 0x0000FFFF; +#ifdef DEBUG + print_buffer(&RxFrame, &RxFrame[0], 1, length, 16); +#endif + /* pass the received frame up for processing */ + if (length) + NetReceive(RxFrame, length); + +#ifdef DEBUG + /* It is useful to clear buffer to be sure that it is consistent */ + memset(RxFrame, 0, sizeof(RxFrame)); +#endif + /* Setup RxBD */ + /* Clear the whole buffer and setup it again - all flags are cleared */ + memset((void *) &rx_bd, 0, sizeof(axidma_bd)); + rx_bd.next = (u32)&rx_bd; + rx_bd.phys = (u32)&RxFrame; + rx_bd.cntrl = sizeof(RxFrame); + + /* write bd to HW */ + flush_cache((u32)&rx_bd, sizeof(axidma_bd)); + + /* it is necessary to flush RxFrame because if you don't do it + * then cache will contain previous packet */ + flush_cache((u32)&RxFrame, sizeof(RxFrame)); + + /* Rx BD is ready - start again */ + dma->dmarx->tail = (u32)&rx_bd; + + debug("axi emac rx complete, framelength = %d\n", length); + + return length; +} + +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"); + + dev->iobase = base_addr; + dma = dev->priv; + dma->dmatx = dma_addr; + dma->dmarx = (u32)dma->dmatx + 0x30; /* rx channel offset */ + dev->init = axiemac_init; + dev->halt = axiemac_halt; + dev->send = axiemac_send; + dev->recv = axiemac_recv; + + eth_register(dev); + + return 0; +} diff --git a/include/configs/microblaze-generic.h b/include/configs/microblaze-generic.h index ce9d426..efa0c73 100644 --- a/include/configs/microblaze-generic.h +++ b/include/configs/microblaze-generic.h @@ -78,6 +78,9 @@ #elif XILINX_LLTEMAC_BASEADDR # define CONFIG_XILINX_LL_TEMAC 1 # define CONFIG_SYS_ENET +#elif defined(XILINX_AXIEMAC_BASEADDR) +# define CONFIG_XILINX_AXIEMAC 1 +# define CONFIG_SYS_ENET #endif #undef ET_DEBUG
Add the first axi_ethernet driver for little-endian Microblaze. Signed-off-by: Michal Simek <monstr@monstr.eu> --- .../xilinx/microblaze-generic/microblaze-generic.c | 4 + drivers/net/Makefile | 1 + drivers/net/xilinx_axi_emac.c | 529 ++++++++++++++++++++ include/configs/microblaze-generic.h | 3 + 4 files changed, 537 insertions(+), 0 deletions(-) create mode 100644 drivers/net/xilinx_axi_emac.c