Message-ID: <20190730102329.GZ1330@shell.armlinux.org.uk>
Date:   Tue, 30 Jul 2019 11:23:29 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Arseny Solokha <asolokha@...kras.ru>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Andrew Lunn <andrew@...n.ch>, netdev <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [RFC PATCH 1/2] gianfar: convert to phylink

On Tue, Jul 30, 2019 at 02:39:58AM +0300, Vladimir Oltean wrote:
> To be honest I don't have a complete answer to that question. The
> literature recommends writing 0x01a0 to the MII_ADVERTISE (0x4)
> register of the MAC PCS for 1000Base-X, and 0x4001 for SGMII.

That looks entirely sane for both modes.

0x01a0 for 802.3z (1000BASE-X) is defined in 802.3 37.2.5.1.3 as:
	bit 5 - full duplex = 1
	bit 6 - half duplex = 0
	bit 7 - pause = 1
	bit 8 - asym_pause = 1

The description of the bits match the config word that is sent in the
link with the exception of bit 14, which is the acknowledgement bit.
Normally, in 802.3z, bit 14 will not be set in the transmitted config
word until we have received the config word from the other end of the
link.

For SGMII, 0x4001 is the acknowledgement code word, which is defined
in the SGMII spec as "tx_config_Reg[15:0] sent from the MAC to the
PHY" which requires:
	bit 0 - must be 1
	bit 1 .. 13, 15 - must be 0, reserved for future use
	bit 14 - must be 1 (auto-negotiation acknowledgement in 802.3z)

> The FMan driver which uses the TSEC MAC does exactly that:
> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/drivers/net/ethernet/freescale/fman/fman_dtsec.c#n58
> However I can't seem to be able to trace down the definition of bit 14
> from 0x4001 - it's reserved in all the manuals I see. I have a hunch
> that it selects the format of the Config_Reg base page between
> 1000Base-X and SGMII.

It could be that is what bit 14 is being used for, or it could just be
that they've taken the values from the appropriate specs, and the
hardware behaves the same way irrespective of whether it is in SGMII
or 1000BASE-X mode.

> > +static int gfar_mac_link_state(struct phylink_config *config,
> > +                              struct phylink_link_state *state)
> > +{
> > +       if (state->interface == PHY_INTERFACE_MODE_SGMII ||
> > +           state->interface == PHY_INTERFACE_MODE_1000BASEX) {
> 
> What if you reduce the indentation level by 1 here, by just exiting if
> the interface mode is not SGMII?

It's only called for in-band negotiation modes anyway, so the above
protection probably doesn't gain much.

> > +               struct gfar_private *priv =
> > +                       netdev_priv(to_net_dev(config->dev));
> > +               u16 tbi_cr;
> > +
> > +               if (!priv->tbi_phy)
> > +                       return -ENODEV;
> > +
> > +               tbi_cr = phy_read(priv->tbi_phy, MII_TBI_CR);
> > +
> > +               state->duplex = !!(tbi_cr & TBI_CR_FULL_DUPLEX);
> 
> Woah there. Aren't you supposed to first ensure state->an_complete is
> ok, based on TBI_MII_Register_Set_SR[AN_Done]? There's also a
> Link_Status bit in that register that you could retrieve.

Indeed.

> > +               if ((tbi_cr & TBI_CR_SPEED_1000_MASK) == TBI_CR_SPEED_1000_MASK)
> > +                       state->speed = SPEED_1000;
> > +       }
> 
> See the Speed_Bit table from TBI_MII_Register_Set_ANLPBPA_SGMII for
> the link partner (aka SGMII PHY) advertisement. You have to do a
> logical-and between that and your own. Also please make sure you
> really are in SGMII AN and not 1000 Base-X when interpreting registers
> 4 & 5 as one way or another.

>From what you've said above, yes, this needs to do exactly that.

> > -static noinline void gfar_update_link_state(struct gfar_private *priv)
> > +static void gfar_mac_config(struct phylink_config *config, unsigned int mode,
> > +                           const struct phylink_link_state *state)
> >  {
> > +       struct gfar_private *priv = netdev_priv(to_net_dev(config->dev));
> >         struct gfar __iomem *regs = priv->gfargrp[0].regs;
> > -       struct net_device *ndev = priv->ndev;
> > -       struct phy_device *phydev = ndev->phydev;
> > -       struct gfar_priv_rx_q *rx_queue = NULL;
> > -       int i;
> > +       u32 maccfg1, new_maccfg1;
> > +       u32 maccfg2, new_maccfg2;
> > +       u32 ecntrl, new_ecntrl;
> > +       u32 tx_flow, new_tx_flow;
> 
> Don't introduce new_ variables. Is there any issue if you
> unconditionally write to the MAC registers?

We do this in every driver, as mac_config() can be called with only a
small number of changes in the settings, and it is important not to
upset the MAC for minor updates.

An example of this is when we are in SGMII mode with an attached PHY.
SGMII will communicate the speed and duplex, but not the results of
the pause negotiation.  We read that from the attached PHY and report
it back by calling mac_config() - but at that point, we don't want to
cause the established link to bounce.

So, mac_config() should be implemented to avoid upsetting an already
established link where possible (unless the configuration items that
affect the link have changed.)

> > +static void gfar_mac_an_restart(struct phylink_config *config)
> > +{
> > +       /* Not supported */
> > +}
> 
> What about running gfar_configure_serdes again?

The intention here is to cause the 802.3z link to renegotiate...

> 
> > +
> > +static void gfar_mac_link_down(struct phylink_config *config, unsigned int mode,
> > +                              phy_interface_t interface)
> > +{
> > +       /* Not supported */
> > +}
> > +
> 
> What about disabling RX_EN and TX_EN from MACCFG1?
> 
> > +static void gfar_mac_link_up(struct phylink_config *config, unsigned int mode,
> > +                            phy_interface_t interface, struct phy_device *phy)
> > +{
> > +       /* Not supported */
> >  }
> >
> 
> What about enabling RX_EN and TX_EN from MACCFG1?

Note that both of these functions must still allow the link to be
established if we are using in-band negotiation - but they are
expected to start/stop the transmission of packets.

> > @@ -149,8 +148,13 @@ extern const char gfar_driver_version[];
> >  #define GFAR_SUPPORTED_GBIT SUPPORTED_1000baseT_Full
> >
> >  /* TBI register addresses */
> > +#define MII_TBI_CR             0x00
> >  #define MII_TBICON             0x11
> >
> > +/* TBI_CR register bit fields */
> > +#define TBI_CR_FULL_DUPLEX     0x0100
> > +#define TBI_CR_SPEED_1000_MASK 0x0040
> > +
> 
> I think BIT() definitions are preferred, even if that means you have
> to convert existing code first.

If MII_TBI_CR is the BMCR of the PCS, how about using the definitions
that we already have in the kernel:

BMCR_SPEED1000 is TBI_CR_SPEED_1000_MASK
BMCR_FULLDPLX is TBI_CR_FULL_DUPLEX

?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Powered by blists - more mailing lists