Hi Thierry,

On Mon, Sep 29, 2014 at 8:18 PM, Thierry Reding
[off-list ref] wrote:

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.

As far as the kernel is concerned, this is a solved problem.

Firmware is going to be doing some dark magic to set up the hardware
to be a dumb frame buffer and some other stuff to add the simplefb
device node - so by this point, adding the clocks (or whatever)
required by the hardware should be fairly uncomplicated - the firmware
already knows the hardware intimately. As for the actual device tree
manipulations, U-boot (or whatever) will probably just grow some
helper functions to make this simple.

Alternatively, it could simply add the relevant data to an existing
device node and munge it's compatible property so simplefb picks it
up.

quoted

quoted

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.

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.

Passing in clock information via the device tree already requires a non-
trivial amount of code in the firmware. A similar amount of code would
be necessary for each type of resource that needs to be kept enabled. In
addition to the above some devices may also require resources that have
no generic bindings. That just doesn't scale.

The only reasonable thing for simplefb to do is not deal with any kind
of resource at all (except perhaps area that contains the framebuffer
memory).

You should really read that thread:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/284726.html

It's quite interesting, because you'll see that:
A) Your approach, even on the platform you're working on, doesn't
   work. Or at least, isn't reliable.

What platform exactly do you think I'm working on? 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.

quoted

B) Other maintainers, precisely like Mark, came to the same conclusion
   than Mike.

Well, and others didn't.

Also I think if you read that thread and look at my proposal it matches
exactly what was discussed as one of the solutions at one point in the
thread.

quoted

quoted

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.

quoted

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, so duplicating what the real driver does within the simplefb
driver is just that, duplication. We don't allow duplication anywhere
else in the kernel, why should simplefb be an exception?

The various subsystems could just grow some accessor functions (where
required) which, with devm, should be as simple as ~10 lines per
subsystem. With ~5 subsystems this is ~50 lines of code; Or we can
disable the automatic disabling of resources and put the system in a
potentially unstable state; Or each subarch could use some shim driver
or some arch code to grab the clocks for simplefb, and we end up with
each subarch having it's own, slightly different version of what is
essentially the same code; Or we have to have every single KMS driver
built in and bloat the kernel.

With ~50 lines of generic code added to simplefb, distros get their
slim multi-subarch kernels, your driver is still generic, power
management works and users get pretty graphics from bootloader to
desktop.

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.

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

If simplefb has called clk_get() then nobody can disable the clocks
it's depending on.

Furthermore isn't it a bug for a driver to call clk_disable() before a
preceding clk_enable()? There are patches being worked on that will
enable per-user clocks and as I understand it they will specifically
disallow drivers to disable the hardware clock if other drivers are
still keeping them on via their own referenc.

Thierry

Thanks,

-- 
Julian Calaby

Email: julian.calaby at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/