Re: [PATCH v3 2/3] clk: Add clk_poll_disable_unprepare()

[Stephen Boyd <sboyd@kernel.org>]

Quoting Laurent Pinchart (2024-03-18 15:55:46)
> On Mon, Mar 18, 2024 at 11:08:41AM +0000, Biju Das wrote:
> > +     int ret;
> > +
> > +     clk_disable(clk);
> > +     ret = clk_poll_disabled(clk, sleep_us, timeout_us);
> > +     clk_unprepare(clk);
> 
> What happens in the clk_disable_unprepare() case, if the clk_unprepare()
> function is called on a clock that hasn't been synchronously disabled ?
> This is ill-defined, a clock provider driver that implements .disable()
> asynchronously would see its .unprepare() operation called with
> different clock states. That behaviour is error-prone, especially given
> that it could be difficult to test clock provider drivers to ensure that
> handle both cases correctly.
> 
> One option could be to turn the .unprepare() operation into a
> synchronization point, requiring drivers that implement .disable()
> asynchronously to implement synchronization in .unprepare(). That way
> you wouldn't need a new API function for clock consumers. The downside
> is that consumers that call clk_disable_unprepare() will never benefit
> from the .disable() operation being asynchronous, which may defeat the
> whole point of this exercise.
> 
> I'm starting to wonder if the simplest option in your case wouldn't be
> to make your clock provider synchronous for the vclk...

Yes. This all looks unnecessary if the device using the clk always
requires the clk to actually be shut down when it is disabled. Just do
that for this vclk and move on. It _is_ tightly coupling this specific
clk to the specific consumer, but that's simplest and most expedient to
implement, i.e. it's practical.

We would only ever need this API if we had a clk consumer which
absolutely required the clk to stop clocking upon returning from
clk_unprepare(), and that clk could be any random clk. It sounds like in
this case that isn't true. We know which clk must be off when
clk_unprepare() returns, so just implement the wait in the
clk_ops::unprepare() callback?