Re: [PATCH v2 3/3] arm64: dts: ti: k3-j784s4: Add overlay for dual port USXGMII+SGMII mode

[Gokul Praveen <g-praveen@ti.com>]

Hi,

On 29/06/26 16:04, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] Array length mismatch between `assigned-clocks` and `assigned-clock-parents` in `&serdes_wiz2`.
> - [Low] Mismatch between node unit-address and 'reg' property in SerDes PHY links.
> - [Low] Dangling continuation backslash on the last item of a Makefile list immediately preceding a comment.
> --
>
> commit bc5648306dcc07a1bc8095cecaf3da3724e52d5d
> Author: Gokul Praveen <g-praveen@ti.com>
>
> arm64: dts: ti: k3-j784s4: Add overlay for dual port USXGMII+SGMII mode
>
> The CPSW9G instance of the CPSW Ethernet Switch supports SGMII and
> USXGMII mode with MAC Ports 1 and 2 of the instance, which are
> connected to ENET Expansion 1 and ENET Expansion 2 slots on
> the EVM respectively, through the Serdes2 instance of the SERDES.
> Enable CPSW9G MAC Ports 1 and 2 in fixed-link configuration
> SGMII and USXGMII mode at 1Gbps and 5 Gbps each.
>
>> diff --git a/arch/arm64/boot/dts/ti/Makefile b/arch/arm64/boot/dts/ti/Makefile
>> index 5269c9619b65c..72c91f8f7814d 100644
>> --- a/arch/arm64/boot/dts/ti/Makefile
>> +++ b/arch/arm64/boot/dts/ti/Makefile
> [ ... ]
>> @@ -337,7 +343,8 @@ dtb- += k3-am625-beagleplay-csi2-ov5640.dtb \
>>   	k3-j784s4-evm-pcie0-pcie1-ep.dtb \
>>   	k3-j784s4-evm-quad-port-eth-exp1.dtb \
>>   	k3-j784s4-evm-usb0-type-a.dtb \
>> -	k3-j784s4-evm-usxgmii-exp1-exp2.dtb
>> -
>> +	k3-j784s4-evm-usxgmii-exp1-exp2.dtb \
>> +	k3-j784s4-evm-usxgmii-sgmii-exp1-exp2_board2.dtb \
>> +	k3-j784s4-evm-sgmii-usxgmii-exp1-exp2_board1.dtb \
>>   # Enable support for device-tree overlays
>>   DTC_FLAGS := -@
> [Severity: Low]
> Does this trailing backslash escape the newline before the comment? Since
> Make processes continuations before comments, this merges the comment onto
> the dtb- line. If the comment is ever removed, DTC_FLAGS := -@ would be
> mistakenly pulled into the dtb- variable.
>
>> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-evm-sgmii-usxgmii-exp1-exp2_board1.dtso b/arch/arm64/boot/dts/ti/k3-j784s4-evm-sgmii-usxgmii-exp1-exp2_board1.dtso
>> new file mode 100644
>> index 0000000000000..4ceb99c5c8088
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-evm-sgmii-usxgmii-exp1-exp2_board1.dtso
> [ ... ]
>> +&serdes_wiz2 {
>> +	assigned-clocks = <&k3_clks 406 6>,  <&k3_clks 406 5>;
>> +	status = "okay";
>> +};
> [Severity: Medium]
> Will this 2-element assigned-clocks array work correctly without overriding
> the assigned-clock-parents array as well? Since assigned-clock-parents has
> only 1 element in the base device tree, the clock framework evaluates
> num_parents based on that length and ignores the second clock here, leaving
> it unparented.
>
> [ ... ]

The clk 5 does not have a parent and only clk 6 does.

Hence, only one assigned-clock-parents is needed.

Also both clk 5 and clk 6 works considere which has been working

and tested.

>> +&serdes2 {
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +	status = "okay";
>> +
>> +	assigned-clock-parents = <&k3_clks 406 5>,
>> +						 <&k3_clks 406 6>,
>> +						 <&k3_clks 406 5>;
>> +
>> +	serdes2_sgmii2_link: phy@1 {
>> +		reg = <2>;
>> +		cdns,num-lanes = <1>;
>> +		#phy-cells = <0>;
>> +		cdns,phy-type = <PHY_TYPE_SGMII>;
>> +		resets = <&serdes_wiz2 3>;
>> +	};
> [Severity: Low]
> Does this node name phy@1 violate the standard device tree schema when the
> reg property is set to 2? This mismatch will trigger a unit_address_vs_reg
> dtc compiler warning.

No , I dont think that matters at all, as the the cpsw driver does not 
relate the

reg property with the unit address.

>> +	serdes2_usxgmii_link: phy@0 {
>> +		reg = <3>;
>> +		cdns,num-lanes = <1>;
>> +		#phy-cells = <0>;
>> +		cdns,phy-type = <PHY_TYPE_USXGMII>;
>> +		resets = <&serdes_wiz2 4>;
>> +	};
> [Severity: Low]
> Similarly here, shouldn't the unit-address in phy@0 match the reg property
> of 3?

No , I dont think that matters at all, as the the cpsw driver does not 
relate the

reg property with the unit address.

>> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-evm-usxgmii-sgmii-exp1-exp2_board2.dtso b/arch/arm64/boot/dts/ti/k3-j784s4-evm-usxgmii-sgmii-exp1-exp2_board2.dtso
>> new file mode 100644
>> index 0000000000000..ef4ebb5515469
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-evm-usxgmii-sgmii-exp1-exp2_board2.dtso
> [ ... ]
>> +&serdes_wiz2 {
>> +	assigned-clocks = <&k3_clks 406 6>,  <&k3_clks 406 5>;
>> +	status = "okay";
>> +};
> [Severity: Medium]
> Like in board1, will this assigned-clocks override fail to assign the second
> clock because it doesn't also update assigned-clock-parents to be a
> 2-element array?
>
> [ ... ]

The clk 5 does not have a parent and only clk 6 does.

Hence, only one assigned-clock-parents is needed.

Also both clk 5 and clk 6 works considere which has been working

and tested.

>> +	serdes2_sgmii2_link: phy@1 {
>> +		reg = <3>;
>> +		cdns,num-lanes = <1>;
>> +		#phy-cells = <0>;
>> +		cdns,phy-type = <PHY_TYPE_SGMII>;
>> +		resets = <&serdes_wiz2 4>;
>> +	};
> [Severity: Low]
> Does this unit-address phy@1 need to be updated to phy@3 to match the reg
> property?

No , I dont think that matters at all, as the the cpsw driver does not 
relate the

reg property with the unit address.

>> +	serdes2_usxgmii_link: phy@0 {
>> +		reg = <2>;
>> +		cdns,num-lanes = <1>;
>> +		#phy-cells = <0>;
>> +		cdns,phy-type = <PHY_TYPE_USXGMII>;
>> +		resets = <&serdes_wiz2 3>;
>> +	};
> [Severity: Low]
> Does this unit-address phy@0 need to be updated to phy@2 to match the reg
> property?
>
No , I dont think that matters at all, as the the cpsw driver does not 
relate the

reg property with the unit address.

Regards

Gokul Praveen