Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3517 bytes --]

On Mon, Sep 29, 2014 at 05:57:18PM +0200, Maxime Ripard wrote:
> On Mon, Sep 29, 2014 at 03:54:00PM +0200, Thierry Reding wrote:
> > On Mon, Sep 29, 2014 at 01:34:36PM +0200, Maxime Ripard wrote:
> > > On Mon, Sep 29, 2014 at 12:44:57PM +0200, Thierry Reding wrote:
> > > > > >> Plus, speaking more specifically about the clocks, that won't prevent
> > > > > >> your clock to be shut down as a side effect of a later clk_disable
> > > > > >> call from another driver.
> > > > > 
> > > > > > Furthermore isn't it a bug for a driver to call clk_disable() before a
> > > > > > preceding clk_enable()? There are patches being worked on that will
> > > > > > enable per-user clocks and as I understand it they will specifically
> > > > > > disallow drivers to disable the hardware clock if other drivers are
> > > > > > still keeping them on via their own referenc.
> > > > > 
> > > > > Calling clk_disable() preceding clk_enable() is a bug.
> > > > > 
> > > > > Calling clk_disable() after clk_enable() will disable the clock (and
> > > > > its parents)
> > > > > if the clock subsystem thinks there are no other users, which is what will
> > > > > happen here.
> > > > 
> > > > Right. I'm not sure this is really applicable to this situation, though.
> > > 
> > > It's actually very easy to do. Have a driver that probes, enables its
> > > clock, fails to probe for any reason, call clk_disable in its exit
> > > path. If there's no other user at that time of this particular clock
> > > tree, it will be shut down. Bam. You just lost your framebuffer.
> > > 
> > > Really, it's just that simple, and relying on the fact that some other
> > > user of the same clock tree will always be their is beyond fragile.
> > 
> > Perhaps the meaning clk_ignore_unused should be revised, then. What you
> > describe isn't at all what I'd expect from such an option. And it does
> > not match the description in Documentation/kernel-parameters.txt either.
> 
> Well, it never says that it also prevent them from being disabled
> later on. But it's true it's not very explicit.

It says:

	Keep all clocks already enabled by bootloader on,
	even if no driver has claimed them. ...

There's no "until" or anything there, so I interpret that as
indefinitely.

> > > > Either way, if there are other users of a clock then they will just as
> > > > likely want to modify the rate at which point simplefb will break just
> > > > as badly.
> > > 
> > > And this can be handled just as well. Register a clock notifier,
> > > refuse any rate change, done. But of course, that would require having
> > > a clock handle.
> > > 
> > > Now, how would *you* prevent such a change?
> > 
> > Like I said in the other thread. If you have two drivers that use the
> > same clock but need different frequencies you've lost anyway.
> 
> Except that the driver that has the most logic (ie not simplefb) will
> have a way to recover and deal with that.

You're making an assumption here. Why would the driver have such logic
if nothing ever prevented it from setting the rate properly before.

> But sure, you can still try to point new issues, get an obvious and
> robust solution, and then discard the issue when the solution doesn't
> go your way...

And you've already proven that you're completely unwilling to even
consider any other solution than what was originally proposed, so I
really don't see how discussing this further with you is going to be
productive.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]