diff mbox series

[v2,03/16] dmaengine: qcom: bam_dma: add LOCK & UNLOCK flag support

Message ID 20240815085725.2740390-4-quic_mdalam@quicinc.com (mailing list archive)
State Not Applicable
Headers show
Series Add cmd descriptor support | expand

Commit Message

Md Sadre Alam Aug. 15, 2024, 8:57 a.m. UTC
Add lock and unlock flag support on command descriptor.
Once lock set in requester pipe, then the bam controller
will lock all others pipe and process the request only
from requester pipe. Unlocking only can be performed from
the same pipe.

If DMA_PREP_LOCK flag passed in command descriptor then requester
of this transaction wanted to lock the BAM controller for this
transaction so BAM driver should set LOCK bit for the HW descriptor.

If DMA_PREP_UNLOCK flag passed in command descriptor then requester
of this transaction wanted to unlock the BAM controller.so BAM driver
should set UNLOCK bit for the HW descriptor.

Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
---

Change in [v2]

* Added LOCK and UNLOCK flag in bam driver

Change in [v1]

* This patch was not included in [v1]

 drivers/dma/qcom/bam_dma.c | 10 +++++++++-
 include/linux/dmaengine.h  |  6 ++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

Comments

Bjorn Andersson Aug. 16, 2024, 4:22 p.m. UTC | #1
On Thu, Aug 15, 2024 at 02:27:12PM GMT, Md Sadre Alam wrote:
> Add lock and unlock flag support on command descriptor.
> Once lock set in requester pipe, then the bam controller
> will lock all others pipe and process the request only
> from requester pipe. Unlocking only can be performed from
> the same pipe.
> 

Is the lock per channel, or for the whole BAM instance?

> If DMA_PREP_LOCK flag passed in command descriptor then requester
> of this transaction wanted to lock the BAM controller for this
> transaction so BAM driver should set LOCK bit for the HW descriptor.

You use the expression "this transaction" here, but if I understand the
calling code the lock is going to be held over multiple DMA operations
and even across asynchronous operations in the crypto driver.

DMA_PREP_LOCK indicates that this is the beginning of a transaction,
consisting of multiple operations that needs to happen while other EEs
are being locked out, and DMA_PREP_UNLOCK marks the end of the
transaction.

That said, I'm not entirely fond of the fact that these flags are not
just used on first and last operation in one sequence, but spread out.

Locking is hard, when you spread the responsibility of locking and
unlocking across different entities it's made harder...

> 
> If DMA_PREP_UNLOCK flag passed in command descriptor then requester
> of this transaction wanted to unlock the BAM controller.so BAM driver
> should set UNLOCK bit for the HW descriptor.
> 
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> ---
> 
> Change in [v2]
> 
> * Added LOCK and UNLOCK flag in bam driver
> 
> Change in [v1]
> 
> * This patch was not included in [v1]

v1 can be found here:
https://lore.kernel.org/all/20231214114239.2635325-7-quic_mdalam@quicinc.com/

And it was also posted once before that:
https://lore.kernel.org/all/1608215842-15381-1-git-send-email-mdalam@codeaurora.org/

In particular during the latter (i.e. first post) we had a rather long
discussion about this feature, so that's certainly worth linking to.

Looks like this series provides some answers to the questions we had
back then.

Regards,
Bjorn

> 
>  drivers/dma/qcom/bam_dma.c | 10 +++++++++-
>  include/linux/dmaengine.h  |  6 ++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 1ac7e250bdaa..ab3b5319aa68 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -58,6 +58,8 @@ struct bam_desc_hw {
>  #define DESC_FLAG_EOB BIT(13)
>  #define DESC_FLAG_NWD BIT(12)
>  #define DESC_FLAG_CMD BIT(11)
> +#define DESC_FLAG_LOCK BIT(10)
> +#define DESC_FLAG_UNLOCK BIT(9)
>  
>  struct bam_async_desc {
>  	struct virt_dma_desc vd;
> @@ -692,9 +694,15 @@ static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
>  		unsigned int curr_offset = 0;
>  
>  		do {
> -			if (flags & DMA_PREP_CMD)
> +			if (flags & DMA_PREP_CMD) {
>  				desc->flags |= cpu_to_le16(DESC_FLAG_CMD);
>  
> +				if (bdev->bam_pipe_lock && flags & DMA_PREP_LOCK)
> +					desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
> +				else if (bdev->bam_pipe_lock && flags & DMA_PREP_UNLOCK)
> +					desc->flags |= cpu_to_le16(DESC_FLAG_UNLOCK);
> +			}
> +
>  			desc->addr = cpu_to_le32(sg_dma_address(sg) +
>  						 curr_offset);
>  
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index b137fdb56093..70f23068bfdc 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -200,6 +200,10 @@ struct dma_vec {
>   *  transaction is marked with DMA_PREP_REPEAT will cause the new transaction
>   *  to never be processed and stay in the issued queue forever. The flag is
>   *  ignored if the previous transaction is not a repeated transaction.
> + *  @DMA_PREP_LOCK: tell the driver that there is a lock bit set on command
> + *  descriptor.
> + *  @DMA_PREP_UNLOCK: tell the driver that there is a un-lock bit set on command
> + *  descriptor.
>   */
>  enum dma_ctrl_flags {
>  	DMA_PREP_INTERRUPT = (1 << 0),
> @@ -212,6 +216,8 @@ enum dma_ctrl_flags {
>  	DMA_PREP_CMD = (1 << 7),
>  	DMA_PREP_REPEAT = (1 << 8),
>  	DMA_PREP_LOAD_EOT = (1 << 9),
> +	DMA_PREP_LOCK = (1 << 10),
> +	DMA_PREP_UNLOCK = (1 << 11),
>  };
>  
>  /**
> -- 
> 2.34.1
>
Md Sadre Alam Aug. 21, 2024, 4:31 p.m. UTC | #2
On 8/16/2024 9:52 PM, Bjorn Andersson wrote:
> On Thu, Aug 15, 2024 at 02:27:12PM GMT, Md Sadre Alam wrote:
>> Add lock and unlock flag support on command descriptor.
>> Once lock set in requester pipe, then the bam controller
>> will lock all others pipe and process the request only
>> from requester pipe. Unlocking only can be performed from
>> the same pipe.
>>
> 
> Is the lock per channel, or for the whole BAM instance?
   This lock is for whole BAM instance. Once LOCK bit will
   set on initiator CMD descriptor BAM will lock all pipes
   which belongs to other EE's and Pipes not in the current
   group.
> 
>> If DMA_PREP_LOCK flag passed in command descriptor then requester
>> of this transaction wanted to lock the BAM controller for this
>> transaction so BAM driver should set LOCK bit for the HW descriptor.
> 
> You use the expression "this transaction" here, but if I understand the
> calling code the lock is going to be held over multiple DMA operations
> and even across asynchronous operations in the crypto driver.
   Yes its correct.
> 
> DMA_PREP_LOCK indicates that this is the beginning of a transaction,
> consisting of multiple operations that needs to happen while other EEs
> are being locked out, and DMA_PREP_UNLOCK marks the end of the
> transaction.
   Yes its correct.
> 
> That said, I'm not entirely fond of the fact that these flags are not
> just used on first and last operation in one sequence, but spread out.
   Yes its correct.
> 
> Locking is hard, when you spread the responsibility of locking and
> unlocking across different entities it's made harder...
   The locking/unlocking always synchronous because unlocking happening
   in the dma callback.
> 
>>
>> If DMA_PREP_UNLOCK flag passed in command descriptor then requester
>> of this transaction wanted to unlock the BAM controller.so BAM driver
>> should set UNLOCK bit for the HW descriptor.
>>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> ---
>>
>> Change in [v2]
>>
>> * Added LOCK and UNLOCK flag in bam driver
>>
>> Change in [v1]
>>
>> * This patch was not included in [v1]
> 
> v1 can be found here:
> https://lore.kernel.org/all/20231214114239.2635325-7-quic_mdalam@quicinc.com/
> 
> And it was also posted once before that:
> https://lore.kernel.org/all/1608215842-15381-1-git-send-email-mdalam@codeaurora.org/
> 
> In particular during the latter (i.e. first post) we had a rather long
> discussion about this feature, so that's certainly worth linking to.
> 
> Looks like this series provides some answers to the questions we had
> back then.
   Will add the link in next post
> 
> Regards,
> Bjorn
> 
>>
>>   drivers/dma/qcom/bam_dma.c | 10 +++++++++-
>>   include/linux/dmaengine.h  |  6 ++++++
>>   2 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
>> index 1ac7e250bdaa..ab3b5319aa68 100644
>> --- a/drivers/dma/qcom/bam_dma.c
>> +++ b/drivers/dma/qcom/bam_dma.c
>> @@ -58,6 +58,8 @@ struct bam_desc_hw {
>>   #define DESC_FLAG_EOB BIT(13)
>>   #define DESC_FLAG_NWD BIT(12)
>>   #define DESC_FLAG_CMD BIT(11)
>> +#define DESC_FLAG_LOCK BIT(10)
>> +#define DESC_FLAG_UNLOCK BIT(9)
>>   
>>   struct bam_async_desc {
>>   	struct virt_dma_desc vd;
>> @@ -692,9 +694,15 @@ static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
>>   		unsigned int curr_offset = 0;
>>   
>>   		do {
>> -			if (flags & DMA_PREP_CMD)
>> +			if (flags & DMA_PREP_CMD) {
>>   				desc->flags |= cpu_to_le16(DESC_FLAG_CMD);
>>   
>> +				if (bdev->bam_pipe_lock && flags & DMA_PREP_LOCK)
>> +					desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
>> +				else if (bdev->bam_pipe_lock && flags & DMA_PREP_UNLOCK)
>> +					desc->flags |= cpu_to_le16(DESC_FLAG_UNLOCK);
>> +			}
>> +
>>   			desc->addr = cpu_to_le32(sg_dma_address(sg) +
>>   						 curr_offset);
>>   
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index b137fdb56093..70f23068bfdc 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -200,6 +200,10 @@ struct dma_vec {
>>    *  transaction is marked with DMA_PREP_REPEAT will cause the new transaction
>>    *  to never be processed and stay in the issued queue forever. The flag is
>>    *  ignored if the previous transaction is not a repeated transaction.
>> + *  @DMA_PREP_LOCK: tell the driver that there is a lock bit set on command
>> + *  descriptor.
>> + *  @DMA_PREP_UNLOCK: tell the driver that there is a un-lock bit set on command
>> + *  descriptor.
>>    */
>>   enum dma_ctrl_flags {
>>   	DMA_PREP_INTERRUPT = (1 << 0),
>> @@ -212,6 +216,8 @@ enum dma_ctrl_flags {
>>   	DMA_PREP_CMD = (1 << 7),
>>   	DMA_PREP_REPEAT = (1 << 8),
>>   	DMA_PREP_LOAD_EOT = (1 << 9),
>> +	DMA_PREP_LOCK = (1 << 10),
>> +	DMA_PREP_UNLOCK = (1 << 11),
>>   };
>>   
>>   /**
>> -- 
>> 2.34.1
>>
diff mbox series

Patch

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 1ac7e250bdaa..ab3b5319aa68 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -58,6 +58,8 @@  struct bam_desc_hw {
 #define DESC_FLAG_EOB BIT(13)
 #define DESC_FLAG_NWD BIT(12)
 #define DESC_FLAG_CMD BIT(11)
+#define DESC_FLAG_LOCK BIT(10)
+#define DESC_FLAG_UNLOCK BIT(9)
 
 struct bam_async_desc {
 	struct virt_dma_desc vd;
@@ -692,9 +694,15 @@  static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
 		unsigned int curr_offset = 0;
 
 		do {
-			if (flags & DMA_PREP_CMD)
+			if (flags & DMA_PREP_CMD) {
 				desc->flags |= cpu_to_le16(DESC_FLAG_CMD);
 
+				if (bdev->bam_pipe_lock && flags & DMA_PREP_LOCK)
+					desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
+				else if (bdev->bam_pipe_lock && flags & DMA_PREP_UNLOCK)
+					desc->flags |= cpu_to_le16(DESC_FLAG_UNLOCK);
+			}
+
 			desc->addr = cpu_to_le32(sg_dma_address(sg) +
 						 curr_offset);
 
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index b137fdb56093..70f23068bfdc 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -200,6 +200,10 @@  struct dma_vec {
  *  transaction is marked with DMA_PREP_REPEAT will cause the new transaction
  *  to never be processed and stay in the issued queue forever. The flag is
  *  ignored if the previous transaction is not a repeated transaction.
+ *  @DMA_PREP_LOCK: tell the driver that there is a lock bit set on command
+ *  descriptor.
+ *  @DMA_PREP_UNLOCK: tell the driver that there is a un-lock bit set on command
+ *  descriptor.
  */
 enum dma_ctrl_flags {
 	DMA_PREP_INTERRUPT = (1 << 0),
@@ -212,6 +216,8 @@  enum dma_ctrl_flags {
 	DMA_PREP_CMD = (1 << 7),
 	DMA_PREP_REPEAT = (1 << 8),
 	DMA_PREP_LOAD_EOT = (1 << 9),
+	DMA_PREP_LOCK = (1 << 10),
+	DMA_PREP_UNLOCK = (1 << 11),
 };
 
 /**