On 23.09.2023 01:29, Russell King (Oracle) wrote:
On Sat, Sep 23, 2023 at 12:57:52AM +0300, Arınç ÜNAL wrote:
I agree. My patch description here failed to explain the actual issue,
which is missing hardware descriptions. Here's what I understand. An
ethernet-controller is a MAC. For the MAC to work properly with its link
partner, at least one of these must be described:
- pointer to a PHY to retrieve link information from the PHY
- pointer to a PCS to retrieve link information from the PCS
- pointer to an SFP to retrieve link information from the SFP
- static link information
What about something like macb? The macb driver:
- attempts to connect a phy using phylink_of_phy_connect()
- if that fails, and there is no phy-handle property, then the driver
will attempt to find the first PHY to exist on its MII bus, and will
connect that using phylink_connect_phy().
So, in this case, if we define a phylink binding to require one of a
phy-handle node, pcs node, sfp node or static link information, then
although macb uses phylink, it then doesn't conform to this phylink
binding. (This is the only driver that uses phy_find_first() which
also uses phylink according to my greps, but I haven't checked for
any other games drivers be using.)
The same thing more or less happens with non-phylink drivers. Take a
look at drivers/net/ethernet/microchip/lan743x_main.c, and notice
that it first attempts to get a PHY from DT. If that fails, it
uses phy_find_first(). If that fails, and we have a LAN7431, then
a gigabit full-duplex fixed-link PHY is used instead. So, what macb
is doing with phylink is no different from what other drivers are
doing with phylib - and it's the driver's choice.
The same way that there are multiple drivers that don't do this,
which want a PHY device to be specified in DT if the driver was
bound to a device that was described in DT - there are phylink
and non-phylink drivers that do this.
This is exactly my point - there is *no* *such* *thing* as a phylink
binding. There is the ethernet-controller binding, which phylink
provides the ability for network drivers to optionally use, but
phylink doesn't require anything from any firmware description, except
to attach a SFP interface, or to describe a fixed-link. Everything else
is really up to the ethernet-controller aka MAC driver to decide how it
wants to deal with things.
We currently work around this by the ethernet-controller YAML having
all these properties as optional. Maybe some drivers extend that YAML
and require certain properties - that is their perogative, but that is
the driver's choice, and is a completely separate issue to whether
the driver is using phylink or not.
The real question is how do we want to describe an ethernet controller
and what properties should we be requiring for it (if any). Maybe if we
want to require one of a PHY, PCS, SFP, or fixed-link, maybe we should
have that as a strictly-checked ethernet controller which drivers can
opt into using if that's what they require.
I'd like to make this clear. We're only talking about deviating from proper
devicetree bindings so that it won't cause too much work or not at all to
fix the incorrect Linux driver policies.
As long as we don't collectively agree on fixing the drivers to work with
proper devicetree bindings, I'd keep the missing ethernet controller
bindings (requiring at least one of PHY, PCS, SFP, fixed-link) as they
currently are on ethernet-controller.yaml, optional. Or rather, I wouldn't
touch anything regarding this as it's nonsensical to change devicetree
bindings because of driver policies.
As you have pointed out with certain examples, once the driver starts
operating out of what the devicetree says, in other words, once the driver
starts guessing the hardware, there's no guarantee it will always guess it
correctly. As seen with the macb driver, the driver assumes that if there's
no phy-handle property, the PHY on its MDIO bus must be used regardless.
But the MAC may be connected to another MAC, PCS or SFP, meaning it doesn't
use the PHY on that bus.
There is also a case for DSA. If there's an implication that the DSA
controlled switch has an MDIO bus (phy_read() and phy_write()), the DSA
driver will connect the switch MACs to the PHYs on the MDIO bus of the
switch, even if there's no description of that MDIO bus on the devicetree.
As unlikely as it is on a real life scenario, there may be a device that
has its switch MACs wired to the PHYs on another MDIO bus.
This is why I've proposed to make the drivers strictly follow what the
devicetree says.
However, to dress this up as "phylink requires xyz, so lets create
a phylink binding description" is just wrong.
Agreed.
Arınç
