[PATCH] nfp: extend speed capabilities advertised

[PATCH] nfp: extend speed capabilities advertised

[Alejandro Lucero]

NFP supports more speeds than just 40 and 100GB, which were
what was advertised before.

Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
 drivers/net/nfp/nfp_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index 27afbfd..77015c4 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -1077,7 +1077,9 @@ static void nfp_net_read_mac(struct nfp_net_hw *hw)
 	dev_info->reta_size = NFP_NET_CFG_RSS_ITBL_SZ;
 	dev_info->hash_key_size = NFP_NET_CFG_RSS_KEY_SZ;
 
-	dev_info->speed_capa = ETH_LINK_SPEED_40G | ETH_LINK_SPEED_100G;
+	dev_info->speed_capa = ETH_SPEED_NUM_1G | ETH_LINK_SPEED_10G |
+			       ETH_SPEED_NUM_25G | ETH_SPEED_NUM_40G |
+			       ETH_SPEED_NUM_50G | ETH_LINK_SPEED_100G;
 }
 
 static const uint32_t *
-- 
1.9.1

Re: [PATCH] nfp: extend speed capabilities advertised

[Ferruh Yigit]

Hi Alejandro,

On 12/19/2016 12:05 PM, Alejandro Lucero wrote:
> NFP supports more speeds than just 40 and 100GB, which were
> what was advertised before.
> 
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> ---
>  drivers/net/nfp/nfp_net.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> index 27afbfd..77015c4 100644
> --- a/drivers/net/nfp/nfp_net.c
> +++ b/drivers/net/nfp/nfp_net.c
> @@ -1077,7 +1077,9 @@ static void nfp_net_read_mac(struct nfp_net_hw *hw)
>  	dev_info->reta_size = NFP_NET_CFG_RSS_ITBL_SZ;
>  	dev_info->hash_key_size = NFP_NET_CFG_RSS_KEY_SZ;
>  
> -	dev_info->speed_capa = ETH_LINK_SPEED_40G | ETH_LINK_SPEED_100G;
> +	dev_info->speed_capa = ETH_SPEED_NUM_1G | ETH_LINK_SPEED_10G |
> +			       ETH_SPEED_NUM_25G | ETH_SPEED_NUM_40G |
> +			       ETH_SPEED_NUM_50G | ETH_LINK_SPEED_100G;

Does all devices driver by this driver supports all these speeds?

I am aware of at least one exception to this, from previous patch [1],
should we take that into account?

Also other than that exception, can you please confirm all other devices
support all above speeds?

[1]
+	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;


>  }
>  
>  static const uint32_t *
> 

Re: [PATCH] nfp: extend speed capabilities advertised

[Alejandro Lucero]

On Mon, Dec 19, 2016 at 2:36 PM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> Hi Alejandro,
>
>
Hi,


> On 12/19/2016 12:05 PM, Alejandro Lucero wrote:
> > NFP supports more speeds than just 40 and 100GB, which were
> > what was advertised before.
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > ---
> >  drivers/net/nfp/nfp_net.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> > index 27afbfd..77015c4 100644
> > --- a/drivers/net/nfp/nfp_net.c
> > +++ b/drivers/net/nfp/nfp_net.c
> > @@ -1077,7 +1077,9 @@ static void nfp_net_read_mac(struct nfp_net_hw *hw)
> >       dev_info->reta_size = NFP_NET_CFG_RSS_ITBL_SZ;
> >       dev_info->hash_key_size = NFP_NET_CFG_RSS_KEY_SZ;
> >
> > -     dev_info->speed_capa = ETH_LINK_SPEED_40G | ETH_LINK_SPEED_100G;
> > +     dev_info->speed_capa = ETH_SPEED_NUM_1G | ETH_LINK_SPEED_10G |
> > +                            ETH_SPEED_NUM_25G | ETH_SPEED_NUM_40G |
> > +                            ETH_SPEED_NUM_50G | ETH_LINK_SPEED_100G;
>
> Does all devices driver by this driver supports all these speeds?
>
> I am aware of at least one exception to this, from previous patch [1],
> should we take that into account?
>
>
So we have different NFP devices and different firmwares.
NFP by design support all those speeds, but the PMD relies on the firmware
for being able to know which is the current configured speed after link
negotiation. PMD development was done with a specific firmware, and I was
told to just report such speed by default. Last firmware versions give that
speed info, but old firmware versions do not.

So, all devices support such a speed range, indeed PMD works with any of
them, but reported speed is always 40G with old firmware. This is a
firmware limitation but we have to support old and new firmware.



> Also other than that exception, can you please confirm all other devices
> support all above speeds?
>
> [1]
> +       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;
>
>
> >  }
> >
> >  static const uint32_t *
> >
>
>

Re: [PATCH] nfp: extend speed capabilities advertised

[Ferruh Yigit]

On 12/19/2016 3:02 PM, Alejandro Lucero wrote:
> 
> 
> On Mon, Dec 19, 2016 at 2:36 PM, Ferruh Yigit <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     Hi Alejandro,
> 
> 
> Hi,
>  
> 
>     On 12/19/2016 12:05 PM, Alejandro Lucero wrote:
>     > NFP supports more speeds than just 40 and 100GB, which were
>     > what was advertised before.
>     >
>     > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com <mailto:alejandro.lucero@netronome.com>>
>     > ---
>     >  drivers/net/nfp/nfp_net.c | 4 +++-
>     >  1 file changed, 3 insertions(+), 1 deletion(-)
>     >
>     > diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
>     > index 27afbfd..77015c4 100644
>     > --- a/drivers/net/nfp/nfp_net.c
>     > +++ b/drivers/net/nfp/nfp_net.c
>     > @@ -1077,7 +1077,9 @@ static void nfp_net_read_mac(struct nfp_net_hw *hw)
>     >       dev_info->reta_size = NFP_NET_CFG_RSS_ITBL_SZ;
>     >       dev_info->hash_key_size = NFP_NET_CFG_RSS_KEY_SZ;
>     >
>     > -     dev_info->speed_capa = ETH_LINK_SPEED_40G | ETH_LINK_SPEED_100G;
>     > +     dev_info->speed_capa = ETH_SPEED_NUM_1G | ETH_LINK_SPEED_10G |
>     > +                            ETH_SPEED_NUM_25G | ETH_SPEED_NUM_40G |
>     > +                            ETH_SPEED_NUM_50G | ETH_LINK_SPEED_100G;
> 
>     Does all devices driver by this driver supports all these speeds?
> 
>     I am aware of at least one exception to this, from previous patch [1],
>     should we take that into account?
> 
> 
> So we have different NFP devices and different firmwares. 
> NFP by design support all those speeds, but the PMD relies on the
> firmware for being able to know which is the current configured speed
> after link negotiation. PMD development was done with a specific
> firmware, and I was told to just report such speed by default. Last
> firmware versions give that speed info, but old firmware versions do not. 
> 
> So, all devices support such a speed range, indeed PMD works with any of
> them, but reported speed is always 40G with old firmware. This is a
> firmware limitation but we have to support old and new firmware.

But this information to the application will be wrong for some (old) FW.
What do you think checking the FW version here and report capability
based on what FW supports?

> 
>  
> 
>     Also other than that exception, can you please confirm all other devices
>     support all above speeds?
> 
>     [1]
>     +       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;
> 
> 
>     >  }
>     >
>     >  static const uint32_t *
>     >
> 
> 

Re: [PATCH] nfp: extend speed capabilities advertised

[Alejandro Lucero]

On Mon, Dec 19, 2016 at 3:05 PM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 12/19/2016 3:02 PM, Alejandro Lucero wrote:
> >
> >
> > On Mon, Dec 19, 2016 at 2:36 PM, Ferruh Yigit <ferruh.yigit@intel.com
> > <mailto:ferruh.yigit@intel.com>> wrote:
> >
> >     Hi Alejandro,
> >
> >
> > Hi,
> >
> >
> >     On 12/19/2016 12:05 PM, Alejandro Lucero wrote:
> >     > NFP supports more speeds than just 40 and 100GB, which were
> >     > what was advertised before.
> >     >
> >     > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com
> <mailto:alejandro.lucero@netronome.com>>
> >     > ---
> >     >  drivers/net/nfp/nfp_net.c | 4 +++-
> >     >  1 file changed, 3 insertions(+), 1 deletion(-)
> >     >
> >     > diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> >     > index 27afbfd..77015c4 100644
> >     > --- a/drivers/net/nfp/nfp_net.c
> >     > +++ b/drivers/net/nfp/nfp_net.c
> >     > @@ -1077,7 +1077,9 @@ static void nfp_net_read_mac(struct
> nfp_net_hw *hw)
> >     >       dev_info->reta_size = NFP_NET_CFG_RSS_ITBL_SZ;
> >     >       dev_info->hash_key_size = NFP_NET_CFG_RSS_KEY_SZ;
> >     >
> >     > -     dev_info->speed_capa = ETH_LINK_SPEED_40G |
> ETH_LINK_SPEED_100G;
> >     > +     dev_info->speed_capa = ETH_SPEED_NUM_1G | ETH_LINK_SPEED_10G
> |
> >     > +                            ETH_SPEED_NUM_25G | ETH_SPEED_NUM_40G
> |
> >     > +                            ETH_SPEED_NUM_50G |
> ETH_LINK_SPEED_100G;
> >
> >     Does all devices driver by this driver supports all these speeds?
> >
> >     I am aware of at least one exception to this, from previous patch
> [1],
> >     should we take that into account?
> >
> >
> > So we have different NFP devices and different firmwares.
> > NFP by design support all those speeds, but the PMD relies on the
> > firmware for being able to know which is the current configured speed
> > after link negotiation. PMD development was done with a specific
> > firmware, and I was told to just report such speed by default. Last
> > firmware versions give that speed info, but old firmware versions do not.
> >
> > So, all devices support such a speed range, indeed PMD works with any of
> > them, but reported speed is always 40G with old firmware. This is a
> > firmware limitation but we have to support old and new firmware.
>
> But this information to the application will be wrong for some (old) FW.
> What do you think checking the FW version here and report capability
> based on what FW supports?
>
>
The driver advertises the right speed range supported. The problem is with
the report about the current link speed configured.
Maybe, is the right thing to do here to not report the current link speed
because the driver really does not know about it?

If you agree with this, I'm afraid the just accepted patch about the link
report needs to be modified.



> >
> >
> >
> >     Also other than that exception, can you please confirm all other
> devices
> >     support all above speeds?
> >
> >     [1]
> >     +       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;
> >
> >
> >     >  }
> >     >
> >     >  static const uint32_t *
> >     >
> >
> >
>
>

Re: [PATCH] nfp: extend speed capabilities advertised

[Marc]

On 19 December 2016 at 17:18, Alejandro Lucero <
alejandro.lucero@netronome.com> wrote:

> On Mon, Dec 19, 2016 at 3:05 PM, Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
>
> > On 12/19/2016 3:02 PM, Alejandro Lucero wrote:
> > >
> > >
> > > On Mon, Dec 19, 2016 at 2:36 PM, Ferruh Yigit <ferruh.yigit@intel.com
> > > <mailto:ferruh.yigit@intel.com>> wrote:
> > >
> > >     Hi Alejandro,
> > >
> > >
> > > Hi,
> > >
> > >
> > >     On 12/19/2016 12:05 PM, Alejandro Lucero wrote:
> > >     > NFP supports more speeds than just 40 and 100GB, which were
> > >     > what was advertised before.
> > >     >
> > >     > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com
> > <mailto:alejandro.lucero@netronome.com>>
> > >     > ---
> > >     >  drivers/net/nfp/nfp_net.c | 4 +++-
> > >     >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >     >
> > >     > diff --git a/drivers/net/nfp/nfp_net.c
> b/drivers/net/nfp/nfp_net.c
> > >     > index 27afbfd..77015c4 100644
> > >     > --- a/drivers/net/nfp/nfp_net.c
> > >     > +++ b/drivers/net/nfp/nfp_net.c
> > >     > @@ -1077,7 +1077,9 @@ static void nfp_net_read_mac(struct
> > nfp_net_hw *hw)
> > >     >       dev_info->reta_size = NFP_NET_CFG_RSS_ITBL_SZ;
> > >     >       dev_info->hash_key_size = NFP_NET_CFG_RSS_KEY_SZ;
> > >     >
> > >     > -     dev_info->speed_capa = ETH_LINK_SPEED_40G |
> > ETH_LINK_SPEED_100G;
> > >     > +     dev_info->speed_capa = ETH_SPEED_NUM_1G |
> ETH_LINK_SPEED_10G
> > |
> > >     > +                            ETH_SPEED_NUM_25G |
> ETH_SPEED_NUM_40G
> > |
> > >     > +                            ETH_SPEED_NUM_50G |
> > ETH_LINK_SPEED_100G;
> > >
> > >     Does all devices driver by this driver supports all these speeds?
> > >
> > >     I am aware of at least one exception to this, from previous patch
> > [1],
> > >     should we take that into account?
> > >
> > >
> > > So we have different NFP devices and different firmwares.
> > > NFP by design support all those speeds, but the PMD relies on the
> > > firmware for being able to know which is the current configured speed
> > > after link negotiation. PMD development was done with a specific
> > > firmware, and I was told to just report such speed by default. Last
> > > firmware versions give that speed info, but old firmware versions do
> not.
> > >
> > > So, all devices support such a speed range, indeed PMD works with any
> of
> > > them, but reported speed is always 40G with old firmware. This is a
> > > firmware limitation but we have to support old and new firmware.
> >
> > But this information to the application will be wrong for some (old) FW.
> > What do you think checking the FW version here and report capability
> > based on what FW supports?
> >
> >
> The driver advertises the right speed range supported. The problem is with
> the report about the current link speed configured.
> Maybe, is the right thing to do here to not report the current link speed
> because the driver really does not know about it?
>
> If you agree with this, I'm afraid the just accepted patch about the link
> report needs to be modified.
>

Alejandro,

If negociated link state has to be changed, then struct rte_eth_dev_data
dev_link field is where to do it.

As Ferruh was saying, dev_info->speed_capa contains the speed capabilties
of the particular NIC in use, not the driver (detecting firmware version
would be the best here).

marc


>
>
>
> > >
> > >
> > >
> > >     Also other than that exception, can you please confirm all other
> > devices
> > >     support all above speeds?
> > >
> > >     [1]
> > >     +       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;
> > >
> > >
> > >     >  }
> > >     >
> > >     >  static const uint32_t *
> > >     >
> > >
> > >
> >
> >
>

Re: [PATCH] nfp: extend speed capabilities advertised

[Ferruh Yigit]

On 12/19/2016 4:18 PM, Alejandro Lucero wrote:
> On Mon, Dec 19, 2016 at 3:05 PM, Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
> 
>> On 12/19/2016 3:02 PM, Alejandro Lucero wrote:
>>>
>>>
>>> On Mon, Dec 19, 2016 at 2:36 PM, Ferruh Yigit <ferruh.yigit@intel.com
>>> <mailto:ferruh.yigit@intel.com>> wrote:
>>>
>>>     Hi Alejandro,
>>>
>>>
>>> Hi,
>>>
>>>
>>>     On 12/19/2016 12:05 PM, Alejandro Lucero wrote:
>>>     > NFP supports more speeds than just 40 and 100GB, which were
>>>     > what was advertised before.
>>>     >
>>>     > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com
>> <mailto:alejandro.lucero@netronome.com>>
>>>     > ---
>>>     >  drivers/net/nfp/nfp_net.c | 4 +++-
>>>     >  1 file changed, 3 insertions(+), 1 deletion(-)
>>>     >
>>>     > diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
>>>     > index 27afbfd..77015c4 100644
>>>     > --- a/drivers/net/nfp/nfp_net.c
>>>     > +++ b/drivers/net/nfp/nfp_net.c
>>>     > @@ -1077,7 +1077,9 @@ static void nfp_net_read_mac(struct
>> nfp_net_hw *hw)
>>>     >       dev_info->reta_size = NFP_NET_CFG_RSS_ITBL_SZ;
>>>     >       dev_info->hash_key_size = NFP_NET_CFG_RSS_KEY_SZ;
>>>     >
>>>     > -     dev_info->speed_capa = ETH_LINK_SPEED_40G |
>> ETH_LINK_SPEED_100G;
>>>     > +     dev_info->speed_capa = ETH_SPEED_NUM_1G | ETH_LINK_SPEED_10G
>> |
>>>     > +                            ETH_SPEED_NUM_25G | ETH_SPEED_NUM_40G
>> |
>>>     > +                            ETH_SPEED_NUM_50G |
>> ETH_LINK_SPEED_100G;
>>>
>>>     Does all devices driver by this driver supports all these speeds?
>>>
>>>     I am aware of at least one exception to this, from previous patch
>> [1],
>>>     should we take that into account?
>>>
>>>
>>> So we have different NFP devices and different firmwares.
>>> NFP by design support all those speeds, but the PMD relies on the
>>> firmware for being able to know which is the current configured speed
>>> after link negotiation. PMD development was done with a specific
>>> firmware, and I was told to just report such speed by default. Last
>>> firmware versions give that speed info, but old firmware versions do not.
>>>
>>> So, all devices support such a speed range, indeed PMD works with any of
>>> them, but reported speed is always 40G with old firmware. This is a
>>> firmware limitation but we have to support old and new firmware.
>>
>> But this information to the application will be wrong for some (old) FW.
>> What do you think checking the FW version here and report capability
>> based on what FW supports?
>>
>>
> The driver advertises the right speed range supported. The problem is with
> the report about the current link speed configured.
> Maybe, is the right thing to do here to not report the current link speed
> because the driver really does not know about it?

Sorry, confused. Is it like following:

"
For new FW, there is no problem, it supports the range between 1G - 50G,
and reports correct current speed.

With old FW, device still can be set to speed range between 1G - 50G,
but it doesn't report current speed correct, and DPDK driver reports
back to application as device current speed is 40G, without really
knowing the current speed.
"

Thanks,
ferruh

> 
> If you agree with this, I'm afraid the just accepted patch about the link
> report needs to be modified.
> 
> 
> 
>>>
>>>
>>>
>>>     Also other than that exception, can you please confirm all other
>> devices
>>>     support all above speeds?
>>>
>>>     [1]
>>>     +       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;
>>>
>>>
>>>     >  }
>>>     >
>>>     >  static const uint32_t *
>>>     >
>>>
>>>
>>
>>

Re: [PATCH] nfp: extend speed capabilities advertised

[Alejandro Lucero]

On Mon, Dec 19, 2016 at 4:35 PM, Marc <marcdevel@gmail.com> wrote:

>
>
> On 19 December 2016 at 17:18, Alejandro Lucero <
> alejandro.lucero@netronome.com> wrote:
>
>> On Mon, Dec 19, 2016 at 3:05 PM, Ferruh Yigit <ferruh.yigit@intel.com>
>> wrote:
>>
>> > On 12/19/2016 3:02 PM, Alejandro Lucero wrote:
>> > >
>> > >
>> > > On Mon, Dec 19, 2016 at 2:36 PM, Ferruh Yigit <ferruh.yigit@intel.com
>> > > <mailto:ferruh.yigit@intel.com>> wrote:
>> > >
>> > >     Hi Alejandro,
>> > >
>> > >
>> > > Hi,
>> > >
>> > >
>> > >     On 12/19/2016 12:05 PM, Alejandro Lucero wrote:
>> > >     > NFP supports more speeds than just 40 and 100GB, which were
>> > >     > what was advertised before.
>> > >     >
>> > >     > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com
>> > <mailto:alejandro.lucero@netronome.com>>
>> > >     > ---
>> > >     >  drivers/net/nfp/nfp_net.c | 4 +++-
>> > >     >  1 file changed, 3 insertions(+), 1 deletion(-)
>> > >     >
>> > >     > diff --git a/drivers/net/nfp/nfp_net.c
>> b/drivers/net/nfp/nfp_net.c
>> > >     > index 27afbfd..77015c4 100644
>> > >     > --- a/drivers/net/nfp/nfp_net.c
>> > >     > +++ b/drivers/net/nfp/nfp_net.c
>> > >     > @@ -1077,7 +1077,9 @@ static void nfp_net_read_mac(struct
>> > nfp_net_hw *hw)
>> > >     >       dev_info->reta_size = NFP_NET_CFG_RSS_ITBL_SZ;
>> > >     >       dev_info->hash_key_size = NFP_NET_CFG_RSS_KEY_SZ;
>> > >     >
>> > >     > -     dev_info->speed_capa = ETH_LINK_SPEED_40G |
>> > ETH_LINK_SPEED_100G;
>> > >     > +     dev_info->speed_capa = ETH_SPEED_NUM_1G |
>> ETH_LINK_SPEED_10G
>> > |
>> > >     > +                            ETH_SPEED_NUM_25G |
>> ETH_SPEED_NUM_40G
>> > |
>> > >     > +                            ETH_SPEED_NUM_50G |
>> > ETH_LINK_SPEED_100G;
>> > >
>> > >     Does all devices driver by this driver supports all these speeds?
>> > >
>> > >     I am aware of at least one exception to this, from previous patch
>> > [1],
>> > >     should we take that into account?
>> > >
>> > >
>> > > So we have different NFP devices and different firmwares.
>> > > NFP by design support all those speeds, but the PMD relies on the
>> > > firmware for being able to know which is the current configured speed
>> > > after link negotiation. PMD development was done with a specific
>> > > firmware, and I was told to just report such speed by default. Last
>> > > firmware versions give that speed info, but old firmware versions do
>> not.
>> > >
>> > > So, all devices support such a speed range, indeed PMD works with any
>> of
>> > > them, but reported speed is always 40G with old firmware. This is a
>> > > firmware limitation but we have to support old and new firmware.
>> >
>> > But this information to the application will be wrong for some (old) FW.
>> > What do you think checking the FW version here and report capability
>> > based on what FW supports?
>> >
>> >
>> The driver advertises the right speed range supported. The problem is with
>> the report about the current link speed configured.
>> Maybe, is the right thing to do here to not report the current link speed
>> because the driver really does not know about it?
>>
>> If you agree with this, I'm afraid the just accepted patch about the link
>> report needs to be modified.
>>
>
> Alejandro,
>
> If negociated link state has to be changed, then struct rte_eth_dev_data
> dev_link field is where to do it.
>
>
Yes. The driver is already doing that.


> As Ferruh was saying, dev_info->speed_capa contains the speed capabilties
> of the particular NIC in use, not the driver (detecting firmware version
> would be the best here).
>
>
The NIC supports all the range. The problem is the driver does not really
knows the speed with old firmware.


> marc
>
>
>>
>>
>>
>> > >
>> > >
>> > >
>> > >     Also other than that exception, can you please confirm all other
>> > devices
>> > >     support all above speeds?
>> > >
>> > >     [1]
>> > >     +       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;
>> > >
>> > >
>> > >     >  }
>> > >     >
>> > >     >  static const uint32_t *
>> > >     >
>> > >
>> > >
>> >
>> >
>>
>
>

Re: [PATCH] nfp: extend speed capabilities advertised

[Alejandro Lucero]

On Mon, Dec 19, 2016 at 4:43 PM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 12/19/2016 4:18 PM, Alejandro Lucero wrote:
> > On Mon, Dec 19, 2016 at 3:05 PM, Ferruh Yigit <ferruh.yigit@intel.com>
> > wrote:
> >
> >> On 12/19/2016 3:02 PM, Alejandro Lucero wrote:
> >>>
> >>>
> >>> On Mon, Dec 19, 2016 at 2:36 PM, Ferruh Yigit <ferruh.yigit@intel.com
> >>> <mailto:ferruh.yigit@intel.com>> wrote:
> >>>
> >>>     Hi Alejandro,
> >>>
> >>>
> >>> Hi,
> >>>
> >>>
> >>>     On 12/19/2016 12:05 PM, Alejandro Lucero wrote:
> >>>     > NFP supports more speeds than just 40 and 100GB, which were
> >>>     > what was advertised before.
> >>>     >
> >>>     > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com
> >> <mailto:alejandro.lucero@netronome.com>>
> >>>     > ---
> >>>     >  drivers/net/nfp/nfp_net.c | 4 +++-
> >>>     >  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>     >
> >>>     > diff --git a/drivers/net/nfp/nfp_net.c
> b/drivers/net/nfp/nfp_net.c
> >>>     > index 27afbfd..77015c4 100644
> >>>     > --- a/drivers/net/nfp/nfp_net.c
> >>>     > +++ b/drivers/net/nfp/nfp_net.c
> >>>     > @@ -1077,7 +1077,9 @@ static void nfp_net_read_mac(struct
> >> nfp_net_hw *hw)
> >>>     >       dev_info->reta_size = NFP_NET_CFG_RSS_ITBL_SZ;
> >>>     >       dev_info->hash_key_size = NFP_NET_CFG_RSS_KEY_SZ;
> >>>     >
> >>>     > -     dev_info->speed_capa = ETH_LINK_SPEED_40G |
> >> ETH_LINK_SPEED_100G;
> >>>     > +     dev_info->speed_capa = ETH_SPEED_NUM_1G |
> ETH_LINK_SPEED_10G
> >> |
> >>>     > +                            ETH_SPEED_NUM_25G |
> ETH_SPEED_NUM_40G
> >> |
> >>>     > +                            ETH_SPEED_NUM_50G |
> >> ETH_LINK_SPEED_100G;
> >>>
> >>>     Does all devices driver by this driver supports all these speeds?
> >>>
> >>>     I am aware of at least one exception to this, from previous patch
> >> [1],
> >>>     should we take that into account?
> >>>
> >>>
> >>> So we have different NFP devices and different firmwares.
> >>> NFP by design support all those speeds, but the PMD relies on the
> >>> firmware for being able to know which is the current configured speed
> >>> after link negotiation. PMD development was done with a specific
> >>> firmware, and I was told to just report such speed by default. Last
> >>> firmware versions give that speed info, but old firmware versions do
> not.
> >>>
> >>> So, all devices support such a speed range, indeed PMD works with any
> of
> >>> them, but reported speed is always 40G with old firmware. This is a
> >>> firmware limitation but we have to support old and new firmware.
> >>
> >> But this information to the application will be wrong for some (old) FW.
> >> What do you think checking the FW version here and report capability
> >> based on what FW supports?
> >>
> >>
> > The driver advertises the right speed range supported. The problem is
> with
> > the report about the current link speed configured.
> > Maybe, is the right thing to do here to not report the current link speed
> > because the driver really does not know about it?
>
> Sorry, confused. Is it like following:
>
> "
> For new FW, there is no problem, it supports the range between 1G - 50G,
> and reports correct current speed.
>
> With old FW, device still can be set to speed range between 1G - 50G,
> but it doesn't report current speed correct, and DPDK driver reports
> back to application as device current speed is 40G, without really
> knowing the current speed.
> "
>
>
Yes, that is. Should then I do anything else or the patch is right for you
now?


> Thanks,
> ferruh
>
> >
> > If you agree with this, I'm afraid the just accepted patch about the link
> > report needs to be modified.
> >
> >
> >
> >>>
> >>>
> >>>
> >>>     Also other than that exception, can you please confirm all other
> >> devices
> >>>     support all above speeds?
> >>>
> >>>     [1]
> >>>     +       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;
> >>>
> >>>
> >>>     >  }
> >>>     >
> >>>     >  static const uint32_t *
> >>>     >
> >>>
> >>>
> >>
> >>
>
>

Re: [PATCH] nfp: extend speed capabilities advertised

[Alejandro Lucero]

I forgot one thing: to update the features file with this new one.

I will wait for your feedback regarding the discussed problem for sending
another version.

Thanks

On Mon, Dec 19, 2016 at 5:59 PM, Alejandro Lucero <
alejandro.lucero@netronome.com> wrote:

>
>
> On Mon, Dec 19, 2016 at 4:43 PM, Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
>
>> On 12/19/2016 4:18 PM, Alejandro Lucero wrote:
>> > On Mon, Dec 19, 2016 at 3:05 PM, Ferruh Yigit <ferruh.yigit@intel.com>
>> > wrote:
>> >
>> >> On 12/19/2016 3:02 PM, Alejandro Lucero wrote:
>> >>>
>> >>>
>> >>> On Mon, Dec 19, 2016 at 2:36 PM, Ferruh Yigit <ferruh.yigit@intel.com
>> >>> <mailto:ferruh.yigit@intel.com>> wrote:
>> >>>
>> >>>     Hi Alejandro,
>> >>>
>> >>>
>> >>> Hi,
>> >>>
>> >>>
>> >>>     On 12/19/2016 12:05 PM, Alejandro Lucero wrote:
>> >>>     > NFP supports more speeds than just 40 and 100GB, which were
>> >>>     > what was advertised before.
>> >>>     >
>> >>>     > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com
>> >> <mailto:alejandro.lucero@netronome.com>>
>> >>>     > ---
>> >>>     >  drivers/net/nfp/nfp_net.c | 4 +++-
>> >>>     >  1 file changed, 3 insertions(+), 1 deletion(-)
>> >>>     >
>> >>>     > diff --git a/drivers/net/nfp/nfp_net.c
>> b/drivers/net/nfp/nfp_net.c
>> >>>     > index 27afbfd..77015c4 100644
>> >>>     > --- a/drivers/net/nfp/nfp_net.c
>> >>>     > +++ b/drivers/net/nfp/nfp_net.c
>> >>>     > @@ -1077,7 +1077,9 @@ static void nfp_net_read_mac(struct
>> >> nfp_net_hw *hw)
>> >>>     >       dev_info->reta_size = NFP_NET_CFG_RSS_ITBL_SZ;
>> >>>     >       dev_info->hash_key_size = NFP_NET_CFG_RSS_KEY_SZ;
>> >>>     >
>> >>>     > -     dev_info->speed_capa = ETH_LINK_SPEED_40G |
>> >> ETH_LINK_SPEED_100G;
>> >>>     > +     dev_info->speed_capa = ETH_SPEED_NUM_1G |
>> ETH_LINK_SPEED_10G
>> >> |
>> >>>     > +                            ETH_SPEED_NUM_25G |
>> ETH_SPEED_NUM_40G
>> >> |
>> >>>     > +                            ETH_SPEED_NUM_50G |
>> >> ETH_LINK_SPEED_100G;
>> >>>
>> >>>     Does all devices driver by this driver supports all these speeds?
>> >>>
>> >>>     I am aware of at least one exception to this, from previous patch
>> >> [1],
>> >>>     should we take that into account?
>> >>>
>> >>>
>> >>> So we have different NFP devices and different firmwares.
>> >>> NFP by design support all those speeds, but the PMD relies on the
>> >>> firmware for being able to know which is the current configured speed
>> >>> after link negotiation. PMD development was done with a specific
>> >>> firmware, and I was told to just report such speed by default. Last
>> >>> firmware versions give that speed info, but old firmware versions do
>> not.
>> >>>
>> >>> So, all devices support such a speed range, indeed PMD works with any
>> of
>> >>> them, but reported speed is always 40G with old firmware. This is a
>> >>> firmware limitation but we have to support old and new firmware.
>> >>
>> >> But this information to the application will be wrong for some (old)
>> FW.
>> >> What do you think checking the FW version here and report capability
>> >> based on what FW supports?
>> >>
>> >>
>> > The driver advertises the right speed range supported. The problem is
>> with
>> > the report about the current link speed configured.
>> > Maybe, is the right thing to do here to not report the current link
>> speed
>> > because the driver really does not know about it?
>>
>> Sorry, confused. Is it like following:
>>
>> "
>> For new FW, there is no problem, it supports the range between 1G - 50G,
>> and reports correct current speed.
>>
>> With old FW, device still can be set to speed range between 1G - 50G,
>> but it doesn't report current speed correct, and DPDK driver reports
>> back to application as device current speed is 40G, without really
>> knowing the current speed.
>> "
>>
>>
> Yes, that is. Should then I do anything else or the patch is right for you
> now?
>
>
>> Thanks,
>> ferruh
>>
>> >
>> > If you agree with this, I'm afraid the just accepted patch about the
>> link
>> > report needs to be modified.
>> >
>> >
>> >
>> >>>
>> >>>
>> >>>
>> >>>     Also other than that exception, can you please confirm all other
>> >> devices
>> >>>     support all above speeds?
>> >>>
>> >>>     [1]
>> >>>     +       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;
>> >>>
>> >>>
>> >>>     >  }
>> >>>     >
>> >>>     >  static const uint32_t *
>> >>>     >
>> >>>
>> >>>
>> >>
>> >>
>>
>>
>

Re: [PATCH] nfp: extend speed capabilities advertised

[Ferruh Yigit]

On 12/19/2016 6:00 PM, Alejandro Lucero wrote:
> I forgot one thing: to update the features file with this new one.
> 
> I will wait for your feedback regarding the discussed problem for
> sending another version.

I think it is good to go, please send updated version.

> 

<...>

> 
>         Sorry, confused. Is it like following:
> 
>         "
>         For new FW, there is no problem, it supports the range between
>         1G - 50G,
>         and reports correct current speed.
> 
>         With old FW, device still can be set to speed range between 1G -
>         50G,
>         but it doesn't report current speed correct, and DPDK driver reports
>         back to application as device current speed is 40G, without really
>         knowing the current speed.
>         "
> 
> 
>     Yes, that is. Should then I do anything else or the patch is right
>     for you now?

So, this patch is correct.

But for the previous patch "net/nfp: report link speed using hardware
info", it would be nice to add above information into source as comment
and into commit log.
If you can, would you mind sending a new version of that patch?

Thanks,
ferruh

>      
<...>

Re: [PATCH] nfp: extend speed capabilities advertised

[Alejandro Lucero]

On Tue, Dec 20, 2016 at 10:25 AM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 12/19/2016 6:00 PM, Alejandro Lucero wrote:
> > I forgot one thing: to update the features file with this new one.
> >
> > I will wait for your feedback regarding the discussed problem for
> > sending another version.
>
> I think it is good to go, please send updated version.
>
>
OK


> >
>
> <...>
>
> >
> >         Sorry, confused. Is it like following:
> >
> >         "
> >         For new FW, there is no problem, it supports the range between
> >         1G - 50G,
> >         and reports correct current speed.
> >
> >         With old FW, device still can be set to speed range between 1G -
> >         50G,
> >         but it doesn't report current speed correct, and DPDK driver
> reports
> >         back to application as device current speed is 40G, without
> really
> >         knowing the current speed.
> >         "
> >
> >
> >     Yes, that is. Should then I do anything else or the patch is right
> >     for you now?
>
> So, this patch is correct.
>
> But for the previous patch "net/nfp: report link speed using hardware
> info", it would be nice to add above information into source as comment
> and into commit log.
> If you can, would you mind sending a new version of that patch?
>
>
I'll do it.

Thanks!


> Thanks,
> ferruh
>
> >
> <...>
>

Re: [PATCH] nfp: extend speed capabilities advertised

[Alejandro Lucero]

On Tue, Dec 20, 2016 at 10:29 AM, Alejandro Lucero <
alejandro.lucero@netronome.com> wrote:

>
>
> On Tue, Dec 20, 2016 at 10:25 AM, Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
>
>> On 12/19/2016 6:00 PM, Alejandro Lucero wrote:
>> > I forgot one thing: to update the features file with this new one.
>> >
>> > I will wait for your feedback regarding the discussed problem for
>> > sending another version.
>>
>> I think it is good to go, please send updated version.
>>
>>
> OK
>
>
>> >
>>
>> <...>
>>
>> >
>> >         Sorry, confused. Is it like following:
>> >
>> >         "
>> >         For new FW, there is no problem, it supports the range between
>> >         1G - 50G,
>> >         and reports correct current speed.
>> >
>> >         With old FW, device still can be set to speed range between 1G -
>> >         50G,
>> >         but it doesn't report current speed correct, and DPDK driver
>> reports
>> >         back to application as device current speed is 40G, without
>> really
>> >         knowing the current speed.
>> >         "
>> >
>> >
>> >     Yes, that is. Should then I do anything else or the patch is right
>> >     for you now?
>>
>> So, this patch is correct.
>>
>> But for the previous patch "net/nfp: report link speed using hardware
>> info", it would be nice to add above information into source as comment
>> and into commit log.
>> If you can, would you mind sending a new version of that patch?
>>
>>
>
Would it be better to report ETH_SPEED_NUM_NONE instead of
ETH_SPEED_NUM_40G for old firmware?
I will add a comment about why, but I think this is better for the user not
getting (sometimes) the right speed.


> I'll do it.
>
> Thanks!
>
>
>> Thanks,
>> ferruh
>>
>> >
>> <...>
>>
>
>