Re: [PATCH v2 1/2] dmaengine: qcom: gpi: Add GPI immediate DMA support

[Jyothi Kumar Seerapu <quic_jseerapu@quicinc.com>]

On 12/2/2024 11:01 AM, Jyothi Kumar Seerapu wrote:
> 
> 
> On 11/30/2024 9:35 AM, Bjorn Andersson wrote:
>> On Fri, Nov 29, 2024 at 05:02:22PM +0530, Jyothi Kumar Seerapu wrote:
>>> On 11/28/2024 8:53 PM, Bjorn Andersson wrote:
>>>> On Thu, Nov 28, 2024 at 07:03:50PM +0530, Jyothi Kumar Seerapu wrote:
>>>>> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
>> [..]
>>>>
>>>>>        /* 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);
>>>>> +    /* Support Immediate dma for write transfers for data length 
>>>>> up to 8 bytes */
>>>>
>>>> And what happens if the developer writing the SPI driver forgets to 
>>>> read
>>>> this comment and sets QCOM_GPI_IMMEDIATE_DMA for a 9 byte transfer?
>>> In V2 patch, QCOM_GPI_IMMEDIATE_DMA is set based on
>>> QCOM_GPI_IMMEDIATE_DMA_LEN only.
>>>
>>
>> I assume you mean "patch 2/2". So, what happens if someone refactors the
>> SPI driver in the future, will they read this comment?
>>
>>> As per Hardware programming guide, immediate dma support is for up to 8
>>> bytes only.
>>> Need to check what is the behavior if we want to handle 9 bytes using
>>> immediate dma feature support.
>>>
>>
>> I'm saying that you have a comment here which says that the caller must
>> not pass len > 8. Write that comment in code to avoid mistakes - either
>> now or in the future.
> 
> Sure, i will update the comment in V3.

If the GPI driver has the control to process it using Immediate DMA or 
normal existing GPI-DMA transfers (else path) based on the length check 
and direction, then i think that the existing comment is fine as 
protocol driver (spi) has no role here to set the transfer type whether 
to handle using Immediate DMA or normal existing DMA method.
please let me know in case still it needs to update.

if (direction == DMA_MEM_TO_DEV && len <= 2 * sizeof(tre->dword[0]))
This change, i will update in V3 patch.

>>
>>>>
>>>>> +    if ((spi->flags & QCOM_GPI_IMMEDIATE_DMA) && direction == 
>>>>> DMA_MEM_TO_DEV) {
>>>>
>>>> Why is this flag introduced?
>>>>
>>>> If I understand the next patch, all DMA_MEM_TO_DEV transfers of <=
>>>> QCOM_GPI_IMMEDIATE_DMA_LEN can use the immediate mode, so why not move
>>>> the condition here?
>>>>
>>>> Also ordering[1].
>>>>
>>>>     if (direction == DMA_MEM_TO_DEV && len <= 2 * 
>>>> sizeof(tre->dword[0]))
>>>>
>>>>
>>> Sure, thanks for the suggestion.
>>> so, instead using "QCOM_GPI_IMMEDIATE_DMA_LEN" need to use " 2 *
>>> sizeof(tre->dword[0])" for 8 bytes length check.
>>>
>>
>> Either one works, but I'm guessing that while 8 is the right number the
>> reason for 8 is that the data is passed in 2 * dword.
> Okay, i will use "2 * sizeof(tre->dword[0]" which gives 8 only.
>>
>>
>> The important thing is that you're encoding the length check here, so
>> that the client can't by mistake trigger immediate mode with > 8 bytes.
>> As a side effect, you no longer need the QCOM_GPI_IMMEDIATE_DMA flag and
>> should be able to drop patch 2.
> 
> Sure thanks, will update the changes in V3.
>>
>>>> [1] Compare "all transfers of length 8 or less, which are mem to 
>>>> device"
>>>> vs "all transfers which are mem to device, with a length of 8 or less".
>>>> The bigger "selection criteria" is the direction, then that's fine 
>>>> tuned
>>>> by the length query.
>>>>
>>>>> +        buf = sg_virt(sgl);
>>>>
>>>> It's a question of style, but I think you could just put the sg_virt()
>>>> directly in the memcpy() call and avoid the extra variable.
>>>
>>> Okay, i will directly put sg_virt() in memcpy().
>>
>> Try it out, pick the option that look the best.
> Yes, will do it in V3.
> 
>>
>> Regards,
>> Bjorn
>>
>