[net-next] net: dsa: honor "max-speed" for implicit PHYs on user ports

Commit Message

Sverdlin, Alexander Dec. 19, 2024, 5:38 p.m. UTC

From: Alexander Sverdlin <[e-mail]>

If the PHYs on user ports are not specified explicitly, but a common
user_mii_bus is being registered and scanned there is no way to limit
Auto Negotiation options currently. If a gigabit switch is deployed in a
way that the ports cannot support gigabit rates (4-wire PCB/magnetics,
for instance), there is no way to limit ports' AN not to advertise gigabit
options. Some PHYs take considerably longer time to AutoNegotiate in such
cases.

Provide a way to limit AN advertisement options by examining "max-speed"
property in the DT node of the corresponding user port and call
phy_set_max_speed() right before attaching the PHY to he port netdevice.

Signed-off-by: Alexander Sverdlin <[e-mail]>
---
 net/dsa/user.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Vladimir Oltean Dec. 19, 2024, 5:46 p.m. UTC | #1

On Thu, Dec 19, 2024 at 06:38:01PM +0100, A. Sverdlin wrote:
> From: Alexander Sverdlin <[e-mail]>
> 
> If the PHYs on user ports are not specified explicitly, but a common
> user_mii_bus is being registered and scanned there is no way to limit
> Auto Negotiation options currently. If a gigabit switch is deployed in a
> way that the ports cannot support gigabit rates (4-wire PCB/magnetics,
> for instance), there is no way to limit ports' AN not to advertise gigabit
> options. Some PHYs take considerably longer time to AutoNegotiate in such
> cases.
> 
> Provide a way to limit AN advertisement options by examining "max-speed"
> property in the DT node of the corresponding user port and call
> phy_set_max_speed() right before attaching the PHY to he port netdevice.
> 
> Signed-off-by: Alexander Sverdlin <[e-mail]>
> ---

The user_mii_bus mechanism is redundant when we have device tree
available (as opposed to probing on platform data), let's not make it
even more redundant. Why don't you just declare the MDIO bus in the
device tree, with the PHYs on it, and place max-speed there?

Andrew Lunn Dec. 19, 2024, 5:46 p.m. UTC | #2

On Thu, Dec 19, 2024 at 06:38:01PM +0100, A. Sverdlin wrote:
> From: Alexander Sverdlin <[e-mail]>
> 
> If the PHYs on user ports are not specified explicitly, but a common
> user_mii_bus is being registered and scanned there is no way to limit
> Auto Negotiation options currently.

Please could you expand on this. This sounds like a reason you would
want to explicitly list the PHY on an MDIO bus so that you can use the
max-speed property in the PHY node.

	Andrew

Sverdlin, Alexander Dec. 19, 2024, 6:50 p.m. UTC | #3

Hello Vladimir, Andrew,

thanks for the quick feedback!

On Thu, 2024-12-19 at 19:46 +0200, Vladimir Oltean wrote:
> On Thu, Dec 19, 2024 at 06:38:01PM +0100, A. Sverdlin wrote:
> > From: Alexander Sverdlin <[e-mail]>
> > 
> > If the PHYs on user ports are not specified explicitly, but a common
> > user_mii_bus is being registered and scanned there is no way to limit
> > Auto Negotiation options currently. If a gigabit switch is deployed in a
> > way that the ports cannot support gigabit rates (4-wire PCB/magnetics,
> > for instance), there is no way to limit ports' AN not to advertise gigabit
> > options. Some PHYs take considerably longer time to AutoNegotiate in such
> > cases.
> > 
> > Provide a way to limit AN advertisement options by examining "max-speed"
> > property in the DT node of the corresponding user port and call
> > phy_set_max_speed() right before attaching the PHY to he port netdevice.
> > 
> > Signed-off-by: Alexander Sverdlin <[e-mail]>
> > ---
> 
> The user_mii_bus mechanism is redundant when we have device tree
> available (as opposed to probing on platform data), let's not make it
> even more redundant. Why don't you just declare the MDIO bus in the
> device tree, with the PHYs on it, and place max-speed there?

There are still switch drivers in tree, which only implement .phy_read/.phy_write
callbacks (which means, they rely on .user_mii_bus ?), even gigabit-capable,
such as vsc73xx, rtl8365mb, rtl8366rb... But I'm actually interested in an
out of tree driver for a new generation of lantiq_gsw hardware, under
Maxlinear branch, which is planned to be submitted upstream at some point.

The relevant question is then, is it acceptable API (.phy_read/.phy_write),
or any new gigabit-capable driver must use some form of mdiobus_register
to populate the MDIO bus explicitly itself?

Vladimir Oltean Dec. 19, 2024, 7:36 p.m. UTC | #4

On Thu, Dec 19, 2024 at 06:50:18PM +0000, Sverdlin, Alexander wrote:
> There are still switch drivers in tree, which only implement .phy_read/.phy_write
> callbacks (which means, they rely on .user_mii_bus ?), even gigabit-capable,
> such as vsc73xx, rtl8365mb, rtl8366rb... But I'm actually interested in an
> out of tree driver for a new generation of lantiq_gsw hardware, under
> Maxlinear branch, which is planned to be submitted upstream at some point.
> 
> The relevant question is then, is it acceptable API (.phy_read/.phy_write),
> or any new gigabit-capable driver must use some form of mdiobus_register
> to populate the MDIO bus explicitly itself?

See the documentation patches which I never managed to finish for general
future directions:
https://patchwork.kernel.org/project/netdevbpf/patch/[e-mail]/

Not explicitly having a phy-handle should be seen a legacy feature,
which we are forced to keep for compatibility with existing drivers.

Sverdlin, Alexander Dec. 20, 2024, 7:17 a.m. UTC | #5

Hi Vladimir!

On Thu, 2024-12-19 at 21:36 +0200, Vladimir Oltean wrote:
> On Thu, Dec 19, 2024 at 06:50:18PM +0000, Sverdlin, Alexander wrote:
> > There are still switch drivers in tree, which only implement .phy_read/.phy_write
> > callbacks (which means, they rely on .user_mii_bus ?), even gigabit-capable,
> > such as vsc73xx, rtl8365mb, rtl8366rb... But I'm actually interested in an
> > out of tree driver for a new generation of lantiq_gsw hardware, under
> > Maxlinear branch, which is planned to be submitted upstream at some point.
> > 
> > The relevant question is then, is it acceptable API (.phy_read/.phy_write),
> > or any new gigabit-capable driver must use some form of mdiobus_register
> > to populate the MDIO bus explicitly itself?
> 
> See the documentation patches which I never managed to finish for general
> future directions:
> https://patchwork.kernel.org/project/netdevbpf/patch/[e-mail]/
> 
> Not explicitly having a phy-handle should be seen a legacy feature,
> which we are forced to keep for compatibility with existing drivers.

Thanks for the references!

I've complitely missed the story of
fe7324b93222 ("net: dsa: OF-ware slave_mii_bus")
vs ae94dc25fd73
("net: dsa: remove OF-based MDIO bus registration from DSA core").

But I'm still having hard time to get the motivation behind removing
2 function calls from the DSA core and forcing all individual DSA drivers
to have this very same boilerplate...

But well, if all the DSA maintainers are so committed to it, this answers
my original question... Please ignore the patch!

Sverdlin, Alexander Dec. 20, 2024, 7:30 a.m. UTC | #6

Hi Vladimir!

On Fri, 2024-12-20 at 08:17 +0100, Alexander Sverdlin wrote:
> > On Thu, Dec 19, 2024 at 06:50:18PM +0000, Sverdlin, Alexander wrote:
> > > There are still switch drivers in tree, which only implement .phy_read/.phy_write
> > > callbacks (which means, they rely on .user_mii_bus ?), even gigabit-capable,
> > > such as vsc73xx, rtl8365mb, rtl8366rb... But I'm actually interested in an
> > > out of tree driver for a new generation of lantiq_gsw hardware, under
> > > Maxlinear branch, which is planned to be submitted upstream at some point.
> > > 
> > > The relevant question is then, is it acceptable API (.phy_read/.phy_write),
> > > or any new gigabit-capable driver must use some form of mdiobus_register
> > > to populate the MDIO bus explicitly itself?
> > 
> > See the documentation patches which I never managed to finish for general
> > future directions:
> > https://patchwork.kernel.org/project/netdevbpf/patch/[e-mail]/
> > 
> > Not explicitly having a phy-handle should be seen a legacy feature,
> > which we are forced to keep for compatibility with existing drivers.
> 
> Thanks for the references!
> 
> I've complitely missed the story of
> fe7324b93222 ("net: dsa: OF-ware slave_mii_bus")
> vs ae94dc25fd73
> ("net: dsa: remove OF-based MDIO bus registration from DSA core").
> 
> But I'm still having hard time to get the motivation behind removing
> 2 function calls from the DSA core and forcing all individual DSA drivers
> to have this very same boilerplate...
> 
> But well, if all the DSA maintainers are so committed to it, this answers
> my original question... Please ignore the patch!

However, after reading the whole referenced thread, I still have a question:
will MFD approach (with both drivers and dt-bindings) will be a requirement
for any new drivers or a simpler approach with "mdio {}" node under the
switch node will still be acceptable?

Andrew Lunn Dec. 20, 2024, 8:28 a.m. UTC | #7

> However, after reading the whole referenced thread, I still have a question:
> will MFD approach (with both drivers and dt-bindings) will be a requirement
> for any new drivers or a simpler approach with "mdio {}" node under the
> switch node will still be acceptable?

There is a long history of MAC drivers having MDIO drivers embedded in
them. So i see no need for an MFD just to have an MDIO device. If
there are additional drivers, GPIO, I2C, etc, then an MFD makes sense.

	Andrew

Vladimir Oltean Dec. 20, 2024, 10:15 a.m. UTC | #8

On Fri, Dec 20, 2024 at 07:17:37AM +0000, Sverdlin, Alexander wrote:
> Thanks for the references!
> 
> I've complitely missed the story of
> fe7324b93222 ("net: dsa: OF-ware slave_mii_bus")
> vs ae94dc25fd73
> ("net: dsa: remove OF-based MDIO bus registration from DSA core").
> 
> But I'm still having hard time to get the motivation behind removing
> 2 function calls from the DSA core and forcing all individual DSA drivers
> to have this very same boilerplate...
> 
> But well, if all the DSA maintainers are so committed to it, this answers
> my original question... Please ignore the patch!

My motivation is that as things stand, ds->ops->phy_read() and
ds->ops->phy_write() are just too attractive to implement, but lure
developers into an OF-unaware internal MDIO bus model which is just
redundant and provides less functionality compared to its OF-aware
counterpart. You can surely understand this if you look at what prompted
you to submit this patch.

As for the OF-aware MDIO bus, the idea would be for DSA to focus less on
imposing a device model, and more on just the Ethernet switch component.
I'm yet to be convinced that it's exactly DSA's business to assist with that.
Being slimmer helps. For example if somebody wanted to add ACPI bindings
for the DSA core, not having to think about MDIO as part of the core
bindings, but as a device specific thing, is a simplifying factor.

You're also exaggerating that all DSA switches have internal PHYs.

Patch

diff --git a/net/dsa/user.c b/net/dsa/user.c
index 4a8de48a6f24..9e3f5b0f9af3 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -2625,6 +2625,13 @@  static int dsa_user_phy_connect(struct net_device *user_dev, int addr,
 
 	user_dev->phydev->dev_flags |= flags;
 
+	if (dp->dn) {
+		u32 max_speed;
+
+		if (!of_property_read_u32(dp->dn, "max-speed", &max_speed))
+			phy_set_max_speed(user_dev->phydev, max_speed);
+	}
+
 	return phylink_connect_phy(dp->pl, user_dev->phydev);
 }