Message-ID: <20200701090607.GI1551@shell.armlinux.org.uk>
Date:   Wed, 1 Jul 2020 10:06:07 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Baruch Siach <baruch@...s.co.il>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Shmuel Hazan <sh@...s.co.il>
Subject: Re: [PATCH v2] net: phy: marvell10g: support XFI rate matching mode

On Wed, Jul 01, 2020 at 10:23:12AM +0300, Baruch Siach wrote:
> Hi Russell,
> 
> On Mon, Jun 29 2020, Russell King - ARM Linux admin wrote:
> > Then there's the whole question of what phydev->speed etc should be set
> > to - the media speed or the host side link speed with the PHY, and then
> > how the host side should configure itself.  At least the 88E6390x
> > switch will force itself to the media side speed using that while in
> > XAUI mode, resulting in a non-functioning speed.  So should the host
> > side force itself to 10G whenever in something like XAUI mode?
> 
> How does the switch discover the media side speed? Is there some sort of
> in-band information exchange?

The media-side results are passed via phydev->speed and phydev->duplex,
and therefore will be passed through phylink. mvpp2 will ignore them
for 10GBASE-R as it has separate MACs - XLG and GMAC, but 88E6390x, it's
just one.  Consequently, it's possible that the port mode is in XAUI,
but you can force the speed to (e.g.) 100M.

What I know from what I can do with this media-side broken 88X3310, is
that it will pass data if the 88E6390x is forced to 10G, but not if it's
forced to 100M.

We're moving from a situation where MAC drivers can expect (with either
phylib or phylink):

	interface = <some 10G interface>
	speed is always 10G
	duplex is always full

to:

	interface = <some 10G interface>
	speed is 10, 100, 1G or 10G
	duplex is half or full

So, adding rate-matching brings with it a non-obvious change in the API
of phylib and phylink:

* what do the phydev->{speed,duplex,pause,asym_pause} represent - the
  media side parameters or the PHY to MAC parameters?
* what do the "speed, duplex, pause" passed into mac_link_up() refer to,
  the media side, or the link side?
  
Both of those need to be properly documented and explained.

The next two points, I haven't re-read the 3310 datasheet.

We also need to consider a situation which is less obvious: if the PHY
is operating in rate matching mode, doesn't generate pause frames
itself as its rate matching buffers fill, but the media side negotiated
for pause frames.  Should we be advertising no support for pause frames
in this case?  Will the PHY pass pause frames through as a priority?
Consider that in a 16k outbound buffer, there could be up to 10 full
sized frames queued, so if the link partner is asking us to stop
sending, it could take up to 10 frames before we actually stop.

What are the requirements for half duplex in rate matching mode - is
that handled internally by the PHY, or do we need to disable all half
duplex advertisements in the PHY. When rate matching, the PHY can no
longer signal the MAC when a collision occurs, as it would normally
do without rate matching.

I think I've covered everything, but may have missed something. I do
think we need documentation updated before we should accept this patch
so that we have the phylib and phylink behaviour in this case clearly
defined from the outset.

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

Powered by blists - more mailing lists