RE: [Patch v3 net-next 5/7] octeontx2-af: advertised link modes support on cgx

From: Hariprasad Kelam
Date: Fri Dec 02 2022 - 02:57:28 EST

Next message: Huang Rui: "Re: [RESEND PATCH V2 0/2] Add speedometer new test cases for amd-pstate-ut"
Previous message: Pavan Chebbi: "Re: [PATCH 2/2] net: encx24j600: Fix invalid logic in reading of MISTAT register"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Chris ,

See inline,

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@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Signed-off-by: Sunil Goutham <sgoutham@xxxxxxxxxxx>
> Signed-off-by: Hariprasad Kelam <hkelam@xxxxxxxxxxx>
> ---
> 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.

>> 10GBase-T to QSGMII mapping is wrong. To correct it, we need to map proper
ETHTOOL_LINK_MODEX to QSGMII keeping all other parameters the same.
As transmit and receive data paths of QSGMII leverage the 1000BASE-X , we may use
ETHTOOL_LINK_MODE_1000baseSX_Full_BIT.
If there are no objections to the above mode, go ahead and submit the patch.

Thanks,
Hariprasad k

> + 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

Next message: Huang Rui: "Re: [RESEND PATCH V2 0/2] Add speedometer new test cases for amd-pstate-ut"
Previous message: Pavan Chebbi: "Re: [PATCH 2/2] net: encx24j600: Fix invalid logic in reading of MISTAT register"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]