Re: [PATCH net-next v6 03/11] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568

[off-list ref] 于2026年9月18日周五 02:38写道：

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

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] drivers/phy/rockchip/phy-rockchip-naneng-combphy.c:380-381 reads
  'rockchip,sgmii-mac-sel' with device_property_read_u32() for every…

--

phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568

This adds a "rockchip,sgmii-mac-sel" DT property so the RK3568 SGMII
interface can be routed to GMAC0 or GMAC1 via the PIPE_GRF_XPCS_CON0
pipe_sgmii_mac_sel bit, defaulting to GMAC1 to match the hardware reset
value.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 229)

quoted

diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
index 7843356a4dd47..7b867e7520064 100644
--- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c

[ ... ]

quoted

@@ -375,6 +377,9 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy

      priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");

+     priv->sgmii_mac_sel = 1;
+     device_property_read_u32(dev, "rockchip,sgmii-mac-sel", &priv->sgmii_mac_sel);
+

[Severity: Low]
This isn't a bug for any schema-checked device tree, but would it be worth
reacting to the return value here?

The binding restricts the property to two values and to one compatible:

  rockchip,sgmii-mac-sel:
    $ref: /schemas/types.yaml#/definitions/uint32
    enum: [0, 1]
    default: 1

  ...
        rockchip,sgmii-mac-sel: true
    else:
      properties:
        rockchip,sgmii-mac-sel: false

In rockchip_combphy_parse_dt() the result of device_property_read_u32() is
dropped, so a malformed property (for example a zero-length or wrong-sized
cell, which returns -ENODATA/-EOVERFLOW and leaves the output untouched)
ends up indistinguishable from an explicit request for GMAC1.

The read also happens for every compatible, including rk3528/rk3562/rk3576/
rk3588 where the binding declares the property invalid; there the value is
parsed and then never used, since only rk3568_combphy_cfg() consumes it.

[ ... ]

quoted

@@ -873,6 +878,8 @@ static int rk3568_combphy_cfg(struct rockchip_combphy_priv *priv)
              break;

      case PHY_TYPE_SGMII:
+             rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_sgmii_mac_sel,
+                                          priv->sgmii_mac_sel > 0);

[Severity: Low]
Should out-of-range values be rejected rather than folded into GMAC1?

The "priv->sgmii_mac_sel > 0" test collapses the u32 to a boolean, so a
device tree with rockchip,sgmii-mac-sel = <2> (or 0xffffffff) that was never
run through dtbs_check gets the GMAC1 routing and rk3568_combphy_cfg() still
returns success, with nothing logged.

A range check in rockchip_combphy_parse_dt() returning -EINVAL, or at least
a dev_warn(), would make such a device tree visible instead of silently
selecting the default route.

Thanks for the review.

The property is validated by dtbschema for all in-tree device trees,
which is where the enum and the compatible gating are enforced. A
malformed or out-of-range value can only come from a bootloader-supplied
DT or an overlay that bypassed dtbs_check, which is a user error rather
than something the driver needs to defend against.

The read result is intentionally ignored: the default of 1 matches the
hardware reset value, so a missing property and an explicit "1" are
semantically identical for this hardware.

Thanks,
Coia