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

[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 <heiko@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: 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);
-- 
2.1.4


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

[PATCH v3 2/2] ARM: dts: rockchip: add hdmi analog power supplies to rk3288 boards

[Heiko Stuebner]

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

Signed-off-by: Heiko Stuebner <heiko@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";
 };
-- 
2.1.4


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

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

[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?

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

> ---
> 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: 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);

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

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

[Philipp Zabel]

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-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> 
> does this look ok now?

Yes, thank you.
Acked-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

> 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Thierry Reding]

On Mon, Mar 23, 2015 at 07:17:49PM +0100, Heiko Stuebner wrote:
> 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?
> 
> 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.

> > ---
> > 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

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Heiko Stübner]

Hi Thierry

Am Freitag, 5. Juni 2015, 13:02:01 schrieb Thierry Reding:
> On Mon, Mar 23, 2015 at 07:17:49PM +0100, Heiko Stuebner wrote:
> > 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?
> > 
> > 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.
> 
> > > ---
> > > 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
> 
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3190 bytes --]

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:
> > On Mon, Mar 23, 2015 at 07:17:49PM +0100, Heiko Stuebner wrote:
> > > 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?
> > > 
> > > 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.
> > 
> > > > ---
> > > > 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
> > 
> > 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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

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

[Heiko Stübner]

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Russell King - ARM Linux]

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.

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.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4158 bytes --]

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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

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

[Russell King - ARM Linux]

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.

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?

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.

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

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

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

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 9379 bytes --]

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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

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

[Russell King - ARM Linux]

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.

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

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

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 5935 bytes --]

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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

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

[Heiko Stübner]

Am Dienstag, 9. Juni 2015, 09:53:32 schrieb Thierry Reding:
> 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.

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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Heiko Stübner]

Am Mittwoch, 10. Juni 2015, 00:36:18 schrieb Russell King - ARM Linux:
> 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.

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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel