Re: [dpdk-dev] [PATCH v4 0/2] ethdev: add port speed capability bitmap

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

On Sun, Sep 13, 2015 at 11:18:33PM +0200, Thomas Monjalon wrote:
> 2015-09-13 21:14, Marc Sune:
> > 2015-09-09 15:33 GMT+02:00 Thomas Monjalon <thomas.monjalon@6wind.com>:
> > > 2015-09-09 15:10, Nélio Laranjeiro:
> > > > I think V2 is better, maybe you can add a function to convert a single
> > > > bitmap value to the equivalent integer and get rid of ETH_SPEED_XXX
> > > macros.
> > > >
> > > > Thomas what is your opinion?
> > >
> > > Your proposal looks good Nelio.
> > 
> > I am confused, specially since you were the one advocating for having a
> > unified set of constants for speeds (discussion in v2).
> 
> Yes, my first thought was advocating an unification between capabilities and
> negotiated link properties.
> After I was convinced by Nelio's arguments: bitmap is good for capabilities
> (especially to describe every capabilities in one field) but integer is better
> for negotiated speed (especially for aggregated links).
> Converting bitmap speed into integer should be easy to implement in a function.
> 
> > In any case, as I see it, if we want to address the comments of  M. Brorup:
> > 
> > http://comments.gmane.org/gmane.comp.networking.dpdk.devel/19664

I read it.

> > 
> > we need bitmaps for rte_eth_conf link_speed to set the advertised speeds.
> 
> Yes I forgot this interesting comment. It is saying we need
> 	1/ capabilities
> 	2/ advertised modes (for auto-negotiation or fixed config)
> 	3/ negotiated mode
> Previously we were focused only on 1/ and 3/.
> 2/ was only limited to a mode configured without negotiation and was using the
> same field as 3/.
> Maybe we should really have 3 different fields. 1/ and 2/ would use a bitmap.
> 
> > Let me know if you really want to come back to v2 or not.
> 
> It needs more discussion. What do you think of the above proposal?
> What is the opinion of Nelio? Morten?

I agree with Morten, and with this proposition (we should be possible to
disable some capabilities in the advertise field).

For that we should have those 3 fields:
  1/ Capability (as a bitmap),  necessary for the user to configure the
     behavior of the PHY.
  2/ Advertise (as a bitmap) to configure the PHY.
  3/ speed, duplex, status (as rte_eth_link structure), necessary to
     verify that the configuration corresponds to what has been set and
     for other purposes.

I still think Marc needs to go back to V2 and continue from this one.
And as Thomas suggested, he could have a function to get rid of the
double defines and use the sames ones for bitmap and non bitmap fields.

Just for information, before I started this discussion I have worked on
a patch to change the rte_eth_link.link_speed from 16 to 32 bit, I did
not pushed it because, it should be pushed afters Marc's one, and it
will need some rework after that.

-- 
Nélio Laranjeiro
6WIND