Re: [PATCH net-next v6 03/11] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-17 18:38:11
Also in:
linux-devicetree, linux-phy, linux-renesas-soc, linux-rockchip, lkml, netdev
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 hunk ↗ jump to hunk
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 hunk ↗ jump to hunk
@@ -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 hunk ↗ jump to hunk
@@ -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. -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915123802.1561724-1-coiaprant%40gmail.com