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

[Maxime Ripard]

On Mon, Sep 29, 2014 at 03:47:15PM +0200, Thierry Reding wrote:
> 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.

This one *is* backward compatible. It doesn't even change the simplefb
behaviour, compared to what it was doing before, if you don't have
this clocks property. What can be a more reasonable default that the
way it used to behave?

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

Sure, we can add any resources we will need. Regulators, reset lines,
pm domains, allocated memory, but I'm not really sure this is what you
want, right?

And if you don't do it, you end up in situations where you have to
tell your users "disable the regulator driver in order for your
display to work".
 
> > > > > 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.

This is a WiP. And I really have the feeling it will be written,
reviewed and merged before we can have the "dirty hack" working.

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

Or maybe it's just showing that the trend to rely more and more on the
firmware is a bad idea?

I really start to consider adding a sunxi-uboot-fb, that would just
duplicate the source code of simplefb but also taking the
clocks. Somehow, I feel like it will be easier (and definitely less of
a hack than using the generic common clock API).

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

Then please point me to something else that needs fixing, so that I
can see the whole picture.

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

Then really, simplefb deserves a more appropriate name. Like
uselessfb, tegrafb, DONOTUSEITYOUSTUPIDDEVfb or whatever makes it not
look generic.

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

But they can recover. Something that simplefb cannot do.

-- 
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/0bf9ed5d/attachment-0001.sig>