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

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

Hi Sean,

On 5/13/2025 9:19 PM, Sean Anderson wrote:
> On 5/13/25 11:29, Karumanchi, Vineeth wrote:
>> Hi Sean,

<..>

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

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

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

-- 
🙏 vineeth