diff mbox series

[net-next,v3,14/47] net: phy: aquantia: Add support for rate adaptation

Message ID 20220715215954.1449214-15-sean.anderson@seco.com (mailing list archive)
State New, archived
Headers show
Series net: dpaa: Convert to phylink | expand

Commit Message

Sean Anderson July 15, 2022, 9:59 p.m. UTC 
This adds support for rate adaptation for phys similar to the AQR107. We
assume that all phys using aqr107_read_status support rate adaptation.
However, it could be possible to determine support based on the firmware
revision if there are phys discovered which do not support rate adaptation.
However, as rate adaptation is advertised in the datasheets for these phys,
I suspect it is supported most boards.

Despite the name, the "config" registers are updated with the current rate
adaptation method (if any). Because they appear to be updated
automatically, I don't know if these registers can be used to disable rate
adaptation.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v3:
- New

 drivers/net/phy/aquantia_main.c | 49 ++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 4 deletions(-)

Comments

Andrew Lunn July 16, 2022, 6:38 p.m. UTC | #1 
> +#define VEND1_GLOBAL_CFG_10M			0x0310
> +#define VEND1_GLOBAL_CFG_100M			0x031b
> +#define VEND1_GLOBAL_CFG_1G			0x031c
> +#define VEND1_GLOBAL_CFG_2_5G			0x031d
> +#define VEND1_GLOBAL_CFG_5G			0x031e
> +#define VEND1_GLOBAL_CFG_10G			0x031f

I completely read this wrong the first time... The common meaning of
#defines line this is

VEND1_GLOBAL_CFG_ is the register and what follows indicates some bits
in the register.

However, this is not true here, these are all registers. Maybe add
_REG to the end? It makes them different to other defines for
registers, but if i parsed it wrong, probably other will as well?

>  static int aqr107_read_rate(struct phy_device *phydev)
>  {
>  	int val;
> +	u32 config_reg;

Revere Christmass tree. config_reg should be first.

       Andrew
Sean Anderson July 16, 2022, 10:45 p.m. UTC | #2 
On 7/16/22 2:38 PM, Andrew Lunn wrote:
>> +#define VEND1_GLOBAL_CFG_10M			0x0310
>> +#define VEND1_GLOBAL_CFG_100M			0x031b
>> +#define VEND1_GLOBAL_CFG_1G			0x031c
>> +#define VEND1_GLOBAL_CFG_2_5G			0x031d
>> +#define VEND1_GLOBAL_CFG_5G			0x031e
>> +#define VEND1_GLOBAL_CFG_10G			0x031f
> 
> I completely read this wrong the first time... The common meaning of
> #defines line this is
> 
> VEND1_GLOBAL_CFG_ is the register and what follows indicates some bits
> in the register.
> 
> However, this is not true here, these are all registers. Maybe add
> _REG to the end? It makes them different to other defines for
> registers, but if i parsed it wrong, probably other will as well?

How about a comment like

/* The following registers all have similar layouts; first the registers... */
#define VEND1_GLOBAL_CFG_10M				0x0310
...
/* ... and now the fields */
#define VEND1_GLOBAL_CFG_RATE_ADAPT			GENMASK(8, 7)

>>   static int aqr107_read_rate(struct phy_device *phydev)
>>   {
>>   	int val;
>> +	u32 config_reg;
> 
> Revere Christmass tree. config_reg should be first.

OK

--Sean
Andrew Lunn July 17, 2022, 1:42 a.m. UTC | #3 
> /* The following registers all have similar layouts; first the registers... */
> #define VEND1_GLOBAL_CFG_10M				0x0310
> ...
> /* ... and now the fields */
> #define VEND1_GLOBAL_CFG_RATE_ADAPT			GENMASK(8, 7)

O.K, that will also help prevent the misunderstanding i had.

     Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 0a2f8c4aa845..e2ddcf0a68fc 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -95,6 +95,17 @@ 
 #define VEND1_GLOBAL_FW_ID_MAJOR		GENMASK(15, 8)
 #define VEND1_GLOBAL_FW_ID_MINOR		GENMASK(7, 0)
 
+#define VEND1_GLOBAL_CFG_10M			0x0310
+#define VEND1_GLOBAL_CFG_100M			0x031b
+#define VEND1_GLOBAL_CFG_1G			0x031c
+#define VEND1_GLOBAL_CFG_2_5G			0x031d
+#define VEND1_GLOBAL_CFG_5G			0x031e
+#define VEND1_GLOBAL_CFG_10G			0x031f
+#define VEND1_GLOBAL_CFG_RATE_ADAPT		GENMASK(8, 7)
+#define VEND1_GLOBAL_CFG_RATE_ADAPT_NONE	0
+#define VEND1_GLOBAL_CFG_RATE_ADAPT_USX		1
+#define VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE	2
+
 #define VEND1_GLOBAL_RSVD_STAT1			0xc885
 #define VEND1_GLOBAL_RSVD_STAT1_FW_BUILD_ID	GENMASK(7, 4)
 #define VEND1_GLOBAL_RSVD_STAT1_PROV_ID		GENMASK(3, 0)
@@ -340,39 +351,56 @@  static int aqr_read_status(struct phy_device *phydev)
 static int aqr107_read_rate(struct phy_device *phydev)
 {
 	int val;
+	u32 config_reg;
 
 	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_STATUS1);
 	if (val < 0)
 		return val;
 
+	if (val & MDIO_AN_TX_VEND_STATUS1_FULL_DUPLEX)
+		phydev->duplex = DUPLEX_FULL;
+	else
+		phydev->duplex = DUPLEX_HALF;
+
 	switch (FIELD_GET(MDIO_AN_TX_VEND_STATUS1_RATE_MASK, val)) {
 	case MDIO_AN_TX_VEND_STATUS1_10BASET:
 		phydev->speed = SPEED_10;
+		config_reg = VEND1_GLOBAL_CFG_10M;
 		break;
 	case MDIO_AN_TX_VEND_STATUS1_100BASETX:
 		phydev->speed = SPEED_100;
+		config_reg = VEND1_GLOBAL_CFG_100M;
 		break;
 	case MDIO_AN_TX_VEND_STATUS1_1000BASET:
 		phydev->speed = SPEED_1000;
+		config_reg = VEND1_GLOBAL_CFG_1G;
 		break;
 	case MDIO_AN_TX_VEND_STATUS1_2500BASET:
 		phydev->speed = SPEED_2500;
+		config_reg = VEND1_GLOBAL_CFG_2_5G;
 		break;
 	case MDIO_AN_TX_VEND_STATUS1_5000BASET:
 		phydev->speed = SPEED_5000;
+		config_reg = VEND1_GLOBAL_CFG_5G;
 		break;
 	case MDIO_AN_TX_VEND_STATUS1_10GBASET:
 		phydev->speed = SPEED_10000;
+		config_reg = VEND1_GLOBAL_CFG_10G;
 		break;
 	default:
 		phydev->speed = SPEED_UNKNOWN;
-		break;
+		return 0;
 	}
 
-	if (val & MDIO_AN_TX_VEND_STATUS1_FULL_DUPLEX)
-		phydev->duplex = DUPLEX_FULL;
+	val = phy_read_mmd(phydev, MDIO_MMD_VEND1, config_reg);
+	if (val < 0)
+		return val;
+
+	if (FIELD_GET(VEND1_GLOBAL_CFG_RATE_ADAPT, val) ==
+	    VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE)
+		phydev->rate_adaptation = RATE_ADAPT_PAUSE;
 	else
-		phydev->duplex = DUPLEX_HALF;
+		phydev->rate_adaptation = RATE_ADAPT_NONE;
 
 	return 0;
 }
@@ -613,6 +641,16 @@  static void aqr107_link_change_notify(struct phy_device *phydev)
 		phydev_info(phydev, "Aquantia 1000Base-T2 mode active\n");
 }
 
+static enum rate_adaptation
+aqr107_get_rate_adaptation(struct phy_device *phydev, phy_interface_t iface)
+{
+	if (iface == PHY_INTERFACE_MODE_10GBASER ||
+	    iface == PHY_INTERFACE_MODE_2500BASEX ||
+	    iface == PHY_INTERFACE_MODE_NA)
+		return RATE_ADAPT_PAUSE;
+	return RATE_ADAPT_NONE;
+}
+
 static int aqr107_suspend(struct phy_device *phydev)
 {
 	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, MDIO_CTRL1,
@@ -674,6 +712,7 @@  static struct phy_driver aqr_driver[] = {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQR107),
 	.name		= "Aquantia AQR107",
 	.probe		= aqr107_probe,
+	.get_rate_adaptation = aqr107_get_rate_adaptation,
 	.config_init	= aqr107_config_init,
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
@@ -692,6 +731,7 @@  static struct phy_driver aqr_driver[] = {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQR115),
 	.name		= "Aquantia AQR115",
 	.probe		= aqr107_probe,
+	.get_rate_adaptation = aqr107_get_rate_adaptation,
 	.config_init	= aqr107_config_init,
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
@@ -710,6 +750,7 @@  static struct phy_driver aqr_driver[] = {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQCS109),
 	.name		= "Aquantia AQCS109",
 	.probe		= aqr107_probe,
+	.get_rate_adaptation = aqr107_get_rate_adaptation,
 	.config_init	= aqcs109_config_init,
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,