| Message ID | 20200716115446.994-11-alobakin@marvell.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | qed/qede: add support for new operating modes | expand |
On Thu, 16 Jul 2020 14:54:43 +0300 Alexander Lobakin wrote: > These ports ship on new boards revisions and are supported by newer > firmware versions. > > Signed-off-by: Alexander Lobakin <alobakin@marvell.com> > Signed-off-by: Igor Russkikh <irusskikh@marvell.com> What is the driver actually doing with them, tho? Looks like you translate some firmware specific field to a driver specific field, but I can't figure out what part of the code cares about hw_info.port_mode > diff --git a/drivers/net/ethernet/qlogic/qed/qed.h b/drivers/net/ethernet/qlogic/qed/qed.h > index 6a1d12da7910..63fcbd5a295a 100644 > --- a/drivers/net/ethernet/qlogic/qed/qed.h > +++ b/drivers/net/ethernet/qlogic/qed/qed.h > @@ -257,6 +257,11 @@ enum QED_PORT_MODE { > QED_PORT_MODE_DE_1X25G, > QED_PORT_MODE_DE_4X25G, > QED_PORT_MODE_DE_2X10G, > + QED_PORT_MODE_DE_2X50G_R1, > + QED_PORT_MODE_DE_4X50G_R1, > + QED_PORT_MODE_DE_1X100G_R2, > + QED_PORT_MODE_DE_2X100G_R2, > + QED_PORT_MODE_DE_1X100G_R4, > }; > > enum qed_dev_cap { > diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c > index d929556247a5..4bad836d0f74 100644 > --- a/drivers/net/ethernet/qlogic/qed/qed_dev.c > +++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c > @@ -4026,6 +4026,21 @@ static int qed_hw_get_nvm_info(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt) > case NVM_CFG1_GLOB_NETWORK_PORT_MODE_4X25G: > p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_4X25G; > break; > + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X50G_R1: > + p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_2X50G_R1; > + break; > + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_4X50G_R1: > + p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_4X50G_R1; > + break; > + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R2: > + p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_1X100G_R2; > + break; > + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X100G_R2: > + p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_2X100G_R2; > + break; > + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R4: > + p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_1X100G_R4; > + break; > default: > DP_NOTICE(p_hwfn, "Unknown port mode in 0x%08x\n", core_cfg); > break; > diff --git a/drivers/net/ethernet/qlogic/qed/qed_hsi.h b/drivers/net/ethernet/qlogic/qed/qed_hsi.h > index a4a845579fd2..debc55923251 100644 > --- a/drivers/net/ethernet/qlogic/qed/qed_hsi.h > +++ b/drivers/net/ethernet/qlogic/qed/qed_hsi.h > @@ -13015,6 +13015,11 @@ struct nvm_cfg1_glob { > #define NVM_CFG1_GLOB_NETWORK_PORT_MODE_1X25G 0xd > #define NVM_CFG1_GLOB_NETWORK_PORT_MODE_4X25G 0xe > #define NVM_CFG1_GLOB_NETWORK_PORT_MODE_2X10G 0xf > +#define NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X50G_R1 0x11 > +#define NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_4X50G_R1 0x12 > +#define NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R2 0x13 > +#define NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X100G_R2 0x14 > +#define NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R4 0x15 > > u32 e_lane_cfg1; > u32 e_lane_cfg2;
From: Jakub Kicinski <kuba@kernel.org> Date: Thu, 16 Jul 2020 18:18:57 -0700 Hi Jakub, > On Thu, 16 Jul 2020 14:54:43 +0300 Alexander Lobakin wrote: >> These ports ship on new boards revisions and are supported by newer >> firmware versions. >> >> Signed-off-by: Alexander Lobakin <alobakin@marvell.com> >> Signed-off-by: Igor Russkikh <irusskikh@marvell.com> > > What is the driver actually doing with them, tho? > > Looks like you translate some firmware specific field to a driver > specific field, but I can't figure out what part of the code cares > about hw_info.port_mode You're right, we just check NVM port type for validity and store it for the case of future expansions. Is that OK or I should do smth with that? >> diff --git a/drivers/net/ethernet/qlogic/qed/qed.h b/drivers/net/ethernet/qlogic/qed/qed.h >> index 6a1d12da7910..63fcbd5a295a 100644 >> --- a/drivers/net/ethernet/qlogic/qed/qed.h >> +++ b/drivers/net/ethernet/qlogic/qed/qed.h >> @@ -257,6 +257,11 @@ enum QED_PORT_MODE { >> QED_PORT_MODE_DE_1X25G, >> QED_PORT_MODE_DE_4X25G, >> QED_PORT_MODE_DE_2X10G, >> + QED_PORT_MODE_DE_2X50G_R1, >> + QED_PORT_MODE_DE_4X50G_R1, >> + QED_PORT_MODE_DE_1X100G_R2, >> + QED_PORT_MODE_DE_2X100G_R2, >> + QED_PORT_MODE_DE_1X100G_R4, >> }; >> >> enum qed_dev_cap { >> diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c >> index d929556247a5..4bad836d0f74 100644 >> --- a/drivers/net/ethernet/qlogic/qed/qed_dev.c >> +++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c >> @@ -4026,6 +4026,21 @@ static int qed_hw_get_nvm_info(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt) >> case NVM_CFG1_GLOB_NETWORK_PORT_MODE_4X25G: >> p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_4X25G; >> break; >> + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X50G_R1: >> + p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_2X50G_R1; >> + break; >> + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_4X50G_R1: >> + p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_4X50G_R1; >> + break; >> + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R2: >> + p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_1X100G_R2; >> + break; >> + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X100G_R2: >> + p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_2X100G_R2; >> + break; >> + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R4: >> + p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_1X100G_R4; >> + break; >> default: >> DP_NOTICE(p_hwfn, "Unknown port mode in 0x%08x\n", core_cfg); >> break; >> diff --git a/drivers/net/ethernet/qlogic/qed/qed_hsi.h b/drivers/net/ethernet/qlogic/qed/qed_hsi.h >> index a4a845579fd2..debc55923251 100644 >> --- a/drivers/net/ethernet/qlogic/qed/qed_hsi.h >> +++ b/drivers/net/ethernet/qlogic/qed/qed_hsi.h >> @@ -13015,6 +13015,11 @@ struct nvm_cfg1_glob { >> #define NVM_CFG1_GLOB_NETWORK_PORT_MODE_1X25G 0xd >> #define NVM_CFG1_GLOB_NETWORK_PORT_MODE_4X25G 0xe >> #define NVM_CFG1_GLOB_NETWORK_PORT_MODE_2X10G 0xf >> +#define NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X50G_R1 0x11 >> +#define NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_4X50G_R1 0x12 >> +#define NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R2 0x13 >> +#define NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X100G_R2 0x14 >> +#define NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R4 0x15 >> >> u32 e_lane_cfg1; >> u32 e_lane_cfg2; Al
> ---------------------------------------------------------------------- > On Thu, 16 Jul 2020 14:54:43 +0300 Alexander Lobakin wrote: >> These ports ship on new boards revisions and are supported by newer >> firmware versions. >> >> Signed-off-by: Alexander Lobakin <alobakin@marvell.com> >> Signed-off-by: Igor Russkikh <irusskikh@marvell.com> > > What is the driver actually doing with them, tho? > > Looks like you translate some firmware specific field to a driver > specific field, but I can't figure out what part of the code cares > about hw_info.port_mode Hi Jakub, You are right, this info is never used/reported. Alexander is extending already existing non used field with new values from our latest hardware revisions. I thought devlink info could be a good place to output such kind of information. Thats basically a layout of *Physical* ports on device - quite useful info I think. Important thing is these ports may not be directly mapped to PCI PFs. So reading `ethtool eth*` may not explain you the real device capabilities. Do you think it makes sense adding such info to `devlink info` then? Thanks Igor
On Fri, 17 Jul 2020 13:49:33 +0300 Igor Russkikh wrote: > > ---------------------------------------------------------------------- > > On Thu, 16 Jul 2020 14:54:43 +0300 Alexander Lobakin wrote: > >> These ports ship on new boards revisions and are supported by newer > >> firmware versions. > >> > >> Signed-off-by: Alexander Lobakin <alobakin@marvell.com> > >> Signed-off-by: Igor Russkikh <irusskikh@marvell.com> > > > > What is the driver actually doing with them, tho? > > > > Looks like you translate some firmware specific field to a driver > > specific field, but I can't figure out what part of the code cares > > about hw_info.port_mode > > Hi Jakub, > > You are right, this info is never used/reported. > > Alexander is extending already existing non used field with new values from > our latest hardware revisions. > > I thought devlink info could be a good place to output such kind of information. > > Thats basically a layout of *Physical* ports on device - quite useful info I > think. > > Important thing is these ports may not be directly mapped to PCI PFs. So > reading `ethtool eth*` may not explain you the real device capabilities. > > Do you think it makes sense adding such info to `devlink info` then? Devlink port has information about physical port, which don't have to map 1:1 to netdevs. It also has lanes and port splitting which you may want to report. For now please make sure to not include any dead code in your submissions (register defines etc. may be okay), perhaps try: diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c index d929556247a5..4bad836d0f74 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_dev.c +++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c @@ -4026,6 +4026,21 @@ static int qed_hw_get_nvm_info(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt) case NVM_CFG1_GLOB_NETWORK_PORT_MODE_4X25G: p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_4X25G; break; + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X50G_R1: + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_4X50G_R1: + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R2: + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X100G_R2: + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R4: + /* TODO: set port_mode when it's actually used */ + break; default: DP_NOTICE(p_hwfn, "Unknown port mode in 0x%08x\n", core_cfg); break; And see if it will pass the muster. Dead code makes it harder to review the patches.
Date: Fri, 17 Jul 2020 11:31:55 -0700 From: Jakub Kicinski <kuba@kernel.org> > On Fri, 17 Jul 2020 13:49:33 +0300 Igor Russkikh wrote: >>> ---------------------------------------------------------------------- >>> On Thu, 16 Jul 2020 14:54:43 +0300 Alexander Lobakin wrote: >>>> These ports ship on new boards revisions and are supported by newer >>>> firmware versions. >>>> >>>> Signed-off-by: Alexander Lobakin <alobakin@marvell.com> >>>> Signed-off-by: Igor Russkikh <irusskikh@marvell.com> >>> >>> What is the driver actually doing with them, tho? >>> >>> Looks like you translate some firmware specific field to a driver >>> specific field, but I can't figure out what part of the code cares >>> about hw_info.port_mode >> >> Hi Jakub, >> >> You are right, this info is never used/reported. >> >> Alexander is extending already existing non used field with new values from >> our latest hardware revisions. >> >> I thought devlink info could be a good place to output such kind of information. >> >> Thats basically a layout of *Physical* ports on device - quite useful info I >> think. >> >> Important thing is these ports may not be directly mapped to PCI PFs. So >> reading `ethtool eth*` may not explain you the real device capabilities. >> >> Do you think it makes sense adding such info to `devlink info` then? > > Devlink port has information about physical port, which don't have to > map 1:1 to netdevs. It also has lanes and port splitting which you may > want to report. > > > For now please make sure to not include any dead code in your > submissions (register defines etc. may be okay), perhaps try: I think it would be better to drop this struct member at all since it's really not used anywhere. There'll be no problem to add it back anytime we might need it, but for now it seems unreasonable to keep it "just in case". Got it, thanks, will send v2 soon. > diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c > index d929556247a5..4bad836d0f74 100644 > --- a/drivers/net/ethernet/qlogic/qed/qed_dev.c > +++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c > @@ -4026,6 +4026,21 @@ static int qed_hw_get_nvm_info(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt) > case NVM_CFG1_GLOB_NETWORK_PORT_MODE_4X25G: > p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_4X25G; > break; > + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X50G_R1: > + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_4X50G_R1: > + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R2: > + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X100G_R2: > + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R4: > + /* TODO: set port_mode when it's actually used */ > + break; > default: > DP_NOTICE(p_hwfn, "Unknown port mode in 0x%08x\n", core_cfg); > break; > > And see if it will pass the muster. > > Dead code makes it harder to review the patches.
diff --git a/drivers/net/ethernet/qlogic/qed/qed.h b/drivers/net/ethernet/qlogic/qed/qed.h index 6a1d12da7910..63fcbd5a295a 100644 --- a/drivers/net/ethernet/qlogic/qed/qed.h +++ b/drivers/net/ethernet/qlogic/qed/qed.h @@ -257,6 +257,11 @@ enum QED_PORT_MODE { QED_PORT_MODE_DE_1X25G, QED_PORT_MODE_DE_4X25G, QED_PORT_MODE_DE_2X10G, + QED_PORT_MODE_DE_2X50G_R1, + QED_PORT_MODE_DE_4X50G_R1, + QED_PORT_MODE_DE_1X100G_R2, + QED_PORT_MODE_DE_2X100G_R2, + QED_PORT_MODE_DE_1X100G_R4, }; enum qed_dev_cap { diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c index d929556247a5..4bad836d0f74 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_dev.c +++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c @@ -4026,6 +4026,21 @@ static int qed_hw_get_nvm_info(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt) case NVM_CFG1_GLOB_NETWORK_PORT_MODE_4X25G: p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_4X25G; break; + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X50G_R1: + p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_2X50G_R1; + break; + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_4X50G_R1: + p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_4X50G_R1; + break; + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R2: + p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_1X100G_R2; + break; + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X100G_R2: + p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_2X100G_R2; + break; + case NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R4: + p_hwfn->hw_info.port_mode = QED_PORT_MODE_DE_1X100G_R4; + break; default: DP_NOTICE(p_hwfn, "Unknown port mode in 0x%08x\n", core_cfg); break; diff --git a/drivers/net/ethernet/qlogic/qed/qed_hsi.h b/drivers/net/ethernet/qlogic/qed/qed_hsi.h index a4a845579fd2..debc55923251 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_hsi.h +++ b/drivers/net/ethernet/qlogic/qed/qed_hsi.h @@ -13015,6 +13015,11 @@ struct nvm_cfg1_glob { #define NVM_CFG1_GLOB_NETWORK_PORT_MODE_1X25G 0xd #define NVM_CFG1_GLOB_NETWORK_PORT_MODE_4X25G 0xe #define NVM_CFG1_GLOB_NETWORK_PORT_MODE_2X10G 0xf +#define NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X50G_R1 0x11 +#define NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_4X50G_R1 0x12 +#define NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R2 0x13 +#define NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_2X100G_R2 0x14 +#define NVM_CFG1_GLOB_NETWORK_PORT_MODE_AHP_1X100G_R4 0x15 u32 e_lane_cfg1; u32 e_lane_cfg2;