From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Simek Date: Thu, 13 Sep 2012 12:16:29 +0200 Subject: [U-Boot] [PATCH 2/4 v2] net: Add driver for Zynq Gem IP In-Reply-To: <201209131128.52353.marex@denx.de> References: <1345098630-27902-1-git-send-email-monstr@monstr.eu> <1345098630-27902-2-git-send-email-monstr@monstr.eu> <201209131128.52353.marex@denx.de> Message-ID: <5051B27D.8040009@monstr.eu> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 09/13/2012 11:28 AM, Marek Vasut wrote: > Dear Michal Simek, > > [...] > >> +static inline int mdio_wait(struct eth_device *dev) >> +{ >> + struct zynq_gem_regs *regs = (struct zynq_gem_regs *)dev->iobase; >> + u32 timeout = 200; >> + >> + /* Wait till MDIO interface is ready to accept a new transaction. */ >> + while (timeout && !(readl(®s->nwsr) & ZYNQ_GEM_NWSR_MDIOIDLE_MASK)) { > > I'd say, rework this to > > while (--timeout) { > if (readl() & ... ) > break; > WATCHDOG_RESET(); > } > > The WATCHDOG_RESET will give you the udelay and restart the WDT if you use any. > Also, I think it's more readable when you omit the complex condition for the > while cycle and split it a bit. make sense > >> + timeout--; >> + udelay(1); >> + } >> + >> + if (!timeout) { >> + printf("%s: Timeout\n", __func__); >> + return 1; >> + } >> + >> + return 0; >> +} > > [...] > >> +static int zynq_gem_init(struct eth_device *dev, bd_t * bis) >> +{ >> + int tmp; >> + int i; >> + struct zynq_gem_regs *regs = (struct zynq_gem_regs *)dev->iobase; >> + struct zynq_gem_priv *priv = dev->priv; >> + struct phy_device *phydev; >> + u32 supported = SUPPORTED_10baseT_Half | >> + SUPPORTED_10baseT_Full | >> + SUPPORTED_100baseT_Half | >> + SUPPORTED_100baseT_Full | >> + SUPPORTED_1000baseT_Half | >> + SUPPORTED_1000baseT_Full; >> + >> + if (priv->initialized) >> + return 0; >> + >> + /* Disable all interrupts */ >> + writel(0xFFFFFFFF, ®s->idr); >> + >> + /* Disable the receiver & transmitter */ >> + writel(0, ®s->nwctrl); >> + writel(0, ®s->txsr); >> + writel(0, ®s->rxsr); >> + writel(0, ®s->phymntnc); >> + >> + /* Clear the Hash registers for the mac address pointed by AddressPtr */ >> + writel(0x0, ®s->hashl); >> + /* Write bits [63:32] in TOP */ >> + writel(0x0, ®s->hashh); >> + >> + /* Clear all counters */ >> + for (i = 0; i <= (sizeof(struct zynq_gem_regs) - >> + offsetof(struct zynq_gem_regs, stat)) / 4; i++) > > Add a const int variable and use it here so you don't have to break the for () . if you like. > >> + readl(®s->stat[i]); >> + >> + /* Setup RxBD space */ >> + memset(&(priv->rx_bd), 0, sizeof(priv->rx_bd)); >> + /* Create the RxBD ring */ >> + memset(&(priv->rxbuffers), 0, sizeof(priv->rxbuffers)); >> + >> + for (i = 0; i < RX_BUF; i++) { >> + priv->rx_bd[i].status = 0xF0000000; >> + priv->rx_bd[i].addr = (u32)((char *) &(priv->rxbuffers) + >> + (i * PKTSIZE_ALIGN)); >> + } >> + /* WRAP bit to last BD */ >> + priv->rx_bd[--i].addr |= ZYNQ_GEM_RXBUF_WRAP_MASK; >> + /* Write RxBDs to IP */ >> + writel((u32) &(priv->rx_bd), ®s->rxqbase); >> + >> + >> + /* MAC Setup */ >> + /* >> + * Following is the setup for Network Configuration register. >> + * Bit 0: Set for 100 Mbps operation. >> + * Bit 1: Set for Full Duplex mode. >> + * Bit 4: Unset to allow Copy all frames - MAC checking >> + * Bit 17: Set for FCS removal. >> + * Bits 20-18: Set with value binary 010 to divide pclk by 32 >> + * (pclk up to 80 MHz) >> + */ >> + writel(0x000A0003, ®s->nwcfg); > > Well you know ... magic numbers and defined bits ;-) :-) Or. let me create macros. > >> + /* >> + * Following is the setup for DMA Configuration register. >> + * Bits 4-0: To set AHB fixed burst length for DMA data operations -> >> + * Set with binary 00100 to use INCR4 AHB bursts. >> + * Bits 9-8: Receiver packet buffer memory size -> >> + * Set with binary 11 to Use full configured addressable space (8 Kb) >> + * Bit 10 : Transmitter packet buffer memory size -> >> + * Set with binary 1 to Use full configured addressable space (4 Kb) >> + * Bits 23-16 : DMA receive buffer size in AHB system memory -> >> + * Set with binary 00011000 to use 1536 byte(1*max length frame/buffer) >> + */ >> + writel(0x00180704, ®s->dmacr); the same here. >> + >> + /* >> + * Following is the setup for Network Control register. >> + * Bit 2: Set to enable Receive operation. >> + * Bit 3: Set to enable Transmitt operation. >> + * Bit 4: Set to enable MDIO operation. >> + */ >> + tmp = readl(®s->nwctrl); >> + /* MDIO, Rx and Tx enable */ >> + tmp |= ZYNQ_GEM_NWCTRL_MDEN_MASK | ZYNQ_GEM_NWCTRL_RXEN_MASK | >> + ZYNQ_GEM_NWCTRL_TXEN_MASK; >> + writel(tmp, ®s->nwctrl); > > setbits_le32() ok. > >> + /* interface - look at tsec */ >> + phydev = phy_connect(priv->bus, priv->phyaddr, dev, 0); >> + >> + phydev->supported &= supported; >> + phydev->advertising = phydev->supported; >> + priv->phydev = phydev; >> + phy_config(phydev); >> + phy_startup(phydev); >> + >> + priv->initialized = 1; >> + return 0; >> +} >> + >> +static int zynq_gem_send(struct eth_device *dev, void *ptr, int len) >> +{ >> + int status; >> + u32 val; >> + struct zynq_gem_priv *priv = dev->priv; >> + struct zynq_gem_regs *regs = (struct zynq_gem_regs *)dev->iobase; >> + >> + if (!priv->initialized) { >> + puts("Error GMAC not initialized"); >> + return -1; >> + } >> + >> + /* setup BD */ >> + writel((u32)&(priv->tx_bd), ®s->txqbase); >> + >> + /* Setup Tx BD */ >> + memset((void *) &(priv->tx_bd), 0, sizeof(struct emac_bd)); >> + >> + priv->tx_bd.addr = (u32)ptr; >> + priv->tx_bd.status = len | ZYNQ_GEM_TXBUF_LAST_MASK | >> + ZYNQ_GEM_TXBUF_WRAP_MASK; >> + >> + /* Start transmit */ >> + val = readl(®s->nwctrl) | ZYNQ_GEM_NWCTRL_STARTTX_MASK; >> + writel(val, ®s->nwctrl); > > setbits_le32() ok > >> + /* Read the stat register to know if the packet has been transmitted */ >> + status = readl(®s->txsr); >> + if (status & (ZYNQ_GEM_TXSR_HRESPNOK_MASK | ZYNQ_GEM_TXSR_URUN_MASK | >> + ZYNQ_GEM_TXSR_BUFEXH_MASK)) { > > Add const int mask for the above. or macro should be fine. > >> + printf("Something has gone wrong here!? Status is 0x%x.\n", >> + status); >> + } >> + >> + /* Clear Tx status register before leaving . */ >> + writel(status, ®s->txsr); >> + return 0; >> +} >> + >> +/* Do not check frame_recd flag in rx_status register 0x20 - just poll BD >> */ +static int zynq_gem_recv(struct eth_device *dev) >> +{ >> + int frame_len; >> + struct zynq_gem_priv *priv = dev->priv; >> + >> + if (!(priv->rx_bd[priv->rxbd_current].addr & ZYNQ_GEM_RXBUF_NEW_MASK)) >> + return 0; >> + >> + if (!(priv->rx_bd[priv->rxbd_current].status & >> + (ZYNQ_GEM_RXBUF_SOF_MASK | ZYNQ_GEM_RXBUF_EOF_MASK))) { >> + printf("GEM: SOF or EOF not set for last buffer received!\n"); >> + return 0; >> + } >> + >> + frame_len = priv->rx_bd[priv->rxbd_current].status & >> + ZYNQ_GEM_RXBUF_LEN_MASK; > > Just introduce some variable for priv->rx_bd[priv->rxbd_current] so you don't > have such long lines no problem. > >> + if (frame_len) { >> + NetReceive((u8 *) (priv->rx_bd[priv->rxbd_current].addr & >> + ZYNQ_GEM_RXBUF_ADD_MASK), frame_len); >> + >> + if (priv->rx_bd[priv->rxbd_current].status & >> + ZYNQ_GEM_RXBUF_SOF_MASK) >> + priv->rx_first_buf = priv->rxbd_current; >> + else { >> + priv->rx_bd[priv->rxbd_current].addr &= >> + ~ZYNQ_GEM_RXBUF_NEW_MASK; >> + priv->rx_bd[priv->rxbd_current].status = 0xF0000000; >> + } >> + >> + if (priv->rx_bd[priv->rxbd_current].status & >> + ZYNQ_GEM_RXBUF_EOF_MASK) { >> + priv->rx_bd[priv->rx_first_buf].addr &= >> + ~ZYNQ_GEM_RXBUF_NEW_MASK; >> + priv->rx_bd[priv->rx_first_buf].status = 0xF0000000; >> + } >> + >> + if ((++priv->rxbd_current) >= RX_BUF) >> + priv->rxbd_current = 0; >> + >> + return frame_len; >> + } >> + >> + return 0; >> +} >> + >> +static void zynq_gem_halt(struct eth_device *dev) >> +{ >> + return; > > Does this need to be implemented at all ? probably not. Let me do that changes and I will send updated version. 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