| Message ID | 1390409349-9406-1-git-send-email-abrodkin@synopsys.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Alexey, On 22.01.2014 17:49, Alexey Brodkin wrote: > From: Alexey Brodkin <Alexey.Brodkin@synopsys.com> > > Up until now this driver only worked with data cache disabled. > To make it work with enabled data cache following changes were required: > > * Flush Tx/Rx buffer descriptors their modification > * Invalidate Tx/Rx buffer descriptors before reading its values > * Flush cache for data passed from CPU to GMAC > * Invalidate cache for data passed from GMAC to CPU > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> A small nitpicking comment below. > Cc: Joe Hershberger <joe.hershberger@ni.com> > Cc: Vipin Kumar <vipin.kumar@st.com> > Cc: Stefan Roese <sr@denx.de> > Cc: Mischa Jonker <mjonker@synopsys.com> > Cc: Shiraz Hashim <shiraz.hashim@st.com> > Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> > Cc: Amit Virdi <amit.virdi@st.com> > Cc: Sonic Zhang <sonic.zhang@analog.com> > > Compared to v2: > 1. Removed trailing white space > --- > drivers/net/designware.c | 53 +++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 50 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/designware.c b/drivers/net/designware.c > index 22155b4..c0c8659 100644 > --- a/drivers/net/designware.c > +++ b/drivers/net/designware.c > @@ -51,6 +51,11 @@ static void tx_descs_init(struct eth_device *dev) > /* Correcting the last pointer of the chain */ > desc_p->dmamac_next = &desc_table_p[0]; > > + /* Flush all Tx buffer descriptors at once */ > + flush_dcache_range((unsigned int)priv->tx_mac_descrtable, > + (unsigned int)priv->tx_mac_descrtable + > + sizeof(priv->tx_mac_descrtable)); > + > writel((ulong)&desc_table_p[0], &dma_p->txdesclistaddr); > } > > @@ -63,6 +68,15 @@ static void rx_descs_init(struct eth_device *dev) > struct dmamacdescr *desc_p; > u32 idx; > > + /* Before passing buffers to GMAC we need to make sure zeros > + * written there right after "priv" structure allocation were > + * flushed into RAM. > + * Otherwise there's a chance to get some of them flushed in RAM when > + * GMAC is already pushing data to RAM via DMA. This way incoming from > + * GMAC data will be corrupted. */ Please use this recommended multi-line comment style: /* * Before ... * ... * * ... will be corrupted. */ Thanks, Stefan
Hello Alexey, In general, a very nice, clean patch. > + /* Flush modified buffer descriptor */ > + flush_dcache_range((unsigned long)desc_p, > + (unsigned long)desc_p + sizeof(struct dmamacdescr)); > + If I remember correctly, there is some bit that tells you if the DMA descriptor is owned by CPU or by GMAC. What if the descriptor size is smaller than the cache line size of the CPU? How do you prevent the CPU from overwriting adiacent DMA descriptors that may be owned by the GMAC? As far as I can remember, in Linux they solve this by mapping the descriptors (not the packet buffers, these are always cacheline aligned) in uncached memory, but we cannot do that in u-boot as the MMU is still disabled. OTOH, as we may not need to have the performance benefits of the CPU and GMAC concurrently accessing the descriptor table, we may be able to work around it by handing off multiple descriptors at once from GMAC to CPU and vice versa (maybe depending on cache line size?). I remember that a similar patch (that looked a lot uglier BTW) solved it by doing uncached accesses to the descriptors, but that would require using arch-specific accessor macro's (and I'm not sure if all architectures support an 'uncached access' attribute/flag with load/store instructions). Mischa
Hi Mischa, On Wed, 2014-01-22 at 17:10 +0000, Mischa Jonker wrote: > Hello Alexey, > > In general, a very nice, clean patch. > > > + /* Flush modified buffer descriptor */ > > + flush_dcache_range((unsigned long)desc_p, > > + (unsigned long)desc_p + sizeof(struct dmamacdescr)); > > + > > If I remember correctly, there is some bit that tells you if the DMA descriptor is owned by CPU or by GMAC. What if the descriptor size is smaller than the cache line size of the CPU? How do you prevent the CPU from overwriting adiacent DMA descriptors that may be owned by the GMAC? Well, good point. It might happen with long cache line easily. > As far as I can remember, in Linux they solve this by mapping the descriptors (not the packet buffers, these are always cacheline aligned) in uncached memory, but we cannot do that in u-boot as the MMU is still disabled. OTOH, as we may not need to have the performance benefits of the CPU and GMAC concurrently accessing the descriptor table, we may be able to work around it by handing off multiple descriptors at once from GMAC to CPU and vice versa (maybe depending on cache line size?). Frankly I don't see a point in trying to process few descriptors at once. The only true failure safe scenario I may think of - CPU waits until all descriptors are processed by GMAC and then CPU processes all Tx or Rx descriptors. In this case CPU won't corrupt the next descriptor used currently by CPU. But this approach will kill benefits of multiple buffers - operation will become synchronous as if we have only 1 buffer for Tx and one for Rx. Which might be a resolution but with penalty of speed/time (which we don't want). > I remember that a similar patch (that looked a lot uglier BTW) solved it by doing uncached accesses to the descriptors, but that would require using arch-specific accessor macro's (and I'm not sure if all architectures support an 'uncached access' attribute/flag with load/store instructions). Word/byte-aligned uncached access is sweet but not universal - how many arches have it? As a summary - this problem is very generic (I saw other places like DW MMC driver which use similar approach - flushing/invalidating buffer descriptors) and without true uncached access (be it with means of MMU or specific accessors without MMU) it's not clear how to act completely fail-safe. Might be we need just to add compile-time warning for cases when cache line is longer than one unit of interest (buffer descriptor in our case) so people at least will be informed and suggest to disable D$ for starters. -Alexey
On Wed, Jan 22, 2014 at 08:49:09PM +0400, Alexey Brodkin wrote: > From: Alexey Brodkin <Alexey.Brodkin@synopsys.com> > > Up until now this driver only worked with data cache disabled. > To make it work with enabled data cache following changes were required: > > * Flush Tx/Rx buffer descriptors their modification > * Invalidate Tx/Rx buffer descriptors before reading its values > * Flush cache for data passed from CPU to GMAC > * Invalidate cache for data passed from GMAC to CPU > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> > > Cc: Joe Hershberger <joe.hershberger@ni.com> > Cc: Vipin Kumar <vipin.kumar@st.com> > Cc: Stefan Roese <sr@denx.de> > Cc: Mischa Jonker <mjonker@synopsys.com> > Cc: Shiraz Hashim <shiraz.hashim@st.com> > Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> > Cc: Amit Virdi <amit.virdi@st.com> > Cc: Sonic Zhang <sonic.zhang@analog.com> Applied to u-boot/master, thanks!
diff --git a/drivers/net/designware.c b/drivers/net/designware.c index 22155b4..c0c8659 100644 --- a/drivers/net/designware.c +++ b/drivers/net/designware.c @@ -51,6 +51,11 @@ static void tx_descs_init(struct eth_device *dev) /* Correcting the last pointer of the chain */ desc_p->dmamac_next = &desc_table_p[0]; + /* Flush all Tx buffer descriptors at once */ + flush_dcache_range((unsigned int)priv->tx_mac_descrtable, + (unsigned int)priv->tx_mac_descrtable + + sizeof(priv->tx_mac_descrtable)); + writel((ulong)&desc_table_p[0], &dma_p->txdesclistaddr); } @@ -63,6 +68,15 @@ static void rx_descs_init(struct eth_device *dev) struct dmamacdescr *desc_p; u32 idx; + /* Before passing buffers to GMAC we need to make sure zeros + * written there right after "priv" structure allocation were + * flushed into RAM. + * Otherwise there's a chance to get some of them flushed in RAM when + * GMAC is already pushing data to RAM via DMA. This way incoming from + * GMAC data will be corrupted. */ + flush_dcache_range((unsigned int)rxbuffs, (unsigned int)rxbuffs + + RX_TOTAL_BUFSIZE); + for (idx = 0; idx < CONFIG_RX_DESCR_NUM; idx++) { desc_p = &desc_table_p[idx]; desc_p->dmamac_addr = &rxbuffs[idx * CONFIG_ETH_BUFSIZE]; @@ -78,6 +92,11 @@ static void rx_descs_init(struct eth_device *dev) /* Correcting the last pointer of the chain */ desc_p->dmamac_next = &desc_table_p[0]; + /* Flush all Rx buffer descriptors at once */ + flush_dcache_range((unsigned int)priv->rx_mac_descrtable, + (unsigned int)priv->rx_mac_descrtable + + sizeof(priv->rx_mac_descrtable)); + writel((ulong)&desc_table_p[0], &dma_p->rxdesclistaddr); } @@ -197,6 +216,11 @@ 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)); + /* Check if the descriptor is owned by CPU */ if (desc_p->txrx_status & DESC_TXSTS_OWNBYDMA) { printf("CPU not owner of tx frame\n"); @@ -205,6 +229,10 @@ static int dw_eth_send(struct eth_device *dev, void *packet, int length) memcpy((void *)desc_p->dmamac_addr, packet, length); + /* Flush data to be sent */ + flush_dcache_range((unsigned long)desc_p->dmamac_addr, + (unsigned long)desc_p->dmamac_addr + length); + #if defined(CONFIG_DW_ALTDESCRIPTOR) desc_p->txrx_status |= DESC_TXSTS_TXFIRST | DESC_TXSTS_TXLAST; desc_p->dmamac_cntl |= (length << DESC_TXCTRL_SIZE1SHFT) & \ @@ -220,6 +248,10 @@ static int dw_eth_send(struct eth_device *dev, void *packet, int length) desc_p->txrx_status = DESC_TXSTS_OWNBYDMA; #endif + /* Flush modified buffer descriptor */ + flush_dcache_range((unsigned long)desc_p, + (unsigned long)desc_p + sizeof(struct dmamacdescr)); + /* Test the wrap-around condition. */ if (++desc_num >= CONFIG_TX_DESCR_NUM) desc_num = 0; @@ -235,18 +267,28 @@ static int dw_eth_send(struct eth_device *dev, void *packet, int length) static int dw_eth_recv(struct eth_device *dev) { struct dw_eth_dev *priv = dev->priv; - u32 desc_num = priv->rx_currdescnum; + u32 status, desc_num = priv->rx_currdescnum; struct dmamacdescr *desc_p = &priv->rx_mac_descrtable[desc_num]; - - u32 status = desc_p->txrx_status; int length = 0; + /* Invalidate entire buffer descriptor */ + invalidate_dcache_range((unsigned long)desc_p, + (unsigned long)desc_p + + sizeof(struct dmamacdescr)); + + status = desc_p->txrx_status; + /* Check if the owner is the CPU */ if (!(status & DESC_RXSTS_OWNBYDMA)) { length = (status & DESC_RXSTS_FRMLENMSK) >> \ DESC_RXSTS_FRMLENSHFT; + /* Invalidate received data */ + invalidate_dcache_range((unsigned long)desc_p->dmamac_addr, + (unsigned long)desc_p->dmamac_addr + + length); + NetReceive(desc_p->dmamac_addr, length); /* @@ -255,6 +297,11 @@ static int dw_eth_recv(struct eth_device *dev) */ desc_p->txrx_status |= DESC_RXSTS_OWNBYDMA; + /* Flush only status field - others weren't changed */ + flush_dcache_range((unsigned long)&desc_p->txrx_status, + (unsigned long)&desc_p->txrx_status + + sizeof(desc_p->txrx_status)); + /* Test the wrap-around condition. */ if (++desc_num >= CONFIG_RX_DESCR_NUM) desc_num = 0;