| Message ID | E1rwfu3-00752s-On@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 03d5a56ef7954442939e0219d30598cecc5cd5f9 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: dsa: bcm_sf2: provide own phylink MAC operations | expand |
On 4/16/24 03:19, Russell King (Oracle) wrote: > Convert bcm_sf2 to provide its own phylink MAC operations, thus > avoiding the shim layer in DSA's port.c > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Acked-by: Florian Fainelli <florian.fainelli@broadcom.com> Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
On Tue, Apr 16, 2024 at 10:44:38AM -0700, Florian Fainelli wrote: > On 4/16/24 03:19, Russell King (Oracle) wrote: > > Convert bcm_sf2 to provide its own phylink MAC operations, thus > > avoiding the shim layer in DSA's port.c > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > Acked-by: Florian Fainelli <florian.fainelli@broadcom.com> > Tested-by: Florian Fainelli <florian.fainelli@broadcom.com> Great, thanks for testing. (Unrelated to this patch... so please don't delay applying based on ongoing discussion!) The other Broadcom driver, b53, isn't going to be as simple - I believe it uses a mixture of the .adjust_link method for shared ports, and .phylink_mac_* for user ports. That makes it very awkward now, given the check that was added (and suggested by Vladimir) to check for the legacy methods if dsa_switch's .phylink_mac_ops is populated. Is there any scope for converting b53 to use only phylink methods for everything, thus eliminating the .adjust_link callback?
On 4/16/2024 11:16 AM, Russell King (Oracle) wrote: > On Tue, Apr 16, 2024 at 10:44:38AM -0700, Florian Fainelli wrote: >> On 4/16/24 03:19, Russell King (Oracle) wrote: >>> Convert bcm_sf2 to provide its own phylink MAC operations, thus >>> avoiding the shim layer in DSA's port.c >>> >>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> >> >> Acked-by: Florian Fainelli <florian.fainelli@broadcom.com> >> Tested-by: Florian Fainelli <florian.fainelli@broadcom.com> > > Great, thanks for testing. > > (Unrelated to this patch... so please don't delay applying based on > ongoing discussion!) > > The other Broadcom driver, b53, isn't going to be as simple - I believe > it uses a mixture of the .adjust_link method for shared ports, and > .phylink_mac_* for user ports. That makes it very awkward now, given > the check that was added (and suggested by Vladimir) to check for the > legacy methods if dsa_switch's .phylink_mac_ops is populated. > > Is there any scope for converting b53 to use only phylink methods for > everything, thus eliminating the .adjust_link callback? It is on the TODO list for sure, and there might be a window later this week to actually work on removing the adjust_link callback once and for all.
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 16 Apr 2024 11:19:03 +0100 you wrote: > Convert bcm_sf2 to provide its own phylink MAC operations, thus > avoiding the shim layer in DSA's port.c > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/net/dsa/bcm_sf2.c | 49 +++++++++++++++++++++++++-------------- > 1 file changed, 32 insertions(+), 17 deletions(-) Here is the summary with links: - [net-next] net: dsa: bcm_sf2: provide own phylink MAC operations https://git.kernel.org/netdev/net-next/c/03d5a56ef795 You are awesome, thank you!
On 4/16/24 20:13, Florian Fainelli wrote: > > > On 4/16/2024 11:16 AM, Russell King (Oracle) wrote: >> On Tue, Apr 16, 2024 at 10:44:38AM -0700, Florian Fainelli wrote: >>> On 4/16/24 03:19, Russell King (Oracle) wrote: >>>> Convert bcm_sf2 to provide its own phylink MAC operations, thus >>>> avoiding the shim layer in DSA's port.c >>>> >>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> >>> >>> Acked-by: Florian Fainelli <florian.fainelli@broadcom.com> >>> Tested-by: Florian Fainelli <florian.fainelli@broadcom.com> >> >> Great, thanks for testing. >> >> (Unrelated to this patch... so please don't delay applying based on >> ongoing discussion!) >> >> The other Broadcom driver, b53, isn't going to be as simple - I believe >> it uses a mixture of the .adjust_link method for shared ports, and >> .phylink_mac_* for user ports. That makes it very awkward now, given >> the check that was added (and suggested by Vladimir) to check for the >> legacy methods if dsa_switch's .phylink_mac_ops is populated. >> >> Is there any scope for converting b53 to use only phylink methods for >> everything, thus eliminating the .adjust_link callback? > > It is on the TODO list for sure, and there might be a window later this > week to actually work on removing the adjust_link callback once and for > all. This is what I came up with so far: https://github.com/ffainelli/linux/commits/b53-phylink Will test later today on the various devices I have.
On Thu, Apr 18, 2024 at 09:52:02AM -0700, Florian Fainelli wrote: > On 4/16/24 20:13, Florian Fainelli wrote: > > > > > > On 4/16/2024 11:16 AM, Russell King (Oracle) wrote: > > > On Tue, Apr 16, 2024 at 10:44:38AM -0700, Florian Fainelli wrote: > > > > On 4/16/24 03:19, Russell King (Oracle) wrote: > > > > > Convert bcm_sf2 to provide its own phylink MAC operations, thus > > > > > avoiding the shim layer in DSA's port.c > > > > > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > > > > > Acked-by: Florian Fainelli <florian.fainelli@broadcom.com> > > > > Tested-by: Florian Fainelli <florian.fainelli@broadcom.com> > > > > > > Great, thanks for testing. > > > > > > (Unrelated to this patch... so please don't delay applying based on > > > ongoing discussion!) > > > > > > The other Broadcom driver, b53, isn't going to be as simple - I believe > > > it uses a mixture of the .adjust_link method for shared ports, and > > > .phylink_mac_* for user ports. That makes it very awkward now, given > > > the check that was added (and suggested by Vladimir) to check for the > > > legacy methods if dsa_switch's .phylink_mac_ops is populated. > > > > > > Is there any scope for converting b53 to use only phylink methods for > > > everything, thus eliminating the .adjust_link callback? > > > > It is on the TODO list for sure, and there might be a window later this > > week to actually work on removing the adjust_link callback once and for > > all. > > This is what I came up with so far: > > https://github.com/ffainelli/linux/commits/b53-phylink > > Will test later today on the various devices I have. A few comments... In "net: dsa: b53: Configure RGMII for 531x5 and MII for 5325", the commit description says about adding to b53_mac_config(), but the code change modifies b53_phylink_mac_link_up(). I would think that the former (as mentioned in the commit message) was the better place for this rather than in b53_phylink_mac_link_up(). However, I see you move this to b53_phylink_mac_prepare() in "net: dsa: b53: Move MII/RGMII configuration to mac_prepare" but I'm not sure why you've chosen mac_prepare() over mac_config(). FYI, the order in which methods at the MAC and PCS are called on a reconfiguration is: mac_prepare() if changing pcs && oldpcs: oldpcs::pcs_disable() if newpcs: newpcs::pre_config() mac_config() if newpcs: newpcs::post_config() if changing pcs: newpcs::pcs_enable() newpcs::pcs_config() newpcs::pcs_an_restart() mac_finish()
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c index bc77ee9e6d0a..ed1e6560df25 100644 --- a/drivers/net/dsa/bcm_sf2.c +++ b/drivers/net/dsa/bcm_sf2.c @@ -740,16 +740,19 @@ static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port, MAC_10 | MAC_100 | MAC_1000; } -static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port, +static void bcm_sf2_sw_mac_config(struct phylink_config *config, unsigned int mode, const struct phylink_link_state *state) { - struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds); + struct dsa_port *dp = dsa_phylink_to_port(config); u32 id_mode_dis = 0, port_mode; + struct bcm_sf2_priv *priv; u32 reg_rgmii_ctrl; u32 reg; - if (port == core_readl(priv, CORE_IMP0_PRT_ID)) + priv = bcm_sf2_to_priv(dp->ds); + + if (dp->index == core_readl(priv, CORE_IMP0_PRT_ID)) return; switch (state->interface) { @@ -770,7 +773,7 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port, return; } - reg_rgmii_ctrl = bcm_sf2_reg_rgmii_cntrl(priv, port); + reg_rgmii_ctrl = bcm_sf2_reg_rgmii_cntrl(priv, dp->index); /* Clear id_mode_dis bit, and the existing port mode, let * RGMII_MODE_EN bet set by mac_link_{up,down} @@ -809,13 +812,16 @@ static void bcm_sf2_sw_mac_link_set(struct dsa_switch *ds, int port, reg_writel(priv, reg, reg_rgmii_ctrl); } -static void bcm_sf2_sw_mac_link_down(struct dsa_switch *ds, int port, +static void bcm_sf2_sw_mac_link_down(struct phylink_config *config, unsigned int mode, phy_interface_t interface) { - struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds); + struct dsa_port *dp = dsa_phylink_to_port(config); + struct bcm_sf2_priv *priv; + int port = dp->index; u32 reg, offset; + priv = bcm_sf2_to_priv(dp->ds); if (priv->wol_ports_mask & BIT(port)) return; @@ -824,23 +830,26 @@ static void bcm_sf2_sw_mac_link_down(struct dsa_switch *ds, int port, reg &= ~LINK_STS; core_writel(priv, reg, offset); - bcm_sf2_sw_mac_link_set(ds, port, interface, false); + bcm_sf2_sw_mac_link_set(dp->ds, port, interface, false); } -static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port, +static void bcm_sf2_sw_mac_link_up(struct phylink_config *config, + struct phy_device *phydev, unsigned int mode, phy_interface_t interface, - struct phy_device *phydev, int speed, int duplex, bool tx_pause, bool rx_pause) { - struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds); - struct ethtool_keee *p = &priv->dev->ports[port].eee; + struct dsa_port *dp = dsa_phylink_to_port(config); + struct bcm_sf2_priv *priv; u32 reg_rgmii_ctrl = 0; + struct ethtool_keee *p; + int port = dp->index; u32 reg, offset; - bcm_sf2_sw_mac_link_set(ds, port, interface, true); + bcm_sf2_sw_mac_link_set(dp->ds, port, interface, true); + priv = bcm_sf2_to_priv(dp->ds); offset = bcm_sf2_port_override_offset(priv, port); if (phy_interface_mode_is_rgmii(interface) || @@ -886,8 +895,10 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port, core_writel(priv, reg, offset); - if (mode == MLO_AN_PHY && phydev) - p->eee_enabled = b53_eee_init(ds, port, phydev); + if (mode == MLO_AN_PHY && phydev) { + p = &priv->dev->ports[port].eee; + p->eee_enabled = b53_eee_init(dp->ds, port, phydev); + } } static void bcm_sf2_sw_fixed_state(struct dsa_switch *ds, int port, @@ -1196,6 +1207,12 @@ static int bcm_sf2_sw_get_sset_count(struct dsa_switch *ds, int port, return cnt; } +static const struct phylink_mac_ops bcm_sf2_phylink_mac_ops = { + .mac_config = bcm_sf2_sw_mac_config, + .mac_link_down = bcm_sf2_sw_mac_link_down, + .mac_link_up = bcm_sf2_sw_mac_link_up, +}; + static const struct dsa_switch_ops bcm_sf2_ops = { .get_tag_protocol = b53_get_tag_protocol, .setup = bcm_sf2_sw_setup, @@ -1206,9 +1223,6 @@ static const struct dsa_switch_ops bcm_sf2_ops = { .get_ethtool_phy_stats = b53_get_ethtool_phy_stats, .get_phy_flags = bcm_sf2_sw_get_phy_flags, .phylink_get_caps = bcm_sf2_sw_get_caps, - .phylink_mac_config = bcm_sf2_sw_mac_config, - .phylink_mac_link_down = bcm_sf2_sw_mac_link_down, - .phylink_mac_link_up = bcm_sf2_sw_mac_link_up, .phylink_fixed_state = bcm_sf2_sw_fixed_state, .suspend = bcm_sf2_sw_suspend, .resume = bcm_sf2_sw_resume, @@ -1399,6 +1413,7 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev) priv->dev = dev; ds = dev->ds; ds->ops = &bcm_sf2_ops; + ds->phylink_mac_ops = &bcm_sf2_phylink_mac_ops; /* Advertise the 8 egress queues */ ds->num_tx_queues = SF2_NUM_EGRESS_QUEUES;
Convert bcm_sf2 to provide its own phylink MAC operations, thus avoiding the shim layer in DSA's port.c Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/dsa/bcm_sf2.c | 49 +++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 17 deletions(-)