Re: [PATCH net-next v4 09/10] arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port

This enables the GMAC0 SGMII LAN port on the Ariaboard Photonicat by
adding the xpcs/xpcs_mii0 overrides, the combphy2 SGMII MAC selection, and
an mdio0 node with the YT8521SC PHY.  It also adds PHY LED descriptions on
both mdio0 and mdio1.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> index 58c1052ba8ef3..25caa44198843 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts

[ ... ]

> @@ -242,6 +243,7 @@ &combphy1 {
>  
>  &combphy2 {
>  	status = "okay";
> +	rockchip,sgmii-mac-sel = <0>;
>  };

[Severity: Low]
The changelog says:

  "Enable combphy2 with rockchip,sgmii-mac-sel = <0> to route the SGMII
   interface to GMAC0."

Is that description accurate?  In the baseline the node already reads:

  &combphy2 {
          status = "okay";
  };

so the only new thing here is the rockchip,sgmii-mac-sel property.  Could
the wording be adjusted to say the property is added to an already enabled
combphy2?

[Severity: Low]
This isn't a bug, but the new properties do not follow the ordering used by
the rest of this file, where properties come roughly alphabetically and
status is last.  Here rockchip,sgmii-mac-sel lands after status, in &gmac0
below pcs-handle comes before managed, and in the new &xpcs override status
is first with phys/phy-names after it.  Compare the existing &gmac1
override, which ends with status = "okay";.  Could these be reordered?

>  
>  &cpu0 {
> @@ -260,9 +262,18 @@ &cpu3 {
>  	cpu-supply = <&vdd_cpu>;
>  };
>  
> -/* Motorcomm YT8521SC LAN port (require SGMII) */
> +/* Motorcomm YT8521SC LAN port */
>  &gmac0 {
> -	status = "disabled";
> +	assigned-clocks = <&cru SCLK_GMAC0_RX_TX>;
> +	assigned-clock-parents = <&xpcs_gmac0_clk>;
> +	pcs-handle = <&xpcs_mii0>;
> +	managed = "in-band-status";
> +	phy-handle = <&sgmii_phy>;
> +	phy-mode = "sgmii";
> +	phy-supply = <&vcc_3v3>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&gmac0_miim>;
> +	status = "okay";
>  };

[ ... ]

> @@ -350,6 +391,25 @@ rgmii_phy: ethernet-phy@3 {
>  		reset-gpios = <&gpio4 RK_PC0 GPIO_ACTIVE_LOW>;
>  		rx-internal-delay-ps = <1500>;
>  		tx-internal-delay-ps = <1500>;
> +
> +		leds {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			led@1 {
> +				reg = <1>;
> +				color = <LED_COLOR_ID_AMBER>;
> +				function = LED_FUNCTION_WAN;
> +				default-state = "keep";
> +			};
> +
> +			led@2 {
> +				reg = <2>;
> +				color = <LED_COLOR_ID_GREEN>;
> +				function = LED_FUNCTION_WAN;
> +				default-state = "keep";
> +			};
> +		};
>  	};
>  };

[Severity: Low]
This hunk describes the LEDs of the pre-existing RGMII WAN PHY on &mdio1,
which is already functional and unaffected by the SGMII enablement.  The
changelog justifies it only with:

  "Also add LED configuration for the existing RGMII PHY on mdio1 for
   consistency."

Would it be preferable to split this into its own patch, so this commit
contains only the SGMII LAN port enablement?

[ ... ]

> @@ -586,3 +646,13 @@ &xin32k {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&clk32k_out1>;
>  };
> +
> +&xpcs {
> +	status = "okay";
> +	phys = <&combphy2 PHY_TYPE_SGMII>;
> +	phy-names = "serdes";
> +};

[Severity: Medium]
Should phys/phy-names live in rk3568.dtsi rather than in each board file?

The binding lists them as required properties of the XPCS node:

  Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml
  required:
    - compatible
    - reg
    - "#address-cells"
    - "#size-cells"
    - clocks
    - clock-names
    - phys
    - phy-names
    - power-domains

but the SoC node in arch/arm64/boot/dts/rockchip/rk3568.dtsi has neither:

  xpcs: pcs@fda00000 {
          compatible = "rockchip,rk3568-xpcs";
          #address-cells = <1>;
          #size-cells = <0>;
          reg = <0x0 0xfda00000 0x0 0x200000>;
          clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
          clock-names = "csr", "eee";
          power-domains = <&power RK3568_PD_PIPE>;
          status = "disabled";

Does this mean every rk3568 dtb reports pcs@fda00000 as missing the required
phys and phy-names under make CHECK_DTBS=y, including boards that do not
enable the XPCS?

The XPCS to combphy2 connection is internal to the SoC, so each board that
enables it will have to repeat the same phandle, as this patch does.  Could
phys = <&combphy2 PHY_TYPE_SGMII>; and phy-names = "serdes"; be moved into
the dtsi node instead?

> +
> +&xpcs_mii0 {
> +	status = "okay";
> +};