Messages in this thread | | | Date | Fri, 13 Oct 2023 10:38:42 +0530 | Subject | Re: [PATCH v9 4/5] arm64: dts: ti: k3-j784s4-evm: Enable DisplayPort-0 | From | Jayesh Choudhary <> |
| |
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>; >>>>> >>>>> >>>>> [...] >>>> >>
| |