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:5.125 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site