Re: [net-next PATCH v4 09/11] net: macb: Move most of mac_config to mac_prepare

Hi Sean,

On 5/13/2025 9:19 PM, Sean Anderson wrote:

On 5/13/25 11:29, Karumanchi, Vineeth wrote:

quoted

Hi Sean,

<..>

quoted

quoted

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

To clarify, this bit enables SGMII timings for AN (as opposed to
1000Base-X). If you don't have AN enabled at 1G, then this bit affects
nothing.

1000Base-X is not currently supported by the built-in PCS. Therefore,
this bit should be set unconditionally at 1G speeds. This patch aims to
avoid functional changes so I have not made it conditional. Making this
bit conditional would be appropriate for a patch adding support for
1000Base-X using the internal PCS.

Yes, agreed.

quoted

quoted

+    if (old != new) {
+        changed = true;
+        gem_writel(bp, NCFGR, new);
+    }

<..>

quoted

     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 ?

Form what I can tell, the USX PCS is only used for 10G interfaces. If
you want to add support for using it at other link speeds, then yes some
of these register writes should be moved to link_up. For the moment it
doesn't matter where they happen.

--Sean

Ok, in the latest cadence IP, all speed configurations are mapped to USX 
registers for both internal PCS's (1000Base-X & 10GBase-R).

-- 
🙏 vineeth