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

[Aradhya Bhatia]

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@xxxxxx>
>>>
>>> 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@xxxxxx>
>>> [j-choudhary@xxxxxx: move all the changes together to enable DP-0 in
>>> EVM]
>>> Signed-off-by: Jayesh Choudhary <j-choudhary@xxxxxx>
>>> ---
>>>   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>;
> 
> 
> [...]