[ECOS] Re: at91 emac ethernet driver lwip diff patch

Wed Jun 10 12:12:00 GMT 2015

Hi eCos list and hi Oliver,

sorry for my late reaction..
I finally reviewed your (Oliver's) changes to my version of the 
if_at91.c driver.
And I think your changes are wrong.
I did not look at the other patches - I do not use LwIP.
For reference I added my source file in attach (it is still on my todo 
list to make it ready for commit..) and Oliver's version (cleaned up a 
bit for easier diff).

Below you find your email with my remarks, and at the end some other 
remarks.

On 23/03/15 12:16, oliver.munz wrote:
> Dear Jürgen
>
> I had problems using the driver whit lwIP. After some debuging i think
> that the driver does two things wrong:
> 1. he adds AT91_EMAC_RX_BUFF_SIZE on line 1260 if the AT91_EMAC_RBD_SR_EOF
> isn't set. But it should do it if the size is 0.
No, the '- buffer_pos' is already contained in 'total_bytes'. Else you 
count it double. Do you agree?
> 2. he dose not calculate the size the same way in at91_eth_recv() and
> at91_eth_deliver().
Yes he does.
Mark that AT91_EMAC_RBD_SR_LEN_MASK contains the length of the complete 
packet. The EMAC hardware puts that packet in 128B buffers, and only the 
last buffer can not be completely full. So for at91_eth_recv() I can 
read those 128B buffers completely, except the last buffer. But the only 
way to know how much bytes are put in that last buffer is to use the 
complete length (T91_EMAC_RBD_SR_LEN_MASK), so the number of bytes in 
the last buffer is (the-packet-length minus the-already-received-bytes).
In case of at91_eth_deliver() I am only interested in the complete size, 
no need to count it separately as sums of 128B buffers 
(AT91_EMAC_RX_BUFF_SIZE).
So I think your code has an error, because you sum all buffers and then 
finally add the complete packet length, so you have almost the double 
length?
> at91_eth_deliver() dont care about the buffers whit the
> size marked as 0.
Indeed I don't check if the AT91_EMAC_RBD_SR_LEN_MASK is valid, because 
I know (from the AT91SAM9260 datasheet - although not very clear: "If 
end of frame is not set, then the only other valid status are bits 12, 
13 and 14.") that it is only valid for the last buffer. And apparently, 
it is 0 in the other cases. And I am also confident that when the EMAC 
sets the EOF bit it has written the correct length.
> I think this leads to problems whit reciveing packeds bigger then
> 128Bytes, because the at91_eth_deliver() function don't let the ip-stack
> (at least the lwIP-stack) allocate the right amount of memory in this case.
This would mean that the length stored in bits 0-11 of the receive 
status is not correct? It should be the length of the complete packet.

Maybe there is a difference between my AT91SAM9260 and your AT91SAM7X?
Let me know if I need to send you the datasheet.

Apart from that, why didn't you use my other fixes? We already use that 
code for many years - always on the same ARM9, but on many different 
boards and many different software projects.

A remark: do you know that the AT91SAM9260 EMAC is not performant enough 
(sort of hardware bug), and that it is advised (see datasheet errata) to 
let the EMAC use the internal SRAM and not the SDRAM? Else you will have 
performance problems at full load.

My code was tested for long time under heavy loads, so you can trust 
(most of) it.
For example this remark in your code:
"   //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
"
Or for example my fix of a wrong datasheet on line 366 about 
AT91_EMAC_TBD_SR_USED in at91_tb_init().

You can use my handling of the error cases.
I also added a reset to the driver, because sometimes things went wrong 
with AT91_EMAC_ISR_TBRE (datasheet: "Transmit buffers exhausted in 
mid-frame").
Mark that that reset to the driver (with "b_reset_tbd_idx" is needed 
when running without IRQs, so for RedBoot. This because RedBoot stalls 
when e.g. you are typing data in the telnet shell, then the EMAC buffers 
are fastly exhausted.

FYI, here a link to my old eCos mails: 
http://ecos.sourceware.org/ml/ecos-discuss/2008-06/msg00016.html.

In your code in at91_eth_deliver() you use the variable 'begpack' 
(begin-of-packet).
Is there something wrong with my implementation of the recovering of the 
buffers (code with "&= ~(AT91_EMAC_RBD_ADDR_OWNER_SW") ?

Kind regards,
Jürgen

P.S.: now the code is again fresh in my head, I will try to react faster 
next time
> I did change the two funktions and now my application seems to work stable
> whitout memory holes...
>
> I would like it if You would look at it.
>
> Thanks Oliver
>
>
> On Fri, 13 Feb 2015 15:42:56 +0100, Lambrecht Jürgen
> <J.Lambrecht@TELEVIC.com> wrote:
>> On 13/02/15 13:55, oliver.munz wrote:
>>> Dear Jürgen
>>>
>>> I'm working whit eCOS and the AT91SAM7X again and i would like to get
>>> Your lateste version of the if_at91.c.
>>>
>>> Thanks
>>>
>>> Oliver Munz
>>>
>> I just took our latest file on CVS, no editing..
>> success with it - feel free to ask stuff
>> Jürgen

On 20/04/15 13:18, oliver.munz wrote:
> This is a patch for the at91 emac driver and the lwIP stack.
>
> It should solve the no PBUF leak and other stack blocking errors. It also
> replaces some "\t" whit "    "...

-- 
Jürgen Lambrecht
R&D Associate
Mobile: +32 499 644 531
Twitter: JGRLambrecht
Tel: +32 (0)51 303045    Fax: +32 (0)51 310670
http://www.televic-rail.com
Televic Rail NV - Leo Bekaertlaan 1 - 8870 Izegem - Belgium
Company number 0825.539.581 - RPR Kortrijk

-------------- next part --------------
A non-text attachment was scrubbed...
Name: if_at91.c
Type: text/x-csrc
Size: 43521 bytes
Desc: if_at91.c
URL: <http://sourceware.org/pipermail/ecos-discuss/attachments/20150610/8b57a64d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: if_at91_OliverM_LwIP.c
Type: text/x-csrc
Size: 29078 bytes
Desc: if_at91_OliverM_LwIP.c
URL: <http://sourceware.org/pipermail/ecos-discuss/attachments/20150610/8b57a64d/attachment-0001.bin>
-------------- next part --------------
-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss