Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752916AbdFQLyR convert rfc822-to-8bit (ORCPT ); Sat, 17 Jun 2017 07:54:17 -0400 Received: from szxga02-in.huawei.com ([45.249.212.188]:8322 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752890AbdFQLyP (ORCPT ); Sat, 17 Jun 2017 07:54:15 -0400 From: Salil Mehta To: Andrew Lunn CC: "davem@davemloft.net" , "Zhuangyuzeng (Yisen)" , huangdaode , "lipeng (Y)" , "mehta.salil.lnk@gmail.com" , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Linuxarm Subject: RE: [PATCH V2 net-next 7/8] net: hns3: Add Ethtool support to HNS3 driver Thread-Topic: [PATCH V2 net-next 7/8] net: hns3: Add Ethtool support to HNS3 driver Thread-Index: AQHS5JpwRmJqIf0BOE2A84H0CqsYQ6IjgjIAgAVsFnA= Date: Sat, 17 Jun 2017 11:54:00 +0000 Message-ID: References: <20170613231035.494020-1-salil.mehta@huawei.com> <20170613231035.494020-8-salil.mehta@huawei.com> <20170614023228.GA16215@lunn.ch> In-Reply-To: <20170614023228.GA16215@lunn.ch> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.203.181.160] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020205.59451864.0038,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=169.254.2.25, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 1230bd7385eee0ec464a50e16cc74827 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5461 Lines: 152 Hi Andrew, > -----Original Message----- > From: Andrew Lunn [mailto:andrew@lunn.ch] > Sent: Wednesday, June 14, 2017 3:32 AM > To: Salil Mehta > Cc: davem@davemloft.net; Zhuangyuzeng (Yisen); huangdaode; lipeng (Y); > mehta.salil.lnk@gmail.com; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org; Linuxarm > Subject: Re: [PATCH V2 net-next 7/8] net: hns3: Add Ethtool support to > HNS3 driver > > > +static const struct hns3_link_mode_mapping hns3_lm_map[] = { > > + {HNS3_LM_FIBRE_BIT, ETHTOOL_LINK_MODE_FIBRE_BIT}, > > + {HNS3_LM_AUTONEG_BIT, ETHTOOL_LINK_MODE_Autoneg_BIT}, > > + {HNS3_LM_TP_BIT, ETHTOOL_LINK_MODE_TP_BIT}, > > + {HNS3_LM_PAUSE_BIT, ETHTOOL_LINK_MODE_Pause_BIT}, > > + {HNS3_LM_BACKPLANE_BIT, ETHTOOL_LINK_MODE_Backplane_BIT}, > > + {HNS3_LM_10BASET_HALF_BIT, ETHTOOL_LINK_MODE_10baseT_Half_BIT}, > > + {HNS3_LM_10BASET_FULL_BIT, ETHTOOL_LINK_MODE_10baseT_Full_BIT}, > > + {HNS3_LM_100BASET_HALF_BIT, ETHTOOL_LINK_MODE_100baseT_Half_BIT}, > > + {HNS3_LM_100BASET_FULL_BIT, ETHTOOL_LINK_MODE_100baseT_Full_BIT}, > > + {HNS3_LM_1000BASET_FULL_BIT, > ETHTOOL_LINK_MODE_1000baseT_Full_BIT}, > > + {HNS3_LM_10000BASEKR_FULL_BIT, > ETHTOOL_LINK_MODE_10000baseKR_Full_BIT}, > > + {HNS3_LM_25000BASEKR_FULL_BIT, > ETHTOOL_LINK_MODE_25000baseKR_Full_BIT}, > > + {HNS3_LM_40000BASELR4_FULL_BIT, > > + ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT}, > > + {HNS3_LM_50000BASEKR2_FULL_BIT, > > + ETHTOOL_LINK_MODE_50000baseKR2_Full_BIT}, > > + {HNS3_LM_100000BASEKR4_FULL_BIT, > > + ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT}, > > +}; > > I don't see anywhere your HNS3_LM_ enum's get used by the hardware. So > it would be better to just use the Linux values and avoid this > translation macro: Actually, the whole idea is to set bits in "supported" & " advertising" bitmap fields part of the " struct ethtool_link_ksettings *cmd". Mapping structure "hns3_link_mode_mapping hns3_lm_map" and its corresponding Bit set macro HNS3_DRV_TO_ETHTOOL_CAPS() is exactly being used to Map and populate these bits in the above 2 bitmaps. Without above we would end up using below macros for each of the bits being Set separately and will clog the hns3_get_link_ksettings() function something similar to below: static int hns3_get_link_ksettings(struct net_device *net_dev, struct ethtool_link_ksettings *cmd) { [....] <........un-necessary repetition of the macros.........> /* setting bit ETHTOOL_LINK_MODE_100baseT_Full_BIT */ ethtool_link_ksettings_add_link_mode(lk_ksettings, name, 100baseT_Full); /* setting bit ETHTOOL_LINK_MODE_1000baseT_Full_BIT */ ethtool_link_ksettings_add_link_mode(lk_ksettings, name, 1000baseT_Full); /* setting bit ETHTOOL_LINK_MODE_1000baseT_Full_BIT */ ethtool_link_ksettings_add_link_mode(lk_ksettings, name, 10000baseT_Full); [....] } OR another solution could be just include all of above repetition of macros as one single macros, for example Broadcom driver has done the same: static void bnxt_fw_to_ethtool_advertised_spds(struct bnxt_link_info *link_info, struct ethtool_link_ksettings *lk_ksettings) { [....] if (link_info->autoneg & BNXT_AUTONEG_FLOW_CTRL) fw_pause = link_info->auto_pause_setting; BNXT_FW_TO_ETHTOOL_SPDS(fw_speeds, fw_pause, lk_ksettings, advertising); } #define BNXT_FW_TO_ETHTOOL_SPDS(fw_speeds, fw_pause, lk_ksettings, name)\ { \ if ((fw_speeds) & BNXT_LINK_SPEED_MSK_100MB) \ ethtool_link_ksettings_add_link_mode(lk_ksettings, name,\ 100baseT_Full); \ if ((fw_speeds) & BNXT_LINK_SPEED_MSK_1GB) \ ethtool_link_ksettings_add_link_mode(lk_ksettings, name,\ 1000baseT_Full); \ if ((fw_speeds) & BNXT_LINK_SPEED_MSK_10GB) \ ethtool_link_ksettings_add_link_mode(lk_ksettings, name,\ 10000baseT_Full); \ if ((fw_speeds) & BNXT_LINK_SPEED_MSK_25GB) \ ethtool_link_ksettings_add_link_mode(lk_ksettings, name,\ 25000baseCR_Full); \ if ((fw_speeds) & BNXT_LINK_SPEED_MSK_40GB) \ ethtool_link_ksettings_add_link_mode(lk_ksettings, name,\ 40000baseCR4_Full);\ [....] \ } We thought mapping macros like ETHTOOL_LINK_MODE_*_BIT to our own internal macros like HNS3_LM_* using struct hns3_link_mode_mapping { u32 hns3_link_mode; u32 ethtool_link_mode; }; 1. Gives us control to set all these bits using the iterative loop. in macro HNS3_DRV_TO_ETHTOOL_CAPS() 2. Specifying the speed bit to be set becomes more cleaner and easier using below syntax. It is just an OR operation: supported_caps = HNS3_LM_BACKPLANE_BIT | HNS3_LM_PAUSE_BIT | HNS3_LM_AUTONEG_BIT | HNS3_LM_40000BASELR4_FULL_BIT | HNS3_LM_10000BASEKR_FULL_BIT | HNS3_LM_1000BASET_FULL_BIT | HNS3_LM_100BASET_FULL_BIT | HNS3_LM_100BASET_HALF_BIT | HNS3_LM_10BASET_FULL_BIT | HNS3_LM_10BASET_HALF_BIT; We would like to retain this format. Hope this explanation is fine? Best regards Salil > > > + > > +#define HNS3_DRV_TO_ETHTOOL_CAPS(caps, lk_ksettings, name) \ > > +{ \ > > + int i; \ > > + \ > > + for (i = 0; i < ARRAY_SIZE(hns3_lm_map); i++) { \ > > + if ((caps) & hns3_lm_map[i].hns3_link_mode) \ > > + __set_bit(hns3_lm_map[i].ethtool_link_mode,\ > > + (lk_ksettings)->link_modes.name); \ > > + } \ > > +} > > Andrew