Re: [U-Boot] [PATCH 2/4 v2] net: Add driver for Zynq Gem IP

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(&regs->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, &regs->idr);
• /* Disable the receiver & transmitter */
• writel(0, &regs->nwctrl);
• writel(0, &regs->txsr);
• writel(0, &regs->rxsr);
• writel(0, &regs->phymntnc);
• /* Clear the Hash registers for the mac address pointed by AddressPtr */
• writel(0x0, &regs->hashl);
• /* Write bits [63:32] in TOP */
• writel(0x0, &regs->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(&regs->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), &regs->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, &regs->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, &regs->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(&regs->nwctrl);
• /* MDIO, Rx and Tx enable */
• tmp |= ZYNQ_GEM_NWCTRL_MDEN_MASK | ZYNQ_GEM_NWCTRL_RXEN_MASK |

•    ZYNQ_GEM_NWCTRL_TXEN_MASK;
  

• writel(tmp, &regs->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), &regs->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(&regs->nwctrl) | ZYNQ_GEM_NWCTRL_STARTTX_MASK;
• writel(val, &regs->nwctrl);

setbits_le32()

ok

...

• /* Read the stat register to know if the packet has been transmitted */
• status = readl(&regs->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, &regs->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