at91 emac ethernet driver lwip diff patch

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

at91 emac ethernet driver lwip diff patch

oliver.munz
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 "    "...

at91eth.diff (21K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: at91 emac ethernet driver lwip diff patch

Lambrecht Jürgen
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
> <[hidden email]> 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


if_at91.c (58K) Download Attachment
if_at91_OliverM_LwIP.c (39K) Download Attachment