[U-Boot] [PATCH 10/12] net: emaclite: Use indirect access in emaclite_recv

[Michal Simek <michal.simek@xilinx.com>]

On 15.12.2015 23:20, Joe Hershberger wrote:
> On Fri, Dec 11, 2015 at 6:03 AM, Michal Simek <michal.simek@xilinx.com> wrote:
>> When IP is configured with pong buffers, IP is receiving packets to ping
>> and then to pong buffer and than ping again.
>> Origin logic in the driver remains there that when ping buffer is
> 
> Origin -> Original

fixed

> 
>> free, pong buffer is checked too and return if both are free.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> Do you know macros which I could use for addressing certain fields in
>> ethernet and IP packet? The code is there because copying huge amount of
>> data is causing performance penalty.
> 
> So you're saying the IP will not tell you the length of the frame
> received? You have to pull it out of the packet data?

Unfortunately yes. Linux driver uses the same logic.
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/xilinx/xilinx_emaclite.c#n414

> 
>> ---
>>  drivers/net/xilinx_emaclite.c | 90 ++++++++++++++++++++++++-------------------
>>  1 file changed, 51 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
>> index e97ce2ce31f3..b5ff4f099251 100644
>> --- a/drivers/net/xilinx_emaclite.c
>> +++ b/drivers/net/xilinx_emaclite.c
>> @@ -22,11 +22,6 @@
>>
>>  #define ENET_ADDR_LENGTH       6
> 
> Use ARP_HLEN from include/net.h
> 
>>
>> -/* EmacLite constants */
>> -#define XEL_BUFFER_OFFSET      0x0800  /* Next buffer's offset */
>> -#define XEL_RSR_OFFSET         0x17FC  /* Rx status */
>> -#define XEL_RXBUFF_OFFSET      0x1000  /* Receive Buffer */
>> -
>>  /* Xmit complete */
>>  #define XEL_TSR_XMIT_BUSY_MASK         0x00000001UL
>>  /* Xmit interrupt enable bit */
>> @@ -86,7 +81,7 @@ struct emaclite_regs {
>>  };
>>
>>  struct xemaclite {
>> -       u32 nextrxbuffertouse;  /* Next RX buffer to read from */
>> +       bool nextrxbuffertouse; /* Next RX buffer to read from */
> 
> When using a boolean for this sort of thing it is good to give it a
> more clear name, such as "use_rx_pong_buffer_next".

done.

> 
>>         u32 txpp;               /* TX ping pong buffer */
>>         u32 rxpp;               /* RX ping pong buffer */
>>         int phyaddr;
>> @@ -455,45 +450,63 @@ static int emaclite_recv(struct eth_device *dev)
>>  {
>>         u32 length;
>>         u32 reg;
>> -       u32 baseaddress;
>> +       u32 *addr, *ack;
>>         struct xemaclite *emaclite = dev->priv;
>> -
>> -       baseaddress = dev->iobase + emaclite->nextrxbuffertouse;
>> -       reg = in_be32 (baseaddress + XEL_RSR_OFFSET);
>> -       debug("Testing data at address 0x%x\n", baseaddress);
>> -       if ((reg & XEL_RSR_RECV_DONE_MASK) == XEL_RSR_RECV_DONE_MASK) {
>> -               if (emaclite->rxpp)
>> -                       emaclite->nextrxbuffertouse ^= XEL_BUFFER_OFFSET;
>> +       struct emaclite_regs *regs = emaclite->regs;
>> +       u32 attempt = 0;
>> +
>> +try_again:
>> +       if (!emaclite->nextrxbuffertouse) {
>> +               reg = in_be32(&regs->rx_ping_rsr);
>> +               debug("Testing data at rx_ping\n");
>> +               if ((reg & XEL_RSR_RECV_DONE_MASK) == XEL_RSR_RECV_DONE_MASK) {
>> +                       debug("Data found in rx_ping buffer\n");
>> +                       addr = &regs->rx_ping;
>> +                       ack = &regs->rx_ping_rsr;
>> +               } else {
>> +                       debug("Data not found in rx_ping buffer\n");
>> +                       /* Pong buffer is not available - return immediatelly */
> 
> immediatelly -> immediately

fixed.

> 
>> +                       if (!emaclite->rxpp)
>> +                               return -1;
>> +
>> +                       /* Try pong buffer if this is first attempt */
>> +                       if (attempt++)
>> +                               return -1;
>> +                       emaclite->nextrxbuffertouse =
>> +                                                 !emaclite->nextrxbuffertouse;
>> +                       goto try_again;
>> +               }
>>         } else {
>> -
>> -               if (!emaclite->rxpp) {
>> -                       debug("No data was available - address 0x%x\n",
>> -                                                               baseaddress);
>> -                       return 0;
>> +               reg = in_be32(&regs->rx_pong_rsr);
>> +               debug("Testing data at rx_pong\n");
>> +               if ((reg & XEL_RSR_RECV_DONE_MASK) == XEL_RSR_RECV_DONE_MASK) {
>> +                       debug("Data found in rx_pong buffer\n");
>> +                       addr = &regs->rx_pong;
>> +                       ack = &regs->rx_pong_rsr;
>>                 } else {
>> -                       baseaddress ^= XEL_BUFFER_OFFSET;
>> -                       reg = in_be32 (baseaddress + XEL_RSR_OFFSET);
>> -                       if ((reg & XEL_RSR_RECV_DONE_MASK) !=
>> -                                               XEL_RSR_RECV_DONE_MASK) {
>> -                               debug("No data was available - address 0x%x\n",
>> -                                               baseaddress);
>> -                               return 0;
>> -                       }
>> +                       debug("Data not found in rx_pong buffer\n");
>> +                       /* Try ping buffer if this is first attempt */
>> +                       if (attempt++)
>> +                               return -1;
>> +                       emaclite->nextrxbuffertouse =
>> +                                                 !emaclite->nextrxbuffertouse;
>> +                       goto try_again;
>>                 }
>>         }
>> +
>> +#define        ETH_TYPE_OFFSET         3
> 
> This is offset in count of 32-bit words.

yep

> 
>> +#define        IP_LENGTH_OFFSET        4
>>         /* Get the length of the frame that arrived */
>> -       switch(((ntohl(in_be32 (baseaddress + XEL_RXBUFF_OFFSET + 0xC))) &
>> -                       0xFFFF0000 ) >> 16) {
>> +       switch (((ntohl(in_be32(addr + ETH_TYPE_OFFSET))) & 0xFFFF0000) >> 16) {
> 
> Why not read 16 bits? Not possible in hardware?

There are a lot of versions of this IP.
It is BE/LE driver and there could be HW issues too. But look below.

> 
>>                 case 0x806:
> 
> Use PROT_ARP from include/net.h

done.

> 
>>                         length = 42 + 20; /* FIXME size of ARP */
> 
> Not sure what this is trying to include, but that seems like it is too big.
> 
> From net.h:
> #define ETHER_HDR_SIZE  (sizeof(struct ethernet_hdr))
> which equals 14 bytes.
> #define ARP_HDR_SIZE    (8+20)
> 
> So it should add up to 14 + 8 + 20 = 42;
> 
> I would use:
> 
>                          length = ETHER_HDR_SIZE + ARP_HDR_SIZE;

Based on
http://image.slidesharecdn.com/arp-140623072259-phpapp01/95/arp-6-638.jpg?cb=1403508219

+ 10 padding and +4 crc.
I see that tsec driver does something with crc but maybe crc are
completely ignored by U-Boot

On the other hand linux is using just ETH_FCS_LEN (4).
I have added +4 for now.


> 
>> -                       debug("ARP Packet\n");
>> +                       debug("ARP Packet %x\n", length);
>>                         break;
>>                 case 0x800:
> 
> Use PROT_IP from include/net.h

fixed.

> 
>>                         length = 14 + 14 +
>> -                       (((ntohl(in_be32 (baseaddress + XEL_RXBUFF_OFFSET +
>> -                                               0x10))) & 0xFFFF0000) >> 16);
>> -                       /* FIXME size of IP packet */
>> -                       debug ("IP Packet\n");
>> +                             (((ntohl(in_be32(addr + IP_LENGTH_OFFSET))) &
>> +                              0xFFFF0000) >> 16);
> 
> Why not read 16 bits?

look below.

> 
> If this is reading the total_length field, it seems that it will not
> handle packet fragments well.

Isn't it fragmentation done on one layer up? IP packet header is there
all the time.


>> +                       debug("IP Packet %x\n", length);
>>                         break;
> 
> Can you afford to read the first to read the first 5 words of the
> packet into a buffer and use the structs in net.h to overlay the
> buffer to access the data? That would be a lot cleaner to look at.

I have tried it and it is working fine. I am not reading just 5 words
but I think it is better to read the full align ARP packet size and then
do another read for the rest.
Please look at v2 with this change I think you will like it more than
this one.


> You also need to somehow track fragmented packets. Not sure how to do
> it universally. You sure there's no way to get the frame size from the
> hardware? That seems very limiting.

I haven't seen any problem like this and I expect none has played with
it too.

Thanks,
Michal