Re: [PATCH net-next v12 2/6] dt-bindings: net: pcs: add rockchip,rk3568-xpcs support

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new binding
  Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml…

--

dt-bindings: net: pcs: add rockchip,rk3568-xpcs support

This patch adds a device tree binding for the Synopsys DesignWare XPCS on
the Rockchip RK3568 SoC. The XPCS sits on the APB3 bus and connects
internally to the Naneng Combo SerDes PHY. It supports 1000BASE-X, SGMII
and QSGMII.

> diff --git a/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml b/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
> new file mode 100644
> index 0000000000000..a331880eb11c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml

[ ... ]

> +properties:
> +  compatible:
> +    const: rockchip,rk3568-xpcs
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: APB3 bus interface clock (clk_csr_i), required for register access
> +      - description: EEE clock (clk_eee_i), required for Energy Efficient Ethernet operation
> +
> +  clock-names:
> +    items:
> +      - const: csr
> +      - const: eee
> +
> +  phys:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - phys
> +  - power-domains
> +
> +additionalProperties: false

[Severity: Low]
Should this binding let the XPCS act as a clock provider?

The RK3568 CRU uses XPCS-named clocks as parents for the GMAC RX/TX
muxes, in drivers/clk/rockchip/clk-rk3568.c:

PNAME(mux_gmac0_rx_tx_p) = { "clk_gmac0_rgmii_speed", "clk_gmac0_rmii_speed", "clk_gmac0_xpcs_mii" };
PNAME(mux_gmac1_rx_tx_p) = { "clk_gmac1_rgmii_speed", "clk_gmac1_rmii_speed", "clk_gmac1_xpcs_mii" };

These are the parents of SCLK_GMAC0_RX_TX and SCLK_GMAC1_RX_TX.
rk356x-base.dtsi passes those clocks to the GMAC as mac_clk_rx,
mac_clk_tx and clk_mac_speed. Nothing in the tree registers
clk_gmac0_xpcs_mii or clk_gmac1_xpcs_mii today.

For SGMII on GMAC0/GMAC1, a board DT would need assigned-clock-parents
to point SCLK_GMACn_RX_TX at the XPCS MII clock. This schema has no
#clock-cells or clock-output-names, and additionalProperties: false
means dt_binding_check rejects them. So the DT can't name the XPCS node
as the source of that clock. Boards would have to use a fixed-clock
placeholder carrying the CRU's string name instead.

Documentation/devicetree/bindings/writing-bindings.rst says "DO attempt
to make bindings complete even if a driver doesn't support some
features". The commit message explains why the SRST_XPCS* resets are
left out but says nothing about the MII clock outputs.

Could #clock-cells (and clock-output-names, if needed) be added for the
per-port MII clocks? Or could the commit message say why they are left
out?

> +
> +examples:

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017054406.1828061-1-coiaprant%40gmail.com