Re: [U-Boot] [PATCH 10/12] net: Rename and cleanup sunxi (Allwinner) emac driver

Ian Campbell Sat, 31 May 2014 10:21:06 -0700

On Fri, 2014-05-30 at 11:06 +0200, Hans de Goede wrote:
>  drivers/net/sunxi_emac.c  | 521 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/sunxi_wemac.c | 525 
> ----------------------------------------------


git format-patch/send-email -M makes this a lot easier to look at. I've
reviewed git show -M b6d2111e9a43c927cc83609625107c5b2a518163 from your
branch instead (pasted below for everyone's reference).

Acked-by: Ian Campbell <i...@hellion.org.uk>

Two general comments which I think are out of scope for this change:

First, I can't see what is using linux/err.h.

Second, there's an awful lot of magic numbers in this driver, but I
don't see any reason to block this patch on that just because you are
fixing the formatting.

Ian.


> commit b6d2111e9a43c927cc83609625107c5b2a518163
> Author: Stefan Roese <s...@denx.de>
> Date:   Mon Nov 5 13:12:49 2012 +0100
> 
>     net: Rename and cleanup sunxi (Allwinner) emac driver
>     
>     There have been 3 versions of the sunxi_emac support patch during its
>     development. Somehow version 2 ended up in upstream u-boot where as
>     the u-boot-sunxi git repo got version 3.
>     
>     This bumps the version in upstream u-boot to version 3 of the patch:
>     - Initialize MII clock earlier so mii access to allow independent use
>     - Name change from WEMAC to EMAC to match mainline kernel & chip manual
>     - Cosmetic code cleanup
>     
>     Signed-off-by: Stefan Roese <s...@denx.de>
>     Signed-off-by: Henrik Nordstrom <hen...@henriknordstrom.net>
>     Signed-off-by: Oliver Schinagl <oli...@schinagl.nl>
>     Signed-off-by: Hans de Goede <hdego...@redhat.com>
> 
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index 6005f7e..da2b5f8 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_DNET) += dnet.o
>  obj-$(CONFIG_E1000) += e1000.o
>  obj-$(CONFIG_E1000_SPI) += e1000_spi.o
>  obj-$(CONFIG_EEPRO100) += eepro100.o
> +obj-$(CONFIG_SUNXI_EMAC) += sunxi_emac.o
>  obj-$(CONFIG_ENC28J60) += enc28j60.o
>  obj-$(CONFIG_EP93XX) += ep93xx_eth.o
>  obj-$(CONFIG_ETHOC) += ethoc.o
> @@ -51,7 +52,6 @@ obj-$(CONFIG_RTL8169) += rtl8169.o
>  obj-$(CONFIG_SH_ETHER) += sh_eth.o
>  obj-$(CONFIG_SMC91111) += smc91111.o
>  obj-$(CONFIG_SMC911X) += smc911x.o
> -obj-$(CONFIG_SUNXI_WEMAC) += sunxi_wemac.o
>  obj-$(CONFIG_DRIVER_TI_EMAC) += davinci_emac.o
>  obj-$(CONFIG_TSEC_ENET) += tsec.o fsl_mdio.o
>  obj-$(CONFIG_DRIVER_TI_CPSW) += cpsw.o
> diff --git a/drivers/net/sunxi_wemac.c b/drivers/net/sunxi_emac.c
> similarity index 78%
> rename from drivers/net/sunxi_wemac.c
> rename to drivers/net/sunxi_emac.c
> index 699a381..5a06d68 100644
> --- a/drivers/net/sunxi_wemac.c
> +++ b/drivers/net/sunxi_emac.c
> @@ -1,5 +1,5 @@
>  /*
> - * sunxi_wemac.c -- Allwinner A10 ethernet driver
> + * sunxi_emac.c -- Allwinner A10 ethernet driver
>   *
>   * (C) Copyright 2012, Stefan Roese <s...@denx.de>
>   *
> @@ -7,16 +7,16 @@
>   */
>  
>  #include <common.h>
> +#include <linux/err.h>
>  #include <malloc.h>
> -#include <net.h>
>  #include <miiphy.h>
> -#include <linux/err.h>
> +#include <net.h>
>  #include <asm/io.h>
>  #include <asm/arch/clock.h>
>  #include <asm/arch/gpio.h>
>  
>  /* EMAC register  */
> -struct wemac_regs {
> +struct emac_regs {
>       u32 ctl;        /* 0x00 */
>       u32 tx_mode;    /* 0x04 */
>       u32 tx_flow;    /* 0x08 */
> @@ -27,7 +27,7 @@ struct wemac_regs {
>       u32 tx_pl1;     /* 0x1c */
>       u32 tx_sta;     /* 0x20 */
>       u32 tx_io_data; /* 0x24 */
> -     u32 tx_io_data1; /* 0x28 */
> +     u32 tx_io_data1;/* 0x28 */
>       u32 tx_tsvl0;   /* 0x2c */
>       u32 tx_tsvh0;   /* 0x30 */
>       u32 tx_tsvl1;   /* 0x34 */
> @@ -141,33 +141,33 @@ struct sunxi_sramc_regs {
>  
>  #define EMAC_MAC_IPGT                0x15
>  
> -#define EMAC_MAC_NBTB_IPG1   0xC
> +#define EMAC_MAC_NBTB_IPG1   0xc
>  #define EMAC_MAC_NBTB_IPG2   0x12
>  
>  #define EMAC_MAC_CW          0x37
> -#define EMAC_MAC_RM          0xF
> +#define EMAC_MAC_RM          0xf
>  
>  #define EMAC_MAC_MFL         0x0600
>  
>  /* Receive status */
> -#define EMAC_CRCERR          (1 << 4)
> -#define EMAC_LENERR          (3 << 5)
> +#define EMAC_CRCERR          (0x1 << 4)
> +#define EMAC_LENERR          (0x3 << 5)
>  
>  #define DMA_CPU_TRRESHOLD    2000
>  
> -struct wemac_eth_dev {
> +struct emac_eth_dev {
>       u32 speed;
>       u32 duplex;
>       u32 phy_configured;
>       int link_printed;
>  };
>  
> -struct wemac_rxhdr {
> +struct emac_rxhdr {
>       s16 rx_len;
>       u16 rx_status;
>  };
>  
> -static void wemac_inblk_32bit(void *reg, void *data, int count)
> +static void emac_inblk_32bit(void *reg, void *data, int count)
>  {
>       int cnt = (count + 3) >> 2;
>  
> @@ -181,7 +181,7 @@ static void wemac_inblk_32bit(void *reg, void *data, int 
> count)
>       }
>  }
>  
> -static void wemac_outblk_32bit(void *reg, void *data, int count)
> +static void emac_outblk_32bit(void *reg, void *data, int count)
>  {
>       int cnt = (count + 3) >> 2;
>  
> @@ -194,14 +194,12 @@ static void wemac_outblk_32bit(void *reg, void *data, 
> int count)
>       }
>  }
>  
> -/*
> - * Read a word from phyxcer
> - */
> -static int wemac_phy_read(const char *devname, unsigned char addr,
> +/* Read a word from phyxcer */
> +static int emac_phy_read(const char *devname, unsigned char addr,
>                         unsigned char reg, unsigned short *value)
>  {
>       struct eth_device *dev = eth_get_dev_by_name(devname);
> -     struct wemac_regs *regs = (struct wemac_regs *)dev->iobase;
> +     struct emac_regs *regs = (struct emac_regs *)dev->iobase;
>  
>       /* issue the phy address and reg */
>       writel(addr << 8 | reg, &regs->mac_madr);
> @@ -221,14 +219,12 @@ static int wemac_phy_read(const char *devname, unsigned 
> char addr,
>       return 0;
>  }
>  
> -/*
> - * Write a word to phyxcer
> - */
> -static int wemac_phy_write(const char *devname, unsigned char addr,
> +/* Write a word to phyxcer */
> +static int emac_phy_write(const char *devname, unsigned char addr,
>                          unsigned char reg, unsigned short value)
>  {
>       struct eth_device *dev = eth_get_dev_by_name(devname);
> -     struct wemac_regs *regs = (struct wemac_regs *)dev->iobase;
> +     struct emac_regs *regs = (struct emac_regs *)dev->iobase;
>  
>       /* issue the phy address and reg */
>       writel(addr << 8 | reg, &regs->mac_madr);
> @@ -250,7 +246,7 @@ static int wemac_phy_write(const char *devname, unsigned 
> char addr,
>  
>  static void emac_setup(struct eth_device *dev)
>  {
> -     struct wemac_regs *regs = (struct wemac_regs *)dev->iobase;
> +     struct emac_regs *regs = (struct emac_regs *)dev->iobase;
>       u32 reg_val;
>       u16 phy_val;
>       u32 duplex_flag;
> @@ -266,7 +262,7 @@ static void emac_setup(struct eth_device *dev)
>       writel(EMAC_MAC_CTL0_SETUP, &regs->mac_ctl0);
>  
>       /* Set MAC CTL1 */
> -     wemac_phy_read(dev->name, 1, 0, &phy_val);
> +     emac_phy_read(dev->name, 1, 0, &phy_val);
>       debug("PHY SETUP, reg 0 value: %x\n", phy_val);
>       duplex_flag = !!(phy_val & (1 << 8));
>  
> @@ -288,9 +284,9 @@ static void emac_setup(struct eth_device *dev)
>       writel(EMAC_MAC_MFL, &regs->mac_maxf);
>  }
>  
> -static void wemac_reset(struct eth_device *dev)
> +static void emac_reset(struct eth_device *dev)
>  {
> -     struct wemac_regs *regs = (struct wemac_regs *)dev->iobase;
> +     struct emac_regs *regs = (struct emac_regs *)dev->iobase;
>  
>       debug("resetting device\n");
>  
> @@ -302,10 +298,10 @@ static void wemac_reset(struct eth_device *dev)
>       udelay(200);
>  }
>  
> -static int sunxi_wemac_eth_init(struct eth_device *dev, bd_t *bd)
> +static int sunxi_emac_eth_init(struct eth_device *dev, bd_t *bd)
>  {
> -     struct wemac_regs *regs = (struct wemac_regs *)dev->iobase;
> -     struct wemac_eth_dev *priv = dev->priv;
> +     struct emac_regs *regs = (struct emac_regs *)dev->iobase;
> +     struct emac_eth_dev *priv = dev->priv;
>       u16 phy_reg;
>  
>       /* Init EMAC */
> @@ -317,10 +313,7 @@ static int sunxi_wemac_eth_init(struct eth_device *dev, 
> bd_t *bd)
>       /* Init MAC */
>  
>       /* Soft reset MAC */
> -     clrbits_le32(&regs->mac_ctl0, 1 << 15);
> -
> -     /* Set MII clock */
> -     clrsetbits_le32(&regs->mac_mcfg, 0xf << 2, 0xd << 2);
> +     clrbits_le32(&regs->mac_ctl0, 0x1 << 15);
>  
>       /* Clear RX counter */
>       writel(0x0, &regs->rx_fbc);
> @@ -336,14 +329,14 @@ static int sunxi_wemac_eth_init(struct eth_device *dev, 
> bd_t *bd)
>  
>       mdelay(1);
>  
> -     wemac_reset(dev);
> +     emac_reset(dev);
>  
>       /* PHY POWER UP */
> -     wemac_phy_read(dev->name, 1, 0, &phy_reg);
> -     wemac_phy_write(dev->name, 1, 0, phy_reg & (~(1 << 11)));
> +     emac_phy_read(dev->name, 1, 0, &phy_reg);
> +     emac_phy_write(dev->name, 1, 0, phy_reg & (~(0x1 << 11)));
>       mdelay(1);
>  
> -     wemac_phy_read(dev->name, 1, 0, &phy_reg);
> +     emac_phy_read(dev->name, 1, 0, &phy_reg);
>  
>       priv->speed = miiphy_speed(dev->name, 0);
>       priv->duplex = miiphy_duplex(dev->name, 0);
> @@ -357,11 +350,11 @@ static int sunxi_wemac_eth_init(struct eth_device *dev, 
> bd_t *bd)
>  
>       /* Set EMAC SPEED depend on PHY */
>       clrsetbits_le32(&regs->mac_supp, 1 << 8,
> -                     ((phy_reg & (1 << 13)) >> 13) << 8);
> +                     ((phy_reg & (0x1 << 13)) >> 13) << 8);
>  
>       /* Set duplex depend on phy */
>       clrsetbits_le32(&regs->mac_ctl1, 1 << 0,
> -                     ((phy_reg & (1 << 8)) >> 8) << 0);
> +                     ((phy_reg & (0x1 << 8)) >> 8) << 0);
>  
>       /* Enable RX/TX */
>       setbits_le32(&regs->ctl, 0x7);
> @@ -369,15 +362,15 @@ static int sunxi_wemac_eth_init(struct eth_device *dev, 
> bd_t *bd)
>       return 0;
>  }
>  
> -static void sunxi_wemac_eth_halt(struct eth_device *dev)
> +static void sunxi_emac_eth_halt(struct eth_device *dev)
>  {
>       /* Nothing to do here */
>  }
>  
> -static int sunxi_wemac_eth_recv(struct eth_device *dev)
> +static int sunxi_emac_eth_recv(struct eth_device *dev)
>  {
> -     struct wemac_regs *regs = (struct wemac_regs *)dev->iobase;
> -     struct wemac_rxhdr rxhdr;
> +     struct emac_regs *regs = (struct emac_regs *)dev->iobase;
> +     struct emac_rxhdr rxhdr;
>       u32 rxcount;
>       u32 reg_val;
>       int rx_len;
> @@ -386,8 +379,7 @@ static int sunxi_wemac_eth_recv(struct eth_device *dev)
>  
>       /* Check packet ready or not */
>  
> -     /*
> -      * Race warning: The first packet might arrive with
> +     /* Race warning: The first packet might arrive with
>        * the interrupts disabled, but the second will fix
>        */
>       rxcount = readl(&regs->rx_fbc);
> @@ -401,26 +393,25 @@ static int sunxi_wemac_eth_recv(struct eth_device *dev)
>       reg_val = readl(&regs->rx_io_data);
>       if (reg_val != 0x0143414d) {
>               /* Disable RX */
> -             clrbits_le32(&regs->ctl, 1 << 2);
> +             clrbits_le32(&regs->ctl, 0x1 << 2);
>  
>               /* Flush RX FIFO */
> -             setbits_le32(&regs->rx_ctl, 1 << 3);
> -             while (readl(&regs->rx_ctl) & (1 << 3))
> +             setbits_le32(&regs->rx_ctl, 0x1 << 3);
> +             while (readl(&regs->rx_ctl) & (0x1 << 3))
>                       ;
>  
>               /* Enable RX */
> -             setbits_le32(&regs->ctl, 1 << 2);
> +             setbits_le32(&regs->ctl, 0x1 << 2);
>  
>               return 0;
>       }
>  
> -     /*
> -      * A packet ready now
> +     /* A packet ready now
>        * Get status/length
>        */
>       good_packet = 1;
>  
> -     wemac_inblk_32bit(&regs->rx_io_data, &rxhdr, sizeof(rxhdr));
> +     emac_inblk_32bit(&regs->rx_io_data, &rxhdr, sizeof(rxhdr));
>  
>       rx_len = rxhdr.rx_len;
>       rx_status = rxhdr.rx_status;
> @@ -440,13 +431,13 @@ static int sunxi_wemac_eth_recv(struct eth_device *dev)
>                       printf("length error\n");
>       }
>  
> -     /* Move data from WEMAC */
> +     /* Move data from EMAC */
>       if (good_packet) {
>               if (rx_len > DMA_CPU_TRRESHOLD) {
>                       printf("Received packet is too big (len=%d)\n", rx_len);
>               } else {
> -                     wemac_inblk_32bit((void *)&regs->rx_io_data,
> -                                       NetRxPackets[0], rx_len);
> +                     emac_inblk_32bit((void *)&regs->rx_io_data,
> +                                      NetRxPackets[0], rx_len);
>  
>                       /* Pass to upper layer */
>                       NetReceive(NetRxPackets[0], rx_len);
> @@ -457,15 +448,15 @@ static int sunxi_wemac_eth_recv(struct eth_device *dev)
>       return 0;
>  }
>  
> -static int sunxi_wemac_eth_send(struct eth_device *dev, void *packet, int 
> len)
> +static int sunxi_emac_eth_send(struct eth_device *dev, void *packet, int len)
>  {
> -     struct wemac_regs *regs = (struct wemac_regs *)dev->iobase;
> +     struct emac_regs *regs = (struct emac_regs *)dev->iobase;
>  
>       /* Select channel 0 */
>       writel(0, &regs->tx_ins);
>  
>       /* Write packet */
> -     wemac_outblk_32bit((void *)&regs->tx_io_data, packet, len);
> +     emac_outblk_32bit((void *)&regs->tx_io_data, packet, len);
>  
>       /* Set TX len */
>       writel(len, &regs->tx_pl0);
> @@ -476,50 +467,55 @@ static int sunxi_wemac_eth_send(struct eth_device *dev, 
> void *packet, int len)
>       return 0;
>  }
>  
> -int sunxi_wemac_initialize(void)
> +int sunxi_emac_initialize(void)
>  {
>       struct sunxi_ccm_reg *const ccm =
>               (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
>       struct sunxi_sramc_regs *sram =
>               (struct sunxi_sramc_regs *)SUNXI_SRAMC_BASE;
> +     struct emac_regs *regs =
> +             (struct emac_regs *)SUNXI_EMAC_BASE;
>       struct eth_device *dev;
> -     struct wemac_eth_dev *priv;
> +     struct emac_eth_dev *priv;
>       int pin;
>  
>       dev = malloc(sizeof(*dev));
>       if (dev == NULL)
>               return -ENOMEM;
>  
> -     priv = (struct wemac_eth_dev *)malloc(sizeof(struct wemac_eth_dev));
> +     priv = (struct emac_eth_dev *)malloc(sizeof(struct emac_eth_dev));
>       if (!priv) {
>               free(dev);
>               return -ENOMEM;
>       }
>  
>       memset(dev, 0, sizeof(*dev));
> -     memset(priv, 0, sizeof(struct wemac_eth_dev));
> +     memset(priv, 0, sizeof(struct emac_eth_dev));
>  
>       /* Map SRAM to EMAC */
>       setbits_le32(&sram->ctrl1, 0x5 << 2);
>  
>       /* Configure pin mux settings for MII Ethernet */
>       for (pin = SUNXI_GPA(0); pin <= SUNXI_GPA(17); pin++)
> -             sunxi_gpio_set_cfgpin(pin, 2);
> +             sunxi_gpio_set_cfgpin(pin, SUNXI_GPA0_EMAC);
>  
>       /* Set up clock gating */
> -     setbits_le32(&ccm->ahb_gate0, 1 << AHB_GATE_OFFSET_EMAC);
> +     setbits_le32(&ccm->ahb_gate0, 0x1 << AHB_GATE_OFFSET_EMAC);
> +
> +     /* Set MII clock */
> +     clrsetbits_le32(&regs->mac_mcfg, 0xf << 2, 0xd << 2);
>  
> -     dev->iobase = SUNXI_EMAC_BASE;
> +     dev->iobase = (int)regs;
>       dev->priv = priv;
> -     dev->init = sunxi_wemac_eth_init;
> -     dev->halt = sunxi_wemac_eth_halt;
> -     dev->send = sunxi_wemac_eth_send;
> -     dev->recv = sunxi_wemac_eth_recv;
> -     strcpy(dev->name, "wemac");
> +     dev->init = sunxi_emac_eth_init;
> +     dev->halt = sunxi_emac_eth_halt;
> +     dev->send = sunxi_emac_eth_send;
> +     dev->recv = sunxi_emac_eth_recv;
> +     strcpy(dev->name, "emac");
>  
>       eth_register(dev);
>  
> -     miiphy_register(dev->name, wemac_phy_read, wemac_phy_write);
> +     miiphy_register(dev->name, emac_phy_read, emac_phy_write);
>  
>       return 0;
>  }
> diff --git a/include/netdev.h b/include/netdev.h
> index 63481ec..e45dd7a 100644
> --- a/include/netdev.h
> +++ b/include/netdev.h
> @@ -78,8 +78,8 @@ int sh_eth_initialize(bd_t *bis);
>  int skge_initialize(bd_t *bis);
>  int smc91111_initialize(u8 dev_num, int base_addr);
>  int smc911x_initialize(u8 dev_num, int base_addr);
> +int sunxi_emac_initialize(bd_t *bis);
>  int sunxi_gmac_initialize(bd_t *bis);
> -int sunxi_wemac_initialize(bd_t *bis);
>  int tsi108_eth_initialize(bd_t *bis);
>  int uec_standard_init(bd_t *bis);
>  int uli526x_initialize(bd_t *bis);

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 10/12] net: Rename and cleanup sunxi (Allwinner) emac driver

Reply via email to