Re: [PATCH v4] dmaengine: qcom: Add ADM driver

[Vinod Koul <vkoul@kernel.org>]

On 09-11-20, 19:04, Jonathan McDowell wrote:
> On Mon, Nov 09, 2020 at 05:11:21PM +0530, Vinod Koul wrote:
> > HI Jonathan,
> > 
> > On 23-09-20, 20:40, Jonathan McDowell wrote:
> > > Add the DMA engine driver for the QCOM Application Data Mover (ADM) DMA
> > > controller found in the MSM8x60 and IPQ/APQ8064 platforms.
> > 
> > Mostly it looks good, some nitpicks
> > 
> > > The ADM supports both memory to memory transactions and memory
> > > to/from peripheral device transactions.  The controller also provides
> > > flow control capabilities for transactions to/from peripheral devices.
> > > 
> > > The initial release of this driver supports slave transfers to/from
> > > peripherals and also incorporates CRCI (client rate control interface)
> > > flow control.
> > 
> > Can you also convert the binding from txt to yaml?
> 
> Seems like that can be a separate patch, but sure, I'll give it a whirl.

Yup a different patch, thanks for looking into that

> > > diff --git a/drivers/dma/qcom/Kconfig b/drivers/dma/qcom/Kconfig
> > > index 3bcb689162c6..0389d60d2604 100644
> > > --- a/drivers/dma/qcom/Kconfig
> > > +++ b/drivers/dma/qcom/Kconfig
> > > @@ -1,4 +1,15 @@
> > >  # SPDX-License-Identifier: GPL-2.0-only
> > > +config QCOM_ADM
> > > +	tristate "Qualcomm ADM support"
> > > +	depends on (ARCH_QCOM || COMPILE_TEST) && !PHYS_ADDR_T_64BIT
> > 
> > why !PHYS_ADDR_T_64BIT ..?
> 
> The hardware only supports a 32 bit physical address, so specifying
> !PHYS_ADDR_T_64BIT gives maximum COMPILE_TEST coverage without having to
> spend effort on kludging things in the code that will never actually be
> needed on real hardware.

Can we mention that in the log please

> 
> > > +	select DMA_ENGINE
> > > +	select DMA_VIRTUAL_CHANNELS
> > > +	help
> > > +	  Enable support for the Qualcomm Application Data Mover (ADM) DMA
> > > +	  controller, as present on MSM8x60, APQ8064, and IPQ8064 devices.
> > > +	  This controller provides DMA capabilities for both general purpose
> > > +	  and on-chip peripheral devices.
> > 
> > > +static const struct of_device_id adm_of_match[] = {
> > > +	{ .compatible = "qcom,adm", },
> > 
> > I know we have merged the binding, but should we not have a soc specific
> > compatible?
> 
> Which soc? Looking at the other QCOM DMA drivers they mostly have
> versioned compatibles and I can't find any indication there are multiple
> variants of this block out there.

So even though ip block can remain same for few versions, we should
trust hw folks enough to give us spicy flavours in next revs :-) so
adding a compatible here like qcom,msm8x60-adm would help us.

BUT, looking at the QC documentation I dont see it being used in recent
chips so ok to go with qcom,adm

Thanks
-- 
~Vinod