[U-Boot] [PATCH 10/12] net: emaclite: Use indirect access in emaclite_recv
Michal Simek
michal.simek at xilinx.com
Thu Dec 17 12:24:32 CET 2015
On 15.12.2015 23:20, Joe Hershberger wrote:
> On Fri, Dec 11, 2015 at 6:03 AM, Michal Simek <michal.simek at 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 at 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(®s->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 = ®s->rx_ping;
>> + ack = ®s->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(®s->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 = ®s->rx_pong;
>> + ack = ®s->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
More information about the U-Boot
mailing list