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

[Jan Hoffmann <jan@3e8.eu>]

> 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