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 <[email protected]>
---
.../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;
};
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;
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
Hi,
On Mon, 18 Dec 2023 12:41:18 +0530
Sneh Shah <[email protected]> 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 <[email protected]>
> ---
> .../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.
> };
>
> 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).
> + 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
Hi,
On 12/18/2023 7:51 PM, Maxime Chevallier wrote:
> Hi,
>
> On Mon, 18 Dec 2023 12:41:18 +0530
> Sneh Shah <[email protected]> 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 <[email protected]>
>> ---
>> .../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
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 <[email protected]>
> ---
> .../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
> 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.
>
> 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
>
On Mon, Dec 18, 2023 at 10:20:03AM -0600, 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 <[email protected]>
> > ---
> > .../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
>
> > 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);
Also, please capture the return value here and propagate the error as
appropriate.
> > + 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.
>
> >
> > 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
> >
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 <[email protected]>
>> ---
>> .../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
>>
>
On Wed, Dec 20, 2023 at 01:02:45PM +0530, Sneh Shah wrote:
>
>
> 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 <[email protected]>
> >> ---
> >> .../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;
Another nit as I look closer: I think this should be grouped by phy_mode
etc just for readability.
> >> };
> >>
> >> 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.
I'd evaluate if you can update that function to clear the ANE bit when
the ane boolean is false. From the usage I see I feel that makes sense,
but correct me if you think I'm wrong.
At the very least let's use the defines from there, and possibly add a
new function if clearing is not acceptable in dwmac_ctrl_ane().
Stepping back, I was asking in general is the need to muck with ANE here
is a Qualcomm specific problem, or is that a generic thing that should be
handled in the core (and the phy_set_speed() bit stay here)? i.e. would
any dwmac5 based IP need to do something like this for SPEED_2500?
> >>
> >> 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
> >>
> >
>
On 12/20/2023 9:29 PM, Andrew Halaney wrote:
> On Wed, Dec 20, 2023 at 01:02:45PM +0530, Sneh Shah wrote:
>>
>>
>> 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 <[email protected]>
>>>> ---
>>>> .../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;
>
> Another nit as I look closer: I think this should be grouped by phy_mode
> etc just for readability.
Didn't get this. can you please elaborate more?
>
>>>> };
>>>>
>>>> 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.
>
> I'd evaluate if you can update that function to clear the ANE bit when
> the ane boolean is false. From the usage I see I feel that makes sense,
> but correct me if you think I'm wrong.
> At the very least let's use the defines from there, and possibly add a
> new function if clearing is not acceptable in dwmac_ctrl_ane().
>
> Stepping back, I was asking in general is the need to muck with ANE here
> is a Qualcomm specific problem, or is that a generic thing that should be
> handled in the core (and the phy_set_speed() bit stay here)? i.e. would
> any dwmac5 based IP need to do something like this for SPEED_2500?
I think disabling ANE for SPEED_2500 is generic not specific to qualcomm. Even in dwxgmac2 versions also we need to disable ANE for SPEED_2500. Autoneg clause 37 stadard doesn't support 2500 speed. So we need to disable autoneg for speed 2500
>
>>>>
>>>> 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
>>>>
>>>
>>
>
On Thu, Dec 21, 2023 at 02:23:57PM +0530, Sneh Shah wrote:
>
>
> On 12/20/2023 9:29 PM, Andrew Halaney wrote:
> > On Wed, Dec 20, 2023 at 01:02:45PM +0530, Sneh Shah wrote:
> >>
> >>
> >> 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 <[email protected]>
> >>>> ---
> >>>> .../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;
> >
> > Another nit as I look closer: I think this should be grouped by phy_mode
> > etc just for readability.
> Didn't get this. can you please elaborate more?
I meant instead of this:
struct qcom_ethqos {
struct platform_device *pdev;
void __iomem *rgmii_base;
void __iomem *mac_base;
int (*configure_func)(struct qcom_ethqos *ethqos);
unsigned int link_clk_rate;
struct clk *link_clk;
struct phy *serdes_phy;
unsigned int speed;
phy_interface_t phy_mode;
const struct ethqos_emac_por *por;
unsigned int num_por;
bool rgmii_config_loopback_en;
bool has_emac_ge_3;
unsigned int serdes_speed;
};
I think this would make more logical sense:
struct qcom_ethqos {
struct platform_device *pdev;
void __iomem *rgmii_base;
void __iomem *mac_base;
int (*configure_func)(struct qcom_ethqos *ethqos);
unsigned int link_clk_rate;
struct clk *link_clk;
struct phy *serdes_phy;
unsigned int serdes_speed;
unsigned int speed;
phy_interface_t phy_mode;
const struct ethqos_emac_por *por;
unsigned int num_por;
bool rgmii_config_loopback_en;
bool has_emac_ge_3;
};
It is definitely nit picking though :)
> >
> >>>> };
> >>>>
> >>>> 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.
> >
> > I'd evaluate if you can update that function to clear the ANE bit when
> > the ane boolean is false. From the usage I see I feel that makes sense,
> > but correct me if you think I'm wrong.
> > At the very least let's use the defines from there, and possibly add a
> > new function if clearing is not acceptable in dwmac_ctrl_ane().
> >
> > Stepping back, I was asking in general is the need to muck with ANE here
> > is a Qualcomm specific problem, or is that a generic thing that should be
> > handled in the core (and the phy_set_speed() bit stay here)? i.e. would
> > any dwmac5 based IP need to do something like this for SPEED_2500?
> I think disabling ANE for SPEED_2500 is generic not specific to qualcomm. Even in dwxgmac2 versions also we need to disable ANE for SPEED_2500. Autoneg clause 37 stadard doesn't support 2500 speed. So we need to disable autoneg for speed 2500
Another nit, sorry for being so picky. Can you please wrap your emails
to around 80 characters? That's the general etiquette when replying
on-list, makes it easier to read (similar to say a commit message).
Thanks for explaining that. Then in my opinion based on what you've said
I think the disabling of ANE for SPEED_2500 should be done outside of
the Qualcomm platform code.
Note, I'm struggling to keep up with the standards at play here, so if
someone else who's a bit more wise on these topics has an opinion I'd
listen to them. I find myself rewatching this presentation from
Maxime/Antoine as a primer on all of this:
https://www.youtube.com/watch?v=K962S9gTBVM
If anyone's got any recommended resources for me to read in particular I
am all ears.
I'll be out the next 2-3 weeks, so don't wait for any responses from me
:)
Thanks,
Andrew
>
> >
> >>>>
> >>>> 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
> >>>>
> >>>
> >>
> >
>
Hello Andrew,
On Thu, 21 Dec 2023 08:30:49 -0600
Andrew Halaney <[email protected]> wrote:
[...]
>
> Note, I'm struggling to keep up with the standards at play here, so if
> someone else who's a bit more wise on these topics has an opinion I'd
> listen to them. I find myself rewatching this presentation from
> Maxime/Antoine as a primer on all of this:
>
> https://www.youtube.com/watch?v=K962S9gTBVM
:)
> If anyone's got any recommended resources for me to read in particular I
> am all ears.
I think Russell and Andrew did a good job clarifying some quirks with
all these standards :
https://elixir.bootlin.com/linux/latest/source/Documentation/networking/phy.rst#L229
There are some more info in Andrew's LPC talk here :
http://vger.kernel.org/lpc_net2018_talks/phylink-and-sfp-slides.pdf
(more phylink related though)
But I agree that this is hard to fully grasp, there are so many
variants everywhere, some standard, some ad-hoc standards, etc.
Maxime
On Thu, Dec 21, 2023 at 02:23:57PM +0530, Sneh Shah wrote:
> On 12/20/2023 9:29 PM, Andrew Halaney wrote:
> > I'd evaluate if you can update that function to clear the ANE bit when
> > the ane boolean is false. From the usage I see I feel that makes sense,
> > but correct me if you think I'm wrong.
> > At the very least let's use the defines from there, and possibly add a
> > new function if clearing is not acceptable in dwmac_ctrl_ane().
> >
> > Stepping back, I was asking in general is the need to muck with ANE here
> > is a Qualcomm specific problem, or is that a generic thing that should be
> > handled in the core (and the phy_set_speed() bit stay here)? i.e. would
> > any dwmac5 based IP need to do something like this for SPEED_2500?
> I think disabling ANE for SPEED_2500 is generic not specific to qualcomm.
> Even in dwxgmac2 versions also we need to disable ANE for SPEED_2500.
> Autoneg clause 37 stadard doesn't support 2500 speed. So we need to
> disable autoneg for speed 2500
(Going back over the history of this addition)
What 802.3 Clause 37 says is utterly _irrelevant_ when discussing Cisco
SGMII. Cisco took 802.3 1000base-X and modified it for their own
purposes, changing the format of the 16-bit control word, adding support
for symbol replication to support 100Mbps and 10Mbps, changing the link
timer, etc. SGMII is *not* 802.3 Clause 37.
I guess you are getting caught up in the widespread crud where
manufacturers stupidly abuse "SGMII" to mean maybe "Cisco SGMII" and
maybe "802.3 1000base-X" because both are "serial gigabit MII". Yes,
both are serial in nature, but Cisco SGMII is not 1000base-X and it
also is not 2500base-X.
What makes this even more difficult is that 2500base-X was never
standardised by the 802.3 committees until very late, so we've ended
up with manufacturers doing their own thing for years. We've ended up
with a mess of different implementations described in different ways
many of which boil down to being 2500base-X without inband AN. For
example, one manufacturer talks about "HS-SGMII", but doesn't permit
the interface to operate at the x10 and x100 symbol replications that
conventional Cisco SGMII uses for 100M and 10M speeds respectfully,
making it in effect no different from 2500base-X.
Now through into this mess various implementations that do not support
inband at 2.5G speeds, those that require inband at 2.5G speeds... one
can get into the situation where one pairs a PHY that requires inband
with a PCS that doesn't support it and the result doesn't work. This
is particularly problematical if the PHY is on a hotpluggable module
like a SFP.
It's a total trainwreck.
I do have some work-in-progress patches that attempt to sort this out
in phylink and identify incompatible situations.
See http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue
commits (I think)...
net: phylink: clean up phylink_resolve()
to:
net: phylink: switch to MLO_AN_PHY when PCS uses outband
and since I'm converting stmmac's hacky PCS that bypasses phylink to
a real phylink_pcs, the ethqos code as it stands presents a blocker
because of this issue. So, I'm intending to post a series in the next
few days (after the bank holiday) and will definitely need to be
tested on ethqos hardware.
However, first we need to get to the bottom of your latest patch that
only sets PHY_INTERFACE_MODE_2500BASEX when plat_dat->flags has the
STMMAC_FLAG_HAS_INTEGRATED_PCS flag _set_, but the stmmac code very
oddly does _not_ use the built-in PCS if this flag is set. See:
stmmac_ethtool_get_link_ksettings()
stmmac_ethtool_set_link_ksettings()
and their use of pcs_link / pcs_duplex / pcs_speed. Also see
stmmac_common_interrupt()
and its use of pcs_link to control the carrier, the dwmac1000 and
dwmac4 code that reads the status from the GMAC, updating the
pcs_link / pcs_duplex / pcs_speed variables.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Sun, May 26, 2024 at 05:27:03PM GMT, Russell King (Oracle) wrote:
> On Thu, Dec 21, 2023 at 02:23:57PM +0530, Sneh Shah wrote:
> > On 12/20/2023 9:29 PM, Andrew Halaney wrote:
> > > I'd evaluate if you can update that function to clear the ANE bit when
> > > the ane boolean is false. From the usage I see I feel that makes sense,
> > > but correct me if you think I'm wrong.
> > > At the very least let's use the defines from there, and possibly add a
> > > new function if clearing is not acceptable in dwmac_ctrl_ane().
> > >
> > > Stepping back, I was asking in general is the need to muck with ANE here
> > > is a Qualcomm specific problem, or is that a generic thing that should be
> > > handled in the core (and the phy_set_speed() bit stay here)? i.e. would
> > > any dwmac5 based IP need to do something like this for SPEED_2500?
> > I think disabling ANE for SPEED_2500 is generic not specific to qualcomm.
> > Even in dwxgmac2 versions also we need to disable ANE for SPEED_2500.
> > Autoneg clause 37 stadard doesn't support 2500 speed. So we need to
> > disable autoneg for speed 2500
>
> (Going back over the history of this addition)
>
> What 802.3 Clause 37 says is utterly _irrelevant_ when discussing Cisco
> SGMII. Cisco took 802.3 1000base-X and modified it for their own
> purposes, changing the format of the 16-bit control word, adding support
> for symbol replication to support 100Mbps and 10Mbps, changing the link
> timer, etc. SGMII is *not* 802.3 Clause 37.
>
> I guess you are getting caught up in the widespread crud where
> manufacturers stupidly abuse "SGMII" to mean maybe "Cisco SGMII" and
> maybe "802.3 1000base-X" because both are "serial gigabit MII". Yes,
> both are serial in nature, but Cisco SGMII is not 1000base-X and it
> also is not 2500base-X.
>
> What makes this even more difficult is that 2500base-X was never
> standardised by the 802.3 committees until very late, so we've ended
> up with manufacturers doing their own thing for years. We've ended up
> with a mess of different implementations described in different ways
> many of which boil down to being 2500base-X without inband AN. For
> example, one manufacturer talks about "HS-SGMII", but doesn't permit
> the interface to operate at the x10 and x100 symbol replications that
> conventional Cisco SGMII uses for 100M and 10M speeds respectfully,
> making it in effect no different from 2500base-X.
>
> Now through into this mess various implementations that do not support
> inband at 2.5G speeds, those that require inband at 2.5G speeds... one
> can get into the situation where one pairs a PHY that requires inband
> with a PCS that doesn't support it and the result doesn't work. This
> is particularly problematical if the PHY is on a hotpluggable module
> like a SFP.
>
> It's a total trainwreck.
>
> I do have some work-in-progress patches that attempt to sort this out
> in phylink and identify incompatible situations.
>
> See http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue
>
> commits (I think)...
>
> net: phylink: clean up phylink_resolve()
>
> to:
>
> net: phylink: switch to MLO_AN_PHY when PCS uses outband
>
> and since I'm converting stmmac's hacky PCS that bypasses phylink to
> a real phylink_pcs, the ethqos code as it stands presents a blocker
> because of this issue. So, I'm intending to post a series in the next
> few days (after the bank holiday) and will definitely need to be
> tested on ethqos hardware.
Whatever you work out here with Qualcomm, I can at least test this on this
board:
https://elixir.bootlin.com/linux/v6.9-rc3/source/arch/arm64/boot/dts/qcom/sa8775p-ride.dts#L266
So basically the same SoC as the one Sneh is adding some support for
here, but on a board with a Marvell 88EA1512 connected via SGMII stuck in
a remote lab somewhere.
I don't have documentation on the IP though... but can at least provide
some testing. Qualcomm has access (I think) to two other boards with the
same SoC, one with some Aquantia phy (that I think is 2500 Mbps
capable), and another with a fixed-link setup at 2500 Mbps. If I
understand correctly the latter works with the driver already.
Please CC me on future patches on the topic and I'll at least give them
a spin and a look, following through some of your threads today this is a
real rats nest.
Thanks,
Andrew
>
> However, first we need to get to the bottom of your latest patch that
> only sets PHY_INTERFACE_MODE_2500BASEX when plat_dat->flags has the
> STMMAC_FLAG_HAS_INTEGRATED_PCS flag _set_, but the stmmac code very
> oddly does _not_ use the built-in PCS if this flag is set. See:
>
> stmmac_ethtool_get_link_ksettings()
> stmmac_ethtool_set_link_ksettings()
>
> and their use of pcs_link / pcs_duplex / pcs_speed. Also see
>
> stmmac_common_interrupt()
>
> and its use of pcs_link to control the carrier, the dwmac1000 and
> dwmac4 code that reads the status from the GMAC, updating the
> pcs_link / pcs_duplex / pcs_speed variables.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
>
On 5/26/2024 9:57 PM, Russell King (Oracle) wrote:
> On Thu, Dec 21, 2023 at 02:23:57PM +0530, Sneh Shah wrote:
>> On 12/20/2023 9:29 PM, Andrew Halaney wrote:
>>> I'd evaluate if you can update that function to clear the ANE bit when
>>> the ane boolean is false. From the usage I see I feel that makes sense,
>>> but correct me if you think I'm wrong.
>>> At the very least let's use the defines from there, and possibly add a
>>> new function if clearing is not acceptable in dwmac_ctrl_ane().
>>>
>>> Stepping back, I was asking in general is the need to muck with ANE here
>>> is a Qualcomm specific problem, or is that a generic thing that should be
>>> handled in the core (and the phy_set_speed() bit stay here)? i.e. would
>>> any dwmac5 based IP need to do something like this for SPEED_2500?
>> I think disabling ANE for SPEED_2500 is generic not specific to qualcomm.
>> Even in dwxgmac2 versions also we need to disable ANE for SPEED_2500.
>> Autoneg clause 37 stadard doesn't support 2500 speed. So we need to
>> disable autoneg for speed 2500
>
> (Going back over the history of this addition)
>
> What 802.3 Clause 37 says is utterly _irrelevant_ when discussing Cisco
> SGMII. Cisco took 802.3 1000base-X and modified it for their own
> purposes, changing the format of the 16-bit control word, adding support
> for symbol replication to support 100Mbps and 10Mbps, changing the link
> timer, etc. SGMII is *not* 802.3 Clause 37.
>
> I guess you are getting caught up in the widespread crud where
> manufacturers stupidly abuse "SGMII" to mean maybe "Cisco SGMII" and
> maybe "802.3 1000base-X" because both are "serial gigabit MII". Yes,
> both are serial in nature, but Cisco SGMII is not 1000base-X and it
> also is not 2500base-X.
>
> What makes this even more difficult is that 2500base-X was never
> standardised by the 802.3 committees until very late, so we've ended
> up with manufacturers doing their own thing for years. We've ended up
> with a mess of different implementations described in different ways
> many of which boil down to being 2500base-X without inband AN. For
> example, one manufacturer talks about "HS-SGMII", but doesn't permit
> the interface to operate at the x10 and x100 symbol replications that
> conventional Cisco SGMII uses for 100M and 10M speeds respectfully,
> making it in effect no different from 2500base-X.
>
> Now through into this mess various implementations that do not support
> inband at 2.5G speeds, those that require inband at 2.5G speeds... one
> can get into the situation where one pairs a PHY that requires inband
> with a PCS that doesn't support it and the result doesn't work. This
> is particularly problematical if the PHY is on a hotpluggable module
> like a SFP.
>
> It's a total trainwreck.
Qualcomm ethernet HW supports 2.5G speed in overclocked SGMII mode.
we internally term it as OCSGMII.
End goal of these patches is to enable SGMII with 2.5G speed support.
The patch in these series enabled up SGMII with 2.5 for cases where we
don't have external phy. ( mac-to-mac connectivity)
The new patch posted extends this for the case when the MAC has an
external phy connected. ( hence we are advertising fr 2.5G speed by adding
2500BASEX as supported interface in phylink)
>
> I do have some work-in-progress patches that attempt to sort this out
> in phylink and identify incompatible situations.
>
> See http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue
>
> commits (I think)...
>
> net: phylink: clean up phylink_resolve()
>
> to:
>
> net: phylink: switch to MLO_AN_PHY when PCS uses outband
>
> and since I'm converting stmmac's hacky PCS that bypasses phylink to
> a real phylink_pcs, the ethqos code as it stands presents a blocker
> because of this issue. So, I'm intending to post a series in the next
> few days (after the bank holiday) and will definitely need to be
> tested on ethqos hardware.
>
I am going over the list of these patches.
> However, first we need to get to the bottom of your latest patch that
> only sets PHY_INTERFACE_MODE_2500BASEX when plat_dat->flags has the
> STMMAC_FLAG_HAS_INTEGRATED_PCS flag _set_, but the stmmac code very
> oddly does _not_ use the built-in PCS if this flag is set. See:
>
> stmmac_ethtool_get_link_ksettings()
> stmmac_ethtool_set_link_ksettings()
>
> and their use of pcs_link / pcs_duplex / pcs_speed. Also see
>
> stmmac_common_interrupt()
>
> and its use of pcs_link to control the carrier, the dwmac1000 and
> dwmac4 code that reads the status from the GMAC, updating the
> pcs_link / pcs_duplex / pcs_speed variables.
In this version of qualcomm ethernet, PCS is not an independent HW
block. It is integrated to MAC block itself. It has very limited
configuration.Here PCS doesn't have it's own link speed/duplex
capabities. Hence we are bypassing all this PCS related functionalities.
I will update with more details on the integrated PCS block autoneg
standard.
>
> Qualcomm ethernet HW supports 2.5G speed in overclocked SGMII mode.
> we internally term it as OCSGMII.
So it still does SGMII inband signalling? Not 2500BaseX signalling? It
is not actually compatible with 2500BaseX?
> End goal of these patches is to enable SGMII with 2.5G speed support.
> The patch in these series enabled up SGMII with 2.5 for cases where we
> don't have external phy. ( mac-to-mac connectivity)
So the other end needs to be an over clocked SGMII MAC, not 2500BaseX?
> The new patch posted extends this for the case when the MAC has an
> external phy connected. ( hence we are advertising fr 2.5G speed by adding
> 2500BASEX as supported interface in phylink)
And i assume it does not actually work against a true 2500BaseX
device, because it is doing SGMII inband signalling?
Hence Russell frustration with the whole 2.5G mess....
Andrew
On Wed, May 29, 2024 at 07:43:15PM +0530, Sneh Shah wrote:
> In this version of qualcomm ethernet, PCS is not an independent HW
> block. It is integrated to MAC block itself. It has very limited
> configuration.Here PCS doesn't have it's own link speed/duplex
> capabities. Hence we are bypassing all this PCS related functionalities.
I want to concentrate on this part first - we'll address the 2.5G
issues separately once I've got a picture of this hardware (and thus
can work out what needs to change in my phylink_pcs implementation to
support the standard Cisco SGMII speeds.
From what I understand you're saying, your integrated PCS is different
from the DesignWare integrated PCS?
Which core does it use? dwmac4_core.c or dwmac1000_core.c, or some
other? Not knowing which core makes asking the following questions
harder, since I'm having to double them up to cover both cores with
their different definitions.
Does it only present its status via the GMAC_PHYIF_CONTROL_STATUS or
GMAC_RGSMIIIS register?
From what you're saying:
- if using the dwmac1000 core, then for the registers at GMAC_PCS_BASE
(0xc0 offset)...
- if using the dwmac4 core, then for registers at GMAC_PCS_BASE
(0xe0 offset)...
.. is it true that only the GMAC_AN_CTRL() register is implemented
and none of the other registers listed in stmmac_pcs.h?
In terms of interrupts when the link status changes, how do they
present? Are they through the GMAC_INT_RGSMIIS interrupt only?
What about GMAC_INT_PCS_LINK or GMAC_INT_PCS_ANE? Or in the case
of the other core, is it through the PCS_RGSMIIIS_IRQ interrupt
only? Similarly, what about PCS_LINK_IRQ or PCS_ANE_IRQ?
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, May 29, 2024 at 04:28:16PM +0200, Andrew Lunn wrote:
> > Qualcomm ethernet HW supports 2.5G speed in overclocked SGMII mode.
> > we internally term it as OCSGMII.
>
> So it still does SGMII inband signalling? Not 2500BaseX signalling? It
> is not actually compatible with 2500BaseX?
>
> > End goal of these patches is to enable SGMII with 2.5G speed support.
> > The patch in these series enabled up SGMII with 2.5 for cases where we
> > don't have external phy. ( mac-to-mac connectivity)
>
> So the other end needs to be an over clocked SGMII MAC, not 2500BaseX?
>
> > The new patch posted extends this for the case when the MAC has an
> > external phy connected. ( hence we are advertising fr 2.5G speed by adding
> > 2500BASEX as supported interface in phylink)
>
> And i assume it does not actually work against a true 2500BaseX
> device, because it is doing SGMII inband signalling?
I really hope the hardware isn't using any SGMII inband signalling
at 2.5G speeds. Other devices explicitly state that SGMII inband
signalling while operating the elevated 2.5G speed is *not* supported!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On 5/30/2024 1:43 AM, Russell King (Oracle) wrote:
> On Wed, May 29, 2024 at 04:28:16PM +0200, Andrew Lunn wrote:
>>> Qualcomm ethernet HW supports 2.5G speed in overclocked SGMII mode.
>>> we internally term it as OCSGMII.
>>
>> So it still does SGMII inband signalling? Not 2500BaseX signalling? It
>> is not actually compatible with 2500BaseX?
>>
>>> End goal of these patches is to enable SGMII with 2.5G speed support.
>>> The patch in these series enabled up SGMII with 2.5 for cases where we
>>> don't have external phy. ( mac-to-mac connectivity)
>>
>> So the other end needs to be an over clocked SGMII MAC, not 2500BaseX?
>>
>>> The new patch posted extends this for the case when the MAC has an
>>> external phy connected. ( hence we are advertising fr 2.5G speed by adding
>>> 2500BASEX as supported interface in phylink)
>>
>> And i assume it does not actually work against a true 2500BaseX
>> device, because it is doing SGMII inband signalling?
>
> I really hope the hardware isn't using any SGMII inband signalling
> at 2.5G speeds. Other devices explicitly state that SGMII inband
> signalling while operating the elevated 2.5G speed is *not* supported!
Do we mean SGMII autoneg by SGMII inband signalling? Our hardware doesn't
support autoneg for 2.5G mode in SGMII.
>
On 5/30/2024 1:39 AM, Russell King (Oracle) wrote:
> On Wed, May 29, 2024 at 07:43:15PM +0530, Sneh Shah wrote:
>> In this version of qualcomm ethernet, PCS is not an independent HW
>> block. It is integrated to MAC block itself. It has very limited
>> configuration.Here PCS doesn't have it's own link speed/duplex
>> capabities. Hence we are bypassing all this PCS related functionalities.
>
> I want to concentrate on this part first - we'll address the 2.5G
> issues separately once I've got a picture of this hardware (and thus
> can work out what needs to change in my phylink_pcs implementation to
> support the standard Cisco SGMII speeds.
>
> From what I understand you're saying, your integrated PCS is different
> from the DesignWare integrated PCS?
It's an inbuilt PCS block within designware ETHQoS core.
>
> Which core does it use? dwmac4_core.c or dwmac1000_core.c, or some
> other? Not knowing which core makes asking the following questions
> harder, since I'm having to double them up to cover both cores with
> their different definitions.
it is dwmac4 core with 0xe0 offset.
>
> Does it only present its status via the GMAC_PHYIF_CONTROL_STATUS or
> GMAC_RGSMIIIS register?
It is present via GMAC_PHYIF_CONTROL_STATUS.
>
> From what you're saying:
> - if using the dwmac1000 core, then for the registers at GMAC_PCS_BASE
> (0xc0 offset)...
> - if using the dwmac4 core, then for registers at GMAC_PCS_BASE
> (0xe0 offset)...
> ... is it true that only the GMAC_AN_CTRL() register is implemented
> and none of the other registers listed in stmmac_pcs.h?
>
> In terms of interrupts when the link status changes, how do they
> present? Are they through the GMAC_INT_RGSMIIS interrupt only?
> What about GMAC_INT_PCS_LINK or GMAC_INT_PCS_ANE? Or in the case
> of the other core, is it through the PCS_RGSMIIIS_IRQ interrupt
> only? Similarly, what about PCS_LINK_IRQ or PCS_ANE_IRQ?
we only have GMAC_AN_CTRL and GMAC_AN_STATUS register.
There is no separate IRQ line for PCS link or autoneg.
It is notified via MAC interrupt line only.
>
> Thanks.
>
On Mon, Jun 03, 2024 at 04:57:15PM +0530, Sneh Shah wrote:
> On 5/30/2024 1:39 AM, Russell King (Oracle) wrote:
> > From what you're saying:
> > - if using the dwmac1000 core, then for the registers at GMAC_PCS_BASE
> > (0xc0 offset)...
> > - if using the dwmac4 core, then for registers at GMAC_PCS_BASE
> > (0xe0 offset)...
> > ... is it true that only the GMAC_AN_CTRL() register is implemented
> > and none of the other registers listed in stmmac_pcs.h?
> >
> > In terms of interrupts when the link status changes, how do they
> > present? Are they through the GMAC_INT_RGSMIIS interrupt only?
> > What about GMAC_INT_PCS_LINK or GMAC_INT_PCS_ANE? Or in the case
> > of the other core, is it through the PCS_RGSMIIIS_IRQ interrupt
> > only? Similarly, what about PCS_LINK_IRQ or PCS_ANE_IRQ?
>
> we only have GMAC_AN_CTRL and GMAC_AN_STATUS register.
> There is no separate IRQ line for PCS link or autoneg.
> It is notified via MAC interrupt line only.
From the sound of it, this is just the standard PCS that everyone else
would use in DW ETHQoS, with the exception that you can run it at 2.5G
without inband signalling.
Thanks for clarifying that. I think we can just use the phylink PCS
that I'm proposing for your case, with the exception of also adding
support for 2.5G speeds, which I will need to sort out.
So, I think I need to get my patch set that query the inband
capabilities of the PCS and PHY into net-next before we can move
forward with 2.5G speeds here.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!