[PATCH net-next v6 09/11] net: stmmac: dwmac-rk: add SGMII support for RK3568
netdev-bot+sashiko at kernel.org
netdev-bot+sashiko at kernel.org
Thu Sep 17 11:38:22 PDT 2026
Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 (1) · Low: 5
- [Medium] SGMII is advertised to phylink for every RK3568 GMAC port, but
the SGMII PCS glue and the GRF SGMII mode bit are only wired up when…
- [Low] Claim: setting plat_dat->default_an_inband = true and clearing
plat_dat->set_clk_tx_rate does not enforce in-band SGMII;…
- [Low] Claim: the new "phy-mode = sgmii implies pcs-handle" conditional
in Documentation/devicetree/bindings/net/rockchip-dwmac.yaml is not…
- [Low] The XPCS node added to arch/arm64/boot/dts/rockchip/rk3568.dtsi
(ethernet-pcs at fda00000, lines 113-142) omits `phys` and `phy-names`,…
- [Low] DWMAC_ROCKCHIP now does `select PCS_XPCS_ROCKCHIP` while having
no PM dependency, and PCS_XPCS_ROCKCHIP…
- [Low] Claim: rk3568_set_to_sgmii() and rk3568_set_to_rmii() discard the
regmap_write() status, so a failure to switch GRF CON1 bit 7 leaves…
Pre-existing issues:
- [Medium] gmac_clk_enable(bsp_priv, true) leaks the bulk clock
prepare/enable references when clk_prepare_enable(bsp_priv->clk_phy)…
--
net: stmmac: dwmac-rk: add SGMII support for RK3568
This adds SGMII glue for the RK3568 GMACs: a set_to_sgmii()/set_to_rmii()
pair that programs GRF CON1 bit 7, pcs_init/pcs_exit/select_pcs callbacks
that create the Rockchip XPCS, and default_an_inband plus a NULL
set_clk_tx_rate for SGMII. rk_gmac_powerup() is also reordered so the
clocks are enabled before the interface-specific configuration, with the
error unwinding folded into one clk_disable label.
Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 386)
A few questions below.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> index ab3c4cf964239..c6559e749c0ce 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> @@ -170,6 +170,7 @@ config DWMAC_ROCKCHIP
> default ARCH_ROCKCHIP
> depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
> select MFD_SYSCON
> + select PCS_XPCS_ROCKCHIP
[Severity: Low]
Can this select produce an unmet dependency warning when PM is disabled?
DWMAC_ROCKCHIP only carries:
depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
while the selected symbol in drivers/net/pcs/Kconfig is:
config PCS_XPCS_ROCKCHIP
tristate "Rockchip XPCS controller support"
default ARCH_ROCKCHIP
depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
select GENERIC_PHY
select PM_GENERIC_DOMAINS
and in kernel/power/Kconfig:
config PM_GENERIC_DOMAINS
bool
depends on PM
With PM=n (for example a COMPILE_TEST build with OF=y and STMMAC_ETH=m),
DWMAC_ROCKCHIP now force-enables PCS_XPCS_ROCKCHIP, which in turn
force-enables PM_GENERIC_DOMAINS against its own depends on PM, so genpd
gets built into a PM=n kernel. Would a conditional select, or a depends
on, be more appropriate here?
Related: PCS_XPCS_ROCKCHIP lives inside the "if PCS_XPCS" block in
drivers/net/pcs/Kconfig, so it also carries depends on PCS_XPCS. Does
DWMAC_ROCKCHIP need to select PCS_XPCS too, to avoid the same kind of
warning when PCS_XPCS=n?
> help
> Support for Ethernet controller on Rockchip RK3288 SoC.
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 8d7042e689261..b3d746dd966ff 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
[ ... ]
> @@ -1330,6 +1395,7 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
>
> bsp_priv->supports_rgmii = ops->supports_rgmii || !!ops->set_to_rgmii;
> bsp_priv->supports_rmii = ops->supports_rmii || !!ops->set_to_rmii;
> + bsp_priv->supports_sgmii = ops->supports_sgmii || !!ops->set_to_sgmii;
>
> if (ops->init) {
> ret = ops->init(bsp_priv);
[ ... ]
> @@ -1379,16 +1449,19 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
> if (ret)
> return ret;
>
> + ret = gmac_clk_enable(bsp_priv, true);
> + if (ret)
> + return ret;
[Severity: Medium]
This isn't a bug introduced by this patch, but since the clock handling in
this function is being reorganised: does gmac_clk_enable(bsp_priv, true)
leak the bulk clock prepare/enable references when the phy clock fails?
static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
{
...
ret = clk_bulk_prepare_enable(bsp_priv->num_clks,
bsp_priv->clks);
if (ret)
return ret;
ret = clk_prepare_enable(bsp_priv->clk_phy);
if (ret)
return ret;
...
bsp_priv->clk_enabled = true;
On the second failure the bulk clocks stay prepared/enabled and
bsp_priv->clk_enabled is still false, so neither the bare return here nor
the new clk_disable label (which is gated on clk_enabled) undoes them. A
later successful gmac_clk_enable(true) would then call
clk_bulk_prepare_enable() a second time. Is it worth rolling the bulk
clocks back inside gmac_clk_enable() while this area is being touched?
> +
> + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_SGMII)
> + goto set_mode;
> +
> ret = rk_get_phy_intf_sel(bsp_priv->phy_iface);
> if (ret < 0)
> - return ret;
> + goto clk_disable;
>
> intf = ret;
>
> - ret = gmac_clk_enable(bsp_priv, true);
> - if (ret)
> - return ret;
> -
> if (bsp_priv->gmac_phy_intf_sel_mask ||
> bsp_priv->gmac_rmii_mode_mask) {
> /* If defined, encode the phy_intf_sel value */
[ ... ]
> @@ -1447,15 +1517,20 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
> if (bsp_priv->ops->set_to_rmii)
> bsp_priv->ops->set_to_rmii(bsp_priv);
> break;
> + case PHY_INTERFACE_MODE_SGMII:
> + dev_info(dev, "init for SGMII\n");
> + if (bsp_priv->ops->set_to_sgmii)
> + bsp_priv->ops->set_to_sgmii(bsp_priv);
> + break;
> default:
> dev_err(dev, "NO interface defined!\n");
> + ret = -EINVAL;
> + goto clk_disable;
> }
>
> ret = rk_phy_powerup(bsp_priv);
> - if (ret) {
> - gmac_clk_enable(bsp_priv, false);
> - return ret;
> - }
> + if (ret)
> + goto clk_disable;
>
> pm_runtime_get_sync(dev);
>
> @@ -1486,6 +1565,9 @@ static void rk_get_interfaces(struct stmmac_priv *priv, void *bsp_priv,
>
> if (rk->supports_rmii)
> __set_bit(PHY_INTERFACE_MODE_RMII, interfaces);
> +
> + if (rk->supports_sgmii)
> + __set_bit(PHY_INTERFACE_MODE_SGMII, interfaces);
> }
[Severity: Medium]
Is SGMII advertised more widely than the PCS glue is actually installed?
rk_gmac_setup() derives the capability purely from the ops table:
bsp_priv->supports_sgmii = ops->supports_sgmii || !!ops->set_to_sgmii;
rk3568_ops now provides .set_to_sgmii, so supports_sgmii is true for every
rk3568 port and rk_get_interfaces() sets PHY_INTERFACE_MODE_SGMII in
config->supported_interfaces even for a port whose phy-mode is rgmii or
rmii.
The PCS side, however, is keyed off the probe-time phy-mode only (see the
rk_gmac_probe() hunk below), so on such a port priv->plat->select_pcs stays
NULL and no XPCS is ever created. If phylink later picks SGMII out of
supported_interfaces (an SFP cage going through
phylink_sfp_select_interface(), or a PHY reporting
PHY_INTERFACE_MODE_SGMII), stmmac_mac_select_pcs() returns NULL:
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_mac_select_pcs() {
if (priv->plat->select_pcs) {
pcs = priv->plat->select_pcs(priv, interface);
...
}
so nothing rejects the selection. Meanwhile rk_gmac_powerup() ran from the
probe-time bsp_priv->phy_iface, so GRF CON1 bit 7 was cleared by
rk3568_set_to_rgmii()/rk3568_set_to_rmii() and is never set, and
rk_set_clk_tx_rate() is still installed. Would that leave the port with a
link that never comes up and no error reported?
Would gating the __set_bit(PHY_INTERFACE_MODE_SGMII, interfaces) on the same
condition that installs pcs_init/select_pcs, and programming the GRF mode
bit for the interface phylink actually selected, avoid this?
>
> static int rk_set_clk_tx_rate(void *bsp_priv_, struct clk *clk_tx_i,
> @@ -1602,6 +1684,17 @@ static int rk_gmac_probe(struct platform_device *pdev)
> plat_dat->suspend = rk_gmac_suspend;
> plat_dat->resume = rk_gmac_resume;
>
> + if (plat_dat->phy_interface == PHY_INTERFACE_MODE_SGMII) {
> + /* SGMII clock always runs at 125 MHz */
> + plat_dat->set_clk_tx_rate = NULL;
> +
> + /* SGMII requires a PCS */
> + plat_dat->default_an_inband = true;
> + plat_dat->pcs_init = rk_pcs_init;
> + plat_dat->pcs_exit = rk_pcs_exit;
> + plat_dat->select_pcs = rk_select_pcs;
> + }
> +
> plat_dat->bsp_priv = rk_gmac_setup(pdev, plat_dat, data);
> if (IS_ERR(plat_dat->bsp_priv))
> return PTR_ERR(plat_dat->bsp_priv);
One more question, about the other patches in the same series rather than
this one:
[Severity: Low]
Does the xpcs node in arch/arm64/boot/dts/rockchip/rk3568.dtsi validate
against the new binding? The binding
Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml has:
required:
- compatible
- reg
- "#address-cells"
- "#size-cells"
- clocks
- clock-names
- phys
- phy-names
- power-domains
while the SoC-level node has neither phys nor phy-names:
xpcs: ethernet-pcs at 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";
Only rk3568-photonicat.dts adds them at board level, and dt-validate does
not skip nodes with status = "disabled", so would every other rk3568 board
report "'phys' is a required property" under CHECK_DTBS? A board that
enables the node without adding both properties would also fail in
xpcs_rk_serdes_phy_init() -> devm_phy_get(dev, "serdes").
Would it be better to keep the PHY assignment optional in the binding, or to
move the properties into the SoC dtsi?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915123802.1561724-1-coiaprant%40gmail.com
More information about the linux-arm-kernel
mailing list