Re: [PATCH net-next v3 08/10] net: stmmac: dwmac-rk: add SGMII support for RK3568

Hi,

quoted

quoted

gmac_clk_enable() is now called before any register access, including
the SGMII mode setup path.  Previously SGMII mode would bypass the
clock enable via a goto, which could cause synchronous external abort
when accessing MAC registers with the clock domain disabled.

Also clean up the error handling in rk_gmac_powerup() by using a
unified clk_disable label, and add error handling for the default
(unhandled interface) case.

SGMII In-band vs Out-of-band
============================
On RK3568, the MAC clock is fixed at 125 MHz and cannot be dynamically
changed by the stmmac core's set_clk_tx_rate callback.  In-band mode
works because the PCS handles rate adaptation internally.  Out-of-band
mode does not work because the MAC would need to change the clock rate
to 125/12.5/1.25 MHz for 1000/100/10 Mbps respectively, and the clock
is fixed.

Enable default_an_inband for SGMII and disable the generic stmmac
set_clk_tx_rate callback.  This forces phylink to use in-band mode,
where the PCS is responsible for speed/duplex negotiation.  Without
this, the stmmac core would attempt to change the clock rate on speed
changes, causing TX to work but RX to fail.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 386)
Signed-off-by: Coia Prant <redacted>

[...]

quoted

+static int rk_pcs_init(struct stmmac_priv *priv)
+{
+     struct device_node *np = priv->device->of_node;
+     struct device_node *pcs_node;
+     struct dw_xpcs *xpcs;
+
+     pcs_node = of_parse_phandle(np, "pcs-handle", 0);
+     if (!pcs_node)
+             return -ENODEV;

Here aswell you make it mandatory to have a PCS, as the generic pcs logic
introduced in patch 1 doesn't handle -ENODEV, it treats it as any other
error.

So, either you return 0 when there's no PCS (so that we don't break platforms
that don't have one), or you handle -ENODEV gracefully in patch 1.

Maxime

Hi,

Currently, `rk_pcs_init` is called only for SGMII, which requires a PCS.

As it stands, only the RK3568 supports SGMII, so we won't break anything here.

Ah yes, it's protected by

if (plat_dat->phy_interface == PHY_INTERFACE_MODE_SGMII) {
 ...
}

My bad, this is fine then :)

the intel thing from patch 1 is still standing though from what I can see ? or have
I made the same error there ?

Maxime