| Message ID | 20230803080441.367341-5-j-choudhary@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable Display for J784S4 and AM69-SK platform | expand |
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>; > + }; > + }; > +};
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>; [...]
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>; > > > [...]
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>; >> >> >> [...] >
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>; >>> >>> >>> [...] >>
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>; >>>> >>>> >>>> [...] >>> >
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 --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>; + }; + }; +};