On 07/13/2013 04:25 PM, Daniel Drake wrote:
On Sat, Jul 13, 2013 at 2:35 AM, Jean-Francois Moine <moinejf@xxxxxxx> wrote:
I use my Cubox for daily jobs as a desktop computer. My kernel is a DT
driven 3.10.0. The dove-drm, tda998x and si5351 (clock) are kernel
modules. I set 3 clocks in the DT for the LCD0: lcdclk, axi and extclk0
(si5351). Normally, the external clock is used, but, sometimes, the
si5351 module is not yet initialized when the drm driver starts. So,
for 1920x1080p, it uses the lcdclk which sets the LCD clock to 133333
(400000/3) instead of 148500. As a result, display and sound still work
correctly on my TV set thru HDMI.
So, it would be nice to have 2 usable clocks per LCD, until we find a
good way to initialize the modules in the right order at startup time.
Having multiple usable clocks is implemented in the patch I referred
you to. However it doesn't solve the "better clock turns up after
initializing the driver" problem, which seems like a tricky one.
Maybe the best solution is to defer probe until all DT-defined clocks
become available. That means that whoever compiles the kernel must
take care to not forget to build the clock drivers for all the clocks
referenced in this part of the DT, but perhaps that is a reasonable
thing to require.
On the other hand, this problem acts in support of a simpler approach
mentioned by Sebastian: developers figure out what the best clock is
for each CRTC on each board, and the DT only specifies that one clock.
The driver uses that clock if it is available and defers probe if it
is not.
Daniel,
sorry for the late response, I just came back from a boat trip around
Capri :D
About the clocks and deferred probing, the issue here is that you might
have trouble to find out if that clock will ever come up. Therefore, I
suggested the easiest heuristic which is "let the DT author decide".
I am fine with allowing more than one clock from DT and get or wait for
all/one clock.
Back to the specific case of the Cubox with new ideas.
The Cubox is based on the Armada 510 (Dove), so, all the hardware must
be described in dove.dtsi. This includes the LCDs and DCON/IRE, the
possible clocks and the (static) v4l2 links:
As a sidenote, I think we have concluded that we are not going to
interact with the armada 510 DCON in any way at the moment. The driver
will not have code that touches it, and the DT will not mention it.
The first person who actually needs to use the DCON will have the
responsibility for doing that work. Nevertheless it makes sense for us
to pick a DT design where we know that the DCON could be slotted in
later.
True, do not link dcon node with any of lcd, tda998x or anything else.
If there is support for it in the driver we just use
of_find_compatible_node and let the driver do the magic. From a physical
POV, lcd0 is directly connected to tda998x through dumb RGB pins. DCON
can control data flow to those pins but should not be DT linked.
lcd0: lcd-controller@820000 {
compatible = "marvell,dove-lcd0";
reg = <0x820000 0x1c8>;
interrupts = <47>;
clocks = <&core_clk 3>, <&lcdclk>;
clock-names = "axi", "lcdclk";
About clock names for Armada 510 LCD: I suggest "axiclk", "lcdpll",
"extclk0", "extclk1". IIRC axiclk is 333MHz (or maybe 2xTCLK),
lcdpll can be derived from 2GHz core PLL with min integer divider
of 5. extclk0/1 are dedicated pins and can be used from both lcd0
and lcd1.
marvell-output = <&dcon 0>;
status = "disabled";
};
lcd1: lcd-controller@810000 {
compatible = "marvell,dove-lcd1";
reg = <0x810000 0x1c8>;
interrupts = <46>;
clocks = <&core_clk 3>, <&lcdclk>;
clock-names = "axi", "lcdclk";
marvell-output = <&dcon 1>;
status = "disabled";
};
/* display controller and image rotation engine */
dcon: display-controller@830000 {
compatible = "marvell,dove-dcon";
reg = <0x830000 0xc4>, /* DCON */
<0x831000 0x8c>; /* IRE */
interrupts = <45>;
marvell-input = <&lcd0>, <&lcd1>;
status = "disabled";
};
I guess the IRE falls into the same category as the DCON - we won't
implement it for now, but knowing where it might fit in is useful.
Why would you put it in the same node as the DCON though? It has its
own separate register space and additionally it is a component shared
with the MMP boards (whereas the DCON is not).
DCON and IRE are summarized in the same register description in Dove FS.
Maybe we can split them up and have two nodes or just one if registers
overlap. Anyway, not that important for now.
The specific Cubox hardware (tda998x and si5351) is described in
dove-cubox.dts, with the new clocks and the v4l2 link dcon0 <--> tda998x.
&i2c0 {
si5351: clock-generator {
...
};
tda998x: hdmi-encoder {
compatible = "nxp,tda998x";
reg = <0x70>;
interrupt-parent = <&gpio0>;
interrupts = <27 2>; /* falling edge */
marvell-input = <&dcon 0>;
};
};
&lcd0 {
status = "okay";
clocks = <&si5351 0>;
clock-names = "extclk0";
};
&dcon {
status = "okay";
marvell-output = <&tda998x>, 0; /* no connector on port B */
};
So now you are taking an approach equivalent to the v4l2 standard
"video interfaces" binding, which is the concept of representing a
connection graph via phandles. This means that our two proposals are
equivalent yet I proposed using a standard interface and you proposed
inventing your own, again without explaining why you don't like the
standard interface.
I suggest to choose the same names v4l2 did, they are the first and
as long as the use case matches, we should reuse their names.
Then, about the software, the drm driver may not need to know anything
more (it scans the DT for the LCDs and gets all the subdevices thanks
to the v4l2 links):
video {
compatible = "marvell,armada-video";
};
For some boards / other SoCs, there may be independant drm devices. In
this case, there would be descriptions as:
video0 {
compatible = "marvell,armada-video0";
marvell,video-devices = <&lcd0>;
};
video1 {
compatible = "marvell,armada-video1";
marvell,video-devices = <&lcd1>;
};
This bit differs from my proposal, but I'm not sure what the benefit
of your design is. In my design, the two above use cases are
represented cleanly using the same DT abstraction (same compatible
string, same required properties, etc). In your design, the two use
cases call for something quite different as shown above.
Of course, compatible string of both (virtual) video devices should
be "marvell,armada-510-video" or if you prefer to follow Sascha Hauer's
suggestion reuse the lcd controller node instead of the "super node".
Sebastian
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
