Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Siddharth Vadapalli <s-vadapalli@ti.com>]

Hello Russell,

Thank you for reviewing the patch.

On 31/03/23 13:27, Russell King (Oracle) wrote:
> On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
>> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
>> extra_modes member of the J784S4 SoC data. Additionally, configure the
>> MAC Control register for supporting USXGMII mode. Also, for USXGMII
>> mode, include MAC_5000FD in the "mac_capabilities" member of struct
>> "phylink_config".
> 
> I don't think TI "get" phylink at all...
> 
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> index 4b4d06199b45..ab33e6fe5b1a 100644
>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
>>  		mac_control |= CPSW_SL_CTL_GIG;
>>  	if (interface == PHY_INTERFACE_MODE_SGMII)
>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
>> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
>> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
> 
> The configuration of the interface mode should *not* happen in
> mac_link_up(), but should happen in e.g. mac_config().

I will move all the interface mode associated configurations to mac_config() in
the v2 series.

> 
>>  	if (speed == SPEED_10 && phy_interface_mode_is_rgmii(interface))
>>  		/* Can be used with in band mode only */
>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
>> @@ -2175,6 +2177,7 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
>>  
>>  	case PHY_INTERFACE_MODE_QSGMII:
>>  	case PHY_INTERFACE_MODE_SGMII:
>> +	case PHY_INTERFACE_MODE_USXGMII:
>>  		if (common->pdata.extra_modes & BIT(port->slave.phy_if)) {
>>  			__set_bit(port->slave.phy_if,
>>  				  port->slave.phylink_config.supported_interfaces);
>> @@ -2182,6 +2185,9 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
>>  			dev_err(dev, "selected phy-mode is not supported\n");
>>  			return -EOPNOTSUPP;
>>  		}
>> +		/* For USXGMII mode, enable MAC_5000FD */
>> +		if (port->slave.phy_if == PHY_INTERFACE_MODE_USXGMII)
>> +			port->slave.phylink_config.mac_capabilities |= MAC_5000FD;
> 
> MAC capabilities should not be conditional in the interface mode.
> Phylink already knows the capabilities of each interface mode, and
> will mask the mac_capabilities accordingly. Phylink wants to know
> what speeds the MAC itself is capable of unbound by the interface
> mode.
> 
> The interface modes that you already support (RGMII, RMII, QSGMII
> and SGMII) do not support anything faster than 1G, so only
> mac_capabilities up to and including 1G speeds will be permitted
> for those interface modes internally by phylink.
> 
> So, making this conditional on USXGMII is just repeating logic that
> is already present internally in phylink.

Thank you for clarifying. I will fix this in the v2 series.

Regards,
Siddharth.