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

[Maxime Ripard]

On Mon, Sep 29, 2014 at 12:18:08PM +0200, Thierry Reding wrote:
> On Mon, Sep 29, 2014 at 11:23:01AM +0200, Maxime Ripard wrote:
> > On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
> [...]
> > > simplefb doesn't deal at all with hardware details. It simply uses what
> > > firmware has set up, which is the only reason why it will work for many
> > > people. What is passed in via its device tree node is the minimum amount
> > > of information needed to draw something into the framebuffer. Also note
> > > that the simplefb device tree node is not statically added to a DTS file
> > > but needs to be dynamically generated by firmware at runtime.
> > 
> > Which makes the whole even simpler, since the firmware already knows
> > all about which clocks it had to enable.
> 
> It makes things very complicated in the firmware because it now needs to
> be able to generate DTB content that we would otherwise be able to do
> much easier with a text editor.

Didn't you just say that it was dynamically generated at runtime? So
we can just ignore the "text editor" case.

> > > If we start extending the binding with board-level details we end up
> > > duplicating the device tree node for the proper video device. Also note
> > > that it won't stop at clocks. Other setups will require regulators to be
> > > listed in this device tree node as well so that they don't get disabled
> > > at late_initcall. And the regulator bindings don't provide a method to
> > > list an arbitrary number of clocks in a single property in the way that
> > > the clocks property works.
> > > 
> > > There may be also resets involved. Fortunately the reset framework is
> > > minimalistic enough not to care about asserting all unused resets at
> > > late_initcall. And other things like power domains may also need to be
> > > kept on.
> > > 
> > > Passing in clock information via the device tree already requires a non-
> > > trivial amount of code in the firmware. A similar amount of code would
> > > be necessary for each type of resource that needs to be kept enabled. In
> > > addition to the above some devices may also require resources that have
> > > no generic bindings. That just doesn't scale.
> > > 
> > > The only reasonable thing for simplefb to do is not deal with any kind
> > > of resource at all (except perhaps area that contains the framebuffer
> > > memory).
> > 
> > You should really read that thread:
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/284726.html
> > 
> > It's quite interesting, because you'll see that:
> > A) Your approach, even on the platform you're working on, doesn't
> >    work. Or at least, isn't reliable.
> 
> What platform exactly do you think I'm working on?

My bad, I thought it was a tegra SoC in there. I should have read more
carefully obviously.

> Why do you think what I proposed isn't going to work or be reliable?
> I don't see any arguments in the thread that would imply that.

The fact that it broke in the first place?

> > B) Other maintainers, precisely like Mark, came to the same conclusion
> >    than Mike.
> 
> Well, and others didn't.

We've been talking about both clocks and regulators up to now. I can
see Mike and Mark both suggesting to use the usual clocks and
regulators APIs, either in that thread or the one I pointed out.

> Also I think if you read that thread and look at my proposal it matches
> exactly what was discussed as one of the solutions at one point in the
> thread.

I've seen it, and replied to that already.

> > > So how about instead of requiring resources to be explicitly claimed we
> > > introduce something like the below patch? The intention being to give
> > > "firmware device" drivers a way of signalling to the clock framework
> > > that they need rely on clocks set up by firmware and when they no longer
> > > need them. This implements essentially what Mark (CC'ing again on this
> > > subthread) suggested earlier in this thread. Basically, it will allow
> > > drivers to determine the time when unused clocks are really unused. It
> > > will of course only work when used correctly by drivers. For the case of
> > > simplefb I'd expect its .probe() implementation to call the new
> > > clk_ignore_unused() function and once it has handed over control of the
> > > display hardware to the real driver it can call clk_unignore_unused() to
> > > signal that all unused clocks that it cares about have now been claimed.
> > > This is "reference counted" and can therefore be used by more than a
> > > single driver if necessary. Similar functionality could be added for
> > > other resource subsystems as needed.
> > 
> > So, just to be clear, instead of doing a generic clk_get and
> > clk_prepare_enable, you're willing to do a just as much generic
> > clk_ignore_unused call?
> 
> Yes.
> 
> > How is that less generic?
> 
> It's more generic. That's the whole point.
> 
> The difference is that with the solution I proposed we don't have to
> keep track of all the resources. We know that firmware has set them up
> and we know that a real driver will properly take them over at some
> point

You keep saying that... and you know that you can't make this
assumption.

> so duplicating what the real driver does within the simplefb driver
> is just that, duplication. We don't allow duplication anywhere else
> in the kernel, why should simplefb be an exception?

Oh come on. Since when a clk_prepare_enable call is duplication?

If so, then we really have a duplication issue, and it's not just for
the clock API.

> > You know that you are going to call that for regulator, reset, power
> > domains, just as you would have needed to with the proper API, unless
> > that with this kind of solution, you would have to modify *every*
> > framework that might interact with any resource involved in getting
> > simplefb running?
> 
> We have to add handling for every kind of resource either way. Also if
> this evolves into a common pattern we can easily wrap it up in a single
> function call.

Unless that in one case, we already have everything needed to handle
everything properly, and in another, you keep hacking more and more
into the involved frameworks.

> > 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.
> 
> If we need to prevent that, then that's something that could be fixed,
> too.

See, you keep hacking it more...

> But both this and the other thread at least agree on the fact that
> simplefb is a shim driver that's going to be replaced by something real
> at some point

I think our definition of "at some point" diverges. Yours seem to be
"at some point during the boot process", which might not even happen
in some platforms (or at least in the foreseeable kernel releases).

> so hopefully concurrent users aren't the problem because that would
> cause the real driver to break too.
> 
> Also note that if some other driver could call clk_disable() it could
> just as easily call clk_set_rate() and break simplefb.

This is not true, see my other reply.

> 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.

It is, but I was talking about a clk_disable after a clk_enable.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140929/1cebdefc/attachment.sig>