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

[Maxime Ripard <maxime.ripard@free-electrons.com>]

[-- Attachment #1: Type: text/plain, Size: 2674 bytes --]

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> On 08/13/2014 01:17 AM, Luc Verhaegen wrote:
> >This claims and enables clocks listed in the simple framebuffer dt node.
> >This is needed so that the display engine, in case the required clocks
> >are known by the kernel code and are described in the dt, will remain
> >properly enabled.
> 
> I think this make simplefb not simple any more, which rather goes
> against the whole point of it.
> 
> I specifically conceived simplefb to know about nothing more than
> the memory address and pixel layout of the memory buffer. I
> certainly don't like the idea of expanding simplefb to anything
> beyond that, and IIRC *not* extending is was a condition agreed when
> it was first merged. If more knowledge than that is required, then
> there needs to be a HW-specific driver to manage any
> clocks/resets/video registers, etc.

I'm sorry, but how is that not simple? clocks enabling is step 1 in a
driver in order to communicate somehow with the controller. Reset is a
different story, because arguably, if simplefb is there, the
controller is already out of reset.

And I don't see why video registers are coming into the discussion
here. The code Luc posted doesn't access any register, at all. It just
makes sure the needed controller keep going.

> The correct way to handle this without a complete DRM/KMS/... driver
> is to avoid the clocks in question being turned off at boot.

Which is exactly what this code does, using the generic DT bindings to
express dependency for a given clock. How is this wrong?

> I thought there was a per-clock flag to prevent disabling an unused
> clock?

No, last time I heard, Mike Turquette was against it.

> If not, perhaps the clock driver should force the clock to be
> enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our
clock driver.

I'm not going to have a huge list of ifdef depending on configuration
options to know which clock to enable, especially when clk_get should
have the consumer device as an argument.

> For example, the Tegra clock driver has a clock initialization table
> which IIRC was used for this purpose before we got a DRM/KMS driver.
> That way, all the details are kept inside the kernel code, and don't
> end up influencing the DT representation of simplefb.

I don't really see how the optional usage of a generic property
influences badly the DT representation of simplefb.

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 --]