devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: "Marek Behún" <kabel@kernel.org>,
	netdev@vger.kernel.org, "Andrew Lunn" <andrew@lunn.ch>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"David Miller" <davem@davemloft.net>,
	"Rob Herring" <robh+dt@kernel.org>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH net-next 8/8] net: phy: marvell10g: select host interface configuration
Date: Thu, 18 Nov 2021 15:59:35 +0000	[thread overview]
Message-ID: <YZZ4Zy6Y8p/fGj5b@shell.armlinux.org.uk> (raw)
In-Reply-To: <20211118150951.jzwl5jickilxbfhy@skbuf>

On Thu, Nov 18, 2021 at 05:09:51PM +0200, Vladimir Oltean wrote:
> On Thu, Nov 18, 2021 at 02:47:44PM +0000, Russell King (Oracle) wrote:
> > You're going to get into problems with this on Layerscape, because
> > reconfiguring the Serdes etc is something I've tried to highlight
> > as being necessary to NXP since SolidRun started using LX2160A. I
> > think there's some slow progress towards that, but it's so slow that
> > I've basically given up caring about it on the Honeycomb/Clearfog CX
> > boards now.
> > 
> > All the SFP cages on my Honeycomb have been configured for the most
> > useful mode to me - 1000BASE-X/SGMII, and I've given up caring about
> > USXGMII/10GBASE-R on those ports.
> 
> Speaking of that, do you know of any SFP modules that would use USXGMII?
> It doesn't appear to be listed in the spec sheet when looking for that.

I only know of one possibility, which is the DM7052 which uses a
Broadcom BCM84881 PHY. I believe the PHY is capable of USXGMII (there's
various references to it on the 'net) to some extent. By that, I mean
it probably does not provide the 16-bit configuration word in USXGMII
mode - just like it doesn't provide it in SGMII mode.

On the DM7052 module, the PHY is not in USXGMII mode, it will switch
interface modes according to the media speed just like the Marvell
88X3310 does. So, it's vital to use the PHY specific driver for this
PHY, and not the generic driver.

Since the datasheet for the PHY is unavailable, it is not known how to
switch it to USXGMII mode, so it may not be possible to do so except
by modifying the pin strapping on the PHY. It may be possible to do it
through vendor registers.

> > > I see that this patch set basically introduces the phydev->host_interfaces
> > > bitmap which is an attempt to find the answer to that question. But when
> > > will we know enough about phydev->host_interfaces in order to safely
> > > make decisions in the PHY driver based on it? phylink sets it, phylib
> > > does not.
> > 
> > It won't be something phylib could set because phylib doesn't know
> > the capabilities of its user - it's information that would need to be
> > provided to phylib.
> 
> So you're saying it would be in phylib's best interest to not set it at
> all, not even to a single bit corresponding to phydev->interface. So PHY
> drivers could work out this way whether they should operate in backwards
> compatibility mode or they could change MACTYPE at will.

That's what I'm thinking - if we end up with a single bit set in the
host interface, does that mean "the host only supports a single
interface type" or does it mean "DT only specified one interface type
and we need to operate in backwards compatibility mode".

If we provide an empty host_interfaces bitmap, then we can easily
detect that it's not set and fallback to compatibility mode - in the
case of 88x3310, that would basically mean not attempting to set
MACTYPE.

> > > And many Aquantia systems use the generic PHY driver, as mentioned.
> > > Additionally, there are old device trees at play here, which only define
> > > the initial SERDES protocol. Would we be changing the behavior for those,
> > > in that we would be configuring the PHY to keep the SERDES protocol
> > > fixed whereas it would have dynamically changed before?
> > 
> > We have the same situation on Macchiatobin. The 88X3310 there defaults
> > to MACTYPE mode 4, and we've supported this for years with DT describing
> > the interface as 10GBASE-R - because we haven't actually cared very much
> > what DT says up to this point for the 88X3310. As I said in my previous
> > reply, the 88X3310 effectively dictates what the PHY interface mode will
> > be, and that is communicated back through phylib to whoever is using
> > phylib.
> 
> So what is the full backwards compatibility strategy with old DT blobs?
> Is it in this patch set? I didn't notice it.

Marek has attempted to create a backwards compatibility in phylink for
this. See phylink_update_phy_modes().

> > > Another question is what to do if there are multiple ways of
> > > establishing a system-side link. For example 1000 Mbps can be achieved
> > > either through SGMII, or USXGMII with symbol replication, or 2500base-x
> > > with flow control, or 10GBaseR with flow control. And I want to test
> > > them all. What would I need to do to change the SERDES protocol from one
> > > value to the other? Changing the phy-mode array in the device tree would
> > > be one option, but that may not always be possible.
> > 
> > First point to make here is that rate adaption at the PHY is really
> > not well supported in Linux, and there is no way to know via phylib if
> > a PHY is capable or not of rate adaption.
> > 
> > Today, if you have a 10GBASE-R link between a PHY doing rate adaption
> > and the "MAC", then what you will get from phylib is:
> > 
> > 	phydev->interface = PHY_INTERFACE_MODE_10GBASER;
> > 	phydev->speed = SPEED_1000;	// result of media negotiation
> > 	phydev->duplex = DUPLEX_FULL;	// result of media negotiation
> > 	phydev->pause = ...;		// result of media negotiation
> > 	phydev->asym_pause = ...;	// result of media negotiation
> > 
> > which will, for the majority of implementations, result in the MAC being
> > forced to a 1G speed, possibly with or without pause enabled.
> > 
> > Due to this, if phylink is being used, the parameters given to
> > mac_link_up/pcs_link_up will be the result of the media negotiation, not
> > what is required on the actual link.
> > 
> > You mention "10GBaseR with flow control" but there is another
> > possibility that exists in real hardware out there. "10GBaseR without
> > flow control" and in that case, the MAC needs to pace its transmission
> > for the media speed (which is a good reason why mac_link_up should be
> > given the result of the media negotiation so it can do transmission
> > pacing.)
> > 
> > I have a follow-up to the response I gave to Sean Anderson on rate-
> > adapting PHYs that I need to finish and send, and it would be better
> > to have any discussion on this topic after I've sent that reply and
> > follow-up to that reply.
> 
> Ok, how would the MAC pace itself to send at a lower data rate, if the
> SERDES protocol is 10G and the PHY doesn't send it PAUSE frames back?
> At least Layerscape systems can't do this AFAIK.

I'm afraid that is an exercise for the reader/MAC driver author since
it's dependent on the hardware.

One would hope that no one would create a system where the PHY needs to
use rate adaption and requires the MAC to pace itself, but the MAC has
no way to achieve that. If such a hardware combination exists, I don't
see how it could work reliably.

However, bear in mind that the 88X3310 on Macchiatobin boards is the
one without MACSEC, so if it is configured to only operate at 10GBASE-R
with rate adaption, then it will not be generating pause frames and it
will expect the MAC to pace. This is about the only platform I have
which I could experiment with a PHY performing rate adaption. However,
I'm not currently sure how useful that would be - it would be nothing
more than an experimentation exercise, and would require MAC pacing
to be implemented in the mvpp2 driver.

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

  parent reply	other threads:[~2021-11-18 15:59 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-17 22:50 [PATCH net-next 0/8] Extend `phy-mode` to string array Marek Behún
2021-11-17 22:50 ` [PATCH net-next 1/8] dt-bindings: ethernet-controller: support multiple PHY connection types Marek Behún
2021-11-18 16:27   ` Andrew Lunn
2021-11-17 22:50 ` [PATCH net-next 2/8] net: Update documentation for *_get_phy_mode() functions Marek Behún
2021-11-18 16:28   ` Andrew Lunn
2021-11-17 22:50 ` [PATCH net-next 3/8] device property: add helper function for getting phy mode bitmap Marek Behún
2021-11-18 16:31   ` Andrew Lunn
2021-11-17 22:50 ` [PATCH net-next 4/8] net: phylink: update supported_interfaces with modes from fwnode Marek Behún
2021-11-18  9:05   ` Russell King (Oracle)
2021-11-18 16:33     ` Andrew Lunn
2021-11-18 17:08       ` Russell King (Oracle)
2021-11-18 17:33         ` Marek Behún
2021-11-18 17:39           ` Russell King (Oracle)
2021-11-18 20:38   ` Sean Anderson
2021-11-17 22:50 ` [PATCH net-next 5/8] net: phylink: pass supported PHY interface modes to phylib Marek Behún
2021-11-18  9:32   ` Russell King (Oracle)
2021-11-18 16:27   ` Andrew Lunn
2021-11-18 16:28     ` Russell King (Oracle)
2021-11-17 22:50 ` [PATCH net-next 6/8] net: phy: marvell10g: Use generic macro for supported interfaces Marek Behún
2021-11-18  9:33   ` Russell King (Oracle)
2021-11-18 16:27   ` Andrew Lunn
2021-11-17 22:50 ` [PATCH net-next 7/8] net: phy: marvell10g: Use tabs instead of spaces for indentation Marek Behún
2021-11-18  9:34   ` Russell King (Oracle)
2021-11-17 22:50 ` [PATCH net-next 8/8] net: phy: marvell10g: select host interface configuration Marek Behún
2021-11-18  9:41   ` Russell King (Oracle)
2021-11-18 13:46     ` Marek Behún
2021-11-18 14:25       ` Russell King (Oracle)
2021-11-18 12:03   ` Vladimir Oltean
2021-11-18 13:22     ` Russell King (Oracle)
2021-11-18 14:20       ` Vladimir Oltean
2021-11-18 14:47         ` Russell King (Oracle)
2021-11-18 15:09           ` Vladimir Oltean
2021-11-18 15:20             ` Marek Behún
2021-11-18 15:59             ` Russell King (Oracle) [this message]
2021-11-18 13:56     ` Marek Behún

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YZZ4Zy6Y8p/fGj5b@shell.armlinux.org.uk \
    --to=linux@armlinux.org.uk \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=kabel@kernel.org \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.com \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).