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