Hi,
Thank you very much for the review...
On 5/15/2023 9:43 PM, Doug Anderson wrote:
Hi,
On Fri, May 12, 2023 at 10:07 AM Vijaya Krishna Nivarthi
<quic_vnivarth@xxxxxxxxxxx> wrote:
@@ -836,35 +858,24 @@ static int setup_se_xfer(struct spi_transfer *xfer,
geni_se_setup_m_cmd(se, m_cmd, FRAGMENTATION);
if (mas->cur_xfer_mode == GENI_SE_DMA) {
+ dma_addr_t dma_ptr_sg;
+ unsigned int dma_len_sg;
+
if (m_cmd & SPI_RX_ONLY) {
- ret = geni_se_rx_dma_prep(se, xfer->rx_buf,
- xfer->len, &mas->rx_se_dma);
- if (ret) {
- dev_err(mas->dev, "Failed to setup Rx dma %d\n", ret);
- mas->rx_se_dma = 0;
- goto unlock_and_return;
- }
+ dma_ptr_sg = sg_dma_address(xfer->rx_sg.sgl);
+ dma_len_sg = sg_dma_len(xfer->rx_sg.sgl);
+ geni_se_rx_init_dma(se, &dma_ptr_sg, dma_len_sg);
nit: probably don't need local variables if you change patch set #1
like I suggested and don't pass in a pointer for the iova.
Done.
One last question: should you call:
dma_set_max_seg_size(dev, INT_MAX)
...in your probe function? I don't think you have any limitations of
maximum segment size, right? Right now if you don't set anything it
looks as if it considers your max to be 64K. That would cause the SPI
framework to break things up into multiple chunks which would make you
fall back to FIFO mode, right?
Actually we would need to call:
dma_set_max_seg_size(dev->parent, INT_MAX)
Please note that in probe()
spi->dma_map_dev = dev->parent;
and in __spi_map_msg()
tx_dev = ctlr->dma_map_dev;
ret = spi_map_buf(ctlr, tx_dev, &xfer->tx_sg,...
Since the dev->parent is QUP containing other SEs and its max_seg_size
seems to be getting set from elsewhere than code (perhaps kernel
scripts) it seemed safer not to modify that.
So I made below change and uploaded v2...
spi->max_dma_len = 0xffff0; /* 24 bits for tx/rx dma length */
which actually doesnt help much because spi_map_buf() picks the lower of
these 2 but added it anyway
desc_len = min_t(size_t, max_seg_size, ctlr->max_dma_len);
Any case as next step we will look into scatter list support to DMA; and
practically we may not have transfers over 64KB, so its ok for now?
Thank you,
Vijay/
Other than that this looks good to me.
-Doug
