This 2.5G copper SFP module contains a RTL8221B-VB-CG PHY which can be
accessed using Rollball protocol.

Add a SFP quirk to enable Rollball protocol. It includes a small delay
to avoid unnecessary attempts at reading PHY registers before the
module is ready.

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 includes the
registers 2/3/8 which are read by "get_phy_c45_ids" during PHY probing.
However, the PHY takes about 4 seconds to establish a link after reset,
so in practice any host that supports TX_DISABLE should be unaffected,
as that puts the PHY into reset.

Signed-off-by: Jan Hoffmann <jan@3e8.eu>
---
 drivers/net/phy/sfp.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 2ec91466acdf..6bbb02117217 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -432,6 +432,19 @@ static void sfp_fixup_rollball_wait4s(struct sfp *sfp)
 	sfp->module_t_wait = msecs_to_jiffies(4000);
 }
 
+static void sfp_fixup_xikestor_2_5g(struct sfp *sfp)
+{
+	sfp_fixup_rollball(sfp);
+
+	/* This module does not immediately respond to Rollball commands. Add
+	 * a small delay to avoid unnecessary PHY access attempts. Note that
+	 * the delay should not be too long, as the RTL8221B-VB-CG PHY inside
+	 * breaks when reading some registers from MMD 30 if it has already
+	 * established a link (which takes about 4 seconds after reset).
+	 */
+	sfp->module_t_wait = msecs_to_jiffies(1000);
+}
+
 static void sfp_fixup_fs_10gt(struct sfp *sfp)
 {
 	sfp_fixup_10gbaset_30m(sfp);
@@ -636,6 +649,8 @@ static const struct sfp_quirk sfp_quirks[] = {
 	SFP_QUIRK_F("Turris", "RTSFP-10", sfp_fixup_rollball),
 	SFP_QUIRK_F("Turris", "RTSFP-10G", sfp_fixup_rollball),
 
+	SFP_QUIRK_F("XikeStor", "SKT-2.5G-100M", sfp_fixup_xikestor_2_5g),
+
 	SFP_QUIRK_S("ZOERAX", "SFP-2.5G-T", sfp_quirk_oem_2_5g),
 };
 
-- 
2.55.0

> 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?

> Signed-off-by: Jan Hoffmann <jan@3e8.eu>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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@3e8.eu>
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
>      Andrew

Thanks,
Jan

> 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.

[Goes and looks at 802.3, clause 45]

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

I assume this PHY does have a valid ID in MMD 1-29?

> 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:

This suggests it does.

I wounder if we can make use of:

	if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff) {
		/* If mostly Fs, there is no device there, then let's probe
		 * MMD 0, as some 10G PHYs have zero Devices In package,
		 * e.g. Cortina CS4315/CS4340 PHY.
		 */
		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, &devs_in_pkg);
		if (phy_reg < 0)
			return -EIO;

		/* no device there, let's get out of here */
		if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff)
			return -ENODEV;
	}

I assume this is not hit for this device?

I _guess_ there are ~0 PHYs which probe based on ID values in
MDIO_MMD_VEND1 or MDIO_MMD_VEND2. So maybe move the code looking for
device present in MDIO_MMD_VEND1 or MDIO_MMD_VEND2 inside this clause?
Then in the normal case we never look in these registers.

If we don't look to see if the MDIO_MMD_VEND1 or MDIO_MMD_VEND2
devices are present, i assume the next loop:

	/* Now probe Device Identifiers for each device present. */
	for (i = 1; i < num_ids; i++) {
		if (!(devs_in_pkg & (1 << i)))
			continue;

will also leave them alone?

But if there is an oddball PHY around which relies on MDIO_MMD_VEND1
or MDIO_MMD_VEND2 IDs, we still look there, if we failed to find
anything anywhere else, and so hopefully it does not cause a
regression?

	Andrew

> I assume this PHY does have a valid ID in MMD 1-29?

Yes, MMDs 1, 3, 7, and 31 all report the same same ID (0x001cc849).

On MMD 30, the PHY ID registers return a seemingly random value (or 
0xdead in the broken state). For other MMDs, the PHY ID registers are 
all zero.

> I wounder if we can make use of:
> 
> 	if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff) {
> 		/* If mostly Fs, there is no device there, then let's probe
> 		 * MMD 0, as some 10G PHYs have zero Devices In package,
> 		 * e.g. Cortina CS4315/CS4340 PHY.
> 		 */
> 		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, &devs_in_pkg);
> 		if (phy_reg < 0)
> 			return -EIO;
> 
> 		/* no device there, let's get out of here */
> 		if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff)
> 			return -ENODEV;
> 	}
> 
> I assume this is not hit for this device?

The devices-in-package registers (5/6) have the same value of 0xc000008b 
for all MMDs (0-31). So the variable devs_in_pkg already contains that 
value at this point and the branch is not taken.

> I _guess_ there are ~0 PHYs which probe based on ID values in
> MDIO_MMD_VEND1 or MDIO_MMD_VEND2. So maybe move the code looking for
> device present in MDIO_MMD_VEND1 or MDIO_MMD_VEND2 inside this clause?
> Then in the normal case we never look in these registers.
> 
> If we don't look to see if the MDIO_MMD_VEND1 or MDIO_MMD_VEND2
> devices are present, i assume the next loop:
> 
> 	/* Now probe Device Identifiers for each device present. */
> 	for (i = 1; i < num_ids; i++) {
> 		if (!(devs_in_pkg & (1 << i)))
> 			continue;
> 
> will also leave them alone?

Since bit 30 in devs_in_pkg is set, this loop is actually where the read 
breaking the PHY takes place. Currently, it is phy_c45_probe_present 
which breaks the PHY by reading register 8. But without that, the same 
would happen when reading the PHY ID from registers 2/3.

> But if there is an oddball PHY around which relies on MDIO_MMD_VEND1
> or MDIO_MMD_VEND2 IDs, we still look there, if we failed to find
> anything anywhere else, and so hopefully it does not cause a
> regression?
> 
> 	Andrew

Thanks,
Jan