Re: [PATCH v3 net-next 1/2] arm64: dts: ti: Add PLL1 refclk to J784S4 SoC SERDES node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] Adding a third clock to the 'serdes2' node violates the 'phy-cadence-torrent.yaml' DT binding. The subsequent binding update in the series is logically incomplete and fails to resolve the validation error.
- [Low] Missing whitespace after the comma in the 'clock-names' list.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the commit fails to comprehensively fix the issue. It adds 'pll1_refclk' to fix multilink configuration for 'serdes2', but ignores identical SERDES nodes ('serdes0', 'serdes1', 'serdes4') that share the exact same bug.
--

commit c37d528887d49433e9d33970e77ad622c557ad5e
Author: Gokul Praveen <g-praveen@ti.com>

arm64: dts: ti: Add PLL1 refclk to J784S4 SoC SERDES node

This commit adds the PLL1 refclk to the clocks and clock-names parameters
for the SERDES2 node. It addresses a multilink configuration failure that
occurs without PLL1 being present in the clocks list.

> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
> index 78fcd0c40abcf..da8d582574d03 100644
> --- a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
> @@ -122,8 +122,9 @@ serdes2: serdes@5020000 {
>  			resets = <&serdes_wiz2 0>;
>  			reset-names = "torrent_reset";
>  			clocks = <&serdes_wiz2 TI_WIZ_PLL0_REFCLK>,
> +				 <&serdes_wiz2 TI_WIZ_PLL1_REFCLK>,
>  				 <&serdes_wiz2 TI_WIZ_PHY_EN_REFCLK>;

[Severity: High]
This is a pre-existing issue, but does the same fix need to be applied to
the other identical serdes nodes?

Other nodes like serdes0, serdes1, and serdes4 in
arch/arm64/boot/dts/ti/k3-j784s4-j742s2-main-common.dtsi share the exact same
configuration issue where TI_WIZ_PLL1_REFCLK is in assigned-clocks but absent
from clocks:

			clocks = <&serdes_wiz0 TI_WIZ_PLL0_REFCLK>,
				 <&serdes_wiz0 TI_WIZ_PHY_EN_REFCLK>;
			clock-names = "refclk", "phy_en_refclk";
			assigned-clocks = <&serdes_wiz0 TI_WIZ_PLL0_REFCLK>,
					  <&serdes_wiz0 TI_WIZ_PLL1_REFCLK>,
					  <&serdes_wiz0 TI_WIZ_REFCLK_DIG>;

Since serdes0 is also configured for multilink (PCIe + USB), will its
configuration fail without a similar update?

> -			clock-names = "refclk", "phy_en_refclk";
> +			clock-names = "refclk","pll1_refclk", "phy_en_refclk";

[Severity: Low]
Will this addition cause make dtbs_check to fail?

Adding a third item to clock-names exceeds the currently defined items in the
phy-cadence-torrent.yaml binding.

Even if a subsequent patch in the series updates the schema by modifying
maxItems and the enum for the second item, dt-schema enforces
additionalItems: false for arrays.

Unless a third item definition is explicitly added to the items array in the
schema, won't validation fail for this node?

[Severity: Low]
this isn't a bug, but there is a missing space after the first comma.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630134324.61085-1-g-praveen@ti.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy