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

[Thierry Reding <thierry.reding@gmail.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 --]