| Message ID | 1398970918.19277.181.camel@hastur.hellion.org.uk |
|---|---|
| State | Superseded |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
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
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 :-)
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.
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.
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.
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.
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
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];
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(-)