Re: [PATCH net-next v2 00/10] define and enforce phylink bindings

[Arınç ÜNAL]

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ç