Re: [RFC PATCH 03/10] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch breaks device tree backward compatibility by forcing the SGMII MAC selection to GMAC0 when the optional `rockchip,sgmii-mac-sel` property is absent, overriding the hardware default.
--

--- Patch [3]: [RFC PATCH 03/10] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568 ---
commit 605f86936fd231092ab8489f15df507702255912
Author: Coia Prant <coiaprant@gmail.com>

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

This commit adds support for routing the SGMII interface to either GMAC0
or GMAC1 on RK3568 by introducing the rockchip,sgmii-mac-sel DT property.

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

> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c

[ ... ]

> @@ -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 = 0;
> +	device_property_read_u32(dev, "rockchip,sgmii-mac-sel", &priv->sgmii_mac_sel);
> +
>  	priv->phy_rst = devm_reset_control_get_exclusive(dev, "phy");

[ ... ]

> @@ -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: High]
Does this break device tree backward compatibility?

When the rockchip,sgmii-mac-sel property is missing from an older device
tree, priv->sgmii_mac_sel defaults to 0 in rockchip_combphy_parse_dt().

By unconditionally calling rockchip_combphy_param_write() here, the driver
now actively clears the MAC selection bit to 0 (GMAC0).

Because the hardware reset value is GMAC1, this actively overrides the default
hardware and bootloader state that older device trees were relying on.

Could the code check if the property was actually present before writing
to the register, to ensure the hardware default is preserved for existing
setups?

>  		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);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=3