[ECOS] bugs in AT91 Ethernet driver

[ECOS] bugs in AT91 Ethernet driver

[Lambrecht Jürgen]

[-- Attachment #1: Type: text/plain, Size: 3739 bytes --]

Hello,

I found some bugs in the AT91 EMAC Ethernet driver - /packages/devs/eth/arm/at91/current/src/if_at91.c.
The bugs are present in at91_eth_recv(..) because the author did not understand how scatter-gather lists work I think. In Redboot, the driver works, and maybe also with LWIP, but not with freebsd.

With my fix, RX seems to work on my AT91SAM9260-EK based board.

TX only works for the first packet the freebsd stack sends out (a gratuitous ARP). The next packet it sends as response to an ARP request does not get out. I Monitor with Wireshark. Debugging in ecos is on to print the packet contents. 
The TX ARP packet is a bit strange: it is a broadcast instead of a unicast as it should be to the originator of the ARP request. And also SG[1] is printed - I thought the sg-list should always start with SG[0]?
I don't find a bug in the TX driver..
Maybe somebody else has an idea what could be wrong?

Kind regards,
Juergen

P.S.: in attachment a unix dif; that's not in the correct format for the ecos list I think - I need to take the time to look up how to....


This is my version of at91_eth_recv(..):

static void
at91_eth_recv(struct eth_drv_sc *sc,
              struct eth_drv_sg *sg_list,
              int sg_len)
{
   at91_eth_priv_t *priv = (at91_eth_priv_t *)sc->driver_private;
   int i;
   cyg_uint32 bytes_in_buffer;
   cyg_uint32 bytes_in_list = 0;
   cyg_uint32 bytes_needed_list = 0;
   cyg_uint32 buffer_pos = 0;
   cyg_uint8 * sg_buf;
   cyg_uint32 total_bytes = 0;
   /* buffer_pos is position in current 128B buffer */
   /* bytes_in_list is position in current sg_list */
   /* total bytes is total no. of packet bytes already copied */
   /* todo: check all cases */
   /*  1. packet in 1 rd-buf OK */
   /*  2. big packet in several rd-buf OK */
   for(i = 0;i<sg_len;i++)
   {
      while(bytes_in_list < sg_list[i].len) //freebsd - i=0: 14B
      {                                     //freebsd - i=1: 128B or remainder of packet
         bytes_needed_list = sg_list[i].len - bytes_in_list;

         if(priv->rbd[priv->curr_rbd_idx].sr & AT91_EMAC_RBD_SR_EOF)
         { /* This 128B buffer contains the End Of the Frame. */
            bytes_in_buffer =
		((priv->rbd[priv->curr_rbd_idx].sr & AT91_EMAC_RBD_SR_LEN_MASK)
		 - total_bytes); /* - buffer_pos; */
             //??? total_bytes already contains buffer_pos; ... 
         }
         else
         { /* AT91_EMAC_RX_BUFF_SIZE = 128B */
            bytes_in_buffer = AT91_EMAC_RX_BUFF_SIZE - buffer_pos;
         }

         sg_buf = (cyg_uint8 *)(sg_list[i].buf);

         if(bytes_needed_list < bytes_in_buffer)
         {
            if(sg_buf != NULL)
               memcpy(sg_buf, /* &sg_buf[bytes_in_list], */
		      &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
		      bytes_needed_list);
            bytes_in_list += bytes_needed_list;
            buffer_pos += bytes_needed_list;
            total_bytes += bytes_needed_list;
         }
         else
         {
            if(sg_buf != NULL)
              memcpy(sg_buf, /* wrong: &sg_buf[bytes_in_list], */
		     &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
		     bytes_in_buffer);
            bytes_in_list += bytes_in_buffer;
            total_bytes += bytes_in_buffer;

            /* Step our buffer on one */
            priv->rbd[priv->curr_rbd_idx].addr &= 
	      ~(AT91_EMAC_RBD_ADDR_OWNER_SW);
            priv->curr_rbd_idx++;
            if(priv->curr_rbd_idx >= CYGNUM_DEVS_ETH_ARM_AT91_RX_BUFS)
            {
               priv->curr_rbd_idx = 0;
            }
            buffer_pos = 0;
         }
      }
      bytes_in_list = 0; /* go to next list */
   }
}

[-- Attachment #2: diff.txt --]
[-- Type: text/plain, Size: 1628 bytes --]

42c42
< // Contributors: Juergen Lambrecht 
---
> // Contributors:  
44,45c44,45
< // Purpose:      BSD compatible network driver
< // Description:  HW network driver for AT91 EMAC block of AT91SAM uC's.
---
> // Purpose:
> // Description:
46,47d45
< // Limitations:  Jumbo frames are not supported because of
< //               AT91_EMAC_RBD_SR_LEN_MASK = 0xFFF
953c951
<    /* buffer_pos is position in current 128B buffer */
---
> 
954,957d951
<    /* bytes_in_list is position in current sg_list */
<    /* total bytes is total no. of packet bytes already copied */
<    /* todo: check all cases */
<    /* 1. packet in 1 rd-buf */
960,961c954,955
<       while(bytes_in_list < sg_list[i].len) //freebsd - i=0: 14B
<       {                                     //freebsd - i=1: 128B or remainder of packet
---
>       while(bytes_in_list < sg_list[i].len)
>       {
965,966c959,960
<          { /* This 128B buffer contains the End Of the Frame. */
<             bytes_in_buffer =
---
>          {
> 	      bytes_in_buffer = 
968c962
< 		 - total_bytes); /* - buffer_pos; */
---
> 		 - total_bytes) - buffer_pos;
969d962
<              //??? total_bytes already contains buffer_pos; ... 
972c965
<          { /* AT91_EMAC_RX_BUFF_SIZE = 128B */
---
>          {
981c974
<                memcpy(sg_buf, /* &sg_buf[bytes_in_list], */
---
>                memcpy(&sg_buf[bytes_in_list],
991c984
<               memcpy(sg_buf, /* wrong: &sg_buf[bytes_in_list], */
---
>               memcpy(&sg_buf[bytes_in_list],
1008d1000
<       bytes_in_list = 0; /* go to next list */

[-- Attachment #3: Type: text/plain, Size: 148 bytes --]

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

Re: [ECOS] bugs in AT91 Ethernet driver

[I-Yanaslov]

----- Original Message ----- 
From: "Lambrecht Jürgen" <J.Lambrecht@TELEVIC.com>
To: <ecos-discuss@sources.redhat.com>
Sent: Saturday, May 31, 2008 2:23 AM
Subject: [ECOS] bugs in AT91 Ethernet driver



Hello.

>I found some bugs in the AT91 EMAC Ethernet driver - 
>/packages/devs/eth/arm/at91/current/src/if_at91.c.

>This is my version of at91_eth_recv(..):

> ..................
>               memcpy(sg_buf, /* &sg_buf[bytes_in_list], */
>      &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
>      bytes_needed_list);
> ..................
>              memcpy(sg_buf, /* wrong: &sg_buf[bytes_in_list], */
>     &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
>     bytes_in_buffer);
>
>            priv->rbd[priv->curr_rbd_idx].addr &=
>      ~(AT91_EMAC_RBD_ADDR_OWNER_SW);
>            priv->curr_rbd_idx++;
>            if(priv->curr_rbd_idx >= CYGNUM_DEVS_ETH_ARM_AT91_RX_BUFS)
>            {
>               priv->curr_rbd_idx = 0;
>            }
>            buffer_pos = 0;
>         }
>      }
>      bytes_in_list = 0; /* go to next list */
>   }
>}
Don't understand, why "memcpy(&sg_buf[bytes_in_list]," is wrong.
Know, you must iterate sg_buf
before memcpy next RXbuffer.

About a TX driver.
At91sam9260 errata says, TX buffer and buffer descriptors must be allocated 
in fast memory, e.g. internal SRAM.
So, eCos scheme with TX buffers which dinamically allocated from the heap 
(heap is in external SDRAM at 9260EK)
is do not work in interrupt mode.
Ivan Yanaslov.




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

Re: [ECOS] bugs in AT91 Ethernet driver

[Andrew Lunn]

On Sat, May 31, 2008 at 12:23:45AM +0200, Lambrecht J?rgen wrote:
> Hello,
> 
> I found some bugs in the AT91 EMAC Ethernet driver - /packages/devs/eth/arm/at91/current/src/if_at91.c.

> The bugs are present in at91_eth_recv(..) because the author did not
> understand how scatter-gather lists work I think. In Redboot, the
> driver works, and maybe also with LWIP, but not with freebsd.

This is quite possible. The driver was developed on the AT91SAM7X
device. This does not have enough resources to run the FreeBSD
stack.

> With my fix, RX seems to work on my AT91SAM9260-EK based board.

Thanks. I will try to understand what you have changed and see if it
looks correct.
 
> P.S.: in attachment a unix dif; that's not in the correct format for
> the ecos list I think - I need to take the time to look up how
> to....

It is not wrong, it is just harder for a human to read, making my job
harder. I don't just blindly apply patches. I try to understand the
problem and see if the fix is correct. The patch is a way into the
problem, especially when the only comments is, its broken, here is the
fix. 

     Andrew

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

Re: [ECOS] bugs in AT91 Ethernet driver

[Andrew Lunn]

> About a TX driver.
> At91sam9260 errata says, TX buffer and buffer descriptors must be 
> allocated in fast memory, e.g. internal SRAM.
> So, eCos scheme with TX buffers which dinamically allocated from the heap 
> (heap is in external SDRAM at 9260EK)
> is do not work in interrupt mode.

This will be because of the history of the driver. It was developed on
the AT91SAM7X which does not have an external bus. Everything is in
SRAM.

However, you are wrong about the TX buffers being on the heap. The
buffer is inside the at91_eth_priv_t structure. This is allocated at
the end of if_at91.c. It will be placed in the data segment. 

What is needed is some compiler/linker magic to put it into a
different segment which is placed in sram. This has been talked about
a few times on the mailling list.

  Andrew

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

Re: [ECOS] bugs in AT91 Ethernet driver

[Andrew Lunn]

Hi Joergen

To make your patch easier to discuss, here it is again, but slightly
modified.

--- if_at91.c   23 Mar 2007 19:02:09 -0000      1.2
+++ if_at91.c   31 May 2008 11:31:39 -0000
@@ -937,6 +937,7 @@
 
    for(i = 0;i<sg_len;i++)
    {
+      bytes_in_list = 0;
       while(bytes_in_list < sg_list[i].len)
       {
          bytes_needed_list = sg_list[i].len - bytes_in_list;

You have this at the end. I put it at the beginning where it think it
more logically belongs. We have just started a new SG buffer, so we
currently have zero bytes in it. This i agree with.

@@ -945,7 +946,7 @@
          {
              bytes_in_buffer = 
                ((priv->rbd[priv->curr_rbd_idx].sr & AT91_EMAC_RBD_SR_LEN_MASK)
-                - total_bytes) - buffer_pos;
+                - total_bytes);
          }
          else
          {

Your comment is correct. total_bytes already includes buffer_pos, so
including it again results in the driver discarding some bytes from
the end of the packet.

@@ -957,7 +958,7 @@
          if(bytes_needed_list < bytes_in_buffer)
          {
             if(sg_buf != NULL)
-               memcpy(&sg_buf[bytes_in_list],
+               memcpy(&sg_buf,
                      &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
                      bytes_needed_list);
             bytes_in_list += bytes_needed_list;
@@ -967,7 +968,7 @@
          else
          {
             if(sg_buf != NULL)
-              memcpy(&sg_buf[bytes_in_list],
+              memcpy(&sg_buf,
                     &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
                     bytes_in_buffer);
             bytes_in_list += bytes_in_buffer;

These two changes i don't understand. I think they are wrong. It could
be you have not found out this is wrong because of other problems
means you have not got past the small ARP packets. The [bytes_in_list]
is to handle when the bytes left in a receiver buffer are less than
the number of bytes left in the SG buffer. It needs to copy as many
bytes are available from one receive buffer and the move onto the next
receive buffer and copy as many bytes as needed into the sg buffer to
fill it up. Only then will it move onto the next sg buffer.

   Andrew


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

Re: [ECOS] bugs in AT91 Ethernet driver

[I-Yanaslov]

> However, you are wrong about the TX buffers being on the heap. The
> buffer is inside the at91_eth_priv_t structure. This is allocated at
> the end of if_at91.c. It will be placed in the data segment.

at91_eth_priv structure is contains only RX buffers, RX buffer descriptors 
and "TX buffer descriptors". No contains "TX buffers".

at91_eth_send() shows " priv->tbd[priv->curr_tbd_idx].addr = sg_list[i].buf; 
" - no memory copy, only address assigment.
But, sg_list[] it is mbuf, dynamically allocated in TCP/IP stack.

Present at91_eth driver is don't use static TX buffers.

Ivan. 


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

Re: [ECOS] bugs in AT91 Ethernet driver

[Andrew Lunn]

On Sat, May 31, 2008 at 03:49:24PM +0400, I-Yanaslov wrote:
>
>> However, you are wrong about the TX buffers being on the heap. The
>> buffer is inside the at91_eth_priv_t structure. This is allocated at
>> the end of if_at91.c. It will be placed in the data segment.
>
> at91_eth_priv structure is contains only RX buffers, RX buffer 
> descriptors and "TX buffer descriptors". No contains "TX buffers".
>
> at91_eth_send() shows " priv->tbd[priv->curr_tbd_idx].addr = 
> sg_list[i].buf; " - no memory copy, only address assigment.
> But, sg_list[] it is mbuf, dynamically allocated in TCP/IP stack.

Ah, OK. My error. That makes it more messy. 

How big is the SRAM on these ARM9 devices? 32K, 64K? If it is big
enough maybe we can move net_mbufs_area[] into the SRAM. Otherwise a
copy is needed for every packet to be transmitted.

     Andrew

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

Re: [ECOS] bugs in AT91 Ethernet driver

[Lambrecht Jürgen]

Major bug found in TX part - see below.

Andrew Lunn wrote:
>> During debugging, I see that 'priv->tx_busy' stays 'true', because the  
>> TX-Complete bit (COMP of TSR reg) is never set - I also see that the  
>> TX-Used Bit Read (UBR of TSR reg) is set, which is normal I think  
>> because the TX buffers have not yet been released (because COMP is not  
>> set..). But the TX-GO bit is 0, meaning that transmit is not busy.
>>
>> I added code to release the TX buffers and to clear 'priv->tx_busy' when  
>> TX-GO is read 0. Then at each ARP request, there is an ARP reply, but it  
>> does not get out.. (monitoring with Wireshark)
>> What's happening - it is like the EMAC TX is blocked, but why?
>
> It might be worth printing out the value of isr in at91_eth_isr. As
> the comment says, error conditions are not handled. Maybe you are
> getting a buffer under run?
Indeed Andrew, looking at the value of the ISR lead me to the cause of error,
and hence the solution. 
  The TX status of the ISR can be read in AT91_EMAC_TSR. The problem is bit 0
of this register: UBR = "Used Bit Read". 
  The datasheet (DS) is very unclear on this point (both the ATSAM9260 DS and
ATSAM7X DS have completely the same section about their EMAC). But after a full
day debugging, I'm quite sure I'm right.
  So, that Used Bit indicates if the concerning packet buffer (with sg_list[i]
data) has already been used by the EMAC. This Used Bit must be set to 0 by the
user SW, and is set to 1 by the EMAC. In case of the TX EMAC, the Used Bit is
set to 1 by the EMAC after transmit of the concerning buffer.
  The TX EMAC holds a private counter/pointer (read-only in TBQP) to the
transmit buffer descriptor list - the start location is the write-only
TBQP. (Only the RX part is explained in the DS). Normally this counter/pointer
cycles trough the circular transmit buffer descriptor list.
  Now, after transmit of the last buffer (that last buffer has to be marked by
the SW with a EOF flag) the TX EMAC increases its TX counter/pointer to already
point the next TX buffer. But that next TX buffer - that is free of course - is
marked by the SW as "Used" because the Used Bit is set. And therefore the UBR
error is flagged. And therefore the TX EMAC *resets its counter/pointer* !!
Because the SW does not reset its tbd_idx pointer, both pointers are out of
sync so all further transmits are blocked. 
  The major bug is that the if_at91.c SW rigorously sets the Used Bit (in the
buffer descriptor) of all free buffers to 1, meaning "Used". A the end of the
EMAC DS (36.4.1.3 for SAM9260 vG and 37.4.1.3 for SAM7 vG point 2): "Mark all
entries in this list as owned by EMAC, i.e. bit 31 of word 1 set to 0."
Therefore I changed at91_tb_init() and at91_reset_tbd() accordingly.
  Solving the "Used Bit" issue is not enough. All the TX errors UBR, RLE, BEX
and UND cause the counter/pointer to be reset. Therefore I changed the
deliver() function and added an extra argument to at91_reset_tbd() to reset the
SW tbd_idx pointer. I also added those TX errors to the IRQ IER and ISR.

In attachment a diff - it also contains my previous changes. I also added some
comments, and added '#ifdef CYGINT_IO_ETH_INT_SUPPORT_REQUIRED' to the start
and stop functions. And I added more debug printing - even in the ISR..
I tested this fix during 5 minutes - long enough to see that a ping finally
works... 
  There are of course still some problems - the ping reply is not synchronized
with the ping request: it runs 1 ping iteration behind - more of it in my next
mail. 

Kind regards,
Juergen

P.S.: The current code can only work if the driver is initialized after each
TX, or because each tx uses all buffers so that the pointer is wrapped.
I guess redboot must work this way??
Or am I still missing something?

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

Re: [ECOS] bugs in AT91 Ethernet driver

[Andrew Lunn]

> During debugging, I see that 'priv->tx_busy' stays 'true', because the  
> TX-Complete bit (COMP of TSR reg) is never set - I also see that the  
> TX-Used Bit Read (UBR of TSR reg) is set, which is normal I think  
> because the TX buffers have not yet been released (because COMP is not  
> set..). But the TX-GO bit is 0, meaning that transmit is not busy.
>
> I added code to release the TX buffers and to clear 'priv->tx_busy' when  
> TX-GO is read 0. Then at each ARP request, there is an ARP reply, but it  
> does not get out.. (monitoring with Wireshark)
> What's happening - it is like the EMAC TX is blocked, but why?

It might be worth printing out the value of isr in at91_eth_isr. As
the comment says, error conditions are not handled. Maybe you are
getting a buffer under run?

        Andrew

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

Re: [ECOS] bugs in AT91 Ethernet driver

[Jürgen Lambrecht]

Lambrecht Jürgen wrote:
>> -----Original Message-----
>> From: ecos-discuss-owner@ecos.sourceware.org [mailto:ecos-discuss-
>> owner@ecos.sourceware.org] On Behalf Of Lambrecht Jürgen
>> Sent: zondag 1 juni 2008 1:22
>> To: Andrew Lunn; I-Yanaslov
>> Cc: ecos-discuss@sources.redhat.com
>> Subject: RE: [ECOS] bugs in AT91 Ethernet driver
>>
>>     
...
>> The AT91SAM9620 has only 2 SRAMs of 4kB each.
>> (The AT91SAM9621 has 160kB of SRAM, but no EMAC and LCD controller
>> instead)
>> So it will have to be the last option.
>> To have a reliable driver, the fix is needed, but that (errata) is not
>> why my TX does not work.
>> The AT91SAM9620 errata is only valid at heavy network load (when there
>> is RX and TX at the same time and then an SDRAM refresh or SDRAM bank
>> opening or so). I only do a ping on a private network for test.
>> And moreover, the Ethernet driver works in Redboot RAM (ok, not that
>> well, 9% of the pings get lost).
>>
>> I will further debug tomorrow...
>>     
> In attachment ' if_at91_jtagram_ecos1.01.06_sg1TXsram1.txt ' with a log.
> It was compiled with if_at91.c patched as below.
> You can see that at startup a gratuitous ARP is sent out, and I receive the packet on my laptop with Wireshark.
> The next TX (ARP reply as reply on RX of ARP request) is sent out by the TCP/IP stack, but does not leave the board as I don't see it in Wireshark.
> It gets stuck somewhere. Maybe the driver is waiting on something, or maybe the TX DMA is blocked??
>
>   
During debugging, I see that 'priv->tx_busy' stays 'true', because the 
TX-Complete bit (COMP of TSR reg) is never set - I also see that the 
TX-Used Bit Read (UBR of TSR reg) is set, which is normal I think 
because the TX buffers have not yet been released (because COMP is not 
set..). But the TX-GO bit is 0, meaning that transmit is not busy.

I added code to release the TX buffers and to clear 'priv->tx_busy' when 
TX-GO is read 0. Then at each ARP request, there is an ARP reply, but it 
does not get out.. (monitoring with Wireshark)
What's happening - it is like the EMAC TX is blocked, but why?

Regards,
Juergen



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

RE: [ECOS] bugs in AT91 Ethernet driver

[Lambrecht Jürgen]

[-- Attachment #1: Type: text/plain, Size: 3365 bytes --]

> -----Original Message-----
> From: ecos-discuss-owner@ecos.sourceware.org [mailto:ecos-discuss-
> owner@ecos.sourceware.org] On Behalf Of Lambrecht Jürgen
> Sent: zondag 1 juni 2008 1:22
> To: Andrew Lunn; I-Yanaslov
> Cc: ecos-discuss@sources.redhat.com
> Subject: RE: [ECOS] bugs in AT91 Ethernet driver
> 
> Thanks for your replies.
> 
> > -----Original Message-----
> > From: ecos-discuss-owner@ecos.sourceware.org [mailto:ecos-discuss-
> > owner@ecos.sourceware.org] On Behalf Of Andrew Lunn
> > Sent: zaterdag 31 mei 2008 14:04
> > To: I-Yanaslov
> > Cc: Andrew Lunn; ecos-discuss@sources.redhat.com
> > Subject: Re: [ECOS] bugs in AT91 Ethernet driver
> >
> > On Sat, May 31, 2008 at 03:49:24PM +0400, I-Yanaslov wrote:
> > >
> > >> However, you are wrong about the TX buffers being on the heap. The
> > >> buffer is inside the at91_eth_priv_t structure. This is allocated
> at
> > >> the end of if_at91.c. It will be placed in the data segment.
> > >
> > > at91_eth_priv structure is contains only RX buffers, RX buffer
> > > descriptors and "TX buffer descriptors". No contains "TX buffers".
> > >
> > > at91_eth_send() shows " priv->tbd[priv->curr_tbd_idx].addr =
> > > sg_list[i].buf; " - no memory copy, only address assigment.
> > > But, sg_list[] it is mbuf, dynamically allocated in TCP/IP stack.
> >
> > Ah, OK. My error. That makes it more messy.
> >
> > How big is the SRAM on these ARM9 devices? 32K, 64K? If it is big
> > enough maybe we can move net_mbufs_area[] into the SRAM. Otherwise a
> > copy is needed for every packet to be transmitted.
> >
> The AT91SAM9620 has only 2 SRAMs of 4kB each.
> (The AT91SAM9621 has 160kB of SRAM, but no EMAC and LCD controller
> instead)
> So it will have to be the last option.
> To have a reliable driver, the fix is needed, but that (errata) is not
> why my TX does not work.
> The AT91SAM9620 errata is only valid at heavy network load (when there
> is RX and TX at the same time and then an SDRAM refresh or SDRAM bank
> opening or so). I only do a ping on a private network for test.
> And moreover, the Ethernet driver works in Redboot RAM (ok, not that
> well, 9% of the pings get lost).
> 
> I will further debug tomorrow...
In attachment ' if_at91_jtagram_ecos1.01.06_sg1TXsram1.txt ' with a log.
It was compiled with if_at91.c patched as below.
You can see that at startup a gratuitous ARP is sent out, and I receive the packet on my laptop with Wireshark.
The next TX (ARP reply as reply on RX of ARP request) is sent out by the TCP/IP stack, but does not leave the board as I don't see it in Wireshark.
It gets stuck somewhere. Maybe the driver is waiting on something, or maybe the TX DMA is blocked??

> And then copy every packet to SRAM before doing DMA...
> 
In attachment my patch.
It contains
- in at91_eth_send(): modification (#ifdef SRAM1_ORIGIN) to copy sg_list[i].buf first to internal SRAM, and only then do a DMA to the EMAC
- in at91_eth_recv(): bugfix described in previous mail
- general: I don't have a PHY attached but a switch. The MII is always up at 100Mbps, so no need to use SMI to read the link status in the phy registers. So put '#ifdef PHY_PRESENT' around those parts of the code

I tested it with my AT91SAM9620-EK based board. And it works just as before (see above).

Kind regards,
Juergen

[-- Attachment #2: if_at91_jtagram_ecos1.01.06_sg1TXsram1.txt --]
[-- Type: text/plain, Size: 4259 bytes --]

«¬}¹•Ñ}¥¹¥ÑuInit: mbinit(0x00000000)
[cyg_net_init] Init: cyg_net_init_devs(0x00000000)
Init device 'at91'
[cyg_net_init] Init: loopattach(0x00000000)
[cyg_net_init] Init: ifinit(0x00000000)
[cyg_net_init] Init: domaininit(0x00000000)
[cyg_net_init] Init: cyg_net_add_domain(0x200a71f8)
New domain internet at 0x00000000
[cyg_net_init] Init: cyg_net_add_domain(0x200a6ba8)
New domain route at 0x00000000
[cyg_net_init] Init: call_route_init(0x00000000)
[cyg_net_init] Done


Televic Echo-Test Application
Juergen Lambrecht, 1.09.11, built 21:45:42, Jun  1 2008

BOOTP[eth0] op: REPLY
       htype: Ethernet
        hlen: 6
        hops: 0
         xid: 0x0
        secs: 0
       flags: 0x0
       hw_addr: 00:0e:3d:01:03:00
     client IP: 10.0.54.249
         my IP: 10.0.54.249
     server IP: 10.0.56.4
    gateway IP: 10.0.127.254
  options:
        subnet mask: 255.255.0.0
       IP broadcast: 10.0.255.255
            gateway: 10.0.127.254
Sending 42 bytes
xmit 42 bytes at 0x200fd856 sg[1]                           //JL: I see this packet in Wireshark
200FD856: FF FF FF FF FF FF 00 0E  3D 01 03 00 08 06 00 01  |........=.......|
200FD866: 08 00 06 04 00 01 00 0E  3D 01 03 00 0A 00 36 F9  |........=.....6.|
200FD876: 00 00 00 00 00 00 0A 00  36 F9                    |........6.      |
at91_eth_control.702: key 110
[eth_drv_ioctl] Warning: Driver can't set multi-cast mode
at91_eth_control.702: key 110
[eth_drv_ioctl] Warning: Driver can't set multi-cast mode
at91_eth_control.702: key 110
[eth_drv_ioctl] Warning: Driver can't set multi-cast mode
End of initializations
rx 14 bytes at 20160afc sg[0]
20160AFC: FF FF FF FF FF FF 00 1D  09 B4 CA 6A 08 06        |...........j..  |
rx 50 bytes at 200fd82c sg[1]
200FD82C: 00 01 08 00 06 04 00 01  00 1D 09 B4 CA 6A 0A 00  |.............j..|
200FD83C: 38 31 00 00 00 00 00 00  0A 00 36 F9 00 00 00 00  |81........6.....|
200FD84C: 00 00 00 00 00 00 00 00  00 00 00 00 00 00 DE 09  |................|
200FD85C: EC 66                                             |.f              |
Sending 42 bytes
xmit 42 bytes at 0x200fd7d6 sg[1]                           //JL: I DONT see this packet in Wireshark??
200FD7D6: FF FF FF FF FF FF 00 0E  3D 01 03 00 08 06 00 01  |........=.......|
200FD7E6: 08 00 06 04 00 01 00 0E  3D 01 03 00 0A 00 36 F9  |........=.....6.|
200FD7F6: 00 00 00 00 00 00 0A 00  36 F9                    |........6.      |
rx 14 bytes at 20160afc sg[0]
20160AFC: FF FF FF FF FF FF 00 1D  09 B4 CA 6A 08 06        |...........j..  |
rx 50 bytes at 200fd6ac sg[1]
200FD6AC: 00 01 08 00 06 04 00 01  00 1D 09 B4 CA 6A 0A 00  |.............j..|
200FD6BC: 38 31 00 00 00 00 00 00  0A 00 36 F9 00 00 00 00  |81........6.....|
200FD6CC: 00 00 00 00 00 00 00 00  00 00 00 00 00 00 DE 09  |................|
200FD6DC: EC 66                                             |.f              |
.14 bytes at 20160afc sg[0]
20160AFC: FF FF FF FF FF FF 00 1D  09 B4 CA 6A 08 00        |...........j..  |
rx 244 bytes at 2011e7c8 sg[1]
2011E7C8: 45 00 00 F0 B3 BE 00 00  80 11 3A 0E 0A 00 38 31  |E.........:...81|
2011E7D8: 0A 00 FF FF 00 8A 00 8A  00 DC D3 B8 11 0E 81 14  |................|
2011E7E8: 0A 00 38 31 00 8A 00 C6  00 00 20 46 45 45 4D 46  |..81...... FEEMF|
2011E7F8: 47 43 4E 46 45 46 45 46  44 43 4E 45 4B 45 4D 44  |GCNFEFEFDCNEKEMD|
2011E808: 42 43 41 43 41 43 41 43  41 41 41 00 20 41 42 41  |BCACACACAAA. ABA|
2011E818: 43 46 50 46 50 45 4E 46  44 45 43 46 43 45 50 46  |CFPFPENFDECFCEPF|
2011E828: 48 46 44 45 46 46 50 46  50 41 43 41 42 00 FF 53  |HFDEFFPFPACAB..S|
2011E838: 4D 42 25 00 00 00 00 00  00 00 00 00 00 00 00 00  |MB%.............|
2011E848: 00 00 00 00 00 00 00 00  00 00 00 00 00 00 11 00  |................|
2011E858: 00 2C 00 00 00 00 00 00  00 00 00 E8 03 00 00 00  |.,..............|
2011E868: 00 00 00 00 00 2C 00 56  00 03 00 01 00 01 00 02  |.....,.V........|
2011E878: 00 3D 00 5C 4D 41 49 4C  53 4C 4F 54 5C 42 52 4F  |.=.\MAILSLOT\BRO|
2011E888: 57 53 45 00 0C 00 E0 93  04 00 54 45 4C 45 56 49  |WSE.......TELEVI|
2011E898: 43 00 00 00 C8 35 11 00  00 10 03 0A 00 10 00 80  |C....5..........|
2011E8A8: 00 10 F8 7F 54 4C 56 2D  54 54 53 2D 4A 4C 31 00  |....TLV-TTS-JL1.|
2011E8B8: B9 81 0D 08                                       |....            |

[-- Attachment #3: if_at91_udiff.txt --]
[-- Type: text/plain, Size: 6451 bytes --]

--- F:\version_ecos\packages\devs\eth\arm\at91\current\src\if_at91.c	2008-03-07 08:35:36.000000000 +-0200
+++ F:\ecos_ronetix\packages\devs\eth\arm\at91\current\src\if_at91.c	2008-06-01 23:09:54.000000000 +-0200
@@ -64,13 +66,15 @@
 #include <cyg/infra/cyg_type.h>
 #include <cyg/infra/cyg_ass.h>
 #include <cyg/infra/diag.h>
 #include <cyg/io/eth/netdev.h>
 #include <cyg/io/eth/eth_drv.h>
 #include <cyg/io/eth/eth_drv_stats.h>
+#ifdef PHY_PRESENT
 #include <cyg/io/eth_phy.h>
+#endif
 #include <errno.h>
 #include <string.h>
 
 // Set up the level of debug output
 #if CYGPKG_DEVS_ETH_ARM_AT91_DEBUG_LEVEL > 0
    #define debug1_printf(args...) diag_printf(args)
@@ -165,13 +169,15 @@
 typedef struct at91_eth_priv_s 
 {
    cyg_uint32    intr_vector;
    char *esa_key;      // RedBoot 'key' for device ESA
    cyg_uint8 *enaddr;
    cyg_uint32 base;    // Base address of device
+#ifdef PHY_PRESENT
    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];
    unsigned long curr_tx_key;
    cyg_bool tx_busy;
    cyg_uint32 last_tbd_idx;
@@ -298,18 +304,19 @@
       CYG_FAIL("Unable to program MII clock");
    }
 
    HAL_WRITE_UINT32(AT91_EMAC + AT91_EMAC_NCFG, cfg);
 }
 
-
+#ifdef PHY_PRESENT
 ETH_PHY_REG_LEVEL_ACCESS_FUNS(at91_phy, 
                               at91_init_phy,
                               NULL,
                               at91_write_phy,
                               at91_read_phy);
-
+#endif
 //======================================================================
 // Receiver buffer handling
 
 // Initialize the receiver buffers and descriptors
 static void
 at91_rb_init(at91_eth_priv_t *priv)
@@ -482,13 +489,15 @@
 {
    struct eth_drv_sc *sc = (struct eth_drv_sc *)tab->device_instance;
    at91_eth_priv_t *priv = (at91_eth_priv_t *)sc->driver_private;
    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 PHY_PRESENT
    unsigned short phy_state = 0;
+#endif
    cyg_uint32 ncfg = 0;
 
    debug1_printf("\nAT91_ETH: Initialising @ %x\n",priv->base);
 
    priv->tx_busy = false;
    priv->curr_tbd_idx = 0;
@@ -557,57 +566,64 @@
    // Setup the transmit descriptors
    at91_tb_init(priv);
 
    // And tell the EMAC where the first transmit buffer descriptor is
    HAL_WRITE_UINT32(priv->base + AT91_EMAC_TBQP, (cyg_uint32)priv->tbd);
 
+#ifdef PHY_PRESENT
    // Setup the PHY
    CYG_ASSERTC(priv->phy);
 
    at91_mdio_enable();
    if (!_eth_phy_init(priv->phy))
    {
       at91_mdio_disable();
+      debug2_printf("_eth_phy_init failed\n");
       return (false);
    }
 
    // Get the current mode and print it
    phy_state = _eth_phy_state(priv->phy);
+#endif
    at91_mdio_disable();
 
    HAL_READ_UINT32(priv->base + AT91_EMAC_NCFG,ncfg);
 
-
+#ifdef PHY_PRESENT
    if ((phy_state & ETH_PHY_STAT_LINK) != 0)
    {
       if (((phy_state & ETH_PHY_STAT_100MB) != 0))
       {
          debug1_printf("AT91_ETH: 100Mbyte/s");
+#endif
          ncfg |= AT91_EMAC_NCFG_SPD_100Mbps;
+#ifdef PHY_PRESENT
       }
       else
       {
          debug1_printf("AT91_ETH: 10Mbyte/s");
          ncfg &= ~(AT91_EMAC_NCFG_SPD_100Mbps);
       }
       if((phy_state & ETH_PHY_STAT_FDX))
       {
          debug1_printf(" Full Duplex\n");
+#endif
          ncfg |= AT91_EMAC_NCFG_FD;
+#ifdef PHY_PRESENT
       }
       else
       {
          debug1_printf(" Half Duplex\n");
          ncfg &= ~(AT91_EMAC_NCFG_FD);
       }
    }
    else
    {
       debug1_printf("AT91_ETH: No Link\n");
    }
-
+#endif
 
    //Setup the network configuration
    ncfg |= (AT91_EMAC_NCFG_RLCE);
 
    HAL_WRITE_UINT32(priv->base + AT91_EMAC_NCFG,ncfg);
 
@@ -721,20 +737,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;
-
+#ifdef SRAM1_ORIGIN /* define it in plf_io.h if present */
+   cyg_uint32 total_bytes = 0; /* position in SRAM1 */
+#endif
    priv->tx_busy = true;
 
    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,12 +770,15 @@
       }
       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;
 
@@ -936,19 +962,22 @@
    cyg_uint32 total_bytes = 0;
 
 
 
    for(i = 0;i<sg_len;i++)
    {
+      bytes_in_list = 0; /* go to next list */
       while(bytes_in_list < sg_list[i].len)
       {
          bytes_needed_list = sg_list[i].len - bytes_in_list;
 
          if(priv->rbd[priv->curr_rbd_idx].sr & AT91_EMAC_RBD_SR_EOF)
          {
 	      bytes_in_buffer = 
 		((priv->rbd[priv->curr_rbd_idx].sr & AT91_EMAC_RBD_SR_LEN_MASK)
-		 - total_bytes) - buffer_pos;
+		 - total_bytes);
          }
          else
          {
             bytes_in_buffer = AT91_EMAC_RX_BUFF_SIZE - buffer_pos;
          }
 
@@ -1001,14 +1030,17 @@
    return(CYGNUM_HAL_INTERRUPT_EMAC);
 }
 
 at91_eth_priv_t at91_priv_data =
 {
    .intr_vector = CYGNUM_HAL_INTERRUPT_EMAC,
-   .base = AT91_EMAC,
+   .base = AT91_EMAC
+#ifdef PHY_PRESENT
+   ,
    .phy = &at91_phy
+#endif
 };
 
 ETH_DRV_SC(at91_sc,
            &at91_priv_data,       // Driver specific data
            "eth0",                // Name for this interface
            at91_eth_start,

[-- Attachment #4: Type: text/plain, Size: 148 bytes --]

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

RE: [ECOS] bugs in AT91 Ethernet driver

[Lambrecht Jürgen]

Thanks for your replies.

> -----Original Message-----
> From: ecos-discuss-owner@ecos.sourceware.org [mailto:ecos-discuss-
> owner@ecos.sourceware.org] On Behalf Of Andrew Lunn
> Sent: zaterdag 31 mei 2008 14:04
> To: I-Yanaslov
> Cc: Andrew Lunn; ecos-discuss@sources.redhat.com
> Subject: Re: [ECOS] bugs in AT91 Ethernet driver
> 
> On Sat, May 31, 2008 at 03:49:24PM +0400, I-Yanaslov wrote:
> >
> >> However, you are wrong about the TX buffers being on the heap. The
> >> buffer is inside the at91_eth_priv_t structure. This is allocated at
> >> the end of if_at91.c. It will be placed in the data segment.
> >
> > at91_eth_priv structure is contains only RX buffers, RX buffer
> > descriptors and "TX buffer descriptors". No contains "TX buffers".
> >
> > at91_eth_send() shows " priv->tbd[priv->curr_tbd_idx].addr =
> > sg_list[i].buf; " - no memory copy, only address assigment.
> > But, sg_list[] it is mbuf, dynamically allocated in TCP/IP stack.
> 
> Ah, OK. My error. That makes it more messy.
> 
> How big is the SRAM on these ARM9 devices? 32K, 64K? If it is big
> enough maybe we can move net_mbufs_area[] into the SRAM. Otherwise a
> copy is needed for every packet to be transmitted.
>
The AT91SAM9620 has only 2 SRAMs of 4kB each.
(The AT91SAM9621 has 160kB of SRAM, but no EMAC and LCD controller instead)
So it will have to be the last option.
To have a reliable driver, the fix is needed, but that (errata) is not why my TX does not work.
The AT91SAM9620 errata is only valid at heavy network load (when there is RX and TX at the same time and then an SDRAM refresh or SDRAM bank opening or so). I only do a ping on a private network for test.
And moreover, the Ethernet driver works in Redboot RAM (ok, not that well, 9% of the pings get lost).

I will further debug tomorrow...
And then copy every packet to SRAM before doing DMA...

Kind regards,
Jürgen
 
>      Andrew
> 
> --
> Before posting, please read the FAQ:
> http://ecos.sourceware.org/fom/ecos
> and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss


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

RE: [ECOS] bugs in AT91 Ethernet driver

[Lambrecht Jürgen]

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: zaterdag 31 mei 2008 13:44
> To: Lambrecht Jürgen
> Cc: ecos-discuss@sources.redhat.com
> Subject: Re: [ECOS] bugs in AT91 Ethernet driver
> 
> Hi Joergen
(;-), my name is written with an 'u' with an umlaut (2 dots) on the u - and that umlaut-u gets screwed up often.. That's why I often type my name as Juergen because it is a German rule to put an 'e' after a letter that should have an umlaut but when you can/want not put one.)
> 
> To make your patch easier to discuss, here it is again, but slightly
> modified.
> 
> --- if_at91.c   23 Mar 2007 19:02:09 -0000      1.2
> +++ if_at91.c   31 May 2008 11:31:39 -0000
> @@ -937,6 +937,7 @@
> 
>     for(i = 0;i<sg_len;i++)
>     {
> +      bytes_in_list = 0;
>        while(bytes_in_list < sg_list[i].len)
>        {
>           bytes_needed_list = sg_list[i].len - bytes_in_list;
> 
> You have this at the end. I put it at the beginning where it think it
> more logically belongs. We have just started a new SG buffer, so we
> currently have zero bytes in it. This i agree with.
> 
indeed, putting it at the beginning is more clear.

> @@ -945,7 +946,7 @@
>           {
>               bytes_in_buffer =
>                 ((priv->rbd[priv->curr_rbd_idx].sr &
> AT91_EMAC_RBD_SR_LEN_MASK)
> -                - total_bytes) - buffer_pos;
> +                - total_bytes);
>           }
>           else
>           {
> 
> Your comment is correct. total_bytes already includes buffer_pos, so
> including it again results in the driver discarding some bytes from
> the end of the packet.
> 
> @@ -957,7 +958,7 @@
>           if(bytes_needed_list < bytes_in_buffer)
>           {
>              if(sg_buf != NULL)
> -               memcpy(&sg_buf[bytes_in_list],
> +               memcpy(&sg_buf,
>                       &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
>                       bytes_needed_list);
>              bytes_in_list += bytes_needed_list;
> @@ -967,7 +968,7 @@
>           else
>           {
>              if(sg_buf != NULL)
> -              memcpy(&sg_buf[bytes_in_list],
> +              memcpy(&sg_buf,
>                      &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
>                      bytes_in_buffer);
>              bytes_in_list += bytes_in_buffer;
> 
> These two changes i don't understand. I think they are wrong. It could
> be you have not found out this is wrong because of other problems
indeed
Because 'bytes_in_list' was not reset, I thought this was an error. Only afterwards I found the 'bytes_in_list' error.

> means you have not got past the small ARP packets. The [bytes_in_list]
indeed, not got past the small ARP packets.

> is to handle when the bytes left in a receiver buffer are less than
> the number of bytes left in the SG buffer. It needs to copy as many
> bytes are available from one receive buffer and the move onto the next
> receive buffer and copy as many bytes as needed into the sg buffer to
> fill it up. Only then will it move onto the next sg buffer.
> 
indeed
I knew this already, but I have made the error-i-thouht-it-was myself when writing my own fpga-memory-mapped Ethernet driver a while ago.
Then I thought sg_list was only 1 buffer and the 'i' was to iterate over the bytes.... Now I know better.

>    Andrew

Thanks Andrew.
This item is closed for the RX part.
For the TX part, see my other reply.

Kind regards,
Juergen

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