Re: Tegra DRM with HDMI support (\o/)

On Fri, Oct 12, 2012 at 03:17:04PM -0600, Stephen Warren wrote:

On 10/11/2012 11:09 PM, Thierry Reding wrote:

quoted

On Thu, Oct 11, 2012 at 05:36:35PM -0600, Stephen Warren wrote:

[...]

quoted

quoted

The code in clock.c uses lots of clk_get_sys() calls with
hard-coded clock names. We really should be using the common
clock DT bindings for this instead of hard-coding names. This is
especially true since the names differ on different SoCs, so
there's a ton of of_device_is_compatible(output->dev->of_node,
"nvidia,tegra30-hdmi") in this code.

I don't quite see how that is supposed to work. Wouldn't that mean
that we needed to setup various entries in the clock tables to be
able to look the clocks up by generic names? Like pll_d would need
to be the "parent" clock of "tegra-hdmi" or similar. In that case
we could call clk_get(hdmi->dev, "parent") instead. Is that what
you had in mind?

There are two ways this could work:

1)

All clocks needed could be represented in the node of that device (or
perhaps in the DC node?) For example, perhaps hdmi might contain:

clocks = <&car TEGRA_CLK_PLL_D>
         <&car TEGRA_CLK_PLL_D_OUT_0>
         <&car TEGRA_CLK_HDMI>
         ...;
clock-names = "pll_d", "pll_d_out_0", "hdmi", ...;

That should work (I think) with pretty much no modification to the
current code, other than calling clk_get(dev, "hdmi") rather than
clk_get_sys(NULL, "hdmi").

I thought DT support didn't work for Tegra yet. If that works we should
probably get rid of the DRM related entries in the clock tables and put
the information in the DT regardless of which of the alternatives is
implemented.

However, I'm not sure this is the best way or not; this still requires
the HDMI driver to implement all the clocking policy (e.g. is the HDMI
clock a child of PLL_D, PLL_C, PLL_P, ... and what rate should the
parent PLL be set to, etc.)

Yes, it's pretty ugly and as you mentioned before it requires many
special cases to be handled depending on the SoC. I'm presuming that
future Tegra generations will be supported by the same driver and the
number of special cases will only increase.

For example, what if the clock frequency HDMI needs can be generated
by PLL_C, so we make PLL_C the parent of HDMI (so as to save power by
not running PLL_D, or to use PLL_D for a second display head), then
something else comes along and /has/ to use PLL_C. If the two childs'
frequency requirements are the same, everything is fine. If the two
child's frequency requirements are not compatible, we might need to
spin up PLL_D and reparent HDMI onto it. More complex scenarios are
possible where PLL_C needs to be reprogrammed, yet can be in a way
that supports both HDMI and some other requirement, so we only need to
temporarily move HDMI onto PLL_D, then reprogram PLL_C, then move HDMI
back to it and spin PLL_D down. All these decisions should be made
centrally in the Tegra clock driver, or at least some kind of
centralized policy management point, and not by the HDMI driver:

That does sound awfully complex. However given that it would allow us to
use only a single clock and program it in whatever way we need in the
drivers is a very strong point in favour of this alternative.

Furthermore, given the above scenarios, it won't be possible to do that
without a central implementation because all clock usage information
needs to be available to make the right decisions.

2)

The HDMI device node only knows about the clocks used directly by the
HDMI module:

clocks = <&car TEGRA_CLK_HDMI>;
clock-names = "hdmi";

(note: that's just an example; I don't know off the top of my head
exactly which clocks that module consumes directly)

Then, the HDMI driver will simply perform a single clk = clk_get(dev,
"hdmi"), and later call clk_set_rate(clk, rate) as appropriate. All
the policy decisions then get made internally to the Tegra clock
driver (since it's a central nexus for all clock outputs from the
Clock And Reset controller (CAR).

From an abstraction or design point of view, the CAR indeed seems the
best place to implement this kind of policy. User drivers shouldn't have
to worry or know about any of these details. The big win here being that
most of the special casing could be removed, therefore making the driver
a whole lot simpler and easier to maintain.

A similar question exists for audio too. Currently, our audio drivers
do something similar to what tegra-drm is doing; (1) above. However, I
have a feeling we may want to convert audio over to model (2) above too.

Oh, and the cpufreq driver has a similar for centralized policy, since
at 216MHz the CPU clocks are derived from one PLL, but for anything
faster we use PLL_X to generate them. Right now, this is implemented
in the cpufreq driver, but again should really be centrally managed.

After reading all of the above I'm definitely in favour of alternative
2). If you think we can pull it off I'd say let's go for it. I'm willing
to spend some time on helping with this, but I must admit I'm not overly
familiar with the clock framework. Maybe Prashant would be better suited
to have a shot at it.

Thierry