Fix potential stall with Coldfire Ethernet

Fix potential stall with Coldfire Ethernet

[Tarmo Kuuse]

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

Hi,

While struggling with eCos on Coldfire 5208 I stumbled on a nasty bug. 
The fast ethernet controller often stalls if transmission is flagged 
immediately after copying data to buffer.

I have no idea why this happens. Maybe my compiler (from CodeSourcery, 
not eCos) does something to confuse the FEC. Maybe the 5208 hardware is 
buggy.

Hopefully the currently supported devices (5282 and 532x) are safe when 
built with eCos toolchain (I cannot test it). The patch could save a few 
neurons for someone.

Patch adds a NOP between filling the buffer and flagging the transmitter.

There is also a small change on how TX buffer descriptors are allocated 
- a loop is used instead of two individual statements. It was quite 
annoying having to debug after changing the BD count.

--
Kind regards,
Tarmo Kuuse

[-- Attachment #2: if_mcf5272_fix_stall.patch --]
[-- Type: text/x-patch, Size: 3076 bytes --]

Index: devs/eth/m68k/mcf5272/current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/eth/m68k/mcf5272/current/ChangeLog,v
retrieving revision 1.3
diff -u -5 -p -r1.3 ChangeLog
--- devs/eth/m68k/mcf5272/current/ChangeLog	29 Jan 2009 17:48:09 -0000	1.3
+++ devs/eth/m68k/mcf5272/current/ChangeLog	3 Mar 2009 10:05:48 -0000
@@ -1,5 +1,10 @@
+2009-03-03  Tarmo Kuuse  <tarmo.kuuse@mail.ee>
+
+	* src/if_mcf5272.c: Fix potential stall on transmission; TX buffer
+	descriptors are initialized in loop
+
 2008-11-17  Bart Veer  <bartv@ecoscentric.com>
 
 	* cdl/mcf5272.cdl, doc/mcf5272_eth.sgml, src/if_mcf5272.c: minor
 	clean-ups.
 
Index: devs/eth/m68k/mcf5272/current/src/if_mcf5272.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/eth/m68k/mcf5272/current/src/if_mcf5272.c,v
retrieving revision 1.2
diff -u -5 -p -r1.2 if_mcf5272.c
--- devs/eth/m68k/mcf5272/current/src/if_mcf5272.c	29 Jan 2009 17:48:09 -0000	1.2
+++ devs/eth/m68k/mcf5272/current/src/if_mcf5272.c	3 Mar 2009 10:05:49 -0000
@@ -288,10 +288,16 @@ mcfxxxx_eth_ramfn(void)
 // This #elif should probably become a #else
 #elif defined(CYGPKG_HAL_M68K_MCF5282) || defined(CYGPKG_HAL_M68K_MCF532x)
 
 # define TX_NEEDS_ALIGNMENT             1
 # define TX_BUFFER_SIZE                 1520
+// It's currently safe to use only 2 TX BD-s since 
+// one BD is declared free at any given time. 
+// Freescale however recommends having at least 3 TX 
+// buffers (see MCF5208 Reference Manual, Rev. 2 or newer,
+// section 19.5.7.1 "Duplicate Frame Transmission").
+// Note: Don't use an odd number.
 # define TX_BUFFER_DESCRIPTOR_COUNT     2
 
 #else
 
 # error Current processor unsupported
@@ -520,12 +526,13 @@ mcfxxxx_eth_init(struct cyg_netdevtab_en
     // Ditto for the tx buffers, if tx involves copying into an
     // aligned buffer. Only one entry in the ring buffer will be used
     // at a time, so the two buffer descriptors can share the same
     // buffer.
 #ifdef TX_NEEDS_ALIGNMENT
-    eth->txbds[0].ethbd_buffer  = txbuffer;
-    eth->txbds[1].ethbd_buffer  = txbuffer;
+	for (i = 0; i < TX_BUFFER_DESCRIPTOR_COUNT; i++) {
+		eth->txbds[i].ethbd_buffer = txbuffer;
+	}
 #endif
     
     // rx_next_buffer tracks the next rx buffer descriptor which the
     // hardware may process.
     eth->rx_next_buffer     = 0;
@@ -737,10 +744,15 @@ mcfxxxx_eth_send(struct eth_drv_sc* sc,
     // for any bits changed by the hardware except on the xxxx which
     // does not have a data cache at all.
 #ifdef HAL_DCACHE_STORE    
     HAL_DCACHE_STORE(&(eth->txbds[0]), TX_BUFFER_DESCRIPTOR_COUNT * sizeof(hal_mcfxxxx_eth_buffer_descriptor));
 #endif    
+
+	// A NOP is needed before flagging the transmitter.
+	// Otherwise the FEC could stall.
+	asm volatile ("nop");
+
     WRITE32(TDAR(eth), HAL_MCFxxxx_ETHx_TDAR_X_DES_ACTIVE);
 }
 
 // The TX interrupt should only trigger when a whole frame has been
 // transmitted. There is no need to worry about nested tx interrupts,

Re: Fix potential stall with Coldfire Ethernet

[Bart Veer]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2502 bytes --]

>>>>> "Tarmo" == Tarmo Kuuse <tarmo.kuuse@mail.ee> writes:

    Tarmo> While struggling with eCos on Coldfire 5208 I stumbled on a
    Tarmo> nasty bug. The fast ethernet controller often stalls if
    Tarmo> transmission is flagged immediately after copying data to
    Tarmo> buffer.

    Tarmo> I have no idea why this happens. Maybe my compiler (from
    Tarmo> CodeSourcery, not eCos) does something to confuse the FEC.
    Tarmo> Maybe the 5208 hardware is buggy.

    Tarmo> Hopefully the currently supported devices (5282 and 532x)
    Tarmo> are safe when built with eCos toolchain (I cannot test it).
    Tarmo> The patch could save a few neurons for someone.

    Tarmo> Patch adds a NOP between filling the buffer and flagging
    Tarmo> the transmitter.

This is not the right fix, and the problem is nothing to do with the
toolchain.

FreeScale have redefined the nop instruction for ColdFires to act as a
barrier instruction. When the cpu executes nop it stalls the
instruction pipeline until all previous instructions have completed.
Also, if the processor has some kind of write buffer between the cache
and external memory then nop will stall until the write buffer has
been emptied. Most of the time you do not need to worry about pipeline
effects like this, but DMA engines and cache interactions nearly
always need very special care.

The operation immediately before starting the transmit is a
HAL_DCACHE_STORE(). Without the nop that macro may not have finished
executing, so the cache store is still happening at the point that the
ethernet engine fetches the buffer descriptors. Hence the ethernet
engine may see bogus buffer descriptors, and confusion results.

The correct solution is to incorporate the nop into your processor's
HAL_DCACHE_STORE() macro, thus making the macro do precisely what it
is supposed to do. That will fix any other uses of HAL_DCACHE_STORE(),
not just the ethernet driver. It also avoids adding an unnecessary nop
instruction on processors which do not need it, e.g. the mcf5272 which
does not have a data cache at all.

Bart

-- 
Bart Veer                                   eCos Configuration Architect
eCosCentric Limited    The eCos experts      http://www.ecoscentric.com/
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
Besuchen Sie uns vom 3.-5.03.09 auf der Embedded World 2009, Stand 11-300
Visit us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300

Re: Fix potential stall with Coldfire Ethernet

[Tarmo Kuuse]

Bart Veer wrote:
> This is not the right fix, and the problem is nothing to do with the
> toolchain.
> 
> FreeScale have redefined the nop instruction for ColdFires to act as a
> barrier instruction. When the cpu executes nop it stalls the
> instruction pipeline until all previous instructions have completed.
> Also, if the processor has some kind of write buffer between the cache
> and external memory then nop will stall until the write buffer has
> been emptied. Most of the time you do not need to worry about pipeline
> effects like this, but DMA engines and cache interactions nearly
> always need very special care.
> 
> The operation immediately before starting the transmit is a
> HAL_DCACHE_STORE(). Without the nop that macro may not have finished
> executing, so the cache store is still happening at the point that the
> ethernet engine fetches the buffer descriptors. Hence the ethernet
> engine may see bogus buffer descriptors, and confusion results.
> 
> The correct solution is to incorporate the nop into your processor's
> HAL_DCACHE_STORE() macro, thus making the macro do precisely what it
> is supposed to do. That will fix any other uses of HAL_DCACHE_STORE(),
> not just the ethernet driver. It also avoids adding an unnecessary nop
> instruction on processors which do not need it, e.g. the mcf5272 which
> does not have a data cache at all.

You are 100% correct about cache/DMA interactions. The MCF5208 Device 
Errdata document admits that the micro's cache implementation is buggy 
and each cache clear operation must be followed by NOP. Also, all cache 
synchronisation essentially boils down to cache clear because there are 
no advanced operations.

I was about to reply that we know about this and have a NOP where needed 
but upon looking closer it turns out HAL_DCACHE_STORE() is not 
implemented at all. Ahh - the pride, thrill and exitement of finding 
totally stupid errors that have consumed hours and days of your life.

It's not all that simple, though. Stalling occurs also when cache is 
disabled. I'll do some more reserarch on this issue next week.

--
Kind regards,
Tarmo Kuuse