[v3,4/4] dt-bindings: media: Document Synopsys Designware HDMI RX

Document the bindings for the Synopsys Designware HDMI RX.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Hans Verkuil <hans.verkuil@cisco.com>

Changes from v2:
	- Document edid-phandle property
---
 .../devicetree/bindings/media/snps,dw-hdmi-rx.txt  | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt

Hi Jose,

On 06/16/2017 06:38 PM, Jose Abreu wrote:
> Document the bindings for the Synopsys Designware HDMI RX.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>

> new file mode 100644
> index 0000000..d30cc1e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
> @@ -0,0 +1,45 @@
> +Synopsys DesignWare HDMI RX Decoder
> +===================================
> +
> +This document defines device tree properties for the Synopsys DesignWare HDMI
> +RX Decoder (DWC HDMI RX). It doesn't constitute a device tree binding
> +specification by itself but is meant to be referenced by platform-specific
> +device tree bindings.
> +
> +When referenced from platform device tree bindings the properties defined in
> +this document are defined as follows.
> +
> +- reg: Memory mapped base address and length of the DWC HDMI RX registers.
> +
> +- interrupts: Reference to the DWC HDMI RX interrupt and 5v sense interrupt.
> +
> +- snps,hdmi-phy-jtag-addr: JTAG address of the phy to be used.
> +
> +- snps,hdmi-phy-version: Version of the phy to be used.
> +
> +- snps,hdmi-phy-cfg-clk: Value of the cfg clk that is delivered to phy.
> +
> +- snps,hdmi-phy-driver: *Exact* name of the phy driver to be used.

I don't think we can put Linux driver name in DT like this, devicetree 
is supposed to be describing hardware in OS agnostic way.

> +- snps,hdmi-ctl-cfg-clk: Value of the cfg clk that is delivered to the
> +controller.

How about creating a separate node for the PHY? The binding could then 
be something like:


/* Fixed clock needed only if respective clock is not already defined    
   in the system */

refclk: clock {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <25000000>;
};

hdmi_phy: phy@f3 {
	/* PHY version can be derived from the compatible string */
	compatible = "snps,dw-hdmi-phy-e405"; 	
	/* JTAG address of the PHY */
	reg = <0xf3>

	clocks = <&refclk>
	clock-names = "cfg-clk";
}

hdmi-controller@0 {
	compatible = "snps,dw-hdmi-rx-controller-vX.X";
	reg = <0x0 0x10000>;
	interrupts = <1 3>; 	
	phys = <&hdmi_phy>;

	clocks = <&refclk>
	clock-names = "cfg-clk";
};

Or it might be better to make the PHY node child node of the RX controller 
node, since the RX controller could be considered a controller of the JTAG 
bus IIUC:

refclk: clock {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <25000000>;
};

hdmi-controller@0 {
	compatible = "snps,dw-hdmi-rx-controller-xxx";
	reg = <0x0 0x10000>;
	interrupts = <1 3>; 	
	clocks = <&refclk>
	clock-names = "cfg-clk";
	#address-cells = <1>;
	#size-cells = <0>;

	phy@f3 {
		/* PHY version can be derived from the compatible string */
		compatible = "snps,dw-hdmi-phy-e405";
 		/* address of the PHY on the JTAG bus */
		reg = <0xf3>

		clocks = <&refclk>
		clock-names = "cfg-clk";
	}
};

Then rather than creating platform device in the RX controller driver 
for the PHY just of_platform_populate() could be used.

> +- edid-phandle: phandle to the EDID driver. It can be, for example, the main
> +wrapper driver.
> +
> +A sample binding is now provided. The compatible string is for a wrapper driver
> +which then instantiates the Synopsys Designware HDMI RX decoder driver.

I would avoid talking about drivers in DT binding documentation, we should 
rather refer to hardware blocks.

> +Example:
> +
> +dw_hdmi_wrapper: dw-hdmi-wrapper@0 {
> +	compatible = "snps,dw-hdmi-rx-wrapper";
> +	reg = <0x0 0x10000>; /* controller regbank */
> +	interrupts = <1 3>; /* controller interrupt + 5v sense interrupt */
> +	snps,hdmi-phy-driver = "dw-hdmi-phy-e405";
> +	snps,hdmi-phy-version = <405>;
> +	snps,hdmi-phy-cfg-clk = <25>; /* MHz */
> +	snps,hdmi-ctl-cfg-clk = <25>; /* MHz */
> +	snps,hdmi-phy-jtag-addr = /bits/ 8 <0xfc>;
> +	edid-phandle = <&dw_hdmi_wrapper>;
> +};

--
Regards,
Sylwester

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

Hi Sylwester,


Thanks for the feedback!


On 18-06-2017 18:34, Sylwester Nawrocki wrote:
> Hi Jose,
>
> On 06/16/2017 06:38 PM, Jose Abreu wrote:
>> Document the bindings for the Synopsys Designware HDMI RX.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> new file mode 100644
>> index 0000000..d30cc1e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
>> @@ -0,0 +1,45 @@
>> +Synopsys DesignWare HDMI RX Decoder
>> +===================================
>> +
>> +This document defines device tree properties for the Synopsys DesignWare HDMI
>> +RX Decoder (DWC HDMI RX). It doesn't constitute a device tree binding
>> +specification by itself but is meant to be referenced by platform-specific
>> +device tree bindings.
>> +
>> +When referenced from platform device tree bindings the properties defined in
>> +this document are defined as follows.
>> +
>> +- reg: Memory mapped base address and length of the DWC HDMI RX registers.
>> +
>> +- interrupts: Reference to the DWC HDMI RX interrupt and 5v sense interrupt.
>> +
>> +- snps,hdmi-phy-jtag-addr: JTAG address of the phy to be used.
>> +
>> +- snps,hdmi-phy-version: Version of the phy to be used.
>> +
>> +- snps,hdmi-phy-cfg-clk: Value of the cfg clk that is delivered to phy.
>> +
>> +- snps,hdmi-phy-driver: *Exact* name of the phy driver to be used.
> I don't think we can put Linux driver name in DT like this, devicetree 
> is supposed to be describing hardware in OS agnostic way.

Yeah, I was aware about that, its just that I have a few
limitations that I need to address. I will highlight them bellow
but I think that with your proposed DT bindings and using
of_platform_populate I can address them :)

>
>> +- snps,hdmi-ctl-cfg-clk: Value of the cfg clk that is delivered to the
>> +controller.
> How about creating a separate node for the PHY? The binding could then 
> be something like:
>
>
> /* Fixed clock needed only if respective clock is not already defined    
>    in the system */
>
> refclk: clock {
> 	compatible = "fixed-clock";
> 	#clock-cells = <0>;
> 	clock-frequency = <25000000>;
> };
>
> hdmi_phy: phy@f3 {
> 	/* PHY version can be derived from the compatible string */
> 	compatible = "snps,dw-hdmi-phy-e405"; 	
> 	/* JTAG address of the PHY */
> 	reg = <0xf3>
>
> 	clocks = <&refclk>
> 	clock-names = "cfg-clk";
> }
>
> hdmi-controller@0 {
> 	compatible = "snps,dw-hdmi-rx-controller-vX.X";
> 	reg = <0x0 0x10000>;
> 	interrupts = <1 3>; 	
> 	phys = <&hdmi_phy>;
>
> 	clocks = <&refclk>
> 	clock-names = "cfg-clk";
> };
>
> Or it might be better to make the PHY node child node of the RX controller 
> node, since the RX controller could be considered a controller of the JTAG 
> bus IIUC:
>
> refclk: clock {
> 	compatible = "fixed-clock";
> 	#clock-cells = <0>;
> 	clock-frequency = <25000000>;
> };
>
> hdmi-controller@0 {
> 	compatible = "snps,dw-hdmi-rx-controller-xxx";
> 	reg = <0x0 0x10000>;
> 	interrupts = <1 3>; 	
> 	clocks = <&refclk>
> 	clock-names = "cfg-clk";
> 	#address-cells = <1>;
> 	#size-cells = <0>;
>
> 	phy@f3 {
> 		/* PHY version can be derived from the compatible string */
> 		compatible = "snps,dw-hdmi-phy-e405";
>  		/* address of the PHY on the JTAG bus */
> 		reg = <0xf3>
>
> 		clocks = <&refclk>
> 		clock-names = "cfg-clk";
> 	}
> };
>
> Then rather than creating platform device in the RX controller driver 
> for the PHY just of_platform_populate() could be used.

Using fixed-clock was already in my todo list. Regarding phy I
need to pass pdata so that the callbacks between controller and
phy are established. I also need to make sure that phy driver
will be loaded by the controller driver. Hmm, and also address of
the phy on th JTAG bus is fed to the controller driver not to the
phy driver. Maybe leave the property as is (the
"snps,hdmi-phy-jtag-addr") or parse it from the phy node?

I also need to pass pdata to the controller driver (the callbacks
for 5v handling) which are agnostic of the controller. These
reasons prevented me from adding compatible strings to both
drivers and just use a wrapper driver instead. This way i do
"modprobe wrapper_driver" and I get all the drivers loaded via
request_module(). Still, I like your approach much better. I saw
that I can pass pdata using of_platform_populate, could you
please confirm if I can still maintain this architecture (i.e.
prevent modules from loading until I get all the chain setup)?

Following your approach I could get something like this:

hdmi_system@YYYY {
    compatible = "snps,dw-hdmi-rx-wrapper";
    reg = <0xYYYY 0xZZZZ>;
    interrupts = <3>;
    #address-cells = <1>;
    #size-cells = <1>;

    hdmi_controller@0 {
        compatible = "snps,dw-hdmi-rx-controller";
        reg = <0x0 0x10000>;
        interrupts = <1>;
        edid-phandle = <&hdmi_system>;
        clocks = <&refclk>;
        clock-names = "ref-clk";
        #address-cells = <1>;
        #size-cells = <0>;

        hdmi_phy@f3 {
            compatible = "snps,dw-hdmi-phy-e405";
            reg = <0xf3>;
            clocks = <&cfgclk>;
            clock-names = "cfg-clk";
        }
    }
};

And then snps,dw-hdmi-rx-wrapper would call of_platform_populate
for controller which would instead call of_platform_populate for
phy. Is this possible, and maintainable? Isn't the controller
driver get auto loaded because of the compatible string match?
And one more thing: The reg address of the hdmi_controller: Isn't
this relative to the parent node? I mean isn't this going to be
0xYYYY + 0x0? Because I don't want that :/

Best regards,
Jose Miguel Abreu

>
>> +- edid-phandle: phandle to the EDID driver. It can be, for example, the main
>> +wrapper driver.
>> +
>> +A sample binding is now provided. The compatible string is for a wrapper driver
>> +which then instantiates the Synopsys Designware HDMI RX decoder driver.
> I would avoid talking about drivers in DT binding documentation, we should 
> rather refer to hardware blocks.
>
>> +Example:
>> +
>> +dw_hdmi_wrapper: dw-hdmi-wrapper@0 {
>> +	compatible = "snps,dw-hdmi-rx-wrapper";
>> +	reg = <0x0 0x10000>; /* controller regbank */
>> +	interrupts = <1 3>; /* controller interrupt + 5v sense interrupt */
>> +	snps,hdmi-phy-driver = "dw-hdmi-phy-e405";
>> +	snps,hdmi-phy-version = <405>;
>> +	snps,hdmi-phy-cfg-clk = <25>; /* MHz */
>> +	snps,hdmi-ctl-cfg-clk = <25>; /* MHz */
>> +	snps,hdmi-phy-jtag-addr = /bits/ 8 <0xfc>;
>> +	edid-phandle = <&dw_hdmi_wrapper>;
>> +};
> --
> Regards,
> Sylwester
>

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

On 06/19/2017 11:01 AM, Jose Abreu wrote:
> Using fixed-clock was already in my todo list. Regarding phy I
> need to pass pdata so that the callbacks between controller and
> phy are established. I also need to make sure that phy driver
> will be loaded by the controller driver. Hmm, and also address of
> the phy on th JTAG bus is fed to the controller driver not to the
> phy driver. Maybe leave the property as is (the
> "snps,hdmi-phy-jtag-addr") or parse it from the phy node?

I think the RX controller can parse it's child phy node to retrieve JTAG 
address from the reg property.  That seems better than creating custom 
property for device address on the bus.

Does the PHY support any other type of control bus, e.g. I2C or SPI?

> I also need to pass pdata to the controller driver (the callbacks
> for 5v handling) which are agnostic of the controller. These

Is this about detecting +5V coming from the HDMI connector? Or some
other voltage supply?

> reasons prevented me from adding compatible strings to both
> drivers and just use a wrapper driver instead. This way i do

If you add struct of_device_id instance to your module and define
MODULE_DEVICE_ALIAS(of, ...) there, it will allow the module to be loaded 
when device with matching compatible string is created in the kernel. 
User space is notified with uevent about device creation.

> "modprobe wrapper_driver" and I get all the drivers loaded via
> request_module(). Still, I like your approach much better. I saw
> that I can pass pdata using of_platform_populate, could you
> please confirm if I can still maintain this architecture (i.e.
> prevent modules from loading until I get all the chain setup)?

You could try to pass platform data this way, that should work. But 
I doubt it's the right directions, I would rather see things done 
in the V4L2 layer. 

> Following your approach I could get something like this:
> 
> hdmi_system@YYYY {
>      compatible = "snps,dw-hdmi-rx-wrapper";

This would need to refer to some hardware block, we should avoid virtual 
device nodes in DT.

>      reg = <0xYYYY 0xZZZZ>;
>      interrupts = <3>;
>      #address-cells = <1>;
>      #size-cells = <1>;

You would need also an (empty) 'ranges' property here.

>      hdmi_controller@0 {
>          compatible = "snps,dw-hdmi-rx-controller";
>          reg = <0x0 0x10000>;
>          interrupts = <1>;
>          edid-phandle = <&hdmi_system>;
>          clocks = <&refclk>;
>          clock-names = "ref-clk";
>          #address-cells = <1>;
>          #size-cells = <0>;
> 
>          hdmi_phy@f3 {
>              compatible = "snps,dw-hdmi-phy-e405";
>              reg = <0xf3>;
>              clocks = <&cfgclk>;
>              clock-names = "cfg-clk";
>          }
>      }
> };
> 
> And then snps,dw-hdmi-rx-wrapper would call of_platform_populate
> for controller which would instead call of_platform_populate for
> phy. Is this possible, and maintainable? Isn't the controller
> driver get auto loaded because of the compatible string match?

As I mentioned above, auto loading should work if you have instance 
of MODULE_DEVICE_TABLE() defined in the module, but the module might
not be available immediately after creating devices with 
of_platform_populate().  You may want to have a look at the v4l2-async 
API (drivers/media/v4l2-core/v4l2-async.c). It allows one driver
to register a notifier for its sub-devices. And the parent driver
can complete initialization when it gets all its v4l2 subdevs
registered.

But I'm not sure about calls from the PHY back to the RX controller, 
possibly v4l2_set_subdev_hostdata()/v4l2_get_subdev_hostdata() could 
be used for passing the ops.

> And one more thing: The reg address of the hdmi_controller: Isn't
> this relative to the parent node? I mean isn't this going to be
> 0xYYYY + 0x0? Because I don't want that :/

Address space of child nodes doesn't need to be relative to 'reg' range 
of parent node, these can be entirely distinct address ranges. See for 
example I2C bus controllers, the I2C addresses of slave devices are not 
much related to the memory mapped IO registers region of the bus controller.

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

Hi Sylwester,


On 19-06-2017 22:18, Sylwester Nawrocki wrote:
> On 06/19/2017 11:01 AM, Jose Abreu wrote:
>> Using fixed-clock was already in my todo list. Regarding phy I
>> need to pass pdata so that the callbacks between controller and
>> phy are established. I also need to make sure that phy driver
>> will be loaded by the controller driver. Hmm, and also address of
>> the phy on th JTAG bus is fed to the controller driver not to the
>> phy driver. Maybe leave the property as is (the
>> "snps,hdmi-phy-jtag-addr") or parse it from the phy node?
> I think the RX controller can parse it's child phy node to retrieve JTAG 
> address from the reg property.  That seems better than creating custom 
> property for device address on the bus.

Ok, will do that.

>
> Does the PHY support any other type of control bus, e.g. I2C or SPI?

It supports I2C but its all done by controller regbank.

>
>> I also need to pass pdata to the controller driver (the callbacks
>> for 5v handling) which are agnostic of the controller. These
> Is this about detecting +5V coming from the HDMI connector? Or some
> other voltage supply?

Yes, 5v signal from HDMI source.

>
>> reasons prevented me from adding compatible strings to both
>> drivers and just use a wrapper driver instead. This way i do
> If you add struct of_device_id instance to your module and define
> MODULE_DEVICE_ALIAS(of, ...) there, it will allow the module to be loaded 
> when device with matching compatible string is created in the kernel. 
> User space is notified with uevent about device creation.

Hmm, I used another approach. I have a list of supported phys in
the controller driver and the corresponding driver name, then I
just match the node, get the driver name and do a request module
the of_platform_populate. Maybe its not the best option?
 
>
>> "modprobe wrapper_driver" and I get all the drivers loaded via
>> request_module(). Still, I like your approach much better. I saw
>> that I can pass pdata using of_platform_populate, could you
>> please confirm if I can still maintain this architecture (i.e.
>> prevent modules from loading until I get all the chain setup)?
> You could try to pass platform data this way, that should work. But 
> I doubt it's the right directions, I would rather see things done 
> in the V4L2 layer. 

But this doesn't exist right? We would need something similar to
v4l2_i2c_new_subdev_board() but for platform devices.

>
>> Following your approach I could get something like this:
>>
>> hdmi_system@YYYY {
>>      compatible = "snps,dw-hdmi-rx-wrapper";
> This would need to refer to some hardware block, we should avoid virtual 
> device nodes in DT.

By wrapper I mean the SoC, i.e. the whole integration of the
controller + phy + EDID + 5v + ...

>
>>      reg = <0xYYYY 0xZZZZ>;
>>      interrupts = <3>;
>>      #address-cells = <1>;
>>      #size-cells = <1>;
> You would need also an (empty) 'ranges' property here.

Yes, I already added it here locally.

>
>>      hdmi_controller@0 {
>>          compatible = "snps,dw-hdmi-rx-controller";
>>          reg = <0x0 0x10000>;
>>          interrupts = <1>;
>>          edid-phandle = <&hdmi_system>;
>>          clocks = <&refclk>;
>>          clock-names = "ref-clk";
>>          #address-cells = <1>;
>>          #size-cells = <0>;
>>
>>          hdmi_phy@f3 {
>>              compatible = "snps,dw-hdmi-phy-e405";
>>              reg = <0xf3>;
>>              clocks = <&cfgclk>;
>>              clock-names = "cfg-clk";
>>          }
>>      }
>> };
>>
>> And then snps,dw-hdmi-rx-wrapper would call of_platform_populate
>> for controller which would instead call of_platform_populate for
>> phy. Is this possible, and maintainable? Isn't the controller
>> driver get auto loaded because of the compatible string match?
> As I mentioned above, auto loading should work if you have instance 
> of MODULE_DEVICE_TABLE() defined in the module, but the module might
> not be available immediately after creating devices with 
> of_platform_populate().  You may want to have a look at the v4l2-async 
> API (drivers/media/v4l2-core/v4l2-async.c). It allows one driver
> to register a notifier for its sub-devices. And the parent driver
> can complete initialization when it gets all its v4l2 subdevs
> registered.
>
> But I'm not sure about calls from the PHY back to the RX controller, 
> possibly v4l2_set_subdev_hostdata()/v4l2_get_subdev_hostdata() could 
> be used for passing the ops.

Ok, I will move to the async API then.

>
>> And one more thing: The reg address of the hdmi_controller: Isn't
>> this relative to the parent node? I mean isn't this going to be
>> 0xYYYY + 0x0? Because I don't want that :/
> Address space of child nodes doesn't need to be relative to 'reg' range 
> of parent node, these can be entirely distinct address ranges. See for 
> example I2C bus controllers, the I2C addresses of slave devices are not 
> much related to the memory mapped IO registers region of the bus controller.

Yes, I already managed to get it working :)

Best regards,
Jose Miguel Abreu

>
> --
> Regards,
> Sylwester

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