DT binding review for Armada display subsystem

DT binding review for Armada display subsystem

[Daniel Drake]

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

Hi,

Based on the outcomes of the "Best practice device tree design for display
subsystems" discussion I have drafted a DT binding. Comments much appreciated.

At a high level, it uses a "super node" as something for the driver to bind
to, needed because there is no clear one device that controls all the
others, and also to distinguish between the Armada 510 possible use cases
of having one video card with two outputs, or two independent video cards.
It uses node-to-node linking beyond that point, V4L2 style.

I have some questions still:

1. What names does the Armada 510 datasheet give to the components? Below
   I renamed the "LCD controllers" (which don't control LCDs on any of the
   current target hardware) to "display controllers". For what we were
   previously referring to as DCON, I renamed to "output selector". I would
   like to match the docs.

   Actually the output selector is not mentioned as per Sebastian's
   suggestion, but I believe it would fit in the below design once the first
   user comes along (e.g. it would slot in the graph between dcon0 and tda99x).

2. On that topic, what names does the Armada 510 datasheet give to the
   available clocks? Lets make them match the docs.

3. What is the "remote" property in the video interfaces binding? Doesn't
   seem to be documented. But I think I copied its usage style below.





Marvell Armada Display Subsystem

Design considerations
---------------------

The display device that emerges from this subsystem is quite heavily
componentized and the formation of the composite device will vary by SoC and
by board. The DT binding is designed with this flexibility in mind.

For example, there are 2 display controllers on the Armada 510.
They could legitametely be used to form two independent video cards, or
they could be combined into a single video card with two outputs, depending
on board wiring and use case.

As there is no clear component that controls the other components, especially
when we wish to combine the two display controllers into one virtual device, we
define a top-level "super node" that describes the basic composition of the
overall display device. That node uses phandles to define the start of the
graph of display components.

In the bindings for the individual display components, phandle properties
are used to represent direct, fixed links to other components. We use the
port/endpoint abstraction from bindings/media/video-interfaces.txt to
represent these links.


display super node
------------------

Required properties:
 - compatible: "marvell,armada-display"
 - marvell,video-devices : List of phandles to the display controllers that
   form this composite device.

Optional properties;
 - reg: Memory address and size of a RAM allocation designated as graphics
   memory
 - linux,video-memory-size: If reg is not provided, this property defines
   how much memory to cut out of regular available RAM to be used as graphics
   memory.


display-controller node
-----------------------

Required properties:
 - compatible: "marvell,armada-510-display", "marvell,mmp2-display" or
  "marvell,mmp3-display"
 - reg: Register space for this display controller
 - clocks: List of clocks available to this device. From common clock binding.
 - clock-names: Names of the given clocks (see below)
 - ports: From video interface binding

Valid clock names for Armada 510: extclk0 extclk1 axi pll

Valid clock names for MMP2: lcd1 lcd2 axi hdmi

Valid clock names for MMP3: lcd1 lcd2 axi hdmi dsi lvds


Example:

    display {
      compatible = "marvell,armada-display";
      reg = <0 0x3f000000 0x1000000>; /* video-mem hole */
      marvell,video-devices = <&dcon0>;
    };

    dcon0: display-controller@20000 {
      compatible = "marvell,armada-510-display-controller";
      reg = <0x20000 0x1000>;
      interrupts = <47>;
      clocks = <&ext_clk0>;
      clock-names = "extclk0";

      port {
        dcon0_1: endpoint {
          remote = <&tda998x>;
        };
      };
    };

    &i2c0 {
      tda998x: hdmi-transmitter@60 {
        compatible = "nxp,tda19988";
        reg = <0x60>;
        port {
          tda998x_1: endpoint {
            remote-endpoint = <&dcon0_1>;
          };
        };
      };
    };

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

DT binding review for Armada display subsystem

[Daniel Drake]

Hi,

Based on the outcomes of the "Best practice device tree design for display
subsystems" discussion I have drafted a DT binding. Comments much appreciated.

At a high level, it uses a "super node" as something for the driver to bind
to, needed because there is no clear one device that controls all the
others, and also to distinguish between the Armada 510 possible use cases
of having one video card with two outputs, or two independent video cards.
It uses node-to-node linking beyond that point, V4L2 style.

I have some questions still:

1. What names does the Armada 510 datasheet give to the components? Below
   I renamed the "LCD controllers" (which don't control LCDs on any of the
   current target hardware) to "display controllers". For what we were
   previously referring to as DCON, I renamed to "output selector". I would
   like to match the docs.

   Actually the output selector is not mentioned as per Sebastian's
   suggestion, but I believe it would fit in the below design once the first
   user comes along (e.g. it would slot in the graph between dcon0 and tda99x).

2. On that topic, what names does the Armada 510 datasheet give to the
   available clocks? Lets make them match the docs.

3. What is the "remote" property in the video interfaces binding? Doesn't
   seem to be documented. But I think I copied its usage style below.





Marvell Armada Display Subsystem

Design considerations
---------------------

The display device that emerges from this subsystem is quite heavily
componentized and the formation of the composite device will vary by SoC and
by board. The DT binding is designed with this flexibility in mind.

For example, there are 2 display controllers on the Armada 510.
They could legitametely be used to form two independent video cards, or
they could be combined into a single video card with two outputs, depending
on board wiring and use case.

As there is no clear component that controls the other components, especially
when we wish to combine the two display controllers into one virtual device, we
define a top-level "super node" that describes the basic composition of the
overall display device. That node uses phandles to define the start of the
graph of display components.

In the bindings for the individual display components, phandle properties
are used to represent direct, fixed links to other components. We use the
port/endpoint abstraction from bindings/media/video-interfaces.txt to
represent these links.


display super node
------------------

Required properties:
 - compatible: "marvell,armada-display"
 - marvell,video-devices : List of phandles to the display controllers that
   form this composite device.

Optional properties;
 - reg: Memory address and size of a RAM allocation designated as graphics
   memory
 - linux,video-memory-size: If reg is not provided, this property defines
   how much memory to cut out of regular available RAM to be used as graphics
   memory.


display-controller node
-----------------------

Required properties:
 - compatible: "marvell,armada-510-display", "marvell,mmp2-display" or
  "marvell,mmp3-display"
 - reg: Register space for this display controller
 - clocks: List of clocks available to this device. From common clock binding.
 - clock-names: Names of the given clocks (see below)
 - ports: From video interface binding

Valid clock names for Armada 510: extclk0 extclk1 axi pll

Valid clock names for MMP2: lcd1 lcd2 axi hdmi

Valid clock names for MMP3: lcd1 lcd2 axi hdmi dsi lvds


Example:

    display {
?? ??   compatible = "marvell,armada-display";
?? ?? ?? reg = <0 0x3f000000 0x1000000>; /* video-mem hole */
?? ?? ?? marvell,video-devices = <&dcon0>;
?? ?? };

    dcon0: display-controller at 20000 {
      compatible = "marvell,armada-510-display-controller";
?? ?? ?? reg = <0x20000 0x1000>;
?? ?? ?? interrupts = <47>;
?? ?? ?? clocks = <&ext_clk0>;
?? ?? ?? clock-names = "extclk0";

      port {
        dcon0_1: endpoint {
          remote = <&tda998x>;
        };
      };
    };

    &i2c0 {
?? ??   tda998x: hdmi-transmitter at 60 {
?? ?? ??   compatible = "nxp,tda19988";
?? ?? ?? ?? reg = <0x60>;
        port {
          tda998x_1: endpoint {
            remote-endpoint = <&dcon0_1>;
          };
        };
?? ?? ?? };
    };

Re: DT binding review for Armada display subsystem

[Jean-Francois Moine]

On Fri, 12 Jul 2013 12:44:28 -0400 (EDT)
Daniel Drake <dsd@laptop.org> wrote:

> Hi,
> 
> Based on the outcomes of the "Best practice device tree design for display
> subsystems" discussion I have drafted a DT binding. Comments much appreciated.
> 
> At a high level, it uses a "super node" as something for the driver to bind
> to, needed because there is no clear one device that controls all the
> others, and also to distinguish between the Armada 510 possible use cases
> of having one video card with two outputs, or two independent video cards.
> It uses node-to-node linking beyond that point, V4L2 style.
> 
> I have some questions still:
> 
> 1. What names does the Armada 510 datasheet give to the components? Below
>    I renamed the "LCD controllers" (which don't control LCDs on any of the
>    current target hardware) to "display controllers". For what we were
>    previously referring to as DCON, I renamed to "output selector". I would
>    like to match the docs.
> 
>    Actually the output selector is not mentioned as per Sebastian's
>    suggestion, but I believe it would fit in the below design once the first
>    user comes along (e.g. it would slot in the graph between dcon0 and tda99x).
> 
> 2. On that topic, what names does the Armada 510 datasheet give to the
>    available clocks? Lets make them match the docs.
> 
> 3. What is the "remote" property in the video interfaces binding? Doesn't
>    seem to be documented. But I think I copied its usage style below.

Hi Daniel,

The Armada 510 spec is:

http://www.marvell.com/application-processors/armada-500/assets/Armada-510-Functional-Spec.pdf

> Marvell Armada Display Subsystem
> 
> Design considerations
> ---------------------
> 
> The display device that emerges from this subsystem is quite heavily
> componentized and the formation of the composite device will vary by SoC and
> by board. The DT binding is designed with this flexibility in mind.
> 
> For example, there are 2 display controllers on the Armada 510.
> They could legitametely be used to form two independent video cards, or
> they could be combined into a single video card with two outputs, depending
> on board wiring and use case.
> 
> As there is no clear component that controls the other components, especially
> when we wish to combine the two display controllers into one virtual device, we
> define a top-level "super node" that describes the basic composition of the
> overall display device. That node uses phandles to define the start of the
> graph of display components.
> 
> In the bindings for the individual display components, phandle properties
> are used to represent direct, fixed links to other components. We use the
> port/endpoint abstraction from bindings/media/video-interfaces.txt to
> represent these links.
> 
> 
> display super node
> ------------------
> 
> Required properties:
>  - compatible: "marvell,armada-display"
>  - marvell,video-devices : List of phandles to the display controllers that
>    form this composite device.
> 
> Optional properties;
>  - reg: Memory address and size of a RAM allocation designated as graphics
>    memory
>  - linux,video-memory-size: If reg is not provided, this property defines
>    how much memory to cut out of regular available RAM to be used as graphics
>    memory.
> 
> 
> display-controller node
> -----------------------
> 
> Required properties:
>  - compatible: "marvell,armada-510-display", "marvell,mmp2-display" or
>   "marvell,mmp3-display"
>  - reg: Register space for this display controller
>  - clocks: List of clocks available to this device. From common clock binding.
>  - clock-names: Names of the given clocks (see below)
>  - ports: From video interface binding
> 
> Valid clock names for Armada 510: extclk0 extclk1 axi pll
> 
> Valid clock names for MMP2: lcd1 lcd2 axi hdmi
> 
> Valid clock names for MMP3: lcd1 lcd2 axi hdmi dsi lvds
> 
> 
> Example:
> 
>     display {
>       compatible = "marvell,armada-display";
>       reg = <0 0x3f000000 0x1000000>; /* video-mem hole */
>       marvell,video-devices = <&dcon0>;
>     };
> 
>     dcon0: display-controller@20000 {
>       compatible = "marvell,armada-510-display-controller";
>       reg = <0x20000 0x1000>;
>       interrupts = <47>;
>       clocks = <&ext_clk0>;
>       clock-names = "extclk0";
> 
>       port {
>         dcon0_1: endpoint {
>           remote = <&tda998x>;
>         };
>       };
>     };
> 
>     &i2c0 {
>       tda998x: hdmi-transmitter@60 {
>         compatible = "nxp,tda19988";
>         reg = <0x60>;
>         port {
>           tda998x_1: endpoint {
>             remote-endpoint = <&dcon0_1>;
>           };
>         };
>       };
>     };

Some remarks:

- I think it is better to keep the names 'lcd' for the memory to dumb panel
  sub-devices and 'dcon' for the dumb panel to LCD/VGA sub-device, as
  named in the spec.

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

- I don't see the use of the phandles in the leaves (dcon and tda998x).

Well, here is how I feel the DTs:

- for the Cubox (one LCD, the DCON is not used, TDA998x for HDMI/DVI
  output):

	video {
		compatible = "marvell,armada-video";
		marvell,video-devices = <&lcd0>		/* only one device */
	};

	/* the LCD reg and interrupts are described in dove.dtsi */

	&lcd0 {
		status = "okay";
		clocks = <&core_clk 3>, <&lcdclk>, <&si5351 0>;
		marvell,clock-type = <0>, <2>, <3>;	/* how to set the clock */
		marvell,connector-type = <11>;		/* HDMIA */
		marvell,external-encoder = <&tda998x>;
	};

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

- 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>
	};

	/* the LCDs reg and interrupts are described in dove.dtsi */

	&lcd0 {						/* LCD */
		status = "okay";
		clocks = <&lcdclk>;			/* only the LCD clock */
		marvell,clock-type = <2>;
		marvell,connector-type = <7>;		/* LVDS */
		display-timings {
			mode {
				hactive = <1920>;
				vactive = <1080>;
				hfront-porch = <88>;
				hsync-len = <44>;
				hback-porch = <148>;
				vfront-porch = <4>;
				vsync-len = <5>;
				vback-porch = <36>;
				clock = <148500>;
			};
		};
	};
	&lcd1 {						/* VGA */
		status = "okay";
		clocks = <&core_clk 3>, <&lcdclk>;	/* only the AXI and LCD clocks */
		marvell,clock-type = <0>, <2>;		/* how to set the clock */
		marvell,port-type = <1>;		/* VGA */
			/* there may be some external encoder to handle DAC */
	};

	/* the DCON reg and interrupts are described in dove.dtsi */

	&dcon { status = "okay"; };

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

DT binding review for Armada display subsystem

[Jean-Francois Moine]

On Fri, 12 Jul 2013 12:44:28 -0400 (EDT)
Daniel Drake <dsd@laptop.org> wrote:

> Hi,
> 
> Based on the outcomes of the "Best practice device tree design for display
> subsystems" discussion I have drafted a DT binding. Comments much appreciated.
> 
> At a high level, it uses a "super node" as something for the driver to bind
> to, needed because there is no clear one device that controls all the
> others, and also to distinguish between the Armada 510 possible use cases
> of having one video card with two outputs, or two independent video cards.
> It uses node-to-node linking beyond that point, V4L2 style.
> 
> I have some questions still:
> 
> 1. What names does the Armada 510 datasheet give to the components? Below
>    I renamed the "LCD controllers" (which don't control LCDs on any of the
>    current target hardware) to "display controllers". For what we were
>    previously referring to as DCON, I renamed to "output selector". I would
>    like to match the docs.
> 
>    Actually the output selector is not mentioned as per Sebastian's
>    suggestion, but I believe it would fit in the below design once the first
>    user comes along (e.g. it would slot in the graph between dcon0 and tda99x).
> 
> 2. On that topic, what names does the Armada 510 datasheet give to the
>    available clocks? Lets make them match the docs.
> 
> 3. What is the "remote" property in the video interfaces binding? Doesn't
>    seem to be documented. But I think I copied its usage style below.

Hi Daniel,

The Armada 510 spec is:

http://www.marvell.com/application-processors/armada-500/assets/Armada-510-Functional-Spec.pdf

> Marvell Armada Display Subsystem
> 
> Design considerations
> ---------------------
> 
> The display device that emerges from this subsystem is quite heavily
> componentized and the formation of the composite device will vary by SoC and
> by board. The DT binding is designed with this flexibility in mind.
> 
> For example, there are 2 display controllers on the Armada 510.
> They could legitametely be used to form two independent video cards, or
> they could be combined into a single video card with two outputs, depending
> on board wiring and use case.
> 
> As there is no clear component that controls the other components, especially
> when we wish to combine the two display controllers into one virtual device, we
> define a top-level "super node" that describes the basic composition of the
> overall display device. That node uses phandles to define the start of the
> graph of display components.
> 
> In the bindings for the individual display components, phandle properties
> are used to represent direct, fixed links to other components. We use the
> port/endpoint abstraction from bindings/media/video-interfaces.txt to
> represent these links.
> 
> 
> display super node
> ------------------
> 
> Required properties:
>  - compatible: "marvell,armada-display"
>  - marvell,video-devices : List of phandles to the display controllers that
>    form this composite device.
> 
> Optional properties;
>  - reg: Memory address and size of a RAM allocation designated as graphics
>    memory
>  - linux,video-memory-size: If reg is not provided, this property defines
>    how much memory to cut out of regular available RAM to be used as graphics
>    memory.
> 
> 
> display-controller node
> -----------------------
> 
> Required properties:
>  - compatible: "marvell,armada-510-display", "marvell,mmp2-display" or
>   "marvell,mmp3-display"
>  - reg: Register space for this display controller
>  - clocks: List of clocks available to this device. From common clock binding.
>  - clock-names: Names of the given clocks (see below)
>  - ports: From video interface binding
> 
> Valid clock names for Armada 510: extclk0 extclk1 axi pll
> 
> Valid clock names for MMP2: lcd1 lcd2 axi hdmi
> 
> Valid clock names for MMP3: lcd1 lcd2 axi hdmi dsi lvds
> 
> 
> Example:
> 
>     display {
> ?? ??   compatible = "marvell,armada-display";
> ?? ?? ?? reg = <0 0x3f000000 0x1000000>; /* video-mem hole */
> ?? ?? ?? marvell,video-devices = <&dcon0>;
> ?? ?? };
> 
>     dcon0: display-controller at 20000 {
>       compatible = "marvell,armada-510-display-controller";
> ?? ?? ?? reg = <0x20000 0x1000>;
> ?? ?? ?? interrupts = <47>;
> ?? ?? ?? clocks = <&ext_clk0>;
> ?? ?? ?? clock-names = "extclk0";
> 
>       port {
>         dcon0_1: endpoint {
>           remote = <&tda998x>;
>         };
>       };
>     };
> 
>     &i2c0 {
> ?? ??   tda998x: hdmi-transmitter at 60 {
> ?? ?? ??   compatible = "nxp,tda19988";
> ?? ?? ?? ?? reg = <0x60>;
>         port {
>           tda998x_1: endpoint {
>             remote-endpoint = <&dcon0_1>;
>           };
>         };
> ?? ?? ?? };
>     };

Some remarks:

- I think it is better to keep the names 'lcd' for the memory to dumb panel
  sub-devices and 'dcon' for the dumb panel to LCD/VGA sub-device, as
  named in the spec.

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

- I don't see the use of the phandles in the leaves (dcon and tda998x).

Well, here is how I feel the DTs:

- for the Cubox (one LCD, the DCON is not used, TDA998x for HDMI/DVI
  output):

	video {
		compatible = "marvell,armada-video";
		marvell,video-devices = <&lcd0>		/* only one device */
	};

	/* the LCD reg and interrupts are described in dove.dtsi */

	&lcd0 {
		status = "okay";
		clocks = <&core_clk 3>, <&lcdclk>, <&si5351 0>;
		marvell,clock-type = <0>, <2>, <3>;	/* how to set the clock */
		marvell,connector-type = <11>;		/* HDMIA */
		marvell,external-encoder = <&tda998x>;
	};

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

- 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>
	};

	/* the LCDs reg and interrupts are described in dove.dtsi */

	&lcd0 {						/* LCD */
		status = "okay";
		clocks = <&lcdclk>;			/* only the LCD clock */
		marvell,clock-type = <2>;
		marvell,connector-type = <7>;		/* LVDS */
		display-timings {
			mode {
				hactive = <1920>;
				vactive = <1080>;
				hfront-porch = <88>;
				hsync-len = <44>;
				hback-porch = <148>;
				vfront-porch = <4>;
				vsync-len = <5>;
				vback-porch = <36>;
				clock = <148500>;
			};
		};
	};
	&lcd1 {						/* VGA */
		status = "okay";
		clocks = <&core_clk 3>, <&lcdclk>;	/* only the AXI and LCD clocks */
		marvell,clock-type = <0>, <2>;		/* how to set the clock */
		marvell,port-type = <1>;		/* VGA */
			/* there may be some external encoder to handle DAC */
	};

	/* the DCON reg and interrupts are described in dove.dtsi */

	&dcon { status = "okay"; };

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: DT binding review for Armada display subsystem

[Daniel Drake]

On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org> wrote:
> - I think it is better to keep the names 'lcd' for the memory to dumb panel
>   sub-devices and 'dcon' for the dumb panel to LCD/VGA sub-device, as
>   named in the spec.

I agree it is worth keeping the spec-defined names, if they do not
cause excessive confusion when mixed with the spec-defined names for
MMP2/MMP3. I'll check the spec you pointed out, thanks.

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

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

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

Thanks,
Daniel

DT binding review for Armada display subsystem

[Daniel Drake]

On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moine <moinejf@free.fr> wrote:
> - I think it is better to keep the names 'lcd' for the memory to dumb panel
>   sub-devices and 'dcon' for the dumb panel to LCD/VGA sub-device, as
>   named in the spec.

I agree it is worth keeping the spec-defined names, if they do not
cause excessive confusion when mixed with the spec-defined names for
MMP2/MMP3. I'll check the spec you pointed out, thanks.

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

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

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

Thanks,
Daniel

Re: DT binding review for Armada display subsystem

[Jean-Francois Moine]

On Fri, 12 Jul 2013 13:00:23 -0600
Daniel Drake <dsd@laptop.org> wrote:

> On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moine <moinejf@free.fr> 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
(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.

> > - 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";
		reg = <0x820000 0x1c8>;
		interrupts = <47>;
		clocks = <&core_clk 3>, <&lcdclk>;
		clock-names = "axi", "lcdclk";
		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";
	};

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

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

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

DT binding review for Armada display subsystem

[Jean-Francois Moine]

On Fri, 12 Jul 2013 13:00:23 -0600
Daniel Drake <dsd@laptop.org> wrote:

> On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moine <moinejf@free.fr> 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
(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.

> > - 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 at 820000 {
		compatible = "marvell,dove-lcd0";
		reg = <0x820000 0x1c8>;
		interrupts = <47>;
		clocks = <&core_clk 3>, <&lcdclk>;
		clock-names = "axi", "lcdclk";
		marvell-output = <&dcon 0>;
		status = "disabled";
	};

	lcd1: lcd-controller at 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 at 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 */
	};

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

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: DT binding review for Armada display subsystem

[Sylwester Nawrocki]

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

DT binding review for Armada display subsystem

[Sylwester Nawrocki]

On 07/13/2013 10:35 AM, Jean-Francois Moine wrote:
> On Fri, 12 Jul 2013 13:00:23 -0600 Daniel Drake<dsd@laptop.org>  wrote:
>> On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moine<moinejf@free.fr>  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 at 820000 {
> 		compatible = "marvell,dove-lcd0";
[...]
> 	};
>
> 	lcd1: lcd-controller at 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 at 820000 {
  		compatible = "marvell,dove-lcd";
		...
  	};


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


> 	/* display controller and image rotation engine */
> 	dcon: display-controller at 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 at 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 at 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

Re: DT binding review for Armada display subsystem

[Russell King - ARM Linux]

On Sat, Jul 13, 2013 at 12:56:50PM +0200, Sylwester Nawrocki wrote:
> 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.

When I designed the clk API, I arranged for things like clk_get() to
take a reference on the module if the clock was supplied by a module.
You'll see some evidence of that by looking back in the git history
for arch/arm/mach-integrator/include/mach/clkdev.h.

If the common clk API has been designed without any thought to clocks
supplied by modules and making sure that in-use clocks don't go away,
then it's going to be a real pain to sort that out.  I don't think
refcounting clocks makes sense (what do you do with an enabled clock
that you then remove the module for but it's still being used?  Do you
just shut it down anyway?  Do you leave it running?  What about
subsequent calls?).

I think this is one case where taking a reference on the module supplying
it makes total sense.

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

Indeed it does.  Just because one TV works with such a wrong clock does not
mean that all TVs and HDMI compatible monitors will do.  It's 11% out.

The reason that audio works is because of the way the HDMI transmitter works
- it can calculate the CTS value to send (by measuring it) and it sends that
value over the link so the TV can regenerate the correct audio clock from
the HDMI clock.

Whether that's going to be universally true, I don't know - it depends on
how much audio data gets sent via each frame.  As the HDMI clock is slower,
there would need to be more audio data sent.

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

They aren't.  They're 100% identical in the Armada 510.

> 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";

I'd much prefer marvell,armada-510-lcd rather than using the codenames for
the devices.  Otherwise we're going to run into totally different things
being used for different devices (eg, armada-xp...)

DT binding review for Armada display subsystem

[Russell King - ARM Linux]

On Sat, Jul 13, 2013 at 12:56:50PM +0200, Sylwester Nawrocki wrote:
> On 07/13/2013 10:35 AM, Jean-Francois Moine wrote:
>> On Fri, 12 Jul 2013 13:00:23 -0600 Daniel Drake<dsd@laptop.org>  wrote:
>>> On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moine<moinejf@free.fr>  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.

When I designed the clk API, I arranged for things like clk_get() to
take a reference on the module if the clock was supplied by a module.
You'll see some evidence of that by looking back in the git history
for arch/arm/mach-integrator/include/mach/clkdev.h.

If the common clk API has been designed without any thought to clocks
supplied by modules and making sure that in-use clocks don't go away,
then it's going to be a real pain to sort that out.  I don't think
refcounting clocks makes sense (what do you do with an enabled clock
that you then remove the module for but it's still being used?  Do you
just shut it down anyway?  Do you leave it running?  What about
subsequent calls?).

I think this is one case where taking a reference on the module supplying
it makes total sense.

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

Indeed it does.  Just because one TV works with such a wrong clock does not
mean that all TVs and HDMI compatible monitors will do.  It's 11% out.

The reason that audio works is because of the way the HDMI transmitter works
- it can calculate the CTS value to send (by measuring it) and it sends that
value over the link so the TV can regenerate the correct audio clock from
the HDMI clock.

Whether that's going to be universally true, I don't know - it depends on
how much audio data gets sent via each frame.  As the HDMI clock is slower,
there would need to be more audio data sent.

>> 	lcd0: lcd-controller at 820000 {
>> 		compatible = "marvell,dove-lcd0";
> [...]
>> 	};
>>
>> 	lcd1: lcd-controller at 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

They aren't.  They're 100% identical in the Armada 510.

> 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 at 820000 {
>  		compatible = "marvell,dove-lcd";

I'd much prefer marvell,armada-510-lcd rather than using the codenames for
the devices.  Otherwise we're going to run into totally different things
being used for different devices (eg, armada-xp...)

Re: DT binding review for Armada display subsystem

[Daniel Drake]

On Sat, Jul 13, 2013 at 2:35 AM, Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org> 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.

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

>         lcd0: lcd-controller@820000 {
>                 compatible = "marvell,dove-lcd0";
>                 reg = <0x820000 0x1c8>;
>                 interrupts = <47>;
>                 clocks = <&core_clk 3>, <&lcdclk>;
>                 clock-names = "axi", "lcdclk";
>                 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).

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

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

Daniel

DT binding review for Armada display subsystem

[Daniel Drake]

On Sat, Jul 13, 2013 at 2:35 AM, Jean-Francois Moine <moinejf@free.fr> 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.

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

>         lcd0: lcd-controller at 820000 {
>                 compatible = "marvell,dove-lcd0";
>                 reg = <0x820000 0x1c8>;
>                 interrupts = <47>;
>                 clocks = <&core_clk 3>, <&lcdclk>;
>                 clock-names = "axi", "lcdclk";
>                 marvell-output = <&dcon 0>;
>                 status = "disabled";
>         };
>
>         lcd1: lcd-controller at 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 at 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).

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

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

Daniel

Re: DT binding review for Armada display subsystem

[Sebastian Hesselbarth]

On 07/13/2013 04:25 PM, Daniel Drake wrote:
> On Sat, Jul 13, 2013 at 2:35 AM, Jean-Francois Moine <moinejf@free.fr> 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

DT binding review for Armada display subsystem

[Sebastian Hesselbarth]

On 07/13/2013 04:25 PM, Daniel Drake wrote:
> On Sat, Jul 13, 2013 at 2:35 AM, Jean-Francois Moine <moinejf@free.fr> 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 at 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 at 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 at 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

Re: DT binding review for Armada display subsystem

[Russell King - ARM Linux]

On Sat, Jul 13, 2013 at 08:25:15AM -0600, Daniel Drake wrote:
> 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.

I don't see much need at the moment for IRE.  IRE isn't going to be
useful for general graphics rotation as it only supports up to 16bpp
RGB.

It looks to me like this module was designed to rotate YUV/camera
images.  If we wanted to rotate a video image, then it would probably
be more efficient to use this, but it will cause a conversion of the
image format in doing so.

DT binding review for Armada display subsystem

[Russell King - ARM Linux]

On Sat, Jul 13, 2013 at 08:25:15AM -0600, Daniel Drake wrote:
> 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.

I don't see much need at the moment for IRE.  IRE isn't going to be
useful for general graphics rotation as it only supports up to 16bpp
RGB.

It looks to me like this module was designed to rotate YUV/camera
images.  If we wanted to rotate a video image, then it would probably
be more efficient to use this, but it will cause a conversion of the
image format in doing so.

Re: DT binding review for Armada display subsystem

[Sebastian Hesselbarth]

On 07/13/2013 01:12 PM, Russell King - ARM Linux wrote:
> On Sat, Jul 13, 2013 at 12:56:50PM +0200, Sylwester Nawrocki wrote:
>> 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.
>
> When I designed the clk API, I arranged for things like clk_get() to
> take a reference on the module if the clock was supplied by a module.
> You'll see some evidence of that by looking back in the git history
> for arch/arm/mach-integrator/include/mach/clkdev.h.

Last time I checked, clkdev API neither implements referencing nor
unregister. This is on Mike's list and IIRC there already has been
a proposal for unregister. Si5351 was the first clkdev driver ever
that could possibly be unloaded, so there may be still some features
missing.

> If the common clk API has been designed without any thought to clocks
> supplied by modules and making sure that in-use clocks don't go away,
> then it's going to be a real pain to sort that out.  I don't think
> refcounting clocks makes sense (what do you do with an enabled clock
> that you then remove the module for but it's still being used?  Do you
> just shut it down anyway?  Do you leave it running?  What about
> subsequent calls?).

Good point, first I thought always disable on last user dropping it but
that may most likely break some platforms. Second thought, get back to
the state when the driver was loaded.

> I think this is one case where taking a reference on the module supplying
> it makes total sense.
>
>>> (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 ?
>
> Indeed it does.  Just because one TV works with such a wrong clock does not
> mean that all TVs and HDMI compatible monitors will do.  It's 11% out.
>
> The reason that audio works is because of the way the HDMI transmitter works
> - it can calculate the CTS value to send (by measuring it) and it sends that
> value over the link so the TV can regenerate the correct audio clock from
> the HDMI clock.

True, wrong clock will most likely not work on all monitors or TV. My
impression for TVs is that the the "cheaper" the brand is, the more
likely they accept any mode/clock combination ;)

> Whether that's going to be universally true, I don't know - it depends on
> how much audio data gets sent via each frame.  As the HDMI clock is slower,
> there would need to be more audio data sent.

Also: audio/video clock relation is sent in AVI packets over HDMI.
Picking a clock with 11% off may not even allow you to send (valid) CTS
information.

>>> 	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
>
> They aren't.  They're 100% identical in the Armada 510.
>
>> 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";
>
> I'd much prefer marvell,armada-510-lcd rather than using the codenames for
> the devices.  Otherwise we're going to run into totally different things
> being used for different devices (eg, armada-xp...)
>

+1 for using marvell,armada-510-lcd. I like the nick-name "dove" but
armada-510 is much easier to find on Marvell's website.

Sebastian

DT binding review for Armada display subsystem

[Sebastian Hesselbarth]

On 07/13/2013 01:12 PM, Russell King - ARM Linux wrote:
> On Sat, Jul 13, 2013 at 12:56:50PM +0200, Sylwester Nawrocki wrote:
>> On 07/13/2013 10:35 AM, Jean-Francois Moine wrote:
>>> On Fri, 12 Jul 2013 13:00:23 -0600 Daniel Drake<dsd@laptop.org>  wrote:
>>>> On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moine<moinejf@free.fr>  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.
>
> When I designed the clk API, I arranged for things like clk_get() to
> take a reference on the module if the clock was supplied by a module.
> You'll see some evidence of that by looking back in the git history
> for arch/arm/mach-integrator/include/mach/clkdev.h.

Last time I checked, clkdev API neither implements referencing nor
unregister. This is on Mike's list and IIRC there already has been
a proposal for unregister. Si5351 was the first clkdev driver ever
that could possibly be unloaded, so there may be still some features
missing.

> If the common clk API has been designed without any thought to clocks
> supplied by modules and making sure that in-use clocks don't go away,
> then it's going to be a real pain to sort that out.  I don't think
> refcounting clocks makes sense (what do you do with an enabled clock
> that you then remove the module for but it's still being used?  Do you
> just shut it down anyway?  Do you leave it running?  What about
> subsequent calls?).

Good point, first I thought always disable on last user dropping it but
that may most likely break some platforms. Second thought, get back to
the state when the driver was loaded.

> I think this is one case where taking a reference on the module supplying
> it makes total sense.
>
>>> (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 ?
>
> Indeed it does.  Just because one TV works with such a wrong clock does not
> mean that all TVs and HDMI compatible monitors will do.  It's 11% out.
>
> The reason that audio works is because of the way the HDMI transmitter works
> - it can calculate the CTS value to send (by measuring it) and it sends that
> value over the link so the TV can regenerate the correct audio clock from
> the HDMI clock.

True, wrong clock will most likely not work on all monitors or TV. My
impression for TVs is that the the "cheaper" the brand is, the more
likely they accept any mode/clock combination ;)

> Whether that's going to be universally true, I don't know - it depends on
> how much audio data gets sent via each frame.  As the HDMI clock is slower,
> there would need to be more audio data sent.

Also: audio/video clock relation is sent in AVI packets over HDMI.
Picking a clock with 11% off may not even allow you to send (valid) CTS
information.

>>> 	lcd0: lcd-controller at 820000 {
>>> 		compatible = "marvell,dove-lcd0";
>> [...]
>>> 	};
>>>
>>> 	lcd1: lcd-controller at 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
>
> They aren't.  They're 100% identical in the Armada 510.
>
>> 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 at 820000 {
>>   		compatible = "marvell,dove-lcd";
>
> I'd much prefer marvell,armada-510-lcd rather than using the codenames for
> the devices.  Otherwise we're going to run into totally different things
> being used for different devices (eg, armada-xp...)
>

+1 for using marvell,armada-510-lcd. I like the nick-name "dove" but
armada-510 is much easier to find on Marvell's website.

Sebastian

Re: DT binding review for Armada display subsystem

[Sylwester Nawrocki]

On 07/13/2013 07:44 PM, Sebastian Hesselbarth wrote:
> On 07/13/2013 01:12 PM, Russell King - ARM Linux wrote:
>> On Sat, Jul 13, 2013 at 12:56:50PM +0200, Sylwester Nawrocki wrote:
>>> 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:
[...]
>>>> 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.
>>
>> When I designed the clk API, I arranged for things like clk_get() to
>> take a reference on the module if the clock was supplied by a module.
>> You'll see some evidence of that by looking back in the git history
>> for arch/arm/mach-integrator/include/mach/clkdev.h.
>
> Last time I checked, clkdev API neither implements referencing nor
> unregister. This is on Mike's list and IIRC there already has been
> a proposal for unregister. Si5351 was the first clkdev driver ever

I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it seems
they're working on v4 with clock object reference counting. Presumably we
need both clk_get() to be taking reference on the module and reference
counted clk free, e.g. in cases where clock provider is a hot-pluggable
device. It might be too paranoid though, I haven't seen hardware
configurations where a clock source could be unplugged safely when whole
system is running.

> that could possibly be unloaded, so there may be still some features
> missing.

I guess there should be a warning in e.g. Kconfig saying that it is safe
to use this driver only with CONFIG_MODULE_UNLOAD or something similar.
Otherwise users might not be happy and might start filling bug reports. :)

>> If the common clk API has been designed without any thought to clocks
>> supplied by modules and making sure that in-use clocks don't go away,
>> then it's going to be a real pain to sort that out. I don't think
>> refcounting clocks makes sense (what do you do with an enabled clock
>> that you then remove the module for but it's still being used? Do you
>> just shut it down anyway? Do you leave it running? What about
>> subsequent calls?).
>
> Good point, first I thought always disable on last user dropping it but
> that may most likely break some platforms. Second thought, get back to
> the state when the driver was loaded.

When last user drops the reference it could be already too late to change
anything in hardware, since the clock provider module is already unloaded.

Anyway I suppose it's best to take reference on the clk provider module
in clk_get(). This can easily lead to circular dependencies though, when
e.g. a module A is a clock consumer of a clock provided by module B, and
module B calls back into module A, so it also takes reference on A.
The only solution I could think of is to call clk_get() only before a clock
is enabled and clk_put() right after it is disabled and stops being used.

>> I think this is one case where taking a reference on the module supplying
>> it makes total sense.
>>
>>>> (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 ?
>>
>> Indeed it does. Just because one TV works with such a wrong clock does
>> not
>> mean that all TVs and HDMI compatible monitors will do. It's 11% out.
>>
>> The reason that audio works is because of the way the HDMI transmitter
>> works
>> - it can calculate the CTS value to send (by measuring it) and it
>> sends that
>> value over the link so the TV can regenerate the correct audio clock from
>> the HDMI clock.
>
> True, wrong clock will most likely not work on all monitors or TV. My
> impression for TVs is that the the "cheaper" the brand is, the more
> likely they accept any mode/clock combination ;)
>
>> Whether that's going to be universally true, I don't know - it depends on
>> how much audio data gets sent via each frame. As the HDMI clock is
>> slower,
>> there would need to be more audio data sent.
>
> Also: audio/video clock relation is sent in AVI packets over HDMI.
> Picking a clock with 11% off may not even allow you to send (valid) CTS
> information.
>
>>>> 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
>>
>> They aren't. They're 100% identical in the Armada 510.
>>
>>> 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";
>>
>> I'd much prefer marvell,armada-510-lcd rather than using the codenames
>> for
>> the devices. Otherwise we're going to run into totally different things
>> being used for different devices (eg, armada-xp...)
>>
>
> +1 for using marvell,armada-510-lcd. I like the nick-name "dove" but
> armada-510 is much easier to find on Marvell's website.

Indeed, I'm not much into Marvell SoCs details but "marvell,armada-510-lcd"
seems more clear.

--
Thanks,
Sylwester

DT binding review for Armada display subsystem

[Sylwester Nawrocki]

On 07/13/2013 07:44 PM, Sebastian Hesselbarth wrote:
> On 07/13/2013 01:12 PM, Russell King - ARM Linux wrote:
>> On Sat, Jul 13, 2013 at 12:56:50PM +0200, Sylwester Nawrocki wrote:
>>> On 07/13/2013 10:35 AM, Jean-Francois Moine wrote:
>>>> On Fri, 12 Jul 2013 13:00:23 -0600 Daniel Drake<dsd@laptop.org> wrote:
>>>>> On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois
>>>>> Moine<moinejf@free.fr> 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
>>>
>>> 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.
>>
>> When I designed the clk API, I arranged for things like clk_get() to
>> take a reference on the module if the clock was supplied by a module.
>> You'll see some evidence of that by looking back in the git history
>> for arch/arm/mach-integrator/include/mach/clkdev.h.
>
> Last time I checked, clkdev API neither implements referencing nor
> unregister. This is on Mike's list and IIRC there already has been
> a proposal for unregister. Si5351 was the first clkdev driver ever

I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it seems
they're working on v4 with clock object reference counting. Presumably we
need both clk_get() to be taking reference on the module and reference
counted clk free, e.g. in cases where clock provider is a hot-pluggable
device. It might be too paranoid though, I haven't seen hardware
configurations where a clock source could be unplugged safely when whole
system is running.

> that could possibly be unloaded, so there may be still some features
> missing.

I guess there should be a warning in e.g. Kconfig saying that it is safe
to use this driver only with CONFIG_MODULE_UNLOAD or something similar.
Otherwise users might not be happy and might start filling bug reports. :)

>> If the common clk API has been designed without any thought to clocks
>> supplied by modules and making sure that in-use clocks don't go away,
>> then it's going to be a real pain to sort that out. I don't think
>> refcounting clocks makes sense (what do you do with an enabled clock
>> that you then remove the module for but it's still being used? Do you
>> just shut it down anyway? Do you leave it running? What about
>> subsequent calls?).
>
> Good point, first I thought always disable on last user dropping it but
> that may most likely break some platforms. Second thought, get back to
> the state when the driver was loaded.

When last user drops the reference it could be already too late to change
anything in hardware, since the clock provider module is already unloaded.

Anyway I suppose it's best to take reference on the clk provider module
in clk_get(). This can easily lead to circular dependencies though, when
e.g. a module A is a clock consumer of a clock provided by module B, and
module B calls back into module A, so it also takes reference on A.
The only solution I could think of is to call clk_get() only before a clock
is enabled and clk_put() right after it is disabled and stops being used.

>> I think this is one case where taking a reference on the module supplying
>> it makes total sense.
>>
>>>> (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 ?
>>
>> Indeed it does. Just because one TV works with such a wrong clock does
>> not
>> mean that all TVs and HDMI compatible monitors will do. It's 11% out.
>>
>> The reason that audio works is because of the way the HDMI transmitter
>> works
>> - it can calculate the CTS value to send (by measuring it) and it
>> sends that
>> value over the link so the TV can regenerate the correct audio clock from
>> the HDMI clock.
>
> True, wrong clock will most likely not work on all monitors or TV. My
> impression for TVs is that the the "cheaper" the brand is, the more
> likely they accept any mode/clock combination ;)
>
>> Whether that's going to be universally true, I don't know - it depends on
>> how much audio data gets sent via each frame. As the HDMI clock is
>> slower,
>> there would need to be more audio data sent.
>
> Also: audio/video clock relation is sent in AVI packets over HDMI.
> Picking a clock with 11% off may not even allow you to send (valid) CTS
> information.
>
>>>> lcd0: lcd-controller at 820000 {
>>>> compatible = "marvell,dove-lcd0";
>>> [...]
>>>> };
>>>>
>>>> lcd1: lcd-controller at 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
>>
>> They aren't. They're 100% identical in the Armada 510.
>>
>>> 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 at 820000 {
>>> compatible = "marvell,dove-lcd";
>>
>> I'd much prefer marvell,armada-510-lcd rather than using the codenames
>> for
>> the devices. Otherwise we're going to run into totally different things
>> being used for different devices (eg, armada-xp...)
>>
>
> +1 for using marvell,armada-510-lcd. I like the nick-name "dove" but
> armada-510 is much easier to find on Marvell's website.

Indeed, I'm not much into Marvell SoCs details but "marvell,armada-510-lcd"
seems more clear.

--
Thanks,
Sylwester

Re: DT binding review for Armada display subsystem

[Russell King - ARM Linux]

On Sat, Jul 13, 2013 at 07:44:58PM +0200, Sebastian Hesselbarth wrote:
> On 07/13/2013 01:12 PM, Russell King - ARM Linux wrote:
>> When I designed the clk API, I arranged for things like clk_get() to
>> take a reference on the module if the clock was supplied by a module.
>> You'll see some evidence of that by looking back in the git history
>> for arch/arm/mach-integrator/include/mach/clkdev.h.
>
> Last time I checked, clkdev API neither implements referencing nor
> unregister.

*Sigh*.

a613163dff04cbfcb7d66b06ef4a5f65498ee59b:
arch/arm/mach-integrator/include/mach/clkdev.h:
-struct clk {
-       unsigned long           rate;
-       const struct clk_ops    *ops;
-       struct module           *owner;
-       const struct icst_params *params;
-       void __iomem            *vcoreg;
-       void                    *data;
-};
-
-static inline int __clk_get(struct clk *clk)
-{
-       return try_module_get(clk->owner);
-}
-
-static inline void __clk_put(struct clk *clk)
-{
-       module_put(clk->owner);
-}

70ee65771424829fd092a1df9afcc7e24c94004b:
arch/arm/mach-integrator/impd1.c:
 static int impd1_probe(struct lm_device *dev)
...
-       for (i = 0; i < ARRAY_SIZE(impd1->vcos); i++) {
-               impd1->vcos[i].ops = &impd1_clk_ops,
-               impd1->vcos[i].owner = THIS_MODULE,
-               impd1->vcos[i].params = &impd1_vco_params,
-               impd1->vcos[i].data = impd1;
-       }
-       impd1->vcos[0].vcoreg = impd1->base + IMPD1_OSC1;
-       impd1->vcos[1].vcoreg = impd1->base + IMPD1_OSC2;
-
-       impd1->clks[0] = clkdev_alloc(&impd1->vcos[0], NULL, "lm%x:01000",
-                                       dev->id);
-       impd1->clks[1] = clkdev_alloc(&fixed_14745600, NULL, "lm%x:00100",
-                                       dev->id);
-       impd1->clks[2] = clkdev_alloc(&fixed_14745600, NULL, "lm%x:00200",
-                                       dev->id);
-       for (i = 0; i < ARRAY_SIZE(impd1->clks); i++)
-               clkdev_add(impd1->clks[i]);
...
 static void impd1_remove(struct lm_device *dev)
...
-       for (i = 0; i < ARRAY_SIZE(impd1->clks); i++)
-               clkdev_drop(impd1->clks[i]);

drivers/clk/clkdev.c:
/*
 * clkdev_drop - remove a clock dynamically allocated
 */
void clkdev_drop(struct clk_lookup *cl)
{
        mutex_lock(&clocks_mutex);
        list_del(&cl->node);
        mutex_unlock(&clocks_mutex);
        kfree(cl);
}
EXPORT_SYMBOL(clkdev_drop);

No, of course, I'm imagining all the above!

Now, today, the IM-PD/1 support got broken in respect of ensuring that
things are properly refcounted in the rush to convert things to this
wonderful new common clk API - because it's oh soooooo much better.  Yes,
right, I'd forgotten the number one rule of kernel programming - always
sacrifice correctness when we get a new fantasic hip idea!  Silly me.

> This is on Mike's list and IIRC there already has been
> a proposal for unregister. Si5351 was the first clkdev driver ever
> that could possibly be unloaded, so there may be still some features
> missing.

Utter rubbish - it's not the first as you can see from the above.
Integrator had this support since the clk and clkdev APIs came along,
because the IM-PD/1 module was implemented as a module, and it has to
create and register clocks for its peripherals.

What you've found is a backwards step with the common clk API, nothing
more.

DT binding review for Armada display subsystem

[Russell King - ARM Linux]

On Sat, Jul 13, 2013 at 07:44:58PM +0200, Sebastian Hesselbarth wrote:
> On 07/13/2013 01:12 PM, Russell King - ARM Linux wrote:
>> When I designed the clk API, I arranged for things like clk_get() to
>> take a reference on the module if the clock was supplied by a module.
>> You'll see some evidence of that by looking back in the git history
>> for arch/arm/mach-integrator/include/mach/clkdev.h.
>
> Last time I checked, clkdev API neither implements referencing nor
> unregister.

*Sigh*.

a613163dff04cbfcb7d66b06ef4a5f65498ee59b:
arch/arm/mach-integrator/include/mach/clkdev.h:
-struct clk {
-       unsigned long           rate;
-       const struct clk_ops    *ops;
-       struct module           *owner;
-       const struct icst_params *params;
-       void __iomem            *vcoreg;
-       void                    *data;
-};
-
-static inline int __clk_get(struct clk *clk)
-{
-       return try_module_get(clk->owner);
-}
-
-static inline void __clk_put(struct clk *clk)
-{
-       module_put(clk->owner);
-}

70ee65771424829fd092a1df9afcc7e24c94004b:
arch/arm/mach-integrator/impd1.c:
 static int impd1_probe(struct lm_device *dev)
...
-       for (i = 0; i < ARRAY_SIZE(impd1->vcos); i++) {
-               impd1->vcos[i].ops = &impd1_clk_ops,
-               impd1->vcos[i].owner = THIS_MODULE,
-               impd1->vcos[i].params = &impd1_vco_params,
-               impd1->vcos[i].data = impd1;
-       }
-       impd1->vcos[0].vcoreg = impd1->base + IMPD1_OSC1;
-       impd1->vcos[1].vcoreg = impd1->base + IMPD1_OSC2;
-
-       impd1->clks[0] = clkdev_alloc(&impd1->vcos[0], NULL, "lm%x:01000",
-                                       dev->id);
-       impd1->clks[1] = clkdev_alloc(&fixed_14745600, NULL, "lm%x:00100",
-                                       dev->id);
-       impd1->clks[2] = clkdev_alloc(&fixed_14745600, NULL, "lm%x:00200",
-                                       dev->id);
-       for (i = 0; i < ARRAY_SIZE(impd1->clks); i++)
-               clkdev_add(impd1->clks[i]);
...
 static void impd1_remove(struct lm_device *dev)
...
-       for (i = 0; i < ARRAY_SIZE(impd1->clks); i++)
-               clkdev_drop(impd1->clks[i]);

drivers/clk/clkdev.c:
/*
 * clkdev_drop - remove a clock dynamically allocated
 */
void clkdev_drop(struct clk_lookup *cl)
{
        mutex_lock(&clocks_mutex);
        list_del(&cl->node);
        mutex_unlock(&clocks_mutex);
        kfree(cl);
}
EXPORT_SYMBOL(clkdev_drop);

No, of course, I'm imagining all the above!

Now, today, the IM-PD/1 support got broken in respect of ensuring that
things are properly refcounted in the rush to convert things to this
wonderful new common clk API - because it's oh soooooo much better.  Yes,
right, I'd forgotten the number one rule of kernel programming - always
sacrifice correctness when we get a new fantasic hip idea!  Silly me.

> This is on Mike's list and IIRC there already has been
> a proposal for unregister. Si5351 was the first clkdev driver ever
> that could possibly be unloaded, so there may be still some features
> missing.

Utter rubbish - it's not the first as you can see from the above.
Integrator had this support since the clk and clkdev APIs came along,
because the IM-PD/1 module was implemented as a module, and it has to
create and register clocks for its peripherals.

What you've found is a backwards step with the common clk API, nothing
more.

Re: DT binding review for Armada display subsystem

[Russell King - ARM Linux]

On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:
> I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it seems
> they're working on v4 with clock object reference counting. Presumably we
> need both clk_get() to be taking reference on the module and reference
> counted clk free, e.g. in cases where clock provider is a hot-pluggable
> device. It might be too paranoid though, I haven't seen hardware
> configurations where a clock source could be unplugged safely when whole
> system is running.

I'm not going to accept refcounting being thrown into clk_get().  The
clkdev API already has refcounting, as much as it needs to.  It just
needs people to use the hooks that I provided back in 2008 when I
created the clkdev API for doing _precisely_ this job.

Have a read through these commits, which backup my statement above:

0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the clkdev API
d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting Integrator to clkdev API

and it will show you how to do refcounting.  The common clk API just
needs to stop defining __clk_get() and __clk_put() to be an empty
function and implement them appropriately for it's clk implementation,
like they were always meant to be.

__clk_get() and __clk_put() are the clk-implementation specific parts
of the clkdev API, because the clkdev API is utterly divorsed from the
internals of what a 'struct clk' actually is.  clkdev just treats a
'struct clk' as a completely opaque type and never bothers poking
about inside it.

DT binding review for Armada display subsystem

[Russell King - ARM Linux]

On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:
> I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it seems
> they're working on v4 with clock object reference counting. Presumably we
> need both clk_get() to be taking reference on the module and reference
> counted clk free, e.g. in cases where clock provider is a hot-pluggable
> device. It might be too paranoid though, I haven't seen hardware
> configurations where a clock source could be unplugged safely when whole
> system is running.

I'm not going to accept refcounting being thrown into clk_get().  The
clkdev API already has refcounting, as much as it needs to.  It just
needs people to use the hooks that I provided back in 2008 when I
created the clkdev API for doing _precisely_ this job.

Have a read through these commits, which backup my statement above:

0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the clkdev API
d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting Integrator to clkdev API

and it will show you how to do refcounting.  The common clk API just
needs to stop defining __clk_get() and __clk_put() to be an empty
function and implement them appropriately for it's clk implementation,
like they were always meant to be.

__clk_get() and __clk_put() are the clk-implementation specific parts
of the clkdev API, because the clkdev API is utterly divorsed from the
internals of what a 'struct clk' actually is.  clkdev just treats a
'struct clk' as a completely opaque type and never bothers poking
about inside it.

Re: DT binding review for Armada display subsystem

[Sylwester Nawrocki]

On 07/13/2013 11:02 PM, Russell King - ARM Linux wrote:
> On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:
>> I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it seems
>> they're working on v4 with clock object reference counting. Presumably we
>> need both clk_get() to be taking reference on the module and reference
>> counted clk free, e.g. in cases where clock provider is a hot-pluggable
>> device. It might be too paranoid though, I haven't seen hardware
>> configurations where a clock source could be unplugged safely when whole
>> system is running.
>
> I'm not going to accept refcounting being thrown into clk_get().  The
> clkdev API already has refcounting, as much as it needs to.  It just
> needs people to use the hooks that I provided back in 2008 when I
> created the clkdev API for doing _precisely_ this job.
>
> Have a read through these commits, which backup my statement above:
>
> 0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the clkdev API
> d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting Integrator to clkdev API
>
> and it will show you how to do refcounting.  The common clk API just
> needs to stop defining __clk_get() and __clk_put() to be an empty
> function and implement them appropriately for it's clk implementation,
> like they were always meant to be.

Sure, I've already seen the above commits. This is how I understood it
as well - __clk_get(), __clk_put() need to be defined by the common clk
API, since it is going to replace all the arch specific implementations.

> __clk_get() and __clk_put() are the clk-implementation specific parts
> of the clkdev API, because the clkdev API is utterly divorsed from the
> internals of what a 'struct clk' actually is.  clkdev just treats a
> 'struct clk' as a completely opaque type and never bothers poking
> about inside it.

OK, but at the clock's implementation side we may need, e.g. to use kref
to keep track of the clock's state, and free it only when it is unprepared,
all its children are unregistered, etc. ? Is it not what you stated in
your comment to patch [1] ?

[1] https://patchwork.kernel.org/patch/2651171/

DT binding review for Armada display subsystem

[Sylwester Nawrocki]

On 07/13/2013 11:02 PM, Russell King - ARM Linux wrote:
> On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:
>> I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it seems
>> they're working on v4 with clock object reference counting. Presumably we
>> need both clk_get() to be taking reference on the module and reference
>> counted clk free, e.g. in cases where clock provider is a hot-pluggable
>> device. It might be too paranoid though, I haven't seen hardware
>> configurations where a clock source could be unplugged safely when whole
>> system is running.
>
> I'm not going to accept refcounting being thrown into clk_get().  The
> clkdev API already has refcounting, as much as it needs to.  It just
> needs people to use the hooks that I provided back in 2008 when I
> created the clkdev API for doing _precisely_ this job.
>
> Have a read through these commits, which backup my statement above:
>
> 0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the clkdev API
> d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting Integrator to clkdev API
>
> and it will show you how to do refcounting.  The common clk API just
> needs to stop defining __clk_get() and __clk_put() to be an empty
> function and implement them appropriately for it's clk implementation,
> like they were always meant to be.

Sure, I've already seen the above commits. This is how I understood it
as well - __clk_get(), __clk_put() need to be defined by the common clk
API, since it is going to replace all the arch specific implementations.

> __clk_get() and __clk_put() are the clk-implementation specific parts
> of the clkdev API, because the clkdev API is utterly divorsed from the
> internals of what a 'struct clk' actually is.  clkdev just treats a
> 'struct clk' as a completely opaque type and never bothers poking
> about inside it.

OK, but at the clock's implementation side we may need, e.g. to use kref
to keep track of the clock's state, and free it only when it is unprepared,
all its children are unregistered, etc. ? Is it not what you stated in
your comment to patch [1] ?

[1] https://patchwork.kernel.org/patch/2651171/

Re: DT binding review for Armada display subsystem

[Russell King - ARM Linux]

On Sun, Jul 14, 2013 at 12:16:58AM +0200, Sylwester Nawrocki wrote:
> On 07/13/2013 11:02 PM, Russell King - ARM Linux wrote:
>> On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:
>>> I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it seems
>>> they're working on v4 with clock object reference counting. Presumably we
>>> need both clk_get() to be taking reference on the module and reference
>>> counted clk free, e.g. in cases where clock provider is a hot-pluggable
>>> device. It might be too paranoid though, I haven't seen hardware
>>> configurations where a clock source could be unplugged safely when whole
>>> system is running.
>>
>> I'm not going to accept refcounting being thrown into clk_get().  The
>> clkdev API already has refcounting, as much as it needs to.  It just
>> needs people to use the hooks that I provided back in 2008 when I
>> created the clkdev API for doing _precisely_ this job.
>>
>> Have a read through these commits, which backup my statement above:
>>
>> 0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the clkdev API
>> d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting Integrator to clkdev API
>>
>> and it will show you how to do refcounting.  The common clk API just
>> needs to stop defining __clk_get() and __clk_put() to be an empty
>> function and implement them appropriately for it's clk implementation,
>> like they were always meant to be.
>
> Sure, I've already seen the above commits. This is how I understood it
> as well - __clk_get(), __clk_put() need to be defined by the common clk
> API, since it is going to replace all the arch specific implementations.
>
>> __clk_get() and __clk_put() are the clk-implementation specific parts
>> of the clkdev API, because the clkdev API is utterly divorsed from the
>> internals of what a 'struct clk' actually is.  clkdev just treats a
>> 'struct clk' as a completely opaque type and never bothers poking
>> about inside it.
>
> OK, but at the clock's implementation side we may need, e.g. to use kref
> to keep track of the clock's state, and free it only when it is unprepared,
> all its children are unregistered, etc. ? Is it not what you stated in
> your comment to patch [1] ?

If you want to do refcounting on a clock (which you run into problems
as I highlighted earlier in this thread) then yes, you need to use a
kref, and take a reference in __clk_get() and drop it in __clk_put()
to make things safe.

Whether you also take a reference on the module supplying the clock or
not depends whether you need to keep the code around to manipulate that
clock while there are users of it.

As I've already said - consider the possibilities with this scenario.
Here's one:

  A clock consumer is using a clock, but the clock supplier has been
  removed.  The clock consumer wants to change the state of the clock
  in some way - eg, system is suspending.  clk_disable() doesn't fail,
  but on resume, clk_enable() does... (and how many people assume that
  clk_enable() never fails?)  And who knows what rate the clock is now
  producing or whether it's even producing anything...

I'm not saying don't do the refcounting thing I mentioned back in June.
I'm merely pointing out the issues that there are.  There isn't a one
right answer here because each has their own advantages and disadvantages
(and problems.)

DT binding review for Armada display subsystem

[Russell King - ARM Linux]

On Sun, Jul 14, 2013 at 12:16:58AM +0200, Sylwester Nawrocki wrote:
> On 07/13/2013 11:02 PM, Russell King - ARM Linux wrote:
>> On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:
>>> I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it seems
>>> they're working on v4 with clock object reference counting. Presumably we
>>> need both clk_get() to be taking reference on the module and reference
>>> counted clk free, e.g. in cases where clock provider is a hot-pluggable
>>> device. It might be too paranoid though, I haven't seen hardware
>>> configurations where a clock source could be unplugged safely when whole
>>> system is running.
>>
>> I'm not going to accept refcounting being thrown into clk_get().  The
>> clkdev API already has refcounting, as much as it needs to.  It just
>> needs people to use the hooks that I provided back in 2008 when I
>> created the clkdev API for doing _precisely_ this job.
>>
>> Have a read through these commits, which backup my statement above:
>>
>> 0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the clkdev API
>> d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting Integrator to clkdev API
>>
>> and it will show you how to do refcounting.  The common clk API just
>> needs to stop defining __clk_get() and __clk_put() to be an empty
>> function and implement them appropriately for it's clk implementation,
>> like they were always meant to be.
>
> Sure, I've already seen the above commits. This is how I understood it
> as well - __clk_get(), __clk_put() need to be defined by the common clk
> API, since it is going to replace all the arch specific implementations.
>
>> __clk_get() and __clk_put() are the clk-implementation specific parts
>> of the clkdev API, because the clkdev API is utterly divorsed from the
>> internals of what a 'struct clk' actually is.  clkdev just treats a
>> 'struct clk' as a completely opaque type and never bothers poking
>> about inside it.
>
> OK, but at the clock's implementation side we may need, e.g. to use kref
> to keep track of the clock's state, and free it only when it is unprepared,
> all its children are unregistered, etc. ? Is it not what you stated in
> your comment to patch [1] ?

If you want to do refcounting on a clock (which you run into problems
as I highlighted earlier in this thread) then yes, you need to use a
kref, and take a reference in __clk_get() and drop it in __clk_put()
to make things safe.

Whether you also take a reference on the module supplying the clock or
not depends whether you need to keep the code around to manipulate that
clock while there are users of it.

As I've already said - consider the possibilities with this scenario.
Here's one:

  A clock consumer is using a clock, but the clock supplier has been
  removed.  The clock consumer wants to change the state of the clock
  in some way - eg, system is suspending.  clk_disable() doesn't fail,
  but on resume, clk_enable() does... (and how many people assume that
  clk_enable() never fails?)  And who knows what rate the clock is now
  producing or whether it's even producing anything...

I'm not saying don't do the refcounting thing I mentioned back in June.
I'm merely pointing out the issues that there are.  There isn't a one
right answer here because each has their own advantages and disadvantages
(and problems.)

Re: DT binding review for Armada display subsystem

[Daniel Drake]

On Fri, Jul 12, 2013 at 10:44 AM, Daniel Drake <dsd-2X9k7bc8m7Mdnm+yROfE0A@public.gmane.org> wrote:
> Based on the outcomes of the "Best practice device tree design for display
> subsystems" discussion I have drafted a DT binding. Comments much appreciated.
>
> At a high level, it uses a "super node" as something for the driver to bind
> to, needed because there is no clear one device that controls all the
> others, and also to distinguish between the Armada 510 possible use cases
> of having one video card with two outputs, or two independent video cards.
> It uses node-to-node linking beyond that point, V4L2 style.

As this hasn't been shot down very far, I've started to implement the
driver side of this binding. I have already run into a couple of
little problems.

First, interrupts. In the dt binding, each "lcd controller" node
defines which interrupt it listens to, and in the armada 510 case
there are indeed 2 interrupts (47 and 46, one for each LCD
controller). And remember that the drm driver itself binds to the
super-node.

Looking at drm_irq_install(), it looks like DRM only supports 1
interrupt line per drm_device. As we have no known users who will want
these two LCD controllers represented as 2 CRTCs on 1 DRM device
(which would require management of 2 interrupt lines), I figured this
might be a reason to ignore that use case for now (from the coding
standpoint), meaning that in all cases we are left with each DRM
device having 1 CRTC, corresponding to 1 LCD controller, which has
exactly 1 interrupt line.

That still doesn't solve the problem though. drm_irq_install calls
into drm_platform to figure out which IRQ number to use, and that code
looks at the platform_device (i.e. super node). In this case we don't
have the irq info there, we have it in the "lcd controller" node.

Do I invent our own "DRM bus" implementation so that we can override
get_irq()? Start to standardise our DT design as a generic drm_dt.c
bus implementation? Any thoughts?


Secondly, devm. I understand from the "best practices" thread that we
want to have exactly one platform_device which corresponds to the
"super node", and all of the individual display controller components
will be represented by DT nodes but *without* their own
platform_device. As we now put things (clocks, interrupts) into DT
nodes that don't have a corresponding platform_device, we lose the
ability to use devm. Russell wasn't too pleased last time I posted a
patch moving away from devm, admittedly last time the patch was bad
and it wasn't necessary, but this time it looks like it might be.


Finally, just curious, do we still want to support non-DT platform
data type setups on this driver? That adds a bit of coding effort. Not
a problem, just wanted to check.

Daniel

DT binding review for Armada display subsystem

[Daniel Drake]

On Fri, Jul 12, 2013 at 10:44 AM, Daniel Drake <dsd@laptop.org> wrote:
> Based on the outcomes of the "Best practice device tree design for display
> subsystems" discussion I have drafted a DT binding. Comments much appreciated.
>
> At a high level, it uses a "super node" as something for the driver to bind
> to, needed because there is no clear one device that controls all the
> others, and also to distinguish between the Armada 510 possible use cases
> of having one video card with two outputs, or two independent video cards.
> It uses node-to-node linking beyond that point, V4L2 style.

As this hasn't been shot down very far, I've started to implement the
driver side of this binding. I have already run into a couple of
little problems.

First, interrupts. In the dt binding, each "lcd controller" node
defines which interrupt it listens to, and in the armada 510 case
there are indeed 2 interrupts (47 and 46, one for each LCD
controller). And remember that the drm driver itself binds to the
super-node.

Looking at drm_irq_install(), it looks like DRM only supports 1
interrupt line per drm_device. As we have no known users who will want
these two LCD controllers represented as 2 CRTCs on 1 DRM device
(which would require management of 2 interrupt lines), I figured this
might be a reason to ignore that use case for now (from the coding
standpoint), meaning that in all cases we are left with each DRM
device having 1 CRTC, corresponding to 1 LCD controller, which has
exactly 1 interrupt line.

That still doesn't solve the problem though. drm_irq_install calls
into drm_platform to figure out which IRQ number to use, and that code
looks at the platform_device (i.e. super node). In this case we don't
have the irq info there, we have it in the "lcd controller" node.

Do I invent our own "DRM bus" implementation so that we can override
get_irq()? Start to standardise our DT design as a generic drm_dt.c
bus implementation? Any thoughts?


Secondly, devm. I understand from the "best practices" thread that we
want to have exactly one platform_device which corresponds to the
"super node", and all of the individual display controller components
will be represented by DT nodes but *without* their own
platform_device. As we now put things (clocks, interrupts) into DT
nodes that don't have a corresponding platform_device, we lose the
ability to use devm. Russell wasn't too pleased last time I posted a
patch moving away from devm, admittedly last time the patch was bad
and it wasn't necessary, but this time it looks like it might be.


Finally, just curious, do we still want to support non-DT platform
data type setups on this driver? That adds a bit of coding effort. Not
a problem, just wanted to check.

Daniel

Re: DT binding review for Armada display subsystem

[Tomasz Figa]

Hi,

On Sunday 14 of July 2013 00:09:55 Russell King - ARM Linux wrote:
> On Sun, Jul 14, 2013 at 12:16:58AM +0200, Sylwester Nawrocki wrote:
> > On 07/13/2013 11:02 PM, Russell King - ARM Linux wrote:
> >> On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:
> >>> I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it
> >>> seems they're working on v4 with clock object reference counting.
> >>> Presumably we need both clk_get() to be taking reference on the
> >>> module and reference counted clk free, e.g. in cases where clock
> >>> provider is a hot-pluggable device. It might be too paranoid
> >>> though, I haven't seen hardware configurations where a clock source
> >>> could be unplugged safely when whole system is running.
> >> 
> >> I'm not going to accept refcounting being thrown into clk_get().  The
> >> clkdev API already has refcounting, as much as it needs to.  It just
> >> needs people to use the hooks that I provided back in 2008 when I
> >> created the clkdev API for doing _precisely_ this job.
> >> 
> >> Have a read through these commits, which backup my statement above:
> >> 
> >> 0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the
> >> clkdev API d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting
> >> Integrator to clkdev API
> >> 
> >> and it will show you how to do refcounting.  The common clk API just
> >> needs to stop defining __clk_get() and __clk_put() to be an empty
> >> function and implement them appropriately for it's clk
> >> implementation,
> >> like they were always meant to be.
> > 
> > Sure, I've already seen the above commits. This is how I understood it
> > as well - __clk_get(), __clk_put() need to be defined by the common
> > clk
> > API, since it is going to replace all the arch specific
> > implementations.> 
> >> __clk_get() and __clk_put() are the clk-implementation specific parts
> >> of the clkdev API, because the clkdev API is utterly divorsed from
> >> the
> >> internals of what a 'struct clk' actually is.  clkdev just treats a
> >> 'struct clk' as a completely opaque type and never bothers poking
> >> about inside it.
> > 
> > OK, but at the clock's implementation side we may need, e.g. to use
> > kref to keep track of the clock's state, and free it only when it is
> > unprepared, all its children are unregistered, etc. ? Is it not what
> > you stated in your comment to patch [1] ?
> 
> If you want to do refcounting on a clock (which you run into problems
> as I highlighted earlier in this thread) then yes, you need to use a
> kref, and take a reference in __clk_get() and drop it in __clk_put()
> to make things safe.
> 
> Whether you also take a reference on the module supplying the clock or
> not depends whether you need to keep the code around to manipulate that
> clock while there are users of it.
> 
> As I've already said - consider the possibilities with this scenario.
> Here's one:
> 
>   A clock consumer is using a clock, but the clock supplier has been
>   removed.  The clock consumer wants to change the state of the clock
>   in some way - eg, system is suspending.  clk_disable() doesn't fail,
>   but on resume, clk_enable() does... (and how many people assume that
>   clk_enable() never fails?)  And who knows what rate the clock is now
>   producing or whether it's even producing anything...
> 
> I'm not saying don't do the refcounting thing I mentioned back in June.
> I'm merely pointing out the issues that there are.  There isn't a one
> right answer here because each has their own advantages and
> disadvantages (and problems.)

What about having Mike on CC for such clock-related discussion?

Best regards,
Tomasz

DT binding review for Armada display subsystem

[Tomasz Figa]

Hi,

On Sunday 14 of July 2013 00:09:55 Russell King - ARM Linux wrote:
> On Sun, Jul 14, 2013 at 12:16:58AM +0200, Sylwester Nawrocki wrote:
> > On 07/13/2013 11:02 PM, Russell King - ARM Linux wrote:
> >> On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:
> >>> I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it
> >>> seems they're working on v4 with clock object reference counting.
> >>> Presumably we need both clk_get() to be taking reference on the
> >>> module and reference counted clk free, e.g. in cases where clock
> >>> provider is a hot-pluggable device. It might be too paranoid
> >>> though, I haven't seen hardware configurations where a clock source
> >>> could be unplugged safely when whole system is running.
> >> 
> >> I'm not going to accept refcounting being thrown into clk_get().  The
> >> clkdev API already has refcounting, as much as it needs to.  It just
> >> needs people to use the hooks that I provided back in 2008 when I
> >> created the clkdev API for doing _precisely_ this job.
> >> 
> >> Have a read through these commits, which backup my statement above:
> >> 
> >> 0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the
> >> clkdev API d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting
> >> Integrator to clkdev API
> >> 
> >> and it will show you how to do refcounting.  The common clk API just
> >> needs to stop defining __clk_get() and __clk_put() to be an empty
> >> function and implement them appropriately for it's clk
> >> implementation,
> >> like they were always meant to be.
> > 
> > Sure, I've already seen the above commits. This is how I understood it
> > as well - __clk_get(), __clk_put() need to be defined by the common
> > clk
> > API, since it is going to replace all the arch specific
> > implementations.> 
> >> __clk_get() and __clk_put() are the clk-implementation specific parts
> >> of the clkdev API, because the clkdev API is utterly divorsed from
> >> the
> >> internals of what a 'struct clk' actually is.  clkdev just treats a
> >> 'struct clk' as a completely opaque type and never bothers poking
> >> about inside it.
> > 
> > OK, but at the clock's implementation side we may need, e.g. to use
> > kref to keep track of the clock's state, and free it only when it is
> > unprepared, all its children are unregistered, etc. ? Is it not what
> > you stated in your comment to patch [1] ?
> 
> If you want to do refcounting on a clock (which you run into problems
> as I highlighted earlier in this thread) then yes, you need to use a
> kref, and take a reference in __clk_get() and drop it in __clk_put()
> to make things safe.
> 
> Whether you also take a reference on the module supplying the clock or
> not depends whether you need to keep the code around to manipulate that
> clock while there are users of it.
> 
> As I've already said - consider the possibilities with this scenario.
> Here's one:
> 
>   A clock consumer is using a clock, but the clock supplier has been
>   removed.  The clock consumer wants to change the state of the clock
>   in some way - eg, system is suspending.  clk_disable() doesn't fail,
>   but on resume, clk_enable() does... (and how many people assume that
>   clk_enable() never fails?)  And who knows what rate the clock is now
>   producing or whether it's even producing anything...
> 
> I'm not saying don't do the refcounting thing I mentioned back in June.
> I'm merely pointing out the issues that there are.  There isn't a one
> right answer here because each has their own advantages and
> disadvantages (and problems.)

What about having Mike on CC for such clock-related discussion?

Best regards,
Tomasz

Re: DT binding review for Armada display subsystem

[Sascha Hauer]

On Mon, Jul 15, 2013 at 02:23:30PM -0600, Daniel Drake wrote:
> On Fri, Jul 12, 2013 at 10:44 AM, Daniel Drake <dsd@laptop.org> wrote:
> > Based on the outcomes of the "Best practice device tree design for display
> > subsystems" discussion I have drafted a DT binding. Comments much appreciated.
> >
> > At a high level, it uses a "super node" as something for the driver to bind
> > to, needed because there is no clear one device that controls all the
> > others, and also to distinguish between the Armada 510 possible use cases
> > of having one video card with two outputs, or two independent video cards.
> > It uses node-to-node linking beyond that point, V4L2 style.
> 
> As this hasn't been shot down very far, I've started to implement the
> driver side of this binding. I have already run into a couple of
> little problems.
> 
> First, interrupts. In the dt binding, each "lcd controller" node
> defines which interrupt it listens to, and in the armada 510 case
> there are indeed 2 interrupts (47 and 46, one for each LCD
> controller). And remember that the drm driver itself binds to the
> super-node.
> 
> Looking at drm_irq_install(), it looks like DRM only supports 1
> interrupt line per drm_device.

You don't have to call drm_irq_install(). Both the exynos and i.MX
driver do without it and at least the i.MX driver uses multiple irqs per
drm_device.

> 
> Secondly, devm. I understand from the "best practices" thread that we
> want to have exactly one platform_device which corresponds to the
> "super node", and all of the individual display controller components
> will be represented by DT nodes but *without* their own
> platform_device.

A super node approach doesn't mean that there's only one platform
device. You can still have a platform device for each component. The
super node should rather be seen as a node which contains phandles
to the different components..

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

DT binding review for Armada display subsystem

[Sascha Hauer]

On Mon, Jul 15, 2013 at 02:23:30PM -0600, Daniel Drake wrote:
> On Fri, Jul 12, 2013 at 10:44 AM, Daniel Drake <dsd@laptop.org> wrote:
> > Based on the outcomes of the "Best practice device tree design for display
> > subsystems" discussion I have drafted a DT binding. Comments much appreciated.
> >
> > At a high level, it uses a "super node" as something for the driver to bind
> > to, needed because there is no clear one device that controls all the
> > others, and also to distinguish between the Armada 510 possible use cases
> > of having one video card with two outputs, or two independent video cards.
> > It uses node-to-node linking beyond that point, V4L2 style.
> 
> As this hasn't been shot down very far, I've started to implement the
> driver side of this binding. I have already run into a couple of
> little problems.
> 
> First, interrupts. In the dt binding, each "lcd controller" node
> defines which interrupt it listens to, and in the armada 510 case
> there are indeed 2 interrupts (47 and 46, one for each LCD
> controller). And remember that the drm driver itself binds to the
> super-node.
> 
> Looking at drm_irq_install(), it looks like DRM only supports 1
> interrupt line per drm_device.

You don't have to call drm_irq_install(). Both the exynos and i.MX
driver do without it and at least the i.MX driver uses multiple irqs per
drm_device.

> 
> Secondly, devm. I understand from the "best practices" thread that we
> want to have exactly one platform_device which corresponds to the
> "super node", and all of the individual display controller components
> will be represented by DT nodes but *without* their own
> platform_device.

A super node approach doesn't mean that there's only one platform
device. You can still have a platform device for each component. The
super node should rather be seen as a node which contains phandles
to the different components..

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: DT binding review for Armada display subsystem

[Daniel Drake]

On Mon, Jul 15, 2013 at 3:09 PM, Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> You don't have to call drm_irq_install(). Both the exynos and i.MX
> driver do without it and at least the i.MX driver uses multiple irqs per
> drm_device.

Good point, thanks. That unblocks that item.

>> Secondly, devm. I understand from the "best practices" thread that we
>> want to have exactly one platform_device which corresponds to the
>> "super node", and all of the individual display controller components
>> will be represented by DT nodes but *without* their own
>> platform_device.
>
> A super node approach doesn't mean that there's only one platform
> device. You can still have a platform device for each component.

Yes. As noted, this was simply a preference that seemed to emerge in
the previous discussion. If in practice it causes too many headaches,
maybe we will change our minds. For now devm is the only issue I have
seen; I am avoiding devm where it is no longer possible under this
design.

The goal of getting this driver working properly on OLPC XO seems to
be a twisty path that presents a new issue at every turn. In the above
work I am trying to first implement the DT binding for Armada
510/cubox, as that is what the current code is aimed at.

I am now facing a problem with i2c/TDA998x which Russell already noted:

http://lists.freedesktop.org/archives/dri-devel/2013-June/039632.html
   What *can't* be done without a rewrite of the DRM slave encoder backend
   is to have the TDA998x I2C device provided by DT.  There are mumblings
   about redoing the slave encoder API, hopefully this will be fixed there.

This is because the existing DRM/encoder system works something like this:
1. i2c driver (e.g. tda998x_drv) registers as an i2c_driver via
drm_i2c_encoder_register()
2. The drm driver (i.e. armada) later has to know that it is expecting
a tda998x instance. It calls drm_i2c_encoder_init() to set this up.

drm_i2c_encoder init requires:
 1. The i2c_adapter in question. In a DT world, we could get this from
finding the parent node of the tda998x node (this is the i2c bus
master), and then using i2c_for_each_dev to find the first i2c adapter
instance that has the same of_node.
 2. i2c_board_info - basically the address of the device on the i2c
bus. This is basically the way of instantiating i2c devices from
platform data. Not the DT way :(

In a DT world the i2c driver would be instantiated from a node in the
DT, which already includes the info that would come in i2c_board_info.
Then later it would have to be linked to a DRM slave/encoder, perhaps
when the DRM device finds the of_node corresponding to it.

So I think the next step is fixing up DRM, as Russell hinted. Unless
someone sees another acceptable option that doesn't break our DT
design.

I am going away for 3-4 weeks now, so you won't be hearing for me for
a while. Just in case someone else wants to pick up the task in the
mean time, here is my work in progress. I'm still reading and
learning, so please don't do any detailed reviews yet :)

http://dev.laptop.org/~dsd/20130717/0001-dove-clk-dt-wip.patch

It includes the previous clock selection patch as this stuff is quite
closely bound with DT support.

Thanks
Daniel

DT binding review for Armada display subsystem

[Daniel Drake]

On Mon, Jul 15, 2013 at 3:09 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> You don't have to call drm_irq_install(). Both the exynos and i.MX
> driver do without it and at least the i.MX driver uses multiple irqs per
> drm_device.

Good point, thanks. That unblocks that item.

>> Secondly, devm. I understand from the "best practices" thread that we
>> want to have exactly one platform_device which corresponds to the
>> "super node", and all of the individual display controller components
>> will be represented by DT nodes but *without* their own
>> platform_device.
>
> A super node approach doesn't mean that there's only one platform
> device. You can still have a platform device for each component.

Yes. As noted, this was simply a preference that seemed to emerge in
the previous discussion. If in practice it causes too many headaches,
maybe we will change our minds. For now devm is the only issue I have
seen; I am avoiding devm where it is no longer possible under this
design.

The goal of getting this driver working properly on OLPC XO seems to
be a twisty path that presents a new issue at every turn. In the above
work I am trying to first implement the DT binding for Armada
510/cubox, as that is what the current code is aimed at.

I am now facing a problem with i2c/TDA998x which Russell already noted:

http://lists.freedesktop.org/archives/dri-devel/2013-June/039632.html
   What *can't* be done without a rewrite of the DRM slave encoder backend
   is to have the TDA998x I2C device provided by DT.  There are mumblings
   about redoing the slave encoder API, hopefully this will be fixed there.

This is because the existing DRM/encoder system works something like this:
1. i2c driver (e.g. tda998x_drv) registers as an i2c_driver via
drm_i2c_encoder_register()
2. The drm driver (i.e. armada) later has to know that it is expecting
a tda998x instance. It calls drm_i2c_encoder_init() to set this up.

drm_i2c_encoder init requires:
 1. The i2c_adapter in question. In a DT world, we could get this from
finding the parent node of the tda998x node (this is the i2c bus
master), and then using i2c_for_each_dev to find the first i2c adapter
instance that has the same of_node.
 2. i2c_board_info - basically the address of the device on the i2c
bus. This is basically the way of instantiating i2c devices from
platform data. Not the DT way :(

In a DT world the i2c driver would be instantiated from a node in the
DT, which already includes the info that would come in i2c_board_info.
Then later it would have to be linked to a DRM slave/encoder, perhaps
when the DRM device finds the of_node corresponding to it.

So I think the next step is fixing up DRM, as Russell hinted. Unless
someone sees another acceptable option that doesn't break our DT
design.

I am going away for 3-4 weeks now, so you won't be hearing for me for
a while. Just in case someone else wants to pick up the task in the
mean time, here is my work in progress. I'm still reading and
learning, so please don't do any detailed reviews yet :)

http://dev.laptop.org/~dsd/20130717/0001-dove-clk-dt-wip.patch

It includes the previous clock selection patch as this stuff is quite
closely bound with DT support.

Thanks
Daniel

Re: DT binding review for Armada display subsystem

[Jean-Francois Moine]

On Wed, 17 Jul 2013 11:50:11 -0600
Daniel Drake <dsd@laptop.org> wrote:

> I am now facing a problem with i2c/TDA998x which Russell already noted:
> 
> http://lists.freedesktop.org/archives/dri-devel/2013-June/039632.html
>    What *can't* be done without a rewrite of the DRM slave encoder backend
>    is to have the TDA998x I2C device provided by DT.  There are mumblings
>    about redoing the slave encoder API, hopefully this will be fixed there.
> 
> This is because the existing DRM/encoder system works something like this:
> 1. i2c driver (e.g. tda998x_drv) registers as an i2c_driver via
> drm_i2c_encoder_register()
> 2. The drm driver (i.e. armada) later has to know that it is expecting
> a tda998x instance. It calls drm_i2c_encoder_init() to set this up.
> 
> drm_i2c_encoder init requires:
>  1. The i2c_adapter in question. In a DT world, we could get this from
> finding the parent node of the tda998x node (this is the i2c bus
> master), and then using i2c_for_each_dev to find the first i2c adapter
> instance that has the same of_node.
>  2. i2c_board_info - basically the address of the device on the i2c
> bus. This is basically the way of instantiating i2c devices from
> platform data. Not the DT way :(
> 
> In a DT world the i2c driver would be instantiated from a node in the
> DT, which already includes the info that would come in i2c_board_info.
> Then later it would have to be linked to a DRM slave/encoder, perhaps
> when the DRM device finds the of_node corresponding to it.
> 
> So I think the next step is fixing up DRM, as Russell hinted. Unless
> someone sees another acceptable option that doesn't break our DT
> design.

Hi Daniel,

I don't see the problem with the TDA998x.

Indeed, it needs some enhancement to handle the DT, but also, the
function drm_i2c_encoder_init() is not usable in the DT case (this
function loads the module, while the module must be loaded by the DT
for it gets all parameters - i2c address and irq).

Here is the simple solution I use:

- the tda998x is declared in the DT, so the module is loaded at system
  startup time.

- its probe function is void.

- when the connector of the drm driver scans the DT, it finds the
  phandle to the tda998x.

- if / after the tda998x is loaded, the connector calls the function
  encoder_init() of the tda998x (and also increment the reference
  counter of the tda998x module to avoid this last one to be unloaded).

- the function encoder_init() of the tda998x scans the DT and it has all
  elements to run.

This working is compatible with the ticlcd driver: calling
drm_i2c_encoder_init() just prevents the tda998x to use the irq (the
i2c address is hard coded, the display connect/disconnect event is
detected by polling and reading the EDID is synchronous).

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

DT binding review for Armada display subsystem

[Jean-Francois Moine]

On Wed, 17 Jul 2013 11:50:11 -0600
Daniel Drake <dsd@laptop.org> wrote:

> I am now facing a problem with i2c/TDA998x which Russell already noted:
> 
> http://lists.freedesktop.org/archives/dri-devel/2013-June/039632.html
>    What *can't* be done without a rewrite of the DRM slave encoder backend
>    is to have the TDA998x I2C device provided by DT.  There are mumblings
>    about redoing the slave encoder API, hopefully this will be fixed there.
> 
> This is because the existing DRM/encoder system works something like this:
> 1. i2c driver (e.g. tda998x_drv) registers as an i2c_driver via
> drm_i2c_encoder_register()
> 2. The drm driver (i.e. armada) later has to know that it is expecting
> a tda998x instance. It calls drm_i2c_encoder_init() to set this up.
> 
> drm_i2c_encoder init requires:
>  1. The i2c_adapter in question. In a DT world, we could get this from
> finding the parent node of the tda998x node (this is the i2c bus
> master), and then using i2c_for_each_dev to find the first i2c adapter
> instance that has the same of_node.
>  2. i2c_board_info - basically the address of the device on the i2c
> bus. This is basically the way of instantiating i2c devices from
> platform data. Not the DT way :(
> 
> In a DT world the i2c driver would be instantiated from a node in the
> DT, which already includes the info that would come in i2c_board_info.
> Then later it would have to be linked to a DRM slave/encoder, perhaps
> when the DRM device finds the of_node corresponding to it.
> 
> So I think the next step is fixing up DRM, as Russell hinted. Unless
> someone sees another acceptable option that doesn't break our DT
> design.

Hi Daniel,

I don't see the problem with the TDA998x.

Indeed, it needs some enhancement to handle the DT, but also, the
function drm_i2c_encoder_init() is not usable in the DT case (this
function loads the module, while the module must be loaded by the DT
for it gets all parameters - i2c address and irq).

Here is the simple solution I use:

- the tda998x is declared in the DT, so the module is loaded at system
  startup time.

- its probe function is void.

- when the connector of the drm driver scans the DT, it finds the
  phandle to the tda998x.

- if / after the tda998x is loaded, the connector calls the function
  encoder_init() of the tda998x (and also increment the reference
  counter of the tda998x module to avoid this last one to be unloaded).

- the function encoder_init() of the tda998x scans the DT and it has all
  elements to run.

This working is compatible with the ticlcd driver: calling
drm_i2c_encoder_init() just prevents the tda998x to use the irq (the
i2c address is hard coded, the display connect/disconnect event is
detected by polling and reading the EDID is synchronous).

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/