| Message ID | 20210122025251.3501362-1-thara.gopinath@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drivers: dma: qcom: bam_dma: Manage clocks when controlled_remotely is set | expand |
On Thu, Jan 21, 2021 at 09:52:51PM -0500, Thara Gopinath wrote: > When bam dma is "controlled remotely", thus far clocks were not controlled > from the Linux. In this scenario, Linux was disabling runtime pm in bam dma > driver and not doing any clock management in suspend/resume hooks. > > With introduction of crypto engine bam dma, the clock is a rpmh resource > that can be controlled from both Linux and TZ/remote side. Now bam dma > clock is getting enabled during probe even though the bam dma can be > "controlled remotely". But due to clocks not being handled properly, > bam_suspend generates a unbalanced clk_unprepare warning during system > suspend. > > To fix the above issue and to enable proper clock-management, this patch > enables runtim-pm and handles bam dma clocks in suspend/resume hooks if > the clock node is present irrespective of controlled_remotely property. Shouldn't the following probe code need some update? Now we have both controlled_remotely and clocks handle for cryptobam node. For example, if devm_clk_get() returns -EPROBE_DEFER, we do not want to continue with bamclk forcing to be NULL, right? 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; } > > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> > --- > drivers/dma/qcom/bam_dma.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c > index 88579857ca1d..b3a34be63e99 100644 > --- a/drivers/dma/qcom/bam_dma.c > +++ b/drivers/dma/qcom/bam_dma.c > @@ -1350,7 +1350,7 @@ static int bam_dma_probe(struct platform_device *pdev) > if (ret) > goto err_unregister_dma; > > - if (bdev->controlled_remotely) { > + if (!bdev->bamclk) { > pm_runtime_disable(&pdev->dev); > return 0; > } > @@ -1438,10 +1438,10 @@ static int __maybe_unused bam_dma_suspend(struct device *dev) > { > struct bam_device *bdev = dev_get_drvdata(dev); > > - if (!bdev->controlled_remotely) > + if (bdev->bamclk) { > pm_runtime_force_suspend(dev); > - > - clk_unprepare(bdev->bamclk); > + clk_unprepare(bdev->bamclk); > + } > > return 0; > } > @@ -1451,12 +1451,14 @@ static int __maybe_unused bam_dma_resume(struct device *dev) > struct bam_device *bdev = dev_get_drvdata(dev); > int ret; > > - ret = clk_prepare(bdev->bamclk); > - if (ret) > - return ret; > + if (bdev->bamclk) { > + ret = clk_prepare(bdev->bamclk); > + if (ret) > + return ret; > > - if (!bdev->controlled_remotely) > - pm_runtime_force_resume(dev); > + if (!bdev->controlled_remotely) Why do we still need controlled_remotely check here? Shawn > + pm_runtime_force_resume(dev); > + } > > return 0; > } > -- > 2.25.1 >
Hi Shawn, Thanks for the review On 1/22/21 12:10 AM, Shawn Guo wrote: > On Thu, Jan 21, 2021 at 09:52:51PM -0500, Thara Gopinath wrote: >> When bam dma is "controlled remotely", thus far clocks were not controlled >> from the Linux. In this scenario, Linux was disabling runtime pm in bam dma >> driver and not doing any clock management in suspend/resume hooks. >> >> With introduction of crypto engine bam dma, the clock is a rpmh resource >> that can be controlled from both Linux and TZ/remote side. Now bam dma >> clock is getting enabled during probe even though the bam dma can be >> "controlled remotely". But due to clocks not being handled properly, >> bam_suspend generates a unbalanced clk_unprepare warning during system >> suspend. >> >> To fix the above issue and to enable proper clock-management, this patch >> enables runtim-pm and handles bam dma clocks in suspend/resume hooks if >> the clock node is present irrespective of controlled_remotely property. > > Shouldn't the following probe code need some update? Now we have both > controlled_remotely and clocks handle for cryptobam node. For example, > if devm_clk_get() returns -EPROBE_DEFER, we do not want to continue with > bamclk forcing to be NULL, right? We still will have to set bdev->bamclk to NULL in certain scenarios. For eg slimbus bam dma is controlled-remotely and the clocks are handled by the remote s/w. Linux does not handle the clocks at all and there is no clock specified in the dt node.This is the norm for the devices that are also controlled by remote s/w. Crypto bam dma is a special case where the clock is actually a rpmh resource and hence can be independently handled from both remote side and Linux by voting. In this case, the dma is controlled remotely but clock can be turned off and on in Linux. Hence the need for this patch. Yes, the probe code needs updating to handle -EPROBE_DEFER (esp if the clock driver is built in as a module) I am not sure if the clock framework handles -EPROBE_DEFER properly either. So that might need updating too. This is a separate activity and not part of this patch. > > 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; > } > >> >> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> >> --- >> drivers/dma/qcom/bam_dma.c | 20 +++++++++++--------- >> 1 file changed, 11 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c >> index 88579857ca1d..b3a34be63e99 100644 >> --- a/drivers/dma/qcom/bam_dma.c >> +++ b/drivers/dma/qcom/bam_dma.c >> @@ -1350,7 +1350,7 @@ static int bam_dma_probe(struct platform_device *pdev) >> if (ret) >> goto err_unregister_dma; >> >> - if (bdev->controlled_remotely) { >> + if (!bdev->bamclk) { >> pm_runtime_disable(&pdev->dev); >> return 0; >> } >> @@ -1438,10 +1438,10 @@ static int __maybe_unused bam_dma_suspend(struct device *dev) >> { >> struct bam_device *bdev = dev_get_drvdata(dev); >> >> - if (!bdev->controlled_remotely) >> + if (bdev->bamclk) { >> pm_runtime_force_suspend(dev); >> - >> - clk_unprepare(bdev->bamclk); >> + clk_unprepare(bdev->bamclk); >> + } >> >> return 0; >> } >> @@ -1451,12 +1451,14 @@ static int __maybe_unused bam_dma_resume(struct device *dev) >> struct bam_device *bdev = dev_get_drvdata(dev); >> int ret; >> >> - ret = clk_prepare(bdev->bamclk); >> - if (ret) >> - return ret; >> + if (bdev->bamclk) { >> + ret = clk_prepare(bdev->bamclk); >> + if (ret) >> + return ret; >> >> - if (!bdev->controlled_remotely) >> - pm_runtime_force_resume(dev); >> + if (!bdev->controlled_remotely) > > Why do we still need controlled_remotely check here? Yes you are right. This should be removed.I will send v2. -- Warm Regards Thara
On Fri, Jan 22, 2021 at 10:44:09AM -0500, Thara Gopinath wrote: > Hi Shawn, > > Thanks for the review > > On 1/22/21 12:10 AM, Shawn Guo wrote: > > On Thu, Jan 21, 2021 at 09:52:51PM -0500, Thara Gopinath wrote: > > > When bam dma is "controlled remotely", thus far clocks were not controlled > > > from the Linux. In this scenario, Linux was disabling runtime pm in bam dma > > > driver and not doing any clock management in suspend/resume hooks. > > > > > > With introduction of crypto engine bam dma, the clock is a rpmh resource > > > that can be controlled from both Linux and TZ/remote side. Now bam dma > > > clock is getting enabled during probe even though the bam dma can be > > > "controlled remotely". But due to clocks not being handled properly, > > > bam_suspend generates a unbalanced clk_unprepare warning during system > > > suspend. > > > > > > To fix the above issue and to enable proper clock-management, this patch > > > enables runtim-pm and handles bam dma clocks in suspend/resume hooks if > > > the clock node is present irrespective of controlled_remotely property. > > > > Shouldn't the following probe code need some update? Now we have both > > controlled_remotely and clocks handle for cryptobam node. For example, > > if devm_clk_get() returns -EPROBE_DEFER, we do not want to continue with > > bamclk forcing to be NULL, right? > > We still will have to set bdev->bamclk to NULL in certain scenarios. For eg > slimbus bam dma is controlled-remotely and the clocks are handled by the > remote s/w. Linux does not handle the clocks at all and there is no clock > specified in the dt node.This is the norm for the devices that are also > controlled by remote s/w. Crypto bam dma is a special case where the clock > is actually a rpmh resource and hence can be independently handled from both > remote side and Linux by voting. In this case, the dma is controlled > remotely but clock can be turned off and on in Linux. Hence the need for > this patch. So is it correct to say that clock is mandatory for !controlled-remotely BAM, while it's optional for controlled-remotely one. If yes, maybe we can do something like below to make the code a bit easier to read? if (controlled-remotely) bdev->bamclk = devm_clk_get_optional(); else bdev->bamclk = devm_clk_get(); > Yes, the probe code needs updating to handle -EPROBE_DEFER (esp if the clock > driver is built in as a module) I am not sure if the clock framework handles > -EPROBE_DEFER properly either. So that > might need updating too. This is a separate activity and not part of this > patch. As the patch breaks the assumption that for controlled-remotely BAM there is no clock to be managed, the probe code becomes buggy right away. Shawn
Hi Shawn, On 1/23/21 2:19 AM, Shawn Guo wrote: > On Fri, Jan 22, 2021 at 10:44:09AM -0500, Thara Gopinath wrote: >> Hi Shawn, >> >> Thanks for the review >> >> On 1/22/21 12:10 AM, Shawn Guo wrote: >>> On Thu, Jan 21, 2021 at 09:52:51PM -0500, Thara Gopinath wrote: >>>> When bam dma is "controlled remotely", thus far clocks were not controlled >>>> from the Linux. In this scenario, Linux was disabling runtime pm in bam dma >>>> driver and not doing any clock management in suspend/resume hooks. >>>> >>>> With introduction of crypto engine bam dma, the clock is a rpmh resource >>>> that can be controlled from both Linux and TZ/remote side. Now bam dma >>>> clock is getting enabled during probe even though the bam dma can be >>>> "controlled remotely". But due to clocks not being handled properly, >>>> bam_suspend generates a unbalanced clk_unprepare warning during system >>>> suspend. >>>> >>>> To fix the above issue and to enable proper clock-management, this patch >>>> enables runtim-pm and handles bam dma clocks in suspend/resume hooks if >>>> the clock node is present irrespective of controlled_remotely property. >>> >>> Shouldn't the following probe code need some update? Now we have both >>> controlled_remotely and clocks handle for cryptobam node. For example, >>> if devm_clk_get() returns -EPROBE_DEFER, we do not want to continue with >>> bamclk forcing to be NULL, right? >> >> We still will have to set bdev->bamclk to NULL in certain scenarios. For eg >> slimbus bam dma is controlled-remotely and the clocks are handled by the >> remote s/w. Linux does not handle the clocks at all and there is no clock >> specified in the dt node.This is the norm for the devices that are also >> controlled by remote s/w. Crypto bam dma is a special case where the clock >> is actually a rpmh resource and hence can be independently handled from both >> remote side and Linux by voting. In this case, the dma is controlled >> remotely but clock can be turned off and on in Linux. Hence the need for >> this patch. > > So is it correct to say that clock is mandatory for !controlled-remotely > BAM, while it's optional for controlled-remotely one. If yes, maybe we > can do something like below to make the code a bit easier to read? Yes. Sure. I will change it to below. > > if (controlled-remotely) > bdev->bamclk = devm_clk_get_optional(); > else > bdev->bamclk = devm_clk_get(); > >> Yes, the probe code needs updating to handle -EPROBE_DEFER (esp if the clock >> driver is built in as a module) I am not sure if the clock framework handles >> -EPROBE_DEFER properly either. So that >> might need updating too. This is a separate activity and not part of this >> patch > > > As the patch breaks the assumption that for controlled-remotely BAM > there is no clock to be managed, the probe code becomes buggy right > away. mmm... not really. Either ways we don't handle -EPROBE_DEFER from clock code. That behavior is not worse because of this patch. I can send a separate patch to fix the -EPROBE_DEFER issue. > > Shawn > -- Warm Regards Thara
diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c index 88579857ca1d..b3a34be63e99 100644 --- a/drivers/dma/qcom/bam_dma.c +++ b/drivers/dma/qcom/bam_dma.c @@ -1350,7 +1350,7 @@ static int bam_dma_probe(struct platform_device *pdev) if (ret) goto err_unregister_dma; - if (bdev->controlled_remotely) { + if (!bdev->bamclk) { pm_runtime_disable(&pdev->dev); return 0; } @@ -1438,10 +1438,10 @@ static int __maybe_unused bam_dma_suspend(struct device *dev) { struct bam_device *bdev = dev_get_drvdata(dev); - if (!bdev->controlled_remotely) + if (bdev->bamclk) { pm_runtime_force_suspend(dev); - - clk_unprepare(bdev->bamclk); + clk_unprepare(bdev->bamclk); + } return 0; } @@ -1451,12 +1451,14 @@ static int __maybe_unused bam_dma_resume(struct device *dev) struct bam_device *bdev = dev_get_drvdata(dev); int ret; - ret = clk_prepare(bdev->bamclk); - if (ret) - return ret; + if (bdev->bamclk) { + ret = clk_prepare(bdev->bamclk); + if (ret) + return ret; - if (!bdev->controlled_remotely) - pm_runtime_force_resume(dev); + if (!bdev->controlled_remotely) + pm_runtime_force_resume(dev); + } return 0; }
When bam dma is "controlled remotely", thus far clocks were not controlled from the Linux. In this scenario, Linux was disabling runtime pm in bam dma driver and not doing any clock management in suspend/resume hooks. With introduction of crypto engine bam dma, the clock is a rpmh resource that can be controlled from both Linux and TZ/remote side. Now bam dma clock is getting enabled during probe even though the bam dma can be "controlled remotely". But due to clocks not being handled properly, bam_suspend generates a unbalanced clk_unprepare warning during system suspend. To fix the above issue and to enable proper clock-management, this patch enables runtim-pm and handles bam dma clocks in suspend/resume hooks if the clock node is present irrespective of controlled_remotely property. Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> --- drivers/dma/qcom/bam_dma.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) -- 2.25.1