Message-ID: <Z736uAVe5MqRn7Se@shell.armlinux.org.uk>
Date: Tue, 25 Feb 2025 17:15:36 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Bjørn Mork <bjorn@...k.no>
Cc: Maxime Chevallier <maxime.chevallier@...tlin.com>, davem@...emloft.net,
	Andrew Lunn <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com,
	Florian Fainelli <f.fainelli@...il.com>,
	Köry Maincent <kory.maincent@...tlin.com>,
	Simon Horman <horms@...nel.org>,
	Romain Gantois <romain.gantois@...tlin.com>,
	Antoine Tenart <atenart@...nel.org>,
	Marek Behún <kabel@...nel.org>
Subject: Re: [PATCH net-next 0/2] net: phy: sfp: Add single-byte SMBus SFP
 access

On Tue, Feb 25, 2025 at 01:38:30PM +0100, Bjørn Mork wrote:
> "Russell King (Oracle)" <linux@...linux.org.uk> writes:
> > On Sun, Feb 23, 2025 at 06:28:45PM +0100, Maxime Chevallier wrote:
> >> Hi everyone,
> >> 
> >> Some PHYs such as the VSC8552 have embedded "Two-wire Interfaces" designed to
> >> access SFP modules downstream. These controllers are actually SMBus controllers
> >> that can only perform single-byte accesses for read and write.
> >
> > This goes against SFF-8472, and likely breaks atomic access to 16-bit
> > PHY registers.
> >
> > For the former, I quote from SFF-8472:
> >
> > "To guarantee coherency of the diagnostic monitoring data, the host is
> > required to retrieve any multi-byte fields from the diagnostic
> > monitoring data structure (e.g. Rx Power MSB - byte 104 in A2h, Rx
> > Power LSB - byte 105 in A2h) by the use of a single two-byte read
> > sequence across the 2-wire interface."
> >
> > So, if using a SMBus controller, I think we should at the very least
> > disable exporting the hwmon parameters as these become non-atomic
> > reads.
> 
> Would SMBus word reads be an alternative for hwmon, if the SMBus
> controller support those?  Should qualify as "a single two-byte read
> sequence across the 2-wire interface."

Looking at the SNIA documents, this should work, so if a SMBus supports
two-byte reads, then we should be able to support hwmon with such a
controller. However, if only single-byte reads are supported, then I
think we should disable hwmon as we'd be going against the statement
I provided above from the docs.

> > Whether PHY access works correctly or not is probably module specific.
> > E.g. reading the MII_BMSR register may not return latched link status
> > because the reads of the high and low bytes may be interpreted as two
> > seperate distinct accesses.
> 
> Bear with me.  Trying to learn here.  AFAIU, we only have a defacto
> specification of the clause 22 phy interface over i2c, based on the
> 88E1111 implementation.  As Maxime pointed out, this explicitly allows
> two sequential distinct byte transactions to read or write the 16bit
> registers. See figures 27 and 30 in
> https://www.marvell.com/content/dam/marvell/en/public-collateral/transceivers/marvell-phys-transceivers-alaska-88e1111-datasheet.pdf

However, note that it's:

 START ADDR(W) REG RE-START ADDR(R) HIGH STOP
 START ADDR(W) REG RE-START ADDR(R) LOW STOP

and not:

 START ADDR(W) REG STOP START ADDR(R) HIGH STOP
 ...

I forget whether SMBus can do re-starts.

> Looks like the latch timing restrictions are missing, but I still do not
> think that's enough reason to disallow access to phys over SMBus.  If
> this is all the interface specification we have?

I'm not sure what you're referring to here with "latch timing
restrictions".

Reading more of the 88E1111, if one only transfers an odd number of
bytes for a register, it looks like that could cause problems. It
doesn't appear to specify what happens if it receives a write e.g.
for upper byte of register 0, and then receives a write for
register 4 (as a hypothetical example - caused where the lower byte of
register 0 transfer failed.)

We do need to think about these corner cases!

> I have been digging around for the RollBall protocol spec, but Google
> isn't very helpful.  This list and the mdio-i2c.c implementation is all
> that comes up.  It does use 4 and 6 byte transactions which will be
> difficult to emulate on SMBus.  But the
> 
> 	/* By experiment it takes up to 70 ms to access a register for these
> 	 * SFPs. Sleep 20ms between iterations and try 10 times.
> 	 */
> 
> comment in i2c_rollball_mii_poll() indicates that it isn't very timing
> sensitive at all. The RollBall SFP+ I have ("FS", "SFP-10G-T") is faster
> than the comment indicates, but still leaves plenty of time for the
> single byte SMBus transactions to complete.

The "RollBall" protocol isn't official afaics. It got called that
because of the first module that Marek happened to have that implemented
it. Having taken several of these modules apart, it's implemented by a
microcontroller, which may be an Arm Cortex based controller, or might
be 8051 based. Whether the firmware implementation at the high-level
code is the same or not is anyone's guess. We just don't know. It
could be entirely different implementations - and that's the problem.
We already know that the timings for this protocol vary depending on
the module (possibly because of differing implementations) which brings
questions up about the behaviour in obscure cases like single-byte
reads.

> Haven't found any formal specification of i2c clause 45 access either.
> But some SFP+ vendors have been nice enough to document their protocol
> in datasheets.  Examples:
> 
> https://www.repotec.com/download/managed-ethernet/ds-ml/01-MOD-M10GTP-DS-verB.pdf
> https://www.apacoe.com.tw/files/ASFPT-TNBT-X-NA%20V1.4.pdf
> 
> They all seem to agree that 2/4/6 byte accesses are required, and they
> offer no single byte alternative even if the presence of a "smart"
> bridge should allow intelligent latching.  So this might be
> "impossible" (aka "hard") to do over SMBus.   I have no such SFP+ so I
> cannot even try.

Indeed. So maybe we need a stronger kernel message to basically blame
the hardware designer and refuse to operate with modules that _might_
use the Rollball protocol. (Note, it's "might" because if we can't
access the PHY, we don't know for certain that this module is a
copper module.)

> > In an ideal world, I'd prefer to say no to hardware designs like this,
> > but unfortunately, hardware designers don't know these details of the
> > protocol, and all they see is "two wire, oh SMBus will do".
> 
> I believe you are reading more into the spec than what's actually there.

So I'm making up the quote above from SFF-8472.  Okay, if that's where
this discussion is going, I'm done here.

NAK to this patch set.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists