Re: [Patch v2 net-next 2/7] octeontx2-af: Add new CGX_CMD to get PHY FEC statistics

* Re: [Patch v2 net-next 2/7] octeontx2-af: Add new CGX_CMD to get PHY FEC statistics
@ 2021-01-30  9:52 Hariprasad Kelam
  2021-01-30 14:26 ` Willem de Bruijn
  0 siblings, 1 reply; 5+ messages in thread
From: Hariprasad Kelam @ 2021-01-30  9:52 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Network Development, LKML, David Miller, Jakub Kicinski,
	Sunil Kovvuri Goutham, Linu Cherian, Geethasowjanya Akula,
	Jerin Jacob Kollanukkaran, Subbaraya Sundeep Bhatta,
	Felix Manlunas, Christina Jacob, Sunil Kovvuri Goutham

Hi Willem,

> -----Original Message-----
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Sent: Thursday, January 28, 2021 1:50 AM
> To: Hariprasad Kelam <hkelam@marvell.com>
> Cc: Network Development <netdev@vger.kernel.org>; LKML <linux-
> kernel@vger.kernel.org>; David Miller <davem@davemloft.net>; Jakub
> Kicinski <kuba@kernel.org>; Sunil Kovvuri Goutham
> <sgoutham@marvell.com>; Linu Cherian <lcherian@marvell.com>;
> Geethasowjanya Akula <gakula@marvell.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Subbaraya Sundeep Bhatta <sbhatta@marvell.com>;
> Felix Manlunas <fmanlunas@marvell.com>; Christina Jacob
> <cjacob@marvell.com>; Sunil Kovvuri Goutham
> <Sunil.Goutham@cavium.com>
> 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 <hkelam@marvell.com>
> wrote:
> >
> > From: Felix Manlunas <fmanlunas@marvell.com>
> >
> > 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 <fmanlunas@marvell.com>
> > Signed-off-by: Christina Jacob <cjacob@marvell.com>
> > Signed-off-by: Sunil Kovvuri Goutham <Sunil.Goutham@cavium.com>
> > Signed-off-by: Hariprasad Kelam <hkelam@marvell.com>
> 
> 
> > +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))
> > +               return -EPERM;
> > +
> > +       rvu_get_cgx_lmac_id(rvu->pf2cgxlmac_map[pf], &cgx_id, &lmac_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], &cgx_id,
> > + &lmac_id);
> > +
> > +       memcpy(&rsp->fwdata, &rvu->fwdata-
> >cgx_fw_data[cgx_id][lmac_id],
> > +              sizeof(struct cgx_lmac_fwdata_s));
> > +       return 0;
> > +}
> > --
> > 2.7.4
> >

^ permalink raw reply	[flat|nested] 5+ messages in thread