| Message ID | 1392726375-32001-2-git-send-email-hariprasad@chelsio.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On 2/18/2014 6:26 AM, Hariprasad Shenai wrote: > From: Kumar Sanghvi <kumaras@chelsio.com> > > Also, create a new Common Code interface to translate Firmware Port Technology > Type values (enum fw_port_type) to string descriptions. This will allow us > to maintain the description translation table in one place rather than in > every driver. > > Based on original work by Scott Bardone and Casey Leedom <leedom@chelsio.com> > > Signed-off-by: Kumar Sanghvi <kumaras@chelsio.com> > --- > drivers/net/ethernet/chelsio/cxgb4/cxgb4.h | 2 +- > drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 24 +++++++++++------ > drivers/net/ethernet/chelsio/cxgb4/t4_hw.c | 35 ++++++++++++++++++++++++- > drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h | 3 +++ > 4 files changed, 54 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h > index 1f4b9b3..0c4edd1 100644 > --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h > @@ -957,7 +957,7 @@ int t4_mc_read(struct adapter *adap, int idx, u32 addr, __be32 *data, > u64 *parity); > int t4_edc_read(struct adapter *adap, int idx, u32 addr, __be32 *data, > u64 *parity); > - > +const char *t4_get_port_type_description(enum fw_port_type port_type); > void t4_get_port_stats(struct adapter *adap, int idx, struct port_stats *p); > void t4_read_mtu_tbl(struct adapter *adap, u16 *mtus, u8 *mtu_log); > void t4_tp_wr_bits_indirect(struct adapter *adap, unsigned int addr, > diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c > index 43ab35f..ee2f123 100644 > --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c > @@ -432,6 +432,9 @@ static void link_report(struct net_device *dev) > case SPEED_100: > s = "100Mbps"; > break; > + case 40000: /* Need a SPEED_40000 in ethtool.h */ > + s = "40Gbps"; > + break; You probably should add SPEED_40000 to include/uapi/linux/ethtool.h as part of this series. Steve. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/19/14 13:12, Steve Wise wrote: > > You probably should add SPEED_40000 to include/uapi/linux/ethtool.h as > part of this series. I'm ~pretty sure~ that the "word on the street" was that the community wanted to get away from the SPEED_XXX symbols since they simply represented the values XXX. Thus they didn't offer any real symbolic isolation from weird constants, etc. I believe that the old SPEED_XXX values were left in place in order to avoid making tons of changes everywhere ... Casey -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014-02-20 10:07 GMT-08:00 Casey Leedom <leedom@chelsio.com>: > > On 02/19/14 13:12, Steve Wise wrote: >> >> >> You probably should add SPEED_40000 to include/uapi/linux/ethtool.h as >> part of this series. > > > I'm ~pretty sure~ that the "word on the street" was that the community > wanted to get away from the SPEED_XXX symbols since they simply represented > the values XXX. Thus they didn't offer any real symbolic isolation from > weird constants, etc. I believe that the old SPEED_XXX values were left in > place in order to avoid making tons of changes everywhere ... Not quite sure where and when you heard that, it seems a little disturbing to add a comment in this patch saying "this I how I should fix things" and not do them, especially when this is a one-liner. Having a well defined constant is easier to grep than having the open-coded 40000 constant which will lead to false positives throughout the tree. -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/20/14 11:00, Florian Fainelli wrote: > 2014-02-20 10:07 GMT-08:00 Casey Leedom <leedom@chelsio.com>: >> On 02/19/14 13:12, Steve Wise wrote: >>> >>> You probably should add SPEED_40000 to include/uapi/linux/ethtool.h as >>> part of this series. >> >> I'm ~pretty sure~ that the "word on the street" was that the community >> wanted to get away from the SPEED_XXX symbols since they simply represented >> the values XXX. Thus they didn't offer any real symbolic isolation from >> weird constants, etc. I believe that the old SPEED_XXX values were left in >> place in order to avoid making tons of changes everywhere ... > Not quite sure where and when you heard that, it seems a little > disturbing to add a comment in this patch saying "this I how I should > fix things" and not do them, especially when this is a one-liner. > Having a well defined constant is easier to grep than having the > open-coded 40000 constant which will lead to false positives > throughout the tree. Like I said, it was a vague memory at best from over a year ago. I seem to remember someone on our team trying to push SPEED_40000 into the kernel and getting rebuffed. Perhaps I didn't have enough coffee that day. In any case, I personally like the idea of SPEED_40000 for exactly the reason you offer: I can search for it meaningfully. So If my vague memory is wrong, yeay! Casey > -- > Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, February 02/20/14, 2014 at 11:16:26 -0800, Casey Leedom wrote: > > On 02/20/14 11:00, Florian Fainelli wrote: > >2014-02-20 10:07 GMT-08:00 Casey Leedom <leedom@chelsio.com>: > >>On 02/19/14 13:12, Steve Wise wrote: > >>> > >>>You probably should add SPEED_40000 to include/uapi/linux/ethtool.h as > >>>part of this series. > >> > >> I'm ~pretty sure~ that the "word on the street" was that the community > >>wanted to get away from the SPEED_XXX symbols since they simply represented > >>the values XXX. Thus they didn't offer any real symbolic isolation from > >>weird constants, etc. I believe that the old SPEED_XXX values were left in > >>place in order to avoid making tons of changes everywhere ... > >Not quite sure where and when you heard that, it seems a little > >disturbing to add a comment in this patch saying "this I how I should > >fix things" and not do them, especially when this is a one-liner. > >Having a well defined constant is easier to grep than having the > >open-coded 40000 constant which will lead to false positives > >throughout the tree. > > Like I said, it was a vague memory at best from over a year ago. I > seem to remember someone on our team trying to push SPEED_40000 into > the kernel and getting rebuffed. Perhaps I didn't have enough > coffee that day. > > In any case, I personally like the idea of SPEED_40000 for exactly > the reason you offer: I can search for it meaningfully. So If my > vague memory is wrong, yeay! BTW, I just now found below thread and discussion related to SPEED_40000 http://www.spinics.net/lists/netdev/msg201449.html > > Casey > > >-- > >Florian > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ah, thanks Kumar. So I wasn't ~completely~ off my rocker remembering that. So it sounds like Ben and David are (or at least, were) in the anti-SPEED_40000 and Florian may be in the "pro" camp ... Casey On 02/20/14 20:37, Kumar Sanghvi wrote: > On Thursday, February 02/20/14, 2014 at 11:16:26 -0800, Casey Leedom wrote: >> On 02/20/14 11:00, Florian Fainelli wrote: >>> 2014-02-20 10:07 GMT-08:00 Casey Leedom <leedom@chelsio.com>: >>>> On 02/19/14 13:12, Steve Wise wrote: >>>>> You probably should add SPEED_40000 to include/uapi/linux/ethtool.h as >>>>> part of this series. >>>> I'm ~pretty sure~ that the "word on the street" was that the community >>>> wanted to get away from the SPEED_XXX symbols since they simply represented >>>> the values XXX. Thus they didn't offer any real symbolic isolation from >>>> weird constants, etc. I believe that the old SPEED_XXX values were left in >>>> place in order to avoid making tons of changes everywhere ... >>> Not quite sure where and when you heard that, it seems a little >>> disturbing to add a comment in this patch saying "this I how I should >>> fix things" and not do them, especially when this is a one-liner. >>> Having a well defined constant is easier to grep than having the >>> open-coded 40000 constant which will lead to false positives >>> throughout the tree. >> Like I said, it was a vague memory at best from over a year ago. I >> seem to remember someone on our team trying to push SPEED_40000 into >> the kernel and getting rebuffed. Perhaps I didn't have enough >> coffee that day. >> >> In any case, I personally like the idea of SPEED_40000 for exactly >> the reason you offer: I can search for it meaningfully. So If my >> vague memory is wrong, yeay! > > BTW, I just now found below thread and discussion related to SPEED_40000 > http://www.spinics.net/lists/netdev/msg201449.html > > >> Casey >> >>> -- >>> Florian >> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-02-20 at 11:00 -0800, Florian Fainelli wrote: > 2014-02-20 10:07 GMT-08:00 Casey Leedom <leedom@chelsio.com>: > > > > On 02/19/14 13:12, Steve Wise wrote: > >> > >> > >> You probably should add SPEED_40000 to include/uapi/linux/ethtool.h as > >> part of this series. > > > > > > I'm ~pretty sure~ that the "word on the street" was that the community > > wanted to get away from the SPEED_XXX symbols since they simply represented > > the values XXX. Thus they didn't offer any real symbolic isolation from > > weird constants, etc. I believe that the old SPEED_XXX values were left in > > place in order to avoid making tons of changes everywhere ... > > Not quite sure where and when you heard that, it seems a little > disturbing to add a comment in this patch saying "this I how I should > fix things" and not do them, especially when this is a one-liner. [...] Yeah, the way to fix that is to remove the comments... Ben.
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h index 1f4b9b3..0c4edd1 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h @@ -957,7 +957,7 @@ int t4_mc_read(struct adapter *adap, int idx, u32 addr, __be32 *data, u64 *parity); int t4_edc_read(struct adapter *adap, int idx, u32 addr, __be32 *data, u64 *parity); - +const char *t4_get_port_type_description(enum fw_port_type port_type); void t4_get_port_stats(struct adapter *adap, int idx, struct port_stats *p); void t4_read_mtu_tbl(struct adapter *adap, u16 *mtus, u8 *mtu_log); void t4_tp_wr_bits_indirect(struct adapter *adap, unsigned int addr, diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c index 43ab35f..ee2f123 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c @@ -432,6 +432,9 @@ static void link_report(struct net_device *dev) case SPEED_100: s = "100Mbps"; break; + case 40000: /* Need a SPEED_40000 in ethtool.h */ + s = "40Gbps"; + break; } netdev_info(dev, "link up, %s, full-duplex, %s PAUSE\n", s, @@ -2199,6 +2202,8 @@ static unsigned int from_fw_linkcaps(unsigned int type, unsigned int caps) else if (type == FW_PORT_TYPE_FIBER_XFI || type == FW_PORT_TYPE_FIBER_XAUI || type == FW_PORT_TYPE_SFP) v |= SUPPORTED_FIBRE; + else if (type == FW_PORT_TYPE_BP40_BA) + v |= SUPPORTED_40000baseSR4_Full; if (caps & FW_PORT_CAP_ANEG) v |= SUPPORTED_Autoneg; @@ -2215,6 +2220,8 @@ static unsigned int to_fw_linkcaps(unsigned int caps) v |= FW_PORT_CAP_SPEED_1G; if (caps & ADVERTISED_10000baseT_Full) v |= FW_PORT_CAP_SPEED_10G; + if (caps & ADVERTISED_40000baseSR4_Full) + v |= FW_PORT_CAP_SPEED_40G; return v; } @@ -2269,6 +2276,8 @@ static unsigned int speed_to_caps(int speed) return FW_PORT_CAP_SPEED_1G; if (speed == SPEED_10000) return FW_PORT_CAP_SPEED_10G; + if (speed == 40000) /* Need SPEED_40000 in ethtool.h */ + return FW_PORT_CAP_SPEED_40G; return 0; } @@ -2296,8 +2305,10 @@ static int set_settings(struct net_device *dev, struct ethtool_cmd *cmd) if (cmd->autoneg == AUTONEG_DISABLE) { cap = speed_to_caps(speed); - if (!(lc->supported & cap) || (speed == SPEED_1000) || - (speed == SPEED_10000)) + if (!(lc->supported & cap) || + (speed == SPEED_1000) || + (speed == SPEED_10000) || + (speed == 40000)) return -EINVAL; lc->requested_speed = cap; lc->advertising = 0; @@ -5801,11 +5812,6 @@ static int init_rss(struct adapter *adap) static void print_port_info(const struct net_device *dev) { - static const char *base[] = { - "R XFI", "R XAUI", "T SGMII", "T XFI", "T XAUI", "KX4", "CX4", - "KX", "KR", "R SFP+", "KR/KX", "KR/KX/KX4" - }; - char buf[80]; char *bufp = buf; const char *spd = ""; @@ -5823,9 +5829,11 @@ static void print_port_info(const struct net_device *dev) bufp += sprintf(bufp, "1000/"); if (pi->link_cfg.supported & FW_PORT_CAP_SPEED_10G) bufp += sprintf(bufp, "10G/"); + if (pi->link_cfg.supported & FW_PORT_CAP_SPEED_40G) + bufp += sprintf(bufp, "40G/"); if (bufp != buf) --bufp; - sprintf(bufp, "BASE-%s", base[pi->port_type]); + sprintf(bufp, "BASE-%s", t4_get_port_type_description(pi->port_type)); netdev_info(dev, "Chelsio %s rev %d %s %sNIC PCIe x%d%s%s\n", adap->params.vpd.id, diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c index 2c10934..514c525 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c +++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c @@ -1155,7 +1155,8 @@ out: } #define ADVERT_MASK (FW_PORT_CAP_SPEED_100M | FW_PORT_CAP_SPEED_1G |\ - FW_PORT_CAP_SPEED_10G | FW_PORT_CAP_ANEG) + FW_PORT_CAP_SPEED_10G | FW_PORT_CAP_SPEED_40G | \ + FW_PORT_CAP_ANEG) /** * t4_link_start - apply link configuration to MAC/PHY @@ -2247,6 +2248,36 @@ static unsigned int get_mps_bg_map(struct adapter *adap, int idx) } /** + * t4_get_port_type_description - return Port Type string description + * @port_type: firmware Port Type enumeration + */ +const char *t4_get_port_type_description(enum fw_port_type port_type) +{ + static const char *const port_type_description[] = { + "R XFI", + "R XAUI", + "T SGMII", + "T XFI", + "T XAUI", + "KX4", + "CX4", + "KX", + "KR", + "R SFP+", + "KR/KX", + "KR/KX/KX4", + "R QSFP_10G", + "", + "R QSFP", + "R BP40_BA", + }; + + if (port_type < ARRAY_SIZE(port_type_description)) + return port_type_description[port_type]; + return "UNKNOWN"; +} + +/** * t4_get_port_stats - collect port statistics * @adap: the adapter * @idx: the port index @@ -3538,6 +3569,8 @@ int t4_handle_fw_rpl(struct adapter *adap, const __be64 *rpl) speed = SPEED_1000; else if (stat & FW_PORT_CMD_LSPEED(FW_PORT_CAP_SPEED_10G)) speed = SPEED_10000; + else if (stat & FW_PORT_CMD_LSPEED(FW_PORT_CAP_SPEED_40G)) + speed = 40000; /* Need SPEED_40000 in ethtool.h */ if (link_ok != lc->link_ok || speed != lc->speed || fc != lc->fc) { /* something changed */ diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h b/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h index 74fea74..af6e124 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h +++ b/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h @@ -1742,6 +1742,9 @@ enum fw_port_type { FW_PORT_TYPE_SFP, FW_PORT_TYPE_BP_AP, FW_PORT_TYPE_BP4_AP, + FW_PORT_TYPE_QSFP_10G, + FW_PORT_TYPE_QSFP, + FW_PORT_TYPE_BP40_BA, FW_PORT_TYPE_NONE = FW_PORT_CMD_PTYPE_MASK };