Messages in this thread | | | Subject | Re: [PATCH] drivers: dma: qcom: bam_dma: Manage clocks when controlled_remotely is set | From | Thara Gopinath <> | Date | Mon, 25 Jan 2021 08:33:15 -0500 |
| |
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
| |