On Wed, 4 Dec 2024 at 15:54, Jyothi Kumar Seerapu <quic_jseerapu@xxxxxxxxxxx> wrote: > > > > On 12/4/2024 7:09 PM, Dmitry Baryshkov wrote: > > On Wed, 4 Dec 2024 at 15:25, Jyothi Kumar Seerapu > > <quic_jseerapu@xxxxxxxxxxx> wrote: > >> > >> > >> > >> On 12/4/2024 6:15 PM, Dmitry Baryshkov wrote: > >>> On Wed, Dec 04, 2024 at 05:50:59PM +0530, Jyothi Kumar Seerapu wrote: > >>>> The DMA TRE(Transfer ring element) buffer contains the DMA > >>>> buffer address. Accessing data from this address can cause > >>>> significant delays in SPI transfers, which can be mitigated to > >>>> some extent by utilizing immediate DMA support. > >>>> > >>>> QCOM GPI DMA hardware supports an immediate DMA feature for data > >>>> up to 8 bytes, storing the data directly in the DMA TRE buffer > >>>> instead of the DMA buffer address. This enhancement enables faster > >>>> SPI data transfers. > >>>> > >>>> This optimization reduces the average transfer time from 25 us to > >>>> 16 us for a single SPI transfer of 8 bytes length, with a clock > >>>> frequency of 50 MHz. > >>>> > >>>> Signed-off-by: Jyothi Kumar Seerapu <quic_jseerapu@xxxxxxxxxxx> > >>>> --- > >>>> > >>>> v2-> v3: > >>>> - When to enable Immediate DMA support, control is moved to GPI driver > >>>> from SPI driver. > >>>> - Optimizations are done in GPI driver related to immediate dma changes. > >>>> - Removed the immediate dma supported changes in qcom-gpi-dma.h file > >>>> and handled in GPI driver. > >>>> > >>>> Link to v2: > >>>> https://lore.kernel.org/all/20241128133351.24593-2-quic_jseerapu@xxxxxxxxxxx/ > >>>> https://lore.kernel.org/all/20241128133351.24593-3-quic_jseerapu@xxxxxxxxxxx/ > >>>> > >>>> v1 -> v2: > >>>> - Separated the patches to dmaengine and spi subsystems > >>>> - Removed the changes which are not required for this feature from > >>>> qcom-gpi-dma.h file. > >>>> - Removed the type conversions used in gpi_create_spi_tre. > >>>> > >>>> Link to v1: > >>>> https://lore.kernel.org/lkml/20241121115201.2191-2-quic_jseerapu@xxxxxxxxxxx/ > >>>> > >>>> drivers/dma/qcom/gpi.c | 32 +++++++++++++++++++++++++++----- > >>>> 1 file changed, 27 insertions(+), 5 deletions(-) > >>>> > >>>> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c > >>>> index 52a7c8f2498f..35451d5a81f7 100644 > >>>> --- a/drivers/dma/qcom/gpi.c > >>>> +++ b/drivers/dma/qcom/gpi.c > >>>> @@ -27,6 +27,7 @@ > >>>> #define TRE_FLAGS_IEOT BIT(9) > >>>> #define TRE_FLAGS_BEI BIT(10) > >>>> #define TRE_FLAGS_LINK BIT(11) > >>>> +#define TRE_FLAGS_IMMEDIATE_DMA BIT(16) > >>>> #define TRE_FLAGS_TYPE GENMASK(23, 16) > >>>> > >>>> /* SPI CONFIG0 WD0 */ > >>>> @@ -64,6 +65,7 @@ > >>>> > >>>> /* DMA TRE */ > >>>> #define TRE_DMA_LEN GENMASK(23, 0) > >>>> +#define TRE_DMA_IMMEDIATE_LEN GENMASK(3, 0) > >>>> > >>>> /* Register offsets from gpi-top */ > >>>> #define GPII_n_CH_k_CNTXT_0_OFFS(n, k) (0x20000 + (0x4000 * (n)) + (0x80 * (k))) > >>>> @@ -1711,6 +1713,8 @@ static int gpi_create_spi_tre(struct gchan *chan, struct gpi_desc *desc, > >>>> dma_addr_t address; > >>>> struct gpi_tre *tre; > >>>> unsigned int i; > >>>> + int len; > >>>> + u8 immediate_dma; > >>>> > >>>> /* first create config tre if applicable */ > >>>> if (direction == DMA_MEM_TO_DEV && spi->set_config) { > >>>> @@ -1763,14 +1767,32 @@ static int gpi_create_spi_tre(struct gchan *chan, struct gpi_desc *desc, > >>>> tre_idx++; > >>>> > >>>> address = sg_dma_address(sgl); > >>>> - tre->dword[0] = lower_32_bits(address); > >>>> - tre->dword[1] = upper_32_bits(address); > >>>> + len = sg_dma_len(sgl); > >>>> > >>>> - tre->dword[2] = u32_encode_bits(sg_dma_len(sgl), TRE_DMA_LEN); > >>>> + immediate_dma = (direction == DMA_MEM_TO_DEV) && len <= 2 * sizeof(tre->dword[0]); > >>> > >>> inline this condition, remove extra brackets and split the line after &&. > >> Hi Dmitry Baryshkov, thanks for the review. > >> Sure, i will make the changes mentioned below. Please let me know otherwise. > >> > >> immediate_dma = direction == DMA_MEM_TO_DEV && > >> len <= 2 * sizeof(tre->dword[0]); > > > > I was suggesting to _inline_ this condition rather than having a > > separate variable for it. > > I can directly use the condition as follows: > if (direction == DMA_MEM_TO_DEV && len <= 2 * sizeof(tre->dword[0])) > > However, this condition also needs to account for the > "TRE_FLAGS_IMMEDIATE_DMA" update. Therefore, I introduced a separate > variable. > > tre->dword[3] |= u32_encode_bits(!!immediate_dma, TRE_FLAGS_IMMEDIATE_DMA); > > Please let me know if it's acceptable to mention the entire condition in > both places instead of using a separate variable. Move the flag setting under the if() too. > > > > > >>>> + > >>>> + /* Support Immediate dma for write transfers for data length up to 8 bytes */ > >>>> + if (immediate_dma) { > >>>> + /* > >>>> + * For Immediate dma, data length may not always be length of 8 bytes, > >>>> + * it can be length less than 8, hence initialize both dword's with 0 > >>>> + */ > >>>> + tre->dword[0] = 0; > >>>> + tre->dword[1] = 0; > >>>> + memcpy(&tre->dword[0], sg_virt(sgl), len); > >>>> + > >>>> + tre->dword[2] = u32_encode_bits(len, TRE_DMA_IMMEDIATE_LEN); > >>>> + } else { > >>>> + tre->dword[0] = lower_32_bits(address); > >>>> + tre->dword[1] = upper_32_bits(address); > >>>> + > >>>> + tre->dword[2] = u32_encode_bits(len, TRE_DMA_LEN); > >>>> + } > >>>> > >>>> tre->dword[3] = u32_encode_bits(TRE_TYPE_DMA, TRE_FLAGS_TYPE); > >>>> - if (direction == DMA_MEM_TO_DEV) > >>>> - tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOT); > >>>> + tre->dword[3] |= u32_encode_bits(!!immediate_dma, TRE_FLAGS_IMMEDIATE_DMA); > >>>> + tre->dword[3] |= u32_encode_bits(!!(direction == DMA_MEM_TO_DEV), > >>>> + TRE_FLAGS_IEOT); > >>>> > >>>> for (i = 0; i < tre_idx; i++) > >>>> dev_dbg(dev, "TRE:%d %x:%x:%x:%x\n", i, desc->tre[i].dword[0], > >>>> -- > >>>> 2.17.1 > >>>> > >>> > > > > > > -- With best wishes Dmitry
