Messages in this thread | | | Subject | Re: [PATCH net-next v3 08/47] net: phylink: Support differing link speeds and interface speeds | From | Sean Anderson <> | Date | Mon, 18 Jul 2022 11:49:25 -0400 |
| |
On 7/16/22 9:26 PM, Andrew Lunn wrote: >> > 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. > >> > 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
| |