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

[Russell King (Oracle)]

On Sat, Sep 23, 2023 at 12:57:52AM +0300, Arınç ÜNAL wrote:
> On 22/09/2023 15:40, Russell King (Oracle) wrote:
> > On Sat, Sep 16, 2023 at 02:08:52PM +0300, Arınç ÜNAL wrote:
> > > Hello there.
> > > 
> > > This patch series defines phylink bindings and enforces them for the
> > > ethernet controllers that need them.
> > > 
> > > Some schemas had to be changed to properly enforce phylink bindings for all
> > > of the affected ethernet controllers. Some of the documents of these
> > > ethernet controllers were non json-schema, which had to be converted.
> > > 
> > > I will convert the remaining documents to json-schema while this patch
> > > series receives reviews.
> > 
> > I can't say that I'm comfortable with this. We appear to be defining
> > bindings based on software implementation, and a desire for the DT
> > tooling to enforce what the software implementation wants. Isn't this
> > against the aims of device tree and device tree binding documentation?
> > Seems to me like feature-creep.
> > 
> > The bindings that phylink parses are already documented in the
> > ethernet controller yaml document. Specifically:
> > 
> > - phylink does not parse the phy-mode property, that is left to the
> >    implementation to pass to phylink, which can implement it any
> >    which way they choose (and even default to something.)
> > 
> > - phylink does not require a phy property - phylink does expect a PHY
> >    to be attached, but how that PHY is attached is up to the ethernet
> >    controller driver. It may call one of the phylink functions that
> >    parses the phy property, or it may manually supply the phy device to
> >    phylink. Either way, phylink does not itself require a PHY property.
> > 
> > - phylink does not require a sfp property - this obviously is optional.
> > 
> > So, all in all, ethernet-controller already describes it, and to create
> > a DT binding document that pretends that phylink requires any of this
> > stuff is, in my mind, wrong.
> > 
> > DSA requires certain properties by dint of the parsing and setup of
> > phylink being in generic code - this is not because phylink requires
> > certain properties, but phylink does require certain information in
> > order to function correctly.
> > 
> > The issue here is _how_ phylink gets that information, and as I state
> > above, it _can_ come from DT, but it can also be given that information
> > manually.
> > 
> > As an example, there are plenty of drivers in the tree which try to
> > parse a phy node, and if that's not present, they try to see if a PHY
> > exists at a default# bus address.
> > 
> > We seem to be digging outselves a hole here, where "phylink must have
> > these properties". No, that is wrong.
> 
> 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.

However, to dress this up as "phylink requires xyz, so lets create
a phylink binding description" is just wrong.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!