Re: DT binding review for Armada display subsystem
On Mon, Jul 15, 2013 at 3:09 PM, Sascha Hauer s.ha...@pengutronix.de wrote: You don't have to call drm_irq_install(). Both the exynos and i.MX driver do without it and at least the i.MX driver uses multiple irqs per drm_device. Good point, thanks. That unblocks that item. Secondly, devm. I understand from the best practices thread that we want to have exactly one platform_device which corresponds to the super node, and all of the individual display controller components will be represented by DT nodes but *without* their own platform_device. A super node approach doesn't mean that there's only one platform device. You can still have a platform device for each component. Yes. As noted, this was simply a preference that seemed to emerge in the previous discussion. If in practice it causes too many headaches, maybe we will change our minds. For now devm is the only issue I have seen; I am avoiding devm where it is no longer possible under this design. The goal of getting this driver working properly on OLPC XO seems to be a twisty path that presents a new issue at every turn. In the above work I am trying to first implement the DT binding for Armada 510/cubox, as that is what the current code is aimed at. I am now facing a problem with i2c/TDA998x which Russell already noted: http://lists.freedesktop.org/archives/dri-devel/2013-June/039632.html What *can't* be done without a rewrite of the DRM slave encoder backend is to have the TDA998x I2C device provided by DT. There are mumblings about redoing the slave encoder API, hopefully this will be fixed there. This is because the existing DRM/encoder system works something like this: 1. i2c driver (e.g. tda998x_drv) registers as an i2c_driver via drm_i2c_encoder_register() 2. The drm driver (i.e. armada) later has to know that it is expecting a tda998x instance. It calls drm_i2c_encoder_init() to set this up. drm_i2c_encoder init requires: 1. The i2c_adapter in question. In a DT world, we could get this from finding the parent node of the tda998x node (this is the i2c bus master), and then using i2c_for_each_dev to find the first i2c adapter instance that has the same of_node. 2. i2c_board_info - basically the address of the device on the i2c bus. This is basically the way of instantiating i2c devices from platform data. Not the DT way :( In a DT world the i2c driver would be instantiated from a node in the DT, which already includes the info that would come in i2c_board_info. Then later it would have to be linked to a DRM slave/encoder, perhaps when the DRM device finds the of_node corresponding to it. So I think the next step is fixing up DRM, as Russell hinted. Unless someone sees another acceptable option that doesn't break our DT design. I am going away for 3-4 weeks now, so you won't be hearing for me for a while. Just in case someone else wants to pick up the task in the mean time, here is my work in progress. I'm still reading and learning, so please don't do any detailed reviews yet :) http://dev.laptop.org/~dsd/20130717/0001-dove-clk-dt-wip.patch It includes the previous clock selection patch as this stuff is quite closely bound with DT support. Thanks Daniel ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: DT binding review for Armada display subsystem
On Fri, Jul 12, 2013 at 10:44 AM, Daniel Drake d...@laptop.org wrote: Based on the outcomes of the Best practice device tree design for display subsystems discussion I have drafted a DT binding. Comments much appreciated. At a high level, it uses a super node as something for the driver to bind to, needed because there is no clear one device that controls all the others, and also to distinguish between the Armada 510 possible use cases of having one video card with two outputs, or two independent video cards. It uses node-to-node linking beyond that point, V4L2 style. As this hasn't been shot down very far, I've started to implement the driver side of this binding. I have already run into a couple of little problems. First, interrupts. In the dt binding, each lcd controller node defines which interrupt it listens to, and in the armada 510 case there are indeed 2 interrupts (47 and 46, one for each LCD controller). And remember that the drm driver itself binds to the super-node. Looking at drm_irq_install(), it looks like DRM only supports 1 interrupt line per drm_device. As we have no known users who will want these two LCD controllers represented as 2 CRTCs on 1 DRM device (which would require management of 2 interrupt lines), I figured this might be a reason to ignore that use case for now (from the coding standpoint), meaning that in all cases we are left with each DRM device having 1 CRTC, corresponding to 1 LCD controller, which has exactly 1 interrupt line. That still doesn't solve the problem though. drm_irq_install calls into drm_platform to figure out which IRQ number to use, and that code looks at the platform_device (i.e. super node). In this case we don't have the irq info there, we have it in the lcd controller node. Do I invent our own DRM bus implementation so that we can override get_irq()? Start to standardise our DT design as a generic drm_dt.c bus implementation? Any thoughts? Secondly, devm. I understand from the best practices thread that we want to have exactly one platform_device which corresponds to the super node, and all of the individual display controller components will be represented by DT nodes but *without* their own platform_device. As we now put things (clocks, interrupts) into DT nodes that don't have a corresponding platform_device, we lose the ability to use devm. Russell wasn't too pleased last time I posted a patch moving away from devm, admittedly last time the patch was bad and it wasn't necessary, but this time it looks like it might be. Finally, just curious, do we still want to support non-DT platform data type setups on this driver? That adds a bit of coding effort. Not a problem, just wanted to check. Daniel ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: DT binding review for Armada display subsystem
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 ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: DT binding review for Armada display subsystem
On Mon, Jul 15, 2013 at 02:23:30PM -0600, Daniel Drake wrote: On Fri, Jul 12, 2013 at 10:44 AM, Daniel Drake d...@laptop.org wrote: Based on the outcomes of the Best practice device tree design for display subsystems discussion I have drafted a DT binding. Comments much appreciated. At a high level, it uses a super node as something for the driver to bind to, needed because there is no clear one device that controls all the others, and also to distinguish between the Armada 510 possible use cases of having one video card with two outputs, or two independent video cards. It uses node-to-node linking beyond that point, V4L2 style. As this hasn't been shot down very far, I've started to implement the driver side of this binding. I have already run into a couple of little problems. First, interrupts. In the dt binding, each lcd controller node defines which interrupt it listens to, and in the armada 510 case there are indeed 2 interrupts (47 and 46, one for each LCD controller). And remember that the drm driver itself binds to the super-node. Looking at drm_irq_install(), it looks like DRM only supports 1 interrupt line per drm_device. You don't have to call drm_irq_install(). Both the exynos and i.MX driver do without it and at least the i.MX driver uses multiple irqs per drm_device. Secondly, devm. I understand from the best practices thread that we want to have exactly one platform_device which corresponds to the super node, and all of the individual display controller components will be represented by DT nodes but *without* their own platform_device. A super node approach doesn't mean that there's only one platform device. You can still have a platform device for each component. The super node should rather be seen as a node which contains phandles to the different components.. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: DT binding review for Armada display subsystem
On 07/13/2013 10:35 AM, Jean-Francois Moine wrote:
On Fri, 12 Jul 2013 13:00:23 -0600 Daniel Draked...@laptop.org wrote:
On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moinemoin...@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 13
(40/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@82 {
compatible = marvell,dove-lcd0;
[...]
};
lcd1: lcd-controller@81 {
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@82 {
compatible = marvell,dove-lcd;
...
};
lcd_1: lcd-controller@82 {
compatible = marvell,dove-lcd;
...
};
Re: DT binding review for Armada display subsystem
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 Draked...@laptop.org wrote:
On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moinemoin...@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 13
(40/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@82 {
compatible = marvell,dove-lcd0;
[...]
};
lcd1: lcd-controller@81 {
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@82 {
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...)
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: DT binding review for Armada display subsystem
On Sat, Jul 13, 2013 at 2:35 AM, Jean-Francois Moine moin...@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 13
(40/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@82 {
compatible = marvell,dove-lcd0;
reg = 0x82 0x1c8;
interrupts = 47;
clocks = core_clk 3, lcdclk;
clock-names = axi, lcdclk;
marvell-output = dcon 0;
status = disabled;
};
lcd1: lcd-controller@81 {
compatible = marvell,dove-lcd1;
reg = 0x81 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@83 {
compatible = marvell,dove-dcon;
reg = 0x83 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
Re: DT binding review for Armada display subsystem
On 07/13/2013 04:25 PM, Daniel Drake wrote:
On Sat, Jul 13, 2013 at 2:35 AM, Jean-Francois Moine moin...@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 13
(40/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@82 {
compatible = marvell,dove-lcd0;
reg = 0x82 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@81 {
compatible = marvell,dove-lcd1;
reg = 0x81 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@83 {
compatible = marvell,dove-dcon;
reg = 0x83 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 {
Re: DT binding review for Armada display subsystem
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. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: DT binding review for Armada display subsystem
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 Draked...@laptop.org wrote:
On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moinemoin...@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 13
(40/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@82 {
compatible = marvell,dove-lcd0;
[...]
};
lcd1: lcd-controller@81 {
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@82 {
compatible = marvell,dove-lcd;
I'd much prefer marvell,armada-510-lcd rather than using the codenames for
the devices. Otherwise we're going
Re: DT binding review for Armada display subsystem
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 Draked...@laptop.org wrote:
On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois
Moinemoin...@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 13
(40/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@82 {
compatible = marvell,dove-lcd0;
[...]
};
lcd1: lcd-controller@81 {
compatible = marvell,dove-lcd1;
[...]
};
Using different compatible strings to indicate multiple instances
Re: DT binding review for Armada display subsystem
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 soo 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.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: DT binding review for Armada display subsystem
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. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: DT binding review for Armada display subsystem
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/ ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: DT binding review for Armada display subsystem
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.) ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
DT binding review for Armada display subsystem
Hi,
Based on the outcomes of the Best practice device tree design for display
subsystems discussion I have drafted a DT binding. Comments much appreciated.
At a high level, it uses a super node as something for the driver to bind
to, needed because there is no clear one device that controls all the
others, and also to distinguish between the Armada 510 possible use cases
of having one video card with two outputs, or two independent video cards.
It uses node-to-node linking beyond that point, V4L2 style.
I have some questions still:
1. What names does the Armada 510 datasheet give to the components? Below
I renamed the LCD controllers (which don't control LCDs on any of the
current target hardware) to display controllers. For what we were
previously referring to as DCON, I renamed to output selector. I would
like to match the docs.
Actually the output selector is not mentioned as per Sebastian's
suggestion, but I believe it would fit in the below design once the first
user comes along (e.g. it would slot in the graph between dcon0 and tda99x).
2. On that topic, what names does the Armada 510 datasheet give to the
available clocks? Lets make them match the docs.
3. What is the remote property in the video interfaces binding? Doesn't
seem to be documented. But I think I copied its usage style below.
Marvell Armada Display Subsystem
Design considerations
-
The display device that emerges from this subsystem is quite heavily
componentized and the formation of the composite device will vary by SoC and
by board. The DT binding is designed with this flexibility in mind.
For example, there are 2 display controllers on the Armada 510.
They could legitametely be used to form two independent video cards, or
they could be combined into a single video card with two outputs, depending
on board wiring and use case.
As there is no clear component that controls the other components, especially
when we wish to combine the two display controllers into one virtual device, we
define a top-level super node that describes the basic composition of the
overall display device. That node uses phandles to define the start of the
graph of display components.
In the bindings for the individual display components, phandle properties
are used to represent direct, fixed links to other components. We use the
port/endpoint abstraction from bindings/media/video-interfaces.txt to
represent these links.
display super node
--
Required properties:
- compatible: marvell,armada-display
- marvell,video-devices : List of phandles to the display controllers that
form this composite device.
Optional properties;
- reg: Memory address and size of a RAM allocation designated as graphics
memory
- linux,video-memory-size: If reg is not provided, this property defines
how much memory to cut out of regular available RAM to be used as graphics
memory.
display-controller node
---
Required properties:
- compatible: marvell,armada-510-display, marvell,mmp2-display or
marvell,mmp3-display
- reg: Register space for this display controller
- clocks: List of clocks available to this device. From common clock binding.
- clock-names: Names of the given clocks (see below)
- ports: From video interface binding
Valid clock names for Armada 510: extclk0 extclk1 axi pll
Valid clock names for MMP2: lcd1 lcd2 axi hdmi
Valid clock names for MMP3: lcd1 lcd2 axi hdmi dsi lvds
Example:
display {
  compatible = marvell,armada-display;
   reg = 0 0x3f00 0x100; /* video-mem hole */
   marvell,video-devices = dcon0;
  };
dcon0: display-controller@2 {
compatible = marvell,armada-510-display-controller;
   reg = 0x2 0x1000;
   interrupts = 47;
   clocks = ext_clk0;
   clock-names = extclk0;
port {
dcon0_1: endpoint {
remote = tda998x;
};
};
};
i2c0 {
  tda998x: hdmi-transmitter@60 {
   compatible = nxp,tda19988;
    reg = 0x60;
port {
tda998x_1: endpoint {
remote-endpoint = dcon0_1;
};
};
   };
};
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: DT binding review for Armada display subsystem
On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moine moin...@free.fr wrote:
- I think it is better to keep the names 'lcd' for the memory to dumb panel
sub-devices and 'dcon' for the dumb panel to LCD/VGA sub-device, as
named in the spec.
I agree it is worth keeping the spec-defined names, if they do not
cause excessive confusion when mixed with the spec-defined names for
MMP2/MMP3. I'll check the spec you pointed out, thanks.
- the phandles to the clocks does not tell how the clock may be set by
the driver (it is an array index in the 510).
In the other threads on clock selection, we decided that that exact
information on how to select a clock (i.e. register bits/values) must
be in the driver, not in the DT. What was suggested there is a
well-documented scheme for clock naming, so the driver knows which
clock is which. That is defined in the proposed DT binding though the
valid clock names part. For an example driver implementation of this
you can see my patch armada_drm clock selection - try 2.
- I don't see the use of the phandles in the leaves (dcon and tda998x).
That is defined by the video interfaces common binding. I'm not
immediately aware of the use, but the ability to easily traverse the
graph in both directions seems like a requirement that could easily
emerge from somewhere.
Well, here is how I feel the DTs:
- for the Cubox (one LCD, the DCON is not used, TDA998x for HDMI/DVI
output):
That is the same as my proposal except you have decided to use direct
phandle properties instead of using the common video interfaces
binding (which is just a slightly more detailed way of describing such
links). In the best practices thread, the suggestion was raised
multiple times of doing what v4l2 does, so thats why I decided to
explore this here.
- for some tablet based on the Armada 510 with a LCD and a VGA connector:
video {
compatible = marvell,armada-video;
marvell,video-devices = lcd0, lcd1, dcon
};
This proposal is slightly different because it does not describe the
relationship between the LCD controllers and the DCON. Focusing
specifically on LCD1, which would be connected to a DAC via a phandle
property in your proposal. The interconnectivity between the
components represented as a graph (and in the v4l2 way, which includes
my proposal) would be:
LCD1 --- DCON --- DAC
However your phandle properties would suggest that LCD1 is directly
connected to the DAC. The driver would have to have special knowledge
that the DCON sits right in the middle. Maybe that is not an issue, as
it is obviously OK for the driver to have *some* knowledge about how
the hardware works :)
I don't think we have a full consensus on whether we want to go the
v4l2 way here. But I figured that would be a good thing to propose
in a first iteration to have a clearer idea of what the result would
look like. It sounds like you are not overly keen on it, I would be
interested to hear exactly why.
Thanks,
Daniel
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
