Re: [PATCH v4 2/4] dmaengine: gpi: Add Lock and Unlock TRE support to access I2C exclusively

[Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>]

Thanks Konrad for the review !

On 11/16/2024 12:53 AM, Konrad Dybcio wrote:
> On 13.11.2024 5:14 PM, Mukesh Kumar Savaliya wrote:
>> GSI DMA provides specific TREs(Transfer ring element) namely Lock and
>> Unlock TRE. It provides mutually exclusive access to I2C controller from
>> any of the processor(Apps,ADSP). Lock prevents other subsystems from
>> concurrently performing DMA transfers and avoids disturbance to data path.
>> Basically for shared I2C usecase, lock the SE(Serial Engine) for one of
>> the processor, complete the transfer, unlock the SE.
>>
>> Apply Lock TRE for the first transfer of shared SE and Apply Unlock
>> TRE for the last transfer.
>>
>> Also change MAX_TRE macro to 5 from 3 because of the two additional TREs.
>>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>>   drivers/dma/qcom/gpi.c           | 37 +++++++++++++++++++++++++++++++-
>>   include/linux/dma/qcom-gpi-dma.h |  6 ++++++
>>   2 files changed, 42 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
>> index 52a7c8f2498f..c9e71c576680 100644
>> --- a/drivers/dma/qcom/gpi.c
>> +++ b/drivers/dma/qcom/gpi.c
>> @@ -2,6 +2,7 @@
>>   /*
>>    * Copyright (c) 2017-2020, The Linux Foundation. All rights reserved.
>>    * Copyright (c) 2020, Linaro Limited
>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>    */
>>   
>>   #include <dt-bindings/dma/qcom-gpi.h>
>> @@ -65,6 +66,14 @@
>>   /* DMA TRE */
>>   #define TRE_DMA_LEN		GENMASK(23, 0)
>>   
>> +/* Lock TRE */
>> +#define TRE_LOCK		BIT(0)
>> +#define TRE_MINOR_TYPE		GENMASK(19, 16)
>> +#define TRE_MAJOR_TYPE		GENMASK(23, 20)
>> +
>> +/* Unlock TRE */
>> +#define TRE_I2C_UNLOCK		BIT(8)
> 
> So the lock is generic.. I'd then expect the unlock to be generic, too?
Absolutely, renamed it for generic as TRE_UNLOCK.
> 
>> +
>>   /* Register offsets from gpi-top */
>>   #define GPII_n_CH_k_CNTXT_0_OFFS(n, k)	(0x20000 + (0x4000 * (n)) + (0x80 * (k)))
>>   #define GPII_n_CH_k_CNTXT_0_EL_SIZE	GENMASK(31, 24)
>> @@ -516,7 +525,7 @@ struct gpii {
>>   	bool ieob_set;
>>   };
>>   
>> -#define MAX_TRE 3
>> +#define MAX_TRE 5
>>   
>>   struct gpi_desc {
>>   	struct virt_dma_desc vd;
>> @@ -1637,6 +1646,19 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
>>   	struct gpi_tre *tre;
>>   	unsigned int i;
>>   
>> +	/* create lock tre for first tranfser */
>> +	if (i2c->shared_se && i2c->first_msg) {
> 
> Does the first/last logic handle errors well? i.e. what if we
> have >= 3 transfers and:
> 
> 1) the first transfer succeeds but the last doesn't
> 2) the first transfer succeeds, the second one doesn't and the lock
>     is submitted again
> 3) the unlock never suceeds
> 
geni_i2c_gpi_xfer() takes care of any of the error. Upon error, it does 
dma_engine_terminate_sync() which resets all the pipes. Internal 
downstream also has same implementation.
> Konrad