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:

quoted

Hi Shawn,

Thanks for the review

On 1/22/21 12:10 AM, Shawn Guo wrote:

quoted

On Thu, Jan 21, 2021 at 09:52:51PM -0500, Thara Gopinath wrote:

quoted

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();
		

quoted

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