[PATCH v2 0/2] dmaengine: core/omap-dma: Support for port window

[PATCH v2 0/2] dmaengine: core/omap-dma: Support for port window

[Peter Ujfalusi]

Hi,

Changes since v1:
- Make sure that the one frame covers the port_window (burst = port_window)
- added comment to explain the double indexed setup to cover the port_window
- Simplifications for the code mentioned by Russell and Vinod


Cover letter from v1:

as I'm trying to convert the remaining OMAP driver to use DMAengine instead of
the legacy omap-dma API I have encountered with the
drivers/usb/musb/tusb6010_omap.c driver.

The TUSB6010 is connected via NOR FLASH interface and it's register space is
mapped in the GPMC memory area. In OMAP SoCs we have support for external DMA
request lines and the TUSB6010 is using those as well.

With asynchronous access the DMA needs to read/write within the FIFO 'window' in
incremental address mode to read/write data.
The constant addressing only works in synchronous mode.

Since the DMA is driven by external DMA requests, the asynchronous mode is also
slave DMA operation, but currently the port window can not be 'swiped' as the
DMAengine only supports single register/address on the slave side.

This series will add support in dma_slave_config to specify the port side window
size and the second patch implements the setup needs in omap-dma driver for such
a transfer.

Regards,
Peter
---
Peter Ujfalusi (2):
  dmaengine: dma_slave_config: add support for slave port window
  dmaengine: omap-dma: Support for slave devices with data port window

 drivers/dma/omap-dma.c    | 61 +++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/dmaengine.h |  8 +++++++
 2 files changed, 67 insertions(+), 2 deletions(-)

-- 
2.10.2

[PATCH v2 0/2] dmaengine: core/omap-dma: Support for port window

[Peter Ujfalusi]

Hi,

Changes since v1:
- Make sure that the one frame covers the port_window (burst = port_window)
- added comment to explain the double indexed setup to cover the port_window
- Simplifications for the code mentioned by Russell and Vinod


Cover letter from v1:

as I'm trying to convert the remaining OMAP driver to use DMAengine instead of
the legacy omap-dma API I have encountered with the
drivers/usb/musb/tusb6010_omap.c driver.

The TUSB6010 is connected via NOR FLASH interface and it's register space is
mapped in the GPMC memory area. In OMAP SoCs we have support for external DMA
request lines and the TUSB6010 is using those as well.

With asynchronous access the DMA needs to read/write within the FIFO 'window' in
incremental address mode to read/write data.
The constant addressing only works in synchronous mode.

Since the DMA is driven by external DMA requests, the asynchronous mode is also
slave DMA operation, but currently the port window can not be 'swiped' as the
DMAengine only supports single register/address on the slave side.

This series will add support in dma_slave_config to specify the port side window
size and the second patch implements the setup needs in omap-dma driver for such
a transfer.

Regards,
Peter
---
Peter Ujfalusi (2):
  dmaengine: dma_slave_config: add support for slave port window
  dmaengine: omap-dma: Support for slave devices with data port window

 drivers/dma/omap-dma.c    | 61 +++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/dmaengine.h |  8 +++++++
 2 files changed, 67 insertions(+), 2 deletions(-)

-- 
2.10.2

[PATCH v2 1/2] dmaengine: dma_slave_config: add support for slave port window

[Peter Ujfalusi]

Some slave devices uses address window instead of single register for read
and/or write of data. With the src/dst_port_window_size the address window
can be specified and the DMAengine driver should use this information to
correctly set up the transfer to loop within the provided window.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 include/linux/dmaengine.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index cc535a478bae..689d44327ef3 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -336,6 +336,12 @@ enum dma_slave_buswidth {
  * may or may not be applicable on memory sources.
  * @dst_maxburst: same as src_maxburst but for destination target
  * mutatis mutandis.
+ * @src_port_window_size: The length of the register area the data need to be
+ * written on the device side. It is only used for devices which is using an
+ * area instead of a single register to receive the data. Typically the DMA
+ * loops in this area in order to transfer the data.
+ * @dst_port_window_size: same as src_port_window_size but for the destination
+ * port.
  * @device_fc: Flow Controller Settings. Only valid for slave channels. Fill
  * with 'true' if peripheral should be flow controller. Direction will be
  * selected at Runtime.
@@ -363,6 +369,8 @@ struct dma_slave_config {
 	enum dma_slave_buswidth dst_addr_width;
 	u32 src_maxburst;
 	u32 dst_maxburst;
+	u32 src_port_window_size;
+	u32 dst_port_window_size;
 	bool device_fc;
 	unsigned int slave_id;
 };
-- 
2.10.2

[PATCH v2 1/2] dmaengine: dma_slave_config: add support for slave port window

[Peter Ujfalusi]

Some slave devices uses address window instead of single register for read
and/or write of data. With the src/dst_port_window_size the address window
can be specified and the DMAengine driver should use this information to
correctly set up the transfer to loop within the provided window.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 include/linux/dmaengine.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index cc535a478bae..689d44327ef3 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -336,6 +336,12 @@ enum dma_slave_buswidth {
  * may or may not be applicable on memory sources.
  * @dst_maxburst: same as src_maxburst but for destination target
  * mutatis mutandis.
+ * @src_port_window_size: The length of the register area the data need to be
+ * written on the device side. It is only used for devices which is using an
+ * area instead of a single register to receive the data. Typically the DMA
+ * loops in this area in order to transfer the data.
+ * @dst_port_window_size: same as src_port_window_size but for the destination
+ * port.
  * @device_fc: Flow Controller Settings. Only valid for slave channels. Fill
  * with 'true' if peripheral should be flow controller. Direction will be
  * selected at Runtime.
@@ -363,6 +369,8 @@ struct dma_slave_config {
 	enum dma_slave_buswidth dst_addr_width;
 	u32 src_maxburst;
 	u32 dst_maxburst;
+	u32 src_port_window_size;
+	u32 dst_port_window_size;
 	bool device_fc;
 	unsigned int slave_id;
 };
-- 
2.10.2

Re: [PATCH v2 1/2] dmaengine: dma_slave_config: add support for slave port window

[Peter Ujfalusi]

Vinod,

On 11/17/2016 02:50 PM, Peter Ujfalusi wrote:
> Some slave devices uses address window instead of single register for read
> and/or write of data. With the src/dst_port_window_size the address window
> can be specified and the DMAengine driver should use this information to
> correctly set up the transfer to loop within the provided window.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  include/linux/dmaengine.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index cc535a478bae..689d44327ef3 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -336,6 +336,12 @@ enum dma_slave_buswidth {
>   * may or may not be applicable on memory sources.
>   * @dst_maxburst: same as src_maxburst but for destination target
>   * mutatis mutandis.
> + * @src_port_window_size: The length of the register area the data need to be
> + * written on the device side. It is only used for devices which is using an
> + * area instead of a single register to receive the data. Typically the DMA
> + * loops in this area in order to transfer the data.
> + * @dst_port_window_size: same as src_port_window_size but for the destination
> + * port.

I think this needs some clarification.
Should the src/dst_port_window_size be in bytes or in words? I think it would
make more sense to define it as number of words to be in sync with the
src/dst_maxburst.

I will need to resend the series either way. The omap-dma patch does not
handle this correctly.

>   * @device_fc: Flow Controller Settings. Only valid for slave channels. Fill
>   * with 'true' if peripheral should be flow controller. Direction will be
>   * selected at Runtime.
> @@ -363,6 +369,8 @@ struct dma_slave_config {
>  	enum dma_slave_buswidth dst_addr_width;
>  	u32 src_maxburst;
>  	u32 dst_maxburst;
> +	u32 src_port_window_size;
> +	u32 dst_port_window_size;
>  	bool device_fc;
>  	unsigned int slave_id;
>  };
> 


-- 
Péter

Re: [PATCH v2 1/2] dmaengine: dma_slave_config: add support for slave port window

[Peter Ujfalusi]

Vinod,

On 11/17/2016 02:50 PM, Peter Ujfalusi wrote:
> Some slave devices uses address window instead of single register for read
> and/or write of data. With the src/dst_port_window_size the address window
> can be specified and the DMAengine driver should use this information to
> correctly set up the transfer to loop within the provided window.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  include/linux/dmaengine.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index cc535a478bae..689d44327ef3 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -336,6 +336,12 @@ enum dma_slave_buswidth {
>   * may or may not be applicable on memory sources.
>   * @dst_maxburst: same as src_maxburst but for destination target
>   * mutatis mutandis.
> + * @src_port_window_size: The length of the register area the data need to be
> + * written on the device side. It is only used for devices which is using an
> + * area instead of a single register to receive the data. Typically the DMA
> + * loops in this area in order to transfer the data.
> + * @dst_port_window_size: same as src_port_window_size but for the destination
> + * port.

I think this needs some clarification.
Should the src/dst_port_window_size be in bytes or in words? I think it would
make more sense to define it as number of words to be in sync with the
src/dst_maxburst.

I will need to resend the series either way. The omap-dma patch does not
handle this correctly.

>   * @device_fc: Flow Controller Settings. Only valid for slave channels. Fill
>   * with 'true' if peripheral should be flow controller. Direction will be
>   * selected at Runtime.
> @@ -363,6 +369,8 @@ struct dma_slave_config {
>  	enum dma_slave_buswidth dst_addr_width;
>  	u32 src_maxburst;
>  	u32 dst_maxburst;
> +	u32 src_port_window_size;
> +	u32 dst_port_window_size;
>  	bool device_fc;
>  	unsigned int slave_id;
>  };
> 


-- 
Péter

Re: [PATCH v2 1/2] dmaengine: dma_slave_config: add support for slave port window

[Vinod Koul]

On Tue, Nov 29, 2016 at 12:47:48PM +0200, Peter Ujfalusi wrote:
> Vinod,
> 
> On 11/17/2016 02:50 PM, Peter Ujfalusi wrote:
> > Some slave devices uses address window instead of single register for read
> > and/or write of data. With the src/dst_port_window_size the address window
> > can be specified and the DMAengine driver should use this information to
> > correctly set up the transfer to loop within the provided window.
> > 
> > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > ---
> >  include/linux/dmaengine.h | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > index cc535a478bae..689d44327ef3 100644
> > --- a/include/linux/dmaengine.h
> > +++ b/include/linux/dmaengine.h
> > @@ -336,6 +336,12 @@ enum dma_slave_buswidth {
> >   * may or may not be applicable on memory sources.
> >   * @dst_maxburst: same as src_maxburst but for destination target
> >   * mutatis mutandis.
> > + * @src_port_window_size: The length of the register area the data need to be
> > + * written on the device side. It is only used for devices which is using an
> > + * area instead of a single register to receive the data. Typically the DMA
> > + * loops in this area in order to transfer the data.
> > + * @dst_port_window_size: same as src_port_window_size but for the destination
> > + * port.
> 
> I think this needs some clarification.
> Should the src/dst_port_window_size be in bytes or in words? I think it would
> make more sense to define it as number of words to be in sync with the
> src/dst_maxburst.

Yes good catch, I would indeed prefer words here


-- 
~Vinod

Re: [PATCH v2 2/2] dmaengine: omap-dma: Support for slave devices with data port window

[Vinod Koul]

On Thu, Nov 17, 2016 at 02:50:17PM +0200, Peter Ujfalusi wrote:
> @@ -921,11 +931,45 @@ static struct dma_async_tx_descriptor *omap_dma_prep_slave_sg(
>  
>  	d->ccr = c->ccr | CCR_SYNC_FRAME;
>  	if (dir == DMA_DEV_TO_MEM) {
> -		d->ccr |= CCR_DST_AMODE_POSTINC | CCR_SRC_AMODE_CONSTANT;
>  		d->csdp = CSDP_DST_BURST_64 | CSDP_DST_PACKED;
> +
> +		d->ccr |= CCR_DST_AMODE_POSTINC;
> +		if (port_window) {
> +			d->ccr |= CCR_SRC_AMODE_DBLIDX;
> +			d->ei = 1;
> +			/*
> +			 * One frame covers the port_window and by  configure
> +			 * the source frame index to be -1 * (port_window - 1)
> +			 * we instruct the sDMA that after a frame is processed
> +			 * it should move back to the start of the window.
> +			 */
> +			d->fi = -(port_window - 1);
> +
> +			if (port_window >= 64)
> +				d->csdp = CSDP_SRC_BURST_64 | CSDP_SRC_PACKED;
> +			else if (port_window >= 32)
> +				d->csdp = CSDP_SRC_BURST_32 | CSDP_SRC_PACKED;
> +			else if (port_window >= 16)
> +				d->csdp = CSDP_SRC_BURST_16 | CSDP_SRC_PACKED;

this and other would look better with a switch..

-- 
~Vinod

Re: [PATCH v2 2/2] dmaengine: omap-dma: Support for slave devices with data port window

[Peter Ujfalusi]

On 11/25/2016 08:12 AM, Vinod Koul wrote:
> On Thu, Nov 17, 2016 at 02:50:17PM +0200, Peter Ujfalusi wrote:
>> @@ -921,11 +931,45 @@ static struct dma_async_tx_descriptor *omap_dma_prep_slave_sg(
>>  
>>  	d->ccr = c->ccr | CCR_SYNC_FRAME;
>>  	if (dir == DMA_DEV_TO_MEM) {
>> -		d->ccr |= CCR_DST_AMODE_POSTINC | CCR_SRC_AMODE_CONSTANT;
>>  		d->csdp = CSDP_DST_BURST_64 | CSDP_DST_PACKED;
>> +
>> +		d->ccr |= CCR_DST_AMODE_POSTINC;
>> +		if (port_window) {
>> +			d->ccr |= CCR_SRC_AMODE_DBLIDX;
>> +			d->ei = 1;
>> +			/*
>> +			 * One frame covers the port_window and by  configure
>> +			 * the source frame index to be -1 * (port_window - 1)
>> +			 * we instruct the sDMA that after a frame is processed
>> +			 * it should move back to the start of the window.
>> +			 */
>> +			d->fi = -(port_window - 1);
>> +
>> +			if (port_window >= 64)
>> +				d->csdp = CSDP_SRC_BURST_64 | CSDP_SRC_PACKED;
>> +			else if (port_window >= 32)
>> +				d->csdp = CSDP_SRC_BURST_32 | CSDP_SRC_PACKED;
>> +			else if (port_window >= 16)
>> +				d->csdp = CSDP_SRC_BURST_16 | CSDP_SRC_PACKED;
> 
> this and other would look better with a switch..

I'm not sure if it will be any better:

switch (port_window) {
case 0 ... 15:
	break;
case 16 ... 31:
	d->csdp = CSDP_SRC_BURST_16 | CSDP_SRC_PACKED;
	break;
case 32 ... 63:
	d->csdp = CSDP_SRC_BURST_32 | CSDP_SRC_PACKED;
	break;
default:
	d->csdp = CSDP_SRC_BURST_64 | CSDP_SRC_PACKED;
	break;
}


-- 
Péter

Re: [PATCH v2 2/2] dmaengine: omap-dma: Support for slave devices with data port window

[Peter Ujfalusi]

On 11/25/2016 08:12 AM, Vinod Koul wrote:
> On Thu, Nov 17, 2016 at 02:50:17PM +0200, Peter Ujfalusi wrote:
>> @@ -921,11 +931,45 @@ static struct dma_async_tx_descriptor *omap_dma_prep_slave_sg(
>>  
>>  	d->ccr = c->ccr | CCR_SYNC_FRAME;
>>  	if (dir == DMA_DEV_TO_MEM) {
>> -		d->ccr |= CCR_DST_AMODE_POSTINC | CCR_SRC_AMODE_CONSTANT;
>>  		d->csdp = CSDP_DST_BURST_64 | CSDP_DST_PACKED;
>> +
>> +		d->ccr |= CCR_DST_AMODE_POSTINC;
>> +		if (port_window) {
>> +			d->ccr |= CCR_SRC_AMODE_DBLIDX;
>> +			d->ei = 1;
>> +			/*
>> +			 * One frame covers the port_window and by  configure
>> +			 * the source frame index to be -1 * (port_window - 1)
>> +			 * we instruct the sDMA that after a frame is processed
>> +			 * it should move back to the start of the window.
>> +			 */
>> +			d->fi = -(port_window - 1);
>> +
>> +			if (port_window >= 64)
>> +				d->csdp = CSDP_SRC_BURST_64 | CSDP_SRC_PACKED;
>> +			else if (port_window >= 32)
>> +				d->csdp = CSDP_SRC_BURST_32 | CSDP_SRC_PACKED;
>> +			else if (port_window >= 16)
>> +				d->csdp = CSDP_SRC_BURST_16 | CSDP_SRC_PACKED;
> 
> this and other would look better with a switch..

I'm not sure if it will be any better:

switch (port_window) {
case 0 ... 15:
	break;
case 16 ... 31:
	d->csdp = CSDP_SRC_BURST_16 | CSDP_SRC_PACKED;
	break;
case 32 ... 63:
	d->csdp = CSDP_SRC_BURST_32 | CSDP_SRC_PACKED;
	break;
default:
	d->csdp = CSDP_SRC_BURST_64 | CSDP_SRC_PACKED;
	break;
}


-- 
Péter

Re: [PATCH v2 2/2] dmaengine: omap-dma: Support for slave devices with data port window

[Vinod Koul]

On Fri, Nov 25, 2016 at 11:38:51AM +0200, Peter Ujfalusi wrote:
> 
> 
> On 11/25/2016 08:12 AM, Vinod Koul wrote:
> > On Thu, Nov 17, 2016 at 02:50:17PM +0200, Peter Ujfalusi wrote:
> >> @@ -921,11 +931,45 @@ static struct dma_async_tx_descriptor *omap_dma_prep_slave_sg(
> >>  
> >>  	d->ccr = c->ccr | CCR_SYNC_FRAME;
> >>  	if (dir == DMA_DEV_TO_MEM) {
> >> -		d->ccr |= CCR_DST_AMODE_POSTINC | CCR_SRC_AMODE_CONSTANT;
> >>  		d->csdp = CSDP_DST_BURST_64 | CSDP_DST_PACKED;
> >> +
> >> +		d->ccr |= CCR_DST_AMODE_POSTINC;
> >> +		if (port_window) {
> >> +			d->ccr |= CCR_SRC_AMODE_DBLIDX;
> >> +			d->ei = 1;
> >> +			/*
> >> +			 * One frame covers the port_window and by  configure
> >> +			 * the source frame index to be -1 * (port_window - 1)
> >> +			 * we instruct the sDMA that after a frame is processed
> >> +			 * it should move back to the start of the window.
> >> +			 */
> >> +			d->fi = -(port_window - 1);
> >> +
> >> +			if (port_window >= 64)
> >> +				d->csdp = CSDP_SRC_BURST_64 | CSDP_SRC_PACKED;
> >> +			else if (port_window >= 32)
> >> +				d->csdp = CSDP_SRC_BURST_32 | CSDP_SRC_PACKED;
> >> +			else if (port_window >= 16)
> >> +				d->csdp = CSDP_SRC_BURST_16 | CSDP_SRC_PACKED;
> > 
> > this and other would look better with a switch..
> 
> I'm not sure if it will be any better:
> 
> switch (port_window) {
> case 0 ... 15:
> 	break;
> case 16 ... 31:
> 	d->csdp = CSDP_SRC_BURST_16 | CSDP_SRC_PACKED;
> 	break;
> case 32 ... 63:
> 	d->csdp = CSDP_SRC_BURST_32 | CSDP_SRC_PACKED;
> 	break;
> default:
> 	d->csdp = CSDP_SRC_BURST_64 | CSDP_SRC_PACKED;
> 	break;
> }

Yeah agree with you on this..

-- 
~Vinod