Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: mdalam=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Thu, 11 Feb 2021 09:31:26 +0530
From:   mdalam@codeaurora.org
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     Vinod Koul <vkoul@kernel.org>, corbet@lwn.net, agross@kernel.org,
        dan.j.williams@intel.com, dmaengine@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, sricharan@codeaurora.org,
        mdalam=codeaurora.org@codeaurora.org
Subject: Re: [PATCH] dmaengine: qcom: bam_dma: Add LOCK and UNLOCK flag bit
 support
In-Reply-To: <YCLH4ZOMjLbywl4u@builder.lan>
References: <20210112101056.GI2771@vkoul-mobl>
 <e3cf7c4fc02c54d17fd2fd213f39005b@codeaurora.org>
 <20210115055806.GE2771@vkoul-mobl>
 <97ce29b230164a5848a38f6448d1be60@codeaurora.org>
 <20210119164511.GE2771@vkoul-mobl>
 <534308caab7c18730ad0cc25248d116f@codeaurora.org>
 <20210201060508.GK2771@vkoul-mobl>
 <9d33d73682f24d92338757e1823ccd88@codeaurora.org>
 <20210201064314.GM2771@vkoul-mobl>
 <73c871d3d674607fafc7b79e602ec587@codeaurora.org>
 <YCLH4ZOMjLbywl4u@builder.lan>
Message-ID: <75b305066801fbfefa162326c63d1241@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

On 2021-02-09 23:05, Bjorn Andersson wrote:
> On Mon 01 Feb 09:50 CST 2021, mdalam@codeaurora.org wrote:
> 
>> On 2021-02-01 12:13, Vinod Koul wrote:
>> > On 01-02-21, 11:52, mdalam@codeaurora.org wrote:
>> > > On 2021-02-01 11:35, Vinod Koul wrote:
>> > > > On 27-01-21, 23:56, mdalam@codeaurora.org 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