[U-Boot] [PATCH 1/6] net: ll_temac: Add LL TEMAC driver to u-boot
Stephan Linz
linz at li-pro.net
Thu Jan 26 23:30:48 CET 2012
Am Donnerstag, den 26.01.2012, 16:07 +0100 schrieb Michal Simek:
> Hi Stephan,
Hi Michal,
>
> Stephan Linz wrote:
> > Xilinx LocalLink Tri-Mode Ether MAC driver can be
> > used by Xilinx Microblaze or Xilinx ppc405/440 in
> > SDMA and FIFO mode. DCR or XPS bus can be used.
> >
> > --snip--
> > ---
> > drivers/net/Makefile | 2 +
> > drivers/net/xilinx_ll_temac.c | 386 ++++++++++++++++++++++++++++++++
> > drivers/net/xilinx_ll_temac_fifo.c | 143 ++++++++++++
> > drivers/net/xilinx_ll_temac_mdio.c | 165 ++++++++++++++
> > drivers/net/xilinx_ll_temac_sdma.c | 370 +++++++++++++++++++++++++++++++
> > include/netdev.h | 1 +
> > include/xilinx_ll_temac.h | 430 ++++++++++++++++++++++++++++++++++++
> > include/xilinx_ll_temac_fifo.h | 122 ++++++++++
> > include/xilinx_ll_temac_mdio.h | 53 +++++
> > include/xilinx_ll_temac_sdma.h | 281 +++++++++++++++++++++++
> > 10 files changed, 1953 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/net/xilinx_ll_temac.c
> > create mode 100644 drivers/net/xilinx_ll_temac_fifo.c
> > create mode 100644 drivers/net/xilinx_ll_temac_mdio.c
> > create mode 100644 drivers/net/xilinx_ll_temac_sdma.c
> > create mode 100644 include/xilinx_ll_temac.h
> > create mode 100644 include/xilinx_ll_temac_fifo.h
> > create mode 100644 include/xilinx_ll_temac_mdio.h
> > create mode 100644 include/xilinx_ll_temac_sdma.h
>
> Wolfgang: I just wanted to know what it is the rule for adding headers.
> I would prefer to add these headers to drivers/net/ folder instead include.
Hm, the reason was the more and more growing up of public interfaces,
for example xilinx_ll_temac_mdio_initialize() and
xilinx_ll_temac_initialize() can be used outside of the driver instead
of it's own standard setup. Yes, I've read your consider, see below. But
if I want to use the xilinx_ll_temac_initialize() outside I need to
export the interface, especially mode flag definitions (FIFO/SDMA/DCR).
So I need public access to the header.
@Wolfgang: What will be the best solution here?
>
>
> >
> > diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> > index f4f7ea3..430f90c 100644
> > --- a/drivers/net/Makefile
> > +++ b/drivers/net/Makefile
> > @@ -77,6 +77,8 @@ 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 xilinx_ll_temac_mdio.o \
> > + xilinx_ll_temac_fifo.o xilinx_ll_temac_sdma.o
> > --snip--
> > +
> > +/*
> > + * Initialize a single ll_temac device with its mdio bus behind ll_temac
> > + *
> > + * Returns 1 if the ll_temac device and the mdio bus were initialized
> > + * otherwise returns 0
> > + */
> > +static int xilinx_ll_temac_eth_init(bd_t *bis, struct ll_temac_info *devinf)
> > +{
> > + struct ll_temac_mdio_info mdioinf;
> > + int ret;
> > +
> > + mdioinf.name = devinf->mdio_busname;
> > + mdioinf.regs = (struct temac_reg *)devinf->base_addr;
> > + ret = xilinx_ll_temac_mdio_initialize(bis, &mdioinf);
> > + if (ret >= 0) {
> > + ret = xilinx_ll_temac_initialize(bis, devinf);
> > + if (ret > 0)
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Initialize all the ll_temac devices
> > + *
> > + * Returns the number of ll_temac devices that were initialized
> > + */
> > +int xilinx_ll_temac_standard_init(bd_t *bis)
> > +{
>
> name seems to me bogus because I can't see any non-standard init.
>
>
> > + struct ll_temac_info devinf;
> > + int count = 0;
> > +
> > +#if defined(CONFIG_XILINX_LL_TEMAC0_BASE_ADDR)
> > + SET_STD_LL_TEMAC_INFO(devinf, 0);
> > + count += xilinx_ll_temac_eth_init(bis, &devinf);
> > +#endif
> > +
> > +#if defined(CONFIG_XILINX_LL_TEMAC1_BASE_ADDR)
> > + SET_STD_LL_TEMAC_INFO(devinf, 1);
> > + count += xilinx_ll_temac_eth_init(bis, &devinf);
> > +#endif
>
> I understand what you want to do but it seems to me better
> to call xilinx_ll_temac_eth_init(bis, BASE_ADDR, DMA/FIFO, flags, phyaddr)
I believe, function with more than three arguments is bad code. So I've
introduced a driver information struct, We can bundle all the specific
IP options in this struct.
> or similar instead of trying to do it in smarter way which must be
> extend if you want to use more than 2 IPs.
The same problem would be in board init code. You must decide which
address, mode, and subaddress you have to use. By implementation of a
"standard initialization" we can avoid a blow out board init code.
I think there is a natural limit of available IPs. We just need to find
this and adapt the code. Here I've typed on 2 IPs. We can easly expand
to 4 or 6 IPs as default.
Please note, this function should implement THE standard case. You are
free to use the underlaying functions for MDIO and MAC initialization in
any other case outside the driver code.
>
> > +
> > + return count;
> > +}
> > diff --git a/drivers/net/xilinx_ll_temac_sdma.c b/drivers/net/xilinx_ll_temac_sdma.c
> > new file mode 100644
> > index 0000000..ae29d2a
> > --- /dev/null
> > +++ b/drivers/net/xilinx_ll_temac_sdma.c
> > @@ -0,0 +1,370 @@
> > +--snip--
> > +
> > +int ll_temac_init_sdma(struct eth_device *dev)
> > +{
> > + struct ll_temac *ll_temac = dev->priv;
> > + struct cdmac_bd *rx_dp;
> > + struct cdmac_bd *tx_dp;
> > + phys_addr_t *ra = ll_temac->sdma_reg_addr;
> > + int i;
> > +
> > + printf("%s: SDMA: %d Rx buffers, %d Tx buffers\n",
> > + dev->name, PKTBUFSRX, TX_BUF_CNT);
> > +
> > + /* Initialize the Rx Buffer descriptors */
> > + for (i = 0; i < PKTBUFSRX; i++) {
> > + rx_dp = &cdmac_bd.rx[i];
> > + memset(rx_dp, 0, sizeof(*rx_dp));
> > + rx_dp->next_p = rx_dp;
> > + rx_dp->buf_len = PKTSIZE_ALIGN;
> > + rx_dp->phys_buf_p = NetRxPackets[i];
>
> xilinx_ll_temac_sdma.c: In function 'll_temac_init_sdma':
> xilinx_ll_temac_sdma.c:186: warning: assignment discards qualifiers from pointer target type
>
> - rx_dp->phys_buf_p = NetRxPackets[i];
> + rx_dp->phys_buf_p = (u8 *)NetRxPackets[i];
Oops, I'll fix it quickly.
>
>
> > +--snip--
> > +
> > +int ll_temac_send_sdma(struct eth_device *dev, volatile void *packet,
> > + int length)
> > +{
> > + unsigned timeout = 50; /* 1usec * 50 = 50usec */
> > + struct cdmac_bd *tx_dp = &cdmac_bd.tx[tx_idx];
> > + struct ll_temac *ll_temac = dev->priv;
> > + phys_addr_t *ra = ll_temac->sdma_reg_addr;
> > +
> > + if (ll_temac_sdma_error(dev)) {
> > +
> > + if (ll_temac_reset_sdma(dev))
> > + return -1;
> > +
> > + ll_temac_init_sdma(dev);
> > + }
> > +
> > + tx_dp->phys_buf_p = packet;
>
>
> xilinx_ll_temac_sdma.c: In function 'll_temac_send_sdma':
> xilinx_ll_temac_sdma.c:343: warning: assignment discards qualifiers from pointer target type
>
> - tx_dp->phys_buf_p = packet;
> + tx_dp->phys_buf_p = (u8 *)packet;
The same here (see above).
>
>
> > + tx_dp->buf_len = length;
> > + tx_dp->sca.stctrl = CDMAC_BD_STCTRL_SOP | CDMAC_BD_STCTRL_EOP |
> > + CDMAC_BD_STCTRL_STOP_ON_END;
> > +
> > + flush_cache((u32)packet, length);
> > + flush_cache((u32)tx_dp, sizeof(*tx_dp));
> > +
> > + /* DMA start by writing to respective TAILDESC_PTR */
> > + ll_temac->out32(ra[TX_CURDESC_PTR], (int)tx_dp);
> > + ll_temac->out32(ra[TX_TAILDESC_PTR], (int)tx_dp);
> > +
> > + /* Find next empty buffer descriptor, preparation for next iteration */
> > + tx_idx = (tx_idx + 1) % TX_BUF_CNT;
> > + tx_dp = &cdmac_bd.tx[tx_idx];
> > +
> > + do {
> > + flush_cache((u32)tx_dp, sizeof(*tx_dp));
> > + udelay(1);
> > + } while (timeout-- && !(tx_dp->sca.stctrl & CDMAC_BD_STCTRL_COMPLETED));
> > +
> > + if (!timeout) {
> > + printf("%s: Timeout\n", __func__);
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/include/netdev.h b/include/netdev.h
> > index b0c21d5..bf95f88 100644
> > --- a/include/netdev.h
> > +++ b/include/netdev.h
> > @@ -102,6 +102,7 @@ int xilinx_axiemac_initialize(bd_t *bis, unsigned long base_addr,
> > unsigned long dma_addr);
> > int xilinx_emaclite_initialize(bd_t *bis, unsigned long base_addr,
> > int txpp, int rxpp);
> > +int xilinx_ll_temac_standard_init(bd_t *bis);
>
>
> I don't like this solution - some comments below above.
see my comments ... above and below
>
>
> >
> > /* Boards with PCI network controllers can call this from their board_eth_init()
> > * function to initialize whatever's on board.
> > diff --git a/include/xilinx_ll_temac.h b/include/xilinx_ll_temac.h
> > new file mode 100644
> > index 0000000..8b86611
> > --- /dev/null
> > +++ b/include/xilinx_ll_temac.h
> > @@ -0,0 +1,430 @@
> > +/*
> > + * Xilinx xps_ll_temac ethernet driver for u-boot
> > + *
> > + * LL_TEMAC interface
> > + *
> > + * Copyright (C) 2011 - 2012 Stephan Linz <linz at li-pro.net>
> > + * Copyright (C) 2008 - 2011 Michal Simek <monstr at monstr.eu>
> > + * Copyright (C) 2008 - 2011 PetaLogix
> > + *
> > + * Based on Yoshio Kashiwagi kashiwagi at co-nss.co.jp driver
> > + * Copyright (C) 2008 Nissin Systems Co.,Ltd.
> > + * March 2008 created
> > + *
> > + * 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.
> > + *
> > + * [0]: http://www.xilinx.com/support/documentation
> > + *
> > + * [S]: [0]/ip_documentation/xps_ll_temac.pdf
> > + * [A]: [0]/application_notes/xapp1041.pdf
> > + */
> > +#ifndef _XILINX_LL_TEMAC_
> > +#define _XILINX_LL_TEMAC_
> > +
> > +#include <config.h>
> > +#include <net.h>
> > +#include <phy.h>
> > +#include <miiphy.h>
> > +
> > +#include <asm/types.h>
> > +#include <asm/byteorder.h>
> > +
> > +#include <xilinx_ll_temac_sdma.h>
> > +
> > +#if !defined(__BIG_ENDIAN)
> > +# error LL_TEMAC requires big endianess
> > +#endif
>
> Not sure if is good to have this in header
All headers are not compliant to little endian. The structs and defines
declared inside can only be used for a big endian controller on a big
endian cpu. Currently we will never see a XPS_LL_TEMAC IP core on a
little endian PPC or Microblaze system, or not? So I've blocked bad
endianess here.
> - you have two limitations in xilinx_ll_temac.c
> for MII and PHYLIB. IMHO will be good to add this there too.
Hm, yes. a good place would be in xilinx_ll_temac_mdio.c too. I'll fix
it.
>
> This chcecking is also at three places.
>
>
> > +
> > +/* default ETH device name */
> > +#if !defined(CONFIG_XILINX_LL_TEMAC0_NAME)
> > +#define CONFIG_XILINX_LL_TEMAC0_NAME "LLTEMAC0"
> > +#endif
> > +
> > +#if !defined(CONFIG_XILINX_LL_TEMAC1_NAME)
> > +#define CONFIG_XILINX_LL_TEMAC1_NAME "LLTEMAC1"
> > +#endif
> > +
> > +/* default MII device name */
> > +#if !defined(CONFIG_XILINX_LL_TEMAC0_MII_NAME)
> > +#define CONFIG_XILINX_LL_TEMAC0_MII_NAME "LLTEMAC0_MII"
> > +#endif
> > +
> > +#if !defined(CONFIG_XILINX_LL_TEMAC1_MII_NAME)
> > +#define CONFIG_XILINX_LL_TEMAC1_MII_NAME "LLTEMAC1_MII"
> > +#endif
>
> People likes games with names but I think it is completely OK
> just choose standard one and create it.
> Even more IMHO name should contain address because if you want
> to use more none will remember which IP is 0/1/2/etc.
Hm, but you can see it on console output. The base address will printed
out and we know about the enumeration.
> In axi driver I use
> sprintf(dev->name, "aximac.%lx", base_addr);
> which seems to me the best.
On this way we can never change the name outside the driver. The name
would be fixed forever. If you need your own namespace then use the
corresponding configuration options, example for the first IP:
CONFIG_XILINX_LL_TEMAC0_NAME -- This can done in combination of
xparameters.h and configs/microblaze-generic.h. If you need a dynamic
namespace (your case) then do not use the "standard initialization". You
can setup your own driver information struct in board init code by a
dynamically generated string and have a direct call to the underlaying
functions for MDIO and MAC initialization.
>
> The same argument for MII name.
same here
>
>
> > +
> > +/* default PHY device address on MII */
> > +#if !defined(CONFIG_PHY_ADDR)
> > +#define CONFIG_PHY_ADDR -1
> > +#endif
> > +#define CONFIG_PHY0_ADDR CONFIG_PHY_ADDR
> > +
> > +#if !defined(CONFIG_PHY1_ADDR)
> > +#define CONFIG_PHY1_ADDR -1
> > +#endif
> > +
> > +/* default parameters for LLTEMAC0 */
> > +#if defined(XILINX_LLTEMAC_BASEADDR)
> > +# if !defined(XILINX_LL_TEMAC0_BASE_ADDR)
> > +# define CONFIG_XILINX_LL_TEMAC0_BASE_ADDR XILINX_LLTEMAC_BASEADDR
> > +# endif
> > +# if defined(XILINX_LLTEMAC_FIFO_BASEADDR)
> > +# if !defined(XILINX_LL_TEMAC0_CTRL_ADDR)
> > +# define CONFIG_XILINX_LL_TEMAC0_CTRL_ADDR XILINX_LLTEMAC_FIFO_BASEADDR
> > +# endif
> > +# if !defined(XILINX_LL_TEMAC0_CTRL_MODE)
> > +# define CONFIG_XILINX_LL_TEMAC0_CTRL_MODE LL_TEMAC_M_FIFO
> > +# endif
> > +# elif defined(XILINX_LLTEMAC_SDMA_CTRL_BASEADDR)
> > +# if !defined(XILINX_LL_TEMAC0_CTRL_ADDR)
> > +# define CONFIG_XILINX_LL_TEMAC0_CTRL_ADDR XILINX_LLTEMAC_SDMA_CTRL_BASEADDR
> > +# endif
> > +# if (XILINX_LLTEMAC_SDMA_USE_DCR == 1)
> > +# if !defined(XILINX_LL_TEMAC0_CTRL_MODE)
grr, have to check CONFIG_XILINX_LL_TEMAC0_CTRL_MODE
will fix it ...
> > +# define CONFIG_XILINX_LL_TEMAC0_CTRL_MODE LL_TEMAC_M_SDMA_DCR
> > +# endif
> > +# else
> > +# define CONFIG_XILINX_LL_TEMAC0_CTRL_MODE LL_TEMAC_M_SDMA_PLB
> > +# endif
> > +# else
> > +# error "missing sub-controller base address for LLTEMAC0"
> > +# endif
> > +#endif
> > +
> > +/* default parameters for LLTEMAC1 */
> > +#if defined(XILINX_LLTEMAC_BASEADDR1)
> > +# if !defined(XILINX_LL_TEMAC1_BASE_ADDR)
> > +# define CONFIG_XILINX_LL_TEMAC1_BASE_ADDR XILINX_LLTEMAC_BASEADDR1
> > +# endif
> > +# if defined(XILINX_LLTEMAC_FIFO_BASEADDR1)
> > +# if !defined(XILINX_LL_TEMAC1_CTRL_ADDR)
> > +# define CONFIG_XILINX_LL_TEMAC1_CTRL_ADDR XILINX_LLTEMAC_FIFO_BASEADDR1
> > +# endif
> > +# if !defined(XILINX_LL_TEMAC1_CTRL_MODE)
> > +# define CONFIG_XILINX_LL_TEMAC1_CTRL_MODE LL_TEMAC_M_FIFO
> > +# endif
> > +# elif defined(XILINX_LLTEMAC_SDMA_CTRL_BASEADDR1)
> > +# if !defined(XILINX_LL_TEMAC1_CTRL_ADDR)
> > +# define CONFIG_XILINX_LL_TEMAC1_CTRL_ADDR XILINX_LLTEMAC_SDMA_CTRL_BASEADDR1
> > +# endif
> > +# if (XILINX_LLTEMAC_SDMA_USE_DCR == 1)
> > +# if !defined(XILINX_LL_TEMAC1_CTRL_MODE)
... same here ...
> > +# define CONFIG_XILINX_LL_TEMAC1_CTRL_MODE LL_TEMAC_M_SDMA_DCR
> > +# endif
> > +# else
> > +# define CONFIG_XILINX_LL_TEMAC1_CTRL_MODE LL_TEMAC_M_SDMA_PLB
> > +# endif
> > +# else
> > +# error "missing sub-controller base address for LLTEMAC1"
> > +# endif
> > +#endif
>
> It seems to me that this is a lot of code which just do nothing interesting.
> There is a lot of if/else/endif to even read it. Why not to do configuration
> directly in board specific file?
Yes, it is an ugly part. There are some stupid redefinitions -- for
example
XILINX_LLTEMAC_BASEADDR to CONFIG_XILINX_LL_TEMAC0_BASE_ADDR
But there are two reasons for this:
1) XILINX_LLTEMAC_BASEADDR should be XILINX_LLTEMAC_BASEADDR0 or
XILINX_LLTEMAC0_BASEADDR -- so we can use CPP to handle it directly
inside the SET_STD_LL_TEMAC_INFO(num) macro
2) Each U-Boot configuration variable have to begin with CONFIG_ (see
README)
The best solution here would be a new (better) syntax in xparameters.h
-- for example:
#define CONFIG_XILINX_LL_TEMAC0_BASE_ADDR 0x44000000
#define CONFIG_XILINX_LL_TEMAC0_CTRL_ADDR 0x42000180
#define CONFIG_XILINX_LL_TEMAC1_BASE_ADDR 0x44000040
#define CONFIG_XILINX_LL_TEMAC1_CTRL_ADDR 0x42000200
This would reduce this part of code to nearly zero.
Furthermore we could define CONFIG_XILINX_LL_TEMAC0_CTRL_MODE with the
right mode value directly in xparameters.h -- for example:
#define CONFIG_XILINX_LL_TEMAC0_CTRL_MODE 0 /* FIFO */
#define CONFIG_XILINX_LL_TEMAC1_CTRL_MODE 1 /* SDMA by PLB */
The xparameters.h header is an auto-generated file. We can change/expand
the generator. Within the meaning of U-Boot we have to change because it
will be included by the generic board configuration and there we need a
proper U-Boot configuration syntax.
We should discuss it in details.
>
> > +
> > +#define STD_LL_TEMAC_INFO(num) \
> > +{ \
> > + .devname = CONFIG_XILINX_LL_TEMAC##num##_NAME, \
> > + .base_addr = CONFIG_XILINX_LL_TEMAC##num##_BASE_ADDR, \
> > + .ctrl_addr = CONFIG_XILINX_LL_TEMAC##num##_CTRL_ADDR, \
> > + .flags = CONFIG_XILINX_LL_TEMAC##num##_CTRL_MODE, \
> > + .mdio_busname = CONFIG_XILINX_LL_TEMAC##num##_MII_NAME, \
> > + .phyaddr = CONFIG_PHY##num##_ADDR, \
> > +}
>
> This is completely unused.
Yes, but usefull if you want to make a static driver configuration array
outside the driver, for example in a specific (not the generic) board
init. So I left it here as an part of the public driver interface.
>
> > +
> > +#define SET_STD_LL_TEMAC_INFO(x, num) \
> > +{ \
> > + x.devname = CONFIG_XILINX_LL_TEMAC##num##_NAME; \
> > + x.base_addr = CONFIG_XILINX_LL_TEMAC##num##_BASE_ADDR; \
> > + x.ctrl_addr = CONFIG_XILINX_LL_TEMAC##num##_CTRL_ADDR; \
> > + x.flags = CONFIG_XILINX_LL_TEMAC##num##_CTRL_MODE; \
> > + x.mdio_busname = CONFIG_XILINX_LL_TEMAC##num##_MII_NAME; \
> > + x.phyaddr = CONFIG_PHY##num##_ADDR; \
> > +}
>
>
> This configuration things seems to me completely weird.
> My preference is to do it in board file without any helper function.
So you have to make it twice or multiple. Each board file have to
implement all the wired redefinitions or decisions based on the
definitions from xparameters.h -- you will implement the same code in
board/xilinx/microblaze-generic/ and board/xilinx/ppc405-generic/ and
board/xilinx/ppc440-generic/ and this for more than 2 IPs ...
Do you prefer this really?
>
>
--snip--
> > +struct ll_temac_info {
> > + int flags;
>
> probably u32?
mybe, but it is an internal value -- uncritically here.
>
> Others patches looks good.
:)
--
Best regards,
Stephan Linz
______________________________________________________________________________
MB-Ref: http://www.li-pro.de/xilinx_mb:mbref:start
OpenDCC: http://www.li-pro.net/opendcc.phtml
PC/M: http://www.li-pro.net/pcm.phtml
Sourceforge: http://sourceforge.net/users/slz
Gitorious: https://gitorious.org/~slz
More information about the U-Boot
mailing list