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

Julian Calaby julian.calaby at gmail.com
Mon Sep 29 04:00:01 PDT 2014

Hi Thierry,

On Mon, Sep 29, 2014 at 8:18 PM, Thierry Reding
<thierry.reding at gmail.com> 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.

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

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

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


Julian Calaby

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

More information about the linux-arm-kernel mailing list