Re: DT binding review for Armada display subsystem

[Sylwester Nawrocki <sylvester.nawrocki-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 07/13/2013 10:35 AM, Jean-Francois Moine wrote:
> On Fri, 12 Jul 2013 13:00:23 -0600 Daniel Drake<dsd-2X9k7bc8m7Mdnm+yROfE0A@public.gmane.org>  wrote:
>> On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moine<moinejf-GANU6spQydw@public.gmane.org>  wrote:

>>> - the phandles to the clocks does not tell how the clock may be set by
>>>    the driver (it is an array index in the 510).
>>
>> In the other threads on clock selection, we decided that that exact
>> information on how to select a clock (i.e. register bits/values) must
>> be in the driver, not in the DT. What was suggested there is a
>> well-documented scheme for clock naming, so the driver knows which
>> clock is which. That is defined in the proposed DT binding though the
>> "valid clock names" part. For an example driver implementation of this
>> you can see my patch "armada_drm clock selection - try 2".
>
> OK.
>
> Sorry to go back to this thread.
>
> 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

Hmm, a bit off topic question, does it work when the si5351 module gets
removed ? I can't see anything preventing clock provider module from being
removed why any of its clocks is used and clk_unregister() function is
currently unimplemented.

> (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.

Doesn't deferred probing help here ?

>>> - I don't see the use of the phandles in the leaves (dcon and tda998x).
>>
>> That is defined by the video interfaces common binding. I'm not
>> immediately aware of the use, but the ability to easily traverse the
>> graph in both directions seems like a requirement that could easily
>> emerge from somewhere.
>
> OK, but I am not convinced...
>
>>> Well, here is how I feel the DTs:
>>>
>>> - for the Cubox (one LCD, the DCON is not used, TDA998x for HDMI/DVI
>>>    output):
>>
>> That is the same as my proposal except you have decided to use direct
>> phandle properties instead of using the common video interfaces
>> binding (which is just a slightly more detailed way of describing such
>> links). In the "best practices" thread, the suggestion was raised
>> multiple times of doing what v4l2 does, so thats why I decided to
>> explore this here.
>>
>>> - for some tablet based on the Armada 510 with a LCD and a VGA connector:
>>>
>>>          video {
>>>                  compatible = "marvell,armada-video";
>>>                  marvell,video-devices =<&lcd0>,<&lcd1>,<&dcon>
>>>          };
>>
>> This proposal is slightly different because it does not describe the
>> relationship between the LCD controllers and the DCON. Focusing
>> specifically on LCD1, which would be connected to a DAC via a phandle
>> property in your proposal. The interconnectivity between the
>> components represented as a graph (and in the v4l2 way, which includes
>> my proposal) would be:
>>
>> LCD1 --- DCON --- DAC
>>
>> However your phandle properties would "suggest" that LCD1 is directly
>> connected to the DAC. The driver would have to have special knowledge
>> that the DCON sits right in the middle. Maybe that is not an issue, as
>> it is obviously OK for the driver to have *some* knowledge about how
>> the hardware works :)
>>
>> I don't think we have a full consensus on whether we want to go the
>> "v4l2 way" here. But I figured that would be a good thing to propose
>> in a first iteration to have a clearer idea of what the result would
>> look like. It sounds like you are not overly keen on it, I would be
>> interested to hear exactly why.
>
> I think this is because I was focused on the software and not on the
> hardware.
>
> 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:
>
> 	lcd0: lcd-controller@820000 {
> 		compatible = "marvell,dove-lcd0";
[...]
> 	};
>
> 	lcd1: lcd-controller@810000 {
> 		compatible = "marvell,dove-lcd1";
[...]
> 	};

Using different compatible strings to indicate multiple instances of same
hardware doesn't seem right. Unless LCD0, LCD1 are really different pieces
of hardware incompatible with each other I think it would be more correct
to use same compatible property and DT node aliases, similarly as is now
done with e.g. I2C busses:

	aliases {
		lcd0 = &lcd_0;	
		lcd1 = &lcd_1;	
	};

  	lcd_0: lcd-controller@820000 {
  		compatible = "marvell,dove-lcd";
		...
  	};


  	lcd_1: lcd-controller@820000 {
  		compatible = "marvell,dove-lcd";
		...
  	};


> 	/* 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";
> 	};
>
> 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 */

Hmm, was this custom binding intended or did you mean rather something
like:

  	&i2c0 {
  		si5351: clock-generator {
  			...
  		};
  		tda998x: hdmi-encoder {
  			compatible = "nxp,tda998x";
  			reg =<0x70>;
  			interrupt-parent =<&gpio0>;
  			interrupts =<27 2>;		/* falling edge */
  			marvell-input =<&dcon 0>;

			port@I {
				reg = <I>; /* input (or reg omitted completely) */
				endpoint {
					remote-endpoint = <&dcon>;
				};
			}
  		};
  	};
  	&lcd0 {
  		status = "okay";
  		clocks =<&si5351 0>;
  		clock-names = "extclk0";
  	};
  	&dcon {
  		status = "okay";
			
		port@A {
			reg = <A>; /* port A */
			endpoint {
				remote-endpoint = <&tda998x>;
			};
		}
		/* no connector on port B */
  	};

I think it should be tried to use common binding for common problems,
then a common parser library could be used, instead of repeating similar
code in each driver.

> 	};
>
> 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>;
> 	};
>
> About the LCD clocks, the drm driver may choose itself one of the
> clocks declared in the DT. If a clock should not be used, if should be
> zeroed in the board specific DT:
>
> 	&lcd0 {
> 		status = "okay";
> 		clocks = 0, 0,<&si5351 0>;
> 		clock-names = "axi", "lcdclk", "extclk0";
> 	};

Hmm, not sure how that could work. IIUC there should be same number
of cells used in the clocks property for each clock specifier (clocks
provider's node #clock-cells), so &si5351 cell would need to be at
offset 4. Maybe there is some convention to specify null phandles but
I'm not aware of it.

--
Regards,
Sylwester