dmaengine: qcom: bam_dma: Add LOCK and UNLOCK flag bit support

Commit Message

Md Sadre Alam Dec. 17, 2020, 2:37 p.m. UTC

This change will add support for LOCK & UNLOCK flag bit support
on CMD descriptor.

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

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

Signed-off-by: Md Sadre Alam <[e-mail]>
---
 Documentation/driver-api/dmaengine/provider.rst | 9 +++++++++
 drivers/dma/qcom/bam_dma.c                      | 9 +++++++++
 include/linux/dmaengine.h                       | 5 +++++
 3 files changed, 23 insertions(+)

Comments

Thara Gopinath Dec. 19, 2020, 3:35 a.m. UTC | #1

On 12/17/20 9:37 AM, Md Sadre Alam wrote:
> This change will add support for LOCK & UNLOCK flag bit support
> on CMD descriptor.
> 
> If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
> transaction wanted to lock the DMA controller for this transaction so
> BAM driver should set LOCK bit for the HW descriptor.
> 
> If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester of this
> transaction wanted to unlock the DMA controller.so BAM driver should set
> UNLOCK bit for the HW descriptor.
Hi,

This is a generic question. What is the point of LOCK/UNLOCK with 
allocating LOCK groups to the individual dma channels? By default
doesn't all channels fall in the same group. This would mean that
a lock does not prevent the dma controller from not executing a
transaction on the other channels.

Md Sadre Alam Dec. 21, 2020, 7:35 a.m. UTC | #2

On 2020-12-19 09:05, Thara Gopinath wrote:
> On 12/17/20 9:37 AM, Md Sadre Alam wrote:
>> This change will add support for LOCK & UNLOCK flag bit support
>> on CMD descriptor.
>> 
>> If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
>> transaction wanted to lock the DMA controller for this transaction so
>> BAM driver should set LOCK bit for the HW descriptor.
>> 
>> If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester of this
>> transaction wanted to unlock the DMA controller.so BAM driver should 
>> set
>> UNLOCK bit for the HW descriptor.
> Hi,
> 
> This is a generic question. What is the point of LOCK/UNLOCK with
> allocating LOCK groups to the individual dma channels? By default
> doesn't all channels fall in the same group. This would mean that
> a lock does not prevent the dma controller from not executing a
> transaction on the other channels.
> 

The Pipe Locking/Unlocking will be only on command-descriptor.
Upon encountering a command descriptor with LOCK bit set, the BAM
will lock all other pipes not related to the current pipe group, and 
keep
handling the current pipe only until it sees the UNLOCK set then it will
release all locked pipes.

The actual locking is done on the new descriptor fetching for 
publishing,
i.e. locked pipe will not fetch new descriptors even if it got 
event/events
adding more descriptors for this pipe (locked pipe).

The bam LOCKING mechanism is needed where different cores needs to share
same hardware block which use bam for their transaction. So if both 
cores
wanted to access the hardware block in parallel via bam, then locking 
mechanism
is needed for bam pipes.

> --
> Warm Regards
> Thara
> 
>> 
>> Signed-off-by: Md Sadre Alam <[e-mail]>
>> ---
>>   Documentation/driver-api/dmaengine/provider.rst | 9 +++++++++
>>   drivers/dma/qcom/bam_dma.c                      | 9 +++++++++
>>   include/linux/dmaengine.h                       | 5 +++++
>>   3 files changed, 23 insertions(+)
>> 
>> diff --git a/Documentation/driver-api/dmaengine/provider.rst 
>> b/Documentation/driver-api/dmaengine/provider.rst
>> index ddb0a81..d7516e2 100644
>> --- a/Documentation/driver-api/dmaengine/provider.rst
>> +++ b/Documentation/driver-api/dmaengine/provider.rst
>> @@ -599,6 +599,15 @@ DMA_CTRL_REUSE
>>     - This flag is only supported if the channel reports the 
>> DMA_LOAD_EOT
>>       capability.
>>   +- DMA_PREP_LOCK
>> +
>> +  - If set , the client driver tells DMA controller I am locking you 
>> for
>> +    this transcation.
>> +
>> +- DMA_PREP_UNLOCK
>> +
>> +  - If set, the client driver will tells DMA controller I am 
>> releasing the lock
>> +
>>   General Design Notes
>>   ====================
>>   diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
>> index 4eeb8bb..cdbe395 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;
>> @@ -644,6 +646,13 @@ static struct dma_async_tx_descriptor 
>> *bam_prep_slave_sg(struct dma_chan *chan,
>>     	/* fill in temporary descriptors */
>>   	desc = async_desc->desc;
>> +	if (flags & DMA_PREP_CMD) {
>> +		if (flags & DMA_PREP_LOCK)
>> +			desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
>> +		if (flags & DMA_PREP_UNLOCK)
>> +			desc->flags |= cpu_to_le16(DESC_FLAG_UNLOCK);
>> +	}
>> +
>>   	for_each_sg(sgl, sg, sg_len, i) {
>>   		unsigned int remainder = sg_dma_len(sg);
>>   		unsigned int curr_offset = 0;
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index dd357a7..79ccadb4 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -190,6 +190,9 @@ struct dma_interleaved_template {
>>    *  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 DMA HW engine going to be 
>> locked for this
>> + *  transaction , until not seen DMA_PREP_UNLOCK flag set.
>> + * @DMA_PREP_UNLOCK: tell the driver to unlock the DMA HW engine.
>>    */
>>   enum dma_ctrl_flags {
>>   	DMA_PREP_INTERRUPT = (1 << 0),
>> @@ -202,6 +205,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),
>>   };
>>     /**
>>

Vinod Koul Dec. 21, 2020, 9:23 a.m. UTC | #3

Hello,

On 17-12-20, 20:07, Md Sadre Alam wrote:
> This change will add support for LOCK & UNLOCK flag bit support
> on CMD descriptor.
> 
> If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
> transaction wanted to lock the DMA controller for this transaction so
> BAM driver should set LOCK bit for the HW descriptor.
> 
> If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester of this
> transaction wanted to unlock the DMA controller.so BAM driver should set
> UNLOCK bit for the HW descriptor.

Can you explain why would we need to first lock and then unlock..? How
would this be used in real world.

I have read a bit of documentation but is unclear to me. Also should
this be exposed as an API to users, sounds like internal to driver..?


> 
> Signed-off-by: Md Sadre Alam <[e-mail]>
> ---
>  Documentation/driver-api/dmaengine/provider.rst | 9 +++++++++
>  drivers/dma/qcom/bam_dma.c                      | 9 +++++++++
>  include/linux/dmaengine.h                       | 5 +++++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/Documentation/driver-api/dmaengine/provider.rst b/Documentation/driver-api/dmaengine/provider.rst
> index ddb0a81..d7516e2 100644
> --- a/Documentation/driver-api/dmaengine/provider.rst
> +++ b/Documentation/driver-api/dmaengine/provider.rst
> @@ -599,6 +599,15 @@ DMA_CTRL_REUSE
>    - This flag is only supported if the channel reports the DMA_LOAD_EOT
>      capability.
>  
> +- DMA_PREP_LOCK
> +
> +  - If set , the client driver tells DMA controller I am locking you for
> +    this transcation.
> +
> +- DMA_PREP_UNLOCK
> +
> +  - If set, the client driver will tells DMA controller I am releasing the lock
> +
>  General Design Notes
>  ====================
>  
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 4eeb8bb..cdbe395 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;
> @@ -644,6 +646,13 @@ static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
>  
>  	/* fill in temporary descriptors */
>  	desc = async_desc->desc;
> +	if (flags & DMA_PREP_CMD) {
> +		if (flags & DMA_PREP_LOCK)
> +			desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
> +		if (flags & DMA_PREP_UNLOCK)
> +			desc->flags |= cpu_to_le16(DESC_FLAG_UNLOCK);
> +	}
> +
>  	for_each_sg(sgl, sg, sg_len, i) {
>  		unsigned int remainder = sg_dma_len(sg);
>  		unsigned int curr_offset = 0;
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index dd357a7..79ccadb4 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -190,6 +190,9 @@ struct dma_interleaved_template {
>   *  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 DMA HW engine going to be locked for this
> + *  transaction , until not seen DMA_PREP_UNLOCK flag set.
> + * @DMA_PREP_UNLOCK: tell the driver to unlock the DMA HW engine.
>   */
>  enum dma_ctrl_flags {
>  	DMA_PREP_INTERRUPT = (1 << 0),
> @@ -202,6 +205,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.7.4

Md Sadre Alam Dec. 21, 2020, 5:33 p.m. UTC | #4

On 2020-12-21 14:53, Vinod Koul wrote:
> Hello,
> 
> On 17-12-20, 20:07, Md Sadre Alam wrote:
>> This change will add support for LOCK & UNLOCK flag bit support
>> on CMD descriptor.
>> 
>> If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
>> transaction wanted to lock the DMA controller for this transaction so
>> BAM driver should set LOCK bit for the HW descriptor.
>> 
>> If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester of this
>> transaction wanted to unlock the DMA controller.so BAM driver should 
>> set
>> UNLOCK bit for the HW descriptor.
> 
> Can you explain why would we need to first lock and then unlock..? How
> would this be used in real world.
> 
> I have read a bit of documentation but is unclear to me. Also should
> this be exposed as an API to users, sounds like internal to driver..?
> 

IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto Hardware 
Engine
will be shared between A53 core & ubi32 core. There is two separate 
driver dedicated
to A53 core and ubi32 core. So to use Crypto Hardware Engine parallelly 
for encryption/description
we need bam locking mechanism. if one driver will submit the request for 
encryption/description
to Crypto then first it has to set LOCK flag bit on command descriptor 
so that other pipes will
get locked.

The Pipe Locking/Unlocking will be only on command-descriptor. Upon 
encountering a command descriptor
with LOCK bit set, The BAM will lock all other pipes not related to the 
current pipe group, and keep
handling the current pipe only until it sees the UNLOCK set then it will 
release all locked pipes.
locked pipe will not fetch new descriptors even if it got event/events 
adding more descriptors for
this pipe (locked pipe).

No need to expose as an API to user because its internal to driver, so 
while preparing command descriptor
just we have to update the LOCK/UNLOCK flag.


> 
>> 
>> Signed-off-by: Md Sadre Alam <[e-mail]>
>> ---
>>  Documentation/driver-api/dmaengine/provider.rst | 9 +++++++++
>>  drivers/dma/qcom/bam_dma.c                      | 9 +++++++++
>>  include/linux/dmaengine.h                       | 5 +++++
>>  3 files changed, 23 insertions(+)
>> 
>> diff --git a/Documentation/driver-api/dmaengine/provider.rst 
>> b/Documentation/driver-api/dmaengine/provider.rst
>> index ddb0a81..d7516e2 100644
>> --- a/Documentation/driver-api/dmaengine/provider.rst
>> +++ b/Documentation/driver-api/dmaengine/provider.rst
>> @@ -599,6 +599,15 @@ DMA_CTRL_REUSE
>>    - This flag is only supported if the channel reports the 
>> DMA_LOAD_EOT
>>      capability.
>> 
>> +- DMA_PREP_LOCK
>> +
>> +  - If set , the client driver tells DMA controller I am locking you 
>> for
>> +    this transcation.
>> +
>> +- DMA_PREP_UNLOCK
>> +
>> +  - If set, the client driver will tells DMA controller I am 
>> releasing the lock
>> +
>>  General Design Notes
>>  ====================
>> 
>> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
>> index 4eeb8bb..cdbe395 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;
>> @@ -644,6 +646,13 @@ static struct dma_async_tx_descriptor 
>> *bam_prep_slave_sg(struct dma_chan *chan,
>> 
>>  	/* fill in temporary descriptors */
>>  	desc = async_desc->desc;
>> +	if (flags & DMA_PREP_CMD) {
>> +		if (flags & DMA_PREP_LOCK)
>> +			desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
>> +		if (flags & DMA_PREP_UNLOCK)
>> +			desc->flags |= cpu_to_le16(DESC_FLAG_UNLOCK);
>> +	}
>> +
>>  	for_each_sg(sgl, sg, sg_len, i) {
>>  		unsigned int remainder = sg_dma_len(sg);
>>  		unsigned int curr_offset = 0;
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index dd357a7..79ccadb4 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -190,6 +190,9 @@ struct dma_interleaved_template {
>>   *  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 DMA HW engine going to be 
>> locked for this
>> + *  transaction , until not seen DMA_PREP_UNLOCK flag set.
>> + * @DMA_PREP_UNLOCK: tell the driver to unlock the DMA HW engine.
>>   */
>>  enum dma_ctrl_flags {
>>  	DMA_PREP_INTERRUPT = (1 << 0),
>> @@ -202,6 +205,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.7.4

Thara Gopinath Dec. 21, 2020, 6:09 p.m. UTC | #5

On 12/21/20 2:35 AM, [e-mail] wrote:
> On 2020-12-19 09:05, Thara Gopinath wrote:
>> On 12/17/20 9:37 AM, Md Sadre Alam wrote:
>>> This change will add support for LOCK & UNLOCK flag bit support
>>> on CMD descriptor.
>>>
>>> If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
>>> transaction wanted to lock the DMA controller for this transaction so
>>> BAM driver should set LOCK bit for the HW descriptor.
>>>
>>> If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester of this
>>> transaction wanted to unlock the DMA controller.so BAM driver should set
>>> UNLOCK bit for the HW descriptor.
>> Hi,
>>
>> This is a generic question. What is the point of LOCK/UNLOCK with
>> allocating LOCK groups to the individual dma channels? By default
>> doesn't all channels fall in the same group. This would mean that
>> a lock does not prevent the dma controller from not executing a
>> transaction on the other channels.
>>
> 
> The Pipe Locking/Unlocking will be only on command-descriptor.
> Upon encountering a command descriptor with LOCK bit set, the BAM
> will lock all other pipes not related to the current pipe group, and keep
> handling the current pipe only until it sees the UNLOCK set then it will
> release all locked pipes.

So unless you assign pipe groups, this will not work as intended right? 
So this patch is only half of the solution. There should also be a patch 
allowing pipe groups to be assigned. Without that extra bit this patch 
does nothing , right ?

Md Sadre Alam Dec. 22, 2020, 12:18 p.m. UTC | #6

On 2020-12-21 23:39, Thara Gopinath wrote:
> On 12/21/20 2:35 AM, [e-mail] wrote:
>> On 2020-12-19 09:05, Thara Gopinath wrote:
>>> On 12/17/20 9:37 AM, Md Sadre Alam wrote:
>>>> This change will add support for LOCK & UNLOCK flag bit support
>>>> on CMD descriptor.
>>>> 
>>>> If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
>>>> transaction wanted to lock the DMA controller for this transaction 
>>>> so
>>>> BAM driver should set LOCK bit for the HW descriptor.
>>>> 
>>>> If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester of 
>>>> this
>>>> transaction wanted to unlock the DMA controller.so BAM driver should 
>>>> set
>>>> UNLOCK bit for the HW descriptor.
>>> Hi,
>>> 
>>> This is a generic question. What is the point of LOCK/UNLOCK with
>>> allocating LOCK groups to the individual dma channels? By default
>>> doesn't all channels fall in the same group. This would mean that
>>> a lock does not prevent the dma controller from not executing a
>>> transaction on the other channels.
>>> 
>> 
>> The Pipe Locking/Unlocking will be only on command-descriptor.
>> Upon encountering a command descriptor with LOCK bit set, the BAM
>> will lock all other pipes not related to the current pipe group, and 
>> keep
>> handling the current pipe only until it sees the UNLOCK set then it 
>> will
>> release all locked pipes.
> 
> So unless you assign pipe groups, this will not work as intended
> right? So this patch is only half of the solution. There should also
> be a patch allowing pipe groups to be assigned. Without that extra bit
> this patch does nothing , right ?

Yes you are right.
We are having some register which will configure the pipe lock group.
But these registers are not exposed to non-secure world. These registers
only accessible through secure world. Currently in IPQ5018 SoC we are 
configuring
these register in secure world to configure pipe lock group.

Md Sadre Alam Jan. 12, 2021, 9:30 a.m. UTC | #7

On 2020-12-22 17:48, [e-mail] wrote:
> On 2020-12-21 23:39, Thara Gopinath wrote:
>> On 12/21/20 2:35 AM, [e-mail] wrote:
>>> On 2020-12-19 09:05, Thara Gopinath wrote:
>>>> On 12/17/20 9:37 AM, Md Sadre Alam wrote:
>>>>> This change will add support for LOCK & UNLOCK flag bit support
>>>>> on CMD descriptor.
>>>>> 
>>>>> If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of 
>>>>> this
>>>>> transaction wanted to lock the DMA controller for this transaction 
>>>>> so
>>>>> BAM driver should set LOCK bit for the HW descriptor.
>>>>> 
>>>>> If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester of 
>>>>> this
>>>>> transaction wanted to unlock the DMA controller.so BAM driver 
>>>>> should set
>>>>> UNLOCK bit for the HW descriptor.
>>>> Hi,
>>>> 
>>>> This is a generic question. What is the point of LOCK/UNLOCK with
>>>> allocating LOCK groups to the individual dma channels? By default
>>>> doesn't all channels fall in the same group. This would mean that
>>>> a lock does not prevent the dma controller from not executing a
>>>> transaction on the other channels.
>>>> 
>>> 
>>> The Pipe Locking/Unlocking will be only on command-descriptor.
>>> Upon encountering a command descriptor with LOCK bit set, the BAM
>>> will lock all other pipes not related to the current pipe group, and 
>>> keep
>>> handling the current pipe only until it sees the UNLOCK set then it 
>>> will
>>> release all locked pipes.
>> 
>> So unless you assign pipe groups, this will not work as intended
>> right? So this patch is only half of the solution. There should also
>> be a patch allowing pipe groups to be assigned. Without that extra bit
>> this patch does nothing , right ?
> 
> Yes you are right.
> We are having some register which will configure the pipe lock group.
> But these registers are not exposed to non-secure world. These 
> registers
> only accessible through secure world. Currently in IPQ5018 SoC we are
> configuring
> these register in secure world to configure pipe lock group.

ping! Is there any update on this ? Do you need any further info ?

Md Sadre Alam Jan. 12, 2021, 9:31 a.m. UTC | #8

On 2020-12-21 23:03, [e-mail] wrote:
> On 2020-12-21 14:53, Vinod Koul wrote:
>> Hello,
>> 
>> On 17-12-20, 20:07, Md Sadre Alam wrote:
>>> This change will add support for LOCK & UNLOCK flag bit support
>>> on CMD descriptor.
>>> 
>>> If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
>>> transaction wanted to lock the DMA controller for this transaction so
>>> BAM driver should set LOCK bit for the HW descriptor.
>>> 
>>> If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester of 
>>> this
>>> transaction wanted to unlock the DMA controller.so BAM driver should 
>>> set
>>> UNLOCK bit for the HW descriptor.
>> 
>> Can you explain why would we need to first lock and then unlock..? How
>> would this be used in real world.
>> 
>> I have read a bit of documentation but is unclear to me. Also should
>> this be exposed as an API to users, sounds like internal to driver..?
>> 
> 
> IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto 
> Hardware Engine
> will be shared between A53 core & ubi32 core. There is two separate
> driver dedicated
> to A53 core and ubi32 core. So to use Crypto Hardware Engine
> parallelly for encryption/description
> we need bam locking mechanism. if one driver will submit the request
> for encryption/description
> to Crypto then first it has to set LOCK flag bit on command descriptor
> so that other pipes will
> get locked.
> 
> The Pipe Locking/Unlocking will be only on command-descriptor. Upon
> encountering a command descriptor
> with LOCK bit set, The BAM will lock all other pipes not related to
> the current pipe group, and keep
> handling the current pipe only until it sees the UNLOCK set then it
> will release all locked pipes.
> locked pipe will not fetch new descriptors even if it got event/events
> adding more descriptors for
> this pipe (locked pipe).
> 
> No need to expose as an API to user because its internal to driver, so
> while preparing command descriptor
> just we have to update the LOCK/UNLOCK flag.


ping! Is there any update on this ? Do you need any further info ?
> 
> 
>> 
>>> 
>>> Signed-off-by: Md Sadre Alam <[e-mail]>
>>> ---
>>>  Documentation/driver-api/dmaengine/provider.rst | 9 +++++++++
>>>  drivers/dma/qcom/bam_dma.c                      | 9 +++++++++
>>>  include/linux/dmaengine.h                       | 5 +++++
>>>  3 files changed, 23 insertions(+)
>>> 
>>> diff --git a/Documentation/driver-api/dmaengine/provider.rst 
>>> b/Documentation/driver-api/dmaengine/provider.rst
>>> index ddb0a81..d7516e2 100644
>>> --- a/Documentation/driver-api/dmaengine/provider.rst
>>> +++ b/Documentation/driver-api/dmaengine/provider.rst
>>> @@ -599,6 +599,15 @@ DMA_CTRL_REUSE
>>>    - This flag is only supported if the channel reports the 
>>> DMA_LOAD_EOT
>>>      capability.
>>> 
>>> +- DMA_PREP_LOCK
>>> +
>>> +  - If set , the client driver tells DMA controller I am locking you 
>>> for
>>> +    this transcation.
>>> +
>>> +- DMA_PREP_UNLOCK
>>> +
>>> +  - If set, the client driver will tells DMA controller I am 
>>> releasing the lock
>>> +
>>>  General Design Notes
>>>  ====================
>>> 
>>> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
>>> index 4eeb8bb..cdbe395 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;
>>> @@ -644,6 +646,13 @@ static struct dma_async_tx_descriptor 
>>> *bam_prep_slave_sg(struct dma_chan *chan,
>>> 
>>>  	/* fill in temporary descriptors */
>>>  	desc = async_desc->desc;
>>> +	if (flags & DMA_PREP_CMD) {
>>> +		if (flags & DMA_PREP_LOCK)
>>> +			desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
>>> +		if (flags & DMA_PREP_UNLOCK)
>>> +			desc->flags |= cpu_to_le16(DESC_FLAG_UNLOCK);
>>> +	}
>>> +
>>>  	for_each_sg(sgl, sg, sg_len, i) {
>>>  		unsigned int remainder = sg_dma_len(sg);
>>>  		unsigned int curr_offset = 0;
>>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>>> index dd357a7..79ccadb4 100644
>>> --- a/include/linux/dmaengine.h
>>> +++ b/include/linux/dmaengine.h
>>> @@ -190,6 +190,9 @@ struct dma_interleaved_template {
>>>   *  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 DMA HW engine going to be 
>>> locked for this
>>> + *  transaction , until not seen DMA_PREP_UNLOCK flag set.
>>> + * @DMA_PREP_UNLOCK: tell the driver to unlock the DMA HW engine.
>>>   */
>>>  enum dma_ctrl_flags {
>>>  	DMA_PREP_INTERRUPT = (1 << 0),
>>> @@ -202,6 +205,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.7.4

Vinod Koul Jan. 12, 2021, 10:10 a.m. UTC | #9

On 12-01-21, 15:01, [e-mail] wrote:
> On 2020-12-21 23:03, [e-mail] wrote:
> > On 2020-12-21 14:53, Vinod Koul wrote:
> > > Hello,
> > > 
> > > On 17-12-20, 20:07, Md Sadre Alam wrote:
> > > > This change will add support for LOCK & UNLOCK flag bit support
> > > > on CMD descriptor.
> > > > 
> > > > If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
> > > > transaction wanted to lock the DMA controller for this transaction so
> > > > BAM driver should set LOCK bit for the HW descriptor.
> > > > 
> > > > If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester
> > > > of this
> > > > transaction wanted to unlock the DMA controller.so BAM driver
> > > > should set
> > > > UNLOCK bit for the HW descriptor.
> > > 
> > > Can you explain why would we need to first lock and then unlock..? How
> > > would this be used in real world.
> > > 
> > > I have read a bit of documentation but is unclear to me. Also should
> > > this be exposed as an API to users, sounds like internal to driver..?
> > > 
> > 
> > IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto Hardware
> > Engine
> > will be shared between A53 core & ubi32 core. There is two separate
> > driver dedicated
> > to A53 core and ubi32 core. So to use Crypto Hardware Engine
> > parallelly for encryption/description
> > we need bam locking mechanism. if one driver will submit the request
> > for encryption/description
> > to Crypto then first it has to set LOCK flag bit on command descriptor
> > so that other pipes will
> > get locked.
> > 
> > The Pipe Locking/Unlocking will be only on command-descriptor. Upon
> > encountering a command descriptor

Can you explain what is a cmd descriptor?

> > with LOCK bit set, The BAM will lock all other pipes not related to
> > the current pipe group, and keep
> > handling the current pipe only until it sees the UNLOCK set then it
> > will release all locked pipes.
> > locked pipe will not fetch new descriptors even if it got event/events
> > adding more descriptors for
> > this pipe (locked pipe).
> > 
> > No need to expose as an API to user because its internal to driver, so
> > while preparing command descriptor
> > just we have to update the LOCK/UNLOCK flag.

So IIUC, no api right? it would be internal to driver..?

Md Sadre Alam Jan. 13, 2021, 7:50 p.m. UTC | #10

On 2021-01-12 15:40, Vinod Koul wrote:
> On 12-01-21, 15:01, [e-mail] wrote:
>> On 2020-12-21 23:03, [e-mail] wrote:
>> > On 2020-12-21 14:53, Vinod Koul wrote:
>> > > Hello,
>> > >
>> > > On 17-12-20, 20:07, Md Sadre Alam wrote:
>> > > > This change will add support for LOCK & UNLOCK flag bit support
>> > > > on CMD descriptor.
>> > > >
>> > > > If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
>> > > > transaction wanted to lock the DMA controller for this transaction so
>> > > > BAM driver should set LOCK bit for the HW descriptor.
>> > > >
>> > > > If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester
>> > > > of this
>> > > > transaction wanted to unlock the DMA controller.so BAM driver
>> > > > should set
>> > > > UNLOCK bit for the HW descriptor.
>> > >
>> > > Can you explain why would we need to first lock and then unlock..? How
>> > > would this be used in real world.
>> > >
>> > > I have read a bit of documentation but is unclear to me. Also should
>> > > this be exposed as an API to users, sounds like internal to driver..?
>> > >
>> >
>> > IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto Hardware
>> > Engine
>> > will be shared between A53 core & ubi32 core. There is two separate
>> > driver dedicated
>> > to A53 core and ubi32 core. So to use Crypto Hardware Engine
>> > parallelly for encryption/description
>> > we need bam locking mechanism. if one driver will submit the request
>> > for encryption/description
>> > to Crypto then first it has to set LOCK flag bit on command descriptor
>> > so that other pipes will
>> > get locked.
>> >
>> > The Pipe Locking/Unlocking will be only on command-descriptor. Upon
>> > encountering a command descriptor
> 
> Can you explain what is a cmd descriptor?

   In BAM pipe descriptor structure there is a field called CMD (Command 
descriptor).
   CMD allows the SW to create descriptors of type Command which does not 
generate any data transmissions
   but configures registers in the Peripheral (write operations, and read 
registers operations ).
   Using command descriptor enables the SW to queue new configurations 
between data transfers in advance.

> 
>> > with LOCK bit set, The BAM will lock all other pipes not related to
>> > the current pipe group, and keep
>> > handling the current pipe only until it sees the UNLOCK set then it
>> > will release all locked pipes.
>> > locked pipe will not fetch new descriptors even if it got event/events
>> > adding more descriptors for
>> > this pipe (locked pipe).
>> >
>> > No need to expose as an API to user because its internal to driver, so
>> > while preparing command descriptor
>> > just we have to update the LOCK/UNLOCK flag.
> 
> So IIUC, no api right? it would be internal to driver..?

   Yes its totally internal to deriver.

Vinod Koul Jan. 15, 2021, 5:58 a.m. UTC | #11

On 14-01-21, 01:20, [e-mail] wrote:
> On 2021-01-12 15:40, Vinod Koul wrote:
> > On 12-01-21, 15:01, [e-mail] wrote:
> > > On 2020-12-21 23:03, [e-mail] wrote:
> > > > On 2020-12-21 14:53, Vinod Koul wrote:
> > > > > Hello,
> > > > >
> > > > > On 17-12-20, 20:07, Md Sadre Alam wrote:
> > > > > > This change will add support for LOCK & UNLOCK flag bit support
> > > > > > on CMD descriptor.
> > > > > >
> > > > > > If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
> > > > > > transaction wanted to lock the DMA controller for this transaction so
> > > > > > BAM driver should set LOCK bit for the HW descriptor.
> > > > > >
> > > > > > If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester
> > > > > > of this
> > > > > > transaction wanted to unlock the DMA controller.so BAM driver
> > > > > > should set
> > > > > > UNLOCK bit for the HW descriptor.
> > > > >
> > > > > Can you explain why would we need to first lock and then unlock..? How
> > > > > would this be used in real world.
> > > > >
> > > > > I have read a bit of documentation but is unclear to me. Also should
> > > > > this be exposed as an API to users, sounds like internal to driver..?
> > > > >
> > > >
> > > > IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto Hardware
> > > > Engine
> > > > will be shared between A53 core & ubi32 core. There is two separate
> > > > driver dedicated
> > > > to A53 core and ubi32 core. So to use Crypto Hardware Engine
> > > > parallelly for encryption/description
> > > > we need bam locking mechanism. if one driver will submit the request
> > > > for encryption/description
> > > > to Crypto then first it has to set LOCK flag bit on command descriptor
> > > > so that other pipes will
> > > > get locked.
> > > >
> > > > The Pipe Locking/Unlocking will be only on command-descriptor. Upon
> > > > encountering a command descriptor
> > 
> > Can you explain what is a cmd descriptor?
> 
>   In BAM pipe descriptor structure there is a field called CMD (Command
> descriptor).
>   CMD allows the SW to create descriptors of type Command which does not
> generate any data transmissions
>   but configures registers in the Peripheral (write operations, and read
> registers operations ).
>   Using command descriptor enables the SW to queue new configurations
> between data transfers in advance.

What and when is the CMD descriptor used for..?

> > 
> > > > with LOCK bit set, The BAM will lock all other pipes not related to
> > > > the current pipe group, and keep
> > > > handling the current pipe only until it sees the UNLOCK set then it
> > > > will release all locked pipes.
> > > > locked pipe will not fetch new descriptors even if it got event/events
> > > > adding more descriptors for
> > > > this pipe (locked pipe).
> > > >
> > > > No need to expose as an API to user because its internal to driver, so
> > > > while preparing command descriptor
> > > > just we have to update the LOCK/UNLOCK flag.
> > 
> > So IIUC, no api right? it would be internal to driver..?
> 
>   Yes its totally internal to deriver.

So no need for this patch then, right?

Md Sadre Alam Jan. 18, 2021, 3:51 a.m. UTC | #12

On 2021-01-15 11:28, Vinod Koul wrote:
> On 14-01-21, 01:20, [e-mail] wrote:
>> On 2021-01-12 15:40, Vinod Koul wrote:
>> > On 12-01-21, 15:01, [e-mail] wrote:
>> > > On 2020-12-21 23:03, [e-mail] wrote:
>> > > > On 2020-12-21 14:53, Vinod Koul wrote:
>> > > > > Hello,
>> > > > >
>> > > > > On 17-12-20, 20:07, Md Sadre Alam wrote:
>> > > > > > This change will add support for LOCK & UNLOCK flag bit support
>> > > > > > on CMD descriptor.
>> > > > > >
>> > > > > > If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
>> > > > > > transaction wanted to lock the DMA controller for this transaction so
>> > > > > > BAM driver should set LOCK bit for the HW descriptor.
>> > > > > >
>> > > > > > If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester
>> > > > > > of this
>> > > > > > transaction wanted to unlock the DMA controller.so BAM driver
>> > > > > > should set
>> > > > > > UNLOCK bit for the HW descriptor.
>> > > > >
>> > > > > Can you explain why would we need to first lock and then unlock..? How
>> > > > > would this be used in real world.
>> > > > >
>> > > > > I have read a bit of documentation but is unclear to me. Also should
>> > > > > this be exposed as an API to users, sounds like internal to driver..?
>> > > > >
>> > > >
>> > > > IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto Hardware
>> > > > Engine
>> > > > will be shared between A53 core & ubi32 core. There is two separate
>> > > > driver dedicated
>> > > > to A53 core and ubi32 core. So to use Crypto Hardware Engine
>> > > > parallelly for encryption/description
>> > > > we need bam locking mechanism. if one driver will submit the request
>> > > > for encryption/description
>> > > > to Crypto then first it has to set LOCK flag bit on command descriptor
>> > > > so that other pipes will
>> > > > get locked.
>> > > >
>> > > > The Pipe Locking/Unlocking will be only on command-descriptor. Upon
>> > > > encountering a command descriptor
>> >
>> > Can you explain what is a cmd descriptor?
>> 
>>   In BAM pipe descriptor structure there is a field called CMD 
>> (Command
>> descriptor).
>>   CMD allows the SW to create descriptors of type Command which does 
>> not
>> generate any data transmissions
>>   but configures registers in the Peripheral (write operations, and 
>> read
>> registers operations ).
>>   Using command descriptor enables the SW to queue new configurations
>> between data transfers in advance.
> 
> What and when is the CMD descriptor used for..?

   CMD descriptor is mainly used for configuring controller register.
   We can read/write controller register via BAM using CMD descriptor 
only.
   CMD descriptor use command pipe for the transaction.
> 
>> >
>> > > > with LOCK bit set, The BAM will lock all other pipes not related to
>> > > > the current pipe group, and keep
>> > > > handling the current pipe only until it sees the UNLOCK set then it
>> > > > will release all locked pipes.
>> > > > locked pipe will not fetch new descriptors even if it got event/events
>> > > > adding more descriptors for
>> > > > this pipe (locked pipe).
>> > > >
>> > > > No need to expose as an API to user because its internal to driver, so
>> > > > while preparing command descriptor
>> > > > just we have to update the LOCK/UNLOCK flag.
>> >
>> > So IIUC, no api right? it would be internal to driver..?
>> 
>>   Yes its totally internal to deriver.
> 
> So no need for this patch then, right?

   This patch is needed , because if some hardware will shared between 
multiple core
   like A53 and ubi32 for example. In IPQ5018 there is only one crypto 
engine and this will
   be shared between A53 core and ubi32 core and in A53 core & ubi32 core 
there are different
   drivers is getting used. So if encryption/decryption request come at 
same time from both the
   driver then things will get messed up. So here we need LOCKING 
mechanism. If first request is
   from A53 core driver then this driver should lock all the pipes other 
than pipe dedicated to
   A53 core. So while preparing CMD descriptor driver should used this 
flag "DMA_PREP_LOCK",
   Since LOCK and UNLOCK flag bit we can set only on CMD descriptor. Once 
request processed then
   driver will set UNLOCK flag on CMD descriptor. Driver should use this 
flag "DMA_PREP_UNLOCK"
   while preparing CMD descriptor. Same logic will be apply for ubi32 
core driver as well.

Vinod Koul Jan. 19, 2021, 4:45 p.m. UTC | #13

On 18-01-21, 09:21, [e-mail] wrote:
> On 2021-01-15 11:28, Vinod Koul wrote:
> > On 14-01-21, 01:20, [e-mail] wrote:
> > > On 2021-01-12 15:40, Vinod Koul wrote:
> > > > On 12-01-21, 15:01, [e-mail] wrote:
> > > > > On 2020-12-21 23:03, [e-mail] wrote:
> > > > > > On 2020-12-21 14:53, Vinod Koul wrote:
> > > > > > > Hello,
> > > > > > >
> > > > > > > On 17-12-20, 20:07, Md Sadre Alam wrote:
> > > > > > > > This change will add support for LOCK & UNLOCK flag bit support
> > > > > > > > on CMD descriptor.
> > > > > > > >
> > > > > > > > If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
> > > > > > > > transaction wanted to lock the DMA controller for this transaction so
> > > > > > > > BAM driver should set LOCK bit for the HW descriptor.
> > > > > > > >
> > > > > > > > If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester
> > > > > > > > of this
> > > > > > > > transaction wanted to unlock the DMA controller.so BAM driver
> > > > > > > > should set
> > > > > > > > UNLOCK bit for the HW descriptor.
> > > > > > >
> > > > > > > Can you explain why would we need to first lock and then unlock..? How
> > > > > > > would this be used in real world.
> > > > > > >
> > > > > > > I have read a bit of documentation but is unclear to me. Also should
> > > > > > > this be exposed as an API to users, sounds like internal to driver..?
> > > > > > >
> > > > > >
> > > > > > IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto Hardware
> > > > > > Engine
> > > > > > will be shared between A53 core & ubi32 core. There is two separate
> > > > > > driver dedicated
> > > > > > to A53 core and ubi32 core. So to use Crypto Hardware Engine
> > > > > > parallelly for encryption/description
> > > > > > we need bam locking mechanism. if one driver will submit the request
> > > > > > for encryption/description
> > > > > > to Crypto then first it has to set LOCK flag bit on command descriptor
> > > > > > so that other pipes will
> > > > > > get locked.
> > > > > >
> > > > > > The Pipe Locking/Unlocking will be only on command-descriptor. Upon
> > > > > > encountering a command descriptor
> > > >
> > > > Can you explain what is a cmd descriptor?
> > > 
> > >   In BAM pipe descriptor structure there is a field called CMD
> > > (Command
> > > descriptor).
> > >   CMD allows the SW to create descriptors of type Command which does
> > > not
> > > generate any data transmissions
> > >   but configures registers in the Peripheral (write operations, and
> > > read
> > > registers operations ).
> > >   Using command descriptor enables the SW to queue new configurations
> > > between data transfers in advance.
> > 
> > What and when is the CMD descriptor used for..?
> 
>   CMD descriptor is mainly used for configuring controller register.
>   We can read/write controller register via BAM using CMD descriptor only.
>   CMD descriptor use command pipe for the transaction.

In which use cases would you need to issue cmd descriptors..?

> > 
> > > >
> > > > > > with LOCK bit set, The BAM will lock all other pipes not related to
> > > > > > the current pipe group, and keep
> > > > > > handling the current pipe only until it sees the UNLOCK set then it
> > > > > > will release all locked pipes.
> > > > > > locked pipe will not fetch new descriptors even if it got event/events
> > > > > > adding more descriptors for
> > > > > > this pipe (locked pipe).
> > > > > >
> > > > > > No need to expose as an API to user because its internal to driver, so
> > > > > > while preparing command descriptor
> > > > > > just we have to update the LOCK/UNLOCK flag.
> > > >
> > > > So IIUC, no api right? it would be internal to driver..?
> > > 
> > >   Yes its totally internal to deriver.
> > 
> > So no need for this patch then, right?
> 
>   This patch is needed , because if some hardware will shared between
>   multiple core like A53 and ubi32 for example. In IPQ5018 there is
>   only one crypto engine and this will be shared between A53 core and
>   ubi32 core and in A53 core & ubi32 core there are different drivers
>   is getting used. So if encryption/decryption request come at same
>   time from both the driver then things will get messed up. So here we
>   need LOCKING mechanism.  If first request is from A53 core driver
>   then this driver should lock all the pipes other than pipe dedicated
>   to A53 core. So while preparing CMD descriptor driver should used
>   this flag "DMA_PREP_LOCK", Since LOCK and UNLOCK flag bit we can set
>   only on CMD descriptor. Once request processed then driver will set
>   UNLOCK flag on CMD descriptor. Driver should use this flag
>   "DMA_PREP_UNLOCK" while preparing CMD descriptor. Same logic will be
>   apply for ubi32 core driver as well.

Why cant this be applied at driver level, based on txn being issued it
can lock issue the txn and then unlock when done. I am not convinced yet
that this needs to be exported to users and can be managed by dmaengine
driver.

Thanks

Md Sadre Alam Jan. 27, 2021, 6:26 p.m. UTC | #14

On 2021-01-19 22:15, Vinod Koul wrote:
> On 18-01-21, 09:21, [e-mail] wrote:
>> On 2021-01-15 11:28, Vinod Koul wrote:
>> > On 14-01-21, 01:20, [e-mail] wrote:
>> > > On 2021-01-12 15:40, Vinod Koul wrote:
>> > > > On 12-01-21, 15:01, [e-mail] wrote:
>> > > > > On 2020-12-21 23:03, [e-mail] wrote:
>> > > > > > On 2020-12-21 14:53, Vinod Koul wrote:
>> > > > > > > Hello,
>> > > > > > >
>> > > > > > > On 17-12-20, 20:07, Md Sadre Alam wrote:
>> > > > > > > > This change will add support for LOCK & UNLOCK flag bit support
>> > > > > > > > on CMD descriptor.
>> > > > > > > >
>> > > > > > > > If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
>> > > > > > > > transaction wanted to lock the DMA controller for this transaction so
>> > > > > > > > BAM driver should set LOCK bit for the HW descriptor.
>> > > > > > > >
>> > > > > > > > If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester
>> > > > > > > > of this
>> > > > > > > > transaction wanted to unlock the DMA controller.so BAM driver
>> > > > > > > > should set
>> > > > > > > > UNLOCK bit for the HW descriptor.
>> > > > > > >
>> > > > > > > Can you explain why would we need to first lock and then unlock..? How
>> > > > > > > would this be used in real world.
>> > > > > > >
>> > > > > > > I have read a bit of documentation but is unclear to me. Also should
>> > > > > > > this be exposed as an API to users, sounds like internal to driver..?
>> > > > > > >
>> > > > > >
>> > > > > > IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto Hardware
>> > > > > > Engine
>> > > > > > will be shared between A53 core & ubi32 core. There is two separate
>> > > > > > driver dedicated
>> > > > > > to A53 core and ubi32 core. So to use Crypto Hardware Engine
>> > > > > > parallelly for encryption/description
>> > > > > > we need bam locking mechanism. if one driver will submit the request
>> > > > > > for encryption/description
>> > > > > > to Crypto then first it has to set LOCK flag bit on command descriptor
>> > > > > > so that other pipes will
>> > > > > > get locked.
>> > > > > >
>> > > > > > The Pipe Locking/Unlocking will be only on command-descriptor. Upon
>> > > > > > encountering a command descriptor
>> > > >
>> > > > Can you explain what is a cmd descriptor?
>> > >
>> > >   In BAM pipe descriptor structure there is a field called CMD
>> > > (Command
>> > > descriptor).
>> > >   CMD allows the SW to create descriptors of type Command which does
>> > > not
>> > > generate any data transmissions
>> > >   but configures registers in the Peripheral (write operations, and
>> > > read
>> > > registers operations ).
>> > >   Using command descriptor enables the SW to queue new configurations
>> > > between data transfers in advance.
>> >
>> > What and when is the CMD descriptor used for..?
>> 
>>   CMD descriptor is mainly used for configuring controller register.
>>   We can read/write controller register via BAM using CMD descriptor 
>> only.
>>   CMD descriptor use command pipe for the transaction.
> 
> In which use cases would you need to issue cmd descriptors..?

   In IPQ5018 there is only one Crypto engine and it will get shared 
between
   UBI32 core & A53 core. So here we need to use command descriptor 
in-order to
   perform LOCKING/UNLOCKING mechanism. Since LOCK/ULOCK flag we can set 
only on
   CMD descriptor.
> 
>> >
>> > > >
>> > > > > > with LOCK bit set, The BAM will lock all other pipes not related to
>> > > > > > the current pipe group, and keep
>> > > > > > handling the current pipe only until it sees the UNLOCK set then it
>> > > > > > will release all locked pipes.
>> > > > > > locked pipe will not fetch new descriptors even if it got event/events
>> > > > > > adding more descriptors for
>> > > > > > this pipe (locked pipe).
>> > > > > >
>> > > > > > No need to expose as an API to user because its internal to driver, so
>> > > > > > while preparing command descriptor
>> > > > > > just we have to update the LOCK/UNLOCK flag.
>> > > >
>> > > > So IIUC, no api right? it would be internal to driver..?
>> > >
>> > >   Yes its totally internal to deriver.
>> >
>> > So no need for this patch then, right?
>> 
>>   This patch is needed , because if some hardware will shared between
>>   multiple core like A53 and ubi32 for example. In IPQ5018 there is
>>   only one crypto engine and this will be shared between A53 core and
>>   ubi32 core and in A53 core & ubi32 core there are different drivers
>>   is getting used. So if encryption/decryption request come at same
>>   time from both the driver then things will get messed up. So here we
>>   need LOCKING mechanism.  If first request is from A53 core driver
>>   then this driver should lock all the pipes other than pipe dedicated
>>   to A53 core. So while preparing CMD descriptor driver should used
>>   this flag "DMA_PREP_LOCK", Since LOCK and UNLOCK flag bit we can set
>>   only on CMD descriptor. Once request processed then driver will set
>>   UNLOCK flag on CMD descriptor. Driver should use this flag
>>   "DMA_PREP_UNLOCK" while preparing CMD descriptor. Same logic will be
>>   apply for ubi32 core driver as well.
> 
> Why cant this be applied at driver level, based on txn being issued it
> can lock issue the txn and then unlock when done. I am not convinced 
> yet
> that this needs to be exported to users and can be managed by dmaengine
> driver.

   The actual LOCK/UNLOCK flag should be set on hardware command 
descriptor.
   so this flag setting should be done in DMA engine driver. The user of 
the DMA
   driver like (in case of IPQ5018) Crypto can use flag "DMA_PREP_LOCK" & 
"DMA_PREP_UNLOCK"
   while preparing CMD descriptor before submitting to the DMA engine. In 
DMA engine driver
   we are checking these flasgs on CMD descriptor and setting actual 
LOCK/UNLOCK flag on hardware
   descriptor.

    if (flags & DMA_PREP_CMD) { <== check for descriptor type
		if (flags & DMA_PREP_LOCK)
			desc->flags |= cpu_to_le16(DESC_FLAG_LOCK); <== Actual LOCK flag 
setting on HW descriptor.
		if (flags & DMA_PREP_UNLOCK)
			desc->flags |= cpu_to_le16(DESC_FLAG_UNLOCK); <== Actual UNLOCK flag 
setting on HW descriptor.
	}
> 
> Thanks

Vinod Koul Feb. 1, 2021, 6:05 a.m. UTC | #15

On 27-01-21, 23:56, [e-mail] wrote:
> On 2021-01-19 22:15, Vinod Koul wrote:
> > On 18-01-21, 09:21, [e-mail] wrote:
> > > On 2021-01-15 11:28, Vinod Koul wrote:
> > > > On 14-01-21, 01:20, [e-mail] wrote:
> > > > > On 2021-01-12 15:40, Vinod Koul wrote:
> > > > > > On 12-01-21, 15:01, [e-mail] wrote:
> > > > > > > On 2020-12-21 23:03, [e-mail] wrote:
> > > > > > > > On 2020-12-21 14:53, Vinod Koul wrote:
> > > > > > > > > Hello,
> > > > > > > > >
> > > > > > > > > On 17-12-20, 20:07, Md Sadre Alam wrote:
> > > > > > > > > > This change will add support for LOCK & UNLOCK flag bit support
> > > > > > > > > > on CMD descriptor.
> > > > > > > > > >
> > > > > > > > > > If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
> > > > > > > > > > transaction wanted to lock the DMA controller for this transaction so
> > > > > > > > > > BAM driver should set LOCK bit for the HW descriptor.
> > > > > > > > > >
> > > > > > > > > > If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester
> > > > > > > > > > of this
> > > > > > > > > > transaction wanted to unlock the DMA controller.so BAM driver
> > > > > > > > > > should set
> > > > > > > > > > UNLOCK bit for the HW descriptor.
> > > > > > > > >
> > > > > > > > > Can you explain why would we need to first lock and then unlock..? How
> > > > > > > > > would this be used in real world.
> > > > > > > > >
> > > > > > > > > I have read a bit of documentation but is unclear to me. Also should
> > > > > > > > > this be exposed as an API to users, sounds like internal to driver..?
> > > > > > > > >
> > > > > > > >
> > > > > > > > IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto Hardware
> > > > > > > > Engine
> > > > > > > > will be shared between A53 core & ubi32 core. There is two separate
> > > > > > > > driver dedicated
> > > > > > > > to A53 core and ubi32 core. So to use Crypto Hardware Engine
> > > > > > > > parallelly for encryption/description
> > > > > > > > we need bam locking mechanism. if one driver will submit the request
> > > > > > > > for encryption/description
> > > > > > > > to Crypto then first it has to set LOCK flag bit on command descriptor
> > > > > > > > so that other pipes will
> > > > > > > > get locked.
> > > > > > > >
> > > > > > > > The Pipe Locking/Unlocking will be only on command-descriptor. Upon
> > > > > > > > encountering a command descriptor
> > > > > >
> > > > > > Can you explain what is a cmd descriptor?
> > > > >
> > > > >   In BAM pipe descriptor structure there is a field called CMD
> > > > > (Command
> > > > > descriptor).
> > > > >   CMD allows the SW to create descriptors of type Command which does
> > > > > not
> > > > > generate any data transmissions
> > > > >   but configures registers in the Peripheral (write operations, and
> > > > > read
> > > > > registers operations ).
> > > > >   Using command descriptor enables the SW to queue new configurations
> > > > > between data transfers in advance.
> > > >
> > > > What and when is the CMD descriptor used for..?
> > > 
> > >   CMD descriptor is mainly used for configuring controller register.
> > >   We can read/write controller register via BAM using CMD descriptor
> > > only.
> > >   CMD descriptor use command pipe for the transaction.
> > 
> > In which use cases would you need to issue cmd descriptors..?
> 
>   In IPQ5018 there is only one Crypto engine and it will get shared
>   between UBI32 core & A53 core. So here we need to use command
>   descriptor in-order to perform LOCKING/UNLOCKING mechanism. Since
>   LOCK/ULOCK flag we can set only on CMD descriptor.

So when will lock/unlock be performed? Can you please explain that..

> > 
> > > >
> > > > > >
> > > > > > > > with LOCK bit set, The BAM will lock all other pipes not related to
> > > > > > > > the current pipe group, and keep
> > > > > > > > handling the current pipe only until it sees the UNLOCK set then it
> > > > > > > > will release all locked pipes.
> > > > > > > > locked pipe will not fetch new descriptors even if it got event/events
> > > > > > > > adding more descriptors for
> > > > > > > > this pipe (locked pipe).
> > > > > > > >
> > > > > > > > No need to expose as an API to user because its internal to driver, so
> > > > > > > > while preparing command descriptor
> > > > > > > > just we have to update the LOCK/UNLOCK flag.
> > > > > >
> > > > > > So IIUC, no api right? it would be internal to driver..?
> > > > >
> > > > >   Yes its totally internal to deriver.
> > > >
> > > > So no need for this patch then, right?
> > > 
> > >   This patch is needed , because if some hardware will shared between
> > >   multiple core like A53 and ubi32 for example. In IPQ5018 there is
> > >   only one crypto engine and this will be shared between A53 core and
> > >   ubi32 core and in A53 core & ubi32 core there are different drivers
> > >   is getting used. So if encryption/decryption request come at same
> > >   time from both the driver then things will get messed up. So here we
> > >   need LOCKING mechanism.  If first request is from A53 core driver
> > >   then this driver should lock all the pipes other than pipe dedicated
> > >   to A53 core. So while preparing CMD descriptor driver should used
> > >   this flag "DMA_PREP_LOCK", Since LOCK and UNLOCK flag bit we can set
> > >   only on CMD descriptor. Once request processed then driver will set
> > >   UNLOCK flag on CMD descriptor. Driver should use this flag
> > >   "DMA_PREP_UNLOCK" while preparing CMD descriptor. Same logic will be
> > >   apply for ubi32 core driver as well.
> > 
> > Why cant this be applied at driver level, based on txn being issued it
> > can lock issue the txn and then unlock when done. I am not convinced yet
> > that this needs to be exported to users and can be managed by dmaengine
> > driver.
> 
>   The actual LOCK/UNLOCK flag should be set on hardware command descriptor.
>   so this flag setting should be done in DMA engine driver. The user of the
> DMA
>   driver like (in case of IPQ5018) Crypto can use flag "DMA_PREP_LOCK" &
> "DMA_PREP_UNLOCK"
>   while preparing CMD descriptor before submitting to the DMA engine. In DMA
> engine driver
>   we are checking these flasgs on CMD descriptor and setting actual
> LOCK/UNLOCK flag on hardware
>   descriptor.


I am not sure I comprehend this yet.. when is that we would need to do
this... is this for each txn submitted to dmaengine.. or something
else..

> 
>    if (flags & DMA_PREP_CMD) { <== check for descriptor type
> 		if (flags & DMA_PREP_LOCK)
> 			desc->flags |= cpu_to_le16(DESC_FLAG_LOCK); <== Actual LOCK flag setting
> on HW descriptor.
> 		if (flags & DMA_PREP_UNLOCK)
> 			desc->flags |= cpu_to_le16(DESC_FLAG_UNLOCK); <== Actual UNLOCK flag
> setting on HW descriptor.
> 	}
> > 
> > Thanks

Md Sadre Alam Feb. 1, 2021, 6:22 a.m. UTC | #16

On 2021-02-01 11:35, Vinod Koul wrote:
> On 27-01-21, 23:56, [e-mail] wrote:
>> On 2021-01-19 22:15, Vinod Koul wrote:
>> > On 18-01-21, 09:21, [e-mail] wrote:
>> > > On 2021-01-15 11:28, Vinod Koul wrote:
>> > > > On 14-01-21, 01:20, [e-mail] wrote:
>> > > > > On 2021-01-12 15:40, Vinod Koul wrote:
>> > > > > > On 12-01-21, 15:01, [e-mail] wrote:
>> > > > > > > On 2020-12-21 23:03, [e-mail] wrote:
>> > > > > > > > On 2020-12-21 14:53, Vinod Koul wrote:
>> > > > > > > > > Hello,
>> > > > > > > > >
>> > > > > > > > > On 17-12-20, 20:07, Md Sadre Alam wrote:
>> > > > > > > > > > This change will add support for LOCK & UNLOCK flag bit support
>> > > > > > > > > > on CMD descriptor.
>> > > > > > > > > >
>> > > > > > > > > > If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
>> > > > > > > > > > transaction wanted to lock the DMA controller for this transaction so
>> > > > > > > > > > BAM driver should set LOCK bit for the HW descriptor.
>> > > > > > > > > >
>> > > > > > > > > > If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester
>> > > > > > > > > > of this
>> > > > > > > > > > transaction wanted to unlock the DMA controller.so BAM driver
>> > > > > > > > > > should set
>> > > > > > > > > > UNLOCK bit for the HW descriptor.
>> > > > > > > > >
>> > > > > > > > > Can you explain why would we need to first lock and then unlock..? How
>> > > > > > > > > would this be used in real world.
>> > > > > > > > >
>> > > > > > > > > I have read a bit of documentation but is unclear to me. Also should
>> > > > > > > > > this be exposed as an API to users, sounds like internal to driver..?
>> > > > > > > > >
>> > > > > > > >
>> > > > > > > > IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto Hardware
>> > > > > > > > Engine
>> > > > > > > > will be shared between A53 core & ubi32 core. There is two separate
>> > > > > > > > driver dedicated
>> > > > > > > > to A53 core and ubi32 core. So to use Crypto Hardware Engine
>> > > > > > > > parallelly for encryption/description
>> > > > > > > > we need bam locking mechanism. if one driver will submit the request
>> > > > > > > > for encryption/description
>> > > > > > > > to Crypto then first it has to set LOCK flag bit on command descriptor
>> > > > > > > > so that other pipes will
>> > > > > > > > get locked.
>> > > > > > > >
>> > > > > > > > The Pipe Locking/Unlocking will be only on command-descriptor. Upon
>> > > > > > > > encountering a command descriptor
>> > > > > >
>> > > > > > Can you explain what is a cmd descriptor?
>> > > > >
>> > > > >   In BAM pipe descriptor structure there is a field called CMD
>> > > > > (Command
>> > > > > descriptor).
>> > > > >   CMD allows the SW to create descriptors of type Command which does
>> > > > > not
>> > > > > generate any data transmissions
>> > > > >   but configures registers in the Peripheral (write operations, and
>> > > > > read
>> > > > > registers operations ).
>> > > > >   Using command descriptor enables the SW to queue new configurations
>> > > > > between data transfers in advance.
>> > > >
>> > > > What and when is the CMD descriptor used for..?
>> > >
>> > >   CMD descriptor is mainly used for configuring controller register.
>> > >   We can read/write controller register via BAM using CMD descriptor
>> > > only.
>> > >   CMD descriptor use command pipe for the transaction.
>> >
>> > In which use cases would you need to issue cmd descriptors..?
>> 
>>   In IPQ5018 there is only one Crypto engine and it will get shared
>>   between UBI32 core & A53 core. So here we need to use command
>>   descriptor in-order to perform LOCKING/UNLOCKING mechanism. Since
>>   LOCK/ULOCK flag we can set only on CMD descriptor.
> 
> So when will lock/unlock be performed? Can you please explain that..

   LOCK/UNLOCK will be performed when two different driver wanted to use 
the
   same HW. eg. In IPQ5018 there is only one Crypto engine and it will be 
shared b/w
   UBI32 core driver and A53 core driver.

   When A53 core wanted to submit request to crypto engine via BAM then 
first it has to
   LOCK all other pipes (pipe dedicated to UBI32 core) and then trigger 
the transaction start.
   Once all the transaction will be completed the A53 core crypto driver 
will release the LOCK
   from all the pipes. Same sequence will be applicable for UBI32 core 
crypto driver as well.
   It depends whose request will come first to the crypto HW.


> 
>> >
>> > > >
>> > > > > >
>> > > > > > > > with LOCK bit set, The BAM will lock all other pipes not related to
>> > > > > > > > the current pipe group, and keep
>> > > > > > > > handling the current pipe only until it sees the UNLOCK set then it
>> > > > > > > > will release all locked pipes.
>> > > > > > > > locked pipe will not fetch new descriptors even if it got event/events
>> > > > > > > > adding more descriptors for
>> > > > > > > > this pipe (locked pipe).
>> > > > > > > >
>> > > > > > > > No need to expose as an API to user because its internal to driver, so
>> > > > > > > > while preparing command descriptor
>> > > > > > > > just we have to update the LOCK/UNLOCK flag.
>> > > > > >
>> > > > > > So IIUC, no api right? it would be internal to driver..?
>> > > > >
>> > > > >   Yes its totally internal to deriver.
>> > > >
>> > > > So no need for this patch then, right?
>> > >
>> > >   This patch is needed , because if some hardware will shared between
>> > >   multiple core like A53 and ubi32 for example. In IPQ5018 there is
>> > >   only one crypto engine and this will be shared between A53 core and
>> > >   ubi32 core and in A53 core & ubi32 core there are different drivers
>> > >   is getting used. So if encryption/decryption request come at same
>> > >   time from both the driver then things will get messed up. So here we
>> > >   need LOCKING mechanism.  If first request is from A53 core driver
>> > >   then this driver should lock all the pipes other than pipe dedicated
>> > >   to A53 core. So while preparing CMD descriptor driver should used
>> > >   this flag "DMA_PREP_LOCK", Since LOCK and UNLOCK flag bit we can set
>> > >   only on CMD descriptor. Once request processed then driver will set
>> > >   UNLOCK flag on CMD descriptor. Driver should use this flag
>> > >   "DMA_PREP_UNLOCK" while preparing CMD descriptor. Same logic will be
>> > >   apply for ubi32 core driver as well.
>> >
>> > Why cant this be applied at driver level, based on txn being issued it
>> > can lock issue the txn and then unlock when done. I am not convinced yet
>> > that this needs to be exported to users and can be managed by dmaengine
>> > driver.
>> 
>>   The actual LOCK/UNLOCK flag should be set on hardware command 
>> descriptor.
>>   so this flag setting should be done in DMA engine driver. The user 
>> of the
>> DMA
>>   driver like (in case of IPQ5018) Crypto can use flag "DMA_PREP_LOCK" 
>> &
>> "DMA_PREP_UNLOCK"
>>   while preparing CMD descriptor before submitting to the DMA engine. 
>> In DMA
>> engine driver
>>   we are checking these flasgs on CMD descriptor and setting actual
>> LOCK/UNLOCK flag on hardware
>>   descriptor.
> 
> 
> I am not sure I comprehend this yet.. when is that we would need to do
> this... is this for each txn submitted to dmaengine.. or something
> else..

  Its not for each transaction submitted to dmaengine. We have to set 
this only
  once on CMD descriptor. So when A53 crypto driver need to change the 
crypto configuration
  then first it will lock the all other pipes using setting the LOCK flag 
bit on CMD
  descriptor and then it can start the transaction , on data descriptor 
this flag will
  not get set once all transaction will be completed the A53 crypto 
driver release the lock on
  all other pipes using UNLOCK flag on CMD descriptor. So LOCK/UNLOCK 
will be only once and not for
  the each transaction.
> 
>> 
>>    if (flags & DMA_PREP_CMD) { <== check for descriptor type
>> 		if (flags & DMA_PREP_LOCK)
>> 			desc->flags |= cpu_to_le16(DESC_FLAG_LOCK); <== Actual LOCK flag 
>> setting
>> on HW descriptor.
>> 		if (flags & DMA_PREP_UNLOCK)
>> 			desc->flags |= cpu_to_le16(DESC_FLAG_UNLOCK); <== Actual UNLOCK 
>> flag
>> setting on HW descriptor.
>> 	}
>> >
>> > Thanks

Vinod Koul Feb. 1, 2021, 6:43 a.m. UTC | #17

On 01-02-21, 11:52, [e-mail] wrote:
> On 2021-02-01 11:35, Vinod Koul wrote:
> > On 27-01-21, 23:56, [e-mail] wrote:

> > >   The actual LOCK/UNLOCK flag should be set on hardware command
> > > descriptor.
> > >   so this flag setting should be done in DMA engine driver. The user
> > > of the
> > > DMA
> > >   driver like (in case of IPQ5018) Crypto can use flag
> > > "DMA_PREP_LOCK" &
> > > "DMA_PREP_UNLOCK"
> > >   while preparing CMD descriptor before submitting to the DMA
> > > engine. In DMA
> > > engine driver
> > >   we are checking these flasgs on CMD descriptor and setting actual
> > > LOCK/UNLOCK flag on hardware
> > >   descriptor.
> > 
> > 
> > I am not sure I comprehend this yet.. when is that we would need to do
> > this... is this for each txn submitted to dmaengine.. or something
> > else..
> 
>  Its not for each transaction submitted to dmaengine. We have to set this
> only
>  once on CMD descriptor. So when A53 crypto driver need to change the crypto
> configuration
>  then first it will lock the all other pipes using setting the LOCK flag bit
> on CMD
>  descriptor and then it can start the transaction , on data descriptor this
> flag will
>  not get set once all transaction will be completed the A53 crypto driver
> release the lock on
>  all other pipes using UNLOCK flag on CMD descriptor. So LOCK/UNLOCK will be
> only once and not for
>  the each transaction.

Okay so why cant the bam driver check cmd descriptor and do lock/unlock
as below, why do we need users to do this.

        if (flags & DMA_PREP_CMD) {
                do_lock_bam();

The point here is that this seems to be internal to dma and should be
handled by dma driver.

Also if we do this, it needs to be done for specific platforms..

Thanks

Md Sadre Alam Feb. 1, 2021, 3:50 p.m. UTC | #18

On 2021-02-01 12:13, Vinod Koul wrote:
> On 01-02-21, 11:52, [e-mail] wrote:
>> On 2021-02-01 11:35, Vinod Koul wrote:
>> > On 27-01-21, 23:56, [e-mail] wrote:
> 
>> > >   The actual LOCK/UNLOCK flag should be set on hardware command
>> > > descriptor.
>> > >   so this flag setting should be done in DMA engine driver. The user
>> > > of the
>> > > DMA
>> > >   driver like (in case of IPQ5018) Crypto can use flag
>> > > "DMA_PREP_LOCK" &
>> > > "DMA_PREP_UNLOCK"
>> > >   while preparing CMD descriptor before submitting to the DMA
>> > > engine. In DMA
>> > > engine driver
>> > >   we are checking these flasgs on CMD descriptor and setting actual
>> > > LOCK/UNLOCK flag on hardware
>> > >   descriptor.
>> >
>> >
>> > I am not sure I comprehend this yet.. when is that we would need to do
>> > this... is this for each txn submitted to dmaengine.. or something
>> > else..
>> 
>>  Its not for each transaction submitted to dmaengine. We have to set 
>> this
>> only
>>  once on CMD descriptor. So when A53 crypto driver need to change the 
>> crypto
>> configuration
>>  then first it will lock the all other pipes using setting the LOCK 
>> flag bit
>> on CMD
>>  descriptor and then it can start the transaction , on data descriptor 
>> this
>> flag will
>>  not get set once all transaction will be completed the A53 crypto 
>> driver
>> release the lock on
>>  all other pipes using UNLOCK flag on CMD descriptor. So LOCK/UNLOCK 
>> will be
>> only once and not for
>>  the each transaction.
> 
> Okay so why cant the bam driver check cmd descriptor and do lock/unlock
> as below, why do we need users to do this.
> 
>         if (flags & DMA_PREP_CMD) {
>                 do_lock_bam();

  User will not decide to do this LOCK/UNLOCK mechanism. It depends on 
use case.
  This LOCK/UNLOCK mechanism not required always. It needs only when 
hardware will be shared
  between different core with different driver.
  The LOCK/UNLOCK flags provides SW to enter ordering between pipes 
execution.
  (Generally, the BAM pipes are total independent from each other and 
work in parallel manner).
  This LOCK/UNLOCK flags are part of actual pipe hardware descriptor.

  Pipe descriptor having the following flags:
  INT : Interrupt
  EOT: End of transfer
  EOB: End of block
  NWD: Notify when done
  CMD: Command
  LOCK: Lock
  UNLOCK: Unlock
  etc.

  Here the BAM driver is common driver for (QPIC, Crypto, QUP etc. in 
IPQ5018)
  So here only Crypto will be shared b/w multiple cores so For crypto 
request only the LOCK/UNLOCK
  mechanism required.
  For other request like for QPIC driver, QUPT driver etc. its not 
required. So Crypto driver has to raise the flag for
  LOCK/UNLOCK while preparing CMD descriptor. The actual locking will 
happen in BAM driver only using condition
  if (flags & DMA_PREP_CMD) {
      if (flags & DMA_PREP_LOCK)
         desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
  }

  So Crypto driver should set this flag DMA_PREP_LOCK while preparing CMD 
descriptor.
  So LOCK should be set on actual hardware pipe descriptor with 
descriptor type CMD.

> 
> The point here is that this seems to be internal to dma and should be
> handled by dma driver.
> 
   This LOCK/UNLOK flags are part of actual hardware descriptor so this 
should be handled by BAM driver only.
   If we set condition like this
   if (flags & DMA_PREP_CMD) {
                 do_lock_bam();
   Then LOCK/UNLOCK will be applied for all the CMD descriptor including 
(QPIC driver, QUP driver , Crypto driver etc.).
   So this is not our intension. So we need to set this LOCK/UNLOCK only 
for the drivers it needs. So Crypto driver needs
   locking mechanism so we will set LOCK/UNLOCK flag on Crypto driver 
request only for other driver request like QPIC driver,
   QUP driver will not set this.

> Also if we do this, it needs to be done for specific platforms..
> 







> Thanks

Md Sadre Alam Feb. 9, 2021, 4:39 p.m. UTC | #19

On 2021-02-01 21:20, [e-mail] wrote:
> On 2021-02-01 12:13, Vinod Koul wrote:
>> On 01-02-21, 11:52, [e-mail] wrote:
>>> On 2021-02-01 11:35, Vinod Koul wrote:
>>> > On 27-01-21, 23:56, [e-mail] wrote:
>> 
>>> > >   The actual LOCK/UNLOCK flag should be set on hardware command
>>> > > descriptor.
>>> > >   so this flag setting should be done in DMA engine driver. The user
>>> > > of the
>>> > > DMA
>>> > >   driver like (in case of IPQ5018) Crypto can use flag
>>> > > "DMA_PREP_LOCK" &
>>> > > "DMA_PREP_UNLOCK"
>>> > >   while preparing CMD descriptor before submitting to the DMA
>>> > > engine. In DMA
>>> > > engine driver
>>> > >   we are checking these flasgs on CMD descriptor and setting actual
>>> > > LOCK/UNLOCK flag on hardware
>>> > >   descriptor.
>>> >
>>> >
>>> > I am not sure I comprehend this yet.. when is that we would need to do
>>> > this... is this for each txn submitted to dmaengine.. or something
>>> > else..
>>> 
>>>  Its not for each transaction submitted to dmaengine. We have to set 
>>> this
>>> only
>>>  once on CMD descriptor. So when A53 crypto driver need to change the 
>>> crypto
>>> configuration
>>>  then first it will lock the all other pipes using setting the LOCK 
>>> flag bit
>>> on CMD
>>>  descriptor and then it can start the transaction , on data 
>>> descriptor this
>>> flag will
>>>  not get set once all transaction will be completed the A53 crypto 
>>> driver
>>> release the lock on
>>>  all other pipes using UNLOCK flag on CMD descriptor. So LOCK/UNLOCK 
>>> will be
>>> only once and not for
>>>  the each transaction.
>> 
>> Okay so why cant the bam driver check cmd descriptor and do 
>> lock/unlock
>> as below, why do we need users to do this.
>> 
>>         if (flags & DMA_PREP_CMD) {
>>                 do_lock_bam();
> 
>  User will not decide to do this LOCK/UNLOCK mechanism. It depends on 
> use case.
>  This LOCK/UNLOCK mechanism not required always. It needs only when
> hardware will be shared
>  between different core with different driver.
>  The LOCK/UNLOCK flags provides SW to enter ordering between pipes 
> execution.
>  (Generally, the BAM pipes are total independent from each other and
> work in parallel manner).
>  This LOCK/UNLOCK flags are part of actual pipe hardware descriptor.
> 
>  Pipe descriptor having the following flags:
>  INT : Interrupt
>  EOT: End of transfer
>  EOB: End of block
>  NWD: Notify when done
>  CMD: Command
>  LOCK: Lock
>  UNLOCK: Unlock
>  etc.
> 
>  Here the BAM driver is common driver for (QPIC, Crypto, QUP etc. in 
> IPQ5018)
>  So here only Crypto will be shared b/w multiple cores so For crypto
> request only the LOCK/UNLOCK
>  mechanism required.
>  For other request like for QPIC driver, QUPT driver etc. its not
> required. So Crypto driver has to raise the flag for
>  LOCK/UNLOCK while preparing CMD descriptor. The actual locking will
> happen in BAM driver only using condition
>  if (flags & DMA_PREP_CMD) {
>      if (flags & DMA_PREP_LOCK)
>         desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
>  }
> 
>  So Crypto driver should set this flag DMA_PREP_LOCK while preparing
> CMD descriptor.
>  So LOCK should be set on actual hardware pipe descriptor with
> descriptor type CMD.
> 
>> 
>> The point here is that this seems to be internal to dma and should be
>> handled by dma driver.
>> 
>   This LOCK/UNLOK flags are part of actual hardware descriptor so this
> should be handled by BAM driver only.
>   If we set condition like this
>   if (flags & DMA_PREP_CMD) {
>                 do_lock_bam();
>   Then LOCK/UNLOCK will be applied for all the CMD descriptor
> including (QPIC driver, QUP driver , Crypto driver etc.).
>   So this is not our intension. So we need to set this LOCK/UNLOCK
> only for the drivers it needs. So Crypto driver needs
>   locking mechanism so we will set LOCK/UNLOCK flag on Crypto driver
> request only for other driver request like QPIC driver,
>   QUP driver will not set this.
> 

    ping! Do you need any further info on this?

>> Also if we do this, it needs to be done for specific platforms..
>> 
> 
> 
> 
> 
> 
> 
> 
>> Thanks

Bjorn Andersson Feb. 9, 2021, 5:35 p.m. UTC | #20

On Mon 01 Feb 09:50 CST 2021, [e-mail] wrote:

> On 2021-02-01 12:13, Vinod Koul wrote:
> > On 01-02-21, 11:52, [e-mail] wrote:
> > > On 2021-02-01 11:35, Vinod Koul wrote:
> > > > On 27-01-21, 23:56, [e-mail] wrote:
> > 
> > > > >   The actual LOCK/UNLOCK flag should be set on hardware command
> > > > > descriptor.
> > > > >   so this flag setting should be done in DMA engine driver. The user
> > > > > of the
> > > > > DMA
> > > > >   driver like (in case of IPQ5018) Crypto can use flag
> > > > > "DMA_PREP_LOCK" &
> > > > > "DMA_PREP_UNLOCK"
> > > > >   while preparing CMD descriptor before submitting to the DMA
> > > > > engine. In DMA
> > > > > engine driver
> > > > >   we are checking these flasgs on CMD descriptor and setting actual
> > > > > LOCK/UNLOCK flag on hardware
> > > > >   descriptor.
> > > >
> > > >
> > > > I am not sure I comprehend this yet.. when is that we would need to do
> > > > this... is this for each txn submitted to dmaengine.. or something
> > > > else..
> > > 
> > >  Its not for each transaction submitted to dmaengine. We have to set
> > > this
> > > only
> > >  once on CMD descriptor. So when A53 crypto driver need to change
> > > the crypto
> > > configuration
> > >  then first it will lock the all other pipes using setting the LOCK
> > > flag bit
> > > on CMD
> > >  descriptor and then it can start the transaction , on data
> > > descriptor this
> > > flag will
> > >  not get set once all transaction will be completed the A53 crypto
> > > driver
> > > release the lock on
> > >  all other pipes using UNLOCK flag on CMD descriptor. So LOCK/UNLOCK
> > > will be
> > > only once and not for
> > >  the each transaction.
> > 
> > Okay so why cant the bam driver check cmd descriptor and do lock/unlock
> > as below, why do we need users to do this.
> > 
> >         if (flags & DMA_PREP_CMD) {
> >                 do_lock_bam();
> 
>  User will not decide to do this LOCK/UNLOCK mechanism. It depends on
>  use case.  This LOCK/UNLOCK mechanism not required always. It needs
>  only when hardware will be shared between different core with
>  different driver.

So you have a single piece of crypto hardware and you're using the BAM's
LOCK/UNLOCK feature to implement a "mutex" on a particular BAM channel?

>  The LOCK/UNLOCK flags provides SW to enter ordering between pipes
> execution.
>  (Generally, the BAM pipes are total independent from each other and work in
> parallel manner).
>  This LOCK/UNLOCK flags are part of actual pipe hardware descriptor.
> 
>  Pipe descriptor having the following flags:
>  INT : Interrupt
>  EOT: End of transfer
>  EOB: End of block
>  NWD: Notify when done
>  CMD: Command
>  LOCK: Lock
>  UNLOCK: Unlock
>  etc.
> 
>  Here the BAM driver is common driver for (QPIC, Crypto, QUP etc. in
> IPQ5018)
>  So here only Crypto will be shared b/w multiple cores so For crypto request
> only the LOCK/UNLOCK
>  mechanism required.
>  For other request like for QPIC driver, QUPT driver etc. its not required.
> So Crypto driver has to raise the flag for
>  LOCK/UNLOCK while preparing CMD descriptor. The actual locking will happen
> in BAM driver only using condition
>  if (flags & DMA_PREP_CMD) {
>      if (flags & DMA_PREP_LOCK)
>         desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
>  }
> 
>  So Crypto driver should set this flag DMA_PREP_LOCK while preparing CMD
> descriptor.
>  So LOCK should be set on actual hardware pipe descriptor with descriptor
> type CMD.
> 

It sounds fairly clear that the actual descriptor modification must
happen in the BAM driver, but the question in my mind is how this is
exposed to the DMAengine clients (e.g. crypto, QPIC etc).

What is the life span of the locked state? Do you always provide a
series of descriptors that starts with a LOCK and ends with an UNLOCK?
Or do you envision that the crypto driver provides a LOCK descriptor and
at some later point provides a UNLOCK descriptor?


Finally, this patch just adds the BAM part of things, where is the patch
that actually makes use of this feature?

Regards,
Bjorn

> > 
> > The point here is that this seems to be internal to dma and should be
> > handled by dma driver.
> > 
>   This LOCK/UNLOK flags are part of actual hardware descriptor so this
> should be handled by BAM driver only.
>   If we set condition like this
>   if (flags & DMA_PREP_CMD) {
>                 do_lock_bam();
>   Then LOCK/UNLOCK will be applied for all the CMD descriptor including
> (QPIC driver, QUP driver , Crypto driver etc.).
>   So this is not our intension. So we need to set this LOCK/UNLOCK only for
> the drivers it needs. So Crypto driver needs
>   locking mechanism so we will set LOCK/UNLOCK flag on Crypto driver request
> only for other driver request like QPIC driver,
>   QUP driver will not set this.
> 
> > Also if we do this, it needs to be done for specific platforms..
> > 
> 
> 
> 
> 
> 
> 
> 
> > Thanks

Md Sadre Alam Feb. 11, 2021, 4:01 a.m. UTC | #21

On 2021-02-09 23:05, Bjorn Andersson wrote:
> On Mon 01 Feb 09:50 CST 2021, [e-mail] wrote:
> 
>> On 2021-02-01 12:13, Vinod Koul wrote:
>> > On 01-02-21, 11:52, [e-mail] wrote:
>> > > On 2021-02-01 11:35, Vinod Koul wrote:
>> > > > On 27-01-21, 23:56, [e-mail] wrote:
>> >
>> > > > >   The actual LOCK/UNLOCK flag should be set on hardware command
>> > > > > descriptor.
>> > > > >   so this flag setting should be done in DMA engine driver. The user
>> > > > > of the
>> > > > > DMA
>> > > > >   driver like (in case of IPQ5018) Crypto can use flag
>> > > > > "DMA_PREP_LOCK" &
>> > > > > "DMA_PREP_UNLOCK"
>> > > > >   while preparing CMD descriptor before submitting to the DMA
>> > > > > engine. In DMA
>> > > > > engine driver
>> > > > >   we are checking these flasgs on CMD descriptor and setting actual
>> > > > > LOCK/UNLOCK flag on hardware
>> > > > >   descriptor.
>> > > >
>> > > >
>> > > > I am not sure I comprehend this yet.. when is that we would need to do
>> > > > this... is this for each txn submitted to dmaengine.. or something
>> > > > else..
>> > >
>> > >  Its not for each transaction submitted to dmaengine. We have to set
>> > > this
>> > > only
>> > >  once on CMD descriptor. So when A53 crypto driver need to change
>> > > the crypto
>> > > configuration
>> > >  then first it will lock the all other pipes using setting the LOCK
>> > > flag bit
>> > > on CMD
>> > >  descriptor and then it can start the transaction , on data
>> > > descriptor this
>> > > flag will
>> > >  not get set once all transaction will be completed the A53 crypto
>> > > driver
>> > > release the lock on
>> > >  all other pipes using UNLOCK flag on CMD descriptor. So LOCK/UNLOCK
>> > > will be
>> > > only once and not for
>> > >  the each transaction.
>> >
>> > Okay so why cant the bam driver check cmd descriptor and do lock/unlock
>> > as below, why do we need users to do this.
>> >
>> >         if (flags & DMA_PREP_CMD) {
>> >                 do_lock_bam();
>> 
>>  User will not decide to do this LOCK/UNLOCK mechanism. It depends on
>>  use case.  This LOCK/UNLOCK mechanism not required always. It needs
>>  only when hardware will be shared between different core with
>>  different driver.
> 
> So you have a single piece of crypto hardware and you're using the 
> BAM's
> LOCK/UNLOCK feature to implement a "mutex" on a particular BAM channel?

   Yes, In IPQ5018 SoC we are having only one Crypto and it will be 
shared between
   UBI32 core & A53 core, and these two cores are running different 
driver to use Crypto.
   The LOCK/UNLOCK flag can be set only on CMD descriptor.
> 
>>  The LOCK/UNLOCK flags provides SW to enter ordering between pipes
>> execution.
>>  (Generally, the BAM pipes are total independent from each other and 
>> work in
>> parallel manner).
>>  This LOCK/UNLOCK flags are part of actual pipe hardware descriptor.
>> 
>>  Pipe descriptor having the following flags:
>>  INT : Interrupt
>>  EOT: End of transfer
>>  EOB: End of block
>>  NWD: Notify when done
>>  CMD: Command
>>  LOCK: Lock
>>  UNLOCK: Unlock
>>  etc.
>> 
>>  Here the BAM driver is common driver for (QPIC, Crypto, QUP etc. in
>> IPQ5018)
>>  So here only Crypto will be shared b/w multiple cores so For crypto 
>> request
>> only the LOCK/UNLOCK
>>  mechanism required.
>>  For other request like for QPIC driver, QUPT driver etc. its not 
>> required.
>> So Crypto driver has to raise the flag for
>>  LOCK/UNLOCK while preparing CMD descriptor. The actual locking will 
>> happen
>> in BAM driver only using condition
>>  if (flags & DMA_PREP_CMD) {
>>      if (flags & DMA_PREP_LOCK)
>>         desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
>>  }
>> 
>>  So Crypto driver should set this flag DMA_PREP_LOCK while preparing 
>> CMD
>> descriptor.
>>  So LOCK should be set on actual hardware pipe descriptor with 
>> descriptor
>> type CMD.
>> 
> 
> It sounds fairly clear that the actual descriptor modification must
> happen in the BAM driver, but the question in my mind is how this is
> exposed to the DMAengine clients (e.g. crypto, QPIC etc).

   I have added these two flags "DMA_PREP_LOCK" & "DMA_PREP_UNLOCK" In 
enum dma_ctrl_flags.

   enum dma_ctrl_flags {
         DMA_PREP_INTERRUPT = (1 << 0),
@@ -202,6 +205,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),
  };

  So these flags we get passed while preparing CMD descriptor in Crypto 
driver. Based on these
  flags only i am setting LOCK/UNLOCK flags on actual hardware descriptor 
in BAM driver.

   if (flags & DMA_PREP_CMD) {
      if (flags & DMA_PREP_LOCK)
          desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);

> 
> What is the life span of the locked state? Do you always provide a
> series of descriptors that starts with a LOCK and ends with an UNLOCK?
> Or do you envision that the crypto driver provides a LOCK descriptor 
> and
> at some later point provides a UNLOCK descriptor?
> 

   While preparing CMD descriptor we will use this LOCK/UNLOCK flags. So 
if i wanted to write
   some 20 registers of Crypto HW via BAM then i will prepare multiple 
command descriptor
   let's say 20 CMD descriptor so in the very first CMD descriptor I will 
set the LOCK (DMA_PREP_LOCK ) flag and
   in the the last CMD descriptor I will set the UNLOCK (DMA_PREP_UNLOCK 
) flag.

> 
> Finally, this patch just adds the BAM part of things, where is the 
> patch
> that actually makes use of this feature?
> 
   Yes , this patch will add BAM part of things. For Crypto i will push 
another patch
   which will use this feature.

> Regards,
> Bjorn
> 
>> >
>> > The point here is that this seems to be internal to dma and should be
>> > handled by dma driver.
>> >
>>   This LOCK/UNLOK flags are part of actual hardware descriptor so this
>> should be handled by BAM driver only.
>>   If we set condition like this
>>   if (flags & DMA_PREP_CMD) {
>>                 do_lock_bam();
>>   Then LOCK/UNLOCK will be applied for all the CMD descriptor 
>> including
>> (QPIC driver, QUP driver , Crypto driver etc.).
>>   So this is not our intension. So we need to set this LOCK/UNLOCK 
>> only for
>> the drivers it needs. So Crypto driver needs
>>   locking mechanism so we will set LOCK/UNLOCK flag on Crypto driver 
>> request
>> only for other driver request like QPIC driver,
>>   QUP driver will not set this.
>> 
>> > Also if we do this, it needs to be done for specific platforms..
>> >
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> > Thanks

Patch

diff --git a/Documentation/driver-api/dmaengine/provider.rst b/Documentation/driver-api/dmaengine/provider.rst
index ddb0a81..d7516e2 100644
--- a/Documentation/driver-api/dmaengine/provider.rst
+++ b/Documentation/driver-api/dmaengine/provider.rst
@@ -599,6 +599,15 @@  DMA_CTRL_REUSE
   - This flag is only supported if the channel reports the DMA_LOAD_EOT
     capability.
 
+- DMA_PREP_LOCK
+
+  - If set , the client driver tells DMA controller I am locking you for
+    this transcation.
+
+- DMA_PREP_UNLOCK
+
+  - If set, the client driver will tells DMA controller I am releasing the lock
+
 General Design Notes
 ====================
 
diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 4eeb8bb..cdbe395 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;
@@ -644,6 +646,13 @@  static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
 
 	/* fill in temporary descriptors */
 	desc = async_desc->desc;
+	if (flags & DMA_PREP_CMD) {
+		if (flags & DMA_PREP_LOCK)
+			desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
+		if (flags & DMA_PREP_UNLOCK)
+			desc->flags |= cpu_to_le16(DESC_FLAG_UNLOCK);
+	}
+
 	for_each_sg(sgl, sg, sg_len, i) {
 		unsigned int remainder = sg_dma_len(sg);
 		unsigned int curr_offset = 0;
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index dd357a7..79ccadb4 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -190,6 +190,9 @@  struct dma_interleaved_template {
  *  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 DMA HW engine going to be locked for this
+ *  transaction , until not seen DMA_PREP_UNLOCK flag set.
+ * @DMA_PREP_UNLOCK: tell the driver to unlock the DMA HW engine.
  */
 enum dma_ctrl_flags {
 	DMA_PREP_INTERRUPT = (1 << 0),
@@ -202,6 +205,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),
 };
 
 /**