| Message ID | 20230803080441.367341-1-j-choudhary@ti.com |
|---|---|
| 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>; > + }; > + }; > +};
On 03-Aug-23 13:34, Jayesh Choudhary wrote: > From: Rahul T R <r-ravikumar@ti.com> > > Add DSS and DP-bridge node for J784S4 SoC. DSS IP in J784S4 is > same as DSS IP in J721E, so same compatible is being used. > The DP is Cadence MHDP8546. DP-bridge > > Signed-off-by: Rahul T R <r-ravikumar@ti.com> > [j-choudhary@ti.com: move dss & mhdp node together in main, fix dss node] > Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com> Given that you make appropriate changes with properties in this patch, wrt patches 4/5 and 5/5, Reviewed-by: Aradhya Bhatia <a-bhatia1@ti.com> > --- > arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi | 63 ++++++++++++++++++++++ > 1 file changed, 63 insertions(+) > > diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi > index 446d7efa715f..824312b9ef9f 100644 > --- a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi > +++ b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi > @@ -1741,4 +1741,67 @@ c71_3: dsp@67800000 { > resets = <&k3_reset 40 1>; > firmware-name = "j784s4-c71_3-fw"; > }; > + > + mhdp: bridge@a000000 { > + compatible = "ti,j721e-mhdp8546"; > + reg = <0x0 0xa000000 0x0 0x30a00>, > + <0x0 0x4f40000 0x0 0x20>; > + reg-names = "mhdptx", "j721e-intg"; > + clocks = <&k3_clks 217 11>; > + interrupt-parent = <&gic500>; > + interrupts = <GIC_SPI 614 IRQ_TYPE_LEVEL_HIGH>; > + power-domains = <&k3_pds 217 TI_SCI_PD_EXCLUSIVE>; > + status = "disabled"; > + > + dp0_ports: ports { > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + }; > + > + dss: dss@4a00000 { > + compatible = "ti,j721e-dss"; > + reg = <0x00 0x04a00000 0x00 0x10000>, /* common_m */ > + <0x00 0x04a10000 0x00 0x10000>, /* common_s0*/ > + <0x00 0x04b00000 0x00 0x10000>, /* common_s1*/ > + <0x00 0x04b10000 0x00 0x10000>, /* common_s2*/ > + <0x00 0x04a20000 0x00 0x10000>, /* vidl1 */ > + <0x00 0x04a30000 0x00 0x10000>, /* vidl2 */ > + <0x00 0x04a50000 0x00 0x10000>, /* vid1 */ > + <0x00 0x04a60000 0x00 0x10000>, /* vid2 */ > + <0x00 0x04a70000 0x00 0x10000>, /* ovr1 */ > + <0x00 0x04a90000 0x00 0x10000>, /* ovr2 */ > + <0x00 0x04ab0000 0x00 0x10000>, /* ovr3 */ > + <0x00 0x04ad0000 0x00 0x10000>, /* ovr4 */ > + <0x00 0x04a80000 0x00 0x10000>, /* vp1 */ > + <0x00 0x04aa0000 0x00 0x10000>, /* vp1 */ > + <0x00 0x04ac0000 0x00 0x10000>, /* vp1 */ > + <0x00 0x04ae0000 0x00 0x10000>, /* vp4 */ > + <0x00 0x04af0000 0x00 0x10000>; /* wb */ > + reg-names = "common_m", "common_s0", > + "common_s1", "common_s2", > + "vidl1", "vidl2","vid1","vid2", > + "ovr1", "ovr2", "ovr3", "ovr4", > + "vp1", "vp2", "vp3", "vp4", > + "wb"; > + clocks = <&k3_clks 218 0>, > + <&k3_clks 218 2>, > + <&k3_clks 218 5>, > + <&k3_clks 218 14>, > + <&k3_clks 218 18>; > + clock-names = "fck", "vp1", "vp2", "vp3", "vp4"; > + power-domains = <&k3_pds 218 TI_SCI_PD_EXCLUSIVE>; > + interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "common_m", > + "common_s0", > + "common_s1", > + "common_s2"; > + status = "disabled"; > + > + dss_ports: ports { > + }; > + }; > };
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>; [...]
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>; >>>>> >>>>> >>>>> [...] >>>> >>
This series adds support for: - DisplayPort for J784S4-EVM - Displayport and HDMI for AM69-SK platform Changelog v8->v9: - Fix compatible of serdes_ln_ctrl node - Fix extra new lines across nodes - Fix node-names to keep them generic Changelog v7->v8: - rebase on tag next-20230731 - add AM69 display support - fix commit heading for patch [2/5] Changelog v6->v7: - change compatible for scm_conf to 'simple-bus' - drop main_cpsw node due to driver dependency on [2] Changelog v5->v6: - Change header file according to [1]. - Add idle-state property in serdes_ln_ctrl node. - Fix dtbs_check warning due to clock-frequency property in serdes_refclk node by disabling the node in main.dtsi and enabling it in board file when the clock-frequency node is actually added. Changelog v4->v5: - rebased the patches on linux-next tip. Changelog v3->v4: - add reg property to serdes_ln_ctrl and fix the node name again to get rid of dtbs_check error. - reorder reg, reg-names and ranges property for main_cpsw1. - correct the order for clocks in serdes_wiz nodes to fix dtbs_check warnings. - fix indentation in reg, reg-names and clock property for dss node. - add comments for the reg type in dss registers. Changelog v3->v2: - fix dtc warnings for 'scm_conf' and 'serdes_ln_ctrl' nodes (Checked all the changes of the series with W=12 option during build) - added clock-frequency for serdes_refclk along with other EVM changes This refclk is being used by all the instances of serdes_wiz which are disabled by default. So configuring refclk when the serdes nodes are used for the first time is okay. Changelog v1->v2: - Moved J784S4 EVM changes together to the last patch (Suggested by Andrew) v8 patch link: <https://lore.kernel.org/all/20230801070019.219660-1-j-choudhary@ti.com/> [1]: <https://lore.kernel.org/all/20230721125732.122421-1-j-choudhary@ti.com/> [2]: <https://lore.kernel.org/all/20230605154153.24025-1-afd@ti.com/> Dasnavis Sabiya (1): arm64: dts: ti: k3-am69-sk: Add DP and HDMI support Rahul T R (2): arm64: dts: ti: k3-j784s4-main: Add DSS and DP-bridge node arm64: dts: ti: k3-j784s4-evm: Enable DisplayPort-0 Siddharth Vadapalli (2): arm64: dts: ti: k3-j784s4-main: Add system controller and SERDES lane mux arm64: dts: ti: k3-j784s4-main: Add WIZ and SERDES PHY nodes arch/arm64/boot/dts/ti/k3-am69-sk.dts | 234 ++++++++++++++++++ arch/arm64/boot/dts/ti/k3-j784s4-evm.dts | 119 +++++++++ arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi | 267 +++++++++++++++++++++ 3 files changed, 620 insertions(+)