[PATCH v3 1/2] drm/bridge: dw-hdmi: support optional supply regulators

[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 <***@sntech.de>
---
changes since v2:
- rename supplies to the names found in the hdmi IP databook
changes since v1:
- follow suggestion from Russell King to keep regulator handling local
to the rockchip implementation for the time being and only generalize
when a real second implementation needs regulator handling

.../devicetree/bindings/drm/bridge/dw_hdmi.txt | 5 ++++
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 32 +++++++++++++++++++++-
2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
index a905c14..bb74640 100644
--- a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
+++ b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
@@ -22,6 +22,11 @@ Optional properties
- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
- clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec"

+Optional supplies:
+rockchip,rk3288-dw-hdmi handles two optional power supplies:
+- vp-supply: 1.0V power supply
+- vph-supply: 1.8V power supply
+
Example:
hdmi: ***@0120000 {
compatible = "fsl,imx6q-hdmi";
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index d236faa..647a240 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -11,6 +11,7 @@
#include <linux/platform_device.h>
#include <linux/mfd/syscon.h>
#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
#include <drm/drm_of.h>
#include <drm/drmP.h>
#include <drm/drm_crtc_helper.h>
@@ -28,6 +29,9 @@ struct rockchip_hdmi {
struct device *dev;
struct regmap *regmap;
struct drm_encoder encoder;
+ struct regulator_bulk_data supplies[2];
+ int nsupplies;
+ bool supplies_enabled;
};

#define to_rockchip_hdmi(x) container_of(x, struct rockchip_hdmi, x)
@@ -179,6 +183,12 @@ static struct drm_encoder_funcs dw_hdmi_rockchip_encoder_funcs = {

static void dw_hdmi_rockchip_encoder_disable(struct drm_encoder *encoder)
{
+ struct rockchip_hdmi *hdmi = to_rockchip_hdmi(encoder);
+
+ if (hdmi->nsupplies > 0 && hdmi->supplies_enabled) {
+ regulator_bulk_disable(hdmi->nsupplies, hdmi->supplies);
+ hdmi->supplies_enabled = false;
+ }
}

static bool
@@ -199,7 +209,16 @@ static void dw_hdmi_rockchip_encoder_commit(struct drm_encoder *encoder)
{
struct rockchip_hdmi *hdmi = to_rockchip_hdmi(encoder);
u32 val;
- int mux;
+ int mux, ret;
+
+ if (hdmi->nsupplies > 0 && !hdmi->supplies_enabled) {
+ ret = regulator_bulk_enable(hdmi->nsupplies, hdmi->supplies);
+ if (ret) {
+ dev_err(hdmi->dev, "could not enable hdmi analog supplies\n");
+ return;
+ }
+ hdmi->supplies_enabled = true;
+ }

mux = rockchip_drm_encoder_get_mux_id(hdmi->dev->of_node, encoder);
if (mux)
@@ -275,6 +294,17 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
if (!iores)
return -ENXIO;

+ hdmi->supplies[0].supply = "vp";
+ hdmi->supplies[1].supply = "vph";
+ hdmi->nsupplies = 2;
+
+ ret = devm_regulator_bulk_get(hdmi->dev,
+ hdmi->nsupplies, hdmi->supplies);
+ if (ret == -EPROBE_DEFER)
+ return ret;
+ if (ret)
+ hdmi->nsupplies = 0;
+
platform_set_drvdata(pdev, hdmi);

encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);

[Heiko Stuebner]

Add the recently added hdmi power supplies to evb and firefly boards.

Signed-off-by: Heiko Stuebner <***@sntech.de>
---
arch/arm/boot/dts/rk3288-evb.dtsi | 2 ++
arch/arm/boot/dts/rk3288-firefly.dtsi | 2 ++
2 files changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi b/arch/arm/boot/dts/rk3288-evb.dtsi
index 5e895a5..edd45a0 100644
--- a/arch/arm/boot/dts/rk3288-evb.dtsi
+++ b/arch/arm/boot/dts/rk3288-evb.dtsi
@@ -118,6 +118,8 @@
};

&hdmi {
+ vp-supply = <&vdd10_lcd>;
+ vph-supply = <&vcc18_lcd>;
ddc-i2c-bus = <&i2c5>;
status = "okay";
};
diff --git a/arch/arm/boot/dts/rk3288-firefly.dtsi b/arch/arm/boot/dts/rk3288-firefly.dtsi
index e6f873a..09fcded 100644
--- a/arch/arm/boot/dts/rk3288-firefly.dtsi
+++ b/arch/arm/boot/dts/rk3288-firefly.dtsi
@@ -180,6 +180,8 @@
};

&hdmi {
+ vp-supply = <&vdd10_lcd>;
+ vph-supply = <&vcc18_lcd>;
ddc-i2c-bus = <&i2c5>;
status = "okay";
};

[Heiko Stuebner]

Hi Philipp,

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

does this look ok now?

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.


Heiko

Post by Heiko Stuebner
---
- rename supplies to the names found in the hdmi IP databook
- follow suggestion from Russell King to keep regulator handling local
to the rockchip implementation for the time being and only generalize
when a real second implementation needs regulator handling
.../devicetree/bindings/drm/bridge/dw_hdmi.txt | 5 ++++
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 32
+++++++++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt index
a905c14..bb74640 100644
--- a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
+++ b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
@@ -22,6 +22,11 @@ Optional properties
- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
- clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec"
+- vp-supply: 1.0V power supply
+- vph-supply: 1.8V power supply
+
compatible = "fsl,imx6q-hdmi";
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c index d236faa..647a240 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -11,6 +11,7 @@
#include <linux/platform_device.h>
#include <linux/mfd/syscon.h>
#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
#include <drm/drm_of.h>
#include <drm/drmP.h>
#include <drm/drm_crtc_helper.h>
@@ -28,6 +29,9 @@ struct rockchip_hdmi {
struct device *dev;
struct regmap *regmap;
struct drm_encoder encoder;
+ struct regulator_bulk_data supplies[2];
+ int nsupplies;
+ bool supplies_enabled;
};
#define to_rockchip_hdmi(x) container_of(x, struct rockchip_hdmi, x)
@@ -179,6 +183,12 @@ static struct drm_encoder_funcs
dw_hdmi_rockchip_encoder_funcs = {
static void dw_hdmi_rockchip_encoder_disable(struct drm_encoder *encoder)
{
+ struct rockchip_hdmi *hdmi = to_rockchip_hdmi(encoder);
+
+ if (hdmi->nsupplies > 0 && hdmi->supplies_enabled) {
+ regulator_bulk_disable(hdmi->nsupplies, hdmi->supplies);
+ hdmi->supplies_enabled = false;
+ }
}
static bool
@@ -199,7 +209,16 @@ static void dw_hdmi_rockchip_encoder_commit(struct
drm_encoder *encoder) {
struct rockchip_hdmi *hdmi = to_rockchip_hdmi(encoder);
u32 val;
- int mux;
+ int mux, ret;
+
+ if (hdmi->nsupplies > 0 && !hdmi->supplies_enabled) {
+ ret = regulator_bulk_enable(hdmi->nsupplies, hdmi->supplies);
+ if (ret) {
+ dev_err(hdmi->dev, "could not enable hdmi analog supplies\n");
+ return;
+ }
+ hdmi->supplies_enabled = true;
+ }
mux = rockchip_drm_encoder_get_mux_id(hdmi->dev->of_node, encoder);
if (mux)
@@ -275,6 +294,17 @@ static int dw_hdmi_rockchip_bind(struct device *dev,
struct device *master, if (!iores)
return -ENXIO;
+ hdmi->supplies[0].supply = "vp";
+ hdmi->supplies[1].supply = "vph";
+ hdmi->nsupplies = 2;
+
+ ret = devm_regulator_bulk_get(hdmi->dev,
+ hdmi->nsupplies, hdmi->supplies);
+ if (ret == -EPROBE_DEFER)
+ return ret;
+ if (ret)
+ hdmi->nsupplies = 0;
+
platform_set_drvdata(pdev, hdmi);
encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);

[Philipp Zabel]

Post by Heiko Stuebner
Hi Philipp,

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

does this look ok now?

Yes, thank you.

Post by Heiko Stuebner
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

[Thierry Reding]

Post by Heiko Stuebner
Hi Philipp,

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

does this look ok now?
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.

Sorry, I had completely missed this.

Post by Heiko Stuebner

Post by Heiko Stuebner
---
- rename supplies to the names found in the hdmi IP databook
- follow suggestion from Russell King to keep regulator handling local
to the rockchip implementation for the time being and only generalize
when a real second implementation needs regulator handling
.../devicetree/bindings/drm/bridge/dw_hdmi.txt | 5 ++++
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 32
+++++++++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt index
a905c14..bb74640 100644
--- a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
+++ b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
@@ -22,6 +22,11 @@ Optional properties
- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
- clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec"
+- vp-supply: 1.0V power supply
+- vph-supply: 1.8V power supply

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.

Thierry

[Heiko Stübner]

Hi Thierry

Post by Thierry Reding

Post by Heiko Stuebner
Hi Philipp,

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

does this look ok now?
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.

Sorry, I had completely missed this.

Post by Heiko Stuebner

Post by Heiko Stuebner
---
- rename supplies to the names found in the hdmi IP databook
- follow suggestion from Russell King to keep regulator handling local
to the rockchip implementation for the time being and only generalize
when a real second implementation needs regulator handling
.../devicetree/bindings/drm/bridge/dw_hdmi.txt | 5 ++++
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 32
+++++++++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt index
a905c14..bb74640 100644
--- a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
+++ b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
@@ -22,6 +22,11 @@ Optional properties
- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
- clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec"
+- vp-supply: 1.0V power supply
+- vph-supply: 1.8V power supply

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.


Heiko

[Thierry Reding]

Post by Heiko Stübner
Hi Thierry

Post by Thierry Reding

Post by Heiko Stuebner
Hi Philipp,

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

does this look ok now?
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.

Sorry, I had completely missed this.

Post by Heiko Stuebner

Post by Heiko Stuebner
---
- rename supplies to the names found in the hdmi IP databook
- follow suggestion from Russell King to keep regulator handling local
to the rockchip implementation for the time being and only generalize
when a real second implementation needs regulator handling
.../devicetree/bindings/drm/bridge/dw_hdmi.txt | 5 ++++
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 32
+++++++++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt index
a905c14..bb74640 100644
--- a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
+++ b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
@@ -22,6 +22,11 @@ Optional properties
- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
- clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec"
+- vp-supply: 1.0V power supply
+- vph-supply: 1.8V power supply

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.

Thierry

[Heiko Stübner]

Hi Thierry

Post by Thierry Reding

Post by Heiko Stübner

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

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

[Russell King - ARM Linux]

Post by Heiko Stübner
Hi Thierry

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

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.

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

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.

[Thierry Reding]

This post might be inappropriate. Click to display it.

[Russell King - ARM Linux]

Post by Thierry Reding

Post by Russell King - ARM Linux

Post by Heiko Stübner
Hi Thierry

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

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.

Post by Thierry Reding
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?

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.

Post by Thierry Reding

Post by Russell King - ARM Linux
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.

Post by Thierry Reding

Post by Russell King - ARM Linux
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.

Post by Thierry Reding

Post by Russell King - ARM Linux
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.

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

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.

[Thierry Reding]

This post might be inappropriate. Click to display it.

[Russell King - ARM Linux]

Post by Thierry Reding

Post by Russell King - ARM Linux
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.

Post by Thierry Reding

Post by Russell King - ARM Linux
Moreover, in the feature list for the HDMI transmitter, it says (though
this is not applicable to iMX6 - and is probably lifted from the
"* 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.

Post by Thierry Reding
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.

Post by Thierry Reding

Post by Russell King - ARM Linux
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.

[Thierry Reding]

Post by Russell King - ARM Linux

Post by Thierry Reding

Post by Russell King - ARM Linux
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.

Post by Russell King - ARM Linux

Post by Thierry Reding

Post by Russell King - ARM Linux
Moreover, in the feature list for the HDMI transmitter, it says (though
this is not applicable to iMX6 - and is probably lifted from the
"* 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.

Post by Thierry Reding
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.

Post by Russell King - ARM Linux

Post by Thierry Reding

Post by Russell King - ARM Linux
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

[Heiko Stübner]

Post by Thierry Reding

Post by Russell King - ARM Linux

Post by Thierry Reding

Post by Russell King - ARM Linux
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.

Post by Russell King - ARM Linux

Post by Thierry Reding

Post by Russell King - ARM Linux
Moreover, in the feature list for the HDMI transmitter, it says (though
this is not applicable to iMX6 - and is probably lifted from the
"* 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.

Post by Thierry Reding
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.

Post by Russell King - ARM Linux

Post by Thierry Reding

Post by Russell King - ARM Linux
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.

Yep.

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.

[Russell King - ARM Linux]

Post by Heiko Stübner
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.

[Heiko Stübner]

Post by Russell King - ARM Linux

Post by Heiko Stübner
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.

they are supplied by the pmic the system uses (named vdd10_lcd and vdd18_lcd
in the soc manuals/schematics) and supply the individual output encoder
analog parts (hdmi, edp, lvds, etc). So if there is no output at all, they
should be able to be turned off.