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:
pgp8BeonFm9zm.pgp
Description: PGP signature
