Re: [PATCH net-next v5 03/11] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-14 20:24:56
Also in:
linux-arm-kernel, linux-devicetree, linux-phy, linux-renesas-soc, linux-rockchip, lkml
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 0 · Low: 1 - [Low] In rk3568_combphy_cfg() (drivers/phy/rockchip/phy-rockchip- naneng-combphy.c:881-882) the newly added write of pipe_sgmii_mac_sel… -- phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568 This adds a "rockchip,sgmii-mac-sel" DT property to pick whether the RK3568 SGMII interface is routed to GMAC0 or GMAC1 via PIPE_GRF_XPCS_CON0 bit 1. The value is read in rockchip_combphy_parse_dt() and programmed from the PHY_TYPE_SGMII branch of rk3568_combphy_cfg(). Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 229) One question about the commit message wording: The hardware reset value is 1 (GMAC1). If the property is set to 0, the driver routes SGMII to GMAC0; if set to 1 (or omitted), it remains at GMAC1. Does "or omitted ... it remains at GMAC1" match the code? The write in rk3568_combphy_cfg() is unconditional, so in the omitted case the driver does not leave the bit alone, it actively programs it to 1. See the inline comment below.
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] Since device_property_read_u32() leaves the output untouched on failure, rockchip_combphy_parse_dt() ends up with priv->sgmii_mac_sel == 1 both when the property says 1 and when the property is absent. Is there a way for the driver to express "leave the bit as firmware left it"? Before this patch the kernel never touched PIPE_GRF_XPCS_CON0 bit 1, so a board whose bootloader routed SGMII to GMAC0 kept that routing. With this change, a DT that instantiates the combphy as PHY_TYPE_SGMII but does not carry the new property has the bit re-programmed to GMAC1 on every phy_init(). Can that break such a board's SGMII link?
quoted hunk ↗ jump to hunk
priv->phy_rst = devm_reset_control_get_exclusive(dev, "phy"); /* fallback to old behaviour */ if (PTR_ERR(priv->phy_rst) == -ENOENT)@@ -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] This write always happens, including in the "property omitted" case, and rockchip_combphy_param_write() sets the write-enable mask along with the value: tmp = en ? reg->enable : reg->disable; mask = GENMASK(reg->bitend, reg->bitstart); val = (tmp << reg->bitstart) | (mask << BIT_WRITEABLE_SHIFT); return regmap_write(base, reg->offset, val); so with priv->sgmii_mac_sel defaulting to 1 the register gets (0x1 << 1) | (GENMASK(1, 1) << 16), i.e. GMAC1 is forced. Would it be better to only issue this write when the property is actually present? A second question: bit 1 of PIPE_GRF_XPCS_CON0 is a single SoC-wide mux, while the property sits on a per-PHY node. If two combphy instances that share the same rockchip,pipe-grf syscon are both brought up as PHY_TYPE_SGMII with different property values, the last phy_init() wins and nothing is reported. Should conflicting values be rejected or at least warned about?
quoted hunk ↗ jump to hunk
rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_xpcs_phy_ready, true); rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_phymode_sel, true); rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_sel_qsgmii, true);@@ -984,6 +991,7 @@ static const struct rockchip_combphy_grfcfg rk3568_combphy_grfcfgs = { .con3_for_sata = { 0x000c, 15, 0, 0x00, 0x4407 }, /* pipe-grf */ .pipe_con0_for_sata = { 0x0000, 15, 0, 0x00, 0x2220 }, + .pipe_sgmii_mac_sel = { 0x0040, 1, 1, 0x00, 0x01 }, .pipe_xpcs_phy_ready = { 0x0040, 2, 2, 0x00, 0x01 },
For what it is worth, the only in-tree SGMII consumer at the end of this series, rk3568-photonicat.dts, sets rockchip,sgmii-mac-sel = <0> explicitly, so no upstream DT changes behaviour today; the concern is about out-of-tree or ported device trees and about the commit message wording. -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com