Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <27ed4b11-14bf-081f-619e-75789d8caced@quicinc.com>
Date:   Wed, 17 May 2023 17:46:51 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH 2/2] spi: spi-geni-qcom: Do not do DMA map/unmap inside
 driver, use framework instead
Content-Language: en-CA
To:     Doug Anderson <dianders@chromium.org>
CC:     <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <broonie@kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-spi@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <quic_msavaliy@quicinc.com>,
        <mka@chromium.org>, <swboyd@chromium.org>,
        <quic_vtanuku@quicinc.com>, <quic_ptalari@quicinc.com>
References: <1683911265-3395-1-git-send-email-quic_vnivarth@quicinc.com>
 <1683911265-3395-3-git-send-email-quic_vnivarth@quicinc.com>
 <CAD=FV=WbMFDsC7dVjLNrtwi-hDEbSx9R8XzVGFW902OSKSFWjA@mail.gmail.com>
From:   Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>
In-Reply-To: <CAD=FV=WbMFDsC7dVjLNrtwi-hDEbSx9R8XzVGFW902OSKSFWjA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@quicinc.com> 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