* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-09-29 11:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929101805.GB26008@ulmo>
[-- Attachment #1: Type: text/plain, Size: 7829 bytes --]
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.
> > > 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?
My bad, I thought it was a tegra SoC in there. I should have read more
carefully obviously.
> 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?
> > B) Other maintainers, precisely like Mark, came to the same conclusion
> > than Mike.
>
> Well, and others didn't.
We've been talking about both clocks and regulators up to now. I can
see Mike and Mark both suggesting to use the usual clocks and
regulators APIs, either in that thread or the one I pointed out.
> 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.
I've seen it, and replied to that already.
> > > 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.
> 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?
Oh come on. Since when a clk_prepare_enable call is duplication?
If so, then we really have a duplication issue, and it's not just for
the clock API.
> > 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.
> > 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...
> 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
I think our definition of "at some point" diverges. Yours seem to be
"at some point during the boot process", which might not even happen
in some platforms (or at least in the foreseeable kernel releases).
> 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.
> 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.
It is, but I was talking about a clk_disable after a clk_enable.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-09-29 13:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAGRGNgW94Jo_EuE1d3ta-yWW3b0sUs+ggyXnLOULV1_CfW=QZQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2873 bytes --]
On Mon, Sep 29, 2014 at 09:00:01PM +1000, Julian Calaby wrote:
> Hi Thierry,
If you address people directly please make sure they are in the To:
line. Or at least Cc.
> On Mon, Sep 29, 2014 at 8:18 PM, Thierry Reding
> <thierry.reding@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.
It's not completely solved. There's still the issue of no generic way to
specify regulators like you can do for clocks, resets or power domains.
But the kernel isn't the real issue here. The issue is the firmware that
now has to go out of its way not only to initialize display hardware but
also create device tree content just to make Linux not turn everything
off.
> 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.
Have you looked at the code needed to do this? It's not at all trivial.
And the point is really that all this information is there already, so
we're completely duplicating this into a dynamically created device tree
node and for what reason? Only to have one driver request all these
resources and have them forcefully released a few seconds later.
> Alternatively, it could simply add the relevant data to an existing
> device node and munge it's compatible property so simplefb picks it
> up.
Yes, I think that'd be a much better solution. Of course it's going to
be very difficult to make that work with a generic driver because now
that generic driver needs to parse the DT binding for any number of
"compatible" devices.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-09-29 13:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929114643.GB4081@lukather>
[-- Attachment #1: Type: text/plain, Size: 7485 bytes --]
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.
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.
> > > > 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
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.
Just because we currently lack this functionality doesn't make it a hack
trying to add it.
> > > 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.
> > 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
>
> I think our definition of "at some point" diverges. Yours seem to be
> "at some point during the boot process", which might not even happen
> in some platforms (or at least in the foreseeable kernel releases).
Then instead of hacking existing drivers to work on your particular
platform you should start looking into hacking your platform drivers to
cope with the lack of a proper display driver. Or alternatively spend
the time getting a proper display driver merged.
Whether simplefb is used as primary framebuffer or just during only boot
until hand-off to a real driver, it still remains a stop-gap solution.
> > 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.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-09-29 13:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929113436.GA4081@lukather>
[-- Attachment #1: Type: text/plain, Size: 2248 bytes --]
On Mon, Sep 29, 2014 at 01:34:36PM +0200, Maxime Ripard wrote:
> On Mon, Sep 29, 2014 at 12:44:57PM +0200, Thierry Reding wrote:
> > > >> 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.
> > >
> > > > 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.
> > >
> > > Calling clk_disable() preceding clk_enable() is a bug.
> > >
> > > Calling clk_disable() after clk_enable() will disable the clock (and
> > > its parents)
> > > if the clock subsystem thinks there are no other users, which is what will
> > > happen here.
> >
> > Right. I'm not sure this is really applicable to this situation, though.
>
> It's actually very easy to do. Have a driver that probes, enables its
> clock, fails to probe for any reason, call clk_disable in its exit
> path. If there's no other user at that time of this particular clock
> tree, it will be shut down. Bam. You just lost your framebuffer.
>
> Really, it's just that simple, and relying on the fact that some other
> user of the same clock tree will always be their is beyond fragile.
Perhaps the meaning clk_ignore_unused should be revised, then. What you
describe isn't at all what I'd expect from such an option. And it does
not match the description in Documentation/kernel-parameters.txt either.
> > Either way, if there are other users of a clock then they will just as
> > likely want to modify the rate at which point simplefb will break just
> > as badly.
>
> And this can be handled just as well. Register a clock notifier,
> refuse any rate change, done. But of course, that would require having
> a clock handle.
>
> Now, how would *you* prevent such a change?
Like I said in the other thread. If you have two drivers that use the
same clock but need different frequencies you've lost anyway.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-09-29 14:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMuHMdXrc4ED0N0Etb-mgCRc437spsxuNZ6Nq_15zhFXSt8Nug@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2645 bytes --]
On Mon, Sep 29, 2014 at 01:32:44PM +0200, Geert Uytterhoeven wrote:
> Hi Thierry,
>
> On Mon, Sep 29, 2014 at 12:44 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> >> >> 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.
> >>
> >> disable_all_power_management(), as this is not limited to clocks.
> >
> > Right. But it isn't all power management either. It just shouldn't turn
> > everything unused off. Clocks, regulators, power domains and so on which
> > are used can very well be power managed.
>
> No they can't, as the clock/regulator/PM domain core cannot know if any
> of the used ones are also used by a shim driver like simplefb.
> Clocks and regulators may be shared. PM domains can contain multiple
> hardware blocks. Without more knowledge, the only safe thing is not
> disabling anything.
Indeed. That's a shame. In the most common case that probably won't
matter all that much, given that the real driver can be expected to load
within a reasonable amount of time.
> >> >> 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.
> >>
> >> > 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.
> >>
> >> Calling clk_disable() preceding clk_enable() is a bug.
> >>
> >> Calling clk_disable() after clk_enable() will disable the clock (and
> >> its parents)
> >> if the clock subsystem thinks there are no other users, which is what will
> >> happen here.
> >
> > Right. I'm not sure this is really applicable to this situation, though.
>
> Yes it is: if all users of a clock/regulator/PM domain are gone, it will
> be disabled. Bad luck for simplefb still needing them.
Hmm... if all users are gone, then aren't the resources unused again and
should therefore be ignored?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Julian Calaby @ 2014-09-29 14:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929132123.GA30998@ulmo>
Hi Thierry,
On Mon, Sep 29, 2014 at 11:21 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Sep 29, 2014 at 09:00:01PM +1000, Julian Calaby wrote:
>> Hi Thierry,
>
> If you address people directly please make sure they are in the To:
> line. Or at least Cc.
Sorry about that, the mailing list I received this through (Google
Groups based) generally strips to: and CC: lines, so my mail client
(Gmail) doesn't do it automatically. I'm still getting used to it.
>> On Mon, Sep 29, 2014 at 8:18 PM, Thierry Reding
>> <thierry.reding@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.
>
> It's not completely solved. There's still the issue of no generic way to
> specify regulators like you can do for clocks, resets or power domains.
> But the kernel isn't the real issue here. The issue is the firmware that
> now has to go out of its way not only to initialize display hardware but
> also create device tree content just to make Linux not turn everything
> off.
My point is that the firmware is going to be doing complicated stuff
already, adding and using some helpers to configure a device tree node
is relatively simple in comparison to dealing with the actual
hardware. It wouldn't surprise me if u-boot, for example, ended up
with a set of functions to handle this exact case as more graphics
hardware gets brought up.
>> 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.
>
> Have you looked at the code needed to do this? It's not at all trivial.
> And the point is really that all this information is there already, so
> we're completely duplicating this into a dynamically created device tree
> node and for what reason? Only to have one driver request all these
> resources and have them forcefully released a few seconds later.
>
>> Alternatively, it could simply add the relevant data to an existing
>> device node and munge it's compatible property so simplefb picks it
>> up.
>
> Yes, I think that'd be a much better solution. Of course it's going to
> be very difficult to make that work with a generic driver because now
> that generic driver needs to parse the DT binding for any number of
> "compatible" devices.
Not necessarily.
The patch that started this discussion can work with any number of
clocks specified in a "clocks" property. Therefore all that needs to
happen is that the final hardware binding specifies it's clocks that
way. This is how, for example, the ahci_platform driver's clock code
works.
I'm sure that as hardware diversifies, the other subsystems will grow
in similar directions and eventually be dealt with using similarly
generic code.
Thanks,
--
Julian Calaby
Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Michal Suchanek @ 2014-09-29 15:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929134708.GB30998@ulmo>
On 29 September 2014 15:47, Thierry Reding <thierry.reding@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
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-09-29 15:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAGRGNgVaT73L-AY1rMdth8RiNpanFB=9fzGaUV_tmS0d4udf5g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5840 bytes --]
On Tue, Sep 30, 2014 at 12:46:11AM +1000, Julian Calaby wrote:
> Hi Thierry,
>
> On Mon, Sep 29, 2014 at 11:21 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Mon, Sep 29, 2014 at 09:00:01PM +1000, Julian Calaby wrote:
> >> Hi Thierry,
> >
> > If you address people directly please make sure they are in the To:
> > line. Or at least Cc.
>
> Sorry about that, the mailing list I received this through (Google
> Groups based) generally strips to: and CC: lines, so my mail client
> (Gmail) doesn't do it automatically. I'm still getting used to it.
Yeah, I wish mailing lists would stop doing that.
> >> On Mon, Sep 29, 2014 at 8:18 PM, Thierry Reding
> >> <thierry.reding@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.
> >
> > It's not completely solved. There's still the issue of no generic way to
> > specify regulators like you can do for clocks, resets or power domains.
> > But the kernel isn't the real issue here. The issue is the firmware that
> > now has to go out of its way not only to initialize display hardware but
> > also create device tree content just to make Linux not turn everything
> > off.
>
> My point is that the firmware is going to be doing complicated stuff
> already, adding and using some helpers to configure a device tree node
> is relatively simple in comparison to dealing with the actual
> hardware. It wouldn't surprise me if u-boot, for example, ended up
> with a set of functions to handle this exact case as more graphics
> hardware gets brought up.
Not all firmware is based on U-Boot. Essentially whatever binding
changes we make will need to be implemented in all firmware. And the
complexity isn't so much about writing the actual DT data, but more
about figuring out which data to write. Every firmware image needs to
know exactly which clocks and other resources to transcribe for a given
board. It'll essentially need to contain some sort of "driver" for each
device that parses a DTB, correlates the data to what it knows of the
device internals and write a subset of that data back into the DTB in a
slightly different format. That's just whacky.
DT was meant to simplify things.
> >> 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.
> >
> > Have you looked at the code needed to do this? It's not at all trivial.
> > And the point is really that all this information is there already, so
> > we're completely duplicating this into a dynamically created device tree
> > node and for what reason? Only to have one driver request all these
> > resources and have them forcefully released a few seconds later.
> >
> >> Alternatively, it could simply add the relevant data to an existing
> >> device node and munge it's compatible property so simplefb picks it
> >> up.
> >
> > Yes, I think that'd be a much better solution. Of course it's going to
> > be very difficult to make that work with a generic driver because now
> > that generic driver needs to parse the DT binding for any number of
> > "compatible" devices.
>
> Not necessarily.
>
> The patch that started this discussion can work with any number of
> clocks specified in a "clocks" property. Therefore all that needs to
> happen is that the final hardware binding specifies it's clocks that
> way.
Are you suggesting that we should be modeling the hardware specific
binding to match what the simplefb binding does?
> I'm sure that as hardware diversifies, the other subsystems will grow
> in similar directions and eventually be dealt with using similarly
> generic code.
For regulators this already works very differently. As opposed to the
clocks/clock-names type of binding it uses one where the consumer name
of the regulator comes from the prefix of a -supply property. That is
you'd get something like this:
foo-supply = <®1>;
bar-supply = <®2>;
And since you don't have enough information in the kernel simplefb
driver to attach any meaning to these, the best you can do would be
iterating over a range and have:
0-supply = <®0>;
1-supply = <®1>;
...
n-supply = <®2>;
This is made more difficult by the fact that these regulators may be
required by components not immediately related to the display engine.
They could be for an attached panel, a video bridge or the +5V pin on
the HDMI connector.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Michal Suchanek @ 2014-09-29 15:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929151949.GC2273@ulmo>
On 29 September 2014 17:19, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Sep 30, 2014 at 12:46:11AM +1000, Julian Calaby wrote:
>> >> On Mon, Sep 29, 2014 at 8:18 PM, Thierry Reding
>> >> <thierry.reding@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.
>> >
>> > It's not completely solved. There's still the issue of no generic way to
>> > specify regulators like you can do for clocks, resets or power domains.
>> > But the kernel isn't the real issue here. The issue is the firmware that
>> > now has to go out of its way not only to initialize display hardware but
>> > also create device tree content just to make Linux not turn everything
>> > off.
>>
>> My point is that the firmware is going to be doing complicated stuff
>> already, adding and using some helpers to configure a device tree node
>> is relatively simple in comparison to dealing with the actual
>> hardware. It wouldn't surprise me if u-boot, for example, ended up
>> with a set of functions to handle this exact case as more graphics
>> hardware gets brought up.
>
> Not all firmware is based on U-Boot. Essentially whatever binding
> changes we make will need to be implemented in all firmware. And the
> complexity isn't so much about writing the actual DT data, but more
> about figuring out which data to write. Every firmware image needs to
> know exactly which clocks and other resources to transcribe for a given
> board. It'll essentially need to contain some sort of "driver" for each
> device that parses a DTB, correlates the data to what it knows of the
> device internals and write a subset of that data back into the DTB in a
> slightly different format. That's just whacky.
>
> DT was meant to simplify things.
The firmware only needs to implement DT parsing and writing if it
wants to communicate the configuration it set up to the kernel. If you
do not have control over the firmware or do not want to write support
for the firmware to generate the DT you can produce a fixed
configuration DT and have the firmware load that with the kernel. It
is fully backwards compatible with dumb firmware that does not support
DT modification during boot. You will just need a different DT for
slightly different devices (eg. tablets with different display) or
different configurations of same device (eg. different displays
connected to the HDMI port of your devboard).
>> I'm sure that as hardware diversifies, the other subsystems will grow
>> in similar directions and eventually be dealt with using similarly
>> generic code.
>
> For regulators this already works very differently. As opposed to the
> clocks/clock-names type of binding it uses one where the consumer name
> of the regulator comes from the prefix of a -supply property. That is
> you'd get something like this:
>
> foo-supply = <®1>;
> bar-supply = <®2>;
>
> And since you don't have enough information in the kernel simplefb
> driver to attach any meaning to these, the best you can do would be
> iterating over a range and have:
>
> 0-supply = <®0>;
> 1-supply = <®1>;
> ...
> n-supply = <®2>;
>
> This is made more difficult by the fact that these regulators may be
> required by components not immediately related to the display engine.
> They could be for an attached panel, a video bridge or the +5V pin on
> the HDMI connector.
So you are saying that listing the properties of the simplefb node and
filtering foo-supply and bar-supply out of that is too difficult?
I hope kernel development did not get this dumb.
Thanks
Michal
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-09-29 15:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOMqctRhpxd9M_FME1_Duve-QnVpqNELNGq9NNrTYbzdYCe8zg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 9804 bytes --]
On Mon, Sep 29, 2014 at 05:04:32PM +0200, Michal Suchanek wrote:
> On 29 September 2014 15:47, Thierry Reding <thierry.reding@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!
I agree, this is not a surprise at all. But like I pointed out this is
bound to happen again and again. So how about we learn from it and add
the functionality needed to prevent it from happening?
> > 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.
Feel free to take that up with the DT ABI stability committee.
> > 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.
Today it's 5, tomorrow 6, next year, who knows.
> >> > > > 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.
I presume some of those tablets will have HDMI connectors. And surely
somebody will eventually put an Allwinner SoC into something that's not
a tablet.
Note also that with only simplefb you won't be able to use any kind of
hardware acceleration, not even 3D. Perhaps you could even pull it off
but certainly not with any kind of double-buffering, so while it might
be an easy way to get something to display, please stop pretending that
DRM/KMS don't matter.
Also this is all hypothetical since Luc said himself that he's already
working on a DRM/KMS driver.
> And there is still the use of simplefb during development and for
> generic distribution kernels.
Generic distribution kernels can use DRM/KMS just fine. And they also
want to use it because it gives them a lot of advanced features using a
standard API.
> 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.
It's not at all special or hacked. It merely uses a couple of
assumptions that differ from what you might expect from a regular
driver. And that's really the essence of simplefb.
> > 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?
I have no doubt that it's going to work. It already does. But unless we
solve this at the core we'll be reiterating this for every new type of
resource that we need to support and for every other driver of the same
nature (i.e. uses a "virtual" device set up by firmware).
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Mark Brown @ 2014-09-29 15:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929134708.GB30998@ulmo>
[-- Attachment #1: Type: text/plain, Size: 2084 bytes --]
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.
Not quite - there was also DT added for them. With just the driver the
regulator API shouldn't have touched them, it'd just have exposed them
read only.
> 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.
So long as we're ensuring that when we don't start supporting resources
without DT additions or at least require DT additions to actively manage
them (which can then include simplefb hookup) we should be fine.
> > > 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
> get you working display. And that's all it is. If you want to do it
> properly go and write a DRM/KMS driver.
Well, for product probably not. But in terms of the runtime of a kernel
I'd not be so sure - people do dogfood and we should be allowing that.
I've used unaccelerated displays with reasonable success for a large
part of my work in the past.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-09-29 15:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929135358.GC30998@ulmo>
[-- Attachment #1: Type: text/plain, Size: 2990 bytes --]
On Mon, Sep 29, 2014 at 03:54:00PM +0200, Thierry Reding wrote:
> On Mon, Sep 29, 2014 at 01:34:36PM +0200, Maxime Ripard wrote:
> > On Mon, Sep 29, 2014 at 12:44:57PM +0200, Thierry Reding wrote:
> > > > >> 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.
> > > >
> > > > > 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.
> > > >
> > > > Calling clk_disable() preceding clk_enable() is a bug.
> > > >
> > > > Calling clk_disable() after clk_enable() will disable the clock (and
> > > > its parents)
> > > > if the clock subsystem thinks there are no other users, which is what will
> > > > happen here.
> > >
> > > Right. I'm not sure this is really applicable to this situation, though.
> >
> > It's actually very easy to do. Have a driver that probes, enables its
> > clock, fails to probe for any reason, call clk_disable in its exit
> > path. If there's no other user at that time of this particular clock
> > tree, it will be shut down. Bam. You just lost your framebuffer.
> >
> > Really, it's just that simple, and relying on the fact that some other
> > user of the same clock tree will always be their is beyond fragile.
>
> Perhaps the meaning clk_ignore_unused should be revised, then. What you
> describe isn't at all what I'd expect from such an option. And it does
> not match the description in Documentation/kernel-parameters.txt either.
Well, it never says that it also prevent them from being disabled
later on. But it's true it's not very explicit.
> > > Either way, if there are other users of a clock then they will just as
> > > likely want to modify the rate at which point simplefb will break just
> > > as badly.
> >
> > And this can be handled just as well. Register a clock notifier,
> > refuse any rate change, done. But of course, that would require having
> > a clock handle.
> >
> > Now, how would *you* prevent such a change?
>
> Like I said in the other thread. If you have two drivers that use the
> same clock but need different frequencies you've lost anyway.
Except that the driver that has the most logic (ie not simplefb) will
have a way to recover and deal with that. You prevented the clock rate
*and* your system can react, I guess you actually won.
But sure, you can still try to point new issues, get an obvious and
robust solution, and then discard the issue when the solution doesn't
go your way...
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH] videomode: provide dummy of_get_display_timing helper
From: Arnd Bergmann @ 2014-09-29 16:00 UTC (permalink / raw)
To: linux-fbdev
If CONFIG_OF is disabled but we build any driver that tries to
use of_get_display_timing, we get this link error:
ERROR: "of_get_display_timing" [drivers/gpu/drm/rcar-du/rcar-du-drm.ko] undefined!
This adds an empty inline function as an alternative, to avoid changing
each driver using this function when we want to build them without
CONFIG_OF.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
diff --git a/include/video/of_display_timing.h b/include/video/of_display_timing.h
index 79e6697af6cf..02b6eb7c3ded 100644
--- a/include/video/of_display_timing.h
+++ b/include/video/of_display_timing.h
@@ -15,8 +15,17 @@ struct display_timings;
#define OF_USE_NATIVE_MODE -1
+#ifdef CONFIG_OF
int of_get_display_timing(struct device_node *np, const char *name,
struct display_timing *dt);
+#else
+static inline int of_get_display_timing(struct device_node *np, const char *name,
+ struct display_timing *dt)
+{
+ return -ENOSYS;
+}
+
+#endif
struct display_timings *of_get_display_timings(struct device_node *np);
int of_display_timings_exist(struct device_node *np);
^ permalink raw reply related
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Mark Brown @ 2014-09-29 16:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929080637.GB12506@ulmo>
[-- Attachment #1: Type: text/plain, Size: 3416 bytes --]
On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
> On Sat, Sep 27, 2014 at 04:56:01PM -0700, Mike Turquette wrote:
> > With that said I think that Luc's approach is very sensible. I'm not
> > sure what purpose in the universe DT is supposed to serve if not for
> > _this_exact_case_. We have a nice abstracted driver, usable by many
> > people. The hardware details of how it is hooked up at the board level
> > can all be hidden behind stable DT bindings that everyone already uses.
> 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.
> 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.
Not really thought this through fully yet but would having phandles to
the relevant devices do the job? Kind of lines up with Grant's idea of
having dummy drivers.
> 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.
We might want to do that in the future, though it's not always the case
that reset is the lowest power state.
> 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.
One thing that makes me a bit nervous about this approach in the context
of the regulator API is the frequency with which one finds shared
supplies. I'm not sure if it's actually a big problem in practice but
it makes me worry a bit. We could probably just do something like make
refcounting down to zero not actually disable anything for standard
regulators to deal with it which might be an idea anyway in the context
of this sort of dodge.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-09-29 16:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929134708.GB30998@ulmo>
[-- Attachment #1: Type: text/plain, Size: 9293 bytes --]
On Mon, Sep 29, 2014 at 03:47:15PM +0200, Thierry Reding 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.
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?
> 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?
And if you don't do it, you end up in situations where you have to
tell your users "disable the regulator driver in order for your
display to work".
> > > > > 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
> get you working display. And that's all it is. If you want to do it
> properly go and write a DRM/KMS driver.
This is a WiP. And I really have the feeling it will be written,
reviewed and merged before we can have the "dirty hack" working.
> > > > 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?
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).
> > > > 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.
> > > 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
> >
> > I think our definition of "at some point" diverges. Yours seem to be
> > "at some point during the boot process", which might not even happen
> > in some platforms (or at least in the foreseeable kernel releases).
>
> Then instead of hacking existing drivers to work on your particular
> platform you should start looking into hacking your platform drivers to
> cope with the lack of a proper display driver. Or alternatively spend
> the time getting a proper display driver merged.
>
> Whether simplefb is used as primary framebuffer or just during only boot
> until hand-off to a real driver, it still remains a stop-gap solution.
Then really, simplefb deserves a more appropriate name. Like
uselessfb, tegrafb, DONOTUSEITYOUSTUPIDDEVfb or whatever makes it not
look generic.
> > > 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.
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Michal Suchanek @ 2014-09-29 16:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929154901.GA24952@ulmo>
On 29 September 2014 17:49, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Sep 29, 2014 at 05:04:32PM +0200, Michal Suchanek wrote:
>> On 29 September 2014 15:47, Thierry Reding <thierry.reding@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!
>
> I agree, this is not a surprise at all. But like I pointed out this is
> bound to happen again and again. So how about we learn from it and add
> the functionality needed to prevent it from happening?
>
>> > 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.
>
> Feel free to take that up with the DT ABI stability committee.
As pointed out you can also generate by hand a DT that exactly
describes what the preinstalled firmware sets up - so long as the
preinstalled firmware sets upa any usable framebuffer at all and
always sets up the same thing. If the preinstalled firmware can set up
the framebuffer in different ways and is not willing to communicate to
the kernel in what way it set up the framebuffer then it is unusable
with simplefb. That sucks but there is no way around that.
>> > 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.
>
> I presume some of those tablets will have HDMI connectors. And surely
Nope. Only 1 output.
> somebody will eventually put an Allwinner SoC into something that's not
> a tablet.
Sure. And you might want to bother with modesetting then.
>
> Note also that with only simplefb you won't be able to use any kind of
> hardware acceleration, not even 3D. Perhaps you could even pull it off
Nope. I won't be able to use that staggering performance of 1 hardware shader.
But my UI will draw really fast unless I want those wobbly windows.
Now who cares about wobbliness when you want to run everything
fullscreen anyway to get the most of the 480p screen estate.
> but certainly not with any kind of double-buffering, so while it might
> be an easy way to get something to display, please stop pretending that
> DRM/KMS don't matter.
Yes, it will matter for 3d and might matter for video acceleration as well.
But will not matter for using a terminal emulator or reading a book.
>> >> > > 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?
>
> I have no doubt that it's going to work. It already does. But unless we
> solve this at the core we'll be reiterating this for every new type of
> resource that we need to support and for every other driver of the same
> nature (i.e. uses a "virtual" device set up by firmware).
Once it's solved for every resource every 'virtual' driver will work
the same way.
I do not see a problem here.
Thanks
Michal
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Luc Verhaegen @ 2014-09-29 16:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929162814.GE4081@lukather>
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:
> >
> > 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?
>
> 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).
In the 2nd round of this discussion, i stated that another fb or even a
drm driver altogether seemed to be the sensible way out of this mess.
I suggest drm_rescue.
> > Then instead of hacking existing drivers to work on your particular
> > platform you should start looking into hacking your platform drivers to
> > cope with the lack of a proper display driver. Or alternatively spend
> > the time getting a proper display driver merged.
> >
> > Whether simplefb is used as primary framebuffer or just during only boot
> > until hand-off to a real driver, it still remains a stop-gap solution.
>
> Then really, simplefb deserves a more appropriate name. Like
> uselessfb, tegrafb, DONOTUSEITYOUSTUPIDDEVfb or whatever makes it not
> look generic.
Very early on, now almost two months back, i used the word "denialfb".
rpifb is the real name of this thing though, but then the dt binding
names would have to change and whatnot.
I don't get the resistance, at least not from a technical point of view.
And i do not care enough to get properly involved in this pointless and
endless discussion. drm_rescue it is.
Luc Verhaegen.
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: jonsmirl @ 2014-09-29 17:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929080637.GB12506@ulmo>
On Mon, Sep 29, 2014 at 4:06 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
>
> On Sat, Sep 27, 2014 at 04:56:01PM -0700, Mike Turquette wrote:
> > Quoting Maxime Ripard (2014-09-02 02:25:08)
> > > On Fri, Aug 29, 2014 at 04:38:14PM +0200, Thierry Reding wrote:
> > > > On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote:
> > > > > On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote:
> > > > > > I would think the memory should still be reserved anyway to make sure
> > > > > > nothing else is writing over it. And it's in the device tree anyway
> > > > > > because the driver needs to know where to put framebuffer content. So
> > > > > > the point I was trying to make is that we can't treat the memory in the
> > > > > > same way as clocks because it needs to be explicitly managed. Whereas
> > > > > > clocks don't. The driver is simply too generic to know what to do with
> > > > > > the clocks.
> > > > >
> > > > > You agreed on the fact that the only thing we need to do with the
> > > > > clocks is claim them. Really, I don't find what's complicated there
> > > > > (or not generic).
> > > >
> > > > That's not what I agreed on. What I said is that the only thing we need
> > > > to do with the clocks is nothing. They are already in the state that
> > > > they need to be.
> > >
> > > Claim was probably a poor choice of words, but still. We have to keep
> > > the clock running, and both the solution you've been giving and this
> > > patch do so in a generic way.
> > >
> > > > > > It doesn't know what frequency they should be running at
> > > > >
> > > > > We don't care about that. Just like we don't care about which
> > > > > frequency is the memory bus running at. It will just run at whatever
> > > > > frequency is appropriate.
> > > >
> > > > Exactly. And you shouldn't have to care about them at all. Firmware has
> > > > already configured the clocks to run at the correct frequencies, and it
> > > > has made sure that they are enabled.
> > > >
> > > > > > or what they're used for
> > > > >
> > > > > And we don't care about that either. You're not interested in what
> > > > > output the framebuffer is setup to use, which is pretty much the same
> > > > > here, this is the same thing here.
> > > >
> > > > That's precisely what I've been saying. The only thing that simplefb
> > > > cares about is the memory it should be using and the format of the
> > > > pixels (and how many of them) it writes into that memory. Everything
> > > > else is assumed to have been set up.
> > > >
> > > > Including clocks.
> > >
> > > We're really discussing in circles here.
> > >
> > > Mike?
> > >
> >
> > -EHIGHLATENCYRESPONSE
> >
> > I forgot about this thread. Sorry.
> >
> > In an attempt to provide the least helpful answer possible, I will stay
> > clear of all of the stuff relating to "how simple should simplefb be"
> > and the related reserved memory discussion.
> >
> > A few times in this thread it is stated that we can't prevent unused
> > clocks from being disabled. That is only partially true.
> >
> > The clock framework DOES provide a flag to prevent UNUSED clocks from
> > being disabled at late_initcall-time by a clock "garbage collector"
> > mechanism. Maxime and others familiar with the clock framework are aware
> > of this.
> >
> > What the framework doesn't do is to allow for a magic flag in DT, in
> > platform_data or elsewhere that says, "don't let me get turned off until
> > the right driver claims me". That would be an external or alternative
> > method for preventing a clock from being disabled. We have a method for
> > preventing clocks from being disabled. It is as follows:
> >
> > struct clk *my_clk = clk_get(...);
> > clk_prepare_enable(my_clk);
> >
> > That is how it should be done. Period.
> >
> > With that said I think that Luc's approach is very sensible. I'm not
> > sure what purpose in the universe DT is supposed to serve if not for
> > _this_exact_case_. We have a nice abstracted driver, usable by many
> > people. The hardware details of how it is hooked up at the board level
> > can all be hidden behind stable DT bindings that everyone already uses.
>
> 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.
>
> 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
This is the key point -- if a design requires duplicating information
in the device tree, the design is broken. The list of clocks is being
duplicated, that is broken design.
Device trees are supposed to reflect the hardware. They are not
supposed to be manipulated into supporting a specific feature on a
single OS.
>
> 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).
>
> 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.
>
> Thierry
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Michal Suchanek @ 2014-09-29 18:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKON4Oy13uu1FTjCwH5EPa0UF9kRs7DqQf2Jcc2E_XKLfXX4_Q@mail.gmail.com>
On 29 September 2014 19:51, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
> On Mon, Sep 29, 2014 at 4:06 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>>
>> On Sat, Sep 27, 2014 at 04:56:01PM -0700, Mike Turquette wrote:
>> > Quoting Maxime Ripard (2014-09-02 02:25:08)
>> > > On Fri, Aug 29, 2014 at 04:38:14PM +0200, Thierry Reding wrote:
>> > > > On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote:
>> > > > > On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote:
>> > > > > > I would think the memory should still be reserved anyway to make sure
>> > > > > > nothing else is writing over it. And it's in the device tree anyway
>> > > > > > because the driver needs to know where to put framebuffer content. So
>> > > > > > the point I was trying to make is that we can't treat the memory in the
>> > > > > > same way as clocks because it needs to be explicitly managed. Whereas
>> > > > > > clocks don't. The driver is simply too generic to know what to do with
>> > > > > > the clocks.
>> > > > >
>> > > > > You agreed on the fact that the only thing we need to do with the
>> > > > > clocks is claim them. Really, I don't find what's complicated there
>> > > > > (or not generic).
>> > > >
>> > > > That's not what I agreed on. What I said is that the only thing we need
>> > > > to do with the clocks is nothing. They are already in the state that
>> > > > they need to be.
>> > >
>> > > Claim was probably a poor choice of words, but still. We have to keep
>> > > the clock running, and both the solution you've been giving and this
>> > > patch do so in a generic way.
>> > >
>> > > > > > It doesn't know what frequency they should be running at
>> > > > >
>> > > > > We don't care about that. Just like we don't care about which
>> > > > > frequency is the memory bus running at. It will just run at whatever
>> > > > > frequency is appropriate.
>> > > >
>> > > > Exactly. And you shouldn't have to care about them at all. Firmware has
>> > > > already configured the clocks to run at the correct frequencies, and it
>> > > > has made sure that they are enabled.
>> > > >
>> > > > > > or what they're used for
>> > > > >
>> > > > > And we don't care about that either. You're not interested in what
>> > > > > output the framebuffer is setup to use, which is pretty much the same
>> > > > > here, this is the same thing here.
>> > > >
>> > > > That's precisely what I've been saying. The only thing that simplefb
>> > > > cares about is the memory it should be using and the format of the
>> > > > pixels (and how many of them) it writes into that memory. Everything
>> > > > else is assumed to have been set up.
>> > > >
>> > > > Including clocks.
>> > >
>> > > We're really discussing in circles here.
>> > >
>> > > Mike?
>> > >
>> >
>> > -EHIGHLATENCYRESPONSE
>> >
>> > I forgot about this thread. Sorry.
>> >
>> > In an attempt to provide the least helpful answer possible, I will stay
>> > clear of all of the stuff relating to "how simple should simplefb be"
>> > and the related reserved memory discussion.
>> >
>> > A few times in this thread it is stated that we can't prevent unused
>> > clocks from being disabled. That is only partially true.
>> >
>> > The clock framework DOES provide a flag to prevent UNUSED clocks from
>> > being disabled at late_initcall-time by a clock "garbage collector"
>> > mechanism. Maxime and others familiar with the clock framework are aware
>> > of this.
>> >
>> > What the framework doesn't do is to allow for a magic flag in DT, in
>> > platform_data or elsewhere that says, "don't let me get turned off until
>> > the right driver claims me". That would be an external or alternative
>> > method for preventing a clock from being disabled. We have a method for
>> > preventing clocks from being disabled. It is as follows:
>> >
>> > struct clk *my_clk = clk_get(...);
>> > clk_prepare_enable(my_clk);
>> >
>> > That is how it should be done. Period.
>> >
>> > With that said I think that Luc's approach is very sensible. I'm not
>> > sure what purpose in the universe DT is supposed to serve if not for
>> > _this_exact_case_. We have a nice abstracted driver, usable by many
>> > people. The hardware details of how it is hooked up at the board level
>> > can all be hidden behind stable DT bindings that everyone already uses.
>>
>> 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.
>>
>> 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
>
>
> This is the key point -- if a design requires duplicating information
> in the device tree, the design is broken. The list of clocks is being
> duplicated, that is broken design.
>
> Device trees are supposed to reflect the hardware. They are not
> supposed to be manipulated into supporting a specific feature on a
> single OS.
Where is the duplication?
If the firmware sets up some clocks in order to scan out a memory
buffer to a display the firmware knows which clocks it used and can
record this in the DT. Presumably the OS then does not need to know
about display hardware and can just read the list. If the OS happens
to have (or later load) a KMS driver that driver has a list of clocks
it uses for driving the display. This will be a superset of what the
firmware used to set up the scanout buffer - for example, the firmware
will probably not turn on any graphics accelerator, and it will
probably configure only one of multiple possible output
configurations.
You can, of course, record some or all of this information statically
and just have the firmware set up what is in the DT without even
reading it. This is however not very flexible - if the firmware can
select to enable different display outputs or set different resolution
it should record that in the DT so that the kernel knows what it set
up. Also if the DT does not match your firmware settings you have a
problem. That's why it is more desirable for the firmware to be able
to record the hardware state it hands over in the DT at boot time.
Thanks
Michal
^ permalink raw reply
* Re: [PATCH] videomode: provide dummy of_get_display_timing helper
From: Laurent Pinchart @ 2014-09-29 20:43 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1957575.to7BPgiTer@wuerfel>
Hi Arnd,
Thank you for the patch.
On Monday 29 September 2014 18:00:33 Arnd Bergmann wrote:
> If CONFIG_OF is disabled but we build any driver that tries to
> use of_get_display_timing, we get this link error:
>
> ERROR: "of_get_display_timing" [drivers/gpu/drm/rcar-du/rcar-du-drm.ko]
> undefined!
>
> This adds an empty inline function as an alternative, to avoid changing
> each driver using this function when we want to build them without
> CONFIG_OF.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Shouldn't empty functions also be defined for of_get_display_timings and
of_display_timings_exist ?
> diff --git a/include/video/of_display_timing.h
> b/include/video/of_display_timing.h index 79e6697af6cf..02b6eb7c3ded 100644
> --- a/include/video/of_display_timing.h
> +++ b/include/video/of_display_timing.h
> @@ -15,8 +15,17 @@ struct display_timings;
>
> #define OF_USE_NATIVE_MODE -1
>
> +#ifdef CONFIG_OF
> int of_get_display_timing(struct device_node *np, const char *name,
> struct display_timing *dt);
> +#else
> +static inline int of_get_display_timing(struct device_node *np, const char
> *name,
> + struct display_timing *dt)
> +{
> + return -ENOSYS;
> +}
> +
> +#endif
> struct display_timings *of_get_display_timings(struct device_node *np);
> int of_display_timings_exist(struct device_node *np);
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Luc Verhaegen @ 2014-09-29 22:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929165842.GC5599@skynet.be>
On Mon, Sep 29, 2014 at 06:58:42PM +0200, Luc Verhaegen wrote:
>
> In the 2nd round of this discussion, i stated that another fb or even a
> drm driver altogether seemed to be the sensible way out of this mess.
>
> I suggest drm_rescue.
>
> Very early on, now almost two months back, i used the word "denialfb".
> rpifb is the real name of this thing though, but then the dt binding
> names would have to change and whatnot.
>
> I don't get the resistance, at least not from a technical point of view.
> And i do not care enough to get properly involved in this pointless and
> endless discussion. drm_rescue it is.
>
> Luc Verhaegen.
So Thierry, let's review what we have achieved here.
1) you keep simplefb absolutely true to the name. Congratulations.
2) Simplefb will only have a single user: the rpi. As the only other
users i can think of, which does not have a full driver and which does
not have clocks automatically disabled, are discrete cards. And they do
not really tend to happen with dt or platform devices.
3) a competing driver will be created, which will do these dt-ishy
things.
4) it's just a matter of time before the rpi either gets a full driver,
or switches over to the driver that everyone else is actually using. And
then the misnomer gets deprecated.
Was that the outcome you were looking for? I think not.
Luc Verhaegen.
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Julian Calaby @ 2014-09-30 0:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929151949.GC2273@ulmo>
Hi Thierry,
On Tue, Sep 30, 2014 at 1:19 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Tue, Sep 30, 2014 at 12:46:11AM +1000, Julian Calaby wrote:
>> Hi Thierry,
>>
>> On Mon, Sep 29, 2014 at 11:21 PM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>> > On Mon, Sep 29, 2014 at 09:00:01PM +1000, Julian Calaby wrote:
>> >> Hi Thierry,
>> >
>> > If you address people directly please make sure they are in the To:
>> > line. Or at least Cc.
>>
>> Sorry about that, the mailing list I received this through (Google
>> Groups based) generally strips to: and CC: lines, so my mail client
>> (Gmail) doesn't do it automatically. I'm still getting used to it.
>
> Yeah, I wish mailing lists would stop doing that.
>
>> >> On Mon, Sep 29, 2014 at 8:18 PM, Thierry Reding
>> >> <thierry.reding@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.
>> >
>> > It's not completely solved. There's still the issue of no generic way to
>> > specify regulators like you can do for clocks, resets or power domains.
>> > But the kernel isn't the real issue here. The issue is the firmware that
>> > now has to go out of its way not only to initialize display hardware but
>> > also create device tree content just to make Linux not turn everything
>> > off.
>>
>> My point is that the firmware is going to be doing complicated stuff
>> already, adding and using some helpers to configure a device tree node
>> is relatively simple in comparison to dealing with the actual
>> hardware. It wouldn't surprise me if u-boot, for example, ended up
>> with a set of functions to handle this exact case as more graphics
>> hardware gets brought up.
>
> Not all firmware is based on U-Boot. Essentially whatever binding
> changes we make will need to be implemented in all firmware. And the
> complexity isn't so much about writing the actual DT data, but more
> about figuring out which data to write. Every firmware image needs to
> know exactly which clocks and other resources to transcribe for a given
> board. It'll essentially need to contain some sort of "driver" for each
> device that parses a DTB, correlates the data to what it knows of the
> device internals and write a subset of that data back into the DTB in a
> slightly different format. That's just whacky.
>
> DT was meant to simplify things.
Ok, fair enough, while it's a tempting solution - it makes the kernel
end of things nice and simple, it's probably not completely scalable.
U-boot definitely does have "drivers" for the hardware it supports,
but we can't expect every bootloader to do that.
>> >> 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.
>> >
>> > Have you looked at the code needed to do this? It's not at all trivial.
>> > And the point is really that all this information is there already, so
>> > we're completely duplicating this into a dynamically created device tree
>> > node and for what reason? Only to have one driver request all these
>> > resources and have them forcefully released a few seconds later.
>> >
>> >> Alternatively, it could simply add the relevant data to an existing
>> >> device node and munge it's compatible property so simplefb picks it
>> >> up.
>> >
>> > Yes, I think that'd be a much better solution. Of course it's going to
>> > be very difficult to make that work with a generic driver because now
>> > that generic driver needs to parse the DT binding for any number of
>> > "compatible" devices.
>>
>> Not necessarily.
>>
>> The patch that started this discussion can work with any number of
>> clocks specified in a "clocks" property. Therefore all that needs to
>> happen is that the final hardware binding specifies it's clocks that
>> way.
>
> Are you suggesting that we should be modeling the hardware specific
> binding to match what the simplefb binding does?
I'll admit that this sounded pretty dumb when I thought over it in the
morning. Let's call it a strawman and let's call me a hilariously
unskilled debater.
I don't have a kernel tree in front of me, so I don't know that all
the helpers used below exist or are named like I've shown, but code
like this could grab all the resources specified in a tree of nodes:
void simplefb_grab_all_resources(node *root)
{
node *next = root;
node *cur;
while( next ) {
cur = next;
get_all_resources(cur);
if( next = of_get_first_child(cur) )
continue;
if( next = of_get_next_sibling(cur) )
continue;
while( !next && cur != root ) {
cur = of_get_parent_node(cur);
next = of_get_next_sibling(cur);
}
if( cur = root )
break;
}
}
If we added in a few tests in get_all_resources() to keep it from
doing things which are obviously stupid (e.g. grabbing resources for
nodes which have drivers we already have registered) that should work
for any binding and could easily be extended to any resource type.
We're already using linked lists to hold references to those
resources, so there's no problems I can see here, other than adding
some code.
>> I'm sure that as hardware diversifies, the other subsystems will grow
>> in similar directions and eventually be dealt with using similarly
>> generic code.
>
> For regulators this already works very differently. As opposed to the
> clocks/clock-names type of binding it uses one where the consumer name
> of the regulator comes from the prefix of a -supply property. That is
> you'd get something like this:
>
> foo-supply = <®1>;
> bar-supply = <®2>;
>
> And since you don't have enough information in the kernel simplefb
> driver to attach any meaning to these, the best you can do would be
> iterating over a range and have:
>
> 0-supply = <®0>;
> 1-supply = <®1>;
> ...
> n-supply = <®2>;
>
> This is made more difficult by the fact that these regulators may be
> required by components not immediately related to the display engine.
> They could be for an attached panel, a video bridge or the +5V pin on
> the HDMI connector.
Surely an iterator could be constructed to go through each node and
process every one named something like "*-supply"?
As for there being regulators for each separate part of the device,
could we read what their current state is then maintain that state?
I'll admit that this is getting rather hairy at this point.
Finally, the clock subsystem has evolved away from "*-clock" bindings
towards the current "clocks" and "clock-names" bindings. I'm sure
other subsystems will follow.
Thanks,
--
Julian Calaby
Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-09-30 4:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929155718.GD4081@lukather>
[-- Attachment #1: Type: text/plain, Size: 3517 bytes --]
On Mon, Sep 29, 2014 at 05:57:18PM +0200, Maxime Ripard wrote:
> On Mon, Sep 29, 2014 at 03:54:00PM +0200, Thierry Reding wrote:
> > On Mon, Sep 29, 2014 at 01:34:36PM +0200, Maxime Ripard wrote:
> > > On Mon, Sep 29, 2014 at 12:44:57PM +0200, Thierry Reding wrote:
> > > > > >> 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.
> > > > >
> > > > > > 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.
> > > > >
> > > > > Calling clk_disable() preceding clk_enable() is a bug.
> > > > >
> > > > > Calling clk_disable() after clk_enable() will disable the clock (and
> > > > > its parents)
> > > > > if the clock subsystem thinks there are no other users, which is what will
> > > > > happen here.
> > > >
> > > > Right. I'm not sure this is really applicable to this situation, though.
> > >
> > > It's actually very easy to do. Have a driver that probes, enables its
> > > clock, fails to probe for any reason, call clk_disable in its exit
> > > path. If there's no other user at that time of this particular clock
> > > tree, it will be shut down. Bam. You just lost your framebuffer.
> > >
> > > Really, it's just that simple, and relying on the fact that some other
> > > user of the same clock tree will always be their is beyond fragile.
> >
> > Perhaps the meaning clk_ignore_unused should be revised, then. What you
> > describe isn't at all what I'd expect from such an option. And it does
> > not match the description in Documentation/kernel-parameters.txt either.
>
> Well, it never says that it also prevent them from being disabled
> later on. But it's true it's not very explicit.
It says:
Keep all clocks already enabled by bootloader on,
even if no driver has claimed them. ...
There's no "until" or anything there, so I interpret that as
indefinitely.
> > > > Either way, if there are other users of a clock then they will just as
> > > > likely want to modify the rate at which point simplefb will break just
> > > > as badly.
> > >
> > > And this can be handled just as well. Register a clock notifier,
> > > refuse any rate change, done. But of course, that would require having
> > > a clock handle.
> > >
> > > Now, how would *you* prevent such a change?
> >
> > Like I said in the other thread. If you have two drivers that use the
> > same clock but need different frequencies you've lost anyway.
>
> Except that the driver that has the most logic (ie not simplefb) will
> have a way to recover and deal with that.
You're making an assumption here. Why would the driver have such logic
if nothing ever prevented it from setting the rate properly before.
> But sure, you can still try to point new issues, get an obvious and
> robust solution, and then discard the issue when the solution doesn't
> go your way...
And you've already proven that you're completely unwilling to even
consider any other solution than what was originally proposed, so I
really don't see how discussing this further with you is going to be
productive.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-09-30 5:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929155517.GR16977@sirena.org.uk>
[-- Attachment #1: Type: text/plain, Size: 1305 bytes --]
On Mon, Sep 29, 2014 at 04:55:17PM +0100, Mark Brown wrote:
> On Mon, Sep 29, 2014 at 03:47:15PM +0200, Thierry Reding wrote:
[...]
> > 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.
>
> So long as we're ensuring that when we don't start supporting resources
> without DT additions or at least require DT additions to actively manage
> them (which can then include simplefb hookup) we should be fine.
I'm not sure I understand what you mean. If we add a driver for the PMIC
that exposes these regulators and then add a DT node for the PMIC, we'd
still need to fix the firmware to generate the appropriate DT properties
to allow simplefb to enable the regulators.
So unless firmware is updated at the same time, regulators will get
disabled because they are unused.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-09-30 5:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929162814.GE4081@lukather>
[-- Attachment #1: Type: text/plain, Size: 6789 bytes --]
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:
> > 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.
>
> 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.
> > 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.
> > > > > 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.
> 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.
> > > > > 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.
> > > > 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.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox