Date:   Thu, 18 Nov 2021 16:20:39 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Marek Behún <kabel@...nel.org>,
        netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org
Subject: Re: [PATCH net-next 8/8] net: phy: marvell10g: select host interface
 configuration

On Thu, Nov 18, 2021 at 01:22:18PM +0000, Russell King (Oracle) wrote:
> On Thu, Nov 18, 2021 at 02:03:34PM +0200, Vladimir Oltean wrote:
> > On Wed, Nov 17, 2021 at 11:50:50PM +0100, Marek Behún wrote:
> > > +static int mv3310_select_mactype(unsigned long *interfaces)
> > > +{
> > > +	if (test_bit(PHY_INTERFACE_MODE_USXGMII, interfaces))
> > > +		return MV_V2_33X0_PORT_CTRL_MACTYPE_USXGMII;
> > > +	else if (test_bit(PHY_INTERFACE_MODE_SGMII, interfaces) &&
> > > +		 test_bit(PHY_INTERFACE_MODE_10GBASER, interfaces))
> > > +		return MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER;
> > > +	else if (test_bit(PHY_INTERFACE_MODE_SGMII, interfaces) &&
> > > +		 test_bit(PHY_INTERFACE_MODE_RXAUI, interfaces))
> > > +		return MV_V2_33X0_PORT_CTRL_MACTYPE_RXAUI;
> > > +	else if (test_bit(PHY_INTERFACE_MODE_SGMII, interfaces) &&
> > > +		 test_bit(PHY_INTERFACE_MODE_XAUI, interfaces))
> > > +		return MV_V2_3310_PORT_CTRL_MACTYPE_XAUI;
> > > +	else if (test_bit(PHY_INTERFACE_MODE_10GBASER, interfaces))
> > > +		return MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_RATE_MATCH;
> > > +	else if (test_bit(PHY_INTERFACE_MODE_RXAUI, interfaces))
> > > +		return MV_V2_33X0_PORT_CTRL_MACTYPE_RXAUI_RATE_MATCH;
> > > +	else if (test_bit(PHY_INTERFACE_MODE_XAUI, interfaces))
> > > +		return MV_V2_3310_PORT_CTRL_MACTYPE_XAUI_RATE_MATCH;
> > > +	else if (test_bit(PHY_INTERFACE_MODE_SGMII, interfaces))
> > > +		return MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER;
> > > +	else
> > > +		return -1;
> > > +}
> > > +
> > 
> > I would like to understand this heuristic better. Both its purpose and
> > its implementation.
> > 
> > It says:
> > (a) If the intersection between interface modes supported by the MAC and
> >     the PHY contains USXGMII, then use USXGMII as a MACTYPE
> > (b) Otherwise, if the intersection contains both 10GBaseR and SGMII, then
> >     use 10GBaseR as MACTYPE
> > (...)
> > (c) Otherwise, if the intersection contains just 10GBaseR (no SGMII), then
> >     use 10GBaseR with rate matching as MACTYPE
> > (...)
> > (d) Otherwise, if the intersection contains just SGMII (no 10GBaseR), then
> >     use 10GBaseR as MACTYPE (no rate matching).
> 
> What is likely confusing you is a misinterpretation of the constant.
> MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER actually means the PHY will
> choose between 10GBASE-R, 5GBASE-R, 2500BASE-X, and SGMII depending
> on the speed negotiated by the media. In this setting, the PHY
> dictates which interface mode will be used.
> 
> I could have named "MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER" as
> "MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_5GBASER_2500BASEX_SGMII_AUTONEG_ON".
> Similar with "MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_NO_SGMII_AN", which
> would be
> "MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_5GBASER_2500BASEX_SGMII_AUTONEG_OFF".
> And "MV_V2_3310_PORT_CTRL_MACTYPE_XAUI" would be
> "MV_V2_3310_PORT_CTRL_MACTYPE_XAUI_5GBASER_2500BASEX_SGMII_AUTONEG_ON".
> 
> Clearly using such long identifiers would have been rediculous,
> especially the second one at 74 characters.

True, but at least there could be a comment above each definition.
There's no size limit to that.

> > First of all, what is MACTYPE exactly? And what is the purpose of
> > changing it? What would happen if this configuration remained fixed, as
> > it were?
> 
> The PHY defines the MAC interface mode depending on the MACTYPE
> setting selected and the results of the media side negotiation.
> 
> I think the above answers your remaining questions.

Ok, so going back to case (d). You said that the full name would be
MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_5GBASER_2500BASEX_SGMII_AUTONEG_ON.
This means that when the only interface mode supported by the host would
be SGMII, the PHY's MACTYPE is still configured to use 2500basex,
5gbaser, 10gbaser for the higher link speeds. Clearly this won't work.
But on the other hand, the phylink validate method will remove
2500baseT, 5000baseT, 1000baseT from the advertising mask of the PHY, so
the system will never end up operating at those speeds, so it should be fine.



The reason why I'm looking at these patches is to see whether they would
bring something useful to Aquantia PHYs. These come with firmware on a
flash that is customized by Aquantia themselves based on the specifications
of a single board. These PHYs have an ability which is very similar to
what I'm seeing here, which is to select, for each negotiated link speed
on the media side, the SERDES protocol to use on the system side. This
is pre-programmed by the firmware, but could be fixed up by the
operating system if done carefully.

The way Layerscape boards use Aquantia PHYs is to always select the
"rate matching" option and keep the SERDES protocol fixed, just
configure the mini MAC inside the PHY to emit PAUSE frames towards the
system to keep the data rate under control. We would be using these PHYs
with the generic C45 driver, which would be mostly enough except for
lack of PHY interrupts, because the firmware already configures
everything.

But on the other hand it gets a bit tiring, especially for PHYs on riser
cards, to have to change firmware in order to test a different SERDES
protocol, so we were experimenting with some changes in the PHY driver
that would basically keep the firmware image fixed, and just fix up the
configuration it made, and do things like "use 2500base-x for the
2500base-T speed, and sgmii for 1000base-T/100base-TX". The ability for
a PHY to work on a board where its firmware image wasn't specifically
designed for it comes in handy sometimes.

I was reluctant to submit any changes to the Aquantia PHY driver because
we don't really have any guarantees that we wouldn't break any system
that uses it. I do know that there are users who do expect SERDES
protocol changes currently, because I do see that aqr107_read_status()
checks for a possibly modified phydev->interface. But without the
ability of knowing what SERDES protocols does the system-side support,
it is pretty difficult to modify the SERDES protocol used for a certain
link speed without breaking something.

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

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.

Powered by blists - more mailing lists