Messages in this thread Patch in this message | | | From | Chris Packham <> | Subject | Re: [Patch v3 net-next 5/7] octeontx2-af: advertised link modes support on cgx | Date | Thu, 1 Dec 2022 02:02:07 +0000 |
| |
Hi Hariprasad, Christina,
Sorry for poking an old thread but Richard and I were looking at some code and noticed something odd about it.
On 1/02/21 18:24, Hariprasad Kelam wrote: > From: Christina Jacob <cjacob@marvell.com> > > CGX supports setting advertised link modes on physical link. > This patch adds support to derive cgx mode from ethtool > link mode and pass it to firmware to configure the same. > > Signed-off-by: Christina Jacob <cjacob@marvell.com> > Signed-off-by: Sunil Goutham <sgoutham@marvell.com> > Signed-off-by: Hariprasad Kelam <hkelam@marvell.com> > --- > drivers/net/ethernet/marvell/octeontx2/af/cgx.c | 114 ++++++++++++++++++++- > .../net/ethernet/marvell/octeontx2/af/cgx_fw_if.h | 32 +++++- > drivers/net/ethernet/marvell/octeontx2/af/mbox.h | 3 +- > 3 files changed, 146 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c > index 5b7d858..9c62129 100644 > --- a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c > +++ b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c > @@ -14,6 +14,7 @@ > #include <linux/pci.h> > #include <linux/netdevice.h> > #include <linux/etherdevice.h> > +#include <linux/ethtool.h> > #include <linux/phy.h> > #include <linux/of.h> > #include <linux/of_mdio.h> > @@ -646,6 +647,7 @@ static inline void cgx_link_usertable_init(void) > cgx_speed_mbps[CGX_LINK_25G] = 25000; > cgx_speed_mbps[CGX_LINK_40G] = 40000; > cgx_speed_mbps[CGX_LINK_50G] = 50000; > + cgx_speed_mbps[CGX_LINK_80G] = 80000; > cgx_speed_mbps[CGX_LINK_100G] = 100000; > > cgx_lmactype_string[LMAC_MODE_SGMII] = "SGMII"; > @@ -693,6 +695,110 @@ static int cgx_link_usertable_index_map(int speed) > return CGX_LINK_NONE; > } > > +static void set_mod_args(struct cgx_set_link_mode_args *args, > + u32 speed, u8 duplex, u8 autoneg, u64 mode) > +{ > + /* Fill default values incase of user did not pass > + * valid parameters > + */ > + if (args->duplex == DUPLEX_UNKNOWN) > + args->duplex = duplex; > + if (args->speed == SPEED_UNKNOWN) > + args->speed = speed; > + if (args->an == AUTONEG_UNKNOWN) > + args->an = autoneg; > + args->mode = mode; > + args->ports = 0; > +} > + > +static void otx2_map_ethtool_link_modes(u64 bitmask, > + struct cgx_set_link_mode_args *args) > +{ > + switch (bitmask) { > + case ETHTOOL_LINK_MODE_10baseT_Half_BIT: > + set_mod_args(args, 10, 1, 1, BIT_ULL(CGX_MODE_SGMII)); > + break; > + case ETHTOOL_LINK_MODE_10baseT_Full_BIT: > + set_mod_args(args, 10, 0, 1, BIT_ULL(CGX_MODE_SGMII)); > + break; > + case ETHTOOL_LINK_MODE_100baseT_Half_BIT: > + set_mod_args(args, 100, 1, 1, BIT_ULL(CGX_MODE_SGMII)); > + break; > + case ETHTOOL_LINK_MODE_100baseT_Full_BIT: > + set_mod_args(args, 100, 0, 1, BIT_ULL(CGX_MODE_SGMII)); > + break; > + case ETHTOOL_LINK_MODE_1000baseT_Half_BIT: > + set_mod_args(args, 1000, 1, 1, BIT_ULL(CGX_MODE_SGMII)); > + break; > + case ETHTOOL_LINK_MODE_1000baseT_Full_BIT: > + set_mod_args(args, 1000, 0, 1, BIT_ULL(CGX_MODE_SGMII)); > + break; > + case ETHTOOL_LINK_MODE_1000baseX_Full_BIT: > + set_mod_args(args, 1000, 0, 0, BIT_ULL(CGX_MODE_1000_BASEX)); > + break; > + case ETHTOOL_LINK_MODE_10000baseT_Full_BIT: > + set_mod_args(args, 1000, 0, 1, BIT_ULL(CGX_MODE_QSGMII));
Here the speed is set to 1000 but it looks like this should be 10000. I would have sent a patch to fix that but the mode is also a bit confusing. Normally I'd expect QSGMII to be used for Quad SGMII (i.e. 4x1G Ethernet ports multiplexed over a single SERDES). I don't see any other mode in enum CGX_MODE_ that is obviously for 10GBase-T so I thought I'd bring this to your attention. I'd still be happy to whip up a patch if you could confirm what the correct mode should be.
> + break; > + case ETHTOOL_LINK_MODE_10000baseSR_Full_BIT: > + set_mod_args(args, 10000, 0, 0, BIT_ULL(CGX_MODE_10G_C2C)); > + break; > + case ETHTOOL_LINK_MODE_10000baseLR_Full_BIT: > + set_mod_args(args, 10000, 0, 0, BIT_ULL(CGX_MODE_10G_C2M)); > + break; > + case ETHTOOL_LINK_MODE_10000baseKR_Full_BIT: > + set_mod_args(args, 10000, 0, 1, BIT_ULL(CGX_MODE_10G_KR)); > + break; > + case ETHTOOL_LINK_MODE_25000baseSR_Full_BIT: > + set_mod_args(args, 25000, 0, 0, BIT_ULL(CGX_MODE_25G_C2C)); > + break; > + case ETHTOOL_LINK_MODE_25000baseCR_Full_BIT: > + set_mod_args(args, 25000, 0, 1, BIT_ULL(CGX_MODE_25G_CR)); > + break; > + case ETHTOOL_LINK_MODE_25000baseKR_Full_BIT: > + set_mod_args(args, 25000, 0, 1, BIT_ULL(CGX_MODE_25G_KR)); > + break; > + case ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT: > + set_mod_args(args, 40000, 0, 0, BIT_ULL(CGX_MODE_40G_C2C)); > + break; > + case ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT: > + set_mod_args(args, 40000, 0, 0, BIT_ULL(CGX_MODE_40G_C2M)); > + break; > + case ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT: > + set_mod_args(args, 40000, 0, 1, BIT_ULL(CGX_MODE_40G_CR4)); > + break; > + case ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT: > + set_mod_args(args, 40000, 0, 1, BIT_ULL(CGX_MODE_40G_KR4)); > + break; > + case ETHTOOL_LINK_MODE_50000baseSR_Full_BIT: > + set_mod_args(args, 50000, 0, 0, BIT_ULL(CGX_MODE_50G_C2C)); > + break; > + case ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT: > + set_mod_args(args, 50000, 0, 0, BIT_ULL(CGX_MODE_50G_C2M)); > + break; > + case ETHTOOL_LINK_MODE_50000baseCR_Full_BIT: > + set_mod_args(args, 50000, 0, 1, BIT_ULL(CGX_MODE_50G_CR)); > + break; > + case ETHTOOL_LINK_MODE_50000baseKR_Full_BIT: > + set_mod_args(args, 50000, 0, 1, BIT_ULL(CGX_MODE_50G_KR)); > + break; > + case ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT: > + set_mod_args(args, 100000, 0, 0, BIT_ULL(CGX_MODE_100G_C2C)); > + break; > + case ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT: > + set_mod_args(args, 100000, 0, 0, BIT_ULL(CGX_MODE_100G_C2M)); > + break; > + case ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT: > + set_mod_args(args, 100000, 0, 1, BIT_ULL(CGX_MODE_100G_CR4)); > + break; > + case ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT: > + set_mod_args(args, 100000, 0, 1, BIT_ULL(CGX_MODE_100G_KR4)); > + break; > + default: > + set_mod_args(args, 0, 1, 0, BIT_ULL(CGX_MODE_MAX)); > + break; > + } > +} > + > static inline void link_status_user_format(u64 lstat, > struct cgx_link_user_info *linfo, > struct cgx *cgx, u8 lmac_id) > @@ -887,13 +993,19 @@ int cgx_set_link_mode(void *cgxd, struct cgx_set_link_mode_args args, > if (!cgx) > return -ENODEV; > > + if (args.mode) > + otx2_map_ethtool_link_modes(args.mode, &args); > + if (!args.speed && args.duplex && !args.an) > + return -EINVAL; > + > req = FIELD_SET(CMDREG_ID, CGX_CMD_MODE_CHANGE, req); > req = FIELD_SET(CMDMODECHANGE_SPEED, > cgx_link_usertable_index_map(args.speed), req); > req = FIELD_SET(CMDMODECHANGE_DUPLEX, args.duplex, req); > req = FIELD_SET(CMDMODECHANGE_AN, args.an, req); > req = FIELD_SET(CMDMODECHANGE_PORT, args.ports, req); > - req = FIELD_SET(CMDMODECHANGE_FLAGS, args.flags, req); > + req = FIELD_SET(CMDMODECHANGE_FLAGS, args.mode, req); > + > return cgx_fwi_cmd_generic(req, &resp, cgx, lmac_id); > } > int cgx_set_fec(u64 fec, int cgx_id, int lmac_id) > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cgx_fw_if.h b/drivers/net/ethernet/marvell/octeontx2/af/cgx_fw_if.h > index 70610e7..dde2bd0 100644 > --- a/drivers/net/ethernet/marvell/octeontx2/af/cgx_fw_if.h > +++ b/drivers/net/ethernet/marvell/octeontx2/af/cgx_fw_if.h > @@ -70,6 +70,36 @@ enum cgx_link_speed { > CGX_LINK_SPEED_MAX, > }; > > +enum CGX_MODE_ { > + CGX_MODE_SGMII, > + CGX_MODE_1000_BASEX, > + CGX_MODE_QSGMII, > + CGX_MODE_10G_C2C, > + CGX_MODE_10G_C2M, > + CGX_MODE_10G_KR, > + CGX_MODE_20G_C2C, > + CGX_MODE_25G_C2C, > + CGX_MODE_25G_C2M, > + CGX_MODE_25G_2_C2C, > + CGX_MODE_25G_CR, > + CGX_MODE_25G_KR, > + CGX_MODE_40G_C2C, > + CGX_MODE_40G_C2M, > + CGX_MODE_40G_CR4, > + CGX_MODE_40G_KR4, > + CGX_MODE_40GAUI_C2C, > + CGX_MODE_50G_C2C, > + CGX_MODE_50G_C2M, > + CGX_MODE_50G_4_C2C, > + CGX_MODE_50G_CR, > + CGX_MODE_50G_KR, > + CGX_MODE_80GAUI_C2C, > + CGX_MODE_100G_C2C, > + CGX_MODE_100G_C2M, > + CGX_MODE_100G_CR4, > + CGX_MODE_100G_KR4, > + CGX_MODE_MAX /* = 29 */ > +}; > /* REQUEST ID types. Input to firmware */ > enum cgx_cmd_id { > CGX_CMD_NONE, > @@ -231,6 +261,6 @@ struct cgx_lnk_sts { > #define CMDMODECHANGE_DUPLEX GENMASK_ULL(12, 12) > #define CMDMODECHANGE_AN GENMASK_ULL(13, 13) > #define CMDMODECHANGE_PORT GENMASK_ULL(21, 14) > -#define CMDMODECHANGE_FLAGS GENMASK_ULL(29, 22) > +#define CMDMODECHANGE_FLAGS GENMASK_ULL(63, 22) > > #endif /* __CGX_FW_INTF_H__ */ > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h > index a050902..05a6da2 100644 > --- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h > +++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h > @@ -462,10 +462,11 @@ struct cgx_set_link_mode_args { > u8 duplex; > u8 an; > u8 ports; > - u8 flags; > + u64 mode; > }; > > struct cgx_set_link_mode_req { > +#define AUTONEG_UNKNOWN 0xff > struct mbox_msghdr hdr; > struct cgx_set_link_mode_args args; > };
Thanks, Chris
| |