Re: [PATCH net-next] net: stmmac: dwmac-qcom-ethqos: Add support for 2.5G SGMII

[Sneh Shah]

On 12/18/2023 9:50 PM, Andrew Halaney wrote:
> On Mon, Dec 18, 2023 at 12:41:18PM +0530, Sneh Shah wrote:
>> Serdes phy needs to operate at 2500 mode for 2.5G speed and 1000
>> mode for 1G/100M/10M speed.
>> Added changes to configure serdes phy and mac based on link speed.
>>
>> Signed-off-by: Sneh Shah <quic_snehshah@xxxxxxxxxxx>
>> ---
>>  .../stmicro/stmmac/dwmac-qcom-ethqos.c        | 31 +++++++++++++++++--
>>  1 file changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>> index d3bf42d0fceb..b3a28dc19161 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>> @@ -21,6 +21,7 @@
>>  #define RGMII_IO_MACRO_CONFIG2		0x1C
>>  #define RGMII_IO_MACRO_DEBUG1		0x20
>>  #define EMAC_SYSTEM_LOW_POWER_DEBUG	0x28
>> +#define ETHQOS_MAC_AN_CTRL		0xE0
>>  
>>  /* RGMII_IO_MACRO_CONFIG fields */
>>  #define RGMII_CONFIG_FUNC_CLK_EN		BIT(30)
>> @@ -78,6 +79,10 @@
>>  #define ETHQOS_MAC_CTRL_SPEED_MODE		BIT(14)
>>  #define ETHQOS_MAC_CTRL_PORT_SEL		BIT(15)
>>  
>> +/*ETHQOS_MAC_AN_CTRL bits */
>> +#define ETHQOS_MAC_AN_CTRL_RAN			BIT(9)
>> +#define ETHQOS_MAC_AN_CTRL_ANE			BIT(12)
>> +
> 
> nit: space please add a space before ETHQOS_MAC_AN_CTRL
> 
will take care of this in next patch

>>  struct ethqos_emac_por {
>>  	unsigned int offset;
>>  	unsigned int value;
>> @@ -109,6 +114,7 @@ struct qcom_ethqos {
>>  	unsigned int num_por;
>>  	bool rgmii_config_loopback_en;
>>  	bool has_emac_ge_3;
>> +	unsigned int serdes_speed;
>>  };
>>  
>>  static int rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset)
>> @@ -600,27 +606,47 @@ static int ethqos_configure_rgmii(struct qcom_ethqos *ethqos)
>>  
>>  static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos)
>>  {
>> -	int val;
>> -
>> +	int val, mac_an_value;
>>  	val = readl(ethqos->mac_base + MAC_CTRL_REG);
>> +	mac_an_value = readl(ethqos->mac_base + ETHQOS_MAC_AN_CTRL);
>>  
>>  	switch (ethqos->speed) {
>> +	case SPEED_2500:
>> +		val &= ~ETHQOS_MAC_CTRL_PORT_SEL;
>> +		rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
>> +			      RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
>> +			      RGMII_IO_MACRO_CONFIG2);
>> +		if (ethqos->serdes_speed != SPEED_2500)
>> +			phy_set_speed(ethqos->serdes_phy, ethqos->speed);
>> +		mac_an_value &= ~ETHQOS_MAC_AN_CTRL_ANE;
>> +		break;
>>  	case SPEED_1000:
>>  		val &= ~ETHQOS_MAC_CTRL_PORT_SEL;
>>  		rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
>>  			      RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
>>  			      RGMII_IO_MACRO_CONFIG2);
>> +		if (ethqos->serdes_speed != SPEED_1000)
>> +			phy_set_speed(ethqos->serdes_phy, ethqos->speed);
>> +		mac_an_value |= ETHQOS_MAC_AN_CTRL_RAN | ETHQOS_MAC_AN_CTRL_ANE;
>>  		break;
>>  	case SPEED_100:
>>  		val |= ETHQOS_MAC_CTRL_PORT_SEL | ETHQOS_MAC_CTRL_SPEED_MODE;
>> +		if (ethqos->serdes_speed != SPEED_1000)
>> +			phy_set_speed(ethqos->serdes_phy, ethqos->speed);
>> +		mac_an_value |= ETHQOS_MAC_AN_CTRL_RAN | ETHQOS_MAC_AN_CTRL_ANE;
>>  		break;
>>  	case SPEED_10:
>>  		val |= ETHQOS_MAC_CTRL_PORT_SEL;
>>  		val &= ~ETHQOS_MAC_CTRL_SPEED_MODE;
>> +		if (ethqos->serdes_speed != SPEED_1000)
>> +			phy_set_speed(ethqos->serdes_phy, ethqos->speed);
>> +		mac_an_value |= ETHQOS_MAC_AN_CTRL_RAN | ETHQOS_MAC_AN_CTRL_ANE;
>>  		break;
>>  	}
>>  
>>  	writel(val, ethqos->mac_base + MAC_CTRL_REG);
>> +	writel(mac_an_value, ethqos->mac_base + ETHQOS_MAC_AN_CTRL);
>> +	ethqos->serdes_speed = ethqos->speed;
> 
> I see these bits are generic and there's some functions in stmmac_pcs.h
> that muck with these...
> 
> Could you help me understand if this really should be Qualcomm specific,
> or if this is something that should be considered for the more core bits
> of the driver? I feel in either case we should take advantage of the
> common definitions in that file if possible.
> 
we do have function dwmac_ctrl_ane in core driver which updates same registers. However, it does not have the option to reset ANE bit, it can only set bits. For SPEED_2500 we need to reset ANE bit. Hence I am adding it here. Not sure if we can extend dwmac_ctrl_ane function to reset bits as well.
>>  
>>  	return val;
>>  }
>> @@ -789,6 +815,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>  				     "Failed to get serdes phy\n");
>>  
>>  	ethqos->speed = SPEED_1000;
>> +	ethqos->serdes_speed = SPEED_1000;
>>  	ethqos_update_link_clk(ethqos, SPEED_1000);
>>  	ethqos_set_func_clk_en(ethqos);
>>  
>> -- 
>> 2.17.1
>>
>