Date:   Tue, 5 Oct 2021 19:16:36 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [RFC net-next PATCH 16/16] net: sfp: Add quirk to ignore PHYs



On 10/5/21 6:17 PM, Russell King (Oracle) wrote:
> On Tue, Oct 05, 2021 at 04:38:23PM -0400, Sean Anderson wrote:
>> There is a level shifter. Between the shifter and the SoC there were
>> 1.8k (!) pull-ups, and between the shifter and the SFP there were 10k
>> pull-ups. I tried replacing the pull-ups between the SoC and the shifter
>> with 10k pull-ups, but noticed no difference. I have also noticed no
>> issues accessing the EEPROM, and I have not noticed any difference
>> accessing other registers (see below). Additionally, this same error is
>> "present" already in xgbe_phy_finisar_phy_quirks(), as noted in the
>> commit message.
>
> Hmm, thanks for checking. So it's something "weird" that this module
> is doing.
>
> As I say, the 88E1111 has a native I2C mode, it sounds like they're not
> using it but have their own, seemingly broken, protocol conversion from
> the I2C bus to MDIO. I've opened and traced the I2C connections on this
> module - they only go to an EEPROM and the 88E1111, so we know this is
> a "genuine" 88E1111 in I2C mode we are talking to.

Well, I had a look inside mine and it had a "Custom Code/Die Revision"
of B2. Nothing else unusual. Just the PHY, magnetics, EEPROM, crystal,
and a regulator.

>> First, reading two bytes at a time
>> 	$ i2ctransfer -y 2 w1@...6 2 r2
>> 	0x01 0xff
>> This behavior is repeatable
>> 	$ i2ctransfer -y 2 w1@...6 2 r2
>> 	0x01 0xff
>> Now, reading one byte at a time
>> 	$ i2ctransfer -y 2 w1@...6 2 r1
>> 	0x01
>> A second write/single read gets us the first byte again.
>> 	$ i2ctransfer -y 2 w1@...6 2 r1
>> 	0x41
>
> I think you mean you get the other half of the first word.

Yes.

> When I try this with a 88E1111 directly connected to the I2C bus, I
> get:
>
> root@...arfog21:~# i2ctransfer -y 1 w1@...6 2 r2
> 0x01 0x41
> root@...arfog21:~# i2ctransfer -y 1 w1@...6 2 r2
> 0x01 0x41
> root@...arfog21:~# i2ctransfer -y 1 w1@...6 2 r1
> 0x01
> root@...arfog21:~# i2ctransfer -y 1 w1@...6 2 r1
> 0x01
>
> So a completely different behaviour. Continuing...
>
>> And doing it for a third time gets us the first byte again.
>> 	$ i2ctransfer -y 2 w1@...6 2 r1
>> 	0x01
>> If we start another one-byte read without writing the address, we get
>> the second byte
>> 	$ i2ctransfer -y 2 r1@...6
>> 	0x41
>
> root@...arfog21:~# i2ctransfer -y 1 w1@...6 2 r1
> 0x01
> root@...arfog21:~# i2ctransfer -y 1 r1@...6
> 0x01
>
> Again, different behaviour.
>
>> And continuing this pattern, we get the next byte.
>> 	$ i2ctransfer -y 2 r1@...6
>> 	0x0c
>> This can be repeated indefinitely
>> 	$ i2ctransfer -y 2 r1@...6
>> 	0xc2
>> 	$ i2ctransfer -y 2 r1@...6
>> 	0x0c
>
> root@...arfog21:~# i2ctransfer -y 1 r1@...6
> 0x01
> root@...arfog21:~# i2ctransfer -y 1 r1@...6
> 0x41
> root@...arfog21:~# i2ctransfer -y 1 r1@...6
> 0x01
>
> Here we eventually start toggling between the high and low bytes of
> the word.
>
>> But stopping in the "middle" of a register fails
>> 	$ i2ctransfer -y 2 w1@...6 2 r1
>> 	Error: Sending messages failed: Input/output error
>
> root@...arfog21:~# i2ctransfer -y 1 w1@...6 2 r1
> 0x01
>
> No error for me.
>
>> We don't have to immediately read a byte:
>> 	$ i2ctransfer -y 2 w1@...6 2
>> 	$ i2ctransfer -y 2 r1@...6
>> 	0x01
>> 	$ i2ctransfer -y 2 r1@...6
>> 	0x41
>
> root@...arfog21:~# i2ctransfer -y 1 w1@...6 2
> root@...arfog21:~# i2ctransfer -y 1 r1@...6
> 0x01
> root@...arfog21:~# i2ctransfer -y 1 r1@...6
> 0x01
>
> Again, no toggling between high/low bytes of the word.
>
>> We can read two bytes indefinitely after "priming the pump"
>> 	$ i2ctransfer -y 2 w1@...6 2 r1
>> 	0x01
>> 	$ i2ctransfer -y 2 r1@...6
>> 	0x41
>> 	$ i2ctransfer -y 2 r2@...6
>> 	0x0c 0xc2
>> 	$ i2ctransfer -y 2 r2@...6
>> 	0x0c 0x01
>> 	$ i2ctransfer -y 2 r2@...6
>> 	0x00 0x00
>> 	$ i2ctransfer -y 2 r2@...6
>> 	0x00 0x04
>> 	$ i2ctransfer -y 2 r2@...6
>> 	0x20 0x01
>> 	$ i2ctransfer -y 2 r2@...6
>> 	0x00 0x00
>
> root@...arfog21:~# i2ctransfer -y 1 w1@...6 2
> root@...arfog21:~# i2ctransfer -y 1 r1@...6
> 0x01
> root@...arfog21:~# i2ctransfer -y 1 r1@...6
> 0x01
> root@...arfog21:~# i2ctransfer -y 1 r2@...6
> 0x01 0x41
> root@...arfog21:~# i2ctransfer -y 1 r2@...6
> 0x01 0x41
> root@...arfog21:~# i2ctransfer -y 1 r2@...6
> 0x01 0x41
> root@...arfog21:~# i2ctransfer -y 1 r2@...6
> 0x01 0x41
> root@...arfog21:~# i2ctransfer -y 1 r2@...6
> 0x01 0x41
> root@...arfog21:~# i2ctransfer -y 1 r2@...6
> 0x01 0x41
>
> No auto-increment of the register.
>
>> But more than that "runs out"
>> 	$ i2ctransfer -y 2 w1@...6 2 r1
>> 	0x01
>> 	$ i2ctransfer -y 2 r1@...6
>> 	0x41
>> 	$ i2ctransfer -y 2 r4@...6
>> 	0x0c 0xc2 0x0c 0x01
>> 	$ i2ctransfer -y 2 r4@...6
>> 	0x00 0x00 0x00 0x04
>> 	$ i2ctransfer -y 2 r4@...6
>> 	0x20 0x01 0xff 0xff
>> 	$ i2ctransfer -y 2 r4@...6
>> 	0x01 0xff 0xff 0xff
>
> root@...arfog21:~# i2ctransfer -y 1 w1@...6 2
> root@...arfog21:~# i2ctransfer -y 1 r1@...6
> 0x01
> root@...arfog21:~# i2ctransfer -y 1 r1@...6
> 0x01
> root@...arfog21:~# i2ctransfer -y 1 r4@...6
> 0x01 0x41 0x0c 0xc2
> root@...arfog21:~# i2ctransfer -y 1 r4@...6
> 0x01 0x41 0x0c 0xc2
> root@...arfog21:~# i2ctransfer -y 1 r4@...6
> 0x01 0x41 0x0c 0xc2
> root@...arfog21:~# i2ctransfer -y 1 r4@...6
> 0x01 0x41 0x0c 0xc2
>
>> However, the above multi-byte reads only works when starting at register
>> 2 or greater.
>> 	$ i2ctransfer -y 2 w1@...6 0 r1
>> 	0x01
>> 	$ i2ctransfer -y 2 r1@...6
>> 	0x40
>> 	$ i2ctransfer -y 2 r2@...6
>> 	0x01 0xff
>>
>> Based on the above session, I believe that it may be best to treat this
>> phy as having an autoincrementing register address which must be read
>> one byte at a time, in multiples of two bytes. I think that existing SFP
>> phys may compatible with this, but unfortunately I do not have any on
>> hand to test with.
>
> Sadly, according to my results above, I think your module is doing
> something strange with the 88E1111.
>
> You say that it's Finisar, but I can only find this module in
> Fiberstore's website: https://www.fs.com/uk/products/20057.html
> Fiberstore commonly use "FS" in the vendor field.

So you are correct. I had seen the xgbe fixup for the same phy_id and
assumed that FS meant the same thing in both cases. You may also notice
that nowhere on their site do they spell out their name.

> You have me wondering what they've done to this PHY to make it respond
> in the way you are seeing.

You may notice on their site that there are different "compatible"
options for e.g. "FS", "Dell", "Cisco", and around 50 other
manufacturers. I wonder if I've come across a module which supports
autoincrementing byte reads in order to be compatible with some
manufacturer's hardware. And perhaps this change has inadvertently
broken the two-byte read capability, but only for the first 3 registers.

--Sean

Powered by blists - more mailing lists