From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michal Simek <monstr@monstr.eu>
Date: Mon, 28 Feb 2011 19:50:55 +0100
Subject: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot
In-Reply-To: <201102281236.29967.vapier@gentoo.org>
References: <1298886033-3350-1-git-send-email-monstr@monstr.eu>
	<201102281236.29967.vapier@gentoo.org>
Message-ID: <4D6BEE8F.3050900@monstr.eu>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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

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