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

[Sean Anderson <sean.anderson@linux.dev>]

On 5/13/25 11:29, Karumanchi, Vineeth wrote:
> 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.

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.

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

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