mbox series

[net-next,v2,00/10] define and enforce phylink bindings

Message ID 20230916110902.234273-1-arinc.unal@arinc9.com (mailing list archive)
Headers show
Series define and enforce phylink bindings | expand

Message

Arınç ÜNAL Sept. 16, 2023, 11:08 a.m. UTC
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.

Cheers.
Arınç

v2:
After many discussions on v1, the concept of the patch series changed a lot
to document the changes from v1.

Link to v1:
https://lore.kernel.org/netdev/20230812091708.34665-1-arinc.unal@arinc9.com/

Comments

Russell King (Oracle) Sept. 22, 2023, 12:40 p.m. UTC | #1
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.
Arınç ÜNAL Sept. 22, 2023, 9:57 p.m. UTC | #2
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

Andrew under the discussion of patch 7 said that enforcing this may expose
bugs on MAC drivers that never looked at the devicetree to control the
MAC's link which would cause regressions, implying we should hold back on
enforcing it. I've agreed not to enforce it, not because it is incorrect
description of ethernet controller hardware - I think it is correct - but
because I won't be the one to deal with the regressions when this
dt-bindings change goes through.

I won't also enforce it selectively, as saying "these drivers use
phylink_fwnode_phy_connect() therefore there won't be any bad surprises on
the hardware they control so let's enforce it only for them" is nonsense in
the context of describing hardware.

I will focus on documenting the missing MDIO bus descriptions on certain
ethernet switches and converting ethernet switch documents (maybe ethernet
controllers too) to json-schema. There's the incorrect link descriptions on
dsa-port.yaml as confirmed by Vladimir on the discussion of v1 series so
I'll fix that.

I've also got some ethernet controller rules that I think won't break any
driver so I will submit them as well.

Arınç
Russell King (Oracle) Sept. 22, 2023, 10:29 p.m. UTC | #3
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.
Andrew Lunn Sept. 22, 2023, 10:36 p.m. UTC | #4
> 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

You are missing:

- The MAC has firmware driving the PHY, nothing for linux to do.

There are properties in ethernet-controller.yaml the MAC driver would
however like to use such as local-mac-address, max-frame-size,
nvmem-cell-names etc.

	Andrew
Andrew Lunn Sept. 22, 2023, 10:44 p.m. UTC | #5
> However, to dress this up as "phylink requires xyz, so lets create
> a phylink binding description" is just wrong.

+1

Also, phylink is a Linux implementation detail. Other OSes using the
binding don't need to have phylink. Yet they can still use the DT
blobs because they should describe the hardware, independent of how
the OS drives that hardware.

    Andrew
Arınç ÜNAL Sept. 23, 2023, 6:06 a.m. UTC | #6
On 23.09.2023 01:44, Andrew Lunn wrote:
>> However, to dress this up as "phylink requires xyz, so lets create
>> a phylink binding description" is just wrong.
> 
> +1
> 
> Also, phylink is a Linux implementation detail. Other OSes using the
> binding don't need to have phylink. Yet they can still use the DT
> blobs because they should describe the hardware, independent of how
> the OS drives that hardware.

I haven't stated it directly but I've been agreeing to this fact since the
start of the discussion on patch 7.

Arınç
Arınç ÜNAL Sept. 23, 2023, 6:28 a.m. UTC | #7
On 23.09.2023 01:36, Andrew Lunn 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
> 
> You are missing:
> 
> - The MAC has firmware driving the PHY, nothing for linux to do.
> 
> There are properties in ethernet-controller.yaml the MAC driver would
> however like to use such as local-mac-address, max-frame-size,
> nvmem-cell-names etc.

This is interesting. This is clearly a hardware difference of the ethernet
controller.

I believe this fits case 1. There's still an MDIO bus the ethernet
controller uses, there's still a PHY on the MDIO bus which the ethernet
controller uses. The only difference is the firmware of the ethernet
controller controls... What exactly does the firmware control that a Linux
driver would have controlled instead? Just configuring the link settings of
the MAC?

If it's just MAC link settings, I believe it would make sense to add a
property on the ethernet controller dt-bindings to state that the hardware
controls the MAC link settings on its own. This way, we would still
describe the MDIO bus and PHY of the ethernet controller while also
pointing out that the MAC link settings are not up to a driver to control.

Arınç
Arınç ÜNAL Sept. 23, 2023, 7:51 a.m. UTC | #8
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ç
Andrew Lunn Sept. 23, 2023, 2:54 p.m. UTC | #9
> 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.

This is partially a result of history. Some of these drivers are older
than DT. This guessing was sufficient to make them work in the systems
of that time. Some drivers are used when DT is not available, e.g. USB
or PCI devices, or even ACPI.

> 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.

There are mv88e6xxx systems which don't have a DT description, just
platform data. So if you need to make code changes, keep that in mind.

	 Andrew
Andrew Lunn Sept. 23, 2023, 3:12 p.m. UTC | #10
On Sat, Sep 23, 2023 at 09:28:41AM +0300, Arınç ÜNAL wrote:
> On 23.09.2023 01:36, Andrew Lunn 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
> > 
> > You are missing:
> > 
> > - The MAC has firmware driving the PHY, nothing for linux to do.
> > 
> > There are properties in ethernet-controller.yaml the MAC driver would
> > however like to use such as local-mac-address, max-frame-size,
> > nvmem-cell-names etc.
> 
> This is interesting. This is clearly a hardware difference of the ethernet
> controller.
> 
> I believe this fits case 1. There's still an MDIO bus the ethernet
> controller uses, there's still a PHY on the MDIO bus which the ethernet
> controller uses.

Why must there be an MDIO bus? All the bus provides is a communication
channel to the PHY. There are PHYs which are memory mapped, or use
I2C. SFP are a good example of I2C, which Linux maps to MDIO just to
make things simple, but the hardware is I2C. Why must there be a PHY?
Maybe it is a Base-K link, i.e. a baseboard link to a switch, or a BMC
or something.

> The only difference is the firmware of the ethernet
> controller controls... What exactly does the firmware control that a Linux
> driver would have controlled instead? Just configuring the link settings of
> the MAC?

A MAC driver implements struct ethtool_ops:::get_link_settings and
set_link_settings. For a MAC driver using phylib or phylink they
typically then call into phylib or phylink to do the actual work,
maybe with a bit of pre-processing in the MAC driver.

A MAC driver using firmware would typically make an RPC into the
firmware to implement these calls.

There is a MAC driver currently under review which does not have a PHY
at all. The MAC is directly connected to a switch, all within one
IC. The link is always running at 5Gbps, the link is always up. It is
physically impossible to connect a PHY, so get_link_settings just
returns hard coded values.

	Andrew
Arınç ÜNAL Sept. 23, 2023, 5:51 p.m. UTC | #11
On 23.09.2023 18:12, Andrew Lunn wrote:
> On Sat, Sep 23, 2023 at 09:28:41AM +0300, Arınç ÜNAL wrote:
>> On 23.09.2023 01:36, Andrew Lunn wrote:
>>> You are missing:
>>>
>>> - The MAC has firmware driving the PHY, nothing for linux to do.
>>>
>>> There are properties in ethernet-controller.yaml the MAC driver would
>>> however like to use such as local-mac-address, max-frame-size,
>>> nvmem-cell-names etc.
>>
>> This is interesting. This is clearly a hardware difference of the ethernet
>> controller.
>>
>> I believe this fits case 1. There's still an MDIO bus the ethernet
>> controller uses, there's still a PHY on the MDIO bus which the ethernet
>> controller uses.
> 
> Why must there be an MDIO bus? All the bus provides is a communication
> channel to the PHY. There are PHYs which are memory mapped, or use
> I2C. SFP are a good example of I2C, which Linux maps to MDIO just to
> make things simple, but the hardware is I2C. Why must there be a PHY?
> Maybe it is a Base-K link, i.e. a baseboard link to a switch, or a BMC
> or something.

There's no requirement for an MDIO bus or a PHY. If the MAC is connected to
a PHY, only the PHY node needs to be described. The PHY can be controlled
by any interface, I2C, MDIO, or something else. If there's no PHY, the
fixed-link property would be used to describe the link.

> 
>> The only difference is the firmware of the ethernet
>> controller controls... What exactly does the firmware control that a Linux
>> driver would have controlled instead? Just configuring the link settings of
>> the MAC?
> 
> A MAC driver implements struct ethtool_ops:::get_link_settings and
> set_link_settings. For a MAC driver using phylib or phylink they
> typically then call into phylib or phylink to do the actual work,
> maybe with a bit of pre-processing in the MAC driver.
> 
> A MAC driver using firmware would typically make an RPC into the
> firmware to implement these calls.
> 
> There is a MAC driver currently under review which does not have a PHY
> at all. The MAC is directly connected to a switch, all within one
> IC. The link is always running at 5Gbps, the link is always up. It is
> physically impossible to connect a PHY, so get_link_settings just
> returns hard coded values.

The fixed-link property would be used to describe the link of the MAC here.

Arınç
Andrew Lunn Sept. 24, 2023, 3:15 a.m. UTC | #12
> > There is a MAC driver currently under review which does not have a PHY
> > at all. The MAC is directly connected to a switch, all within one
> > IC. The link is always running at 5Gbps, the link is always up. It is
> > physically impossible to connect a PHY, so get_link_settings just
> > returns hard coded values.
> 
> The fixed-link property would be used to describe the link of the MAC here.

Fixed-link make sense for a general purpose MAC which could be
connected to a PHY, or could also be used without a PHY. fixed-link
simplifies the code in that the MAC driver does not see a difference,
it all looks like a PHY.

However for a MAC which cannot be connected to a PHY, there is no need
to emulate a PHY. The MAC driver will be simpler. So i would not
recommend a fixed-link in this situation.

	  Andrew
Arınç ÜNAL Sept. 24, 2023, 7:49 a.m. UTC | #13
On 24/09/2023 06:15, Andrew Lunn wrote:
>>> There is a MAC driver currently under review which does not have a PHY
>>> at all. The MAC is directly connected to a switch, all within one
>>> IC. The link is always running at 5Gbps, the link is always up. It is
>>> physically impossible to connect a PHY, so get_link_settings just
>>> returns hard coded values.
>>
>> The fixed-link property would be used to describe the link of the MAC here.
> 
> Fixed-link make sense for a general purpose MAC which could be
> connected to a PHY, or could also be used without a PHY. fixed-link
> simplifies the code in that the MAC driver does not see a difference,
> it all looks like a PHY.
> 
> However for a MAC which cannot be connected to a PHY, there is no need
> to emulate a PHY. The MAC driver will be simpler. So i would not
> recommend a fixed-link in this situation.

There's a link, it must be described. The MAC driver can configure the link
without reading the fixed-link property as there's no room for guessing.

The phy-handle, pcs-handle, and sfp properties point out there's a PHY. The
fixed-link property can be used standalone to describe MAC to MAC links.

For this specific ethernet controller, the phy-handle, pcs-handle, and sfp
properties can be disallowed on its schema to point out the ethernet
controller cannot be connected to a PHY.

Arınç
Andrew Lunn Sept. 24, 2023, 2:55 p.m. UTC | #14
On Sun, Sep 24, 2023 at 10:49:49AM +0300, Arınç ÜNAL wrote:
> On 24/09/2023 06:15, Andrew Lunn wrote:
> > > > There is a MAC driver currently under review which does not have a PHY
> > > > at all. The MAC is directly connected to a switch, all within one
> > > > IC. The link is always running at 5Gbps, the link is always up. It is
> > > > physically impossible to connect a PHY, so get_link_settings just
> > > > returns hard coded values.
> > > 
> > > The fixed-link property would be used to describe the link of the MAC here.
> > 
> > Fixed-link make sense for a general purpose MAC which could be
> > connected to a PHY, or could also be used without a PHY. fixed-link
> > simplifies the code in that the MAC driver does not see a difference,
> > it all looks like a PHY.
> > 
> > However for a MAC which cannot be connected to a PHY, there is no need
> > to emulate a PHY. The MAC driver will be simpler. So i would not
> > recommend a fixed-link in this situation.
> 
> There's a link, it must be described.

Why must it be described?

Lets take this to the extreme to make a point. The chip has a ground
pin. Must i describe that?

     Andrew
Arınç ÜNAL Sept. 25, 2023, 7:47 a.m. UTC | #15
On 24.09.2023 17:55, Andrew Lunn wrote:
> On Sun, Sep 24, 2023 at 10:49:49AM +0300, Arınç ÜNAL wrote:
>> On 24/09/2023 06:15, Andrew Lunn wrote:
>>>>> There is a MAC driver currently under review which does not have a PHY
>>>>> at all. The MAC is directly connected to a switch, all within one
>>>>> IC. The link is always running at 5Gbps, the link is always up. It is
>>>>> physically impossible to connect a PHY, so get_link_settings just
>>>>> returns hard coded values.
>>>>
>>>> The fixed-link property would be used to describe the link of the MAC here.
>>>
>>> Fixed-link make sense for a general purpose MAC which could be
>>> connected to a PHY, or could also be used without a PHY. fixed-link
>>> simplifies the code in that the MAC driver does not see a difference,
>>> it all looks like a PHY.
>>>
>>> However for a MAC which cannot be connected to a PHY, there is no need
>>> to emulate a PHY. The MAC driver will be simpler. So i would not
>>> recommend a fixed-link in this situation.
>>
>> There's a link, it must be described.
> 
> Why must it be described?
> 
> Lets take this to the extreme to make a point. The chip has a ground
> pin. Must i describe that?

I think it depends on how important the information is, to be put on the
devicetree. I don't think a ground pin of an SoC is important enough to be
described on the devicetree. It could be described as a text on the
relevant devicetree document though. I've recently submitted a patch that
does a similar thing. I've described which pin groups represent which pins.

https://lore.kernel.org/lkml/20230917162837.277405-2-arinc.unal@arinc9.com/

For an ethernet controller, its link is the core part of the hardware.
Therefore describing the link was deemed important. Hence certain
properties were made to describe the link on the devicetree.

All I proposed was to make sure these properties are always defined on the
devicetree since, for an ethernet controller to exist, it must have a link.

Arınç