[U-Boot] [PATCH v2 2/2] net/designware: ensure cache invalidations are aligned to ARCH_DMA_MINALIGN

[U-Boot] [PATCH v2 2/2] net/designware: ensure cache invalidations are aligned to ARCH_DMA_MINALIGN

[Ian Campbell]

This is required at least on ARM.

When sending instead of simply invalidating the entire descriptor, flush
as little as possible while still respecting ARCH_DMA_MINALIGN, as
requested by Alexey.

Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
---
v2: - collapsed "net/designware: align cache invalidation on rx" and
      "net/designware: invalidate entire descriptor in dw_eth_send" into
      one.
    - roundup sizeof(txrx_status) to ARCH_DMA_MINALIGN instead of just
      invalidating the entire descriptor.
---
 drivers/net/designware.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index b70df82..02ceb91 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -280,10 +280,18 @@ static int dw_eth_send(struct eth_device *dev, void *packet, int length)
 	u32 desc_num = priv->tx_currdescnum;
 	struct dmamacdescr *desc_p = &priv->tx_mac_descrtable[desc_num];
 
-	/* Invalidate only "status" field for the following check */
-	invalidate_dcache_range((unsigned long)&desc_p->txrx_status,
-				(unsigned long)&desc_p->txrx_status +
-				sizeof(desc_p->txrx_status));
+	/*
+	 * Strictly we only need to invalidate the "txrx_status" field
+	 * for the following check, but on some platforms we cannot
+	 * invalidate only 4 bytes, so roundup to
+	 * ARCH_DMA_MINALIGN. This is safe because the individual
+	 * descriptors in the array are each aligned to
+	 * ARCH_DMA_MINALIGN.
+	 */
+	invalidate_dcache_range(
+		(unsigned long)desc_p,
+		(unsigned long)desc_p +
+		roundup(sizeof(desc_p->txrx_status), ARCH_DMA_MINALIGN));
 
 	/* Check if the descriptor is owned by CPU */
 	if (desc_p->txrx_status & DESC_TXSTS_OWNBYDMA) {
@@ -351,7 +359,7 @@ static int dw_eth_recv(struct eth_device *dev)
 		/* Invalidate received data */
 		invalidate_dcache_range((unsigned long)desc_p->dmamac_addr,
 					(unsigned long)desc_p->dmamac_addr +
-					length);
+					roundup(length, ARCH_DMA_MINALIGN));
 
 		NetReceive(desc_p->dmamac_addr, length);
 
-- 
1.9.0

[U-Boot] [PATCH 3/2] net/designware: reorder struct dw_eth_dev to pack more efficiently.

[Ian Campbell]

The {tx,rx}_mac_descrtable fields are aligned to ARCH_DMA_MINALIGN, which could
be 256 or even larger. That means there is a potentially huge hole in the
struct before those fields, so move them to the front where they are better
packed.

Moving them to the front also helps ensure that so long as dw_eth_dev is
properly aligned (which it is since "net/designware: ensure device private data
is DMA aligned.") the {tx,rx}_mac_descrtable will be too, or at least avoids
having to worry too much about compiler specifics.

Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
---
 drivers/net/designware.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/designware.h b/drivers/net/designware.h
index 382b0c7..6d94b3a 100644
--- a/drivers/net/designware.h
+++ b/drivers/net/designware.h
@@ -215,13 +215,13 @@ struct dmamacdescr {
 #endif
 
 struct dw_eth_dev {
+	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
+	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
+
 	u32 interface;
 	u32 tx_currdescnum;
 	u32 rx_currdescnum;
 
-	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
-	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
-
 	char txbuffs[TX_TOTAL_BUFSIZE];
 	char rxbuffs[RX_TOTAL_BUFSIZE];
 
-- 
1.9.0

[U-Boot] [PATCH 3/2] net/designware: reorder struct dw_eth_dev to pack more efficiently.

[Marek Vasut]

On Thursday, May 01, 2014 at 09:01:58 PM, Ian Campbell wrote:
> The {tx,rx}_mac_descrtable fields are aligned to ARCH_DMA_MINALIGN, which
> could be 256 or even larger. That means there is a potentially huge hole
> in the struct before those fields, so move them to the front where they
> are better packed.
> 
> Moving them to the front also helps ensure that so long as dw_eth_dev is
> properly aligned (which it is since "net/designware: ensure device private
> data is DMA aligned.") the {tx,rx}_mac_descrtable will be too, or at least
> avoids having to worry too much about compiler specifics.
> 
> Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[U-Boot] [linux-sunxi] [PATCH 3/2] net/designware: reorder struct dw_eth_dev to pack more efficiently.

[Siarhei Siamashka]

On Thu, 01 May 2014 20:01:58 +0100
Ian Campbell <ijc@hellion.org.uk> wrote:

> The {tx,rx}_mac_descrtable fields are aligned to ARCH_DMA_MINALIGN, which could
> be 256 or even larger. That means there is a potentially huge hole in the
> struct before those fields, so move them to the front where they are better
> packed.
> 
> Moving them to the front also helps ensure that so long as dw_eth_dev is
> properly aligned (which it is since "net/designware: ensure device private data
> is DMA aligned.") the {tx,rx}_mac_descrtable will be too, or at least avoids
> having to worry too much about compiler specifics.
> 
> Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> ---
>  drivers/net/designware.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/designware.h b/drivers/net/designware.h
> index 382b0c7..6d94b3a 100644
> --- a/drivers/net/designware.h
> +++ b/drivers/net/designware.h
> @@ -215,13 +215,13 @@ struct dmamacdescr {
>  #endif
>  
>  struct dw_eth_dev {
> +	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
> +	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
> +
>  	u32 interface;
>  	u32 tx_currdescnum;
>  	u32 rx_currdescnum;
>  
> -	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
> -	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
> -
>  	char txbuffs[TX_TOTAL_BUFSIZE];
>  	char rxbuffs[RX_TOTAL_BUFSIZE];

After this reordering, txbuffs and rxbuffs buffers become DMA unaligned.
And they are also used with the cache flush/invalidate operations all
over the place, causing all the same "v7_dcache_inval_range - start
address is not aligned" failures.

The txbuffs/rxbuffs buffers probably should immediately follow
dmamacdescr structs and also have their own alignment enforcement
attribute.

As for the buffer sizes, we have the following defines:

#define CONFIG_TX_DESCR_NUM	16
#define CONFIG_RX_DESCR_NUM	16
#define CONFIG_ETH_BUFSIZE	2048
#define TX_TOTAL_BUFSIZE	(CONFIG_ETH_BUFSIZE * CONFIG_TX_DESCR_NUM)
#define RX_TOTAL_BUFSIZE	(CONFIG_ETH_BUFSIZE * CONFIG_RX_DESCR_NUM)

CONFIG_ETH_BUFSIZE is a power of two, which is good. Still maybe an
extra assertion check to verify/confirm that it is divisible by the
cache line size would make the code cleaner? But that's just a nitpick,
because the "v7_dcache_inval_range" function is noisy enough if anything
is wrong :-)

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [linux-sunxi] [PATCH 3/2] net/designware: reorder struct dw_eth_dev to pack more efficiently.

[Ian Campbell]

On Wed, 2014-05-14 at 10:44 +0300, Siarhei Siamashka wrote:
> > diff --git a/drivers/net/designware.h b/drivers/net/designware.h
> > index 382b0c7..6d94b3a 100644
> > --- a/drivers/net/designware.h
> > +++ b/drivers/net/designware.h
> > @@ -215,13 +215,13 @@ struct dmamacdescr {
> >  #endif
> >  
> >  struct dw_eth_dev {
> > +	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
> > +	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
> > +
> >  	u32 interface;
> >  	u32 tx_currdescnum;
> >  	u32 rx_currdescnum;
> >  
> > -	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
> > -	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
> > -
> >  	char txbuffs[TX_TOTAL_BUFSIZE];
> >  	char rxbuffs[RX_TOTAL_BUFSIZE];
> 
> After this reordering, txbuffs and rxbuffs buffers become DMA unaligned.

Right, this is fixed in the repost which is part of "[PATCH v3 0/5]
net/designware: fixes for data cache, phylib and burst size".

Ian.

[U-Boot] [linux-sunxi] [PATCH 3/2] net/designware: reorder struct dw_eth_dev to pack more efficiently.

[Siarhei Siamashka]

On Wed, 14 May 2014 08:52:50 +0100
Ian Campbell <ijc@hellion.org.uk> wrote:

> On Wed, 2014-05-14 at 10:44 +0300, Siarhei Siamashka wrote:
> > > diff --git a/drivers/net/designware.h b/drivers/net/designware.h
> > > index 382b0c7..6d94b3a 100644
> > > --- a/drivers/net/designware.h
> > > +++ b/drivers/net/designware.h
> > > @@ -215,13 +215,13 @@ struct dmamacdescr {
> > >  #endif
> > >  
> > >  struct dw_eth_dev {
> > > +	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
> > > +	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
> > > +
> > >  	u32 interface;
> > >  	u32 tx_currdescnum;
> > >  	u32 rx_currdescnum;
> > >  
> > > -	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
> > > -	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
> > > -
> > >  	char txbuffs[TX_TOTAL_BUFSIZE];
> > >  	char rxbuffs[RX_TOTAL_BUFSIZE];
> > 
> > After this reordering, txbuffs and rxbuffs buffers become DMA unaligned.
> 
> Right, this is fixed in the repost which is part of "[PATCH v3 0/5]
> net/designware: fixes for data cache, phylib and burst size".

Oh, you just forgot to add the linux-sunxi list to CC when sending
the updated patches. This explains why I don't see them in my mailbox.

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [linux-sunxi] [PATCH 3/2] net/designware: reorder struct dw_eth_dev to pack more efficiently.

[Siarhei Siamashka]

On Wed, 14 May 2014 11:01:59 +0300
Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote:

> On Wed, 14 May 2014 08:52:50 +0100
> Ian Campbell <ijc@hellion.org.uk> wrote:
> 
> > On Wed, 2014-05-14 at 10:44 +0300, Siarhei Siamashka wrote:
> > > > diff --git a/drivers/net/designware.h b/drivers/net/designware.h
> > > > index 382b0c7..6d94b3a 100644
> > > > --- a/drivers/net/designware.h
> > > > +++ b/drivers/net/designware.h
> > > > @@ -215,13 +215,13 @@ struct dmamacdescr {
> > > >  #endif
> > > >  
> > > >  struct dw_eth_dev {
> > > > +	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
> > > > +	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
> > > > +
> > > >  	u32 interface;
> > > >  	u32 tx_currdescnum;
> > > >  	u32 rx_currdescnum;
> > > >  
> > > > -	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
> > > > -	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
> > > > -
> > > >  	char txbuffs[TX_TOTAL_BUFSIZE];
> > > >  	char rxbuffs[RX_TOTAL_BUFSIZE];
> > > 
> > > After this reordering, txbuffs and rxbuffs buffers become DMA unaligned.
> > 
> > Right, this is fixed in the repost which is part of "[PATCH v3 0/5]
> > net/designware: fixes for data cache, phylib and burst size".
> 
> Oh, you just forgot to add the linux-sunxi list to CC when sending
> the updated patches. This explains why I don't see them in my mailbox.

However, as I can see at
    http://lists.denx.de/pipermail/u-boot/2014-May/179218.html
you are still sandwiching interface, tx_currdescnum and rx_currdescnum
between the DMA aligned stuff. This does not really improve packing.

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [linux-sunxi] [PATCH 3/2] net/designware: reorder struct dw_eth_dev to pack more efficiently.

[Ian Campbell]

On Wed, 2014-05-14 at 11:32 +0300, Siarhei Siamashka wrote:
> On Wed, 14 May 2014 11:01:59 +0300
> Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote:
> 
> > On Wed, 14 May 2014 08:52:50 +0100
> > Ian Campbell <ijc@hellion.org.uk> wrote:
> > 
> > > On Wed, 2014-05-14 at 10:44 +0300, Siarhei Siamashka wrote:
> > > > > diff --git a/drivers/net/designware.h b/drivers/net/designware.h
> > > > > index 382b0c7..6d94b3a 100644
> > > > > --- a/drivers/net/designware.h
> > > > > +++ b/drivers/net/designware.h
> > > > > @@ -215,13 +215,13 @@ struct dmamacdescr {
> > > > >  #endif
> > > > >  
> > > > >  struct dw_eth_dev {
> > > > > +	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
> > > > > +	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
> > > > > +
> > > > >  	u32 interface;
> > > > >  	u32 tx_currdescnum;
> > > > >  	u32 rx_currdescnum;
> > > > >  
> > > > > -	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
> > > > > -	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
> > > > > -
> > > > >  	char txbuffs[TX_TOTAL_BUFSIZE];
> > > > >  	char rxbuffs[RX_TOTAL_BUFSIZE];
> > > > 
> > > > After this reordering, txbuffs and rxbuffs buffers become DMA unaligned.
> > > 
> > > Right, this is fixed in the repost which is part of "[PATCH v3 0/5]
> > > net/designware: fixes for data cache, phylib and burst size".
> > 
> > Oh, you just forgot to add the linux-sunxi list to CC when sending
> > the updated patches. This explains why I don't see them in my mailbox.
> 
> However, as I can see at
>     http://lists.denx.de/pipermail/u-boot/2014-May/179218.html
> you are still sandwiching interface, tx_currdescnum and rx_currdescnum
> between the DMA aligned stuff. This does not really improve packing.

Yes, I thought I had also moved them, perhaps I forgot to refresh the
patch before git send-email.

I'll take a look and resend (not today thought probably).

Ian.

[U-Boot] [linux-sunxi] [PATCH 3/2] net/designware: reorder struct dw_eth_dev to pack more efficiently.

[Marek Vasut]

On Wednesday, May 14, 2014 at 10:01:59 AM, Siarhei Siamashka wrote:
> On Wed, 14 May 2014 08:52:50 +0100
> 
> Ian Campbell <ijc@hellion.org.uk> wrote:
> > On Wed, 2014-05-14 at 10:44 +0300, Siarhei Siamashka wrote:
> > > > diff --git a/drivers/net/designware.h b/drivers/net/designware.h
> > > > index 382b0c7..6d94b3a 100644
> > > > --- a/drivers/net/designware.h
> > > > +++ b/drivers/net/designware.h
> > > > @@ -215,13 +215,13 @@ struct dmamacdescr {
> > > > 
> > > >  #endif
> > > >  
> > > >  struct dw_eth_dev {
> > > > 
> > > > +	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
> > > > +	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
> > > > +
> > > > 
> > > >  	u32 interface;
> > > >  	u32 tx_currdescnum;
> > > >  	u32 rx_currdescnum;
> > > > 
> > > > -	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
> > > > -	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
> > > > -
> > > > 
> > > >  	char txbuffs[TX_TOTAL_BUFSIZE];
> > > >  	char rxbuffs[RX_TOTAL_BUFSIZE];
> > > 
> > > After this reordering, txbuffs and rxbuffs buffers become DMA
> > > unaligned.
> > 
> > Right, this is fixed in the repost which is part of "[PATCH v3 0/5]
> > net/designware: fixes for data cache, phylib and burst size".
> 
> Oh, you just forgot to add the linux-sunxi list to CC when sending
> the updated patches. This explains why I don't see them in my mailbox.

Well yes, U-Boot patches usually don't go into Linux MLs ;-)

Best regards,
Marek Vasut