Re: [ECOS] FW: Re: [ECOS] bugs in AT91 Ethernet driver

[Andrew Lunn <andrew at lunn dot ch>]

Hi Juergen

I would like to start cleaning this up for inclusion into CVS.
This is going to take a few iterations. I think some of your changes
can be done in better ways. 

Attached is two patches for the first iteration. This fixes the
receiver SG bugs and allowing the PHY driver part to be
removed. Rather than your PHY_PRESENT i changed to
CYGPKG_DEVS_ETH_PHY. If you remove this package from your
configuration, the PHY parts in the if_at91.c will not be compiled.

First apply to a clean checkout the two patches in this order:
if_at91_1_phy.diff 
if_at91_9_rest.diff

The second one will give some warnings like the following, but it is
OK:

$ patch -p0 < ~/if_at91_udiff2.txt
patching file src/if_at91.c
Hunk #1 succeeded at 345 (offset 5 lines).
Hunk #2 succeeded at 659 (offset 13 lines).
Hunk #3 succeeded at 735 (offset 13 lines).
Hunk #4 succeeded at 760 (offset 13 lines).
Hunk #5 succeeded at 793 (offset 13 lines).
Hunk #6 succeeded at 845 (offset 13 lines).
Hunk #7 succeeded at 873 (offset 13 lines).

The first file contains the changes for PHY and RX SG. The second file
is the rest. Please test this still works for you.

I guess the iterations will be:
1) CYGINT_IO_ETH_INT_SUPPORT_REQUIRED
2) TX buffer descriptor changes
3) TX from sram

1) should be easy.

2) i think there must be a cleaner way to do this. You get an error
each time which to me sounds wrong and i don't like the b_reset_tbd_id
flag you added etc.

3) is going to be interesting, since it is only required for devices
with an external bus. We probably need to add a macro to the HAL which
we pass the tbd to so it can mangle it. 

   Andrew

Index: ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/eth/arm/at91/current/ChangeLog,v
retrieving revision 1.3
diff -u -r1.3 ChangeLog
--- ChangeLog	9 Apr 2007 12:59:30 -0000	1.3
+++ ChangeLog	7 Jun 2008 13:37:37 -0000
@@ -1,3 +1,12 @@
+2008-06-04  Jürgen Lambrecht <J.Lambrecht@TELEVIC.com>
+            Andrew Lunn  <andrew.lunn@ascom.ch>
+	
+	* src/if_at91.c (at91_eth_recv): Fix receive into multiple
+	SG buffers, which the FreeBSD stack uses, unlike LWIP and
+	RedBoot.
+	* src/if_at91.c: Make PHY support depend on having the
+	CYGPKG_DEVS_ETH_PHY package loaded.
+
 2007-04-08  Uwe Kindler  <uwe_kindler@web.de>
 
 	* cdl/at91_eth.cdl: Fixed typo. (Removed AT91RM9200 from
Index: src/if_at91.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/eth/arm/at91/current/src/if_at91.c,v
retrieving revision 1.2
diff -u -r1.2 if_at91.c
--- src/if_at91.c	23 Mar 2007 19:02:09 -0000	1.2
+++ src/if_at91.c	7 Jun 2008 13:37:38 -0000
@@ -39,10 +39,10 @@
 //#####DESCRIPTIONBEGIN####
 //
 // Author(s):    Andrew Lunn, John Eigelaar
-// Contributors:  
+// Contributors: Juergen Lambrecht  
 // Date:         2006-05-10
 // Purpose:
-// Description:
+// Description:  network driver for AT91 EMAC block of AT91SAM uC's.
 //
 //####DESCRIPTIONEND####
 //
@@ -67,7 +67,9 @@
 #include <cyg/io/eth/netdev.h>
 #include <cyg/io/eth/eth_drv.h>
 #include <cyg/io/eth/eth_drv_stats.h>
-#include <cyg/io/eth_phy.h>
+#ifdef CYGPKG_DEVS_ETH_PHY
+   #include <cyg/io/eth_phy.h>
+#endif
 #include <errno.h>
 #include <string.h>
 
@@ -168,7 +170,9 @@
    char *esa_key;      // RedBoot 'key' for device ESA
    cyg_uint8 *enaddr;
    cyg_uint32 base;    // Base address of device
+#ifdef CYGPKG_DEVS_ETH_PHY
    eth_phy_access_t *phy;
+#endif
    rbd_t rbd[CYGNUM_DEVS_ETH_ARM_AT91_RX_BUFS];
    rb_t  rb[CYGNUM_DEVS_ETH_ARM_AT91_RX_BUFS];
    tbd_t tbd[CYGNUM_DEVS_ETH_ARM_AT91_TX_BUFS];
@@ -183,6 +187,7 @@
 #endif
 } at91_eth_priv_t;
 
+#ifdef CYGPKG_DEVS_ETH_PHY
 //============================================================================
 // PHY access bits and pieces
 // 
@@ -306,7 +311,7 @@
                               NULL,
                               at91_write_phy,
                               at91_read_phy);
-
+#endif
 //======================================================================
 // Receiver buffer handling
 
@@ -485,7 +490,9 @@
    bool esa_ok = false;
    unsigned char enaddr[6] = { CYGPKG_DEVS_ETH_ARM_AT91_MACADDR};
    unsigned char enzero[6] = { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0};
+#ifdef CYGPKG_DEVS_ETH_PHY
    unsigned short phy_state = 0;
+#endif
    cyg_uint32 ncfg = 0;
 
    debug1_printf("\nAT91_ETH: Initialising @ %x\n",priv->base);
@@ -560,6 +567,7 @@
    // And tell the EMAC where the first transmit buffer descriptor is
    HAL_WRITE_UINT32(priv->base + AT91_EMAC_TBQP, (cyg_uint32)priv->tbd);
 
+#ifdef CYGPKG_DEVS_ETH_PHY
    // Setup the PHY
    CYG_ASSERTC(priv->phy);
 
@@ -573,10 +581,11 @@
    // Get the current mode and print it
    phy_state = _eth_phy_state(priv->phy);
    at91_mdio_disable();
+#endif
 
    HAL_READ_UINT32(priv->base + AT91_EMAC_NCFG,ncfg);
 
-
+#ifdef CYGPKG_DEVS_ETH_PHY
    if ((phy_state & ETH_PHY_STAT_LINK) != 0)
    {
       if (((phy_state & ETH_PHY_STAT_100MB) != 0))
@@ -604,7 +613,11 @@
    {
       debug1_printf("AT91_ETH: No Link\n");
    }
-
+#else
+   // Hard code to 100Mbps since we cannot ask the PHY what
+   // to use.
+   ncfg |= AT91_EMAC_NCFG_SPD_100Mbps;
+#endif
 
    //Setup the network configuration
    ncfg |= (AT91_EMAC_NCFG_RLCE);
@@ -937,6 +950,8 @@
 
    for(i = 0;i<sg_len;i++)
    {
+      bytes_needed_list = 0;
+     
       while(bytes_in_list < sg_list[i].len)
       {
          bytes_needed_list = sg_list[i].len - bytes_in_list;
@@ -945,7 +960,7 @@
          {
 	      bytes_in_buffer = 
 		((priv->rbd[priv->curr_rbd_idx].sr & AT91_EMAC_RBD_SR_LEN_MASK)
-		 - total_bytes) - buffer_pos;
+		 - total_bytes);
          }
          else
          {
@@ -1005,7 +1020,9 @@
 {
    .intr_vector = CYGNUM_HAL_INTERRUPT_EMAC,
    .base = AT91_EMAC,
+#ifdef CYGPKG_DEVS_ETH_PHY
    .phy = &at91_phy
+#endif
 };
 
 ETH_DRV_SC(at91_sc,

--- src/if_at91.c	2008-03-07 08:35:36.000000000 +-0200
+++ src/if_at91.c	2008-06-04 00:01:51.000000000 +-0200
@@ -333,13 +340,13 @@
 at91_tb_init(at91_eth_priv_t *priv)
 {
    int i;
    for (i = 0 ; i < CYGNUM_DEVS_ETH_ARM_AT91_TX_BUFS; i++)
    {
       priv->tbd[i].addr = 0;
-      priv->tbd[i].sr = AT91_EMAC_TBD_SR_USED;
+      priv->tbd[i].sr = 0; /* datasheet 36.4.1.3 for SAM9260 vG and 37.4.1.3 for SAM7 vG point 2 */
    }
    // Set the wrap bit on the last entry
    priv->tbd[CYGNUM_DEVS_ETH_ARM_AT91_TX_BUFS-1].sr |= AT91_EMAC_TBD_SR_WRAP;
 }
 
 //======================================================================
@@ -630,31 +646,40 @@
 
 // This function is called to stop the interface.
 static void 
 at91_eth_stop(struct eth_drv_sc *sc)
 {
    at91_eth_priv_t *priv = (at91_eth_priv_t *)sc->driver_private;
+#ifdef CYGINT_IO_ETH_INT_SUPPORT_REQUIRED
+   cyg_uint32 bits;
 
+   bits = (AT91_EMAC_ISR_RCOM | AT91_EMAC_ISR_TOVR | AT91_EMAC_ISR_TUND |
+           AT91_EMAC_ISR_RTRY | AT91_EMAC_ISR_TBRE | AT91_EMAC_ISR_TCOM);
+
+   HAL_WRITE_UINT32(priv->base + AT91_EMAC_IDR, bits);
+#endif
    at91_disable(priv);
 }
 
 // This function is called to "start up" the interface. It may be called
 // multiple times, even when the hardware is already running.
 static void
 at91_eth_start(struct eth_drv_sc *sc, unsigned char *enaddr, int flags)
 {
    at91_eth_priv_t *priv = (at91_eth_priv_t *)sc->driver_private;
+#ifdef CYGINT_IO_ETH_INT_SUPPORT_REQUIRED
    cyg_uint32 bits;
 
    // Enable the interrupts we are interested in
    // TODO: We probably need to add at least the RBNA interrupt here
    //       as well in order to do some error handling
-   bits = (AT91_EMAC_ISR_RCOM | AT91_EMAC_ISR_TCOM);
+   bits = (AT91_EMAC_ISR_RCOM | AT91_EMAC_ISR_TOVR | AT91_EMAC_ISR_TUND |
+           AT91_EMAC_ISR_RTRY | AT91_EMAC_ISR_TBRE | AT91_EMAC_ISR_TCOM);
 
    HAL_WRITE_UINT32(priv->base + AT91_EMAC_IER, bits);
-
+#endif
    // Enable the receiver and transmitter
    at91_enable(priv);
 }
 
 // This function is called for low level "control" operations
 static int
@@ -697,12 +722,13 @@
 //
 // We allocate one buffer descriptor per scatter/gather entry. We assume that
 // a typical packet will not have more than 3 such entries, and so we say we
 // can send a packet when we have 3 or more buffer descriptors free
 //
 // TODO: Implement what the comment actually says!
+// JL: more difficult with copy to SRAM..
 static int
 at91_eth_can_send(struct eth_drv_sc *sc)
 {
    int can_send;
    at91_eth_priv_t *priv = (at91_eth_priv_t *)sc->driver_private;
    if(priv->tx_busy)
@@ -721,20 +747,27 @@
 at91_eth_send(struct eth_drv_sc *sc, struct eth_drv_sg *sg_list, int sg_len, 
               int total_len, unsigned long key)
 {
    at91_eth_priv_t *priv = (at91_eth_priv_t *)sc->driver_private;
    int i;
    cyg_uint32 sr;
-
-   priv->tx_busy = true;
+#ifdef SRAM1_ORIGIN /* define it in plf_io.h if present */
+   cyg_uint32 total_bytes = 0; /* position in SRAM1 */
+#endif
+   priv->tx_busy = true; /* for can_send() */
 
-   priv->last_tbd_idx = priv->curr_tbd_idx;
+   priv->last_tbd_idx = priv->curr_tbd_idx; //??
 
    for(i = 0;i<sg_len;i++)
    {
+#ifdef SRAM1_ORIGIN
+      memcpy((cyg_uint8 *)(SRAM1_ORIGIN+total_bytes), (cyg_uint8 *)(sg_list[i].buf), sg_list[i].len);
+      priv->tbd[priv->curr_tbd_idx].addr = SRAM1_ORIGIN+total_bytes;
+#else
       priv->tbd[priv->curr_tbd_idx].addr = sg_list[i].buf;
+#endif
 
       sr = (sg_list[i].len & AT91_EMAC_TBD_SR_LEN_MASK);
       // Set the End Of Frame bit in the last descriptor
       if(i == (sg_len-1))
       {
          sr |= AT91_EMAC_TBD_SR_EOF;
@@ -747,37 +780,45 @@
       }
       else
       {
          priv->tbd[priv->curr_tbd_idx].sr = (sr | AT91_EMAC_TBD_SR_WRAP);
          priv->curr_tbd_idx = 0;
       }
+#ifdef SRAM1_ORIGIN
+      total_bytes += sg_list[i].len;
+#endif
    }
 
    // Store away the key for when the transmit has completed
    // and we need to tell the stack which transmit has completed.
    priv->curr_tx_key = key;
 
    at91_start_transmitter(priv);
 }
 
-static void at91_reset_tbd(at91_eth_priv_t *priv)
+/* datasheet 36.4.1.3 for SAM9260 vG and 37.4.1.3 for SAM7 vG point 2: 
+   set AT91_EMAC_TBD_SR_USED=0 */
+static void at91_reset_tbd(at91_eth_priv_t *priv, bool b_reset_tbd_idx)
 {
      while(priv->curr_tbd_idx != priv->last_tbd_idx)
      {
         if(priv->last_tbd_idx == (CYGNUM_DEVS_ETH_ARM_AT91_TX_BUFS-1))
         {
-           priv->tbd[priv->last_tbd_idx].sr = 
-             (AT91_EMAC_TBD_SR_USED|AT91_EMAC_TBD_SR_WRAP);
+           priv->tbd[priv->last_tbd_idx].sr = AT91_EMAC_TBD_SR_WRAP;
            priv->last_tbd_idx = 0;
         }
         else
         {
-           priv->tbd[priv->last_tbd_idx].sr = AT91_EMAC_TBD_SR_USED;
+           priv->tbd[priv->last_tbd_idx].sr = 0;
            priv->last_tbd_idx++;
         }
-     }
+     }
+     if (b_reset_tbd_idx)
+     {
+        priv->curr_tbd_idx = 0; //reset, because EMAC also resets its queue 
+     }                          //pointer to the TBQP
 }
 
 
 //======================================================================
 
 #ifdef CYGINT_IO_ETH_INT_SUPPORT_REQUIRED
@@ -791,21 +832,23 @@
 
    /* Get the interrupt status */
    HAL_READ_UINT32(priv->base+AT91_EMAC_ISR,isr);
 
    ret = CYG_ISR_HANDLED;
 
-   //TODO: We should probably be handling some of the error interrupts as well
-   if(isr & AT91_EMAC_ISR_TCOM)
+   if(isr & (AT91_EMAC_ISR_TOVR | AT91_EMAC_ISR_TUND | AT91_EMAC_ISR_RTRY | AT91_EMAC_ISR_TBRE | AT91_EMAC_ISR_TCOM))
    {
       ret = CYG_ISR_CALL_DSR;
+      debug2_printf("TX IRQ\n");
    }
 
+   //TODO: We should probably be handling some of the error interrupts as well
    if(isr & AT91_EMAC_ISR_RCOM)
    {
       ret = CYG_ISR_CALL_DSR;
+      debug2_printf("RX IRQ\n");
    }
    cyg_interrupt_acknowledge(vector);
    return(ret);
 }
 #endif
 
@@ -817,56 +860,79 @@
    cyg_uint32 tsr;
    cyg_uint32 rsr;
 
    cyg_uint32 ctr;
    cyg_uint32 cnt;
    cyg_uint32 idx;
+   bool b_reset_tbd_idx = false;
 
-   /* Get the Transmit Status */
+   /* Get the Transmit Status and clear it */
    HAL_READ_UINT32(priv->base+AT91_EMAC_TSR,tsr);
    HAL_WRITE_UINT32(priv->base+AT91_EMAC_TSR,tsr);
 
-   /* Get the Receive Status */
+   /* Get the Receive Status and clear it */
    HAL_READ_UINT32(priv->base+AT91_EMAC_RSR,rsr);
    HAL_WRITE_UINT32(priv->base+AT91_EMAC_RSR,rsr);
 
 
    //TODO: The interrupts other than RCOMP and TCOMP needs to be
    //      handled properly especially stuff like RBNA which could have
    //      serious effects on driver performance
 
-   /* Service the TX buffers */
+   /* Service the TX buffers after IRQ */
+   if (tsr&AT91_EMAC_TSR_OVR)  //0
+   {
+      debug1_printf("AT91_ETH: Tx UBR\n");
+      b_reset_tbd_idx = true;
+   }
+
    if (tsr&AT91_EMAC_TSR_COL)  //1
    {
       debug1_printf("AT91_ETH: Tx COL\n");
    }
 
    if (tsr&AT91_EMAC_TSR_RLE)  //2
    {
       debug1_printf("AT91_ETH: Tx RLE\n");
+      b_reset_tbd_idx = true;
    }
 
+   if ((tsr&AT91_EMAC_TSR_TXIDLE) == 0) //3
+   {
+      debug2_printf("AT91_ETH: Tx IDLE\n");
+   }
+
    if (tsr&AT91_EMAC_TSR_BNQ)  //4
    {
       debug1_printf("AT91_ETH: Tx BEX\n");
+      b_reset_tbd_idx = true;
    }
 
    if (tsr&AT91_EMAC_TSR_UND)  //6
    {
       debug1_printf("AT91_ETH: Tx UND\n");
+      b_reset_tbd_idx = true;
    }
 
    /* Check that the last transmission is completed */
+   /* After each transmit, it is best to immediately reset tbd */
+   /* If there were errors, tbd_idx must be reset also */
    if (tsr&AT91_EMAC_TSR_COMP) //5
    {
-      at91_reset_tbd(priv);
+      debug2_printf("AT91_ETH: Tx COMP.\n");
+      at91_reset_tbd(priv, b_reset_tbd_idx);
       _eth_drv_tx_done(sc,priv->curr_tx_key,0);
       priv->tx_busy = false;
    }
+   else if (b_reset_tbd_idx == true)
+   {
+      at91_reset_tbd(priv, b_reset_tbd_idx);
+   }
 
-   /* Service the RX buffers when we get something */
+
+   /* Service the RX buffers when we get something after IRQ */
    if (rsr&AT91_EMAC_RSR_REC)
    {
       /* Do this all until we find the first EMAC Buffer */
       while (priv->rbd[priv->curr_rbd_idx].addr & AT91_EMAC_RBD_ADDR_OWNER_SW)
       {
 

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss