| Message ID | 1438784892-27888-4-git-send-email-Liviu.Dudau@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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. >
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.)
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. >
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>;
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(-)