Re: [PATCH RESEND 1/4] crypto: caam: add caam-dma node to SEC4.0 device tree binding

[Kim Phillips <kim.phillips@arm.com>]

On Mon, 13 Nov 2017 09:44:06 +0000
Radu Andrei Alexe <radu.alexe@nxp.com> wrote:

> On 11/10/2017 6:44 PM, Kim Phillips wrote:
> > On Fri, 10 Nov 2017 08:02:01 +0000
> > Radu Andrei Alexe <radu.alexe@nxp.com> wrote:
> > 
> >> On 11/9/2017 6:34 PM, Kim Phillips wrote:
> >>> On Thu, 9 Nov 2017 11:54:13 +0000
> >>> Radu Andrei Alexe <radu.alexe@nxp.com> wrote:
> >>>> The next patch version will create the platform device dynamically at
> >>>> run time.
> >>>
> >>> Why create a new device when that h/w already has one?
> >>>
> >>> Why doesn't the existing crypto driver register dma capabilities with
> >>> the dma driver subsystem?
> >>>
> >> I can think of two reasons:
> >>
> >> 1. The code that this driver introduces has nothing to do with crypto
> >> and everything to do with dma.
> > 
> > I would think that at least a crypto "null" algorithm implementation
> > would share code.
> >
> >> Placing the code in the same directory as
> >> the caam subsystem would only create confusion for the reader of an
> >> already complex driver.
> > 
> > this different directory argument seems to be identical to your 2 below:
> > 
> >> 2. I wanted this driver to be tracked by the dma engine team. They have
> >> the right expertise to provide adequate feedback. If all the code was in
> >> the crypto directory they wouldn't know about this driver or any
> >> subsequent changes to it.
> > 
> > dma subsystem bits could still be put in the dma area if deemed
> > necessary but I don't think it is: I see
> > drivers/crypto/ccp/ccp-dmaengine.c calls dma_async_device_register for
> > example.
> > 
> > I also don't see how that complicates things much further.
> > 
> 
> So who made their review? The guys from crypto?

Don't see how that's relevant here, but people applying patches should
solicit acks from the appropriate sources, esp. if a patch is across
multiple subsystems.

> If someone wants to enable only the DMA functionality of the CCP and not 
> the crypto part how do they do it? Look for it in the crypto submenu?

Why would they want to do that?

In any case, I suspect you're thinking about cross-subsystem Kconfig
entries, which is common, but something like that can be a module
parameter, too.

I would say that maybe CRYPTO_DEV_FSL_CAAM should be made to not depend
on CRYPTO_HW, but I think that's overkill for the addition of this
minor feature.

> > What is the rationale for using the crypto h/w as a dma engine anyway?
> > Are there supporting performance figures?
> 
> We have a platform that doesn't have a dedicated DMA controller but has 
> the CAAM hardware block that can perform dma transfers.  We have a

OK, please mention that next time.

> use-case where we need to issue large transfers (hundred of MBs) 
> asynchronously, without using the core.

Curious: what subsystem does that?

Thanks,

Kim