| Message ID | 20200515104758.6934-11-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: dw: Add generic DW DMA controller support | expand |
On Fri, May 15, 2020 at 01:47:49PM +0300, Serge Semin wrote: > Each channel of DMA controller may have a limited length of burst > transaction (number of IO operations performed at ones in a single > DMA client request). This parameter can be used to setup the most > optimal DMA Tx/Rx data level values. In order to avoid the Tx buffer > overrun we can set the DMA Tx level to be of FIFO depth minus the > maximum burst transactions length. To prevent the Rx buffer underflow > the DMA Rx level should be set to the maximum burst transactions length. > This commit setups the DMA channels and the DW SPI DMA Tx/Rx levels > in accordance with these rules. It's good one, but see my comments. I think this patch should go before previous one. (and without changes regarding FIFO length) > +static void mid_spi_maxburst_init(struct dw_spi *dws) > +{ > + struct dma_slave_caps caps; > + u32 max_burst, def_burst; > + int ret; > + > + def_burst = dws->fifo_len / 2; > + > + ret = dma_get_slave_caps(dws->rxchan, &caps); > + if (!ret && caps.max_burst) > + max_burst = caps.max_burst; > + else > + max_burst = RX_BURST_LEVEL; > + > + dws->rxburst = (def_burst > max_burst) ? max_burst : def_burst; min() ? > + > + ret = dma_get_slave_caps(dws->txchan, &caps); > + if (!ret && caps.max_burst) > + max_burst = caps.max_burst; > + else > + max_burst = TX_BURST_LEVEL; > + > + dws->txburst = (def_burst > max_burst) ? max_burst : def_burst; Ditto. > +} > + > static int mid_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) > { > struct dw_dma_slave slave = {0}; > @@ -67,6 +92,8 @@ static int mid_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) > dws->master->dma_rx = dws->rxchan; > dws->master->dma_tx = dws->txchan; > > + mid_spi_maxburst_init(dws); > + > return 0; > > free_rxchan: > @@ -92,6 +119,8 @@ static int mid_spi_dma_init_generic(struct device *dev, struct dw_spi *dws) > dws->master->dma_rx = dws->rxchan; > dws->master->dma_tx = dws->txchan; > > + mid_spi_maxburst_init(dws); > + > return 0; > } > > @@ -195,7 +224,7 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_tx(struct dw_spi *dws, > memset(&txconf, 0, sizeof(txconf)); > txconf.direction = DMA_MEM_TO_DEV; > txconf.dst_addr = dws->dma_addr; > - txconf.dst_maxburst = TX_BURST_LEVEL; > + txconf.dst_maxburst = dws->txburst; > txconf.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > txconf.dst_addr_width = convert_dma_width(dws->n_bytes); > txconf.device_fc = false; > @@ -268,7 +297,7 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_rx(struct dw_spi *dws, > memset(&rxconf, 0, sizeof(rxconf)); > rxconf.direction = DMA_DEV_TO_MEM; > rxconf.src_addr = dws->dma_addr; > - rxconf.src_maxburst = RX_BURST_LEVEL; > + rxconf.src_maxburst = dws->rxburst; > rxconf.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > rxconf.src_addr_width = convert_dma_width(dws->n_bytes); > rxconf.device_fc = false; > @@ -293,8 +322,8 @@ static int mid_spi_dma_setup(struct dw_spi *dws, struct spi_transfer *xfer) > { > u16 imr = 0, dma_ctrl = 0; > > - dw_writel(dws, DW_SPI_DMARDLR, RX_BURST_LEVEL - 1); > - dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - TX_BURST_LEVEL); > + dw_writel(dws, DW_SPI_DMARDLR, dws->rxburst - 1); > + dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst); > > if (xfer->tx_buf) { > dma_ctrl |= SPI_DMA_TDMAE; ... > /* DMA info */ > struct dma_chan *txchan; > + u32 txburst; > struct dma_chan *rxchan; > + u32 rxburst; Leave u32 together, it may be optimal on 64-bit architectures where ABIs require padding.
On Sat, May 16, 2020 at 11:01:33PM +0300, Serge Semin wrote: > On Fri, May 15, 2020 at 05:38:42PM +0300, Andy Shevchenko wrote: > > On Fri, May 15, 2020 at 01:47:49PM +0300, Serge Semin wrote: > > > Each channel of DMA controller may have a limited length of burst > > > transaction (number of IO operations performed at ones in a single > > > DMA client request). This parameter can be used to setup the most > > > optimal DMA Tx/Rx data level values. In order to avoid the Tx buffer > > > overrun we can set the DMA Tx level to be of FIFO depth minus the > > > maximum burst transactions length. To prevent the Rx buffer underflow > > > the DMA Rx level should be set to the maximum burst transactions length. > > > This commit setups the DMA channels and the DW SPI DMA Tx/Rx levels > > > in accordance with these rules. ... > > > /* DMA info */ > > > struct dma_chan *txchan; > > > + u32 txburst; > > > struct dma_chan *rxchan; > > > + u32 rxburst; > > > > Leave u32 together, it may be optimal on 64-bit architectures where ABIs require padding. > > It's not like anyone cared about padding in this structure in the first place) I think I have been caring (to some extend). > Though if v3 is required I'll group these members together. From what I see v3 is what Mark and me are waiting for. Mark, are we on the same page here?
On Mon, May 18, 2020 at 02:03:43PM +0300, Andy Shevchenko wrote: > On Sat, May 16, 2020 at 11:01:33PM +0300, Serge Semin wrote: > > Though if v3 is required I'll group these members together. > From what I see v3 is what Mark and me are waiting for. Mark, are we on the > same page here? I loose track of what's going on with this specific patch, sorry - it's patch 10 in the series so there's a good chance it has dependencies on some of the earlier changes anyway.
On Mon, May 18, 2020 at 02:03:43PM +0300, Andy Shevchenko wrote: > On Sat, May 16, 2020 at 11:01:33PM +0300, Serge Semin wrote: > > On Fri, May 15, 2020 at 05:38:42PM +0300, Andy Shevchenko wrote: > > > On Fri, May 15, 2020 at 01:47:49PM +0300, Serge Semin wrote: > > > > Each channel of DMA controller may have a limited length of burst > > > > transaction (number of IO operations performed at ones in a single > > > > DMA client request). This parameter can be used to setup the most > > > > optimal DMA Tx/Rx data level values. In order to avoid the Tx buffer > > > > overrun we can set the DMA Tx level to be of FIFO depth minus the > > > > maximum burst transactions length. To prevent the Rx buffer underflow > > > > the DMA Rx level should be set to the maximum burst transactions length. > > > > This commit setups the DMA channels and the DW SPI DMA Tx/Rx levels > > > > in accordance with these rules. > > ... > > > > > /* DMA info */ > > > > struct dma_chan *txchan; > > > > + u32 txburst; > > > > struct dma_chan *rxchan; > > > > + u32 rxburst; > > > > > > Leave u32 together, it may be optimal on 64-bit architectures where ABIs require padding. > > > > It's not like anyone cared about padding in this structure in the first place) > > I think I have been caring (to some extend). Well, If you have then instead of asking to rearrange just two members (which by the way finely grouped by the Tx-Rx affiliation) why not sending a patch, which would refactor the whole structure so to be optimal for the x64 platforms? I don't really see why this gets very important for you seeing Mark is Ok with this. My current commit follows the common driver design including the DW SSI data members grouping. On the second thought I'll leave it as is then. -Sergey > > > Though if v3 is required I'll group these members together. > > From what I see v3 is what Mark and me are waiting for. Mark, are we on the > same page here? > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, May 18, 2020 at 3:53 PM Serge Semin <Sergey.Semin@baikalelectronics.ru> wrote: > On Mon, May 18, 2020 at 02:03:43PM +0300, Andy Shevchenko wrote: > > On Sat, May 16, 2020 at 11:01:33PM +0300, Serge Semin wrote: > > > On Fri, May 15, 2020 at 05:38:42PM +0300, Andy Shevchenko wrote: > > > > On Fri, May 15, 2020 at 01:47:49PM +0300, Serge Semin wrote: ... > > > > > struct dma_chan *txchan; > > > > > + u32 txburst; > > > > > struct dma_chan *rxchan; > > > > > + u32 rxburst; > > > > > > > > Leave u32 together, it may be optimal on 64-bit architectures where ABIs require padding. > > > > > > It's not like anyone cared about padding in this structure in the first place) > > > > I think I have been caring (to some extend). > > Well, If you have then instead of asking to rearrange just two members (which > by the way finely grouped by the Tx-Rx affiliation) why not sending a > patch, which would refactor the whole structure so to be optimal for the x64 > platforms? I don't really see why this gets very important for you seeing > Mark is Ok with this. My current commit follows the common driver design > including the DW SSI data members grouping. On the second thought I'll leave > it as is then. Again same issue here. What is really easy to do for you here, will become a burden and additional churn to anybody else. So, why not to minimize it in the first place? Same with comma in another patch. Sorry, I really don't get it.
On Mon, May 18, 2020 at 04:25:20PM +0300, Andy Shevchenko wrote: > On Mon, May 18, 2020 at 3:53 PM Serge Semin > <Sergey.Semin@baikalelectronics.ru> wrote: > > On Mon, May 18, 2020 at 02:03:43PM +0300, Andy Shevchenko wrote: > > > On Sat, May 16, 2020 at 11:01:33PM +0300, Serge Semin wrote: > > > > On Fri, May 15, 2020 at 05:38:42PM +0300, Andy Shevchenko wrote: > > > > > On Fri, May 15, 2020 at 01:47:49PM +0300, Serge Semin wrote: > > ... > > > > > > > struct dma_chan *txchan; > > > > > > + u32 txburst; > > > > > > struct dma_chan *rxchan; > > > > > > + u32 rxburst; > > > > > > > > > > Leave u32 together, it may be optimal on 64-bit architectures where ABIs require padding. > > > > > > > > It's not like anyone cared about padding in this structure in the first place) > > > > > > I think I have been caring (to some extend). > > > > Well, If you have then instead of asking to rearrange just two members (which > > by the way finely grouped by the Tx-Rx affiliation) why not sending a > > patch, which would refactor the whole structure so to be optimal for the x64 > > platforms? I don't really see why this gets very important for you seeing > > Mark is Ok with this. My current commit follows the common driver design > > including the DW SSI data members grouping. On the second thought I'll leave > > it as is then. > > Again same issue here. What is really easy to do for you here, will > become a burden and additional churn to anybody else. > So, why not to minimize it in the first place? Same with comma in > another patch. Sorry, I really don't get it. If comma is more or less understandable (though adding it is absolutely redundant there and doesn't worth even a bit of time spending for the discussion), here you consider the patch from padding point of view. The driver developer didn't care about it, but did care about grouping the members in a corresponding way. The padding burden will be there anyway and should be fixed for the whole structure in an additional patch. Until then the way of grouping should be preserved. -Sergey > > -- > With Best Regards, > Andy Shevchenko
On Mon, May 18, 2020 at 04:43:06PM +0300, Serge Semin wrote: > On Mon, May 18, 2020 at 04:25:20PM +0300, Andy Shevchenko wrote: > > On Mon, May 18, 2020 at 3:53 PM Serge Semin > > <Sergey.Semin@baikalelectronics.ru> wrote: > > > On Mon, May 18, 2020 at 02:03:43PM +0300, Andy Shevchenko wrote: > > > > On Sat, May 16, 2020 at 11:01:33PM +0300, Serge Semin wrote: > > > > > On Fri, May 15, 2020 at 05:38:42PM +0300, Andy Shevchenko wrote: > > > > > > On Fri, May 15, 2020 at 01:47:49PM +0300, Serge Semin wrote: ... > > > > > It's not like anyone cared about padding in this structure in the first place) > > > > > > > > I think I have been caring (to some extend). > > > > > > Well, If you have then instead of asking to rearrange just two members (which > > > by the way finely grouped by the Tx-Rx affiliation) why not sending a > > > patch, which would refactor the whole structure so to be optimal for the x64 > > > platforms? I don't really see why this gets very important for you seeing > > > Mark is Ok with this. My current commit follows the common driver design > > > including the DW SSI data members grouping. On the second thought I'll leave > > > it as is then. > > > > Again same issue here. What is really easy to do for you here, will > > become a burden and additional churn to anybody else. > > So, why not to minimize it in the first place? Same with comma in > > another patch. Sorry, I really don't get it. > > If comma is more or less understandable (though adding it is absolutely > redundant there and doesn't worth even a bit of time spending for the > discussion), here you consider the patch from padding point of view. > The driver developer didn't care about it, but did care about grouping the > members in a corresponding way. The padding burden will be there anyway and > should be fixed for the whole structure in an additional patch. Until then > the way of grouping should be preserved. Like you said, we spent already much more time than that simple change can be satisfied. And like you said, "deleloper ... did care about groupping members in a corresponding way". So, if we look at this in the original code, my suggestion, besides padding benefit, is consistent with existing pattern in that data structure. Note, I agree on extern keyword change can be postponed (it was in the original code), but here you introduce a new code...
On Mon, May 18, 2020 at 05:48:34PM +0300, Andy Shevchenko wrote: > On Mon, May 18, 2020 at 04:43:06PM +0300, Serge Semin wrote: > > On Mon, May 18, 2020 at 04:25:20PM +0300, Andy Shevchenko wrote: > > > On Mon, May 18, 2020 at 3:53 PM Serge Semin > > > <Sergey.Semin@baikalelectronics.ru> wrote: > > > > On Mon, May 18, 2020 at 02:03:43PM +0300, Andy Shevchenko wrote: > > > > > On Sat, May 16, 2020 at 11:01:33PM +0300, Serge Semin wrote: > > > > > > On Fri, May 15, 2020 at 05:38:42PM +0300, Andy Shevchenko wrote: > > > > > > > On Fri, May 15, 2020 at 01:47:49PM +0300, Serge Semin wrote: > > ... > > > > > > > It's not like anyone cared about padding in this structure in the first place) > > > > > > > > > > I think I have been caring (to some extend). > > > > > > > > Well, If you have then instead of asking to rearrange just two members (which > > > > by the way finely grouped by the Tx-Rx affiliation) why not sending a > > > > patch, which would refactor the whole structure so to be optimal for the x64 > > > > platforms? I don't really see why this gets very important for you seeing > > > > Mark is Ok with this. My current commit follows the common driver design > > > > including the DW SSI data members grouping. On the second thought I'll leave > > > > it as is then. > > > > > > Again same issue here. What is really easy to do for you here, will > > > become a burden and additional churn to anybody else. > > > So, why not to minimize it in the first place? Same with comma in > > > another patch. Sorry, I really don't get it. > > > > If comma is more or less understandable (though adding it is absolutely > > redundant there and doesn't worth even a bit of time spending for the > > discussion), here you consider the patch from padding point of view. > > The driver developer didn't care about it, but did care about grouping the > > members in a corresponding way. The padding burden will be there anyway and > > should be fixed for the whole structure in an additional patch. Until then > > the way of grouping should be preserved. > > Like you said, we spent already much more time than that simple change can be > satisfied. And like you said, "deleloper ... did care about groupping members > in a corresponding way". So, if we look at this in the original code, my > suggestion, besides padding benefit, is consistent with existing pattern in > that data structure. What pattern do you mean? As I see it, my implementation is consistent with current structure structure, while yours is not. -Sergey > > Note, I agree on extern keyword change can be postponed (it was in the original > code), but here you introduce a new code... > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c index e43914dbcadf..70cb68290385 100644 --- a/drivers/spi/spi-dw-mid.c +++ b/drivers/spi/spi-dw-mid.c @@ -35,6 +35,31 @@ static bool mid_spi_dma_chan_filter(struct dma_chan *chan, void *param) return true; } +static void mid_spi_maxburst_init(struct dw_spi *dws) +{ + struct dma_slave_caps caps; + u32 max_burst, def_burst; + int ret; + + def_burst = dws->fifo_len / 2; + + ret = dma_get_slave_caps(dws->rxchan, &caps); + if (!ret && caps.max_burst) + max_burst = caps.max_burst; + else + max_burst = RX_BURST_LEVEL; + + dws->rxburst = (def_burst > max_burst) ? max_burst : def_burst; + + ret = dma_get_slave_caps(dws->txchan, &caps); + if (!ret && caps.max_burst) + max_burst = caps.max_burst; + else + max_burst = TX_BURST_LEVEL; + + dws->txburst = (def_burst > max_burst) ? max_burst : def_burst; +} + static int mid_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) { struct dw_dma_slave slave = {0}; @@ -67,6 +92,8 @@ static int mid_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) dws->master->dma_rx = dws->rxchan; dws->master->dma_tx = dws->txchan; + mid_spi_maxburst_init(dws); + return 0; free_rxchan: @@ -92,6 +119,8 @@ static int mid_spi_dma_init_generic(struct device *dev, struct dw_spi *dws) dws->master->dma_rx = dws->rxchan; dws->master->dma_tx = dws->txchan; + mid_spi_maxburst_init(dws); + return 0; } @@ -195,7 +224,7 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_tx(struct dw_spi *dws, memset(&txconf, 0, sizeof(txconf)); txconf.direction = DMA_MEM_TO_DEV; txconf.dst_addr = dws->dma_addr; - txconf.dst_maxburst = TX_BURST_LEVEL; + txconf.dst_maxburst = dws->txburst; txconf.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; txconf.dst_addr_width = convert_dma_width(dws->n_bytes); txconf.device_fc = false; @@ -268,7 +297,7 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_rx(struct dw_spi *dws, memset(&rxconf, 0, sizeof(rxconf)); rxconf.direction = DMA_DEV_TO_MEM; rxconf.src_addr = dws->dma_addr; - rxconf.src_maxburst = RX_BURST_LEVEL; + rxconf.src_maxburst = dws->rxburst; rxconf.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; rxconf.src_addr_width = convert_dma_width(dws->n_bytes); rxconf.device_fc = false; @@ -293,8 +322,8 @@ static int mid_spi_dma_setup(struct dw_spi *dws, struct spi_transfer *xfer) { u16 imr = 0, dma_ctrl = 0; - dw_writel(dws, DW_SPI_DMARDLR, RX_BURST_LEVEL - 1); - dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - TX_BURST_LEVEL); + dw_writel(dws, DW_SPI_DMARDLR, dws->rxburst - 1); + dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst); if (xfer->tx_buf) { dma_ctrl |= SPI_DMA_TDMAE; diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h index 4902f937c3d7..d0c8b7d3a5d2 100644 --- a/drivers/spi/spi-dw.h +++ b/drivers/spi/spi-dw.h @@ -141,7 +141,9 @@ struct dw_spi { /* DMA info */ struct dma_chan *txchan; + u32 txburst; struct dma_chan *rxchan; + u32 rxburst; unsigned long dma_chan_busy; dma_addr_t dma_addr; /* phy address of the Data register */ const struct dw_spi_dma_ops *dma_ops;
Each channel of DMA controller may have a limited length of burst transaction (number of IO operations performed at ones in a single DMA client request). This parameter can be used to setup the most optimal DMA Tx/Rx data level values. In order to avoid the Tx buffer overrun we can set the DMA Tx level to be of FIFO depth minus the maximum burst transactions length. To prevent the Rx buffer underflow the DMA Rx level should be set to the maximum burst transactions length. This commit setups the DMA channels and the DW SPI DMA Tx/Rx levels in accordance with these rules. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Paul Burton <paulburton@kernel.org> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Allison Randal <allison@lohutok.net> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Gareth Williams <gareth.williams.jx@renesas.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: linux-mips@vger.kernel.org Cc: devicetree@vger.kernel.org --- drivers/spi/spi-dw-mid.c | 37 +++++++++++++++++++++++++++++++++---- drivers/spi/spi-dw.h | 2 ++ 2 files changed, 35 insertions(+), 4 deletions(-)