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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi,

>>> 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 <coiaprant@gmail.com>
>>
>> [...]
>>
>>> +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