diff mbox

[dpdk-dev,v2] nfp: report link speed using hardware info

Message ID 1479485218-11931-1-git-send-email-alejandro.lucero@netronome.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
checkpatch/checkpatch success coding style OK

Commit Message

Alejandro Lucero Nov. 18, 2016, 4:06 p.m. UTC 
  Previous reported speed was hardcoded.

Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
 drivers/net/nfp/nfp_net.c      | 28 ++++++++++++++++++++++++++--
 drivers/net/nfp/nfp_net_ctrl.h | 13 +++++++++++++
 2 files changed, 39 insertions(+), 2 deletions(-)

Comments

Thomas Monjalon Nov. 18, 2016, 4:29 p.m. UTC | #1 
2016-11-18 16:06, Alejandro Lucero:
> Previous reported speed was hardcoded.
> 
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> ---
>  drivers/net/nfp/nfp_net.c      | 28 ++++++++++++++++++++++++++--
>  drivers/net/nfp/nfp_net_ctrl.h | 13 +++++++++++++
>  2 files changed, 39 insertions(+), 2 deletions(-)

You should update the doc in the same patch:
	doc/guides/nics/features/nfp.ini
It will be the first feature as the file appears to be empty.
So you will need another patch to fill other existing features.

I have an unrelated question: why nfp is disabled in the default build?
Alejandro Lucero Nov. 18, 2016, 4:50 p.m. UTC | #2 
On Fri, Nov 18, 2016 at 4:29 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> 2016-11-18 16:06, Alejandro Lucero:
> > Previous reported speed was hardcoded.
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > ---
> >  drivers/net/nfp/nfp_net.c      | 28 ++++++++++++++++++++++++++--
> >  drivers/net/nfp/nfp_net_ctrl.h | 13 +++++++++++++
> >  2 files changed, 39 insertions(+), 2 deletions(-)
>
> You should update the doc in the same patch:
>         doc/guides/nics/features/nfp.ini
> It will be the first feature as the file appears to be empty.
> So you will need another patch to fill other existing features.
>

Yes. I'm just working on updating that file properly.
May I delay this doc change for including it with that other one?
It will be a bit weird to just have one feature there.


>
> I have an unrelated question: why nfp is disabled in the default build?
>
>
Because NFP PMD can just work if Netronome BSP is installed in the system.
We do not support PF with the PMD, so it requires a Linux PF driver which
comes with the BSP.

The compilation has no dependencies, but we had our own UIO driver before
(now using igb_uio). So basically, we wanted the people aware of this
dependency and to specifically configure this option.

I know what you are surely going to ask about DPDK in Linux distributions,
and that this being a bad idea. The fact is, we have people using NFP PMD
as part of a product, so installing that product implies to (automatically)
install the BSP and a specific DPDK version with the NFP PMD enabled. But
yes, maybe we should modify this and to add some sort of BSP check inside
the PMD.

So, thanks for the heads up. I will think about this.
Thomas Monjalon Nov. 21, 2016, 9:22 a.m. UTC | #3 
2016-11-18 16:50, Alejandro Lucero:
> On Fri, Nov 18, 2016 at 4:29 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
> wrote:
> > You should update the doc in the same patch:
> >         doc/guides/nics/features/nfp.ini
> > It will be the first feature as the file appears to be empty.
> > So you will need another patch to fill other existing features.
> >
> 
> Yes. I'm just working on updating that file properly.
> May I delay this doc change for including it with that other one?
> It will be a bit weird to just have one feature there.

It will provide you a good motivation to fix the doc :)
Up to you
Ferruh Yigit Nov. 21, 2016, 10:57 a.m. UTC | #4 
On 11/18/2016 4:50 PM, Alejandro Lucero wrote:
> On Fri, Nov 18, 2016 at 4:29 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
> wrote:
> 
>> 2016-11-18 16:06, Alejandro Lucero:
>>> Previous reported speed was hardcoded.
>>>
>>> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
>>> ---
>>>  drivers/net/nfp/nfp_net.c      | 28 ++++++++++++++++++++++++++--
>>>  drivers/net/nfp/nfp_net_ctrl.h | 13 +++++++++++++
>>>  2 files changed, 39 insertions(+), 2 deletions(-)
>>
>> You should update the doc in the same patch:
>>         doc/guides/nics/features/nfp.ini
>> It will be the first feature as the file appears to be empty.
>> So you will need another patch to fill other existing features.
>>
> 
> Yes. I'm just working on updating that file properly.
> May I delay this doc change for including it with that other one?
> It will be a bit weird to just have one feature there.

I think yes, but can you please send the documentation patch before
integration deadline of this release?

> 
> 
>>
>> I have an unrelated question: why nfp is disabled in the default build?
>>
>>
> Because NFP PMD can just work if Netronome BSP is installed in the system.

Is this BSP open source and freely available? If so, can you update the
document with details (how/where to get it) please?

> We do not support PF with the PMD, so it requires a Linux PF driver which
> comes with the BSP.
> 
> The compilation has no dependencies, but we had our own UIO driver before
> (now using igb_uio). So basically, we wanted the people aware of this
> dependency and to specifically configure this option.
> 
> I know what you are surely going to ask about DPDK in Linux distributions,
> and that this being a bad idea. The fact is, we have people using NFP PMD
> as part of a product, so installing that product implies to (automatically)
> install the BSP and a specific DPDK version with the NFP PMD enabled. But
> yes, maybe we should modify this and to add some sort of BSP check inside
> the PMD.

Is there a DPDK version <-> BSP version dependency?

> 
> So, thanks for the heads up. I will think about this.
>
Ferruh Yigit Nov. 21, 2016, 11:18 a.m. UTC | #5 
On 11/18/2016 4:06 PM, Alejandro Lucero wrote:
> Previous reported speed was hardcoded.
> 
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> ---
>  drivers/net/nfp/nfp_net.c      | 28 ++++++++++++++++++++++++++--
>  drivers/net/nfp/nfp_net_ctrl.h | 13 +++++++++++++
>  2 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> index c6b1587..24f3164 100644
> --- a/drivers/net/nfp/nfp_net.c
> +++ b/drivers/net/nfp/nfp_net.c
> @@ -816,6 +816,17 @@ static void nfp_net_read_mac(struct nfp_net_hw *hw)
>  	struct rte_eth_link link, old;
>  	uint32_t nn_link_status;
>  
> +	static const uint32_t ls_to_ethtool[] = {
> +		[NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] = ETH_SPEED_NUM_NONE,
> +		[NFP_NET_CFG_STS_LINK_RATE_UNKNOWN]     = ETH_SPEED_NUM_NONE,
> +		[NFP_NET_CFG_STS_LINK_RATE_1G]          = ETH_SPEED_NUM_1G,
> +		[NFP_NET_CFG_STS_LINK_RATE_10G]         = ETH_SPEED_NUM_10G,
> +		[NFP_NET_CFG_STS_LINK_RATE_25G]         = ETH_SPEED_NUM_25G,
> +		[NFP_NET_CFG_STS_LINK_RATE_40G]         = ETH_SPEED_NUM_40G,
> +		[NFP_NET_CFG_STS_LINK_RATE_50G]         = ETH_SPEED_NUM_50G,
> +		[NFP_NET_CFG_STS_LINK_RATE_100G]        = ETH_SPEED_NUM_100G,
> +	};
> +
>  	PMD_DRV_LOG(DEBUG, "Link update\n");
>  
>  	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> @@ -831,8 +842,21 @@ static void nfp_net_read_mac(struct nfp_net_hw *hw)
>  		link.link_status = ETH_LINK_UP;
>  
>  	link.link_duplex = ETH_LINK_FULL_DUPLEX;
> -	/* Other cards can limit the tx and rx rate per VF */
> -	link.link_speed = ETH_SPEED_NUM_40G;
> +
> +	nn_link_status = (nn_link_status >> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
> +			 NFP_NET_CFG_STS_LINK_RATE_MASK;
> +
> +	if ((NFD_CFG_MAJOR_VERSION_of(hw->ver) < 4) ||
> +	    ((NFD_CFG_MINOR_VERSION_of(hw->ver) == 4) &&
> +	    (NFD_CFG_MINOR_VERSION_of(hw->ver) == 0)))
> +		link.link_speed = ETH_SPEED_NUM_40G;

For specific firmware version, speed is still hardcoded to 40G, can you
please mention from this and if possible its reason in commit log?

> +	else {
> +		if (nn_link_status == NFP_NET_CFG_STS_LINK_RATE_UNKNOWN ||

This check can be redundant, since
ls_to_ethtool[NFP_NET_CFG_STS_LINK_RATE_UNKNOWN] => ETH_SPEED_NUM_NONE

> +		    nn_link_status >= RTE_DIM(ls_to_ethtool))
> +			link.link_speed = ETH_SPEED_NUM_NONE;
> +		else
> +			link.link_speed = ls_to_ethtool[nn_link_status];
> +	}
>  
>  	if (old.link_status != link.link_status) {
>  		nfp_net_dev_atomic_write_link_status(dev, &link);
> diff --git a/drivers/net/nfp/nfp_net_ctrl.h b/drivers/net/nfp/nfp_net_ctrl.h
> index fce8251..f9aaba3 100644
> --- a/drivers/net/nfp/nfp_net_ctrl.h
> +++ b/drivers/net/nfp/nfp_net_ctrl.h
> @@ -157,6 +157,19 @@
>  #define   NFP_NET_CFG_VERSION_MINOR(x)    (((x) & 0xff) <<  0)
>  #define NFP_NET_CFG_STS                 0x0034
>  #define   NFP_NET_CFG_STS_LINK            (0x1 << 0) /* Link up or down */
> +/* Link rate */
> +#define   NFP_NET_CFG_STS_LINK_RATE_SHIFT 1
> +#define   NFP_NET_CFG_STS_LINK_RATE_MASK  0xF
> +#define   NFP_NET_CFG_STS_LINK_RATE       \
> +	  (NFP_NET_CFG_STS_LINK_RATE_MASK << NFP_NET_CFG_STS_LINK_RATE_SHIFT)

This macro is not used at all, just fyi.

> +#define   NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED   0
> +#define   NFP_NET_CFG_STS_LINK_RATE_UNKNOWN       1
> +#define   NFP_NET_CFG_STS_LINK_RATE_1G            2
> +#define   NFP_NET_CFG_STS_LINK_RATE_10G           3
> +#define   NFP_NET_CFG_STS_LINK_RATE_25G           4
> +#define   NFP_NET_CFG_STS_LINK_RATE_40G           5
> +#define   NFP_NET_CFG_STS_LINK_RATE_50G           6
> +#define   NFP_NET_CFG_STS_LINK_RATE_100G          7
>  #define NFP_NET_CFG_CAP                 0x0038
>  #define NFP_NET_CFG_MAX_TXRINGS         0x003c
>  #define NFP_NET_CFG_MAX_RXRINGS         0x0040
>
Alejandro Lucero Nov. 21, 2016, 2:47 p.m. UTC | #6 
On Mon, Nov 21, 2016 at 11:18 AM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 11/18/2016 4:06 PM, Alejandro Lucero wrote:
> > Previous reported speed was hardcoded.
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > ---
> >  drivers/net/nfp/nfp_net.c      | 28 ++++++++++++++++++++++++++--
> >  drivers/net/nfp/nfp_net_ctrl.h | 13 +++++++++++++
> >  2 files changed, 39 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> > index c6b1587..24f3164 100644
> > --- a/drivers/net/nfp/nfp_net.c
> > +++ b/drivers/net/nfp/nfp_net.c
> > @@ -816,6 +816,17 @@ static void nfp_net_read_mac(struct nfp_net_hw *hw)
> >       struct rte_eth_link link, old;
> >       uint32_t nn_link_status;
> >
> > +     static const uint32_t ls_to_ethtool[] = {
> > +             [NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] =
> ETH_SPEED_NUM_NONE,
> > +             [NFP_NET_CFG_STS_LINK_RATE_UNKNOWN]     =
> ETH_SPEED_NUM_NONE,
> > +             [NFP_NET_CFG_STS_LINK_RATE_1G]          = ETH_SPEED_NUM_1G,
> > +             [NFP_NET_CFG_STS_LINK_RATE_10G]         =
> ETH_SPEED_NUM_10G,
> > +             [NFP_NET_CFG_STS_LINK_RATE_25G]         =
> ETH_SPEED_NUM_25G,
> > +             [NFP_NET_CFG_STS_LINK_RATE_40G]         =
> ETH_SPEED_NUM_40G,
> > +             [NFP_NET_CFG_STS_LINK_RATE_50G]         =
> ETH_SPEED_NUM_50G,
> > +             [NFP_NET_CFG_STS_LINK_RATE_100G]        =
> ETH_SPEED_NUM_100G,
> > +     };
> > +
> >       PMD_DRV_LOG(DEBUG, "Link update\n");
> >
> >       hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > @@ -831,8 +842,21 @@ static void nfp_net_read_mac(struct nfp_net_hw *hw)
> >               link.link_status = ETH_LINK_UP;
> >
> >       link.link_duplex = ETH_LINK_FULL_DUPLEX;
> > -     /* Other cards can limit the tx and rx rate per VF */
> > -     link.link_speed = ETH_SPEED_NUM_40G;
> > +
> > +     nn_link_status = (nn_link_status >> NFP_NET_CFG_STS_LINK_RATE_SHIFT)
> &
> > +                      NFP_NET_CFG_STS_LINK_RATE_MASK;
> > +
> > +     if ((NFD_CFG_MAJOR_VERSION_of(hw->ver) < 4) ||
> > +         ((NFD_CFG_MINOR_VERSION_of(hw->ver) == 4) &&
> > +         (NFD_CFG_MINOR_VERSION_of(hw->ver) == 0)))
> > +             link.link_speed = ETH_SPEED_NUM_40G;
>
> For specific firmware version, speed is still hardcoded to 40G, can you
> please mention from this and if possible its reason in commit log?
>
> > +     else {
> > +             if (nn_link_status == NFP_NET_CFG_STS_LINK_RATE_UNKNOWN ||
>
> This check can be redundant, since
> ls_to_ethtool[NFP_NET_CFG_STS_LINK_RATE_UNKNOWN] => ETH_SPEED_NUM_NONE
>
>
This is for checking any wrong value from firmware/hardware.


> > +                 nn_link_status >= RTE_DIM(ls_to_ethtool))
> > +                     link.link_speed = ETH_SPEED_NUM_NONE;
> > +             else
> > +                     link.link_speed = ls_to_ethtool[nn_link_status];
> > +     }
> >
> >       if (old.link_status != link.link_status) {
> >               nfp_net_dev_atomic_write_link_status(dev, &link);
> > diff --git a/drivers/net/nfp/nfp_net_ctrl.h b/drivers/net/nfp/nfp_net_
> ctrl.h
> > index fce8251..f9aaba3 100644
> > --- a/drivers/net/nfp/nfp_net_ctrl.h
> > +++ b/drivers/net/nfp/nfp_net_ctrl.h
> > @@ -157,6 +157,19 @@
> >  #define   NFP_NET_CFG_VERSION_MINOR(x)    (((x) & 0xff) <<  0)
> >  #define NFP_NET_CFG_STS                 0x0034
> >  #define   NFP_NET_CFG_STS_LINK            (0x1 << 0) /* Link up or down
> */
> > +/* Link rate */
> > +#define   NFP_NET_CFG_STS_LINK_RATE_SHIFT 1
> > +#define   NFP_NET_CFG_STS_LINK_RATE_MASK  0xF
> > +#define   NFP_NET_CFG_STS_LINK_RATE       \
> > +       (NFP_NET_CFG_STS_LINK_RATE_MASK << NFP_NET_CFG_STS_LINK_RATE_
> SHIFT)
>
> This macro is not used at all, just fyi.
>
>
Thanks. I think I can remove it.


> > +#define   NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED   0
> > +#define   NFP_NET_CFG_STS_LINK_RATE_UNKNOWN       1
> > +#define   NFP_NET_CFG_STS_LINK_RATE_1G            2
> > +#define   NFP_NET_CFG_STS_LINK_RATE_10G           3
> > +#define   NFP_NET_CFG_STS_LINK_RATE_25G           4
> > +#define   NFP_NET_CFG_STS_LINK_RATE_40G           5
> > +#define   NFP_NET_CFG_STS_LINK_RATE_50G           6
> > +#define   NFP_NET_CFG_STS_LINK_RATE_100G          7
> >  #define NFP_NET_CFG_CAP                 0x0038
> >  #define NFP_NET_CFG_MAX_TXRINGS         0x003c
> >  #define NFP_NET_CFG_MAX_RXRINGS         0x0040
> >
>
>
diff mbox

Patch

diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index c6b1587..24f3164 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -816,6 +816,17 @@  static void nfp_net_read_mac(struct nfp_net_hw *hw)
 	struct rte_eth_link link, old;
 	uint32_t nn_link_status;
 
+	static const uint32_t ls_to_ethtool[] = {
+		[NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] = ETH_SPEED_NUM_NONE,
+		[NFP_NET_CFG_STS_LINK_RATE_UNKNOWN]     = ETH_SPEED_NUM_NONE,
+		[NFP_NET_CFG_STS_LINK_RATE_1G]          = ETH_SPEED_NUM_1G,
+		[NFP_NET_CFG_STS_LINK_RATE_10G]         = ETH_SPEED_NUM_10G,
+		[NFP_NET_CFG_STS_LINK_RATE_25G]         = ETH_SPEED_NUM_25G,
+		[NFP_NET_CFG_STS_LINK_RATE_40G]         = ETH_SPEED_NUM_40G,
+		[NFP_NET_CFG_STS_LINK_RATE_50G]         = ETH_SPEED_NUM_50G,
+		[NFP_NET_CFG_STS_LINK_RATE_100G]        = ETH_SPEED_NUM_100G,
+	};
+
 	PMD_DRV_LOG(DEBUG, "Link update\n");
 
 	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
@@ -831,8 +842,21 @@  static void nfp_net_read_mac(struct nfp_net_hw *hw)
 		link.link_status = ETH_LINK_UP;
 
 	link.link_duplex = ETH_LINK_FULL_DUPLEX;
-	/* Other cards can limit the tx and rx rate per VF */
-	link.link_speed = ETH_SPEED_NUM_40G;
+
+	nn_link_status = (nn_link_status >> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
+			 NFP_NET_CFG_STS_LINK_RATE_MASK;
+
+	if ((NFD_CFG_MAJOR_VERSION_of(hw->ver) < 4) ||
+	    ((NFD_CFG_MINOR_VERSION_of(hw->ver) == 4) &&
+	    (NFD_CFG_MINOR_VERSION_of(hw->ver) == 0)))
+		link.link_speed = ETH_SPEED_NUM_40G;
+	else {
+		if (nn_link_status == NFP_NET_CFG_STS_LINK_RATE_UNKNOWN ||
+		    nn_link_status >= RTE_DIM(ls_to_ethtool))
+			link.link_speed = ETH_SPEED_NUM_NONE;
+		else
+			link.link_speed = ls_to_ethtool[nn_link_status];
+	}
 
 	if (old.link_status != link.link_status) {
 		nfp_net_dev_atomic_write_link_status(dev, &link);
diff --git a/drivers/net/nfp/nfp_net_ctrl.h b/drivers/net/nfp/nfp_net_ctrl.h
index fce8251..f9aaba3 100644
--- a/drivers/net/nfp/nfp_net_ctrl.h
+++ b/drivers/net/nfp/nfp_net_ctrl.h
@@ -157,6 +157,19 @@ 
 #define   NFP_NET_CFG_VERSION_MINOR(x)    (((x) & 0xff) <<  0)
 #define NFP_NET_CFG_STS                 0x0034
 #define   NFP_NET_CFG_STS_LINK            (0x1 << 0) /* Link up or down */
+/* Link rate */
+#define   NFP_NET_CFG_STS_LINK_RATE_SHIFT 1
+#define   NFP_NET_CFG_STS_LINK_RATE_MASK  0xF
+#define   NFP_NET_CFG_STS_LINK_RATE       \
+	  (NFP_NET_CFG_STS_LINK_RATE_MASK << NFP_NET_CFG_STS_LINK_RATE_SHIFT)
+#define   NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED   0
+#define   NFP_NET_CFG_STS_LINK_RATE_UNKNOWN       1
+#define   NFP_NET_CFG_STS_LINK_RATE_1G            2
+#define   NFP_NET_CFG_STS_LINK_RATE_10G           3
+#define   NFP_NET_CFG_STS_LINK_RATE_25G           4
+#define   NFP_NET_CFG_STS_LINK_RATE_40G           5
+#define   NFP_NET_CFG_STS_LINK_RATE_50G           6
+#define   NFP_NET_CFG_STS_LINK_RATE_100G          7
 #define NFP_NET_CFG_CAP                 0x0038
 #define NFP_NET_CFG_MAX_TXRINGS         0x003c
 #define NFP_NET_CFG_MAX_RXRINGS         0x0040