diff mbox

[dpdk-dev,v3,2/4] net/e1000: add firmware version get

Message ID 1482841816-54143-3-git-send-email-qiming.yang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel compilation success Compilation OK

Commit Message

Qiming Yang Dec. 27, 2016, 12:30 p.m. UTC 
  This patch adds a new function eth_igb_fw_version_get.

Signed-off-by: Qiming Yang <qiming.yang@intel.com>
---
v3 changes:
 * use eth_igb_fw_version_get(struct rte_eth_dev *dev, u32 *fw_major,
   u32 *fw_minor, u32 *fw_minor, u32 *fw_patch, u32 *etrack_id) instead
   of eth_igb_fw_version_get(struct rte_eth_dev *dev, char *fw_version,
   int fw_length). Add statusment in /doc/guides/nics/features/igb.ini.
---
---
 doc/guides/nics/features/igb.ini |  1 +
 drivers/net/e1000/igb_ethdev.c   | 43 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

Comments

Ferruh Yigit Jan. 3, 2017, 3:02 p.m. UTC | #1 
On 12/27/2016 12:30 PM, Qiming Yang wrote:
> This patch adds a new function eth_igb_fw_version_get.
> 
> Signed-off-by: Qiming Yang <qiming.yang@intel.com>
> ---
> v3 changes:
>  * use eth_igb_fw_version_get(struct rte_eth_dev *dev, u32 *fw_major,
>    u32 *fw_minor, u32 *fw_minor, u32 *fw_patch, u32 *etrack_id) instead
>    of eth_igb_fw_version_get(struct rte_eth_dev *dev, char *fw_version,
>    int fw_length). Add statusment in /doc/guides/nics/features/igb.ini.
> ---
> ---
>  doc/guides/nics/features/igb.ini |  1 +
>  drivers/net/e1000/igb_ethdev.c   | 43 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/doc/guides/nics/features/igb.ini b/doc/guides/nics/features/igb.ini
> index 9fafe72..ffd87ba 100644
> --- a/doc/guides/nics/features/igb.ini
> +++ b/doc/guides/nics/features/igb.ini
> @@ -39,6 +39,7 @@ EEPROM dump          = Y
>  Registers dump       = Y
>  BSD nic_uio          = Y
>  Linux UIO            = Y
> +FW version           = Y

Please keep same location with default.ini file. Why you are putting
this just into middle of the uio and vfio?

>  Linux VFIO           = Y
>  x86-32               = Y
>  x86-64               = Y
> diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
> index 4a15447..25344b7 100644
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -120,6 +120,8 @@ static int eth_igb_xstats_get_names(struct rte_eth_dev *dev,
>  				    unsigned limit);
>  static void eth_igb_stats_reset(struct rte_eth_dev *dev);
>  static void eth_igb_xstats_reset(struct rte_eth_dev *dev);
> +static void eth_igb_fw_version_get(struct rte_eth_dev *dev, u32 *fw_major,
> +		u32 *fw_minor, u32 *fw_patch, u32 *etrack_id);

I think you can use a struct as parameter here. But beware, that struct
should NOT be a public struct.

>  static void eth_igb_infos_get(struct rte_eth_dev *dev,
>  			      struct rte_eth_dev_info *dev_info);
>  static const uint32_t *eth_igb_supported_ptypes_get(struct rte_eth_dev *dev);
> @@ -389,6 +391,7 @@ static const struct eth_dev_ops eth_igb_ops = {
>  	.xstats_get_names     = eth_igb_xstats_get_names,
>  	.stats_reset          = eth_igb_stats_reset,
>  	.xstats_reset         = eth_igb_xstats_reset,
> +	.fw_version_get       = eth_igb_fw_version_get,
>  	.dev_infos_get        = eth_igb_infos_get,
>  	.dev_supported_ptypes_get = eth_igb_supported_ptypes_get,
>  	.mtu_set              = eth_igb_mtu_set,
> @@ -1981,6 +1984,46 @@ eth_igbvf_stats_reset(struct rte_eth_dev *dev)
>  }
>  

<...>
Qiming Yang Jan. 4, 2017, 3:14 a.m. UTC | #2 
See the reply below.

-----Original Message-----
From: Yigit, Ferruh 
Sent: Tuesday, January 3, 2017 11:03 PM
To: Yang, Qiming <qiming.yang@intel.com>; dev@dpdk.org; thomas.monjalon@6wind.com
Cc: Horton, Remy <remy.horton@intel.com>
Subject: Re: [PATCH v3 2/4] net/e1000: add firmware version get

On 12/27/2016 12:30 PM, Qiming Yang wrote:
> This patch adds a new function eth_igb_fw_version_get.
> 
> Signed-off-by: Qiming Yang <qiming.yang@intel.com>
> ---
> v3 changes:
>  * use eth_igb_fw_version_get(struct rte_eth_dev *dev, u32 *fw_major,
>    u32 *fw_minor, u32 *fw_minor, u32 *fw_patch, u32 *etrack_id) instead
>    of eth_igb_fw_version_get(struct rte_eth_dev *dev, char *fw_version,
>    int fw_length). Add statusment in /doc/guides/nics/features/igb.ini.
> ---
> ---
>  doc/guides/nics/features/igb.ini |  1 +
>  drivers/net/e1000/igb_ethdev.c   | 43 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/doc/guides/nics/features/igb.ini 
> b/doc/guides/nics/features/igb.ini
> index 9fafe72..ffd87ba 100644
> --- a/doc/guides/nics/features/igb.ini
> +++ b/doc/guides/nics/features/igb.ini
> @@ -39,6 +39,7 @@ EEPROM dump          = Y
>  Registers dump       = Y
>  BSD nic_uio          = Y
>  Linux UIO            = Y
> +FW version           = Y

Please keep same location with default.ini file. Why you are putting this just into middle of the uio and vfio?
Qiming: It's a clerical error, I want to add this line at the end of this file.

>  Linux VFIO           = Y
>  x86-32               = Y
>  x86-64               = Y
> diff --git a/drivers/net/e1000/igb_ethdev.c 
> b/drivers/net/e1000/igb_ethdev.c index 4a15447..25344b7 100644
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -120,6 +120,8 @@ static int eth_igb_xstats_get_names(struct rte_eth_dev *dev,
>  				    unsigned limit);
>  static void eth_igb_stats_reset(struct rte_eth_dev *dev);  static 
> void eth_igb_xstats_reset(struct rte_eth_dev *dev);
> +static void eth_igb_fw_version_get(struct rte_eth_dev *dev, u32 *fw_major,
> +		u32 *fw_minor, u32 *fw_patch, u32 *etrack_id);

I think you can use a struct as parameter here. But beware, that struct should NOT be a public struct.
Qiming: I think only add a private struct for igb is unnecessary. Keep the arguments consistent with rte_eth_dev_fw_info_get is better.
What do you think?

>  static void eth_igb_infos_get(struct rte_eth_dev *dev,
>  			      struct rte_eth_dev_info *dev_info);  static const uint32_t 
> *eth_igb_supported_ptypes_get(struct rte_eth_dev *dev); @@ -389,6 
> +391,7 @@ static const struct eth_dev_ops eth_igb_ops = {
>  	.xstats_get_names     = eth_igb_xstats_get_names,
>  	.stats_reset          = eth_igb_stats_reset,
>  	.xstats_reset         = eth_igb_xstats_reset,
> +	.fw_version_get       = eth_igb_fw_version_get,
>  	.dev_infos_get        = eth_igb_infos_get,
>  	.dev_supported_ptypes_get = eth_igb_supported_ptypes_get,
>  	.mtu_set              = eth_igb_mtu_set,
> @@ -1981,6 +1984,46 @@ eth_igbvf_stats_reset(struct rte_eth_dev *dev)  
> }
>  

<...>
Ferruh Yigit Jan. 4, 2017, 8:47 a.m. UTC | #3 
On 1/4/2017 3:14 AM, Yang, Qiming wrote:
> See the reply below.
> 
> -----Original Message-----
> From: Yigit, Ferruh 
> Sent: Tuesday, January 3, 2017 11:03 PM
> To: Yang, Qiming <qiming.yang@intel.com>; dev@dpdk.org; thomas.monjalon@6wind.com
> Cc: Horton, Remy <remy.horton@intel.com>
> Subject: Re: [PATCH v3 2/4] net/e1000: add firmware version get
> 
> On 12/27/2016 12:30 PM, Qiming Yang wrote:
>> This patch adds a new function eth_igb_fw_version_get.
>>
>> Signed-off-by: Qiming Yang <qiming.yang@intel.com>
>> ---
>> v3 changes:
>>  * use eth_igb_fw_version_get(struct rte_eth_dev *dev, u32 *fw_major,
>>    u32 *fw_minor, u32 *fw_minor, u32 *fw_patch, u32 *etrack_id) instead
>>    of eth_igb_fw_version_get(struct rte_eth_dev *dev, char *fw_version,
>>    int fw_length). Add statusment in /doc/guides/nics/features/igb.ini.
>> ---
>> ---
>>  doc/guides/nics/features/igb.ini |  1 +
>>  drivers/net/e1000/igb_ethdev.c   | 43 ++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 44 insertions(+)
>>
>> diff --git a/doc/guides/nics/features/igb.ini 
>> b/doc/guides/nics/features/igb.ini
>> index 9fafe72..ffd87ba 100644
>> --- a/doc/guides/nics/features/igb.ini
>> +++ b/doc/guides/nics/features/igb.ini
>> @@ -39,6 +39,7 @@ EEPROM dump          = Y
>>  Registers dump       = Y
>>  BSD nic_uio          = Y
>>  Linux UIO            = Y
>> +FW version           = Y
> 
> Please keep same location with default.ini file. Why you are putting this just into middle of the uio and vfio?
> Qiming: It's a clerical error, I want to add this line at the end of this file.
> 
>>  Linux VFIO           = Y
>>  x86-32               = Y
>>  x86-64               = Y
>> diff --git a/drivers/net/e1000/igb_ethdev.c 
>> b/drivers/net/e1000/igb_ethdev.c index 4a15447..25344b7 100644
>> --- a/drivers/net/e1000/igb_ethdev.c
>> +++ b/drivers/net/e1000/igb_ethdev.c
>> @@ -120,6 +120,8 @@ static int eth_igb_xstats_get_names(struct rte_eth_dev *dev,
>>  				    unsigned limit);
>>  static void eth_igb_stats_reset(struct rte_eth_dev *dev);  static 
>> void eth_igb_xstats_reset(struct rte_eth_dev *dev);
>> +static void eth_igb_fw_version_get(struct rte_eth_dev *dev, u32 *fw_major,
>> +		u32 *fw_minor, u32 *fw_patch, u32 *etrack_id);
> 
> I think you can use a struct as parameter here. But beware, that struct should NOT be a public struct.
> Qiming: I think only add a private struct for igb is unnecessary. Keep the arguments consistent with rte_eth_dev_fw_info_get is better.
> What do you think?

Both are OK.
Normally, I believe using struct is better. But we are not using struct
in public API because of the ABI compatibility issues. Here it is
internal usage, there is no ABI breakage concern, so it may be possible
to use a struct.
But if you prefer to keep the arguments same here with public API, that
is fine.

> 
>>  static void eth_igb_infos_get(struct rte_eth_dev *dev,
>>  			      struct rte_eth_dev_info *dev_info);  static const uint32_t 
>> *eth_igb_supported_ptypes_get(struct rte_eth_dev *dev); @@ -389,6 
>> +391,7 @@ static const struct eth_dev_ops eth_igb_ops = {
>>  	.xstats_get_names     = eth_igb_xstats_get_names,
>>  	.stats_reset          = eth_igb_stats_reset,
>>  	.xstats_reset         = eth_igb_xstats_reset,
>> +	.fw_version_get       = eth_igb_fw_version_get,
>>  	.dev_infos_get        = eth_igb_infos_get,
>>  	.dev_supported_ptypes_get = eth_igb_supported_ptypes_get,
>>  	.mtu_set              = eth_igb_mtu_set,
>> @@ -1981,6 +1984,46 @@ eth_igbvf_stats_reset(struct rte_eth_dev *dev)  
>> }
>>  
> 
> <...>
>
diff mbox

Patch

diff --git a/doc/guides/nics/features/igb.ini b/doc/guides/nics/features/igb.ini
index 9fafe72..ffd87ba 100644
--- a/doc/guides/nics/features/igb.ini
+++ b/doc/guides/nics/features/igb.ini
@@ -39,6 +39,7 @@  EEPROM dump          = Y
 Registers dump       = Y
 BSD nic_uio          = Y
 Linux UIO            = Y
+FW version           = Y
 Linux VFIO           = Y
 x86-32               = Y
 x86-64               = Y
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index 4a15447..25344b7 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -120,6 +120,8 @@  static int eth_igb_xstats_get_names(struct rte_eth_dev *dev,
 				    unsigned limit);
 static void eth_igb_stats_reset(struct rte_eth_dev *dev);
 static void eth_igb_xstats_reset(struct rte_eth_dev *dev);
+static void eth_igb_fw_version_get(struct rte_eth_dev *dev, u32 *fw_major,
+		u32 *fw_minor, u32 *fw_patch, u32 *etrack_id);
 static void eth_igb_infos_get(struct rte_eth_dev *dev,
 			      struct rte_eth_dev_info *dev_info);
 static const uint32_t *eth_igb_supported_ptypes_get(struct rte_eth_dev *dev);
@@ -389,6 +391,7 @@  static const struct eth_dev_ops eth_igb_ops = {
 	.xstats_get_names     = eth_igb_xstats_get_names,
 	.stats_reset          = eth_igb_stats_reset,
 	.xstats_reset         = eth_igb_xstats_reset,
+	.fw_version_get       = eth_igb_fw_version_get,
 	.dev_infos_get        = eth_igb_infos_get,
 	.dev_supported_ptypes_get = eth_igb_supported_ptypes_get,
 	.mtu_set              = eth_igb_mtu_set,
@@ -1981,6 +1984,46 @@  eth_igbvf_stats_reset(struct rte_eth_dev *dev)
 }
 
 static void
+eth_igb_fw_version_get(struct rte_eth_dev *dev, u32 *fw_major, u32 *fw_minor,
+			u32 *fw_patch, u32 *etrack_id)
+{
+	struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct e1000_fw_version fw;
+
+	e1000_get_fw_version(hw, &fw);
+
+	switch (hw->mac.type) {
+	case e1000_i210:
+	case e1000_i211:
+		if (!(e1000_get_flash_presence_i210(hw))) {
+			*fw_major = fw.invm_major;
+			*fw_minor = fw.invm_minor;
+			break;
+		}
+		/* fall through */
+	default:
+		/* if option rom is valid, display its version too*/
+		if (fw.or_valid) {
+			*fw_major = fw.eep_major;
+			*fw_minor = fw.eep_minor;
+			*etrack_id = fw.etrack_id;
+			*fw_patch = fw.or_patch;
+		/* no option rom */
+		} else {
+			if (fw.etrack_id != 0X0000) {
+			*fw_major = fw.eep_major;
+			*fw_minor = fw.eep_minor;
+			*etrack_id = fw.etrack_id;
+			} else {
+			*fw_major = fw.eep_major;
+			*fw_minor = fw.eep_minor;
+			}
+		}
+		break;
+	}
+}
+
+static void
 eth_igb_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 {
 	struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);