Message-ID: <46249e7e-ac2c-00d3-b3b0-7b15848e7b7c@quicinc.com>
Date:   Mon, 17 Apr 2023 21:27:16 +0530
From:   Vijaya Krishna Nivarthi <quic_vnivarth@...cinc.com>
To:     Doug Anderson <dianders@...omium.org>,
        Mark Brown <broonie@...nel.org>
CC:     <agross@...nel.org>, <andersson@...nel.org>,
        <konrad.dybcio@...aro.org>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>,
        <cros-qcom-dts-watchers@...omium.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-spi@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <quic_msavaliy@...cinc.com>, <mka@...omium.org>,
        <swboyd@...omium.org>, <quic_vtanuku@...cinc.com>
Subject: Re: [PATCH v3 3/3] spi: spi-qcom-qspi: Add DMA mode support

Hi,


On 4/17/2023 7:37 PM, Doug Anderson wrote:
> Hi,
>
> On Mon, Apr 17, 2023 at 5:12 AM Mark Brown <broonie@...nel.org> wrote:
>> On Fri, Apr 14, 2023 at 03:05:58PM -0700, Doug Anderson wrote:
>>
>>> Having alignment requirements like this doesn't seem like it should be
>>> that unusual, though, and that's why it feels like the logic belongs
>>> in the SPI core. In fact, it seems like this is _supposed_ to be
>>> handled in the SPI core, but it isn't? In "spi.h" I see
>>> "dma_alignment" that claims to be exactly what you need. As far as I
>>> can tell, though, the core doesn't use this? ...so I'm kinda confused.
>>> As far as I can tell this doesn't do anything and thus anyone setting
>>> it today is broken?
>> SPI consumers should only be providing dmaable buffers.
> Ah, I think I see.
>
> 1. In "struct spi_transfer" the @tx_buf and @rx_buf are documented to
> have "dma-safe memory".
>
> 2. On ARM64 anyway, I see "ARCH_DMA_MINALIGN" is 128.
>
> So there is no reason to do any special rules to force alignment to
> 32-bytes because that's already guaranteed. Presumably that means you
> can drop a whole pile of code and things will still work fine.
>
> -Doug


Thank you very much Mark and Doug for review and inputs.


spi_map_buf is taking into consideration max_dma_len (in spi.h) when it 
is set.

For example if set to 1024 instead of 65536(the actual max_dma_len of 
HW), 4 entries are created in the sg_table for a buffer of size 4096.

However, Like Doug pointed, dma_alignment is not being used by core.

Some drivers seem to be setting both of these in probe() but 
dma_alignment is probably not taking effect.

Is it up to the SPI consumers to read this and ensure they are providing 
dmaable buffers of required alignment?


The dma_addresses coming from core are aligned for larger sized buffers 
but for small ones like 1 and 3 bytes they are not aligned.

Hence if the code handing the alignment part is not present, smaller 
transfers fail.

For example, Below debug patch does

a) all transfers in DMA

b) prints DMA addresses and lengths

====== patch start ======

diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
index 60c4f554..8d24022 100644
--- a/drivers/spi/spi-qcom-qspi.c
+++ b/drivers/spi/spi-qcom-qspi.c
@@ -438,8 +438,14 @@ static int qcom_qspi_setup_dma_desc(struct 
qcom_qspi *ctrl,
                 return -EINVAL;
         }

-       for (i = 0; i < sgt->nents; i++)
+       for (i = 0; i < sgt->nents; i++) {
+               dma_addr_t temp_addr;
                 sg_total_len += sg_dma_len(sgt->sgl + i);
+
+               temp_addr = sg_dma_address(sgt->sgl + i);
+               dev_err_ratelimited(ctrl->dev, "%s pilli-20230417 i-%d, 
nents-%d, len-%d, dma_address-%pad\n",
+                               __func__, i, sgt->nents, 
sg_dma_len(sgt->sgl + i), &temp_addr);
+       }
         if (sg_total_len != xfer->len) {
                 dev_err(ctrl->dev, "Data lengths mismatch\n");
                 return -EINVAL;
@@ -517,6 +523,7 @@ static void qcom_qspi_dma_xfer(struct qcom_qspi *ctrl)
  static bool qcom_qspi_can_dma(struct spi_controller *ctlr,
                          struct spi_device *slv, struct spi_transfer *xfer)
  {
+       return true;
         return xfer->len > QSPI_MAX_BYTES_FIFO ? true : false;
  }

====== patch end =======

and below is sample outcome...

[   23.620397] qcom_qspi 88dc000.spi: qcom_qspi_setup_dma_desc 
pilli-20230417 i-0, nents-1, len-1, dma_address-0x00000000fff3e000
[   23.638392] qcom_qspi 88dc000.spi: qcom_qspi_setup_dma_desc 
pilli-20230417 i-0, nents-1, len-3, dma_address-0x00000000fff3f001
[   23.650238] qcom_qspi 88dc000.spi: qcom_qspi_setup_dma_desc 
pilli-20230417 i-0, nents-1, len-1, dma_address-0x00000000fff40004
[   23.662039] qcom_qspi 88dc000.spi: qcom_qspi_setup_dma_desc 
pilli-20230417 i-0, nents-1, len-8, dma_address-0x00000000fff44080
[   23.673965] qcom_qspi 88dc000.spi: qcom_qspi_setup_dma_desc 
pilli-20230417 i-0, nents-1, len-1, dma_address-0x00000000fff44000
[   23.685749] qcom_qspi 88dc000.spi: qcom_qspi_setup_dma_desc 
pilli-20230417 i-0, nents-1, len-3, dma_address-0x00000000fff40001
[   23.697630] qcom_qspi 88dc000.spi: qcom_qspi_setup_dma_desc 
pilli-20230417 i-0, nents-1, len-1, dma_address-0x00000000fff3f004
[   23.709552] qcom_qspi 88dc000.spi: qcom_qspi_setup_dma_desc 
pilli-20230417 i-0, nents-1, len-8, dma_address-0x00000000fff3e600
[   23.721460] qcom_qspi 88dc000.spi: qcom_qspi_setup_dma_desc 
pilli-20230417 i-0, nents-1, len-1, dma_address-0x00000000fff3e000
[   23.733257] qcom_qspi 88dc000.spi: qcom_qspi_setup_dma_desc 
pilli-20230417 i-0, nents-1, len-3, dma_address-0x00000000fff3f001

If we use the dma_address from sg table as is, transfers fail.

I have not checked the spi-nor driver, but is it the consumer driver's 
job to ensure required alignment in all cases?

Can you Please comment?

Thank you,

-Vijay/

Powered by blists - more mailing lists