Re: [PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



你好,

On 01/02/2023 13:28, Roger Lu (陸瑞傑) wrote:
Hi Matthias Sir,

On Tue, 2023-01-31 at 14:19 +0100, Matthias Brugger wrote:

On 11/01/2023 08:45, Roger Lu wrote:
In MediaTek HW design, svs and thermal both use the same clk source.
It means that svs clk reference count from CCF includes thermal control
counts. That makes svs driver confuse whether it disabled svs's main clk
or not from CCF's perspective and lead to turn off their shared clk
unexpectedly. Therefore, we add svs clk control APIs to make sure svs's
main clk is controlled well by svs driver itself.

Here is a NG example. Rely on CCF's reference count and cause problem.

thermal probe (clk ref = 1)
-> svs probe (clk ref = 2)
     -> svs suspend (clk ref = 1)
        -> thermal suspend (clk ref = 0)
        -> thermal resume (clk ref = 1)
     -> svs resume (encounter error, clk ref = 1)
     -> svs suspend (clk ref = 0)
        -> thermal suspend (Fail here, thermal HW control w/o clk)

Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare() on
err in svs_resume()")
Signed-off-by: Roger Lu <roger.lu@xxxxxxxxxxxx>

That looks wrong. Although I don't out of my mind, there should be a way to
tell
the clock framework that this clock is shared between several devices.

I wonder if using clk_enable and clk_disable in svs_resume/suspend wouldn't
be
enough.

Oh yes, Common Clock Framework (CCF) knows the clock shared between several
devices and maintains clock "on/off" by reference count.


The thing is if you use clk_prepare_enable then the clock framework check's if the clock is already prepared, which could happen like you described in the svs_resume (encount error) case in the commit message. The question is, can't we just use clk_enable and clk_disable in resume/suspend and only prepare the clock in the probe function?

We concern how to stop running svs_suspend() when svs clk is already disabled by
svs_resume(). Take an example as below, if we refers to __clk_is_enabled()
result for knowing svs clk status, it will return "true" all the time because
thermal clk is still on. This causes the problem mentioned in commit message.


I would expect that the kernel takes care that we can't enter a resume path for a device before the suspend path has finished. Honestly I don't really understand the problem here. It seems something different then what you described in the commit message.

Please help me understand better.

谢谢,再见

Matthias

static int svs_suspend(struct device *dev)
{
...
	if (!__clk_is_enabled(svsp->main_clk)) //always get `true`
		return 0;
...
}


Regards,
Matthias

... [snip] ...



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux