[U-Boot] [PATCH 1/3] net: davinci_emac: Remove useless dcache ops on descriptors

[U-Boot] [PATCH 1/3] net: davinci_emac: Remove useless dcache ops on descriptors

[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 <karl.beldan+oss@gmail.com>
---
 drivers/net/davinci_emac.c | 28 ----------------------------
 1 file changed, 28 deletions(-)

diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
index b030498..947bfab 100644
--- a/drivers/net/davinci_emac.c
+++ b/drivers/net/davinci_emac.c
@@ -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);
 	}
-- 
2.9.2

[U-Boot] [PATCH 2/3] net: davinci_emac: Round up top buffer boundaries for dcache ops

[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 <karl.beldan+oss@gmail.com>
---
 drivers/net/davinci_emac.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
index 947bfab..55461b0 100644
--- a/drivers/net/davinci_emac.c
+++ b/drivers/net/davinci_emac.c
@@ -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);
-- 
2.9.2

[U-Boot] [PATCH 2/3] net: davinci_emac: Round up top buffer boundaries for dcache ops

[Tom Rini]

On Sun, Aug 14, 2016 at 03:03:16PM +0000, Karl Beldan wrote:

> 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 <karl.beldan+oss@gmail.com>
> ---
>  drivers/net/davinci_emac.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
> index 947bfab..55461b0 100644
> --- a/drivers/net/davinci_emac.c
> +++ b/drivers/net/davinci_emac.c
> @@ -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.

-- 
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>

[U-Boot] [PATCH 2/3] net: davinci_emac: Round up top buffer boundaries for dcache ops

[Karl Beldan]

On Sun, Aug 14, 2016 at 11:47:25AM -0400, Tom Rini wrote:
> On Sun, Aug 14, 2016 at 03:03:16PM +0000, Karl Beldan wrote:
> 
> > 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 <karl.beldan+oss@gmail.com>
> > ---
> >  drivers/net/davinci_emac.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
> > index 947bfab..55461b0 100644
> > --- a/drivers/net/davinci_emac.c
> > +++ b/drivers/net/davinci_emac.c
> > @@ -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.
> 

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

[U-Boot] [PATCH 2/3] net: davinci_emac: Round up top buffer boundaries for dcache ops

[Tom Rini]

On Sun, Aug 14, 2016 at 07:43:56PM +0000, Karl Beldan wrote:
> On Sun, Aug 14, 2016 at 11:47:25AM -0400, Tom Rini wrote:
> > On Sun, Aug 14, 2016 at 03:03:16PM +0000, Karl Beldan wrote:
> > 
> > > 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 <karl.beldan+oss@gmail.com>
> > > ---
> > >  drivers/net/davinci_emac.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
> > > index 947bfab..55461b0 100644
> > > --- a/drivers/net/davinci_emac.c
> > > +++ b/drivers/net/davinci_emac.c
> > > @@ -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.
> > 
> 
> 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!

> 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.

-- 
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>

[U-Boot] [PATCH 2/3] net: davinci_emac: Round up top buffer boundaries for dcache ops

[Karl Beldan]

On Sun, Aug 14, 2016 at 07:43:56PM +0000, Karl Beldan wrote:
> On Sun, Aug 14, 2016 at 11:47:25AM -0400, Tom Rini wrote:
> > On Sun, Aug 14, 2016 at 03:03:16PM +0000, Karl Beldan wrote:
> > 
> > > 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 <karl.beldan+oss@gmail.com>
> > > ---
> > >  drivers/net/davinci_emac.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
> > > index 947bfab..55461b0 100644
> > > --- a/drivers/net/davinci_emac.c
> > > +++ b/drivers/net/davinci_emac.c
> > > @@ -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.
> > 
> 
> 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.

> 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

[U-Boot] [PATCH 2/3] net: davinci_emac: Round up top buffer boundaries for dcache ops

[Joe Hershberger]

On Sun, Aug 14, 2016 at 10:47 AM, Tom Rini <trini@konsulko.com> wrote:
> On Sun, Aug 14, 2016 at 03:03:16PM +0000, Karl Beldan wrote:
>
>> 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 <karl.beldan+oss@gmail.com>
>> ---
>>  drivers/net/davinci_emac.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
>> index 947bfab..55461b0 100644
>> --- a/drivers/net/davinci_emac.c
>> +++ b/drivers/net/davinci_emac.c
>> @@ -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.

Correct, please resend.

Thanks,
-Joe

[U-Boot] [PATCH 3/3] net: davinci_emac: Invalidate only the received portion of a buffer

[Karl Beldan]

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

Signed-off-by: Karl Beldan <karl.beldan+oss@gmail.com>
---
 drivers/net/davinci_emac.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
index 55461b0..e26e727 100644
--- a/drivers/net/davinci_emac.c
+++ b/drivers/net/davinci_emac.c
@@ -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 */
-- 
2.9.2

[U-Boot] [PATCH 3/3] net: davinci_emac: Invalidate only the received portion of a buffer

[Joe Hershberger]

On Sun, Aug 14, 2016 at 10:03 AM, Karl Beldan <karl.beldan@gmail.com> wrote:
> ATM when receiving a packet the whole buffer is invalidated, this change
> optimizes this behaviour.
>
> Signed-off-by: Karl Beldan <karl.beldan+oss@gmail.com>
> ---
>  drivers/net/davinci_emac.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
> index 55461b0..e26e727 100644
> --- a/drivers/net/davinci_emac.c
> +++ b/drivers/net/davinci_emac.c
> @@ -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));

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

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

Thanks,
-Joe

[U-Boot] [PATCH 1/3] net: davinci_emac: Remove useless dcache ops on descriptors

[Tom Rini]

On Sun, Aug 14, 2016 at 03:03:15PM +0000, Karl Beldan wrote:

> 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 <karl.beldan+oss@gmail.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
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>

[U-Boot] [PATCH 1/3] net: davinci_emac: Remove useless dcache ops on descriptors

[Joe Hershberger]

On Sun, Aug 14, 2016 at 10:03 AM, Karl Beldan <karl.beldan@gmail.com> wrote:
> 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 <karl.beldan+oss@gmail.com>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] net: davinci_emac: Remove useless dcache ops on descriptors

[Joe Hershberger]

Hi karl,

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

Thanks!
-Joe