Message ID | 20241204140821.1858263-2-saikrishnag@marvell.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | CN20K silicon with mbox support | expand |
On Wed, 4 Dec 2024 19:38:16 +0530 Sai Krishna wrote: > -#define RVU_PFVF_PF_SHIFT 10 > -#define RVU_PFVF_PF_MASK 0x3F > -#define RVU_PFVF_FUNC_SHIFT 0 > -#define RVU_PFVF_FUNC_MASK 0x3FF > +#define RVU_PFVF_PF_SHIFT rvu_pcifunc_pf_shift > +#define RVU_PFVF_PF_MASK rvu_pcifunc_pf_mask > +#define RVU_PFVF_FUNC_SHIFT rvu_pcifunc_func_shift > +#define RVU_PFVF_FUNC_MASK rvu_pcifunc_func_mask Why do you maintain these defines? Looks like an unnecessary indirection. Given these are simple mask and shift values they probably have trivial users. Start by adding helpers which perform the conversions using those, then you can more easily update constants.
Hi Jakub, >From: Jakub Kicinski <kuba@kernel.org> >Sent: Sunday, December 8, 2024 8:08 AM >To: Sai Krishna Gajula <saikrishnag@marvell.com> >Cc: davem@davemloft.net; edumazet@google.com; pabeni@redhat.com; >netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Sunil Kovvuri Goutham ><sgoutham@marvell.com>; Geethasowjanya Akula <gakula@marvell.com>; >Linu Cherian <lcherian@marvell.com>; Jerin Jacob <jerinj@marvell.com>; >Hariprasad Kelam <hkelam@marvell.com>; Subbaraya Sundeep Bhatta ><sbhatta@marvell.com>; andrew+netdev@lunn.ch; kalesh- >anakkur.purayil@broadcom.com >Subject: Re: [net-next PATCH v5 1/6] octeontx2: Set appropriate PF, VF masks >and shifts based on silicon > > >On Wed, 4 Dec 2024 19:38:16 +0530 Sai Krishna wrote: >> -#define RVU_PFVF_PF_SHIFT 10 >> -#define RVU_PFVF_PF_MASK 0x3F >> -#define RVU_PFVF_FUNC_SHIFT 0 >> -#define RVU_PFVF_FUNC_MASK 0x3FF >> +#define RVU_PFVF_PF_SHIFT rvu_pcifunc_pf_shift >> +#define RVU_PFVF_PF_MASK rvu_pcifunc_pf_mask >> +#define RVU_PFVF_FUNC_SHIFT rvu_pcifunc_func_shift >> +#define RVU_PFVF_FUNC_MASK rvu_pcifunc_func_mask > >Why do you maintain these defines? Looks like an unnecessary >indirection. > >Given these are simple mask and shift values they probably have trivial >users. Start by adding helpers which perform the conversions using >those, then you can more easily update constants. > There are too many places these masks are used hence added this indirection. # grep RVU_PFVF_ drivers/* -inr | wc -l 135 Thanks, Sundeep
On Mon, 9 Dec 2024 09:09:35 +0000 Subbaraya Sundeep Bhatta wrote: > >On Wed, 4 Dec 2024 19:38:16 +0530 Sai Krishna wrote: > >> -#define RVU_PFVF_PF_SHIFT 10 > >> -#define RVU_PFVF_PF_MASK 0x3F > >> -#define RVU_PFVF_FUNC_SHIFT 0 > >> -#define RVU_PFVF_FUNC_MASK 0x3FF > >> +#define RVU_PFVF_PF_SHIFT rvu_pcifunc_pf_shift > >> +#define RVU_PFVF_PF_MASK rvu_pcifunc_pf_mask > >> +#define RVU_PFVF_FUNC_SHIFT rvu_pcifunc_func_shift > >> +#define RVU_PFVF_FUNC_MASK rvu_pcifunc_func_mask > > > >Why do you maintain these defines? Looks like an unnecessary > >indirection. > > > >Given these are simple mask and shift values they probably have trivial > >users. Start by adding helpers which perform the conversions using > >those, then you can more easily update constants. > > There are too many places these masks are used hence added this > indirection. > # grep RVU_PFVF_ drivers/* -inr | wc -l > 135 Yes, I have checked before making the suggestion. Add a helper first, you can use cocci to do the conversions.
Hi Jakub, >From: Jakub Kicinski <mailto:kuba@kernel.org> >Sent: Tuesday, December 10, 2024 2:55 AM >To: Subbaraya Sundeep Bhatta <mailto:sbhatta@marvell.com> >Cc: Sai Krishna Gajula <mailto:saikrishnag@marvell.com>; >mailto:davem@davemloft.net; mailto:edumazet@google.com; >mailto:pabeni@redhat.com; mailto:netdev@vger.kernel.org; mailto:linux- >kernel@vger.kernel.org; Sunil Kovvuri Goutham <mailto:sgoutham@marvell.com>; >Geethasowjanya Akula <mailto:gakula@marvell.com>; Linu Cherian ><mailto:lcherian@marvell.com>; Jerin Jacob <mailto:jerinj@marvell.com>; >Hariprasad Kelam <mailto:hkelam@marvell.com>; >mailto:andrew+netdev@lunn.ch; mailto:kalesh-anakkur.purayil@broadcom.com >Subject: Re: [EXTERNAL] Re: [net-next PATCH v5 1/6] octeontx2: Set appropriate >PF, VF masks and shifts based on silicon >> >> -#define RVU_PFVF_PF_SHIFT 10 >> >> -#define RVU_PFVF_PF_MASK 0x3F >> >> -#define RVU_PFVF_FUNC_SHIFT 0 >> >> -#define RVU_PFVF_FUNC_MASK 0x3FF >> >> +#define RVU_PFVF_PF_SHIFT rvu_pcifunc_pf_shift >> >> +#define RVU_PFVF_PF_MASK rvu_pcifunc_pf_mask >> >> +#define RVU_PFVF_FUNC_SHIFT rvu_pcifunc_func_shift >> >> +#define RVU_PFVF_FUNC_MASK rvu_pcifunc_func_mask >> > >> >Why do you maintain these defines? Looks like an unnecessary >> >indirection. >> > >> >Given these are simple mask and shift values they probably have trivial >> >users. Start by adding helpers which perform the conversions using >> >those, then you can more easily update constants. >> >> There are too many places these masks are used hence added this >> indirection. >> # grep RVU_PFVF_ drivers/* -inr | wc -l >> 135 > >Yes, I have checked before making the suggestion. >Add a helper first, you can use cocci to do the conversions. Initially I thought of doing similar kind of changes by visiting each line using these masks. But patch would be much bigger and also to avoid any regression issues for existing silicons and crypto driver, I thought this would be safe and simple. Please let me know if this is still not okay for you. Thanks, Sundeep
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.c b/drivers/net/ethernet/marvell/octeontx2/af/mbox.c index 1e5aa5397504..791c468a10c5 100644 --- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.c +++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.c @@ -13,6 +13,21 @@ #include "mbox.h" #include "rvu_trace.h" +/* Default values of PF and VF bit encodings in PCIFUNC for + * CN9XXX and CN10K series silicons. + */ +u16 rvu_pcifunc_pf_shift = 10; +EXPORT_SYMBOL(rvu_pcifunc_pf_shift); + +u16 rvu_pcifunc_pf_mask = 0x3F; +EXPORT_SYMBOL(rvu_pcifunc_pf_mask); + +u16 rvu_pcifunc_func_shift; +EXPORT_SYMBOL(rvu_pcifunc_func_shift); + +u16 rvu_pcifunc_func_mask = 0x3FF; +EXPORT_SYMBOL(rvu_pcifunc_func_mask); + static const u16 msgs_offset = ALIGN(sizeof(struct mbox_hdr), MBOX_MSG_ALIGN); void __otx2_mbox_reset(struct otx2_mbox *mbox, int devid) diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h index 62c07407eb94..9969615fb994 100644 --- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h +++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h @@ -50,6 +50,11 @@ #define MBOX_DIR_PFVF_UP 6 /* PF sends messages to VF */ #define MBOX_DIR_VFPF_UP 7 /* VF replies to PF */ +extern u16 rvu_pcifunc_pf_shift; +extern u16 rvu_pcifunc_pf_mask; +extern u16 rvu_pcifunc_func_shift; +extern u16 rvu_pcifunc_func_mask; + struct otx2_mbox_dev { void *mbase; /* This dev's mbox region */ void *hwbase; diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c index cd0d7b7774f1..7c0f520d658b 100644 --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c @@ -400,11 +400,6 @@ static void rvu_update_rsrc_map(struct rvu *rvu, struct rvu_pfvf *pfvf, rvu_write64(rvu, BLKADDR_RVUM, reg | (devnum << 16), num_lfs); } -inline int rvu_get_pf(u16 pcifunc) -{ - return (pcifunc >> RVU_PFVF_PF_SHIFT) & RVU_PFVF_PF_MASK; -} - void rvu_get_pf_numvfs(struct rvu *rvu, int pf, int *numvfs, int *hwvf) { u64 cfg; diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h index a383b5ef5b2d..0f5017e93c6f 100644 --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h @@ -41,10 +41,10 @@ #define MAX_CPT_BLKS 2 /* PF_FUNC */ -#define RVU_PFVF_PF_SHIFT 10 -#define RVU_PFVF_PF_MASK 0x3F -#define RVU_PFVF_FUNC_SHIFT 0 -#define RVU_PFVF_FUNC_MASK 0x3FF +#define RVU_PFVF_PF_SHIFT rvu_pcifunc_pf_shift +#define RVU_PFVF_PF_MASK rvu_pcifunc_pf_mask +#define RVU_PFVF_FUNC_SHIFT rvu_pcifunc_func_shift +#define RVU_PFVF_FUNC_MASK rvu_pcifunc_func_mask #ifdef CONFIG_DEBUG_FS struct dump_ctx { @@ -834,7 +834,6 @@ int rvu_alloc_rsrc_contig(struct rsrc_bmap *rsrc, int nrsrc); void rvu_free_rsrc_contig(struct rsrc_bmap *rsrc, int nrsrc, int start); bool rvu_rsrc_check_contig(struct rsrc_bmap *rsrc, int nrsrc); u16 rvu_get_rsrc_mapcount(struct rvu_pfvf *pfvf, int blkaddr); -int rvu_get_pf(u16 pcifunc); struct rvu_pfvf *rvu_get_pfvf(struct rvu *rvu, int pcifunc); void rvu_get_pf_numvfs(struct rvu *rvu, int pf, int *numvfs, int *hwvf); bool is_block_implemented(struct rvu_hwinfo *hw, int blkaddr); @@ -875,6 +874,11 @@ static inline bool is_rep_dev(struct rvu *rvu, u16 pcifunc) return false; } +static inline int rvu_get_pf(u16 pcifunc) +{ + return (pcifunc >> RVU_PFVF_PF_SHIFT) & RVU_PFVF_PF_MASK; +} + /* CGX APIs */ static inline bool is_pf_cgxmapped(struct rvu *rvu, u8 pf) { diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h index 566848663fea..4681df652ff1 100644 --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h @@ -27,6 +27,7 @@ #include "otx2_reg.h" #include "otx2_txrx.h" #include "otx2_devlink.h" +#include <rvu.h> #include <rvu_trace.h> #include "qos.h" #include "rep.h" @@ -869,21 +870,11 @@ MBOX_UP_MCS_MESSAGES /* Time to wait before watchdog kicks off */ #define OTX2_TX_TIMEOUT (100 * HZ) -#define RVU_PFVF_PF_SHIFT 10 -#define RVU_PFVF_PF_MASK 0x3F -#define RVU_PFVF_FUNC_SHIFT 0 -#define RVU_PFVF_FUNC_MASK 0x3FF - static inline bool is_otx2_vf(u16 pcifunc) { return !!(pcifunc & RVU_PFVF_FUNC_MASK); } -static inline int rvu_get_pf(u16 pcifunc) -{ - return (pcifunc >> RVU_PFVF_PF_SHIFT) & RVU_PFVF_PF_MASK; -} - static inline dma_addr_t otx2_dma_map_page(struct otx2_nic *pfvf, struct page *page, size_t offset, size_t size, diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_reg.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_reg.h index e3aee6e36215..858f084b9d47 100644 --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_reg.h +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_reg.h @@ -138,36 +138,6 @@ #define NIX_LF_CINTX_ENA_W1S(a) (NIX_LFBASE | 0xD40 | (a) << 12) #define NIX_LF_CINTX_ENA_W1C(a) (NIX_LFBASE | 0xD50 | (a) << 12) -/* NIX AF transmit scheduler registers */ -#define NIX_AF_SMQX_CFG(a) (0x700 | (u64)(a) << 16) -#define NIX_AF_TL4X_SDP_LINK_CFG(a) (0xB10 | (u64)(a) << 16) -#define NIX_AF_TL1X_SCHEDULE(a) (0xC00 | (u64)(a) << 16) -#define NIX_AF_TL1X_CIR(a) (0xC20 | (u64)(a) << 16) -#define NIX_AF_TL1X_TOPOLOGY(a) (0xC80 | (u64)(a) << 16) -#define NIX_AF_TL2X_PARENT(a) (0xE88 | (u64)(a) << 16) -#define NIX_AF_TL2X_SCHEDULE(a) (0xE00 | (u64)(a) << 16) -#define NIX_AF_TL2X_TOPOLOGY(a) (0xE80 | (u64)(a) << 16) -#define NIX_AF_TL2X_CIR(a) (0xE20 | (u64)(a) << 16) -#define NIX_AF_TL2X_PIR(a) (0xE30 | (u64)(a) << 16) -#define NIX_AF_TL3X_PARENT(a) (0x1088 | (u64)(a) << 16) -#define NIX_AF_TL3X_SCHEDULE(a) (0x1000 | (u64)(a) << 16) -#define NIX_AF_TL3X_SHAPE(a) (0x1010 | (u64)(a) << 16) -#define NIX_AF_TL3X_CIR(a) (0x1020 | (u64)(a) << 16) -#define NIX_AF_TL3X_PIR(a) (0x1030 | (u64)(a) << 16) -#define NIX_AF_TL3X_TOPOLOGY(a) (0x1080 | (u64)(a) << 16) -#define NIX_AF_TL4X_PARENT(a) (0x1288 | (u64)(a) << 16) -#define NIX_AF_TL4X_SCHEDULE(a) (0x1200 | (u64)(a) << 16) -#define NIX_AF_TL4X_SHAPE(a) (0x1210 | (u64)(a) << 16) -#define NIX_AF_TL4X_CIR(a) (0x1220 | (u64)(a) << 16) -#define NIX_AF_TL4X_PIR(a) (0x1230 | (u64)(a) << 16) -#define NIX_AF_TL4X_TOPOLOGY(a) (0x1280 | (u64)(a) << 16) -#define NIX_AF_MDQX_SCHEDULE(a) (0x1400 | (u64)(a) << 16) -#define NIX_AF_MDQX_SHAPE(a) (0x1410 | (u64)(a) << 16) -#define NIX_AF_MDQX_CIR(a) (0x1420 | (u64)(a) << 16) -#define NIX_AF_MDQX_PIR(a) (0x1430 | (u64)(a) << 16) -#define NIX_AF_MDQX_PARENT(a) (0x1480 | (u64)(a) << 16) -#define NIX_AF_TL3_TL2X_LINKX_CFG(a, b) (0x1700 | (u64)(a) << 16 | (b) << 3) - /* LMT LF registers */ #define LMT_LFBASE BIT_ULL(RVU_FUNC_BLKADDR_SHIFT) #define LMT_LF_LMTLINEX(a) (LMT_LFBASE | 0x000 | (a) << 12)