On 7/16/22 9:26 PM, Andrew Lunn wrote:

quoted

quoted

This seem error prone when new PHY_INTERFACE_MODES are added. I would
prefer a WARN_ON_ONCE() in the default: so we get to know about such
problems.

Actually, this is the reason I did not add a default: clause to the
switch (and instead listed everything out). If a new interface mode is
added, there will be a warning (as I discovered when preparing this
patch).

Ah, the compiler produces a warning. O.K. that is good. Better than an
WARN_ON_ONCE at runtime.

quoted

quoted

Bike shedding a bit, but would it be better to use host_side_speed and
line_side_speed? When you say link_speed, which link are your
referring to? Since we are talking about the different sides of the
PHY doing different speeds, the naming does need to be clear.

When I say "link" I mean the thing that the PMD speaks. That is, one of
the ethtool link mode bits. I am thinking of a topology like


MAC (+PCS) <-- phy interface mode (MII) --> phy <-- link mode --> far-end phy

The way it has been done up to now, the phy interface mode and the link
mode have the same speed. For some MIIs, (such as MII or GMII) this is
actually the case, since the data clock changes depending on the data
speed. For others (SGMII/USXGMII) the data is repeated, but the clock
rate stays the same. In particular, the MAC doesn't care what the actual
link speed is, just what configuration it has to use (so it selects the
right clock etc).

The exception to the above is when you have no phy (such as for
1000BASE-X):

MAC (+PCS) <-- MDI --> PMD <-- link mode --> far-end PMD

All of the phy interface modes which can be used this way are
"non-adaptive." That is, in the above case they have a fixed speed.

That said, I would like to keep the "phy interface mode speed" named
"speed" so I don't have to write up a semantic patch to rename it in all
the drivers.

So you want phydev->speed to be the host side speed. That leaves the
line side speed as a new variable, so it can be called line_side_speed?

I just find link_speed ambiguous, and line_side_speed less so.

I would rather use something with "link" to match up with
ETHTOOL_LINK_MODE_*. Ideally "speed" would be something like
"interface_speed" to match up with PHY_INTERFACE_MODE_*.

The documentation for phydev->speed needs updating to make it clear it
is the host side speed.

OK

--Sean