diff mbox series

[v2,10/19] dma: imx-sdma: Add multi fifo support

Message ID 20220328112744.1575631-11-s.hauer@pengutronix.de
State Superseded
Headers show
Series ASoC: fsl_micfil: Driver updates | expand

Commit Message

Sascha Hauer March 28, 2022, 11:27 a.m. UTC
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---

Notes:
    Changes since v1:
    - Drop unused variable sw_done_sel
    - Evaluate sdmac->direction directly instead of storing value in n_fifos

 drivers/dma/imx-sdma.c                | 57 +++++++++++++++++++++++++++
 include/linux/platform_data/dma-imx.h |  7 ++++
 2 files changed, 64 insertions(+)

Comments

Fabio Estevam March 29, 2022, 10:55 a.m. UTC | #1
Hi Sascha,

On Mon, Mar 28, 2022 at 8:28 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Please add a commit log, thanks.
Vinod Koul March 31, 2022, 5:26 a.m. UTC | #2
On 28-03-22, 13:27, Sascha Hauer wrote:
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

it is dmaengine: xxx

Also is this patch dependent on rest of the series, if not consider
sending separately

> diff --git a/include/linux/platform_data/dma-imx.h b/include/linux/platform_data/dma-imx.h
> index 281adbb26e6bd..4a43a048e1b4d 100644
> --- a/include/linux/platform_data/dma-imx.h
> +++ b/include/linux/platform_data/dma-imx.h
> @@ -39,6 +39,7 @@ enum sdma_peripheral_type {
>  	IMX_DMATYPE_SSI_DUAL,	/* SSI Dual FIFO */
>  	IMX_DMATYPE_ASRC_SP,	/* Shared ASRC */
>  	IMX_DMATYPE_SAI,	/* SAI */
> +	IMX_DMATYPE_MULTI_SAI,	/* MULTI FIFOs For Audio */
>  };
>  
>  enum imx_dma_prio {
> @@ -65,4 +66,10 @@ static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
>  		!strcmp(chan->device->dev->driver->name, "imx-dma");
>  }
>  
> +struct sdma_peripheral_config {
> +	int n_fifos_src;
> +	int n_fifos_dst;
> +	bool sw_done;
> +};

Not more platform data :(

Can you explain this structure and why this is required? What do these
fields refer to..?
Sascha Hauer March 31, 2022, 6:49 a.m. UTC | #3
On Thu, Mar 31, 2022 at 10:56:09AM +0530, Vinod Koul wrote:
> On 28-03-22, 13:27, Sascha Hauer wrote:
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> it is dmaengine: xxx

Ok.

> 
> Also is this patch dependent on rest of the series, if not consider
> sending separately

The rest of this series indeed depends on this patch.

> 
> > diff --git a/include/linux/platform_data/dma-imx.h b/include/linux/platform_data/dma-imx.h
> > index 281adbb26e6bd..4a43a048e1b4d 100644
> > --- a/include/linux/platform_data/dma-imx.h
> > +++ b/include/linux/platform_data/dma-imx.h
> > @@ -39,6 +39,7 @@ enum sdma_peripheral_type {
> >  	IMX_DMATYPE_SSI_DUAL,	/* SSI Dual FIFO */
> >  	IMX_DMATYPE_ASRC_SP,	/* Shared ASRC */
> >  	IMX_DMATYPE_SAI,	/* SAI */
> > +	IMX_DMATYPE_MULTI_SAI,	/* MULTI FIFOs For Audio */
> >  };
> >  
> >  enum imx_dma_prio {
> > @@ -65,4 +66,10 @@ static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
> >  		!strcmp(chan->device->dev->driver->name, "imx-dma");
> >  }
> >  
> > +struct sdma_peripheral_config {
> > +	int n_fifos_src;
> > +	int n_fifos_dst;
> > +	bool sw_done;
> > +};
> 
> Not more platform data :(

I'm not sure what you are referring to as platform_data. This is not the
classical platform_data that is attached to a platform_device to
configure behaviour of that device. It is rather data that needs to be
communicated from the clients of the SDMA engine to the SDMA engine.

I have put this into include/linux/platform_data/dma-imx.h because
that's the only existing include file that is available. I could move
this to a new file if you like that better.

> 
> Can you explain this structure and why this is required? What do these
> fields refer to..?

The reasoning for this structure is described in the commit message that
I have forgotten:

    The i.MX SDMA engine can read from / write to multiple successive
    hardware FIFO registers, referred to as "Multi FIFO support". This is
    needed for the micfil driver and certain configurations of the SAI
    driver. This patch adds support for this feature.

    The number of FIFOs to read from / write to must be communicated from
    the client driver to the SDMA engine. For this the struct
    dma_slave_config::peripheral_config field is used.

I can describe the individual fields of struct sdma_peripheral_config in
the header file if that's your point.

Sascha
Vinod Koul March 31, 2022, 6:54 a.m. UTC | #4
On 31-03-22, 08:49, Sascha Hauer wrote:
> On Thu, Mar 31, 2022 at 10:56:09AM +0530, Vinod Koul wrote:
> > On 28-03-22, 13:27, Sascha Hauer wrote:
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > 
> > it is dmaengine: xxx
> 
> Ok.
> 
> > 
> > Also is this patch dependent on rest of the series, if not consider
> > sending separately
> 
> The rest of this series indeed depends on this patch.
> 
> > 
> > > diff --git a/include/linux/platform_data/dma-imx.h b/include/linux/platform_data/dma-imx.h
> > > index 281adbb26e6bd..4a43a048e1b4d 100644
> > > --- a/include/linux/platform_data/dma-imx.h
> > > +++ b/include/linux/platform_data/dma-imx.h
> > > @@ -39,6 +39,7 @@ enum sdma_peripheral_type {
> > >  	IMX_DMATYPE_SSI_DUAL,	/* SSI Dual FIFO */
> > >  	IMX_DMATYPE_ASRC_SP,	/* Shared ASRC */
> > >  	IMX_DMATYPE_SAI,	/* SAI */
> > > +	IMX_DMATYPE_MULTI_SAI,	/* MULTI FIFOs For Audio */
> > >  };
> > >  
> > >  enum imx_dma_prio {
> > > @@ -65,4 +66,10 @@ static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
> > >  		!strcmp(chan->device->dev->driver->name, "imx-dma");
> > >  }
> > >  
> > > +struct sdma_peripheral_config {
> > > +	int n_fifos_src;
> > > +	int n_fifos_dst;
> > > +	bool sw_done;
> > > +};
> > 
> > Not more platform data :(
> 
> I'm not sure what you are referring to as platform_data. This is not the
> classical platform_data that is attached to a platform_device to
> configure behaviour of that device. It is rather data that needs to be
> communicated from the clients of the SDMA engine to the SDMA engine.
> 
> I have put this into include/linux/platform_data/dma-imx.h because
> that's the only existing include file that is available. I could move
> this to a new file if you like that better.

Lets move to include/linux/dma/

> 
> > 
> > Can you explain this structure and why this is required? What do these
> > fields refer to..?
> 
> The reasoning for this structure is described in the commit message that
> I have forgotten:
> 
>     The i.MX SDMA engine can read from / write to multiple successive
>     hardware FIFO registers, referred to as "Multi FIFO support". This is
>     needed for the micfil driver and certain configurations of the SAI
>     driver. This patch adds support for this feature.
> 
>     The number of FIFOs to read from / write to must be communicated from
>     the client driver to the SDMA engine. For this the struct
>     dma_slave_config::peripheral_config field is used.
> 
> I can describe the individual fields of struct sdma_peripheral_config in
> the header file if that's your point.

So you need to know the number of fifo right, what does sw_done imply?

Also if this is hardware information, why not use dma-cells for this?
Sascha Hauer March 31, 2022, 7:58 a.m. UTC | #5
On Thu, Mar 31, 2022 at 12:24:46PM +0530, Vinod Koul wrote:
> On 31-03-22, 08:49, Sascha Hauer wrote:
> > On Thu, Mar 31, 2022 at 10:56:09AM +0530, Vinod Koul wrote:
> > > On 28-03-22, 13:27, Sascha Hauer wrote:
> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > 
> > > it is dmaengine: xxx
> > 
> > Ok.
> > 
> > > 
> > > Also is this patch dependent on rest of the series, if not consider
> > > sending separately
> > 
> > The rest of this series indeed depends on this patch.
> > 
> > > 
> > > > diff --git a/include/linux/platform_data/dma-imx.h b/include/linux/platform_data/dma-imx.h
> > > > index 281adbb26e6bd..4a43a048e1b4d 100644
> > > > --- a/include/linux/platform_data/dma-imx.h
> > > > +++ b/include/linux/platform_data/dma-imx.h
> > > > @@ -39,6 +39,7 @@ enum sdma_peripheral_type {
> > > >  	IMX_DMATYPE_SSI_DUAL,	/* SSI Dual FIFO */
> > > >  	IMX_DMATYPE_ASRC_SP,	/* Shared ASRC */
> > > >  	IMX_DMATYPE_SAI,	/* SAI */
> > > > +	IMX_DMATYPE_MULTI_SAI,	/* MULTI FIFOs For Audio */
> > > >  };
> > > >  
> > > >  enum imx_dma_prio {
> > > > @@ -65,4 +66,10 @@ static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
> > > >  		!strcmp(chan->device->dev->driver->name, "imx-dma");
> > > >  }
> > > >  
> > > > +struct sdma_peripheral_config {
> > > > +	int n_fifos_src;
> > > > +	int n_fifos_dst;
> > > > +	bool sw_done;
> > > > +};
> > > 
> > > Not more platform data :(
> > 
> > I'm not sure what you are referring to as platform_data. This is not the
> > classical platform_data that is attached to a platform_device to
> > configure behaviour of that device. It is rather data that needs to be
> > communicated from the clients of the SDMA engine to the SDMA engine.
> > 
> > I have put this into include/linux/platform_data/dma-imx.h because
> > that's the only existing include file that is available. I could move
> > this to a new file if you like that better.
> 
> Lets move to include/linux/dma/

Ok.

> 
> > 
> > > 
> > > Can you explain this structure and why this is required? What do these
> > > fields refer to..?
> > 
> > The reasoning for this structure is described in the commit message that
> > I have forgotten:
> > 
> >     The i.MX SDMA engine can read from / write to multiple successive
> >     hardware FIFO registers, referred to as "Multi FIFO support". This is
> >     needed for the micfil driver and certain configurations of the SAI
> >     driver. This patch adds support for this feature.
> > 
> >     The number of FIFOs to read from / write to must be communicated from
> >     the client driver to the SDMA engine. For this the struct
> >     dma_slave_config::peripheral_config field is used.
> > 
> > I can describe the individual fields of struct sdma_peripheral_config in
> > the header file if that's your point.
> 
> So you need to know the number of fifo right, what does sw_done imply?

Honestly I don't know. Setting sw_done results in the DONE_SEL0 bit in
the SDMA engine being set. This is described in the reference manual
as:

DONE_SEL0 Select Done from SW or HW for channel 0
          0 HW
          1 SW

I can only assume that the signaling when a channel has transferred
enough data (the generation of the channel done interrupt?) can either
be done in hardware or in software in the SDMA engine. What I can tell
for sure is that I need this bit set ;)

> 
> Also if this is hardware information, why not use dma-cells for this?

The information is not static. For the micfil the number of fifos equals
the number of channels that are recorded, see next patch:

+       micfil->dma_params_rx.peripheral_config = &micfil->sdmacfg;
+       micfil->dma_params_rx.peripheral_size = sizeof(micfil->sdmacfg);
+       micfil->sdmacfg.n_fifos_src = channels;
+       micfil->sdmacfg.sw_done = true;

Sascha
Sascha Hauer April 1, 2022, 12:01 p.m. UTC | #6
On Thu, Mar 31, 2022 at 12:24:46PM +0530, Vinod Koul wrote:
> On 31-03-22, 08:49, Sascha Hauer wrote:
> > On Thu, Mar 31, 2022 at 10:56:09AM +0530, Vinod Koul wrote:
> > > On 28-03-22, 13:27, Sascha Hauer wrote:
> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > 
> > > it is dmaengine: xxx
> > 
> > Ok.
> > 
> > > 
> > > Also is this patch dependent on rest of the series, if not consider
> > > sending separately
> > 
> > The rest of this series indeed depends on this patch.
> > 
> > > 
> > > > diff --git a/include/linux/platform_data/dma-imx.h b/include/linux/platform_data/dma-imx.h
> > > > index 281adbb26e6bd..4a43a048e1b4d 100644
> > > > --- a/include/linux/platform_data/dma-imx.h
> > > > +++ b/include/linux/platform_data/dma-imx.h
> > > > @@ -39,6 +39,7 @@ enum sdma_peripheral_type {
> > > >  	IMX_DMATYPE_SSI_DUAL,	/* SSI Dual FIFO */
> > > >  	IMX_DMATYPE_ASRC_SP,	/* Shared ASRC */
> > > >  	IMX_DMATYPE_SAI,	/* SAI */
> > > > +	IMX_DMATYPE_MULTI_SAI,	/* MULTI FIFOs For Audio */
> > > >  };
> > > >  
> > > >  enum imx_dma_prio {
> > > > @@ -65,4 +66,10 @@ static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
> > > >  		!strcmp(chan->device->dev->driver->name, "imx-dma");
> > > >  }
> > > >  
> > > > +struct sdma_peripheral_config {
> > > > +	int n_fifos_src;
> > > > +	int n_fifos_dst;
> > > > +	bool sw_done;
> > > > +};
> > > 
> > > Not more platform data :(
> > 
> > I'm not sure what you are referring to as platform_data. This is not the
> > classical platform_data that is attached to a platform_device to
> > configure behaviour of that device. It is rather data that needs to be
> > communicated from the clients of the SDMA engine to the SDMA engine.
> > 
> > I have put this into include/linux/platform_data/dma-imx.h because
> > that's the only existing include file that is available. I could move
> > this to a new file if you like that better.
> 
> Lets move to include/linux/dma/

What about the other stuff in include/linux/platform_data/dma-imx.h,
should this go to include/linux/dma/ as well? There is nothing in it
that is platform_data at all.

Sascha
diff mbox series

Patch

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 1038f6bc7f846..f08c409231b7b 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -14,6 +14,7 @@ 
 #include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/types.h>
+#include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/mm.h>
 #include <linux/interrupt.h>
@@ -73,6 +74,7 @@ 
 #define SDMA_CHNENBL0_IMX35	0x200
 #define SDMA_CHNENBL0_IMX31	0x080
 #define SDMA_CHNPRI_0		0x100
+#define SDMA_DONE0_CONFIG	0x1000
 
 /*
  * Buffer descriptor status values.
@@ -180,6 +182,12 @@ 
 				 BIT(DMA_MEM_TO_DEV) | \
 				 BIT(DMA_DEV_TO_DEV))
 
+#define SDMA_WATERMARK_LEVEL_N_FIFOS	GENMASK(15, 12)
+#define SDMA_WATERMARK_LEVEL_SW_DONE	BIT(23)
+
+#define SDMA_DONE0_CONFIG_DONE_SEL	BIT(7)
+#define SDMA_DONE0_CONFIG_DONE_DIS	BIT(6)
+
 /**
  * struct sdma_script_start_addrs - SDMA script start pointers
  *
@@ -441,6 +449,9 @@  struct sdma_channel {
 	struct work_struct		terminate_worker;
 	struct list_head                terminated;
 	bool				is_ram_script;
+	unsigned int			n_fifos_src;
+	unsigned int			n_fifos_dst;
+	bool				sw_done;
 };
 
 #define IMX_DMA_SG_LOOP		BIT(0)
@@ -773,6 +784,14 @@  static void sdma_event_enable(struct sdma_channel *sdmac, unsigned int event)
 	val = readl_relaxed(sdma->regs + chnenbl);
 	__set_bit(channel, &val);
 	writel_relaxed(val, sdma->regs + chnenbl);
+
+	/* Set SDMA_DONEx_CONFIG is sw_done enabled */
+	if (sdmac->sw_done) {
+		val = readl_relaxed(sdma->regs + SDMA_DONE0_CONFIG);
+		val |= SDMA_DONE0_CONFIG_DONE_SEL;
+		val &= ~SDMA_DONE0_CONFIG_DONE_DIS;
+		writel_relaxed(val, sdma->regs + SDMA_DONE0_CONFIG);
+	}
 }
 
 static void sdma_event_disable(struct sdma_channel *sdmac, unsigned int event)
@@ -1022,6 +1041,10 @@  static int sdma_get_pc(struct sdma_channel *sdmac,
 	case IMX_DMATYPE_IPU_MEMORY:
 		emi_2_per = sdma->script_addrs->ext_mem_2_ipu_addr;
 		break;
+	case IMX_DMATYPE_MULTI_SAI:
+		per_2_emi = sdma->script_addrs->sai_2_mcu_addr;
+		emi_2_per = sdma->script_addrs->mcu_2_sai_addr;
+		break;
 	default:
 		dev_err(sdma->dev, "Unsupported transfer type %d\n",
 			peripheral_type);
@@ -1198,6 +1221,22 @@  static void sdma_set_watermarklevel_for_p2p(struct sdma_channel *sdmac)
 	sdmac->watermark_level |= SDMA_WATERMARK_LEVEL_CONT;
 }
 
+static void sdma_set_watermarklevel_for_sais(struct sdma_channel *sdmac)
+{
+	unsigned int n_fifos;
+
+	if (sdmac->sw_done)
+		sdmac->watermark_level |= SDMA_WATERMARK_LEVEL_SW_DONE;
+
+	if (sdmac->direction == DMA_DEV_TO_MEM)
+		n_fifos = sdmac->n_fifos_src;
+	else
+		n_fifos = sdmac->n_fifos_dst;
+
+	sdmac->watermark_level |=
+			FIELD_PREP(SDMA_WATERMARK_LEVEL_N_FIFOS, n_fifos);
+}
+
 static int sdma_config_channel(struct dma_chan *chan)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
@@ -1234,6 +1273,10 @@  static int sdma_config_channel(struct dma_chan *chan)
 			    sdmac->peripheral_type == IMX_DMATYPE_ASRC)
 				sdma_set_watermarklevel_for_p2p(sdmac);
 		} else {
+			if (sdmac->peripheral_type ==
+					IMX_DMATYPE_MULTI_SAI)
+				sdma_set_watermarklevel_for_sais(sdmac);
+
 			__set_bit(sdmac->event_id0, sdmac->event_mask);
 		}
 
@@ -1691,9 +1734,23 @@  static int sdma_config(struct dma_chan *chan,
 		       struct dma_slave_config *dmaengine_cfg)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
+	struct sdma_engine *sdma = sdmac->sdma;
 
 	memcpy(&sdmac->slave_config, dmaengine_cfg, sizeof(*dmaengine_cfg));
 
+	if (dmaengine_cfg->peripheral_config) {
+		struct sdma_peripheral_config *sdmacfg = dmaengine_cfg->peripheral_config;
+		if (dmaengine_cfg->peripheral_size != sizeof(struct sdma_peripheral_config)) {
+			dev_err(sdma->dev, "Invalid peripheral size %zu, expected %zu\n",
+				dmaengine_cfg->peripheral_size,
+				sizeof(struct sdma_peripheral_config));
+			return -EINVAL;
+		}
+		sdmac->n_fifos_src = sdmacfg->n_fifos_src;
+		sdmac->n_fifos_dst = sdmacfg->n_fifos_dst;
+		sdmac->sw_done = sdmacfg->sw_done;
+	}
+
 	/* Set ENBLn earlier to make sure dma request triggered after that */
 	if (sdmac->event_id0 >= sdmac->sdma->drvdata->num_events)
 		return -EINVAL;
diff --git a/include/linux/platform_data/dma-imx.h b/include/linux/platform_data/dma-imx.h
index 281adbb26e6bd..4a43a048e1b4d 100644
--- a/include/linux/platform_data/dma-imx.h
+++ b/include/linux/platform_data/dma-imx.h
@@ -39,6 +39,7 @@  enum sdma_peripheral_type {
 	IMX_DMATYPE_SSI_DUAL,	/* SSI Dual FIFO */
 	IMX_DMATYPE_ASRC_SP,	/* Shared ASRC */
 	IMX_DMATYPE_SAI,	/* SAI */
+	IMX_DMATYPE_MULTI_SAI,	/* MULTI FIFOs For Audio */
 };
 
 enum imx_dma_prio {
@@ -65,4 +66,10 @@  static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
 		!strcmp(chan->device->dev->driver->name, "imx-dma");
 }
 
+struct sdma_peripheral_config {
+	int n_fifos_src;
+	int n_fifos_dst;
+	bool sw_done;
+};
+
 #endif