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

[Michal Suchanek]

On 29 September 2014 15:47, Thierry Reding <thierry.reding at gmail.com> 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.
>
> 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.

So what? You get a driver for regulators and suddenly find that
nothing registered regulators because they were on all the time anyway
and everything breaks? What a surprise!

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

Sure. And what can you do about that? It's not like the original Snow
firmware writes anything of use to the DT at all. So to run a
development kernel you need a development firmware. If you add new
features to the kernel that involve intefacing to the firmware you
need to update the firmware as well. Once support for Snow platform is
stable you can expect that users can use a stable release of firmware
indefinitely.

>
> And the same will continue to happen for every new type of resource
> you're going to add.

The like 5 resource types, yes. Some of which may not even apply to simplefb.

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

Why not? You can use shadowfb (software acceleration) with simplefb
all right. Shadowfb is hands down the fastest video acceleration we
have on anything but hardware that has _very_ slow CPU or that can run
Intel UXA drivers. With manufacturers adding more and more superfluous
cores to the CPUs shadowfb is actually not too stressing on the
system, either.

On hardware like Allwinner A13 (single core) the use of shadowfb over
actual video acceleration hardware has its benefits and drawbacks and
is neither clearly better nor worse. The same hardware tends to have
only one fixed video output - a tablet LCD panel. On such hardware
modesetting is needless luxury if u-boot provides the simplfb
bindings, at least for some use cases.

And there is still the use of simplefb during development and for
generic distribution kernels. I prefer that during development of the
KMS driver and during bootup before the KMS module becomes available
the system behaves normally, not in special hacked way that is useless
for anything but this special system mode. Worse, such special mode
will not be regularly tested on well supported hardware and will
likely bitrot and lead to mysterious issues at boot time when KMS is
not available and this special system mode is in effect.

> get you working display. And that's all it is. If you want to do it
> properly go and write a DRM/KMS driver.
>
>> > > 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.

How is proper resource management not going to work for drivers other
than simplefb?

Thanks

Michal