imx-sdma cleanup

PATCH In review Joshua Clayton 1 participant 11 messages Jun 15, 2015 – Jun 16, 2015

Subsystems (2)

Arm i.MX and Layerscape dmaengine

lore

Share & tools

lore

Joshua Clayton

author

2015-06-15 16:18 UTC #0 lore raw

cover imx-sdma cleanup

The primary purpose of this patch series, was to
combine common parts of sdma_prep_slave_sg() and
sdma_prep_dma_cyclic().

Code that is common between cyclic and slave
prep functions is combined, which also makes it easier to see how they meaningfully differ.

In addition, we remove several unneeded or duplicative variables from struct sdma_channel, add a few more printks on errors, and simplify some internal functions.

Tested on imx6q

Joshua Clayton (10):
dma: imx-sdma: constify local structs
dma: imx-sdma: pass sdma engine into functions
dma: imx-sdma: use a container_of function
dma: sdma-imx set dma script address directly
dma: sdma-imx: print an error when context load fails.
dma: imx-sdma: config in sdma_config_channel()
dma: imx-sdma: validate word size when set
dma: imx-sdma: extract common sdma prep code
dma: imx-sdma: use a for loop
dma: imx-sdma: extract sdma_set_buffer_descriptor()

 drivers/dma/imx-sdma.c | 387 +++++++++++++++++++++++--------------------------
 1 file changed, 185 insertions(+), 202 deletions(-)

Signature · 1 line

2.1.4

Joshua Clayton

author

2015-06-15 16:18 UTC #1 lore raw

PATCH 1/10 dma: imx-sdma: constify local structs

Signed-off-by Joshua Clayton <[email protected]>

---
 drivers/dma/imx-sdma.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

drivers/dma/imx-sdma.c +11 ~~-11~~

index d0df198..0671d6d 100644
@@ -310,7 +310,7 @@ struct sdma_firmware_header {
 struct sdma_driver_data {
 	int chnenbl0;
 	int num_events;
-	struct sdma_script_start_addrs	*script_addrs;
+	const struct sdma_script_start_addrs	*script_addrs;
 };
 
 struct sdma_engine {

@@ -335,7 +335,7 @@ static struct sdma_driver_data sdma_imx31 = {
 	.num_events = 32,
 };
 
-static struct sdma_script_start_addrs sdma_script_imx25 = {
+static const struct sdma_script_start_addrs sdma_script_imx25 = {
 	.ap_2_ap_addr = 729,
 	.uart_2_mcu_addr = 904,
 	.per_2_app_addr = 1255,

@@ -351,18 +351,18 @@ static struct sdma_script_start_addrs sdma_script_imx25 = {
 	.shp_2_mcu_addr = 979,
 };
 
-static struct sdma_driver_data sdma_imx25 = {
+static const struct sdma_driver_data sdma_imx25 = {
 	.chnenbl0 = SDMA_CHNENBL0_IMX35,
 	.num_events = 48,
 	.script_addrs = &sdma_script_imx25,
 };
 
-static struct sdma_driver_data sdma_imx35 = {
+static const struct sdma_driver_data sdma_imx35 = {
 	.chnenbl0 = SDMA_CHNENBL0_IMX35,
 	.num_events = 48,
 };
 
-static struct sdma_script_start_addrs sdma_script_imx51 = {
+static const struct sdma_script_start_addrs sdma_script_imx51 = {
 	.ap_2_ap_addr = 642,
 	.uart_2_mcu_addr = 817,
 	.mcu_2_app_addr = 747,

@@ -375,13 +375,13 @@ static struct sdma_script_start_addrs sdma_script_imx51 = {
 	.shp_2_mcu_addr = 892,
 };
 
-static struct sdma_driver_data sdma_imx51 = {
+static const struct sdma_driver_data sdma_imx51 = {
 	.chnenbl0 = SDMA_CHNENBL0_IMX35,
 	.num_events = 48,
 	.script_addrs = &sdma_script_imx51,
 };
 
-static struct sdma_script_start_addrs sdma_script_imx53 = {
+static const struct sdma_script_start_addrs sdma_script_imx53 = {
 	.ap_2_ap_addr = 642,
 	.app_2_mcu_addr = 683,
 	.mcu_2_app_addr = 747,

@@ -395,13 +395,13 @@ static struct sdma_script_start_addrs sdma_script_imx53 = {
 	.mcu_2_firi_addr = 1290,
 };
 
-static struct sdma_driver_data sdma_imx53 = {
+static const struct sdma_driver_data sdma_imx53 = {
 	.chnenbl0 = SDMA_CHNENBL0_IMX35,
 	.num_events = 48,
 	.script_addrs = &sdma_script_imx53,
 };
 
-static struct sdma_script_start_addrs sdma_script_imx6q = {
+static const struct sdma_script_start_addrs sdma_script_imx6q = {
 	.ap_2_ap_addr = 642,
 	.uart_2_mcu_addr = 817,
 	.mcu_2_app_addr = 747,

@@ -414,13 +414,13 @@ static struct sdma_script_start_addrs sdma_script_imx6q = {
 	.mcu_2_spdif_addr = 1134,
 };
 
-static struct sdma_driver_data sdma_imx6q = {
+static const struct sdma_driver_data sdma_imx6q = {
 	.chnenbl0 = SDMA_CHNENBL0_IMX35,
 	.num_events = 48,
 	.script_addrs = &sdma_script_imx6q,
 };
 
-static struct platform_device_id sdma_devtypes[] = {
+static const struct platform_device_id sdma_devtypes[] = {
 	{
 		.name = "imx25-sdma",
 		.driver_data = (unsigned long)&sdma_imx25,

Signature · 1 line

2.1.4

Joshua Clayton

author

2015-06-15 16:18 UTC #2 lore raw

PATCH 2/10 dma: imx-sdma: pass sdma engine into functions

In reply to Joshua Clayton

Some ostensibly dma channel centric functions do their work either exclusively or primarily on the sdma engine struct. Change these functions to pass a struct sdma_engine and integer channel

Signed-off-by Joshua Clayton <[email protected]>

---
 drivers/dma/imx-sdma.c | 65 +++++++++++++++++++++++---------------------------
 1 file changed, 30 insertions(+), 35 deletions(-)

drivers/dma/imx-sdma.c +30 ~~-35~~

index 0671d6d..c255664 100644
@@ -467,11 +467,9 @@ static inline u32 chnenbl_ofs(struct sdma_engine *sdma, unsigned int event)
 	return chnenbl0 + event * 4;
 }
 
-static int sdma_config_ownership(struct sdma_channel *sdmac,
+static int sdma_config_ownership(struct sdma_engine *sdma, int channel,
 		bool event_override, bool mcu_override, bool dsp_override)
 {
-	struct sdma_engine *sdma = sdmac->sdma;
-	int channel = sdmac->channel;
 	unsigned long evt, mcu, dsp;
 
 	if (event_override && mcu_override && dsp_override)

@@ -569,10 +567,9 @@ static int sdma_load_script(struct sdma_engine *sdma, void *buf, int size,
 	return ret;
 }
 
-static void sdma_event_enable(struct sdma_channel *sdmac, unsigned int event)
+static void sdma_event_enable(struct sdma_engine *sdma, int channel,
+			unsigned int event)
 {
-	struct sdma_engine *sdma = sdmac->sdma;
-	int channel = sdmac->channel;
 	unsigned long val;
 	u32 chnenbl = chnenbl_ofs(sdma, event);

@@ -581,10 +578,9 @@ static void sdma_event_enable(struct sdma_channel *sdmac, unsigned int event)
 	writel_relaxed(val, sdma->regs + chnenbl);
 }
 
-static void sdma_event_disable(struct sdma_channel *sdmac, unsigned int event)
+static void sdma_event_disable(struct sdma_engine *sdma, int channel,
+		unsigned int event)
 {
-	struct sdma_engine *sdma = sdmac->sdma;
-	int channel = sdmac->channel;
 	u32 chnenbl = chnenbl_ofs(sdma, event);
 	unsigned long val;

@@ -830,20 +826,19 @@ static int sdma_load_context(struct sdma_channel *sdmac)
 	return ret;
 }
 
-static void sdma_disable_channel(struct sdma_channel *sdmac)
+static void sdma_disable_channel(struct sdma_engine *sdma, int channel)
 {
-	struct sdma_engine *sdma = sdmac->sdma;
-	int channel = sdmac->channel;
-
 	writel_relaxed(BIT(channel), sdma->regs + SDMA_H_STATSTOP);
-	sdmac->status = DMA_ERROR;
+	sdma->channel[channel].status = DMA_ERROR;
 }
 
 static int sdma_config_channel(struct sdma_channel *sdmac)
 {
 	int ret;
+	int channel = sdmac->channel;
+	struct sdma_engine *sdma = sdmac->sdma;
 
-	sdma_disable_channel(sdmac);
+	sdma_disable_channel(sdma, channel);
 
 	sdmac->event_mask[0] = 0;
 	sdmac->event_mask[1] = 0;

@@ -851,20 +846,20 @@ static int sdma_config_channel(struct sdma_channel *sdmac)
 	sdmac->per_addr = 0;
 
 	if (sdmac->event_id0) {
-		if (sdmac->event_id0 >= sdmac->sdma->drvdata->num_events)
+		if (sdmac->event_id0 >= sdma->drvdata->num_events)
 			return -EINVAL;
-		sdma_event_enable(sdmac, sdmac->event_id0);
+		sdma_event_enable(sdma, channel, sdmac->event_id0);
 	}
 
 	switch (sdmac->peripheral_type) {
 	case IMX_DMATYPE_DSP:
-		sdma_config_ownership(sdmac, false, true, true);
+		sdma_config_ownership(sdma, channel, false, true, true);
 		break;
 	case IMX_DMATYPE_MEMORY:
-		sdma_config_ownership(sdmac, false, true, false);
+		sdma_config_ownership(sdma, channel, false, true, false);
 		break;
 	default:
-		sdma_config_ownership(sdmac, true, true, false);
+		sdma_config_ownership(sdma, channel, true, true, false);
 		break;
 	}

@@ -896,12 +891,9 @@ static int sdma_config_channel(struct sdma_channel *sdmac)
 	return ret;
 }
 
-static int sdma_set_channel_priority(struct sdma_channel *sdmac,
+static int sdma_set_channel_priority(struct sdma_engine *sdma, int channel,
 		unsigned int priority)
 {
-	struct sdma_engine *sdma = sdmac->sdma;
-	int channel = sdmac->channel;
-
 	if (priority < MXC_SDMA_MIN_PRIORITY
 	    || priority > MXC_SDMA_MAX_PRIORITY) {
 		return -EINVAL;

@@ -928,7 +920,7 @@ static int sdma_request_channel(struct sdma_channel *sdmac)
 	sdma->channel_control[channel].base_bd_ptr = sdmac->bd_phys;
 	sdma->channel_control[channel].current_bd_ptr = sdmac->bd_phys;
 
-	sdma_set_channel_priority(sdmac, MXC_SDMA_DEFAULT_PRIORITY);
+	sdma_set_channel_priority(sdma, channel, MXC_SDMA_DEFAULT_PRIORITY);
 	return 0;
 out:

@@ -958,6 +950,7 @@ static dma_cookie_t sdma_tx_submit(struct dma_async_tx_descriptor *tx)
 static int sdma_alloc_chan_resources(struct dma_chan *chan)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
+	struct sdma_engine *sdma = sdmac->sdma;
 	struct imx_dma_data *data = chan->private;
 	int prio, ret;

@@ -980,14 +973,14 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
 	sdmac->peripheral_type = data->peripheral_type;
 	sdmac->event_id0 = data->dma_request;
 
-	clk_enable(sdmac->sdma->clk_ipg);
-	clk_enable(sdmac->sdma->clk_ahb);
+	clk_enable(sdma->clk_ipg);
+	clk_enable(sdma->clk_ahb);
 
 	ret = sdma_request_channel(sdmac);
 	if (ret)
 		return ret;
 
-	ret = sdma_set_channel_priority(sdmac, prio);
+	ret = sdma_set_channel_priority(sdma, sdmac->channel, prio);
 	if (ret)
 		return ret;

@@ -1003,18 +996,19 @@ static void sdma_free_chan_resources(struct dma_chan *chan)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
 	struct sdma_engine *sdma = sdmac->sdma;
+	int channel = sdmac->channel;
 
-	sdma_disable_channel(sdmac);
+	sdma_disable_channel(sdma, channel);
 
 	if (sdmac->event_id0)
-		sdma_event_disable(sdmac, sdmac->event_id0);
+		sdma_event_disable(sdma, channel, sdmac->event_id0);
 	if (sdmac->event_id1)
-		sdma_event_disable(sdmac, sdmac->event_id1);
+		sdma_event_disable(sdma, channel, sdmac->event_id1);
 
 	sdmac->event_id0 = 0;
 	sdmac->event_id1 = 0;
 
-	sdma_set_channel_priority(sdmac, 0);
+	sdma_set_channel_priority(sdma, channel, 0);
 
 	dma_free_coherent(NULL, PAGE_SIZE, sdmac->bd, sdmac->bd_phys);

@@ -1207,11 +1201,12 @@ static int sdma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 		unsigned long arg)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
+	struct sdma_engine *sdma = sdmac->sdma;
 	struct dma_slave_config *dmaengine_cfg = (void *)arg;
 
 	switch (cmd) {
 	case DMA_TERMINATE_ALL:
-		sdma_disable_channel(sdmac);
+		sdma_disable_channel(sdma, sdmac->channel);
 		return 0;
 	case DMA_SLAVE_CONFIG:
 		if (dmaengine_cfg->direction == DMA_DEV_TO_MEM) {

@@ -1390,7 +1385,7 @@ static int sdma_init(struct sdma_engine *sdma)
 	if (ret)
 		goto err_dma_alloc;
 
-	sdma_config_ownership(&sdma->channel[0], false, true, false);
+	sdma_config_ownership(sdma, 0, false, true, false);
 
 	/* Set Command Channel (Channel Zero) */
 	writel_relaxed(0x4050, sdma->regs + SDMA_CHN0ADDR);

@@ -1405,7 +1400,7 @@ static int sdma_init(struct sdma_engine *sdma)
 	writel_relaxed(SDMA_H_CONFIG_CSM, sdma->regs + SDMA_H_CONFIG);
 
 	/* Initializes channel's priorities */
-	sdma_set_channel_priority(&sdma->channel[0], 7);
+	sdma_set_channel_priority(sdma, 0, 7);
 
 	clk_disable(sdma->clk_ipg);
 	clk_disable(sdma->clk_ahb);

Signature · 1 line

2.1.4

Joshua Clayton

author

2015-06-15 16:18 UTC #3 lore raw

PATCH 3/10 dma: imx-sdma: use a container_of function

In reply to Joshua Clayton

Rather than including a struct sdma_engine pointer in each sdma_channel, introduce to_sdma_engine() to get the parent struct.

Signed-off-by Joshua Clayton <[email protected]>

---
 drivers/dma/imx-sdma.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

drivers/dma/imx-sdma.c +15 ~~-12~~

index c255664..7c8703f 100644
@@ -246,7 +246,6 @@ struct sdma_engine;
  * @num_bd		max NUM_BD. number of descriptors currently handling
  */
 struct sdma_channel {
-	struct sdma_engine		*sdma;
 	unsigned int			channel;
 	enum dma_transfer_direction		direction;
 	enum sdma_peripheral_type	peripheral_type;

@@ -467,6 +466,11 @@ static inline u32 chnenbl_ofs(struct sdma_engine *sdma, unsigned int event)
 	return chnenbl0 + event * 4;
 }
 
+static struct sdma_engine *to_sdma_engine(struct sdma_channel *sdmac)
+{
+	return container_of(sdmac, struct sdma_engine, channel[sdmac->channel]);
+}
+
 static int sdma_config_ownership(struct sdma_engine *sdma, int channel,
 		bool event_override, bool mcu_override, bool dsp_override)
 {

@@ -687,7 +691,7 @@ static irqreturn_t sdma_int_handler(int irq, void *dev_id)
 static void sdma_get_pc(struct sdma_channel *sdmac,
 		enum sdma_peripheral_type peripheral_type)
 {
-	struct sdma_engine *sdma = sdmac->sdma;
+	struct sdma_engine *sdma = to_sdma_engine(sdmac);
 	int per_2_emi = 0, emi_2_per = 0;
 	/*
 	 * These are needed once we start to support transfers between

@@ -776,7 +780,7 @@ static void sdma_get_pc(struct sdma_channel *sdmac,
 
 static int sdma_load_context(struct sdma_channel *sdmac)
 {
-	struct sdma_engine *sdma = sdmac->sdma;
+	struct sdma_engine *sdma = to_sdma_engine(sdmac);
 	int channel = sdmac->channel;
 	int load_address;
 	struct sdma_context_data *context = sdma->context;

@@ -836,7 +840,7 @@ static int sdma_config_channel(struct sdma_channel *sdmac)
 {
 	int ret;
 	int channel = sdmac->channel;
-	struct sdma_engine *sdma = sdmac->sdma;
+	struct sdma_engine *sdma = to_sdma_engine(sdmac);
 
 	sdma_disable_channel(sdma, channel);

@@ -906,7 +910,7 @@ static int sdma_set_channel_priority(struct sdma_engine *sdma, int channel,
 
 static int sdma_request_channel(struct sdma_channel *sdmac)
 {
-	struct sdma_engine *sdma = sdmac->sdma;
+	struct sdma_engine *sdma = to_sdma_engine(sdmac);
 	int channel = sdmac->channel;
 	int ret = -EBUSY;

@@ -950,7 +954,7 @@ static dma_cookie_t sdma_tx_submit(struct dma_async_tx_descriptor *tx)
 static int sdma_alloc_chan_resources(struct dma_chan *chan)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
-	struct sdma_engine *sdma = sdmac->sdma;
+	struct sdma_engine *sdma = to_sdma_engine(sdmac);
 	struct imx_dma_data *data = chan->private;
 	int prio, ret;

@@ -995,7 +999,7 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
 static void sdma_free_chan_resources(struct dma_chan *chan)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
-	struct sdma_engine *sdma = sdmac->sdma;
+	struct sdma_engine *sdma = to_sdma_engine(sdmac);
 	int channel = sdmac->channel;
 
 	sdma_disable_channel(sdma, channel);

@@ -1022,7 +1026,7 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
 		unsigned long flags, void *context)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
-	struct sdma_engine *sdma = sdmac->sdma;
+	struct sdma_engine *sdma = to_sdma_engine(sdmac);
 	int ret, i, count;
 	int channel = sdmac->channel;
 	struct scatterlist *sg;

@@ -1123,7 +1127,7 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 		unsigned long flags)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
-	struct sdma_engine *sdma = sdmac->sdma;
+	struct sdma_engine *sdma = to_sdma_engine(sdmac);
 	int num_periods = buf_len / period_len;
 	int channel = sdmac->channel;
 	int ret, i = 0, buf = 0;

@@ -1201,7 +1205,7 @@ static int sdma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 		unsigned long arg)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
-	struct sdma_engine *sdma = sdmac->sdma;
+	struct sdma_engine *sdma = to_sdma_engine(sdmac);
 	struct dma_slave_config *dmaengine_cfg = (void *)arg;
 
 	switch (cmd) {

@@ -1250,7 +1254,7 @@ static enum dma_status sdma_tx_status(struct dma_chan *chan,
 static void sdma_issue_pending(struct dma_chan *chan)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
-	struct sdma_engine *sdma = sdmac->sdma;
+	struct sdma_engine *sdma = to_sdma_engine(sdmac);
 
 	if (sdmac->status == DMA_IN_PROGRESS)
 		sdma_enable_channel(sdma, sdmac->channel);

@@ -1539,7 +1543,6 @@ static int sdma_probe(struct platform_device *pdev)
 	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
 		struct sdma_channel *sdmac = &sdma->channel[i];
 
-		sdmac->sdma = sdma;
 		spin_lock_init(&sdmac->lock);
 
 		sdmac->chan.device = &sdma->dma_device;

Signature · 1 line

2.1.4

Joshua Clayton

author

2015-06-15 16:18 UTC #4 lore raw

PATCH 4/10 dma: sdma-imx set dma script address directly

In reply to Joshua Clayton

SDMA uses different firmware scripts per device type and per direction, but only one script per channel may be used at a time. By moving the call to sdma_get_pc() into get_context, it will be called any time the dma direction may change.

Then there is no need to store multiple script addresses, so those may be removed from struct sdma_channel.

We also add an error message should setting the script fail to find a valid script address.

Signed-off-by Joshua Clayton <[email protected]>

---
 drivers/dma/imx-sdma.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

drivers/dma/imx-sdma.c +16 ~~-18~~

index 7c8703f..fdf88c4 100644
@@ -257,7 +257,6 @@ struct sdma_channel {
 	unsigned int			period_len;
 	struct sdma_buffer_descriptor	*bd;
 	dma_addr_t			bd_phys;
-	unsigned int			pc_from_device, pc_to_device;
 	unsigned long			flags;
 	dma_addr_t			per_address;
 	unsigned long			event_mask[2];

@@ -688,8 +687,8 @@ static irqreturn_t sdma_int_handler(int irq, void *dev_id)
 /*
  * sets the pc of SDMA script according to the peripheral type
  */
-static void sdma_get_pc(struct sdma_channel *sdmac,
-		enum sdma_peripheral_type peripheral_type)
+static s32 sdma_get_pc(struct sdma_channel *sdmac,
+		enum dma_transfer_direction direction)
 {
 	struct sdma_engine *sdma = to_sdma_engine(sdmac);
 	int per_2_emi = 0, emi_2_per = 0;

@@ -699,10 +698,7 @@ static void sdma_get_pc(struct sdma_channel *sdmac,
 	 */
 	int per_2_per = 0, emi_2_emi = 0;
 
-	sdmac->pc_from_device = 0;
-	sdmac->pc_to_device = 0;
-
-	switch (peripheral_type) {
+	switch (sdmac->peripheral_type) {
 	case IMX_DMATYPE_MEMORY:
 		emi_2_emi = sdma->script_addrs->ap_2_ap_addr;
 		break;

@@ -774,8 +770,14 @@ static void sdma_get_pc(struct sdma_channel *sdmac,
 		break;
 	}
 
-	sdmac->pc_from_device = per_2_emi;
-	sdmac->pc_to_device = emi_2_per;
+	switch (direction) {
+	case DMA_MEM_TO_DEV:
+		return emi_2_per;
+	case DMA_DEV_TO_MEM:
+		return per_2_emi;
+	default:
+		return 0;
+	}
 }
 
 static int sdma_load_context(struct sdma_channel *sdmac)

@@ -788,15 +790,13 @@ static int sdma_load_context(struct sdma_channel *sdmac)
 	int ret;
 	unsigned long flags;
 
-	if (sdmac->direction == DMA_DEV_TO_MEM) {
-		load_address = sdmac->pc_from_device;
-	} else {
-		load_address = sdmac->pc_to_device;
+	load_address = sdma_get_pc(sdmac, sdmac->direction);
+	if (!load_address) {
+		dev_err(sdma->dev, "SDMA channel %d: No dma script found\n",
+			channel);
+		return -EFAULT;
 	}
 
-	if (load_address < 0)
-		return load_address;
-
 	dev_dbg(sdma->dev, "load_address = %d\n", load_address);
 	dev_dbg(sdma->dev, "wml = 0x%08x\n", (u32)sdmac->watermark_level);
 	dev_dbg(sdma->dev, "shp_addr = 0x%08x\n", sdmac->shp_addr);

@@ -867,8 +867,6 @@ static int sdma_config_channel(struct sdma_channel *sdmac)
 		break;
 	}
 
-	sdma_get_pc(sdmac, sdmac->peripheral_type);
-
 	if ((sdmac->peripheral_type != IMX_DMATYPE_MEMORY) &&
 			(sdmac->peripheral_type != IMX_DMATYPE_DSP)) {
 		/* Handle multiple event channels differently */

Signature · 1 line

2.1.4

Joshua Clayton

author

2015-06-15 16:19 UTC #5 lore raw

PATCH 5/10 dma: sdma-imx: print an error when context load fails.

In reply to Joshua Clayton

Its not nice to have the context load fail silently.

Signed-off-by Joshua Clayton <[email protected]>

---
 drivers/dma/imx-sdma.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/dma/imx-sdma.c +4 -0

index fdf88c4..8358a1a 100644
@@ -827,6 +827,10 @@ static int sdma_load_context(struct sdma_channel *sdmac)
 
 	spin_unlock_irqrestore(&sdma->channel_0_lock, flags);
 
+	if (ret)
+		dev_err(sdma->dev, "SDMA channel %d: chan0 timed out\n",
+			channel);
+
 	return ret;
 }

Signature · 1 line

2.1.4

Joshua Clayton

author

2015-06-15 16:19 UTC #6 lore raw

PATCH 6/10 dma: imx-sdma: config in sdma_config_channel()

In reply to Joshua Clayton

Move functionality that is part of sdma_config_channel() from the switch statement into the function itself. This puts code together that belongs together, and gets rid of a single use variable in struct sdma_channel

Signed-off-by Joshua Clayton <[email protected]>

---
 drivers/dma/imx-sdma.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

drivers/dma/imx-sdma.c +17 ~~-16~~

index 8358a1a..a13383b 100644
@@ -258,7 +258,6 @@ struct sdma_channel {
 	struct sdma_buffer_descriptor	*bd;
 	dma_addr_t			bd_phys;
 	unsigned long			flags;
-	dma_addr_t			per_address;
 	unsigned long			event_mask[2];
 	unsigned long			watermark_level;
 	u32				shp_addr, per_addr;

@@ -840,12 +839,26 @@ static void sdma_disable_channel(struct sdma_engine *sdma, int channel)
 	sdma->channel[channel].status = DMA_ERROR;
 }
 
-static int sdma_config_channel(struct sdma_channel *sdmac)
+static int sdma_config_channel(struct sdma_channel *sdmac,
+		struct dma_slave_config *dmaengine_cfg)
 {
 	int ret;
 	int channel = sdmac->channel;
 	struct sdma_engine *sdma = to_sdma_engine(sdmac);
+	dma_addr_t per_address;
 
+	if (dmaengine_cfg->direction == DMA_DEV_TO_MEM) {
+		per_address = dmaengine_cfg->src_addr;
+		sdmac->watermark_level = dmaengine_cfg->src_maxburst *
+					dmaengine_cfg->src_addr_width;
+		sdmac->word_size = dmaengine_cfg->src_addr_width;
+	} else {
+		per_address = dmaengine_cfg->dst_addr;
+		sdmac->watermark_level = dmaengine_cfg->dst_maxburst *
+					dmaengine_cfg->dst_addr_width;
+		sdmac->word_size = dmaengine_cfg->dst_addr_width;
+	}
+	sdmac->direction = dmaengine_cfg->direction;
 	sdma_disable_channel(sdma, channel);
 
 	sdmac->event_mask[0] = 0;

@@ -887,7 +900,7 @@ static int sdma_config_channel(struct sdma_channel *sdmac)
 		/* Watermark Level */
 		sdmac->watermark_level |= sdmac->watermark_level;
 		/* Address */
-		sdmac->shp_addr = sdmac->per_address;
+		sdmac->shp_addr = per_address;
 	} else {
 		sdmac->watermark_level = 0; /* FIXME: M3_BASE_ADDRESS */
 	}

@@ -1215,19 +1228,7 @@ static int sdma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 		sdma_disable_channel(sdma, sdmac->channel);
 		return 0;
 	case DMA_SLAVE_CONFIG:
-		if (dmaengine_cfg->direction == DMA_DEV_TO_MEM) {
-			sdmac->per_address = dmaengine_cfg->src_addr;
-			sdmac->watermark_level = dmaengine_cfg->src_maxburst *
-						dmaengine_cfg->src_addr_width;
-			sdmac->word_size = dmaengine_cfg->src_addr_width;
-		} else {
-			sdmac->per_address = dmaengine_cfg->dst_addr;
-			sdmac->watermark_level = dmaengine_cfg->dst_maxburst *
-						dmaengine_cfg->dst_addr_width;
-			sdmac->word_size = dmaengine_cfg->dst_addr_width;
-		}
-		sdmac->direction = dmaengine_cfg->direction;
-		return sdma_config_channel(sdmac);
+		return sdma_config_channel(sdmac, dmaengine_cfg);
 	default:
 		return -ENOSYS;
 	}

Signature · 1 line

2.1.4

Joshua Clayton

author

2015-06-15 16:19 UTC #7 lore raw

PATCH 7/10 dma: imx-sdma: validate word size when set

In reply to Joshua Clayton

Rather than failing during an sdma prep command, dma_config_channel should fail if an invalid word size is selected.

Signed-off-by Joshua Clayton <[email protected]>

---
 drivers/dma/imx-sdma.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/dma/imx-sdma.c +7 -7

index a13383b..c9badd4 100644
@@ -858,6 +858,13 @@ static int sdma_config_channel(struct sdma_channel *sdmac,
 					dmaengine_cfg->dst_addr_width;
 		sdmac->word_size = dmaengine_cfg->dst_addr_width;
 	}
+
+	if (sdmac->word_size > DMA_SLAVE_BUSWIDTH_4_BYTES) {
+		dev_err(sdma->dev, "SDMA channel %d: invalid word size: %d\n",
+				sdmac->channel, sdmac->word_size);
+		return -EINVAL;
+	}
+
 	sdmac->direction = dmaengine_cfg->direction;
 	sdma_disable_channel(sdma, channel);

@@ -1088,11 +1095,6 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
 		bd->mode.count = count;
 		sdmac->chn_count += count;
 
-		if (sdmac->word_size > DMA_SLAVE_BUSWIDTH_4_BYTES) {
-			ret =  -EINVAL;
-			goto err_out;
-		}
-
 		switch (sdmac->word_size) {
 		case DMA_SLAVE_BUSWIDTH_4_BYTES:
 			bd->mode.command = 0;

@@ -1183,8 +1185,6 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 
 		bd->mode.count = period_len;
 
-		if (sdmac->word_size > DMA_SLAVE_BUSWIDTH_4_BYTES)
-			goto err_out;
 		if (sdmac->word_size == DMA_SLAVE_BUSWIDTH_4_BYTES)
 			bd->mode.command = 0;
 		else

Signature · 1 line

2.1.4

Joshua Clayton

author

2015-06-15 16:19 UTC #8 lore raw

PATCH 8/10 dma: imx-sdma: extract common sdma prep code

In reply to Joshua Clayton

Extract the code that is equivalent between
the slave and cyclic sdma functions.
Add some better errors for failure conditions.

Signed-off-by Joshua Clayton <[email protected]>

---
 drivers/dma/imx-sdma.c | 76 +++++++++++++++++++++-----------------------------
 1 file changed, 32 insertions(+), 44 deletions(-)

drivers/dma/imx-sdma.c +32 ~~-44~~

index c9badd4..4d447be 100644
@@ -1018,6 +1018,34 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
 	return 0;
 }
 
+int sdma_prep_common(struct sdma_channel *sdmac, int buf_count,
+			enum dma_transfer_direction direction)
+{
+	struct sdma_engine *sdma = to_sdma_engine(sdmac);
+
+	if (sdmac->status == DMA_IN_PROGRESS) {
+		dev_err(sdma->dev, "SDMA channel %d: dma already in progress\n",
+				sdmac->channel);
+		return -EBUSY;
+	}
+
+	sdmac->status = DMA_IN_PROGRESS;
+	sdmac->buf_tail = 0;
+	sdmac->direction = direction;
+
+	dev_dbg(sdma->dev, "setting up %d entries for channel %d.\n",
+			buf_count, sdmac->channel);
+
+	if (buf_count > NUM_BD) {
+		dev_err(sdma->dev, "SDMA channel %d: maximum number of buffers exceeded: %d > %d\n",
+				sdmac->channel, buf_count, NUM_BD);
+		return -EINVAL;
+	}
+	sdmac->num_bd = buf_count;
+
+	return sdma_load_context(sdmac);
+}
+
 static void sdma_free_chan_resources(struct dma_chan *chan)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);

@@ -1053,28 +1081,12 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
 	int channel = sdmac->channel;
 	struct scatterlist *sg;
 
-	if (sdmac->status == DMA_IN_PROGRESS)
-		return NULL;
-	sdmac->status = DMA_IN_PROGRESS;
-
-	sdmac->flags = 0;
-
-	sdmac->buf_tail = 0;
-
-	dev_dbg(sdma->dev, "setting up %d entries for channel %d.\n",
-			sg_len, channel);
-
-	sdmac->direction = direction;
-	ret = sdma_load_context(sdmac);
+	ret = sdma_prep_common(sdmac, sg_len, direction);
 	if (ret)
 		goto err_out;
 
-	if (sg_len > NUM_BD) {
-		dev_err(sdma->dev, "SDMA channel %d: maximum number of sg exceeded: %d > %d\n",
-				channel, sg_len, NUM_BD);
-		ret = -EINVAL;
-		goto err_out;
-	}
+	sdmac->flags = 0;
+
 
 	sdmac->chn_count = 0;
 	for_each_sg(sgl, sg, sg_len, i) {

@@ -1129,7 +1141,6 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
 		bd->mode.status = param;
 	}
 
-	sdmac->num_bd = sg_len;
 	sdma->channel_control[channel].current_bd_ptr = sdmac->bd_phys;
 
 	return &sdmac->desc;

@@ -1149,33 +1160,11 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 	int channel = sdmac->channel;
 	int ret, i = 0, buf = 0;
 
-	dev_dbg(sdma->dev, "%s channel: %d\n", __func__, channel);
-
-	if (sdmac->status == DMA_IN_PROGRESS)
-		return NULL;
-
-	sdmac->status = DMA_IN_PROGRESS;
+	sdma_prep_common(sdmac, buf_len / period_len, direction);
 
-	sdmac->buf_tail = 0;
 	sdmac->period_len = period_len;
 
 	sdmac->flags |= IMX_DMA_SG_LOOP;
-	sdmac->direction = direction;
-	ret = sdma_load_context(sdmac);
-	if (ret)
-		goto err_out;
-
-	if (num_periods > NUM_BD) {
-		dev_err(sdma->dev, "SDMA channel %d: maximum number of sg exceeded: %d > %d\n",
-				channel, num_periods, NUM_BD);
-		goto err_out;
-	}
-
-	if (period_len > 0xffff) {
-		dev_err(sdma->dev, "SDMA channel %d: maximum period size exceeded: %d > %d\n",
-				channel, period_len, 0xffff);
-		goto err_out;
-	}
 
 	while (buf < buf_len) {
 		struct sdma_buffer_descriptor *bd = &sdmac->bd[i];

@@ -1207,7 +1196,6 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 		i++;
 	}
 
-	sdmac->num_bd = num_periods;
 	sdma->channel_control[channel].current_bd_ptr = sdmac->bd_phys;
 
 	return &sdmac->desc;

Signature · 1 line

2.1.4

Joshua Clayton

author

2015-06-15 16:19 UTC #9 lore raw

PATCH 9/10 dma: imx-sdma: use a for loop

In reply to Joshua Clayton

Switch out the strange while loop for a for loop Someone might have thought the this construction potects against odd sized buffers, but num_periods will alwasy be no bigger than the number of whole buffer descriptors

Signed-off-by Joshua Clayton <[email protected]>

---
 drivers/dma/imx-sdma.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

drivers/dma/imx-sdma.c +2 -5

index 4d447be..dfebef9 100644
@@ -1158,7 +1158,7 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 	struct sdma_engine *sdma = to_sdma_engine(sdmac);
 	int num_periods = buf_len / period_len;
 	int channel = sdmac->channel;
-	int ret, i = 0, buf = 0;
+	int ret, i;
 
 	sdma_prep_common(sdmac, buf_len / period_len, direction);

@@ -1166,7 +1166,7 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 
 	sdmac->flags |= IMX_DMA_SG_LOOP;
 
-	while (buf < buf_len) {
+	for (i = 0; i < num_periods; i++) {
 		struct sdma_buffer_descriptor *bd = &sdmac->bd[i];
 		int param;

@@ -1191,9 +1191,6 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 		bd->mode.status = param;
 
 		dma_addr += period_len;
-		buf += period_len;
-
-		i++;
 	}
 
 	sdma->channel_control[channel].current_bd_ptr = sdmac->bd_phys;

Signature · 1 line

2.1.4

Joshua Clayton

author

2015-06-16 14:57 UTC #10 lore raw

In reply to Joshua Clayton

Quoted 3 lines · Joshua Clayton

The primary purpose of this patch series, was to
combine common parts of sdma_prep_slave_sg() and
sdma_prep_dma_cyclic().

...
My apologies. I did not check that these patches apply against linux-next. I am reworking a couple of the patches.

Sorry for the premature submission.

Signature · 1 line

~Joshua Clayton