Hi,
On 12/18/2023 7:51 PM, Maxime Chevallier wrote:
> Hi,
>
> On Mon, 18 Dec 2023 12:41:18 +0530
> Sneh Shah <quic_snehshah@xxxxxxxxxxx> 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)
>> +
>> 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;
>
> Looks like you are storing SPEED_XXX definitions here, which can be
> negative in case of SPEED_UNKNOWN, so this should be an int.
Agree, will update this in next patch.
>
>> };
>>
>> 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);
>
> I understand that SGMII's serdes always runs at 1000 / 2500, but this
> check doesn't make much sense then, if the speed isn't 1000, then you
> set the serdes PHY's speed to 100, and the assignment that comes after
> that switch-case will also set the serdes speed to 100.
>
> Also, if the serdes PHY really needs to be configured differently for
> 10/100/1000, then switching from speed 1000 to speed 100 for example
> won't trigger a serdes PHY reconfiguration here.
>
> My guess is that you want something like :
>
> phy_set_speed(ethqos->serdes_phy, SPEED_1000)
>
> and the assignment at the end of the switch-case should be
> SPEED_1000/SPEED_2500 only (see the comment bellow).
Good point, we have only serdes speed of 1000 and 2500. So for 1000/100/10 we should set ethqos_serdes to speed_1000 and pass speed_1000 to phy_set_speed in case of 1000/100/10. Will update this in next patch.
>
>> + 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);
>
> Same remark here
>
>> + 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;
>
> Although the code will behave the same, as you are storing the true
> serdes speed here, shouldn't it be either SPEED_1000 or SPEED_2500 ?
>
> You'll end-up storing SPEED_10 / SPEED_100 should the link use these
> speeds, which doesn't represent the true serdes speed.
>
> This would spare serdes reconfigurations when alternating between
> 10/100/1000M speeds.
>
>> 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);
>>
>
> Thanks,
>
> Maxime
