[PATCH V2 0/6] spi/spi-pl022 fixes

[PATCH V2 0/6] spi/spi-pl022 fixes

[Viresh Kumar]

Hi Grant,

I have added Tested-by: Linus Walleij <linus.walleij@linaro.org> on all patches.

This patchset mainly covers following fixes:
- formatting related issues
- Passing GFP_ATOMIC for sg allocation from tasklet
- Fixing calculate_effective_freq() routine
- Allocate/free DMA channels as and when required.

Changes since V1:
- Replace GFP_NOWAIT with GFP_ATOMIC
- Wrote inline routine spi_rate instead of macro

Viresh Kumar (6):
  spi/spi-pl022: Resolve formatting issues
  spi/spi-pl022: Use GFP_ATOMIC for allocation from tasklet
  spi/spi-pl022: Don't allocate more sg than required.
  spi/spi-pl022: calculate_effective_freq() must set rate <= requested
    rate
  spi/spi-pl022: Call pl022_dma_remove(pl022) only if enable_dma is
    true
  spi/spi-pl022: Request/free DMA channels as and when required.

 drivers/spi/spi-pl022.c |  269 ++++++++++++++++++++++++++---------------------
 1 files changed, 151 insertions(+), 118 deletions(-)

-- 
1.7.2.2

[PATCH V2 1/6] spi/spi-pl022: Resolve formatting issues

[Viresh Kumar]

There were few formatting related issues in code. This patch fixes them.
Fixes include:
- Remove extra blank lines
- align code to 80 cols
- combine several lines to one line
- Replace multiple spaces with tabs
- Remove spaces before labels

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-pl022.c |   54 +++++++++++++++++-----------------------------
 1 files changed, 20 insertions(+), 34 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 730b4a3..f600d00 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -113,7 +113,6 @@
 #define SSP_CR0_MASK_CSS_ST	(0x1FUL << 16)
 #define SSP_CR0_MASK_FRF_ST	(0x3UL << 21)
 
-
 /*
  * SSP Control Register 0  - SSP_CR1
  */
@@ -283,7 +282,6 @@
 
 #define SPI_POLLING_TIMEOUT 1000
 
-
 /*
  * The type of reading going on on this chip
  */
@@ -752,7 +750,6 @@ static void readwriter(struct pl022 *pl022)
 	 */
 }
 
-
 /**
  * next_transfer - Move to the Next transfer in the current spi message
  * @pl022: SSP driver private data structure
@@ -1534,8 +1531,7 @@ static void pump_messages(struct work_struct *work)
 	/* Initial message state */
 	pl022->cur_msg->state = STATE_START;
 	pl022->cur_transfer = list_entry(pl022->cur_msg->transfers.next,
-					    struct spi_transfer,
-					    transfer_list);
+					    struct spi_transfer, transfer_list);
 
 	/* Setup the SPI using the per chip configuration */
 	pl022->cur_chip = spi_get_ctldata(pl022->cur_msg->spi);
@@ -1557,7 +1553,6 @@ static void pump_messages(struct work_struct *work)
 		do_interrupt_dma_transfer(pl022);
 }
 
-
 static int __init init_queue(struct pl022 *pl022)
 {
 	INIT_LIST_HEAD(&pl022->queue);
@@ -1566,8 +1561,8 @@ static int __init init_queue(struct pl022 *pl022)
 	pl022->running = false;
 	pl022->busy = false;
 
-	tasklet_init(&pl022->pump_transfers,
-			pump_transfers,	(unsigned long)pl022);
+	tasklet_init(&pl022->pump_transfers, pump_transfers,
+			(unsigned long)pl022);
 
 	INIT_WORK(&pl022->pump_messages, pump_messages);
 	pl022->workqueue = create_singlethread_workqueue(
@@ -1578,7 +1573,6 @@ static int __init init_queue(struct pl022 *pl022)
 	return 0;
 }
 
-
 static int start_queue(struct pl022 *pl022)
 {
 	unsigned long flags;
@@ -1601,7 +1595,6 @@ static int start_queue(struct pl022 *pl022)
 	return 0;
 }
 
-
 static int stop_queue(struct pl022 *pl022)
 {
 	unsigned long flags;
@@ -1861,7 +1854,6 @@ static int calculate_effective_freq(struct pl022 *pl022,
 	return 0;
 }
 
-
 /*
  * A piece of default chip info unless the platform
  * supplies it.
@@ -1879,7 +1871,6 @@ static const struct pl022_config_chip pl022_default_chip_info = {
 	.cs_control = null_cs_control,
 };
 
-
 /**
  * pl022_setup - setup function registered to SPI master framework
  * @spi: spi device which is requesting setup
@@ -1956,7 +1947,6 @@ static int pl022_setup(struct spi_device *spi)
 		goto err_config_params;
 	}
 
-
 	status = verify_controller_parameters(pl022, chip_info);
 	if (status) {
 		dev_err(&spi->dev, "controller data is incorrect");
@@ -2096,12 +2086,13 @@ static int pl022_setup(struct spi_device *spi)
 	}
 	SSP_WRITE_BITS(chip->cr1, SSP_DISABLED, SSP_CR1_MASK_SSE, 1);
 	SSP_WRITE_BITS(chip->cr1, chip_info->hierarchy, SSP_CR1_MASK_MS, 2);
-	SSP_WRITE_BITS(chip->cr1, chip_info->slave_tx_disable, SSP_CR1_MASK_SOD, 3);
+	SSP_WRITE_BITS(chip->cr1, chip_info->slave_tx_disable, SSP_CR1_MASK_SOD,
+		3);
 
 	/* Save controller_state */
 	spi_set_ctldata(spi, chip);
 	return status;
- err_config_params:
+err_config_params:
 	spi_set_ctldata(spi, NULL);
 	kfree(chip);
 	return status;
@@ -2122,7 +2113,6 @@ static void pl022_cleanup(struct spi_device *spi)
 	kfree(chip);
 }
 
-
 static int __devinit
 pl022_probe(struct amba_device *adev, const struct amba_id *id)
 {
@@ -2243,23 +2233,23 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
 	amba_vcore_disable(adev);
 	return 0;
 
- err_spi_register:
- err_start_queue:
- err_init_queue:
+err_spi_register:
+err_start_queue:
+err_init_queue:
 	destroy_queue(pl022);
 	pl022_dma_remove(pl022);
 	free_irq(adev->irq[0], pl022);
 	pm_runtime_disable(&adev->dev);
- err_no_irq:
+err_no_irq:
 	clk_put(pl022->clk);
- err_no_clk:
+err_no_clk:
 	iounmap(pl022->virtbase);
- err_no_ioremap:
+err_no_ioremap:
 	amba_release_regions(adev);
- err_no_ioregion:
+err_no_ioregion:
 	spi_master_put(master);
- err_no_master:
- err_no_pdata:
+err_no_master:
+err_no_pdata:
 	return status;
 }
 
@@ -2337,7 +2327,6 @@ static struct vendor_data vendor_arm = {
 	.loopback = true,
 };
 
-
 static struct vendor_data vendor_st = {
 	.fifodepth = 32,
 	.max_bpw = 32,
@@ -2392,9 +2381,9 @@ static struct amba_id pl022_ids[] = {
 		 * and 32 locations deep TX/RX FIFO but no extended
 		 * CR0/CR1 register
 		 */
-		.id     = 0x00080023,
-		.mask   = 0xffffffff,
-		.data   = &vendor_st_pl023,
+		.id	= 0x00080023,
+		.mask	= 0xffffffff,
+		.data	= &vendor_st_pl023,
 	},
 	{
 		.id	= 0x10080023,
@@ -2411,23 +2400,20 @@ static struct amba_driver pl022_driver = {
 	.id_table	= pl022_ids,
 	.probe		= pl022_probe,
 	.remove		= __devexit_p(pl022_remove),
-	.suspend        = pl022_suspend,
-	.resume         = pl022_resume,
+	.suspend	= pl022_suspend,
+	.resume		= pl022_resume,
 };
 
-
 static int __init pl022_init(void)
 {
 	return amba_driver_register(&pl022_driver);
 }
-
 subsys_initcall(pl022_init);
 
 static void __exit pl022_exit(void)
 {
 	amba_driver_unregister(&pl022_driver);
 }
-
 module_exit(pl022_exit);
 
 MODULE_AUTHOR("Linus Walleij <linus.walleij@stericsson.com>");
-- 
1.7.2.2

[PATCH V2 1/6] spi/spi-pl022: Resolve formatting issues

[Grant Likely]

On Wed, Aug 10, 2011 at 02:20:54PM +0530, Viresh Kumar wrote:
> There were few formatting related issues in code. This patch fixes them.
> Fixes include:
> - Remove extra blank lines
> - align code to 80 cols
> - combine several lines to one line
> - Replace multiple spaces with tabs
> - Remove spaces before labels
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> Tested-by: Linus Walleij <linus.walleij@linaro.org>
> @@ -2243,23 +2233,23 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>  	amba_vcore_disable(adev);
>  	return 0;
>  
> - err_spi_register:
> - err_start_queue:
> - err_init_queue:
> +err_spi_register:
> +err_start_queue:
> +err_init_queue:
>  	destroy_queue(pl022);
>  	pl022_dma_remove(pl022);
>  	free_irq(adev->irq[0], pl022);
>  	pm_runtime_disable(&adev->dev);
> - err_no_irq:
> +err_no_irq:
>  	clk_put(pl022->clk);
> - err_no_clk:
> +err_no_clk:
>  	iounmap(pl022->virtbase);
> - err_no_ioremap:
> +err_no_ioremap:
>  	amba_release_regions(adev);
> - err_no_ioregion:
> +err_no_ioregion:
>  	spi_master_put(master);
> - err_no_master:
> - err_no_pdata:
> +err_no_master:
> +err_no_pdata:

These label indentations are actually on purpose.  I'll apply the rest
of the patch, but I'm going to remove this hunk.  Indenting error
labels by 1 space makes 'diff' show the function name instead of the
label in the context line.

g.

[PATCH V2 2/6] spi/spi-pl022: Use GFP_ATOMIC for allocation from tasklet

[Viresh Kumar]

tasklets don't allow invocation to sleeping routines. In configure_dma()
routine, sg_alloc_table() was called with GFP_KERNEL flag and so this causes
crash when called from tasklet.

Replace GFP_KERNEL with GFP_ATOMIC to get this fixed.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-pl022.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index f600d00..80116be 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -1019,11 +1019,11 @@ static int configure_dma(struct pl022 *pl022)
 	pages = (pl022->cur_transfer->len >> PAGE_SHIFT) + 1;
 	dev_dbg(&pl022->adev->dev, "using %d pages for transfer\n", pages);
 
-	ret = sg_alloc_table(&pl022->sgt_rx, pages, GFP_KERNEL);
+	ret = sg_alloc_table(&pl022->sgt_rx, pages, GFP_ATOMIC);
 	if (ret)
 		goto err_alloc_rx_sg;
 
-	ret = sg_alloc_table(&pl022->sgt_tx, pages, GFP_KERNEL);
+	ret = sg_alloc_table(&pl022->sgt_tx, pages, GFP_ATOMIC);
 	if (ret)
 		goto err_alloc_tx_sg;
 
-- 
1.7.2.2

[PATCH V2 3/6] spi/spi-pl022: Don't allocate more sg than required.

[Viresh Kumar]

In routine configure_dma(), if transfer->len = PAGE_SIZE, then pages is one more
than required. While leads to one more sg getting allocated.

This is wrong. Correct this to allocate correct number of sg.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-pl022.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 80116be..1c8b9ec 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -1016,7 +1016,8 @@ static int configure_dma(struct pl022 *pl022)
 	dmaengine_slave_config(txchan, &tx_conf);
 
 	/* Create sglists for the transfers */
-	pages = (pl022->cur_transfer->len >> PAGE_SHIFT) + 1;
+	pages = ((pl022->cur_transfer->len + (1 << PAGE_SHIFT) - 1)
+			>> PAGE_SHIFT);
 	dev_dbg(&pl022->adev->dev, "using %d pages for transfer\n", pages);
 
 	ret = sg_alloc_table(&pl022->sgt_rx, pages, GFP_ATOMIC);
-- 
1.7.2.2

[PATCH V2 3/6] spi/spi-pl022: Don't allocate more sg than required.

[Russell King - ARM Linux]

On Wed, Aug 10, 2011 at 02:20:56PM +0530, Viresh Kumar wrote:
> In routine configure_dma(), if transfer->len = PAGE_SIZE, then pages is one more
> than required. While leads to one more sg getting allocated.
> 
> This is wrong. Correct this to allocate correct number of sg.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> Tested-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/spi/spi-pl022.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
> index 80116be..1c8b9ec 100644
> --- a/drivers/spi/spi-pl022.c
> +++ b/drivers/spi/spi-pl022.c
> @@ -1016,7 +1016,8 @@ static int configure_dma(struct pl022 *pl022)
>  	dmaengine_slave_config(txchan, &tx_conf);
>  
>  	/* Create sglists for the transfers */
> -	pages = (pl022->cur_transfer->len >> PAGE_SHIFT) + 1;
> +	pages = ((pl022->cur_transfer->len + (1 << PAGE_SHIFT) - 1)
> +			>> PAGE_SHIFT);

It would be far better for this to be:

	pages = DIV_ROUND_UP(pl022->cur_transfer->len, PAGE_SIZE);

The compiler will probably optimize it to the same code anyway.

[PATCH V2 3/6] spi/spi-pl022: Don't allocate more sg than required.

[viresh kumar]

On 08/10/2011 02:24 PM, Russell King - ARM Linux wrote:
> It would be far better for this to be:
> 
> 	pages = DIV_ROUND_UP(pl022->cur_transfer->len, PAGE_SIZE);
> 
> The compiler will probably optimize it to the same code anyway.

I thought of it too, but missed to update code. Thanks.

-- 
viresh

[PATCH V2 3/6] spi/spi-pl022: Don't allocate more sg than required.

[Sergei Shtylyov]

Hello.

On 10-08-2011 12:50, Viresh Kumar wrote:

> In routine configure_dma(), if transfer->len = PAGE_SIZE, then pages is one more
> than required. While leads to one more sg getting allocated.

> This is wrong. Correct this to allocate correct number of sg.

> Signed-off-by: Viresh Kumar<viresh.kumar@st.com>
> Tested-by: Linus Walleij<linus.walleij@linaro.org>
> ---
>   drivers/spi/spi-pl022.c |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)

> diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
> index 80116be..1c8b9ec 100644
> --- a/drivers/spi/spi-pl022.c
> +++ b/drivers/spi/spi-pl022.c
> @@ -1016,7 +1016,8 @@ static int configure_dma(struct pl022 *pl022)
>   	dmaengine_slave_config(txchan,&tx_conf);
>
>   	/* Create sglists for the transfers */
> -	pages = (pl022->cur_transfer->len>>  PAGE_SHIFT) + 1;
> +	pages = ((pl022->cur_transfer->len + (1 << PAGE_SHIFT) - 1)
> +			>>  PAGE_SHIFT);

    No need for parens around the rvalue.

WBR, Sergei

[PATCH V2 3/6] spi/spi-pl022: Don't allocate more sg than required.

[viresh kumar]

On 08/10/2011 05:12 PM, Sergei Shtylyov wrote:
>> > -	pages = (pl022->cur_transfer->len>>  PAGE_SHIFT) + 1;
>> > +	pages = ((pl022->cur_transfer->len + (1 << PAGE_SHIFT) - 1)
>> > +			>>  PAGE_SHIFT);
>     No need for parens around the rvalue.

Oops, that was a mistake. Anyway i have send V3 for the same and this issue
is not present anymore.

-- 
viresh

[PATCH V2 4/6] spi/spi-pl022: calculate_effective_freq() must set rate <= requested rate

[Viresh Kumar]

There were few issues with calculate_effective_freq() routine:
- It was returning first rate found >= requested rate. Now, if system have spi's
  rate as 83 MHz, with possible prescaled rates as 83, 41.5, 20.75, 13.83 (as we
  can prescale with multiples of 2). If user has given rate to be programmed as
  22 MHz, then driver programmes it to 41.5 MHz. This looks to be incorrect, as
  user might have given the upper limit of the device, and we are programming it
  above it.
- Driver finds the first satisfying rate and programmes it, but with other
  values of scr & cpsdvsr, it is possible to get more closer rate.

This patch fixes these two issues, with some reformatting inside the code.  This
also creates a inline routine to calculate prescaled rate based on spi's rate,
cpsdvsr and scr.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-pl022.c |  102 +++++++++++++++++++++++-----------------------
 1 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 1c8b9ec..d1bcc79 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -1791,67 +1791,67 @@ static int pl022_transfer(struct spi_device *spi, struct spi_message *msg)
 	return 0;
 }
 
-static int calculate_effective_freq(struct pl022 *pl022,
-				    int freq,
-				    struct ssp_clock_params *clk_freq)
+static inline u32 spi_rate(u32 rate, u16 cpsdvsr, u16 scr)
+{
+	return rate / (cpsdvsr * (1 + scr));
+}
+
+static int calculate_effective_freq(struct pl022 *pl022, int freq, struct
+				    ssp_clock_params * clk_freq)
 {
 	/* Lets calculate the frequency parameters */
-	u16 cpsdvsr = 2;
-	u16 scr = 0;
-	bool freq_found = false;
-	u32 rate;
-	u32 max_tclk;
-	u32 min_tclk;
+	u16 cpsdvsr = CPSDVR_MIN, scr = SCR_MIN;
+	u32 rate, max_tclk, min_tclk, best_freq = 0, best_cpsdvsr = 0,
+		best_scr = 0, tmp, found = 0;
 
 	rate = clk_get_rate(pl022->clk);
 	/* cpsdvscr = 2 & scr 0 */
-	max_tclk = (rate / (CPSDVR_MIN * (1 + SCR_MIN)));
+	max_tclk = spi_rate(rate, CPSDVR_MIN, SCR_MIN);
 	/* cpsdvsr = 254 & scr = 255 */
-	min_tclk = (rate / (CPSDVR_MAX * (1 + SCR_MAX)));
-
-	if ((freq <= max_tclk) && (freq >= min_tclk)) {
-		while (cpsdvsr <= CPSDVR_MAX && !freq_found) {
-			while (scr <= SCR_MAX && !freq_found) {
-				if ((rate /
-				     (cpsdvsr * (1 + scr))) > freq)
-					scr += 1;
-				else {
-					/*
-					 * This bool is made true when
-					 * effective frequency >=
-					 * target frequency is found
-					 */
-					freq_found = true;
-					if ((rate /
-					     (cpsdvsr * (1 + scr))) != freq) {
-						if (scr == SCR_MIN) {
-							cpsdvsr -= 2;
-							scr = SCR_MAX;
-						} else
-							scr -= 1;
-					}
-				}
-			}
-			if (!freq_found) {
-				cpsdvsr += 2;
-				scr = SCR_MIN;
-			}
-		}
-		if (cpsdvsr != 0) {
-			dev_dbg(&pl022->adev->dev,
-				"SSP Effective Frequency is %u\n",
-				(rate / (cpsdvsr * (1 + scr))));
-			clk_freq->cpsdvsr = (u8) (cpsdvsr & 0xFF);
-			clk_freq->scr = (u8) (scr & 0xFF);
-			dev_dbg(&pl022->adev->dev,
-				"SSP cpsdvsr = %d, scr = %d\n",
-				clk_freq->cpsdvsr, clk_freq->scr);
-		}
-	} else {
+	min_tclk = spi_rate(rate, CPSDVR_MAX, SCR_MAX);
+
+	if (!((freq <= max_tclk) && (freq >= min_tclk))) {
 		dev_err(&pl022->adev->dev,
 			"controller data is incorrect: out of range frequency");
 		return -EINVAL;
 	}
+
+	/*
+	 * best_freq will give closest possible available rate (<= requested
+	 * freq) for all values of scr & cpsdvsr.
+	 */
+	while ((cpsdvsr <= CPSDVR_MAX) && !found) {
+		while (scr <= SCR_MAX) {
+			tmp = spi_rate(rate, cpsdvsr, scr);
+
+			if (tmp > freq)
+				scr++;
+			/*
+			 * If found exact value, update and break.
+			 * If found more closer value, update and continue.
+			 */
+			else if ((tmp == freq) || (tmp > best_freq)) {
+				best_freq = tmp;
+				best_cpsdvsr = cpsdvsr;
+				best_scr = scr;
+
+				if (tmp == freq)
+					break;
+			}
+			scr++;
+		}
+		cpsdvsr += 2;
+		scr = SCR_MIN;
+	}
+
+	clk_freq->cpsdvsr = (u8) (best_cpsdvsr & 0xFF);
+	clk_freq->scr = (u8) (best_scr & 0xFF);
+	dev_dbg(&pl022->adev->dev,
+		"SSP Target Frequency is: %u, Effective Frequency is %u\n",
+		freq, best_freq);
+	dev_dbg(&pl022->adev->dev, "SSP cpsdvsr = %d, scr = %d\n",
+		clk_freq->cpsdvsr, clk_freq->scr);
+
 	return 0;
 }
 
-- 
1.7.2.2

[PATCH V2 5/6] spi/spi-pl022: Call pl022_dma_remove(pl022) only if enable_dma is true

[Viresh Kumar]

pl022_dma_remove() should be called only if enable_dma is true. There is no
point calling it when pl022_dma_probe() is not called, which again depends on
enable_dma.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-pl022.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index d1bcc79..01e84e3 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -2238,7 +2238,9 @@ err_spi_register:
 err_start_queue:
 err_init_queue:
 	destroy_queue(pl022);
-	pl022_dma_remove(pl022);
+	if (platform_info->enable_dma)
+		pl022_dma_remove(pl022);
+
 	free_irq(adev->irq[0], pl022);
 	pm_runtime_disable(&adev->dev);
 err_no_irq:
@@ -2266,7 +2268,9 @@ pl022_remove(struct amba_device *adev)
 	if (destroy_queue(pl022) != 0)
 		dev_err(&adev->dev, "queue remove failed\n");
 	load_ssp_default_config(pl022);
-	pl022_dma_remove(pl022);
+	if (pl022->master_info->enable_dma)
+		pl022_dma_remove(pl022);
+
 	free_irq(adev->irq[0], pl022);
 	clk_disable(pl022->clk);
 	clk_put(pl022->clk);
-- 
1.7.2.2

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Viresh Kumar]

Currently we request DMA channels at probe time and free them at remove. They
are always occupied, irrespective of their usage.

They must be allocated when they are required and must be freed after we are
done with transfers. So that they can be used by other users.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-pl022.c |   98 +++++++++++++++++++++++++++++++++-------------
 1 files changed, 70 insertions(+), 28 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 01e84e3..a596b96 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -389,6 +389,7 @@ struct pl022 {
 	struct sg_table			sgt_rx;
 	struct sg_table			sgt_tx;
 	char				*dummypage;
+	dma_cap_mask_t			mask;
 #endif
 };
 
@@ -1093,16 +1094,33 @@ err_alloc_rx_sg:
 
 static int __init pl022_dma_probe(struct pl022 *pl022)
 {
-	dma_cap_mask_t mask;
+	/* set dma mask */
+	dma_cap_zero(pl022->mask);
+	dma_cap_set(DMA_SLAVE, pl022->mask);
 
-	/* Try to acquire a generic DMA engine slave channel */
-	dma_cap_zero(mask);
-	dma_cap_set(DMA_SLAVE, mask);
+	pl022->dummypage = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!pl022->dummypage) {
+		dev_err(&pl022->adev->dev,
+			"Failed to work in dma mode, work without dma!\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int pl022_alloc_dmachan(struct pl022 *pl022)
+{
 	/*
-	 * We need both RX and TX channels to do DMA, else do none
-	 * of them.
+	 * Both channels should be allocated or not allocated. It is wrong if
+	 * only one allocated
 	 */
-	pl022->dma_rx_channel = dma_request_channel(mask,
+	if (pl022->dma_rx_channel && pl022->dma_tx_channel)
+		return 0;
+
+	BUG_ON(pl022->dma_rx_channel || pl022->dma_tx_channel);
+
+	/* We need both RX and TX channels to do DMA, else do none of them. */
+	pl022->dma_rx_channel = dma_request_channel(pl022->mask,
 					    pl022->master_info->dma_filter,
 					    pl022->master_info->dma_rx_param);
 	if (!pl022->dma_rx_channel) {
@@ -1110,7 +1128,7 @@ static int __init pl022_dma_probe(struct pl022 *pl022)
 		goto err_no_rxchan;
 	}
 
-	pl022->dma_tx_channel = dma_request_channel(mask,
+	pl022->dma_tx_channel = dma_request_channel(pl022->mask,
 					    pl022->master_info->dma_filter,
 					    pl022->master_info->dma_tx_param);
 	if (!pl022->dma_tx_channel) {
@@ -1118,20 +1136,12 @@ static int __init pl022_dma_probe(struct pl022 *pl022)
 		goto err_no_txchan;
 	}
 
-	pl022->dummypage = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!pl022->dummypage) {
-		dev_dbg(&pl022->adev->dev, "no DMA dummypage!\n");
-		goto err_no_dummypage;
-	}
-
-	dev_info(&pl022->adev->dev, "setup for DMA on RX %s, TX %s\n",
+	dev_dbg(&pl022->adev->dev, "setup for DMA on RX %s, TX %s\n",
 		 dma_chan_name(pl022->dma_rx_channel),
 		 dma_chan_name(pl022->dma_tx_channel));
 
 	return 0;
 
-err_no_dummypage:
-	dma_release_channel(pl022->dma_tx_channel);
 err_no_txchan:
 	dma_release_channel(pl022->dma_rx_channel);
 	pl022->dma_rx_channel = NULL;
@@ -1143,22 +1153,30 @@ err_no_rxchan:
 
 static void terminate_dma(struct pl022 *pl022)
 {
-	struct dma_chan *rxchan = pl022->dma_rx_channel;
-	struct dma_chan *txchan = pl022->dma_tx_channel;
-
-	dmaengine_terminate_all(rxchan);
-	dmaengine_terminate_all(txchan);
-	unmap_free_dma_scatter(pl022);
+	if (pl022->dma_rx_channel)
+		dmaengine_terminate_all(pl022->dma_rx_channel);
+	if (pl022->dma_tx_channel)
+		dmaengine_terminate_all(pl022->dma_tx_channel);
 }
 
-static void pl022_dma_remove(struct pl022 *pl022)
+static void pl022_free_dmachan(struct pl022 *pl022)
 {
-	if (pl022->busy)
-		terminate_dma(pl022);
 	if (pl022->dma_tx_channel)
 		dma_release_channel(pl022->dma_tx_channel);
 	if (pl022->dma_rx_channel)
 		dma_release_channel(pl022->dma_rx_channel);
+
+	pl022->dma_rx_channel = pl022->dma_tx_channel = NULL;
+}
+
+static void pl022_dma_remove(struct pl022 *pl022)
+{
+	if (pl022->busy) {
+		terminate_dma(pl022);
+		unmap_free_dma_scatter(pl022);
+	}
+
+	pl022_free_dmachan(pl022);
 	kfree(pl022->dummypage);
 }
 
@@ -1176,6 +1194,19 @@ static inline int pl022_dma_probe(struct pl022 *pl022)
 static inline void pl022_dma_remove(struct pl022 *pl022)
 {
 }
+
+static inline int pl022_alloc_dmachan(struct pl022 *pl022)
+{
+	return 0;
+}
+
+static inline void terminate_dma(struct pl022 *pl022)
+{
+}
+
+static void pl022_free_dmachan(struct pl022 *pl022)
+{
+}
 #endif
 
 /**
@@ -1401,16 +1432,20 @@ static void do_interrupt_dma_transfer(struct pl022 *pl022)
 	}
 	/* If we're using DMA, set up DMA here */
 	if (pl022->cur_chip->enable_dma) {
+		int status = pl022_alloc_dmachan(pl022);
+		if (status)
+			goto switch_to_irq_mode;
+
 		/* Configure DMA transfer */
 		if (configure_dma(pl022)) {
 			dev_dbg(&pl022->adev->dev,
 				"configuration of DMA failed, fall back to interrupt mode\n");
-			goto err_config_dma;
+			goto switch_to_irq_mode;
 		}
 		/* Disable interrupts in DMA mode, IRQ from DMA controller */
 		irqflags = DISABLE_ALL_INTERRUPTS;
 	}
-err_config_dma:
+switch_to_irq_mode:
 	/* Enable SSP, turn on interrupts */
 	writew((readw(SSP_CR1(pl022->virtbase)) | SSP_CR1_MASK_SSE),
 	       SSP_CR1(pl022->virtbase));
@@ -1513,7 +1548,14 @@ static void pump_messages(struct work_struct *work)
 	spin_lock_irqsave(&pl022->queue_lock, flags);
 	if (list_empty(&pl022->queue) || !pl022->running) {
 		pl022->busy = false;
+
 		spin_unlock_irqrestore(&pl022->queue_lock, flags);
+
+		/* free dma channels */
+		if (pl022->master_info->enable_dma) {
+			terminate_dma(pl022);
+			pl022_free_dmachan(pl022);
+		}
 		return;
 	}
 	/* Make sure we are not already running a message */
-- 
1.7.2.2

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Russell King - ARM Linux]

On Wed, Aug 10, 2011 at 02:20:59PM +0530, Viresh Kumar wrote:
> Currently we request DMA channels at probe time and free them at remove. They
> are always occupied, irrespective of their usage.
> 
> They must be allocated when they are required and must be freed after we are
> done with transfers. So that they can be used by other users.

Which DMA engine driver requires this?

Normally, when we have DMA engine drivers with multiple request signals,
the slave peripheral side publishes several virtual channels which are
claimed by the peripheral drivers.  This (amongst other things) allows
the peripheral drivers to hold claim to one of the virtual channels
all the time that it's required.

This actually results in better usage of the DMA controller, as the
virtual channels can be assigned to physical channels dynamically
according to what the physical channels are doing.

Plus, actually, the idea that DMA channels should be a short-term thing
is broken as soon as you start considering things like UARTs, where you
need a channel tied up long-term for the receive side of things.

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[viresh kumar]

On 08/10/2011 02:30 PM, Russell King - ARM Linux wrote:
>> > They must be allocated when they are required and must be freed after we are
>> > done with transfers. So that they can be used by other users.
> Which DMA engine driver requires this?
> 

dw_dmac.c

> Normally, when we have DMA engine drivers with multiple request signals,
> the slave peripheral side publishes several virtual channels which are
> claimed by the peripheral drivers.  This (amongst other things) allows
> the peripheral drivers to hold claim to one of the virtual channels
> all the time that it's required.

If users of DMA expect DMA engine drivers to work this way, then we should
have this mentioned clearly in DMA slave documentation.

@Dan/Vinod: What do you say?

-- 
viresh

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Koul, Vinod]

On Wed, 2011-08-10 at 14:59 +0530, viresh kumar wrote:
> On 08/10/2011 02:30 PM, Russell King - ARM Linux wrote:
> >> > They must be allocated when they are required and must be freed after we are
> >> > done with transfers. So that they can be used by other users.
> > Which DMA engine driver requires this?
> > 
> 
> dw_dmac.c
> 
> > Normally, when we have DMA engine drivers with multiple request signals,
> > the slave peripheral side publishes several virtual channels which are
> > claimed by the peripheral drivers.  This (amongst other things) allows
> > the peripheral drivers to hold claim to one of the virtual channels
> > all the time that it's required.
> 
> If users of DMA expect DMA engine drivers to work this way, then we should
> have this mentioned clearly in DMA slave documentation.
> 
> @Dan/Vinod: What do you say?
I would agree on both counts :)

In some cases it does make sense to hold the channel for the lifetime
like uart or where the channel has been tied to an interface by SoC
designer.
But in some cases like dw_dmac it seems you can assign channels
dynamically to each usage, and runtime allocation ensures we have best
utilization.
So i would argue that there is no "one size fits all" here, if you can
manage channels dynamically and utilize more efficiently then go ahead,
but if you cant (h/w and usage constraint) then you should not be forced
to do so.

On DMA Engine API, it doesn't force for any of the above. You are free
to choose based on the usage and capability

And on your patch, are you able to dynamically assign the channels for
platform? What is the intended usage? (as Russell articulated it is bad
to dynamically assign channel for something like uart)

-- 
~Vinod

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[viresh kumar]

On 08/10/2011 03:31 PM, Koul, Vinod wrote:
> And on your patch, are you able to dynamically assign the channels for
> platform? What is the intended usage? (as Russell articulated it is bad
> to dynamically assign channel for something like uart)

Are you talking about channels or DMA request lines? For channels yes,
we can always allocate channels as they are independent of peripherals.
About request lines, they are muxed in our case between several
peripherals, but support for that has to be added in dw_dmac.

SPI is not as much heavily used as uart. So, it might be fine there to
allocate channels dynamically.

-- 
viresh

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Russell King - ARM Linux]

On Wed, Aug 10, 2011 at 03:44:13PM +0530, viresh kumar wrote:
> On 08/10/2011 03:31 PM, Koul, Vinod wrote:
> > And on your patch, are you able to dynamically assign the channels for
> > platform? What is the intended usage? (as Russell articulated it is bad
> > to dynamically assign channel for something like uart)
> 
> Are you talking about channels or DMA request lines? For channels yes,
> we can always allocate channels as they are independent of peripherals.
> About request lines, they are muxed in our case between several
> peripherals, but support for that has to be added in dw_dmac.

Right, and when you do, you'll probably have to go to a virtual channel
implementation, which solves the problem of keeping a channel allocated
and makes this patch redundant.

I assert that any DMA engine implementation where request signals can
be assigned dynamically to DMA channels should be using a virtual channel
implementation.

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Koul, Vinod]

On Wed, 2011-08-10 at 11:32 +0100, Russell King - ARM Linux wrote:
> On Wed, Aug 10, 2011 at 03:44:13PM +0530, viresh kumar wrote:
> > On 08/10/2011 03:31 PM, Koul, Vinod wrote:
> > > And on your patch, are you able to dynamically assign the channels for
> > > platform? What is the intended usage? (as Russell articulated it is bad
> > > to dynamically assign channel for something like uart)
> > 
> > Are you talking about channels or DMA request lines? For channels yes,
> > we can always allocate channels as they are independent of peripherals.
> > About request lines, they are muxed in our case between several
> > peripherals, but support for that has to be added in dw_dmac.
> 
> Right, and when you do, you'll probably have to go to a virtual channel
> implementation, which solves the problem of keeping a channel allocated
> and makes this patch redundant.
> 
> I assert that any DMA engine implementation where request signals can
> be assigned dynamically to DMA channels should be using a virtual channel
> implementation.
Agreed, virtual channels can ensure that channels can be shared
dynamically. If h/w has capability it should be able to do a spi
transfer followed by emmc transfer ans so forth...
Current model of giving client exclusive access to a channel doesn't
allow this

-- 
~Vinod

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Russell King - ARM Linux]

On Wed, Aug 10, 2011 at 03:31:42PM +0530, Koul, Vinod wrote:
> I would agree on both counts :)
> 
> In some cases it does make sense to hold the channel for the lifetime
> like uart or where the channel has been tied to an interface by SoC
> designer.
> But in some cases like dw_dmac it seems you can assign channels
> dynamically to each usage, and runtime allocation ensures we have best
> utilization.

If dw_dmac can assign channels dynamically at runtime to request signals,
it is no different from pl08x, where we have essentially the same
capability, and we do have the virtual channel support.

The virtual channel support is far more flexible than picking a physical
channel at allocation time, because it means you can reassign the
virtual channel at any point when a transfer is not in progress.  Plus
it means that you don't have to keep doing the channel allocation,
configuration and freeing on every transfer which would be hugely
wasteful.  Not to mention that it burdens peripheral drivers with
unnecessary additional complexity - which means additional bugs.

I would encourage all DMA engine drivers which have this capability to
switch to a virtual channel setup to ensure maximum interoperability
between different peripheral drivers.

I'd also suggest that we probably want to make the virtual layer a
library for DMA engine implementations to use.  We really don't want
every DMA engine implementation re-creating that support time and time
again.  I'll look into pulling the virtual channel stuff out of PL08x
over the next month or so.

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Jassi Brar]

On Wed, Aug 10, 2011 at 3:31 PM, Koul, Vinod <vinod.koul@intel.com> wrote:
> On Wed, 2011-08-10 at 14:59 +0530, viresh kumar wrote:
>> On 08/10/2011 02:30 PM, Russell King - ARM Linux wrote:
>> >> > They must be allocated when they are required and must be freed after we are
>> >> > done with transfers. So that they can be used by other users.
>> > Which DMA engine driver requires this?
>> >
>>
>> dw_dmac.c
>>
>> > Normally, when we have DMA engine drivers with multiple request signals,
>> > the slave peripheral side publishes several virtual channels which are
>> > claimed by the peripheral drivers. ?This (amongst other things) allows
>> > the peripheral drivers to hold claim to one of the virtual channels
>> > all the time that it's required.
>>
>> If users of DMA expect DMA engine drivers to work this way, then we should
>> have this mentioned clearly in DMA slave documentation.
>>
>> @Dan/Vinod: What do you say?
> I would agree on both counts :)
>
> In some cases it does make sense to hold the channel for the lifetime
> like uart or where the channel has been tied to an interface by SoC
> designer.
> But in some cases like dw_dmac it seems you can assign channels
> dynamically to each usage, and runtime allocation ensures we have best
> utilization.
> So i would argue that there is no "one size fits all" here, if you can
> manage channels dynamically and utilize more efficiently then go ahead,
> but if you cant (h/w and usage constraint) then you should not be forced
> to do so.

The idea is to have channel allocation as purely a s/w thing - no
actual commitment
of h/w resources. So we can afford to have channel allocated for the
whole lifetime
of a client.

Some dmac drivers are written 'improperly', keeping in mind the
platforms that have fixed
ReqSig->Peri map and no more clients than actual req-sigs are active
simultaneously.
But such dmac drivers will fail if a new platform decides to hijack req-signals.

So, imho, it is absolutely a good thing for every dmac driver to be
designed for re-routable
ReqSig->Peri map... which would force their design to allocate
virtual/software channels to clients
without commit much(any?) h/w resources.

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Russell King - ARM Linux]

On Wed, Aug 10, 2011 at 04:01:13PM +0530, Jassi Brar wrote:
> On Wed, Aug 10, 2011 at 3:31 PM, Koul, Vinod <vinod.koul@intel.com> wrote:
> > On Wed, 2011-08-10 at 14:59 +0530, viresh kumar wrote:
> >> On 08/10/2011 02:30 PM, Russell King - ARM Linux wrote:
> >> >> > They must be allocated when they are required and must be freed after we are
> >> >> > done with transfers. So that they can be used by other users.
> >> > Which DMA engine driver requires this?
> >> >
> >>
> >> dw_dmac.c
> >>
> >> > Normally, when we have DMA engine drivers with multiple request signals,
> >> > the slave peripheral side publishes several virtual channels which are
> >> > claimed by the peripheral drivers. ?This (amongst other things) allows
> >> > the peripheral drivers to hold claim to one of the virtual channels
> >> > all the time that it's required.
> >>
> >> If users of DMA expect DMA engine drivers to work this way, then we should
> >> have this mentioned clearly in DMA slave documentation.
> >>
> >> @Dan/Vinod: What do you say?
> > I would agree on both counts :)
> >
> > In some cases it does make sense to hold the channel for the lifetime
> > like uart or where the channel has been tied to an interface by SoC
> > designer.
> > But in some cases like dw_dmac it seems you can assign channels
> > dynamically to each usage, and runtime allocation ensures we have best
> > utilization.
> > So i would argue that there is no "one size fits all" here, if you can
> > manage channels dynamically and utilize more efficiently then go ahead,
> > but if you cant (h/w and usage constraint) then you should not be forced
> > to do so.
> 
> The idea is to have channel allocation as purely a s/w thing - no
> actual commitment
> of h/w resources. So we can afford to have channel allocated for the
> whole lifetime
> of a client.
> 
> Some dmac drivers are written 'improperly', keeping in mind the
> platforms that have fixed
> ReqSig->Peri map and no more clients than actual req-sigs are active
> simultaneously.
> But such dmac drivers will fail if a new platform decides to hijack req-signals.
> 
> So, imho, it is absolutely a good thing for every dmac driver to be
> designed for re-routable
> ReqSig->Peri map... which would force their design to allocate
> virtual/software channels to clients
> without commit much(any?) h/w resources.

We discussed channel allocation at Linaro.  However, I am now _really_
disappointed.

I discussed this with Linus on the bus back from Cambridge in the evening,
and it appears that the story you gave me was inaccurate - Linus had not
agreed to your proposal and saw more or less the same problems with it
which I've been on at you about via your other email alias/lkml.  So that's
essentially invalidated everything we discussed there as part of my thinking
was "if Linus is happy with it, then...".

I am now convinced that you'll say *anything* just to get your idea into
the kernel.

So, the stakes have now been raised further for you: what I want to see
from you is a _working_ _implementation_ for those three platforms which
I outlined.  I don't want more discussion.  I want patches from you.
Nothing else.  We'll then review the code changes themselves rather than
a vague idea communicated by email/verbally.

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Jassi Brar]

On Wed, Aug 10, 2011 at 4:10 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Aug 10, 2011 at 04:01:13PM +0530, Jassi Brar wrote:
>> On Wed, Aug 10, 2011 at 3:31 PM, Koul, Vinod <vinod.koul@intel.com> wrote:
>> > On Wed, 2011-08-10 at 14:59 +0530, viresh kumar wrote:
>> >> On 08/10/2011 02:30 PM, Russell King - ARM Linux wrote:
>> >> >> > They must be allocated when they are required and must be freed after we are
>> >> >> > done with transfers. So that they can be used by other users.
>> >> > Which DMA engine driver requires this?
>> >> >
>> >>
>> >> dw_dmac.c
>> >>
>> >> > Normally, when we have DMA engine drivers with multiple request signals,
>> >> > the slave peripheral side publishes several virtual channels which are
>> >> > claimed by the peripheral drivers. ?This (amongst other things) allows
>> >> > the peripheral drivers to hold claim to one of the virtual channels
>> >> > all the time that it's required.
>> >>
>> >> If users of DMA expect DMA engine drivers to work this way, then we should
>> >> have this mentioned clearly in DMA slave documentation.
>> >>
>> >> @Dan/Vinod: What do you say?
>> > I would agree on both counts :)
>> >
>> > In some cases it does make sense to hold the channel for the lifetime
>> > like uart or where the channel has been tied to an interface by SoC
>> > designer.
>> > But in some cases like dw_dmac it seems you can assign channels
>> > dynamically to each usage, and runtime allocation ensures we have best
>> > utilization.
>> > So i would argue that there is no "one size fits all" here, if you can
>> > manage channels dynamically and utilize more efficiently then go ahead,
>> > but if you cant (h/w and usage constraint) then you should not be forced
>> > to do so.
>>
>> The idea is to have channel allocation as purely a s/w thing - no
>> actual commitment
>> of h/w resources. So we can afford to have channel allocated for the
>> whole lifetime
>> of a client.
>>
>> Some dmac drivers are written 'improperly', keeping in mind the
>> platforms that have fixed
>> ReqSig->Peri map and no more clients than actual req-sigs are active
>> simultaneously.
>> But such dmac drivers will fail if a new platform decides to hijack req-signals.
>>
>> So, imho, it is absolutely a good thing for every dmac driver to be
>> designed for re-routable
>> ReqSig->Peri map... which would force their design to allocate
>> virtual/software channels to clients
>> without commit much(any?) h/w resources.
>
> We discussed channel allocation at Linaro. ?However, I am now _really_
> disappointed.
Honestly, I don't see how this deviates from my proposal and why you think
a dmac driver designed for fixed ReqSig->Peri map is future-proof (my that
very assertion made you really disappointed)!

All the PL080 platforms that I have worked, had a fixed map...and I am sure you
wouldn't have been happy looking at a dmac driver based upon that assumption.
Similarly for any other dmac driver developer, it makes sense to be safe by
assuming the dmac could have flexible map on some platform. And this precisely
why I said
{
 So, imho, it is absolutely a good thing for every dmac driver to be
 designed for re-routable ReqSig->Peri map... which would force their
design to allocate
 virtual/software channels to clients without commit much(any?) h/w resources.
}


> I discussed this with Linus on the bus back from Cambridge in the evening,
> and it appears that the story you gave me was inaccurate - Linus had not
> agreed to your proposal and saw more or less the same problems with it
> which I've been on at you about via your other email alias/lkml. ?So that's
> essentially invalidated everything we discussed there as part of my thinking
> was "if Linus is happy with it, then...".
IIRC, Linus W mainly opined to involve device pointer during channel selection.
Other than that I thought there was only ambiguity about implementation details.

Linus W, was there anything you said wouldn't work with the scheme ?
Please tell now on the record.

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Linus Walleij]

On Wed, Aug 10, 2011 at 1:24 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Wed, Aug 10, 2011 at 4:10 PM, Russell King - ARM Linux
>>
>> I discussed this with Linus on the bus back from Cambridge in the evening,
>> and it appears that the story you gave me was inaccurate - Linus had not
>> agreed to your proposal and saw more or less the same problems with it
>> which I've been on at you about via your other email alias/lkml. ?So that's
>> essentially invalidated everything we discussed there as part of my thinking
>> was "if Linus is happy with it, then...".
>
> IIRC, Linus W mainly opined to involve device pointer during channel selection.
> Other than that I thought there was only ambiguity about implementation details.

Bah now we have two "Linus oracles" in this world instead
of just one... haha :-)

> Linus W, was there anything you said wouldn't work with the scheme ?
> Please tell now on the record.

It would *work* but the current proposal is *not elegant* IMO.

Let me put it like this:

Yes, there is a problem with how all platforms have to pass in a
filter function and some opaque cookie for every driver to look up
its DMA channel.

You seem to want to address this problem, which is good.

I think your scheme passing in a device ID and instance tuple
e.g. REQ(MMC,2) would *work*, but I don't think it is a proper
solution and I would never ACK it, as I said.

The lookup of a device DMA channel should follow the
design pattern set by regulators and clocks. Which means
you use struct device * or alternatively a string (if the
instance is not available) to look it up, such that:

struct dma_slave_map {
   char name[16];
   struct device *dev;
   char dev_name[16];
   struct devive *dma_dev;
   char dma_dev_name[16];
};

struct dma_slave_map[] = {
  {
     .name = "MMC-RX",
     .dev_name = "mmc.0",
     .dma_dev_name = "pl08x.0",
  },
  {
     .name = "MMC-TX",
     .dev_name = "mmc.0",
     .dma_dev_name = "pl08x.0",
  },
  {
     .name = "SSP-RX",
     .dev_name = "pl022.0",
     .dma_dev_name = "pl08x.1",
  },
  ...
};

The dmaengine core should take this mapping, and
the device driver would only have to:

struct dma_chan *rxc;
struct dma_chan *txc;
struct device *dev;

rxc = dma_request_slave_channel(dev, "MMC-RX");
txc = dma_request_slave_channel(dev, "MMC-TX");

I also recognized that you needed a priority scheme and
a few other bits for the above, so struct dma_slave_map
may need a few more members, but the above would
sure solve all usecases we have today. mem2mem could
use the old request function and doesn't need anything
like this.

Pros: well established design pattern, channels tied to
devices, intuitive for anyone who used clock or
regulator frameworks.

If the majority is happy with this scheme I can even
try implementing it.

Yours,
Linus Walleij

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Jassi Brar]

On Wed, Aug 10, 2011 at 5:24 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Aug 10, 2011 at 1:24 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> On Wed, Aug 10, 2011 at 4:10 PM, Russell King - ARM Linux
>>>
>>> I discussed this with Linus on the bus back from Cambridge in the evening,
>>> and it appears that the story you gave me was inaccurate - Linus had not
>>> agreed to your proposal and saw more or less the same problems with it
>>> which I've been on at you about via your other email alias/lkml. ?So that's
>>> essentially invalidated everything we discussed there as part of my thinking
>>> was "if Linus is happy with it, then...".
>>
>> IIRC, Linus W mainly opined to involve device pointer during channel selection.
>> Other than that I thought there was only ambiguity about implementation details.
>
> Bah now we have two "Linus oracles" in this world instead
> of just one... haha :-)
>


>> Linus W, was there anything you said wouldn't work with the scheme ?
>> Please tell now on the record.
>
> It would *work* but the current proposal is *not elegant* IMO.

would *work*  -> You could find no case that the scheme wouldn't support.
*not elegant*  -> Your opinion. Let us see.


> I think your scheme passing in a device ID and instance tuple
> e.g. REQ(MMC,2) would *work*, but I don't think it is a proper
> solution and I would never ACK it, as I said.
As much as I would feel 'deprived' of your ACK, I am not yet changing
the implementation for that part.


> The lookup of a device DMA channel should follow the
> design pattern set by regulators and clocks.
Nopes. It depends upon the subsystem.
We should strive towards making this scheme as 'standalone' as
possible.
Client having to specify the device it wants a channel for, is a
waste - otherwise we don't fully get rid of platform assistance for
channel selection!
Also that way, a client is actually asking for a 'channel' rather than
only specifying its requirements to the API and the API has enough
information to return the appropriate channel(which is what I propose).

> Which means
> you use struct device * or alternatively a string (if the
> instance is not available) to look it up, such that:
>
> struct dma_slave_map {
> ? char name[16];
> ? struct device *dev;
> ? char dev_name[16];
> ? struct devive *dma_dev;
> ? char dma_dev_name[16];
> };
>
> struct dma_slave_map[] = {
> ?{
> ? ? .name = "MMC-RX",
> ? ? .dev_name = "mmc.0",
> ? ? .dma_dev_name = "pl08x.0",
> ?},
> ?{
> ? ? .name = "MMC-TX",
> ? ? .dev_name = "mmc.0",
> ? ? .dma_dev_name = "pl08x.0",
> ?},
> ?{
> ? ? .name = "SSP-RX",
> ? ? .dev_name = "pl022.0",
> ? ? .dma_dev_name = "pl08x.1",
> ?},

1) This is implementation detail.
2) Not a very good one at that.
     a) Just think how many bytes you take to specify a channel ?
         Mine takes less than 4 bytes for equivalent information.
     b) Think about the mess of string copy, compare etc, when we can
         simply extract and manipulate bit-fields from, say, u32?


>
> The dmaengine core should take this mapping, and
> the device driver would only have to:
>
> struct dma_chan *rxc;
> struct dma_chan *txc;
> struct device *dev;
>
> rxc = dma_request_slave_channel(dev, "MMC-RX");
> txc = dma_request_slave_channel(dev, "MMC-TX");
Absolutely "not-very-good" !
We can do without the 'dev' argument.
How does a client request duplex channel ?
   MMC-RXTX or MMC-TXRX or TXRX-MMC ?
Do you propose to implement a string parser in the core ?!
Not to mention how limited requirements this scheme could
express to the core.


> I also recognized that you needed a priority scheme and
> a few other bits for the above,
You didn't recognize. I told you. IIRC you suggested I should actually
sell that point!
The priority is an additional feature that easily helps a situation when
a peri could be reached from two different dmacs and the board can
prioritize an already busy dmac over the one that would only serve
this peripheral - to save power by keeping the idle dmac off.
Your string manipulation would only get messier if it had to support
that feature.

> If the majority is happy with this scheme I can even
> try implementing it.
Interesting! Only a few days ago you were seeing eternal sunshine in
filter-functions... and now you plan to implement my proposal by
replacing bit-fields with strings.
Frankly, I don't care much who, if anybody, implements it anymore.

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Vinod Koul]

On Wed, 2011-08-10 at 18:46 +0530, Jassi Brar wrote:
> On Wed, Aug 10, 2011 at 5:24 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Wed, Aug 10, 2011 at 1:24 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >> On Wed, Aug 10, 2011 at 4:10 PM, Russell King - ARM Linux
> >>>
> >>> I discussed this with Linus on the bus back from Cambridge in the evening,
> >>> and it appears that the story you gave me was inaccurate - Linus had not
> >>> agreed to your proposal and saw more or less the same problems with it
> >>> which I've been on at you about via your other email alias/lkml.  So that's
> >>> essentially invalidated everything we discussed there as part of my thinking
> >>> was "if Linus is happy with it, then...".
> >>
> >> IIRC, Linus W mainly opined to involve device pointer during channel selection.
> >> Other than that I thought there was only ambiguity about implementation details.
> >
> > Bah now we have two "Linus oracles" in this world instead
> > of just one... haha :-)
> >
> 
> 
> >> Linus W, was there anything you said wouldn't work with the scheme ?
> >> Please tell now on the record.
> >
> > It would *work* but the current proposal is *not elegant* IMO.
> 
> would *work*  -> You could find no case that the scheme wouldn't support.
> *not elegant*  -> Your opinion. Let us see.
> 
> 
> > I think your scheme passing in a device ID and instance tuple
> > e.g. REQ(MMC,2) would *work*, but I don't think it is a proper
> > solution and I would never ACK it, as I said.
> As much as I would feel 'deprived' of your ACK, I am not yet changing
> the implementation for that part.
> 
> 
> > The lookup of a device DMA channel should follow the
> > design pattern set by regulators and clocks.
> Nopes. It depends upon the subsystem.
> We should strive towards making this scheme as 'standalone' as
> possible.
> Client having to specify the device it wants a channel for, is a
> waste - otherwise we don't fully get rid of platform assistance for
> channel selection!
On one of the platforms I work on we have 3 DMACs, peripheral 1, 2 and 3
can only use channels from controller 1, and the peripherals 4, 5 from
controller 2 and so on. They _absolutely_ need channel from specific
controller, so above suits it :).
Btw all three controllers _have_exactly_same_caps_ so dma engine will
not see any difference in channels. They don't know which peripherals
are connected to them, that's platform information which this scheme
seems to be suited to...
Can you tell me how your scheme will work in this case?

> Also that way, a client is actually asking for a 'channel' rather than
> only specifying its requirements to the API and the API has enough
> information to return the appropriate channel(which is what I propose).
> 
> > Which means
> > you use struct device * or alternatively a string (if the
> > instance is not available) to look it up, such that:
> >
> > struct dma_slave_map {
> >   char name[16];
> >   struct device *dev;
> >   char dev_name[16];
> >   struct devive *dma_dev;
> >   char dma_dev_name[16];
> > };
> >
> > struct dma_slave_map[] = {
> >  {
> >     .name = "MMC-RX",
> >     .dev_name = "mmc.0",
> >     .dma_dev_name = "pl08x.0",
> >  },
> >  {
> >     .name = "MMC-TX",
> >     .dev_name = "mmc.0",
> >     .dma_dev_name = "pl08x.0",
> >  },
> >  {
> >     .name = "SSP-RX",
> >     .dev_name = "pl022.0",
> >     .dma_dev_name = "pl08x.1",
> >  },
> 
> 1) This is implementation detail.
> 2) Not a very good one at that.
>      a) Just think how many bytes you take to specify a channel ?
>          Mine takes less than 4 bytes for equivalent information.
>      b) Think about the mess of string copy, compare etc, when we can
>          simply extract and manipulate bit-fields from, say, u32?
> 
> 
> >
> > The dmaengine core should take this mapping, and
> > the device driver would only have to:
> >
> > struct dma_chan *rxc;
> > struct dma_chan *txc;
> > struct device *dev;
> >
> > rxc = dma_request_slave_channel(dev, "MMC-RX");
> > txc = dma_request_slave_channel(dev, "MMC-TX");
> Absolutely "not-very-good" !
> We can do without the 'dev' argument.
> How does a client request duplex channel ?
>    MMC-RXTX or MMC-TXRX or TXRX-MMC ?
> Do you propose to implement a string parser in the core ?!
> Not to mention how limited requirements this scheme could
> express to the core.
> 
> 
> > I also recognized that you needed a priority scheme and
> > a few other bits for the above,
> You didn't recognize. I told you. IIRC you suggested I should actually
> sell that point!
> The priority is an additional feature that easily helps a situation when
> a peri could be reached from two different dmacs and the board can
> prioritize an already busy dmac over the one that would only serve
> this peripheral - to save power by keeping the idle dmac off.
> Your string manipulation would only get messier if it had to support
> that feature.
> 
> > If the majority is happy with this scheme I can even
> > try implementing it.
> Interesting! Only a few days ago you were seeing eternal sunshine in
> filter-functions... and now you plan to implement my proposal by
> replacing bit-fields with strings.
> Frankly, I don't care much who, if anybody, implements it anymore.
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Jassi Brar]

On Thu, Aug 11, 2011 at 2:28 AM, Vinod Koul <vkoul@infradead.org> wrote:
>> > The lookup of a device DMA channel should follow the
>> > design pattern set by regulators and clocks.
>> Nopes. It depends upon the subsystem.
>> We should strive towards making this scheme as 'standalone' as
>> possible.
>> Client having to specify the device it wants a channel for, is a
>> waste - otherwise we don't fully get rid of platform assistance for
>> channel selection!
> On one of the platforms I work on we have 3 DMACs, peripheral 1, 2 and 3
> can only use channels from controller 1, and the peripherals 4, 5 from
> controller 2 and so on. They _absolutely_ need channel from specific
> controller, so above suits it :).
> Btw all three controllers _have_exactly_same_caps_ so dma engine will
> not see any difference in channels. They don't know which peripherals
> are connected to them, that's platform information which this scheme
> seems to be suited to...
> Can you tell me how your scheme will work in this case?
Actually the setup is quite simple.
Say, the Peri-4 is the UART which could only be reached from DMAC-2.

DMAENGINE should simply manage a pool of total channels in the platform,
disregarding the dmacs they come from.

Each channel in the common pool will have a 'capability-tag' of u32 type - which
specifies various capabilities of the channel expressed in bit-fields.
It is the job of
platform (via dmac driver) to set 'tag' of each channel appropriately.
Among others, a channel's reach to a particular device is a
'capability' (expressed
in 7-bits DEV_TYPE field-mask of the 'tag')

In your case, while setting up channels before adding them to the common pool,
the platform (via dmac driver) will ensure exactly the channel which
can reach UART
has set DEV_UART value in the DEV_TYPE field of its capability-tag.

After all the dmac instances have been probed, _exactly_ one channel in the pool
will have the 'UART' capability mentioned in the DEV_TYPE field.

The serial driver(client), will simply specify 'Ability to reach UART'
as one of its
requirements to the core - by setting the DEV_TYPE field with DEV_UART value
of the 'request-tag' (a u32 number, say).

While searching for appropriate channel for the serial client, the core will
iterate over the pool and, of course, only one channel will have DEV_TYPE
field set to DEV_UART - the one which platform decided!

Please have a look at the end of  https://lkml.org/lkml/2011/7/29/211
(esp the helpers) to get an idea of data structures involved.


*******************************
UART(client) driver snippet :-
*******************************
struct dma_chan *chan_rx;
u32 req_cap;

/* Reset the requirement list */
DCH_RESET_CAP(req_cap);

/* Specify ability to reach UART as a requirement */
DCH_REQCAP_DEV(req_cap, DCHAN_TODEV_UART);

/* Specify we require to Read from UART */
DCH_REQCAP_P2M(req_cap);

/* ...... specify other requirements */

/* Ask for the channel */
chan_rx = dma_request_channel(req_cap);



***************
dmaengine.c
***************
struct dma_chan *dma_request_channel(u32 req_cap)  // typedef u32 to
look nice ;)
{
 struct dma_chan *ch;

           /* !!! Not final implementation !!! */
 list_for_each_entry(ch, &channel_pool, chan_node) {
      if (GET_DEVTYPE(req_cap) != GET_DEVTYPE(ch->tag))
          continue;

      if ((IS_DCH_M2M(req_cap) && !IS_DCH_M2M(ch->tag))
         continue;

      if ((IS_DCH_M2P(req_cap) && !IS_DCH_M2P(ch->tag))
         continue;

      if ((IS_DCH_P2M(req_cap) && !IS_DCH_P2M(ch->tag))
         continue;

      if ((IS_DCH_P2P(req_cap) && !IS_DCH_P2P(ch->tag))
         continue;

      /* weed out channels that don't have further capabilities required */

      /* At the end of checks this channel meets every requirement */
      return ch;
  }
  /* Will never reach here in a bug-free platform */
  return NULL;
}


**************************************
DMAC <- PLATFORM <- BOARD
***************************************
Well, that is between dmac and platform and out of scope of DMAENGINE API.
They can decide upon any data structure to pass the info needed.

But@some point someone must set :-
  _Only_ for the channel of DMAC-2 that could talk to the UART.

{
DCH_RESET_CAP(ch->tag);

/* Declare 'capability' to talk to UART */
SET_DEVTYPE(ch->tag, DCHAN_TODEV_UART);

/* Declare 'capability' to do RX i.e, P->M */
SET_DCHDIR(ch->tag,  0, 0, 1, 0);

/* Declare other 'capabilities' of the channel */

/* Add enough dmac's private data to channel so as to be able
  to later figure out the ReqSig, DMAC etc associated with it */

/* Add the channel to the global pool in dmaengine.c */
}

Please have a look at the end of  https://lkml.org/lkml/2011/7/29/211
(esp the helpers) to get an idea of data structures involved.

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Koul, Vinod]

On Thu, 2011-08-11 at 00:29 +0530, Jassi Brar wrote:
> On Thu, Aug 11, 2011 at 2:28 AM, Vinod Koul <vkoul@infradead.org> wrote:
> >> > The lookup of a device DMA channel should follow the
> >> > design pattern set by regulators and clocks.
> >> Nopes. It depends upon the subsystem.
> >> We should strive towards making this scheme as 'standalone' as
> >> possible.
> >> Client having to specify the device it wants a channel for, is a
> >> waste - otherwise we don't fully get rid of platform assistance for
> >> channel selection!
> > On one of the platforms I work on we have 3 DMACs, peripheral 1, 2 and 3
> > can only use channels from controller 1, and the peripherals 4, 5 from
> > controller 2 and so on. They _absolutely_ need channel from specific
> > controller, so above suits it :).
> > Btw all three controllers _have_exactly_same_caps_ so dma engine will
> > not see any difference in channels. They don't know which peripherals
> > are connected to them, that's platform information which this scheme
> > seems to be suited to...
> > Can you tell me how your scheme will work in this case?
> Actually the setup is quite simple.
> Say, the Peri-4 is the UART which could only be reached from DMAC-2.
> 
> DMAENGINE should simply manage a pool of total channels in the platform,
> disregarding the dmacs they come from.
> 
> Each channel in the common pool will have a 'capability-tag' of u32 type - which
> specifies various capabilities of the channel expressed in bit-fields.
> It is the job of
> platform (via dmac driver) to set 'tag' of each channel appropriately.
> Among others, a channel's reach to a particular device is a
> 'capability' (expressed
> in 7-bits DEV_TYPE field-mask of the 'tag')
> 
> In your case, while setting up channels before adding them to the common pool,
> the platform (via dmac driver) will ensure exactly the channel which
> can reach UART
> has set DEV_UART value in the DEV_TYPE field of its capability-tag.
> 
> After all the dmac instances have been probed, _exactly_ one channel in the pool
> will have the 'UART' capability mentioned in the DEV_TYPE field.
> 
> The serial driver(client), will simply specify 'Ability to reach UART'
> as one of its
> requirements to the core - by setting the DEV_TYPE field with DEV_UART value
> of the 'request-tag' (a u32 number, say).
> 
> While searching for appropriate channel for the serial client, the core will
> iterate over the pool and, of course, only one channel will have DEV_TYPE
> field set to DEV_UART - the one which platform decided!
> 
> Please have a look at the end of  https://lkml.org/lkml/2011/7/29/211
> (esp the helpers) to get an idea of data structures involved.
> 
> 
> *******************************
> UART(client) driver snippet :-
> *******************************
> struct dma_chan *chan_rx;
> u32 req_cap;
> 
> /* Reset the requirement list */
> DCH_RESET_CAP(req_cap);
> 
> /* Specify ability to reach UART as a requirement */
> DCH_REQCAP_DEV(req_cap, DCHAN_TODEV_UART);
> 
> /* Specify we require to Read from UART */
> DCH_REQCAP_P2M(req_cap);
> 
> /* ...... specify other requirements */
> 
> /* Ask for the channel */
> chan_rx = dma_request_channel(req_cap);
> 
> 
> 
> ***************
> dmaengine.c
> ***************
> struct dma_chan *dma_request_channel(u32 req_cap)  // typedef u32 to
> look nice ;)
> {
>  struct dma_chan *ch;
> 
>            /* !!! Not final implementation !!! */
>  list_for_each_entry(ch, &channel_pool, chan_node) {
>       if (GET_DEVTYPE(req_cap) != GET_DEVTYPE(ch->tag))
>           continue;
> 
>       if ((IS_DCH_M2M(req_cap) && !IS_DCH_M2M(ch->tag))
>          continue;
> 
>       if ((IS_DCH_M2P(req_cap) && !IS_DCH_M2P(ch->tag))
>          continue;
> 
>       if ((IS_DCH_P2M(req_cap) && !IS_DCH_P2M(ch->tag))
>          continue;
> 
>       if ((IS_DCH_P2P(req_cap) && !IS_DCH_P2P(ch->tag))
>          continue;
> 
>       /* weed out channels that don't have further capabilities required */
> 
>       /* At the end of checks this channel meets every requirement */
>       return ch;
>   }
>   /* Will never reach here in a bug-free platform */
>   return NULL;
> }
> 
> 
> **************************************
> DMAC <- PLATFORM <- BOARD
> ***************************************
> Well, that is between dmac and platform and out of scope of DMAENGINE API.
> They can decide upon any data structure to pass the info needed.
> 
> But at some point someone must set :-
>   _Only_ for the channel of DMAC-2 that could talk to the UART.
That is why I am skeptical about this implementation approach. Here
CHAN_TODEV_UART is how peripheral is connected to DMAC which is a
platform capability which magically needs to be published by generic
DMAC driver and requested by UART driver...
Sorry I still don't get this schema and how it can be scaled and be
generic enough to let it carry with various implementations.

Can you publish your complete idea rather than bits and pieces...


-- 
~Vinod

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Jassi Brar]

On Tue, Aug 16, 2011 at 5:25 PM, Koul, Vinod <vinod.koul@intel.com> wrote:
>> >> > The lookup of a device DMA channel should follow the
>> >> > design pattern set by regulators and clocks.
>> >> Nopes. It depends upon the subsystem.
>> >> We should strive towards making this scheme as 'standalone' as
>> >> possible.
>> >> Client having to specify the device it wants a channel for, is a
>> >> waste - otherwise we don't fully get rid of platform assistance for
>> >> channel selection!
>> > On one of the platforms I work on we have 3 DMACs, peripheral 1, 2 and 3
>> > can only use channels from controller 1, and the peripherals 4, 5 from
>> > controller 2 and so on. They _absolutely_ need channel from specific
>> > controller, so above suits it :).
>> > Btw all three controllers _have_exactly_same_caps_ so dma engine will
>> > not see any difference in channels. They don't know which peripherals
>> > are connected to them, that's platform information which this scheme
>> > seems to be suited to...
>> > Can you tell me how your scheme will work in this case?
>> Actually the setup is quite simple.
>> Say, the Peri-4 is the UART which could only be reached from DMAC-2.
>>
>> DMAENGINE should simply manage a pool of total channels in the platform,
>> disregarding the dmacs they come from.
>>
>> Each channel in the common pool will have a 'capability-tag' of u32 type - which
>> specifies various capabilities of the channel expressed in bit-fields.
>> It is the job of
>> platform (via dmac driver) to set 'tag' of each channel appropriately.
>> Among others, a channel's reach to a particular device is a
>> 'capability' (expressed
>> in 7-bits DEV_TYPE field-mask of the 'tag')
>>
>> In your case, while setting up channels before adding them to the common pool,
>> the platform (via dmac driver) will ensure exactly the channel which
>> can reach UART
>> has set DEV_UART value in the DEV_TYPE field of its capability-tag.
>>
>> After all the dmac instances have been probed, _exactly_ one channel in the pool
>> will have the 'UART' capability mentioned in the DEV_TYPE field.
>>
>> The serial driver(client), will simply specify 'Ability to reach UART'
>> as one of its
>> requirements to the core - by setting the DEV_TYPE field with DEV_UART value
>> of the 'request-tag' (a u32 number, say).
>>
>> While searching for appropriate channel for the serial client, the core will
>> iterate over the pool and, of course, only one channel will have DEV_TYPE
>> field set to DEV_UART - the one which platform decided!
>>
>> Please have a look at the end of ?https://lkml.org/lkml/2011/7/29/211
>> (esp the helpers) to get an idea of data structures involved.
>>
>>
>> *******************************
>> UART(client) driver snippet :-
>> *******************************
>> struct dma_chan *chan_rx;
>> u32 req_cap;
>>
>> /* Reset the requirement list */
>> DCH_RESET_CAP(req_cap);
>>
>> /* Specify ability to reach UART as a requirement */
>> DCH_REQCAP_DEV(req_cap, DCHAN_TODEV_UART);
>>
>> /* Specify we require to Read from UART */
>> DCH_REQCAP_P2M(req_cap);
>>
>> /* ...... specify other requirements */
>>
>> /* Ask for the channel */
>> chan_rx = dma_request_channel(req_cap);
>>
>>
>>
>> ***************
>> dmaengine.c
>> ***************
>> struct dma_chan *dma_request_channel(u32 req_cap) ?// typedef u32 to
>> look nice ;)
>> {
>> ?struct dma_chan *ch;
>>
>> ? ? ? ? ? ?/* !!! Not final implementation !!! */
>> ?list_for_each_entry(ch, &channel_pool, chan_node) {
>> ? ? ? if (GET_DEVTYPE(req_cap) != GET_DEVTYPE(ch->tag))
>> ? ? ? ? ? continue;
>>
>> ? ? ? if ((IS_DCH_M2M(req_cap) && !IS_DCH_M2M(ch->tag))
>> ? ? ? ? ?continue;
>>
>> ? ? ? if ((IS_DCH_M2P(req_cap) && !IS_DCH_M2P(ch->tag))
>> ? ? ? ? ?continue;
>>
>> ? ? ? if ((IS_DCH_P2M(req_cap) && !IS_DCH_P2M(ch->tag))
>> ? ? ? ? ?continue;
>>
>> ? ? ? if ((IS_DCH_P2P(req_cap) && !IS_DCH_P2P(ch->tag))
>> ? ? ? ? ?continue;
>>
>> ? ? ? /* weed out channels that don't have further capabilities required */
>>
>> ? ? ? /* At the end of checks this channel meets every requirement */
>> ? ? ? return ch;
>> ? }
>> ? /* Will never reach here in a bug-free platform */
>> ? return NULL;
>> }
>>
>>
>> **************************************
>> DMAC <- PLATFORM <- BOARD
>> ***************************************
>> Well, that is between dmac and platform and out of scope of DMAENGINE API.
>> They can decide upon any data structure to pass the info needed.
>>
>> But at some point someone must set :-
>> ? _Only_ for the channel of DMAC-2 that could talk to the UART.
> That is why I am skeptical about this implementation approach. Here
> CHAN_TODEV_UART is how peripheral is connected to DMAC which is a
> platform capability which magically needs to be published by generic
> DMAC driver and requested by UART driver...
Not really. Set by generic dmac driver in conjunction with platform (or maybe
also board).... which would look at the type of controller it has and
the CHAN_TODEV_
its client driver requests.
S3C DMA API has been doing similar, albeit for fixed ReqSig->Peri map,
for quite some time now.

> Sorry I still don't get this schema and how it can be scaled and be
> generic enough to let it carry with various implementations.
>
> Can you publish your complete idea rather than bits and pieces...
I already explained the complete idea. I don't have any implementation yet.
Clients and dmaengine.c is easier to manage.
But changes to >20 dmac drivers is the biggest effort - though they anyway
need such modifications if we are to have the DMAENGINE utopia someday.
In free time, I will modify a dmac driver or two, but it might take
prohibitively
long if I am expected to update possibly all the 20 dmac drivers and the backend
platforms by
  a) Making dmac drivers platform agnostic and for re-routable ReqSig-Peri map.
       That implies dmac drivers managing 'virtual-channel' front end
and physical
       channel and ReqSig->Peri link management in the backend with
help from platform.
  b) Modifying platforms/boards to pass channel map and link re-routing callback
      pointers to generic dmac drivers.

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Koul, Vinod]

On Tue, 2011-08-16 at 20:21 +0530, Jassi Brar wrote:
> On Tue, Aug 16, 2011 at 5:25 PM, Koul, Vinod <vinod.koul@intel.com> wrote:

> 
> > Sorry I still don't get this schema and how it can be scaled and be
> > generic enough to let it carry with various implementations.
> >
> > Can you publish your complete idea rather than bits and pieces...
> I already explained the complete idea. I don't have any implementation yet.
> Clients and dmaengine.c is easier to manage.
> But changes to >20 dmac drivers is the biggest effort - though they anyway
> need such modifications if we are to have the DMAENGINE utopia someday.
> In free time, I will modify a dmac driver or two, but it might take
> prohibitively
> long if I am expected to update possibly all the 20 dmac drivers and the backend
> platforms by
It would help if you can send RFC of changes in one driver and dmaengine
changes. I want to get the dmaengine changes understood well and be able
to deal with all scenarios we have. Without complete code its rather
hard :(
We can do these changes to other drivers over a period of time, that can
be taken over a period of time and we can manage that, this part is
easy.
>   a) Making dmac drivers platform agnostic and for re-routable ReqSig-Peri map.
>        That implies dmac drivers managing 'virtual-channel' front end
> and physical
>        channel and ReqSig->Peri link management in the backend with
> help from platform.
>   b) Modifying platforms/boards to pass channel map and link re-routing callback
>       pointers to generic dmac drivers.


-- 
~Vinod

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Linus Walleij]

2011/8/10 Jassi Brar <jassisinghbrar@gmail.com>:
> On Wed, Aug 10, 2011 at 5:24 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> Linus W, was there anything you said wouldn't work with the scheme ?
>>> Please tell now on the record.
>>
>> It would *work* but the current proposal is *not elegant* IMO.
>
> would *work* ?-> You could find no case that the scheme wouldn't support.

Well there is the usecase where we have a lot of devices, but it is
true we don't have that kind of hardware around.

> *not elegant* ?-> Your opinion. Let us see.

Well, yeah, such is life.

> Client having to specify the device it wants a channel for, is a
> waste - otherwise we don't fully get rid of platform assistance for
> channel selection!

I think I saw you proposal define REQ(MMC,2) for example,
isn't that specifying the device?

>> rxc = dma_request_slave_channel(dev, "MMC-RX");
>> txc = dma_request_slave_channel(dev, "MMC-TX");
>
> Absolutely "not-very-good" !
> We can do without the 'dev' argument.

You still need to pass in something referring you back to
the device.

> How does a client request duplex channel ?

Currently the duplex channels using DMA-engine switch
direction of the channel at runtime with the config call.0
There is a "bidirectional" define in dmaengine.h but I
haven't figured out if that is useful for slave transfers
at all really.

> Do you propose to implement a string parser in the core ?!

Yes, the clock and regulator framework already does that.
But it is only used when you cannot pass in a struct device *
directly, like from device tree.

>> I also recognized that you needed a priority scheme and
>> a few other bits for the above,
>
> You didn't recognize. I told you. IIRC you suggested I should actually
> sell that point!

http://en.wiktionary.org/wiki/recognise

   1. (transitive) To match something or someone which one currently
perceives to a memory of some previous encounter with the same entity.
   2. (transitive) To acknowledge the existence or legality of
something.; treat as worthy of consideration or valid.

          The US and a number of EU countries are expected to
recognise Kosovo on Monday.

   3. (transitive) To acknowledge or consider as something.
   4. (transitive) To realise or discover the nature of something;
apprehend quality in; realise or admit that.
   5. (transitive) To give an award.

Sorry for spelling with "z".

>> If the majority is happy with this scheme I can even
>> try implementing it.
>
> Interesting! Only a few days ago you were seeing eternal sunshine in
> filter-functions... and now you plan to implement my proposal by
> replacing bit-fields with strings.

Yes your argument for a better channel-lookup functionality
are perfectly valid, that is what I mean when I say I recognize
it. We only disagree on how to implement it.

As for eternal sunshine, well, it rains here in Sweden today
so the kernel is a pretty sunny place comparatively.

Thanks,
Linus Walleij

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Jassi Brar]

On Thu, Aug 11, 2011 at 6:25 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 2011/8/10 Jassi Brar <jassisinghbrar@gmail.com>:
>> On Wed, Aug 10, 2011 at 5:24 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>> Linus W, was there anything you said wouldn't work with the scheme ?
>>>> Please tell now on the record.
>>>
>>> It would *work* but the current proposal is *not elegant* IMO.
>>
>> would *work* ?-> You could find no case that the scheme wouldn't support.
>
> Well there is the usecase where we have a lot of devices, but it is
> true we don't have that kind of hardware around.
I am pissed off by Russell's allegation that
{
Linus had not agreed to your proposal and saw more or less the same
problems with it which I've been on at you about via your other email
alias/lkml.
}
whereas what you say now, and when we discussed, does not repeat a single
concern of his!
You never say/said what 'problem' do you see.

Russell has been skeptical if my idea would work at all for his
versatile setups.
While you say it would *work* but you just want the idea implemented
using device pointer and strings !

Btw, do bring on any esoteric setup that you have in mind even if you never
expect to have it in real.

I am not married to my idea. I don't wanna push any further if it wouldn't work.

The biggest challenge I see is the huge modification needed. Not some
weird setup !

>> Client having to specify the device it wants a channel for, is a
>> waste - otherwise we don't fully get rid of platform assistance for
>> channel selection!
>
> I think I saw you proposal define REQ(MMC,2) for example,
> isn't that specifying the device?
Not using platform provided device pointers, but by using globally
defined values. (See my last reply to Vinod's setup
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-August/060860.html)

The notion of "must-use" device pointer sucks!
More so when we can have simpler and at least as good implementation
without using them !

Do you have any reason for using device pointer and strings, other
than just "because clock and regulator use them" ??

>>> rxc = dma_request_slave_channel(dev, "MMC-RX");
>>> txc = dma_request_slave_channel(dev, "MMC-TX");
>>
>> Absolutely "not-very-good" !
>> We can do without the 'dev' argument.
>
> You still need to pass in something referring you back to
> the device.
Nopes. As I said please see my reply to Vinod's setup.


>> Do you propose to implement a string parser in the core ?!
>
> Yes, the clock and regulator framework already does that.
> But it is only used when you cannot pass in a struct device *
> directly, like from device tree.
Dude, I have utter disrespect for using strings in a case such as
expressing requirements.
I have already explained how we can easily and in a _better_ way
do without them (again see my last reply to Vindo's setup).
Tell me 1 reason why using strings, in this case, would be better ?

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Linus Walleij]

2011/8/11 Jassi Brar <jassisinghbrar@gmail.com>:

> Do you have any reason for using device pointer and strings, other
> than just "because clock and regulator use them" ??

Basically no.

But I think these frameworks are very workable and proven
to work in practice. So I like them.

When setting up the platform the coder would to be aware
that there are totally orthogonal concepts, which IMO
makes things complicated.

>>> Do you propose to implement a string parser in the core ?!
>>
>> Yes, the clock and regulator framework already does that.
>> But it is only used when you cannot pass in a struct device *
>> directly, like from device tree.
>
> Dude, I have utter disrespect for using strings in a case such as
> expressing requirements.

It's mainly a strcmp(), and it's only comparing to the
well-established namespace that all devices have anyway,
due to the way the device model is done.

Basically when binding clocks or regulators it's:

struct device *dev;
strcmp(map_string, dev_name(dev));

By this time struct device exist of course, since
it's the device driver calling to get its clock/regulator.

dev_name() comes from <linux/device.h> and basically
takes the kobject name or an optional initilizer name
for the device. So the names are pretty static, you don't
need to parse them, just compare.

> I have already explained how we can easily and in a _better_ way
> do without them (again see my last reply to Vindo's setup).
> Tell me 1 reason why using strings, in this case, would be better ?

I have no other reasons than the above.

People like Russell (clkdevice) and Liam Girdwood (regulator)
who I know are smarter than me and have worked with
these subsystems for years choose that model, so I trust
their judgement.

Thanks,
Linus Walleij

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Jassi Brar]

On Thu, Aug 11, 2011 at 8:18 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 2011/8/11 Jassi Brar <jassisinghbrar@gmail.com>:
>
>> Do you have any reason for using device pointer and strings, other
>> than just "because clock and regulator use them" ??
>
> Basically no.

Dear, I am speechless !!
Best of luck.

>>>> Do you propose to implement a string parser in the core ?!
>>>
>>> Yes, the clock and regulator framework already does that.
>>> But it is only used when you cannot pass in a struct device *
>>> directly, like from device tree.
>>
>> Dude, I have utter disrespect for using strings in a case such as
>> expressing requirements.
>
> It's mainly a strcmp(), and it's only comparing to the
> well-established namespace that all devices have anyway,
> due to the way the device model is done.
>
> Basically when binding clocks or regulators it's:
>
> struct device *dev;
> strcmp(map_string, dev_name(dev));
>
> By this time struct device exist of course, since
> it's the device driver calling to get its clock/regulator.
>
> dev_name() comes from <linux/device.h> and basically
> takes the kobject name or an optional initilizer name
> for the device. So the names are pretty static, you don't
> need to parse them, just compare.
I think I know why and how clkdev and regulator use strings.
Here we are talking about dmac possibly expressing 8 parameters
as strings, not just 2.


>> I have already explained how we can easily and in a _better_ way
>> do without them (again see my last reply to Vindo's setup).
>> Tell me 1 reason why using strings, in this case, would be better ?
>
> I have no other reasons than the above.
>
> People like Russell (clkdevice) and Liam Girdwood (regulator)
> who I know are smarter than me and have worked with
> these subsystems for years choose that model, so I trust
> their judgement.
Let me provide you even more 'ammunition'
ASoC and USB-Gadget employs strings too
Though only Regulators, ASoC and CLKDEV(?) really _have-to_
employ strings.
USB-Gadget use them mainly for historical reasons - we can very well
replace that scheme with bit-fields as I propose.

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Koul, Vinod]

On Thu, 2011-08-11 at 16:48 +0200, Linus Walleij wrote:
> 2011/8/11 Jassi Brar <jassisinghbrar@gmail.com>:
> 
> > Do you have any reason for using device pointer and strings, other
> > than just "because clock and regulator use them" ??
> 
> Basically no.
> 
> But I think these frameworks are very workable and proven
> to work in practice. So I like them.
> 
> When setting up the platform the coder would to be aware
> that there are totally orthogonal concepts, which IMO
> makes things complicated.
> 
> >>> Do you propose to implement a string parser in the core ?!
> >>
> >> Yes, the clock and regulator framework already does that.
> >> But it is only used when you cannot pass in a struct device *
> >> directly, like from device tree.
> >
> > Dude, I have utter disrespect for using strings in a case such as
> > expressing requirements.
> 
> It's mainly a strcmp(), and it's only comparing to the
> well-established namespace that all devices have anyway,
> due to the way the device model is done.
> 
> Basically when binding clocks or regulators it's:
> 
> struct device *dev;
> strcmp(map_string, dev_name(dev));
> 
> By this time struct device exist of course, since
> it's the device driver calling to get its clock/regulator.
> 
> dev_name() comes from <linux/device.h> and basically
> takes the kobject name or an optional initilizer name
> for the device. So the names are pretty static, you don't
> need to parse them, just compare.
> 
> > I have already explained how we can easily and in a _better_ way
> > do without them (again see my last reply to Vindo's setup).
> > Tell me 1 reason why using strings, in this case, would be better ?
> 
> I have no other reasons than the above.
> 
> People like Russell (clkdevice) and Liam Girdwood (regulator)
> who I know are smarter than me and have worked with
> these subsystems for years choose that model, so I trust
> their judgement.
I would agree the overhead of using strings is negligible if you compare
the benefits it gives in usability and being easy for someone to
configure.
Jassi being samsung asoc maintainer should already agree to that :-)


-- 
~Vinod

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Jassi Brar]

On Wed, Aug 10, 2011 at 2:59 PM, viresh kumar <viresh.kumar@st.com> wrote:
> On 08/10/2011 02:30 PM, Russell King - ARM Linux wrote:
>>> > They must be allocated when they are required and must be freed after we are
>>> > done with transfers. So that they can be used by other users.
>> Which DMA engine driver requires this?
>>
>
> dw_dmac.c
>
>> Normally, when we have DMA engine drivers with multiple request signals,
>> the slave peripheral side publishes several virtual channels which are
>> claimed by the peripheral drivers. ?This (amongst other things) allows
>> the peripheral drivers to hold claim to one of the virtual channels
>> all the time that it's required.
>
> If users of DMA expect DMA engine drivers to work this way, then we should
> have this mentioned clearly in DMA slave documentation.

The requirement stems from the fact that most DMACs(esp third party) could be
made to reroute req-signals by the platform, it has not much to do with the API.
IMO, all dmac drivers should be implemented that way to be on the safer side.

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Russell King - ARM Linux]

On Wed, Aug 10, 2011 at 03:39:28PM +0530, Jassi Brar wrote:
> On Wed, Aug 10, 2011 at 2:59 PM, viresh kumar <viresh.kumar@st.com> wrote:
> > On 08/10/2011 02:30 PM, Russell King - ARM Linux wrote:
> >>> > They must be allocated when they are required and must be freed after we are
> >>> > done with transfers. So that they can be used by other users.
> >> Which DMA engine driver requires this?
> >>
> >
> > dw_dmac.c
> >
> >> Normally, when we have DMA engine drivers with multiple request signals,
> >> the slave peripheral side publishes several virtual channels which are
> >> claimed by the peripheral drivers. ?This (amongst other things) allows
> >> the peripheral drivers to hold claim to one of the virtual channels
> >> all the time that it's required.
> >
> > If users of DMA expect DMA engine drivers to work this way, then we should
> > have this mentioned clearly in DMA slave documentation.
> 
> The requirement stems from the fact that most DMACs(esp third party) could be
> made to reroute req-signals by the platform, it has not much to do with the API.
> IMO, all dmac drivers should be implemented that way to be on the safer side.

No it isn't.  It's to do with how the physical channels are used.

[PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.

[Jassi Brar]

On Wed, Aug 10, 2011 at 4:00 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Aug 10, 2011 at 03:39:28PM +0530, Jassi Brar wrote:
>> On Wed, Aug 10, 2011 at 2:59 PM, viresh kumar <viresh.kumar@st.com> wrote:
>> > On 08/10/2011 02:30 PM, Russell King - ARM Linux wrote:
>> >>> > They must be allocated when they are required and must be freed after we are
>> >>> > done with transfers. So that they can be used by other users.
>> >> Which DMA engine driver requires this?
>> >>
>> >
>> > dw_dmac.c
>> >
>> >> Normally, when we have DMA engine drivers with multiple request signals,
>> >> the slave peripheral side publishes several virtual channels which are
>> >> claimed by the peripheral drivers. ?This (amongst other things) allows
>> >> the peripheral drivers to hold claim to one of the virtual channels
>> >> all the time that it's required.
>> >
>> > If users of DMA expect DMA engine drivers to work this way, then we should
>> > have this mentioned clearly in DMA slave documentation.
>>
>> The requirement stems from the fact that most DMACs(esp third party) could be
>> made to reroute req-signals by the platform, it has not much to do with the API.
>> IMO, all dmac drivers should be implemented that way to be on the safer side.
>
> No it isn't. ?It's to do with how the physical channels are used.
IMO, a dmac driver developer sees only two aspects - "virtual channel"
at frontend and
"ReqSig/PhyChan" management at the backend.  While ReqSig and Physical Channels
are different h/w entities, the driver developer usually works having
tied them together.

For ex, PL330 has 8 physical channels(ARM calls them threads) and 32
peripheral interfaces(ReqSig),
where as the dmac driver freely allocates 32 virtual channels and
keeps in mind that only 8 peripheral
interfaces can be active at any time.

So I am unable to see how I said something different that you do.

[PATCH V3 3/6] spi/spi-pl022: Don't allocate more sg than required.

[Viresh Kumar]

In routine configure_dma(), if transfer->len = PAGE_SIZE, then pages is one more
than required. While leads to one more sg getting allocated.

This is wrong. Correct this to allocate correct number of sg.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-pl022.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 80116be..832361e3a 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -1016,7 +1016,7 @@ static int configure_dma(struct pl022 *pl022)
 	dmaengine_slave_config(txchan, &tx_conf);
 
 	/* Create sglists for the transfers */
-	pages = (pl022->cur_transfer->len >> PAGE_SHIFT) + 1;
+	pages = DIV_ROUND_UP(pl022->cur_transfer->len, PAGE_SIZE);
 	dev_dbg(&pl022->adev->dev, "using %d pages for transfer\n", pages);
 
 	ret = sg_alloc_table(&pl022->sgt_rx, pages, GFP_ATOMIC);
-- 
1.7.2.2

[PATCH V2 0/6] spi/spi-pl022 fixes

[Viresh Kumar]

On 8/10/2011 2:20 PM, Viresh KUMAR wrote:
> Hi Grant,
> 
> I have added Tested-by: Linus Walleij <linus.walleij@linaro.org> on all patches.
> 
> This patchset mainly covers following fixes:
> - formatting related issues
> - Passing GFP_ATOMIC for sg allocation from tasklet
> - Fixing calculate_effective_freq() routine
> - Allocate/free DMA channels as and when required.
> 

Grant,

Did you already pushed this patchset?
There are few concerns on PATCH 6/6 and i am not sure if there are any decisions
on it.
@Linus: What do you say?

So, probably you can push first 5 patches for now.

--
viresh

[PATCH V2 0/6] spi/spi-pl022 fixes

[Linus Walleij]

On Thu, Sep 1, 2011 at 12:04 PM, Viresh Kumar <viresh.kumar@st.com> wrote:
> On 8/10/2011 2:20 PM, Viresh KUMAR wrote:
>>
>> This patchset mainly covers following fixes:
>> - formatting related issues
>> - Passing GFP_ATOMIC for sg allocation from tasklet
>> - Fixing calculate_effective_freq() routine
>> - Allocate/free DMA channels as and when required.
>>
>
> Grant,
>
> Did you already pushed this patchset?
> There are few concerns on PATCH 6/6 and i am not sure if there are any decisions
> on it.
> @Linus: What do you say?

I agree. Acked-by: Linus Walleij <linus.walleij@linaro.org> for
all except 6/6, it's probably just as possible to keep the channel handle
taken in the driver if we modify the affected DMA driver instead.
This would be better for performance I think.

Thanks,
Linus Walleij

[PATCH V2 0/6] spi/spi-pl022 fixes

[Viresh Kumar]

On 9/1/2011 4:26 PM, Linus Walleij wrote:
> I agree. Acked-by: Linus Walleij <linus.walleij@linaro.org> for
> all except 6/6, it's probably just as possible to keep the channel handle
> taken in the driver if we modify the affected DMA driver instead.
> This would be better for performance I think.

Hi Grant,

Any updates on this?

-- 
viresh

[PATCH V2 0/6] spi/spi-pl022 fixes

[Grant Likely]

On Tue, Sep 20, 2011 at 04:46:44PM +0530, Viresh Kumar wrote:
> On 9/1/2011 4:26 PM, Linus Walleij wrote:
> > I agree. Acked-by: Linus Walleij <linus.walleij@linaro.org> for
> > all except 6/6, it's probably just as possible to keep the channel handle
> > taken in the driver if we modify the affected DMA driver instead.
> > This would be better for performance I think.
> 
> Hi Grant,
> 
> Any updates on this?

Applied 1-5, thanks.

g.

> 
> -- 
> viresh