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

[Thierry Reding]

On Mon, Sep 29, 2014 at 01:46:43PM +0200, Maxime Ripard wrote:
> 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.

Perhaps read the sentence again. I said "that we would *otherwise* be
able to do much easier with a text editor.".

My point remains that there shouldn't be a need to generate DTB content
of this complexity at all.

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

That's exactly the point. And it's going to break again and again as
simplefb is extended with new things. Generally DT bindings should be
backwards compatible. When extended they should provide a way to fall
back to a reasonable default. There's simply no way you can do that
with simplefb.

What happened in the Snow example is that regulators that were
previously on would all of a sudden be automatically disabled on boot
because there was now a driver that registered them with a generic
framework.

The same thing is going to happen with simplefb for your device. If you
later realize that you need a regulator to keep the panel going, you'll
have to add code to your firmware to populate the corresponding
properties, otherwise the regulator will end up unused and will be
automatically disabled. At the same time you're going to break upstream
for all users of your old firmware because it doesn't add that property
yet.

And the same will continue to happen for every new type of resource
you're going to add.

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

Why not? Are you really expecting to keep running with simplefb forever?
Nobody is going to seriously use an upstream kernel in any product with
only simplefb as a framebuffer. I've said before that this is a hack to
get you working display. And that's all it is. If you want to do it
properly go and write a DRM/KMS driver.

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

This is a fundamental issue that we are facing and I'm trying to come up
with a solution that is future-proof and will work for drivers other
than simplefb.

Just because we currently lack this functionality doesn't make it a hack
trying to add it.

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

If you don't see this as a problem that exists beyond simplefb then I
would of course not expect you to think that anything needs fixing.

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

Then instead of hacking existing drivers to work on your particular
platform you should start looking into hacking your platform drivers to
cope with the lack of a proper display driver. Or alternatively spend
the time getting a proper display driver merged.

Whether simplefb is used as primary framebuffer or just during only boot
until hand-off to a real driver, it still remains a stop-gap solution.

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

If you mean that you could register a clock notifier to prevent clock
changes, then that's going to lead to other drivers failing since they
can now no longer set the rate that they need.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140929/be42a7b5/attachment.sig>