* DT binding review for Armada display subsystem
@ 2013-07-13 8:35 ` Jean-Francois Moine
0 siblings, 0 replies; 40+ messages in thread
From: Jean-Francois Moine @ 2013-07-13 8:35 UTC (permalink / raw)
To: linux-arm-kernel
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/
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: DT binding review for Armada display subsystem
2013-07-13 8:35 ` Jean-Francois Moine
@ 2013-07-13 10:56 ` Sylwester Nawrocki
-1 siblings, 0 replies; 40+ messages in thread
From: Sylwester Nawrocki @ 2013-07-13 10:56 UTC (permalink / raw)
To: Jean-Francois Moine
Cc: Daniel Drake, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-arm-kernel,
s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ, Russell King,
Sebastian Hesselbarth
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
^ permalink raw reply [flat|nested] 40+ messages in thread* DT binding review for Armada display subsystem
@ 2013-07-13 10:56 ` Sylwester Nawrocki
0 siblings, 0 replies; 40+ messages in thread
From: Sylwester Nawrocki @ 2013-07-13 10:56 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread[parent not found: <51E13272.5040903-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: DT binding review for Armada display subsystem
2013-07-13 10:56 ` Sylwester Nawrocki
@ 2013-07-13 11:12 ` Russell King - ARM Linux
-1 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2013-07-13 11:12 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Jean-Francois Moine, Daniel Drake,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ, linux-arm-kernel,
Sebastian Hesselbarth
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...)
^ permalink raw reply [flat|nested] 40+ messages in thread* DT binding review for Armada display subsystem
@ 2013-07-13 11:12 ` Russell King - ARM Linux
0 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2013-07-13 11:12 UTC (permalink / raw)
To: linux-arm-kernel
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...)
^ permalink raw reply [flat|nested] 40+ messages in thread[parent not found: <20130713111239.GM24642-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>]
* Re: DT binding review for Armada display subsystem
2013-07-13 11:12 ` Russell King - ARM Linux
@ 2013-07-13 17:44 ` Sebastian Hesselbarth
-1 siblings, 0 replies; 40+ messages in thread
From: Sebastian Hesselbarth @ 2013-07-13 17:44 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Jean-Francois Moine, Daniel Drake,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Sylwester Nawrocki,
s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread* DT binding review for Armada display subsystem
@ 2013-07-13 17:44 ` Sebastian Hesselbarth
0 siblings, 0 replies; 40+ messages in thread
From: Sebastian Hesselbarth @ 2013-07-13 17:44 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread[parent not found: <51E1921A.3070207-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: DT binding review for Armada display subsystem
2013-07-13 17:44 ` Sebastian Hesselbarth
@ 2013-07-13 20:43 ` Sylwester Nawrocki
-1 siblings, 0 replies; 40+ messages in thread
From: Sylwester Nawrocki @ 2013-07-13 20:43 UTC (permalink / raw)
To: Sebastian Hesselbarth
Cc: Jean-Francois Moine, Russell King - ARM Linux, Daniel Drake,
Jiada Wang, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread* DT binding review for Armada display subsystem
@ 2013-07-13 20:43 ` Sylwester Nawrocki
0 siblings, 0 replies; 40+ messages in thread
From: Sylwester Nawrocki @ 2013-07-13 20:43 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread[parent not found: <51E1BBF1.1020009-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: DT binding review for Armada display subsystem
2013-07-13 20:43 ` Sylwester Nawrocki
@ 2013-07-13 21:02 ` Russell King - ARM Linux
-1 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2013-07-13 21:02 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Jean-Francois Moine, Daniel Drake, Jiada Wang,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ, linux-arm-kernel,
Sebastian Hesselbarth
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* DT binding review for Armada display subsystem
@ 2013-07-13 21:02 ` Russell King - ARM Linux
0 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2013-07-13 21:02 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <20130713210236.GP24642-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>]
* Re: DT binding review for Armada display subsystem
2013-07-13 21:02 ` Russell King - ARM Linux
@ 2013-07-13 22:16 ` Sylwester Nawrocki
-1 siblings, 0 replies; 40+ messages in thread
From: Sylwester Nawrocki @ 2013-07-13 22:16 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Jean-Francois Moine, Daniel Drake, Jiada Wang,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ, linux-arm-kernel,
Sebastian Hesselbarth
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/
^ permalink raw reply [flat|nested] 40+ messages in thread
* DT binding review for Armada display subsystem
@ 2013-07-13 22:16 ` Sylwester Nawrocki
0 siblings, 0 replies; 40+ messages in thread
From: Sylwester Nawrocki @ 2013-07-13 22:16 UTC (permalink / raw)
To: linux-arm-kernel
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/
^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <51E1D1DA.8080102-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: DT binding review for Armada display subsystem
2013-07-13 22:16 ` Sylwester Nawrocki
@ 2013-07-13 23:09 ` Russell King - ARM Linux
-1 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2013-07-13 23:09 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Jean-Francois Moine, Daniel Drake, Jiada Wang,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ, linux-arm-kernel,
Sebastian Hesselbarth
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.)
^ permalink raw reply [flat|nested] 40+ messages in thread
* DT binding review for Armada display subsystem
@ 2013-07-13 23:09 ` Russell King - ARM Linux
0 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2013-07-13 23:09 UTC (permalink / raw)
To: linux-arm-kernel
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.)
^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <20130713230955.GQ24642-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>]
* Re: DT binding review for Armada display subsystem
2013-07-13 23:09 ` Russell King - ARM Linux
@ 2013-07-15 20:35 ` Tomasz Figa
-1 siblings, 0 replies; 40+ messages in thread
From: Tomasz Figa @ 2013-07-15 20:35 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Jean-Francois Moine, Russell King - ARM Linux, Daniel Drake,
Jiada Wang, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Mike Turquette,
Sylwester Nawrocki, s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ,
Sebastian Hesselbarth
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* DT binding review for Armada display subsystem
@ 2013-07-15 20:35 ` Tomasz Figa
0 siblings, 0 replies; 40+ messages in thread
From: Tomasz Figa @ 2013-07-15 20:35 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: DT binding review for Armada display subsystem
2013-07-13 17:44 ` Sebastian Hesselbarth
@ 2013-07-13 20:51 ` Russell King - ARM Linux
-1 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2013-07-13 20:51 UTC (permalink / raw)
To: Sebastian Hesselbarth
Cc: Jean-Francois Moine, Daniel Drake,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Sylwester Nawrocki,
s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ, linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 40+ messages in thread* DT binding review for Armada display subsystem
@ 2013-07-13 20:51 ` Russell King - ARM Linux
0 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2013-07-13 20:51 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: DT binding review for Armada display subsystem
2013-07-13 8:35 ` Jean-Francois Moine
@ 2013-07-13 14:25 ` Daniel Drake
-1 siblings, 0 replies; 40+ messages in thread
From: Daniel Drake @ 2013-07-13 14:25 UTC (permalink / raw)
To: Jean-Francois Moine
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Russell King,
s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ, linux-arm-kernel,
Sebastian Hesselbarth
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
^ permalink raw reply [flat|nested] 40+ messages in thread* DT binding review for Armada display subsystem
@ 2013-07-13 14:25 ` Daniel Drake
0 siblings, 0 replies; 40+ messages in thread
From: Daniel Drake @ 2013-07-13 14:25 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: DT binding review for Armada display subsystem
2013-07-13 14:25 ` Daniel Drake
@ 2013-07-13 17:36 ` Sebastian Hesselbarth
-1 siblings, 0 replies; 40+ messages in thread
From: Sebastian Hesselbarth @ 2013-07-13 17:36 UTC (permalink / raw)
To: Daniel Drake
Cc: Jean-Francois Moine, devicetree-discuss, dri-devel, Grant Likely,
Russell King, s.hauer, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread* DT binding review for Armada display subsystem
@ 2013-07-13 17:36 ` Sebastian Hesselbarth
0 siblings, 0 replies; 40+ messages in thread
From: Sebastian Hesselbarth @ 2013-07-13 17:36 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <CAMLZHHSrX2T+pU3RosmjS3EggV3mX_J8D2OJD52sww_n6Y7B+A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: DT binding review for Armada display subsystem
2013-07-13 14:25 ` Daniel Drake
@ 2013-07-13 17:38 ` Russell King - ARM Linux
-1 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2013-07-13 17:38 UTC (permalink / raw)
To: Daniel Drake
Cc: Jean-Francois Moine, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ, linux-arm-kernel,
Sebastian Hesselbarth
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.
^ permalink raw reply [flat|nested] 40+ messages in thread
* DT binding review for Armada display subsystem
@ 2013-07-13 17:38 ` Russell King - ARM Linux
0 siblings, 0 replies; 40+ messages in thread
From: Russell King - ARM Linux @ 2013-07-13 17:38 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 40+ messages in thread