Message-ID: <CA+h21hqp4L5z3QQ6KV+TkegVg_i+veEXRLSmv7670Tw22t2sSA@mail.gmail.com>
Date:   Sat, 24 Aug 2019 15:49:30 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
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

Hi Russell,

On Tue, 30 Jul 2019 at 13:23, Russell King - ARM Linux admin
<linux@...linux.org.uk> wrote:
>
> 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.
>

Mystery solved - indeed it appears that there is no hardware logic for
propagating the AN results from the PCS to the MAC. Hence there is no
hardware distinction between 1000Base-X and SGMII. That must be
handled in PHYLINK.

> > > +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.
>

Or that.

> > > +               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.)
>

Ok.

> > > +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...
>

Yes, gfar_configure_serdes does this:

    phy_write(tbiphy, MII_BMCR,
          BMCR_ANENABLE | BMCR_ANRESTART | BMCR_FULLDPLX |
          BMCR_SPEED1000);

> >
> > > +
> > > +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.
>

I don't think AN pages count as Ethernet frames, and the RX_EN and
TX_EN bits are MAC settings anyway - it is the PCS below it that would
process them.

> > > @@ -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
>
> ?

Or that, yes.

>
> --
> 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

Thanks,
-Vladimir

Powered by blists - more mailing lists