Marek Vasut wrote:
> 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];"
+ UAW0 and UAW1 with 0x700 offset.
struct axi_ethernet {
u32 reserved[3];
u32 is; /* 0xC: Interrupt status */
u32 reserved2;
u32 ie; /* 0x14: Interrupt enable */
u32 reserved3[251];
u32 rcw1; /* 0x404: Rx Configuration Word 1 */
u32 tc; /* 0x408: Tx Configuration */
u32 reserved4;
u32 emmc; /* 0x410: EMAC mode configuration */
u32 reserved5[59];
u32 mdio_mc; /* 0x500: MII Management Config */
u32 mdio_mcr; /* 0x504: MII Management Control */
u32 mdio_mwd; /* 0x508: MII Management Write Data */
u32 mdio_mrd; /* 0x50C: MII Management Read Data */
u32 reserved6[124];
u32 uaw0; /* 0x700: Unicast address word 0 */
u32 uaw1; /* 0x704: Unicast address word 1 */
};
Are you really sure that this is nice?
>
>>>> +
>>>> +/* 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.
At offset 40b40410. It is really hard to decode it if values are (1 << n).
With defined macro you directly see that this is on 100Mbit/s LAN.
U-Boot-mONStR> md 40b40400
40b40400: ddccbbaa 6800ffee 4a000000 60000000 .......h...J...`
40b40410: 40000000 00000000 000ce000 000ce000 ...@............
40b40420: 000ce000 000ce000 000ce000 000ce000 ................
40b40430: 000ce000 000ce000 000ce000 000ce000 ................
I am fine to use 1 << n solution but definitely in our repo I will use in way I
like.
>
>>>> +/* 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.
Just the whole private structure with addresses, + phy.
As for pt. 2. --
> "currently", so there's possibility, in future this won't hold?
BTW: I am also sharing rx/tx buffer descriptors for dma.
When do you expect that u-boot will be able to use several MACs in one time?
>
>> 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?
Custodian. But I won't do that.
If you think that all archs should have it then move it to generic location
which clean code duplication and I will include it.
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
