[PATCH v2 0/5] bam dma fixes and one dt extension

[PATCH v2 0/5] bam dma fixes and one dt extension

[Stanimir Varbanov]

resend and bump to v2 with fowolling changes:
 - fixed spelling mistaikes in 3/5
 - created a one more patch which renames a define from
   BAM_MAX_DATA_SIZE to BAM_FIFO_SIZE

regards,
Stan

v1 can be found at:

https://lkml.org/lkml/2015/12/1/113 

Stanimir Varbanov (5):
  dmaengine: qcom: bam_dma: fix dma free memory on remove
  dmaengine: qcom: bam_dma: clear BAM interrupt only if it is rised
  dmaengine: qcom: bam_dma: add controlled remotely dt property
  dmaengine: qcom: bam_dma: use correct pipe FIFO size
  dmaengine: qcom: bam_dma: rename BAM_MAX_DATA_SIZE define

 .../devicetree/bindings/dma/qcom_bam_dma.txt       |  2 ++
 drivers/dma/qcom/bam_dma.c                         | 38 +++++++++++++++-------
 2 files changed, 28 insertions(+), 12 deletions(-)

-- 
1.9.1

[PATCH v2 1/5] dmaengine: qcom: bam_dma: fix dma free memory on remove

[Stanimir Varbanov]

Building the driver as a module and when removing the already
inserted module gives below:

[ 1389.392788] Unable to handle kernel paging request at virtual address ffffffbdc000001c
[ 1389.421321] pgd = ffffffc02fa87000
[ 1389.447899] [ffffffbdc000001c] *pgd=0000000000000000, *pud=0000000000000000
[ 1389.460142] Internal error: Oops: 96000006 [#1] PREEMPT SMP
[ 1389.466963] Modules linked in: qcom_bam_dma(-)
[ 1389.486608] CPU: 2 PID: 2442 Comm: rmmod Not tainted 4.2.0+ #407
[ 1389.493885] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
[ 1389.501196] task: ffffffc035bae2c0 ti: ffffffc0368a8000 task.ti: ffffffc0368a8000
[ 1389.508566] PC is at __free_pages+0xc/0x40
[ 1389.515893] LR is at free_pages.part.93+0x30/0x38
[ 1389.523141] pc : [<ffffffc00016180c>] lr : [<ffffffc00016197c>] pstate: 80000145
[ 1389.530602] sp : ffffffc0368abc20
[ 1389.537931] x29: ffffffc0368abc20 x28: ffffffc0368a8000
[ 1389.549153] x27: 0000000000000000 x26: 0000000000000000
[ 1389.560412] x25: ffffffc000cb2000 x24: 0000000000000170
[ 1389.571530] x23: 0000000000000004 x22: ffffffc036bc5010
[ 1389.582721] x21: ffffffc036bc5010 x20: 0000000000000000
[ 1389.593981] x19: 0000000000000002 x18: 0000007fcbc8e8b0
[ 1389.605301] x17: 0000007f9b8226ec x16: ffffffc0002089e8
[ 1389.616647] x15: 0000007f9b8a0588 x14: 0ffffffffffffffc
[ 1389.628039] x13: 0000000000000030 x12: 0000000000000000
[ 1389.639436] x11: 0000000000000008 x10: ffffffc000ecc000
[ 1389.650872] x9 : ffffffc035bae2c0 x8 : ffffffc035bae9a8
[ 1389.662367] x7 : ffffffc035bae9a0 x6 : 0000000000000000
[ 1389.673906] x5 : ffffffbdc000001c x4 : 0000000080000000
[ 1389.685475] x3 : ffffffbdc0000000 x2 : 0000004080000000
[ 1389.697049] x1 : 0000000000000003 x0 : ffffffbdc0000000

The memory has been already freed by bam_free_chan() so fix this
by skiping already freed memory.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/dma/qcom/bam_dma.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index d5e0a9c3ad5d..a486bc0f82e0 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -1234,6 +1234,9 @@ static int bam_dma_remove(struct platform_device *pdev)
 		bam_dma_terminate_all(&bdev->channels[i].vc.chan);
 		tasklet_kill(&bdev->channels[i].vc.task);
 
+		if (!bdev->channels[i].fifo_virt)
+			continue;
+
 		dma_free_wc(bdev->dev, BAM_DESC_FIFO_SIZE,
 			    bdev->channels[i].fifo_virt,
 			    bdev->channels[i].fifo_phys);
-- 
1.9.1

[PATCH v2 2/5] dmaengine: qcom: bam_dma: clear BAM interrupt only if it is rised

[Stanimir Varbanov]

Currently we write BAM_IRQ_CLR register with zero even when no
BAM_IRQ occured. This write has some bad side effects when the
BAM instance is for the crypto engine. In case of crypto engine
some of the BAM registers are xPU protected and they cannot be
controlled by the driver.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
Reviewed-by: Andy Gross <andy.gross@linaro.org>
---
 drivers/dma/qcom/bam_dma.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index a486bc0f82e0..789d5f836bf7 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -801,13 +801,17 @@ static irqreturn_t bam_dma_irq(int irq, void *data)
 	if (srcs & P_IRQ)
 		tasklet_schedule(&bdev->task);
 
-	if (srcs & BAM_IRQ)
+	if (srcs & BAM_IRQ) {
 		clr_mask = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_STTS));
 
-	/* don't allow reorder of the various accesses to the BAM registers */
-	mb();
+		/*
+		 * don't allow reorder of the various accesses to the BAM
+		 * registers
+		 */
+		mb();
 
-	writel_relaxed(clr_mask, bam_addr(bdev, 0, BAM_IRQ_CLR));
+		writel_relaxed(clr_mask, bam_addr(bdev, 0, BAM_IRQ_CLR));
+	}
 
 	return IRQ_HANDLED;
 }
-- 
1.9.1

Re: [PATCH v2 2/5] dmaengine: qcom: bam_dma: clear BAM interrupt only if it is rised

[Vinod Koul]

On Wed, Apr 06, 2016 at 01:56:19AM +0300, Stanimir Varbanov wrote:

s/rised/raised ?

> Currently we write BAM_IRQ_CLR register with zero even when no
> BAM_IRQ occured. This write has some bad side effects when the
> BAM instance is for the crypto engine. In case of crypto engine
> some of the BAM registers are xPU protected and they cannot be
> controlled by the driver.

-- 
~Vinod

Re: [PATCH v2 2/5] dmaengine: qcom: bam_dma: clear BAM interrupt only if it is rised

[gpramod]

On 2016-04-06 04:26, Stanimir Varbanov wrote:
> Currently we write BAM_IRQ_CLR register with zero even when no
> BAM_IRQ occured. This write has some bad side effects when the
> BAM instance is for the crypto engine. In case of crypto engine
> some of the BAM registers are xPU protected and they cannot be
> controlled by the driver.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> Reviewed-by: Andy Gross <andy.gross@linaro.org>
> ---
>  drivers/dma/qcom/bam_dma.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 

Tested-by: Pramod Gurav <gpramod@codeaurora.org>

> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index a486bc0f82e0..789d5f836bf7 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -801,13 +801,17 @@ static irqreturn_t bam_dma_irq(int irq, void 
> *data)
>  	if (srcs & P_IRQ)
>  		tasklet_schedule(&bdev->task);
> 
> -	if (srcs & BAM_IRQ)
> +	if (srcs & BAM_IRQ) {
>  		clr_mask = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_STTS));
> 
> -	/* don't allow reorder of the various accesses to the BAM registers 
> */
> -	mb();
> +		/*
> +		 * don't allow reorder of the various accesses to the BAM
> +		 * registers
> +		 */
> +		mb();
> 
> -	writel_relaxed(clr_mask, bam_addr(bdev, 0, BAM_IRQ_CLR));
> +		writel_relaxed(clr_mask, bam_addr(bdev, 0, BAM_IRQ_CLR));
> +	}
> 
>  	return IRQ_HANDLED;
>  }

[PATCH v2 3/5] dmaengine: qcom: bam_dma: add controlled remotely dt property

[Stanimir Varbanov]

Some of the peripherals has bam which is controlled by remote
processor, thus the bam dma driver must avoid register writes
which initialise bam hw block. Those registers are protected
from xPU block and any writes to them will lead to secure
violation and system reboot.

Adding the contolled_remotely flag in bam driver to avoid
not permitted register writes in bam_init function.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
Reviewed-by: Andy Gross <andy.gross@linaro.org>
---
 Documentation/devicetree/bindings/dma/qcom_bam_dma.txt | 2 ++
 drivers/dma/qcom/bam_dma.c                             | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
index 1c9d48ea4914..9cbf5d9df8fd 100644
--- a/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
+++ b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
@@ -13,6 +13,8 @@ Required properties:
 - clock-names: must contain "bam_clk" entry
 - qcom,ee : indicates the active Execution Environment identifier (0-7) used in
   the secure world.
+- qcom,controlled-remotely : optional, indicates that the bam is controlled by
+  remote proccessor i.e. execution environment.
 
 Example:
 
diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 789d5f836bf7..d0f878a78fae 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -387,6 +387,7 @@ struct bam_device {
 
 	/* execution environment ID, from DT */
 	u32 ee;
+	bool controlled_remotely;
 
 	const struct reg_offset_data *layout;
 
@@ -1042,6 +1043,9 @@ static int bam_init(struct bam_device *bdev)
 	val = readl_relaxed(bam_addr(bdev, 0, BAM_NUM_PIPES));
 	bdev->num_channels = val & BAM_NUM_PIPES_MASK;
 
+	if (bdev->controlled_remotely)
+		return 0;
+
 	/* s/w reset bam */
 	/* after reset all pipes are disabled and idle */
 	val = readl_relaxed(bam_addr(bdev, 0, BAM_CTRL));
@@ -1129,6 +1133,9 @@ static int bam_dma_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	bdev->controlled_remotely = of_property_read_bool(pdev->dev.of_node,
+						"qcom,controlled-remotely");
+
 	bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk");
 	if (IS_ERR(bdev->bamclk))
 		return PTR_ERR(bdev->bamclk);
-- 
1.9.1

Re: [PATCH v2 3/5] dmaengine: qcom: bam_dma: add controlled remotely dt property

[Vinod Koul]

On Wed, Apr 06, 2016 at 01:56:20AM +0300, Stanimir Varbanov wrote:
>  Documentation/devicetree/bindings/dma/qcom_bam_dma.txt | 2 ++
>  drivers/dma/qcom/bam_dma.c                             | 7 +++++++

The binding should be a separate patch..

>  
> +	bdev->controlled_remotely = of_property_read_bool(pdev->dev.of_node,
> +						"qcom,controlled-remotely");
> +

we need some defaults here, how will this work with boards withe older DT.

-- 
~Vinod

Re: [PATCH v2 3/5] dmaengine: qcom: bam_dma: add controlled remotely dt property

[Andy Gross]

On Tue, Apr 05, 2016 at 04:44:25PM -0700, Vinod Koul wrote:
> On Wed, Apr 06, 2016 at 01:56:20AM +0300, Stanimir Varbanov wrote:
> >  Documentation/devicetree/bindings/dma/qcom_bam_dma.txt | 2 ++
> >  drivers/dma/qcom/bam_dma.c                             | 7 +++++++
> 
> The binding should be a separate patch..
> 
> >  
> > +	bdev->controlled_remotely = of_property_read_bool(pdev->dev.of_node,
> > +						"qcom,controlled-remotely");
> > +
> 
> we need some defaults here, how will this work with boards withe older DT.

By default the bam driver should assume that it can access the registers.  It is
only in specific cases where we have trustzone locking down these registers and
assigning them to other remote processors.

Re: [PATCH v2 3/5] dmaengine: qcom: bam_dma: add controlled remotely dt property

[Stanimir Varbanov]

On 04/06/2016 02:44 AM, Vinod Koul wrote:
> On Wed, Apr 06, 2016 at 01:56:20AM +0300, Stanimir Varbanov wrote:
>>  Documentation/devicetree/bindings/dma/qcom_bam_dma.txt | 2 ++
>>  drivers/dma/qcom/bam_dma.c                             | 7 +++++++
> 
> The binding should be a separate patch..

I'm not sure, isn't this rule valid only when we introduce the binding
document? But if you insist I can make a separate patch.

> 
>>  
>> +	bdev->controlled_remotely = of_property_read_bool(pdev->dev.of_node,
>> +						"qcom,controlled-remotely");
>> +
> 
> we need some defaults here, how will this work with boards withe older DT.
> 

if the introduced property missing from DT node the driver behavior is
preserved.

-- 
regards,
Stan

Re: [PATCH v2 3/5] dmaengine: qcom: bam_dma: add controlled remotely dt property

[Rob Herring]

On Wed, Apr 06, 2016 at 01:56:20AM +0300, Stanimir Varbanov wrote:
> Some of the peripherals has bam which is controlled by remote
> processor, thus the bam dma driver must avoid register writes
> which initialise bam hw block. Those registers are protected
> from xPU block and any writes to them will lead to secure
> violation and system reboot.
> 
> Adding the contolled_remotely flag in bam driver to avoid
> not permitted register writes in bam_init function.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> Reviewed-by: Andy Gross <andy.gross@linaro.org>
> ---
>  Documentation/devicetree/bindings/dma/qcom_bam_dma.txt | 2 ++
>  drivers/dma/qcom/bam_dma.c                             | 7 +++++++
>  2 files changed, 9 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

On being a separate patch, yes that is always the preference. However, 
this change is so small it doesn't matter to me. But this is going in 
thru Vinod's tree, so it is his call.

Rob

Re: [PATCH v2 3/5] dmaengine: qcom: bam_dma: add controlled remotely dt property

[gpramod]

On 2016-04-06 04:26, Stanimir Varbanov wrote:
> Some of the peripherals has bam which is controlled by remote
> processor, thus the bam dma driver must avoid register writes
> which initialise bam hw block. Those registers are protected
> from xPU block and any writes to them will lead to secure
> violation and system reboot.
> 
> Adding the contolled_remotely flag in bam driver to avoid
> not permitted register writes in bam_init function.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> Reviewed-by: Andy Gross <andy.gross@linaro.org>
> ---
>  Documentation/devicetree/bindings/dma/qcom_bam_dma.txt | 2 ++
>  drivers/dma/qcom/bam_dma.c                             | 7 +++++++
>  2 files changed, 9 insertions(+)
> 

Tested-by: Pramod Gurav <gpramod@codeaurora.org>

> diff --git a/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
> b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
> index 1c9d48ea4914..9cbf5d9df8fd 100644
> --- a/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
> +++ b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
> @@ -13,6 +13,8 @@ Required properties:
>  - clock-names: must contain "bam_clk" entry
>  - qcom,ee : indicates the active Execution Environment identifier 
> (0-7) used in
>    the secure world.
> +- qcom,controlled-remotely : optional, indicates that the bam is 
> controlled by
> +  remote proccessor i.e. execution environment.
> 
>  Example:
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 789d5f836bf7..d0f878a78fae 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -387,6 +387,7 @@ struct bam_device {
> 
>  	/* execution environment ID, from DT */
>  	u32 ee;
> +	bool controlled_remotely;
> 
>  	const struct reg_offset_data *layout;
> 
> @@ -1042,6 +1043,9 @@ static int bam_init(struct bam_device *bdev)
>  	val = readl_relaxed(bam_addr(bdev, 0, BAM_NUM_PIPES));
>  	bdev->num_channels = val & BAM_NUM_PIPES_MASK;
> 
> +	if (bdev->controlled_remotely)
> +		return 0;
> +
>  	/* s/w reset bam */
>  	/* after reset all pipes are disabled and idle */
>  	val = readl_relaxed(bam_addr(bdev, 0, BAM_CTRL));
> @@ -1129,6 +1133,9 @@ static int bam_dma_probe(struct platform_device 
> *pdev)
>  		return ret;
>  	}
> 
> +	bdev->controlled_remotely = of_property_read_bool(pdev->dev.of_node,
> +						"qcom,controlled-remotely");
> +
>  	bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk");
>  	if (IS_ERR(bdev->bamclk))
>  		return PTR_ERR(bdev->bamclk);

[PATCH v2 4/5] dmaengine: qcom: bam_dma: use correct pipe FIFO size

[Stanimir Varbanov]

The pipe fifo size register must instruct the bam hw
how many hw descriptors can be pushed to fifo. Currently
we isntruct the hw with 32KBytes but wrap the tail in
bam_start_dma in BAM_P_EVNT_REG on 4095 i.e. 32760. This
leads to stalled transactions when the tail wraps.

Fix this by use the correct fifo size in BAM_P_FIFO_SIZES
register i.e. 32K - 8.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/dma/qcom/bam_dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index d0f878a78fae..7e5ad1c25e21 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -459,7 +459,7 @@ static void bam_chan_init_hw(struct bam_chan *bchan,
 	 */
 	writel_relaxed(ALIGN(bchan->fifo_phys, sizeof(struct bam_desc_hw)),
 			bam_addr(bdev, bchan->id, BAM_P_DESC_FIFO_ADDR));
-	writel_relaxed(BAM_DESC_FIFO_SIZE,
+	writel_relaxed(BAM_MAX_DATA_SIZE,
 			bam_addr(bdev, bchan->id, BAM_P_FIFO_SIZES));
 
 	/* enable the per pipe interrupts, enable EOT, ERR, and INT irqs */
-- 
1.9.1

Re: [PATCH v2 4/5] dmaengine: qcom: bam_dma: use correct pipe FIFO size

[Vinod Koul]

On Wed, Apr 06, 2016 at 01:56:21AM +0300, Stanimir Varbanov wrote:
> The pipe fifo size register must instruct the bam hw
> how many hw descriptors can be pushed to fifo. Currently
> we isntruct the hw with 32KBytes but wrap the tail in
    ^^^^^^^^^
typo 

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/5] dmaengine: qcom: bam_dma: rename BAM_MAX_DATA_SIZE define

[Stanimir Varbanov]

It seems that the define has not been with acurate name and
makes confusion while reading the code. The more acurate
name should be BAM_FIFO_SIZE.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/dma/qcom/bam_dma.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 7e5ad1c25e21..969b48176745 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -342,7 +342,7 @@ static const struct reg_offset_data bam_v1_7_reg_info[] = {
 
 #define BAM_DESC_FIFO_SIZE	SZ_32K
 #define MAX_DESCRIPTORS (BAM_DESC_FIFO_SIZE / sizeof(struct bam_desc_hw) - 1)
-#define BAM_MAX_DATA_SIZE	(SZ_32K - 8)
+#define BAM_FIFO_SIZE	(SZ_32K - 8)
 
 struct bam_chan {
 	struct virt_dma_chan vc;
@@ -459,7 +459,7 @@ static void bam_chan_init_hw(struct bam_chan *bchan,
 	 */
 	writel_relaxed(ALIGN(bchan->fifo_phys, sizeof(struct bam_desc_hw)),
 			bam_addr(bdev, bchan->id, BAM_P_DESC_FIFO_ADDR));
-	writel_relaxed(BAM_MAX_DATA_SIZE,
+	writel_relaxed(BAM_FIFO_SIZE,
 			bam_addr(bdev, bchan->id, BAM_P_FIFO_SIZES));
 
 	/* enable the per pipe interrupts, enable EOT, ERR, and INT irqs */
@@ -605,7 +605,7 @@ static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
 
 	/* calculate number of required entries */
 	for_each_sg(sgl, sg, sg_len, i)
-		num_alloc += DIV_ROUND_UP(sg_dma_len(sg), BAM_MAX_DATA_SIZE);
+		num_alloc += DIV_ROUND_UP(sg_dma_len(sg), BAM_FIFO_SIZE);
 
 	/* allocate enough room to accomodate the number of entries */
 	async_desc = kzalloc(sizeof(*async_desc) +
@@ -636,10 +636,10 @@ static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
 			desc->addr = cpu_to_le32(sg_dma_address(sg) +
 						 curr_offset);
 
-			if (remainder > BAM_MAX_DATA_SIZE) {
-				desc->size = cpu_to_le16(BAM_MAX_DATA_SIZE);
-				remainder -= BAM_MAX_DATA_SIZE;
-				curr_offset += BAM_MAX_DATA_SIZE;
+			if (remainder > BAM_FIFO_SIZE) {
+				desc->size = cpu_to_le16(BAM_FIFO_SIZE);
+				remainder -= BAM_FIFO_SIZE;
+				curr_offset += BAM_FIFO_SIZE;
 			} else {
 				desc->size = cpu_to_le16(remainder);
 				remainder = 0;
@@ -1174,7 +1174,7 @@ static int bam_dma_probe(struct platform_device *pdev)
 	/* set max dma segment size */
 	bdev->common.dev = bdev->dev;
 	bdev->common.dev->dma_parms = &bdev->dma_parms;
-	ret = dma_set_max_seg_size(bdev->common.dev, BAM_MAX_DATA_SIZE);
+	ret = dma_set_max_seg_size(bdev->common.dev, BAM_FIFO_SIZE);
 	if (ret) {
 		dev_err(bdev->dev, "cannot set maximum segment size\n");
 		goto err_bam_channel_exit;
-- 
1.9.1

Re: [PATCH v2 5/5] dmaengine: qcom: bam_dma: rename BAM_MAX_DATA_SIZE define

[Vinod Koul]

On Wed, Apr 06, 2016 at 01:56:22AM +0300, Stanimir Varbanov wrote:
> It seems that the define has not been with acurate name and
> makes confusion while reading the code. The more acurate
> name should be BAM_FIFO_SIZE.

And not sure by that, what do you mean by FIFO size. In dmaengine context we
typically refer to FIFO as periphral FIFO size. But the changes belo imple
that you are uisng this for block..?

> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/dma/qcom/bam_dma.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 7e5ad1c25e21..969b48176745 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -342,7 +342,7 @@ static const struct reg_offset_data bam_v1_7_reg_info[] = {
>  
>  #define BAM_DESC_FIFO_SIZE	SZ_32K
>  #define MAX_DESCRIPTORS (BAM_DESC_FIFO_SIZE / sizeof(struct bam_desc_hw) - 1)
> -#define BAM_MAX_DATA_SIZE	(SZ_32K - 8)
> +#define BAM_FIFO_SIZE	(SZ_32K - 8)
>  
>  struct bam_chan {
>  	struct virt_dma_chan vc;
> @@ -459,7 +459,7 @@ static void bam_chan_init_hw(struct bam_chan *bchan,
>  	 */
>  	writel_relaxed(ALIGN(bchan->fifo_phys, sizeof(struct bam_desc_hw)),
>  			bam_addr(bdev, bchan->id, BAM_P_DESC_FIFO_ADDR));
> -	writel_relaxed(BAM_MAX_DATA_SIZE,
> +	writel_relaxed(BAM_FIFO_SIZE,
>  			bam_addr(bdev, bchan->id, BAM_P_FIFO_SIZES));
>  
>  	/* enable the per pipe interrupts, enable EOT, ERR, and INT irqs */
> @@ -605,7 +605,7 @@ static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
>  
>  	/* calculate number of required entries */
>  	for_each_sg(sgl, sg, sg_len, i)
> -		num_alloc += DIV_ROUND_UP(sg_dma_len(sg), BAM_MAX_DATA_SIZE);
> +		num_alloc += DIV_ROUND_UP(sg_dma_len(sg), BAM_FIFO_SIZE);
>  
>  	/* allocate enough room to accomodate the number of entries */
>  	async_desc = kzalloc(sizeof(*async_desc) +
> @@ -636,10 +636,10 @@ static struct dma_async_tx_descriptor *bam_prep_slave_sg(struct dma_chan *chan,
>  			desc->addr = cpu_to_le32(sg_dma_address(sg) +
>  						 curr_offset);
>  
> -			if (remainder > BAM_MAX_DATA_SIZE) {
> -				desc->size = cpu_to_le16(BAM_MAX_DATA_SIZE);
> -				remainder -= BAM_MAX_DATA_SIZE;
> -				curr_offset += BAM_MAX_DATA_SIZE;
> +			if (remainder > BAM_FIFO_SIZE) {
> +				desc->size = cpu_to_le16(BAM_FIFO_SIZE);
> +				remainder -= BAM_FIFO_SIZE;
> +				curr_offset += BAM_FIFO_SIZE;
>  			} else {
>  				desc->size = cpu_to_le16(remainder);
>  				remainder = 0;
> @@ -1174,7 +1174,7 @@ static int bam_dma_probe(struct platform_device *pdev)
>  	/* set max dma segment size */
>  	bdev->common.dev = bdev->dev;
>  	bdev->common.dev->dma_parms = &bdev->dma_parms;
> -	ret = dma_set_max_seg_size(bdev->common.dev, BAM_MAX_DATA_SIZE);
> +	ret = dma_set_max_seg_size(bdev->common.dev, BAM_FIFO_SIZE);
>  	if (ret) {
>  		dev_err(bdev->dev, "cannot set maximum segment size\n");
>  		goto err_bam_channel_exit;
> -- 
> 1.9.1
> 

-- 
~Vinod

Re: [PATCH v2 5/5] dmaengine: qcom: bam_dma: rename BAM_MAX_DATA_SIZE define

[Stanimir Varbanov]

On 04/06/2016 02:47 AM, Vinod Koul wrote:
> On Wed, Apr 06, 2016 at 01:56:22AM +0300, Stanimir Varbanov wrote:
>> It seems that the define has not been with acurate name and
>> makes confusion while reading the code. The more acurate
>> name should be BAM_FIFO_SIZE.
> 
> And not sure by that, what do you mean by FIFO size. In dmaengine context we

By BAM_FIFO_SIZE I meant a FIFO depth for hw descriptors, i.e. how many
hw descriptors could be pushed into the descriptor FIFO. In our case we
wrote BAM_P_FIFO_SIZES register with SZ_32K - 8, which means that the
FIFO will be 4095 hw descriptors deep.

In fact the important patch in this series 4/5 where I corrected the
value we wrote in BAM_P_FIFO_SIZES register.

4/5 and 5/5 can be postponed till we have better decision...

-- 
regards,
Stan