[PATCH v2 0/5] dmaengine: qcom: bam_dma: fixes for remotely controlled bam

[PATCH v2 0/5] dmaengine: qcom: bam_dma: fixes for remotely controlled bam

[srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A]

From: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Hi Andy,

I did hit few issues while trying out SLIMBus BAM on DB820c, this BAM instance
is remotely controlled and powered up after ADSP is booted using QMI commands.

Firstly some of the master registers are written even when the BAM is remotely
controlled, and secondly reading registers when bam is not ready yet.

These 4 patches address these issues, there are few more issues like doing PM
in simillar usecase, these will be addressed soon.

Thanks,
Srini

Changes since v1:
	- Move dt bindings to separate patch
	- merge num-channels and num-ees driver changes
	- fixed few typos.
	- added runtime pm disable, as suggested by Sagar

Srinivas Kandagatla (5):
  dmaengine: qcom: bam_dma: make bam clk optional
  dt-bindings: dmaengine: bam_dma: add remote controlled bindings
  dmaengine: qcom: bam_dma: get num-channels and num-ees from dt
  dmaengine: qcom: bam_dma: do not write to global regs in remote mode
  dmaengine: qcom: bam_dma: disable runtime pm on remote controlled

 .../devicetree/bindings/dma/qcom_bam_dma.txt       |  4 ++
 drivers/dma/qcom/bam_dma.c                         | 66 +++++++++++++++-------
 2 files changed, 51 insertions(+), 19 deletions(-)

-- 
2.15.1

--
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 1/5] dmaengine: qcom: bam_dma: make bam clk optional

[srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A]

From: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

When BAM is remotely controlled it does not sound correct to control
its clk on Linux side. Make it optional, so that its not mandatory
for remote controlled BAM instances.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/dma/qcom/bam_dma.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index d076940e0c69..ae4c7b6930b8 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -1233,13 +1233,14 @@ static int bam_dma_probe(struct platform_device *pdev)
 						"qcom,controlled-remotely");
 
 	bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk");
-	if (IS_ERR(bdev->bamclk))
-		return PTR_ERR(bdev->bamclk);
-
-	ret = clk_prepare_enable(bdev->bamclk);
-	if (ret) {
-		dev_err(bdev->dev, "failed to prepare/enable clock\n");
-		return ret;
+	if (IS_ERR(bdev->bamclk)) {
+		bdev->bamclk = NULL;
+	} else {
+		ret = clk_prepare_enable(bdev->bamclk);
+		if (ret) {
+			dev_err(bdev->dev, "failed to prepare/enable clock\n");
+			return ret;
+		}
 	}
 
 	ret = bam_init(bdev);
-- 
2.15.1

--
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

Re: [PATCH v2 1/5] dmaengine: qcom: bam_dma: make bam clk optional

[Bjorn Andersson]

On Wed 14 Feb 06:44 PST 2018, Srinivas Kandagatla wrote:
> @@ -1233,13 +1233,14 @@ static int bam_dma_probe(struct platform_device *pdev)
>  						"qcom,controlled-remotely");
>  
>  	bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk");
> -	if (IS_ERR(bdev->bamclk))
> -		return PTR_ERR(bdev->bamclk);
> -
> -	ret = clk_prepare_enable(bdev->bamclk);
> -	if (ret) {
> -		dev_err(bdev->dev, "failed to prepare/enable clock\n");
> -		return ret;
> +	if (IS_ERR(bdev->bamclk)) {

In the case of !bdev->controlled_remotely I think this should still be
an error.

> +		bdev->bamclk = NULL;
> +	} else {
> +		ret = clk_prepare_enable(bdev->bamclk);
> +		if (ret) {
> +			dev_err(bdev->dev, "failed to prepare/enable clock\n");
> +			return ret;
> +		}

The rest of the driver will keep operating the bamclk (which is okay),
so for symmetry purposes I think you should just keep the
clk_prepare_enable() block unmodified.

Regards,
Bjorn

Re: [PATCH v2 1/5] dmaengine: qcom: bam_dma: make bam clk optional

[Srinivas Kandagatla]

Thanks for the Review,

On 14/02/18 15:41, Bjorn Andersson wrote:
>>   
>>   	bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk");
>> -	if (IS_ERR(bdev->bamclk))
>> -		return PTR_ERR(bdev->bamclk);
>> -
>> -	ret = clk_prepare_enable(bdev->bamclk);
>> -	if (ret) {
>> -		dev_err(bdev->dev, "failed to prepare/enable clock\n");
>> -		return ret;
>> +	if (IS_ERR(bdev->bamclk)) {
> In the case of !bdev->controlled_remotely I think this should still be
> an error.
> 
Yep makes sense,

You mean something like this?

------------------------->cut<------------------------
         bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk");
         if (IS_ERR(bdev->bamclk)) {
                 if (!bdev->controlled_remotely);
                         return PTR_ERR(bdev->bamclk);

                 bdev->bamclk = NULL;
         }

         ret = clk_prepare_enable(bdev->bamclk);
         if (ret) {
                 dev_err(bdev->dev, "failed to prepare/enable clock\n");
                 return ret;
         }

------------------------->cut<------------------------


>> +		bdev->bamclk = NULL;
>> +	} else {
>> +		ret = clk_prepare_enable(bdev->bamclk);
>> +		if (ret) {
>> +			dev_err(bdev->dev, "failed to prepare/enable clock\n");
>> +			return ret;
>> +		}
> The rest of the driver will keep operating the bamclk (which is okay),
> so for symmetry purposes I think you should just keep the
> clk_prepare_enable() block unmodified.
> 
Yep, with above change, this block should be unchanged.


--srini
> Regards,
> Bjorn

Re: [PATCH v2 1/5] dmaengine: qcom: bam_dma: make bam clk optional

[Bjorn Andersson]

On Wed 14 Feb 10:19 PST 2018, Srinivas Kandagatla wrote:

> Thanks for the Review,
> 
> On 14/02/18 15:41, Bjorn Andersson wrote:
> > >   	bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk");
> > > -	if (IS_ERR(bdev->bamclk))
> > > -		return PTR_ERR(bdev->bamclk);
> > > -
> > > -	ret = clk_prepare_enable(bdev->bamclk);
> > > -	if (ret) {
> > > -		dev_err(bdev->dev, "failed to prepare/enable clock\n");
> > > -		return ret;
> > > +	if (IS_ERR(bdev->bamclk)) {
> > In the case of !bdev->controlled_remotely I think this should still be
> > an error.
> > 
> Yep makes sense,
> 
> You mean something like this?
> 
> ------------------------->cut<------------------------
>         bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk");
>         if (IS_ERR(bdev->bamclk)) {
>                 if (!bdev->controlled_remotely);
>                         return PTR_ERR(bdev->bamclk);
> 
>                 bdev->bamclk = NULL;
>         }
> 
>         ret = clk_prepare_enable(bdev->bamclk);
>         if (ret) {
>                 dev_err(bdev->dev, "failed to prepare/enable clock\n");
>                 return ret;
>         }
> 
> ------------------------->cut<------------------------
> 

Yes

Regards,
Bjorn
--
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 2/5] dt-bindings: dmaengine: bam_dma: add remote controlled bindings

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

This patch adds 2 new properties for remote controlled bam dt bindings.
1. num-channels to indicate number of dma channels.
2. qcom,num-ees to indicate number of Execution Environments.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 Documentation/devicetree/bindings/dma/qcom_bam_dma.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
index 9cbf5d9df8fd..cf5b9e44432c 100644
--- a/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
+++ b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
@@ -15,6 +15,10 @@ Required properties:
   the secure world.
 - qcom,controlled-remotely : optional, indicates that the bam is controlled by
   remote proccessor i.e. execution environment.
+- num-channels : optional, indicates supported number of DMA channels in a
+  remotely controlled bam.
+- qcom,num-ees : optional, indicates supported number of Execution Environments
+  in a remotely controlled bam.
 
 Example:
 
-- 
2.15.1

[PATCH v2 3/5] dmaengine: qcom: bam_dma: get num-channels and num-ees from dt

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

When Linux is master of BAM, it can directly read registers to know number
of supported channels, however when its remotely controlled reading these
registers would trigger a crash if the BAM is not yet initialized or
powered up on the remote side.

This patch allows driver to read num-channels and num-ees from Device Tree
for remotely controlled BAM.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/dma/qcom/bam_dma.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index ae4c7b6930b8..146b5d03256f 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -393,6 +393,7 @@ struct bam_device {
 	struct device_dma_parameters dma_parms;
 	struct bam_chan *channels;
 	u32 num_channels;
+	u32 num_ees;
 
 	/* execution environment ID, from DT */
 	u32 ee;
@@ -1128,15 +1129,19 @@ static int bam_init(struct bam_device *bdev)
 	u32 val;
 
 	/* read revision and configuration information */
-	val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION)) >> NUM_EES_SHIFT;
-	val &= NUM_EES_MASK;
+	if (!bdev->num_ees) {
+		val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
+		bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK;
+	}
 
 	/* check that configured EE is within range */
-	if (bdev->ee >= val)
+	if (bdev->ee >= bdev->num_ees)
 		return -EINVAL;
 
-	val = readl_relaxed(bam_addr(bdev, 0, BAM_NUM_PIPES));
-	bdev->num_channels = val & BAM_NUM_PIPES_MASK;
+	if (!bdev->num_channels) {
+		val = readl_relaxed(bam_addr(bdev, 0, BAM_NUM_PIPES));
+		bdev->num_channels = val & BAM_NUM_PIPES_MASK;
+	}
 
 	if (bdev->controlled_remotely)
 		return 0;
@@ -1232,6 +1237,18 @@ static int bam_dma_probe(struct platform_device *pdev)
 	bdev->controlled_remotely = of_property_read_bool(pdev->dev.of_node,
 						"qcom,controlled-remotely");
 
+	if (bdev->controlled_remotely) {
+		ret = of_property_read_u32(pdev->dev.of_node, "num-channels",
+					   &bdev->num_channels);
+		if (ret)
+			dev_err(bdev->dev, "num-channels unspecified in dt\n");
+
+		ret = of_property_read_u32(pdev->dev.of_node, "qcom,num-ees",
+					   &bdev->num_ees);
+		if (ret)
+			dev_err(bdev->dev, "num-ees unspecified in dt\n");
+	}
+
 	bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk");
 	if (IS_ERR(bdev->bamclk)) {
 		bdev->bamclk = NULL;
-- 
2.15.1

[PATCH v2 4/5] dmaengine: qcom: bam_dma: do not write to global regs in remote mode

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

BAM_DESC_CNT_TRSHLD register is global register, which can only be written
when BAM is in master mode, So check the mode of operation before writing
it.

Without this check SOC's xPU would catch such access and crash the system.
First noticed on DB820c while testing SLIMBus BAM.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/dma/qcom/bam_dma.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 146b5d03256f..88ec515ad125 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -935,12 +935,15 @@ static void bam_apply_new_config(struct bam_chan *bchan,
 	struct bam_device *bdev = bchan->bdev;
 	u32 maxburst;
 
-	if (dir == DMA_DEV_TO_MEM)
-		maxburst = bchan->slave.src_maxburst;
-	else
-		maxburst = bchan->slave.dst_maxburst;
+	if (!bdev->controlled_remotely) {
+		if (dir == DMA_DEV_TO_MEM)
+			maxburst = bchan->slave.src_maxburst;
+		else
+			maxburst = bchan->slave.dst_maxburst;
 
-	writel_relaxed(maxburst, bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD));
+		writel_relaxed(maxburst,
+			       bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD));
+	}
 
 	bchan->reconfigure = 0;
 }
-- 
2.15.1

[PATCH v2 5/5] dmaengine: qcom: bam_dma: disable runtime pm on remote controlled

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Remotely controlled BAM instance should not do any power management from
CPU side, as cpu can not reliably say if the BAM is busy or not.

Disable it for such instances.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/dma/qcom/bam_dma.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 88ec515ad125..6e4fe398f0c8 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -1330,6 +1330,11 @@ static int bam_dma_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_unregister_dma;
 
+	if (bdev->controlled_remotely) {
+		pm_runtime_disable(&pdev->dev);
+		return 0;
+	}
+
 	pm_runtime_irq_safe(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, BAM_DMA_AUTOSUSPEND_DELAY);
 	pm_runtime_use_autosuspend(&pdev->dev);
@@ -1413,7 +1418,8 @@ static int __maybe_unused bam_dma_suspend(struct device *dev)
 {
 	struct bam_device *bdev = dev_get_drvdata(dev);
 
-	pm_runtime_force_suspend(dev);
+	if (!bdev->controlled_remotely)
+		pm_runtime_force_suspend(dev);
 
 	clk_unprepare(bdev->bamclk);
 
@@ -1429,7 +1435,8 @@ static int __maybe_unused bam_dma_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	pm_runtime_force_resume(dev);
+	if (!bdev->controlled_remotely)
+		pm_runtime_force_resume(dev);
 
 	return 0;
 }
-- 
2.15.1