Re: [PATCH net-next v4 04/10] dt-bindings: net: pcs: add rockchip,rk3568-xpcs support

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
dt-bindings: net: pcs: add rockchip,rk3568-xpcs support

This adds a new standalone schema describing the Synopsys DesignWare XPCS
as integrated on the Rockchip RK3568, with a CSR window, csr/eee clocks, a
Naneng Combo SerDes phy, a power domain and four MII child ports.  A few
questions about the property set and the file naming below.

> diff --git a/Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml b/Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml
> new file mode 100644
> index 0000000000000..0852d0bcb66a2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml
> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/pcs/rockchip-dwxpcs.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#

[Severity: Low]

This isn't a bug, but should the file be named after the documented
compatible, i.e. rockchip,rk3568-xpcs.yaml?  The other schemas in
Documentation/devicetree/bindings/net/pcs/ all use that form:
snps,dw-xpcs.yaml, fsl,lynx-pcs.yaml, mediatek,sgmiisys.yaml,
renesas,rzn1-miic.yaml.

A rename would also have to update the $id line above, since it encodes the
same name.

Related: the example at the bottom uses pcs@fda00000 and pcs-mii@0, while
the sibling PCS binding uses the generic ethernet-pcs@... node name, and no
$nodename constraint is added here.  The SoC dtsi added later in the series
copies the pcs@/pcs-mii@ names verbatim.

> +
> +title: Rockchip RK3568 Synopsys DesignWare Ethernet PCS

[ ... ]

> +properties:
> +  compatible:
> +    const: rockchip,rk3568-xpcs

[Severity: Low]

This isn't a bug, but would a set of SoC-specific if/then blocks in
Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml be preferred
over a second schema for the same IP?

To be clear, a snps,dw-xpcs fallback compatible or a $ref to that schema
would be wrong here.  The glue driver added later in the series builds CSR
addresses with only a 3-bit MMD field:

	return FIELD_PREP(0x70000, dev) | FIELD_PREP(0xffff, reg);

and remaps the vendor MMDs in xpcs_rk_mdio_read_remapping() /
xpcs_rk_mdio_write_remapping(), so the generic direct mapping described in
snps,dw-xpcs.yaml would compute different offsets.

So the only question is where the description should live, not whether the
compatible should be shared.

> +
> +  reg:
> +    maxItems: 1

[Severity: Low]

Should the sbd_intr_o output be described as an optional interrupts
property?  With additionalProperties: false at the end of the file, a
conforming device tree cannot carry it, although adding it later stays
backward compatible.

The single unnamed 2 MiB window itself does match the driver:
xpcs_rk_init_res() uses platform_get_resource(pdev, IORESOURCE_MEM, 0) and
requires resource_size(res) >= SZ_2M, and the largest computed offset
0x7FFFF << 2 = 0x1FFFFC still falls inside 0x200000, so reg-names and
reg-io-width are not needed.

> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  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
> +
> +  phy-names:
> +    const: serdes
> +
> +  power-domains:
> +    maxItems: 1

[Severity: Low]

Should resets/reset-names be described here too?  The RK3568 CRU exposes
four dedicated reset lines for this block in
include/dt-bindings/clock/rk3568-cru.h:

	#define SRST_XPCS		150
	#define SRST_XPCS_TX_DIV10	151
	#define SRST_XPCS_RX_DIV10	152
	#define SRST_XPCS_XGXS_RX	153

Since the schema ends with additionalProperties: false, a conforming device
tree cannot express them at all, and the glue driver added later in the
series never requests a reset control, so it depends on those resets already
being de-asserted.

Peer blocks in the same island do describe theirs, e.g. combphy0 with
resets = <&cru SRST_PIPEPHY0> and gmac0 with resets = <&cru SRST_A_GMAC0>.

Note the XPCS core does provide a register-level reset via xpcs_soft_reset()
in drivers/net/pcs/pcs-xpcs.c, so a state-machine restart is possible
without the CRU resets, and an optional resets property can be added later
without breaking the ABI.

> +
> +patternProperties:
> +  "^pcs-mii@[0-3]$":
> +    type: object
> +    description:
> +      One of the four MII ports of the XPCS. The port is linked to an
> +      Ethernet MAC controller via the pcs-handle property in the MAC's
> +      device tree node.
> +
> +    properties:
> +      reg:
> +        description: MII port number.
> +        enum: [0, 1, 2, 3]

[Severity: Low]

This isn't a bug, but is pcs-mii@N the intended name for these per-port
children rather than the generic ethernet-pcs@N?

Resolving a MAC pcs-handle that points at one of these children does work
with the rest of the series: xpcs_rk_create() in
drivers/net/pcs/pcs-xpcs-rk.c reads the child reg, walks up with
of_get_parent() and calls xpcs_create_mdiodev(pxpcs->bus, port), and
rk_pcs_init() in drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c does
of_parse_phandle(np, "pcs-handle", 0).  So the open point is just the node
naming.

> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +  - clocks
> +  - clock-names
> +  - phys
> +  - phy-names
> +  - power-domains

[Severity: Medium]

Does requiring phys and phy-names here make dtbs_check fail for the RK3568
boards?  The SoC node added later in the same series omits both, because the
SerDes selection is board specific:

arch/arm64/boot/dts/rockchip/rk3568.dtsi:
	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";

and only the board fills them in:

arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts:
	&xpcs {
		status = "okay";
		phys = <&combphy2 PHY_TYPE_SGMII>;
		phy-names = "serdes";
	};

dtschema validates nodes irrespective of status = "disabled", so wouldn't
every rk3568*.dtb apart from the Photonicat report "'phys', 'phy-names' are
required properties"?

Should phys/phy-names be dropped from required (or made conditional), or
should the SoC dtsi carry them instead?

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/rk3568-cru.h>
> +    #include <dt-bindings/power/rk3568-power.h>
> +    #include <dt-bindings/phy/phy.h>
> +
> +    pcs@fda00000 {
> +      compatible = "rockchip,rk3568-xpcs";
> +      reg = <0xfda00000 0x200000>;
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
> +      clock-names = "csr", "eee";
> +      phys = <&combphy2 PHY_TYPE_SGMII>;
> +      phy-names = "serdes";
> +      power-domains = <&power RK3568_PD_PIPE>;
> +
> +      pcs-mii@0 {
> +        reg = <0>;
> +      };
> +    };