| Message ID | 20211110105922.217895-19-bhupesh.sharma@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Enable Qualcomm Crypto Engine on sm8150 & sm8250 | expand |
On Wed 10 Nov 04:59 CST 2021, Bhupesh Sharma wrote: > Since the Qualcomm qce crypto driver needs the BAM dma driver to be > setup first (to allow crypto operations), it makes sense to defer > the qce crypto driver probing in case the BAM dma driver is not yet > probed. > To me this sentence implies that qce_crypto_probe() doesn't return -EPROBE_DEFER from qce_dma_request(), but looking at the code I don't see why this would be... > Move the code leg requesting dma channels earlier in the > probe() flow. This fixes the qce probe failure issues when both qce > and BMA dma are compiled as static part of the kernel. > As far as I can tell the only actual difference is that you're moving the qce_dma_request() above the icc_set_bw() call and the three clk_prepare_enable() calls. This is a very valid optimization, but where does qce_crypto_probe() fail and why does it need the BAM driver to have probed before we try to turn on the clocks and (our) interconnect vote? > Cc: Thara Gopinath <thara.gopinath@linaro.org> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- > drivers/crypto/qce/core.c | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c > index 7c90401a2ef1..84ed9e253d5d 100644 > --- a/drivers/crypto/qce/core.c > +++ b/drivers/crypto/qce/core.c > @@ -209,9 +209,19 @@ static int qce_crypto_probe(struct platform_device *pdev) > if (ret < 0) > return ret; > > + /* qce driver requires BAM dma driver to be setup first. > + * In case the dma channel are not set yet, this check > + * helps use to return -EPROBE_DEFER earlier. > + */ This comment warrants the change, but I don't see that it will add value in the code once the patch is merged. Please drop it. > + ret = qce_dma_request(qce->dev, &qce->dma); > + if (ret) I presume this is where your added dev_err() in err: comes in handy. I definitely think this warrants an error print; so return dev_err_probe() here would be the right thing to do (in the previous patch). > + return ret; > + > qce->mem_path = devm_of_icc_get(qce->dev, "memory"); > - if (IS_ERR(qce->mem_path)) > - return PTR_ERR(qce->mem_path); But I see no reason for moving it above devm_of_icc_get() and the devm_clk_get*() calls - as they don't actually do anything, but moving qce_dma_request() above them forces the introduction of goto here. Regards, Bjorn > + if (IS_ERR(qce->mem_path)) { > + ret = PTR_ERR(qce->mem_path); > + goto err; > + } > > qce->core = devm_clk_get_optional(qce->dev, "core"); > if (IS_ERR(qce->core)) { > @@ -247,10 +257,6 @@ static int qce_crypto_probe(struct platform_device *pdev) > if (ret) > goto err_clks_iface; > > - ret = qce_dma_request(qce->dev, &qce->dma); > - if (ret) > - goto err_clks; > - > ret = qce_check_version(qce); > if (ret) > goto err_clks; > @@ -265,12 +271,10 @@ static int qce_crypto_probe(struct platform_device *pdev) > > ret = qce_register_algs(qce); > if (ret) > - goto err_dma; > + goto err_clks; > > return 0; > > -err_dma: > - qce_dma_release(&qce->dma); > err_clks: > clk_disable_unprepare(qce->bus); > err_clks_iface: > @@ -280,6 +284,7 @@ static int qce_crypto_probe(struct platform_device *pdev) > err_mem_path_disable: > icc_set_bw(qce->mem_path, 0, 0); > err: > + qce_dma_release(&qce->dma); > dev_err(dev, "%s failed : %d\n", __func__, ret); > return ret; > } > -- > 2.31.1 >
diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c index 7c90401a2ef1..84ed9e253d5d 100644 --- a/drivers/crypto/qce/core.c +++ b/drivers/crypto/qce/core.c @@ -209,9 +209,19 @@ static int qce_crypto_probe(struct platform_device *pdev) if (ret < 0) return ret; + /* qce driver requires BAM dma driver to be setup first. + * In case the dma channel are not set yet, this check + * helps use to return -EPROBE_DEFER earlier. + */ + ret = qce_dma_request(qce->dev, &qce->dma); + if (ret) + return ret; + qce->mem_path = devm_of_icc_get(qce->dev, "memory"); - if (IS_ERR(qce->mem_path)) - return PTR_ERR(qce->mem_path); + if (IS_ERR(qce->mem_path)) { + ret = PTR_ERR(qce->mem_path); + goto err; + } qce->core = devm_clk_get_optional(qce->dev, "core"); if (IS_ERR(qce->core)) { @@ -247,10 +257,6 @@ static int qce_crypto_probe(struct platform_device *pdev) if (ret) goto err_clks_iface; - ret = qce_dma_request(qce->dev, &qce->dma); - if (ret) - goto err_clks; - ret = qce_check_version(qce); if (ret) goto err_clks; @@ -265,12 +271,10 @@ static int qce_crypto_probe(struct platform_device *pdev) ret = qce_register_algs(qce); if (ret) - goto err_dma; + goto err_clks; return 0; -err_dma: - qce_dma_release(&qce->dma); err_clks: clk_disable_unprepare(qce->bus); err_clks_iface: @@ -280,6 +284,7 @@ static int qce_crypto_probe(struct platform_device *pdev) err_mem_path_disable: icc_set_bw(qce->mem_path, 0, 0); err: + qce_dma_release(&qce->dma); dev_err(dev, "%s failed : %d\n", __func__, ret); return ret; }
Since the Qualcomm qce crypto driver needs the BAM dma driver to be setup first (to allow crypto operations), it makes sense to defer the qce crypto driver probing in case the BAM dma driver is not yet probed. Move the code leg requesting dma channels earlier in the probe() flow. This fixes the qce probe failure issues when both qce and BMA dma are compiled as static part of the kernel. Cc: Thara Gopinath <thara.gopinath@linaro.org> Cc: Bjorn Andersson <bjorn.andersson@linaro.org> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> --- drivers/crypto/qce/core.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)