* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-08-27 13:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140827125613.GF23186@ulmo>
[-- Attachment #1: Type: text/plain, Size: 11763 bytes --]
On Wed, Aug 27, 2014 at 02:56:14PM +0200, Thierry Reding wrote:
> > >> So second of all, Thierry, what exactly is the technical argument against
> > >> adding support for dealing with clocks to simplefb ?
> > >
> > > I've already stated technical arguments against it. You could just go
> > > back and read the thread again, or would you rather want me to repeat
> > > them for your convenience?
> >
> > It is a long thread, IIRC your main arguments against this are:
> >
> > 1) You don't want this kind of platform-specific knowledge in simplefb
> > 2) That simplefb is supposed to be simple, and this is not simple
> >
> > As for 1. several people have already argued that clocks as such are
> > an abstract concept, found one many platforms and as such are not
> > platform specific.
>
> That alone doesn't justify this patch. SoCs often have a requirement to
> enable clocks in a specific order, for example. Other systems may need
> to coordinate clocks with resets in a specific order or enable power
> domains and such. All that is very SoC specific and if we don't make it
> very clear what this driver assumes about the state of the system, then
> we can't object to anybody adding support for those later on either. The
> result of that is going to be a driver that needs to handle all kinds of
> combinations of resources.
Which is not an issue in our case, since the firmware enabled them
already.
> So while clocks themselves are fairly generic the way in which they need
> to be used is highly platform-specific.
>
> Let me also reiterate what I've said a couple of times already. The
> issue here isn't that simplefb needs to manage these clocks in any way.
> Firmware has already set them up so that display works. The reason why
> you need this patch is so that the clock framework doesn't automatically
> turn them off. With the proposed patch the driver enables these clocks,
> but that's not what it needs to do, they are already on. It's a work
> around to prevent the clock framework into not disabling them.
>
> So as far as I'm concerned this points to a fundamental issue with how
> the clock framework handles unused clocks. Therefore that is the problem
> that should be solved, not worked around.
Well, this is again a philosophical difference them. I find like the
right thing to do for the clock framework to disable the clocks that
are enabled if no one reports them as used. That also comes down to
all the drivers that require a clock to stay enabled should report to
the clock framework to tell that it uses this clock. Fortunately, we
have everything in place to do so.
> > Any generic hw driver dealing with embedded platforms, be it ahci, ohci,
> > ehci, etc. has ended up adding abstract support for things like clocks
> > (and other resources). I say abstract support here, since to all these
> > drivers the support was added in a way that no platform specific knowledge
> > is necessary, they just deal with clocks not caring about the specific
> > underlying clock implementation, clock names, etc.
>
> Yes, I know that. You and I both had a very similar discussion not so
> long ago. But the above aren't quite the same as simplefb. The devices
> follow at least a well-known standard (EHCI, OHCI, AHCI), so there's
> bound to be more commonalities between them than between the various
> display devices that simplefb potentially can support.
>
> Interestingly though, if you look at what hardware really is supported
> by the generic drivers that deal with embedded platforms, there isn't
> all that much diversity (I've manually stripped out non-DTS occurrences
> since they aren't relevant for this discussion):
>
> $ git grep -n generic-ohci
> arch/arm/boot/dts/sun4i-a10.dtsi:451: compatible = "allwinner,sun4i-a10-ohci", "generic-ohci";
> arch/arm/boot/dts/sun4i-a10.dtsi:492: compatible = "allwinner,sun4i-a10-ohci", "generic-ohci";
> arch/arm/boot/dts/sun5i-a10s.dtsi:403: compatible = "allwinner,sun5i-a10s-ohci", "generic-ohci";
> arch/arm/boot/dts/sun5i-a13.dtsi:376: compatible = "allwinner,sun5i-a13-ohci", "generic-ohci";
> arch/arm/boot/dts/sun6i-a31.dtsi:410: compatible = "allwinner,sun6i-a31-ohci", "generic-ohci";
> arch/arm/boot/dts/sun6i-a31.dtsi:432: compatible = "allwinner,sun6i-a31-ohci", "generic-ohci";
> arch/arm/boot/dts/sun6i-a31.dtsi:443: compatible = "allwinner,sun6i-a31-ohci", "generic-ohci";
> arch/arm/boot/dts/sun7i-a20.dtsi:535: compatible = "allwinner,sun7i-a20-ohci", "generic-ohci";
> arch/arm/boot/dts/sun7i-a20.dtsi:576: compatible = "allwinner,sun7i-a20-ohci", "generic-ohci";
> arch/powerpc/boot/dts/akebono.dts:144: compatible = "ibm,476gtr-ohci", "generic-ohci";
> arch/powerpc/boot/dts/akebono.dts:151: compatible = "ibm,476gtr-ohci", "generic-ohci";
>
> $ git grep -n generic-ehci
> arch/arm/boot/dts/rk3288.dtsi:199: compatible = "generic-ehci";
> arch/arm/boot/dts/rk3288.dtsi:210: compatible = "generic-ehci";
> arch/arm/boot/dts/sun4i-a10.dtsi:441: compatible = "allwinner,sun4i-a10-ehci", "generic-ehci";
> arch/arm/boot/dts/sun4i-a10.dtsi:482: compatible = "allwinner,sun4i-a10-ehci", "generic-ehci";
> arch/arm/boot/dts/sun5i-a10s.dtsi:393: compatible = "allwinner,sun5i-a10s-ehci", "generic-ehci";
> arch/arm/boot/dts/sun5i-a13.dtsi:366: compatible = "allwinner,sun5i-a13-ehci", "generic-ehci";
> arch/arm/boot/dts/sun6i-a31.dtsi:399: compatible = "allwinner,sun6i-a31-ehci", "generic-ehci";
> arch/arm/boot/dts/sun6i-a31.dtsi:421: compatible = "allwinner,sun6i-a31-ehci", "generic-ehci";
> arch/arm/boot/dts/sun7i-a20.dtsi:525: compatible = "allwinner,sun7i-a20-ehci", "generic-ehci";
> arch/arm/boot/dts/sun7i-a20.dtsi:566: compatible = "allwinner,sun7i-a20-ehci", "generic-ehci";
> arch/powerpc/boot/dts/akebono.dts:130: compatible = "ibm,476gtr-ehci", "generic-ehci";
>
> For generic-ahci there aren't any matches, but here are a few that are
> marked compatible with it using different compatible strings:
>
> $ git grep -n snps,spear-ahci
> arch/arm/boot/dts/spear1310.dtsi:60: compatible = "snps,spear-ahci";
> arch/arm/boot/dts/spear1310.dtsi:69: compatible = "snps,spear-ahci";
> arch/arm/boot/dts/spear1310.dtsi:78: compatible = "snps,spear-ahci";
> arch/arm/boot/dts/spear1340.dtsi:43: compatible = "snps,spear-ahci";
>
> $ git grep -n snps,exynos5440-ahci
> arch/arm/boot/dts/exynos5440.dtsi:241: compatible = "snps,exynos5440-ahci";
>
> $ git grep -n 'ibm,476gtr-ahci'
> arch/powerpc/boot/dts/akebono.dts:123: compatible = "ibm,476gtr-ahci";
>
> $ git grep -n 'snps,dwc-ahci'
> arch/arm/boot/dts/dra7.dtsi:1049: compatible = "snps,dwc-ahci";
> arch/arm/boot/dts/exynos5250.dtsi:265: compatible = "snps,dwc-ahci";
> arch/arm/boot/dts/omap5.dtsi:919: compatible = "snps,dwc-ahci";
>
> That looks fairly homogenous to me. There are also things like this
> (from the ahci_platform.c driver):
>
> if (of_device_is_compatible(dev->of_node, "hisilicon,hisi-ahci"))
> hpriv->flags |= AHCI_HFLAG_NO_FBS | AHCI_HFLAG_NO_NCQ;
>
> With more supported hardware there's bound to be more quirks like that.
>
> So there's a couple of SoCs and boards that actually are generic enough
> to work with a generic driver. And then there's a whole bunch of other
> drivers for hardware that's compliant with the same standard yet needs
> different drivers. To me that's a clear indication that there isn't
> enough genericity to warrant a generic driver in the first place.
>
> The commonality is in the functionality and defined by the standard
> registers. But there's little to no commonality in how that interface is
> glued into the SoC. Luckily the above subsystems implement the standard
> hardware programming in a library, so that non-generic drivers can still
> make use of most of the generic code.
To be fair, these additions are a couple of release old, and are quite
recent. You can't really make any judgement based on barely 2 releases
of history.
> > If you look at how almost all dt bindings work, then the dt node for
> > a device specifies the resources needed, I don't see why simplefb would
> > be so special that it should be different here. I agree that it is
> > important to get the abstractions right here, but to me it seems that
> > the right abstraction is to be consistent with how all other devices
> > are abstracted and to add needed resources to the dt node for the
> > simplefb.
>
> But simplefb is fundamentally different from other devices. It isn't a
> physical device at all. It's a virtual device that reuses resources as
> set up by some other piece of code (firmware). It implies that there's
> nothing that needs to be managed. It should only create a framebuffer
> with the given parameters and allow the kernel (and userspace) to render
> into it.
Nothing should be managed, but everything should stay as is. Again,
this is something that we all seem to agree on, yet differ completely
on how to implement that.
> The only way you can deal with such virtual, completely generic devices
> is by being very explicit about the requirements. For simplefb the
> assumptions are that firmware set everything up and passes information
> about what it set up to the kernel so that it can be reused. None of the
> resources need to be explicitly managed because they have all been
> properly configured. For that reason, again, I think the right way is
> for the kernel not to switch off any of the used resources.
So, you're saying that the firmware should inform the kernel about
what clocks it has set up?
I do agree on that too.
And it looks like we reached an agreement then, since it's exactly
what this patch is relying on.
> If you want to equate simplefb to other drivers then it is fundamentally
> broken anyway. Given only what's in the DTB the simplefb driver won't be
> able to do anything useful. Consider what would happen if the firmware
> didn't set up all the resources. Then the DT is missing resets, power
> supplies and all that to make it work.
That would mean that the framebuffer wasn't working in the first
place, which breaks the initial assumption, doesn't it?
> > > so I think we should keep the discussion going for a bit and
> > > see if we really can't come up with something that will fix this for a
> > > more general case rather than just your particular use-case. It's
> > > evidently a recurring problem that others suffer from, too.
> >
> > Yes needing resources despite the device being behind some generic
> > interface where the main driver code for that interface has no knowledge
> > about such resources sofar, because e.g. on PC-s this was not necessary
> > is a recurring problem, which has been solved already 3 times at least,
> > see the ohci-, ehci- and ahci-platform drivers, and for all 3 the conclusion
> > was that the best solution was to simple add the resources to the dt-node,
> > and add code to claim and enable them.
>
> And as I pointed out above in all three cases the generic platform
> driver is only marginally useful because many SoCs have specific
> requirements that make them incompatible with the generic driver.
s/marginally useful/marginally used/
--
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: Hans de Goede @ 2014-08-27 13:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140827125613.GF23186@ulmo>
Hi,
On 08/27/2014 02:56 PM, Thierry Reding wrote:
> On Wed, Aug 27, 2014 at 12:35:24PM +0200, Hans de Goede wrote:
<snip>
>>>> So second of all, Thierry, what exactly is the technical argument against
>>>> adding support for dealing with clocks to simplefb ?
>>>
>>> I've already stated technical arguments against it. You could just go
>>> back and read the thread again, or would you rather want me to repeat
>>> them for your convenience?
>>
>> It is a long thread, IIRC your main arguments against this are:
>>
>> 1) You don't want this kind of platform-specific knowledge in simplefb
>> 2) That simplefb is supposed to be simple, and this is not simple
>>
>> As for 1. several people have already argued that clocks as such are
>> an abstract concept, found one many platforms and as such are not
>> platform specific.
>
> That alone doesn't justify this patch. SoCs often have a requirement to
> enable clocks in a specific order, for example. Other systems may need
> to coordinate clocks with resets in a specific order or enable power
> domains and such. All that is very SoC specific and if we don't make it
> very clear what this driver assumes about the state of the system, then
> we can't object to anybody adding support for those later on either. The
> result of that is going to be a driver that needs to handle all kinds of
> combinations of resources.
The problems your talking about here all have to do with ordering how
things are enabled, but what we're talking about here is keeping things
enabled which are already enabled by the bootloader, so there is no
ordering problem.
> So while clocks themselves are fairly generic the way in which they need
> to be used is highly platform-specific.
>
> Let me also reiterate what I've said a couple of times already. The
> issue here isn't that simplefb needs to manage these clocks in any way.
> Firmware has already set them up so that display works. The reason why
> you need this patch is so that the clock framework doesn't automatically
> turn them off. With the proposed patch the driver enables these clocks,
> but that's not what it needs to do, they are already on. It's a work
> around to prevent the clock framework into not disabling them.
>
> So as far as I'm concerned this points to a fundamental issue with how
> the clock framework handles unused clocks. Therefore that is the problem
> that should be solved, not worked around.
Hmm I see, in my mind the problem is not that the clk framework disables
unused clocks, but that no one is marking the clocks in question as used.
Someone should mark these clocks as used so that they do not get disabled.
We could add a clk_mark_in_use function and have the simplefb patch call
that instead of clk_prepare_and_enable, or maybe even better just only
claim the clks and teach the clk framework to not disable claimed clk
in its cleanup run. That would make it clear that simplefb is not enabling
anything, just claiming resource its need to avoid them getting removed
from underneath simplefb, would that work for you ?
>> Any generic hw driver dealing with embedded platforms, be it ahci, ohci,
>> ehci, etc. has ended up adding abstract support for things like clocks
>> (and other resources). I say abstract support here, since to all these
>> drivers the support was added in a way that no platform specific knowledge
>> is necessary, they just deal with clocks not caring about the specific
>> underlying clock implementation, clock names, etc.
>
> Yes, I know that. You and I both had a very similar discussion not so
> long ago. But the above aren't quite the same as simplefb. The devices
> follow at least a well-known standard (EHCI, OHCI, AHCI), so there's
> bound to be more commonalities between them than between the various
> display devices that simplefb potentially can support.
>
> Interestingly though, if you look at what hardware really is supported
> by the generic drivers that deal with embedded platforms, there isn't
> all that much diversity (I've manually stripped out non-DTS occurrences
> since they aren't relevant for this discussion):
That is because the generic platform drivers were only added recently
to stop the insanity where each new soc got its own ohci-soc.c /
ehci-soc.c file.
<snip>
> arch/arm/boot/dts/rk3288.dtsi:199: compatible = "generic-ehci";
> arch/arm/boot/dts/rk3288.dtsi:210: compatible = "generic-ehci";
(offtopic) And I see that despite the recent addition, we actually already have our
first non sunxi user, good, I didn't even know the rockchip guys were using this :)
And I know that there are quite a few more in the pipeline (waiting for their
usb phy and dt patches to get merged).
<snip>
> So there's a couple of SoCs and boards that actually are generic enough
> to work with a generic driver. And then there's a whole bunch of other
> drivers for hardware that's compliant with the same standard yet needs
> different drivers.
No, there are a lot of other drivers which were written before someone
decided that having 10-20 drivers which were 90% copy and paste of each
other was a bad idea, but we're really going offtopic here.
<snip>
>> This really is unavoidable. The framebuffer memory needed was mentioned
>> as another example of a resource in this thread. You suggested using the
>> new mem-reserve framework for this. I agree that that is the right thing
>> to do, but even then we still need a way to tie this reserved mem to
>> the simplefb, so that the kernel can know when it is no longer used
>> and can be added to the general memory pool.
>
> Yes, memory handling seems like it isn't handled optimally by simplefb.
> To be fair the simplefb driver predates the reserved-memory bindings. I
> think it might be worthwhile to investigate on how to extend the binding
> to make use of that, though. I think the assumption currently is that
> the framebuffer memory will be lost forever (it needs to be carved out
> of the physical memory that the kernel gets to see). I couldn't find any
> code that returned reserved-memory to the kernel, but it's certainly a
> feature that I think we want. Especially if the firmware framebuffer is
> large (as in 1080p).
Ack.
>> If you look at how almost all dt bindings work, then the dt node for
>> a device specifies the resources needed, I don't see why simplefb would
>> be so special that it should be different here. I agree that it is
>> important to get the abstractions right here, but to me it seems that
>> the right abstraction is to be consistent with how all other devices
>> are abstracted and to add needed resources to the dt node for the
>> simplefb.
>
> But simplefb is fundamentally different from other devices. It isn't a
> physical device at all. It's a virtual device that reuses resources as
> set up by some other piece of code (firmware). It implies that there's
> nothing that needs to be managed. It should only create a framebuffer
> with the given parameters and allow the kernel (and userspace) to render
> into it.
>
> The only way you can deal with such virtual, completely generic devices
> is by being very explicit about the requirements. For simplefb the
> assumptions are that firmware set everything up and passes information
> about what it set up to the kernel so that it can be reused.
> None of the resources need to be explicitly managed because they have all
> been properly configured. For that reason, again, I think the right way is
> for the kernel not to switch off any of the used resources.
The key word here is "the used resources" to me this is not about simlefb
managing resources, but marking them as used as long as it needs them, like
it will need to do for the reserved mem chunk.
<snip>
>> As for 2. Yes this will add some more code to simplefb, more code but
>> code which is not complicated in anyway, so the simplefb code would
>> become larger but still stay simple. Were as all the suggested
>> alternatives sofar are at least an order of magnitude more complicated,
>> crossing many subsystems, demanding coordination with userspace, etc.
>
> I'm not objecting to this patch because of the complexity, but because
> it is an illusion that the code is in any way generic.
Ok.
Regards,
Hans
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-08-27 13:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140827093121.GA23186@ulmo>
[-- Attachment #1: Type: text/plain, Size: 883 bytes --]
Hi,
On Wed, Aug 27, 2014 at 11:31:24AM +0200, Thierry Reding wrote:
> > and only pretty much you against it,
>
> At least Stephen also spoke out about it. But this quickly devolved into
> the kind of thread you want to get out of as quickly as possible, so I
> can't blame him for not continuing this discussion. In fact I'm very
> much regretting getting into this in the first place. You clearly have
> no intention to even consider any possible other solution that what was
> posted, so it's pretty useless to try and convince you.
No. I'm ready to consider any solution that doesn't involve hardcoding
these clocks in the kernel clock driver. I believe that is a pretty
similar, yet opposite, stance than the one you're currently making.
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: Maxime Ripard @ 2014-08-27 13:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAGRGNgWy=ijkakoKtXiEnFuVQpQT6WXW45-nL4x9DvfTKoA1OA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3284 bytes --]
Hi Julian,
On Wed, Aug 27, 2014 at 10:45:14PM +1000, Julian Calaby wrote:
> Hi All,
>
> On Wed, Aug 27, 2014 at 6:37 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > Hi Maxime,
> >
> > On Wed, Aug 27, 2014 at 10:22 AM, Maxime Ripard
> > <maxime.ripard@free-electrons.com> wrote:
> >> On Wed, Aug 27, 2014 at 08:40:57AM +0100, Mark Brown wrote:
> >>> On Tue, Aug 26, 2014 at 08:40:09PM +0200, Henrik Nordström wrote:
> >>>
> >>> > It is not clear to me where the hardware resources should be listed in
> >>> > DT, being it a simplefb node or part of the actual hardware device node
> >>> > properly marked as dynamic boot defaults or something else? It's
> >>> > somewhere inbetween hardware and virtual device, and somewhat volatile.
> >>> > As far as simplefb is concerned it is a hardware desription of the
> >>> > framebuffer, but for a kms driver it's no more than firmware handover of
> >>> > boottime settings and ceases to exists once the kms driver have
> >>> > reconfigured the hardware.
> >>>
> >>> Is simplefb something that should be in the device tree distinctly in
> >>> the first place - shouldn't it be a subset of the functionality of the
> >>> video nodes? It's the same hardware being driven differently.
> >>
> >> Therorically, yes, but that would mean knowing beforehand what the
> >> final binding will look like, even before submitting the driver. Since
> >> the bindings are always reviewed, and most of the time changed
> >> slightly, that wouldn't work very well with the DT as a stable ABI
> >> policy I guess.
> >
> > If you don't know how the bindings for a device will look like at the time of
> > writing your DTS, you're always screwed, whether you add a simpefb
> > node or not.
> >
> > If you know how the bindings look like, just add the device, with an extra
> > "linux,simplefb" compatibility value.
> > If you don't know how the bindings look like, do your utter best in
> > guessing. Your DTS must be amended later anyway, either because
> > you guessed wrong[*] (in case you added a node to have simplefb
> > working), or because you have to add a real device node (in case you
> > didn't add one for simplefb).
> >
> > [*] Actually you may have guessed right, in which case it'll continue
> > working as-is, and everybody will be happy.
> > Whether you want to keep backwards-compatibility in your future driver
> > with the "guessed wrong" node is up to you.
>
> I apologise if I'm stepping on anyone's toes or horribly
> misrepresenting device tree's capabilities, but couldn't we start out
> with something like:
>
> disp@whatever {
> compatible = "sunxi,sun4i-disp";
> clocks = <&stuff 1>, <&stuff 2>, <&stuff 3>;
Unfortunately, it's slightly more complicated than that, since there
is several clocks involved, that feed several different IPs that need
to stay on.
In this patch, and given the current code we have, we need to have the
gates for the HDMI controller, the LCD controller and the display
backend, which are three different IPs, with one clock each, that
would be described by three different nodes.
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-08-27 13:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53FDD40A.3040106@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2150 bytes --]
On Wed, Aug 27, 2014 at 02:50:18PM +0200, Hans de Goede wrote:
> Hi,
>
> On 08/27/2014 02:44 PM, jonsmirl@gmail.com wrote:
>
> <snip>
>
> >> 1) Serving as *the* fb until a kms driver is present
> >
> > Why can't we whip together a real device specific fbdev driver and
> > quit trying to run without a device specific driver in place? History
> > has shown that to be bad idea except during early boot when there
> > isn't a choice.
>
> Writing a kms driver and getting it upstream is not a simple task, and
> starting with a too limited implementation is dangerous as we need to get
> the devicetree binding first from the get go.
>
> IOW this is not really a realistic answer to problem 1.
>
> <snip>
>
> >> 2) Allowing for output of early kernel boot messages.
> >
> > For this to work the kernel needs to know two things - address of the
> > framebuffer and the x/y layout. Then just nuke this display when the
> > kernel finishes the boot process.
>
> Please read my earlier mails on how completely flawed it is to write
> "when the kernel finishes the boot process". The problem is that there
> is no way to really determine when this happens.
That doesn't mean we can't create new ways to signal this. One option
would be for userspace to signal this, another would be to have critical
subsystems broadcast such events so that things like the clock framework
can listen for those and react accordingly. The fact remains that
disabling all unused clocks unconditinally at late_initcall time is not
(always) the right thing to do.
What this really is is a policy decision and traditionally we don't like
to dictate policy in the kernel, so having something like an optional
feature that would allow userspace to notify the kernel that it's
initialized doesn't sound like a bad idea. This doesn't necessarily have
to mean that all possible devices have been probed (users could choose
to blacklist modules, etc.) but just an indication that the system is
done with essential tasks (for desktop distributions I would assume that
means the graphical login manager has started).
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: jonsmirl @ 2014-08-27 12:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53FDD40A.3040106@redhat.com>
On Wed, Aug 27, 2014 at 8:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 08/27/2014 02:44 PM, jonsmirl@gmail.com wrote:
>
> <snip>
>
>>> 1) Serving as *the* fb until a kms driver is present
>>
>> Why can't we whip together a real device specific fbdev driver and
>> quit trying to run without a device specific driver in place? History
>> has shown that to be bad idea except during early boot when there
>> isn't a choice.
>
> Writing a kms driver and getting it upstream is not a simple task, and
> starting with a too limited implementation is dangerous as we need to get
> the devicetree binding first from the get go.
I didn't say write a kms driver, I said write a fbdev driver. Look in
drivers/video/fbdev. fbdev drivers are used on many embedded systems.
Its what we used for years before kms came along.
>
> IOW this is not really a realistic answer to problem 1.
>
> <snip>
>
>>> 2) Allowing for output of early kernel boot messages.
>>
>> For this to work the kernel needs to know two things - address of the
>> framebuffer and the x/y layout. Then just nuke this display when the
>> kernel finishes the boot process.
>
> Please read my earlier mails on how completely flawed it is to write
> "when the kernel finishes the boot process". The problem is that there
> is no way to really determine when this happens.
Let me rephrase that, when the existing code in the clk system and
regulator systems decide to do their clean up the display will get
nuked.
>
> Regards,
>
> Hans
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-27 12:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53FDB46C.5010609@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 17631 bytes --]
On Wed, Aug 27, 2014 at 12:35:24PM +0200, Hans de Goede wrote:
> Hi,
>
> On 08/27/2014 11:31 AM, Thierry Reding wrote:
> > Can you please fix you mail setup. You're addressing me directly in this
> > email, yet I'm neither in To: nor Cc: headers. That's really annoying.
>
> Sorry, I'll pay closer attention that you're in the To when I'm directly
> addressing you next time.
>
> > On Tue, Aug 26, 2014 at 09:59:00PM +0200, Hans de Goede wrote:
>
> <snip>
>
> >>> Either way, Mark already suggested a different alternative in another
> >>> subthread, namely to add a new kind of checkpoint at which subsystems
> >>> can call a "disable unused" function that's called later than a late
> >>> initcall. This is not going to fix things for you immediately because
> >>> the clocks will still be switched off (only later) if you don't have
> >>> a real driver that's claiming the clocks. But you can work around that
> >>> for now by making the relevant clocks always on and remove that
> >>> workaround once a real driver is loaded that knows how to handle them
> >>> properly.
> >>
> >> This will simply not work, and is a ton more complicated then
> >> simply teaching simplefb about a clocks property, which is a really simple
> >> and clean patch.
> >>
> >> First of all let me explain why this won't work. When should those
> >> subsystems call this "later then a late initcall" call ? the kms driver
> >> may very well be a kernel module, which will be loaded by userspace,
> >> so we would need this to be triggered by userspace, but when ?
> >>
> >> When the initrd is done? What then if the kms driver is not in the initrd,
> >> so maybe when udev is done enumerating devices, but what then if the kernel
> >> is still enumerating hardware at this time?
> >
> > Usually an initrd knows how to wait for all or a given set of devices to
> > be probed. udev is pretty good for that.
>
> udev does not have a clue how to wait for all devices, it can be used to
> wait for a specific device to show up, that is true, but this requires
> adding specific logic for this (class of) device to the initrd init process,
> which is really sub-optimal.
I thought pretty much every distribution that did anything remotely
related to early DRM/KMS like plymouth and whatnot already had this
specific logic. It's been a while since I dealt with all that, but
something roughly like this used to be present in pretty much all
distributions:
udevadm trigger --subsystem-match=graphics \
--subsystem-match=drm \
--subsystem-match=tty
udevadm settle
The effect of that being that udevadm settle would block as long as
devices were still being probed. Admittedly this was all long before
deferred probing was introduced, so I have no idea how well it plays
with it. But optimizing deferred probing is an entirely different topic
currently under discussion that should help sorting this out, too.
> >> Will we just put a sleep 30
> >> in our initscripts to make sure the kernel is done scanning all busses,
> >> will that be long enough? Maybe we need to re-introduce the scsi_wait_done
> >> kernel module which was added as an ugly hack to allow userspace to wait
> >> for the kernel to have finished scanning scsi (and ata / sata) busses,
> >> for controllers found at the time the module was load. Which never worked
> >> reliable, because it would be possible that the controller itself still
> >> needed to be discovered. We've spend years cleaning up userspace enough
> >> to be able to kill scsi_wait_done. We've already been down this road of
> >> waiting for hw enumeration to be done, and the problem is that on
> >> modern systems *it is never done*.
> >
> > Those are mostly integration issues. If you don't load the driver from
> > initrd then I don't think anybody will complain when there's flicker
> > when it finally gets loaded. The whole point of this is to have early
> > access to the framebuffer and get display and graphics going as soon as
> > possible, so please don't pull in hypothetical scenarios where people
> > manually load drivers half an hour after the system has booted.
>
> In another mail in this thread you've mentioned 2 uses for simplefb:
>
> 1) Serving as *the* fb until a kms driver is present
> 2) Allowing for output of early kernel boot messages.
>
> I agree that those are the 2 use-cases we're dealing with here.
>
> Note that the whole discussion we're having hear about disabling the clocks
> once hardware probing is "done", is moot for 1. One possible way mentioned
> of dealing with 1. is to mark these clocks as always on for now, and then we
> need to drop this hack (I cannot call this anything else then a hack), once
> we get kms, meaning interdependencies between the dropping patch and the kms
> adding patches, etc. All not very pretty.
I don't think it's that bad. When your real driver is merged you can
still keep the clocks always on for a release cycle if you worry about
any fallout. It's unlikely to going to be your last driver that you
merge and people aren't immediately going to build products based on it,
so it's unlikely that anyone will notice the wasted power.
> For 2. We have what you call "mostly integration issues". Allow me to copy
> and paste in another bit of this thread here:
>
> >> I've been doing low level Linux development for 10 years now and I've
> >> worked a lot on storage (raid, iscsi, fcoe support, etc.) in the past.
> >
> > Ah, so this is now officially a pissing contest, is it?
>
> No I'm merely trying to explain that I've a ton of experience with what
> you seem to think of as merely integration issues, and they are a royal PITA,
> and thus to be avoided at all costs. Really, please just trust me on this
> one, doing the whole add some later then late init call type is a bad idea.
Well, you could equally just trust me on this one when I say adding
clock support for this driver is bad. Stalemate.
> >> So second of all, Thierry, what exactly is the technical argument against
> >> adding support for dealing with clocks to simplefb ?
> >
> > I've already stated technical arguments against it. You could just go
> > back and read the thread again, or would you rather want me to repeat
> > them for your convenience?
>
> It is a long thread, IIRC your main arguments against this are:
>
> 1) You don't want this kind of platform-specific knowledge in simplefb
> 2) That simplefb is supposed to be simple, and this is not simple
>
> As for 1. several people have already argued that clocks as such are
> an abstract concept, found one many platforms and as such are not
> platform specific.
That alone doesn't justify this patch. SoCs often have a requirement to
enable clocks in a specific order, for example. Other systems may need
to coordinate clocks with resets in a specific order or enable power
domains and such. All that is very SoC specific and if we don't make it
very clear what this driver assumes about the state of the system, then
we can't object to anybody adding support for those later on either. The
result of that is going to be a driver that needs to handle all kinds of
combinations of resources.
So while clocks themselves are fairly generic the way in which they need
to be used is highly platform-specific.
Let me also reiterate what I've said a couple of times already. The
issue here isn't that simplefb needs to manage these clocks in any way.
Firmware has already set them up so that display works. The reason why
you need this patch is so that the clock framework doesn't automatically
turn them off. With the proposed patch the driver enables these clocks,
but that's not what it needs to do, they are already on. It's a work
around to prevent the clock framework into not disabling them.
So as far as I'm concerned this points to a fundamental issue with how
the clock framework handles unused clocks. Therefore that is the problem
that should be solved, not worked around.
> Any generic hw driver dealing with embedded platforms, be it ahci, ohci,
> ehci, etc. has ended up adding abstract support for things like clocks
> (and other resources). I say abstract support here, since to all these
> drivers the support was added in a way that no platform specific knowledge
> is necessary, they just deal with clocks not caring about the specific
> underlying clock implementation, clock names, etc.
Yes, I know that. You and I both had a very similar discussion not so
long ago. But the above aren't quite the same as simplefb. The devices
follow at least a well-known standard (EHCI, OHCI, AHCI), so there's
bound to be more commonalities between them than between the various
display devices that simplefb potentially can support.
Interestingly though, if you look at what hardware really is supported
by the generic drivers that deal with embedded platforms, there isn't
all that much diversity (I've manually stripped out non-DTS occurrences
since they aren't relevant for this discussion):
$ git grep -n generic-ohci
arch/arm/boot/dts/sun4i-a10.dtsi:451: compatible = "allwinner,sun4i-a10-ohci", "generic-ohci";
arch/arm/boot/dts/sun4i-a10.dtsi:492: compatible = "allwinner,sun4i-a10-ohci", "generic-ohci";
arch/arm/boot/dts/sun5i-a10s.dtsi:403: compatible = "allwinner,sun5i-a10s-ohci", "generic-ohci";
arch/arm/boot/dts/sun5i-a13.dtsi:376: compatible = "allwinner,sun5i-a13-ohci", "generic-ohci";
arch/arm/boot/dts/sun6i-a31.dtsi:410: compatible = "allwinner,sun6i-a31-ohci", "generic-ohci";
arch/arm/boot/dts/sun6i-a31.dtsi:432: compatible = "allwinner,sun6i-a31-ohci", "generic-ohci";
arch/arm/boot/dts/sun6i-a31.dtsi:443: compatible = "allwinner,sun6i-a31-ohci", "generic-ohci";
arch/arm/boot/dts/sun7i-a20.dtsi:535: compatible = "allwinner,sun7i-a20-ohci", "generic-ohci";
arch/arm/boot/dts/sun7i-a20.dtsi:576: compatible = "allwinner,sun7i-a20-ohci", "generic-ohci";
arch/powerpc/boot/dts/akebono.dts:144: compatible = "ibm,476gtr-ohci", "generic-ohci";
arch/powerpc/boot/dts/akebono.dts:151: compatible = "ibm,476gtr-ohci", "generic-ohci";
$ git grep -n generic-ehci
arch/arm/boot/dts/rk3288.dtsi:199: compatible = "generic-ehci";
arch/arm/boot/dts/rk3288.dtsi:210: compatible = "generic-ehci";
arch/arm/boot/dts/sun4i-a10.dtsi:441: compatible = "allwinner,sun4i-a10-ehci", "generic-ehci";
arch/arm/boot/dts/sun4i-a10.dtsi:482: compatible = "allwinner,sun4i-a10-ehci", "generic-ehci";
arch/arm/boot/dts/sun5i-a10s.dtsi:393: compatible = "allwinner,sun5i-a10s-ehci", "generic-ehci";
arch/arm/boot/dts/sun5i-a13.dtsi:366: compatible = "allwinner,sun5i-a13-ehci", "generic-ehci";
arch/arm/boot/dts/sun6i-a31.dtsi:399: compatible = "allwinner,sun6i-a31-ehci", "generic-ehci";
arch/arm/boot/dts/sun6i-a31.dtsi:421: compatible = "allwinner,sun6i-a31-ehci", "generic-ehci";
arch/arm/boot/dts/sun7i-a20.dtsi:525: compatible = "allwinner,sun7i-a20-ehci", "generic-ehci";
arch/arm/boot/dts/sun7i-a20.dtsi:566: compatible = "allwinner,sun7i-a20-ehci", "generic-ehci";
arch/powerpc/boot/dts/akebono.dts:130: compatible = "ibm,476gtr-ehci", "generic-ehci";
For generic-ahci there aren't any matches, but here are a few that are
marked compatible with it using different compatible strings:
$ git grep -n snps,spear-ahci
arch/arm/boot/dts/spear1310.dtsi:60: compatible = "snps,spear-ahci";
arch/arm/boot/dts/spear1310.dtsi:69: compatible = "snps,spear-ahci";
arch/arm/boot/dts/spear1310.dtsi:78: compatible = "snps,spear-ahci";
arch/arm/boot/dts/spear1340.dtsi:43: compatible = "snps,spear-ahci";
$ git grep -n snps,exynos5440-ahci
arch/arm/boot/dts/exynos5440.dtsi:241: compatible = "snps,exynos5440-ahci";
$ git grep -n 'ibm,476gtr-ahci'
arch/powerpc/boot/dts/akebono.dts:123: compatible = "ibm,476gtr-ahci";
$ git grep -n 'snps,dwc-ahci'
arch/arm/boot/dts/dra7.dtsi:1049: compatible = "snps,dwc-ahci";
arch/arm/boot/dts/exynos5250.dtsi:265: compatible = "snps,dwc-ahci";
arch/arm/boot/dts/omap5.dtsi:919: compatible = "snps,dwc-ahci";
That looks fairly homogenous to me. There are also things like this
(from the ahci_platform.c driver):
if (of_device_is_compatible(dev->of_node, "hisilicon,hisi-ahci"))
hpriv->flags |= AHCI_HFLAG_NO_FBS | AHCI_HFLAG_NO_NCQ;
With more supported hardware there's bound to be more quirks like that.
So there's a couple of SoCs and boards that actually are generic enough
to work with a generic driver. And then there's a whole bunch of other
drivers for hardware that's compliant with the same standard yet needs
different drivers. To me that's a clear indication that there isn't
enough genericity to warrant a generic driver in the first place.
The commonality is in the functionality and defined by the standard
registers. But there's little to no commonality in how that interface is
glued into the SoC. Luckily the above subsystems implement the standard
hardware programming in a library, so that non-generic drivers can still
make use of most of the generic code.
> This really is unavoidable. The framebuffer memory needed was mentioned
> as another example of a resource in this thread. You suggested using the
> new mem-reserve framework for this. I agree that that is the right thing
> to do, but even then we still need a way to tie this reserved mem to
> the simplefb, so that the kernel can know when it is no longer used
> and can be added to the general memory pool.
Yes, memory handling seems like it isn't handled optimally by simplefb.
To be fair the simplefb driver predates the reserved-memory bindings. I
think it might be worthwhile to investigate on how to extend the binding
to make use of that, though. I think the assumption currently is that
the framebuffer memory will be lost forever (it needs to be carved out
of the physical memory that the kernel gets to see). I couldn't find any
code that returned reserved-memory to the kernel, but it's certainly a
feature that I think we want. Especially if the firmware framebuffer is
large (as in 1080p).
> If you look at how almost all dt bindings work, then the dt node for
> a device specifies the resources needed, I don't see why simplefb would
> be so special that it should be different here. I agree that it is
> important to get the abstractions right here, but to me it seems that
> the right abstraction is to be consistent with how all other devices
> are abstracted and to add needed resources to the dt node for the
> simplefb.
But simplefb is fundamentally different from other devices. It isn't a
physical device at all. It's a virtual device that reuses resources as
set up by some other piece of code (firmware). It implies that there's
nothing that needs to be managed. It should only create a framebuffer
with the given parameters and allow the kernel (and userspace) to render
into it.
The only way you can deal with such virtual, completely generic devices
is by being very explicit about the requirements. For simplefb the
assumptions are that firmware set everything up and passes information
about what it set up to the kernel so that it can be reused. None of the
resources need to be explicitly managed because they have all been
properly configured. For that reason, again, I think the right way is
for the kernel not to switch off any of the used resources.
If you want to equate simplefb to other drivers then it is fundamentally
broken anyway. Given only what's in the DTB the simplefb driver won't be
able to do anything useful. Consider what would happen if the firmware
didn't set up all the resources. Then the DT is missing resets, power
supplies and all that to make it work.
> As for 2. Yes this will add some more code to simplefb, more code but
> code which is not complicated in anyway, so the simplefb code would
> become larger but still stay simple. Were as all the suggested
> alternatives sofar are at least an order of magnitude more complicated,
> crossing many subsystems, demanding coordination with userspace, etc.
I'm not objecting to this patch because of the complexity, but because
it is an illusion that the code is in any way generic.
> > so I think we should keep the discussion going for a bit and
> > see if we really can't come up with something that will fix this for a
> > more general case rather than just your particular use-case. It's
> > evidently a recurring problem that others suffer from, too.
>
> Yes needing resources despite the device being behind some generic
> interface where the main driver code for that interface has no knowledge
> about such resources sofar, because e.g. on PC-s this was not necessary
> is a recurring problem, which has been solved already 3 times at least,
> see the ohci-, ehci- and ahci-platform drivers, and for all 3 the conclusion
> was that the best solution was to simple add the resources to the dt-node,
> and add code to claim and enable them.
And as I pointed out above in all three cases the generic platform
driver is only marginally useful because many SoCs have specific
requirements that make them incompatible with the generic driver.
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: Hans de Goede @ 2014-08-27 12:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKON4Owj=b7r_iuOOrr_5R++oSbxqF0G0i4puFz7aTaUF+weuw@mail.gmail.com>
Hi,
On 08/27/2014 02:44 PM, jonsmirl@gmail.com wrote:
<snip>
>> 1) Serving as *the* fb until a kms driver is present
>
> Why can't we whip together a real device specific fbdev driver and
> quit trying to run without a device specific driver in place? History
> has shown that to be bad idea except during early boot when there
> isn't a choice.
Writing a kms driver and getting it upstream is not a simple task, and
starting with a too limited implementation is dangerous as we need to get
the devicetree binding first from the get go.
IOW this is not really a realistic answer to problem 1.
<snip>
>> 2) Allowing for output of early kernel boot messages.
>
> For this to work the kernel needs to know two things - address of the
> framebuffer and the x/y layout. Then just nuke this display when the
> kernel finishes the boot process.
Please read my earlier mails on how completely flawed it is to write
"when the kernel finishes the boot process". The problem is that there
is no way to really determine when this happens.
Regards,
Hans
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Julian Calaby @ 2014-08-27 12:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMuHMdVkEPBtVT0dcDqQOQ_J0p+zYJwaUW8LqQX0Z+h6CHihRg@mail.gmail.com>
Hi All,
On Wed, Aug 27, 2014 at 6:37 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Maxime,
>
> On Wed, Aug 27, 2014 at 10:22 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
>> On Wed, Aug 27, 2014 at 08:40:57AM +0100, Mark Brown wrote:
>>> On Tue, Aug 26, 2014 at 08:40:09PM +0200, Henrik Nordström wrote:
>>>
>>> > It is not clear to me where the hardware resources should be listed in
>>> > DT, being it a simplefb node or part of the actual hardware device node
>>> > properly marked as dynamic boot defaults or something else? It's
>>> > somewhere inbetween hardware and virtual device, and somewhat volatile.
>>> > As far as simplefb is concerned it is a hardware desription of the
>>> > framebuffer, but for a kms driver it's no more than firmware handover of
>>> > boottime settings and ceases to exists once the kms driver have
>>> > reconfigured the hardware.
>>>
>>> Is simplefb something that should be in the device tree distinctly in
>>> the first place - shouldn't it be a subset of the functionality of the
>>> video nodes? It's the same hardware being driven differently.
>>
>> Therorically, yes, but that would mean knowing beforehand what the
>> final binding will look like, even before submitting the driver. Since
>> the bindings are always reviewed, and most of the time changed
>> slightly, that wouldn't work very well with the DT as a stable ABI
>> policy I guess.
>
> If you don't know how the bindings for a device will look like at the time of
> writing your DTS, you're always screwed, whether you add a simpefb
> node or not.
>
> If you know how the bindings look like, just add the device, with an extra
> "linux,simplefb" compatibility value.
> If you don't know how the bindings look like, do your utter best in
> guessing. Your DTS must be amended later anyway, either because
> you guessed wrong[*] (in case you added a node to have simplefb
> working), or because you have to add a real device node (in case you
> didn't add one for simplefb).
>
> [*] Actually you may have guessed right, in which case it'll continue
> working as-is, and everybody will be happy.
> Whether you want to keep backwards-compatibility in your future driver
> with the "guessed wrong" node is up to you.
I apologise if I'm stepping on anyone's toes or horribly
misrepresenting device tree's capabilities, but couldn't we start out
with something like:
disp@whatever {
compatible = "sunxi,sun4i-disp";
clocks = <&stuff 1>, <&stuff 2>, <&stuff 3>;
}
as our binding?
u-boot could then set up a framebuffer and mangle this to:
disp@whatever {
compatible = "sunxi,sun4i-disp", "linux,simplefb";
clocks = <&stuff 1>, <&stuff 2>, <&stuff 3>;
// simplefb stuff
}
Hell, if we have a reserved memory driver, couldn't we then mangle
this to something like:
disp@whatever {
compatible = "sunxi,sun4i-disp", "linux,simplefb";
clocks = <&stuff 1>, <&stuff 2>, <&stuff 3>;
// simplefb stuff
// reserved mem stuff
}
simplefb is modified to be smart enough to grab the clocks (that's a
small amount of code that almost every DT-enabled driver has, right?)
and if we're going to use the reserved memory driver to ensure nobody
messes with it's in-memory framebuffer, it'll have to deal with that
too.
Eventually sunxi-kms gets submitted, and it takes the
"sunxi,sun4i-disp" compatible, which we've already defined, however we
need some more stuff so we just add more stuff to the binding and end
up with something like:
disp@whatever {
compatible = "sunxi,sun4i-disp";
clocks = <&stuff 1>, <&stuff 2>, <&stuff 3>;
// sunxi-kms stuff
}
I believe there'd be no backwards compatibility issues as we're only
adding, not subtracting or modifying. We already know which clocks are
required for the display IP, right? And if not, we can add them. I
believe this minimises the mangling u-boot has to do (at least
initially) and is forward and backward compatible.
That'd work, right?
Thanks,
--
Julian Calaby
Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: jonsmirl @ 2014-08-27 12:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53FDB46C.5010609@redhat.com>
On Wed, Aug 27, 2014 at 6:35 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> On 08/27/2014 11:31 AM, Thierry Reding wrote:
snip
> In another mail in this thread you've mentioned 2 uses for simplefb:
>
> 1) Serving as *the* fb until a kms driver is present
> 2) Allowing for output of early kernel boot messages.
>
> I agree that those are the 2 use-cases we're dealing with here.
>
> Note that the whole discussion we're having hear about disabling the clocks
> once hardware probing is "done", is moot for 1. One possible way mentioned
> of dealing with 1. is to mark these clocks as always on for now, and then we
> need to drop this hack (I cannot call this anything else then a hack), once
> we get kms, meaning interdependencies between the dropping patch and the kms
> adding patches, etc. All not very pretty.
>
> For 2. We have what you call "mostly integration issues". Allow me to copy
> and paste in another bit of this thread here:
These are two independent problems.
> 1) Serving as *the* fb until a kms driver is present
Why can't we whip together a real device specific fbdev driver and
quit trying to run without a device specific driver in place? History
has shown that to be bad idea except during early boot when there
isn't a choice.
fbdev drivers are pretty simple things and you don't have to make one
that implements every feature. fbdev driver don't even have to
implement mode setting. They can just live with the mode initially
provided and refuse to change it. Communicating this initial mode info
on the kernel command line is already supported so we don't even need
a device tree change. This is the transition tool until KMS gets
written.
But it needs to be a device specific fbdev implementation, not
simplefb. That's because the device specific driver is going to claim
those clocks and regulators.
> 2) Allowing for output of early kernel boot messages.
For this to work the kernel needs to know two things - address of the
framebuffer and the x/y layout. Then just nuke this display when the
kernel finishes the boot process.
One of two things will happen.
1) the fbdev driver has been loaded. It has claimed the clocks, taken
care of memory, etc. If KMS is around, load it instead. When kernel
tries to nuke the display nothing will happen.
2) Your display goes blank because the kernel has disabled clocks and
reclaimed the memory. Take that as a hint to write a fbdev driver.
pages more snipped....
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Hans de Goede @ 2014-08-27 10:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140827093121.GA23186@ulmo>
Hi,
On 08/27/2014 11:31 AM, Thierry Reding wrote:
> Can you please fix you mail setup. You're addressing me directly in this
> email, yet I'm neither in To: nor Cc: headers. That's really annoying.
Sorry, I'll pay closer attention that you're in the To when I'm directly
addressing you next time.
> On Tue, Aug 26, 2014 at 09:59:00PM +0200, Hans de Goede wrote:
<snip>
>>> Either way, Mark already suggested a different alternative in another
>>> subthread, namely to add a new kind of checkpoint at which subsystems
>>> can call a "disable unused" function that's called later than a late
>>> initcall. This is not going to fix things for you immediately because
>>> the clocks will still be switched off (only later) if you don't have
>>> a real driver that's claiming the clocks. But you can work around that
>>> for now by making the relevant clocks always on and remove that
>>> workaround once a real driver is loaded that knows how to handle them
>>> properly.
>>
>> This will simply not work, and is a ton more complicated then
>> simply teaching simplefb about a clocks property, which is a really simple
>> and clean patch.
>>
>> First of all let me explain why this won't work. When should those
>> subsystems call this "later then a late initcall" call ? the kms driver
>> may very well be a kernel module, which will be loaded by userspace,
>> so we would need this to be triggered by userspace, but when ?
>>
>> When the initrd is done? What then if the kms driver is not in the initrd,
>> so maybe when udev is done enumerating devices, but what then if the kernel
>> is still enumerating hardware at this time?
>
> Usually an initrd knows how to wait for all or a given set of devices to
> be probed. udev is pretty good for that.
udev does not have a clue how to wait for all devices, it can be used to
wait for a specific device to show up, that is true, but this requires
adding specific logic for this (class of) device to the initrd init process,
which is really sub-optimal.
>> Will we just put a sleep 30
>> in our initscripts to make sure the kernel is done scanning all busses,
>> will that be long enough? Maybe we need to re-introduce the scsi_wait_done
>> kernel module which was added as an ugly hack to allow userspace to wait
>> for the kernel to have finished scanning scsi (and ata / sata) busses,
>> for controllers found at the time the module was load. Which never worked
>> reliable, because it would be possible that the controller itself still
>> needed to be discovered. We've spend years cleaning up userspace enough
>> to be able to kill scsi_wait_done. We've already been down this road of
>> waiting for hw enumeration to be done, and the problem is that on
>> modern systems *it is never done*.
>
> Those are mostly integration issues. If you don't load the driver from
> initrd then I don't think anybody will complain when there's flicker
> when it finally gets loaded. The whole point of this is to have early
> access to the framebuffer and get display and graphics going as soon as
> possible, so please don't pull in hypothetical scenarios where people
> manually load drivers half an hour after the system has booted.
In another mail in this thread you've mentioned 2 uses for simplefb:
1) Serving as *the* fb until a kms driver is present
2) Allowing for output of early kernel boot messages.
I agree that those are the 2 use-cases we're dealing with here.
Note that the whole discussion we're having hear about disabling the clocks
once hardware probing is "done", is moot for 1. One possible way mentioned
of dealing with 1. is to mark these clocks as always on for now, and then we
need to drop this hack (I cannot call this anything else then a hack), once
we get kms, meaning interdependencies between the dropping patch and the kms
adding patches, etc. All not very pretty.
For 2. We have what you call "mostly integration issues". Allow me to copy
and paste in another bit of this thread here:
>> I've been doing low level Linux development for 10 years now and I've
>> worked a lot on storage (raid, iscsi, fcoe support, etc.) in the past.
>
> Ah, so this is now officially a pissing contest, is it?
No I'm merely trying to explain that I've a ton of experience with what
you seem to think of as merely integration issues, and they are a royal PITA,
and thus to be avoided at all costs. Really, please just trust me on this
one, doing the whole add some later then late init call type is a bad idea.
>> So second of all, Thierry, what exactly is the technical argument against
>> adding support for dealing with clocks to simplefb ?
>
> I've already stated technical arguments against it. You could just go
> back and read the thread again, or would you rather want me to repeat
> them for your convenience?
It is a long thread, IIRC your main arguments against this are:
1) You don't want this kind of platform-specific knowledge in simplefb
2) That simplefb is supposed to be simple, and this is not simple
As for 1. several people have already argued that clocks as such are
an abstract concept, found one many platforms and as such are not
platform specific.
Any generic hw driver dealing with embedded platforms, be it ahci, ohci,
ehci, etc. has ended up adding abstract support for things like clocks
(and other resources). I say abstract support here, since to all these
drivers the support was added in a way that no platform specific knowledge
is necessary, they just deal with clocks not caring about the specific
underlying clock implementation, clock names, etc.
This really is unavoidable. The framebuffer memory needed was mentioned
as another example of a resource in this thread. You suggested using the
new mem-reserve framework for this. I agree that that is the right thing
to do, but even then we still need a way to tie this reserved mem to
the simplefb, so that the kernel can know when it is no longer used
and can be added to the general memory pool.
If you look at how almost all dt bindings work, then the dt node for
a device specifies the resources needed, I don't see why simplefb would
be so special that it should be different here. I agree that it is
important to get the abstractions right here, but to me it seems that
the right abstraction is to be consistent with how all other devices
are abstracted and to add needed resources to the dt node for the
simplefb.
As for 2. Yes this will add some more code to simplefb, more code but
code which is not complicated in anyway, so the simplefb code would
become larger but still stay simple. Were as all the suggested
alternatives sofar are at least an order of magnitude more complicated,
crossing many subsystems, demanding coordination with userspace, etc.
>> I've heard a lot of people in favor of it,
>
> Of course you have, all the people in favour of it are sunxi people that
> really want to see this merged because it gives them working display
> after U-Boot.
I don't think that that is entirely fair towards Maxime, yes Maxime is
a sunxi person, but he usually pushed back hard against anything he
considers not clean enough, even if it would enable highly desirable
functionality.
>
>> and only pretty much you against it,
>
> At least Stephen also spoke out about it. But this quickly devolved into
> the kind of thread you want to get out of as quickly as possible, so I
> can't blame him for not continuing this discussion. In fact I'm very
> much regretting getting into this in the first place. You clearly have
> no intention to even consider any possible other solution that what was
> posted, so it's pretty useless to try and convince you.
>
>> and your argument so far seems to boil down to "I don't like it",
>
> I'm not surprised that you think so since you seem to be very selective
> in what you read (or register) and what you don't.
See above, I've summarized the 2 arguments which you've made as
"I don't like it" before since to me they seem to be subjective arguments,
you say it is too platform specific, but where is the divide line for
something being _too_ platform specific ? I realize in the end everything
is pretty much subjective. I apologize for the "I don't like it"
characterization that was not fair of me.
>
>> is not really a technical sound argument IMHO. Moreover all the
>> alternatives seem to be horribly complicated and most of them also
>> seem to have several issues which will make them simply not work
>> reliable.
>
> I'm not an unreasonable person, or at least I like to think so, and if
> there really isn't a better solution then I won't object to this patch
> any longer.
That is good to hear.
> But as it stands I am not convinced that this is the best solution
I'm very much open to a better solution, but so far everything suggested
is so much more complicated, that I don;t consider any of the suggestions
done so far better.
> so I think we should keep the discussion going for a bit and
> see if we really can't come up with something that will fix this for a
> more general case rather than just your particular use-case. It's
> evidently a recurring problem that others suffer from, too.
Yes needing resources despite the device being behind some generic
interface where the main driver code for that interface has no knowledge
about such resources sofar, because e.g. on PC-s this was not necessary
is a recurring problem, which has been solved already 3 times at least,
see the ohci-, ehci- and ahci-platform drivers, and for all 3 the conclusion
was that the best solution was to simple add the resources to the dt-node,
and add code to claim and enable them.
Regards,
Hans
>
> Thierry
>
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-08-27 10:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMuHMdURvJaM7qge+Po5e=F45k9-NeHsTiQykCjCmwr6_ud08A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2237 bytes --]
On Wed, Aug 27, 2014 at 11:05:29AM +0200, Geert Uytterhoeven wrote:
> On Wed, Aug 27, 2014 at 10:55 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> >> >> Is simplefb something that should be in the device tree distinctly in
> >> >> the first place - shouldn't it be a subset of the functionality of the
> >> >> video nodes? It's the same hardware being driven differently.
> >> >
> >> > Therorically, yes, but that would mean knowing beforehand what the
> >> > final binding will look like, even before submitting the driver. Since
> >> > the bindings are always reviewed, and most of the time changed
> >> > slightly, that wouldn't work very well with the DT as a stable ABI
> >> > policy I guess.
> >>
> >> If you don't know how the bindings for a device will look like at the time of
> >> writing your DTS, you're always screwed, whether you add a simpefb
> >> node or not.
> >>
> >> If you know how the bindings look like, just add the device, with an extra
> >> "linux,simplefb" compatibility value.
> >> If you don't know how the bindings look like, do your utter best in
> >> guessing. Your DTS must be amended later anyway, either because
> >> you guessed wrong[*] (in case you added a node to have simplefb
> >> working), or because you have to add a real device node (in case you
> >> didn't add one for simplefb).
> >
> > Let's be conservative and consider the case where we would guess
> > wrong.
> >
> > If we just rely on a simplefb node, when reviewing and integrating the
> > "new" bindings to describe accureately the various IPs involved in the
> > display path, we would obviously create new compatibles for
> > them. Since it's new compatibles, we can come up with any binding we'd
> > like, without have to consider the backward compatibility, since it's
> > a new binding.
> >
> > Then, we just remove the simplefb, all is good.
>
> I would keep the simplefb compatible value. Else you break compatibility
> with old kernels that don't have your new driver.
Yes, true. Since the simplefb will be injected by u-boot, it will be
there anyway.
--
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: Henrik Nordström @ 2014-08-27 10:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140827085538.GS15297@lukather>
ons 2014-08-27 klockan 10:55 +0200 skrev Maxime Ripard:
> If we just rely on a simplefb node, when reviewing and integrating the
> "new" bindings to describe accureately the various IPs involved in the
> display path, we would obviously create new compatibles for
> them. Since it's new compatibles, we can come up with any binding we'd
> like, without have to consider the backward compatibility, since it's
> a new binding.
My gut feeling is that the attributes needed by simplefb belong in the
actual hardware node somehow, with a standard layout so it can be
applied on all compatible video devices.
The complication is that these attributes is volatile and not really
set/defined by hardware. Similar to UART baud rate (and kernel command
line if you like).
Regards
Henrik
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-27 10:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140827085538.GS15297@lukather>
[-- Attachment #1: Type: text/plain, Size: 3458 bytes --]
On Wed, Aug 27, 2014 at 10:55:38AM +0200, Maxime Ripard wrote:
> Hi!
>
> On Wed, Aug 27, 2014 at 10:37:36AM +0200, Geert Uytterhoeven wrote:
> > Hi Maxime,
> >
> > On Wed, Aug 27, 2014 at 10:22 AM, Maxime Ripard
> > <maxime.ripard@free-electrons.com> wrote:
> > > On Wed, Aug 27, 2014 at 08:40:57AM +0100, Mark Brown wrote:
> > >> On Tue, Aug 26, 2014 at 08:40:09PM +0200, Henrik Nordström wrote:
> > >>
> > >> > It is not clear to me where the hardware resources should be listed in
> > >> > DT, being it a simplefb node or part of the actual hardware device node
> > >> > properly marked as dynamic boot defaults or something else? It's
> > >> > somewhere inbetween hardware and virtual device, and somewhat volatile.
> > >> > As far as simplefb is concerned it is a hardware desription of the
> > >> > framebuffer, but for a kms driver it's no more than firmware handover of
> > >> > boottime settings and ceases to exists once the kms driver have
> > >> > reconfigured the hardware.
> > >>
> > >> Is simplefb something that should be in the device tree distinctly in
> > >> the first place - shouldn't it be a subset of the functionality of the
> > >> video nodes? It's the same hardware being driven differently.
> > >
> > > Therorically, yes, but that would mean knowing beforehand what the
> > > final binding will look like, even before submitting the driver. Since
> > > the bindings are always reviewed, and most of the time changed
> > > slightly, that wouldn't work very well with the DT as a stable ABI
> > > policy I guess.
> >
> > If you don't know how the bindings for a device will look like at the time of
> > writing your DTS, you're always screwed, whether you add a simpefb
> > node or not.
> >
> > If you know how the bindings look like, just add the device, with an extra
> > "linux,simplefb" compatibility value.
> > If you don't know how the bindings look like, do your utter best in
> > guessing. Your DTS must be amended later anyway, either because
> > you guessed wrong[*] (in case you added a node to have simplefb
> > working), or because you have to add a real device node (in case you
> > didn't add one for simplefb).
>
> Let's be conservative and consider the case where we would guess
> wrong.
>
> If we just rely on a simplefb node, when reviewing and integrating the
> "new" bindings to describe accureately the various IPs involved in the
> display path, we would obviously create new compatibles for
> them. Since it's new compatibles, we can come up with any binding we'd
> like, without have to consider the backward compatibility, since it's
> a new binding.
>
> Then, we just remove the simplefb, all is good.
>
> If we were to try to create our bindings for all the IPs involved, and
> were not pleased with the binding anymore when merging the driver,
> then we would have to break the bindings, since we don't introduce a
> new compatible anymore, but modifying an existing one.
It's usually a bad idea to merge a binding without an appropriate
implementation thereof in a driver. It's been done in the past and very
often has resulted in bindings that turned out unusable.
You could start out small and only describe the various individual IP
blocks that exist in the hardware with reg, interrupts, clock, etc.
properties, most of which should be known up front. Then you could try
to find out where simplefb fits best.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH V2] video : remove redundant error check
From: DaeSeok Youn @ 2014-08-27 10:00 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Jean-Christophe PLAGNIOL-VILLARD, Antonino Daplas, jg1.han,
laurent.pinchart, Rob Clark, daniel.vetter, Julia Lawall,
linux-fbdev, linux-kernel
In-Reply-To: <53FC629A.2050101@ti.com>
Hi,
2014-08-26 19:34 GMT+09:00 Tomi Valkeinen <tomi.valkeinen@ti.com>:
> On 16/05/14 12:31, Daeseok Youn wrote:
>> It doesn't need to check "err" for printing info.
>> And also use pr_info instead of printk.
>>
>> Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
>> ---
>> V2: removes unneeded lines for sending a patch
>>
>> drivers/video/fbdev/i810/i810_main.c | 7 +++----
>> 1 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/i810/i810_main.c b/drivers/video/fbdev/i810/i810_main.c
>> index bb674e4..15cb397 100644
>> --- a/drivers/video/fbdev/i810/i810_main.c
>> +++ b/drivers/video/fbdev/i810/i810_main.c
>> @@ -1910,13 +1910,12 @@ static void i810fb_find_init_mode(struct fb_info *info)
>>
>> for (i = 0; i < par->ddc_num + 1; i++) {
>> err = i810_probe_i2c_connector(info, &par->edid, i);
>> - if (!err)
>> + if (!err) {
>> + pr_info("i810fb_init_pci: DDC probe successful\n");
>> break;
>> + }
>> }
>>
>> - if (!err)
>> - printk("i810fb_init_pci: DDC probe successful\n");
>> -
>> fb_edid_to_monspecs(par->edid, specs);
>>
>> if (specs->modedb = NULL)
>>
>
> I don't know... I think I personally like more the original version. In
> fact, the whole print looks quite useless to me, or at least it should
> be a debug print.
Yes. this patch doesn't need.
Thanks for review.
regards,
Daeseok Youn.
>
> Tomi
>
>
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-27 9:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140827084526.GR15297@lukather>
[-- Attachment #1: Type: text/plain, Size: 2721 bytes --]
On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote:
> On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote:
> > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote:
> > > On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote:
[...]
> > > > > Mike Turquette repeatedly said that he was against such a DT property:
> > > > > https://lkml.org/lkml/2014/5/12/693
> > > >
> > > > Mike says in that email that he's opposing the addition of a property
> > > > for clocks that is the equivalent of regulator-always-on. That's not
> > > > what this is about. If at all it'd be a property to mark a clock that
> > > > should not be disabled by default because it's essential.
> > >
> > > It's just semantic. How is "a clock that should not be disabled by
> > > default because it's essential" not a clock that stays always on?
> >
> > Because a clock that should not be disabled by default can be turned off
> > when appropriate. A clock that is always on can't be turned off.
>
> If a clock is essential, then it should never be disabled. Or we don't
> share the same meaning of essential.
Essential for the particular use-case.
> > > > But you can work around that for now by making the relevant clocks
> > > > always on and remove that workaround once a real driver is loaded
> > > > that knows how to handle them properly.
> > >
> > > So, let me get this straight. The clock provider driver should behave
> > > as a clock consumer because it knows that in some cases, it might not
> > > have any willingful enough consumer? Doesn't that even ring your
> > > this-is-a-clear-abstraction-violation-bell just a tiny bit?
> >
> > No. The clock driver should simply not allow the clocks to be disabled
> > if it isn't safe to do so.
>
> Again, we do seem to differ on our interpretation of an essential
> clock. To me, it is perfectly safe to disable the clocks. The system
> will still be responding, the memory will be there, the CPU will keep
> running, and we do have a way to recover from that disabled to clock
> (for example to enable it back).
>
> Plus, again, in Mike's mail, it's clearly said that adding hacks like
> this to the clock driver should only be considered in the case where
> we don't have a consuming driver, which is not our case here.
Well, that depends on what you mean by the consuming driver. simplefb
isn't a traditional driver in that sense. There will eventually, in a
more fully-featured system, be a driver that properly consumes the
clocks. Now, since this is only a temporary measure I think it's one of
the cases where having this encoded in the clock driver would be
appropriate.
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-08-27 9:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53FD9017.40705@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 5411 bytes --]
On Wed, Aug 27, 2014 at 10:00:23AM +0200, Hans de Goede wrote:
> Hi,
>
> On 08/27/2014 08:54 AM, Thierry Reding wrote:
> > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote:
> >> On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote:
> >>>>>> Any decent enough SoC, with a decent support in the kernel will have
> >>>>>> clocks for this, and I really wonder how simplefb will behave once its
> >>>>>> clocks will be turned off...
> >>>>>
> >>>>> There are other devices besides ARM SoCs that may want to use this
> >>>>> driver and that don't have clock support.
> >>>>
> >>>> And in this case, with this patch, simplefb will not claim any clock,
> >>>> nor will fail probing.
> >>>>
> >>>>> But you're missing my point. What I'm saying is that the simplefb driver
> >>>>> is meant to serve as a way to take over whatever framebuffer a firmware
> >>>>> set up. Therefore I think it makes the most sense to assume that nothing
> >>>>> needs to be controlled in any way since already been set up by firmware.
> >>>>> Eventually there should be a driver that takes over from simplefb that
> >>>>> knows how to properly handle the device's specifics, but that's not
> >>>>> simplefb.
> >>>>
> >>>> I guess such a hand over if it were to happen in the kernel would
> >>>> involve the second driver claiming the resources before the first one
> >>>> release them. How is that different in this case?
> >>>
> >>> It's different in that that driver will be hardware specific and know
> >>> exactly what clock and other resources are required. It will have a
> >>> device-specific binding.
> >>
> >> Except that you made simplefb a generic driver. So we have two choices
> >> here: either we don't anything but the trivial case, and no one with a
> >> rather more advanced hardware will be able to use it, or we try to
> >> grab any resource that might be of use, which means clocks,
> >> regulators, reserved memory region, or whatever, so that we pretty
> >> much cover all cases. It really is as simple as that.
> >
> > No, it should be even simpler. simplefb shouldn't have to know any of
> > this. It's just taking what firmware has set up and uses that. It's a
> > stop-gap solution to provide information on the display until a real
> > driver can be loaded and initializes the display hardware properly.
> >
> >>>>> The goal of this patch series is to keep clocks from being turned off.
> >>>>> But that's not what it does. What it does is turn clocks on to prevent
> >>>>> them from being turned off. In my opinion that's a workaround for a
> >>>>> deficiency in the kernel (and the firmware/kernel interface) and I think
> >>>>> it should be fixed at the root. So a much better solution would be to
> >>>>> establish a way for firmware to communicate to the kernel that a given
> >>>>> resource has been enabled by firmware and shouldn't be disabled. Such a
> >>>>> solution can be implement for all types of resources and can be reused
> >>>>> by all drivers since they don't have to worry about these details.
> >>>>
> >>>> Mike Turquette repeatedly said that he was against such a DT property:
> >>>> https://lkml.org/lkml/2014/5/12/693
> >>>
> >>> Mike says in that email that he's opposing the addition of a property
> >>> for clocks that is the equivalent of regulator-always-on. That's not
> >>> what this is about. If at all it'd be a property to mark a clock that
> >>> should not be disabled by default because it's essential.
> >>
> >> It's just semantic. How is "a clock that should not be disabled by
> >> default because it's essential" not a clock that stays always on?
> >
> > Because a clock that should not be disabled by default can be turned off
> > when appropriate. A clock that is always on can't be turned off.
> >
> >> Plus, you should read the mail further. It's clearly said that
> >> consumer drivers should call clk_prepare_enable themselves, and that
> >> the only exception to that is the case where we don't have such a
> >> driver (and only valid as a temporary exception, which I guess you'll
> >> soon turn into temporary-until-a-drm-kms-driver-is-merged).
> >
> > Exactly. simplefb is only a temporary measure. It's not meant to be used
> > as a full-blown framebuffer driver. There are two use-cases where it is
> > an appropriate solution:
> >
> > 1) As a stop-gap solution for when the platform doesn't support a full
> > display driver yet. In that case you will want simplefb to stay
> > around forever.
> >
> > 2) To get early boot output before a full display driver can be loaded
> > in which case simplefb should go away after the display driver has
> > taken over.
> >
> > In case of 2) the most straightforward solution is to not disable any
> > clocks until all drivers have had a chance to claim them. When the full
> > driver has been probed it should have claimed the clocks so they would
> > no longer get disabled.
>
> Please read my long reply from yesterday evening why this will never
> work, as there is not a well defined moment when "all drivers have had a
> chance to claim them"
>
> I've been doing low level Linux development for 10 years now and I've
> worked a lot on storage (raid, iscsi, fcoe support, etc.) in the past.
Ah, so this is now officially a pissing contest, is it?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCHv2 27/27] OMAPDSS: connector-analog-tv: Add DT support
From: Tomi Valkeinen @ 2014-08-27 9:37 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-omap, linux-fbdev, devicetree
In-Reply-To: <21704808.AJTjtXoZly@avalon>
[-- Attachment #1: Type: text/plain, Size: 3891 bytes --]
On 26/08/14 19:58, Laurent Pinchart wrote:
> Hi Tomi,
>
> On Monday 16 December 2013 16:56:34 Tomi Valkeinen wrote:
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>> .../video/omap2/displays-new/connector-analog-tv.c | 66 ++++++++++++++++++-
>> 1 file changed, 65 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/omap2/displays-new/connector-analog-tv.c
>> b/drivers/video/omap2/displays-new/connector-analog-tv.c index
>> ccd9073f706f..ebed25a86487 100644
>> --- a/drivers/video/omap2/displays-new/connector-analog-tv.c
>> +++ b/drivers/video/omap2/displays-new/connector-analog-tv.c
>> @@ -12,6 +12,7 @@
>> #include <linux/slab.h>
>> #include <linux/module.h>
>> #include <linux/platform_device.h>
>> +#include <linux/of.h>
>>
>> #include <video/omapdss.h>
>> #include <video/omap-panel-data.h>
>> @@ -42,6 +43,12 @@ static const struct omap_video_timings tvc_pal_timings =
>> { .interlace = true,
>> };
>>
>> +static const struct of_device_id tvc_of_match[];
>> +
>> +struct tvc_of_data {
>> + enum omap_dss_venc_type connector_type;
>> +};
>> +
>> #define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
>>
>> static int tvc_connect(struct omap_dss_device *dssdev)
>> @@ -92,7 +99,10 @@ static int tvc_enable(struct omap_dss_device *dssdev)
>> in->ops.atv->set_timings(in, &ddata->timings);
>>
>> in->ops.atv->set_type(in, ddata->connector_type);
>> - in->ops.atv->invert_vid_out_polarity(in, ddata->invert_polarity);
>> +
>> + if (!ddata->dev->of_node)
>> + in->ops.atv->invert_vid_out_polarity(in,
>> + ddata->invert_polarity);
>>
>> r = in->ops.atv->enable(in);
>> if (r)
>> @@ -205,6 +215,35 @@ static int tvc_probe_pdata(struct platform_device
>> *pdev) return 0;
>> }
>>
>> +static int tvc_probe_of(struct platform_device *pdev)
>> +{
>> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> + struct device_node *node = pdev->dev.of_node;
>> + struct omap_dss_device *in;
>> + const struct of_device_id *match;
>> + const struct tvc_of_data *data;
>> +
>> + match = of_match_node(tvc_of_match, pdev->dev.of_node);
>> + if (!match) {
>> + dev_err(&pdev->dev, "unsupported device\n");
>> + return -ENODEV;
>> + }
>> +
>> + data = match->data;
>> +
>> + in = omapdss_of_find_source_for_first_ep(node);
>> + if (IS_ERR(in)) {
>> + dev_err(&pdev->dev, "failed to find video source\n");
>> + return PTR_ERR(in);
>> + }
>> +
>> + ddata->in = in;
>> +
>> + ddata->connector_type = data->connector_type;
>> +
>> + return 0;
>> +}
>> +
>> static int tvc_probe(struct platform_device *pdev)
>> {
>> struct panel_drv_data *ddata;
>> @@ -222,6 +261,10 @@ static int tvc_probe(struct platform_device *pdev)
>> r = tvc_probe_pdata(pdev);
>> if (r)
>> return r;
>> + } else if (pdev->dev.of_node) {
>> + r = tvc_probe_of(pdev);
>> + if (r)
>> + return r;
>> } else {
>> return -ENODEV;
>> }
>> @@ -263,12 +306,33 @@ static int __exit tvc_remove(struct platform_device
>> *pdev) return 0;
>> }
>>
>> +static const struct tvc_of_data tv_svideo_data = {
>> + .connector_type = OMAP_DSS_VENC_TYPE_SVIDEO,
>> +};
>> +
>> +static const struct tvc_of_data tv_composite_video_data = {
>> + .connector_type = OMAP_DSS_VENC_TYPE_COMPOSITE,
>> +};
>> +
>> +static const struct of_device_id tvc_of_match[] = {
>> + {
>> + .compatible = "svideo-connector",
>> + .data = &tv_svideo_data,
>> + },
>> + {
>> + .compatible = "composite-video-connector",
>
> I've just noticed that this doesn't match the bindings that document the
> compatible value to be "composite-connector".
Thanks. arch/arm/boot/dts/omap3-n900.dts uses the same value as in the
bindings document, so I think the proper fix is to change the compatible
value for this driver. I'll make a patch.
Tomi
[-- Attachment #2: OpenPGP 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-08-27 9:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53FCE704.4030103@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 8086 bytes --]
Can you please fix you mail setup. You're addressing me directly in this
email, yet I'm neither in To: nor Cc: headers. That's really annoying.
On Tue, Aug 26, 2014 at 09:59:00PM +0200, Hans de Goede wrote:
> Hi,
>
> On 08/26/2014 04:35 PM, Thierry Reding wrote:
> >On Tue, Aug 26, 2014 at 03:53:41PM +0200, Maxime Ripard wrote:
> >>On Tue, Aug 26, 2014 at 10:04:33AM +0200, Thierry Reding wrote:
> >>>>>No. simplefb just wants to write to some memory that hardware has been
> >>>>>set up to scan out. The platform requires that the clocks be on. Other
> >>>>>platforms may not even allow turning off the clocks.
> >>>>
> >>>>Like what? the rpi? Come on. Just because the videocore is some black
> >>>>box we know nothing about doesn't mean we should use it as an example.
> >>>
> >>>You make it sound like the Raspberry Pi is somehow less important than
> >>>sunxi.
> >>
> >>No. What I mean is that it seems like we are somehow punished, or at
> >>least blamed, for having a better and more complete kernel support.
> >
> >This isn't a competition. Nobody's punishing or blaming anyone. This is
> >about finding the best solution for the problem at hand.
> >
> >>>>Any decent enough SoC, with a decent support in the kernel will have
> >>>>clocks for this, and I really wonder how simplefb will behave once its
> >>>>clocks will be turned off...
> >>>
> >>>There are other devices besides ARM SoCs that may want to use this
> >>>driver and that don't have clock support.
> >>
> >>And in this case, with this patch, simplefb will not claim any clock,
> >>nor will fail probing.
> >>
> >>>But you're missing my point. What I'm saying is that the simplefb driver
> >>>is meant to serve as a way to take over whatever framebuffer a firmware
> >>>set up. Therefore I think it makes the most sense to assume that nothing
> >>>needs to be controlled in any way since already been set up by firmware.
> >>>Eventually there should be a driver that takes over from simplefb that
> >>>knows how to properly handle the device's specifics, but that's not
> >>>simplefb.
> >>
> >>I guess such a hand over if it were to happen in the kernel would
> >>involve the second driver claiming the resources before the first one
> >>release them. How is that different in this case?
> >
> >It's different in that that driver will be hardware specific and know
> >exactly what clock and other resources are required. It will have a
> >device-specific binding.
> >
> >>>The goal of this patch series is to keep clocks from being turned off.
> >>>But that's not what it does. What it does is turn clocks on to prevent
> >>>them from being turned off. In my opinion that's a workaround for a
> >>>deficiency in the kernel (and the firmware/kernel interface) and I think
> >>>it should be fixed at the root. So a much better solution would be to
> >>>establish a way for firmware to communicate to the kernel that a given
> >>>resource has been enabled by firmware and shouldn't be disabled. Such a
> >>>solution can be implement for all types of resources and can be reused
> >>>by all drivers since they don't have to worry about these details.
> >>
> >>Mike Turquette repeatedly said that he was against such a DT property:
> >>https://lkml.org/lkml/2014/5/12/693
> >
> >Mike says in that email that he's opposing the addition of a property
> >for clocks that is the equivalent of regulator-always-on. That's not
> >what this is about. If at all it'd be a property to mark a clock that
> >should not be disabled by default because it's essential.
> >
> >Adding Mike on this subthread too.
> >
> >Either way, Mark already suggested a different alternative in another
> >subthread, namely to add a new kind of checkpoint at which subsystems
> >can call a "disable unused" function that's called later than a late
> >initcall. This is not going to fix things for you immediately because
> >the clocks will still be switched off (only later) if you don't have
> >a real driver that's claiming the clocks. But you can work around that
> >for now by making the relevant clocks always on and remove that
> >workaround once a real driver is loaded that knows how to handle them
> >properly.
>
> This will simply not work, and is a ton more complicated then
> simply teaching simplefb about a clocks property, which is a really simple
> and clean patch.
>
> First of all let me explain why this won't work. When should those
> subsystems call this "later then a late initcall" call ? the kms driver
> may very well be a kernel module, which will be loaded by userspace,
> so we would need this to be triggered by userspace, but when ?
>
> When the initrd is done? What then if the kms driver is not in the initrd,
> so maybe when udev is done enumerating devices, but what then if the kernel
> is still enumerating hardware at this time?
Usually an initrd knows how to wait for all or a given set of devices to
be probed. udev is pretty good for that.
> Will we just put a sleep 30
> in our initscripts to make sure the kernel is done scanning all busses,
> will that be long enough? Maybe we need to re-introduce the scsi_wait_done
> kernel module which was added as an ugly hack to allow userspace to wait
> for the kernel to have finished scanning scsi (and ata / sata) busses,
> for controllers found at the time the module was load. Which never worked
> reliable, because it would be possible that the controller itself still
> needed to be discovered. We've spend years cleaning up userspace enough
> to be able to kill scsi_wait_done. We've already been down this road of
> waiting for hw enumeration to be done, and the problem is that on
> modern systems *it is never done*.
Those are mostly integration issues. If you don't load the driver from
initrd then I don't think anybody will complain when there's flicker
when it finally gets loaded. The whole point of this is to have early
access to the framebuffer and get display and graphics going as soon as
possible, so please don't pull in hypothetical scenarios where people
manually load drivers half an hour after the system has booted.
> So second of all, Thierry, what exactly is the technical argument against
> adding support for dealing with clocks to simplefb ?
I've already stated technical arguments against it. You could just go
back and read the thread again, or would you rather want me to repeat
them for your convenience?
> I've heard a lot of people in favor of it,
Of course you have, all the people in favour of it are sunxi people that
really want to see this merged because it gives them working display
after U-Boot.
> and only pretty much you against it,
At least Stephen also spoke out about it. But this quickly devolved into
the kind of thread you want to get out of as quickly as possible, so I
can't blame him for not continuing this discussion. In fact I'm very
much regretting getting into this in the first place. You clearly have
no intention to even consider any possible other solution that what was
posted, so it's pretty useless to try and convince you.
> and your argument so far seems to boil down to "I don't like it",
I'm not surprised that you think so since you seem to be very selective
in what you read (or register) and what you don't.
> is not really a technical sound argument IMHO. Moreover all the
> alternatives seem to be horribly complicated and most of them also
> seem to have several issues which will make them simply not work
> reliable.
I'm not an unreasonable person, or at least I like to think so, and if
there really isn't a better solution then I won't object to this patch
any longer. But as it stands I am not convinced that this is the best
solution, so I think we should keep the discussion going for a bit and
see if we really can't come up with something that will fix this for a
more general case rather than just your particular use-case. It's
evidently a recurring problem that others suffer from, too.
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: Geert Uytterhoeven @ 2014-08-27 9:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140827085538.GS15297@lukather>
On Wed, Aug 27, 2014 at 10:55 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
>> >> Is simplefb something that should be in the device tree distinctly in
>> >> the first place - shouldn't it be a subset of the functionality of the
>> >> video nodes? It's the same hardware being driven differently.
>> >
>> > Therorically, yes, but that would mean knowing beforehand what the
>> > final binding will look like, even before submitting the driver. Since
>> > the bindings are always reviewed, and most of the time changed
>> > slightly, that wouldn't work very well with the DT as a stable ABI
>> > policy I guess.
>>
>> If you don't know how the bindings for a device will look like at the time of
>> writing your DTS, you're always screwed, whether you add a simpefb
>> node or not.
>>
>> If you know how the bindings look like, just add the device, with an extra
>> "linux,simplefb" compatibility value.
>> If you don't know how the bindings look like, do your utter best in
>> guessing. Your DTS must be amended later anyway, either because
>> you guessed wrong[*] (in case you added a node to have simplefb
>> working), or because you have to add a real device node (in case you
>> didn't add one for simplefb).
>
> Let's be conservative and consider the case where we would guess
> wrong.
>
> If we just rely on a simplefb node, when reviewing and integrating the
> "new" bindings to describe accureately the various IPs involved in the
> display path, we would obviously create new compatibles for
> them. Since it's new compatibles, we can come up with any binding we'd
> like, without have to consider the backward compatibility, since it's
> a new binding.
>
> Then, we just remove the simplefb, all is good.
I would keep the simplefb compatible value. Else you break compatibility
with old kernels that don't have your new driver.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-08-27 8:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMuHMdVkEPBtVT0dcDqQOQ_J0p+zYJwaUW8LqQX0Z+h6CHihRg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2938 bytes --]
Hi!
On Wed, Aug 27, 2014 at 10:37:36AM +0200, Geert Uytterhoeven wrote:
> Hi Maxime,
>
> On Wed, Aug 27, 2014 at 10:22 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Wed, Aug 27, 2014 at 08:40:57AM +0100, Mark Brown wrote:
> >> On Tue, Aug 26, 2014 at 08:40:09PM +0200, Henrik Nordström wrote:
> >>
> >> > It is not clear to me where the hardware resources should be listed in
> >> > DT, being it a simplefb node or part of the actual hardware device node
> >> > properly marked as dynamic boot defaults or something else? It's
> >> > somewhere inbetween hardware and virtual device, and somewhat volatile.
> >> > As far as simplefb is concerned it is a hardware desription of the
> >> > framebuffer, but for a kms driver it's no more than firmware handover of
> >> > boottime settings and ceases to exists once the kms driver have
> >> > reconfigured the hardware.
> >>
> >> Is simplefb something that should be in the device tree distinctly in
> >> the first place - shouldn't it be a subset of the functionality of the
> >> video nodes? It's the same hardware being driven differently.
> >
> > Therorically, yes, but that would mean knowing beforehand what the
> > final binding will look like, even before submitting the driver. Since
> > the bindings are always reviewed, and most of the time changed
> > slightly, that wouldn't work very well with the DT as a stable ABI
> > policy I guess.
>
> If you don't know how the bindings for a device will look like at the time of
> writing your DTS, you're always screwed, whether you add a simpefb
> node or not.
>
> If you know how the bindings look like, just add the device, with an extra
> "linux,simplefb" compatibility value.
> If you don't know how the bindings look like, do your utter best in
> guessing. Your DTS must be amended later anyway, either because
> you guessed wrong[*] (in case you added a node to have simplefb
> working), or because you have to add a real device node (in case you
> didn't add one for simplefb).
Let's be conservative and consider the case where we would guess
wrong.
If we just rely on a simplefb node, when reviewing and integrating the
"new" bindings to describe accureately the various IPs involved in the
display path, we would obviously create new compatibles for
them. Since it's new compatibles, we can come up with any binding we'd
like, without have to consider the backward compatibility, since it's
a new binding.
Then, we just remove the simplefb, all is good.
If we were to try to create our bindings for all the IPs involved, and
were not pleased with the binding anymore when merging the driver,
then we would have to break the bindings, since we don't introduce a
new compatible anymore, but modifying an existing one.
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: Maxime Ripard @ 2014-08-27 8:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140827065440.GG15640@ulmo>
[-- Attachment #1: Type: text/plain, Size: 9067 bytes --]
On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote:
> On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote:
> > On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote:
> > > > > > Any decent enough SoC, with a decent support in the kernel will have
> > > > > > clocks for this, and I really wonder how simplefb will behave once its
> > > > > > clocks will be turned off...
> > > > >
> > > > > There are other devices besides ARM SoCs that may want to use this
> > > > > driver and that don't have clock support.
> > > >
> > > > And in this case, with this patch, simplefb will not claim any clock,
> > > > nor will fail probing.
> > > >
> > > > > But you're missing my point. What I'm saying is that the simplefb driver
> > > > > is meant to serve as a way to take over whatever framebuffer a firmware
> > > > > set up. Therefore I think it makes the most sense to assume that nothing
> > > > > needs to be controlled in any way since already been set up by firmware.
> > > > > Eventually there should be a driver that takes over from simplefb that
> > > > > knows how to properly handle the device's specifics, but that's not
> > > > > simplefb.
> > > >
> > > > I guess such a hand over if it were to happen in the kernel would
> > > > involve the second driver claiming the resources before the first one
> > > > release them. How is that different in this case?
> > >
> > > It's different in that that driver will be hardware specific and know
> > > exactly what clock and other resources are required. It will have a
> > > device-specific binding.
> >
> > Except that you made simplefb a generic driver. So we have two choices
> > here: either we don't anything but the trivial case, and no one with a
> > rather more advanced hardware will be able to use it, or we try to
> > grab any resource that might be of use, which means clocks,
> > regulators, reserved memory region, or whatever, so that we pretty
> > much cover all cases. It really is as simple as that.
>
> No, it should be even simpler. simplefb shouldn't have to know any of
> this. It's just taking what firmware has set up and uses that. It's a
> stop-gap solution to provide information on the display until a real
> driver can be loaded and initializes the display hardware properly.
That was not the original intention of this driver then. Stephen's
commit log (commit 26549c8d36a6) was even mentionning some use cases:
"
Examples use-cases include:
* The built-in LCD panels on the Samsung ARM chromebook, and Tegra
devices, and likely many other ARM or embedded systems. These cannot
yet be supported using a full graphics driver, since the panel control
should be provided by the CDF (Common Display Framework), which has been
stuck in design/review for quite some time. One could support these
panels using custom SoC-specific code, but there is a desire to use
common infra-structure rather than having each SoC vendor invent their
own code, hence the desire to wait for CDF.
* Hardware for which a full graphics driver is not yet available, and
the path to obtain one upstream isn't yet clear. For example, the
Raspberry Pi.
* Any hardware in early stages of upstreaming, before a full graphics
driver has been tackled. This driver can provide a graphical boot
console (even full X support) much earlier in the upstreaming process,
thus making new SoC or board support more generally useful earlier.
"
We're clearly in the use cases 2 and 3.
I understand that you're suggesting a 4th use-case, which seems
totally valid, but let's not forget the reasons why it has been merged
in the first place.
> > > > > The goal of this patch series is to keep clocks from being turned off.
> > > > > But that's not what it does. What it does is turn clocks on to prevent
> > > > > them from being turned off. In my opinion that's a workaround for a
> > > > > deficiency in the kernel (and the firmware/kernel interface) and I think
> > > > > it should be fixed at the root. So a much better solution would be to
> > > > > establish a way for firmware to communicate to the kernel that a given
> > > > > resource has been enabled by firmware and shouldn't be disabled. Such a
> > > > > solution can be implement for all types of resources and can be reused
> > > > > by all drivers since they don't have to worry about these details.
> > > >
> > > > Mike Turquette repeatedly said that he was against such a DT property:
> > > > https://lkml.org/lkml/2014/5/12/693
> > >
> > > Mike says in that email that he's opposing the addition of a property
> > > for clocks that is the equivalent of regulator-always-on. That's not
> > > what this is about. If at all it'd be a property to mark a clock that
> > > should not be disabled by default because it's essential.
> >
> > It's just semantic. How is "a clock that should not be disabled by
> > default because it's essential" not a clock that stays always on?
>
> Because a clock that should not be disabled by default can be turned off
> when appropriate. A clock that is always on can't be turned off.
If a clock is essential, then it should never be disabled. Or we don't
share the same meaning of essential.
> > Plus, you should read the mail further. It's clearly said that
> > consumer drivers should call clk_prepare_enable themselves, and that
> > the only exception to that is the case where we don't have such a
> > driver (and only valid as a temporary exception, which I guess you'll
> > soon turn into temporary-until-a-drm-kms-driver-is-merged).
>
> Exactly. simplefb is only a temporary measure. It's not meant to be used
> as a full-blown framebuffer driver. There are two use-cases where it is
> an appropriate solution:
>
> 1) As a stop-gap solution for when the platform doesn't support a full
> display driver yet. In that case you will want simplefb to stay
> around forever.
>
> 2) To get early boot output before a full display driver can be loaded
> in which case simplefb should go away after the display driver has
> taken over.
>
> In case of 2) the most straightforward solution is to not disable any
> clocks until all drivers have had a chance to claim them. When the full
> driver has been probed it should have claimed the clocks so they would
> no longer get disabled.
>
> For 1) I think it's fair to say that it's only a temporary solution to
> get something on the screen. There won't be any kind of acceleration at
> all, no power saving. Nothing but a dumb framebuffer. In that case it
> should be a simple matter of keeping a select number of clocks always on
> in the clock driver until support is more complete and it can be
> properly handled. It's not like it's going to make a difference anyway
> since simplefb won't ever disable them either.
We do agree on that. The only thing we seem to disagree on is how to
keep these clocks running.
> > > Adding Mike on this subthread too.
> > >
> > > Either way, Mark already suggested a different alternative in another
> > > subthread, namely to add a new kind of checkpoint at which subsystems
> > > can call a "disable unused" function that's called later than a late
> > > initcall. This is not going to fix things for you immediately because
> > > the clocks will still be switched off (only later) if you don't have
> > > a real driver that's claiming the clocks.
> >
> > Great, I'm glad we found a solution for a completely unrelated issue.
>
> It's not at all unrelated. See above.
Kind of, these patches were about the first use case you mentionned,
and your solution tackles the second one.
> > > But you can work around that for now by making the relevant clocks
> > > always on and remove that workaround once a real driver is loaded
> > > that knows how to handle them properly.
> >
> > So, let me get this straight. The clock provider driver should behave
> > as a clock consumer because it knows that in some cases, it might not
> > have any willingful enough consumer? Doesn't that even ring your
> > this-is-a-clear-abstraction-violation-bell just a tiny bit?
>
> No. The clock driver should simply not allow the clocks to be disabled
> if it isn't safe to do so.
Again, we do seem to differ on our interpretation of an essential
clock. To me, it is perfectly safe to disable the clocks. The system
will still be responding, the memory will be there, the CPU will keep
running, and we do have a way to recover from that disabled to clock
(for example to enable it back).
Plus, again, in Mike's mail, it's clearly said that adding hacks like
this to the clock driver should only be considered in the case where
we don't have a consuming driver, which is not our case here.
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: Geert Uytterhoeven @ 2014-08-27 8:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140827082208.GQ15297@lukather>
Hi Maxime,
On Wed, Aug 27, 2014 at 10:22 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Wed, Aug 27, 2014 at 08:40:57AM +0100, Mark Brown wrote:
>> On Tue, Aug 26, 2014 at 08:40:09PM +0200, Henrik Nordström wrote:
>>
>> > It is not clear to me where the hardware resources should be listed in
>> > DT, being it a simplefb node or part of the actual hardware device node
>> > properly marked as dynamic boot defaults or something else? It's
>> > somewhere inbetween hardware and virtual device, and somewhat volatile.
>> > As far as simplefb is concerned it is a hardware desription of the
>> > framebuffer, but for a kms driver it's no more than firmware handover of
>> > boottime settings and ceases to exists once the kms driver have
>> > reconfigured the hardware.
>>
>> Is simplefb something that should be in the device tree distinctly in
>> the first place - shouldn't it be a subset of the functionality of the
>> video nodes? It's the same hardware being driven differently.
>
> Therorically, yes, but that would mean knowing beforehand what the
> final binding will look like, even before submitting the driver. Since
> the bindings are always reviewed, and most of the time changed
> slightly, that wouldn't work very well with the DT as a stable ABI
> policy I guess.
If you don't know how the bindings for a device will look like at the time of
writing your DTS, you're always screwed, whether you add a simpefb
node or not.
If you know how the bindings look like, just add the device, with an extra
"linux,simplefb" compatibility value.
If you don't know how the bindings look like, do your utter best in
guessing. Your DTS must be amended later anyway, either because
you guessed wrong[*] (in case you added a node to have simplefb
working), or because you have to add a real device node (in case you
didn't add one for simplefb).
[*] Actually you may have guessed right, in which case it'll continue
working as-is, and everybody will be happy.
Whether you want to keep backwards-compatibility in your future driver
with the "guessed wrong" node is up to you.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-08-27 8:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140827074057.GS17528@sirena.org.uk>
[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]
On Wed, Aug 27, 2014 at 08:40:57AM +0100, Mark Brown wrote:
> On Tue, Aug 26, 2014 at 08:40:09PM +0200, Henrik Nordström wrote:
>
> > It is not clear to me where the hardware resources should be listed in
> > DT, being it a simplefb node or part of the actual hardware device node
> > properly marked as dynamic boot defaults or something else? It's
> > somewhere inbetween hardware and virtual device, and somewhat volatile.
> > As far as simplefb is concerned it is a hardware desription of the
> > framebuffer, but for a kms driver it's no more than firmware handover of
> > boottime settings and ceases to exists once the kms driver have
> > reconfigured the hardware.
>
> Is simplefb something that should be in the device tree distinctly in
> the first place - shouldn't it be a subset of the functionality of the
> video nodes? It's the same hardware being driven differently.
Therorically, yes, but that would mean knowing beforehand what the
final binding will look like, even before submitting the driver. Since
the bindings are always reviewed, and most of the time changed
slightly, that wouldn't work very well with the DT as a stable ABI
policy I guess.
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: Henrik Nordström @ 2014-08-27 8:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826135341.GM15297@lukather>
tis 2014-08-26 klockan 15:53 +0200 skrev Maxime Ripard:
> > it should be fixed at the root. So a much better solution would be to
> > establish a way for firmware to communicate to the kernel that a given
> > resource has been enabled by firmware and shouldn't be disabled. Such a
> > solution can be implement for all types of resources and can be reused
> > by all drivers since they don't have to worry about these details.
>
> Mike Turquette repeatedly said that he was against such a DT property:
> https://lkml.org/lkml/2014/5/12/693
And not really what we want here. The clocks should be turned off if not
in use, i.e. when simplefb is unbound from the device and no other
driver has taken over. Maybe even in the case where there is no simplefb
driver in-kernel, but if the use case is to be generalized then this
kind of resources need to be held until explicitly released to allow
resources to be held until their driver is loaded, which may happen long
after kernel bootup.
But again, for many use cases it is a virtual hardware property, not
really hardware in it's own but still smells a lot like hardware. These
parameters is only valid until the hardware have been reconfigured.
Regards
Henrik
^ 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