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:

quoted

On Wed, Jan 03, 2018 at 04:20:36PM +0100, Andrew Lunn wrote:

quoted

quoted

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

quoted

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)

quoted

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