Am Montag, den 23.03.2015, 19:17 +0100 schrieb Heiko Stuebner:

Hi Philipp,

Am Donnerstag, 12. März 2015, 21:45:19 schrieb Heiko Stuebner:

...

At least the Rockchip variant of the dw_hdmi can have controllable power supplies providing 1.0 and 1.8V. Therefore add the possibility for the generic bridge driver to enable supplies provided by the hw-specific drivers.

Signed-off-by: Heiko Stuebner heiko@sntech.de

does this look ok now?

Yes, thank you. Acked-by: Philipp Zabel p.zabel@pengutronix.de

And as we talked about in Chemnitz, who will be taking such bridge-related changes, as you mentioned some last bridge-patches going through Thierry.

Indeed, recent drm/bridge fixes were merged through the drm/panel tree. Thierry, will you continue to do that or should I collect the dw_hdmi patches?

regards Philipp

Hi Thierry

Am Freitag, 5. Juni 2015, 14:23:14 schrieb Thierry Reding:

On Fri, Jun 05, 2015 at 02:16:40PM +0200, Heiko Stübner wrote:

...

...

If this is specific to the Rockchip implementation, shouldn't this go into Documentation/devicetree/bindings/video/dw_hdmi-rockchip.txt? It could then simply go into the Rockchip DRM tree.

actually, we determined that the supply names are universal to the IP (both in imx and rockchip and probably more if there are more users out there). Just Russell requested that we don't pollute the generic code until necessary, as it looks like the supply of those is somehow handled internally on the imx.

If it's universal then there should be no need to mention the Rockchip compatible specifically. Also, it might be better to submit this as two separate patches, one for the binding and another for the driver.

I could extract the DT binding piece myself and apply only that, then somebody else can apply the Rockchip change to that driver separately.

so I've sent a v4 where the dt-binding is split off (and without the Rockchip specific text), so if you want to take patch1, Mark could take patch2 and I would take the dts changes in patch3.

Heiko

On Sat, Jun 06, 2015 at 12:10:58AM +0100, Russell King - ARM Linux wrote:

On Fri, Jun 05, 2015 at 02:16:40PM +0200, Heiko Stübner wrote:

...

Hi Thierry

Am Freitag, 5. Juni 2015, 13:02:01 schrieb Thierry Reding:

...

If this is specific to the Rockchip implementation, shouldn't this go into Documentation/devicetree/bindings/video/dw_hdmi-rockchip.txt? It could then simply go into the Rockchip DRM tree.

actually, we determined that the supply names are universal to the IP (both in imx and rockchip and probably more if there are more users out there). Just Russell requested that we don't pollute the generic code until necessary, as it looks like the supply of those is somehow handled internally on the imx.

Why do you think it's universal?

Let's start from the beginning, before we create something that's not representative of the hardware.

dw_hdmi actually drives two pieces of hardware - the HDMI transmitter, and a separate hardware block, the HDMI phy.

There are at least two possible HDMI phys referenced in the iMX documentation - there is one called HEAC which doesn't appear in iMX devices afaik, and the one which does, which is a 3D phy.

I'm not sure I understand correctly. Are these PHYs exchangeable? Does the DesignWare IP provide them or could they be provided separately? I see register definitions for a "source" PHY and a "master" PHY, where the latter seems to be an I2C controller rather than a PHY. Perhaps it is used to communicate with an external PHY?

Can somebody give me a quick summary (or point me to documentation) on how this works?

The 3D phy top level IO diagram does have VPH and VP power supplies, but it also has VP_FILT0, VP_FILT1, VP_FILT2, VP_FILT3 and GD labelled up as "Power supply signals". Where they connect to in the rest of the system - or whether they are connected - is undocumented.

The HDMI transmitter itself is not documented what it's supplies actually are.

So, what we currently have in DT for dw_hdmi is something which doesn't _quite_ reflect the hardware right now, and to go adding VP and VPH supplies to the generic binding is wrong - it doesn't follow the hardware structure detailed in the iMX documentation I have.

As the Rockchip documentation is not available, I can't comment whether it would be current proposal is appropriate for Rockchip or not, which is why I haven't commented on this other than saying that it's not appropriate to be a generic dw_hdmi binding.

If we wanted to model this correctly, then for iMX, I would suggest that the HDMI phy should be modelled in DT, and _all_ the six VP* supplies are modelled - and we should assume that "GD" is a "power good" signal, though we don't know that for certain.

Perhaps VP_FILT* are sourced from the same supply as VP, so maybe we don't have to care at the DT level?

What we also don't know on iMX6 is what voltages any of these are supplied with.

A rather common nuisance with these licensed IP blocks is that some of the details may be hidden in SoC glue, resulting in the interface being different on each SoC. Given all of the above it sounds like i.MX could have some internal glue to supply VP and VPH and there'd be no sensible way to represent them in DT.

So, as we don't have much certainty here, and we know that adding it to what is the HDMI transmitter would be wrong, I'd suggest not modelling it in a generic way at present.

Does DesignWare by any chance offer documentation about the IP? If not I agree that the safest thing to do for now would be to make this Rockchip specific. If it should prove to be generic after all (in which case i.MX would seem to provide SoC glue to supply them) we can still promote the supplies to be generic. They'd need to be optional to handle the i.MX, though. Or perhaps a dummy regulator could be referenced, representing the SoC-internal supply to the block. Maybe there even is a (SoC- specific) way to control this internal regulator?

Thierry

On Mon, Jun 08, 2015 at 03:29:26PM +0100, Russell King - ARM Linux wrote:

On Mon, Jun 08, 2015 at 04:02:58PM +0200, Thierry Reding wrote:

...

On Sat, Jun 06, 2015 at 12:10:58AM +0100, Russell King - ARM Linux wrote:

...

On Fri, Jun 05, 2015 at 02:16:40PM +0200, Heiko Stübner wrote:

...

Hi Thierry

Am Freitag, 5. Juni 2015, 13:02:01 schrieb Thierry Reding:

...

If this is specific to the Rockchip implementation, shouldn't this go into Documentation/devicetree/bindings/video/dw_hdmi-rockchip.txt? It could then simply go into the Rockchip DRM tree.

actually, we determined that the supply names are universal to the IP (both in imx and rockchip and probably more if there are more users out there). Just Russell requested that we don't pollute the generic code until necessary, as it looks like the supply of those is somehow handled internally on the imx.

Why do you think it's universal?

Let's start from the beginning, before we create something that's not representative of the hardware.

dw_hdmi actually drives two pieces of hardware - the HDMI transmitter, and a separate hardware block, the HDMI phy.

There are at least two possible HDMI phys referenced in the iMX documentation - there is one called HEAC which doesn't appear in iMX devices afaik, and the one which does, which is a 3D phy.

I'm not sure I understand correctly. Are these PHYs exchangeable? Does the DesignWare IP provide them or could they be provided separately?

You're asking questions we have no real answers to - all we have is the available documentation provided by Freescale - we don't even have the Rochchip documentation.

From the Freescale documentation, which documents the HDMI transmitter entirely separately from the HDMI phy, I would say that the IP is structured as two entirely separate blocks, and the chip designer is free to choose an appropriate phy from a range of HDMI phys.

Does it associate any of the registers with the HDMI PHY? According to the register definitions in drivers/gpu/drm/bridge/dw_hdmi.h the PHY registers are interleaved with other registers that are likely part of the HDMI transmitter proper (there are things like HDCP and CEC engines in that IP as well).

Moreover, in the feature list for the HDMI transmitter, it says (though this is not applicable to iMX6 - and is probably lifted from the Synopsis documentation):

"* Option to remove pixel repetition clock from HDMI TX interface for an easy integration with third party HDMI TX PHYs"

So - here's the question: what do we do when we find something using the Synopsis design-ware HDMI transmitter with a different HDMI phy?

Remember, VP, VPH, VP_FILT_* are all part of the HDMI phy according to the freescale documentation, and not part of the HDMI transmitter.

...

I see register definitions for a "source" PHY and a "master" PHY, where the latter seems to be an I2C controller rather than a PHY. Perhaps it is used to communicate with an external PHY?

I'm not sure where you get that from. Maybe you could give a chapter reference?

That's just from skimming drivers/gpu/drm/bridge/dw_hdmi.h (register offsets 0x3000 and the following). The large gap between them and the preceding FC registers could be an additional indication that they are indeed separate IP blocks. Then again, the AUD registers start at an offset of 0x3100, so maybe it's no indication at all.

Are these the registers used to control the PHY that you're referring to? It seems odd to me that the documentation would specify these registers interleaved with other registers if the PHYs can be freely chosen from a set of HDMI PHYs. Perhaps the set of PHYs that can be used need to expose the same integration interface? Maybe these registers aren't used to control the PHY at all but rather configure the interface between transmitter and PHY?

Sorry if these are more questions that the documentation doesn't answer. Just trying to understand what's going on.

Looking deeper at this HEAC issue, that is a separate, smaller phy which doesn't have much to do with video - though it remains rather undocumented in the freescale docs. That's only going on a block diagram near the start of the HDMI transmitter chapter though.

HEAC could be "HDMI Ethernet Audio Control", see here:

http://en.wikipedia.org/wiki/HDMI#ARC http://en.wikipedia.org/wiki/HDMI#HEC

...

...

If we wanted to model this correctly, then for iMX, I would suggest that the HDMI phy should be modelled in DT, and _all_ the six VP* supplies are modelled - and we should assume that "GD" is a "power good" signal, though we don't know that for certain.

Perhaps VP_FILT* are sourced from the same supply as VP, so maybe we don't have to care at the DT level?

We'd be guessing - and that's exactly my point. Designing something at DT level based on guesswork is no way to go, especially with a generic bit of IP which would be re-used across multiple different platforms.

Agreed.

...

...

What we also don't know on iMX6 is what voltages any of these are supplied with.

A rather common nuisance with these licensed IP blocks is that some of the details may be hidden in SoC glue, resulting in the interface being different on each SoC. Given all of the above it sounds like i.MX could have some internal glue to supply VP and VPH and there'd be no sensible way to represent them in DT.

... or it's entirely possible that all these supplies are always fed by the iMX6 SoC all the time.

Yes, that's what I was implying.

...

...

So, as we don't have much certainty here, and we know that adding it to what is the HDMI transmitter would be wrong, I'd suggest not modelling it in a generic way at present.

Does DesignWare by any chance offer documentation about the IP? If not I agree that the safest thing to do for now would be to make this Rockchip specific.

That's _exactly_ why I raised the point originally, and said that it should be Rockchip specific until we know better. Glad you've caught up with my thinking. :)

However, my thinking _now_ is that it shouldn't even be modelled as part of our HDMI transmitter DT blob, but we should be modelling the HDMI transmitter separately from the HDMI phy as two separate DT blobs, and then the HDMI phy blob gets the (possibly optional) VPH and VP supplies.

Yes, I agree. One problem that you could run into with such a split is that the PHY control registers may be interleaved with the registers of the HDMI transmitter. But perhaps my understanding is wrong and the PHY is truly controlled via a completely own set of registers. Looking at the implementation, the HDMI_PHY_* registers seem to be used to control various aspects of an interface between transmitter and PHY rather than the PHY itself and it seems like the PHY itself is really an I2C slave accessed using the I2C master controlled using the HDMI_PHY_I2CM_* registers.

So perhaps the HDMI transmitter DT node really should expose the PHY as an I2C child. In fact, looking at HDMI_PHY_I2CM_SLAVE_ADDR_* field values (lines 870 and 871 in dw_hdmi.h) it should have two children, one for the "GEN2" PHY and one for the "HEAC" PHY. You could even go as far as exposing the HDMI_PHY_I2CM_* registers as I2C master and implement the PHYs as proper I2C client drivers, but that might be overkill, especially considering that you'd need to have extra API to convey the HDMI configuration. Still, might be worth considering to make the split really explicit.

I guess I've answered most of my earlier questions myself...

What if someone designs a chip with their own HDMI phy which uses different supplies?

That's really one more reason to write proper drivers for the PHYs. If you really ever have to deal with a different PHY chances are that more than just the supplies will be different, in which case you'd need to somehow also abstract away the interoperation between HDMI transmitter and PHY.

This is one of the problems I have with DT - DT needs us to describe the hardware correctly first time around, when we may not have all the available facts to make proper decisions, and wrong decisions can very well lead to serious headaches later on because we didn't model something correctly - and we're stuck with the wrong decisions for ever.

I feel the same way. Although there have been occasional exceptions to the DT ABI stability rule if it could be proven that an existing binding was completely wrong, I've made the experience that putting as little as possible into DT (and at the same time resist the temptation of a generic solution) helps keep the headaches in check.

Fortunately the dw-hdmi driver gets much of this right already (it's helper code rather than a proper driver), so keeping things like power supplies specific to a particular integration should be easy to do and prevent potential future incompatibilities.

Thierry

On Mon, Jun 08, 2015 at 05:34:21PM +0100, Russell King - ARM Linux wrote:

On Mon, Jun 08, 2015 at 05:44:53PM +0200, Thierry Reding wrote:

...

On Mon, Jun 08, 2015 at 03:29:26PM +0100, Russell King - ARM Linux wrote:

...

You're asking questions we have no real answers to - all we have is the available documentation provided by Freescale - we don't even have the Rochchip documentation.

From the Freescale documentation, which documents the HDMI transmitter entirely separately from the HDMI phy, I would say that the IP is structured as two entirely separate blocks, and the chip designer is free to choose an appropriate phy from a range of HDMI phys.

Does it associate any of the registers with the HDMI PHY? According to the register definitions in drivers/gpu/drm/bridge/dw_hdmi.h the PHY registers are interleaved with other registers that are likely part of the HDMI transmitter proper (there are things like HDCP and CEC engines in that IP as well).

The protocol stuff is handled by the HDMI Tx - the phy is the conversion from the internal digital to the interface domain.

I could write lots to describe it, but really, the best thing to do is to download the iMX6 documentation and take a peek at it...

https://community.freescale.com/docs/DOC-101840

Particularly, look at the HDMI 3D Phy section, the system level block diagram (34.1.5.1), and 34.2.1 which has the top level I/O diagram for just the 3D phy.

The previous section (section 33) covers the HDMI Tx.

Okay, that matches the picture that's been forming in my mind after reading the code.

...

...

Moreover, in the feature list for the HDMI transmitter, it says (though this is not applicable to iMX6 - and is probably lifted from the Synopsis documentation):

"* Option to remove pixel repetition clock from HDMI TX interface for an easy integration with third party HDMI TX PHYs"

So - here's the question: what do we do when we find something using the Synopsis design-ware HDMI transmitter with a different HDMI phy?

Remember, VP, VPH, VP_FILT_* are all part of the HDMI phy according to the freescale documentation, and not part of the HDMI transmitter.

...

I see register definitions for a "source" PHY and a "master" PHY, where the latter seems to be an I2C controller rather than a PHY. Perhaps it is used to communicate with an external PHY?

I'm not sure where you get that from. Maybe you could give a chapter reference?

That's just from skimming drivers/gpu/drm/bridge/dw_hdmi.h (register offsets 0x3000 and the following). The large gap between them and the preceding FC registers could be an additional indication that they are indeed separate IP blocks. Then again, the AUD registers start at an offset of 0x3100, so maybe it's no indication at all.

Ah, you're getting slightly confused.

All those registers are to do with the HDMI Tx itself. The block at 0x3000 to 0x3007 is to do with the HDMI Tx receiving various status signals from the HDMI Phy, such as HPD, rxsense, and whether the phy PLL is locked (which I'm fairly certain is mis-described, as there is no output signal from the phy to indicate this, but there is a TX_READY signal.)

The set at 0x3020-0x3032 are to do with talking to the HDMI phy over the dedicated I2C interface integrated into the HDMI Tx. All the HDMI Phy registers are accessed through this I2C interface.

Good, that also matches what I understood from reading the driver code.

...

...

What if someone designs a chip with their own HDMI phy which uses different supplies?

That's really one more reason to write proper drivers for the PHYs. If you really ever have to deal with a different PHY chances are that more than just the supplies will be different, in which case you'd need to somehow also abstract away the interoperation between HDMI transmitter and PHY.

Exactly, but that didn't happen before this code moved out of drivers/staging, which is rather unfortunate, but everyone was too busy to put much thought into that.

This is one of the problems when drivers are taken from BSPs rather than going away and writing something from documentation: if you start afresh, you can model the code according to the documentation without any of the original BSP decisions in place. If you start from the BSP code, you have a whole pile of changes you need to identify, and it's very easy to overlook something, because what you have works.

It's something which _should_ be done nevertheless. The problem is finding someone with the time and motivation to do a good job.

Interestingly, though, it seems like the PHYs for both i.MX and Rockchip are register-compatible because they work with the same driver code. It could be coincidence, or simply that they both use some standard PHY as provided by DW along with the HDMI transmitter IP.

Most importantly, like you said, any "damage" has already been done and in order to keep devices working with existing DTBs we'll have to add fallback code for backwards-compatibility. I'm thinking instantiating I2C clients for drivers matching the i.MX/Rockchip code directly from the HDMI bridge driver if no child nodes are found in DTB.

As for finding somebody with the motivation and time to do it, I suspect we could just wait for the next user of DW HDMI IP and let them deal with it. Perhaps documenting our findings in the driver would be helpful if that time comes.

For the matter at hand, I think we all agree that keeping the supplies Rockchip-specific for now is the right thing to do.

Thierry

On Wed, Jun 10, 2015 at 01:29:43AM +0200, Heiko Stübner wrote:

Allthough from the discussion and explanations I've now also got the "don't make it worse" feeling, so am somehow reluctant to add supplies that also might describe this inadequately.

The Rockchip-specific documentation only lists the two supplies as AVDD_1V0, AVDD_1V8 with a description of "DC supply voltage for Analog part of HDMI". The supplies aren't mentioned otherwise at all, so I guess I'll ponder this a bit more at first.

Are they controllable from software at all? If they aren't, then I'd suggest leaving them out until we have a distinction between the Tx and Phy - much as we do for iMX6.