[U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Marek Vasut <marek.vasut@gmail.com>]

On Wednesday, August 31, 2011 04:46:22 PM Michal Simek wrote:
> Marek Vasut wrote:
> > On Tuesday, August 30, 2011 02:05:19 PM Michal Simek wrote:
> >> Add the first axi_ethernet driver for little-endian Microblaze.
> >> 
> >> Signed-off-by: Michal Simek <monstr@monstr.eu>
> >> ---
> >> 
> >>  drivers/net/Makefile          |    1 +
> >>  drivers/net/xilinx_axi_emac.c |  622
> >> 
> >> +++++++++++++++++++++++++++++++++++++++++ include/netdev.h             
> >> |
> >> 
> >>   1 +
> >>  
> >>  3 files changed, 624 insertions(+), 0 deletions(-)
> >>  create mode 100644 drivers/net/xilinx_axi_emac.c
> >> 
> >> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> >> index 4541eaf..ae9d4cb 100644
> >> --- a/drivers/net/Makefile
> >> +++ b/drivers/net/Makefile
> >> @@ -83,6 +83,7 @@ COBJS-$(CONFIG_TSEC_ENET) += tsec.o fsl_mdio.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..ce79b80
> >> --- /dev/null
> >> +++ b/drivers/net/xilinx_axi_emac.c
> >> @@ -0,0 +1,622 @@
> >> +/*
> >> + * 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>
> >> +#include <phy.h>
> >> +#include <miiphy.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_MWD_OFFSET	0x00000508 /* MII Management Write Data */
> >> +#define XAE_MDIO_MRD_OFFSET	0x0000050C /* MII Management Read Data */
> > 
> > Please use struct xae_regs {...} as the rest of the u-boot.
> 
> struct is not useful here because it will be big with a lot of reserved
> values.

I see like 10 registers here, you can use "uint32_t reserved[n];"

> 
> >> +
> >> +/* 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 */ +
> > 
> > Use (1 << n) ?
> 
> just another solution - I prefer to use 32bit value - easier when you
> decode it by md.

It's hard to read, really.

> 
> >> +/* 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 */
> >> +
> > 
> > [...]
> 
> same here..
> 
> >> +#define DMAALIGN	128
> >> +
> >> +static u8 RxFrame[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN))) ;
> > 
> > Don't use cammelcase, all lowcase please.
> 
> Agree - will be done in v2.
> 
>   Also, can't you allocate this with
> 
> > memalign and hide it in axidma_priv or something ?
> 
> There are two things in one.
> 1. Adding it to axidma_priv means that I will need to dynamically allocate
> big private structure which is bad thing to do for several eth devices.
> This is FPGA you can create a lot of MACs that's why I think it is better
> not to do so.
> 2. Current style is sharing this rxframe buffer among all controllers
> because only one MAC works. Others are stopped which means that no packet
> come to them.

Ok, I don't think I understand pt. 1. -- you need to keep the state for each of 
the ethernet devices on the FPGA separate, don't you. As for pt. 2. -- 
"currently", so there's possibility, in future this won't hold?

> 
> BTW: Looking for that memalign function - thanks.
> 
> >> +
> >> +/* 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;
> >> +	int phyaddr;
> >> +
> >> +	struct phy_device *phydev;
> >> +	struct mii_dev *bus;
> >> +};
> >> +
> >> +/* BD descriptors */
> >> +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;
> >> +};
> >> +
> >> +/* Static BDs - driver uses only one BD */
> >> +static struct axidma_bd tx_bd __attribute((aligned(DMAALIGN)));
> >> +static struct axidma_bd rx_bd __attribute((aligned(DMAALIGN)));
> >> +
> >> +static inline void aximac_out32(u32 addr, u32 offset, u32 val)
> >> +{
> >> +	out_be32((u32 *)(addr + offset), val);
> > 
> > Please fix these casts ... though I don't think you even need these
> > functions.
> 
> Cast is necessary. I use that helper just because of recast.
> If you see solution which will be elegant, I am open to use it.

See above -- use the structure and then just use &axiregs->reg.

> 
> >> +}
> >> +
> >> +static inline u32 aximac_in32(u32 addr, u32 offset)
> >> +{
> >> +	return in_be32((u32 *)(addr + offset));
> >> +}
> >> +
> >> +
> >> +/* Use MII register 1 (MII status register) to detect PHY */
> >> +#define PHY_DETECT_REG  1

[...]

> >> +	/* Write new speed setting out to Axi Ethernet */
> >> +	aximac_out32(dev->iobase, XAE_EMMC_OFFSET, emmc_reg);
> > 
> > Use clrsetbits() here.
> 
> Not defined for Microblaze - just for ARM/PPC.
> Not going to use it.

Please fix then. You're the microblaze maintainer, right?

[...]

Cheers!