Re: [PATCH] dmaengine: qcom: bam_dma: Add LOCK and UNLOCK flag bit support

From: mdalam
Date: Wed Feb 10 2021 - 23:02:31 EST

On 2021-02-09 23:05, Bjorn Andersson wrote:
On Mon 01 Feb 09:50 CST 2021, mdalam@xxxxxxxxxxxxxx wrote:

On 2021-02-01 12:13, Vinod Koul wrote:
> On 01-02-21, 11:52, mdalam@xxxxxxxxxxxxxx wrote:
> > On 2021-02-01 11:35, Vinod Koul wrote:
> > > On 27-01-21, 23:56, mdalam@xxxxxxxxxxxxxx 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