[PATCH V2 0/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[PATCH V2 0/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Alim Akhtar]

This patch modifies the amba-pl08x driver for s3c64xx.
The DMA controller of S3C64XX is a variant of PrimeCell pl080 DMAC.
S3C64xx contents extra register to hold the TransferSize.

Below diagram explains the s3c64xx LLI structure and offsets.
	-----------------------------------------------------------------
	|       Offset          |       Contents                        |
	-----------------------------------------------------------------
	| Next LLI Address      | Source Address for Next xfer          |
	----------------------------------------------------------------- 
	| Next LLI Address+0x04 | Destination Address for Next xfer     |
	-----------------------------------------------------------------
	| Next LLI Address+0x08 | Next LLI address for Next xfer        |
	-----------------------------------------------------------------
	| Next LLI Address+0x0c | DMACCxControl0 data for Next xfer     |
	-----------------------------------------------------------------
	| Next LLI Address+0x10 | DMACCxControl1 xfer size for Next xfer|
	----------------------------------------------------------------- 

Changes since v1:
	- Rebased with "samsung_dma" branch of 
	http://git.infradead.org/users/vkoul/slave-dma.git

	- Addressed review comments from linus.walleij 
	http://www.spinics.net/lists/arm-kernel/msg135411.html

Alim Akhtar (1):
  dmaengine/amba-pl08x: Add support for s3c64xx DMAC

 drivers/dma/amba-pl08x.c |  135 ++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 112 insertions(+), 23 deletions(-)

-- 
1.7.2.3

[PATCH V2 1/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Alim Akhtar]

Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
---
 drivers/dma/amba-pl08x.c |  135 ++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 112 insertions(+), 23 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index cd8df7f..501540f 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -66,8 +66,25 @@
  *    after the final transfer signalled by LBREQ or LSREQ.  The DMAC
  *    will then move to the next LLI entry.
  *
- * Global TODO:
- * - Break out common code from arch/arm/mach-s3c64xx and share
+ * Samsung S3C64xx SoCs uses a variant of PL080 DMAC. It contains an extra
+ * control register to hold the TransferSize. Below is the LLI structure
+ * and offsets of S3C64xx DMAC.
+ *	-----------------------------------------------------------------
+ *	|	Offset		|	Contents		        |
+ *	-----------------------------------------------------------------
+ *	| Next LLI Address	| Source Address for Next xfer	        |
+ *	-----------------------------------------------------------------
+ *	| Next LLI Address+0x04	| Destination Address for Next xfer     |
+ *	-----------------------------------------------------------------
+ *	| Next LLI Address+0x08	| Next LLI address for next xfer        |
+ *	-----------------------------------------------------------------
+ *	| Next LLI Address+0x0c	| DMACCxControl0 data for next xfer     |
+ *	-----------------------------------------------------------------
+ *	| Next LLI Address+0x10	| DMACCxControl1 xfer size for next xfer|
+ *	-----------------------------------------------------------------
+ * Also S3C64XX has a config register at offset 0x14
+ * Have a look at arch/arm/include/asm/hardware/pl080.h for complete register
+ * details.
  */
 #include <linux/amba/bus.h>
 #include <linux/amba/pl08x.h>
@@ -97,6 +114,8 @@ static struct amba_driver pl08x_amba_driver;
 struct vendor_data {
 	u8 channels;
 	bool dualmaster;
+	/* To identify samsung DMAC */
+	bool is_pl080_s3c;
 };
 
 /*
@@ -110,6 +129,11 @@ struct pl08x_lli {
 	u32 dst;
 	u32 lli;
 	u32 cctl;
+	/*
+	 * Samsung pl080 DMAC has one exrta control register
+	 * which is used to hold the transfer_size
+	 */
+	u32 cctl1;
 };
 
 /**
@@ -171,9 +195,20 @@ static inline struct pl08x_txd *to_pl08x_txd(struct dma_async_tx_descriptor *tx)
 /* Whether a certain channel is busy or not */
 static int pl08x_phy_channel_busy(struct pl08x_phy_chan *ch)
 {
+	struct pl08x_dma_chan *plchan = ch->serving;
+	struct pl08x_driver_data *pl08x;
 	unsigned int val;
 
-	val = readl(ch->base + PL080_CH_CONFIG);
+	if (plchan == NULL)
+		return false;
+
+	pl08x = plchan->host;
+
+	if (pl08x->vd->is_pl080_s3c)
+		val = readl(ch->base + PL080S_CH_CONFIG);
+	else
+		val = readl(ch->base + PL080_CH_CONFIG);
+
 	return val & PL080_CONFIG_ACTIVE;
 }
 
@@ -207,7 +242,12 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
 	writel(lli->dst, phychan->base + PL080_CH_DST_ADDR);
 	writel(lli->lli, phychan->base + PL080_CH_LLI);
 	writel(lli->cctl, phychan->base + PL080_CH_CONTROL);
-	writel(txd->ccfg, phychan->base + PL080_CH_CONFIG);
+
+	if (pl08x->vd->is_pl080_s3c) {
+		writel(txd->ccfg, phychan->base + PL080S_CH_CONFIG);
+		writel(lli->cctl1, phychan->base + PL080S_CH_CONTROL2);
+	} else
+		writel(txd->ccfg, phychan->base + PL080_CH_CONFIG);
 
 	/* Enable the DMA channel */
 	/* Do not access config register until channel shows as disabled */
@@ -215,11 +255,23 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
 		cpu_relax();
 
 	/* Do not access config register until channel shows as inactive */
-	val = readl(phychan->base + PL080_CH_CONFIG);
-	while ((val & PL080_CONFIG_ACTIVE) || (val & PL080_CONFIG_ENABLE))
+	if (pl08x->vd->is_pl080_s3c) {
+		val = readl(phychan->base + PL080S_CH_CONFIG);
+		while ((val & PL080_CONFIG_ACTIVE) ||
+			(val & PL080_CONFIG_ENABLE))
+			val = readl(phychan->base + PL080S_CH_CONFIG);
+
+		writel(val | PL080_CONFIG_ENABLE,
+			phychan->base + PL080S_CH_CONFIG);
+	} else {
 		val = readl(phychan->base + PL080_CH_CONFIG);
+			while ((val & PL080_CONFIG_ACTIVE) ||
+				(val & PL080_CONFIG_ENABLE))
+				val = readl(phychan->base + PL080_CH_CONFIG);
 
-	writel(val | PL080_CONFIG_ENABLE, phychan->base + PL080_CH_CONFIG);
+		writel(val | PL080_CONFIG_ENABLE,
+			phychan->base + PL080_CH_CONFIG);
+	}
 }
 
 /*
@@ -236,12 +288,19 @@ static void pl08x_pause_phy_chan(struct pl08x_phy_chan *ch)
 {
 	u32 val;
 	int timeout;
+	struct pl08x_dma_chan *plchan = ch->serving;
+	struct pl08x_driver_data *pl08x = plchan->host;
 
 	/* Set the HALT bit and wait for the FIFO to drain */
-	val = readl(ch->base + PL080_CH_CONFIG);
-	val |= PL080_CONFIG_HALT;
-	writel(val, ch->base + PL080_CH_CONFIG);
-
+	if (pl08x->vd->is_pl080_s3c) {
+		val = readl(ch->base + PL080S_CH_CONFIG);
+		val |= PL080_CONFIG_HALT;
+		writel(val, ch->base + PL080S_CH_CONFIG);
+	} else {
+		val = readl(ch->base + PL080_CH_CONFIG);
+		val |= PL080_CONFIG_HALT;
+		writel(val, ch->base + PL080_CH_CONFIG);
+	}
 	/* Wait for channel inactive */
 	for (timeout = 1000; timeout; timeout--) {
 		if (!pl08x_phy_channel_busy(ch))
@@ -255,11 +314,19 @@ static void pl08x_pause_phy_chan(struct pl08x_phy_chan *ch)
 static void pl08x_resume_phy_chan(struct pl08x_phy_chan *ch)
 {
 	u32 val;
+	struct pl08x_dma_chan *plchan = ch->serving;
+	struct pl08x_driver_data *pl08x = plchan->host;
 
 	/* Clear the HALT bit */
-	val = readl(ch->base + PL080_CH_CONFIG);
-	val &= ~PL080_CONFIG_HALT;
-	writel(val, ch->base + PL080_CH_CONFIG);
+	if (pl08x->vd->is_pl080_s3c) {
+		val = readl(ch->base + PL080S_CH_CONFIG);
+		val &= ~PL080_CONFIG_HALT;
+		writel(val, ch->base + PL080S_CH_CONFIG);
+	} else {
+		val = readl(ch->base + PL080_CH_CONFIG);
+		val &= ~PL080_CONFIG_HALT;
+		writel(val, ch->base + PL080_CH_CONFIG);
+	}
 }
 
 /*
@@ -271,12 +338,17 @@ static void pl08x_resume_phy_chan(struct pl08x_phy_chan *ch)
 static void pl08x_terminate_phy_chan(struct pl08x_driver_data *pl08x,
 	struct pl08x_phy_chan *ch)
 {
-	u32 val = readl(ch->base + PL080_CH_CONFIG);
-
-	val &= ~(PL080_CONFIG_ENABLE | PL080_CONFIG_ERR_IRQ_MASK |
-	         PL080_CONFIG_TC_IRQ_MASK);
-
-	writel(val, ch->base + PL080_CH_CONFIG);
+	if (pl08x->vd->is_pl080_s3c) {
+		u32 val = readl(ch->base + PL080S_CH_CONFIG);
+		val &= ~(PL080_CONFIG_ENABLE | PL080_CONFIG_ERR_IRQ_MASK |
+				PL080_CONFIG_TC_IRQ_MASK);
+		writel(val, ch->base + PL080S_CH_CONFIG);
+	} else {
+		u32 val = readl(ch->base + PL080_CH_CONFIG);
+		val &= ~(PL080_CONFIG_ENABLE | PL080_CONFIG_ERR_IRQ_MASK |
+				PL080_CONFIG_TC_IRQ_MASK);
+		writel(val, ch->base + PL080_CH_CONFIG);
+	}
 
 	writel(1 << ch->id, pl08x->base + PL080_ERR_CLEAR);
 	writel(1 << ch->id, pl08x->base + PL080_TC_CLEAR);
@@ -569,6 +641,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	u32 cctl, early_bytes = 0;
 	size_t max_bytes_per_lli, total_bytes = 0;
 	struct pl08x_lli *llis_va;
+	size_t lli_len = 0, target_len, tsize, odd_bytes;
 
 	txd->llis_va = dma_pool_alloc(pl08x->pool, GFP_NOWAIT, &txd->llis_bus);
 	if (!txd->llis_va) {
@@ -700,7 +773,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		 * width left
 		 */
 		while (bd.remainder > (mbus->buswidth - 1)) {
-			size_t lli_len, tsize, width;
+			size_t width;
 
 			/*
 			 * If enough left try to send max possible,
@@ -759,6 +832,9 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	llis_va[num_llis - 1].lli = 0;
 	/* The final LLI element shall also fire an interrupt. */
 	llis_va[num_llis - 1].cctl |= PL080_CONTROL_TC_IRQ_EN;
+	/* Keep the TransferSize seperate to fill samsung specific register */
+	if (pl08x->vd->is_pl080_s3c)
+		llis_va[num_llis - 1].cctl1 |= lli_len;
 
 #ifdef VERBOSE_DEBUG
 	{
@@ -771,8 +847,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 			dev_vdbg(&pl08x->adev->dev,
 				 "%3d @%p: 0x%08x 0x%08x 0x%08x 0x%08x\n",
 				 i, &llis_va[i], llis_va[i].src,
-				 llis_va[i].dst, llis_va[i].lli, llis_va[i].cctl
-				);
+				 llis_va[i].dst, llis_va[i].lli,
+				 llis_va[i].cctl);
 		}
 	}
 #endif
@@ -1979,6 +2055,12 @@ static struct vendor_data vendor_pl081 = {
 	.dualmaster = false,
 };
 
+static struct vendor_data vendor_pl080_s3c = {
+	.channels = 8,
+	.dualmaster = true,
+	.is_pl080_s3c = true,
+};
+
 static struct amba_id pl08x_ids[] = {
 	/* PL080 */
 	{
@@ -1998,6 +2080,13 @@ static struct amba_id pl08x_ids[] = {
 		.mask	= 0x00ffffff,
 		.data	= &vendor_pl080,
 	},
+	/* Samsung DMAC is PL080 variant*/
+	{
+		.id	= 0x00041082,
+		.mask	= 0x000fffff,
+		.data	= &vendor_pl080_s3c,
+
+	},
 	{ 0, 0 },
 };
 
-- 
1.7.2.3

Re: [PATCH V2 1/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Viresh Kumar]

On 9/28/2011 11:20 AM, Alim Akhtar wrote:
> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---
>  drivers/dma/amba-pl08x.c |  135 ++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 112 insertions(+), 23 deletions(-)
> 

It would be good if you can add pick some part from cover-letter and put it in
commit log too.

> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index cd8df7f..501540f 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -110,6 +129,11 @@ struct pl08x_lli {
>  	u32 dst;
>  	u32 lli;
>  	u32 cctl;
> +	/*
> +	 * Samsung pl080 DMAC has one exrta control register
> +	 * which is used to hold the transfer_size
> +	 */
> +	u32 cctl1;

Will you write transfer_size in cctl also? What is the purpose of cctl1?

> @@ -215,11 +255,23 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
>  		cpu_relax();
>  
>  	/* Do not access config register until channel shows as inactive */
> -	val = readl(phychan->base + PL080_CH_CONFIG);
> -	while ((val & PL080_CONFIG_ACTIVE) || (val & PL080_CONFIG_ENABLE))
> +	if (pl08x->vd->is_pl080_s3c) {
> +		val = readl(phychan->base + PL080S_CH_CONFIG);
> +		while ((val & PL080_CONFIG_ACTIVE) ||
> +			(val & PL080_CONFIG_ENABLE))
> +			val = readl(phychan->base + PL080S_CH_CONFIG);
> +
> +		writel(val | PL080_CONFIG_ENABLE,
> +			phychan->base + PL080S_CH_CONFIG);
> +	} else {
>  		val = readl(phychan->base + PL080_CH_CONFIG);
> +			while ((val & PL080_CONFIG_ACTIVE) ||
> +				(val & PL080_CONFIG_ENABLE))
> +				val = readl(phychan->base + PL080_CH_CONFIG);
>  
> -	writel(val | PL080_CONFIG_ENABLE, phychan->base + PL080_CH_CONFIG);
> +		writel(val | PL080_CONFIG_ENABLE,
> +			phychan->base + PL080_CH_CONFIG);
> +	}

You have similar stuff in most the the changes in this patch, because offset of
config register changes for s3c, you placed in if,else block.

If you check these changes again, there is a lot of code duplication in this if,else
blocks. The only different thing in if,else is the offset of CH_CONFIG register.

It would be much better if you can do following:

u32 ch_cfg_off;

	if (pl08x->vd->is_pl080_s3c)
		ch_cfg_off = PL080S_CH_CONFIG;
	else
		ch_cfg_off = PL080_CH_CONFIG;

Now, this offset can be used in existing code, without much code duplication.

> @@ -569,6 +641,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  	u32 cctl, early_bytes = 0;
>  	size_t max_bytes_per_lli, total_bytes = 0;
>  	struct pl08x_lli *llis_va;
> +	size_t lli_len = 0, target_len, tsize, odd_bytes;
>  
>  	txd->llis_va = dma_pool_alloc(pl08x->pool, GFP_NOWAIT, &txd->llis_bus);
>  	if (!txd->llis_va) {
> @@ -700,7 +773,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  		 * width left
>  		 */
>  		while (bd.remainder > (mbus->buswidth - 1)) {
> -			size_t lli_len, tsize, width;
> +			size_t width;
>  

why do we need above two changes. odd_bytes and target_len are still unused.

>  			/*
>  			 * If enough left try to send max possible,
> @@ -759,6 +832,9 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  	llis_va[num_llis - 1].lli = 0;
>  	/* The final LLI element shall also fire an interrupt. */
>  	llis_va[num_llis - 1].cctl |= PL080_CONTROL_TC_IRQ_EN;
> +	/* Keep the TransferSize seperate to fill samsung specific register */
> +	if (pl08x->vd->is_pl080_s3c)
> +		llis_va[num_llis - 1].cctl1 |= lli_len;

I couldn't get this completely. Why do you keep only length of
last lli in cctl1. what about all other llis. Also why |=
and not directly cctl1 = lli_len

--
viresh

Re: [PATCH V2 1/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Linus Walleij]

Sorry if I missed a few nitpicks last time, anyway it's looking much better now:

On Wed, Sep 28, 2011 at 7:50 AM, Alim Akhtar <alim.akhtar@samsung.com> wrote:
> +       /*
> +        * Samsung pl080 DMAC has one exrta control register

s/exrta/exstra

> +       if (pl08x->vd->is_pl080_s3c) {
> +               writel(txd->ccfg, phychan->base + PL080S_CH_CONFIG);
> +               writel(lli->cctl1, phychan->base + PL080S_CH_CONTROL2);
> +       } else
> +               writel(txd->ccfg, phychan->base + PL080_CH_CONFIG);

What do you think about adding a field to the struct pl08x
like

u32 config_reg

that we set to the proper config register (PL080S_CH_CONFIG or
PL080_CH_CONFIG in probe(), so the above
becomes the simpler variant:

writel(txd->ccfg, phychan->base + pl08x->config_reg);
if (pl08x->vd->is_pl080_s3c)
    writel(lli->cctl1, phychan->base + PL080S_CH_CONTROL2);

> +       if (pl08x->vd->is_pl080_s3c) {
> +               val = readl(phychan->base + PL080S_CH_CONFIG);
> +               while ((val & PL080_CONFIG_ACTIVE) ||
> +                       (val & PL080_CONFIG_ENABLE))
> +                       val = readl(phychan->base + PL080S_CH_CONFIG);
> +
> +               writel(val | PL080_CONFIG_ENABLE,
> +                       phychan->base + PL080S_CH_CONFIG);
> +       } else {
>                val = readl(phychan->base + PL080_CH_CONFIG);
> +                       while ((val & PL080_CONFIG_ACTIVE) ||
> +                               (val & PL080_CONFIG_ENABLE))
> +                               val = readl(phychan->base + PL080_CH_CONFIG);
>
> -       writel(val | PL080_CONFIG_ENABLE, phychan->base + PL080_CH_CONFIG);
> +               writel(val | PL080_CONFIG_ENABLE,
> +                       phychan->base + PL080_CH_CONFIG);
> +       }

This would also become much simpler with that approach I think...

>        /* Set the HALT bit and wait for the FIFO to drain */
> -       val = readl(ch->base + PL080_CH_CONFIG);
> -       val |= PL080_CONFIG_HALT;
> -       writel(val, ch->base + PL080_CH_CONFIG);
> -
> +       if (pl08x->vd->is_pl080_s3c) {
> +               val = readl(ch->base + PL080S_CH_CONFIG);
> +               val |= PL080_CONFIG_HALT;
> +               writel(val, ch->base + PL080S_CH_CONFIG);
> +       } else {
> +               val = readl(ch->base + PL080_CH_CONFIG);
> +               val |= PL080_CONFIG_HALT;
> +               writel(val, ch->base + PL080_CH_CONFIG);
> +       }

This would become simply:

val = readl(ch->base + pl08x->config_reg);
val |= PL080_CONFIG_HALT;
writel(val, ch->base + pl08x->config_reg);


>        /* Clear the HALT bit */
> -       val = readl(ch->base + PL080_CH_CONFIG);
> -       val &= ~PL080_CONFIG_HALT;
> -       writel(val, ch->base + PL080_CH_CONFIG);
> +       if (pl08x->vd->is_pl080_s3c) {
> +               val = readl(ch->base + PL080S_CH_CONFIG);
> +               val &= ~PL080_CONFIG_HALT;
> +               writel(val, ch->base + PL080S_CH_CONFIG);
> +       } else {
> +               val = readl(ch->base + PL080_CH_CONFIG);
> +               val &= ~PL080_CONFIG_HALT;
> +               writel(val, ch->base + PL080_CH_CONFIG);
> +       }

This would get rid of the if/else clauses

> +       if (pl08x->vd->is_pl080_s3c) {
> +               u32 val = readl(ch->base + PL080S_CH_CONFIG);
> +               val &= ~(PL080_CONFIG_ENABLE | PL080_CONFIG_ERR_IRQ_MASK |
> +                               PL080_CONFIG_TC_IRQ_MASK);
> +               writel(val, ch->base + PL080S_CH_CONFIG);
> +       } else {
> +               u32 val = readl(ch->base + PL080_CH_CONFIG);
> +               val &= ~(PL080_CONFIG_ENABLE | PL080_CONFIG_ERR_IRQ_MASK |
> +                               PL080_CONFIG_TC_IRQ_MASK);
> +               writel(val, ch->base + PL080_CH_CONFIG);
> +       }

As would this...

> +       /* Samsung DMAC is PL080 variant*/
> +       {
> +               .id     = 0x00041082,
> +               .mask   = 0x000fffff,
> +               .data   = &vendor_pl080_s3c,

Does the hardware realy have this primecell number or is it something that is
hardcoded from the board/device tree?

If it is hardcoded then no objections.

In the latter case, replace 0x41 (= 'A', ARM) with something like
0x55 'U' for Samsung or so. Or 0x51 'S'. Or whatever you like.

Then add that to include/linux/amba/bus.h as
AMBA_VENDOR_SAMSUNG = 0x55,
so we have this under some kind of control.

Yours,
Linus Walleij

Re: [PATCH V2 1/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Alim Akhtar]

Hi Viresh,

Thanks for reviewing the patch.

On Wed, Sep 28, 2011 at 1:15 PM, Viresh Kumar <viresh.kumar@st.com> wrote:
> On 9/28/2011 11:20 AM, Alim Akhtar wrote:
>> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
>> ---
>>  drivers/dma/amba-pl08x.c |  135 ++++++++++++++++++++++++++++++++++++++--------
>>  1 files changed, 112 insertions(+), 23 deletions(-)
>>
>
> It would be good if you can add pick some part from cover-letter and put it in
> commit log too.
>
Ok, I will write more comments in the commit log.

>> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
>> index cd8df7f..501540f 100644
>> --- a/drivers/dma/amba-pl08x.c
>> +++ b/drivers/dma/amba-pl08x.c
>> @@ -110,6 +129,11 @@ struct pl08x_lli {
>>       u32 dst;
>>       u32 lli;
>>       u32 cctl;
>> +     /*
>> +      * Samsung pl080 DMAC has one exrta control register
>> +      * which is used to hold the transfer_size
>> +      */
>> +     u32 cctl1;
>
> Will you write transfer_size in cctl also? What is the purpose of cctl1?
>
The main difference between Primecell PL080 and samsung variant is in
LLI control register bit [0:11] is reserved in case of samsung pl080
and one extra register is add to hold the transfer size at offset
0x10. The purpose of cctl1 is store the transfer_size.

>> @@ -215,11 +255,23 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
>>               cpu_relax();
>>
>>       /* Do not access config register until channel shows as inactive */
>> -     val = readl(phychan->base + PL080_CH_CONFIG);
>> -     while ((val & PL080_CONFIG_ACTIVE) || (val & PL080_CONFIG_ENABLE))
>> +     if (pl08x->vd->is_pl080_s3c) {
>> +             val = readl(phychan->base + PL080S_CH_CONFIG);
>> +             while ((val & PL080_CONFIG_ACTIVE) ||
>> +                     (val & PL080_CONFIG_ENABLE))
>> +                     val = readl(phychan->base + PL080S_CH_CONFIG);
>> +
>> +             writel(val | PL080_CONFIG_ENABLE,
>> +                     phychan->base + PL080S_CH_CONFIG);
>> +     } else {
>>               val = readl(phychan->base + PL080_CH_CONFIG);
>> +                     while ((val & PL080_CONFIG_ACTIVE) ||
>> +                             (val & PL080_CONFIG_ENABLE))
>> +                             val = readl(phychan->base + PL080_CH_CONFIG);
>>
>> -     writel(val | PL080_CONFIG_ENABLE, phychan->base + PL080_CH_CONFIG);
>> +             writel(val | PL080_CONFIG_ENABLE,
>> +                     phychan->base + PL080_CH_CONFIG);
>> +     }
>
> You have similar stuff in most the the changes in this patch, because offset of
> config register changes for s3c, you placed in if,else block.
>
> If you check these changes again, there is a lot of code duplication in this if,else
> blocks. The only different thing in if,else is the offset of CH_CONFIG register.
>
> It would be much better if you can do following:
>
> u32 ch_cfg_off;
>
>        if (pl08x->vd->is_pl080_s3c)
>                ch_cfg_off = PL080S_CH_CONFIG;
>        else
>                ch_cfg_off = PL080_CH_CONFIG;
>
> Now, this offset can be used in existing code, without much code duplication.
>
I will use suggestion and remove the code duplications.

>> @@ -569,6 +641,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>>       u32 cctl, early_bytes = 0;
>>       size_t max_bytes_per_lli, total_bytes = 0;
>>       struct pl08x_lli *llis_va;
>> +     size_t lli_len = 0, target_len, tsize, odd_bytes;
>>
>>       txd->llis_va = dma_pool_alloc(pl08x->pool, GFP_NOWAIT, &txd->llis_bus);
>>       if (!txd->llis_va) {
>> @@ -700,7 +773,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>>                * width left
>>                */
>>               while (bd.remainder > (mbus->buswidth - 1)) {
>> -                     size_t lli_len, tsize, width;
>> +                     size_t width;
>>
>
> why do we need above two changes. odd_bytes and target_len are still unused.
>
sorry, I will remove the used variables.
>>                       /*
>>                        * If enough left try to send max possible,
>> @@ -759,6 +832,9 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>>       llis_va[num_llis - 1].lli = 0;
>>       /* The final LLI element shall also fire an interrupt. */
>>       llis_va[num_llis - 1].cctl |= PL080_CONTROL_TC_IRQ_EN;
>> +     /* Keep the TransferSize seperate to fill samsung specific register */
>> +     if (pl08x->vd->is_pl080_s3c)
>> +             llis_va[num_llis - 1].cctl1 |= lli_len;
>
> I couldn't get this completely. Why do you keep only length of
> last lli in cctl1. what about all other llis. Also why |=
> and not directly cctl1 = lli_len
>
I will write more explanation about it.
> --
> viresh
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH V2 1/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Alim Akhtar]

HI Linus,
Thanks for reviewing again.

On Wed, Sep 28, 2011 at 1:31 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> Sorry if I missed a few nitpicks last time, anyway it's looking much better now:
>
> On Wed, Sep 28, 2011 at 7:50 AM, Alim Akhtar <alim.akhtar@samsung.com> wrote:
>> +       /*
>> +        * Samsung pl080 DMAC has one exrta control register
>
> s/exrta/exstra
>
I will correct this.
>> +       if (pl08x->vd->is_pl080_s3c) {
>> +               writel(txd->ccfg, phychan->base + PL080S_CH_CONFIG);
>> +               writel(lli->cctl1, phychan->base + PL080S_CH_CONTROL2);
>> +       } else
>> +               writel(txd->ccfg, phychan->base + PL080_CH_CONFIG);
>
> What do you think about adding a field to the struct pl08x
> like
>
> u32 config_reg
>
> that we set to the proper config register (PL080S_CH_CONFIG or
> PL080_CH_CONFIG in probe(), so the above
> becomes the simpler variant:
>
> writel(txd->ccfg, phychan->base + pl08x->config_reg);
> if (pl08x->vd->is_pl080_s3c)
>    writel(lli->cctl1, phychan->base + PL080S_CH_CONTROL2);
>
I will try implement this way or as viresh has pointed out to remove
the code duplications.

>> +       if (pl08x->vd->is_pl080_s3c) {
>> +               val = readl(phychan->base + PL080S_CH_CONFIG);
>> +               while ((val & PL080_CONFIG_ACTIVE) ||
>> +                       (val & PL080_CONFIG_ENABLE))
>> +                       val = readl(phychan->base + PL080S_CH_CONFIG);
>> +
>> +               writel(val | PL080_CONFIG_ENABLE,
>> +                       phychan->base + PL080S_CH_CONFIG);
>> +       } else {
>>                val = readl(phychan->base + PL080_CH_CONFIG);
>> +                       while ((val & PL080_CONFIG_ACTIVE) ||
>> +                               (val & PL080_CONFIG_ENABLE))
>> +                               val = readl(phychan->base + PL080_CH_CONFIG);
>>
>> -       writel(val | PL080_CONFIG_ENABLE, phychan->base + PL080_CH_CONFIG);
>> +               writel(val | PL080_CONFIG_ENABLE,
>> +                       phychan->base + PL080_CH_CONFIG);
>> +       }
>
> This would also become much simpler with that approach I think...
>
OK, i will do that.
>>        /* Set the HALT bit and wait for the FIFO to drain */
>> -       val = readl(ch->base + PL080_CH_CONFIG);
>> -       val |= PL080_CONFIG_HALT;
>> -       writel(val, ch->base + PL080_CH_CONFIG);
>> -
>> +       if (pl08x->vd->is_pl080_s3c) {
>> +               val = readl(ch->base + PL080S_CH_CONFIG);
>> +               val |= PL080_CONFIG_HALT;
>> +               writel(val, ch->base + PL080S_CH_CONFIG);
>> +       } else {
>> +               val = readl(ch->base + PL080_CH_CONFIG);
>> +               val |= PL080_CONFIG_HALT;
>> +               writel(val, ch->base + PL080_CH_CONFIG);
>> +       }
>
> This would become simply:
>
> val = readl(ch->base + pl08x->config_reg);
> val |= PL080_CONFIG_HALT;
> writel(val, ch->base + pl08x->config_reg);
>
>
ok, i will do that.
>>        /* Clear the HALT bit */
>> -       val = readl(ch->base + PL080_CH_CONFIG);
>> -       val &= ~PL080_CONFIG_HALT;
>> -       writel(val, ch->base + PL080_CH_CONFIG);
>> +       if (pl08x->vd->is_pl080_s3c) {
>> +               val = readl(ch->base + PL080S_CH_CONFIG);
>> +               val &= ~PL080_CONFIG_HALT;
>> +               writel(val, ch->base + PL080S_CH_CONFIG);
>> +       } else {
>> +               val = readl(ch->base + PL080_CH_CONFIG);
>> +               val &= ~PL080_CONFIG_HALT;
>> +               writel(val, ch->base + PL080_CH_CONFIG);
>> +       }
>
> This would get rid of the if/else clauses
>
ok, understand
>> +       if (pl08x->vd->is_pl080_s3c) {
>> +               u32 val = readl(ch->base + PL080S_CH_CONFIG);
>> +               val &= ~(PL080_CONFIG_ENABLE | PL080_CONFIG_ERR_IRQ_MASK |
>> +                               PL080_CONFIG_TC_IRQ_MASK);
>> +               writel(val, ch->base + PL080S_CH_CONFIG);
>> +       } else {
>> +               u32 val = readl(ch->base + PL080_CH_CONFIG);
>> +               val &= ~(PL080_CONFIG_ENABLE | PL080_CONFIG_ERR_IRQ_MASK |
>> +                               PL080_CONFIG_TC_IRQ_MASK);
>> +               writel(val, ch->base + PL080_CH_CONFIG);
>> +       }
>
> As would this...
>
>> +       /* Samsung DMAC is PL080 variant*/
>> +       {
>> +               .id     = 0x00041082,
>> +               .mask   = 0x000fffff,
>> +               .data   = &vendor_pl080_s3c,
>
> Does the hardware realy have this primecell number or is it something that is
> hardcoded from the board/device tree?
>
It is a hardcoded value as s3c64xx does not have Identification registers.

> If it is hardcoded then no objections.
>
> In the latter case, replace 0x41 (= 'A', ARM) with something like
> 0x55 'U' for Samsung or so. Or 0x51 'S'. Or whatever you like.
>
> Then add that to include/linux/amba/bus.h as
> AMBA_VENDOR_SAMSUNG = 0x55,
> so we have this under some kind of control.
>
> Yours,
> Linus Walleij
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>



-- 
aLim akHtaR
mAin hUn nA :-)

Re: [PATCH V2 1/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Viresh Kumar]

On 9/28/2011 2:20 PM, Alim Akhtar wrote:
> The main difference between Primecell PL080 and samsung variant is in
> LLI control register bit [0:11] is reserved in case of samsung pl080
> and one extra register is add to hold the transfer size at offset
> 0x10. The purpose of cctl1 is store the transfer_size.

So, actually you need to modify pl08x_fill_lli_for_desc() and
pl08x_cctl_bits() routines.

Updating cctl1 on the last lli will not solve your purpose,
and transfers needing more than one lli will fail.
BTW, did you try testing your patch for more than one LLI.

-- 
viresh

Re: [PATCH V2 1/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Alim Akhtar]

On Wed, Sep 28, 2011 at 3:24 PM, Viresh Kumar <viresh.kumar@st.com> wrote:
> On 9/28/2011 2:20 PM, Alim Akhtar wrote:
>> The main difference between Primecell PL080 and samsung variant is in
>> LLI control register bit [0:11] is reserved in case of samsung pl080
>> and one extra register is add to hold the transfer size at offset
>> 0x10. The purpose of cctl1 is store the transfer_size.
>
> So, actually you need to modify pl08x_fill_lli_for_desc() and
> pl08x_cctl_bits() routines.
>
I did Modified pl08x_cctl_bits(), but for some reason i reverted it back.
what i was doing something like returning just __retbits__ instead of
retbits |= tsize << PL080_CONTROL_TRANSFER_SIZE_SHIFT;
and doing the below for the __non-s3c__ controllers in the
pl08x_fill_lli_for_desc().
cctl |= 1 << PL080_CONTROL_TRANSFER_SIZE_SHIFT;
pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);

Do you think that will help?
basically i need to extract transfer_size from the cctl and write back
to the cctl1.

> Updating cctl1 on the last lli will not solve your purpose,
> and transfers needing more than one lli will fail.
> BTW, did you try testing your patch for more than one LLI.
>
point taken, i was testing with max 4095 bytes of data, which needs a
single LLI.

> --
> viresh
>

Re: [PATCH V2 1/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Viresh Kumar]

On 9/28/2011 5:24 PM, Alim Akhtar wrote:
> I did Modified pl08x_cctl_bits(), but for some reason i reverted it back.
> what i was doing something like returning just __retbits__ instead of
> retbits |= tsize << PL080_CONTROL_TRANSFER_SIZE_SHIFT;
> and doing the below for the __non-s3c__ controllers in the
> pl08x_fill_lli_for_desc().
> cctl |= 1 << PL080_CONTROL_TRANSFER_SIZE_SHIFT;
> pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
> 

One way out would be:
Don't do retbits |= tsize << PL080_CONTROL_TRANSFER_SIZE_SHIFT in
pl08x_cctl_bits() and do this conditionally in pl08x_fill_lli_for_desc().
This needs adding tsize argument in fill_lli_**() routine and removing it
from cctl_bits().

Probably this is what you were mentioning.

-- 
viresh

Re: [PATCH V2 0/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Linus Walleij]

On Wed, Sep 28, 2011 at 7:50 AM, Alim Akhtar <alim.akhtar@samsung.com> wrote:

> This patch modifies the amba-pl08x driver for s3c64xx.
> The DMA controller of S3C64XX is a variant of PrimeCell pl080 DMAC.
> S3C64xx contents extra register to hold the TransferSize.

Alim what is happening with this patch series?

I was really happy with the rate of progress we had and Viresh & I
have given most feedback needed to complete this I think.

Yours,
Linus Walleij

Re: [PATCH V2 0/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Alim Akhtar]

Hello Linus,
sorry for the delay in submitting V3 of the pl080 patches for s3c64xx.
I am very busy with my current assignment.
I will come back to my patch series incorporating yours and Viresh
suggestions in v3.

Thanks!!

Regards,
Alim

On Thu, Dec 8, 2011 at 8:43 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Sep 28, 2011 at 7:50 AM, Alim Akhtar <alim.akhtar@samsung.com> wrote:
>
>> This patch modifies the amba-pl08x driver for s3c64xx.
>> The DMA controller of S3C64XX is a variant of PrimeCell pl080 DMAC.
>> S3C64xx contents extra register to hold the TransferSize.
>
> Alim what is happening with this patch series?
>
> I was really happy with the rate of progress we had and Viresh & I
> have given most feedback needed to complete this I think.
>
> Yours,
> Linus Walleij
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
aLim akHtaR
mAin hUn nA :-)

Re: [PATCH V2 0/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Linus Walleij]

On Thu, Dec 8, 2011 at 2:03 AM, Alim Akhtar <alim.akhtar@gmail.com> wrote:

> Hello Linus,
> sorry for the delay in submitting V3 of the pl080 patches for s3c64xx.
> I am very busy with my current assignment.
> I will come back to my patch series incorporating yours and Viresh
> suggestions in v3.

Ping!

Yours,
Linus Walleij

Re: [PATCH V2 0/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Linus Walleij]

On Thu, Dec 8, 2011 at 9:03 AM, Alim Akhtar <alim.akhtar@gmail.com> wrote:

> Hello Linus,
> sorry for the delay in submitting V3 of the pl080 patches for s3c64xx.
> I am very busy with my current assignment.
> I will come back to my patch series incorporating yours and Viresh
> suggestions in v3.

Ping on this, is there something in specific that you need help on here?
We really need to get rid of this forked code, and it should be beneficial
to S3C because we have fixed a lot of bugs in the PL08x driver now,
plus it'll likely be a prerequisite to DT support for S3C.

Yours,
Linus Walleij

Re: [PATCH V2 0/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Russell King - ARM Linux]

On Mon, May 28, 2012 at 12:41:42PM +0800, Linus Walleij wrote:
> On Thu, Dec 8, 2011 at 9:03 AM, Alim Akhtar <alim.akhtar@gmail.com> wrote:
> 
> > Hello Linus,
> > sorry for the delay in submitting V3 of the pl080 patches for s3c64xx.
> > I am very busy with my current assignment.
> > I will come back to my patch series incorporating yours and Viresh
> > suggestions in v3.
> 
> Ping on this, is there something in specific that you need help on here?
> We really need to get rid of this forked code, and it should be beneficial
> to S3C because we have fixed a lot of bugs in the PL08x driver now,
> plus it'll likely be a prerequisite to DT support for S3C.

So this hasn't gone in?

This is going to be painful - I have 40 odd patches to PL08x converting it
to virt-dma now...  It's probably going to be easier for the S3C conversion
to be based upon my work rather than me having to rework all those patches.

Re: [PATCH V2 0/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Alim Akhtar]

Hi Russell,

Sorry, did not get time to rework on s3c pl080 dma patches.

I will re-base s3c pl080 patches on top of your virt-dma changes.


Regards,

Alim

On Mon, May 28, 2012 at 1:06 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, May 28, 2012 at 12:41:42PM +0800, Linus Walleij wrote:
>> On Thu, Dec 8, 2011 at 9:03 AM, Alim Akhtar <alim.akhtar@gmail.com> wrote:
>>
>> > Hello Linus,
>> > sorry for the delay in submitting V3 of the pl080 patches for s3c64xx.
>> > I am very busy with my current assignment.
>> > I will come back to my patch series incorporating yours and Viresh
>> > suggestions in v3.
>>
>> Ping on this, is there something in specific that you need help on here?
>> We really need to get rid of this forked code, and it should be beneficial
>> to S3C because we have fixed a lot of bugs in the PL08x driver now,
>> plus it'll likely be a prerequisite to DT support for S3C.
>
> So this hasn't gone in?
>
> This is going to be painful - I have 40 odd patches to PL08x converting it
> to virt-dma now...  It's probably going to be easier for the S3C conversion
> to be based upon my work rather than me having to rework all those patches.

Re: [PATCH V2 0/1] dmaengine/amba-pl08x: Add support for s3c64xx DMAC

[Linus Walleij]

On Wed, Sep 28, 2011 at 7:50 AM, Alim Akhtar <alim.akhtar@samsung.com> wrote:

> This patch modifies the amba-pl08x driver for s3c64xx.
> The DMA controller of S3C64XX is a variant of PrimeCell pl080 DMAC.
> S3C64xx contents extra register to hold the TransferSize.

Ping on this. Russell's patches have landed in linux-next so you can
rebase your patches on top of what's in linux-next for a convenient
testbed.

Yours,
Linus Walleij