Hi,
Here is version 3 with following changes:
- fixed a typo in 2/6.
- the patch which adds dt property is seperated in two, one
which adding binding document and another one which modifing
the driver - requested by Vinod.
- fixed a typo in 5/6.
- added collected Acked,Tested-by tags.
The v2 can be found at:
https://lkml.org/lkml/2016/4/5/922
regards,
Stan
Stanimir Varbanov (6):
dmaengine: qcom: bam_dma: fix dma free memory on remove
dmaengine: qcom: bam_dma: clear BAM interrupt only if it is raised
dmaengine: qcom: bam_dma: document controlled-remotely dt property
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.7.9.5
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 <[email protected]>
---
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.7.9.5
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 <[email protected]>
Reviewed-by: Andy Gross <[email protected]>
Tested-by: Pramod Gurav <[email protected]>
---
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.7.9.5
Extend BAM dt bindings with controlled-remotely property. The
property will be needed to handle cases where we need to skip
register writes to initialise BAM hardware block.
Signed-off-by: Stanimir Varbanov <[email protected]>
Reviewed-by: Andy Gross <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
.../devicetree/bindings/dma/qcom_bam_dma.txt | 2 ++
1 file changed, 2 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:
--
1.7.9.5
The pipe fifo size register must instruct the bam hw
how many hw descriptors can be pushed to fifo. Currently
we instruct 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 <[email protected]>
---
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.7.9.5
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 <[email protected]>
---
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.7.9.5
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 <[email protected]>
Reviewed-by: Andy Gross <[email protected]>
Tested-by: Pramod Gurav <[email protected]>
---
drivers/dma/qcom/bam_dma.c | 7 +++++++
1 file changed, 7 insertions(+)
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.7.9.5
On Mon, Apr 11, 2016 at 11:38:43AM +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.
>
> Signed-off-by: Stanimir Varbanov <[email protected]>
Reviewed-by: Andy Gross <[email protected]>
On Mon, Apr 11, 2016 at 11:38:38AM +0300, Stanimir Varbanov wrote:
> 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 <[email protected]>
Reviewed-by: Andy Gross <[email protected]>
On Mon, Apr 11, 2016 at 11:38:37AM +0300, Stanimir Varbanov wrote:
> Hi,
>
> Here is version 3 with following changes:
> - fixed a typo in 2/6.
> - the patch which adds dt property is seperated in two, one
> which adding binding document and another one which modifing
> the driver - requested by Vinod.
> - fixed a typo in 5/6.
> - added collected Acked,Tested-by tags.
Applied, thanks
--
~Vinod