net: davinci_emac: Remove useless dcache ops on descriptors

PATCH In review Karl Beldan 5 participants 12 messages Aug 14, 2016 – Aug 23, 2016 Network drivers

Share & tools

Karl Beldan

author

2016-08-14 15:03 UTC #0 lore raw

PATCH 1/3 net: davinci_emac: Remove useless dcache ops on descriptors

ATM the rx and tx descriptors are handled as cached memory while they lie in a dedicated RAM of the SoCs, which is an uncached area. Removing the said dcache ops, while optimizing the logic and clarifying the code, also gets rid of most of the check_cache_range() incurred warnings:
CACHE: Misaligned operation at range

Signed-off-by Karl Beldan <[email protected]>

---
 drivers/net/davinci_emac.c | 28 ----------------------------
 1 file changed, 28 deletions(-)

drivers/net/davinci_emac.c +0 ~~-28~~

index b030498..947bfab 100644
@@ -108,26 +108,6 @@ static u_int8_t	num_phy;
 
 phy_t				phy[CONFIG_SYS_DAVINCI_EMAC_PHY_COUNT];
 
-static inline void davinci_flush_rx_descs(void)
-{
-	/* flush the whole RX descs area */
-	flush_dcache_range(EMAC_WRAPPER_RAM_ADDR + EMAC_RX_DESC_BASE,
-			EMAC_WRAPPER_RAM_ADDR + EMAC_TX_DESC_BASE);
-}
-
-static inline void davinci_invalidate_rx_descs(void)
-{
-	/* invalidate the whole RX descs area */
-	invalidate_dcache_range(EMAC_WRAPPER_RAM_ADDR + EMAC_RX_DESC_BASE,
-			EMAC_WRAPPER_RAM_ADDR + EMAC_TX_DESC_BASE);
-}
-
-static inline void davinci_flush_desc(emac_desc *desc)
-{
-	flush_dcache_range((unsigned long)desc,
-			(unsigned long)desc + sizeof(*desc));
-}
-
 static int davinci_eth_set_mac_addr(struct eth_device *dev)
 {
 	unsigned long		mac_hi;

@@ -491,8 +471,6 @@ static int davinci_eth_open(struct eth_device *dev, bd_t *bis)
 	emac_rx_active_tail = rx_desc;
 	emac_rx_queue_active = 1;
 
-	davinci_flush_rx_descs();
-
 	/* Enable TX/RX */
 	writel(EMAC_MAX_ETHERNET_PKT_SIZE, &adap_emac->RXMAXLEN);
 	writel(0, &adap_emac->RXBUFFEROFFSET);

@@ -655,7 +633,6 @@ static int davinci_eth_send_packet (struct eth_device *dev,
 
 	flush_dcache_range((unsigned long)packet,
 			(unsigned long)packet + length);
-	davinci_flush_desc(emac_tx_desc);
 
 	/* Send the packet */
 	writel(BD_TO_HW((unsigned long)emac_tx_desc), &adap_emac->TX0HDP);

@@ -689,8 +666,6 @@ static int davinci_eth_rcv_packet (struct eth_device *dev)
 	volatile emac_desc *tail_desc;
 	int status, ret = -1;
 
-	davinci_invalidate_rx_descs();
-
 	rx_curr_desc = emac_rx_active_head;
 	if (!rx_curr_desc)
 		return 0;

@@ -729,7 +704,6 @@ static int davinci_eth_rcv_packet (struct eth_device *dev)
 		rx_curr_desc->buff_off_len = EMAC_MAX_ETHERNET_PKT_SIZE;
 		rx_curr_desc->pkt_flag_len = EMAC_CPPI_OWNERSHIP_BIT;
 		rx_curr_desc->next = 0;
-		davinci_flush_desc(rx_curr_desc);
 
 		if (emac_rx_active_head == 0) {
 			printf ("INFO: emac_rcv_pkt: active queue head = 0\n");

@@ -747,13 +721,11 @@ static int davinci_eth_rcv_packet (struct eth_device *dev)
 			tail_desc->next = BD_TO_HW((ulong) curr_desc);
 			status = tail_desc->pkt_flag_len;
 			if (status & EMAC_CPPI_EOQ_BIT) {
-				davinci_flush_desc(tail_desc);
 				writel(BD_TO_HW((ulong)curr_desc),
 				       &adap_emac->RX0HDP);
 				status &= ~EMAC_CPPI_EOQ_BIT;
 				tail_desc->pkt_flag_len = status;
 			}
-			davinci_flush_desc(tail_desc);
 		}
 		return (ret);
 	}

Signature · 1 line

2.9.2

Karl Beldan

author

2016-08-14 15:03 UTC #1 lore raw

PATCH 2/3 net: davinci_emac: Round up top buffer boundaries for dcache ops

check_cache_range() warns that the top boundaries are not properly aligned while flushing and invalidating the buffers and make these operations to fail.
ATM the RX bottom boundaries are aligned by design with EMAC_RXBUF_SIZE, properly aligned with ARCH_DMA_MINALIGN, however the top ones are not.

This gets rid of the warnings:
CACHE: Misaligned operation at range

Signed-off-by Karl Beldan <[email protected]>

---
 drivers/net/davinci_emac.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/net/davinci_emac.c +4 -2

index 947bfab..55461b0 100644
@@ -632,7 +632,8 @@ static int davinci_eth_send_packet (struct eth_device *dev,
 				      EMAC_CPPI_EOP_BIT);
 
 	flush_dcache_range((unsigned long)packet,
-			(unsigned long)packet + length);
+			   round_up((unsigned long)packet + length,
+				    ARCH_DMA_MINALIGN));
 
 	/* Send the packet */
 	writel(BD_TO_HW((unsigned long)emac_tx_desc), &adap_emac->TX0HDP);

@@ -677,7 +678,8 @@ static int davinci_eth_rcv_packet (struct eth_device *dev)
 		} else {
 			unsigned long tmp = (unsigned long)rx_curr_desc->buffer;
 
-			invalidate_dcache_range(tmp, tmp + EMAC_RXBUF_SIZE);
+			invalidate_dcache_range(tmp, round_up(tmp + EMAC_RXBUF_SIZE,
+							      ARCH_DMA_MINALIGN));
 			net_process_received_packet(
 				rx_curr_desc->buffer,
 				rx_curr_desc->buff_off_len & 0xffff);

Signature · 1 line

2.9.2

Tom Rini

2016-08-14 15:47 UTC #2 lore raw

PATCH 2/3 net: davinci_emac: Round up top buffer boundaries for dcache ops

Quoted 19 lines · Karl Beldan

check_cache_range() warns that the top boundaries are not properly aligned while flushing and invalidating the buffers and make these operations to fail.
ATM the RX bottom boundaries are aligned by design with EMAC_RXBUF_SIZE, properly aligned with ARCH_DMA_MINALIGN, however the top ones are not.
This gets rid of the warnings:
CACHE: Misaligned operation at range
Signed-off-by Karl Beldan <[email protected]>
---
 drivers/net/davinci_emac.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/net/davinci_emac.c +0 -0
index 947bfab..55461b0 100644

drivers/net/davinci_emac.c static int davinci_eth_send_packet (struct eth_device *dev, · line 636

Quoted 6 lines

drivers/net/davinci_emac.c +2 -1

@@ -632,7 +632,8 @@ static int davinci_eth_send_packet (struct eth_device *dev,
 				      EMAC_CPPI_EOP_BIT);
 
 	flush_dcache_range((unsigned long)packet,
-			(unsigned long)packet + length);
+			   round_up((unsigned long)packet + length,
+				    ARCH_DMA_MINALIGN));

It's preferred to use:

                        (unsigned long)packet + ALIGN(length, PKTALIGN));

here instead of ARCH_DMA_MINALIGN.

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160814/8cbdeff6/attachment.sig>

Karl Beldan

2016-08-14 19:43 UTC #3 lore raw

PATCH 2/3 net: davinci_emac: Round up top buffer boundaries for dcache ops

Quoted 23 lines · Tom Rini

Quoted 21 lines · Karl Beldan
check_cache_range() warns that the top boundaries are not properly aligned while flushing and invalidating the buffers and make these operations to fail.
ATM the RX bottom boundaries are aligned by design with EMAC_RXBUF_SIZE, properly aligned with ARCH_DMA_MINALIGN, however the top ones are not.
This gets rid of the warnings:
CACHE: Misaligned operation at range
Signed-off-by Karl Beldan <[email protected]>
---
 drivers/net/davinci_emac.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/net/davinci_emac.c +0 -0
index 947bfab..55461b0 100644
@@ -632,7 +632,8 @@ static int davinci_eth_send_packet (struct eth_device *dev,
 				      EMAC_CPPI_EOP_BIT);
 

Quoted 9 lines

drivers/net/davinci_emac.c static int davinci_eth_send_packet (struct eth_device *dev, · line 636
Quoted 4 lines
drivers/net/davinci_emac.c +2 -1
 	flush_dcache_range((unsigned long)packet,
-			(unsigned long)packet + length);
+			   round_up((unsigned long)packet + length,
+				    ARCH_DMA_MINALIGN));
It's preferred to use:
                        (unsigned long)packet + ALIGN(length, PKTALIGN)); 
here instead of ARCH_DMA_MINALIGN.

Hmm, I think your suggestion is buggy.
The cache primitives act on [laddr, haddr[, i.e. haddr is excluded, IOW you are missing the tail of the packet (that's why I rounded up).

Conceptually I still prefer ARCH_DMA_MINALIGN, also all other code in the base does so.

Rgds,
Karl

Tom Rini

2016-08-14 21:36 UTC #4 lore raw

PATCH 2/3 net: davinci_emac: Round up top buffer boundaries for dcache ops

Quoted 34 lines · Karl Beldan

Quoted 23 lines · Tom Rini
Quoted 21 lines · Karl Beldan
check_cache_range() warns that the top boundaries are not properly aligned while flushing and invalidating the buffers and make these operations to fail.
ATM the RX bottom boundaries are aligned by design with EMAC_RXBUF_SIZE, properly aligned with ARCH_DMA_MINALIGN, however the top ones are not.
This gets rid of the warnings:
CACHE: Misaligned operation at range
Signed-off-by Karl Beldan <[email protected]>
---
 drivers/net/davinci_emac.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/net/davinci_emac.c +0 -0
index 947bfab..55461b0 100644
@@ -632,7 +632,8 @@ static int davinci_eth_send_packet (struct eth_device *dev,
 				      EMAC_CPPI_EOP_BIT);
 
Quoted 9 lines
drivers/net/davinci_emac.c static int davinci_eth_send_packet (struct eth_device *dev, · line 636
Quoted 4 lines
drivers/net/davinci_emac.c +2 -1
 	flush_dcache_range((unsigned long)packet,
-			(unsigned long)packet + length);
+			   round_up((unsigned long)packet + length,
+				    ARCH_DMA_MINALIGN));
It's preferred to use:
                        (unsigned long)packet + ALIGN(length, PKTALIGN)); 
here instead of ARCH_DMA_MINALIGN.

Quoted 3 lines

Hmm, I think your suggestion is buggy.
The cache primitives act on [laddr, haddr[, i.e. haddr is excluded, IOW you are missing the tail of the packet (that's why I rounded up).

Joe, we've got some instances using ALIGN(length, PKTALIGN) and others using roundup. And I can't imagine that we shouldn't be using the same thing everywhere here. So, what do you say to Karl's comment? Thanks!

Quoted 2 lines

Conceptually I still prefer ARCH_DMA_MINALIGN, also all other code in the base does so.

Being network code, we should use PKTALIGN here and elsewhere.

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160814/d2f5ab32/attachment.sig>

Karl Beldan

2016-08-15 16:47 UTC #5 lore raw

PATCH 2/3 net: davinci_emac: Round up top buffer boundaries for dcache ops

In reply to Karl Beldan

Quoted 34 lines · Karl Beldan

Quoted 23 lines · Tom Rini
Quoted 21 lines · Karl Beldan
check_cache_range() warns that the top boundaries are not properly aligned while flushing and invalidating the buffers and make these operations to fail.
ATM the RX bottom boundaries are aligned by design with EMAC_RXBUF_SIZE, properly aligned with ARCH_DMA_MINALIGN, however the top ones are not.
This gets rid of the warnings:
CACHE: Misaligned operation at range
Signed-off-by Karl Beldan <[email protected]>
---
 drivers/net/davinci_emac.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/net/davinci_emac.c +0 -0
index 947bfab..55461b0 100644
@@ -632,7 +632,8 @@ static int davinci_eth_send_packet (struct eth_device *dev,
 				      EMAC_CPPI_EOP_BIT);
 
Quoted 9 lines
drivers/net/davinci_emac.c static int davinci_eth_send_packet (struct eth_device *dev, · line 636
Quoted 4 lines
drivers/net/davinci_emac.c +2 -1
 	flush_dcache_range((unsigned long)packet,
-			(unsigned long)packet + length);
+			   round_up((unsigned long)packet + length,
+				    ARCH_DMA_MINALIGN));
It's preferred to use:
                        (unsigned long)packet + ALIGN(length, PKTALIGN)); 
here instead of ARCH_DMA_MINALIGN.

Quoted 4 lines

Hmm, I think your suggestion is buggy.
The cache primitives act on [laddr, haddr[, i.e. haddr is excluded, IOW you are missing the tail of the packet (that's why I rounded up).

Just checked, your ALIGN macro also rounds up so your suggestion is not buggy, my bad.

Quoted 3 lines

Conceptually I still prefer ARCH_DMA_MINALIGN, also all other code in the base does so.

Your suggestion is seconded by Joe, I'll send v2.

Rgds,
Karl

Joe Hershberger

2016-08-15 16:06 UTC #6 lore raw

PATCH 2/3 net: davinci_emac: Round up top buffer boundaries for dcache ops

In reply to Tom Rini

Quoted 23 lines · Tom Rini

Quoted 21 lines · Karl Beldan
check_cache_range() warns that the top boundaries are not properly aligned while flushing and invalidating the buffers and make these operations to fail.
ATM the RX bottom boundaries are aligned by design with EMAC_RXBUF_SIZE, properly aligned with ARCH_DMA_MINALIGN, however the top ones are not.
This gets rid of the warnings:
CACHE: Misaligned operation at range
Signed-off-by Karl Beldan <[email protected]>
---
 drivers/net/davinci_emac.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/net/davinci_emac.c +0 -0
index 947bfab..55461b0 100644
@@ -632,7 +632,8 @@ static int davinci_eth_send_packet (struct eth_device *dev,
                                    EMAC_CPPI_EOP_BIT);
 

Quoted 8 lines

drivers/net/davinci_emac.c static int davinci_eth_send_packet (struct eth_device *dev, · line 636

Quoted 4 lines

drivers/net/davinci_emac.c +2 -1

      flush_dcache_range((unsigned long)packet,
-                     (unsigned long)packet + length);
+                        round_up((unsigned long)packet + length,
+                                 ARCH_DMA_MINALIGN));

It's preferred to use:

                        (unsigned long)packet + ALIGN(length, PKTALIGN));

here instead of ARCH_DMA_MINALIGN.

Correct, please resend.

Thanks,
-Joe

Karl Beldan

author

2016-08-14 15:03 UTC #7 lore raw

PATCH 3/3 net: davinci_emac: Invalidate only the received portion of a buffer

In reply to Karl Beldan

ATM when receiving a packet the whole buffer is invalidated, this change optimizes this behaviour.

Signed-off-by Karl Beldan <[email protected]>

---
 drivers/net/davinci_emac.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/net/davinci_emac.c +6 -6

index 55461b0..e26e727 100644
@@ -677,13 +677,13 @@ static int davinci_eth_rcv_packet (struct eth_device *dev)
 			printf ("WARN: emac_rcv_pkt: Error in packet\n");
 		} else {
 			unsigned long tmp = (unsigned long)rx_curr_desc->buffer;
+			unsigned short len =
+				rx_curr_desc->buff_off_len & 0xffff;
 
-			invalidate_dcache_range(tmp, round_up(tmp + EMAC_RXBUF_SIZE,
-							      ARCH_DMA_MINALIGN));
-			net_process_received_packet(
-				rx_curr_desc->buffer,
-				rx_curr_desc->buff_off_len & 0xffff);
-			ret = rx_curr_desc->buff_off_len & 0xffff;
+			invalidate_dcache_range(tmp, round_up(tmp + len,
+						ARCH_DMA_MINALIGN));
+			net_process_received_packet(rx_curr_desc->buffer, len);
+			ret = len;
 		}
 
 		/* Ack received packet descriptor */

Signature · 1 line

2.9.2

Joe Hershberger

2016-08-15 16:09 UTC #8 lore raw

PATCH 3/3 net: davinci_emac: Invalidate only the received portion of a buffer

Quoted 17 lines · Karl Beldan

ATM when receiving a packet the whole buffer is invalidated, this change optimizes this behaviour.

Signed-off-by Karl Beldan <[email protected]>

---
 drivers/net/davinci_emac.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/net/davinci_emac.c +1 -0

index 55461b0..e26e727 100644
@@ -677,13 +677,13 @@ static int davinci_eth_rcv_packet (struct eth_device *dev)
                        printf ("WARN: emac_rcv_pkt: Error in packet\n");
                } else {
                        unsigned long tmp = (unsigned long)rx_curr_desc->buffer;
+                       unsigned short len =

drivers/net/davinci_emac.c static int davinci_eth_rcv_packet (struct eth_device *dev) · line 684

Quoted 10 lines

drivers/net/davinci_emac.c +3 -6

+                               rx_curr_desc->buff_off_len & 0xffff;
 
-                       invalidate_dcache_range(tmp, round_up(tmp + EMAC_RXBUF_SIZE,
-                                                             ARCH_DMA_MINALIGN));
-                       net_process_received_packet(
-                               rx_curr_desc->buffer,
-                               rx_curr_desc->buff_off_len & 0xffff);
-                       ret = rx_curr_desc->buff_off_len & 0xffff;
+                       invalidate_dcache_range(tmp, round_up(tmp + len,
+                                               ARCH_DMA_MINALIGN));

Here again, please use tmp + ALIGN(len, PKTALIGN)

drivers/net/davinci_emac.c static int davinci_eth_rcv_packet (struct eth_device *dev) · line 687

Quoted 3 lines

drivers/net/davinci_emac.c +2 -0

+                       net_process_received_packet(rx_curr_desc->buffer, len);
+                       ret = len;
                }

Thanks,
-Joe

Tom Rini

2016-08-14 15:47 UTC #9 lore raw

PATCH 1/3 net: davinci_emac: Remove useless dcache ops on descriptors

In reply to Karl Beldan

Quoted 8 lines · Karl Beldan

ATM the rx and tx descriptors are handled as cached memory while they lie in a dedicated RAM of the SoCs, which is an uncached area. Removing the said dcache ops, while optimizing the logic and clarifying the code, also gets rid of most of the check_cache_range() incurred warnings:
CACHE: Misaligned operation at range
Signed-off-by Karl Beldan <[email protected]>

Reviewed-by Tom Rini <[email protected]>

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160814/b1490442/attachment.sig>

Joe Hershberger

2016-08-15 16:05 UTC #10 lore raw

PATCH 1/3 net: davinci_emac: Remove useless dcache ops on descriptors

In reply to Karl Beldan

Quoted 8 lines · Karl Beldan

ATM the rx and tx descriptors are handled as cached memory while they lie in a dedicated RAM of the SoCs, which is an uncached area. Removing the said dcache ops, while optimizing the logic and clarifying the code, also gets rid of most of the check_cache_range() incurred warnings:
CACHE: Misaligned operation at range
Signed-off-by Karl Beldan <[email protected]>

Acked-by Joe Hershberger <[email protected]>

Joe Hershberger

2016-08-23 02:28 UTC #11 lore raw

In reply to Karl Beldan

Hi karl,

https://patchwork.ozlabs.org/patch/659047/ was applied to u-boot-net.git.

Thanks!
-Joe