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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bdc5c3d3-a57e-4f80-92bb-3448e80a288f@amd.com>
Date: Thu, 15 May 2025 11:44:16 +0530
From: "Karumanchi, Vineeth" <vineeth@....com>
To: sean.anderson@...ux.dev, vineeth.karumanchi@....com,
 netdev@...r.kernel.org, andrew+netdev@...n.ch, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
 linux@...linux.org.uk
Cc: upstream@...oha.com, horms@...nel.org, hkallweit1@...il.com,
 kory.maincent@...tlin.com, linux-kernel@...r.kernel.org,
 ansuelsmth@...il.com, claudiu.beznea@...rochip.com,
 nicolas.ferre@...rochip.com
Subject: 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:
>> 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