diff mbox series

[v9,4/5] arm64: dts: ti: k3-j784s4-evm: Enable DisplayPort-0

Message ID 20230803080441.367341-5-j-choudhary@ti.com
State New
Headers
Series Enable Display for J784S4 and AM69-SK platform |

Commit Message

Jayesh Choudhary Aug. 3, 2023, 8:04 a.m. UTC 
  From: Rahul T R <r-ravikumar@ti.com>

Enable display for J784S4 EVM.

Add assigned clocks for DSS, DT node for DisplayPort PHY and pinmux for
DP HPD. Add the clock frequency for serdes_refclk.

Add the endpoint nodes to describe connection from:
DSS => MHDP => DisplayPort connector.

Also add the GPIO expander-4 node and pinmux for main_i2c4 which is
required for controlling DP power. Set status for all required nodes
for DP-0 as "okay".

Signed-off-by: Rahul T R <r-ravikumar@ti.com>
[j-choudhary@ti.com: move all the changes together to enable DP-0 in EVM]
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---
 arch/arm64/boot/dts/ti/k3-j784s4-evm.dts | 119 +++++++++++++++++++++++
 1 file changed, 119 insertions(+)

Comments

Aradhya Bhatia Aug. 4, 2023, 7:22 p.m. UTC | #1 
Hi Jayesh,


On 03-Aug-23 13:34, Jayesh Choudhary wrote:
> From: Rahul T R <r-ravikumar@ti.com>
> 
> Enable display for J784S4 EVM.
> 
> Add assigned clocks for DSS, DT node for DisplayPort PHY and pinmux for
> DP HPD. Add the clock frequency for serdes_refclk.
> 
> Add the endpoint nodes to describe connection from:
> DSS => MHDP => DisplayPort connector.
> 
> Also add the GPIO expander-4 node and pinmux for main_i2c4 which is
> required for controlling DP power. Set status for all required nodes
> for DP-0 as "okay".
> 
> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
> [j-choudhary@ti.com: move all the changes together to enable DP-0 in EVM]
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> ---
>  arch/arm64/boot/dts/ti/k3-j784s4-evm.dts | 119 +++++++++++++++++++++++
>  1 file changed, 119 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts b/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts
> index 7ad152a1b90f..005357d70122 100644
> --- a/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts
> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts
> @@ -249,6 +249,28 @@ vdd_sd_dv: regulator-TLV71033 {
>  		states = <1800000 0x0>,
>  			 <3300000 0x1>;
>  	};
> +
> +	dp0_pwr_3v3: regulator-dp0-prw {
> +		compatible = "regulator-fixed";
> +		regulator-name = "dp0-pwr";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&exp4 0 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +	};
> +
> +	dp0: connector-dp0 {
> +		compatible = "dp-connector";
> +		label = "DP0";
> +		type = "full-size";
> +		dp-pwr-supply = <&dp0_pwr_3v3>;
> +
> +		port {
> +			dp0_connector_in: endpoint {
> +				remote-endpoint = <&dp0_out>;
> +			};
> +		};
> +	};
>  };
>  
>  &main_pmx0 {
> @@ -286,6 +308,19 @@ vdd_sd_dv_pins_default: vdd-sd-dv-default-pins {
>  			J784S4_IOPAD(0x020, PIN_INPUT, 7) /* (AJ35) MCAN15_RX.GPIO0_8 */
>  		>;
>  	};
> +
> +	dp0_pins_default: dp0-default-pins {
> +		pinctrl-single,pins = <
> +			J784S4_IOPAD(0x0cc, PIN_INPUT, 12) /* (AM37) SPI0_CS0.DP0_HPD */
> +		>;
> +	};
> +
> +	main_i2c4_pins_default: main-i2c4-default-pins {
> +		pinctrl-single,pins = <
> +			J784S4_IOPAD(0x014, PIN_INPUT_PULLUP, 8) /* (AG33) MCAN14_TX.I2C4_SCL */
> +			J784S4_IOPAD(0x010, PIN_INPUT_PULLUP, 8) /* (AH33) MCAN13_RX.I2C4_SDA */
> +		>;
> +	};
>  };
>  
>  &wkup_pmx2 {
> @@ -827,3 +862,87 @@ adc {
>  		ti,adc-channels = <0 1 2 3 4 5 6 7>;
>  	};
>  };
> +
> +&serdes_refclk {
> +	status = "okay";
> +	clock-frequency = <100000000>;
> +};
> +
> +&dss {
> +	status = "okay";
> +	assigned-clocks = <&k3_clks 218 2>,
> +			  <&k3_clks 218 5>,
> +			  <&k3_clks 218 14>,
> +			  <&k3_clks 218 18>;
> +	assigned-clock-parents = <&k3_clks 218 3>,
> +				 <&k3_clks 218 7>,
> +				 <&k3_clks 218 16>,
> +				 <&k3_clks 218 22>;
> +};
> +
> +&serdes_wiz4 {
> +	status = "okay";
> +};
> +
> +&serdes4 {
> +	status = "okay";
> +	serdes4_dp_link: phy@0 {
> +		reg = <0>;
> +		cdns,num-lanes = <4>;
> +		#phy-cells = <0>;
> +		cdns,phy-type = <PHY_TYPE_DP>;
> +		resets = <&serdes_wiz4 1>, <&serdes_wiz4 2>,
> +			 <&serdes_wiz4 3>, <&serdes_wiz4 4>;
> +	};
> +};
> +
> +&mhdp {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&dp0_pins_default>;
> +	phys = <&serdes4_dp_link>;
> +	phy-names = "dpphy";
> +};
> +
> +&dss_ports {
> +	port {

Port index has not been added here. Since this port outputs to MHDP
bridge, this should be "port@0", and a "reg = <0>;" property should be
added below (along with the address and size cells properties).

I suppose this works functionally in this case, because the port gets
defaulted to "0" by the driver. But in future, when we add support for
other dss output(s) on j784s4-evm, the driver will need indices to
distinguish among them.

> +		dpi0_out: endpoint {
> +			remote-endpoint = <&dp0_in>;
> +		};
> +	};
> +};
> +
> +&main_i2c4 {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&main_i2c4_pins_default>;
> +	clock-frequency = <400000>;
> +
> +	exp4: gpio@20 {
> +		compatible = "ti,tca6408";
> +		reg = <0x20>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +	};
> +};
> +
> +&dp0_ports {
> +	#address-cells = <1>;
> +	#size-cells = <0>;

These properties are being repeated from the MHDP node in the main.dtsi
file, in the previous patch. You can drop them from one of the places.

I would suggest keeping them in the main.dtsi file and removing them
here, so that repetition is avoided across different platform.dts files,
but I will leave the call up to you. In any case, they need to be
removed from one of the two places.

Btw, same will be applicable for dss_ports as well (once you add port
index). Separate address and size cells properties won't be required in
the j784s4-evm.dts and am69-sk.dts platform files, once they are already
there in j784s4-main.dtsi.

With the changes suggested above,

Reviewed-by: Aradhya Bhatia <a-bhatia1@ti.com>

> +
> +	port@0 {
> +		reg = <0>;
> +
> +		dp0_in: endpoint {
> +			remote-endpoint = <&dpi0_out>;
> +		};
> +	};
> +
> +	port@4 {
> +		reg = <4>;
> +
> +		dp0_out: endpoint {
> +			remote-endpoint = <&dp0_connector_in>;
> +		};
> +	};
> +};
Jayesh Choudhary Aug. 7, 2023, 12:24 p.m. UTC | #2 
Hello Aradhya,

Thank you for the review.

On 05/08/23 00:52, Aradhya Bhatia wrote:
> Hi Jayesh,
> 
> 
> On 03-Aug-23 13:34, Jayesh Choudhary wrote:
>> From: Rahul T R <r-ravikumar@ti.com>
>>
>> Enable display for J784S4 EVM.
>>
>> Add assigned clocks for DSS, DT node for DisplayPort PHY and pinmux for
>> DP HPD. Add the clock frequency for serdes_refclk.
>>
>> Add the endpoint nodes to describe connection from:
>> DSS => MHDP => DisplayPort connector.
>>
>> Also add the GPIO expander-4 node and pinmux for main_i2c4 which is
>> required for controlling DP power. Set status for all required nodes
>> for DP-0 as "okay".
>>
>> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
>> [j-choudhary@ti.com: move all the changes together to enable DP-0 in EVM]
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> ---
>>   arch/arm64/boot/dts/ti/k3-j784s4-evm.dts | 119 +++++++++++++++++++++++
>>   1 file changed, 119 insertions(+)

[...]

>> +		reg = <0>;
>> +		cdns,num-lanes = <4>;
>> +		#phy-cells = <0>;
>> +		cdns,phy-type = <PHY_TYPE_DP>;
>> +		resets = <&serdes_wiz4 1>, <&serdes_wiz4 2>,
>> +			 <&serdes_wiz4 3>, <&serdes_wiz4 4>;
>> +	};
>> +};
>> +
>> +&mhdp {
>> +	status = "okay";
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&dp0_pins_default>;
>> +	phys = <&serdes4_dp_link>;
>> +	phy-names = "dpphy";
>> +};
>> +
>> +&dss_ports {
>> +	port {
> 
> Port index has not been added here. Since this port outputs to MHDP
> bridge, this should be "port@0", and a "reg = <0>;" property should be
> added below (along with the address and size cells properties).
> 
> I suppose this works functionally in this case, because the port gets
> defaulted to "0" by the driver. But in future, when we add support for
> other dss output(s) on j784s4-evm, the driver will need indices to
> distinguish among them.
> 

Okay. It makes sense.
Just one thing here. Adding reg here would require it to have #address-
cells and #size-cell but since we have only single child port that too
at reg=<0>, it would throw dtbs_check warning:

arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi:1828.20-1831.5: Warning
(graph_child_address): /bus@100000/dss@4a00000/ports: graph node has
single child node 'port@0', #address-cells/#size-cells are not necessary
   also defined at arch/arm64/boot/dts/ti/k3-j784s4-evm.dts:911.12-919.3


-jayesh

>> +		dpi0_out: endpoint {
>> +			remote-endpoint = <&dp0_in>;


[...]
Aradhya Bhatia Aug. 7, 2023, 12:56 p.m. UTC | #3 
Hi Jayesh,

On 07-Aug-23 17:54, Jayesh Choudhary wrote:
> Hello Aradhya,
> 
> Thank you for the review.
> 
> On 05/08/23 00:52, Aradhya Bhatia wrote:
>> Hi Jayesh,
>>
>>
>> On 03-Aug-23 13:34, Jayesh Choudhary wrote:
>>> From: Rahul T R <r-ravikumar@ti.com>
>>>
>>> Enable display for J784S4 EVM.
>>>
>>> Add assigned clocks for DSS, DT node for DisplayPort PHY and pinmux for
>>> DP HPD. Add the clock frequency for serdes_refclk.
>>>
>>> Add the endpoint nodes to describe connection from:
>>> DSS => MHDP => DisplayPort connector.
>>>
>>> Also add the GPIO expander-4 node and pinmux for main_i2c4 which is
>>> required for controlling DP power. Set status for all required nodes
>>> for DP-0 as "okay".
>>>
>>> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
>>> [j-choudhary@ti.com: move all the changes together to enable DP-0 in
>>> EVM]
>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>> ---
>>>   arch/arm64/boot/dts/ti/k3-j784s4-evm.dts | 119 +++++++++++++++++++++++
>>>   1 file changed, 119 insertions(+)
> 
> [...]
> 
>>> +        reg = <0>;
>>> +        cdns,num-lanes = <4>;
>>> +        #phy-cells = <0>;
>>> +        cdns,phy-type = <PHY_TYPE_DP>;
>>> +        resets = <&serdes_wiz4 1>, <&serdes_wiz4 2>,
>>> +             <&serdes_wiz4 3>, <&serdes_wiz4 4>;
>>> +    };
>>> +};
>>> +
>>> +&mhdp {
>>> +    status = "okay";
>>> +    pinctrl-names = "default";
>>> +    pinctrl-0 = <&dp0_pins_default>;
>>> +    phys = <&serdes4_dp_link>;
>>> +    phy-names = "dpphy";
>>> +};
>>> +
>>> +&dss_ports {
>>> +    port {
>>
>> Port index has not been added here. Since this port outputs to MHDP
>> bridge, this should be "port@0", and a "reg = <0>;" property should be
>> added below (along with the address and size cells properties).
>>
>> I suppose this works functionally in this case, because the port gets
>> defaulted to "0" by the driver. But in future, when we add support for
>> other dss output(s) on j784s4-evm, the driver will need indices to
>> distinguish among them.
>>
> 
> Okay. It makes sense.
> Just one thing here. Adding reg here would require it to have #address-
> cells and #size-cell but since we have only single child port that too
> at reg=<0>, it would throw dtbs_check warning:
> 
> arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi:1828.20-1831.5: Warning
> (graph_child_address): /bus@100000/dss@4a00000/ports: graph node has
> single child node 'port@0', #address-cells/#size-cells are not necessary
>   also defined at arch/arm64/boot/dts/ti/k3-j784s4-evm.dts:911.12-919.3
> 

Okay! Was not aware about this. I still think "port@0" should be
specified instead of just "port" and the warning should be ignored, if
possible.

If there were only a "port@1" child node, this warning would not have
come up, and I believe "port@0" should be treated just the same.

Moreover, while we can add these properties at a later stage as an
incremental patch, adding the size and address cells in the dtsi would
affect other platform dts files as well, that use this SoC.

For e.g., the patch 5/5 of this series, on AM69-SK will still require
the size and address cells for its ports. The clean up then will be that
much more, when adding those incremental patches.

Anyway, I will let Nishanth and Vignesh take the final call on this.

Regards
Aradhya

> 
>>> +        dpi0_out: endpoint {
>>> +            remote-endpoint = <&dp0_in>;
> 
> 
> [...]
Andrew Davis Aug. 7, 2023, 3:49 p.m. UTC | #4 
On 8/7/23 7:56 AM, Aradhya Bhatia wrote:
> Hi Jayesh,
> 
> On 07-Aug-23 17:54, Jayesh Choudhary wrote:
>> Hello Aradhya,
>>
>> Thank you for the review.
>>
>> On 05/08/23 00:52, Aradhya Bhatia wrote:
>>> Hi Jayesh,
>>>
>>>
>>> On 03-Aug-23 13:34, Jayesh Choudhary wrote:
>>>> From: Rahul T R <r-ravikumar@ti.com>
>>>>
>>>> Enable display for J784S4 EVM.
>>>>
>>>> Add assigned clocks for DSS, DT node for DisplayPort PHY and pinmux for
>>>> DP HPD. Add the clock frequency for serdes_refclk.
>>>>
>>>> Add the endpoint nodes to describe connection from:
>>>> DSS => MHDP => DisplayPort connector.
>>>>
>>>> Also add the GPIO expander-4 node and pinmux for main_i2c4 which is
>>>> required for controlling DP power. Set status for all required nodes
>>>> for DP-0 as "okay".
>>>>
>>>> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
>>>> [j-choudhary@ti.com: move all the changes together to enable DP-0 in
>>>> EVM]
>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>> ---
>>>>    arch/arm64/boot/dts/ti/k3-j784s4-evm.dts | 119 +++++++++++++++++++++++
>>>>    1 file changed, 119 insertions(+)
>>
>> [...]
>>
>>>> +        reg = <0>;
>>>> +        cdns,num-lanes = <4>;
>>>> +        #phy-cells = <0>;
>>>> +        cdns,phy-type = <PHY_TYPE_DP>;
>>>> +        resets = <&serdes_wiz4 1>, <&serdes_wiz4 2>,
>>>> +             <&serdes_wiz4 3>, <&serdes_wiz4 4>;
>>>> +    };
>>>> +};
>>>> +
>>>> +&mhdp {
>>>> +    status = "okay";
>>>> +    pinctrl-names = "default";
>>>> +    pinctrl-0 = <&dp0_pins_default>;
>>>> +    phys = <&serdes4_dp_link>;
>>>> +    phy-names = "dpphy";
>>>> +};
>>>> +
>>>> +&dss_ports {
>>>> +    port {
>>>
>>> Port index has not been added here. Since this port outputs to MHDP
>>> bridge, this should be "port@0", and a "reg = <0>;" property should be
>>> added below (along with the address and size cells properties).
>>>
>>> I suppose this works functionally in this case, because the port gets
>>> defaulted to "0" by the driver. But in future, when we add support for
>>> other dss output(s) on j784s4-evm, the driver will need indices to
>>> distinguish among them.
>>>
>>
>> Okay. It makes sense.
>> Just one thing here. Adding reg here would require it to have #address-
>> cells and #size-cell but since we have only single child port that too
>> at reg=<0>, it would throw dtbs_check warning:
>>
>> arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi:1828.20-1831.5: Warning
>> (graph_child_address): /bus@100000/dss@4a00000/ports: graph node has
>> single child node 'port@0', #address-cells/#size-cells are not necessary
>>    also defined at arch/arm64/boot/dts/ti/k3-j784s4-evm.dts:911.12-919.3
>>
> 
> Okay! Was not aware about this. I still think "port@0" should be
> specified instead of just "port" and the warning should be ignored, if
> possible.
> 

Do not ignore new DT check warnings, if you go with "port@0" (which you
need to do as the "ti,j721e-dss" binding requires it) you must also add
the #address-cells/#size-cells.

Andrew

> If there were only a "port@1" child node, this warning would not have
> come up, and I believe "port@0" should be treated just the same.
> 
> Moreover, while we can add these properties at a later stage as an
> incremental patch, adding the size and address cells in the dtsi would
> affect other platform dts files as well, that use this SoC.
> 
> For e.g., the patch 5/5 of this series, on AM69-SK will still require
> the size and address cells for its ports. The clean up then will be that
> much more, when adding those incremental patches.
> 
> Anyway, I will let Nishanth and Vignesh take the final call on this.
> 
> Regards
> Aradhya
> 
>>
>>>> +        dpi0_out: endpoint {
>>>> +            remote-endpoint = <&dp0_in>;
>>
>>
>> [...]
>
Aradhya Bhatia Aug. 7, 2023, 6:29 p.m. UTC | #5 
On 07-Aug-23 21:19, Andrew Davis wrote:
> On 8/7/23 7:56 AM, Aradhya Bhatia wrote:
>> Hi Jayesh,
>>
>> On 07-Aug-23 17:54, Jayesh Choudhary wrote:
>>> Hello Aradhya,
>>>
>>> Thank you for the review.
>>>
>>> On 05/08/23 00:52, Aradhya Bhatia wrote:
>>>> Hi Jayesh,
>>>>
>>>>
>>>> On 03-Aug-23 13:34, Jayesh Choudhary wrote:
>>>>> From: Rahul T R <r-ravikumar@ti.com>
>>>>>
>>>>> Enable display for J784S4 EVM.
>>>>>
>>>>> Add assigned clocks for DSS, DT node for DisplayPort PHY and pinmux
>>>>> for
>>>>> DP HPD. Add the clock frequency for serdes_refclk.
>>>>>
>>>>> Add the endpoint nodes to describe connection from:
>>>>> DSS => MHDP => DisplayPort connector.
>>>>>
>>>>> Also add the GPIO expander-4 node and pinmux for main_i2c4 which is
>>>>> required for controlling DP power. Set status for all required nodes
>>>>> for DP-0 as "okay".
>>>>>
>>>>> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
>>>>> [j-choudhary@ti.com: move all the changes together to enable DP-0 in
>>>>> EVM]
>>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>>> ---
>>>>>    arch/arm64/boot/dts/ti/k3-j784s4-evm.dts | 119
>>>>> +++++++++++++++++++++++
>>>>>    1 file changed, 119 insertions(+)
>>>
>>> [...]
>>>
>>>>> +        reg = <0>;
>>>>> +        cdns,num-lanes = <4>;
>>>>> +        #phy-cells = <0>;
>>>>> +        cdns,phy-type = <PHY_TYPE_DP>;
>>>>> +        resets = <&serdes_wiz4 1>, <&serdes_wiz4 2>,
>>>>> +             <&serdes_wiz4 3>, <&serdes_wiz4 4>;
>>>>> +    };
>>>>> +};
>>>>> +
>>>>> +&mhdp {
>>>>> +    status = "okay";
>>>>> +    pinctrl-names = "default";
>>>>> +    pinctrl-0 = <&dp0_pins_default>;
>>>>> +    phys = <&serdes4_dp_link>;
>>>>> +    phy-names = "dpphy";
>>>>> +};
>>>>> +
>>>>> +&dss_ports {
>>>>> +    port {
>>>>
>>>> Port index has not been added here. Since this port outputs to MHDP
>>>> bridge, this should be "port@0", and a "reg = <0>;" property should be
>>>> added below (along with the address and size cells properties).
>>>>
>>>> I suppose this works functionally in this case, because the port gets
>>>> defaulted to "0" by the driver. But in future, when we add support for
>>>> other dss output(s) on j784s4-evm, the driver will need indices to
>>>> distinguish among them.
>>>>
>>>
>>> Okay. It makes sense.
>>> Just one thing here. Adding reg here would require it to have #address-
>>> cells and #size-cell but since we have only single child port that too
>>> at reg=<0>, it would throw dtbs_check warning:
>>>
>>> arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi:1828.20-1831.5: Warning
>>> (graph_child_address): /bus@100000/dss@4a00000/ports: graph node has
>>> single child node 'port@0', #address-cells/#size-cells are not necessary
>>>    also defined at arch/arm64/boot/dts/ti/k3-j784s4-evm.dts:911.12-919.3
>>>
>>
>> Okay! Was not aware about this. I still think "port@0" should be
>> specified instead of just "port" and the warning should be ignored, if
>> possible.
>>
> 
> Do not ignore new DT check warnings, if you go with "port@0" (which you
> need to do as the "ti,j721e-dss" binding requires it) you must also add
> the #address-cells/#size-cells.
> 

The warning that Jayesh mentioned above comes when "port@0" is
mentioned, *along-with* the #address-cells/#size-cells properties.
Essentially, it wants us to not use "port@0" when only single port is
being added whose reg values is 0.

This warning does not come when only a single port other than 0,
"port@1" for e.g., is being used. That's the warning, that should get
ignored, if possible.

However, just mentioning "port@0", without the #address-cells/
#size-cells, would be plain wrong.

Regards
Aradhya

> 
>> If there were only a "port@1" child node, this warning would not have
>> come up, and I believe "port@0" should be treated just the same.
>>
>> Moreover, while we can add these properties at a later stage as an
>> incremental patch, adding the size and address cells in the dtsi would
>> affect other platform dts files as well, that use this SoC.
>>
>> For e.g., the patch 5/5 of this series, on AM69-SK will still require
>> the size and address cells for its ports. The clean up then will be that
>> much more, when adding those incremental patches.
>>
>> Anyway, I will let Nishanth and Vignesh take the final call on this.
>>
>> Regards
>> Aradhya
>>
>>>
>>>>> +        dpi0_out: endpoint {
>>>>> +            remote-endpoint = <&dp0_in>;
>>>
>>>
>>> [...]
>>
Andrew Davis Aug. 7, 2023, 6:54 p.m. UTC | #6 
On 8/7/23 1:29 PM, Aradhya Bhatia wrote:
> 
> 
> On 07-Aug-23 21:19, Andrew Davis wrote:
>> On 8/7/23 7:56 AM, Aradhya Bhatia wrote:
>>> Hi Jayesh,
>>>
>>> On 07-Aug-23 17:54, Jayesh Choudhary wrote:
>>>> Hello Aradhya,
>>>>
>>>> Thank you for the review.
>>>>
>>>> On 05/08/23 00:52, Aradhya Bhatia wrote:
>>>>> Hi Jayesh,
>>>>>
>>>>>
>>>>> On 03-Aug-23 13:34, Jayesh Choudhary wrote:
>>>>>> From: Rahul T R <r-ravikumar@ti.com>
>>>>>>
>>>>>> Enable display for J784S4 EVM.
>>>>>>
>>>>>> Add assigned clocks for DSS, DT node for DisplayPort PHY and pinmux
>>>>>> for
>>>>>> DP HPD. Add the clock frequency for serdes_refclk.
>>>>>>
>>>>>> Add the endpoint nodes to describe connection from:
>>>>>> DSS => MHDP => DisplayPort connector.
>>>>>>
>>>>>> Also add the GPIO expander-4 node and pinmux for main_i2c4 which is
>>>>>> required for controlling DP power. Set status for all required nodes
>>>>>> for DP-0 as "okay".
>>>>>>
>>>>>> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
>>>>>> [j-choudhary@ti.com: move all the changes together to enable DP-0 in
>>>>>> EVM]
>>>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>>>> ---
>>>>>>     arch/arm64/boot/dts/ti/k3-j784s4-evm.dts | 119
>>>>>> +++++++++++++++++++++++
>>>>>>     1 file changed, 119 insertions(+)
>>>>
>>>> [...]
>>>>
>>>>>> +        reg = <0>;
>>>>>> +        cdns,num-lanes = <4>;
>>>>>> +        #phy-cells = <0>;
>>>>>> +        cdns,phy-type = <PHY_TYPE_DP>;
>>>>>> +        resets = <&serdes_wiz4 1>, <&serdes_wiz4 2>,
>>>>>> +             <&serdes_wiz4 3>, <&serdes_wiz4 4>;
>>>>>> +    };
>>>>>> +};
>>>>>> +
>>>>>> +&mhdp {
>>>>>> +    status = "okay";
>>>>>> +    pinctrl-names = "default";
>>>>>> +    pinctrl-0 = <&dp0_pins_default>;
>>>>>> +    phys = <&serdes4_dp_link>;
>>>>>> +    phy-names = "dpphy";
>>>>>> +};
>>>>>> +
>>>>>> +&dss_ports {
>>>>>> +    port {
>>>>>
>>>>> Port index has not been added here. Since this port outputs to MHDP
>>>>> bridge, this should be "port@0", and a "reg = <0>;" property should be
>>>>> added below (along with the address and size cells properties).
>>>>>
>>>>> I suppose this works functionally in this case, because the port gets
>>>>> defaulted to "0" by the driver. But in future, when we add support for
>>>>> other dss output(s) on j784s4-evm, the driver will need indices to
>>>>> distinguish among them.
>>>>>
>>>>
>>>> Okay. It makes sense.
>>>> Just one thing here. Adding reg here would require it to have #address-
>>>> cells and #size-cell but since we have only single child port that too
>>>> at reg=<0>, it would throw dtbs_check warning:
>>>>
>>>> arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi:1828.20-1831.5: Warning
>>>> (graph_child_address): /bus@100000/dss@4a00000/ports: graph node has
>>>> single child node 'port@0', #address-cells/#size-cells are not necessary
>>>>     also defined at arch/arm64/boot/dts/ti/k3-j784s4-evm.dts:911.12-919.3
>>>>
>>>
>>> Okay! Was not aware about this. I still think "port@0" should be
>>> specified instead of just "port" and the warning should be ignored, if
>>> possible.
>>>
>>
>> Do not ignore new DT check warnings, if you go with "port@0" (which you
>> need to do as the "ti,j721e-dss" binding requires it) you must also add
>> the #address-cells/#size-cells.
>>
> 
> The warning that Jayesh mentioned above comes when "port@0" is
> mentioned, *along-with* the #address-cells/#size-cells properties.
> Essentially, it wants us to not use "port@0" when only single port is
> being added whose reg values is 0.
> 
> This warning does not come when only a single port other than 0,
> "port@1" for e.g., is being used. That's the warning, that should get
> ignored, if possible.
> 

Ah, I see now.

Almost seems like a bug in dtc checks, but checking the code it
looks deliberate, although I cannot see why..

Rob,

Could you provide some guidance on why graph nodes are handled
this way? Seems this is valid:

ports {
	#address-cells = <1>;
	#size-cells = <0>;

	port@1 {
		reg = <1>;
	};
}

but this is not:

ports {
	#address-cells = <1>;
	#size-cells = <0>;

	port@0 {
		reg = <0>;
	};
};

I'm guessing we allow port 0 to not be numbered if it is the only
one for legacy convenience, but *forcing* it to not be numbered
when it would otherwise be more consistent seems overly strict.

Andrew

> However, just mentioning "port@0", without the #address-cells/
> #size-cells, would be plain wrong.
> 
> Regards
> Aradhya
> 
>>
>>> If there were only a "port@1" child node, this warning would not have
>>> come up, and I believe "port@0" should be treated just the same.
>>>
>>> Moreover, while we can add these properties at a later stage as an
>>> incremental patch, adding the size and address cells in the dtsi would
>>> affect other platform dts files as well, that use this SoC.
>>>
>>> For e.g., the patch 5/5 of this series, on AM69-SK will still require
>>> the size and address cells for its ports. The clean up then will be that
>>> much more, when adding those incremental patches.
>>>
>>> Anyway, I will let Nishanth and Vignesh take the final call on this.
>>>
>>> Regards
>>> Aradhya
>>>
>>>>
>>>>>> +        dpi0_out: endpoint {
>>>>>> +            remote-endpoint = <&dp0_in>;
>>>>
>>>>
>>>> [...]
>>>
>
Jayesh Choudhary Oct. 13, 2023, 5:08 a.m. UTC | #7 
On 08/08/23 00:24, Andrew Davis wrote:
> On 8/7/23 1:29 PM, Aradhya Bhatia wrote:
>>
>>
>> On 07-Aug-23 21:19, Andrew Davis wrote:
>>> On 8/7/23 7:56 AM, Aradhya Bhatia wrote:
>>>> Hi Jayesh,
>>>>
>>>> On 07-Aug-23 17:54, Jayesh Choudhary wrote:
>>>>> Hello Aradhya,
>>>>>
>>>>> Thank you for the review.
>>>>>
>>>>> On 05/08/23 00:52, Aradhya Bhatia wrote:
>>>>>> Hi Jayesh,
>>>>>>
>>>>>>
>>>>>> On 03-Aug-23 13:34, Jayesh Choudhary wrote:
>>>>>>> From: Rahul T R <r-ravikumar@ti.com>
>>>>>>>
>>>>>>> Enable display for J784S4 EVM.
>>>>>>>
>>>>>>> Add assigned clocks for DSS, DT node for DisplayPort PHY and pinmux
>>>>>>> for
>>>>>>> DP HPD. Add the clock frequency for serdes_refclk.
>>>>>>>
>>>>>>> Add the endpoint nodes to describe connection from:
>>>>>>> DSS => MHDP => DisplayPort connector.
>>>>>>>
>>>>>>> Also add the GPIO expander-4 node and pinmux for main_i2c4 which is
>>>>>>> required for controlling DP power. Set status for all required nodes
>>>>>>> for DP-0 as "okay".
>>>>>>>
>>>>>>> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
>>>>>>> [j-choudhary@ti.com: move all the changes together to enable DP-0 in
>>>>>>> EVM]
>>>>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>>>>> ---
>>>>>>>     arch/arm64/boot/dts/ti/k3-j784s4-evm.dts | 119
>>>>>>> +++++++++++++++++++++++
>>>>>>>     1 file changed, 119 insertions(+)
>>>>>
>>>>> [...]
>>>>>
>>>>>>> +        reg = <0>;
>>>>>>> +        cdns,num-lanes = <4>;
>>>>>>> +        #phy-cells = <0>;
>>>>>>> +        cdns,phy-type = <PHY_TYPE_DP>;
>>>>>>> +        resets = <&serdes_wiz4 1>, <&serdes_wiz4 2>,
>>>>>>> +             <&serdes_wiz4 3>, <&serdes_wiz4 4>;
>>>>>>> +    };
>>>>>>> +};
>>>>>>> +
>>>>>>> +&mhdp {
>>>>>>> +    status = "okay";
>>>>>>> +    pinctrl-names = "default";
>>>>>>> +    pinctrl-0 = <&dp0_pins_default>;
>>>>>>> +    phys = <&serdes4_dp_link>;
>>>>>>> +    phy-names = "dpphy";
>>>>>>> +};
>>>>>>> +
>>>>>>> +&dss_ports {
>>>>>>> +    port {
>>>>>>
>>>>>> Port index has not been added here. Since this port outputs to MHDP
>>>>>> bridge, this should be "port@0", and a "reg = <0>;" property 
>>>>>> should be
>>>>>> added below (along with the address and size cells properties).
>>>>>>
>>>>>> I suppose this works functionally in this case, because the port gets
>>>>>> defaulted to "0" by the driver. But in future, when we add support 
>>>>>> for
>>>>>> other dss output(s) on j784s4-evm, the driver will need indices to
>>>>>> distinguish among them.
>>>>>>
>>>>>
>>>>> Okay. It makes sense.
>>>>> Just one thing here. Adding reg here would require it to have 
>>>>> #address-
>>>>> cells and #size-cell but since we have only single child port that too
>>>>> at reg=<0>, it would throw dtbs_check warning:
>>>>>
>>>>> arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi:1828.20-1831.5: Warning
>>>>> (graph_child_address): /bus@100000/dss@4a00000/ports: graph node has
>>>>> single child node 'port@0', #address-cells/#size-cells are not 
>>>>> necessary
>>>>>     also defined at 
>>>>> arch/arm64/boot/dts/ti/k3-j784s4-evm.dts:911.12-919.3
>>>>>
>>>>
>>>> Okay! Was not aware about this. I still think "port@0" should be
>>>> specified instead of just "port" and the warning should be ignored, if
>>>> possible.
>>>>
>>>
>>> Do not ignore new DT check warnings, if you go with "port@0" (which you
>>> need to do as the "ti,j721e-dss" binding requires it) you must also add
>>> the #address-cells/#size-cells.
>>>
>>
>> The warning that Jayesh mentioned above comes when "port@0" is
>> mentioned, *along-with* the #address-cells/#size-cells properties.
>> Essentially, it wants us to not use "port@0" when only single port is
>> being added whose reg values is 0.
>>
>> This warning does not come when only a single port other than 0,
>> "port@1" for e.g., is being used. That's the warning, that should get
>> ignored, if possible.
>>
> 
> Ah, I see now.
> 
> Almost seems like a bug in dtc checks, but checking the code it
> looks deliberate, although I cannot see why..
> 
> Rob,
> 
> Could you provide some guidance on why graph nodes are handled
> this way? Seems this is valid:
> 
> ports {
>      #address-cells = <1>;
>      #size-cells = <0>;
> 
>      port@1 {
>          reg = <1>;
>      };
> }
> 
> but this is not:
> 
> ports {
>      #address-cells = <1>;
>      #size-cells = <0>;
> 
>      port@0 {
>          reg = <0>;
>      };
> };
> 
> I'm guessing we allow port 0 to not be numbered if it is the only
> one for legacy convenience, but *forcing* it to not be numbered
> when it would otherwise be more consistent seems overly strict.
> 
> Andrew

Hello Rob, Krzysztof,

For this series, v11 has been already reviewed by Roger and Aradhya:
<https://lore.kernel.org/all/77701023-7bd1-4e04-aa44-0e46aa087c4f@kernel.org/>

Only this warning persist. Can you ACK the series so that it can
be queued/merged.
If W=1 warning is not acceptable, I can revert to port description
here in v9.

Warm Regards,
Jayesh



> 
>> However, just mentioning "port@0", without the #address-cells/
>> #size-cells, would be plain wrong.
>>
>> Regards
>> Aradhya
>>
>>>
>>>> If there were only a "port@1" child node, this warning would not have
>>>> come up, and I believe "port@0" should be treated just the same.
>>>>
>>>> Moreover, while we can add these properties at a later stage as an
>>>> incremental patch, adding the size and address cells in the dtsi would
>>>> affect other platform dts files as well, that use this SoC.
>>>>
>>>> For e.g., the patch 5/5 of this series, on AM69-SK will still require
>>>> the size and address cells for its ports. The clean up then will be 
>>>> that
>>>> much more, when adding those incremental patches.
>>>>
>>>> Anyway, I will let Nishanth and Vignesh take the final call on this.
>>>>
>>>> Regards
>>>> Aradhya
>>>>
>>>>>
>>>>>>> +        dpi0_out: endpoint {
>>>>>>> +            remote-endpoint = <&dp0_in>;
>>>>>
>>>>>
>>>>> [...]
>>>>
>>
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts b/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts
index 7ad152a1b90f..005357d70122 100644
--- a/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts
+++ b/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts
@@ -249,6 +249,28 @@  vdd_sd_dv: regulator-TLV71033 {
 		states = <1800000 0x0>,
 			 <3300000 0x1>;
 	};
+
+	dp0_pwr_3v3: regulator-dp0-prw {
+		compatible = "regulator-fixed";
+		regulator-name = "dp0-pwr";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		gpio = <&exp4 0 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+	};
+
+	dp0: connector-dp0 {
+		compatible = "dp-connector";
+		label = "DP0";
+		type = "full-size";
+		dp-pwr-supply = <&dp0_pwr_3v3>;
+
+		port {
+			dp0_connector_in: endpoint {
+				remote-endpoint = <&dp0_out>;
+			};
+		};
+	};
 };
 
 &main_pmx0 {
@@ -286,6 +308,19 @@  vdd_sd_dv_pins_default: vdd-sd-dv-default-pins {
 			J784S4_IOPAD(0x020, PIN_INPUT, 7) /* (AJ35) MCAN15_RX.GPIO0_8 */
 		>;
 	};
+
+	dp0_pins_default: dp0-default-pins {
+		pinctrl-single,pins = <
+			J784S4_IOPAD(0x0cc, PIN_INPUT, 12) /* (AM37) SPI0_CS0.DP0_HPD */
+		>;
+	};
+
+	main_i2c4_pins_default: main-i2c4-default-pins {
+		pinctrl-single,pins = <
+			J784S4_IOPAD(0x014, PIN_INPUT_PULLUP, 8) /* (AG33) MCAN14_TX.I2C4_SCL */
+			J784S4_IOPAD(0x010, PIN_INPUT_PULLUP, 8) /* (AH33) MCAN13_RX.I2C4_SDA */
+		>;
+	};
 };
 
 &wkup_pmx2 {
@@ -827,3 +862,87 @@  adc {
 		ti,adc-channels = <0 1 2 3 4 5 6 7>;
 	};
 };
+
+&serdes_refclk {
+	status = "okay";
+	clock-frequency = <100000000>;
+};
+
+&dss {
+	status = "okay";
+	assigned-clocks = <&k3_clks 218 2>,
+			  <&k3_clks 218 5>,
+			  <&k3_clks 218 14>,
+			  <&k3_clks 218 18>;
+	assigned-clock-parents = <&k3_clks 218 3>,
+				 <&k3_clks 218 7>,
+				 <&k3_clks 218 16>,
+				 <&k3_clks 218 22>;
+};
+
+&serdes_wiz4 {
+	status = "okay";
+};
+
+&serdes4 {
+	status = "okay";
+	serdes4_dp_link: phy@0 {
+		reg = <0>;
+		cdns,num-lanes = <4>;
+		#phy-cells = <0>;
+		cdns,phy-type = <PHY_TYPE_DP>;
+		resets = <&serdes_wiz4 1>, <&serdes_wiz4 2>,
+			 <&serdes_wiz4 3>, <&serdes_wiz4 4>;
+	};
+};
+
+&mhdp {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&dp0_pins_default>;
+	phys = <&serdes4_dp_link>;
+	phy-names = "dpphy";
+};
+
+&dss_ports {
+	port {
+		dpi0_out: endpoint {
+			remote-endpoint = <&dp0_in>;
+		};
+	};
+};
+
+&main_i2c4 {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&main_i2c4_pins_default>;
+	clock-frequency = <400000>;
+
+	exp4: gpio@20 {
+		compatible = "ti,tca6408";
+		reg = <0x20>;
+		gpio-controller;
+		#gpio-cells = <2>;
+	};
+};
+
+&dp0_ports {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	port@0 {
+		reg = <0>;
+
+		dp0_in: endpoint {
+			remote-endpoint = <&dpi0_out>;
+		};
+	};
+
+	port@4 {
+		reg = <4>;
+
+		dp0_out: endpoint {
+			remote-endpoint = <&dp0_connector_in>;
+		};
+	};
+};