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

["Karumanchi, Vineeth" <vineeth.karumanchi@amd.com>]

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/

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);
> +	}

<..>

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