Re: [PATCH v2 3/3] net: macb: add support for high speed interface

[Russell King - ARM Linux admin]

On Mon, Dec 16, 2019 at 12:49:59PM +0000, Milind Parab wrote:
> >> > +	if (bp->phy_interface == PHY_INTERFACE_MODE_USXGMII) {
> >>
> >> Why bp->phy_interface and not state->interface?
> 
> okay, this needs to change to state->interface
> 
> >>
> >> If you don't support selecting between USXGMII and other modes at
> >> runtime, should macb_validate() be allowing ethtool link modes for
> >> it when it's different from the configured setting?
> 
> We have separate SGMII and USXGMII PCS, which are enabled and programmed 
> by MAC driver. Also, there are separate low speed (up to 1G) and high 
> speed MAC which can be programmed though MAC driver. 
> As long as, PHY (PMA, external to Cadence MAC controller) can handle 
> this change, GEM can work with interface changes at a runtime.
> 
> >>
> >> > +		if (gem_mac_usx_configure(bp, state) < 0) {
> >> > +			spin_unlock_irqrestore(&bp->lock, flags);
> >> > +			phylink_mac_change(bp->phylink, false);
> >>
> >> I guess this is the reason you're waiting for the USXGMII block
> >> to lock - do you not have any way to raise an interrupt when
> >> something changes with the USXGMII (or for that matter SGMII)
> >> blocks?  Without that, you're fixed to a single speed.
> 
> Yes, we need to wait (poll) until USXGMII block lock is set.
> Interrupt for USXGMII block lock set event is not supported.

You should poll for that status. We already have some polling support
in phylink (in the case of a fixed link using the callback, or a GPIO
that has no interrupt support) so it probably makes sense to extend
that functionality for MACs that do not provide status interrupts.

> >BTW, if you don't have an macb_mac_pcs_get_state() implementation,
> >and from what you described last time around, I don't see how SGMII
> >nor this new addition of USXGMII can work for you. Both these
> >protocols use in-band control words, which should be read and
> >interpreted in macb_mac_pcs_get_state().
> >
> >What I think you're trying to do is to use your PCS PHY as a normal
> >PHY, or maybe you're ignoring the PCS PHY completely and relying on
> >an external PHY (and hence always using MLO_AN_PHY or MLO_AN_FIXED
> >mode.)
> 
> We are limiting our functionality to 10G fixed link using PCS and SFP+
> Though the Cadence MAC is a full functional ethernet MAC controller, 
> we are not sure what PHY or PCS be used in the end system.
> Hence we are using PCS PHY as a normal PHY and not dependent on 
> macb_mac_pcs_get_state().

If you use the PCS PHY as a normal PHY, then this knocks out the
idea below of using phylib to access the external PHY - it is not
possible to stack two phylib controlled PHYs sanely on one network
device.

> Also it should be noted that we are 
> not doing any change in SGMII. Status available in PCS is 
> just a "status transferred" from PHY. So in case of SGMII, whether 
> we read from PCS or from PHY, it is the same information.

So how do you plan to deal with a PHY that you can't read the
status from? This is where the SGMII in-band is required.  Such
SFP modules do exist.

> Below are listed all the possible use cases of Cadence GEM 10G controller
> 
> Basic MII MAC/PHY interconnect using MDIO for link status xfer.
>  +-------------+                                    +--------+
>  |             |                                    |        |
>  | GEM MAC/DMA | <------ GMII/RGMII/RMII/MII -----> |  PHY   |
>  |             |                                    |        |
>  +-------------+                                    +--------+
>        ^                                                 ^
>        |_____________________ MDIO ______________________|
> 
> No PHY. No status xfer required. GEM PCS responsible for auto-negotiation
> across link. Driver must interrogate PCS registers within GEM.
>  +-------------+                                    +--------+
>  |             |       |        |                   |        |
>  | GEM MAC/DMA | <---> | SerDes | <- 1000BASE-X ->  |  SFP   |
>  |    PCS      |       | (PMA)  |                   |        |
>  +-------------+                                    +--------+      

This setup requires macb_mac_pcs_get_state() to be implemented to
interrogate the PCS at the MAC.  Note that some SFPs require SGMII,
and others can also operate at 2500baseX.

> SGMII MAC/PHY interconnect using MDIO for link status xfer.
>  +-------------+                                    +--------+
>  |             |       |        |                   |        |
>  | GEM MAC/DMA | <---> | SerDes | <--- SGMII --->   |  PHY   |
>  |  SGMII PCS  |       | (PMA)  |                   |        |
>  +-------------+                                    +--------+
>        ^                                                 ^
>        |_____________________ MDIO ______________________|
> 
> SGMII MAC/PHY interconnect using inline status xfer. Multi-rate.
> Driver must interrogate PCS registers within GEM.
>  +-------------+                                    +--------+
>  |             |       |        |                   |        |
>  | GEM MAC/DMA | <---> | SerDes | <--- SGMII --->   |  PHY   |
>  |  SGMII PCS  |       | (PMA)  |                   |        |
>  +-------------+                                    +--------+

This setup requires macb_mac_pcs_get_state() to be implemented to
interogate the SGMII PCS at the MAC.

> Up to 2.5G. MAC/PHY interconnect. Rate determined by 2.5GBASE-T PHY capability.
>  +--------------+                                  +-----------+
>  |              |       |        |                 |           |
>  | GEM MAC/DMA  | <---> | SerDes | <-2500BASE-X->  |2.5GBASE-T |
>  |2.5GBASE-X PCS|       | (PMA)  |                 |   PHY     |
>  +--------------+                                  +-----------+

This is fixed at 2.5G speeds.

> No ability for host to interrogate Optical.
>  +--------------+                                  +-----------+
>  |              |       |        |                 |  SFP+     |
>  | GEM MAC/DMA  | <---> | SerDes | <---- SFI-----> | Optical   |
>  |   USX PCS|   |       | (PMA)  |                 | Module    |
>  +--------------+                                  +-----------+
> 
> Additional 3rd party I2C IP required (not part of GEM) for module
> interrogation (MDIO to I2C handled by SW
>  +--------------+                                  +-----------+
>  |              |       |        |                 |  SFP+     |
>  | GEM MAC/DMA  | <---> | SerDes | <---- SFI-----> | Optical   |
>  |   USX PCS|   |       | (PMA)  |                 | Module    |
>  +--------------+                                  +-----------+
>                                                          ^
>         +--------+                                       |
>         | I2C    |                                       |
>         | Master | <-------------------------------------|
>         +--------+

The kernel supports this through the sfp and phylink support. SFI is
more commonly known as 10GBASE-R. Note that this is *not* USXGMII.
Link status needs to come from the MAC side, so macb_mac_pcs_get_state()
is required.

> Rate determined by 10GBASE-T PHY capability through auto-negotiation. 
> I2C IP required
>  +--------------+                                  +-----------+
>  |              |       |        |                 |  SFP+ to  |
>  | GEM MAC/DMA  | <---> | SerDes | <---- SFI-----> | 10GBASE-T |
>  |   USX PCS|   |       | (PMA)  |                 |           |
>  +--------------+                                  +-----------+
>                                                          ^
>         +--------+                                       |
>         | I2C    |                                       |
>         | Master | <-------------------------------------|
>         +--------+

The 10G copper module I have uses 10GBASE-R, 5000BASE-X, 2500BASE-X,
and SGMII (without in-band status), dynamically switching between
these depending on the results of the copper side negotiation.

> USXGMII PHY. Uses MDIO or equivalent for status xfer
>  +-------------+                                    +--------+
>  |             |       |        |                   |        |
>  | GEM MAC/DMA | <---> | SerDes | <--- USXGMII ---> |  PHY   |
>  |  USX PCS    |       | (PMA)  |                   |        |
>  +-------------+                                    +--------+
>        ^                                                 ^
>        |_____________________ MDIO ______________________|

Overall, please implement phylink properly for your MAC, rather than
the current half-hearted approach that *will* break in various
circumstances.

I think part of the problem here is that you have a different view how
phylink should be used to cover all these cases from my view. I'm not
prepared to guarantee that the phylink code will work with your view
into the future. I would much prefer there to be consistency in the
way phylink is used between implementations so we don't end up with
major maintanence problems into the future, so having consistency is
important.

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