| Message ID | 20211110105922.217895-14-bhupesh.sharma@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Enable Qualcomm Crypto Engine on sm8150 & sm8250 | expand |
Hi Bhupesh, On 11/10/21 12:59 PM, Bhupesh Sharma wrote: > From: Thara Gopinath <thara.gopinath@linaro.org> > > 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. > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > [Make header file inclusion alphabetical and use 'devm_of_icc_get()'] > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> please let me ask you to swap your and Thara's sob tags above, there is a rule applicable to all cases dealing with someone's else changes: From Documentation/process/submitting-patches.rst: Any further SoBs (Signed-off-by:'s) following the author's SoB are from people handling and transporting the patch, but were not involved in its development. SoB chains should reflect the **real** route a patch took as it was propagated to the maintainers and ultimately to Linus, with the first SoB entry signalling primary authorship of a single author. -- Best wishes, Vladimir
Hi Vladimir, On Fri, 12 Nov 2021 at 16:02, Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> wrote: > > Hi Bhupesh, > > On 11/10/21 12:59 PM, Bhupesh Sharma wrote: > > From: Thara Gopinath <thara.gopinath@linaro.org> > > > > 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. > > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > > Cc: Rob Herring <robh+dt@kernel.org> > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > > [Make header file inclusion alphabetical and use 'devm_of_icc_get()'] > > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> > > please let me ask you to swap your and Thara's sob tags above, there is > a rule applicable to all cases dealing with someone's else changes: > > From Documentation/process/submitting-patches.rst: > > Any further SoBs (Signed-off-by:'s) following the author's SoB are from > people handling and transporting the patch, but were not involved in its > development. SoB chains should reflect the **real** route a patch took > as it was propagated to the maintainers and ultimately to Linus, with > the first SoB entry signalling primary authorship of a single author. Sure, I will fix it in v6. Regards, Bhupesh
On Wed 10 Nov 04:59 CST 2021, Bhupesh Sharma wrote: > From: Thara Gopinath <thara.gopinath@linaro.org> > > 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. > This patch acquires the path, presumably between BAM and DDR, but I don't see that it actually do anything with it. So if this makes sm8250 work I presume it's because you'll get probe deferred until the interconnect provider is in place and it will hold buses at max speed until sync_state - and then in runtime perhaps the crypto driver hides the fact that BAM doesn't vote for its bandwidth? I'm sceptical about hiding behind such circumstances, but at least the commit message should be clear on what's going on - or you need cast some bandwidth votes when the block is expected to transfer data. > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > [Make header file inclusion alphabetical and use 'devm_of_icc_get()'] > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> As Vladimir pointed out, the S-o-b of a patch should be read in chronological order. Please read the section about Developer's Certificate of Origin (link below), it should make it clear why the specific order is important. https://www.kernel.org/doc/html/latest/process/submitting-patches.html#developer-s-certificate-of-origin-1-1 > --- > drivers/dma/qcom/bam_dma.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c > index c8a77b428b52..19fb17db467f 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> > @@ -392,6 +393,7 @@ struct bam_device { > const struct reg_offset_data *layout; > > struct clk *bamclk; > + struct icc_path *mem_path; > int irq; > > /* dma start transaction tasklet */ > @@ -1284,6 +1286,15 @@ static int bam_dma_probe(struct platform_device *pdev) > return ret; > } > > + /* Ensure that interconnects are initialized */ > + bdev->mem_path = devm_of_icc_get(bdev->dev, "memory"); > + Please drop this empty line. > + if (IS_ERR(bdev->mem_path)) { > + ret = PTR_ERR(bdev->mem_path); > + dev_err(bdev->dev, "failed to acquire icc path %d\n", ret); We typically want to avoid printing error messages when ret is -EPROBE_DEFER. The best way around this is to utilize the dev_err_probe() helper function. Replace the two lines above with: ret = dev_err_probe(bdev->dev, PTR_ERR(bdev->mem_path), "failed to acquire icc path\n") > + goto err_disable_clk; If you move the devm_of_icc_get() right before clk_prepare_enable() (still after devm_clk_get*()) you can simply return dev_err_probe() - and will avoid toggling the CE clock unnecessarily on EPROBE_DEFER. Regards, Bjorn > + } > + > ret = bam_init(bdev); > if (ret) > goto err_disable_clk; > -- > 2.31.1 >
On 10/11/2021 13:59, Bhupesh Sharma wrote: > From: Thara Gopinath <thara.gopinath@linaro.org> > > 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. > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > [Make header file inclusion alphabetical and use 'devm_of_icc_get()'] > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> > --- > drivers/dma/qcom/bam_dma.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c > index c8a77b428b52..19fb17db467f 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> > @@ -392,6 +393,7 @@ struct bam_device { > const struct reg_offset_data *layout; > > struct clk *bamclk; > + struct icc_path *mem_path; > int irq; > > /* dma start transaction tasklet */ > @@ -1284,6 +1286,15 @@ static int bam_dma_probe(struct platform_device *pdev) > return ret; > } > > + /* Ensure that interconnects are initialized */ > + bdev->mem_path = devm_of_icc_get(bdev->dev, "memory"); Also, as a note, the "memory" is not a good name for the ICC path. Usually they take the form of "src-dst". However in this case you can probably use NULL for the first and the only icc path. > + Extra newline, not necessary. > + if (IS_ERR(bdev->mem_path)) { > + ret = PTR_ERR(bdev->mem_path); > + dev_err(bdev->dev, "failed to acquire icc path %d\n", ret); > + goto err_disable_clk; > + } > + > ret = bam_init(bdev); > if (ret) > goto err_disable_clk;
diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c index c8a77b428b52..19fb17db467f 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> @@ -392,6 +393,7 @@ struct bam_device { const struct reg_offset_data *layout; struct clk *bamclk; + struct icc_path *mem_path; int irq; /* dma start transaction tasklet */ @@ -1284,6 +1286,15 @@ static int bam_dma_probe(struct platform_device *pdev) return ret; } + /* Ensure that interconnects are initialized */ + bdev->mem_path = devm_of_icc_get(bdev->dev, "memory"); + + if (IS_ERR(bdev->mem_path)) { + ret = PTR_ERR(bdev->mem_path); + dev_err(bdev->dev, "failed to acquire icc path %d\n", ret); + goto err_disable_clk; + } + ret = bam_init(bdev); if (ret) goto err_disable_clk;