[PATCH RFC 06/15] drm/armada: move variant initialisation to CRTC init

[Sebastian Hesselbarth]

On 07/11/2014 04:37 PM, Russell King - ARM Linux wrote:
> On Sat, Jul 05, 2014 at 01:58:37PM +0200, Sebastian Hesselbarth wrote:
>> On 07/05/2014 12:38 PM, Russell King wrote:
>>> Move the variant initialisation entirely to the CRTC init function -
>>> the variant support is really about the CRTC properties than the whole
>>> system, and we want to treat each CRTC individually when we support DT.
>>>
>>> Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
>>> ---
>> [...]
>>> diff --git a/drivers/gpu/drm/armada/armada_crtc.h 
>>> b/drivers/gpu/drm/armada/armada_crtc.h
>>> index 531a9b0bdcfb..3f0e70bb2e9c 100644
>>> --- a/drivers/gpu/drm/armada/armada_crtc.h
>>> +++ b/drivers/gpu/drm/armada/armada_crtc.h
>>> @@ -38,6 +38,7 @@ struct armada_crtc {
>>> unsignednum;
>>> void __iomem*base;
>>> struct clk  *clk;
>>> +   struct clk  *extclk[2];
>>
>> Russell,
>>
>> I wonder, if we should rename above array srcclk instead of extclk
>> while moving it anyway. That way we can use it for the other variant
>> specific clocks, too.
>
> As the patches are prepared with this change, I'd prefer to submit them
> as-is, and then we can update that as and when the support for things
> like the MMP/610 is finished off.  I think they're good to go, so I'll
> send them off later today to David.

Ok, sounds fine to me.

> This leaves the TDA998x componentisation patches which I need to kick
> out, and the initial DT changes.  Once those are in place, we should
> have almost all ducks lined up for working DRM support - it'll certainly
> be advanced enough to describe the LCD controllers and the TDA998x as
> three separate DT entities using the of graph helpers.

Ok.

> What's left is the display-subsystem { } entity to describe the makeup
> of the subsystem.  That's not included as we currently need to pass
> a block of memory, and the DT support for reserving chunks of memory
> appeared (last time I looked) to only be botch-merged (only half of it
> seems to have been merged making the whole reserved memory thing
> totally useless - why people only half-merge features I've no idea.)

There was a follow-up patch set for this some days ago
http://comments.gmane.org/gmane.linux.ports.arm.kernel/337686

Sebastian

[PATCH RFC 06/15] drm/armada: move variant initialisation to CRTC init

[Sebastian Hesselbarth]

On 07/05/2014 02:21 PM, Russell King - ARM Linux wrote:
> On Sat, Jul 05, 2014 at 01:58:37PM +0200, Sebastian Hesselbarth wrote:
>> On 07/05/2014 12:38 PM, Russell King wrote:
>>> Move the variant initialisation entirely to the CRTC init function -
>>> the variant support is really about the CRTC properties than the whole
>>> system, and we want to treat each CRTC individually when we support DT.
>>>
>>> Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
>>> ---
>> [...]
>>> diff --git a/drivers/gpu/drm/armada/armada_crtc.h 
>>> b/drivers/gpu/drm/armada/armada_crtc.h
>>> index 531a9b0bdcfb..3f0e70bb2e9c 100644
>>> --- a/drivers/gpu/drm/armada/armada_crtc.h
>>> +++ b/drivers/gpu/drm/armada/armada_crtc.h
>>> @@ -38,6 +38,7 @@ struct armada_crtc {
>>> unsignednum;
>>> void __iomem*base;
>>> struct clk  *clk;
>>> +   struct clk  *extclk[2];
>>
>> I wonder, if we should rename above array srcclk instead of extclk
>> while moving it anyway. That way we can use it for the other variant
>> specific clocks, too.
> 
> pixelclk may be a better name for it.  I would like to think about the

The name was derived from the "SCLK_SOURCE_SELECT" bits in Dove FS, but
any other name not limited to external clocks is fine, too.

> clock handling further though - the issues surrounding clock selection
> are not limited to just Armada - imx-drm has the exact same problem.
> 
> The issue with clocking of CRTCs is that it seems to be common that:
> 
> 1. you have multiple clocks to choose from, some of which may be more
>suitable than others depending on the type of output.

Given the limited capabilities of the internal clock dividers, on Dove
the heuristic seems to be fairly simple: always prefer the external
clock. This is true for all actively supported Dove boards (Cubox and
{d2,d3}plug) as all have an external PLL connected.

I'd even say to make the external clock mandatory. Hitting a standard
pixclk frequency with one of the internal clocks is pure coincidence.

As long as there is no board using anything else than HDMI transmitter
on dumb RGB, we shouldn't try to foresee any suitable heuristic.

> 2. clocks end up being shared between multiple CRTCs, and one CRTC
>can (at the moment) interfere with the clock rate delivered to
>another CRTC.
> 
> This happens on imx-drm today, where the two DIs (CRTCs) are in use -
> one for HDMI, the other for LVDS.  We end up with HDMI set first to
> 148.5MHz, and then LVDS sets it's clock to 65MHz, which results in
> HDMI receiving a clock at over 500MHz!  At the moment, there are hacks
> to solve this by adjusting the muxes in the clock paths to ensure that
> they both derive from different PLLs - moving the LVDS onto the USB OTG
> PLL rather than the video PLL.  That works fine until USB OTG wants
> to change the OTG PLL.

Again, luckily on Cubox we have 2 VCOs for the two external clocks
(audio and video) so we won't have to deal with it. For the {d2,d3}plug
I'll have to check the IDT datasheet again.

In particular, for imx maybe it is possible to identify some clock tree
configurations for specific use-cases. I don't think there is any non-
manual way to tell the best clock tree config.

> There's also the issue whether the output can cope with fractional
> clock-skipping dividers - entirely synchronous display systems can
> (such as synchronously clocked LCD panels), but asynchronous display
> systems (such as HDMI, TV out, etc) can't.  That said, the other
> parameter that needs to be taken account of here is that even with the
> fractional divider, the minimum output clock period isn't the average
> frequency, but the maximum frequency, which may violate a panel's minimum
> clock period specification.

Yeah, the fractional divider isn't made for external HDMI transmitters
for sure. I have seen from your branch that there is some Armada 610
stub for OLPC, do they have a dumb panel directly connected?

I also saw that they do have an external PLL, so maybe we should stick
to the external clock inputs as long as no other configurations pops-up
(which may never happen).

Sebastian

> I think there's lots to do on the clocking side, and as it's a fairly
> complex problem which is common to multiple implementations, I think
> that any solution should not be specific.
> 
> However, this topic isn't one which I want to work on until I have
> reduced down my patch sets to something more manageable - something
> which I'm desperate to do.  (I've been trying to avoid adding any
> further patches to any tree for some time now.)  This is why (eg) I'm
> not going to fix the kernel oops-able bugs I found in the SGTL5000
> codec - someone else can do that.

[PATCH RFC 06/15] drm/armada: move variant initialisation to CRTC init

[Sebastian Hesselbarth]

On 07/05/2014 12:38 PM, Russell King wrote:
> Move the variant initialisation entirely to the CRTC init function -
> the variant support is really about the CRTC properties than the whole
> system, and we want to treat each CRTC individually when we support DT.
> 
> Signed-off-by: Russell King 
> ---
[...]
> diff --git a/drivers/gpu/drm/armada/armada_crtc.h 
> b/drivers/gpu/drm/armada/armada_crtc.h
> index 531a9b0bdcfb..3f0e70bb2e9c 100644
> --- a/drivers/gpu/drm/armada/armada_crtc.h
> +++ b/drivers/gpu/drm/armada/armada_crtc.h
> @@ -38,6 +38,7 @@ struct armada_crtc {
>   unsignednum;
>   void __iomem*base;
>   struct clk  *clk;
> + struct clk  *extclk[2];

Russell,

I wonder, if we should rename above array srcclk instead of extclk
while moving it anyway. That way we can use it for the other variant
specific clocks, too.

FWIW, I totally agree the it was the right thing to wait for you
to sort out the dependencies. Good work, great patience.

Sebastian

[PATCH v2] drm/i2c: tda998x: Change the compatible strings

[Sebastian Hesselbarth]

On 03/23/2014 09:03 PM, Russell King - ARM Linux wrote:
> On Sun, Mar 23, 2014 at 07:12:05PM +0100, Sebastian Hesselbarth wrote:
>> On 03/23/2014 11:19 AM, Jean-Francois Moine wrote:
>>> On Fri, 21 Mar 2014 14:37:52 +0100
>>> Sebastian Hesselbarth  wrote:
>>>>> Required properties;
>>>>> -  - compatible: must be "nxp,tda998x"
>>>>> +  - compatible: may be "nxp,tda9989", "nxp,tda19988" or "nxp,tda19989"
>>>>
>>>> There is a "DT is ABI" policy and although there is no mainline Linux
>>>> user of current compatible, the correct way would be to deprecate
>>>> "nxp,tda998x" and introduce new compatibles.
>>>
>>> Pratically, what is this way?
>>
>> Currently, there is no effective way to deprecate a binding or
>> compatible. You just add the one(s) that are more sensible and
>> you mark the old one as DEPRECATED by simply writing it in the
>> binding doc.
>>
>> The driver should support the old binding at least for a while.
>
> It doesn't need to - it's only been in development trees so far, and
> never been in a mainline full release.  Until it does, the binding
> does not become stable.

Ok, I see. Thanks for the clarification. A note about it would
have been nice though. Anyway, sorry for the noise.

Sebastian

[PATCH v2] drm/i2c: tda998x: Change the compatible strings

[Sebastian Hesselbarth]

On 03/23/2014 11:19 AM, Jean-Francois Moine wrote:
> On Fri, 21 Mar 2014 14:37:52 +0100
> Sebastian Hesselbarth  wrote:
>>>Required properties;
>>> -  - compatible: must be "nxp,tda998x"
>>> +  - compatible: may be "nxp,tda9989", "nxp,tda19988" or "nxp,tda19989"
>>
>> There is a "DT is ABI" policy and although there is no mainline Linux
>> user of current compatible, the correct way would be to deprecate
>> "nxp,tda998x" and introduce new compatibles.
>
> Pratically, what is this way?

Currently, there is no effective way to deprecate a binding or
compatible. You just add the one(s) that are more sensible and
you mark the old one as DEPRECATED by simply writing it in the
binding doc.

The driver should support the old binding at least for a while.

Sebastian

[PATCH v2] drm/i2c: tda998x: Change the compatible strings

[Sebastian Hesselbarth]

On 03/21/2014 11:55 AM, Jean-Francois Moine wrote:
> The tda998x driver accepts only 3 chips from the TDA998x family.
> This patch changes the driver compatible strings to these chips.

Jean-Francois,

be careful with building a DT binding from a Linux driver. Although
we constantly struggle to define a binding independent of Linux, it
should not reflect what Linux is capable of but describe the HW in
general.

> Signed-off-by: Jean-Francois Moine 
> ---
> v2: change the subject to drm/i2c
> This patch applies after
>   drm/i2c: tda998x: Fix lack of required reg in DT documentation
> ---
>   Documentation/devicetree/bindings/drm/i2c/tda998x.txt | 4 ++--
>   drivers/gpu/drm/i2c/tda998x_drv.c | 4 +++-
>   2 files changed, 5 insertions(+), 3 deletions(-)
> diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt 
> b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> index fc7effa..e3f3d65 100644
> --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> @@ -1,7 +1,7 @@
>   Device-Tree bindings for the NXP TDA998x HDMI transmitter
>
>   Required properties;
> -  - compatible: must be "nxp,tda998x"
> +  - compatible: may be "nxp,tda9989", "nxp,tda19988" or "nxp,tda19989"

There is a "DT is ABI" policy and although there is no mainline Linux
user of current compatible, the correct way would be to deprecate
"nxp,tda998x" and introduce new compatibles.

Also, as long as we don't know about any major differences between
9989, 1998[89] it is fine to just have one of them defined. As soon
as we discover any difference that cannot be solved in another way,
we can add a new compatible.

What we _know_ is that 998[134] are different from 9989,1998[89]
with respect to additional CEC feature. But we also _know_ that the
exact version/revision of 9989,1998[89] can be probed from i2c
registers.

DT maintainers will know better, but as long as we have no prove that
998[134] can also be properly distinguished by i2c registers, just add
"nxp,tda9989" (which was probably the first revision released) and
assume 1998[89] are "compatible enough". Or add all three and make
"nxp,tda9989" the mandatory compatible.

You can leave out 998[134] for now.

> - reg: I2C address
>

The line above and below reg property look like there is a tab. If
so, can you please get rid of the blank line above and fix the line
below?

> @@ -20,7 +20,7 @@ Optional properties:
>   Example:
>
>   tda998x: hdmi-encoder {
> - compatible = "nxp,tda998x";
> + compatible = "nxp,tda19988";

Depending on above decision this becomes either

compatible = "nxp,tda9989";

or

compatible = "nxp,tda19988", "nxp,tda9989";

>   reg = <0x70>;
>   interrupt-parent = <>;
>   interrupts = <27 2>;/* falling edge */
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 48af5ca..fd6751c 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1367,7 +1367,9 @@ fail:
>
>   #ifdef CONFIG_OF
>   static const struct of_device_id tda998x_dt_ids[] = {
> - { .compatible = "nxp,tda998x", },
> + { .compatible = "nxp,tda9989", },
> + { .compatible = "nxp,tda19988", },
> + { .compatible = "nxp,tda19989", },

Independent of the decision above, just "nxp,tda9989" is
sufficient.

Sebastian

>   { }
>   };
>   MODULE_DEVICE_TABLE(of, tda998x_dt_ids);
>

[PATCH] ASoC: tda998x: Fix lack of required reg in DT documentation

[Sebastian Hesselbarth]

On 03/20/2014 02:52 PM, Jean-Francois Moine wrote:
> On Thu, 20 Mar 2014 14:32:18 +0100
> Sebastian Hesselbarth  wrote:
>
>> Ok, I had another round of google'ing and found this:
>> http://hipstercircuits.com/wp-content/uploads/2013/05/TDA19988.pdf
>>
>> There, the datasheet specifically for TDA19988 only states 0x70 and
>> 0x34 as the two i2c addresses. Therefore, TDA19988 has fixed i2c
>> addresses while TDA9983b has configurable (main) i2c address.
>>
>> Not as easy as we thought ;)
>>
>> I suggest reword the reg property to:
>> "- reg: shall be set to the I2C address"
>>
>> and optionally list all known addresses for each TDA[1]998x in the
>> binding.
>
> Thanks for the link.
>
> OK, then, as the linux tda998x driver handles only the tda 19988 and
> 19989 chips, the HDMI I2C address is always 0x70.
>
> So, question: Russell and Sebastian, do you still want an other patch?

Up to Russell, but reg property is required for i2c slaves and should
be documented.

> Other question: the CEC address is hard-coded to 0x34 in the driver.
> Should it be configurable in the DT?

Looking at nxp's website, detailed information about TDA[1]998x have
vanished. Luckily, there are some hints in the parametric search:

There is TDA998[14] without any CEC support and
TDA1998[89] with CEC support. Both TDA19988 and TDA19989 have
fixed i2c addresses 0x70 and 0x34 respectively.

So, the answer is:

Let the driver access CEC i2c slave only if it is TDA1998[89]. The
HDMI part should be quite compatible with TDA998x, so if anyone
has it on his board, he is invited to add proper support.

For the binding, reg address should contain reg property to the
HDMI i2c slave. Add proper compatibles/i2c_ids for nxp,tda19988 and
nxp,tda19989 and check that to add the CEC i2c slave on 0x34.

If, someday there will be a tda19990 with configurable addresses,
I am sure you can derive it from i2c main address, e.g. 0x70<>0x34,
0x71<>0x35, aso.

No need to take care of configurable CEC slave address now, neither
in the driver nor binding.

Sebastian

[PATCH] ASoC: tda998x: Fix lack of required reg in DT documentation

[Sebastian Hesselbarth]

On 03/20/2014 02:01 PM, Jean-Francois Moine wrote:
> On Thu, 20 Mar 2014 13:32:24 +0100
> Sebastian Hesselbarth  wrote:
>
>>> +  - reg: I2C address - must be <0x70>
>>
>> TDA9983b datasheet says:
>>
>> "Bits A0 and A1 of the I2C-bus device address are externally selected
>> by pins A0 and A1."
>>
>> Therefore, 0x70, 0x71, 0x72, and 0x73 are valid i2c addresses.
>
> Sebastian,
>
> That's interesting!
>
> For the Cubox and the AMX33XX boards, the I2C address of the HDMI
> registers is 0x70, and the I2C address of the CEC registers is 0x34.
>
> For other boards using the TDA998x family, if the I2C address is
> different from 0x70, have you an idea about what can be the CEC I2C
> address? (this value is actually hard-coded in the TDA998x driver)
>

Ok, I had another round of google'ing and found this:
http://hipstercircuits.com/wp-content/uploads/2013/05/TDA19988.pdf

There, the datasheet specifically for TDA19988 only states 0x70 and
0x34 as the two i2c addresses. Therefore, TDA19988 has fixed i2c
addresses while TDA9983b has configurable (main) i2c address.

Not as easy as we thought ;)

I suggest reword the reg property to:
"- reg: shall be set to the I2C address"

and optionally list all known addresses for each TDA[1]998x in the
binding.

Sebastian

[PATCH] ASoC: tda998x: Fix lack of required reg in DT documentation

[Sebastian Hesselbarth]

On 03/20/2014 09:58 AM, Jean-Francois Moine wrote:
> The I2C address (reg) is required for the TDA998x driver to be loaded
> and initialized.
>
> Signed-off-by: Jean-Francois Moine 
> ---
> This patch applies to linux-next.
> ---
>   Documentation/devicetree/bindings/drm/i2c/tda998x.txt | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt 
> b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> index d7df01c..fc7effa 100644
> --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> @@ -3,6 +3,8 @@ Device-Tree bindings for the NXP TDA998x HDMI transmitter
>   Required properties;
> - compatible: must be "nxp,tda998x"
>
> +  - reg: I2C address - must be <0x70>

TDA9983b datasheet says:

"Bits A0 and A1 of the I2C-bus device address are externally selected
by pins A0 and A1."

Therefore, 0x70, 0x71, 0x72, and 0x73 are valid i2c addresses.

Sebastian

>   Optional properties:
> - interrupts: interrupt number and trigger type
>   default: polling
>

[PATCH 0/9] Doc/DT: DT bindings for various display components

[Sebastian Hesselbarth]

On 02/28/14 14:47, Philipp Zabel wrote:
> Am Freitag, den 28.02.2014, 13:57 +0100 schrieb Sebastian Hesselbarth:
>> On 02/28/14 13:20, Tomi Valkeinen wrote:
>>> This series is a re-send of
>>> http://article.gmane.org/gmane.linux.drivers.devicetree/61739
>>>
>> ...
>>> Shortly about the display components in the series, in the order of probable
>>> public interest:
>>>
>>> * Analog TV, DVI and HDMI Connectors represent a respective connector on the
>>> board. They don't do much, but they do mark the end of the video 
>>> pipeline (from
>>> the board's pov), and they should also in the future offer ways to 
>>> handle
>>> things like the +5V pin on DVI and HDMI connector and HPD pin.
>>>
>>> * MIPI DPI panel and MIPI DSI CM panels represent bindings for simple panels
>>> using the respective video bus.
>>>
>>> * Sony acx565akm is an SPI controlled panel using flatlink video bus.
>>>
>>> * TFP410 is a DPI to DVI encoder.
>>>
>>> * TPD12S015 is a HDMI companion chip, used on OMAP boards.
>>
>> Tomi,
>>
>> Out of curiosity, will there be DT nodes for pull-up resistors soon,
>> too? ;)
>>
>> Honestly, TPD12S015 is a level shifter, there is nothing in it that
>> would justify a DT node nor a driver.
>>
>> Above you already note, that connector nodes should offer HPD in the
>> future, but I guess the binding should represent that now already.
>> I will be a DT stub anyway, the corresponding video sink driver will
>> have to look it up.
>>
>> Looking through the bindings for DVI and HDMI, I guess HPD gpio is
>> better kept in those nodes. From the relevant (DT) properties DVI and
>> HDMI connectors are in no way different.
>
> I like the idea of adding actual connector nodes to the board device
> trees. A TV encoder driver for example could this way detect from the
> device tree whether it is connected to a VGA, Composite, or S-Video
> connector (or maybe to both Composite and S-Video connectors at the same
> time).

I agree that different connectors help providing a better
user-experienced view of board layout. But I doubt that besides the
differentiation (HDMI, DVI, VGA, SVideo, ...) and the endpoint there
should be anything more in that node.

Sebastian

[PATCH 0/9] Doc/DT: DT bindings for various display components

[Sebastian Hesselbarth]

On 02/28/14 14:14, Tomi Valkeinen wrote:
> On 28/02/14 14:57, Sebastian Hesselbarth wrote:
>
>> Out of curiosity, will there be DT nodes for pull-up resistors soon,
>> too? ;)
>
> If they don't work automatically, yes, we need DT nodes and drivers for
> them.
>
>> Honestly, TPD12S015 is a level shifter, there is nothing in it that
>> would justify a DT node nor a driver.
>
> TPD requires a power. Who turns that on? It also has two GPIOs, LS_OE
> and CT_CP_HPD, which need to be controlled based on what the user wants
> and the state of the HPD line. Who controls those?

Strictly speaking TPD12S015 has _no_ GPIO but only buffers. It
translates one voltage to another. The controlling instance is
your "video card" that is really interested in the actual state
of HPD signal.

Also the same for power, TPD12S015 doesn't decide to be powered up
or down but the "video card" does. We have GPIO regulators that
deal with that situation already.

Consider the same board but replace TPD12S015 with another level-
shifter, you still want OMAP video driver work with that out-of-the-box,
don't you? Fact is, OMAP IP requires GPIOs to sense HPD status hence
that GPIO is a property of the corresponding OMAP node. How level-
translation happens is irrelevant here.

>> Above you already note, that connector nodes should offer HPD in the
>> future, but I guess the binding should represent that now already.
>
> I think it can be added when somebody uses it. I don't see why that
> would cause trouble later to those that don't use it.

Thinking about it again, HPD gpio shouldn't be a property of the
connector at all but again the controlling instance. The connector
cannot deal with the information provided by HPD nor can it determine
if anyone is listening to HPD events.

>> I will be a DT stub anyway, the corresponding video sink driver will
>> have to look it up.
>
> I'm not sure what you mean with that. Yes, it's not the most complex DT
> nodes out there.
 >
>> Looking through the bindings for DVI and HDMI, I guess HPD gpio is
>> better kept in those nodes. From the relevant (DT) properties DVI and
>> HDMI connectors are in no way different.
>
> Well, I think the HPD gpio should be where it's most logical to have it.

Right, but this is usually the controlling instance and not the
consuming one. E.g. to detect presence of an MMC card by GPIO, you'd
put that into the MMC _controller_ not any card node.

> I mean, you could have a setup where you have the SoC HDMI encoder and
> and the HDMI connector, and the HPD pin goes directly to the HDMI
> encoder, which has HW support for it. In that case, the HDMI encoder
> node should contain the HPD, and the HDMI encoder should handle it.

I wonder, if in case of an dedicated HPD pin, you would ever expose that
in DT.

> Or, your HDMI encoder could not have any kind of support for HPD. In
> that case you could have the HDMI connector driver handle the hotplug
> event. You could of course make the HDMI encoder driver handle the HPD
> gpio, but I usually try to have the driver handle the hardware device in
> question.

Having a driver for a dumb connector seems to be a little exaggerated.
Consider your generic HDMI connector "driver" connected to dedicated HPD
case above. It is pretty useless then. OTOH video controllers with 
dedicated HPD know very well they can control HPD themselves, video 
controllers without dedicated HPD also know very well that they need
GPIO for it.

> In OMAP's case, we have the TPD chip between the HDMI encoder and the
> connector, and the logical place to handle HPD GPIO in that case is the
> TPD driver, as that's where the HPD is connected to and the TPD needs to
> be configured according to the state of the HPD.

Is it really the logical place to handle HPD? I'd have put it into the
HDMI encoder because it's the unit most interested in the state of HPD.

Please, don't get me wrong, I like all this to be baked into a binding -
just wondering if a level-shifter driver plus corresponding DT node
is too much detail in here.

Sebastian

[PATCH 0/9] Doc/DT: DT bindings for various display components

[Sebastian Hesselbarth]

On 02/28/14 13:20, Tomi Valkeinen wrote:
> This series is a re-send of
> http://article.gmane.org/gmane.linux.drivers.devicetree/61739
>
...
> Shortly about the display components in the series, in the order of probable
> public interest:
>
> * Analog TV, DVI and HDMI Connectors represent a respective connector on the
>board. They don't do much, but they do mark the end of the video pipeline 
> (from
>the board's pov), and they should also in the future offer ways to handle
>things like the +5V pin on DVI and HDMI connector and HPD pin.
>
> * MIPI DPI panel and MIPI DSI CM panels represent bindings for simple panels
>using the respective video bus.
>
> * Sony acx565akm is an SPI controlled panel using flatlink video bus.
>
> * TFP410 is a DPI to DVI encoder.
>
> * TPD12S015 is a HDMI companion chip, used on OMAP boards.

Tomi,

Out of curiosity, will there be DT nodes for pull-up resistors soon,
too? ;)

Honestly, TPD12S015 is a level shifter, there is nothing in it that
would justify a DT node nor a driver.

Above you already note, that connector nodes should offer HPD in the
future, but I guess the binding should represent that now already.
I will be a DT stub anyway, the corresponding video sink driver will
have to look it up.

Looking through the bindings for DVI and HDMI, I guess HPD gpio is
better kept in those nodes. From the relevant (DT) properties DVI and
HDMI connectors are in no way different.

Sebastian

[PATCH v5 00/23]

[Sebastian Hesselbarth]

On 02/02/2014 07:23 PM, Russell King - ARM Linux wrote:
> On Sun, Feb 02, 2014 at 07:06:06PM +0100, Jean-Francois Moine wrote:
>> - on encoder_destroy(), the function drm_i2c_encoder_destroy()
>>unregisters the i2c client, so, with a DT, a second encoder_init()
>>would crash.
>
> I think this is one of the down-sides of trying to bolt DT into this:
> the drm encoder slave support is not designed to cope with an i2c client
> device pre-created.
>
> In fact, I can't see how this stuff comes anywhere close to working in
> a DT setup: in such a scenario, you declare that there's a tda998x
> device in DT.  I2C parses this, and creates an i2c_client itself for
> the tda998x.
>
> When the TDA998x driver initialises, it finds this i2c client and
> binds to it, calling tda998x_probe(), which does nothing.
>
> However, the only way to attach a slave encoder to a DRM device is via
> a call to drm_i2c_encoder_init(), which unconditionally calls
> i2c_new_device().  This creates a _new_ i2c_client structure, again
> unconditionally, for the tda998x.  This must be bound by the I2C
> subsystem to a driver - hopefully the tda998x driver, which then
> calls it's encoder_init function.
>
> None of this will happen if DT has already created an i2c_client at
> the appropriate address, because DRMs i2c_new_device() will fail.

drm_i2c_encoder_init() could look at .of_node of the i2c_board_info.
If it is there, do not try to i2c_new_device as it has already been
registered by DT i2c auto-probing.

Sebastian

[PATCH v3 15/24] drm/i2c: tda998x: use irq for connection status and EDID read

[Sebastian Hesselbarth]

On 01/22/14 23:27, Russell King - ARM Linux wrote:
> On Sun, Jan 19, 2014 at 07:58:43PM +0100, Jean-Francois Moine wrote:
>> This patch adds the optional treatment of the tda998x IRQ.
>>
>> The interrupt function is used to know the display connection status
>> without polling and to speedup reading the EDID.
>>
>> The interrupt number may be defined either in the DT or at encoder set
>> config time for non-DT boards.
>>
>> Signed-off-by: Jean-Francois Moine 
>> ---
[...]
>> @@ -720,6 +787,10 @@ tda998x_encoder_set_config(struct drm_encoder *encoder, 
>> void *params)
>>  priv->audio_port = p->audio_cfg;
>>  priv->audio_format = p->audio_format;
>>  }
>> +
>> +priv->irq = p->irq;
>> +if (p->irq)
>> +tda_irq_init(priv);
>
> If we're going to do it this way, this should probably release the IRQ if
> there was one before re-claiming it, just in case this function gets called
> more than once by some driver using it.
>
> The alternative is, as I said before, to use the infrastructure which is
> already there, namely setting the interrupt via struct i2c_client's
> irq member.  Yes, that doesn't satisfy Sebastian's comment about using
> a GPIO, but there's no sign of GPIO usage in here at the moment anyway.
> So we might as well use what's already provided.

Russell,

I am fine with using an irq instead of gpio here. I remember you telling
me on a similar patch, that from the gpio you can derive the irq but
not the other way round. Anyway, I also remember reading discussions
about DT gpios vs interrupts, and IIRC the outcome was that passing
interrupts is fine, too.

We usually have both interrupt-controller; and gpio-controller; set on
DT gpio controllers, so let's stick with irq.

And: Thanks for reviewing this again, I am still too busy to keep up
with it.

Sebastian

[PATCH v2 13/28] drm/i2c: tda998x: use irq for connection status and EDID read

[Sebastian Hesselbarth]

On 01/12/2014 07:51 PM, Jean-Francois Moine wrote:
> On Sat, 11 Jan 2014 19:35:21 +0100
> Sebastian Hesselbarth  wrote:
>
>> At least for the DT part, I'd suggest to not ask for interrupt directly
>> but use a proper gpios property. The can of course be converted to
>> priv->int_irq in some tda998x_dt_probe.
>
> May you give me more information?

Sure, see [1].

[1] http://lists.freedesktop.org/archives/dri-devel/2013-May/038822.html

Sebastian

[PATCH v2 13/28] drm/i2c: tda998x: use irq for connection status and EDID read

[Sebastian Hesselbarth]

On 01/11/2014 07:14 PM, Russell King - ARM Linux wrote:
> On Thu, Jan 09, 2014 at 12:04:12PM +0100, Jean-Francois Moine wrote:
>> @@ -1250,6 +1311,39 @@ tda998x_encoder_init(struct i2c_client *client,
>>  priv->vip_cntrl_2 = video;
>>  }
>>
>> +/* install the optional HDMI connect IRQ */
>> +priv->int_irq = irq_of_parse_and_map(np, 0);
>> +if (priv->int_irq < 0)
>> +priv->int_irq = NO_IRQ;
>> +if (priv->int_irq != NO_IRQ) {
>
> NAK.  Do not use NO_IRQ.  Use <= 0 instead, or just test against zero for
> no IRQ.  It would also be nice to offer this facility to non-DT platforms
> via client->irq.  Not every arch in the Linux kernel uses DT.

At least for the DT part, I'd suggest to not ask for interrupt directly
but use a proper gpios property. The can of course be converted to
priv->int_irq in some tda998x_dt_probe.

Sebastian

Re: [PATCH 0/5] Armada DRM stuff

[Sebastian Hesselbarth]

On 10/07/2013 12:07 AM, Russell King - ARM Linux wrote:

The Armada DRM drivers again, along with the TDA998x HDMI output support,
and an illustration to show how to add Armada 610 support (and others.)

Rob has looked at this a couple of times since its last posting, and
has provided additional useful feedback which has been incorporated.
I believe all the major issues have been addressed now.


Tested-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com

on Marvell Armada 510, SolidRun CuBox with quick-hack DT support added.

Let's please get this driver mainlined and start working on proper DT
support for it. Thanks for driver and the patience to work out all
issues!

Sebastian

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] DRM: Armada: add support for drm tda19988 driver

[Sebastian Hesselbarth]

On 10/07/2013 01:09 PM, Russell King - ARM Linux wrote:

On Mon, Oct 07, 2013 at 12:48:20PM +0200, Jean-Francois Moine wrote:

On Mon, 7 Oct 2013 10:44:04 +0100
Rabeeh did the most he could to have a working Cubox. He used bad
written drivers and he had not the time to think about how the drivers
could be enhanced.


What does I2S give us in terms of enhancement which SPDIF does not,
remembering that audio is transmitted over HDMI in a format which closely
resembles SPDIF (but with a 28-bit subframe size.)


With respect to what _most_ users want, we should use SPDIF and ignore
I2S. Consumer audio fits well into SPDIF and we get pass-through, which
we loose on I2S.

I2S _can_ support more than two channels, but only if you wire up more
DATA lines. Those are not available on Dove, so its I2S is limited to
two channel audio.

The CuBox is simply not designed for HBR or multi-channel PCM, it is a
_consumer_ settop box replacement.

Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] DRM: Armada: add support for drm tda19988 driver

[Sebastian Hesselbarth]

On 10/07/2013 05:53 PM, Mark Brown wrote:

On Mon, Oct 07, 2013 at 01:29:30PM +0200, Sebastian Hesselbarth wrote:


I2S _can_ support more than two channels, but only if you wire up more
DATA lines. Those are not available on Dove, so its I2S is limited to
two channel audio.


A lot of devices implement extra channels with TDM rather than with
extra wires - you get all the left channels and all the right channels
grouped together (like with DSP/PCM modes except the channels are
ordered differently).  I'm never sure that's actually useful, though,
since it's not something that just works with most hosts that don't
understand it in hardware and most of those can do DSP/PCM anyway.


True, I didn't mention TDM modes squeezing multiple channels into one
L/R frame. Neither Dove nor TDA998x support this, but thanks for the
clarification.


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 8/8] drm/tilcdc fixup mode to workaround sync for tda998x

[Sebastian Hesselbarth]

From: Darren Etheridge <detheri...@ti.com>

Add a fixup function that will flip the hsync priority and
add a hskew value that is used to shift the tda998x to the
right by a variable number of pixels depending on the mode.
This works around an issue with the sync timings that tilcdc
is outputing.

Signed-off-by: Darren Etheridge 
Tested-by: Sebastian Hesselbarth 
---
Changelog:
v1->v2:
- fix typo in commit line (s/workaound/workaround)

Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c  |7 ++-
 drivers/gpu/drm/tilcdc/tilcdc_slave.c |   27 ++-
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 7418dcd..6d05240 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -379,7 +379,12 @@ static int tilcdc_crtc_mode_set(struct drm_crtc *crtc,
else
tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_EDGE);

-   if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+   /*
+* use value from adjusted_mode here as this might have been
+* changed as part of the fixup for slave encoders to solve the
+* issue where tilcdc timings are not VESA compliant
+*/
+   if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
else
tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c 
b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
index 0bf5999..f6e9c26 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
@@ -73,13 +73,38 @@ static void slave_encoder_prepare(struct drm_encoder 
*encoder)
tilcdc_crtc_set_panel_info(encoder->crtc, _info);
 }

+static bool slave_encoder_fixup(struct drm_encoder *encoder,
+   const struct drm_display_mode *mode,
+   struct drm_display_mode *adjusted_mode)
+{
+   /*
+* tilcdc does not generate VESA-complient sync but aligns
+* VS on the second edge of HS instead of first edge.
+* We use adjusted_mode, to fixup sync by aligning both rising
+* edges and add HSKEW offset to let the slave encoder fix it up.
+*/
+   adjusted_mode->hskew = mode->hsync_end - mode->hsync_start;
+   adjusted_mode->flags |= DRM_MODE_FLAG_HSKEW;
+
+   if (mode->flags & DRM_MODE_FLAG_NHSYNC) {
+   adjusted_mode->flags |= DRM_MODE_FLAG_PHSYNC;
+   adjusted_mode->flags &= ~DRM_MODE_FLAG_NHSYNC;
+   } else {
+   adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC;
+   adjusted_mode->flags &= ~DRM_MODE_FLAG_PHSYNC;
+   }
+
+   return drm_i2c_encoder_mode_fixup(encoder, mode, adjusted_mode);
+}
+
+
 static const struct drm_encoder_funcs slave_encoder_funcs = {
.destroy= slave_encoder_destroy,
 };

 static const struct drm_encoder_helper_funcs slave_encoder_helper_funcs = {
.dpms   = drm_i2c_encoder_dpms,
-   .mode_fixup = drm_i2c_encoder_mode_fixup,
+   .mode_fixup = slave_encoder_fixup,
.prepare= slave_encoder_prepare,
.commit = drm_i2c_encoder_commit,
.mode_set   = drm_i2c_encoder_mode_set,
-- 
1.7.10.4

[PATCH v2 7/8] drm/i2c: tda998x: prepare for broken sync workaround

[Sebastian Hesselbarth]

Some LCD controller cannot provide valid VESA style sync, i.e. coincident
HS/VS edges. First, this patch adds hskew passed from the adjusted_mode to
reference pixel calculation to allow those controllers to add an offset
relative to the expected reference pixel.

Signed-off-by: Darren Etheridge 
Signed-off-by: Sebastian Hesselbarth 
---
Changelog:
for v1:
- reword comment

Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 92fcb3d..c2bd711 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -782,6 +782,14 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
de_pix_s = mode->htotal - mode->hdisplay;
ref_pix  = 3 + hs_pix_s;

+   /*
+* Attached LCD controllers may generate broken sync. Allow
+* those to adjust the position of the rising VS edge by adding
+* HSKEW to ref_pix.
+*/
+   if (adjusted_mode->flags & DRM_MODE_FLAG_HSKEW)
+   ref_pix += adjusted_mode->hskew;
+
if ((mode->flags & DRM_MODE_FLAG_INTERLACE) == 0) {
ref_line = 1 + mode->vsync_start - mode->vdisplay;
vwin1_line_s = mode->vtotal - mode->vdisplay - 1;
-- 
1.7.10.4

[PATCH v2 6/8] drm/i2c: tda998x: fix sync generation and calculation

[Sebastian Hesselbarth]

This fixes the wrong sync generation and sync calculation of TDA998x
for HS/VS-based sync detection.

Signed-off-by: Sebastian Hesselbarth 
Tested-by: Darren Etheridge 
---
Changelog:
v1->v2:
- revert calculation of hs/de_pix_s/e (Reported by Russell King)

Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |  181 +++--
 1 file changed, 115 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 2b64dfa..92fcb3d 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -140,8 +140,12 @@ struct tda998x_priv {
 #define REG_VS_LINE_END_1_LSB REG(0x00, 0xae) /* write */
 #define REG_VS_PIX_END_1_MSB  REG(0x00, 0xaf) /* write */
 #define REG_VS_PIX_END_1_LSB  REG(0x00, 0xb0) /* write */
+#define REG_VS_LINE_STRT_2_MSBREG(0x00, 0xb1) /* write */
+#define REG_VS_LINE_STRT_2_LSBREG(0x00, 0xb2) /* write */
 #define REG_VS_PIX_STRT_2_MSB REG(0x00, 0xb3) /* write */
 #define REG_VS_PIX_STRT_2_LSB REG(0x00, 0xb4) /* write */
+#define REG_VS_LINE_END_2_MSB REG(0x00, 0xb5) /* write */
+#define REG_VS_LINE_END_2_LSB REG(0x00, 0xb6) /* write */
 #define REG_VS_PIX_END_2_MSB  REG(0x00, 0xb7) /* write */
 #define REG_VS_PIX_END_2_LSB  REG(0x00, 0xb8) /* write */
 #define REG_HS_PIX_START_MSB  REG(0x00, 0xb9) /* write */
@@ -152,21 +156,29 @@ struct tda998x_priv {
 #define REG_VWIN_START_1_LSB  REG(0x00, 0xbe) /* write */
 #define REG_VWIN_END_1_MSBREG(0x00, 0xbf) /* write */
 #define REG_VWIN_END_1_LSBREG(0x00, 0xc0) /* write */
+#define REG_VWIN_START_2_MSB  REG(0x00, 0xc1) /* write */
+#define REG_VWIN_START_2_LSB  REG(0x00, 0xc2) /* write */
+#define REG_VWIN_END_2_MSBREG(0x00, 0xc3) /* write */
+#define REG_VWIN_END_2_LSBREG(0x00, 0xc4) /* write */
 #define REG_DE_START_MSB  REG(0x00, 0xc5) /* write */
 #define REG_DE_START_LSB  REG(0x00, 0xc6) /* write */
 #define REG_DE_STOP_MSB   REG(0x00, 0xc7) /* write */
 #define REG_DE_STOP_LSB   REG(0x00, 0xc8) /* write */
 #define REG_TBG_CNTRL_0   REG(0x00, 0xca) /* write */
+# define TBG_CNTRL_0_TOP_TGL  (1 << 0)
+# define TBG_CNTRL_0_TOP_SEL  (1 << 1)
+# define TBG_CNTRL_0_DE_EXT   (1 << 2)
+# define TBG_CNTRL_0_TOP_EXT  (1 << 3)
 # define TBG_CNTRL_0_FRAME_DIS(1 << 5)
 # define TBG_CNTRL_0_SYNC_MTHD(1 << 6)
 # define TBG_CNTRL_0_SYNC_ONCE(1 << 7)
 #define REG_TBG_CNTRL_1   REG(0x00, 0xcb) /* write */
-# define TBG_CNTRL_1_VH_TGL_0 (1 << 0)
-# define TBG_CNTRL_1_VH_TGL_1 (1 << 1)
-# define TBG_CNTRL_1_VH_TGL_2 (1 << 2)
-# define TBG_CNTRL_1_VHX_EXT_DE   (1 << 3)
-# define TBG_CNTRL_1_VHX_EXT_HS   (1 << 4)
-# define TBG_CNTRL_1_VHX_EXT_VS   (1 << 5)
+# define TBG_CNTRL_1_H_TGL(1 << 0)
+# define TBG_CNTRL_1_V_TGL(1 << 1)
+# define TBG_CNTRL_1_TGL_EN   (1 << 2)
+# define TBG_CNTRL_1_X_EXT(1 << 3)
+# define TBG_CNTRL_1_H_EXT(1 << 4)
+# define TBG_CNTRL_1_V_EXT(1 << 5)
 # define TBG_CNTRL_1_DWIN_DIS (1 << 6)
 #define REG_ENABLE_SPACE  REG(0x00, 0xd6) /* write */
 #define REG_HVF_CNTRL_0   REG(0x00, 0xe4) /* write */
@@ -735,43 +747,70 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
struct drm_display_mode *adjusted_mode)
 {
struct tda998x_priv *priv = to_tda998x_priv(encoder);
-   uint16_t hs_start, hs_end, line_start, line_end;
-   uint16_t vwin_start, vwin_end, de_start, de_end;
-   uint16_t ref_pix, ref_line, pix_start2;
+   uint16_t ref_pix, ref_line, n_pix, n_line;
+   uint16_t hs_pix_s, hs_pix_e;
+   uint16_t vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
+   uint16_t vs2_pix_s, vs2_pix_e, vs2_line_s, vs2_line_e;
+   uint16_t vwin1_line_s, vwin1_line_e;
+   uint16_t vwin2_line_s, vwin2_line_e;
+   uint16_t de_pix_s, de_pix_e;
uint8_t reg, div, rep;

-   hs_start   = mode->hsync_start - mode->hdisplay;
-   hs_end = mode->hsync_end - mode->hdisplay;
-   line_start = 1;
-   line_end   = 1 + mode->vsync_end - mode->vsync_start;
-   vwin_start = mode->vtotal - mode->vsync_start;
-   vwin_end   = vwin_start + mode->vdisplay;
-   de_start   = mode->htotal - mode->hdisplay;
-   de_end = mode->htotal;
-
-   pix_start2 = 0;
-   if (mode->flags & DRM_MODE_FLAG_INTERLACE)
-   pix_start2 = (mode->htotal / 2) + 

[PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

[Sebastian Hesselbarth]

From: Russell King <rmk+ker...@arm.linux.org.uk>

This patch adds tda998x specific parameters to allow it to be configured
for different boards using it. Also, this implements rudimentary audio
support for S/PDIF attached controllers.

Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
Signed-off-by: Sebastian Hesselbarth 
Tested-by: Darren Etheridge 
---
Changelog:
for v1:
- set AUDIO_DIV to SERCLK/16 for modes with pixclk >100MHz
- also calculate CTS
v1->v2:
- Remove CTS calculation as it isn't used in current TDA998x setup
  (Reported by Russell King)
- Remove debug left-over (Reported by Russell King)
- Use correct tda998x include (Reported by Russell King)

Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |  268 +++--
 include/drm/i2c/tda998x.h |   30 +
 2 files changed, 290 insertions(+), 8 deletions(-)
 create mode 100644 include/drm/i2c/tda998x.h

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 527d11b..2b64dfa 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -23,7 +23,7 @@
 #include 
 #include 
 #include 
-
+#include 

 #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)

@@ -32,9 +32,11 @@ struct tda998x_priv {
uint16_t rev;
uint8_t current_page;
int dpms;
+   bool is_hdmi_sink;
u8 vip_cntrl_0;
u8 vip_cntrl_1;
u8 vip_cntrl_2;
+   struct tda998x_encoder_params params;
 };

 #define to_tda998x_priv(x)  ((struct tda998x_priv 
*)to_encoder_slave(x)->slave_priv)
@@ -71,10 +73,13 @@ struct tda998x_priv {
 # define I2C_MASTER_DIS_MM(1 << 0)
 # define I2C_MASTER_DIS_FILT  (1 << 1)
 # define I2C_MASTER_APP_STRT_LAT  (1 << 2)
+#define REG_FEAT_POWERDOWNREG(0x00, 0x0e) /* read/write */
+# define FEAT_POWERDOWN_SPDIF (1 << 3)
 #define REG_INT_FLAGS_0   REG(0x00, 0x0f) /* read/write */
 #define REG_INT_FLAGS_1   REG(0x00, 0x10) /* read/write */
 #define REG_INT_FLAGS_2   REG(0x00, 0x11) /* read/write */
 # define INT_FLAGS_2_EDID_BLK_RD  (1 << 1)
+#define REG_ENA_ACLK  REG(0x00, 0x16) /* read/write */
 #define REG_ENA_VP_0  REG(0x00, 0x18) /* read/write */
 #define REG_ENA_VP_1  REG(0x00, 0x19) /* read/write */
 #define REG_ENA_VP_2  REG(0x00, 0x1a) /* read/write */
@@ -113,6 +118,7 @@ struct tda998x_priv {
 #define REG_VIP_CNTRL_5   REG(0x00, 0x25) /* write */
 # define VIP_CNTRL_5_CKCASE   (1 << 0)
 # define VIP_CNTRL_5_SP_CNT(x)(((x) & 3) << 1)
+#define REG_MUX_APREG(0x00, 0x26) /* read/write */
 #define REG_MUX_VP_VIP_OUTREG(0x00, 0x27) /* read/write */
 #define REG_MAT_CONTRLREG(0x00, 0x80) /* write */
 # define MAT_CONTRL_MAT_SC(x) (((x) & 3) << 0)
@@ -175,6 +181,12 @@ struct tda998x_priv {
 # define HVF_CNTRL_1_PAD(x)   (((x) & 3) << 4)
 # define HVF_CNTRL_1_SEMI_PLANAR  (1 << 6)
 #define REG_RPT_CNTRL REG(0x00, 0xf0) /* write */
+#define REG_I2S_FORMATREG(0x00, 0xfc) /* read/write */
+# define I2S_FORMAT(x)(((x) & 3) << 0)
+#define REG_AIP_CLKSELREG(0x00, 0xfd) /* write */
+# define AIP_CLKSEL_FS(x) (((x) & 3) << 0)
+# define AIP_CLKSEL_CLK_POL(x)(((x) & 1) << 2)
+# define AIP_CLKSEL_AIP(x)(((x) & 7) << 3)


 /* Page 02h: PLL settings */
@@ -198,6 +210,12 @@ struct tda998x_priv {
 #define REG_PLL_SCGR1 REG(0x02, 0x09) /* read/write */
 #define REG_PLL_SCGR2 REG(0x02, 0x0a) /* read/write */
 #define REG_AUDIO_DIV REG(0x02, 0x0e) /* read/write */
+# define AUDIO_DIV_SERCLK_1   0
+# define AUDIO_DIV_SERCLK_2   1
+# define AUDIO_DIV_SERCLK_4   2
+# define AUDIO_DIV_SERCLK_8   3
+# define AUDIO_DIV_SERCLK_16  4
+# define AUDIO_DIV_SERCLK_32  5
 #define REG_SEL_CLK   REG(0x02, 0x11) /* read/write */
 # define SEL_CLK_SEL_CLK1 (1 << 0)
 # define SEL_CLK_SEL_VRF_CLK(x)   (((x) & 3) << 1)
@@ -216,6 +234,11 @@ struct tda998x_priv {


 /* Page 10h: information frames and packets */
+#define REG_IF1_HB0   REG(0x10, 0x20) /* read/write */
+#define REG_IF2_HB0   REG(0x10, 0x40) /* read/write */
+#define REG_IF3_HB0   REG(0x10, 0x60) /* read/write */
+#define REG_IF4_HB0   REG(0x10, 0x80) /* read/write */
+#define REG_IF5_HB0   REG(0x10, 0xa0) /* read/write */


 /* Page 11h: audio settings and content info packets */
@@ -

[PATCH v2 4/8] drm/i2c: tda998x: prepare for video input configuration

[Sebastian Hesselbarth]

From: Russell King <rmk+ker...@arm.linux.org.uk>

The video-input-port (VIP) is highly configurable. This prepares
current driver to allow to configure VIP configuration, as some
boards connect lcd controller and TDA998x "pin-swapped" and depend
on VIP to swap the pins by register configuration.

Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
Tested-by: Darren Etheridge 
Tested-by: Sebastian Hesselbarth 
---
Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |   16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index a701411..527d11b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -32,6 +32,9 @@ struct tda998x_priv {
uint16_t rev;
uint8_t current_page;
int dpms;
+   u8 vip_cntrl_0;
+   u8 vip_cntrl_1;
+   u8 vip_cntrl_2;
 };

 #define to_tda998x_priv(x)  ((struct tda998x_priv 
*)to_encoder_slave(x)->slave_priv)
@@ -448,12 +451,9 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
reg_write(encoder, REG_ENA_VP_1, 0xff);
reg_write(encoder, REG_ENA_VP_2, 0xff);
/* set muxing after enabling ports: */
-   reg_write(encoder, REG_VIP_CNTRL_0,
-   VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3));
-   reg_write(encoder, REG_VIP_CNTRL_1,
-   VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1));
-   reg_write(encoder, REG_VIP_CNTRL_2,
-   VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5));
+   reg_write(encoder, REG_VIP_CNTRL_0, priv->vip_cntrl_0);
+   reg_write(encoder, REG_VIP_CNTRL_1, priv->vip_cntrl_1);
+   reg_write(encoder, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
break;
case DRM_MODE_DPMS_OFF:
/* disable audio and video ports */
@@ -823,6 +823,10 @@ tda998x_encoder_init(struct i2c_client *client,
if (!priv)
return -ENOMEM;

+   priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
+   priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
+   priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
+
priv->current_page = 0;
priv->cec = i2c_new_dummy(client->adapter, 0x34);
priv->dpms = DRM_MODE_DPMS_OFF;
-- 
1.7.10.4

[PATCH v2 3/8] drm/i2c: tda998x: fix npix/nline programming

[Sebastian Hesselbarth]

From: Russell King <rmk+ker...@arm.linux.org.uk>

The npix/nline registers are supposed to be programmed with the total
number of pixels/lines, not the displayed pixels/lines, and not minus
one either.

Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
Tested-by: Darren Etheridge 
Tested-by: Sebastian Hesselbarth 
---
Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index cb9b13a..a701411 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -587,8 +587,8 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
reg_set(encoder, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);

reg_write(encoder, REG_VIDFORMAT, 0x00);
-   reg_write16(encoder, REG_NPIX_MSB, mode->hdisplay - 1);
-   reg_write16(encoder, REG_NLINE_MSB, mode->vdisplay - 1);
+   reg_write16(encoder, REG_NPIX_MSB, mode->htotal);
+   reg_write16(encoder, REG_NLINE_MSB, mode->vtotal);
reg_write16(encoder, REG_VS_LINE_STRT_1_MSB, line_start);
reg_write16(encoder, REG_VS_LINE_END_1_MSB, line_end);
reg_write16(encoder, REG_VS_PIX_STRT_1_MSB, hs_start);
-- 
1.7.10.4

[PATCH v2 2/8] drm/i2c: tda998x: ensure VIP output mux is properly set

[Sebastian Hesselbarth]

From: Russell King <rmk+ker...@arm.linux.org.uk>

When switching between various drivers for this device, it's possible
that some critical registers are left containing values which affect
the device operation.  One such case encountered is the VIP output
mux register.  This defaults to 0x24 on powerup, but other drivers may
set this to 0x12.  This results in incorrect colours.

Fix this by ensuring that the register is always set to the power on
default setting.

Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
Tested-by: Darren Etheridge 
Tested-by: Sebastian Hesselbarth 
---
Changelog:
v1->v2:
- move reg_write to tda998x_reset as last patch was based on an
  old version (Reported by Russell King)

Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index d71c408..cb9b13a 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -110,6 +110,7 @@ struct tda998x_priv {
 #define REG_VIP_CNTRL_5   REG(0x00, 0x25) /* write */
 # define VIP_CNTRL_5_CKCASE   (1 << 0)
 # define VIP_CNTRL_5_SP_CNT(x)(((x) & 3) << 1)
+#define REG_MUX_VP_VIP_OUTREG(0x00, 0x27) /* read/write */
 #define REG_MAT_CONTRLREG(0x00, 0x80) /* write */
 # define MAT_CONTRL_MAT_SC(x) (((x) & 3) << 0)
 # define MAT_CONTRL_MAT_BP(1 << 2)
@@ -415,6 +416,9 @@ tda998x_reset(struct drm_encoder *encoder)
reg_write(encoder, REG_PLL_SCGR1,0x5b);
reg_write(encoder, REG_PLL_SCGR2,0x00);
reg_write(encoder, REG_PLL_SCG2, 0x10);
+
+   /* Write the default value MUX register */
+   reg_write(encoder, REG_MUX_VP_VIP_OUT, 0x24);
 }

 /* DRM encoder functions */
-- 
1.7.10.4

[PATCH v2 1/8] drm/i2c: tda998x: fix EDID reading on TDA19988 devices

[Sebastian Hesselbarth]

From: Russell King <rmk+ker...@arm.linux.org.uk>

TDA19988 devices need their RAM enabled in order to read EDID
information.  Add support for this.

Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
Signed-off-by: Rob Clark 
Tested-by: Darren Etheridge 
Tested-by: Sebastian Hesselbarth 
---
Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |   14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index e68b58a..d71c408 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -229,6 +229,8 @@ struct tda998x_priv {

 /* Page 12h: HDCP and OTP */
 #define REG_TX3   REG(0x12, 0x9a) /* read/write */
+#define REG_TX4   REG(0x12, 0x9b) /* read/write */
+# define TX4_PD_RAM   (1 << 1)
 #define REG_TX33  REG(0x12, 0xb8) /* read/write */
 # define TX33_HDMI(1 << 1)

@@ -673,6 +675,7 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, 
int blk)
 static uint8_t *
 do_get_edid(struct drm_encoder *encoder)
 {
+   struct tda998x_priv *priv = to_tda998x_priv(encoder);
int j = 0, valid_extensions = 0;
uint8_t *block, *new;
bool print_bad_edid = drm_debug & DRM_UT_KMS;
@@ -680,6 +683,9 @@ do_get_edid(struct drm_encoder *encoder)
if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
return NULL;

+   if (priv->rev == TDA19988)
+   reg_clear(encoder, REG_TX4, TX4_PD_RAM);
+
/* base block fetch */
if (read_edid_block(encoder, block, 0))
goto fail;
@@ -689,7 +695,7 @@ do_get_edid(struct drm_encoder *encoder)

/* if there's no extensions, we're done */
if (block[0x7e] == 0)
-   return block;
+   goto done;

new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
if (!new)
@@ -716,9 +722,15 @@ do_get_edid(struct drm_encoder *encoder)
block = new;
}

+done:
+   if (priv->rev == TDA19988)
+   reg_set(encoder, REG_TX4, TX4_PD_RAM);
+
return block;

 fail:
+   if (priv->rev == TDA19988)
+   reg_set(encoder, REG_TX4, TX4_PD_RAM);
dev_warn(encoder->dev->dev, "failed to read EDID\n");
kfree(block);
return NULL;
-- 
1.7.10.4

[PATCH v2 0/8] Several NXP TDA998x patches

[Sebastian Hesselbarth]

This patch set picks up several patches sent during the past months
related with NXP TDA998x HDMI transmitter driver. The patches have
been tested on Marvell Dove (Armada DRM) on several HDMI and DVI modes
with audio playing on S/PDIF.

I bumped all patches to v2, although only patches 2, 5, and 6 have
changed. I also fixed a typo in commit line of patch 8. As Darren
Etheridge already gave his Tested-by and tilcdc related patches have
not changed, I added it to all patches for v2, too.

I have not added Russell King's Tested-by, as I hope he will have a
close look at what I changed after his review comments.

Darren Etheridge (1):
  drm/tilcdc fixup mode to workaround sync for tda998x

Russell King (5):
  drm/i2c: tda998x: fix EDID reading on TDA19988 devices
  drm/i2c: tda998x: ensure VIP output mux is properly set
  drm/i2c: tda998x: fix npix/nline programming
  drm/i2c: tda998x: prepare for video input configuration
  drm/i2c: tda998x: add video and audio input configuration

Sebastian Hesselbarth (2):
  drm/i2c: tda998x: fix sync generation and calculation
  drm/i2c: tda998x: prepare for broken sync workaround

 drivers/gpu/drm/i2c/tda998x_drv.c |  481 +++--
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c  |7 +-
 drivers/gpu/drm/tilcdc/tilcdc_slave.c |   27 +-
 include/drm/i2c/tda998x.h |   30 ++
 4 files changed, 467 insertions(+), 78 deletions(-)
 create mode 100644 include/drm/i2c/tda998x.h

---
Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
-- 
1.7.10.4

[PATCH 5/8] drm/i2c: tda998x: add video and audio input configuration

[Sebastian Hesselbarth]

On 08/14/13 16:12, Russell King - ARM Linux wrote:
> On Tue, Aug 06, 2013 at 12:20:15AM +0200, Sebastian Hesselbarth wrote:
>> @@ -0,0 +1,23 @@
>> +#ifndef __TDA998X_H__
>> +#define __TDA998X_H__
>> +
>> +enum tda998x_audio_format {
>> +AFMT_I2S,
>> +AFMT_SPDIF,
>> +};
>> +
>> +struct tda998x_encoder_params {
>> +int audio_cfg;
>> +int audio_clk_cfg;
>> +enum tda998x_audio_format audio_format;
>> +int audio_sample_rate;
>> +char audio_frame[6];
>> +int swap_a, mirr_a;
>> +int swap_b, mirr_b;
>> +int swap_c, mirr_c;
>> +int swap_d, mirr_d;
>> +int swap_e, mirr_e;
>> +int swap_f, mirr_f;
>> +};
>
> You really should pick my version of this header file from the message
> I sent earlier:
>
>   E1UllOe-00058q-Ep at rmk-PC.arm.linux.org.uk
>   [PATCH RFC 7/8] drm/i2c: nxp-tda998x: add video and audio input 
> configuration
>
> if you're going to suggest that it's something I produced.
>

Right, that one above must have been the one I made up from the
one RFC you forgot to add it. I must have messed it up and will
take yours, of course.

Sebastian

[PATCH 6/8] drm/i2c: tda998x: fix sync generation and calculation

[Sebastian Hesselbarth]

On 08/14/13 14:41, Russell King - ARM Linux wrote:
> On Tue, Aug 06, 2013 at 12:20:16AM +0200, Sebastian Hesselbarth wrote:
>> +de_pix_s = mode->htotal - mode->hdisplay;
>> +de_pix_e = de_pix_s + mode->hdisplay;
>> +hs_pix_s = mode->hsync_start - mode->hdisplay;
>> +hs_pix_e = hs_pix_s + mode->hsync_end - mode->hsync_start;
>
> I still think the above is over-complicating the calculation and making it
> less readable.

Yeah, you also didn't like it the last time. I must admit, I have
left it that way because I did all the near-end/far-end scope checks
on the calculation above and I wasn't that eager to touch them again.

But I agree and will revert the lines in question and update the
others accordingly.

> The values in 'mode' represent this timing representation:
> 0=hdshdehss  hse   ht
> |-|->|--->|>|
>
> What we want to do is to rotate that around to this:
> 0 hss  hse   hds   ht=hde
> |->|--->|>|-|
>
>  From that, you can see quite clearly that the end of the displayed line
> is now at htotal, and the start of the displayed line (hds) is hdisplay
> clocks before that.  So:
>
>   de_pix_e = mode->htotal;
>   de_pix_s = de_pix_e - mode->hdisplay;
>
> Everything else (hss, hse) has moved to the left by hdisplay clocks, so:
>
>   hs_pix_s = mode->hsync_start - mode->hdisplay;
>   hs_pix_e = mode->hsync_end - mode->hdisplay;
>
> That's what you get if you simplify your calculations as well.  If we then
> go back and look at the original code:
>
> -   hs_start   = mode->hsync_start - mode->hdisplay;
> -   hs_end = mode->hsync_end - mode->hdisplay;
> -   de_start   = mode->htotal - mode->hdisplay;
> -   de_end = mode->htotal;
>
> it's what was originally there, so I don't see any point in touching that
> calculation.
>
> We also have:
>
> -   ref_pix = 3 + hs_start;
> +   ref_pix  = 3 + mode->hsync_start - mode->hdisplay;
>
> which we can see is also the same calculation in essence.  I don't think
> the change helps readability.
>

Re: [PATCH 5/8] drm/i2c: tda998x: add video and audio input configuration

[Sebastian Hesselbarth]

On 08/14/13 16:12, Russell King - ARM Linux wrote:

On Tue, Aug 06, 2013 at 12:20:15AM +0200, Sebastian Hesselbarth wrote:

@@ -0,0 +1,23 @@
+#ifndef __TDA998X_H__
+#define __TDA998X_H__
+
+enum tda998x_audio_format {
+   AFMT_I2S,
+   AFMT_SPDIF,
+};
+
+struct tda998x_encoder_params {
+   int audio_cfg;
+   int audio_clk_cfg;
+   enum tda998x_audio_format audio_format;
+   int audio_sample_rate;
+   char audio_frame[6];
+   int swap_a, mirr_a;
+   int swap_b, mirr_b;
+   int swap_c, mirr_c;
+   int swap_d, mirr_d;
+   int swap_e, mirr_e;
+   int swap_f, mirr_f;
+};


You really should pick my version of this header file from the message
I sent earlier:

e1ulloe-00058q...@rmk-pc.arm.linux.org.uk
[PATCH RFC 7/8] drm/i2c: nxp-tda998x: add video and audio input 
configuration

if you're going to suggest that it's something I produced.



Right, that one above must have been the one I made up from the
one RFC you forgot to add it. I must have messed it up and will
take yours, of course.

Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/8] drm/i2c: tda998x: fix sync generation and calculation

[Sebastian Hesselbarth]

On 08/14/13 14:41, Russell King - ARM Linux wrote:

On Tue, Aug 06, 2013 at 12:20:16AM +0200, Sebastian Hesselbarth wrote:

+   de_pix_s = mode-htotal - mode-hdisplay;
+   de_pix_e = de_pix_s + mode-hdisplay;
+   hs_pix_s = mode-hsync_start - mode-hdisplay;
+   hs_pix_e = hs_pix_s + mode-hsync_end - mode-hsync_start;


I still think the above is over-complicating the calculation and making it
less readable.


Yeah, you also didn't like it the last time. I must admit, I have
left it that way because I did all the near-end/far-end scope checks
on the calculation above and I wasn't that eager to touch them again.

But I agree and will revert the lines in question and update the
others accordingly.


The values in 'mode' represent this timing representation:
0=hdshdehss  hse   ht
|-|-|---||

What we want to do is to rotate that around to this:
0 hss  hse   hds   ht=hde
|-|---||-|

 From that, you can see quite clearly that the end of the displayed line
is now at htotal, and the start of the displayed line (hds) is hdisplay
clocks before that.  So:

de_pix_e = mode-htotal;
de_pix_s = de_pix_e - mode-hdisplay;

Everything else (hss, hse) has moved to the left by hdisplay clocks, so:

hs_pix_s = mode-hsync_start - mode-hdisplay;
hs_pix_e = mode-hsync_end - mode-hdisplay;

That's what you get if you simplify your calculations as well.  If we then
go back and look at the original code:

-   hs_start   = mode-hsync_start - mode-hdisplay;
-   hs_end = mode-hsync_end - mode-hdisplay;
-   de_start   = mode-htotal - mode-hdisplay;
-   de_end = mode-htotal;

it's what was originally there, so I don't see any point in touching that
calculation.

We also have:

-   ref_pix = 3 + hs_start;
+   ref_pix  = 3 + mode-hsync_start - mode-hdisplay;

which we can see is also the same calculation in essence.  I don't think
the change helps readability.



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/8] drm/i2c: tda998x: fix EDID reading on TDA19988 devices

[Sebastian Hesselbarth]

From: Russell King rmk+ker...@arm.linux.org.uk

TDA19988 devices need their RAM enabled in order to read EDID
information.  Add support for this.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Signed-off-by: Rob Clark robdcl...@gmail.com
Tested-by: Darren Etheridge detheri...@ti.com
Tested-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
---
Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |   14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index e68b58a..d71c408 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -229,6 +229,8 @@ struct tda998x_priv {
 
 /* Page 12h: HDCP and OTP */
 #define REG_TX3   REG(0x12, 0x9a) /* read/write */
+#define REG_TX4   REG(0x12, 0x9b) /* read/write */
+# define TX4_PD_RAM   (1  1)
 #define REG_TX33  REG(0x12, 0xb8) /* read/write */
 # define TX33_HDMI(1  1)
 
@@ -673,6 +675,7 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, 
int blk)
 static uint8_t *
 do_get_edid(struct drm_encoder *encoder)
 {
+   struct tda998x_priv *priv = to_tda998x_priv(encoder);
int j = 0, valid_extensions = 0;
uint8_t *block, *new;
bool print_bad_edid = drm_debug  DRM_UT_KMS;
@@ -680,6 +683,9 @@ do_get_edid(struct drm_encoder *encoder)
if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
return NULL;
 
+   if (priv-rev == TDA19988)
+   reg_clear(encoder, REG_TX4, TX4_PD_RAM);
+
/* base block fetch */
if (read_edid_block(encoder, block, 0))
goto fail;
@@ -689,7 +695,7 @@ do_get_edid(struct drm_encoder *encoder)
 
/* if there's no extensions, we're done */
if (block[0x7e] == 0)
-   return block;
+   goto done;
 
new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
if (!new)
@@ -716,9 +722,15 @@ do_get_edid(struct drm_encoder *encoder)
block = new;
}
 
+done:
+   if (priv-rev == TDA19988)
+   reg_set(encoder, REG_TX4, TX4_PD_RAM);
+
return block;
 
 fail:
+   if (priv-rev == TDA19988)
+   reg_set(encoder, REG_TX4, TX4_PD_RAM);
dev_warn(encoder-dev-dev, failed to read EDID\n);
kfree(block);
return NULL;
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/8] drm/i2c: tda998x: fix npix/nline programming

[Sebastian Hesselbarth]

From: Russell King rmk+ker...@arm.linux.org.uk

The npix/nline registers are supposed to be programmed with the total
number of pixels/lines, not the displayed pixels/lines, and not minus
one either.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Tested-by: Darren Etheridge detheri...@ti.com
Tested-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
---
Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index cb9b13a..a701411 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -587,8 +587,8 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
reg_set(encoder, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
 
reg_write(encoder, REG_VIDFORMAT, 0x00);
-   reg_write16(encoder, REG_NPIX_MSB, mode-hdisplay - 1);
-   reg_write16(encoder, REG_NLINE_MSB, mode-vdisplay - 1);
+   reg_write16(encoder, REG_NPIX_MSB, mode-htotal);
+   reg_write16(encoder, REG_NLINE_MSB, mode-vtotal);
reg_write16(encoder, REG_VS_LINE_STRT_1_MSB, line_start);
reg_write16(encoder, REG_VS_LINE_END_1_MSB, line_end);
reg_write16(encoder, REG_VS_PIX_STRT_1_MSB, hs_start);
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/8] drm/i2c: tda998x: ensure VIP output mux is properly set

[Sebastian Hesselbarth]

From: Russell King rmk+ker...@arm.linux.org.uk

When switching between various drivers for this device, it's possible
that some critical registers are left containing values which affect
the device operation.  One such case encountered is the VIP output
mux register.  This defaults to 0x24 on powerup, but other drivers may
set this to 0x12.  This results in incorrect colours.

Fix this by ensuring that the register is always set to the power on
default setting.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Tested-by: Darren Etheridge detheri...@ti.com
Tested-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
---
Changelog:
v1-v2:
- move reg_write to tda998x_reset as last patch was based on an
  old version (Reported by Russell King)

Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index d71c408..cb9b13a 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -110,6 +110,7 @@ struct tda998x_priv {
 #define REG_VIP_CNTRL_5   REG(0x00, 0x25) /* write */
 # define VIP_CNTRL_5_CKCASE   (1  0)
 # define VIP_CNTRL_5_SP_CNT(x)(((x)  3)  1)
+#define REG_MUX_VP_VIP_OUTREG(0x00, 0x27) /* read/write */
 #define REG_MAT_CONTRLREG(0x00, 0x80) /* write */
 # define MAT_CONTRL_MAT_SC(x) (((x)  3)  0)
 # define MAT_CONTRL_MAT_BP(1  2)
@@ -415,6 +416,9 @@ tda998x_reset(struct drm_encoder *encoder)
reg_write(encoder, REG_PLL_SCGR1,0x5b);
reg_write(encoder, REG_PLL_SCGR2,0x00);
reg_write(encoder, REG_PLL_SCG2, 0x10);
+
+   /* Write the default value MUX register */
+   reg_write(encoder, REG_MUX_VP_VIP_OUT, 0x24);
 }
 
 /* DRM encoder functions */
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/8] Several NXP TDA998x patches

[Sebastian Hesselbarth]

This patch set picks up several patches sent during the past months
related with NXP TDA998x HDMI transmitter driver. The patches have
been tested on Marvell Dove (Armada DRM) on several HDMI and DVI modes
with audio playing on S/PDIF.

I bumped all patches to v2, although only patches 2, 5, and 6 have
changed. I also fixed a typo in commit line of patch 8. As Darren
Etheridge already gave his Tested-by and tilcdc related patches have
not changed, I added it to all patches for v2, too.

I have not added Russell King's Tested-by, as I hope he will have a
close look at what I changed after his review comments.

Darren Etheridge (1):
  drm/tilcdc fixup mode to workaround sync for tda998x

Russell King (5):
  drm/i2c: tda998x: fix EDID reading on TDA19988 devices
  drm/i2c: tda998x: ensure VIP output mux is properly set
  drm/i2c: tda998x: fix npix/nline programming
  drm/i2c: tda998x: prepare for video input configuration
  drm/i2c: tda998x: add video and audio input configuration

Sebastian Hesselbarth (2):
  drm/i2c: tda998x: fix sync generation and calculation
  drm/i2c: tda998x: prepare for broken sync workaround

 drivers/gpu/drm/i2c/tda998x_drv.c |  481 +++--
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c  |7 +-
 drivers/gpu/drm/tilcdc/tilcdc_slave.c |   27 +-
 include/drm/i2c/tda998x.h |   30 ++
 4 files changed, 467 insertions(+), 78 deletions(-)
 create mode 100644 include/drm/i2c/tda998x.h

---
Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/8] drm/i2c: tda998x: prepare for video input configuration

[Sebastian Hesselbarth]

From: Russell King rmk+ker...@arm.linux.org.uk

The video-input-port (VIP) is highly configurable. This prepares
current driver to allow to configure VIP configuration, as some
boards connect lcd controller and TDA998x pin-swapped and depend
on VIP to swap the pins by register configuration.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Tested-by: Darren Etheridge detheri...@ti.com
Tested-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
---
Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |   16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index a701411..527d11b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -32,6 +32,9 @@ struct tda998x_priv {
uint16_t rev;
uint8_t current_page;
int dpms;
+   u8 vip_cntrl_0;
+   u8 vip_cntrl_1;
+   u8 vip_cntrl_2;
 };
 
 #define to_tda998x_priv(x)  ((struct tda998x_priv 
*)to_encoder_slave(x)-slave_priv)
@@ -448,12 +451,9 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
reg_write(encoder, REG_ENA_VP_1, 0xff);
reg_write(encoder, REG_ENA_VP_2, 0xff);
/* set muxing after enabling ports: */
-   reg_write(encoder, REG_VIP_CNTRL_0,
-   VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3));
-   reg_write(encoder, REG_VIP_CNTRL_1,
-   VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1));
-   reg_write(encoder, REG_VIP_CNTRL_2,
-   VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5));
+   reg_write(encoder, REG_VIP_CNTRL_0, priv-vip_cntrl_0);
+   reg_write(encoder, REG_VIP_CNTRL_1, priv-vip_cntrl_1);
+   reg_write(encoder, REG_VIP_CNTRL_2, priv-vip_cntrl_2);
break;
case DRM_MODE_DPMS_OFF:
/* disable audio and video ports */
@@ -823,6 +823,10 @@ tda998x_encoder_init(struct i2c_client *client,
if (!priv)
return -ENOMEM;
 
+   priv-vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
+   priv-vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
+   priv-vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
+
priv-current_page = 0;
priv-cec = i2c_new_dummy(client-adapter, 0x34);
priv-dpms = DRM_MODE_DPMS_OFF;
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

[Sebastian Hesselbarth]

From: Russell King rmk+ker...@arm.linux.org.uk

This patch adds tda998x specific parameters to allow it to be configured
for different boards using it. Also, this implements rudimentary audio
support for S/PDIF attached controllers.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
Tested-by: Darren Etheridge detheri...@ti.com
---
Changelog:
for v1:
- set AUDIO_DIV to SERCLK/16 for modes with pixclk 100MHz
- also calculate CTS
v1-v2:
- Remove CTS calculation as it isn't used in current TDA998x setup
  (Reported by Russell King)
- Remove debug left-over (Reported by Russell King)
- Use correct tda998x include (Reported by Russell King)

Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |  268 +++--
 include/drm/i2c/tda998x.h |   30 +
 2 files changed, 290 insertions(+), 8 deletions(-)
 create mode 100644 include/drm/i2c/tda998x.h

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 527d11b..2b64dfa 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -23,7 +23,7 @@
 #include drm/drm_crtc_helper.h
 #include drm/drm_encoder_slave.h
 #include drm/drm_edid.h
-
+#include drm/i2c/tda998x.h
 
 #define DBG(fmt, ...) DRM_DEBUG(fmt\n, ##__VA_ARGS__)
 
@@ -32,9 +32,11 @@ struct tda998x_priv {
uint16_t rev;
uint8_t current_page;
int dpms;
+   bool is_hdmi_sink;
u8 vip_cntrl_0;
u8 vip_cntrl_1;
u8 vip_cntrl_2;
+   struct tda998x_encoder_params params;
 };
 
 #define to_tda998x_priv(x)  ((struct tda998x_priv 
*)to_encoder_slave(x)-slave_priv)
@@ -71,10 +73,13 @@ struct tda998x_priv {
 # define I2C_MASTER_DIS_MM(1  0)
 # define I2C_MASTER_DIS_FILT  (1  1)
 # define I2C_MASTER_APP_STRT_LAT  (1  2)
+#define REG_FEAT_POWERDOWNREG(0x00, 0x0e) /* read/write */
+# define FEAT_POWERDOWN_SPDIF (1  3)
 #define REG_INT_FLAGS_0   REG(0x00, 0x0f) /* read/write */
 #define REG_INT_FLAGS_1   REG(0x00, 0x10) /* read/write */
 #define REG_INT_FLAGS_2   REG(0x00, 0x11) /* read/write */
 # define INT_FLAGS_2_EDID_BLK_RD  (1  1)
+#define REG_ENA_ACLK  REG(0x00, 0x16) /* read/write */
 #define REG_ENA_VP_0  REG(0x00, 0x18) /* read/write */
 #define REG_ENA_VP_1  REG(0x00, 0x19) /* read/write */
 #define REG_ENA_VP_2  REG(0x00, 0x1a) /* read/write */
@@ -113,6 +118,7 @@ struct tda998x_priv {
 #define REG_VIP_CNTRL_5   REG(0x00, 0x25) /* write */
 # define VIP_CNTRL_5_CKCASE   (1  0)
 # define VIP_CNTRL_5_SP_CNT(x)(((x)  3)  1)
+#define REG_MUX_APREG(0x00, 0x26) /* read/write */
 #define REG_MUX_VP_VIP_OUTREG(0x00, 0x27) /* read/write */
 #define REG_MAT_CONTRLREG(0x00, 0x80) /* write */
 # define MAT_CONTRL_MAT_SC(x) (((x)  3)  0)
@@ -175,6 +181,12 @@ struct tda998x_priv {
 # define HVF_CNTRL_1_PAD(x)   (((x)  3)  4)
 # define HVF_CNTRL_1_SEMI_PLANAR  (1  6)
 #define REG_RPT_CNTRL REG(0x00, 0xf0) /* write */
+#define REG_I2S_FORMATREG(0x00, 0xfc) /* read/write */
+# define I2S_FORMAT(x)(((x)  3)  0)
+#define REG_AIP_CLKSELREG(0x00, 0xfd) /* write */
+# define AIP_CLKSEL_FS(x) (((x)  3)  0)
+# define AIP_CLKSEL_CLK_POL(x)(((x)  1)  2)
+# define AIP_CLKSEL_AIP(x)(((x)  7)  3)
 
 
 /* Page 02h: PLL settings */
@@ -198,6 +210,12 @@ struct tda998x_priv {
 #define REG_PLL_SCGR1 REG(0x02, 0x09) /* read/write */
 #define REG_PLL_SCGR2 REG(0x02, 0x0a) /* read/write */
 #define REG_AUDIO_DIV REG(0x02, 0x0e) /* read/write */
+# define AUDIO_DIV_SERCLK_1   0
+# define AUDIO_DIV_SERCLK_2   1
+# define AUDIO_DIV_SERCLK_4   2
+# define AUDIO_DIV_SERCLK_8   3
+# define AUDIO_DIV_SERCLK_16  4
+# define AUDIO_DIV_SERCLK_32  5
 #define REG_SEL_CLK   REG(0x02, 0x11) /* read/write */
 # define SEL_CLK_SEL_CLK1 (1  0)
 # define SEL_CLK_SEL_VRF_CLK(x)   (((x)  3)  1)
@@ -216,6 +234,11 @@ struct tda998x_priv {
 
 
 /* Page 10h: information frames and packets */
+#define REG_IF1_HB0   REG(0x10, 0x20) /* read/write */
+#define REG_IF2_HB0   REG(0x10, 0x40) /* read/write */
+#define REG_IF3_HB0   REG(0x10, 0x60) /* read/write */
+#define REG_IF4_HB0   REG(0x10, 0x80) /* read/write */
+#define REG_IF5_HB0   REG(0x10, 0xa0) /* read/write */
 
 
 /* Page 11h: audio settings and content info packets */
@@ -225,10 +248,33

[PATCH v2 7/8] drm/i2c: tda998x: prepare for broken sync workaround

[Sebastian Hesselbarth]

Some LCD controller cannot provide valid VESA style sync, i.e. coincident
HS/VS edges. First, this patch adds hskew passed from the adjusted_mode to
reference pixel calculation to allow those controllers to add an offset
relative to the expected reference pixel.

Signed-off-by: Darren Etheridge detheri...@ti.com
Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
---
Changelog:
for v1:
- reword comment

Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 92fcb3d..c2bd711 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -782,6 +782,14 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
de_pix_s = mode-htotal - mode-hdisplay;
ref_pix  = 3 + hs_pix_s;
 
+   /*
+* Attached LCD controllers may generate broken sync. Allow
+* those to adjust the position of the rising VS edge by adding
+* HSKEW to ref_pix.
+*/
+   if (adjusted_mode-flags  DRM_MODE_FLAG_HSKEW)
+   ref_pix += adjusted_mode-hskew;
+
if ((mode-flags  DRM_MODE_FLAG_INTERLACE) == 0) {
ref_line = 1 + mode-vsync_start - mode-vdisplay;
vwin1_line_s = mode-vtotal - mode-vdisplay - 1;
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 6/8] drm/i2c: tda998x: fix sync generation and calculation

[Sebastian Hesselbarth]

This fixes the wrong sync generation and sync calculation of TDA998x
for HS/VS-based sync detection.

Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
Tested-by: Darren Etheridge detheri...@ti.com
---
Changelog:
v1-v2:
- revert calculation of hs/de_pix_s/e (Reported by Russell King)

Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |  181 +++--
 1 file changed, 115 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 2b64dfa..92fcb3d 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -140,8 +140,12 @@ struct tda998x_priv {
 #define REG_VS_LINE_END_1_LSB REG(0x00, 0xae) /* write */
 #define REG_VS_PIX_END_1_MSB  REG(0x00, 0xaf) /* write */
 #define REG_VS_PIX_END_1_LSB  REG(0x00, 0xb0) /* write */
+#define REG_VS_LINE_STRT_2_MSBREG(0x00, 0xb1) /* write */
+#define REG_VS_LINE_STRT_2_LSBREG(0x00, 0xb2) /* write */
 #define REG_VS_PIX_STRT_2_MSB REG(0x00, 0xb3) /* write */
 #define REG_VS_PIX_STRT_2_LSB REG(0x00, 0xb4) /* write */
+#define REG_VS_LINE_END_2_MSB REG(0x00, 0xb5) /* write */
+#define REG_VS_LINE_END_2_LSB REG(0x00, 0xb6) /* write */
 #define REG_VS_PIX_END_2_MSB  REG(0x00, 0xb7) /* write */
 #define REG_VS_PIX_END_2_LSB  REG(0x00, 0xb8) /* write */
 #define REG_HS_PIX_START_MSB  REG(0x00, 0xb9) /* write */
@@ -152,21 +156,29 @@ struct tda998x_priv {
 #define REG_VWIN_START_1_LSB  REG(0x00, 0xbe) /* write */
 #define REG_VWIN_END_1_MSBREG(0x00, 0xbf) /* write */
 #define REG_VWIN_END_1_LSBREG(0x00, 0xc0) /* write */
+#define REG_VWIN_START_2_MSB  REG(0x00, 0xc1) /* write */
+#define REG_VWIN_START_2_LSB  REG(0x00, 0xc2) /* write */
+#define REG_VWIN_END_2_MSBREG(0x00, 0xc3) /* write */
+#define REG_VWIN_END_2_LSBREG(0x00, 0xc4) /* write */
 #define REG_DE_START_MSB  REG(0x00, 0xc5) /* write */
 #define REG_DE_START_LSB  REG(0x00, 0xc6) /* write */
 #define REG_DE_STOP_MSB   REG(0x00, 0xc7) /* write */
 #define REG_DE_STOP_LSB   REG(0x00, 0xc8) /* write */
 #define REG_TBG_CNTRL_0   REG(0x00, 0xca) /* write */
+# define TBG_CNTRL_0_TOP_TGL  (1  0)
+# define TBG_CNTRL_0_TOP_SEL  (1  1)
+# define TBG_CNTRL_0_DE_EXT   (1  2)
+# define TBG_CNTRL_0_TOP_EXT  (1  3)
 # define TBG_CNTRL_0_FRAME_DIS(1  5)
 # define TBG_CNTRL_0_SYNC_MTHD(1  6)
 # define TBG_CNTRL_0_SYNC_ONCE(1  7)
 #define REG_TBG_CNTRL_1   REG(0x00, 0xcb) /* write */
-# define TBG_CNTRL_1_VH_TGL_0 (1  0)
-# define TBG_CNTRL_1_VH_TGL_1 (1  1)
-# define TBG_CNTRL_1_VH_TGL_2 (1  2)
-# define TBG_CNTRL_1_VHX_EXT_DE   (1  3)
-# define TBG_CNTRL_1_VHX_EXT_HS   (1  4)
-# define TBG_CNTRL_1_VHX_EXT_VS   (1  5)
+# define TBG_CNTRL_1_H_TGL(1  0)
+# define TBG_CNTRL_1_V_TGL(1  1)
+# define TBG_CNTRL_1_TGL_EN   (1  2)
+# define TBG_CNTRL_1_X_EXT(1  3)
+# define TBG_CNTRL_1_H_EXT(1  4)
+# define TBG_CNTRL_1_V_EXT(1  5)
 # define TBG_CNTRL_1_DWIN_DIS (1  6)
 #define REG_ENABLE_SPACE  REG(0x00, 0xd6) /* write */
 #define REG_HVF_CNTRL_0   REG(0x00, 0xe4) /* write */
@@ -735,43 +747,70 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
struct drm_display_mode *adjusted_mode)
 {
struct tda998x_priv *priv = to_tda998x_priv(encoder);
-   uint16_t hs_start, hs_end, line_start, line_end;
-   uint16_t vwin_start, vwin_end, de_start, de_end;
-   uint16_t ref_pix, ref_line, pix_start2;
+   uint16_t ref_pix, ref_line, n_pix, n_line;
+   uint16_t hs_pix_s, hs_pix_e;
+   uint16_t vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
+   uint16_t vs2_pix_s, vs2_pix_e, vs2_line_s, vs2_line_e;
+   uint16_t vwin1_line_s, vwin1_line_e;
+   uint16_t vwin2_line_s, vwin2_line_e;
+   uint16_t de_pix_s, de_pix_e;
uint8_t reg, div, rep;
 
-   hs_start   = mode-hsync_start - mode-hdisplay;
-   hs_end = mode-hsync_end - mode-hdisplay;
-   line_start = 1;
-   line_end   = 1 + mode-vsync_end - mode-vsync_start;
-   vwin_start = mode-vtotal - mode-vsync_start;
-   vwin_end   = vwin_start + mode-vdisplay;
-   de_start   = mode-htotal - mode-hdisplay;
-   de_end = mode-htotal;
-
-   pix_start2 = 0;
-   if (mode-flags  DRM_MODE_FLAG_INTERLACE)
-   pix_start2 = (mode-htotal / 2) + hs_start;
-
-   /* TODO how is this value calculated?  It is 2 for all common
-* formats in the tables in out

[PATCH v2 8/8] drm/tilcdc fixup mode to workaround sync for tda998x

[Sebastian Hesselbarth]

From: Darren Etheridge detheri...@ti.com

Add a fixup function that will flip the hsync priority and
add a hskew value that is used to shift the tda998x to the
right by a variable number of pixels depending on the mode.
This works around an issue with the sync timings that tilcdc
is outputing.

Signed-off-by: Darren Etheridge detheri...@ti.com
Tested-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
---
Changelog:
v1-v2:
- fix typo in commit line (s/workaound/workaround)

Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c  |7 ++-
 drivers/gpu/drm/tilcdc/tilcdc_slave.c |   27 ++-
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 7418dcd..6d05240 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -379,7 +379,12 @@ static int tilcdc_crtc_mode_set(struct drm_crtc *crtc,
else
tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_EDGE);
 
-   if (mode-flags  DRM_MODE_FLAG_NHSYNC)
+   /*
+* use value from adjusted_mode here as this might have been
+* changed as part of the fixup for slave encoders to solve the
+* issue where tilcdc timings are not VESA compliant
+*/
+   if (adjusted_mode-flags  DRM_MODE_FLAG_NHSYNC)
tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
else
tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c 
b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
index 0bf5999..f6e9c26 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
@@ -73,13 +73,38 @@ static void slave_encoder_prepare(struct drm_encoder 
*encoder)
tilcdc_crtc_set_panel_info(encoder-crtc, slave_info);
 }
 
+static bool slave_encoder_fixup(struct drm_encoder *encoder,
+   const struct drm_display_mode *mode,
+   struct drm_display_mode *adjusted_mode)
+{
+   /*
+* tilcdc does not generate VESA-complient sync but aligns
+* VS on the second edge of HS instead of first edge.
+* We use adjusted_mode, to fixup sync by aligning both rising
+* edges and add HSKEW offset to let the slave encoder fix it up.
+*/
+   adjusted_mode-hskew = mode-hsync_end - mode-hsync_start;
+   adjusted_mode-flags |= DRM_MODE_FLAG_HSKEW;
+
+   if (mode-flags  DRM_MODE_FLAG_NHSYNC) {
+   adjusted_mode-flags |= DRM_MODE_FLAG_PHSYNC;
+   adjusted_mode-flags = ~DRM_MODE_FLAG_NHSYNC;
+   } else {
+   adjusted_mode-flags |= DRM_MODE_FLAG_NHSYNC;
+   adjusted_mode-flags = ~DRM_MODE_FLAG_PHSYNC;
+   }
+
+   return drm_i2c_encoder_mode_fixup(encoder, mode, adjusted_mode);
+}
+
+
 static const struct drm_encoder_funcs slave_encoder_funcs = {
.destroy= slave_encoder_destroy,
 };
 
 static const struct drm_encoder_helper_funcs slave_encoder_helper_funcs = {
.dpms   = drm_i2c_encoder_dpms,
-   .mode_fixup = drm_i2c_encoder_mode_fixup,
+   .mode_fixup = slave_encoder_fixup,
.prepare= slave_encoder_prepare,
.commit = drm_i2c_encoder_commit,
.mode_set   = drm_i2c_encoder_mode_set,
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 8/8] drm/tilcdc fixup mode to workaound sync for tda998x

[Sebastian Hesselbarth]

From: Darren Etheridge 

Add a fixup function that will flip the hsync priority and
add a hskew value that is used to shift the tda998x to the
right by a variable number of pixels depending on the mode.
This works around an issue with the sync timings that tilcdc
is outputing.

Signed-off-by: Darren Etheridge 
---
Changelog:
for v1:
- reword comment

Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King 
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c  |7 ++-
 drivers/gpu/drm/tilcdc/tilcdc_slave.c |   27 ++-
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 7418dcd..6d05240 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -379,7 +379,12 @@ static int tilcdc_crtc_mode_set(struct drm_crtc *crtc,
else
tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_EDGE);

-   if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+   /*
+* use value from adjusted_mode here as this might have been
+* changed as part of the fixup for slave encoders to solve the
+* issue where tilcdc timings are not VESA compliant
+*/
+   if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
else
tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c 
b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
index 0bf5999..f6e9c26 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
@@ -73,13 +73,38 @@ static void slave_encoder_prepare(struct drm_encoder 
*encoder)
tilcdc_crtc_set_panel_info(encoder->crtc, _info);
 }

+static bool slave_encoder_fixup(struct drm_encoder *encoder,
+   const struct drm_display_mode *mode,
+   struct drm_display_mode *adjusted_mode)
+{
+   /*
+* tilcdc does not generate VESA-complient sync but aligns
+* VS on the second edge of HS instead of first edge.
+* We use adjusted_mode, to fixup sync by aligning both rising
+* edges and add HSKEW offset to let the slave encoder fix it up.
+*/
+   adjusted_mode->hskew = mode->hsync_end - mode->hsync_start;
+   adjusted_mode->flags |= DRM_MODE_FLAG_HSKEW;
+
+   if (mode->flags & DRM_MODE_FLAG_NHSYNC) {
+   adjusted_mode->flags |= DRM_MODE_FLAG_PHSYNC;
+   adjusted_mode->flags &= ~DRM_MODE_FLAG_NHSYNC;
+   } else {
+   adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC;
+   adjusted_mode->flags &= ~DRM_MODE_FLAG_PHSYNC;
+   }
+
+   return drm_i2c_encoder_mode_fixup(encoder, mode, adjusted_mode);
+}
+
+
 static const struct drm_encoder_funcs slave_encoder_funcs = {
.destroy= slave_encoder_destroy,
 };

 static const struct drm_encoder_helper_funcs slave_encoder_helper_funcs = {
.dpms   = drm_i2c_encoder_dpms,
-   .mode_fixup = drm_i2c_encoder_mode_fixup,
+   .mode_fixup = slave_encoder_fixup,
.prepare= slave_encoder_prepare,
.commit = drm_i2c_encoder_commit,
.mode_set   = drm_i2c_encoder_mode_set,
-- 
1.7.10.4

[PATCH 7/8] drm/i2c: tda998x: prepare for broken sync workaround

[Sebastian Hesselbarth]

From: Darren Etheridge <detheri...@ti.com>

Some LCD controller cannot provide valid VESA style sync, i.e. coincident
HS/VS edges. First, this patch adds hskew passed from the adjusted_mode to
reference pixel calculation to allow those controllers to add an offset
relative to the expected reference pixel.

Signed-off-by: Darren Etheridge 
Signed-off-by: Sebastian Hesselbarth 
---
Changelog:
for v1:
- reword comment

Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 7bf227a..7dc5650 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -784,6 +784,15 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
n_line   = mode->vtotal;

ref_pix  = 3 + mode->hsync_start - mode->hdisplay;
+
+   /*
+* Attached LCD controllers may generate broken sync. Allow
+* those to adjust the position of the rising VS edge by adding
+* HSKEW to ref_pix.
+*/
+   if (adjusted_mode->flags & DRM_MODE_FLAG_HSKEW)
+   ref_pix += adjusted_mode->hskew;
+
de_pix_s = mode->htotal - mode->hdisplay;
de_pix_e = de_pix_s + mode->hdisplay;
hs_pix_s = mode->hsync_start - mode->hdisplay;
-- 
1.7.10.4

[PATCH 6/8] drm/i2c: tda998x: fix sync generation and calculation

[Sebastian Hesselbarth]

This fixes the wrong sync generation and sync calculation of TDA998x
for HS/VS-based sync detection.

Signed-off-by: Sebastian Hesselbarth 
---
Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |  181 +++--
 1 file changed, 115 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index da04939..7bf227a 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -140,8 +140,12 @@ struct tda998x_priv {
 #define REG_VS_LINE_END_1_LSB REG(0x00, 0xae) /* write */
 #define REG_VS_PIX_END_1_MSB  REG(0x00, 0xaf) /* write */
 #define REG_VS_PIX_END_1_LSB  REG(0x00, 0xb0) /* write */
+#define REG_VS_LINE_STRT_2_MSBREG(0x00, 0xb1) /* write */
+#define REG_VS_LINE_STRT_2_LSBREG(0x00, 0xb2) /* write */
 #define REG_VS_PIX_STRT_2_MSB REG(0x00, 0xb3) /* write */
 #define REG_VS_PIX_STRT_2_LSB REG(0x00, 0xb4) /* write */
+#define REG_VS_LINE_END_2_MSB REG(0x00, 0xb5) /* write */
+#define REG_VS_LINE_END_2_LSB REG(0x00, 0xb6) /* write */
 #define REG_VS_PIX_END_2_MSB  REG(0x00, 0xb7) /* write */
 #define REG_VS_PIX_END_2_LSB  REG(0x00, 0xb8) /* write */
 #define REG_HS_PIX_START_MSB  REG(0x00, 0xb9) /* write */
@@ -152,21 +156,29 @@ struct tda998x_priv {
 #define REG_VWIN_START_1_LSB  REG(0x00, 0xbe) /* write */
 #define REG_VWIN_END_1_MSBREG(0x00, 0xbf) /* write */
 #define REG_VWIN_END_1_LSBREG(0x00, 0xc0) /* write */
+#define REG_VWIN_START_2_MSB  REG(0x00, 0xc1) /* write */
+#define REG_VWIN_START_2_LSB  REG(0x00, 0xc2) /* write */
+#define REG_VWIN_END_2_MSBREG(0x00, 0xc3) /* write */
+#define REG_VWIN_END_2_LSBREG(0x00, 0xc4) /* write */
 #define REG_DE_START_MSB  REG(0x00, 0xc5) /* write */
 #define REG_DE_START_LSB  REG(0x00, 0xc6) /* write */
 #define REG_DE_STOP_MSB   REG(0x00, 0xc7) /* write */
 #define REG_DE_STOP_LSB   REG(0x00, 0xc8) /* write */
 #define REG_TBG_CNTRL_0   REG(0x00, 0xca) /* write */
+# define TBG_CNTRL_0_TOP_TGL  (1 << 0)
+# define TBG_CNTRL_0_TOP_SEL  (1 << 1)
+# define TBG_CNTRL_0_DE_EXT   (1 << 2)
+# define TBG_CNTRL_0_TOP_EXT  (1 << 3)
 # define TBG_CNTRL_0_FRAME_DIS(1 << 5)
 # define TBG_CNTRL_0_SYNC_MTHD(1 << 6)
 # define TBG_CNTRL_0_SYNC_ONCE(1 << 7)
 #define REG_TBG_CNTRL_1   REG(0x00, 0xcb) /* write */
-# define TBG_CNTRL_1_VH_TGL_0 (1 << 0)
-# define TBG_CNTRL_1_VH_TGL_1 (1 << 1)
-# define TBG_CNTRL_1_VH_TGL_2 (1 << 2)
-# define TBG_CNTRL_1_VHX_EXT_DE   (1 << 3)
-# define TBG_CNTRL_1_VHX_EXT_HS   (1 << 4)
-# define TBG_CNTRL_1_VHX_EXT_VS   (1 << 5)
+# define TBG_CNTRL_1_H_TGL(1 << 0)
+# define TBG_CNTRL_1_V_TGL(1 << 1)
+# define TBG_CNTRL_1_TGL_EN   (1 << 2)
+# define TBG_CNTRL_1_X_EXT(1 << 3)
+# define TBG_CNTRL_1_H_EXT(1 << 4)
+# define TBG_CNTRL_1_V_EXT(1 << 5)
 # define TBG_CNTRL_1_DWIN_DIS (1 << 6)
 #define REG_ENABLE_SPACE  REG(0x00, 0xd6) /* write */
 #define REG_HVF_CNTRL_0   REG(0x00, 0xe4) /* write */
@@ -742,43 +754,70 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
struct drm_display_mode *adjusted_mode)
 {
struct tda998x_priv *priv = to_tda998x_priv(encoder);
-   uint16_t hs_start, hs_end, line_start, line_end;
-   uint16_t vwin_start, vwin_end, de_start, de_end;
-   uint16_t ref_pix, ref_line, pix_start2;
+   uint16_t ref_pix, ref_line, n_pix, n_line;
+   uint16_t hs_pix_s, hs_pix_e;
+   uint16_t vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
+   uint16_t vs2_pix_s, vs2_pix_e, vs2_line_s, vs2_line_e;
+   uint16_t vwin1_line_s, vwin1_line_e;
+   uint16_t vwin2_line_s, vwin2_line_e;
+   uint16_t de_pix_s, de_pix_e;
uint8_t reg, div, rep;

-   hs_start   = mode->hsync_start - mode->hdisplay;
-   hs_end = mode->hsync_end - mode->hdisplay;
-   line_start = 1;
-   line_end   = 1 + mode->vsync_end - mode->vsync_start;
-   vwin_start = mode->vtotal - mode->vsync_start;
-   vwin_end   = vwin_start + mode->vdisplay;
-   de_start   = mode->htotal - mode->hdisplay;
-   de_end = mode->htotal;
-
-   pix_start2 = 0;
-   if (mode->flags & DRM_MODE_FLAG_INTERLACE)
-   pix_start2 = (mode->htotal / 2) + hs_start;
-
-   /* TODO how is this value calculated?  It is 2 for all common
-* formats in the tables in 

[PATCH 5/8] drm/i2c: tda998x: add video and audio input configuration

[Sebastian Hesselbarth]

From: Russell King <rmk+ker...@arm.linux.org.uk>

This patch adds tda998x specific parameters to allow it to be configured
for different boards using it. Also, this implements rudimentary audio
support for S/PDIF attached controllers.

Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
Signed-off-by: Sebastian Hesselbarth 
---
Changelog:
for v1:
- set AUDIO_DIV to SERCLK/16 for modes with pixclk >100MHz
- also calculate CTS

Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |  315 -
 include/drm/i2c/tda998x.h |   23 +++
 2 files changed, 330 insertions(+), 8 deletions(-)
 create mode 100644 include/drm/i2c/tda998x.h

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index d6afd02..da04939 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -23,7 +23,7 @@
 #include 
 #include 
 #include 
-
+#include 

 #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)

@@ -32,9 +32,11 @@ struct tda998x_priv {
uint16_t rev;
uint8_t current_page;
int dpms;
+   bool is_hdmi_sink;
u8 vip_cntrl_0;
u8 vip_cntrl_1;
u8 vip_cntrl_2;
+   struct tda998x_encoder_params params;
 };

 #define to_tda998x_priv(x)  ((struct tda998x_priv 
*)to_encoder_slave(x)->slave_priv)
@@ -71,10 +73,13 @@ struct tda998x_priv {
 # define I2C_MASTER_DIS_MM(1 << 0)
 # define I2C_MASTER_DIS_FILT  (1 << 1)
 # define I2C_MASTER_APP_STRT_LAT  (1 << 2)
+#define REG_FEAT_POWERDOWNREG(0x00, 0x0e) /* read/write */
+# define FEAT_POWERDOWN_SPDIF (1 << 3)
 #define REG_INT_FLAGS_0   REG(0x00, 0x0f) /* read/write */
 #define REG_INT_FLAGS_1   REG(0x00, 0x10) /* read/write */
 #define REG_INT_FLAGS_2   REG(0x00, 0x11) /* read/write */
 # define INT_FLAGS_2_EDID_BLK_RD  (1 << 1)
+#define REG_ENA_ACLK  REG(0x00, 0x16) /* read/write */
 #define REG_ENA_VP_0  REG(0x00, 0x18) /* read/write */
 #define REG_ENA_VP_1  REG(0x00, 0x19) /* read/write */
 #define REG_ENA_VP_2  REG(0x00, 0x1a) /* read/write */
@@ -113,6 +118,7 @@ struct tda998x_priv {
 #define REG_VIP_CNTRL_5   REG(0x00, 0x25) /* write */
 # define VIP_CNTRL_5_CKCASE   (1 << 0)
 # define VIP_CNTRL_5_SP_CNT(x)(((x) & 3) << 1)
+#define REG_MUX_APREG(0x00, 0x26) /* read/write */
 #define REG_MUX_VP_VIP_OUTREG(0x00, 0x27) /* read/write */
 #define REG_MAT_CONTRLREG(0x00, 0x80) /* write */
 # define MAT_CONTRL_MAT_SC(x) (((x) & 3) << 0)
@@ -175,6 +181,12 @@ struct tda998x_priv {
 # define HVF_CNTRL_1_PAD(x)   (((x) & 3) << 4)
 # define HVF_CNTRL_1_SEMI_PLANAR  (1 << 6)
 #define REG_RPT_CNTRL REG(0x00, 0xf0) /* write */
+#define REG_I2S_FORMATREG(0x00, 0xfc) /* read/write */
+# define I2S_FORMAT(x)(((x) & 3) << 0)
+#define REG_AIP_CLKSELREG(0x00, 0xfd) /* write */
+# define AIP_CLKSEL_FS(x) (((x) & 3) << 0)
+# define AIP_CLKSEL_CLK_POL(x)(((x) & 1) << 2)
+# define AIP_CLKSEL_AIP(x)(((x) & 7) << 3)


 /* Page 02h: PLL settings */
@@ -198,6 +210,12 @@ struct tda998x_priv {
 #define REG_PLL_SCGR1 REG(0x02, 0x09) /* read/write */
 #define REG_PLL_SCGR2 REG(0x02, 0x0a) /* read/write */
 #define REG_AUDIO_DIV REG(0x02, 0x0e) /* read/write */
+# define AUDIO_DIV_SERCLK_1   0
+# define AUDIO_DIV_SERCLK_2   1
+# define AUDIO_DIV_SERCLK_4   2
+# define AUDIO_DIV_SERCLK_8   3
+# define AUDIO_DIV_SERCLK_16  4
+# define AUDIO_DIV_SERCLK_32  5
 #define REG_SEL_CLK   REG(0x02, 0x11) /* read/write */
 # define SEL_CLK_SEL_CLK1 (1 << 0)
 # define SEL_CLK_SEL_VRF_CLK(x)   (((x) & 3) << 1)
@@ -216,6 +234,11 @@ struct tda998x_priv {


 /* Page 10h: information frames and packets */
+#define REG_IF1_HB0   REG(0x10, 0x20) /* read/write */
+#define REG_IF2_HB0   REG(0x10, 0x40) /* read/write */
+#define REG_IF3_HB0   REG(0x10, 0x60) /* read/write */
+#define REG_IF4_HB0   REG(0x10, 0x80) /* read/write */
+#define REG_IF5_HB0   REG(0x10, 0xa0) /* read/write */


 /* Page 11h: audio settings and content info packets */
@@ -225,10 +248,33 @@ struct tda998x_priv {
 # define AIP_CNTRL_0_LAYOUT   (1 << 2)
 # define AIP_CNTRL_0_ACR_MAN  (1 << 5)
 # define AIP_CNTRL_0_RST_CTS  (1 << 6)
+#define REG_CA_I2SREG(0x11, 0x01) /*

[PATCH 4/8] drm/i2c: tda998x: prepare for video input configuration

[Sebastian Hesselbarth]

From: Russell King <rmk+ker...@arm.linux.org.uk>

The video-input-port (VIP) is highly configurable. This prepares
current driver to allow to configure VIP configuration, as some
boards connect lcd controller and TDA998x "pin-swapped" and depend
on VIP to swap the pins by register configuration.

Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
Tested-by: Sebastian Hesselbarth 
---
Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |   16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index da2917b..d6afd02 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -32,6 +32,9 @@ struct tda998x_priv {
uint16_t rev;
uint8_t current_page;
int dpms;
+   u8 vip_cntrl_0;
+   u8 vip_cntrl_1;
+   u8 vip_cntrl_2;
 };

 #define to_tda998x_priv(x)  ((struct tda998x_priv 
*)to_encoder_slave(x)->slave_priv)
@@ -447,12 +450,9 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
reg_write(encoder, REG_ENA_VP_1, 0xff);
reg_write(encoder, REG_ENA_VP_2, 0xff);
/* set muxing after enabling ports: */
-   reg_write(encoder, REG_VIP_CNTRL_0,
-   VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3));
-   reg_write(encoder, REG_VIP_CNTRL_1,
-   VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1));
-   reg_write(encoder, REG_VIP_CNTRL_2,
-   VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5));
+   reg_write(encoder, REG_VIP_CNTRL_0, priv->vip_cntrl_0);
+   reg_write(encoder, REG_VIP_CNTRL_1, priv->vip_cntrl_1);
+   reg_write(encoder, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
break;
case DRM_MODE_DPMS_OFF:
/* disable audio and video ports */
@@ -822,6 +822,10 @@ tda998x_encoder_init(struct i2c_client *client,
if (!priv)
return -ENOMEM;

+   priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
+   priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
+   priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
+
priv->current_page = 0;
priv->cec = i2c_new_dummy(client->adapter, 0x34);
priv->dpms = DRM_MODE_DPMS_OFF;
-- 
1.7.10.4

[PATCH 3/8] drm/i2c: tda998x: fix npix/nline programming

[Sebastian Hesselbarth]

From: Russell King <rmk+ker...@arm.linux.org.uk>

The npix/nline registers are supposed to be programmed with the total
number of pixels/lines, not the displayed pixels/lines, and not minus
one either.

Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
Tested-by: Sebastian Hesselbarth 
---
Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 4b4db95..da2917b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -586,8 +586,8 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
reg_set(encoder, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);

reg_write(encoder, REG_VIDFORMAT, 0x00);
-   reg_write16(encoder, REG_NPIX_MSB, mode->hdisplay - 1);
-   reg_write16(encoder, REG_NLINE_MSB, mode->vdisplay - 1);
+   reg_write16(encoder, REG_NPIX_MSB, mode->htotal);
+   reg_write16(encoder, REG_NLINE_MSB, mode->vtotal);
reg_write16(encoder, REG_VS_LINE_STRT_1_MSB, line_start);
reg_write16(encoder, REG_VS_LINE_END_1_MSB, line_end);
reg_write16(encoder, REG_VS_PIX_STRT_1_MSB, hs_start);
-- 
1.7.10.4

[PATCH 2/8] drm/i2c: tda998x: ensure VIP output mux is properly set

[Sebastian Hesselbarth]

From: Russell King <rmk+ker...@arm.linux.org.uk>

When switching between various drivers for this device, it's possible
that some critical registers are left containing values which affect
the device operation.  One such case encountered is the VIP output
mux register.  This defaults to 0x24 on powerup, but other drivers may
set this to 0x12.  This results in incorrect colours.

Fix this by ensuring that the register is always set to the power on
default setting.

Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
Tested-by: Sebastian Hesselbarth 
---
Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index d71c408..4b4db95 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -110,6 +110,7 @@ struct tda998x_priv {
 #define REG_VIP_CNTRL_5   REG(0x00, 0x25) /* write */
 # define VIP_CNTRL_5_CKCASE   (1 << 0)
 # define VIP_CNTRL_5_SP_CNT(x)(((x) & 3) << 1)
+#define REG_MUX_VP_VIP_OUTREG(0x00, 0x27) /* read/write */
 #define REG_MAT_CONTRLREG(0x00, 0x80) /* write */
 # define MAT_CONTRL_MAT_SC(x) (((x) & 3) << 0)
 # define MAT_CONTRL_MAT_BP(1 << 2)
@@ -438,6 +439,8 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)

switch (mode) {
case DRM_MODE_DPMS_ON:
+   /* Write the default value MUX register */
+   reg_write(encoder, REG_MUX_VP_VIP_OUT, 0x24);
/* enable audio and video ports */
reg_write(encoder, REG_ENA_AP, 0xff);
reg_write(encoder, REG_ENA_VP_0, 0xff);
-- 
1.7.10.4

[PATCH 1/8] drm/i2c: tda998x: fix EDID reading on TDA19988 devices

[Sebastian Hesselbarth]

From: Russell King <rmk+ker...@arm.linux.org.uk>

TDA19988 devices need their RAM enabled in order to read EDID
information.  Add support for this.

Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
Tested-by: Sebastian Hesselbarth 
---
Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |   14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index e68b58a..d71c408 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -229,6 +229,8 @@ struct tda998x_priv {

 /* Page 12h: HDCP and OTP */
 #define REG_TX3   REG(0x12, 0x9a) /* read/write */
+#define REG_TX4   REG(0x12, 0x9b) /* read/write */
+# define TX4_PD_RAM   (1 << 1)
 #define REG_TX33  REG(0x12, 0xb8) /* read/write */
 # define TX33_HDMI(1 << 1)

@@ -673,6 +675,7 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, 
int blk)
 static uint8_t *
 do_get_edid(struct drm_encoder *encoder)
 {
+   struct tda998x_priv *priv = to_tda998x_priv(encoder);
int j = 0, valid_extensions = 0;
uint8_t *block, *new;
bool print_bad_edid = drm_debug & DRM_UT_KMS;
@@ -680,6 +683,9 @@ do_get_edid(struct drm_encoder *encoder)
if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
return NULL;

+   if (priv->rev == TDA19988)
+   reg_clear(encoder, REG_TX4, TX4_PD_RAM);
+
/* base block fetch */
if (read_edid_block(encoder, block, 0))
goto fail;
@@ -689,7 +695,7 @@ do_get_edid(struct drm_encoder *encoder)

/* if there's no extensions, we're done */
if (block[0x7e] == 0)
-   return block;
+   goto done;

new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
if (!new)
@@ -716,9 +722,15 @@ do_get_edid(struct drm_encoder *encoder)
block = new;
}

+done:
+   if (priv->rev == TDA19988)
+   reg_set(encoder, REG_TX4, TX4_PD_RAM);
+
return block;

 fail:
+   if (priv->rev == TDA19988)
+   reg_set(encoder, REG_TX4, TX4_PD_RAM);
dev_warn(encoder->dev->dev, "failed to read EDID\n");
kfree(block);
return NULL;
-- 
1.7.10.4

[PATCH 0/8] Several NXP TDA998x patches

[Sebastian Hesselbarth]

This patch set picks up several patches sent during the past months
related with NXP TDA998x HDMI transmitter driver. The patches have
been tested on Marvell Dove (Armada DRM) on several HDMI modes with
audio playing on S/PDIF. I have also quickly tested on Beaglebone
Black (tilcdc) for one DVI mode.

As I squashed some patches and fixed some audio issues, it would
be great to have a formal Tested-by or Acked-by from Russell and
Darren for the whole patch set.

Darren Etheridge (2):
  drm/i2c: tda998x: prepare for broken sync workaround
  drm/tilcdc fixup mode to workaound sync for tda998x

Russell King (5):
  drm/i2c: tda998x: fix EDID reading on TDA19988 devices
  drm/i2c: tda998x: ensure VIP output mux is properly set
  drm/i2c: tda998x: fix npix/nline programming
  drm/i2c: tda998x: prepare for video input configuration
  drm/i2c: tda998x: add video and audio input configuration

Sebastian Hesselbarth (1):
  drm/i2c: tda998x: fix sync generation and calculation

 arch/arm/boot/dts/am335x-boneblack.dts |2 +-
 drivers/gpu/drm/i2c/tda998x_drv.c  |  526 +++-
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c   |7 +-
 drivers/gpu/drm/tilcdc/tilcdc_slave.c  |   27 +-
 include/drm/i2c/tda998x.h  |   23 ++
 5 files changed, 507 insertions(+), 78 deletions(-)
 create mode 100644 include/drm/i2c/tda998x.h

---
Cc: David Airlie 
Cc: Darren Etheridge 
Cc: Rob Clark 
Cc: Russell King <rmk+kernel at arm.linux.org.uk>
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
-- 
1.7.10.4

[PATCH 1/8] drm/i2c: tda998x: fix EDID reading on TDA19988 devices

[Sebastian Hesselbarth]

From: Russell King rmk+ker...@arm.linux.org.uk

TDA19988 devices need their RAM enabled in order to read EDID
information.  Add support for this.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Tested-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
---
Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |   14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index e68b58a..d71c408 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -229,6 +229,8 @@ struct tda998x_priv {
 
 /* Page 12h: HDCP and OTP */
 #define REG_TX3   REG(0x12, 0x9a) /* read/write */
+#define REG_TX4   REG(0x12, 0x9b) /* read/write */
+# define TX4_PD_RAM   (1  1)
 #define REG_TX33  REG(0x12, 0xb8) /* read/write */
 # define TX33_HDMI(1  1)
 
@@ -673,6 +675,7 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, 
int blk)
 static uint8_t *
 do_get_edid(struct drm_encoder *encoder)
 {
+   struct tda998x_priv *priv = to_tda998x_priv(encoder);
int j = 0, valid_extensions = 0;
uint8_t *block, *new;
bool print_bad_edid = drm_debug  DRM_UT_KMS;
@@ -680,6 +683,9 @@ do_get_edid(struct drm_encoder *encoder)
if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
return NULL;
 
+   if (priv-rev == TDA19988)
+   reg_clear(encoder, REG_TX4, TX4_PD_RAM);
+
/* base block fetch */
if (read_edid_block(encoder, block, 0))
goto fail;
@@ -689,7 +695,7 @@ do_get_edid(struct drm_encoder *encoder)
 
/* if there's no extensions, we're done */
if (block[0x7e] == 0)
-   return block;
+   goto done;
 
new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
if (!new)
@@ -716,9 +722,15 @@ do_get_edid(struct drm_encoder *encoder)
block = new;
}
 
+done:
+   if (priv-rev == TDA19988)
+   reg_set(encoder, REG_TX4, TX4_PD_RAM);
+
return block;
 
 fail:
+   if (priv-rev == TDA19988)
+   reg_set(encoder, REG_TX4, TX4_PD_RAM);
dev_warn(encoder-dev-dev, failed to read EDID\n);
kfree(block);
return NULL;
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/8] drm/i2c: tda998x: fix npix/nline programming

[Sebastian Hesselbarth]

From: Russell King rmk+ker...@arm.linux.org.uk

The npix/nline registers are supposed to be programmed with the total
number of pixels/lines, not the displayed pixels/lines, and not minus
one either.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Tested-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
---
Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 4b4db95..da2917b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -586,8 +586,8 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
reg_set(encoder, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
 
reg_write(encoder, REG_VIDFORMAT, 0x00);
-   reg_write16(encoder, REG_NPIX_MSB, mode-hdisplay - 1);
-   reg_write16(encoder, REG_NLINE_MSB, mode-vdisplay - 1);
+   reg_write16(encoder, REG_NPIX_MSB, mode-htotal);
+   reg_write16(encoder, REG_NLINE_MSB, mode-vtotal);
reg_write16(encoder, REG_VS_LINE_STRT_1_MSB, line_start);
reg_write16(encoder, REG_VS_LINE_END_1_MSB, line_end);
reg_write16(encoder, REG_VS_PIX_STRT_1_MSB, hs_start);
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/8] Several NXP TDA998x patches

[Sebastian Hesselbarth]

This patch set picks up several patches sent during the past months
related with NXP TDA998x HDMI transmitter driver. The patches have
been tested on Marvell Dove (Armada DRM) on several HDMI modes with
audio playing on S/PDIF. I have also quickly tested on Beaglebone
Black (tilcdc) for one DVI mode.

As I squashed some patches and fixed some audio issues, it would
be great to have a formal Tested-by or Acked-by from Russell and
Darren for the whole patch set.

Darren Etheridge (2):
  drm/i2c: tda998x: prepare for broken sync workaround
  drm/tilcdc fixup mode to workaound sync for tda998x

Russell King (5):
  drm/i2c: tda998x: fix EDID reading on TDA19988 devices
  drm/i2c: tda998x: ensure VIP output mux is properly set
  drm/i2c: tda998x: fix npix/nline programming
  drm/i2c: tda998x: prepare for video input configuration
  drm/i2c: tda998x: add video and audio input configuration

Sebastian Hesselbarth (1):
  drm/i2c: tda998x: fix sync generation and calculation

 arch/arm/boot/dts/am335x-boneblack.dts |2 +-
 drivers/gpu/drm/i2c/tda998x_drv.c  |  526 +++-
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c   |7 +-
 drivers/gpu/drm/tilcdc/tilcdc_slave.c  |   27 +-
 include/drm/i2c/tda998x.h  |   23 ++
 5 files changed, 507 insertions(+), 78 deletions(-)
 create mode 100644 include/drm/i2c/tda998x.h

---
Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/8] drm/i2c: tda998x: ensure VIP output mux is properly set

[Sebastian Hesselbarth]

From: Russell King rmk+ker...@arm.linux.org.uk

When switching between various drivers for this device, it's possible
that some critical registers are left containing values which affect
the device operation.  One such case encountered is the VIP output
mux register.  This defaults to 0x24 on powerup, but other drivers may
set this to 0x12.  This results in incorrect colours.

Fix this by ensuring that the register is always set to the power on
default setting.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Tested-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
---
Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index d71c408..4b4db95 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -110,6 +110,7 @@ struct tda998x_priv {
 #define REG_VIP_CNTRL_5   REG(0x00, 0x25) /* write */
 # define VIP_CNTRL_5_CKCASE   (1  0)
 # define VIP_CNTRL_5_SP_CNT(x)(((x)  3)  1)
+#define REG_MUX_VP_VIP_OUTREG(0x00, 0x27) /* read/write */
 #define REG_MAT_CONTRLREG(0x00, 0x80) /* write */
 # define MAT_CONTRL_MAT_SC(x) (((x)  3)  0)
 # define MAT_CONTRL_MAT_BP(1  2)
@@ -438,6 +439,8 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 
switch (mode) {
case DRM_MODE_DPMS_ON:
+   /* Write the default value MUX register */
+   reg_write(encoder, REG_MUX_VP_VIP_OUT, 0x24);
/* enable audio and video ports */
reg_write(encoder, REG_ENA_AP, 0xff);
reg_write(encoder, REG_ENA_VP_0, 0xff);
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/8] drm/i2c: tda998x: prepare for video input configuration

[Sebastian Hesselbarth]

From: Russell King rmk+ker...@arm.linux.org.uk

The video-input-port (VIP) is highly configurable. This prepares
current driver to allow to configure VIP configuration, as some
boards connect lcd controller and TDA998x pin-swapped and depend
on VIP to swap the pins by register configuration.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Tested-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
---
Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |   16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index da2917b..d6afd02 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -32,6 +32,9 @@ struct tda998x_priv {
uint16_t rev;
uint8_t current_page;
int dpms;
+   u8 vip_cntrl_0;
+   u8 vip_cntrl_1;
+   u8 vip_cntrl_2;
 };
 
 #define to_tda998x_priv(x)  ((struct tda998x_priv 
*)to_encoder_slave(x)-slave_priv)
@@ -447,12 +450,9 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
reg_write(encoder, REG_ENA_VP_1, 0xff);
reg_write(encoder, REG_ENA_VP_2, 0xff);
/* set muxing after enabling ports: */
-   reg_write(encoder, REG_VIP_CNTRL_0,
-   VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3));
-   reg_write(encoder, REG_VIP_CNTRL_1,
-   VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1));
-   reg_write(encoder, REG_VIP_CNTRL_2,
-   VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5));
+   reg_write(encoder, REG_VIP_CNTRL_0, priv-vip_cntrl_0);
+   reg_write(encoder, REG_VIP_CNTRL_1, priv-vip_cntrl_1);
+   reg_write(encoder, REG_VIP_CNTRL_2, priv-vip_cntrl_2);
break;
case DRM_MODE_DPMS_OFF:
/* disable audio and video ports */
@@ -822,6 +822,10 @@ tda998x_encoder_init(struct i2c_client *client,
if (!priv)
return -ENOMEM;
 
+   priv-vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
+   priv-vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
+   priv-vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
+
priv-current_page = 0;
priv-cec = i2c_new_dummy(client-adapter, 0x34);
priv-dpms = DRM_MODE_DPMS_OFF;
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/8] drm/i2c: tda998x: add video and audio input configuration

[Sebastian Hesselbarth]

From: Russell King rmk+ker...@arm.linux.org.uk

This patch adds tda998x specific parameters to allow it to be configured
for different boards using it. Also, this implements rudimentary audio
support for S/PDIF attached controllers.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
---
Changelog:
for v1:
- set AUDIO_DIV to SERCLK/16 for modes with pixclk 100MHz
- also calculate CTS

Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |  315 -
 include/drm/i2c/tda998x.h |   23 +++
 2 files changed, 330 insertions(+), 8 deletions(-)
 create mode 100644 include/drm/i2c/tda998x.h

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index d6afd02..da04939 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -23,7 +23,7 @@
 #include drm/drm_crtc_helper.h
 #include drm/drm_encoder_slave.h
 #include drm/drm_edid.h
-
+#include drm/i2c/tda998x.h
 
 #define DBG(fmt, ...) DRM_DEBUG(fmt\n, ##__VA_ARGS__)
 
@@ -32,9 +32,11 @@ struct tda998x_priv {
uint16_t rev;
uint8_t current_page;
int dpms;
+   bool is_hdmi_sink;
u8 vip_cntrl_0;
u8 vip_cntrl_1;
u8 vip_cntrl_2;
+   struct tda998x_encoder_params params;
 };
 
 #define to_tda998x_priv(x)  ((struct tda998x_priv 
*)to_encoder_slave(x)-slave_priv)
@@ -71,10 +73,13 @@ struct tda998x_priv {
 # define I2C_MASTER_DIS_MM(1  0)
 # define I2C_MASTER_DIS_FILT  (1  1)
 # define I2C_MASTER_APP_STRT_LAT  (1  2)
+#define REG_FEAT_POWERDOWNREG(0x00, 0x0e) /* read/write */
+# define FEAT_POWERDOWN_SPDIF (1  3)
 #define REG_INT_FLAGS_0   REG(0x00, 0x0f) /* read/write */
 #define REG_INT_FLAGS_1   REG(0x00, 0x10) /* read/write */
 #define REG_INT_FLAGS_2   REG(0x00, 0x11) /* read/write */
 # define INT_FLAGS_2_EDID_BLK_RD  (1  1)
+#define REG_ENA_ACLK  REG(0x00, 0x16) /* read/write */
 #define REG_ENA_VP_0  REG(0x00, 0x18) /* read/write */
 #define REG_ENA_VP_1  REG(0x00, 0x19) /* read/write */
 #define REG_ENA_VP_2  REG(0x00, 0x1a) /* read/write */
@@ -113,6 +118,7 @@ struct tda998x_priv {
 #define REG_VIP_CNTRL_5   REG(0x00, 0x25) /* write */
 # define VIP_CNTRL_5_CKCASE   (1  0)
 # define VIP_CNTRL_5_SP_CNT(x)(((x)  3)  1)
+#define REG_MUX_APREG(0x00, 0x26) /* read/write */
 #define REG_MUX_VP_VIP_OUTREG(0x00, 0x27) /* read/write */
 #define REG_MAT_CONTRLREG(0x00, 0x80) /* write */
 # define MAT_CONTRL_MAT_SC(x) (((x)  3)  0)
@@ -175,6 +181,12 @@ struct tda998x_priv {
 # define HVF_CNTRL_1_PAD(x)   (((x)  3)  4)
 # define HVF_CNTRL_1_SEMI_PLANAR  (1  6)
 #define REG_RPT_CNTRL REG(0x00, 0xf0) /* write */
+#define REG_I2S_FORMATREG(0x00, 0xfc) /* read/write */
+# define I2S_FORMAT(x)(((x)  3)  0)
+#define REG_AIP_CLKSELREG(0x00, 0xfd) /* write */
+# define AIP_CLKSEL_FS(x) (((x)  3)  0)
+# define AIP_CLKSEL_CLK_POL(x)(((x)  1)  2)
+# define AIP_CLKSEL_AIP(x)(((x)  7)  3)
 
 
 /* Page 02h: PLL settings */
@@ -198,6 +210,12 @@ struct tda998x_priv {
 #define REG_PLL_SCGR1 REG(0x02, 0x09) /* read/write */
 #define REG_PLL_SCGR2 REG(0x02, 0x0a) /* read/write */
 #define REG_AUDIO_DIV REG(0x02, 0x0e) /* read/write */
+# define AUDIO_DIV_SERCLK_1   0
+# define AUDIO_DIV_SERCLK_2   1
+# define AUDIO_DIV_SERCLK_4   2
+# define AUDIO_DIV_SERCLK_8   3
+# define AUDIO_DIV_SERCLK_16  4
+# define AUDIO_DIV_SERCLK_32  5
 #define REG_SEL_CLK   REG(0x02, 0x11) /* read/write */
 # define SEL_CLK_SEL_CLK1 (1  0)
 # define SEL_CLK_SEL_VRF_CLK(x)   (((x)  3)  1)
@@ -216,6 +234,11 @@ struct tda998x_priv {
 
 
 /* Page 10h: information frames and packets */
+#define REG_IF1_HB0   REG(0x10, 0x20) /* read/write */
+#define REG_IF2_HB0   REG(0x10, 0x40) /* read/write */
+#define REG_IF3_HB0   REG(0x10, 0x60) /* read/write */
+#define REG_IF4_HB0   REG(0x10, 0x80) /* read/write */
+#define REG_IF5_HB0   REG(0x10, 0xa0) /* read/write */
 
 
 /* Page 11h: audio settings and content info packets */
@@ -225,10 +248,33 @@ struct tda998x_priv {
 # define AIP_CNTRL_0_LAYOUT   (1  2)
 # define AIP_CNTRL_0_ACR_MAN  (1  5)
 # define AIP_CNTRL_0_RST_CTS  (1  6)
+#define REG_CA_I2SREG(0x11, 0x01) /* read/write */
+# define CA_I2S_CA_I2S(x) (((x)  31

[PATCH 7/8] drm/i2c: tda998x: prepare for broken sync workaround

[Sebastian Hesselbarth]

From: Darren Etheridge detheri...@ti.com

Some LCD controller cannot provide valid VESA style sync, i.e. coincident
HS/VS edges. First, this patch adds hskew passed from the adjusted_mode to
reference pixel calculation to allow those controllers to add an offset
relative to the expected reference pixel.

Signed-off-by: Darren Etheridge detheri...@ti.com
Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
---
Changelog:
for v1:
- reword comment

Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 7bf227a..7dc5650 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -784,6 +784,15 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
n_line   = mode-vtotal;
 
ref_pix  = 3 + mode-hsync_start - mode-hdisplay;
+
+   /*
+* Attached LCD controllers may generate broken sync. Allow
+* those to adjust the position of the rising VS edge by adding
+* HSKEW to ref_pix.
+*/
+   if (adjusted_mode-flags  DRM_MODE_FLAG_HSKEW)
+   ref_pix += adjusted_mode-hskew;
+
de_pix_s = mode-htotal - mode-hdisplay;
de_pix_e = de_pix_s + mode-hdisplay;
hs_pix_s = mode-hsync_start - mode-hdisplay;
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 8/8] drm/tilcdc fixup mode to workaound sync for tda998x

[Sebastian Hesselbarth]

From: Darren Etheridge detheri...@ti.com

Add a fixup function that will flip the hsync priority and
add a hskew value that is used to shift the tda998x to the
right by a variable number of pixels depending on the mode.
This works around an issue with the sync timings that tilcdc
is outputing.

Signed-off-by: Darren Etheridge detheri...@ti.com
---
Changelog:
for v1:
- reword comment

Cc: David Airlie airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c  |7 ++-
 drivers/gpu/drm/tilcdc/tilcdc_slave.c |   27 ++-
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 7418dcd..6d05240 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -379,7 +379,12 @@ static int tilcdc_crtc_mode_set(struct drm_crtc *crtc,
else
tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_EDGE);
 
-   if (mode-flags  DRM_MODE_FLAG_NHSYNC)
+   /*
+* use value from adjusted_mode here as this might have been
+* changed as part of the fixup for slave encoders to solve the
+* issue where tilcdc timings are not VESA compliant
+*/
+   if (adjusted_mode-flags  DRM_MODE_FLAG_NHSYNC)
tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
else
tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c 
b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
index 0bf5999..f6e9c26 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
@@ -73,13 +73,38 @@ static void slave_encoder_prepare(struct drm_encoder 
*encoder)
tilcdc_crtc_set_panel_info(encoder-crtc, slave_info);
 }
 
+static bool slave_encoder_fixup(struct drm_encoder *encoder,
+   const struct drm_display_mode *mode,
+   struct drm_display_mode *adjusted_mode)
+{
+   /*
+* tilcdc does not generate VESA-complient sync but aligns
+* VS on the second edge of HS instead of first edge.
+* We use adjusted_mode, to fixup sync by aligning both rising
+* edges and add HSKEW offset to let the slave encoder fix it up.
+*/
+   adjusted_mode-hskew = mode-hsync_end - mode-hsync_start;
+   adjusted_mode-flags |= DRM_MODE_FLAG_HSKEW;
+
+   if (mode-flags  DRM_MODE_FLAG_NHSYNC) {
+   adjusted_mode-flags |= DRM_MODE_FLAG_PHSYNC;
+   adjusted_mode-flags = ~DRM_MODE_FLAG_NHSYNC;
+   } else {
+   adjusted_mode-flags |= DRM_MODE_FLAG_NHSYNC;
+   adjusted_mode-flags = ~DRM_MODE_FLAG_PHSYNC;
+   }
+
+   return drm_i2c_encoder_mode_fixup(encoder, mode, adjusted_mode);
+}
+
+
 static const struct drm_encoder_funcs slave_encoder_funcs = {
.destroy= slave_encoder_destroy,
 };
 
 static const struct drm_encoder_helper_funcs slave_encoder_helper_funcs = {
.dpms   = drm_i2c_encoder_dpms,
-   .mode_fixup = drm_i2c_encoder_mode_fixup,
+   .mode_fixup = slave_encoder_fixup,
.prepare= slave_encoder_prepare,
.commit = drm_i2c_encoder_commit,
.mode_set   = drm_i2c_encoder_mode_set,
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/2] drm/tilcdc drm/i2c/tda998x workaround for sync issues on TI SoC

[Sebastian Hesselbarth]

On 07/25/2013 09:32 PM, Rob Clark wrote:

On Thu, Jul 25, 2013 at 2:32 PM, Darren Etheridge detheri...@ti.com wrote:

[...]

This patch set inverts the hsync signal coming from the tilcdc so the NXP
is kept happy and then shifts the output to the right to compensate for the
sync timing issues.  Display modes from the NXP have been verified using a
HDMI analyzer and are reporting correct timings at the output stage.

Hopefully this will allow the dove/tda driver changes to progress now that
were blocked as per this discussion:
http://lists.freedesktop.org/archives/dri-devel/2013-July/040900.html



Good find Darren!  The patches look good to me from a quick review.
It would be good to get a tested-by from someone on cubox, but it is
good that we finally found the issue so that we can unblock further
tda998x development.


Darren,

I now fully understand the issues of AM335x's LCD controller and your
fix for it. I suggest to clarify the comments you added to tilcdc to
allow others to understand it more quickly.

Actually, the LCD controller always aligns vsync to the second edge
of hsync, which will never give VESA-compliant sync. The (elegant)
workaround you are proposing is to align both rising edges, so at
least TDA998x can sync on those with some hskew added. Lucky you that
it ignores hsync length but only looks for rising HS/VS edges ;)

Should we prepare a new patch set comprising the following patches?

Russell King:
drm/i2c: nxp-tda998x: fix EDID reading on TDA19988 devices
drm/i2c: nxp-tda998x: ensure VIP output mux is properly set
drm/i2c: nxp-tda998x: fix npix/nline programming
drm/i2c: nxp-tda998x: prepare for video input configuration
drm/i2c: nxp-tda998x: add video and audio input configuration

Sebastian Hesselbarth:
drm/i2c: tda998x: fix sync generation and calculation

Darren Etheridge:
drm/i2c/tda998x prepare for tilcdc sync workaround
drm/tilcdc fixup mode to workaound sync for tda998x

Or do we keep them separated and possibly resend them if David cannot
find them anymore?

Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/2] drm/tilcdc drm/i2c/tda998x workaround for sync issues on TI SoC

[Sebastian Hesselbarth]

On 07/25/2013 09:32 PM, Rob Clark wrote:
> On Thu, Jul 25, 2013 at 2:32 PM, Darren Etheridge  
> wrote:
[...]
>> This patch set inverts the hsync signal coming from the tilcdc so the NXP
>> is kept happy and then shifts the output to the right to compensate for the
>> sync timing issues.  Display modes from the NXP have been verified using a
>> HDMI analyzer and are reporting correct timings at the output stage.
>>
>> Hopefully this will allow the dove/tda driver changes to progress now that
>> were blocked as per this discussion:
>> http://lists.freedesktop.org/archives/dri-devel/2013-July/040900.html
>>
>
> Good find Darren!  The patches look good to me from a quick review.
> It would be good to get a tested-by from someone on cubox, but it is
> good that we finally found the issue so that we can unblock further
> tda998x development.

Darren,

I now fully understand the issues of AM335x's LCD controller and your
fix for it. I suggest to clarify the comments you added to tilcdc to
allow others to understand it more quickly.

Actually, the LCD controller always aligns vsync to the second edge
of hsync, which will never give VESA-compliant sync. The (elegant)
workaround you are proposing is to align both rising edges, so at
least TDA998x can sync on those with some hskew added. Lucky you that
it ignores hsync length but only looks for rising HS/VS edges ;)

Should we prepare a new patch set comprising the following patches?

Russell King:
drm/i2c: nxp-tda998x: fix EDID reading on TDA19988 devices
drm/i2c: nxp-tda998x: ensure VIP output mux is properly set
drm/i2c: nxp-tda998x: fix npix/nline programming
drm/i2c: nxp-tda998x: prepare for video input configuration
drm/i2c: nxp-tda998x: add video and audio input configuration

Sebastian Hesselbarth:
drm/i2c: tda998x: fix sync generation and calculation

Darren Etheridge:
drm/i2c/tda998x prepare for tilcdc sync workaround
drm/tilcdc fixup mode to workaound sync for tda998x

Or do we keep them separated and possibly resend them if David cannot
find them anymore?

Sebastian

[PATCH 0/2] drm/tilcdc drm/i2c/tda998x workaround for sync issues on TI SoC

[Sebastian Hesselbarth]

On 07/25/2013 09:32 PM, Rob Clark wrote:
> On Thu, Jul 25, 2013 at 2:32 PM, Darren Etheridge  
> wrote:
>> Russell King and Sebastian Hasselbarth had proposed some very good changes
>> for the tda998x HDMI encoder driver.  But when those changes were tested
>> on BeagleBone Black against the tilcdc driver many modes would no longer
>> display correctly.  After analyzing the sync signals from the TI lcd 
>> contoller
>> to the nxp it is apparent that the hsync/vsync's are not rising at the same
>> time as per the VESA spec and this is causing the HDMI encoder to get
>> messed up and failing to lock correctly.
>>
>> This series of patches should be applied on top of:
>>
>> Russell King's
>> rmk's Dove DRM/TDA19988 Cubox driver series
>>
>> Sebastian Hasselbarth's
>> drm/i2c: tda998x: fix sync generation and calculation
>>
>> I have done as much of the change as I can in the tilcdc driver but there
>> is a small unavoidable change in the tda998x driver.  However I have been
>> careful not to break anything from the Dove drivers perspective. It
>> would be great if somebody can test on Cubox and confirm that.
>>
>> This patch set inverts the hsync signal coming from the tilcdc so the NXP
>> is kept happy and then shifts the output to the right to compensate for the
>> sync timing issues.  Display modes from the NXP have been verified using a
>> HDMI analyzer and are reporting correct timings at the output stage.
>>
>> Hopefully this will allow the dove/tda driver changes to progress now that
>> were blocked as per this discussion:
>> http://lists.freedesktop.org/archives/dri-devel/2013-July/040900.html
>>
>
> Good find Darren!  The patches look good to me from a quick review.
> It would be good to get a tested-by from someone on cubox, but it is
> good that we finally found the issue so that we can unblock further
> tda998x development.

Thanks to Darren, I can now test tda998x sync changes on both Cubox and
Beaglebone Black. I don't think the changes will affect Armada DRM in
any way, as it is not setting adjusted_mode.

I will put a scope on AM335x/TDA998x sync signals to fully understand
the issues tilcdc has, but I can think of a flag for TDA998x to always
accept falling input hsync/vsync and tilcdc to supply that sync. That
will maybe allow us to get rid of hskew workaround.

As far as I understand the issue, tilcdc always aligns VS with the
rising HS edge? If so, enforcing positive HS/VS on tilcdc and telling
TDA998x that it is always positive, may be a cleaner workaround. TDA998x
can invert input and output sync independently.

In any way, it will take some time to get a working setup. If you are
happy with the patches, I can still send some follow-up patches later.

Sebastian

Re: [PATCH 0/2] drm/tilcdc drm/i2c/tda998x workaround for sync issues on TI SoC

[Sebastian Hesselbarth]

On 07/25/2013 09:32 PM, Rob Clark wrote:

On Thu, Jul 25, 2013 at 2:32 PM, Darren Etheridge detheri...@ti.com wrote:

Russell King and Sebastian Hasselbarth had proposed some very good changes
for the tda998x HDMI encoder driver.  But when those changes were tested
on BeagleBone Black against the tilcdc driver many modes would no longer
display correctly.  After analyzing the sync signals from the TI lcd contoller
to the nxp it is apparent that the hsync/vsync's are not rising at the same
time as per the VESA spec and this is causing the HDMI encoder to get
messed up and failing to lock correctly.

This series of patches should be applied on top of:

Russell King's
rmk's Dove DRM/TDA19988 Cubox driver series

Sebastian Hasselbarth's
drm/i2c: tda998x: fix sync generation and calculation

I have done as much of the change as I can in the tilcdc driver but there
is a small unavoidable change in the tda998x driver.  However I have been
careful not to break anything from the Dove drivers perspective. It
would be great if somebody can test on Cubox and confirm that.

This patch set inverts the hsync signal coming from the tilcdc so the NXP
is kept happy and then shifts the output to the right to compensate for the
sync timing issues.  Display modes from the NXP have been verified using a
HDMI analyzer and are reporting correct timings at the output stage.

Hopefully this will allow the dove/tda driver changes to progress now that
were blocked as per this discussion:
http://lists.freedesktop.org/archives/dri-devel/2013-July/040900.html



Good find Darren!  The patches look good to me from a quick review.
It would be good to get a tested-by from someone on cubox, but it is
good that we finally found the issue so that we can unblock further
tda998x development.


Thanks to Darren, I can now test tda998x sync changes on both Cubox and
Beaglebone Black. I don't think the changes will affect Armada DRM in
any way, as it is not setting adjusted_mode.

I will put a scope on AM335x/TDA998x sync signals to fully understand
the issues tilcdc has, but I can think of a flag for TDA998x to always
accept falling input hsync/vsync and tilcdc to supply that sync. That
will maybe allow us to get rid of hskew workaround.

As far as I understand the issue, tilcdc always aligns VS with the
rising HS edge? If so, enforcing positive HS/VS on tilcdc and telling
TDA998x that it is always positive, may be a cleaner workaround. TDA998x
can invert input and output sync independently.

In any way, it will take some time to get a working setup. If you are
happy with the patches, I can still send some follow-up patches later.

Sebastian

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DT binding review for Armada display subsystem

[Sebastian Hesselbarth]

On 07/13/2013 04:25 PM, Daniel Drake wrote:

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

[Sebastian Hesselbarth]

On 07/13/2013 01:12 PM, Russell King - ARM Linux wrote:

On Sat, Jul 13, 2013 at 12:56:50PM +0200, Sylwester Nawrocki wrote:

On 07/13/2013 10:35 AM, Jean-Francois Moine wrote:

On Fri, 12 Jul 2013 13:00:23 -0600 Daniel 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 

DT binding review for Armada display subsystem

[Sebastian Hesselbarth]

On 07/13/2013 01:12 PM, Russell King - ARM Linux wrote:
> On Sat, Jul 13, 2013 at 12:56:50PM +0200, Sylwester Nawrocki wrote:
>> On 07/13/2013 10:35 AM, Jean-Francois Moine wrote:
>>> On Fri, 12 Jul 2013 13:00:23 -0600 Daniel Drake  wrote:
 On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moine  
 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 at 82 {
>>> compatible = "marvell,dove-lcd0";
>> [...]
>>> };
>>>
>>> lcd1: lcd-controller at 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 = _0;  
>>  lcd1 = _1;  
>>  };
>>
>>  lcd_0: 

DT binding review for Armada display subsystem

[Sebastian Hesselbarth]

On 07/13/2013 04:25 PM, Daniel Drake wrote:
> On Sat, Jul 13, 2013 at 2:35 AM, Jean-Francois Moine  
> 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 at 82 {
>>  compatible = "marvell,dove-lcd0";
>>  reg = <0x82 0x1c8>;
>>  interrupts = <47>;
>>  clocks = <_clk 3>, <>;
>>  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 = < 0>;
>>  status = "disabled";
>>  };
>>
>>  lcd1: lcd-controller at 81 {
>>  compatible = "marvell,dove-lcd1";
>>  reg = <0x81 0x1c8>;
>>  interrupts = <46>;
>>  clocks = <_clk 3>, <>;
>>  clock-names = "axi", "lcdclk";
>>  marvell-output = < 1>;
>>  status = "disabled";
>>  };
>>
>>  /* display controller and image rotation engine */
>>  dcon: display-controller at 83 {
>>  compatible = "marvell,dove-dcon";
>>  reg = <0x83 0xc4>,  /* DCON */
>><0x831000 0x8c>;  /* IRE */
>>  interrupts = <45>;
>>  marvell-input = <>, <>;
>>  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. 

Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/05/13 11:51, Grant Likely wrote:
> On Fri, Jul 5, 2013 at 10:34 AM, Sebastian Hesselbarth
>  wrote:
>> So for the discussion, I can see that there have been some voting for
>> super-node, some for node-to-node linking. Although I initially proposed
>> super-nodes, I can also happily live with node-to-node linking alone.
>>
>> Either someone can give an example where one of the approaches will not
>> work (i.MX, exynos?), Grant or one of the DRM maintainers has a
>> preference, or we are stuck at the decision.
>
> I tend to prefer a top-level super nodes with phandles to all of the
> components that compose the device when there is no clear one device
> that controls all the others. There is some precedence for that in
> other subsystems (leds, asoc, etc). Sound in particular has a lot of
> different bits and pieces that are interconnected with audio channels,
> gpios, and other things that get quite complicated, so it is
> convenient to have a single node that describes how they all fit
> together *and* allows for a platform to use a completely different
> device driver if required.

Actually, I consider the super-node not as the single point for _all_
components involved but more as the top node that allows you to have
a single starting point from where can explore the links on a
node-to-node basis. This by coincidence perfectly fits what will
be required for a DRM driver to match against.

Sascha Hauer just also replied to a mail earlier mentioning references 
to external i2c encoders put _into_ the phandles of the super-node.
This is not what I consider this super-node for. Maybe the following
drawings also help a little bit.

(X) Hardware layout inside the SoC:
{BUS}<->{RAM}
   |
   +<->{LCD0}-+ +->{LCD0-PINS}
   |  +->{DCON}-+
   +<->{LCD1}-+ +->{LCD1-PINS}

 From a logical point-of-view and just because we have no single
starting point on Marvell SoCs the use cases can be described as
follows. (x) denotes a device tree node, --> a link installed by
some phandle property, [x] device tree nodes not linked but handled
by driver looking it up in DT, ==> first user visible video stream.

(1) single card, single lcd-controller:
[DCON]
(SUPERNODE)--->(LCD0)-->(HDMI)==>

(2) multiple cards, single lcd-controller:
[DCON]
(SUPERNODE0)-->(LCD0)-->(HDMI)==>
(SUPERNODE1)-->(LCD1)==>

(3) single card, multiple lcd-controller:
[DCON]
 +->(LCD0)-->(HDMI)==>
(SUPERNODE)-+
 +->(LCD1)==>

So the super-node is just used as a single starting point for the
node-to-node walk. IMHO this is very compatible with what v4l2 guys
came up with - except that you _can_ install a virtual starting
point where it is missing from a SoC device point-of-view. SoCs with
two unrelated lcd-controllers will pick up the lcd-controller node
for their DRM drivers.

As mentioned before, to achieve the same you can leave the super-node
and use lcd-controller nodes with "slave-mode" type-of property.

Maybe calling it "super-node" after some point of the discussion was
misleading. It is *not* an umbrella node with phandles to every device
involved, but *the* root node for your logical graph/tree/chain of
device nodes required for video.

> node-to-node linking works well if there an absolute 'master' can be
> identified for the virtual device. ie. Ethernet MAC devices use a
> "phy-device" property to link to the phy it requires. In that case it
> is pretty clear that the Ethernet MAC is in charge and it uses the
> PHY.
>
> In either case it is absolutely required that the 'master' driver
> knows how to find and wait for all the subservient devices before
> probing can complete.
>
> I know that isn't a solid answer, but you know the problem space
> better than I. Take the above into account, make a decision and post a
> binding proposal for review.

Well, I have given a proposal of what I already implemented during
Russell's Armada DRM driver RFCs. I am fine with *anyone* picking up
*any* solution discussed here as long as it involves phandles linking
SoC nodes (lcd-controller) with external I2C nodes (hdmi-transceiver).

If it is super-node or master/slave properties, I don't care as long
as it is somehow related to HW and not some SW subsystem requirements.
I can think of both solutions solving Marvell SoC DRM driver "issues"
and I guess even for the most of other SoCs as well.

The only scenario out of the three above that can possibly start
displaying video while waiting for sub-drivers is (3). You can
output video through LCD1 while waiting for HDMI.

But, that is in no way related to "best practice device tree design
for display subsystems" which this discussion is about but
implementation details of DRM or any other subsystem.

The pure existence 

Re: Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/04/13 09:05, Inki Dae wrote:

-Original Message-
From: Sebastian Hesselbarth [mailto:sebastian.hesselba...@gmail.com]
Sent: Wednesday, July 03, 2013 8:52 PM
To: Inki Dae
Cc: 'Russell King'; devicetree-disc...@lists.ozlabs.org; 'Jean-Francois
Moine'; 'Sascha Hauer'; 'Daniel Drake'; dri-devel@lists.freedesktop.org
Subject: Re: Best practice device tree design for display subsystems/DRM

On 07/03/13 13:43, Inki Dae wrote:

I do not understand why you keep referring to the SoC dtsi. Im my
example, I said that it is made up and joined from both SoC dtsi and
board dts.

So, of course, lcd controller nodes and dcon are part of dove.dtsi
because they are physically available on every Dove SoC.

Also, the connection from lcd0 to the external HDMI encoder node
is in the board dts because you can happily build a Dove SoC board
with a different HDMI encoder or with no encoder at all.

The video-card super node is not in any way specific to DRM and


In case of fbdev, framebuffer driver would use lcd0 or lcd1 driver, or

lcd0

and lcd1 drivers which are placed in drivers/video/backlight/.

And let's assume the following:

On board A
   Display controller - lcd 0
On board B
   Display controller - lcd 1
On board C
   Display controller - lcd 0 and lcd 1

Without the super node, user could configure Linux kernel through

menuconfig

like below;
board A:  enabling lcd 0, and disabling lcd 1,
board B: disabling lcd 0, and enabling lcd 1,
board C: enabling lcd 0 and lcd 1.

All we have to do is to configure menuconfig to enable only drivers for
certain board. Why does fbdev need the super node? Please give me

comments

if there is my missing point.


I assume when you say configure menuconfig you mean
create a CONFIG_DISPLAY_CONTROLLER_AS_USED_ON_BOARD_A,
CONFIG_DISPLAY_CONTROLLER_AS_USED_ON_BOARD_B, ... ?

This finally will require you to have
(a) #ifdef mess for every single board _and_ driver above
(b) new CONFIG_.._BOARD_D plus new #ifdefs in fbdev driver for every
  new board
(c) A new set of the above CONFIG_/#ifdef for DRM driver

This can also be done with device_tree and supernode approach,
so for your example above:

BoardA.dts:
video { card0 { video-devices = lcd0; }; };

BoardB.dts:
video { card0 { video-devices = lcd1; }; };

BoardC.dts:
video { card0 { video-devices = lcd0 lcd1; }; };

and in the driver your are prepared for looping over the video-devices
property and parsing the compatible string of the nodes passed.



As I mentioned before, fbdev don't need the super node, card0. Please see
the below,

BoardA.dts:
video { dcon: display-controller@83 { video-devices = lcd0; }; };

BoardB.dts:
video { dcon: display-controller@83 { video-devices = lcd1; }; };

BoardC.dts:
video { dcon: display-controller@83 { video-devices = lcd0 lcd1; };
};

With the above dts file, does the fbdev have any problem? I just changed the
super node to real hardware node. That is why the super node is specific to
DRM.


Inki,

I guess there is a misunderstanding of what lcd-controller and display-
controller are for on Dove. lcd-controller reads framebuffer from
memory, optionally does some conversions/scaling, and drives the SoCs
pins with pixel data and sync. display-controller (dcon) on Dove is for
mirroring lcd0 framebuffer to lcd1 framebuffer and some other things.

And, as stated several times, you cannot move internal-registers out of
the corresponding node on Dove. You _need_ that parent node for address
mapping.

IMHO also fbdev needs the super-node because lcd0/1, dcon,
hdmi-transmitter, programmable-pll is what make up what you would call
a graphics card on x86. There is no such graphics card on most SoCs
but it is built up by using separate devices and SoC internal devices.

Moreover, it is highly board dependent because you will easily find
another board manufacturer chosing a different hdmi-transmitter or
programmable-pll, using two lcd-controllers or just one. And there is
no way of probing the boards configuration.

So even fvdev needs the super-node, there is no difference what video
subsystem you use - just because DT describes HW (even virtual one) not
subsystem.

Sebastian

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/04/13 10:33, Sascha Hauer wrote:

On Wed, Jul 03, 2013 at 10:52:49AM +0100, Russell King wrote:

Sorry but I'd like to say that this cannot be used commonly. Shouldn't you
really consider Linux framebuffer or other subsystems? The above dtsi file
is specific to DRM subsystem. And I think the dtsi file has no any
dependency on certain subsystem so board dtsi file should be considered for
all device drivers based on other subsystems: i.e., Linux framebuffer, DRM,
and so no. So I *strongly* object to it. All we have to do is to keep the
dtsi file as is, and to find other better way that can be used commonly in
DRM.


+1 for not encoding the projected usecase of the graphics subsystem into
the devicetree. Whether the two LCD controllers shall be used together
or separately should not affect the devicetree. devicetree is about
hardware description, not configuration.


And if we listen to that argument, then this problem is basically
impossible to solve sanely.

Are we really saying that we have no acceptable way to represent
componentized devices in DT?  If that's true, then DT fails to represent
quite a lot of ARM hardware, and frankly we shouldn't be using it.  I
can't believe that's true though.

The problem is that even with an ASoC like approach, that doesn't work
here because there's no way to know how many components to expect.
That's what the supernode is doing - telling us what components group
together to form a device.


A componentized device never completes and it doesn't have to. A
componentized device can start once there is a path from an input
(crtc, i2s unit) to an output (connector, speaker).

Consider what happens with a supernode approach. Your board provides a
devicetree which has a supernode with hdmi and lvds referenced. Now you
build a kernel with the hdmi driver disabled. You would still expect the
lvds port to be working without having the kernel wait for the supernode
being complete.

Without supernode you can just start once you have everything between
the crtc and lvds nodes. If later a hdmi device joins in then you can
either notify the users (provided the DRM/KMS API supports it) or just
ignore it until the DRM device gets reopened.


Sascha,

that is what it is all about. You assume you a priori know what devices
will be required for the componentized device to successfully output
a video stream.

We have shown setups where you don't know what is required. Cubox
_needs_ lcd0 and hdmi-transmitter, olpc just needs lcd0 and has
built-in hdmi in the SoC (IIRC). The driver needs to know what to
wait for, and that is given by the DT super-node.

I consider kernels with missing drivers compared to what is given in
the DT as broken setup. You cannot complain about missing SATA if you
leave out SATA driver, or - if you implemented the driver into two
parts - leave out one of the two SATA driver parts.

Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/04/13 10:53, Sascha Hauer wrote:

On Thu, Jul 04, 2013 at 10:45:40AM +0200, Sebastian Hesselbarth wrote:

On 07/04/13 10:33, Sascha Hauer wrote:


A componentized device never completes and it doesn't have to. A
componentized device can start once there is a path from an input
(crtc, i2s unit) to an output (connector, speaker).

Consider what happens with a supernode approach. Your board provides a
devicetree which has a supernode with hdmi and lvds referenced. Now you
build a kernel with the hdmi driver disabled. You would still expect the
lvds port to be working without having the kernel wait for the supernode
being complete.

Without supernode you can just start once you have everything between
the crtc and lvds nodes. If later a hdmi device joins in then you can
either notify the users (provided the DRM/KMS API supports it) or just
ignore it until the DRM device gets reopened.


Sascha,

that is what it is all about. You assume you a priori know what devices
will be required for the componentized device to successfully output
a video stream.

We have shown setups where you don't know what is required. Cubox
_needs_ lcd0 and hdmi-transmitter,


Then your Cubox devicetree has a link (that's how they call it in v4l2,
a link doesn't necessarily is a direct connection but can have multiple
devices in it) between lcd0 and hdmi.


I haven't looked up v4l2 link yet. But (a) if it is a separate node
how is that different from the super-node we are talking about or (b)
if it is a property, where do you put it?


  olpc just needs lcd0 and has built-in hdmi in the SoC (IIRC).


And olpc has a link with lcd0 as the source and the builtin hdmi.


Sure, as my DT proposal that Inki shamelessly copied, has a link
property for lcd0 on Cubox, while OPLC's lcd0 hasn't.


The driver needs to know what to wait for, and that is given by the DT 
super-node.


You need a source (described in the devicetree), a sink (also described
in the devicetree) and a link between them, nothing more.


What if you need source-to-multi-sink or source-to-transceiver-to-sink?
The DT proposal I have given, is source-to-sink on a per device node
basis. If you don't like OPLC's lcd0 having no link while Cubox' lcd0
has, put a fake node for the possibly connected HDMI monitor in there.
ASoC does that for SPDIF jack by also having a codecs driver although
there is nothing to control.


I consider kernels with missing drivers compared to what is given in
the DT as broken setup.


What if your devicetree describes components not yet in mainline. Would
you consider mainline kernels broken then?


No. But I do not 1:1 match device tree nodes with subsystem drivers.
The devices are there, there is no driver.

Currently, for CuBox DT, video _is_ broken because there is no way to
represent the way lcd0, hdmi-transmitter, and pll are connected to form
a single video card. All separate drivers exist, you may be lucky to get
video out of the CuBox because of some reset values that allow you to
get it by coincidence. But as soon as you try the very same kernel on a
different board, your experience will change quickly.

Sebastian

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/04/13 11:23, Sascha Hauer wrote:

On Thu, Jul 04, 2013 at 11:10:35AM +0200, Sebastian Hesselbarth wrote:

On 07/04/13 10:53, Sascha Hauer wrote:

On Thu, Jul 04, 2013 at 10:45:40AM +0200, Sebastian Hesselbarth wrote:

On 07/04/13 10:33, Sascha Hauer wrote:


A componentized device never completes and it doesn't have to. A
componentized device can start once there is a path from an input
(crtc, i2s unit) to an output (connector, speaker).

Consider what happens with a supernode approach. Your board provides a
devicetree which has a supernode with hdmi and lvds referenced. Now you
build a kernel with the hdmi driver disabled. You would still expect the
lvds port to be working without having the kernel wait for the supernode
being complete.

Without supernode you can just start once you have everything between
the crtc and lvds nodes. If later a hdmi device joins in then you can
either notify the users (provided the DRM/KMS API supports it) or just
ignore it until the DRM device gets reopened.


Sascha,

that is what it is all about. You assume you a priori know what devices
will be required for the componentized device to successfully output
a video stream.

We have shown setups where you don't know what is required. Cubox
_needs_ lcd0 and hdmi-transmitter,


Then your Cubox devicetree has a link (that's how they call it in v4l2,
a link doesn't necessarily is a direct connection but can have multiple
devices in it) between lcd0 and hdmi.


I haven't looked up v4l2 link yet. But (a) if it is a separate node
how is that different from the super-node we are talking about or (b)
if it is a property, where do you put it?


Sorry, I should have explained this. The basic idea the v4l2 guys are
following is that they describe their hardware pipelines in the devicetree.

Each device can have ports which are connected via links. In the
devicetree a link basically becomes a phandle (a remote device will have
a phandle pointing back to the original device). For an overview have a
look at

Documentation/devicetree/bindings/media/video-interfaces.txt

With this you can describe the whole graph of devices you have in the
devicetree. The examples in this file have a path from a camera sensor
via a MIPI converter to a capture interface.

The difference to a supernode is that this approach describes the data
flow in the devicetree so that we can iterate over it to find links
between source and sink rather than relying on a list of subdevices to
be completed.


Agree. But that is not that different from linux,video-external-encoder
property I made up, except that the name is different.

And, I still see no way with that source/sink linking _alone_ how to
tell that either lcd0 and lcd1 act as a _single_ video card or lcd0 and
lcd1 are used in a _two_ video card setup.

There is no single device node on Dove that would sufficiently act as
the top node for a working video card on all boards. And there is no
framebuffer node to link each of the lcd0/1 nodes to.

That is what the super-node is for, form a virtual device called
video card to act as a container for all those SoC devices that are
not sufficient for a working video setup on their own.

If lcd0 needs that hdmi-transmitter you link it to the lcd0 node -
not the super-node. If lcd0 needs some pll clock you link it to the
lcd0 node - again not the super-node.

The super-node(s) just connects all SoC devices that shall be part of
your board-specific video card(s) - for Dove that is any combination of
lcd0, lcd0, dcon and video memory allocation.

Sebastian

Sebastian

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/04/13 11:30, Sascha Hauer wrote:

On Thu, Jul 04, 2013 at 10:11:31AM +0100, Russell King wrote:

On Thu, Jul 04, 2013 at 10:58:17AM +0200, Sascha Hauer wrote:

On Thu, Jul 04, 2013 at 09:40:52AM +0100, Russell King wrote:

Wrong.  Please read the example with the diagrams I gave.  Consider
what happens if you have two display devices connected to a single
output, one which fixes the allowable mode and one which _can_
reformat the selected mode.


What you describe here is a forced clone mode. This could be described
in the devicetree so that a driver wouldn't start before all connected
displays (links) are present, but this should be limited to the affected
path, not to the whole componentized device.


Okay, to throw a recent argument back at you: so what in this scenario
if you have a driver for the fixed-mode device but not the other device?

It's exactly the same problem which you were describing to Sebastian
just a moment ago with drivers missing from the supernode approach -
you can't start if one of those forced clone drivers is missing.


Indeed, then you will see nothing on your display, but I rather make
this setup a special case than the rather usual case that we do not
have compiled in all drivers for all devices referenced in the
supernode.


The super-node links SoC internal devices that do not necessarily match
with the subsystem driver. You have one single DRM driver exploiting
several device nodes for a single video card.

But you need one device node to hook the driver to.

Sebastian

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/04/13 12:09, Sascha Hauer wrote:

On Thu, Jul 04, 2013 at 11:44:41AM +0200, Sebastian Hesselbarth wrote:

On 07/04/13 11:30, Sascha Hauer wrote:

On Thu, Jul 04, 2013 at 10:11:31AM +0100, Russell King wrote:

On Thu, Jul 04, 2013 at 10:58:17AM +0200, Sascha Hauer wrote:

On Thu, Jul 04, 2013 at 09:40:52AM +0100, Russell King wrote:

Wrong.  Please read the example with the diagrams I gave.  Consider
what happens if you have two display devices connected to a single
output, one which fixes the allowable mode and one which _can_
reformat the selected mode.


What you describe here is a forced clone mode. This could be described
in the devicetree so that a driver wouldn't start before all connected
displays (links) are present, but this should be limited to the affected
path, not to the whole componentized device.


Okay, to throw a recent argument back at you: so what in this scenario
if you have a driver for the fixed-mode device but not the other device?

It's exactly the same problem which you were describing to Sebastian
just a moment ago with drivers missing from the supernode approach -
you can't start if one of those forced clone drivers is missing.


Indeed, then you will see nothing on your display, but I rather make
this setup a special case than the rather usual case that we do not
have compiled in all drivers for all devices referenced in the
supernode.


The super-node links SoC internal devices that do not necessarily match
with the subsystem driver. You have one single DRM driver exploiting
several device nodes for a single video card.

But you need one device node to hook the driver to.


Currently on i.MX we use a platform_device for this purpose.


Sascha,

I have the impression that we are not that far away in our proposals.

The platform_device you are using on i.MX is what we have been referring
as the super-node during the discussion. I see device nodes as some
kind of platform_device - no all really end up in one as it depends on
the bus probing the nodes - but they are logical nodes that sometimes
1:1 match the physical nodes (devices).

The remaining issue I see at least for Dove and the DRM driver that
will be compatible with Armada 510 and e.g. PXA2128 or Armada 610 is:

We cannot match the DRM driver with any of the devices_nodes in question
(a) using lcd-controller will always end-up in two DRM drivers on Dove
having two lcd controllers
(b) using display-controller will not work on other SoCs because it is
unique to Armada 510.

With (a) you could tell lcd1 to go to slave-mode as v4l2 does but that
will also lead to very SoC specific drivers. Moreover, you will also
have to tell lcd0 to be either stand-alone or master-mode. You need to
know weather to wait for DRM driver loaded on lcd1 (slave) to fail after
reading slave-mode property.

The super-node solves it easily and has a strong relation to a virtual
video card. The actual point-to-point links match v4l2 approach.

Even the v4l2 approach could be used to describe all possible
combinations we discussed. But I do not see the beauty of it as it will
make dealing with translation of device node to subsystem requirements
and even physical SoC IP a lot more complicated.

With v4l2 you will have to link
(= denoting visible video stream, - logical link)

the single card, single lcd-controller case:
(LCD0)-(HDMI)=

the multiple card, single lcd-controller case:
(LCD0)-(DCON)-(HDMI)= and
(LCD1)---+  +=

and the single card, multiple lcd-controller case:
(LCD0)-(LCD1)-(DCON)-(HDMI)=
+=

All this may allow you to determine the required setup in the driver
but it relates in no way how the data flow is nor how the devices
are physically connected.

Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/05/13 10:43, Grant Likely wrote:

On Wed, Jul 3, 2013 at 10:02 AM, Sascha Hauer s.ha...@pengutronix.de wrote:

On Wed, Jul 03, 2013 at 05:57:18PM +0900, Inki Dae wrote:

video {
 /* Single video card w/ multiple lcd controllers */
 card0 {
 compatible = marvell,armada-510-display;
 reg = 0 0x3f00 0x100; /* video-mem hole */
 /* later: linux,video-memory-size = 0x100; */
 marvell,video-devices = lcd0 lcd1 dcon;
 };

 /* OR: Multiple video card w/ single lcd controllers */
 card0 {
 compatible = marvell,armada-510-display;
 ...
 marvell,video-devices = lcd0;
 };

 card1 {
 compatible = marvell,armada-510-display;
 ...
 marvell,video-devices = lcd1;
 };
};


Sorry but I'd like to say that this cannot be used commonly. Shouldn't you
really consider Linux framebuffer or other subsystems? The above dtsi file
is specific to DRM subsystem. And I think the dtsi file has no any
dependency on certain subsystem so board dtsi file should be considered for
all device drivers based on other subsystems: i.e., Linux framebuffer, DRM,
and so no. So I *strongly* object to it. All we have to do is to keep the
dtsi file as is, and to find other better way that can be used commonly in
DRM.


+1 for not encoding the projected usecase of the graphics subsystem into
the devicetree. Whether the two LCD controllers shall be used together
or separately should not affect the devicetree. devicetree is about
hardware description, not configuration.


It is however relevant to encode information about how devices are
related to each other. That could be an orthogonal binding though to
describe how displays are oriented relative to each other.


Grant,

from what I can see from either super-node approach or v4l2 links with
respect to what we discuss about Marvell SoCs, both are more or less
equivalent.

The only issue left for how to describe that in DT in a sane way is:

Are we using a super-node or node properties for the virtual graphics
card to tell if there is one card with two lcd-controllers (lcd0/lcd1
above) or two cards with one lcd-controller.

You see the super-node solution above, but even with orthogonal
properties we can achieve the same. If we hook up the DRM (or fbdev or
whatever) driver to the lcd-controller nodes, we will have two driver
instances trying to register a DRM card.

For the two card scenario, everything is fine. The driver knows about
a possible DCON (output mux/mirror) and looks for a compatible
available node. Both driver instances may need to access DCON registers
but that is a driver issue and not DT related.

For the one card with two lcd-controllers scenario the only difference
between super-node and node-to-node linking alone is that you get two
driver instances in the first place. With a property equivalent to v4l2
slave-mode that you put on e.g. lcd1, the driver loaded for lcd1 node
bails out silently. The driver loaded for lcd0 also looks for
lcd-controller nodes with slave-mode property and picks up lcd1.

This possibly leads to races, but IMHO as long as the driver looks for
its slave-mode property early, everything should be fine.

All other links required, e.g. lcd0 - hdmi-transmitter, belong to the
respective nodes in both approaches. DCON existence and requirement is
implicitly put in the driver and not required in DT.

Of course, this is independent of how to handle and register sub-drivers
for DRM. But this is subsystem dependent and also not related to DT.

So for the discussion, I can see that there have been some voting for
super-node, some for node-to-node linking. Although I initially proposed
super-nodes, I can also happily live with node-to-node linking alone.

Either someone can give an example where one of the approaches will not
work (i.MX, exynos?), Grant or one of the DRM maintainers has a
preference, or we are stuck at the decision.

Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/04/13 12:09, Sascha Hauer wrote:
> On Thu, Jul 04, 2013 at 11:44:41AM +0200, Sebastian Hesselbarth wrote:
>> On 07/04/13 11:30, Sascha Hauer wrote:
>>> On Thu, Jul 04, 2013 at 10:11:31AM +0100, Russell King wrote:
>>>> On Thu, Jul 04, 2013 at 10:58:17AM +0200, Sascha Hauer wrote:
>>>>> On Thu, Jul 04, 2013 at 09:40:52AM +0100, Russell King wrote:
>>>>>> Wrong.  Please read the example with the diagrams I gave.  Consider
>>>>>> what happens if you have two display devices connected to a single
>>>>>> output, one which fixes the allowable mode and one which _can_
>>>>>> reformat the selected mode.
>>>>>
>>>>> What you describe here is a forced clone mode. This could be described
>>>>> in the devicetree so that a driver wouldn't start before all connected
>>>>> displays (links) are present, but this should be limited to the affected
>>>>> path, not to the whole componentized device.
>>>>
>>>> Okay, to throw a recent argument back at you: so what in this scenario
>>>> if you have a driver for the fixed-mode device but not the other device?
>>>>
>>>> It's exactly the same problem which you were describing to Sebastian
>>>> just a moment ago with drivers missing from the supernode approach -
>>>> you can't start if one of those "forced clone" drivers is missing.
>>>
>>> Indeed, then you will see nothing on your display, but I rather make
>>> this setup a special case than the rather usual case that we do not
>>> have compiled in all drivers for all devices referenced in the
>>> supernode.
>>
>> The super-node links SoC internal devices that do not necessarily match
>> with the subsystem driver. You have one single DRM driver exploiting
>> several device nodes for a single video card.
>>
>> But you need one device node to hook the driver to.
>
> Currently on i.MX we use a platform_device for this purpose.

Sascha,

I have the impression that we are not that far away in our proposals.

The platform_device you are using on i.MX is what we have been referring
as the "super-node" during the discussion. I see device nodes as some
kind of platform_device - no all really end up in one as it depends on
the bus probing the nodes - but they are logical nodes that sometimes
1:1 match the physical nodes (devices).

The remaining issue I see at least for Dove and the DRM driver that
will be compatible with Armada 510 and e.g. PXA2128 or Armada 610 is:

We cannot match the DRM driver with any of the devices_nodes in question
(a) using lcd-controller will always end-up in two DRM drivers on Dove
 having two lcd controllers
(b) using display-controller will not work on other SoCs because it is
 unique to Armada 510.

With (a) you could tell lcd1 to go to "slave-mode" as v4l2 does but that
will also lead to very SoC specific drivers. Moreover, you will also
have to tell lcd0 to be either stand-alone or master-mode. You need to
know weather to wait for DRM driver loaded on lcd1 (slave) to fail after
reading "slave-mode" property.

The super-node solves it easily and has a strong relation to a virtual
video card. The actual point-to-point links match v4l2 approach.

Even the v4l2 approach could be used to describe all possible
combinations we discussed. But I do not see the beauty of it as it will
make dealing with translation of device node to subsystem requirements
and even physical SoC IP a lot more complicated.

With v4l2 you will have to link
(=> denoting visible video stream, -> logical link)

the single card, single lcd-controller case:
(LCD0)->(HDMI)=>

the multiple card, single lcd-controller case:
(LCD0)->(DCON)->(HDMI)=> and
(LCD1)---+  +=>

and the single card, multiple lcd-controller case:
(LCD0)->(LCD1)->(DCON)->(HDMI)=>
 +=>

All this may allow you to determine the required setup in the driver
but it relates in no way how the data flow is nor how the devices
are physically connected.

Sebastian

Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/04/13 11:30, Sascha Hauer wrote:
> On Thu, Jul 04, 2013 at 10:11:31AM +0100, Russell King wrote:
>> On Thu, Jul 04, 2013 at 10:58:17AM +0200, Sascha Hauer wrote:
>>> On Thu, Jul 04, 2013 at 09:40:52AM +0100, Russell King wrote:
 Wrong.  Please read the example with the diagrams I gave.  Consider
 what happens if you have two display devices connected to a single
 output, one which fixes the allowable mode and one which _can_
 reformat the selected mode.
>>>
>>> What you describe here is a forced clone mode. This could be described
>>> in the devicetree so that a driver wouldn't start before all connected
>>> displays (links) are present, but this should be limited to the affected
>>> path, not to the whole componentized device.
>>
>> Okay, to throw a recent argument back at you: so what in this scenario
>> if you have a driver for the fixed-mode device but not the other device?
>>
>> It's exactly the same problem which you were describing to Sebastian
>> just a moment ago with drivers missing from the supernode approach -
>> you can't start if one of those "forced clone" drivers is missing.
>
> Indeed, then you will see nothing on your display, but I rather make
> this setup a special case than the rather usual case that we do not
> have compiled in all drivers for all devices referenced in the
> supernode.

The super-node links SoC internal devices that do not necessarily match
with the subsystem driver. You have one single DRM driver exploiting
several device nodes for a single video card.

But you need one device node to hook the driver to.

Sebastian

Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/04/13 11:23, Sascha Hauer wrote:
> On Thu, Jul 04, 2013 at 11:10:35AM +0200, Sebastian Hesselbarth wrote:
>> On 07/04/13 10:53, Sascha Hauer wrote:
>>> On Thu, Jul 04, 2013 at 10:45:40AM +0200, Sebastian Hesselbarth wrote:
>>>> On 07/04/13 10:33, Sascha Hauer wrote:
>>>>>
>>>>> A componentized device never completes and it doesn't have to. A
>>>>> componentized device can start once there is a path from an input
>>>>> (crtc, i2s unit) to an output (connector, speaker).
>>>>>
>>>>> Consider what happens with a supernode approach. Your board provides a
>>>>> devicetree which has a supernode with hdmi and lvds referenced. Now you
>>>>> build a kernel with the hdmi driver disabled. You would still expect the
>>>>> lvds port to be working without having the kernel wait for the supernode
>>>>> being complete.
>>>>>
>>>>> Without supernode you can just start once you have everything between
>>>>> the crtc and lvds nodes. If later a hdmi device joins in then you can
>>>>> either notify the users (provided the DRM/KMS API supports it) or just
>>>>> ignore it until the DRM device gets reopened.
>>>>
>>>> Sascha,
>>>>
>>>> that is what it is all about. You assume you a priori know what devices
>>>> will be required for the componentized device to successfully output
>>>> a video stream.
>>>>
>>>> We have shown setups where you don't know what is required. Cubox
>>>> _needs_ lcd0 and hdmi-transmitter,
>>>
>>> Then your Cubox devicetree has a link (that's how they call it in v4l2,
>>> a link doesn't necessarily is a direct connection but can have multiple
>>> devices in it) between lcd0 and hdmi.
>>
>> I haven't looked up v4l2 "link" yet. But (a) if it is a separate node
>> how is that different from the "super-node" we are talking about or (b)
>> if it is a property, where do you put it?
>
> Sorry, I should have explained this. The basic idea the v4l2 guys are
> following is that they describe their hardware pipelines in the devicetree.
>
> Each device can have ports which are connected via links. In the
> devicetree a link basically becomes a phandle (a remote device will have
> a phandle pointing back to the original device). For an overview have a
> look at
>
> Documentation/devicetree/bindings/media/video-interfaces.txt
>
> With this you can describe the whole graph of devices you have in the
> devicetree. The examples in this file have a path from a camera sensor
> via a MIPI converter to a capture interface.
>
> The difference to a supernode is that this approach describes the data
> flow in the devicetree so that we can iterate over it to find links
> between source and sink rather than relying on a list of subdevices to
> be completed.

Agree. But that is not that different from linux,video-external-encoder
property I made up, except that the name is different.

And, I still see no way with that source/sink linking _alone_ how to
tell that either lcd0 and lcd1 act as a _single_ video card or lcd0 and
lcd1 are used in a _two_ video card setup.

There is no single device node on Dove that would sufficiently act as
the top node for a working video card on all boards. And there is no
framebuffer node to link each of the lcd0/1 nodes to.

That is what the super-node is for, form a virtual device called
video card to act as a container for all those SoC devices that are
not sufficient for a working video setup on their own.

If lcd0 needs that hdmi-transmitter you link it to the lcd0 node -
not the super-node. If lcd0 needs some pll clock you link it to the
lcd0 node - again not the super-node.

The super-node(s) just connects all SoC devices that shall be part of
your board-specific video card(s) - for Dove that is any combination of
lcd0, lcd0, dcon and video memory allocation.

Sebastian

Sebastian

Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/04/13 10:53, Sascha Hauer wrote:
> On Thu, Jul 04, 2013 at 10:45:40AM +0200, Sebastian Hesselbarth wrote:
>> On 07/04/13 10:33, Sascha Hauer wrote:
>>>
>>> A componentized device never completes and it doesn't have to. A
>>> componentized device can start once there is a path from an input
>>> (crtc, i2s unit) to an output (connector, speaker).
>>>
>>> Consider what happens with a supernode approach. Your board provides a
>>> devicetree which has a supernode with hdmi and lvds referenced. Now you
>>> build a kernel with the hdmi driver disabled. You would still expect the
>>> lvds port to be working without having the kernel wait for the supernode
>>> being complete.
>>>
>>> Without supernode you can just start once you have everything between
>>> the crtc and lvds nodes. If later a hdmi device joins in then you can
>>> either notify the users (provided the DRM/KMS API supports it) or just
>>> ignore it until the DRM device gets reopened.
>>
>> Sascha,
>>
>> that is what it is all about. You assume you a priori know what devices
>> will be required for the componentized device to successfully output
>> a video stream.
>>
>> We have shown setups where you don't know what is required. Cubox
>> _needs_ lcd0 and hdmi-transmitter,
>
> Then your Cubox devicetree has a link (that's how they call it in v4l2,
> a link doesn't necessarily is a direct connection but can have multiple
> devices in it) between lcd0 and hdmi.

I haven't looked up v4l2 "link" yet. But (a) if it is a separate node
how is that different from the "super-node" we are talking about or (b)
if it is a property, where do you put it?

>>   olpc just needs lcd0 and has built-in hdmi in the SoC (IIRC).
>
> And olpc has a link with lcd0 as the source and the builtin hdmi.

Sure, as my DT proposal that Inki shamelessly copied, has a "link"
property for lcd0 on Cubox, while OPLC's lcd0 hasn't.

>> The driver needs to know what to wait for, and that is given by the DT 
>> super-node.
>
> You need a source (described in the devicetree), a sink (also described
> in the devicetree) and a link between them, nothing more.

What if you need source-to-multi-sink or source-to-transceiver-to-sink?
The DT proposal I have given, is source-to-sink on a per device node
basis. If you don't like OPLC's lcd0 having no "link" while Cubox' lcd0
has, put a fake node for the possibly connected HDMI monitor in there.
ASoC does that for SPDIF jack by also having a "codecs" driver although
there is nothing to control.

>> I consider kernels with missing drivers compared to what is given in
>> the DT as broken setup.
>
> What if your devicetree describes components not yet in mainline. Would
> you consider mainline kernels broken then?

No. But I do not 1:1 match device tree nodes with subsystem drivers.
The devices are there, there is no driver.

Currently, for CuBox DT, video _is_ broken because there is no way to
represent the way lcd0, hdmi-transmitter, and pll are connected to form
a single video card. All separate drivers exist, you may be lucky to get
video out of the CuBox because of some reset values that allow you to
get it by coincidence. But as soon as you try the very same kernel on a
different board, your experience will change quickly.

Sebastian

Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/04/13 10:33, Sascha Hauer wrote:
> On Wed, Jul 03, 2013 at 10:52:49AM +0100, Russell King wrote:
 Sorry but I'd like to say that this cannot be used commonly. Shouldn't you
 really consider Linux framebuffer or other subsystems? The above dtsi file
 is specific to DRM subsystem. And I think the dtsi file has no any
 dependency on certain subsystem so board dtsi file should be considered for
 all device drivers based on other subsystems: i.e., Linux framebuffer, DRM,
 and so no. So I *strongly* object to it. All we have to do is to keep the
 dtsi file as is, and to find other better way that can be used commonly in
 DRM.
>>>
>>> +1 for not encoding the projected usecase of the graphics subsystem into
>>> the devicetree. Whether the two LCD controllers shall be used together
>>> or separately should not affect the devicetree. devicetree is about
>>> hardware description, not configuration.
>>
>> And if we listen to that argument, then this problem is basically
>> impossible to solve sanely.
>>
>> Are we really saying that we have no acceptable way to represent
>> componentized devices in DT?  If that's true, then DT fails to represent
>> quite a lot of ARM hardware, and frankly we shouldn't be using it.  I
>> can't believe that's true though.
>>
>> The problem is that even with an ASoC like approach, that doesn't work
>> here because there's no way to know how many "components" to expect.
>> That's what the "supernode" is doing - telling us what components group
>> together to form a device.
>
> A componentized device never completes and it doesn't have to. A
> componentized device can start once there is a path from an input
> (crtc, i2s unit) to an output (connector, speaker).
>
> Consider what happens with a supernode approach. Your board provides a
> devicetree which has a supernode with hdmi and lvds referenced. Now you
> build a kernel with the hdmi driver disabled. You would still expect the
> lvds port to be working without having the kernel wait for the supernode
> being complete.
>
> Without supernode you can just start once you have everything between
> the crtc and lvds nodes. If later a hdmi device joins in then you can
> either notify the users (provided the DRM/KMS API supports it) or just
> ignore it until the DRM device gets reopened.

Sascha,

that is what it is all about. You assume you a priori know what devices
will be required for the componentized device to successfully output
a video stream.

We have shown setups where you don't know what is required. Cubox
_needs_ lcd0 and hdmi-transmitter, olpc just needs lcd0 and has
built-in hdmi in the SoC (IIRC). The driver needs to know what to
wait for, and that is given by the DT super-node.

I consider kernels with missing drivers compared to what is given in
the DT as broken setup. You cannot complain about missing SATA if you
leave out SATA driver, or - if you implemented the driver into two
parts - leave out one of the two SATA driver parts.

Sebastian

Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/04/13 09:05, Inki Dae wrote:
>> -Original Message-
>> From: Sebastian Hesselbarth [mailto:sebastian.hesselbarth at gmail.com]
>> Sent: Wednesday, July 03, 2013 8:52 PM
>> To: Inki Dae
>> Cc: 'Russell King'; devicetree-discuss at lists.ozlabs.org; 'Jean-Francois
>> Moine'; 'Sascha Hauer'; 'Daniel Drake'; dri-devel at lists.freedesktop.org
>> Subject: Re: Best practice device tree design for display subsystems/DRM
>>
>> On 07/03/13 13:43, Inki Dae wrote:
>>>> I do not understand why you keep referring to the SoC dtsi. Im my
>>>> example, I said that it is made up and joined from both SoC dtsi and
>>>> board dts.
>>>>
>>>> So, of course, lcd controller nodes and dcon are part of dove.dtsi
>>>> because they are physically available on every Dove SoC.
>>>>
>>>> Also, the connection from lcd0 to the external HDMI encoder node
>>>> is in the board dts because you can happily build a Dove SoC board
>>>> with a different HDMI encoder or with no encoder at all.
>>>>
>>>> The video-card super node is not in any way specific to DRM and
>>>
>>> In case of fbdev, framebuffer driver would use lcd0 or lcd1 driver, or
>> lcd0
>>> and lcd1 drivers which are placed in drivers/video/backlight/.
>>>
>>> And let's assume the following:
>>>
>>> On board A
>>>Display controller - lcd 0
>>> On board B
>>>Display controller - lcd 1
>>> On board C
>>>Display controller - lcd 0 and lcd 1
>>>
>>> Without the super node, user could configure Linux kernel through
>> menuconfig
>>> like below;
>>> board A:  enabling lcd 0, and disabling lcd 1,
>>> board B: disabling lcd 0, and enabling lcd 1,
>>> board C: enabling lcd 0 and lcd 1.
>>>
>>> All we have to do is to configure menuconfig to enable only drivers for
>>> certain board. Why does fbdev need the super node? Please give me
>> comments
>>> if there is my missing point.
>>
>> I assume when you say "configure menuconfig" you mean
>> "create a CONFIG_DISPLAY_CONTROLLER_AS_USED_ON_BOARD_A,
>> CONFIG_DISPLAY_CONTROLLER_AS_USED_ON_BOARD_B, ..." ?
>>
>> This finally will require you to have
>> (a) #ifdef mess for every single board _and_ driver above
>> (b) new CONFIG_.._BOARD_D plus new #ifdefs in fbdev driver for every
>>   new board
>> (c) A new set of the above CONFIG_/#ifdef for DRM driver
>>
>> This can also be done with device_tree and supernode approach,
>> so for your example above:
>>
>> BoardA.dts:
>> video { card0 { video-devices = <>; }; };
>>
>> BoardB.dts:
>> video { card0 { video-devices = <>; }; };
>>
>> BoardC.dts:
>> video { card0 { video-devices = < >; }; };
>>
>> and in the driver your are prepared for looping over the video-devices
>> property and parsing the compatible string of the nodes passed.
>>
>
> As I mentioned before, fbdev don't need the super node, card0. Please see
> the below,
>
> BoardA.dts:
> video { dcon: display-controller at 83 { video-devices = <>; }; };
>
> BoardB.dts:
> video { dcon: display-controller at 83 { video-devices = <>; }; };
>
> BoardC.dts:
> video { dcon: display-controller at 83 { video-devices = < >; };
> };
>
> With the above dts file, does the fbdev have any problem? I just changed the
> super node to real hardware node. That is why the super node is specific to
> DRM.

Inki,

I guess there is a misunderstanding of what lcd-controller and display-
controller are for on Dove. lcd-controller reads framebuffer from
memory, optionally does some conversions/scaling, and drives the SoCs
pins with pixel data and sync. display-controller (dcon) on Dove is for
mirroring lcd0 framebuffer to lcd1 framebuffer and some other things.

And, as stated several times, you cannot move internal-registers out of
the corresponding node on Dove. You _need_ that parent node for address
mapping.

IMHO also fbdev needs the super-node because lcd0/1, dcon,
hdmi-transmitter, programmable-pll is what make up what you would call
a graphics card on x86. There is no such "graphics card" on most SoCs
but it is built up by using separate devices and SoC internal devices.

Moreover, it is highly board dependent because you will easily find
another board manufacturer chosing a different hdmi-transmitter or
programmable-pll, using two lcd-controllers or just one. And there is
no way of probing the boards configuration.

So even fvdev needs the super-node, there is no difference what video
subsystem you use - just because DT describes HW (even virtual one) not
subsystem.

Sebastian

Re: Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/03/13 11:53, Russell King wrote:

On Wed, Jul 03, 2013 at 06:48:41PM +0900, Inki Dae wrote:

That's not whether we can write device driver or not. dtsi is common spot in
other subsystems. Do you think the cardX node is meaningful to other
subsystems?


Yes, because fbdev could also use it to solve the same problem which we're
having with DRM.


Inki,

I do not understand why you keep referring to the SoC dtsi. Im my
example, I said that it is made up and joined from both SoC dtsi and
board dts.

So, of course, lcd controller nodes and dcon are part of dove.dtsi
because they are physically available on every Dove SoC.

Also, the connection from lcd0 to the external HDMI encoder node
is in the board dts because you can happily build a Dove SoC board
with a different HDMI encoder or with no encoder at all.

The video-card super node is not in any way specific to DRM and
describes a virtual graphics card comprising both SoC and board
components (on a per-board basis). You can have both DRM or fbdev
use that virtual video card node to register your subsystem drivers
required to provide video output.

I agree to what Sascha said, the decision to put one or two
virtual graphics card in the device tree depending on the use
case is sketchy. You can have one card/two card on the same
board, so at this point device tree is not describing HW but
use case.

But honestly, I see no way around it and it is the only way
to allow to even have the decision for one or two cards at all.
There is no way for auto-probing the users intention...

Sebastian

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/03/13 11:52, Russell King wrote:

On Wed, Jul 03, 2013 at 11:02:42AM +0200, Sascha Hauer wrote:

On Wed, Jul 03, 2013 at 05:57:18PM +0900, Inki Dae wrote:

video {
/* Single video card w/ multiple lcd controllers */
card0 {
compatible = marvell,armada-510-display;
reg = 0 0x3f00 0x100; /* video-mem hole */
/* later: linux,video-memory-size = 0x100; */
marvell,video-devices = lcd0 lcd1 dcon;
};

/* OR: Multiple video card w/ single lcd controllers */
card0 {
compatible = marvell,armada-510-display;
...
marvell,video-devices = lcd0;
};

card1 {
compatible = marvell,armada-510-display;
...
marvell,video-devices = lcd1;
};
};


Sorry but I'd like to say that this cannot be used commonly. Shouldn't you
really consider Linux framebuffer or other subsystems? The above dtsi file
is specific to DRM subsystem. And I think the dtsi file has no any
dependency on certain subsystem so board dtsi file should be considered for
all device drivers based on other subsystems: i.e., Linux framebuffer, DRM,
and so no. So I *strongly* object to it. All we have to do is to keep the
dtsi file as is, and to find other better way that can be used commonly in
DRM.


+1 for not encoding the projected usecase of the graphics subsystem into
the devicetree. Whether the two LCD controllers shall be used together
or separately should not affect the devicetree. devicetree is about
hardware description, not configuration.


And if we listen to that argument, then this problem is basically
impossible to solve sanely.

Are we really saying that we have no acceptable way to represent
componentized devices in DT?  If that's true, then DT fails to represent
quite a lot of ARM hardware, and frankly we shouldn't be using it.  I
can't believe that's true though.


I think DT is able to describe componentized devices, as long as you
ignore DRM/fbdev/ASoC's demands and try to have a look at the HW without
any specific backend in mind.

We both had a similar discussion about ASoC's separation of bus-side and
codec-side subdevices. In HW, there is no such separation but one single
audio controller (at least on Dove). Moreover, a full featured, again
virtual, sound card comprises a lot more than just what is in the SoC.
There is external codecs, jacks, aso.


The problem is that even with an ASoC like approach, that doesn't work
here because there's no way to know how many components to expect.
That's what the supernode is doing - telling us what components group
together to form a device.


True. The supernode forms a virtual device on top of the individual
components of both SoC and board. For the driver subsystem, all that is
required should be probed by starting from the supernode. If what is
found is not sufficient for the driver subsystem to register a working
subsystem device, bail out. If there is more than you expect, ignore
it and cross your fingers. IMHO DT is not the solution for describing
the world, but it is sufficient for any subsystem driver to find what
it needs to know.


Moreover, if you pay attention to my proposal, what you will realise is
that it isn't DRM specific - it's totally subsystem agnostic.  All it's
doing is collecting a set of other devices together and only then
publishing a device representing the full set of sub-devices.

Now think about that: what is DRM specific about that solution?  What
is the DRM specific about collecting a set of devices together and
publishing a new device ?

How is this not describing the hardware?  If I attach a HDMI transceiver
to the DCON which is then connected to LCD0, is it not describing the
hardware to put into DT that LCD0, DCON, and the HDMI transceiver are
all connected together and therefore are required?  One of the points of
DT after all is that it can and should be used to represent the
relationship between devices.

No - using the tree approach doesn't work, because LCD0, LCD1 and DCON
are all on the same physical bus, but are themselves connected together.
If you like, there are multiple heirarchies here - there's the bus
heirarchy, and then there's the device heirarchy.  Both of these
heirarchies need to be represented in DT, otherwise you're not describing
the hardware properly.


IMHO DT is more than describing physical connections between devices but
also describing logical connections between devices. While, for example,
a SATA bus master could happily do DMA writes to the GPIO registers just
because it is physically connected to it, it makes no sense. OTOH an LED
connected to a gpio pin is not directly connected to the GPIO register
but at least needs to know who to ask for toggling the line.

I know there may be better examples than those above, but describing
a virtual video card with a supernode connecting separate devices is

Re: Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/03/13 13:32, Russell King wrote:

On Wed, Jul 03, 2013 at 12:52:37PM +0200, Sebastian Hesselbarth wrote:

But honestly, I see no way around it and it is the only way
to allow to even have the decision for one or two cards at all.
There is no way for auto-probing the users intention...


Russell,

in general, for me it is hard to find out when you are really asking
questions, use rhetorical questions or sarcasm. I am not a native 
speaker, so do not take any of the below personal if I am not getting

the point of it.


It's not _just_ about the users intention - for get that, because really
it's to do with solving a much bigger question, and that question is:

How do we know when all the components are present?


By exploiting phandles passed to the supernode. That supernode is very
board specific so it is defined in a board dts and can take into account
every available/unavailable physical device.


In other words, how do we know that we have LCD0 and LCD1 connected to
the DCON, which is connected to a LCD and/or a HDMI tranceiver.  How


About LCD0/LCD1 connected to DCON, you have to deal with that in the
subsystem driver, i.e. DRM driver knows about some possible DCON but
registers it only if there is a phandle to a node compatible with
marvell,armada-510-dcon passed.

About LCD (panel)/HDMI transceiver, use a (hopefully standard) property
to hook the device (LCD0 on Dove) with the node of the HDMI transmitter.


do we know that the on-board VGA DACs are wired up and to be used?


Boards not using VGA DAC (LCD1 on Dove) just disable the device_node
and do not pass the node's phandle in the supernode.


How do we know which I2C bus the VGA port is connected to, and whether
to expect an I2C bus?


Again, passing a phandle to the i2c-controller node in lcd1 node.
Please note that the pure existence of this phandle property does not
in any way imply you have to use it at all in the driver. But if you
have a driver for Dove's LCD1 and that needs to know how it can access
monitor's EDID, connect it to the i2c-controller node.

I understand that this very i2c-controller driver may not be loaded
the time DRM driver accesses it. But that is not related to DT but
driver core. Currently, I guess -EPROBEDEFER or bail out is the only
option. But from a driver POV you can still say: somebody told me
to use i2c0 for EDID, so don't blame me it is not there.


Let's look at the Cubox setup (sorry, but you _will_ have to use a
fixed-width font for this):

CPU bus
|
+-I2C -TDA998X --(HDMI)-- Display
||
|   (RGB888+clock+sync)
+-LCD0 -.   /
+--DCON ---(VGA)--- not wired
+-LCD1 (unused)-'

DCON can allegedly route the data from LCD0 or LCD1 to the parallel
interface which the TDA998x sits on, and/or the VGA interface.  In
the case of other setups, the TDA998x may be a LCD panel.


dove.dtsi:
...
soc {
internal-regs {
...
i2c0: i2c-controller@abcd0 {
compatible = marvell,mv64xxx-i2c;
...
status = disabled;
};

lcd0: lcd-controller@82 {
compatible = marvell,armada-510-lcd;
reg = 0x82 0x1000;
status = disabled;
};

lcd1: lcd-controller@81 {
compatible = marvell,armada-510-lcd;
reg = 0x81 0x1000;
status = disabled;
};

dcon: display-controller@83 {
compatible = marvell,armada-510-dcon;
reg = 0x83 0x100;
status = disabled;
};
};
};

dove-cubox.dts:
/include/ dove.dtsi

video {
card0 {
compatible = marvell,armada-510-video,
 linux,video-card;
linux,video-memory-size = 0x10;
linux,video-devices = lcd0 dcon;
};
};

dcon { status = okay; };

lcd0 {
status = okay;
clocks = si5351 0;
clock-names = extclk0;
/* pin config 0 = DUMB_RGB888 */
marvell,pin-configuration = 0;
...
linux,video-external-encoder = tda998x;
};

i2c0 {
status = okay;

tda998x: hdmi-transmitter@60 {
compatible = nxp,tda19988;
reg = 0x60;
/* pin config 18 = RGB888 */
nxp,pin-configuration = 18;
/* HPD gpio pin */
interrupt-gpios = gpio0 12;
};

si5351: programmable-pll {
/* Note: this binding already exists */
compatible = silabs,5351a-msop10;
...
#clock-cells = 1;

/* referenced as si5351 0 */
clk0: {
silabs,drive-strength = 8

Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/03/13 13:32, Russell King wrote:
> On Wed, Jul 03, 2013 at 12:52:37PM +0200, Sebastian Hesselbarth wrote:
>> But honestly, I see no way around it and it is the only way
>> to allow to even have the decision for one or two cards at all.
>> There is no way for auto-probing the users intention...

Russell,

in general, for me it is hard to find out when you are really asking
questions, use rhetorical questions or sarcasm. I am not a native 
speaker, so do not take any of the below personal if I am not getting
the point of it.

> It's not _just_ about the users intention - for get that, because really
> it's to do with solving a much bigger question, and that question is:
>
>   How do we know when all the components are present?

By exploiting phandles passed to the supernode. That supernode is very
board specific so it is defined in a board dts and can take into account
every available/unavailable physical device.

> In other words, how do we know that we have LCD0 and LCD1 connected to
> the DCON, which is connected to a LCD and/or a HDMI tranceiver.  How

About LCD0/LCD1 connected to DCON, you have to deal with that in the
subsystem driver, i.e. DRM driver knows about some possible DCON but
registers it only if there is a phandle to a node compatible with
"marvell,armada-510-dcon" passed.

About LCD (panel)/HDMI transceiver, use a (hopefully standard) property
to hook the device (LCD0 on Dove) with the node of the HDMI transmitter.

> do we know that the on-board VGA DACs are wired up and to be used?

Boards not using VGA DAC (LCD1 on Dove) just disable the device_node
and do not pass the node's phandle in the supernode.

> How do we know which I2C bus the VGA port is connected to, and whether
> to expect an I2C bus?

Again, passing a phandle to the i2c-controller node in lcd1 node.
Please note that the pure existence of this phandle property does not
in any way imply you have to use it at all in the driver. But if you
have a driver for Dove's LCD1 and that needs to know how it can access
monitor's EDID, connect it to the i2c-controller node.

I understand that this very i2c-controller driver may not be loaded
the time DRM driver accesses it. But that is not related to DT but
driver core. Currently, I guess -EPROBEDEFER or bail out is the only
option. But from a driver POV you can still say: "somebody told me
to use i2c0 for EDID, so don't blame me it is not there".

> Let's look at the Cubox setup (sorry, but you _will_ have to use a
> fixed-width font for this):
>
> CPU bus
> |
> +-I2C -TDA998X --(HDMI)--> Display
> ||
> |   (RGB888+clock+sync)
> +-LCD0 -.   /
> +--DCON ---(VGA)---> not wired
> +-LCD1 (unused)-'
>
> DCON can allegedly route the data from LCD0 or LCD1 to the parallel
> interface which the TDA998x sits on, and/or the VGA interface.  In
> the case of other setups, the TDA998x may be a LCD panel.

dove.dtsi:
...
soc {
internal-regs {
...
i2c0: i2c-controller at abcd0 {
compatible = "marvell,mv64xxx-i2c";
...
status = "disabled";
};

lcd0: lcd-controller at 82 {
compatible = "marvell,armada-510-lcd";
reg = <0x82 0x1000>;
status = "disabled";
};

lcd1: lcd-controller at 81 {
compatible = "marvell,armada-510-lcd";
reg = <0x81 0x1000>;
status = "disabled";
};

dcon: display-controller at 83 {
compatible = "marvell,armada-510-dcon";
reg = <0x83 0x100>;
status = "disabled";
};
};
};

dove-cubox.dts:
/include/ "dove.dtsi"

video {
card0 {
compatible = "marvell,armada-510-video",
  "linux,video-card";
linux,video-memory-size = <0x10>;
linux,video-devices = < >;
};
};

 { status = "okay"; };

 {
status = "okay";
clocks = < 0>;
clock-names = "extclk0";
/* pin config 0 = DUMB_RGB888 */
marvell,pin-configuration = <0>;
...
linux,video-external-encoder = <>;
};

 {
status = "okay";

tda998x: hdmi-transmitter at 60 {
compatible = "nxp,tda19988";
reg = <0x60>;
/* pin config 18 = RGB888 */
nxp,pin-configuration

Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/03/13 13:43, Inki Dae wrote:
>> I do not understand why you keep referring to the SoC dtsi. Im my
>> example, I said that it is made up and joined from both SoC dtsi and
>> board dts.
>>
>> So, of course, lcd controller nodes and dcon are part of dove.dtsi
>> because they are physically available on every Dove SoC.
>>
>> Also, the connection from lcd0 to the external HDMI encoder node
>> is in the board dts because you can happily build a Dove SoC board
>> with a different HDMI encoder or with no encoder at all.
>>
>> The video-card super node is not in any way specific to DRM and
>
> In case of fbdev, framebuffer driver would use lcd0 or lcd1 driver, or lcd0
> and lcd1 drivers which are placed in drivers/video/backlight/.
>
> And let's assume the following:
>
> On board A
>   Display controller - lcd 0
> On board B
>   Display controller - lcd 1
> On board C
>   Display controller - lcd 0 and lcd 1
>
> Without the super node, user could configure Linux kernel through menuconfig
> like below;
>   board A:  enabling lcd 0, and disabling lcd 1,
>   board B: disabling lcd 0, and enabling lcd 1,
>   board C: enabling lcd 0 and lcd 1.
>
> All we have to do is to configure menuconfig to enable only drivers for
> certain board. Why does fbdev need the super node? Please give me comments
> if there is my missing point.

I assume when you say "configure menuconfig" you mean
"create a CONFIG_DISPLAY_CONTROLLER_AS_USED_ON_BOARD_A,
CONFIG_DISPLAY_CONTROLLER_AS_USED_ON_BOARD_B, ..." ?

This finally will require you to have
(a) #ifdef mess for every single board _and_ driver above
(b) new CONFIG_.._BOARD_D plus new #ifdefs in fbdev driver for every
 new board
(c) A new set of the above CONFIG_/#ifdef for DRM driver

This can also be done with device_tree and supernode approach,
so for your example above:

BoardA.dts:
video { card0 { video-devices = <>; }; };

BoardB.dts:
video { card0 { video-devices = <>; }; };

BoardC.dts:
video { card0 { video-devices = < >; }; };

and in the driver your are prepared for looping over the video-devices
property and parsing the compatible string of the nodes passed.

Sebastian

Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/03/13 11:52, Russell King wrote:
> On Wed, Jul 03, 2013 at 11:02:42AM +0200, Sascha Hauer wrote:
>> On Wed, Jul 03, 2013 at 05:57:18PM +0900, Inki Dae wrote:
 video {
/* Single video card w/ multiple lcd controllers */
card0 {
compatible = "marvell,armada-510-display";
reg = <0 0x3f00 0x100>; /* video-mem hole */
/* later: linux,video-memory-size = <0x100>; */
marvell,video-devices = <  >;
};

/* OR: Multiple video card w/ single lcd controllers */
card0 {
compatible = "marvell,armada-510-display";
...
marvell,video-devices = <>;
};

card1 {
compatible = "marvell,armada-510-display";
...
marvell,video-devices = <>;
};
 };
>>>
>>> Sorry but I'd like to say that this cannot be used commonly. Shouldn't you
>>> really consider Linux framebuffer or other subsystems? The above dtsi file
>>> is specific to DRM subsystem. And I think the dtsi file has no any
>>> dependency on certain subsystem so board dtsi file should be considered for
>>> all device drivers based on other subsystems: i.e., Linux framebuffer, DRM,
>>> and so no. So I *strongly* object to it. All we have to do is to keep the
>>> dtsi file as is, and to find other better way that can be used commonly in
>>> DRM.
>>
>> +1 for not encoding the projected usecase of the graphics subsystem into
>> the devicetree. Whether the two LCD controllers shall be used together
>> or separately should not affect the devicetree. devicetree is about
>> hardware description, not configuration.
>
> And if we listen to that argument, then this problem is basically
> impossible to solve sanely.
>
> Are we really saying that we have no acceptable way to represent
> componentized devices in DT?  If that's true, then DT fails to represent
> quite a lot of ARM hardware, and frankly we shouldn't be using it.  I
> can't believe that's true though.

I think DT is able to describe componentized devices, as long as you
ignore DRM/fbdev/ASoC's demands and try to have a look at the HW without
any specific backend in mind.

We both had a similar discussion about ASoC's separation of bus-side and
codec-side subdevices. In HW, there is no such separation but one single
audio controller (at least on Dove). Moreover, a full featured, again
virtual, sound card comprises a lot more than just what is in the SoC.
There is external codecs, jacks, aso.

> The problem is that even with an ASoC like approach, that doesn't work
> here because there's no way to know how many "components" to expect.
> That's what the "supernode" is doing - telling us what components group
> together to form a device.

True. The supernode forms a virtual device on top of the individual
components of both SoC and board. For the driver subsystem, all that is
required should be probed by starting from the supernode. If what is
found is not sufficient for the driver subsystem to register a working
subsystem device, bail out. If there is more than you expect, ignore
it and cross your fingers. IMHO DT is not the solution for describing
the world, but it is sufficient for any subsystem driver to find what
it needs to know.

> Moreover, if you pay attention to my proposal, what you will realise is
> that it isn't DRM specific - it's totally subsystem agnostic.  All it's
> doing is collecting a set of other devices together and only then
> publishing a device representing the full set of sub-devices.
>
> Now think about that: what is DRM specific about that solution?  What
> is the DRM specific about "collecting a set of devices together and
> publishing a new device" ?
>
> How is this not "describing the hardware"?  If I attach a HDMI transceiver
> to the DCON which is then connected to LCD0, is it not "describing the
> hardware" to put into DT that LCD0, DCON, and the HDMI transceiver are
> all connected together and therefore are required?  One of the points of
> DT after all is that it can and should be used to represent the
> relationship between devices.
>
> No - using the tree approach doesn't work, because LCD0, LCD1 and DCON
> are all on the same physical bus, but are themselves connected together.
> If you like, there are multiple heirarchies here - there's the bus
> heirarchy, and then there's the device heirarchy.  Both of these
> heirarchies need to be represented in DT, otherwise you're not describing
> the hardware properly.

IMHO DT is more than describing physical connections between devices but
also describing logical connections between devices. While, for example,
a SATA bus master could happily do DMA writes to the GPIO registers just
because it is physically connected to it, it makes no sense. OTOH an LED
connected to a gpio pin is not directly connected to the GPIO register
but at least needs to know who to ask for toggling the line.

I know 

Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/03/13 11:53, Russell King wrote:
> On Wed, Jul 03, 2013 at 06:48:41PM +0900, Inki Dae wrote:
>> That's not whether we can write device driver or not. dtsi is common spot in
>> other subsystems. Do you think the cardX node is meaningful to other
>> subsystems?
>
> Yes, because fbdev could also use it to solve the same problem which we're
> having with DRM.

Inki,

I do not understand why you keep referring to the SoC dtsi. Im my
example, I said that it is made up and joined from both SoC dtsi and
board dts.

So, of course, lcd controller nodes and dcon are part of dove.dtsi
because they are physically available on every Dove SoC.

Also, the connection from lcd0 to the external HDMI encoder node
is in the board dts because you can happily build a Dove SoC board
with a different HDMI encoder or with no encoder at all.

The video-card super node is not in any way specific to DRM and
describes a virtual graphics card comprising both SoC and board
components (on a per-board basis). You can have both DRM or fbdev
use that virtual video card node to register your subsystem drivers
required to provide video output.

I agree to what Sascha said, the decision to put one or two
virtual graphics card in the device tree depending on the use
case is sketchy. You can have one card/two card on the same
board, so at this point device tree is not describing HW but
use case.

But honestly, I see no way around it and it is the only way
to allow to even have the decision for one or two cards at all.
There is no way for auto-probing the users intention...

Sebastian

Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/03/13 11:02, Sascha Hauer wrote:
> On Wed, Jul 03, 2013 at 05:57:18PM +0900, Inki Dae wrote:
>>> video {
>>> /* Single video card w/ multiple lcd controllers */
>>> card0 {
>>> compatible = "marvell,armada-510-display";
>>> reg = <0 0x3f00 0x100>; /* video-mem hole */
>>> /* later: linux,video-memory-size = <0x100>; */
>>> marvell,video-devices = <  >;
>>> };
>>>
>>> /* OR: Multiple video card w/ single lcd controllers */
>>> card0 {
>>> compatible = "marvell,armada-510-display";
>>> ...
>>> marvell,video-devices = <>;
>>> };
>>>
>>> card1 {
>>> compatible = "marvell,armada-510-display";
>>> ...
>>> marvell,video-devices = <>;
>>> };
>>> };
>>
>> Sorry but I'd like to say that this cannot be used commonly. Shouldn't you
>> really consider Linux framebuffer or other subsystems? The above dtsi file
>> is specific to DRM subsystem. And I think the dtsi file has no any
>> dependency on certain subsystem so board dtsi file should be considered for
>> all device drivers based on other subsystems: i.e., Linux framebuffer, DRM,
>> and so no. So I *strongly* object to it. All we have to do is to keep the
>> dtsi file as is, and to find other better way that can be used commonly in
>> DRM.

Sascha, Inki,

can you clarify how the above will _not_ allow you to write a fb driver
exploiting the cardX nodes?

While lcd controller and dcon are physically available, the video card
is just a virtual combination of those.

> +1 for not encoding the projected usecase of the graphics subsystem into
> the devicetree. Whether the two LCD controllers shall be used together
> or separately should not affect the devicetree. devicetree is about
> hardware description, not configuration.

Have you had a look at gpio-leds? It _is_ actually a configuration of
GPIO to be used as LED triggers. IMHO DT is just fine for describing
even "virtual" hardware you make up out of existing devices. Without it
there is no way for the subsystems to determine the configuration.

Regarding gpio-leds, how should the driver know the single gpio line
out of tens of available lines, if you do not use a virtual gpio led
node to describe it?

Sebastian

Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/03/13 08:55, Sascha Hauer wrote:
> On Wed, Jul 03, 2013 at 08:02:05AM +1000, Dave Airlie wrote:
>> Have you also considered how suspend/resume works in such a place,
>> where every driver is independent? The ChromeOS guys have bitched
>> before about the exynos driver which is has lots of sub-drivers, how
>> do you control the s/r ordering in a crazy system like that? I'd
>> prefer a central driver, otherwise there is too many moving parts.
>
> Composing a DRM device out of subdevices doesn't necessarily mean the
> components should be suspended/resumed in arbitrary order. The DRM
> device should always be suspended first (thus deactivating sub devices
> as necessary and as done already) and resumed last.
>
> Note that a super node approach does not solve this magically. We would
> still have to make sure that the i2c bus masters on our SoC are suspended
> after the DRM device.

+1 for a video card supernode that at best should be some very generic
node with standard properties provided by DRM backend. IIRC there was
a proposal for of_video_card a while ago.

At least for Marvell SoCs, moving device nodes out of the bus structure
will not work. The parent bus is _required_ for address mapping as the
base address is configurable. Using phandles can solve this without
moving nodes.

Also, having separate device nodes does not require a separate driver
for each nodes. All nodes get platform_devices registered, but you can
choose not to have a matching driver for it. Then the video card super
node can pick up that nodes by using the phandles passed and register
a single DRM driver claiming the devices.

Moreover, if we talk about SoC graphics, we have to take audio into
account. If you move all nodes to your video card super node, you will
add another bunch of issues for ASoC linking to e.g. the I2C HDMI
transmitter SPDIF codec.

IMHO phandles and super node subnodes are equivalent from a driver
point-of-view but phandles are more likely to cause less pain for
other subsystems.

The super node approach will also allow to have the same SoC/board
components being used as single video card or multiple video cards
environment. There is virtually no way to automatically determine what
devices belong to "your" video card(s) in a SoC, so we need something to
describe those cards.

One thing I am concerned about is what Sascha pointed out above. If you
hook up an external I2C encoder to your card, you cannot make sure I2C
bus is suspended before DRM device. To be honest, proposing a solution
for that is still way beyond my expertise wrt to Linux internals, so I
am not even trying it. Maybe I am even missing a very important point
for the super node/phandle proposal, if so, please clarify.

Sebastian

Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/02/2013 11:04 PM, Daniel Drake wrote:
> On Tue, Jul 2, 2013 at 1:57 PM, Sebastian Hesselbarth
>   wrote:
>> I am against a super node which contains lcd and dcon/ire nodes. You can
>> enable those devices on a per board basis. We add them to dove.dtsi but
>> disable them by default (status = "disabled").
>>
>> The DRM driver itself should get a video-card node outside of
>> soc/internal-regs where you can put e.g. video memory hole (or video
>> mem size if it will be taken from RAM later)
>
> For completeness of the discussion, and my understanding too, can you
> explain your objections to the display super-node in a bit more
> detail?

lcd-controller nodes and dcon node will need to be children of
internal-regs nodes. The internal-regs node is required for address
translation as the mbus base address can be configured. The above does
not permit a super-node - but you cannot have the nodes above outside
of internal-regs.

As Russell stated, he wants a proposal for the "unusual case" i.e.
you have two lcd controllers. You use one for Xorg and the other for
e.g. running a linux terminal console.

This would require some reference from the super node to the lcd
controller to sort out which DRM device (represented by the super
node) should be using which lcd controller device.

Using status = "disabled" alone will only allow to enable or disable
lcd controller nodes but not assign any of it to your two super-nodes.

So my current proposal after thinking about Russell's statements
whould be phandles as Jean-Francois already mentioned. I am not sure
what OF maintainers will think about it, but that is another thing.

Basically, you will have:
(Note: names and property-names are just to show how it could work,
and example is joined from possible future dove.dtsi and dove-board.dts)

video {
/* Single video card w/ multiple lcd controllers */
card0 {
compatible = "marvell,armada-510-display";
reg = <0 0x3f00 0x100>; /* video-mem hole */
/* later: linux,video-memory-size = <0x100>; */
marvell,video-devices = <  >;
};

/* OR: Multiple video card w/ single lcd controllers */
card0 {
compatible = "marvell,armada-510-display";
...
marvell,video-devices = <>;
};

card1 {
compatible = "marvell,armada-510-display";
...
marvell,video-devices = <>;
};
};

mbus {
compatible = "marvell,dove-mbus";
ranges = <...>;

sb-regs {
ranges = <0 0xf100 0 0x10>;
...
}

nb-regs {
ranges = <0 0xf180 0 0x10>;

lcd0: lcd-controller at 2 {
compatible = "marvell,armada-510-lcd";
reg = <0x2 0x1000>;
interrupts = <47>;
...
/* use EXTCLK0 with lcd0 */
clocks = <_clk0>;
clock-names = "extclk0";
marvell,external-encoder = <>;
};

lcd1: lcd-controller at 1 {
compatible = "marvell,armada-510-lcd";
reg = <0x1 0x1000>;
interrupts = <46>;
...
/* use LCDPLL with lcd1 */
clocks = <_pll_clk>;
clock-names = "lcdpll";
};
}
};

 {
tda998x: hdmi-transmitter at 60 {
compatible = "nxp,tda19988";
reg = <0x60>;
...
}
};

Each lcd controller node represents a platform_device and the display
nodes above should look up phandles and determine type (ctrc or dcon)
by compatible string of the nodes the phandles point to.

Sebastian

Re: armada_drm clock selection - try 2

[Sebastian Hesselbarth]

On 07/01/2013 10:30 PM, Daniel Drake wrote:

Here is a new patch which should incorporate all your previous feedback.
Now each variant passes clock info to the main driver via a new
armada_clk_info structure.

A helper function in the core lets each variant find the best clock.
As you suggested we first try external (dedicated) clocks, where we can
change the rate to find an exact match. We fall back on looking at the rates
of the other clocks and we return the clock that provides us with the closest
match after integer division (rejecting those that don't bring us within 1%).

There is still the possibility that two CRTCs on the same device end up using
the same clock, so I added a usage counter to each clock to prevent the rate
from being changed by another CRTC.

This is compile-tested only - but after your feedback I will add the
remaining required hacks and test it on XO.

diff --git a/drivers/gpu/drm/armada/armada_510.c 
b/drivers/gpu/drm/armada/armada_510.c
index a016888..7dff2dc 100644
--- a/drivers/gpu/drm/armada/armada_510.c
+++ b/drivers/gpu/drm/armada/armada_510.c
@@ -17,12 +17,29 @@

  static int armada510_init(struct armada_private *priv, struct device *dev)
  {
-   priv-extclk[0] = devm_clk_get(dev, ext_ref_clk_1);
+   struct armada_clk_info *clk_info = devm_kzalloc(dev,
+   sizeof(struct armada_clk_info) * 4, GFP_KERNEL);

-   if (IS_ERR(priv-extclk[0])  PTR_ERR(priv-extclk[0]) == -ENOENT)
-   priv-extclk[0] = ERR_PTR(-EPROBE_DEFER);
+   if (!clk_info)
+   return -ENOMEM;

-   return PTR_RET(priv-extclk[0]);
+   /* External clocks */
+   clk_info[0].clk = devm_clk_get(dev, ext_ref_clk_0);
+   clk_info[0].is_dedicated = true;


Daniel,

I guess extclk0 and extclk1 should be sufficient for clock names.
Also, they are not dedicated as you can have CRTC0 and CRTC1 use e.g.
extclk0 simultaneously. See below for .is_dedicated in general.

[...]

diff --git a/drivers/gpu/drm/armada/armada_drm.h 
b/drivers/gpu/drm/armada/armada_drm.h
index e8c4f80..4fe8ec5 100644
--- a/drivers/gpu/drm/armada/armada_drm.h
+++ b/drivers/gpu/drm/armada/armada_drm.h
@@ -55,6 +55,20 @@ void armada_drm_vbl_event_remove_unlocked(struct armada_crtc 
*,
__e-fn = _f;\
  } while (0)

+struct armada_clk_info {
+   struct clk *clk;
+
+   /* If this clock is dedicated to us, we can change its rate without
+* worrying about any other users in other parts of the system. */
+   bool is_dedicated;
+
+   /* However, we cannot share the same dedicated clock between two CRTCs
+* if each CRTC wants a different rate. Track the number of users. */
+   int usage_count;


You can share the same clock between two CRTCs. Just consider CRTC1
using a mode with half pixel clock as CRTC0. Not that I think this will
ever happen, but it is possible.


+   /* The bits in the SCLK register that select this clock */
+   uint32_t sclk;
+};

  struct armada_private;

@@ -77,7 +91,8 @@ struct armada_private {
struct drm_fb_helper*fbdev;
struct armada_crtc  *dcrtc[2];
struct drm_mm   linear;
-   struct clk  *extclk[2];
+   int num_clks;
+   struct armada_clk_info  *clk_info;
struct drm_property *csc_yuv_prop;
struct drm_property *csc_rgb_prop;
struct drm_property *colorkey_prop;
@@ -99,6 +114,9 @@ void __armada_drm_queue_unref_work(struct drm_device *,
  void armada_drm_queue_unref_work(struct drm_device *,
struct drm_framebuffer *);

+struct armada_clk_info *armada_drm_find_best_clock(struct armada_private *priv,
+   long rate, int *divider);
+
  extern const struct drm_mode_config_funcs armada_drm_mode_config_funcs;

  int armada_fbdev_init(struct drm_device *);
diff --git a/drivers/gpu/drm/armada/armada_drv.c 
b/drivers/gpu/drm/armada/armada_drv.c
index e0a08e9..411d56f 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -16,6 +16,97 @@
  #include armada_ioctl.h
  #include armada_ioctlP.h

+/* Find the best clock and integer divisor for a given rate.
+ * NULL is returned when no clock can be found.
+ * When the return value is non-NULL, the divider output variable is set
+ * appropriately, and the clock is returned in prepared state. */
+struct armada_clk_info *armada_drm_find_best_clock(struct armada_private *priv,
+   long rate, int *divider)


I prefer not to try to find the best clock (source) at all. Let the
user pass the clock name by e.g. platform_data (or DT) and just try to
get the requested pixclk or a integer multiple of it. You will never be
able to find the best clock for all use cases.

Just figure out, if integer division is sufficient to get requested
pixclk and clk_set_rate otherwise. This may be useful for current mode
running at 148.5MHz (1080p50) and new mode at 74.25MHz (1080i50, 720p).


+{
+  

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Sebastian Hesselbarth]

On 07/01/2013 11:55 PM, Rob Clark wrote:

On Mon, Jul 1, 2013 at 4:52 AM, Sebastian Hesselbarth
sebastian.hesselba...@gmail.com  wrote:

- TDA998x irq handling - ignored
- TDA998x sync fix - ignored


At least the sync fix, looks like I missed it (it probably is a good
idea to CC me if you want me to look at it).  Looks like there was
some follow-up discussion on both patches, unless I missed seeing a
newer version of those patches.


Yeah, I will update the patch for current mainline linux as it was
based on some of Russell's RFC patches.


Sometimes if you think a patch has been ignored/forgotten, it doesn't
hurt to ping on mailing list or #dri-devel..  a lot of us are working
not just on kernel (the relatively small part in the whole linux
graphics stack), but also mesa and/or x11.  Some times things end up
several pages down in the mail folder.  It's not because we are all
sitting on a beach drinking margaritas, or because we don't like you.
It is just because we are busy and missed it.

Last few months I've been pretty buried in r/e + gallium driver for
new gpu, so I wasn't always checking dri-devel list every day.  At
least now I am in drm-driver mode again ;-)


It is ok, I don't blame anyone here. Darren wants to test it on tilcdc.
I also found a datasheet for TDA9983b which is kind of compatible but
has more information about register layout in it. IIRC digikey also has
it.


- Fix drm I2C slave encoder probing


If you have a better idea about how to make the slave encoder probing
better (and/or more generic to support stuff other than i2c), please
send RFC patch.  (And if you already did this, please send updated
version, see previous point about sometimes missing patches.)


Also, will send an updated fix.

Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: armada_drm clock selection - try 2

[Sebastian Hesselbarth]

On 07/02/13 03:57, Daniel Drake wrote:

On Mon, Jul 1, 2013 at 3:48 PM, Sebastian Hesselbarth
sebastian.hesselba...@gmail.com wrote:

I prefer not to try to find the best clock (source) at all. Let the
user pass the clock name by e.g. platform_data (or DT) and just try to
get the requested pixclk or a integer multiple of it. You will never be
able to find the best clock for all use cases.

Just figure out, if integer division is sufficient to get requested
pixclk and clk_set_rate otherwise. This may be useful for current mode
running at 148.5MHz (1080p50) and new mode at 74.25MHz (1080i50, 720p).


I am not opposed to this approach, it is nice and simple, but as
Russell points out we do additionally need to distinguish between
clocks that are ours to play with, vs those that are shared with
other devices. It would be a bad idea to try call clk_set_rate() on
the AXI bus clock, for example, changing the clock for a whole bunch
of other devices. This is what the is_dedicated flag is about. However
such a flag could be easily added to the DT/platform data definition
that you propose.


Daniel,

now I got the idea of .is_dedicated. At least for Dove, we could still
implement AXI bus clock as fixed-rate clock, so you cannot mess with it.
I am almost sure, it will hang Dove when you change it, as there are
some devices depending on it (and the correct rate of it). Moreover,
LCDCLK is dedicated to CRTC0/1 so AXICLK is the only clock not
dedicated to LCD controllers.

But I am fine with .is_dedicated - I just think we should not try to
find the best clock source but leave that decision to the user
(=developer, DTS author; not userspace user).

Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/02/2013 09:19 PM, Russell King wrote:

On Tue, Jul 02, 2013 at 08:42:55PM +0200, Jean-Francois Moine wrote:

It seems that you did not look at the NVIDIA Tegra driver (I got its
general concept for my own driver, but I used a simple atomic counter):

- at probe time, the main driver (drivers/gpu/host1x/drm/drm.c) scans
   the DT and finds its external modules. These ones are put in a
   clients list.

- when loaded, the other modules register themselves into the main
   driver. This last one checks if each module is in the client list.
   If so, the module is moved from the client to an active list.

- when the client list is empty, this means all modules are started,
   and, so, the main driver starts the drm stuff.

The active list is kept for module unloading.


Please tell me how this works with the two LCD controllers if you wish
to drive the two LCD controllers as entirely separate devices.  Given
that the above requires the use of global data in the driver, how do
you distinguish between the two?


Putting phandles in the 'display' seems more flexible (I did not do so
because I knew the hardware - 2 LCDs and the dcon/ire).


Except you haven't looked at the bigger picture - the Armada 510 is
unusual in that it has two LCD controllers and the DCON.  All of the
other SoCs using this IP block that I've been able to research have
only one LCD controller and no DCON.  I don't think they even have an
IRE (image rotation engine) either.

Neither have you considered the case where you may wish to keep the
two LCD controllers entirely separate (eg, you want X to drive one
but something else on the other.)  X drives the DRM device as a whole,
including all CRTCs which make up that device - with them combined
into one DRM device, you can't ask X to leave one controller alone
because you're doing something else with it.  (This is just the simple
extension of the common case of a single LCD controller, so it's
really nothing special.)

So, the unusual case _is_ the Armada 510 with its two LCD controllers
and DCON which we _could_ work out some way of wrapping up into one
DRM device, or we could just ignore the special case, ignore the DCON
and just keep the two LCD CRTCs as two separate and independent DRM
devices.

I'm actually starting to come towards the conclusion that we should go
for the easiest solution, which is the one I just mentioned, and forget
trying to combine these devices into one super DRM driver.


I am against a super node which contains lcd and dcon/ire nodes. You can
enable those devices on a per board basis. We add them to dove.dtsi but
disable them by default (status = disabled).

The DRM driver itself should get a video-card node outside of 
soc/internal-regs where you can put e.g. video memory hole (or video

mem size if it will be taken from RAM later)

About the unusual case, I guess we should try to get both lcd
controllers into one DRM driver. Then support mirror or screen
extension X already provides. For those applications where you want
X on one lcd and some other totally different video stream - wait
for someone to come up with a request or proposal.

Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/02/2013 11:04 PM, Daniel Drake wrote:

On Tue, Jul 2, 2013 at 1:57 PM, Sebastian Hesselbarth
sebastian.hesselba...@gmail.com  wrote:

I am against a super node which contains lcd and dcon/ire nodes. You can
enable those devices on a per board basis. We add them to dove.dtsi but
disable them by default (status = disabled).

The DRM driver itself should get a video-card node outside of
soc/internal-regs where you can put e.g. video memory hole (or video
mem size if it will be taken from RAM later)


For completeness of the discussion, and my understanding too, can you
explain your objections to the display super-node in a bit more
detail?


lcd-controller nodes and dcon node will need to be children of
internal-regs nodes. The internal-regs node is required for address
translation as the mbus base address can be configured. The above does
not permit a super-node - but you cannot have the nodes above outside
of internal-regs.

As Russell stated, he wants a proposal for the unusual case i.e.
you have two lcd controllers. You use one for Xorg and the other for
e.g. running a linux terminal console.

This would require some reference from the super node to the lcd
controller to sort out which DRM device (represented by the super
node) should be using which lcd controller device.

Using status = disabled alone will only allow to enable or disable
lcd controller nodes but not assign any of it to your two super-nodes.

So my current proposal after thinking about Russell's statements
whould be phandles as Jean-Francois already mentioned. I am not sure
what OF maintainers will think about it, but that is another thing.

Basically, you will have:
(Note: names and property-names are just to show how it could work,
and example is joined from possible future dove.dtsi and dove-board.dts)

video {
/* Single video card w/ multiple lcd controllers */
card0 {
compatible = marvell,armada-510-display;
reg = 0 0x3f00 0x100; /* video-mem hole */
/* later: linux,video-memory-size = 0x100; */
marvell,video-devices = lcd0 lcd1 dcon;
};

/* OR: Multiple video card w/ single lcd controllers */
card0 {
compatible = marvell,armada-510-display;
...
marvell,video-devices = lcd0;
};

card1 {
compatible = marvell,armada-510-display;
...
marvell,video-devices = lcd1;
};
};

mbus {
compatible = marvell,dove-mbus;
ranges = ...;

sb-regs {
ranges = 0 0xf100 0 0x10;
...
}

nb-regs {
ranges = 0 0xf180 0 0x10;

lcd0: lcd-controller@2 {
compatible = marvell,armada-510-lcd;
reg = 0x2 0x1000;
interrupts = 47;
...
/* use EXTCLK0 with lcd0 */
clocks = ext_clk0;
clock-names = extclk0;
marvell,external-encoder = tda998x;
};

lcd1: lcd-controller@1 {
compatible = marvell,armada-510-lcd;
reg = 0x1 0x1000;
interrupts = 46;
...
/* use LCDPLL with lcd1 */
clocks = lcd_pll_clk;
clock-names = lcdpll;
};
}
};

i2c0 {
tda998x: hdmi-transmitter@60 {
compatible = nxp,tda19988;
reg = 0x60;
...
}
};

Each lcd controller node represents a platform_device and the display
nodes above should look up phandles and determine type (ctrc or dcon)
by compatible string of the nodes the phandles point to.

Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/03/13 08:55, Sascha Hauer wrote:

On Wed, Jul 03, 2013 at 08:02:05AM +1000, Dave Airlie wrote:

Have you also considered how suspend/resume works in such a place,
where every driver is independent? The ChromeOS guys have bitched
before about the exynos driver which is has lots of sub-drivers, how
do you control the s/r ordering in a crazy system like that? I'd
prefer a central driver, otherwise there is too many moving parts.


Composing a DRM device out of subdevices doesn't necessarily mean the
components should be suspended/resumed in arbitrary order. The DRM
device should always be suspended first (thus deactivating sub devices
as necessary and as done already) and resumed last.

Note that a super node approach does not solve this magically. We would
still have to make sure that the i2c bus masters on our SoC are suspended
after the DRM device.


+1 for a video card supernode that at best should be some very generic
node with standard properties provided by DRM backend. IIRC there was
a proposal for of_video_card a while ago.

At least for Marvell SoCs, moving device nodes out of the bus structure
will not work. The parent bus is _required_ for address mapping as the
base address is configurable. Using phandles can solve this without
moving nodes.

Also, having separate device nodes does not require a separate driver
for each nodes. All nodes get platform_devices registered, but you can
choose not to have a matching driver for it. Then the video card super
node can pick up that nodes by using the phandles passed and register
a single DRM driver claiming the devices.

Moreover, if we talk about SoC graphics, we have to take audio into
account. If you move all nodes to your video card super node, you will
add another bunch of issues for ASoC linking to e.g. the I2C HDMI
transmitter SPDIF codec.

IMHO phandles and super node subnodes are equivalent from a driver
point-of-view but phandles are more likely to cause less pain for
other subsystems.

The super node approach will also allow to have the same SoC/board
components being used as single video card or multiple video cards
environment. There is virtually no way to automatically determine what
devices belong to your video card(s) in a SoC, so we need something to
describe those cards.

One thing I am concerned about is what Sascha pointed out above. If you
hook up an external I2C encoder to your card, you cannot make sure I2C
bus is suspended before DRM device. To be honest, proposing a solution
for that is still way beyond my expertise wrt to Linux internals, so I
am not even trying it. Maybe I am even missing a very important point
for the super node/phandle proposal, if so, please clarify.

Sebastian
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Best practice device tree design for display subsystems/DRM

[Sebastian Hesselbarth]

On 07/02/2013 09:19 PM, Russell King wrote:
> On Tue, Jul 02, 2013 at 08:42:55PM +0200, Jean-Francois Moine wrote:
>> It seems that you did not look at the NVIDIA Tegra driver (I got its
>> general concept for my own driver, but I used a simple atomic counter):
>>
>> - at probe time, the main driver (drivers/gpu/host1x/drm/drm.c) scans
>>the DT and finds its external modules. These ones are put in a
>>"clients" list.
>>
>> - when loaded, the other modules register themselves into the main
>>driver. This last one checks if each module is in the "client" list.
>>If so, the module is moved from the "client" to an "active" list".
>>
>> - when the "client" list is empty, this means all modules are started,
>>and, so, the main driver starts the drm stuff.
>>
>> The active list is kept for module unloading.
>
> Please tell me how this works with the two LCD controllers if you wish
> to drive the two LCD controllers as entirely separate devices.  Given
> that the above requires the use of global data in the driver, how do
> you distinguish between the two?
>
>> Putting "phandle"s in the 'display' seems more flexible (I did not do so
>> because I knew the hardware - 2 LCDs and the dcon/ire).
>
> Except you haven't looked at the bigger picture - the Armada 510 is
> unusual in that it has two LCD controllers and the DCON.  All of the
> other SoCs using this IP block that I've been able to research have
> only one LCD controller and no DCON.  I don't think they even have an
> IRE (image rotation engine) either.
>
> Neither have you considered the case where you may wish to keep the
> two LCD controllers entirely separate (eg, you want X to drive one
> but something else on the other.)  X drives the DRM device as a whole,
> including all CRTCs which make up that device - with them combined
> into one DRM device, you can't ask X to leave one controller alone
> because you're doing something else with it.  (This is just the simple
> extension of the common case of a single LCD controller, so it's
> really nothing special.)
>
> So, the unusual case _is_ the Armada 510 with its two LCD controllers
> and DCON which we _could_ work out some way of wrapping up into one
> DRM device, or we could just ignore the special case, ignore the DCON
> and just keep the two LCD CRTCs as two separate and independent DRM
> devices.
>
> I'm actually starting to come towards the conclusion that we should go
> for the easiest solution, which is the one I just mentioned, and forget
> trying to combine these devices into one super DRM driver.

I am against a super node which contains lcd and dcon/ire nodes. You can
enable those devices on a per board basis. We add them to dove.dtsi but
disable them by default (status = "disabled").

The DRM driver itself should get a video-card node outside of 
soc/internal-regs where you can put e.g. video memory hole (or video
mem size if it will be taken from RAM later)

About the unusual case, I guess we should try to get both lcd
controllers into one DRM driver. Then support mirror or screen
extension X already provides. For those applications where you want
X on one lcd and some other totally different video stream - wait
for someone to come up with a request or proposal.

Sebastian

armada_drm clock selection - try 2

[Sebastian Hesselbarth]

On 07/02/13 03:57, Daniel Drake wrote:
> On Mon, Jul 1, 2013 at 3:48 PM, Sebastian Hesselbarth
>  wrote:
>> I prefer not to try to find the best clock (source) at all. Let the
>> user pass the clock name by e.g. platform_data (or DT) and just try to
>> get the requested pixclk or a integer multiple of it. You will never be
>> able to find the best clock for all use cases.
>>
>> Just figure out, if integer division is sufficient to get requested
>> pixclk and clk_set_rate otherwise. This may be useful for current mode
>> running at 148.5MHz (1080p50) and new mode at 74.25MHz (1080i50, 720p).
>
> I am not opposed to this approach, it is nice and simple, but as
> Russell points out we do additionally need to distinguish between
> clocks that are "ours" to play with, vs those that are shared with
> other devices. It would be a bad idea to try call clk_set_rate() on
> the AXI bus clock, for example, changing the clock for a whole bunch
> of other devices. This is what the is_dedicated flag is about. However
> such a flag could be easily added to the DT/platform data definition
> that you propose.

Daniel,

now I got the idea of .is_dedicated. At least for Dove, we could still
implement AXI bus clock as fixed-rate clock, so you cannot mess with it.
I am almost sure, it will hang Dove when you change it, as there are
some devices depending on it (and the correct rate of it). Moreover,
LCDCLK is dedicated to CRTC0/1 so AXICLK is the only clock not
dedicated to LCD controllers.

But I am fine with .is_dedicated - I just think we should not try to
find the best clock source but leave that decision to the user
(=developer, DTS author; not userspace user).

Sebastian

[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Sebastian Hesselbarth]

On 07/01/2013 11:55 PM, Rob Clark wrote:
> On Mon, Jul 1, 2013 at 4:52 AM, Sebastian Hesselbarth
>   wrote:
>> - TDA998x irq handling - ignored
>> - TDA998x sync fix - ignored
>
> At least the sync fix, looks like I missed it (it probably is a good
> idea to CC me if you want me to look at it).  Looks like there was
> some follow-up discussion on both patches, unless I missed seeing a
> newer version of those patches.

Yeah, I will update the patch for current mainline linux as it was
based on some of Russell's RFC patches.

> Sometimes if you think a patch has been ignored/forgotten, it doesn't
> hurt to ping on mailing list or #dri-devel..  a lot of us are working
> not just on kernel (the relatively small part in the whole linux
> graphics stack), but also mesa and/or x11.  Some times things end up
> several pages down in the mail folder.  It's not because we are all
> sitting on a beach drinking margaritas, or because we don't like you.
> It is just because we are busy and missed it.
>
> Last few months I've been pretty buried in r/e + gallium driver for
> new gpu, so I wasn't always checking dri-devel list every day.  At
> least now I am in drm-driver mode again ;-)

It is ok, I don't blame anyone here. Darren wants to test it on tilcdc.
I also found a datasheet for TDA9983b which is kind of compatible but
has more information about register layout in it. IIRC digikey also has
it.

>> - Fix drm I2C slave encoder probing
>>
> If you have a better idea about how to make the slave encoder probing
> better (and/or more generic to support stuff other than i2c), please
> send RFC patch.  (And if you already did this, please send updated
> version, see previous point about sometimes missing patches.)

Also, will send an updated fix.

Sebastian

armada_drm clock selection - try 2

[Sebastian Hesselbarth]

On 07/01/2013 10:30 PM, Daniel Drake wrote:
> Here is a new patch which should incorporate all your previous feedback.
> Now each variant passes clock info to the main driver via a new
> armada_clk_info structure.
>
> A helper function in the core lets each variant find the best clock.
> As you suggested we first try external ("dedicated") clocks, where we can
> change the rate to find an exact match. We fall back on looking at the rates
> of the other clocks and we return the clock that provides us with the closest
> match after integer division (rejecting those that don't bring us within 1%).
>
> There is still the possibility that two CRTCs on the same device end up using
> the same clock, so I added a usage counter to each clock to prevent the rate
> from being changed by another CRTC.
>
> This is compile-tested only - but after your feedback I will add the
> remaining required hacks and test it on XO.
>
> diff --git a/drivers/gpu/drm/armada/armada_510.c 
> b/drivers/gpu/drm/armada/armada_510.c
> index a016888..7dff2dc 100644
> --- a/drivers/gpu/drm/armada/armada_510.c
> +++ b/drivers/gpu/drm/armada/armada_510.c
> @@ -17,12 +17,29 @@
>
>   static int armada510_init(struct armada_private *priv, struct device *dev)
>   {
> - priv->extclk[0] = devm_clk_get(dev, "ext_ref_clk_1");
> + struct armada_clk_info *clk_info = devm_kzalloc(dev,
> + sizeof(struct armada_clk_info) * 4, GFP_KERNEL);
>
> - if (IS_ERR(priv->extclk[0])&&  PTR_ERR(priv->extclk[0]) == -ENOENT)
> - priv->extclk[0] = ERR_PTR(-EPROBE_DEFER);
> + if (!clk_info)
> + return -ENOMEM;
>
> - return PTR_RET(priv->extclk[0]);
> + /* External clocks */
> + clk_info[0].clk = devm_clk_get(dev, "ext_ref_clk_0");
> + clk_info[0].is_dedicated = true;

Daniel,

I guess "extclk0" and "extclk1" should be sufficient for clock names.
Also, they are not dedicated as you can have CRTC0 and CRTC1 use e.g.
extclk0 simultaneously. See below for .is_dedicated in general.

[...]
> diff --git a/drivers/gpu/drm/armada/armada_drm.h 
> b/drivers/gpu/drm/armada/armada_drm.h
> index e8c4f80..4fe8ec5 100644
> --- a/drivers/gpu/drm/armada/armada_drm.h
> +++ b/drivers/gpu/drm/armada/armada_drm.h
> @@ -55,6 +55,20 @@ void armada_drm_vbl_event_remove_unlocked(struct 
> armada_crtc *,
>   __e->fn = _f;   \
>   } while (0)
>
> +struct armada_clk_info {
> + struct clk *clk;
> +
> + /* If this clock is dedicated to us, we can change its rate without
> +  * worrying about any other users in other parts of the system. */
> + bool is_dedicated;
> +
> + /* However, we cannot share the same dedicated clock between two CRTCs
> +  * if each CRTC wants a different rate. Track the number of users. */
> + int usage_count;

You can share the same clock between two CRTCs. Just consider CRTC1
using a mode with half pixel clock as CRTC0. Not that I think this will
ever happen, but it is possible.

> + /* The bits in the SCLK register that select this clock */
> + uint32_t sclk;
> +};
>
>   struct armada_private;
>
> @@ -77,7 +91,8 @@ struct armada_private {
>   struct drm_fb_helper*fbdev;
>   struct armada_crtc  *dcrtc[2];
>   struct drm_mm   linear;
> - struct clk  *extclk[2];
> + int num_clks;
> + struct armada_clk_info  *clk_info;
>   struct drm_property *csc_yuv_prop;
>   struct drm_property *csc_rgb_prop;
>   struct drm_property *colorkey_prop;
> @@ -99,6 +114,9 @@ void __armada_drm_queue_unref_work(struct drm_device *,
>   void armada_drm_queue_unref_work(struct drm_device *,
>   struct drm_framebuffer *);
>
> +struct armada_clk_info *armada_drm_find_best_clock(struct armada_private 
> *priv,
> + long rate, int *divider);
> +
>   extern const struct drm_mode_config_funcs armada_drm_mode_config_funcs;
>
>   int armada_fbdev_init(struct drm_device *);
> diff --git a/drivers/gpu/drm/armada/armada_drv.c 
> b/drivers/gpu/drm/armada/armada_drv.c
> index e0a08e9..411d56f 100644
> --- a/drivers/gpu/drm/armada/armada_drv.c
> +++ b/drivers/gpu/drm/armada/armada_drv.c
> @@ -16,6 +16,97 @@
>   #include "armada_ioctl.h"
>   #include "armada_ioctlP.h"
>
> +/* Find the best clock and integer divisor for a given rate.
> + * NULL is returned when no clock can be found.
> + * When the return value is non-NULL, the divider output variable is set
> + * appropriately, and the clock is returned in prepared state. */
> +struct armada_clk_info *armada_drm_find_best_clock(struct armada_private 
> *priv,
> + long rate, int *divider)

I prefer not to try to find the best clock (source) at all. Let the
user pass the clock name by e.g. platform_data (or DT) and just try to
get the requested pixclk or a integer multiple of it. You will never be
able to find the best clock for all use cases.

Just figure out, if integer division is sufficient to get requested
pixclk and