Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 22 Jul 2022 09:45:26 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Sean Anderson <sean.anderson@seco.com>
Cc:     netdev@vger.kernel.org, Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Alexandru Marginean <alexandru.marginean@nxp.com>,
        Paolo Abeni <pabeni@redhat.com>,
        "David S . Miller" <davem@davemloft.net>,
        linux-kernel@vger.kernel.org, Vladimir Oltean <olteanv@gmail.com>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>
Subject: Re: [PATCH v2 07/11] net: phylink: Adjust link settings based on
 rate adaptation
Message-ID: <YtpjpowrI3VEyGs2@shell.armlinux.org.uk>
References: <20220719235002.1944800-1-sean.anderson@seco.com>
 <20220719235002.1944800-8-sean.anderson@seco.com>
 <YtelzB1uO0zACa42@shell.armlinux.org.uk>
 <YthKSYje5e+swg08@shell.armlinux.org.uk>
 <84f4f37e-044c-0fd8-7ba4-cba54200d9fe@seco.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <84f4f37e-044c-0fd8-7ba4-cba54200d9fe@seco.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Thu, Jul 21, 2022 at 05:48:05PM -0400, Sean Anderson wrote:
> On 7/20/22 2:32 PM, Russell King (Oracle) wrote:
> > On Wed, Jul 20, 2022 at 07:50:52AM +0100, Russell King (Oracle) wrote:
> >> We can do that by storing the PHY rate adaption state, and processing
> >> that in phylink_link_up().
> > 
> > Something like this? I haven't included the IPG (open loop) stuff in
> > this - I think when we come to implement that, we need a new mac
> > method to call to set the IPG just before calling mac_link_up().
> > Something like:
> > 
> >  void mac_set_ipg(struct phylink_config *config, int packet_speed,
> > 		  int interface_speed);
> > 
> > Note that we also have PCS that do rate adaption too, and I think
> > fitting those in with the code below may be easier than storing the
> > media and phy interface speed/duplex separately.
> 
> This is another area where the MAC has to know a lot about the PCS.
> We don't keep track of the PCS interface mode, so the MAC has to know
> how to connect to the PCS. That could already include some rate
> adaptation, but I suspect it is all done like GMII (where the clock
> speed changes).

In many cases, we don't even know what the interface used to connect the
PCS to the MAC actually is (we'd have to use something like _INTERNAL).
Particularly when the PCS and MAC are integrated on the same die,
manufacturers tend not to tell people how the two blocks are connected.

Even if we assume did use GMII internally for everything (and I mean
everything), then decoding the GMII interface mode to mean SPEED_1000
won't work for anything over 1G speeds - so we can't do that. The
more I think about it, the less meaning the interface mode between
the PCS and MAC is for our purposes - unless we positively know for
every device what that mode is, and can reliably translate that into
the speed of that connection to derive the correct "speed" for the
MAC.

The point of bringing this up was just to bear it in mind, and I
think when we add support for this, then...

> >  static void phylink_link_up(struct phylink *pl,
> >  			    struct phylink_link_state link_state)
> >  {
> >  	struct net_device *ndev = pl->netdev;
> > +	int speed, duplex;
> > +	bool rx_pause;
> > +
> > +	speed = link_state.speed;
> > +	duplex = link_state.duplex;
> > +	rx_pause = !!(link_state.pause & MLO_PAUSE_RX);
> > +
> > +	switch (state->rate_adaption) {
> > +	case RATE_ADAPT_PAUSE:
> > +		/* The PHY is doing rate adaption from the media rate (in
> > +		 * the link_state) to the interface speed, and will send
> > +		 * pause frames to the MAC to limit its transmission speed.
> > +		 */
> > +		speed = phylink_interface_max_speed(link_state.interface);
> > +		duplex = DUPLEX_FULL;
> > +		rx_pause = true;
> > +		break;
> > +
> > +	case RATE_ADAPT_CRS:
> > +		/* The PHY is doing rate adaption from the media rate (in
> > +		 * the link_state) to the interface speed, and will cause
> > +		 * collisions to the MAC to limit its transmission speed.
> > +		 */
> > +		speed = phylink_interface_max_speed(link_state.interface);
> > +		duplex = DUPLEX_HALF;
> > +		break;
> > +	}
> >  
> >  	pl->cur_interface = link_state.interface;
> >  
> >  	if (pl->pcs && pl->pcs->ops->pcs_link_up)
> >  		pl->pcs->ops->pcs_link_up(pl->pcs, pl->cur_link_an_mode,
> > -					 pl->cur_interface,
> > -					 link_state.speed, link_state.duplex);
> > +					 pl->cur_interface, speed, duplex);
> >  

... we would want to update the speed, duplex and rx_pause parameters
here for the MAC.

> >  	pl->mac_ops->mac_link_up(pl->config, pl->phydev,
> >  				 pl->cur_link_an_mode, pl->cur_interface,
> > -				 link_state.speed, link_state.duplex,
> > +				 speed, duplex,
> >  				 !!(link_state.pause & MLO_PAUSE_TX),
> > -				 !!(link_state.pause & MLO_PAUSE_RX));
> > +				 rx_pause);

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