[PATCH net-next 0/3] net: phy: aquantia: report Aquantia-specific settings and features

[PATCH net-next 0/3] net: phy: aquantia: report Aquantia-specific settings and features

[Heiner Kallweit]

This series detects and reports quite some Aquantia-specific settings
and features.

Heiner Kallweit (3):
  net: phy: aquantia: report remote capabilities if link partner is
    Aquantia PHY
  net: phy: aquantia: add debug message with PHY details like firmware
    version
  net: phy: aquantia: inform about proprietary 1000Base-T2 mode being in
    use

 drivers/net/phy/aquantia_main.c | 119 ++++++++++++++++++++++++++++++++
 1 file changed, 119 insertions(+)

-- 
2.21.0

[PATCH net-next 1/3] net: phy: aquantia: report remote capabilities if link partner is Aquantia PHY

[Heiner Kallweit]

If both link partners are Aquantia PHY's then additional information is
exchanged as part of the auto-negotiation. Report remote capabilities
if link partner is Aquantia PHY.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/aquantia_main.c | 49 +++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 5c148df28..48d63dd1a 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -58,6 +58,16 @@
 #define MDIO_AN_RX_LP_STAT1			0xe820
 #define MDIO_AN_RX_LP_STAT1_1000BASET_FULL	BIT(15)
 #define MDIO_AN_RX_LP_STAT1_1000BASET_HALF	BIT(14)
+#define MDIO_AN_RX_LP_STAT1_SHORT_REACH		BIT(13)
+#define MDIO_AN_RX_LP_STAT1_AQRATE_DOWNSHIFT	BIT(12)
+#define MDIO_AN_RX_LP_STAT1_AQ_PHY		BIT(2)
+
+#define MDIO_AN_RX_LP_STAT4			0xe823
+#define MDIO_AN_RX_LP_STAT4_FW_MAJOR		GENMASK(15, 8)
+#define MDIO_AN_RX_LP_STAT4_FW_MINOR		GENMASK(7, 0)
+
+#define MDIO_AN_RX_VEND_STAT3			0xe832
+#define MDIO_AN_RX_VEND_STAT3_AFR		BIT(0)
 
 /* Vendor specific 1, MDIO_MMD_VEND1 */
 #define VEND1_GLOBAL_INT_STD_STATUS		0xfc00
@@ -376,6 +386,43 @@ static int aqcs109_config_init(struct phy_device *phydev)
 	return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT);
 }
 
+static void aqr107_link_change_notify(struct phy_device *phydev)
+{
+	u8 fw_major, fw_minor;
+	bool downshift, short_reach, afr;
+	int val;
+
+	if (phydev->state != PHY_RUNNING || phydev->autoneg == AUTONEG_DISABLE)
+		return;
+
+	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RX_LP_STAT1);
+	/* call failed or link partner is no Aquantia PHY */
+	if (val < 0 || !(val & MDIO_AN_RX_LP_STAT1_AQ_PHY))
+		return;
+
+	short_reach = val & MDIO_AN_RX_LP_STAT1_SHORT_REACH;
+	downshift = val & MDIO_AN_RX_LP_STAT1_AQRATE_DOWNSHIFT;
+
+	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RX_LP_STAT4);
+	if (val < 0)
+		return;
+
+	fw_major = FIELD_GET(MDIO_AN_RX_LP_STAT4_FW_MAJOR, val);
+	fw_minor = FIELD_GET(MDIO_AN_RX_LP_STAT4_FW_MINOR, val);
+
+	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RX_VEND_STAT3);
+	if (val < 0)
+		return;
+
+	afr = val & MDIO_AN_RX_VEND_STAT3_AFR;
+
+	phydev_dbg(phydev, "Link partner is Aquantia PHY, FW %u.%u%s%s%s\n",
+		   fw_major, fw_minor,
+		   short_reach ? ", short reach mode" : "",
+		   downshift ? ", fast-retrain downshift advertised" : "",
+		   afr ? ", fast reframe advertised" : "");
+}
+
 static struct phy_driver aqr_driver[] = {
 {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQ1202),
@@ -430,6 +477,7 @@ static struct phy_driver aqr_driver[] = {
 	.read_status	= aqr107_read_status,
 	.get_tunable    = aqr107_get_tunable,
 	.set_tunable    = aqr107_set_tunable,
+	.link_change_notify = aqr107_link_change_notify,
 },
 {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQCS109),
@@ -444,6 +492,7 @@ static struct phy_driver aqr_driver[] = {
 	.read_status	= aqr107_read_status,
 	.get_tunable    = aqr107_get_tunable,
 	.set_tunable    = aqr107_set_tunable,
+	.link_change_notify = aqr107_link_change_notify,
 },
 {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQR405),
-- 
2.21.0

Re: [PATCH net-next 1/3] net: phy: aquantia: report remote capabilities if link partner is Aquantia PHY

[Florian Fainelli]

On 3/23/2019 6:14 AM, Heiner Kallweit wrote:
> If both link partners are Aquantia PHY's then additional information is
> exchanged as part of the auto-negotiation. Report remote capabilities
> if link partner is Aquantia PHY.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Petr is trying to introduce a link down reason, but it sounds like we
should also have have a way to enhance the link up with information like
the one you are providing here. It would also be nice if in the future,
ethtool could display additional vendor specific information when the
link is up. ethtool over netlink should facilitate that.

Thanks!
-- 
Florian

Re: [PATCH net-next 1/3] net: phy: aquantia: report remote capabilities if link partner is Aquantia PHY

[Heiner Kallweit]

On 24.03.2019 02:01, Florian Fainelli wrote:
> 
> 
> On 3/23/2019 6:14 AM, Heiner Kallweit wrote:
>> If both link partners are Aquantia PHY's then additional information is
>> exchanged as part of the auto-negotiation. Report remote capabilities
>> if link partner is Aquantia PHY.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Petr is trying to introduce a link down reason, but it sounds like we
> should also have have a way to enhance the link up with information like
> the one you are providing here. It would also be nice if in the future,
> ethtool could display additional vendor specific information when the
> link is up. ethtool over netlink should facilitate that.
> 
Interesting idea. Most likely the available information will be quite
different, dependent on the vendor. So the PHY driver may have to
expose a list of items, each with a name and type.

> Thanks!
> 
Heiner

[PATCH net-next 2/3] net: phy: aquantia: report PHY details like firmware version

[Heiner Kallweit]

Add reporting firmware details. These details are available only once
the firmware has finished initializing the chip. This can take some
time and we need to poll for init completion.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/aquantia_main.c | 58 +++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 48d63dd1a..01a6d5cd4 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -70,6 +70,14 @@
 #define MDIO_AN_RX_VEND_STAT3_AFR		BIT(0)
 
 /* Vendor specific 1, MDIO_MMD_VEND1 */
+#define VEND1_GLOBAL_FW_ID			0x0020
+#define VEND1_GLOBAL_FW_ID_MAJOR		GENMASK(15, 8)
+#define VEND1_GLOBAL_FW_ID_MINOR		GENMASK(7, 0)
+
+#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)
+
 #define VEND1_GLOBAL_INT_STD_STATUS		0xfc00
 #define VEND1_GLOBAL_INT_VEND_STATUS		0xfc01
 
@@ -349,6 +357,48 @@ static int aqr107_set_tunable(struct phy_device *phydev,
 	}
 }
 
+/* If we configure settings whilst firmware is still initializing the chip,
+ * then these settings may be overwritten. Therefore make sure chip
+ * initialization has completed. Use presence of the firmware ID as
+ * indicator for initialization having completed.
+ * The chip also provides a "reset completed" bit, but it's cleared after
+ * read. Therefore function would time out if called again.
+ */
+static void aqr107_wait_reset_complete(struct phy_device *phydev)
+{
+	int val, retries = 100;
+
+	do {
+		val = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_FW_ID);
+		if (val < 0)
+			return;
+		msleep(20);
+	} while (!val && --retries);
+}
+
+static void aqr107_chip_info(struct phy_device *phydev)
+{
+	u8 fw_major, fw_minor, build_id, prov_id;
+	int val;
+
+	val = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_FW_ID);
+	if (val < 0)
+		return;
+
+	fw_major = FIELD_GET(VEND1_GLOBAL_FW_ID_MAJOR, val);
+	fw_minor = FIELD_GET(VEND1_GLOBAL_FW_ID_MINOR, val);
+
+	val = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_RSVD_STAT1);
+	if (val < 0)
+		return;
+
+	build_id = FIELD_GET(VEND1_GLOBAL_RSVD_STAT1_FW_BUILD_ID, val);
+	prov_id = FIELD_GET(VEND1_GLOBAL_RSVD_STAT1_PROV_ID, val);
+
+	phydev_dbg(phydev, "FW %u.%u, Build %u, Provisioning %u\n",
+		   fw_major, fw_minor, build_id, prov_id);
+}
+
 static int aqr107_config_init(struct phy_device *phydev)
 {
 	/* Check that the PHY interface type is compatible */
@@ -357,9 +407,13 @@ static int aqr107_config_init(struct phy_device *phydev)
 	    phydev->interface != PHY_INTERFACE_MODE_10GKR)
 		return -ENODEV;
 
+	aqr107_wait_reset_complete(phydev);
+
 	/* ensure that a latched downshift event is cleared */
 	aqr107_read_downshift_event(phydev);
 
+	aqr107_chip_info(phydev);
+
 	return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT);
 }
 
@@ -372,6 +426,8 @@ static int aqcs109_config_init(struct phy_device *phydev)
 	    phydev->interface != PHY_INTERFACE_MODE_2500BASEX)
 		return -ENODEV;
 
+	aqr107_wait_reset_complete(phydev);
+
 	/* AQCS109 belongs to a chip family partially supporting 10G and 5G.
 	 * PMA speed ability bits are the same for all members of the family,
 	 * AQCS109 however supports speeds up to 2.5G only.
@@ -383,6 +439,8 @@ static int aqcs109_config_init(struct phy_device *phydev)
 	/* ensure that a latched downshift event is cleared */
 	aqr107_read_downshift_event(phydev);
 
+	aqr107_chip_info(phydev);
+
 	return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT);
 }
 
-- 
2.21.0

Re: [PATCH net-next 2/3] net: phy: aquantia: report PHY details like firmware version

[Florian Fainelli]

On 3/23/2019 6:15 AM, Heiner Kallweit wrote:
> Add reporting firmware details. These details are available only once
> the firmware has finished initializing the chip. This can take some
> time and we need to poll for init completion.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---

[snip]
>  
> +/* If we configure settings whilst firmware is still initializing the chip,
> + * then these settings may be overwritten. Therefore make sure chip
> + * initialization has completed. Use presence of the firmware ID as
> + * indicator for initialization having completed.
> + * The chip also provides a "reset completed" bit, but it's cleared after
> + * read. Therefore function would time out if called again.
> + */

Is there a way to say, run a checksum calculation on the firmware image
to assess its health/validity as well as read the firmware ID? What
happens if the PHY is not provisioned with a firmware image? Is it
expecting for a specific set of MDIO vendor commands to load it over MDIO?

> +static void aqr107_wait_reset_complete(struct phy_device *phydev)
> +{
> +	int val, retries = 100;
> +
> +	do {
> +		val = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_FW_ID);
> +		if (val < 0)
> +			return;
> +		msleep(20);
> +	} while (!val && --retries);

Should not this return 0/-ETIMEDOUT and have the caller propagate that
error code?
-- 
Florian

Re: [PATCH net-next 2/3] net: phy: aquantia: report PHY details like firmware version

[Heiner Kallweit]

On 24.03.2019 02:04, Florian Fainelli wrote:
> 
> 
> On 3/23/2019 6:15 AM, Heiner Kallweit wrote:
>> Add reporting firmware details. These details are available only once
>> the firmware has finished initializing the chip. This can take some
>> time and we need to poll for init completion.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
> 
> [snip]
>>  
>> +/* If we configure settings whilst firmware is still initializing the chip,
>> + * then these settings may be overwritten. Therefore make sure chip
>> + * initialization has completed. Use presence of the firmware ID as
>> + * indicator for initialization having completed.
>> + * The chip also provides a "reset completed" bit, but it's cleared after
>> + * read. Therefore function would time out if called again.
>> + */
> 
> Is there a way to say, run a checksum calculation on the firmware image
> to assess its health/validity as well as read the firmware ID? What
> happens if the PHY is not provisioned with a firmware image? Is it
> expecting for a specific set of MDIO vendor commands to load it over MDIO?
> 
It's also possible to load firmware over MDIO, but typically an internal
boot loader reads the firmware directly from a SPI NOR flash.
Vendor registers allow access to the flash and it should be possible to
expose the flash as MTD, but that's a different exercise (I think some
experimental work has been done in that direction already).
Based on the datasheet I'm not sure whether the PHY can work w/o firmware.
The firmware is more than the provisioned register defaults.


>> +static void aqr107_wait_reset_complete(struct phy_device *phydev)
>> +{
>> +	int val, retries = 100;
>> +
>> +	do {
>> +		val = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_FW_ID);
>> +		if (val < 0)
>> +			return;
>> +		msleep(20);
>> +	} while (!val && --retries);
> 
> Should not this return 0/-ETIMEDOUT and have the caller propagate that
> error code?
> 
Yes, most likely it's better to do this. I'll add it in a v2.

[PATCH net-next 3/3] net: phy: aquantia: inform about proprietary 1000Base-T2 mode being in use

[Heiner Kallweit]

The AQCS109 supports a proprietary 2-pair 1Gbps mode. The standard
registers don't allow to tell between 1000BaseT and 1000BaseT2.
Add reporting this proprietary mode based on a vendor register.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/aquantia_main.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 01a6d5cd4..1013711e1 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -78,6 +78,10 @@
 #define VEND1_GLOBAL_RSVD_STAT1_FW_BUILD_ID	GENMASK(7, 4)
 #define VEND1_GLOBAL_RSVD_STAT1_PROV_ID		GENMASK(3, 0)
 
+#define VEND1_GLOBAL_RSVD_STAT9			0xc88d
+#define VEND1_GLOBAL_RSVD_STAT9_MODE		GENMASK(7, 0)
+#define VEND1_GLOBAL_RSVD_STAT9_1000BT2		0x23
+
 #define VEND1_GLOBAL_INT_STD_STATUS		0xfc00
 #define VEND1_GLOBAL_INT_VEND_STATUS		0xfc01
 
@@ -448,7 +452,7 @@ static void aqr107_link_change_notify(struct phy_device *phydev)
 {
 	u8 fw_major, fw_minor;
 	bool downshift, short_reach, afr;
-	int val;
+	int mode, val;
 
 	if (phydev->state != PHY_RUNNING || phydev->autoneg == AUTONEG_DISABLE)
 		return;
@@ -479,6 +483,14 @@ static void aqr107_link_change_notify(struct phy_device *phydev)
 		   short_reach ? ", short reach mode" : "",
 		   downshift ? ", fast-retrain downshift advertised" : "",
 		   afr ? ", fast reframe advertised" : "");
+
+	val = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_RSVD_STAT9);
+	if (val < 0)
+		return;
+
+	mode = FIELD_GET(VEND1_GLOBAL_RSVD_STAT9_MODE, val);
+	if (mode == VEND1_GLOBAL_RSVD_STAT9_1000BT2)
+		phydev_info(phydev, "Aquantia 1000Base-T2 mode active\n");
 }
 
 static struct phy_driver aqr_driver[] = {
-- 
2.21.0

Re: [PATCH net-next 3/3] net: phy: aquantia: inform about proprietary 1000Base-T2 mode being in use

[Florian Fainelli]

On 3/23/2019 6:16 AM, Heiner Kallweit wrote:
> The AQCS109 supports a proprietary 2-pair 1Gbps mode. The standard
> registers don't allow to tell between 1000BaseT and 1000BaseT2.
> Add reporting this proprietary mode based on a vendor register.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian