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

[Thierry Reding]

On Mon, Sep 29, 2014 at 06:28:14PM +0200, Maxime Ripard wrote:
> 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?

My point is that if we decide not to consider all resources handled by
firmware then we need to go all the way. At that point you start having
problems as evidenced by the Snow example.

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

No it's not what I want. *You* want to add resource management to this
driver. What I'm saying is that if we start adding clocks then we can no
longer say no to resets or regulators or power domains either.

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

If you think it's a bad idea to rely on firmware then you shouldn't be
using simplefb in the first place.

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

You're not getting it are you? What makes you think sunxi-uboot-fb is
going to be any different? This isn't about a name.

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

We've been over this before. I'm tired of having to repeat myself.

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

Can they? That's a pretty bold assumption.

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/20140930/07ad3f26/attachment.sig>