Re: [RFC PATCH 05/10] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Devicetree anti-pattern: dummy fixed-clock nodes used for internal SoC clocks and left unassigned.
- [Low] State mismatch: fixed-clock nodes are enabled by default despite commit message claims.
--

--- Patch [5]: [RFC PATCH 05/10] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes ---
commit b47e1fcebd43374d6431fc20ffa64a0397513d68
Author: Coia Prant <coiaprant@gmail.com>

arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes

The RK3568 SoC integrates a Synopsys DesignWare XPCS that provides
the Physical Coding Sublayer for 1000BASE-X, SGMII, and QSGMII
interfaces via its four MII ports. Add the XPCS device node and
its pcs-mii sub-nodes to the SoC device tree.

> Also add two fixed-clock nodes (xpcs_gmac0_clk and xpcs_gmac1_clk)
> providing the 125 MHz reference clock for the GMACs when operating
> with XPCS.  These clocks are used as the assigned-clock-parents
> for the respective GMAC nodes.

[Severity: Low]
The commit message states these clocks are used as assigned-clock-parents
for the respective GMAC nodes, but the patch doesn't appear to add the
assigned-clock-parents properties to the gmac0 or gmac1 nodes. Are these
assignments missing?

> diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> index 3bc653f027f1f..989e164c0eb39 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi

[ ... ]

> +	xpcs_gmac0_clk: xpcs-gmac0-clock {
> +		compatible = "fixed-clock";
> +		clock-frequency = <125000000>;
> +		clock-output-names = "clk_gmac0_xpcs_mii";
> +		#clock-cells = <0>;
> +	};

[Severity: Low]
Should this internal SoC clock be modeled as a clock provider from the
generating XPCS IP node using #clock-cells, rather than using a dummy
fixed-clock?

Additionally, the commit message states "All nodes are left disabled by
default", but this node is missing the status = "disabled" property. Will
this leave the clock unconditionally enabled?

> +
> +	xpcs_gmac1_clk: xpcs-gmac1-clock {
> +		compatible = "fixed-clock";
> +		clock-frequency = <125000000>;
> +		clock-output-names = "clk_gmac1_xpcs_mii";
> +		#clock-cells = <0>;
> +	};

[Severity: Low]
Similarly, does this node also need a status = "disabled" property to align
with the behavior described in the commit message?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=5

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