Re: [PATCH] dmaengine: axi-dmac: simple device_config operation implemented

[Ardelean, Alexandru]

On Mon, 2019-10-14 at 12:31 +0530, Vinod Koul wrote:
> [External]
> 

Hey,

> On 13-09-19, 17:54, Alexandru Ardelean wrote:
> > From: Rodrigo Alencar <alencar.fmce@xxxxxxxxxxxx>
> > 
> > dmaengine_slave_config is called by dmaengine_pcm_hw_params when using
> > axi-i2s with axi-dmac. If device_config is NULL, -ENOSYS  is returned,
> > which breaks the snd_pcm_hw_params function.
> > This is a fix for the error:
> 
> and what is that?
> 
> > $ aplay -D plughw:ADAU1761 /usr/share/sounds/alsa/Front_Center.wav
> > Playing WAVE '/usr/share/sounds/alsa/Front_Center.wav' : Signed 16 bit
> > Little Endian, Rate 48000 Hz, Mono
> > axi-i2s 43c20000.axi-i2s: ASoC: 43c20000.axi-i2s hw params failed: -38

Error is above this line [code -38].

> > aplay: set_params:1403: Unable to install hw params:
> > ACCESS:  RW_INTERLEAVED
> > FORMAT:  S16_LE
> > SUBFORMAT:  STD
> > SAMPLE_BITS: 16
> > FRAME_BITS: 16
> > CHANNELS: 1
> > RATE: 48000
> > PERIOD_TIME: 125000
> > PERIOD_SIZE: 6000
> > PERIOD_BYTES: 12000
> > PERIODS: 4
> > BUFFER_TIME: 500000
> > BUFFER_SIZE: 24000
> > BUFFER_BYTES: 48000
> > TICK_TIME: 0
> > 
> > Signed-off-by: Rodrigo Alencar <alencar.fmce@xxxxxxxxxxxx>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> > ---
> > 
> > Note: Fixes tag not added intentionally.
> > 
> >  drivers/dma/dma-axi-dmac.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
> > index a0ee404b736e..ab2677343202 100644
> > --- a/drivers/dma/dma-axi-dmac.c
> > +++ b/drivers/dma/dma-axi-dmac.c
> > @@ -564,6 +564,21 @@ static struct dma_async_tx_descriptor
> > *axi_dmac_prep_slave_sg(
> >  	return vchan_tx_prep(&chan->vchan, &desc->vdesc, flags);
> >  }
> >  
> > +static int axi_dmac_device_config(struct dma_chan *c,
> > +			struct dma_slave_config *slave_config)
> > +{
> > +	struct axi_dmac_chan *chan = to_axi_dmac_chan(c);
> > +	struct axi_dmac *dmac = chan_to_axi_dmac(chan);
> > +
> > +	/* no configuration required, a sanity check is done instead */
> > +	if (slave_config->direction != chan->direction) {
> 
>  slave_config->direction is a deprecated field, pls dont use that

ack
any alternative recommendations of what to do in this case?
i can take a look, but if you have something on-the-top-of-your-head, i'm
open to suggestions
we can also just drop this completely and let userspace fail

> 
> > +		dev_err(dmac->dma_dev.dev, "Direction not supported by this
> > DMA Channel");
> > +		return -EINVAL;
> 
> So you intent to support slave dma but do not use dma_slave_config.. how
> are you getting the slave address and other details?

This DMA controller is a bit special.
It gets synthesized in FPGA, so the configuration is fixed and cannot be
changed at runtime. Maybe later we would allow/implement this
functionality, but this is a question for my HDL colleagues.

Two things are done (in this order):
1. For some paramters, axi_dmac_parse_chan_dt() is used to determine things
from device-tree; as it's an FPGA core, things are synthesized once and
cannot change (yet)
2. For other parameters, the axi_dmac_detect_caps() is used to guess some
of them at probe time, by doing some reg reads/writes

I'll admit that maybe the whole approach could be done a bit
differently/better. But I guess this approach was chosen by the fact that
it's FPGA.

Btw: if I'm talking crap, or I may sound like I don't know what I'm talking
about, that could also be true. I am not quite versed in the DMAEngine
framework.

Thanks
Alex

> 
> Thanks