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

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

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

On Mon, Sep 29, 2014 at 05:11:01PM +0100, Mark Brown wrote:
> On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
> > On Sat, Sep 27, 2014 at 04:56:01PM -0700, Mike Turquette wrote:
> 
> > > With that said I think that Luc's approach is very sensible. I'm not
> > > sure what purpose in the universe DT is supposed to serve if not for
> > > _this_exact_case_. We have a nice abstracted driver, usable by many
> > > people. The hardware details of how it is hooked up at the board level
> > > can all be hidden behind stable DT bindings that everyone already uses.
> 
> > 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.
> 
> > 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.
> 
> Not really thought this through fully yet but would having phandles to
> the relevant devices do the job?  Kind of lines up with Grant's idea of
> having dummy drivers.

One of the arguments that came up during the discussion of the sunxi
patches is that simplefb is going to be used precisely because there is
no real driver for the display part of the SoC yet and nobody knows what
the binding will look like. So there's nothing to point at currently and
for the same reason having a dummy driver won't work. There's simply no
definition of what resources are needed.

> > 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.
> 
> We might want to do that in the future, though it's not always the case
> that reset is the lowest power state.

That proves my point. If we ever decide to assert resets by default
we'll have yet another subsystem that can potentially break existing
DTs.

In the end it brings us back to the very fundamental principles of DT
that's been causing so much pain. For things to work properly and in a
stable way you have to get the bindings right and complete from the
start. That is, it needs to describe every aspect of the hardware block
and all links to other components.

But there is no way you can do that for a virtual device like simplefb
because it is a generic abstraction for hardware that varies wildly. The
only way to make it work is to abstract away all the resource management
and consider it to be done elsewhere.

> > 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.
> 
> One thing that makes me a bit nervous about this approach in the context
> of the regulator API is the frequency with which one finds shared
> supplies.  I'm not sure if it's actually a big problem in practice but
> it makes me worry a bit.  We could probably just do something like make
> refcounting down to zero not actually disable anything for standard
> regulators to deal with it which might be an idea anyway in the context
> of this sort of dodge.

Yes, that's sort of how I expected clk_ignore_unused to work. The way I
understood it, it would cause all unused clocks to be ignored (that is
stay enabled if they are).

Of course as Geert pointed out in another subthread, taking this all the
way means that we have to disable all power management because the
firmware device may be sharing resources with other devices and which
therefore are not unused. That's a pretty strong argument and I don't
have a solution for that. It is only really a problem for cases where
the firmware virtual device isn't taken over by a proper driver at some
point, though.

Thierry

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