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 <mon...@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 <mon...@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!
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
