[3/4] arm64: Juno: Add HDLCD support to the Juno boards.
diff mbox

Message ID 1438784892-27888-4-git-send-email-Liviu.Dudau@arm.com
State New
Headers show

Commit Message

Liviu Dudau Aug. 5, 2015, 2:28 p.m. UTC
ARM's Juno board has two HDLCD controllers, each linked to an NXP
TDA19988 HDMI transmitter that provides output encoding. Add them
to the device tree.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 arch/arm64/boot/dts/arm/juno-base.dtsi | 70 +++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 2 deletions(-)

Comments

Russell King - ARM Linux Aug. 5, 2015, 5:53 p.m. UTC | #1
On Wed, Aug 05, 2015 at 03:28:11PM +0100, Liviu Dudau wrote:
> +		hdmi-transmitter@71 {
>  			compatible = "nxp,tda998x";
>  			reg = <0x71>;
> +			port {
> +				tda998x_1_input: endpoint@0 {
> +					remote-endpoint = <&hdlcd1_output>;
> +				};
> +
> +				tda998x_1_output: endpoint@1 {
> +					remote-endpoint = <&hdmi1_connector_output>;
> +				};
> +			};

This isn't compliant with the TDA998x binding, and is very likely to
screw Jean's work on adding audio support.  See emails on lakml and
Documentation/devicetree/bindings/drm/i2c/tda998x.txt (which looks
like it needs updating to include the ports stuff.)

Also, the whole question of representing connectors in a DRM model is
yet to be established.  Yes, DT should describe the hardware, but we
don't yet know _how_ to describe physical connectors with stuff
implemented on top of DRM yet, and we have nothing that makes use of
this.

Also note that ePAPR requires a reg= property if you specify a
unit-address (the bit after the @ sign) so the above is non-compliant
with ePAPR as well.
Liviu Dudau Aug. 5, 2015, 7:03 p.m. UTC | #2
On Wed, Aug 05, 2015 at 06:53:03PM +0100, Russell King - ARM Linux wrote:
> On Wed, Aug 05, 2015 at 03:28:11PM +0100, Liviu Dudau wrote:
> > +		hdmi-transmitter@71 {
> >  			compatible = "nxp,tda998x";
> >  			reg = <0x71>;
> > +			port {
> > +				tda998x_1_input: endpoint@0 {
> > +					remote-endpoint = <&hdlcd1_output>;
> > +				};
> > +
> > +				tda998x_1_output: endpoint@1 {
> > +					remote-endpoint = <&hdmi1_connector_output>;
> > +				};
> > +			};

Hi Russell,

Thanks for reviewing this, I will integrate your comments in the
next revision.

> 
> This isn't compliant with the TDA998x binding, and is very likely to
> screw Jean's work on adding audio support.  See emails on lakml and
> Documentation/devicetree/bindings/drm/i2c/tda998x.txt (which looks
> like it needs updating to include the ports stuff.)

I have to confess that I am not entirely up to speed with the TDA19988
situation at the moment. Andrew Jackson was dealing with that and
working with Jean to get that in the upstream, but his contract has
ended and he has moved to other things. While I did get through a
(limited) set of patches that Jean has sent around, I'm a bit confused
about the latest state of play. Is there an authoritative source from
where I can grab the patches that are going to be in 4.3? Otherwise,
as far as the current patchset base (4.2-rc2) is concerned, I am
compliant with the bindings if you ignore the port subnode (see further
below). I am happy to update the patchset to match what is going into
mainline, but just reading the emails in the mailing list I'm not
exactly sure on the sequencing of things here.

> 
> Also, the whole question of representing connectors in a DRM model is
> yet to be established.  Yes, DT should describe the hardware, but we
> don't yet know _how_ to describe physical connectors with stuff
> implemented on top of DRM yet, and we have nothing that makes use of
> this.

Please help me understand the current situation: you have added
support for components that the video drivers can use and the bindings
for that are described in Documentation/devicetree/bindings/media/video-interfaces.txt.
According to that document my patch should be compliant once I add the
reg= property. Is that something that cannot be used with tda998x driver
or is there any other reason why you think the patch is not compliant?

If you are referring to connecting an encoder with a HDMI connector, I
have tested that and it seems to work, although my situation is simple
because there are no options in my setup: one HDLCD connects to one TDA19988
which connects to one HDMI output.

> 
> Also note that ePAPR requires a reg= property if you specify a
> unit-address (the bit after the @ sign) so the above is non-compliant
> with ePAPR as well.

Thanks, I will add the reg property.

Best regards,
Liviu

> 
> -- 
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.
>
Russell King - ARM Linux Aug. 5, 2015, 9:48 p.m. UTC | #3
On Wed, Aug 05, 2015 at 08:03:12PM +0100, Liviu Dudau wrote:
> I have to confess that I am not entirely up to speed with the TDA19988
> situation at the moment. Andrew Jackson was dealing with that and
> working with Jean to get that in the upstream, but his contract has
> ended and he has moved to other things.

Umm, I'm the maintainer for TDA998x:

NXP TDA998X DRM DRIVER
M:      Russell King <rmk+kernel@arm.linux.org.uk>
S:      Supported
F:      drivers/gpu/drm/i2c/tda998x_drv.c
F:      include/drm/i2c/tda998x.h

It would be nice if people worked with the actual maintainers of things
rather than random other people...

> > Also, the whole question of representing connectors in a DRM model is
> > yet to be established.  Yes, DT should describe the hardware, but we
> > don't yet know _how_ to describe physical connectors with stuff
> > implemented on top of DRM yet, and we have nothing that makes use of
> > this.
> 
> Please help me understand the current situation: you have added
> support for components that the video drivers can use and the bindings
> for that are described in Documentation/devicetree/bindings/media/video-interfaces.txt.

No.  I added the component helpers, which are entirely firmware agnostic.
The media bindings were created by others, and through development done
by Pengutronix, they were factored out of media into common DT code and
re-used for the IMX DRM driver.  The binding document which describes
that work is not the one you refer to above, but this one:

Documentation/devicetree/bindings/graph.txt

This started them as a basis for DRM drivers on ARM - but it's never been
officially "adopted" as a method to describe DRM drivers - it's only what
some drivers are using.  It ought to be nailed down as a way to ensure
inter-operability between components though, but no one has really made
that decision.

> According to that document my patch should be compliant once I add the
> reg= property. Is that something that cannot be used with tda998x driver
> or is there any other reason why you think the patch is not compliant?

Jean's proposal to add audio support to the TDA998x driver does it via
this change to the binding spec:

+Optional nodes:
+
+  - port: up to three ports.
+       The ports are defined according to [1].
+
+    Video port.
+       There may be only one video port.
+       This one must contain the following property:
+
+       - port-type: must be "rgb"
+
+       and may contain the optional property:
+
+       - reg: 24 bits value which defines how the video controller
+               output is wired to the TDA998x input (video pins)
+               When absent, the default value is <0x230145>.
+
+    Audio ports.
+       There may be one or two audio ports.
+       These ones must contain the following properties:
+
+       - port-type: must be "i2s" or "spdif"
+
+       - reg: 8 bits value which defines how the audio controller
+               output is wired to the TDA998x input (audio pins)
+
+[1] Documentation/devicetree/bindings/graph.txt

(That's not a particularly precise definition, but it's what we have at
the moment.)

> If you are referring to connecting an encoder with a HDMI connector, I
> have tested that and it seems to work, although my situation is simple
> because there are no options in my setup: one HDLCD connects to one
> TDA19988 which connects to one HDMI output.

Right now, the TDA998x will ignore the additional information, but that
won't be the case with Jean's audio work (see the above binding
information.)
Liviu Dudau Aug. 6, 2015, 10:16 a.m. UTC | #4
On Wed, Aug 05, 2015 at 10:48:54PM +0100, Russell King - ARM Linux wrote:
> On Wed, Aug 05, 2015 at 08:03:12PM +0100, Liviu Dudau wrote:
> > I have to confess that I am not entirely up to speed with the TDA19988
> > situation at the moment. Andrew Jackson was dealing with that and
> > working with Jean to get that in the upstream, but his contract has
> > ended and he has moved to other things.
> 
> Umm, I'm the maintainer for TDA998x:
> 
> NXP TDA998X DRM DRIVER
> M:      Russell King <rmk+kernel@arm.linux.org.uk>
> S:      Supported
> F:      drivers/gpu/drm/i2c/tda998x_drv.c
> F:      include/drm/i2c/tda998x.h
> 
> It would be nice if people worked with the actual maintainers of things
> rather than random other people...

Sorry, it was my mistake, I have blindly followed the get_maintainers.pl
generated list of email rather than engage my brain and realise that part
of the patch also affects TDA19988 driver.

> 
> > > Also, the whole question of representing connectors in a DRM model is
> > > yet to be established.  Yes, DT should describe the hardware, but we
> > > don't yet know _how_ to describe physical connectors with stuff
> > > implemented on top of DRM yet, and we have nothing that makes use of
> > > this.
> > 
> > Please help me understand the current situation: you have added
> > support for components that the video drivers can use and the bindings
> > for that are described in Documentation/devicetree/bindings/media/video-interfaces.txt.
> 
> No.  I added the component helpers, which are entirely firmware agnostic.
> The media bindings were created by others, and through development done
> by Pengutronix, they were factored out of media into common DT code and
> re-used for the IMX DRM driver.  The binding document which describes
> that work is not the one you refer to above, but this one:
> 
> Documentation/devicetree/bindings/graph.txt
> 
> This started them as a basis for DRM drivers on ARM - but it's never been
> officially "adopted" as a method to describe DRM drivers - it's only what
> some drivers are using.  It ought to be nailed down as a way to ensure
> inter-operability between components though, but no one has really made
> that decision.

OK, I'm interested on whom do I need to talk to in order for the official
"adoption" to happen here. 

> 
> > According to that document my patch should be compliant once I add the
> > reg= property. Is that something that cannot be used with tda998x driver
> > or is there any other reason why you think the patch is not compliant?
> 
> Jean's proposal to add audio support to the TDA998x driver does it via
> this change to the binding spec:
> 
> +Optional nodes:
> +
> +  - port: up to three ports.
> +       The ports are defined according to [1].
> +
> +    Video port.
> +       There may be only one video port.
> +       This one must contain the following property:
> +
> +       - port-type: must be "rgb"
> +
> +       and may contain the optional property:
> +
> +       - reg: 24 bits value which defines how the video controller
> +               output is wired to the TDA998x input (video pins)
> +               When absent, the default value is <0x230145>.
> +
> +    Audio ports.
> +       There may be one or two audio ports.
> +       These ones must contain the following properties:
> +
> +       - port-type: must be "i2s" or "spdif"
> +
> +       - reg: 8 bits value which defines how the audio controller
> +               output is wired to the TDA998x input (audio pins)
> +
> +[1] Documentation/devicetree/bindings/graph.txt
> 
> (That's not a particularly precise definition, but it's what we have at
> the moment.)
> 
> > If you are referring to connecting an encoder with a HDMI connector, I
> > have tested that and it seems to work, although my situation is simple
> > because there are no options in my setup: one HDLCD connects to one
> > TDA19988 which connects to one HDMI output.
> 
> Right now, the TDA998x will ignore the additional information, but that
> won't be the case with Jean's audio work (see the above binding
> information.)

Yes, I have seen that patchset. I will apply it to my tree and send an
updated version with the port-type property when I send v2.

Best regards,
Liviu

> 
> -- 
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.
>

Patch
diff mbox

diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi b/arch/arm64/boot/dts/arm/juno-base.dtsi
index c624208..a12a2b1 100644
--- a/arch/arm64/boot/dts/arm/juno-base.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-base.dtsi
@@ -118,6 +118,34 @@ 
 		clock-names = "apb_pclk";
 	};
 
+	hdlcd@7ff50000 {
+		compatible = "arm,hdlcd";
+		reg = <0 0x7ff50000 0 0x1000>;
+		interrupts = <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&scpi_clk 4>;
+		clock-names = "pxlclk";
+
+		port {
+			hdlcd1_output: endpoint@0 {
+				remote-endpoint = <&tda998x_1_input>;
+			};
+		};
+	};
+
+	hdlcd@7ff60000 {
+		compatible = "arm,hdlcd";
+		reg = <0 0x7ff60000 0 0x1000>;
+		interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&scpi_clk 4>;
+		clock-names = "pxlclk";
+
+		port {
+			hdlcd0_output: endpoint@0 {
+				remote-endpoint = <&tda998x_0_input>;
+			};
+		};
+	};
+
 	soc_uart0: uart@7ff80000 {
 		compatible = "arm,pl011", "arm,primecell";
 		reg = <0x0 0x7ff80000 0x0 0x1000>;
@@ -136,14 +164,32 @@ 
 		i2c-sda-hold-time-ns = <500>;
 		clocks = <&soc_smc50mhz>;
 
-		dvi0: dvi-transmitter@70 {
+		hdmi-transmitter@70 {
 			compatible = "nxp,tda998x";
 			reg = <0x70>;
+			port {
+				tda998x_0_input: endpoint@0 {
+					remote-endpoint = <&hdlcd0_output>;
+				};
+
+				tda998x_0_output: endpoint@1 {
+					remote-endpoint = <&hdmi0_connector_output>;
+				};
+			};
 		};
 
-		dvi1: dvi-transmitter@71 {
+		hdmi-transmitter@71 {
 			compatible = "nxp,tda998x";
 			reg = <0x71>;
+			port {
+				tda998x_1_input: endpoint@0 {
+					remote-endpoint = <&hdlcd1_output>;
+				};
+
+				tda998x_1_output: endpoint@1 {
+					remote-endpoint = <&hdmi1_connector_output>;
+				};
+			};
 		};
 	};
 
@@ -177,6 +223,26 @@ 
 		      <0x00000008 0x80000000 0x1 0x80000000>;
 	};
 
+	hdmi0: connector@0 {
+		compatible = "hdmi-connector";
+		type = "a";
+		port {
+			hdmi0_connector_output: endpoint {
+				remote-endpoint = <&tda998x_0_output>;
+			};
+		};
+	};
+
+	hdmi1: connector@1 {
+		compatible = "hdmi-connector";
+		type = "a";
+		port {
+			hdmi1_connector_output: endpoint {
+				remote-endpoint = <&tda998x_1_output>;
+			};
+		};
+	};
+
 	smb {
 		compatible = "simple-bus";
 		#address-cells = <2>;