diff mbox series

[v7,1/1] dma: qcom: bam_dma: Add support to initialize interconnect path

Message ID 20220921030649.1436434-2-bhupesh.sharma@linaro.org
State New
Headers show
Series dma: qcom: bam_dma: Add support to initialize interconnect path | expand

Commit Message

Bhupesh Sharma Sept. 21, 2022, 3:06 a.m. UTC 
From: Thara Gopinath <thara.gopinath@gmail.com>

BAM dma engine associated with certain hardware blocks could require
relevant interconnect pieces be initialized prior to the dma engine
initialization. For e.g. crypto bam dma engine on sm8250. Such requirement
is passed on to the bam dma driver from dt via the "interconnects"
property. Add support in bam_dma driver to check whether the interconnect
path is accessible/enabled prior to attempting driver intializations.

If interconnects are not yet setup, defer the BAM DMA driver probe().

Cc: Bjorn Andersson <andersson@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Thara Gopinath <thara.gopinath@gmail.com>
Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
[Bhupesh: Make header file inclusion alphabetical and use 'devm_of_icc_get()']
---
 drivers/dma/qcom/bam_dma.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Vinod Koul Sept. 22, 2022, 2:38 a.m. UTC | #1 
On 21-09-22, 08:36, Bhupesh Sharma wrote:
> From: Thara Gopinath <thara.gopinath@gmail.com>
> 
> BAM dma engine associated with certain hardware blocks could require
> relevant interconnect pieces be initialized prior to the dma engine
> initialization. For e.g. crypto bam dma engine on sm8250. Such requirement
> is passed on to the bam dma driver from dt via the "interconnects"
> property. Add support in bam_dma driver to check whether the interconnect
> path is accessible/enabled prior to attempting driver intializations.
> 
> If interconnects are not yet setup, defer the BAM DMA driver probe().
> 
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Thara Gopinath <thara.gopinath@gmail.com>
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> [Bhupesh: Make header file inclusion alphabetical and use 'devm_of_icc_get()']
> ---
>  drivers/dma/qcom/bam_dma.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 2ff787df513e..a5b0cf28ffb7 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -26,6 +26,7 @@
>  #include <linux/kernel.h>
>  #include <linux/io.h>
>  #include <linux/init.h>
> +#include <linux/interconnect.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
> @@ -394,6 +395,7 @@ struct bam_device {
>  	const struct reg_offset_data *layout;
>  
>  	struct clk *bamclk;
> +	struct icc_path *mem_path;
>  	int irq;
>  
>  	/* dma start transaction tasklet */
> @@ -1294,6 +1296,14 @@ static int bam_dma_probe(struct platform_device *pdev)
>  	if (IS_ERR(bdev->bamclk))
>  		return PTR_ERR(bdev->bamclk);
>  
> +	/* Ensure that interconnects are initialized */
> +	bdev->mem_path = devm_of_icc_get(bdev->dev, "memory");
> +	if (IS_ERR(bdev->mem_path)) {
> +		ret = dev_err_probe(bdev->dev, PTR_ERR(bdev->mem_path),
> +				    "failed to acquire icc path\n");
> +		return ret;
> +	}

So this makes us fail on older DT where icc path may not be present.
Should this not be an optional thing?

> +
>  	ret = clk_prepare_enable(bdev->bamclk);
>  	if (ret) {
>  		dev_err(bdev->dev, "failed to prepare/enable clock\n");
> -- 
> 2.37.1
Dmitry Baryshkov Dec. 28, 2022, 4:15 p.m. UTC | #2 
On 22/09/2022 05:38, Vinod Koul wrote:
> On 21-09-22, 08:36, Bhupesh Sharma wrote:
>> From: Thara Gopinath <thara.gopinath@gmail.com>
>>
>> BAM dma engine associated with certain hardware blocks could require
>> relevant interconnect pieces be initialized prior to the dma engine
>> initialization. For e.g. crypto bam dma engine on sm8250. Such requirement
>> is passed on to the bam dma driver from dt via the "interconnects"
>> property. Add support in bam_dma driver to check whether the interconnect
>> path is accessible/enabled prior to attempting driver intializations.
>>
>> If interconnects are not yet setup, defer the BAM DMA driver probe().
>>
>> Cc: Bjorn Andersson <andersson@kernel.org>
>> Cc: Rob Herring <robh@kernel.org>
>> Signed-off-by: Thara Gopinath <thara.gopinath@gmail.com>
>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>> [Bhupesh: Make header file inclusion alphabetical and use 'devm_of_icc_get()']
>> ---
>>   drivers/dma/qcom/bam_dma.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
>> index 2ff787df513e..a5b0cf28ffb7 100644
>> --- a/drivers/dma/qcom/bam_dma.c
>> +++ b/drivers/dma/qcom/bam_dma.c
>> @@ -26,6 +26,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/io.h>
>>   #include <linux/init.h>
>> +#include <linux/interconnect.h>
>>   #include <linux/slab.h>
>>   #include <linux/module.h>
>>   #include <linux/interrupt.h>
>> @@ -394,6 +395,7 @@ struct bam_device {
>>   	const struct reg_offset_data *layout;
>>   
>>   	struct clk *bamclk;
>> +	struct icc_path *mem_path;
>>   	int irq;
>>   
>>   	/* dma start transaction tasklet */
>> @@ -1294,6 +1296,14 @@ static int bam_dma_probe(struct platform_device *pdev)
>>   	if (IS_ERR(bdev->bamclk))
>>   		return PTR_ERR(bdev->bamclk);
>>   
>> +	/* Ensure that interconnects are initialized */
>> +	bdev->mem_path = devm_of_icc_get(bdev->dev, "memory");
>> +	if (IS_ERR(bdev->mem_path)) {
>> +		ret = dev_err_probe(bdev->dev, PTR_ERR(bdev->mem_path),
>> +				    "failed to acquire icc path\n");
>> +		return ret;
>> +	}
> 
> So this makes us fail on older DT where icc path may not be present.
> Should this not be an optional thing?

If "interconnects" property is not present, of_icc_get() returns NULL. 
So the driver will not err out (correct). All ICC functions will treat 
NULL path correctly (by doing nothing). So this patch is correct.

However we'd need v8 anyway, there needs to be a bindings update.

Bhupesh, this has been lingering for some time. We need this for several 
platforms. Any chance for the v8?

> 
>> +
>>   	ret = clk_prepare_enable(bdev->bamclk);
>>   	if (ret) {
>>   		dev_err(bdev->dev, "failed to prepare/enable clock\n");
>> -- 
>> 2.37.1
>
Vladimir Zapolskiy Feb. 16, 2023, 2:18 p.m. UTC | #3 
On 9/21/22 06:06, Bhupesh Sharma wrote:
> From: Thara Gopinath <thara.gopinath@gmail.com>
> 
> BAM dma engine associated with certain hardware blocks could require
> relevant interconnect pieces be initialized prior to the dma engine
> initialization. For e.g. crypto bam dma engine on sm8250. Such requirement

Apparently it's proven that the change description is incorrect, Qualcomm
crypto engine is working fine on SM8250 and even more recent platforms,
so far there is no obvious necessity in this change.

> is passed on to the bam dma driver from dt via the "interconnects"
> property. Add support in bam_dma driver to check whether the interconnect
> path is accessible/enabled prior to attempting driver intializations.
> 
> If interconnects are not yet setup, defer the BAM DMA driver probe().
> 
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Thara Gopinath <thara.gopinath@gmail.com>
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> [Bhupesh: Make header file inclusion alphabetical and use 'devm_of_icc_get()']
> ---
>   drivers/dma/qcom/bam_dma.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 2ff787df513e..a5b0cf28ffb7 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -26,6 +26,7 @@
>   #include <linux/kernel.h>
>   #include <linux/io.h>
>   #include <linux/init.h>
> +#include <linux/interconnect.h>
>   #include <linux/slab.h>
>   #include <linux/module.h>
>   #include <linux/interrupt.h>
> @@ -394,6 +395,7 @@ struct bam_device {
>   	const struct reg_offset_data *layout;
>   
>   	struct clk *bamclk;
> +	struct icc_path *mem_path;
>   	int irq;
>   
>   	/* dma start transaction tasklet */
> @@ -1294,6 +1296,14 @@ static int bam_dma_probe(struct platform_device *pdev)
>   	if (IS_ERR(bdev->bamclk))
>   		return PTR_ERR(bdev->bamclk);
>   
> +	/* Ensure that interconnects are initialized */
> +	bdev->mem_path = devm_of_icc_get(bdev->dev, "memory");
> +	if (IS_ERR(bdev->mem_path)) {
> +		ret = dev_err_probe(bdev->dev, PTR_ERR(bdev->mem_path),
> +				    "failed to acquire icc path\n");
> +		return ret;
> +	}
> +
>   	ret = clk_prepare_enable(bdev->bamclk);
>   	if (ret) {
>   		dev_err(bdev->dev, "failed to prepare/enable clock\n");

I'm resurrecting the comments on this change to emphasize the observation
that the change is not needed at all to run QCE.

--
Best wishes,
Vladimir
diff mbox series

Patch

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 2ff787df513e..a5b0cf28ffb7 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -26,6 +26,7 @@ 
 #include <linux/kernel.h>
 #include <linux/io.h>
 #include <linux/init.h>
+#include <linux/interconnect.h>
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
@@ -394,6 +395,7 @@  struct bam_device {
 	const struct reg_offset_data *layout;
 
 	struct clk *bamclk;
+	struct icc_path *mem_path;
 	int irq;
 
 	/* dma start transaction tasklet */
@@ -1294,6 +1296,14 @@  static int bam_dma_probe(struct platform_device *pdev)
 	if (IS_ERR(bdev->bamclk))
 		return PTR_ERR(bdev->bamclk);
 
+	/* Ensure that interconnects are initialized */
+	bdev->mem_path = devm_of_icc_get(bdev->dev, "memory");
+	if (IS_ERR(bdev->mem_path)) {
+		ret = dev_err_probe(bdev->dev, PTR_ERR(bdev->mem_path),
+				    "failed to acquire icc path\n");
+		return ret;
+	}
+
 	ret = clk_prepare_enable(bdev->bamclk);
 	if (ret) {
 		dev_err(bdev->dev, "failed to prepare/enable clock\n");