Re: [PATCH net-next v2 4/4] net: mvpp2: 2500baseX support

[Antoine Tenart <antoine.tenart@free-electrons.com>]

Hi Andrew,

On Wed, Jan 03, 2018 at 04:53:11PM +0100, Andrew Lunn wrote:
> On Wed, Jan 03, 2018 at 04:32:27PM +0100, Antoine Tenart wrote:
> > On Wed, Jan 03, 2018 at 04:20:36PM +0100, Andrew Lunn wrote:
> > > > @@ -4612,6 +4616,9 @@ static int mvpp22_comphy_init(struct mvpp2_port *port)
> > > >  	case PHY_INTERFACE_MODE_1000BASEX:
> > > >  		mode = PHY_MODE_SGMII;
> > > >  		break;
> > > > +	case PHY_INTERFACE_MODE_2500BASEX:
> > > > +		mode = PHY_MODE_2500SGMII;
> > > > +		break;
> > > 
> > > I think this is the source of confusion with linux/phy.h and
> > > linux/phy/phy.h.
> > > 
> > > What would PHY_INTERFACE_MODE_2500SGMII use?
> > > 
> > > Where is this all getting confused? Should the caller to
> > > mvpp22_comphy_init() actually be passing PHY_INTERFACE_MODE_2500SGMII?
> > > What is the MAC actually doing at this point? 2500BASEX or 2500SGMII?
> > 
> > PHY_INTERFACE_MODE_2500BASEX is the PHY mode whereas PHY_MODE_2500SGMII
> > is the mode used by the common PHY driver (i.e. the one configuring the
> > serdes lanes).
> 
> > There's no PHY_INTERFACE_MODE_2500SGMII mode.
> 
> At the moment there is no PHY_INTERFACE_MODE_2500SGMII. However,
> there are some devices which can do 2.5G SGMII. So it will appear
> sometime. This piece of code then looks even stranger.

True. As said by Stefan the Marvell common PHY configures the serdes
lanes to a given mode, regardless of the actual use of the lanes by the
physical layer. So these modes are valid:

PPv2 (SGMII) - COMPHY (SGMII)
PPv2 (1000BaseX) - COMPHY (SGMII)
PPv2 (2500BaseX) - COMPHY (2500SGMII)

> > Sure, I can add a comment to state this function is a translation
> > between the net PHY mode and the generic PHY mode (it's a n-to-1
> > translation).
> 
> I think from an API design point of view, passing PHY_MODE_2500BASEX
> to comphy makes more sense. That is what the MAC wants to do. How the
> comphy achieves that should be internal to the comphy.

I though about that. The reason I did not is I wanted to use the
existing generic PHY framework helpers. They take a generic PHY mode in
input. I also wanted to avoid having a custom callback between the PPv2
driver and the COMPHY driver as phy_set_mode() seems to perfectly fit
the need.

The issue really is there are two PHY frameworks, one for network
devices and one for the other ones. The COMPHY is used by network
controllers, but also by SATA or USB ones.

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com