Hi,

On 08/27/2014 08:54 AM, Thierry Reding wrote:

On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote:

quoted

On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote:

quoted

quoted

quoted

quoted

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.

quoted

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.

quoted

quoted

quoted

quoted

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.

quoted

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.

We've been down this road before, and all it leads to is pain, some more
pain and then even more pain. Followed by the conclusion that everything
needed to be refactored to use a hotplug model, and that instead of
waiting for hardware / disk-enumeration to be done (so we can mount
filesystems), the right thing to do is to actually wait for the block
device(s) holding said filesystems to show up.

The same applies here, the right thing to do is to wait for the kms
driver claiming the clocks to actually show up. And the easiest and
cleanest way to do that is to have simplefb claim the clocks, and
have it release them when the kms driver loads and asks simplefb
to unload.

Oh and one more thing, this whole make a list of special clocks which
should be disabled later model is flawed too, because it assumes a
fixed list of clocks, but which clocks actually get used may very well
depend on whether e.g. a vga or hdmi monitor is attached, so u-boot
would then still need to communicate the list of clocks actually used
somehow, and if it needs to pass a list of clocks, the most natural
way to do so is through a clocks property...

Regards,

Hans