dmaengine.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 0/1] dma: qcom: bam_dma: Add support to initialize interconnect path
@ 2022-09-21  3:06 Bhupesh Sharma
  2022-09-21  3:06 ` [PATCH v7 1/1] " Bhupesh Sharma
  0 siblings, 1 reply; 8+ messages in thread
From: Bhupesh Sharma @ 2022-09-21  3:06 UTC (permalink / raw)
  To: dmaengine
  Cc: agross, linux-crypto, linux-kernel, linux-arm-msm,
	thara.gopinath, devicetree, andersson, bhupesh.sharma,
	bhupesh.linux, vkoul, Jordan Crouse

Changes since v6:
=================
- v6 can be seen here: https://lore.kernel.org/linux-arm-msm/30756e6f-952f-ccf2-b493-e515ba4f0a64@linaro.org/
- As per Krzysztof's suggestion on v6, clubbed the crypto driver and
  dt-bindings changes together. Now the overall v5 patchset into 3
  separate patchsets, one each for the following areas to allow easier
  review and handling from the maintainer:
        arm-msm, crypto and dma
- This patch targets the dma tree specifically.

Changes since v5:
=================
- v5 can be seen here: https://lore.kernel.org/lkml/20211110105922.217895-1-bhupesh.sharma@linaro.org/
- As per Bjorn's suggestion on irc, broke down the patchset into 4
  separate patchsets, one each for the following areas to allow easier
  review and handling from the maintainer:
        arm-msm, crypto, dma and devicetree
- Addressed Rob's, Vladimir's and Bjorn's review comments received on
  v5.
- Added Tested-by from Jordan received on the v5.

Changes since v4:
=================
- v4 for sm8250 can be seen here: https://lore.kernel.org/linux-arm-msm/20211013105541.68045-1-bhupesh.sharma@linaro.org/
- v1 for sm8150 qce enablement can be seen here: https://lore.kernel.org/linux-arm-msm/20211013165823.88123-1-bhupesh.sharma@linaro.org/
- Merged the sm8150 and sm8250 enablement patches in the same patchset,
  as per suggestions from Bjorn.
- Dropped a couple of patches from v4, as these have been picked by
  Bjorn already via his tree.
- Addressed review comments from Vladimir, Thara and Rob.
- Collect Reviewed-by from Rob and Thara on some of the patches from the
  v4 patchset.

Changes since v3:
=================
- v3 can be seen here: https://lore.kernel.org/linux-arm-msm/20210519143700.27392-1-bhupesh.sharma@linaro.org/
- Dropped a couple of patches from v3, on basis of the review comments:
   ~ [PATCH 13/17] crypto: qce: core: Make clocks optional
   ~ [PATCH 15/17] crypto: qce: Convert the device found dev_dbg() to dev_info()
- Addressed review comments from Thara, Rob and Stephan Gerhold.
- Collect Reviewed-by from Rob and Thara on some of the patches from the
  v3 patchset.

Changes since v2:
=================
- v2 can be seen here: https://lore.kernel.org/dmaengine/20210505213731.538612-1-bhupesh.sharma@linaro.org/
- Drop a couple of patches from v1, which tried to address the defered
  probing of qce driver in case bam dma driver is not yet probed.
  Replace it instead with a single (simpler) patch [PATCH 16/17].
- Convert bam dma and qce crypto dt-bindings to YAML.
- Addressed review comments from Thara, Bjorn, Vinod and Rob.

Changes since v1:
=================
- v1 can be seen here: https://lore.kernel.org/linux-arm-msm/20210310052503.3618486-1-bhupesh.sharma@linaro.org/
- v1 did not work well as reported earlier by Dmitry, so v2 contains the following
  changes/fixes:
  ~ Enable the interconnect path b/w BAM DMA and main memory first
    before trying to access the BAM DMA registers.
  ~ Enable the interconnect path b/w qce crytpo and main memory first
    before trying to access the qce crypto registers.
  ~ Make sure to document the required and optional properties for both
    BAM DMA and qce crypto drivers.
  ~ Add a few debug related print messages in case the qce crypto driver
    passes or fails to probe.
  ~ Convert the qce crypto driver probe to a defered one in case the BAM DMA
    or the interconnect driver(s) (needed on specific Qualcomm parts) are not
    yet probed.

BAM dma engine associated with certain Qualcomm Snapdragon hardware blocks requires
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().

Tested on sm8150-mtp, sa8155p-adp, sm8250-mtp and RB5 boards.

Cc: vkoul@kernel.org
Cc: thara.gopinath@gmail.com
Cc: andersson@kernel.org
Tested-by: Jordan Crouse <jorcrous@amazon.com>

Thara Gopinath (1):
  dma: qcom: bam_dma: Add support to initialize interconnect path

 drivers/dma/qcom/bam_dma.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

-- 
2.37.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v7 1/1] dma: qcom: bam_dma: Add support to initialize interconnect path
  2022-09-21  3:06 [PATCH v7 0/1] dma: qcom: bam_dma: Add support to initialize interconnect path Bhupesh Sharma
@ 2022-09-21  3:06 ` Bhupesh Sharma
  2022-09-22  2:38   ` Vinod Koul
  2023-02-16 14:18   ` Vladimir Zapolskiy
  0 siblings, 2 replies; 8+ messages in thread
From: Bhupesh Sharma @ 2022-09-21  3:06 UTC (permalink / raw)
  To: dmaengine
  Cc: agross, linux-crypto, linux-kernel, linux-arm-msm,
	thara.gopinath, devicetree, andersson, bhupesh.sharma,
	bhupesh.linux, vkoul, Rob Herring

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;
+	}
+
 	ret = clk_prepare_enable(bdev->bamclk);
 	if (ret) {
 		dev_err(bdev->dev, "failed to prepare/enable clock\n");
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v7 1/1] dma: qcom: bam_dma: Add support to initialize interconnect path
  2022-09-21  3:06 ` [PATCH v7 1/1] " Bhupesh Sharma
@ 2022-09-22  2:38   ` Vinod Koul
  2022-09-23  5:10     ` Bhupesh Sharma
  2022-12-28 16:15     ` Dmitry Baryshkov
  2023-02-16 14:18   ` Vladimir Zapolskiy
  1 sibling, 2 replies; 8+ messages in thread
From: Vinod Koul @ 2022-09-22  2:38 UTC (permalink / raw)
  To: Bhupesh Sharma
  Cc: dmaengine, agross, linux-crypto, linux-kernel, linux-arm-msm,
	thara.gopinath, devicetree, andersson, bhupesh.linux,
	Rob Herring

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

-- 
~Vinod

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7 1/1] dma: qcom: bam_dma: Add support to initialize interconnect path
  2022-09-22  2:38   ` Vinod Koul
@ 2022-09-23  5:10     ` Bhupesh Sharma
  2022-12-28 16:15     ` Dmitry Baryshkov
  1 sibling, 0 replies; 8+ messages in thread
From: Bhupesh Sharma @ 2022-09-23  5:10 UTC (permalink / raw)
  To: Vinod Koul
  Cc: dmaengine, agross, linux-crypto, linux-kernel, linux-arm-msm,
	thara.gopinath, devicetree, andersson, bhupesh.linux,
	Rob Herring

On Thu, 22 Sept 2022 at 08:08, Vinod Koul <vkoul@kernel.org> 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?

That's a good point. I am not sure if Thara tried this on platforms
which don't have the icc path available.

I will fix this in v8.

Thanks,
Bhupesh

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7 1/1] dma: qcom: bam_dma: Add support to initialize interconnect path
  2022-09-22  2:38   ` Vinod Koul
  2022-09-23  5:10     ` Bhupesh Sharma
@ 2022-12-28 16:15     ` Dmitry Baryshkov
  1 sibling, 0 replies; 8+ messages in thread
From: Dmitry Baryshkov @ 2022-12-28 16:15 UTC (permalink / raw)
  To: Vinod Koul, Bhupesh Sharma
  Cc: dmaengine, agross, linux-crypto, linux-kernel, linux-arm-msm,
	thara.gopinath, devicetree, andersson, bhupesh.linux,
	Rob Herring

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
> 

-- 
With best wishes
Dmitry


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7 1/1] dma: qcom: bam_dma: Add support to initialize interconnect path
  2022-09-21  3:06 ` [PATCH v7 1/1] " Bhupesh Sharma
  2022-09-22  2:38   ` Vinod Koul
@ 2023-02-16 14:18   ` Vladimir Zapolskiy
  2023-02-17  4:00     ` Bhupesh Sharma
  1 sibling, 1 reply; 8+ messages in thread
From: Vladimir Zapolskiy @ 2023-02-16 14:18 UTC (permalink / raw)
  To: Bhupesh Sharma, dmaengine
  Cc: agross, linux-crypto, linux-kernel, linux-arm-msm,
	thara.gopinath, devicetree, andersson, bhupesh.linux, vkoul,
	Rob Herring

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7 1/1] dma: qcom: bam_dma: Add support to initialize interconnect path
  2023-02-16 14:18   ` Vladimir Zapolskiy
@ 2023-02-17  4:00     ` Bhupesh Sharma
  2023-02-17  7:07       ` Vladimir Zapolskiy
  0 siblings, 1 reply; 8+ messages in thread
From: Bhupesh Sharma @ 2023-02-17  4:00 UTC (permalink / raw)
  To: Vladimir Zapolskiy
  Cc: dmaengine, agross, linux-crypto, linux-kernel, linux-arm-msm,
	thara.gopinath, devicetree, andersson, bhupesh.linux, vkoul,
	Rob Herring

On Thu, 16 Feb 2023 at 19:49, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> 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.

Since your v9 patchset produces no entry in $ cat /proc/crypto on
either RB5 (qrb5165) or (with an additional patch) on sm8150-mtp or
sa8115p-adp with the default arm64 defconfig with linux-next, I am not
sure we can conclude QCE is working with these changes.

Please share more details on how you tested this.

Regards,
Bhupesh

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7 1/1] dma: qcom: bam_dma: Add support to initialize interconnect path
  2023-02-17  4:00     ` Bhupesh Sharma
@ 2023-02-17  7:07       ` Vladimir Zapolskiy
  0 siblings, 0 replies; 8+ messages in thread
From: Vladimir Zapolskiy @ 2023-02-17  7:07 UTC (permalink / raw)
  To: Bhupesh Sharma
  Cc: dmaengine, agross, linux-crypto, linux-kernel, linux-arm-msm,
	thara.gopinath, devicetree, andersson, bhupesh.linux, vkoul,
	Rob Herring

On 2/17/23 06:00, Bhupesh Sharma wrote:
> On Thu, 16 Feb 2023 at 19:49, Vladimir Zapolskiy
> <vladimir.zapolskiy@linaro.org> wrote:
>>
>> 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.
> 
> Since your v9 patchset produces no entry in $ cat /proc/crypto on
> either RB5 (qrb5165) or (with an additional patch) on sm8150-mtp or
> sa8115p-adp with the default arm64 defconfig with linux-next, I am not
> sure we can conclude QCE is working with these changes.

Hopefully someone else can conclude it independently.

You may notice that v9 patchset series does not contain sm8150 specific
changes, but sm8250 one (v9 11/14) is present, testing on RB5 is valid.

> Please share more details on how you tested this.

I do compile linux-next with v9 QCE series applied on top of it and run the
kernel on RB5 board, there is nothing really complicated about it.

If you missed it, the 'cryptobench benchmark' test etc. have been already
described, please find it here:

   https://lore.kernel.org/linux-crypto/731af3cf-6f18-caf9-ef65-ec73b0744ad9@linaro.org/

--
Best wishes,
Vladimir

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-02-17  7:07 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-21  3:06 [PATCH v7 0/1] dma: qcom: bam_dma: Add support to initialize interconnect path Bhupesh Sharma
2022-09-21  3:06 ` [PATCH v7 1/1] " Bhupesh Sharma
2022-09-22  2:38   ` Vinod Koul
2022-09-23  5:10     ` Bhupesh Sharma
2022-12-28 16:15     ` Dmitry Baryshkov
2023-02-16 14:18   ` Vladimir Zapolskiy
2023-02-17  4:00     ` Bhupesh Sharma
2023-02-17  7:07       ` Vladimir Zapolskiy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).