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

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

[Alexandru Ardelean]

From: Rodrigo Alencar <alencar.fmce@imbel.gov.br>

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:

$ 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
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@imbel.gov.br>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---

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) {
+		dev_err(dmac->dma_dev.dev, "Direction not supported by this DMA Channel");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static struct dma_async_tx_descriptor *axi_dmac_prep_dma_cyclic(
 	struct dma_chan *c, dma_addr_t buf_addr, size_t buf_len,
 	size_t period_len, enum dma_transfer_direction direction,
@@ -878,6 +893,7 @@ static int axi_dmac_probe(struct platform_device *pdev)
 	dma_dev->device_tx_status = dma_cookie_status;
 	dma_dev->device_issue_pending = axi_dmac_issue_pending;
 	dma_dev->device_prep_slave_sg = axi_dmac_prep_slave_sg;
+	dma_dev->device_config = axi_dmac_device_config;
 	dma_dev->device_prep_dma_cyclic = axi_dmac_prep_dma_cyclic;
 	dma_dev->device_prep_interleaved_dma = axi_dmac_prep_interleaved;
 	dma_dev->device_terminate_all = axi_dmac_terminate_all;
-- 
2.20.1

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

[Vinod Koul]

On 13-09-19, 17:54, Alexandru Ardelean wrote:
> From: Rodrigo Alencar <alencar.fmce@imbel.gov.br>
> 
> 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
> 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@imbel.gov.br>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> 
> 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

> +		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?

Thanks
-- 
~Vinod

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@imbel.gov.br>
> > 
> > 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@imbel.gov.br>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> > 
> > 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

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

[Vinod Koul]

On 15-10-19, 07:05, Ardelean, Alexandru wrote:
> 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@imbel.gov.br>
> > > 
> > > 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].

Right and it would help explaining a bit more on the error!

> 
> > > 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@imbel.gov.br>
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > ---
> > > 
> > > 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

Yeah it is tricky, this should be ideally implemented properly.

> > > +		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

So the question for you hw folks is how would a controller work with
multiple slave devices, do they need to synthesize it everytime?

Rather than that why cant they make the peripheral addresses
programmable so that you dont need updating fpga everytime!

> 
> 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.

Well FPGA doesnt mean hw should know everything, having SW program
things is not a terrible idea

-- 
~Vinod

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

[Lars-Peter Clausen]

On 10/15/19 12:43 PM, Vinod Koul wrote:
> On 15-10-19, 07:05, Ardelean, Alexandru wrote:
>> 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@imbel.gov.br>
>>>>
>>>> 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].
> 
> Right and it would help explaining a bit more on the error!
> 
>>
>>>> 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@imbel.gov.br>
>>>> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>>>> ---
>>>>
>>>> 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?

iirc direction is checked when the channel is requested, there should be
no need to check it again.

>> 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
> 
> Yeah it is tricky, this should be ideally implemented properly.
> 
>>>> +		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
> 
> So the question for you hw folks is how would a controller work with
> multiple slave devices, do they need to synthesize it everytime?
> 
> Rather than that why cant they make the peripheral addresses
> programmable so that you dont need updating fpga everytime!

The DMA has a direct connection to the peripheral and the peripheral
data port is not connected to the general purpose memory interconnect.
So you can't write to it by an MMIO address and	 there is no address
that needs to be configured. For an FPGA based design this is quite a
good solution in terms of resource usage, performance and simplicity. A
direct connection requires less resources than connection it to the
central memory interconnect, while at the same time having lower latency
and not eating up any additional bandwidth on the central memory connect.

So slave config in this case is a noop and all it can do is verify that
the requested configuration matches the available configuration.

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

[Vinod Koul]

On 15-10-19, 23:06, Lars-Peter Clausen wrote:

> >> 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
> > 
> > So the question for you hw folks is how would a controller work with
> > multiple slave devices, do they need to synthesize it everytime?
> > 
> > Rather than that why cant they make the peripheral addresses
> > programmable so that you dont need updating fpga everytime!
> 
> The DMA has a direct connection to the peripheral and the peripheral
> data port is not connected to the general purpose memory interconnect.
> So you can't write to it by an MMIO address and	 there is no address
> that needs to be configured. For an FPGA based design this is quite a
> good solution in terms of resource usage, performance and simplicity. A
> direct connection requires less resources than connection it to the
> central memory interconnect, while at the same time having lower latency
> and not eating up any additional bandwidth on the central memory connect.

thanks for explanation!

> So slave config in this case is a noop and all it can do is verify that
> the requested configuration matches the available configuration.

okay so noop it is!

-- 
~Vinod

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

[Ardelean, Alexandru]

On Wed, 2019-10-16 at 10:38 +0530, Vinod Koul wrote:
> [External]
> 
> On 15-10-19, 23:06, Lars-Peter Clausen wrote:
> 
> > > > 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
> > > 
> > > So the question for you hw folks is how would a controller work with
> > > multiple slave devices, do they need to synthesize it everytime?
> > > 
> > > Rather than that why cant they make the peripheral addresses
> > > programmable so that you dont need updating fpga everytime!
> > 
> > The DMA has a direct connection to the peripheral and the peripheral
> > data port is not connected to the general purpose memory interconnect.
> > So you can't write to it by an MMIO address and	 there is no
> > address
> > that needs to be configured. For an FPGA based design this is quite a
> > good solution in terms of resource usage, performance and simplicity. A
> > direct connection requires less resources than connection it to the
> > central memory interconnect, while at the same time having lower
> > latency
> > and not eating up any additional bandwidth on the central memory
> > connect.
> 
> thanks for explanation!

also many thanks [from my side] for the explanations :)

> 
> > So slave config in this case is a noop and all it can do is verify that
> > the requested configuration matches the available configuration.
> 
> okay so noop it is!
>