Re: [PATCH net-next] net: sfp: add quirk for XikeStor SKT-2.5G-100M

[Jan Hoffmann]

Hi Andrew,
> > Note: The RTL8221B PHY in this module becomes stuck in a broken state
> > after attempting to read some registers on MMD 30 while it has already
> > established a link (this is a general issue with this PHY which happens
> > when it is configured for rate adaptation mode).
>
> This sounds like something which should be fixed in the PHY
> driver. Can you take a look at that. Is there an errata for it with a
> suggested workaround?

Unfortunately, the read access which can break the PHY happens already 
in "get_phy_c45_ids", i.e. before the PHY driver is attached (and this 
is the only place where such a read access occurs).


I am not sure if there is any errata or official documentation about 
this issue.

What I do know is based on experimentation with mdio-tools (with some 
clues from existing driver code):

If bit 0 of register 0x75f3 on MMD 30 is set, reading any register on 
MMD 30 except for the actual SerDes registers (and also registers 5/6) 
breaks the PHY. In the broken state, all registers on MMD 30 (as well as 
registers 5/6 on all other MMDs) only read "0xdead". To make the PHY 
work again properly, a hardware reset is necessary.

When the PHY is configured to use either 2500Base-X with rate matching 
or HiSGMII only, it sets this bit as soon as a link is established. The 
PHY doesn't set this bit when 2500Base-X+SGMII or HiSGMII+SGMII mode is 
configured (i.e. dynamic switching depending on link speed).

(The driver unsets this bit when configuring the interface mode in 
"rtl822x_set_serdes_option_mode". Otherwise I probably would have never 
found the relation of that register to the issue.)


Some devices are unaffected by this issue, for example if the bootloader 
already configures the PHY to use 2500Base-X+SGMII or HiSGMII+SGMII (or 
at least can be made to do so).

Since the PHY takes about 4 seconds after hardware reset to establish a 
link, any devices where the kernel can do a reset are also not affected 
(like this SFP module, as long as the host supports TX_DISABLE).


I would really like to have a general fix for cases where neither of 
these two workarounds happen to already be in place. But I'm not sure 
how this could be done cleanly, as it requires special handling for 
these PHYs in the function that reads the PHY ID (or even before that).

Downstream in OpenWrt, I added a patch for "get_phy_c45_ids" to avoid 
reading MMD 30 from RTL8221B PHYs based on the PHY ID in MMD 1:

https://github.com/openwrt/openwrt/commit/88dcd8c303b688b344c8f22c24df6a49f6b6b707

But hardcoding it like this in the kernel core feels more like a hack 
than a solution.

One idea I had is if it would be possible to make "get_phy_c45_ids" 
change its behaviour based on a device tree property or a SFP quirk.

> > Signed-off-by: Jan Hoffmann <jan@xxxxxx>
>
> Reviewed-by: Andrew Lunn <andrew@xxxxxxx>
>
>      Andrew

Thanks,
Jan