[PATCH] mtd: onenand: omap2: Simplify the DMA setup for various paths

[PATCH] mtd: onenand: omap2: Simplify the DMA setup for various paths

[Peter Ujfalusi]

We have 4 functions containing almost identical DMA setup code. Create one
function which can set up the DMA for both read and write and use this in
place for the setup code in the driver.
The new function will use wait_for_completion_timeout() and it will figure
out the best data_type to be used for the transfer instead of hardwiring
32 or 16 bit data.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/mtd/onenand/omap2.c | 106 ++++++++++++++++++--------------------------
 1 file changed, 42 insertions(+), 64 deletions(-)

diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
index 0aacf125938b..58576c9babb0 100644
--- a/drivers/mtd/onenand/omap2.c
+++ b/drivers/mtd/onenand/omap2.c
@@ -291,6 +291,30 @@ static inline int omap2_onenand_bufferram_offset(struct mtd_info *mtd, int area)
 	return 0;
 }
 
+static inline int omap2_onenand_dma_transfer(struct omap2_onenand *c,
+					     dma_addr_t src, dma_addr_t dst,
+					     size_t count)
+{
+	int data_type = __ffs((src | dst | count));
+
+	if (data_type > OMAP_DMA_DATA_TYPE_S32)
+		data_type = OMAP_DMA_DATA_TYPE_S32;
+
+	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
+				     count / BIT(data_type), 1, 0, 0, 0);
+	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
+				src, 0, 0);
+	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
+				 dst, 0, 0);
+
+	reinit_completion(&c->dma_done);
+	omap_start_dma(c->dma_channel);
+	if (wait_for_completion_timeout(&c->dma_done, msecs_to_jiffies(20)))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
 #if defined(CONFIG_ARCH_OMAP3) || defined(MULTI_OMAP2)
 
 static int omap3_onenand_read_bufferram(struct mtd_info *mtd, int area,
@@ -301,10 +325,9 @@ static int omap3_onenand_read_bufferram(struct mtd_info *mtd, int area,
 	struct onenand_chip *this = mtd->priv;
 	dma_addr_t dma_src, dma_dst;
 	int bram_offset;
-	unsigned long timeout;
 	void *buf = (void *)buffer;
 	size_t xtra;
-	volatile unsigned *done;
+	int ret;
 
 	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
 	if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
@@ -341,25 +364,10 @@ static int omap3_onenand_read_bufferram(struct mtd_info *mtd, int area,
 		goto out_copy;
 	}
 
-	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
-				     count >> 2, 1, 0, 0, 0);
-	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				dma_src, 0, 0);
-	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				 dma_dst, 0, 0);
-
-	reinit_completion(&c->dma_done);
-	omap_start_dma(c->dma_channel);
-
-	timeout = jiffies + msecs_to_jiffies(20);
-	done = &c->dma_done.done;
-	while (time_before(jiffies, timeout))
-		if (*done)
-			break;
-
+	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
 	dma_unmap_single(&c->pdev->dev, dma_dst, count, DMA_FROM_DEVICE);
 
-	if (!*done) {
+	if (ret) {
 		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
 		goto out_copy;
 	}
@@ -379,9 +387,8 @@ static int omap3_onenand_write_bufferram(struct mtd_info *mtd, int area,
 	struct onenand_chip *this = mtd->priv;
 	dma_addr_t dma_src, dma_dst;
 	int bram_offset;
-	unsigned long timeout;
 	void *buf = (void *)buffer;
-	volatile unsigned *done;
+	int ret;
 
 	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
 	if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
@@ -412,25 +419,10 @@ static int omap3_onenand_write_bufferram(struct mtd_info *mtd, int area,
 		return -1;
 	}
 
-	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
-				     count >> 2, 1, 0, 0, 0);
-	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				dma_src, 0, 0);
-	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				 dma_dst, 0, 0);
-
-	reinit_completion(&c->dma_done);
-	omap_start_dma(c->dma_channel);
-
-	timeout = jiffies + msecs_to_jiffies(20);
-	done = &c->dma_done.done;
-	while (time_before(jiffies, timeout))
-		if (*done)
-			break;
-
+	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
 	dma_unmap_single(&c->pdev->dev, dma_src, count, DMA_TO_DEVICE);
 
-	if (!*done) {
+	if (ret) {
 		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
 		goto out_copy;
 	}
@@ -469,7 +461,7 @@ static int omap2_onenand_read_bufferram(struct mtd_info *mtd, int area,
 	struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
 	struct onenand_chip *this = mtd->priv;
 	dma_addr_t dma_src, dma_dst;
-	int bram_offset;
+	int bram_offset, ret;
 
 	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
 	/* DMA is not used.  Revisit PM requirements before enabling it. */
@@ -491,20 +483,13 @@ static int omap2_onenand_read_bufferram(struct mtd_info *mtd, int area,
 		return -1;
 	}
 
-	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
-				     count / 4, 1, 0, 0, 0);
-	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				dma_src, 0, 0);
-	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				 dma_dst, 0, 0);
-
-	reinit_completion(&c->dma_done);
-	omap_start_dma(c->dma_channel);
-	wait_for_completion(&c->dma_done);
-
+	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
 	dma_unmap_single(&c->pdev->dev, dma_dst, count, DMA_FROM_DEVICE);
 
-	return 0;
+	if (ret)
+		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
+
+	return ret;
 }
 
 static int omap2_onenand_write_bufferram(struct mtd_info *mtd, int area,
@@ -514,7 +499,7 @@ static int omap2_onenand_write_bufferram(struct mtd_info *mtd, int area,
 	struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
 	struct onenand_chip *this = mtd->priv;
 	dma_addr_t dma_src, dma_dst;
-	int bram_offset;
+	int bram_offset, ret;
 
 	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
 	/* DMA is not used.  Revisit PM requirements before enabling it. */
@@ -536,20 +521,13 @@ static int omap2_onenand_write_bufferram(struct mtd_info *mtd, int area,
 		return -1;
 	}
 
-	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S16,
-				     count / 2, 1, 0, 0, 0);
-	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				dma_src, 0, 0);
-	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				 dma_dst, 0, 0);
-
-	reinit_completion(&c->dma_done);
-	omap_start_dma(c->dma_channel);
-	wait_for_completion(&c->dma_done);
-
+	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
 	dma_unmap_single(&c->pdev->dev, dma_src, count, DMA_TO_DEVICE);
 
-	return 0;
+	if (ret)
+		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
+
+	return ret;
 }
 
 #else
-- 
2.6.4

[PATCH] mtd: onenand: omap2: Simplify the DMA setup for various paths

[Peter Ujfalusi]

We have 4 functions containing almost identical DMA setup code. Create one
function which can set up the DMA for both read and write and use this in
place for the setup code in the driver.
The new function will use wait_for_completion_timeout() and it will figure
out the best data_type to be used for the transfer instead of hardwiring
32 or 16 bit data.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/mtd/onenand/omap2.c | 106 ++++++++++++++++++--------------------------
 1 file changed, 42 insertions(+), 64 deletions(-)

diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
index 0aacf125938b..58576c9babb0 100644
--- a/drivers/mtd/onenand/omap2.c
+++ b/drivers/mtd/onenand/omap2.c
@@ -291,6 +291,30 @@ static inline int omap2_onenand_bufferram_offset(struct mtd_info *mtd, int area)
 	return 0;
 }
 
+static inline int omap2_onenand_dma_transfer(struct omap2_onenand *c,
+					     dma_addr_t src, dma_addr_t dst,
+					     size_t count)
+{
+	int data_type = __ffs((src | dst | count));
+
+	if (data_type > OMAP_DMA_DATA_TYPE_S32)
+		data_type = OMAP_DMA_DATA_TYPE_S32;
+
+	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
+				     count / BIT(data_type), 1, 0, 0, 0);
+	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
+				src, 0, 0);
+	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
+				 dst, 0, 0);
+
+	reinit_completion(&c->dma_done);
+	omap_start_dma(c->dma_channel);
+	if (wait_for_completion_timeout(&c->dma_done, msecs_to_jiffies(20)))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
 #if defined(CONFIG_ARCH_OMAP3) || defined(MULTI_OMAP2)
 
 static int omap3_onenand_read_bufferram(struct mtd_info *mtd, int area,
@@ -301,10 +325,9 @@ static int omap3_onenand_read_bufferram(struct mtd_info *mtd, int area,
 	struct onenand_chip *this = mtd->priv;
 	dma_addr_t dma_src, dma_dst;
 	int bram_offset;
-	unsigned long timeout;
 	void *buf = (void *)buffer;
 	size_t xtra;
-	volatile unsigned *done;
+	int ret;
 
 	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
 	if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
@@ -341,25 +364,10 @@ static int omap3_onenand_read_bufferram(struct mtd_info *mtd, int area,
 		goto out_copy;
 	}
 
-	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
-				     count >> 2, 1, 0, 0, 0);
-	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				dma_src, 0, 0);
-	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				 dma_dst, 0, 0);
-
-	reinit_completion(&c->dma_done);
-	omap_start_dma(c->dma_channel);
-
-	timeout = jiffies + msecs_to_jiffies(20);
-	done = &c->dma_done.done;
-	while (time_before(jiffies, timeout))
-		if (*done)
-			break;
-
+	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
 	dma_unmap_single(&c->pdev->dev, dma_dst, count, DMA_FROM_DEVICE);
 
-	if (!*done) {
+	if (ret) {
 		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
 		goto out_copy;
 	}
@@ -379,9 +387,8 @@ static int omap3_onenand_write_bufferram(struct mtd_info *mtd, int area,
 	struct onenand_chip *this = mtd->priv;
 	dma_addr_t dma_src, dma_dst;
 	int bram_offset;
-	unsigned long timeout;
 	void *buf = (void *)buffer;
-	volatile unsigned *done;
+	int ret;
 
 	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
 	if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
@@ -412,25 +419,10 @@ static int omap3_onenand_write_bufferram(struct mtd_info *mtd, int area,
 		return -1;
 	}
 
-	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
-				     count >> 2, 1, 0, 0, 0);
-	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				dma_src, 0, 0);
-	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				 dma_dst, 0, 0);
-
-	reinit_completion(&c->dma_done);
-	omap_start_dma(c->dma_channel);
-
-	timeout = jiffies + msecs_to_jiffies(20);
-	done = &c->dma_done.done;
-	while (time_before(jiffies, timeout))
-		if (*done)
-			break;
-
+	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
 	dma_unmap_single(&c->pdev->dev, dma_src, count, DMA_TO_DEVICE);
 
-	if (!*done) {
+	if (ret) {
 		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
 		goto out_copy;
 	}
@@ -469,7 +461,7 @@ static int omap2_onenand_read_bufferram(struct mtd_info *mtd, int area,
 	struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
 	struct onenand_chip *this = mtd->priv;
 	dma_addr_t dma_src, dma_dst;
-	int bram_offset;
+	int bram_offset, ret;
 
 	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
 	/* DMA is not used.  Revisit PM requirements before enabling it. */
@@ -491,20 +483,13 @@ static int omap2_onenand_read_bufferram(struct mtd_info *mtd, int area,
 		return -1;
 	}
 
-	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
-				     count / 4, 1, 0, 0, 0);
-	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				dma_src, 0, 0);
-	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				 dma_dst, 0, 0);
-
-	reinit_completion(&c->dma_done);
-	omap_start_dma(c->dma_channel);
-	wait_for_completion(&c->dma_done);
-
+	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
 	dma_unmap_single(&c->pdev->dev, dma_dst, count, DMA_FROM_DEVICE);
 
-	return 0;
+	if (ret)
+		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
+
+	return ret;
 }
 
 static int omap2_onenand_write_bufferram(struct mtd_info *mtd, int area,
@@ -514,7 +499,7 @@ static int omap2_onenand_write_bufferram(struct mtd_info *mtd, int area,
 	struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
 	struct onenand_chip *this = mtd->priv;
 	dma_addr_t dma_src, dma_dst;
-	int bram_offset;
+	int bram_offset, ret;
 
 	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
 	/* DMA is not used.  Revisit PM requirements before enabling it. */
@@ -536,20 +521,13 @@ static int omap2_onenand_write_bufferram(struct mtd_info *mtd, int area,
 		return -1;
 	}
 
-	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S16,
-				     count / 2, 1, 0, 0, 0);
-	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				dma_src, 0, 0);
-	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
-				 dma_dst, 0, 0);
-
-	reinit_completion(&c->dma_done);
-	omap_start_dma(c->dma_channel);
-	wait_for_completion(&c->dma_done);
-
+	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
 	dma_unmap_single(&c->pdev->dev, dma_src, count, DMA_TO_DEVICE);
 
-	return 0;
+	if (ret)
+		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
+
+	return ret;
 }
 
 #else
-- 
2.6.4

Re: [PATCH] mtd: onenand: omap2: Simplify the DMA setup for various paths

[Brian Norris]

On Mon, Dec 14, 2015 at 11:49:32AM +0200, Peter Ujfalusi wrote:
> We have 4 functions containing almost identical DMA setup code. Create one
> function which can set up the DMA for both read and write and use this in
> place for the setup code in the driver.
> The new function will use wait_for_completion_timeout() and it will figure
> out the best data_type to be used for the transfer instead of hardwiring
> 32 or 16 bit data.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Does anyone use this driver? I've seen practically zero activity on the
entire OneNAND codebase in the last few years, and I presumed it was
essentially dead.

If it's not dead, I'd like to know some contingency of people who are
willing to actually maintain (or at least review) this stuff.

Kyungmin, are you still out there? Or Tony, do you know of any users for
this?

Peter, are you actually using this, or are you just refactoring for the
fun of it?

Brian

> ---
>  drivers/mtd/onenand/omap2.c | 106 ++++++++++++++++++--------------------------
>  1 file changed, 42 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
> index 0aacf125938b..58576c9babb0 100644
> --- a/drivers/mtd/onenand/omap2.c
> +++ b/drivers/mtd/onenand/omap2.c
> @@ -291,6 +291,30 @@ static inline int omap2_onenand_bufferram_offset(struct mtd_info *mtd, int area)
>  	return 0;
>  }
>  
> +static inline int omap2_onenand_dma_transfer(struct omap2_onenand *c,
> +					     dma_addr_t src, dma_addr_t dst,
> +					     size_t count)
> +{
> +	int data_type = __ffs((src | dst | count));
> +
> +	if (data_type > OMAP_DMA_DATA_TYPE_S32)
> +		data_type = OMAP_DMA_DATA_TYPE_S32;
> +
> +	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
> +				     count / BIT(data_type), 1, 0, 0, 0);
> +	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> +				src, 0, 0);
> +	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> +				 dst, 0, 0);
> +
> +	reinit_completion(&c->dma_done);
> +	omap_start_dma(c->dma_channel);
> +	if (wait_for_completion_timeout(&c->dma_done, msecs_to_jiffies(20)))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
>  #if defined(CONFIG_ARCH_OMAP3) || defined(MULTI_OMAP2)
>  
>  static int omap3_onenand_read_bufferram(struct mtd_info *mtd, int area,
> @@ -301,10 +325,9 @@ static int omap3_onenand_read_bufferram(struct mtd_info *mtd, int area,
>  	struct onenand_chip *this = mtd->priv;
>  	dma_addr_t dma_src, dma_dst;
>  	int bram_offset;
> -	unsigned long timeout;
>  	void *buf = (void *)buffer;
>  	size_t xtra;
> -	volatile unsigned *done;
> +	int ret;
>  
>  	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
>  	if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
> @@ -341,25 +364,10 @@ static int omap3_onenand_read_bufferram(struct mtd_info *mtd, int area,
>  		goto out_copy;
>  	}
>  
> -	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
> -				     count >> 2, 1, 0, 0, 0);
> -	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> -				dma_src, 0, 0);
> -	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> -				 dma_dst, 0, 0);
> -
> -	reinit_completion(&c->dma_done);
> -	omap_start_dma(c->dma_channel);
> -
> -	timeout = jiffies + msecs_to_jiffies(20);
> -	done = &c->dma_done.done;
> -	while (time_before(jiffies, timeout))
> -		if (*done)
> -			break;
> -
> +	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
>  	dma_unmap_single(&c->pdev->dev, dma_dst, count, DMA_FROM_DEVICE);
>  
> -	if (!*done) {
> +	if (ret) {
>  		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
>  		goto out_copy;
>  	}
> @@ -379,9 +387,8 @@ static int omap3_onenand_write_bufferram(struct mtd_info *mtd, int area,
>  	struct onenand_chip *this = mtd->priv;
>  	dma_addr_t dma_src, dma_dst;
>  	int bram_offset;
> -	unsigned long timeout;
>  	void *buf = (void *)buffer;
> -	volatile unsigned *done;
> +	int ret;
>  
>  	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
>  	if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
> @@ -412,25 +419,10 @@ static int omap3_onenand_write_bufferram(struct mtd_info *mtd, int area,
>  		return -1;
>  	}
>  
> -	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
> -				     count >> 2, 1, 0, 0, 0);
> -	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> -				dma_src, 0, 0);
> -	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> -				 dma_dst, 0, 0);
> -
> -	reinit_completion(&c->dma_done);
> -	omap_start_dma(c->dma_channel);
> -
> -	timeout = jiffies + msecs_to_jiffies(20);
> -	done = &c->dma_done.done;
> -	while (time_before(jiffies, timeout))
> -		if (*done)
> -			break;
> -
> +	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
>  	dma_unmap_single(&c->pdev->dev, dma_src, count, DMA_TO_DEVICE);
>  
> -	if (!*done) {
> +	if (ret) {
>  		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
>  		goto out_copy;
>  	}
> @@ -469,7 +461,7 @@ static int omap2_onenand_read_bufferram(struct mtd_info *mtd, int area,
>  	struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
>  	struct onenand_chip *this = mtd->priv;
>  	dma_addr_t dma_src, dma_dst;
> -	int bram_offset;
> +	int bram_offset, ret;
>  
>  	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
>  	/* DMA is not used.  Revisit PM requirements before enabling it. */
> @@ -491,20 +483,13 @@ static int omap2_onenand_read_bufferram(struct mtd_info *mtd, int area,
>  		return -1;
>  	}
>  
> -	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
> -				     count / 4, 1, 0, 0, 0);
> -	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> -				dma_src, 0, 0);
> -	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> -				 dma_dst, 0, 0);
> -
> -	reinit_completion(&c->dma_done);
> -	omap_start_dma(c->dma_channel);
> -	wait_for_completion(&c->dma_done);
> -
> +	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
>  	dma_unmap_single(&c->pdev->dev, dma_dst, count, DMA_FROM_DEVICE);
>  
> -	return 0;
> +	if (ret)
> +		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
> +
> +	return ret;
>  }
>  
>  static int omap2_onenand_write_bufferram(struct mtd_info *mtd, int area,
> @@ -514,7 +499,7 @@ static int omap2_onenand_write_bufferram(struct mtd_info *mtd, int area,
>  	struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
>  	struct onenand_chip *this = mtd->priv;
>  	dma_addr_t dma_src, dma_dst;
> -	int bram_offset;
> +	int bram_offset, ret;
>  
>  	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
>  	/* DMA is not used.  Revisit PM requirements before enabling it. */
> @@ -536,20 +521,13 @@ static int omap2_onenand_write_bufferram(struct mtd_info *mtd, int area,
>  		return -1;
>  	}
>  
> -	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S16,
> -				     count / 2, 1, 0, 0, 0);
> -	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> -				dma_src, 0, 0);
> -	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> -				 dma_dst, 0, 0);
> -
> -	reinit_completion(&c->dma_done);
> -	omap_start_dma(c->dma_channel);
> -	wait_for_completion(&c->dma_done);
> -
> +	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
>  	dma_unmap_single(&c->pdev->dev, dma_src, count, DMA_TO_DEVICE);
>  
> -	return 0;
> +	if (ret)
> +		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
> +
> +	return ret;
>  }
>  
>  #else
> -- 
> 2.6.4
> 

Re: [PATCH] mtd: onenand: omap2: Simplify the DMA setup for various paths

[Tony Lindgren]

* Brian Norris <computersforpeace@gmail.com> [151218 10:11]:
> On Mon, Dec 14, 2015 at 11:49:32AM +0200, Peter Ujfalusi wrote:
> > We have 4 functions containing almost identical DMA setup code. Create one
> > function which can set up the DMA for both read and write and use this in
> > place for the setup code in the driver.
> > The new function will use wait_for_completion_timeout() and it will figure
> > out the best data_type to be used for the transfer instead of hardwiring
> > 32 or 16 bit data.
> > 
> > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> Does anyone use this driver? I've seen practically zero activity on the
> entire OneNAND codebase in the last few years, and I presumed it was
> essentially dead.
> 
> If it's not dead, I'd like to know some contingency of people who are
> willing to actually maintain (or at least review) this stuff.
> 
> Kyungmin, are you still out there? Or Tony, do you know of any users for
> this?
> 
> Peter, are you actually using this, or are you just refactoring for the
> fun of it?

It's used for n8x0 and n900, but mostly in read-only mode. I suggest we
remove the DMA support for it completely because of the following:

1. The DMA support for this driver is not done correctly. The pin used
   as GPIO should be used as external DMA request line.

2. AFAIK the DMA for this driver is mostly disabld, probably largely
   due to #1 above

3. If we remove DMA support, we can then easily switch to use the
   generic onenand driver.

I'd like to hear Aaro's comments too before doing this tough.

Regards,

Tony



> >  drivers/mtd/onenand/omap2.c | 106 ++++++++++++++++++--------------------------
> >  1 file changed, 42 insertions(+), 64 deletions(-)
> > 
> > diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
> > index 0aacf125938b..58576c9babb0 100644
> > --- a/drivers/mtd/onenand/omap2.c
> > +++ b/drivers/mtd/onenand/omap2.c
> > @@ -291,6 +291,30 @@ static inline int omap2_onenand_bufferram_offset(struct mtd_info *mtd, int area)
> >  	return 0;
> >  }
> >  
> > +static inline int omap2_onenand_dma_transfer(struct omap2_onenand *c,
> > +					     dma_addr_t src, dma_addr_t dst,
> > +					     size_t count)
> > +{
> > +	int data_type = __ffs((src | dst | count));
> > +
> > +	if (data_type > OMAP_DMA_DATA_TYPE_S32)
> > +		data_type = OMAP_DMA_DATA_TYPE_S32;
> > +
> > +	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
> > +				     count / BIT(data_type), 1, 0, 0, 0);
> > +	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> > +				src, 0, 0);
> > +	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> > +				 dst, 0, 0);
> > +
> > +	reinit_completion(&c->dma_done);
> > +	omap_start_dma(c->dma_channel);
> > +	if (wait_for_completion_timeout(&c->dma_done, msecs_to_jiffies(20)))
> > +		return -ETIMEDOUT;
> > +
> > +	return 0;
> > +}
> > +
> >  #if defined(CONFIG_ARCH_OMAP3) || defined(MULTI_OMAP2)
> >  
> >  static int omap3_onenand_read_bufferram(struct mtd_info *mtd, int area,
> > @@ -301,10 +325,9 @@ static int omap3_onenand_read_bufferram(struct mtd_info *mtd, int area,
> >  	struct onenand_chip *this = mtd->priv;
> >  	dma_addr_t dma_src, dma_dst;
> >  	int bram_offset;
> > -	unsigned long timeout;
> >  	void *buf = (void *)buffer;
> >  	size_t xtra;
> > -	volatile unsigned *done;
> > +	int ret;
> >  
> >  	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
> >  	if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
> > @@ -341,25 +364,10 @@ static int omap3_onenand_read_bufferram(struct mtd_info *mtd, int area,
> >  		goto out_copy;
> >  	}
> >  
> > -	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
> > -				     count >> 2, 1, 0, 0, 0);
> > -	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> > -				dma_src, 0, 0);
> > -	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> > -				 dma_dst, 0, 0);
> > -
> > -	reinit_completion(&c->dma_done);
> > -	omap_start_dma(c->dma_channel);
> > -
> > -	timeout = jiffies + msecs_to_jiffies(20);
> > -	done = &c->dma_done.done;
> > -	while (time_before(jiffies, timeout))
> > -		if (*done)
> > -			break;
> > -
> > +	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
> >  	dma_unmap_single(&c->pdev->dev, dma_dst, count, DMA_FROM_DEVICE);
> >  
> > -	if (!*done) {
> > +	if (ret) {
> >  		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
> >  		goto out_copy;
> >  	}
> > @@ -379,9 +387,8 @@ static int omap3_onenand_write_bufferram(struct mtd_info *mtd, int area,
> >  	struct onenand_chip *this = mtd->priv;
> >  	dma_addr_t dma_src, dma_dst;
> >  	int bram_offset;
> > -	unsigned long timeout;
> >  	void *buf = (void *)buffer;
> > -	volatile unsigned *done;
> > +	int ret;
> >  
> >  	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
> >  	if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
> > @@ -412,25 +419,10 @@ static int omap3_onenand_write_bufferram(struct mtd_info *mtd, int area,
> >  		return -1;
> >  	}
> >  
> > -	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
> > -				     count >> 2, 1, 0, 0, 0);
> > -	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> > -				dma_src, 0, 0);
> > -	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> > -				 dma_dst, 0, 0);
> > -
> > -	reinit_completion(&c->dma_done);
> > -	omap_start_dma(c->dma_channel);
> > -
> > -	timeout = jiffies + msecs_to_jiffies(20);
> > -	done = &c->dma_done.done;
> > -	while (time_before(jiffies, timeout))
> > -		if (*done)
> > -			break;
> > -
> > +	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
> >  	dma_unmap_single(&c->pdev->dev, dma_src, count, DMA_TO_DEVICE);
> >  
> > -	if (!*done) {
> > +	if (ret) {
> >  		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
> >  		goto out_copy;
> >  	}
> > @@ -469,7 +461,7 @@ static int omap2_onenand_read_bufferram(struct mtd_info *mtd, int area,
> >  	struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
> >  	struct onenand_chip *this = mtd->priv;
> >  	dma_addr_t dma_src, dma_dst;
> > -	int bram_offset;
> > +	int bram_offset, ret;
> >  
> >  	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
> >  	/* DMA is not used.  Revisit PM requirements before enabling it. */
> > @@ -491,20 +483,13 @@ static int omap2_onenand_read_bufferram(struct mtd_info *mtd, int area,
> >  		return -1;
> >  	}
> >  
> > -	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S32,
> > -				     count / 4, 1, 0, 0, 0);
> > -	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> > -				dma_src, 0, 0);
> > -	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> > -				 dma_dst, 0, 0);
> > -
> > -	reinit_completion(&c->dma_done);
> > -	omap_start_dma(c->dma_channel);
> > -	wait_for_completion(&c->dma_done);
> > -
> > +	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
> >  	dma_unmap_single(&c->pdev->dev, dma_dst, count, DMA_FROM_DEVICE);
> >  
> > -	return 0;
> > +	if (ret)
> > +		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
> > +
> > +	return ret;
> >  }
> >  
> >  static int omap2_onenand_write_bufferram(struct mtd_info *mtd, int area,
> > @@ -514,7 +499,7 @@ static int omap2_onenand_write_bufferram(struct mtd_info *mtd, int area,
> >  	struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
> >  	struct onenand_chip *this = mtd->priv;
> >  	dma_addr_t dma_src, dma_dst;
> > -	int bram_offset;
> > +	int bram_offset, ret;
> >  
> >  	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
> >  	/* DMA is not used.  Revisit PM requirements before enabling it. */
> > @@ -536,20 +521,13 @@ static int omap2_onenand_write_bufferram(struct mtd_info *mtd, int area,
> >  		return -1;
> >  	}
> >  
> > -	omap_set_dma_transfer_params(c->dma_channel, OMAP_DMA_DATA_TYPE_S16,
> > -				     count / 2, 1, 0, 0, 0);
> > -	omap_set_dma_src_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> > -				dma_src, 0, 0);
> > -	omap_set_dma_dest_params(c->dma_channel, 0, OMAP_DMA_AMODE_POST_INC,
> > -				 dma_dst, 0, 0);
> > -
> > -	reinit_completion(&c->dma_done);
> > -	omap_start_dma(c->dma_channel);
> > -	wait_for_completion(&c->dma_done);
> > -
> > +	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
> >  	dma_unmap_single(&c->pdev->dev, dma_src, count, DMA_TO_DEVICE);
> >  
> > -	return 0;
> > +	if (ret)
> > +		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
> > +
> > +	return ret;
> >  }
> >  
> >  #else
> > -- 
> > 2.6.4
> > 

Re: [PATCH] mtd: onenand: omap2: Simplify the DMA setup for various paths

[Peter Ujfalusi]

On 12/18/2015 08:11 PM, Brian Norris wrote:
> On Mon, Dec 14, 2015 at 11:49:32AM +0200, Peter Ujfalusi wrote:
>> We have 4 functions containing almost identical DMA setup code. Create one
>> function which can set up the DMA for both read and write and use this in
>> place for the setup code in the driver.
>> The new function will use wait_for_completion_timeout() and it will figure
>> out the best data_type to be used for the transfer instead of hardwiring
>> 32 or 16 bit data.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> Does anyone use this driver? I've seen practically zero activity on the
> entire OneNAND codebase in the last few years, and I presumed it was
> essentially dead.
> 
> If it's not dead, I'd like to know some contingency of people who are
> willing to actually maintain (or at least review) this stuff.
> 
> Kyungmin, are you still out there? Or Tony, do you know of any users for
> this?
> 
> Peter, are you actually using this, or are you just refactoring for the
> fun of it?

Not really for fun, but I want to get rid of all legacy/direct sDMA use so at
the end we will have omap_start_dma() visible in two files:
arch/arm/plat-omap/dma.c
drivers/dma/omap-dma.c

from there it will be possible to get rid of the plat-omap code. This onenand
driver was the first in the 'git grep omap_start_dma' result ;)

-- 
Péter

Re: [PATCH] mtd: onenand: omap2: Simplify the DMA setup for various paths

[Peter Ujfalusi]

On 12/18/2015 08:11 PM, Brian Norris wrote:
> On Mon, Dec 14, 2015 at 11:49:32AM +0200, Peter Ujfalusi wrote:
>> We have 4 functions containing almost identical DMA setup code. Create one
>> function which can set up the DMA for both read and write and use this in
>> place for the setup code in the driver.
>> The new function will use wait_for_completion_timeout() and it will figure
>> out the best data_type to be used for the transfer instead of hardwiring
>> 32 or 16 bit data.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> Does anyone use this driver? I've seen practically zero activity on the
> entire OneNAND codebase in the last few years, and I presumed it was
> essentially dead.
> 
> If it's not dead, I'd like to know some contingency of people who are
> willing to actually maintain (or at least review) this stuff.
> 
> Kyungmin, are you still out there? Or Tony, do you know of any users for
> this?
> 
> Peter, are you actually using this, or are you just refactoring for the
> fun of it?

Not really for fun, but I want to get rid of all legacy/direct sDMA use so at
the end we will have omap_start_dma() visible in two files:
arch/arm/plat-omap/dma.c
drivers/dma/omap-dma.c

from there it will be possible to get rid of the plat-omap code. This onenand
driver was the first in the 'git grep omap_start_dma' result ;)

-- 
Péter

Re: [PATCH] mtd: onenand: omap2: Simplify the DMA setup for various paths

[Brian Norris]

On Fri, Dec 18, 2015 at 09:48:09PM +0200, Peter Ujfalusi wrote:
> On 12/18/2015 08:11 PM, Brian Norris wrote:
> > Peter, are you actually using this, or are you just refactoring for the
> > fun of it?
> 
> Not really for fun, but I want to get rid of all legacy/direct sDMA use so at
> the end we will have omap_start_dma() visible in two files:
> arch/arm/plat-omap/dma.c
> drivers/dma/omap-dma.c
> 
> from there it will be possible to get rid of the plat-omap code. This onenand
> driver was the first in the 'git grep omap_start_dma' result ;)

OK, well killing broken DMA support altogether might make more sense,
and deleting an entire driver would be even better. Of course, having a
real tester would be nice too. I'll wait on Tony/Aaro.

Brian

Re: [PATCH] mtd: onenand: omap2: Simplify the DMA setup for various paths

[Aaro Koskinen]

Hi,

On Fri, Dec 18, 2015 at 10:39:48AM -0800, Tony Lindgren wrote:
> * Brian Norris <computersforpeace@gmail.com> [151218 10:11]:
> > On Mon, Dec 14, 2015 at 11:49:32AM +0200, Peter Ujfalusi wrote:
> > > We have 4 functions containing almost identical DMA setup code. Create one
> > > function which can set up the DMA for both read and write and use this in
> > > place for the setup code in the driver.
> > > The new function will use wait_for_completion_timeout() and it will figure
> > > out the best data_type to be used for the transfer instead of hardwiring
> > > 32 or 16 bit data.
> > > 
> > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > 
> > Does anyone use this driver? I've seen practically zero activity on the
> > entire OneNAND codebase in the last few years, and I presumed it was
> > essentially dead.
> > 
> > If it's not dead, I'd like to know some contingency of people who are
> > willing to actually maintain (or at least review) this stuff.
> > 
> > Kyungmin, are you still out there? Or Tony, do you know of any users for
> > this?
> > 
> > Peter, are you actually using this, or are you just refactoring for the
> > fun of it?
> 
> It's used for n8x0 and n900, but mostly in read-only mode. I suggest we

It's also used on n9/n950 phones. Also write functionality is needed
for mtdoops and kernel flashing.

> remove the DMA support for it completely because of the following:
> 
> 1. The DMA support for this driver is not done correctly. The pin used
>    as GPIO should be used as external DMA request line.
> 
> 2. AFAIK the DMA for this driver is mostly disabld, probably largely
>    due to #1 above
> 
> 3. If we remove DMA support, we can then easily switch to use the
>    generic onenand driver.
> 
> I'd like to hear Aaro's comments too before doing this tough.

Probably DMA support is not that critical. Looks like with DT boot it's
not even possible to enable it at the moment.

A.

Re: [PATCH] mtd: onenand: omap2: Simplify the DMA setup for various paths

[Peter Ujfalusi]

On 12/19/2015 03:30 PM, Aaro Koskinen wrote:
> Probably DMA support is not that critical. Looks like with DT boot it's
> not even possible to enable it at the moment.

I think by adding 'dma-channel' property is there to enable the DMA mode via DT.

As for stripping out the DMA support: I'm fine with that if no one objects,
but in the header of the file we have:
 *  Author: Jarkko Lavinen <jarkko.lavinen@nokia.com> and Juha Yrjölä
 *  IRQ and DMA support written by Timo Teras

So I believe the DMA and IRQ code was a big stunt itself to receive it's own
line there...

-- 
Péter

Re: [PATCH] mtd: onenand: omap2: Simplify the DMA setup for various paths

[Peter Ujfalusi]

On 12/19/2015 03:30 PM, Aaro Koskinen wrote:
> Probably DMA support is not that critical. Looks like with DT boot it's
> not even possible to enable it at the moment.

I think by adding 'dma-channel' property is there to enable the DMA mode via DT.

As for stripping out the DMA support: I'm fine with that if no one objects,
but in the header of the file we have:
 *  Author: Jarkko Lavinen <jarkko.lavinen@nokia.com> and Juha Yrjölä
 *  IRQ and DMA support written by Timo Teras

So I believe the DMA and IRQ code was a big stunt itself to receive it's own
line there...

-- 
Péter