Re: [RFC PATCH net-next 4/5] net: macb: Configure High Speed Mac for given speed.

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

Hi Maxime,

On 10/9/2024 12:06 PM, Maxime Chevallier wrote:
> Hello,
>
> On Wed, 9 Oct 2024 11:09:45 +0530
> Vineeth Karumanchi <vineeth.karumanchi@amd.com> wrote:
>
>> HS Mac configuration steps:
>> - Configure speed and serdes rate bits of USX_CONTROL register from
>>    user specified speed in the device-tree.
>> - Enable HS Mac for 5G and 10G speeds.
>> - Reset RX receive path to achieve USX block lock for the
>>    configured serdes rate.
>> - Wait for USX block lock synchronization.
>>
>> Move the initialization instances to macb_usx_pcs_link_up().
>>
>> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> [...]
>
>>   
>>   /* DMA buffer descriptor might be different size
>>    * depends on hardware configuration:
>> @@ -564,14 +565,59 @@ static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
>>   				 int duplex)
>>   {
>>   	struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
>> -	u32 config;
>> +	u32 speed_val, serdes_rate, config;
>> +	bool hs_mac = false;
>> +
>> +	switch (speed) {
>> +	case SPEED_1000:
>> +		speed_val = HS_SPEED_1000M;
>> +		serdes_rate = MACB_SERDES_RATE_1G;
>> +		break;
>> +	case SPEED_2500:
>> +		speed_val = HS_SPEED_2500M;
>> +		serdes_rate = MACB_SERDES_RATE_2_5G;
>> +		break;
>> +	case SPEED_5000:
>> +		speed_val = HS_SPEED_5000M;
>> +		serdes_rate = MACB_SERDES_RATE_5G;
>> +		hs_mac = true;
>> +		break;
> You support some new speeds and modes, so you also need to update :
>
>   - The macb_select_pcs() code, as right now it will return NULL for any
> mode that isn't 10GBaseR or SGMII, so for 2500/5000 speeds, that
> probably won't work. And for 1000, the default PCS will be used and not
> USX
>
>   - the phylink mac_capabilities, so far 2500 and 5000 speeds aren't
> reported as supported.
>
>   - the phylink supported_interfaces, I suppose the IP uses 2500BaseX
> and 5GBaseT ? or maybe some usxgmii flavors ?

with 10GBaseR, ethtool is showing multiple speed support. ( 1G, 2.5G, 5G 
and 10G )
The only check I see with 10GbaseR is max speed shouldn't be greater 
than 10G, which
suits the IP requirement.

>> +	case SPEED_10000:
>> +		speed_val = HS_SPEED_10000M;
>> +		serdes_rate = MACB_SERDES_RATE_10G;
>> +		hs_mac = true;
>> +		break;
>> +	default:
>> +		netdev_err(bp->dev, "Specified speed not supported\n");
>> +		return;
>> +	}
>> +
>> +	/* Enable HS MAC for high speeds */
>> +	if (hs_mac) {
>> +		config = macb_or_gem_readl(bp, NCR);
>> +		config |= GEM_BIT(ENABLE_HS_MAC);
>> +		macb_or_gem_writel(bp, NCR, config);
>> +	}
> It looks like you moved the MAC selection between HS MAC and non-HS MAC
> from the phylink .mac_config to PCS config.
>
> This configuration is indeed a MAC-side configuration from what I
> understand, you shouldn't need to set that in PCS code. Maybe instead,
> check the interface mode in macb_mac_config, and look if you're in
> 5GBaseR / 10GBaseR to select the MAC ?

Yes, agreed!

As our current HW setup doesn't have AN and PHY, to support speed change 
using ethtool
I have moved the above MAC config into PCS. I will explore on setting 
MAC config based on speed
instead of interface in  macb_mac_config().

Please let me know your thoughts and comments.

-- 
🙏 vineeth

[...]