On Tue, Sep 30, 2014 at 07:21:11AM +0200, Thierry Reding wrote:

On Mon, Sep 29, 2014 at 06:28:14PM +0200, Maxime Ripard wrote:

quoted

On Mon, Sep 29, 2014 at 03:47:15PM +0200, Thierry Reding wrote:

quoted

On Mon, Sep 29, 2014 at 01:46:43PM +0200, Maxime Ripard wrote:

quoted

On Mon, Sep 29, 2014 at 12:18:08PM +0200, Thierry Reding wrote:

quoted

On Mon, Sep 29, 2014 at 11:23:01AM +0200, Maxime Ripard wrote:

quoted

On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:

[...]

quoted

quoted

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.

quoted

quoted

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.

Agreed.

quoted

quoted

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.

Yes, because resource management can be more than just "keep the thing
enabled". It might also be about not modifying anything, just like we
saw for the clocks, but that might also apply to regulators voltage.

And the only way I can think of to deal with that properly is to have
resources management in the driver.

quoted

quoted

quoted

quoted

quoted

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.

quoted

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.

At least, we would get to do any binding and resource management we
want. And that's a big win.

quoted

quoted

quoted

quoted

quoted

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.

quoted

quoted

quoted

quoted

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.

* They might 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/20140930/7db8c888/attachment.sig>