lkml.org 
[lkml]   [2020]   [Oct]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3] net: macb: add support for high speed interface
On Wed, Oct 21, 2020 at 07:44:05PM +0200, Parshuram Thombare wrote:
> This patch adds support for 10GBASE-R interface to the linux driver for
> Cadence's ethernet controller.
> This controller has separate MAC's and PCS'es for low and high speed paths.
> High speed PCS supports 100M, 1G, 2.5G, 5G and 10G through rate adaptation
> implementation. However, since it doesn't support auto negotiation, linux
> driver is modified to support 10GBASE-R insted of USXGMII.
>
> Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
> ---
> Changes between v2 and v3:
> 1. Replace USXGMII interface by 10GBASE-R interface.
> 2. Adopted new phylink pcs_ops for high speed PCS.
> 3. Added pcs_get_state for high speed PCS.

Hi,

If you're going to support pcs_ops for the 10GBASE-R mode, can you
also convert macb to use pcs_ops for the lower speed modes as well?

The reason is that when you have pcs_ops in place, there are slight
behaviour differences in the way phylink calls the MAC functions,
and in the long run I would like to eventually retire the old ways
(so we don't have to keep compatible with old in-kernel APIs alive
for ever.)

I think all that needs to happen is a pcs_ops for the non-10GBASE-R
mode which moves macb_mac_pcs_get_state() and macb_mac_an_restart()
to it, and implements a stub pcs_config(). So it should be simple
to do.

Further comments below.

> +static int macb_usx_pcs_config(struct phylink_pcs *pcs,
> + unsigned int mode,
> + phy_interface_t interface,
> + const unsigned long *advertising,
> + bool permit_pause_to_mac)
> +{
> + struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
> + u32 val;
> +
> + val = gem_readl(bp, NCFGR);
> + val = GEM_BIT(PCSSEL) | (~GEM_BIT(SGMIIEN) & val);
> + gem_writel(bp, NCFGR, val);

This looks like it's configuring the MAC rather than the PCS - it
should probably be in mac_prepare() or mac_config().

Note that the order of calls when changing the interface mode will
be:

- mac_prepare()
- mac_config()
- pcs_config()
- pcs_an_restart() optionally
- mac_finish()

> +static int macb_mac_prepare(struct phylink_config *config, unsigned int mode,
> + phy_interface_t interface)
> +{
> + struct net_device *ndev = to_net_dev(config->dev);
> + struct macb *bp = netdev_priv(ndev);
> +
> + if (interface == PHY_INTERFACE_MODE_10GBASER) {
> + bp->phylink_pcs.ops = &macb_phylink_usx_pcs_ops;
> + phylink_set_pcs(bp->phylink, &bp->phylink_pcs);
> + }

What happens if phylink requests 10GBASE-R and subsequently selects
a different interface mode? You end up with the PCS still registered
and phylink will use it even when not in 10GBASE-R mode - so its
functions will also be called.

Note also that if a PCS is registered, phylink will omit to call
mac_config() unless the interface mode is being changed. If no PCS
is registered, it will call mac_config() in the old way (which
includes link up events.)

Thanks.

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

\
 
 \ /
  Last update: 2020-10-21 20:51    [W:0.080 / U:2.276 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site