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

[Thierry Reding]

On Tue, Sep 30, 2014 at 11:38:50AM +0200, Michal Suchanek wrote:
> On 30 September 2014 10:54, Thierry Reding <thierry.reding at gmail.com> wrote:
> > On Tue, Sep 30, 2014 at 09:52:58AM +0200, Maxime Ripard wrote:
> >> 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:
> >> > > On Mon, Sep 29, 2014 at 03:47:15PM +0200, Thierry Reding wrote:
> > [...]
> >> > > > 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.
> >
> > We've already determined that simplefb can't do anything meaningful with
> > the resources other than keep them in the status quo. It simply doesn't
> > have enough knowledge to do so. It doesn't know the exact pixel clock or
> > what voltage the attached panel needs.
> >
> >> And the only way I can think of to deal with that properly is to have
> >> resources management in the driver.
> >
> > My point is that if we had a proper way to tell the kernel not to do
> > anything with resources owned by firmware, then the driver wouldn't
> > have to do anything with the resources.
> 
> The firmware on sunxi does not own any resources whatsoever. It ceases
> running once it executes the kernel. This is different from ACPI and
> UEFI where you have pieces of the firmware lingering indefinitely and
> potentially getting invoked by user pressing some button or some other
> hardware event. It is also different from rpi where the Linux kernel
> effectively runs in a virtual environment created by the firmware
> hypervisor.

You know all that because you of course wrote every single firmware
implementation that does and will ever exist for sunxi. There's nothing
keeping anyone from running UEFI on a sunxi SoC.

> So on sunxi and many other ARM machines the Linux kernel is the sole
> owner of any resources that might happen to be available on the
> machine. There is no firmware owning them when the Linux kernel is
> running, ever.

Of course this is part of the abstraction. The idea is that the device
is a virtual one created by firmware. Therefore firmware owns the
resources until the virtual device has been handed over to the kernel.

If you're into splitting hairs, then the simplefb device shouldn't exist
in the first place.

> And we do have a proper way to tell to the kernel what these resources
> are used for - inserting description of them into the simplefb DT
> node. Sure the simplefb cannot manage the resources in any way and but
> it does own them. When it is running they are in use, when it stops
> they are free to be reclaimed by the platform driver.

Yes. And again I'm not saying anything different. What I'm saying is
that we shouldn't need to know about the resources and instead hide that
within the firmware, for the same reason that we're already hiding the
register programming in hardware, namely to create an abstraction that
works irrespective of the underlying hardware.

> But you refuse to marge the kernel change for this proper management
> to happen.

I have no authority to merge these changes nor have I any way of vetoing
them. All I'm saying is that I think it's a bad idea. If nobody else
thinks it is then it will eventually get merged irrespective of what I'm
saying. And when that happens I'll shut up and live happily ever after.
But it doesn't magically convince me that it's a good idea.

> The alternate proposal to stop the kernel from managing resources
> while simplefb is running and refernce count drivers that cannot
> manage resources is indeed a workable, general alternative.
> 
> It does however switch the kernel into special mode when resources are
> not managed and will needlessly hinder eg. development and testing of
> powermanagement and hotplug for drivers unrelated to kms in parallel
> to kms.
> 
> But let's look at this special mode closer.
> 
> Under normal boot sequence when the built-in drivers are initialized
> or around that time the kernel does a pass in which it disables unused
> clocks and possibly reclaims other resources.
> 
> The boot has finished for the kernel and now it hands over to
> userspace and it is up to the userspace to load any more drivers (such
> as kms) or not. If at that point a driver like simplefb exists it
> should have called that stop_managing_resources() which should replace
> clk_ignore_unused() so that other resources are properly handled
> without the driver ever having to know about them.
> 
> For clocks this should be simple. At least on sunxi the clock driver
> can tell if the clock is gated and can potentially be in use. It can
> even mark clocks that are used at this point to know not to ever
> disable them. Also it should refuse to ever change a clock frequency
> on these clocks since if one of them was used for simplefb it should
> break. It does not matter it's a mmc bus clock completely unrelated to
> display. If you assume no knowledge you cannot change the mmc clock
> when a different card is inserted or when a card is inserted into an
> empty slot. If the firmware probed the slot the clock might be running
> anyway.
> 
> Other resources might be more difficult to tackle. Is it possible to
> change voltage regulators? Even the fact that it is set to 0 does not
> necessarily mean that it is unused and then other driver claiming it
> later can change the voltage. So no changing the regulators, ever.
> 
> And there goes gpio. You cannot assume that you can change any pin
> muxing or any pin level whatsoever. The pin might be connected to the
> display. So no loading of drivers that need any pin assignment
> changes.

The original patches don't tackle any of these problems. And in addition
to what's been mentioned elsewhere you're adding two new types of
resources that need to be potentially claimed here. Fortunately for GPIO
and pinmux the default is to not touch them at all unless explicitly
claimed and controlled. At least as far as I know.

> I guess by now it's clear that if this mode that assumes no knowledge
> of the underlying hardware and requires the kernel to not manage
> resources in order to keep dumb drivers running  completely paralyzes
> the kernel and will among other things prevent loading of the proper
> kms driver.

Yes, Geert already made that argument and I admit that it exposes a flaw
in the solution that I proposed.

> >> > > 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.
> >
> > So instead of trying to understand the concerns that I've expressed and
> > constructively contribute to finding a solution that works for everybody
> > you'd rather go and write a driver from scratch. Way to go.
> 
> It's the constructive thing to do when the existing driver cannot be
> extended to work for everybody.

No, it isn't. If a generic driver doesn't work for everybody then it
isn't generic and we should fix it. Not duplicate it and add platform
specific quirks.

> > I've already said that I'm not saying strictly no to these patches, but
> > what I want to see happen is some constructive discussion about whether
> > we can find better ways to do it. If we can't then I'm all for merging
> > these patches. Fortunately other (sub)threads have been somewhat more
> > constructive and actually come up with alternatives that should make
> > everyone happier.
> 
> What are those alternatives?

Sorry, you've got to do some of the work yourself. They've been
mentioned in this thread and the one that Maxime pointed to the other
day.

> > If you're going to do SoC-specific bindings and resource management you
> > are in fact implementing what Grant suggested in a subthread. You're
> > implementing a dummy driver only for resource management, which isn't
> > really a bad thing. It can serve as a placeholder for now until you add
> > the real driver. And you can also use the simplefb driver to provide
> > the framebuffer.
> 
> Oh, so back to the proposal to make a driver that claims the required
> resources and then instantiates an unextended simplefb that is
> oblivious to the resources to be kept simple?

Pretty much, yes.

> Did I not propose that way back?

Yes, I think you did.

> Was it not already rejected?

No, I don't think it was. In fact I don't think anything was really
rejected yet, we're still in the middle of a discussion.

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/4c41b394/attachment-0001.sig>