All of lore.kernel.org
 help / color / mirror / Atom feed
* [net-next PATCH] net: dsa: rtl8366rb: Switch to phylink
@ 2020-09-05 22:48 Linus Walleij
  2020-09-06  1:56 ` Florian Fainelli
  2020-09-06 17:51 ` Jakub Kicinski
  0 siblings, 2 replies; 4+ messages in thread
From: Linus Walleij @ 2020-09-05 22:48 UTC (permalink / raw)
  To: Andrew Lunn, Vivien Didelot, Florian Fainelli, netdev, David S . Miller
  Cc: Linus Walleij

This switches the RTL8366RB over to using phylink callbacks
instead of .adjust_link(). This is a pretty template
switchover. All we adjust is the CPU port so that is why
the code only inspects this port.

We enhance by adding proper error messages, also disabling
the CPU port on the way down and moving dev_info() to
dev_dbg().

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/dsa/rtl8366rb.c | 42 ++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index f763f93f600f..574e5e469966 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -969,8 +969,9 @@ static enum dsa_tag_protocol rtl8366_get_tag_protocol(struct dsa_switch *ds,
 	return DSA_TAG_PROTO_RTL4_A;
 }
 
-static void rtl8366rb_adjust_link(struct dsa_switch *ds, int port,
-				  struct phy_device *phydev)
+void rtl8366rb_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode,
+			   phy_interface_t interface, struct phy_device *phydev,
+			   int speed, int duplex, bool tx_pause, bool rx_pause)
 {
 	struct realtek_smi *smi = ds->priv;
 	int ret;
@@ -978,25 +979,51 @@ static void rtl8366rb_adjust_link(struct dsa_switch *ds, int port,
 	if (port != smi->cpu_port)
 		return;
 
-	dev_info(smi->dev, "adjust link on CPU port (%d)\n", port);
+	dev_dbg(smi->dev, "MAC link up on CPU port (%d)\n", port);
 
 	/* Force the fixed CPU port into 1Gbit mode, no autonegotiation */
 	ret = regmap_update_bits(smi->map, RTL8366RB_MAC_FORCE_CTRL_REG,
 				 BIT(port), BIT(port));
-	if (ret)
+	if (ret) {
+		dev_err(smi->dev, "failed to force 1Gbit on CPU port\n");
 		return;
+	}
 
 	ret = regmap_update_bits(smi->map, RTL8366RB_PAACR2,
 				 0xFF00U,
 				 RTL8366RB_PAACR_CPU_PORT << 8);
-	if (ret)
+	if (ret) {
+		dev_err(smi->dev, "failed to set PAACR on CPU port\n");
 		return;
+	}
 
 	/* Enable the CPU port */
 	ret = regmap_update_bits(smi->map, RTL8366RB_PECR, BIT(port),
 				 0);
-	if (ret)
+	if (ret) {
+		dev_err(smi->dev, "failed to enable the CPU port\n");
+		return;
+	}
+}
+
+void rtl8366rb_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode,
+			     phy_interface_t interface)
+{
+	struct realtek_smi *smi = ds->priv;
+	int ret;
+
+	if (port != smi->cpu_port)
+		return;
+
+	dev_dbg(smi->dev, "MAC link down on CPU port (%d)\n", port);
+
+	/* Disable the CPU port */
+	ret = regmap_update_bits(smi->map, RTL8366RB_PECR, BIT(port),
+				 BIT(port));
+	if (ret) {
+		dev_err(smi->dev, "failed to disable the CPU port\n");
 		return;
+	}
 }
 
 static void rb8366rb_set_port_led(struct realtek_smi *smi,
@@ -1439,7 +1466,8 @@ static int rtl8366rb_detect(struct realtek_smi *smi)
 static const struct dsa_switch_ops rtl8366rb_switch_ops = {
 	.get_tag_protocol = rtl8366_get_tag_protocol,
 	.setup = rtl8366rb_setup,
-	.adjust_link = rtl8366rb_adjust_link,
+	.phylink_mac_link_up = rtl8366rb_mac_link_up,
+	.phylink_mac_link_down = rtl8366rb_mac_link_down,
 	.get_strings = rtl8366_get_strings,
 	.get_ethtool_stats = rtl8366_get_ethtool_stats,
 	.get_sset_count = rtl8366_get_sset_count,
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [net-next PATCH] net: dsa: rtl8366rb: Switch to phylink
  2020-09-05 22:48 [net-next PATCH] net: dsa: rtl8366rb: Switch to phylink Linus Walleij
@ 2020-09-06  1:56 ` Florian Fainelli
  2020-09-06  8:24   ` Russell King - ARM Linux admin
  2020-09-06 17:51 ` Jakub Kicinski
  1 sibling, 1 reply; 4+ messages in thread
From: Florian Fainelli @ 2020-09-06  1:56 UTC (permalink / raw)
  To: Linus Walleij, Andrew Lunn, Vivien Didelot, netdev,
	David S . Miller, Russell King

+Russell,

On 9/5/2020 3:48 PM, Linus Walleij wrote:
> This switches the RTL8366RB over to using phylink callbacks
> instead of .adjust_link(). This is a pretty template
> switchover. All we adjust is the CPU port so that is why
> the code only inspects this port.
> 
> We enhance by adding proper error messages, also disabling
> the CPU port on the way down and moving dev_info() to
> dev_dbg().
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

The part of the former adjust_link, especially the part that forces the 
link to 1Gbit/sec, full duplex and no-autonegotiation probably belongs 
to a phylink_mac_config() implementation.

Assuming that someone connects such a switch to a 10/100 Ethernet MAC 
and provides a fixed-link property in Device Tree, we should at least 
attempt to configure the CPU port interface based on those link 
settings, that is not happening today.
-- 
Florian

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [net-next PATCH] net: dsa: rtl8366rb: Switch to phylink
  2020-09-06  1:56 ` Florian Fainelli
@ 2020-09-06  8:24   ` Russell King - ARM Linux admin
  0 siblings, 0 replies; 4+ messages in thread
From: Russell King - ARM Linux admin @ 2020-09-06  8:24 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Linus Walleij, Andrew Lunn, Vivien Didelot, netdev, David S . Miller

On Sat, Sep 05, 2020 at 06:56:45PM -0700, Florian Fainelli wrote:
> +Russell,
> 
> On 9/5/2020 3:48 PM, Linus Walleij wrote:
> > This switches the RTL8366RB over to using phylink callbacks
> > instead of .adjust_link(). This is a pretty template
> > switchover. All we adjust is the CPU port so that is why
> > the code only inspects this port.
> > 
> > We enhance by adding proper error messages, also disabling
> > the CPU port on the way down and moving dev_info() to
> > dev_dbg().
> > 
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> 
> The part of the former adjust_link, especially the part that forces the link
> to 1Gbit/sec, full duplex and no-autonegotiation probably belongs to a
> phylink_mac_config() implementation.
> 
> Assuming that someone connects such a switch to a 10/100 Ethernet MAC and
> provides a fixed-link property in Device Tree, we should at least attempt to
> configure the CPU port interface based on those link settings, that is not
> happening today.

The CPU port has been the subject of much discussion; I thought the
conclusion was that phylink would not be used for the CPU port anymore.

The problem is, DSA has this idea that if there's nothing specified for
the CPU port, that port will be configured to the highest speed and
duplex mode possible, but that isn't compatible with phylink.  When
there's no SFP or fixed-link specifier, phylink assumes that a PHY will
be present, which is the expectation for network drivers.  Consequently,
phylink will be in "PHY" mode, but there is no PHY for a CPU link, so
phylink will never see the link come up. Moreover, phylink has no idea
what the maximum speed of the port is, so it has no parameters to call
the link_up() methods with.

I did toy with adding yet another callback for DSA that would happen
late which gave an opportunity for DSA to report that and reconfigure
phylink for a fixed-link, but Andrew's conclusion was not to use phylink
for CPU ports.

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [net-next PATCH] net: dsa: rtl8366rb: Switch to phylink
  2020-09-05 22:48 [net-next PATCH] net: dsa: rtl8366rb: Switch to phylink Linus Walleij
  2020-09-06  1:56 ` Florian Fainelli
@ 2020-09-06 17:51 ` Jakub Kicinski
  1 sibling, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2020-09-06 17:51 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, netdev, David S . Miller

On Sun,  6 Sep 2020 00:48:28 +0200 Linus Walleij wrote:
> -static void rtl8366rb_adjust_link(struct dsa_switch *ds, int port,
> -				  struct phy_device *phydev)
> +void rtl8366rb_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode,
> +			   phy_interface_t interface, struct phy_device *phydev,
> +			   int speed, int duplex, bool tx_pause, bool rx_pause)
>  {

> +void rtl8366rb_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode,
> +			     phy_interface_t interface)
> +{	.get_sset_count = rtl8366_get_sset_count,


drivers/net/dsa/rtl8366rb.c:972:6: warning: no previous prototype for ‘rtl8366rb_mac_link_up’ [-Wmissing-prototypes]
  972 | void rtl8366rb_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode,
      |      ^~~~~~~~~~~~~~~~~~~~~
drivers/net/dsa/rtl8366rb.c:1009:6: warning: no previous prototype for ‘rtl8366rb_mac_link_down’ [-Wmissing-prototypes]
 1009 | void rtl8366rb_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode,
      |      ^~~~~~~~~~~~~~~~~~~~~~~
drivers/net/dsa/rtl8366rb.c:972:6: warning: symbol 'rtl8366rb_mac_link_up' was not declared. Should it be static?
drivers/net/dsa/rtl8366rb.c:1009:6: warning: symbol 'rtl8366rb_mac_link_down' was not declared. Should it be static?

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-09-06 17:51 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-05 22:48 [net-next PATCH] net: dsa: rtl8366rb: Switch to phylink Linus Walleij
2020-09-06  1:56 ` Florian Fainelli
2020-09-06  8:24   ` Russell King - ARM Linux admin
2020-09-06 17:51 ` Jakub Kicinski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.