[net-next,2/3] net: phy: aquantia: report PHY details like firmware version
diff mbox series

Message ID ba786642-58dc-2db6-9033-0d95f38acebe@gmail.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • net: phy: aquantia: report Aquantia-specific settings and features
Related show

Commit Message

Heiner Kallweit March 23, 2019, 1:15 p.m. UTC
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(+)

Comments

Florian Fainelli March 24, 2019, 1:04 a.m. UTC | #1
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?
Heiner Kallweit March 24, 2019, 9:42 a.m. UTC | #2
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
diff mbox series

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);
 }