Re: [PATCH v2] mailbox: remove runtime GCE clk control

[Jason-JH Lin (林睿祥)]

Hi Angelo,

Thanks for the reviews.

On Wed, 2023-10-04 at 11:20 +0200, AngeloGioacchino Del Regno wrote:
> Il 04/10/23 10:54, Jason-JH.Lin ha scritto:
> > 1. GCE is a frequently used module, so runtime controlling
> > GCE clock won't save too much power and its original design
> > doesn't expect it to be enabled and disabled too frequently.
> > 
> > 2. Runtime controlling GCE clock will cause display HW register
> > configured in worng stream done event issue below:
> >    GCE should config HW in every vblanking duration.
> >    The stream done event is the start signal of vblanking.
> > 
> >    If stream done event is sent between GCE clk_disable
> >    and clk_enable. After GCE clk_enable the stream done event
> >    may not appear immediately and have about 3us delay.
> > 
> >    Normal case:
> >    clk_disable -> get EventA -> clk_enable -> clear EventA
> >    -> wait EventB -> get EventB -> config HW
> > 
> >    Abnormal case:
> >    clk_disable -> get EventA -> clk_enable -> EventA delay appear
> >    -> clear EventA fail -> wait EventB but get EventA -> config HW
> > 
> > So just remove the runtime GCE clock contorl.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@xxxxxxxxxxxx>
> 
> Instead of entirely removing the logic that controls the clocks and
> always
> refuse to save power, what about using autosuspend?
> 
> If the two cases that you're describing are happening always in a
> range of
> time, we could *yes* remove the "manual" bulk disable/enable calls,
> but then
> we could use runtime_suspend/runtime_resume callbacks for that.
> 
> Hint: pm_runtime_set_autosuspend_delay(dev, 1000);
> 
> Regards,
> Angelo
> 

These 2 issues are caused by GCE bulk_clk enable/disable too
frequently.

As I now, pm_runtime_set_autosuspend_delay() is for controlling the
power domain. The power domain of GCE is infrasys which can only be
enabled/disabled by spm during the whole system resume/suspend.
So I'm not sure about how can pm_runtime_set_autosuspend_delay() save 
power for GCE bulk_clk in this case.

Could you give more hint for me please?

Regards,
Jason-JH.Lin