Re: [net-next PATCH v4 09/11] net: macb: Move most of mac_config to mac_prepare
From: Karumanchi, Vineeth <hidden>
Date: 2025-05-13 15:29:13
Also in:
lkml
Hi Sean, Sorry for the delayed response. We are working on MACB with two internal PCS's (10G-BASER, 1000-BASEX) supporting 1G, 2.5G, 5G, and 10G with AN disabled. I have sent an initial RFC : https://lore.kernel.org/netdev/20241009053946.3198805-1-vineeth.karumanchi@amd.com/ (local) Currently, we are working on integrating the MAC in fixed-link and phy-mode. Please see my inline comments. On 5/12/2025 9:44 PM, Sean Anderson wrote:
mac_prepare is called every time the interface is changed, so we can do all of our configuration there, instead of in mac_config. This will be useful for the next patch where we will set the PCS bit based on whether we are using our internal PCS. No functional change intended. Signed-off-by: Sean Anderson <sean.anderson@linux.dev> ---
<...>
+static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+ phy_interface_t interface,
+ const unsigned long *advertising,
+ bool permit_pause_to_mac)
+{
+ struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
+ bool changed = false;
+ unsigned long flags;
+ u32 old, new;
+
+ spin_lock_irqsave(&bp->lock, flags);
+ old = new = gem_readl(bp, NCFGR);
+ new |= GEM_BIT(SGMIIEN);This bit represents the AN feature, can we make it conditional to facilitate IP's with AN disabled.
+ if (old != new) {
+ changed = true;
+ gem_writel(bp, NCFGR, new);
+ }<..>
quoted hunk ↗ jump to hunk
static void macb_usx_pcs_get_state(struct phylink_pcs *pcs,@@ -589,45 +661,60 @@ static int macb_usx_pcs_config(struct phylink_pcs *pcs, bool permit_pause_to_mac) { struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs); + unsigned long flags; + bool changed; + u16 old, new; - gem_writel(bp, USX_CONTROL, gem_readl(bp, USX_CONTROL) | - GEM_BIT(SIGNAL_OK)); + spin_lock_irqsave(&bp->lock, flags); + if (macb_pcs_config_an(bp, neg_mode, interface, advertising)) + changed = true; - return 0; -} + old = new = gem_readl(bp, USX_CONTROL); + new |= GEM_BIT(SIGNAL_OK); + if (old != new) { + changed = true; + gem_writel(bp, USX_CONTROL, new); + } -static void macb_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode, - struct phylink_link_state *state) -{ - state->link = 0; -} + old = new = gem_readl(bp, USX_CONTROL); + new = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, new); + new = GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M, new); + new &= ~(GEM_BIT(TX_SCR_BYPASS) | GEM_BIT(RX_SCR_BYPASS)); + new |= GEM_BIT(TX_EN); + if (old != new) { + changed = true; + gem_writel(bp, USX_CONTROL, new); + }
The above speed/rate configuration was moved from macb_usx_pcs_link_up() where speed is an argument, which can be leveraged to configure multiple speeds. Can we achieve configuring for multiple speeds from macb_usx_pcs_config() in fixed-link and phy-mode ? -- 🙏 vineeth