Re: [Patch v2 net-next 2/7] octeontx2-af: Add new CGX_CMD to get PHY FEC statistics
Hi Willem,
> -Original Message-
> From: Willem de Bruijn
> Sent: Saturday, January 30, 2021 7:57 PM
> To: Hariprasad Kelam
> Cc: Network Development ; LKML ker...@vger.kernel.org>; David Miller ; Jakub
> Kicinski ; Sunil Kovvuri Goutham
> ; Linu Cherian ;
> Geethasowjanya Akula ; Jerin Jacob Kollanukkaran
> ; Subbaraya Sundeep Bhatta ;
> Felix Manlunas ; Christina Jacob
> ; Sunil Kovvuri Goutham
>
> Subject: [EXT] Re: [Patch v2 net-next 2/7] octeontx2-af: Add new CGX_CMD
> to get PHY FEC statistics
> On Sat, Jan 30, 2021 at 4:53 AM Hariprasad Kelam
> wrote:
> >
> > Hi Willem,
> >
> > > -Original Message-
> > > From: Willem de Bruijn
> > > Sent: Thursday, January 28, 2021 1:50 AM
> > > To: Hariprasad Kelam
> > > Cc: Network Development ; LKML > > ker...@vger.kernel.org>; David Miller ; Jakub
> > > Kicinski ; Sunil Kovvuri Goutham
> > > ; Linu Cherian ;
> > > Geethasowjanya Akula ; Jerin Jacob Kollanukkaran
> > > ; Subbaraya Sundeep Bhatta
> > > ; Felix Manlunas ;
> > > Christina Jacob ; Sunil Kovvuri Goutham
> > >
> > > Subject: [EXT] Re: [Patch v2 net-next 2/7] octeontx2-af: Add new
> > > CGX_CMD to get PHY FEC statistics
> > >
> > > On Wed, Jan 27, 2021 at 4:04 AM Hariprasad Kelam
> > >
> > > wrote:
> > > >
> > > > From: Felix Manlunas
> > > >
> > > > This patch adds support to fetch fec stats from PHY. The stats are
> > > > put in the shared data struct fwdata. A PHY driver indicates that
> > > > it has FEC stats by setting the flag fwdata.phy.misc.has_fec_stats
> > > >
> > > > Besides CGX_CMD_GET_PHY_FEC_STATS, also add CGX_CMD_PRBS and
> > > > CGX_CMD_DISPLAY_EYE to enum cgx_cmd_id so that Linux's enum list
> > > > is in sync with firmware's enum list.
> > > >
> > > > Signed-off-by: Felix Manlunas
> > > > Signed-off-by: Christina Jacob
> > > > Signed-off-by: Sunil Kovvuri Goutham
> > > > Signed-off-by: Hariprasad Kelam
> > >
> > >
> > > > +struct phy_s {
> > > > + struct {
> > > > + u64 can_change_mod_type : 1;
> > > > + u64 mod_type: 1;
> > > > + u64 has_fec_stats : 1;
> > >
> > > this style is not customary
> >
> > These structures are shared with firmware and stored in a shared memory.
> Any change in size of structures will break compatibility. To avoid frequent
> compatible issues with new vs old firmware we have put spaces where ever
> we see that there could be more fields added in future.
> > So changing this to u8 can have an impact in future.
>
> My comment was intended much simpler: don't add whitespace between the
> bit-field variable name and its size expression.
>
> u64 mod_type:1;
>
> not
>
> u64 mod_type : 1;
>
> At least, I have not seen that style anywhere else in the kernel.
Got it . Will fix this in next version.
Thanks,
Hariprasad k
Re: [Patch v2 net-next 2/7] octeontx2-af: Add new CGX_CMD to get PHY FEC statistics
On Sat, Jan 30, 2021 at 4:53 AM Hariprasad Kelam wrote:
>
> Hi Willem,
>
> > -Original Message-
> > From: Willem de Bruijn
> > Sent: Thursday, January 28, 2021 1:50 AM
> > To: Hariprasad Kelam
> > Cc: Network Development ; LKML > ker...@vger.kernel.org>; David Miller ; Jakub
> > Kicinski ; Sunil Kovvuri Goutham
> > ; Linu Cherian ;
> > Geethasowjanya Akula ; Jerin Jacob Kollanukkaran
> > ; Subbaraya Sundeep Bhatta ;
> > Felix Manlunas ; Christina Jacob
> > ; Sunil Kovvuri Goutham
> >
> > Subject: [EXT] Re: [Patch v2 net-next 2/7] octeontx2-af: Add new CGX_CMD
> > to get PHY FEC statistics
> >
> > On Wed, Jan 27, 2021 at 4:04 AM Hariprasad Kelam
> > wrote:
> > >
> > > From: Felix Manlunas
> > >
> > > This patch adds support to fetch fec stats from PHY. The stats are put
> > > in the shared data struct fwdata. A PHY driver indicates that it has
> > > FEC stats by setting the flag fwdata.phy.misc.has_fec_stats
> > >
> > > Besides CGX_CMD_GET_PHY_FEC_STATS, also add CGX_CMD_PRBS and
> > > CGX_CMD_DISPLAY_EYE to enum cgx_cmd_id so that Linux's enum list is in
> > > sync with firmware's enum list.
> > >
> > > Signed-off-by: Felix Manlunas
> > > Signed-off-by: Christina Jacob
> > > Signed-off-by: Sunil Kovvuri Goutham
> > > Signed-off-by: Hariprasad Kelam
> >
> >
> > > +struct phy_s {
> > > + struct {
> > > + u64 can_change_mod_type : 1;
> > > + u64 mod_type: 1;
> > > + u64 has_fec_stats : 1;
> >
> > this style is not customary
>
> These structures are shared with firmware and stored in a shared memory. Any
> change in size of structures will break compatibility. To avoid frequent
> compatible issues with new vs old firmware we have put spaces where ever we
> see that there could be more fields added in future.
> So changing this to u8 can have an impact in future.
My comment was intended much simpler: don't add whitespace between the
bit-field variable name and its size expression.
u64 mod_type:1;
not
u64 mod_type : 1;
At least, I have not seen that style anywhere else in the kernel.
Re: [Patch v2 net-next 2/7] octeontx2-af: Add new CGX_CMD to get PHY FEC statistics
Hi Willem,
> -Original Message-
> From: Willem de Bruijn
> Sent: Thursday, January 28, 2021 1:50 AM
> To: Hariprasad Kelam
> Cc: Network Development ; LKML ker...@vger.kernel.org>; David Miller ; Jakub
> Kicinski ; Sunil Kovvuri Goutham
> ; Linu Cherian ;
> Geethasowjanya Akula ; Jerin Jacob Kollanukkaran
> ; Subbaraya Sundeep Bhatta ;
> Felix Manlunas ; Christina Jacob
> ; Sunil Kovvuri Goutham
>
> Subject: [EXT] Re: [Patch v2 net-next 2/7] octeontx2-af: Add new CGX_CMD
> to get PHY FEC statistics
>
> On Wed, Jan 27, 2021 at 4:04 AM Hariprasad Kelam
> wrote:
> >
> > From: Felix Manlunas
> >
> > This patch adds support to fetch fec stats from PHY. The stats are put
> > in the shared data struct fwdata. A PHY driver indicates that it has
> > FEC stats by setting the flag fwdata.phy.misc.has_fec_stats
> >
> > Besides CGX_CMD_GET_PHY_FEC_STATS, also add CGX_CMD_PRBS and
> > CGX_CMD_DISPLAY_EYE to enum cgx_cmd_id so that Linux's enum list is in
> > sync with firmware's enum list.
> >
> > Signed-off-by: Felix Manlunas
> > Signed-off-by: Christina Jacob
> > Signed-off-by: Sunil Kovvuri Goutham
> > Signed-off-by: Hariprasad Kelam
>
>
> > +struct phy_s {
> > + struct {
> > + u64 can_change_mod_type : 1;
> > + u64 mod_type: 1;
> > + u64 has_fec_stats : 1;
>
> this style is not customary
These structures are shared with firmware and stored in a shared memory. Any
change in size of structures will break compatibility. To avoid frequent
compatible issues with new vs old firmware we have put spaces where ever we see
that there could be more fields added in future.
So changing this to u8 can have an impact in future.
>
> > + } misc;
> > + struct fec_stats_s {
> > + u32 rsfec_corr_cws;
> > + u32 rsfec_uncorr_cws;
> > + u32 brfec_corr_blks;
> > + u32 brfec_uncorr_blks;
> > + } fec_stats;
> > +};
> > +
> > +struct cgx_lmac_fwdata_s {
> > + u16 rw_valid;
> > + u64 supported_fec;
> > + u64 supported_an;
>
> are these intended to be individual u64's?
>
The above fields are used as bitmaps to store fec BASER/RS etc.
As stated above to avoid frequent compatible issues between old & new firmware
. we are creating spaces.
> > + u64 supported_link_modes;
> > + /* only applicable if AN is supported */
> > + u64 advertised_fec;
> > + u64 advertised_link_modes;
> > + /* Only applicable if SFP/QSFP slot is present */
> > + struct sfp_eeprom_s sfp_eeprom;
> > + struct phy_s phy;
> > +#define LMAC_FWDATA_RESERVED_MEM 1021
> > + u64 reserved[LMAC_FWDATA_RESERVED_MEM];
> > +};
> > +
> > +struct cgx_fw_data {
> > + struct mbox_msghdr hdr;
> > + struct cgx_lmac_fwdata_s fwdata; };
> > +
> > /* NPA mbox message formats */
> >
> > /* NPA mailbox error codes
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> > b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> > index b1a6ecf..c824f1e 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> > @@ -350,6 +350,10 @@ struct rvu_fwdata {
> > u64 msixtr_base;
> > #define FWDATA_RESERVED_MEM 1023
> > u64 reserved[FWDATA_RESERVED_MEM];
> > + /* Do not add new fields below this line */
> > +#define CGX_MAX 5
> > +#define CGX_LMACS_MAX 4
> > + struct cgx_lmac_fwdata_s
> cgx_fw_data[CGX_MAX][CGX_LMACS_MAX];
>
> Probably want to move the comment below the field.
Agreed will fix this in next version.
Thanks,
Hariprasad k
> > };
> >
> > struct ptp;
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> > b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> > index 74f494b..7fac9ab 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> > @@ -694,6 +694,19 @@ int rvu_mbox_handler_cgx_cfg_pause_frm(struct
> rvu *rvu,
> > return 0;
> > }
> >
> > +int rvu_mbox_handler_cgx_get_phy_fec_stats(struct rvu *rvu, struct
> msg_req *req,
> > + struct msg_rsp *rsp) {
> > + int pf = rvu_get_pf(req->hdr.pcifunc);
> > + u8 cgx_id, lmac_id;
> > +
> > + if (!is_pf_cgxmapped(rvu, pf))
> > +
Re: [Patch v2 net-next 2/7] octeontx2-af: Add new CGX_CMD to get PHY FEC statistics
On Wed, Jan 27, 2021 at 4:04 AM Hariprasad Kelam wrote:
>
> From: Felix Manlunas
>
> This patch adds support to fetch fec stats from PHY. The stats are
> put in the shared data struct fwdata. A PHY driver indicates
> that it has FEC stats by setting the flag fwdata.phy.misc.has_fec_stats
>
> Besides CGX_CMD_GET_PHY_FEC_STATS, also add CGX_CMD_PRBS and
> CGX_CMD_DISPLAY_EYE to enum cgx_cmd_id so that Linux's enum list is in sync
> with firmware's enum list.
>
> Signed-off-by: Felix Manlunas
> Signed-off-by: Christina Jacob
> Signed-off-by: Sunil Kovvuri Goutham
> Signed-off-by: Hariprasad Kelam
> +struct phy_s {
> + struct {
> + u64 can_change_mod_type : 1;
> + u64 mod_type: 1;
> + u64 has_fec_stats : 1;
this style is not customary
> + } misc;
> + struct fec_stats_s {
> + u32 rsfec_corr_cws;
> + u32 rsfec_uncorr_cws;
> + u32 brfec_corr_blks;
> + u32 brfec_uncorr_blks;
> + } fec_stats;
> +};
> +
> +struct cgx_lmac_fwdata_s {
> + u16 rw_valid;
> + u64 supported_fec;
> + u64 supported_an;
are these intended to be individual u64's?
> + u64 supported_link_modes;
> + /* only applicable if AN is supported */
> + u64 advertised_fec;
> + u64 advertised_link_modes;
> + /* Only applicable if SFP/QSFP slot is present */
> + struct sfp_eeprom_s sfp_eeprom;
> + struct phy_s phy;
> +#define LMAC_FWDATA_RESERVED_MEM 1021
> + u64 reserved[LMAC_FWDATA_RESERVED_MEM];
> +};
> +
> +struct cgx_fw_data {
> + struct mbox_msghdr hdr;
> + struct cgx_lmac_fwdata_s fwdata;
> +};
> +
> /* NPA mbox message formats */
>
> /* NPA mailbox error codes
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> index b1a6ecf..c824f1e 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> @@ -350,6 +350,10 @@ struct rvu_fwdata {
> u64 msixtr_base;
> #define FWDATA_RESERVED_MEM 1023
> u64 reserved[FWDATA_RESERVED_MEM];
> + /* Do not add new fields below this line */
> +#define CGX_MAX 5
> +#define CGX_LMACS_MAX 4
> + struct cgx_lmac_fwdata_s cgx_fw_data[CGX_MAX][CGX_LMACS_MAX];
Probably want to move the comment below the field.
> };
>
> struct ptp;
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> index 74f494b..7fac9ab 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> @@ -694,6 +694,19 @@ int rvu_mbox_handler_cgx_cfg_pause_frm(struct rvu *rvu,
> return 0;
> }
>
> +int rvu_mbox_handler_cgx_get_phy_fec_stats(struct rvu *rvu, struct msg_req
> *req,
> + struct msg_rsp *rsp)
> +{
> + int pf = rvu_get_pf(req->hdr.pcifunc);
> + u8 cgx_id, lmac_id;
> +
> + if (!is_pf_cgxmapped(rvu, pf))
> + return -EPERM;
> +
> + rvu_get_cgx_lmac_id(rvu->pf2cgxlmac_map[pf], _id, _id);
> + return cgx_get_phy_fec_stats(rvu_cgx_pdata(cgx_id, rvu), lmac_id);
> +}
> +
> /* Finds cumulative status of NIX rx/tx counters from LF of a PF and those
> * from its VFs as well. ie. NIX rx/tx counters at the CGX port level
> */
> @@ -800,3 +813,22 @@ int rvu_mbox_handler_cgx_set_fec_param(struct rvu *rvu,
> rsp->fec = cgx_set_fec(req->fec, cgx_id, lmac_id);
> return 0;
> }
> +
> +int rvu_mbox_handler_cgx_get_aux_link_info(struct rvu *rvu, struct msg_req
> *req,
> + struct cgx_fw_data *rsp)
> +{
> + int pf = rvu_get_pf(req->hdr.pcifunc);
> + u8 cgx_id, lmac_id;
> +
> + if (!rvu->fwdata)
> + return -ENXIO;
> +
> + if (!is_pf_cgxmapped(rvu, pf))
> + return -EPERM;
> +
> + rvu_get_cgx_lmac_id(rvu->pf2cgxlmac_map[pf], _id, _id);
> +
> + memcpy(>fwdata, >fwdata->cgx_fw_data[cgx_id][lmac_id],
> + sizeof(struct cgx_lmac_fwdata_s));
> + return 0;
> +}
> --
> 2.7.4
>
[Patch v2 net-next 2/7] octeontx2-af: Add new CGX_CMD to get PHY FEC statistics
From: Felix Manlunas
This patch adds support to fetch fec stats from PHY. The stats are
put in the shared data struct fwdata. A PHY driver indicates
that it has FEC stats by setting the flag fwdata.phy.misc.has_fec_stats
Besides CGX_CMD_GET_PHY_FEC_STATS, also add CGX_CMD_PRBS and
CGX_CMD_DISPLAY_EYE to enum cgx_cmd_id so that Linux's enum list is in sync
with firmware's enum list.
Signed-off-by: Felix Manlunas
Signed-off-by: Christina Jacob
Signed-off-by: Sunil Kovvuri Goutham
Signed-off-by: Hariprasad Kelam
---
drivers/net/ethernet/marvell/octeontx2/af/cgx.c| 12 ++
drivers/net/ethernet/marvell/octeontx2/af/cgx.h| 1 +
.../net/ethernet/marvell/octeontx2/af/cgx_fw_if.h | 5 +++
drivers/net/ethernet/marvell/octeontx2/af/mbox.h | 43 ++
drivers/net/ethernet/marvell/octeontx2/af/rvu.h| 4 ++
.../net/ethernet/marvell/octeontx2/af/rvu_cgx.c| 32
6 files changed, 97 insertions(+)
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
index 5489dab..b3ae84c 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
@@ -859,6 +859,18 @@ int cgx_set_fec(u64 fec, int cgx_id, int lmac_id)
return err;
}
+int cgx_get_phy_fec_stats(void *cgxd, int lmac_id)
+{
+ struct cgx *cgx = cgxd;
+ u64 req = 0, resp;
+
+ if (!cgx)
+ return -ENODEV;
+
+ req = FIELD_SET(CMDREG_ID, CGX_CMD_GET_PHY_FEC_STATS, req);
+ return cgx_fwi_cmd_generic(req, , cgx, lmac_id);
+}
+
static int cgx_fwi_link_change(struct cgx *cgx, int lmac_id, bool enable)
{
u64 req = 0;
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cgx.h
b/drivers/net/ethernet/marvell/octeontx2/af/cgx.h
index 1824e95..c5294b7 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/cgx.h
+++ b/drivers/net/ethernet/marvell/octeontx2/af/cgx.h
@@ -154,5 +154,6 @@ void cgx_lmac_ptp_config(void *cgxd, int lmac_id, bool
enable);
u8 cgx_lmac_get_p2x(int cgx_id, int lmac_id);
int cgx_set_fec(u64 fec, int cgx_id, int lmac_id);
int cgx_get_fec_stats(void *cgxd, int lmac_id, struct cgx_fec_stats_rsp *rsp);
+int cgx_get_phy_fec_stats(void *cgxd, int lmac_id);
#endif /* CGX_H */
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 3485596..65f832a 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/cgx_fw_if.h
+++ b/drivers/net/ethernet/marvell/octeontx2/af/cgx_fw_if.h
@@ -89,6 +89,11 @@ enum cgx_cmd_id {
CGX_CMD_SET_AN,
CGX_CMD_GET_ADV_LINK_MODES,
CGX_CMD_GET_ADV_FEC,
+ CGX_CMD_GET_PHY_MOD_TYPE, /* line-side modulation type: NRZ or PAM4 */
+ CGX_CMD_SET_PHY_MOD_TYPE,
+ CGX_CMD_PRBS,
+ CGX_CMD_DISPLAY_EYE,
+ CGX_CMD_GET_PHY_FEC_STATS,
};
/* async event ids */
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
index a59a355..34e61a6 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
+++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
@@ -151,6 +151,8 @@ M(CGX_CFG_PAUSE_FRM,0x20E, cgx_cfg_pause_frm,
cgx_pause_frm_cfg,\
cgx_pause_frm_cfg) \
M(CGX_FEC_SET, 0x210, cgx_set_fec_param, fec_mode, fec_mode) \
M(CGX_FEC_STATS, 0x211, cgx_fec_stats, msg_req, cgx_fec_stats_rsp) \
+M(CGX_GET_PHY_FEC_STATS, 0x212, cgx_get_phy_fec_stats, msg_req, msg_rsp) \
+M(CGX_FW_DATA_GET, 0x213, cgx_get_aux_link_info, msg_req, cgx_fw_data) \
/* NPA mbox IDs (range 0x400 - 0x5FF) */ \
/* NPA mbox IDs (range 0x400 - 0x5FF) */ \
M(NPA_LF_ALLOC,0x400, npa_lf_alloc,
\
@@ -411,6 +413,47 @@ struct fec_mode {
int fec;
};
+struct sfp_eeprom_s {
+#define SFP_EEPROM_SIZE 256
+ u16 sff_id;
+ u8 buf[SFP_EEPROM_SIZE];
+ u64 reserved;
+};
+
+struct phy_s {
+ struct {
+ u64 can_change_mod_type : 1;
+ u64 mod_type: 1;
+ u64 has_fec_stats : 1;
+ } misc;
+ struct fec_stats_s {
+ u32 rsfec_corr_cws;
+ u32 rsfec_uncorr_cws;
+ u32 brfec_corr_blks;
+ u32 brfec_uncorr_blks;
+ } fec_stats;
+};
+
+struct cgx_lmac_fwdata_s {
+ u16 rw_valid;
+ u64 supported_fec;
+ u64 supported_an;
+ u64 supported_link_modes;
+ /* only applicable if AN is supported */
+ u64 advertised_fec;
+ u64 advertised_link_modes;
+ /* Only applicable if SFP/QSFP slot is present */
+ struct sfp_eeprom_s sfp_eeprom;
+ struct phy_s phy;
+#define LMAC_FWDATA_RESERVED_MEM 1021
+ u64 reserved[LMAC_FWDATA_RESERVED_MEM];
+};
+
+struct cgx_fw_data {
+ struct mbox_msghdr hdr;
+
