| Message ID | 20200730154545.3965-3-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dmaengine: dw: Introduce non-mem peripherals optimizations | expand |
On Thu, Jul 30, 2020 at 06:45:42PM +0300, Serge Semin wrote: > CFGx.FIFO_MODE field controls a DMA-controller "FIFO readiness" criterion. > In other words it determines when to start pushing data out of a DW > DMAC channel FIFO to a destination peripheral or from a source > peripheral to the DW DMAC channel FIFO. Currently FIFO-mode is set to one > for all DW DMAC channels. It means they are tuned to flush data out of > FIFO (to a memory peripheral or by accepting the burst transaction > requests) when FIFO is at least half-full (except at the end of the block > transfer, when FIFO-flush mode is activated) and are configured to get > data to the FIFO when it's at least half-empty. > > Such configuration is a good choice when there is no slave device involved > in the DMA transfers. In that case the number of bursts per block is less > than when CFGx.FIFO_MODE = 0 and, hence, the bus utilization will improve. > But the latency of DMA transfers may increase when CFGx.FIFO_MODE = 1, > since DW DMAC will wait for the channel FIFO contents to be either > half-full or half-empty depending on having the destination or the source > transfers. Such latencies might be dangerous in case if the DMA transfers > are expected to be performed from/to a slave device. Since normally > peripheral devices keep data in internal FIFOs, any latency at some > critical moment may cause one being overflown and consequently losing > data. This especially concerns a case when either a peripheral device is > relatively fast or the DW DMAC engine is relatively slow with respect to > the incoming data pace. > > In order to solve problems, which might be caused by the latencies > described above, let's enable the FIFO half-full/half-empty "FIFO > readiness" criterion only for DMA transfers with no slave device involved. > Thanks to the commit ???????????? ("dmaengine: dw: Initialize channel See below. > before each transfer") we can freely do that in the generic > dw_dma_initialize_chan() method. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Thanks! > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > Note the DMA-engine repository git.infradead.org/users/vkoul/slave-dma.git > isn't accessible. So I couldn't find out the Andy' commit hash to use it in > the log. It's dmaengine.git on git.kernel.org. > --- > drivers/dma/dw/dw.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma/dw/dw.c b/drivers/dma/dw/dw.c > index 7a085b3c1854..d9810980920a 100644 > --- a/drivers/dma/dw/dw.c > +++ b/drivers/dma/dw/dw.c > @@ -14,7 +14,7 @@ > static void dw_dma_initialize_chan(struct dw_dma_chan *dwc) > { > struct dw_dma *dw = to_dw_dma(dwc->chan.device); > - u32 cfghi = DWC_CFGH_FIFO_MODE; > + u32 cfghi = is_slave_direction(dwc->direction) ? 0 : DWC_CFGH_FIFO_MODE; > u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority); > bool hs_polarity = dwc->dws.hs_polarity; > > -- > 2.27.0 >
On Thu, Jul 30, 2020 at 07:24:28PM +0300, Andy Shevchenko wrote: > On Thu, Jul 30, 2020 at 06:45:42PM +0300, Serge Semin wrote: > > CFGx.FIFO_MODE field controls a DMA-controller "FIFO readiness" criterion. > > In other words it determines when to start pushing data out of a DW > > DMAC channel FIFO to a destination peripheral or from a source > > peripheral to the DW DMAC channel FIFO. Currently FIFO-mode is set to one > > for all DW DMAC channels. It means they are tuned to flush data out of > > FIFO (to a memory peripheral or by accepting the burst transaction > > requests) when FIFO is at least half-full (except at the end of the block > > transfer, when FIFO-flush mode is activated) and are configured to get > > data to the FIFO when it's at least half-empty. > > > > Such configuration is a good choice when there is no slave device involved > > in the DMA transfers. In that case the number of bursts per block is less > > than when CFGx.FIFO_MODE = 0 and, hence, the bus utilization will improve. > > But the latency of DMA transfers may increase when CFGx.FIFO_MODE = 1, > > since DW DMAC will wait for the channel FIFO contents to be either > > half-full or half-empty depending on having the destination or the source > > transfers. Such latencies might be dangerous in case if the DMA transfers > > are expected to be performed from/to a slave device. Since normally > > peripheral devices keep data in internal FIFOs, any latency at some > > critical moment may cause one being overflown and consequently losing > > data. This especially concerns a case when either a peripheral device is > > relatively fast or the DW DMAC engine is relatively slow with respect to > > the incoming data pace. > > > > In order to solve problems, which might be caused by the latencies > > described above, let's enable the FIFO half-full/half-empty "FIFO > > readiness" criterion only for DMA transfers with no slave device involved. > > > Thanks to the commit ???????????? ("dmaengine: dw: Initialize channel > > See below. > > > before each transfer") we can freely do that in the generic > > dw_dma_initialize_chan() method. > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Thanks! > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > --- > > > > Note the DMA-engine repository git.infradead.org/users/vkoul/slave-dma.git > > isn't accessible. So I couldn't find out the Andy' commit hash to use it in > > the log. > > It's dmaengine.git on git.kernel.org. Ah, thanks! I've just found out that the repo address has been changed. But I've also scanned the "next" branch of the repo: https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git Your commit isn't there. Am I missing something? -Sergey > > > --- > > drivers/dma/dw/dw.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/dma/dw/dw.c b/drivers/dma/dw/dw.c > > index 7a085b3c1854..d9810980920a 100644 > > --- a/drivers/dma/dw/dw.c > > +++ b/drivers/dma/dw/dw.c > > @@ -14,7 +14,7 @@ > > static void dw_dma_initialize_chan(struct dw_dma_chan *dwc) > > { > > struct dw_dma *dw = to_dw_dma(dwc->chan.device); > > - u32 cfghi = DWC_CFGH_FIFO_MODE; > > + u32 cfghi = is_slave_direction(dwc->direction) ? 0 : DWC_CFGH_FIFO_MODE; > > u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority); > > bool hs_polarity = dwc->dws.hs_polarity; > > > > -- > > 2.27.0 > > > > -- > With Best Regards, > Andy Shevchenko > >
On Thu, Jul 30, 2020 at 07:31:54PM +0300, Serge Semin wrote: > On Thu, Jul 30, 2020 at 07:24:28PM +0300, Andy Shevchenko wrote: > > On Thu, Jul 30, 2020 at 06:45:42PM +0300, Serge Semin wrote: ... > > > Thanks to the commit ???????????? ("dmaengine: dw: Initialize channel ... > > > Note the DMA-engine repository git.infradead.org/users/vkoul/slave-dma.git > > > isn't accessible. So I couldn't find out the Andy' commit hash to use it in > > > the log. > > > It's dmaengine.git on git.kernel.org. > > Ah, thanks! I've just found out that the repo address has been changed. But I've > also scanned the "next" branch of the repo: > https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git > > Your commit isn't there. Am I missing something? It's a fix. It went to upstream branch (don't remember its name by heart in Vinod's repo).
On Thu, Jul 30, 2020 at 07:47:03PM +0300, Andy Shevchenko wrote: > On Thu, Jul 30, 2020 at 07:31:54PM +0300, Serge Semin wrote: > > On Thu, Jul 30, 2020 at 07:24:28PM +0300, Andy Shevchenko wrote: > > > On Thu, Jul 30, 2020 at 06:45:42PM +0300, Serge Semin wrote: > > ... > > > > > Thanks to the commit ???????????? ("dmaengine: dw: Initialize channel > > ... > > > > > Note the DMA-engine repository git.infradead.org/users/vkoul/slave-dma.git > > > > isn't accessible. So I couldn't find out the Andy' commit hash to use it in > > > > the log. > > > > > It's dmaengine.git on git.kernel.org. > > > > Ah, thanks! I've just found out that the repo address has been changed. But I've > > also scanned the "next" branch of the repo: > > https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git > > > > Your commit isn't there. Am I missing something? > > It's a fix. It went to upstream branch (don't remember its name by heart in > Vinod's repo). Right. Found it. Thanks. -Sergey > > -- > With Best Regards, > Andy Shevchenko > >
On 30-07-20, 20:13, Serge Semin wrote: > On Thu, Jul 30, 2020 at 07:47:03PM +0300, Andy Shevchenko wrote: > > On Thu, Jul 30, 2020 at 07:31:54PM +0300, Serge Semin wrote: > > > On Thu, Jul 30, 2020 at 07:24:28PM +0300, Andy Shevchenko wrote: > > > > On Thu, Jul 30, 2020 at 06:45:42PM +0300, Serge Semin wrote: > > > > ... > > > > > > > Thanks to the commit ???????????? ("dmaengine: dw: Initialize channel > > > > ... > > > > > > > Note the DMA-engine repository git.infradead.org/users/vkoul/slave-dma.git > > > > > isn't accessible. So I couldn't find out the Andy' commit hash to use it in > > > > > the log. Yeah I moved tree to k.org after disk issue with infradead, change patch was on dmaengine ML > > > > It's dmaengine.git on git.kernel.org. > > > > > > Ah, thanks! I've just found out that the repo address has been changed. But I've > > > also scanned the "next" branch of the repo: > > > https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git > > > > > > Your commit isn't there. Am I missing something? > > > > > It's a fix. It went to upstream branch (don't remember its name by heart in > > Vinod's repo). Yes it is Linus's tree now and in dmaengine you can find in fixes branch https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git/commit/?h=fixes&id=99ba8b9b0d9780e9937eb1d488d120e9e5c2533d > > Right. Found it. Thanks. > > -Sergey > > > > > -- > > With Best Regards, > > Andy Shevchenko > > > >
On Fri, Jul 31, 2020 at 10:22:54PM +0530, Vinod Koul wrote: > On 30-07-20, 20:13, Serge Semin wrote: > > On Thu, Jul 30, 2020 at 07:47:03PM +0300, Andy Shevchenko wrote: > > > On Thu, Jul 30, 2020 at 07:31:54PM +0300, Serge Semin wrote: > > > > On Thu, Jul 30, 2020 at 07:24:28PM +0300, Andy Shevchenko wrote: > > > > > On Thu, Jul 30, 2020 at 06:45:42PM +0300, Serge Semin wrote: > > > > > > ... > > > > > > > > > Thanks to the commit ???????????? ("dmaengine: dw: Initialize channel > > > > > > ... > > > > > > > > > Note the DMA-engine repository git.infradead.org/users/vkoul/slave-dma.git > > > > > > isn't accessible. So I couldn't find out the Andy' commit hash to use it in > > > > > > the log. > > Yeah I moved tree to k.org after disk issue with infradead, change patch > was on dmaengine ML > > > > > > It's dmaengine.git on git.kernel.org. > > > > > > > > Ah, thanks! I've just found out that the repo address has been changed. But I've > > > > also scanned the "next" branch of the repo: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git > > > > > > > > Your commit isn't there. Am I missing something? > > > > > > > > It's a fix. It went to upstream branch (don't remember its name by heart in > > > Vinod's repo). > > Yes it is Linus's tree now and in dmaengine you can find in fixes branch > > https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git/commit/?h=fixes&id=99ba8b9b0d9780e9937eb1d488d120e9e5c2533d Thanks for pointing out to the commit. I'll add the hash to the patch log and resend the series shortly. -Sergey > > > > > Right. Found it. Thanks. > > > > -Sergey > > > > > > > > -- > > > With Best Regards, > > > Andy Shevchenko > > > > > > > > -- > ~Vinod
diff --git a/drivers/dma/dw/dw.c b/drivers/dma/dw/dw.c index 7a085b3c1854..d9810980920a 100644 --- a/drivers/dma/dw/dw.c +++ b/drivers/dma/dw/dw.c @@ -14,7 +14,7 @@ static void dw_dma_initialize_chan(struct dw_dma_chan *dwc) { struct dw_dma *dw = to_dw_dma(dwc->chan.device); - u32 cfghi = DWC_CFGH_FIFO_MODE; + u32 cfghi = is_slave_direction(dwc->direction) ? 0 : DWC_CFGH_FIFO_MODE; u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority); bool hs_polarity = dwc->dws.hs_polarity;
CFGx.FIFO_MODE field controls a DMA-controller "FIFO readiness" criterion. In other words it determines when to start pushing data out of a DW DMAC channel FIFO to a destination peripheral or from a source peripheral to the DW DMAC channel FIFO. Currently FIFO-mode is set to one for all DW DMAC channels. It means they are tuned to flush data out of FIFO (to a memory peripheral or by accepting the burst transaction requests) when FIFO is at least half-full (except at the end of the block transfer, when FIFO-flush mode is activated) and are configured to get data to the FIFO when it's at least half-empty. Such configuration is a good choice when there is no slave device involved in the DMA transfers. In that case the number of bursts per block is less than when CFGx.FIFO_MODE = 0 and, hence, the bus utilization will improve. But the latency of DMA transfers may increase when CFGx.FIFO_MODE = 1, since DW DMAC will wait for the channel FIFO contents to be either half-full or half-empty depending on having the destination or the source transfers. Such latencies might be dangerous in case if the DMA transfers are expected to be performed from/to a slave device. Since normally peripheral devices keep data in internal FIFOs, any latency at some critical moment may cause one being overflown and consequently losing data. This especially concerns a case when either a peripheral device is relatively fast or the DW DMAC engine is relatively slow with respect to the incoming data pace. In order to solve problems, which might be caused by the latencies described above, let's enable the FIFO half-full/half-empty "FIFO readiness" criterion only for DMA transfers with no slave device involved. Thanks to the commit ???????????? ("dmaengine: dw: Initialize channel before each transfer") we can freely do that in the generic dw_dma_initialize_chan() method. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> --- Note the DMA-engine repository git.infradead.org/users/vkoul/slave-dma.git isn't accessible. So I couldn't find out the Andy' commit hash to use it in the log. --- drivers/dma/dw/dw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)