Re: [PATCH 2/2] spi: spi-geni-qcom: Do not do DMA map/unmap inside driver, use framework instead

[Vijaya Krishna Nivarthi]

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