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
