Re: [PATCH v2 03/32] elx: libefc_sli: Data structures and defines for mbox commands

[Hannes Reinecke <hare@suse.de>]

On 12/20/19 11:36 PM, James Smart wrote:
> This patch continues the libefc_sli SLI-4 library population.
> 
> This patch adds definitions for SLI-4 mailbox commands
> and responses.
> 
> Signed-off-by: Ram Vegesna <ram.vegesna@broadcom.com>
> Signed-off-by: James Smart <jsmart2021@gmail.com>
> ---
>  drivers/scsi/elx/libefc_sli/sli4.h | 1728 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 1727 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/elx/libefc_sli/sli4.h b/drivers/scsi/elx/libefc_sli/sli4.h
> index f86a9e72ed43..c9bd3f71b27b 100644
> --- a/drivers/scsi/elx/libefc_sli/sli4.h
> +++ b/drivers/scsi/elx/libefc_sli/sli4.h
> @@ -1995,7 +1995,7 @@ struct sli4_fc_xri_aborted_cqe {
>  #define SLI4_ELS_REQUEST64_DIR_READ		0x1
>  
>  #define SLI4_ELS_REQUEST64_OTHER		0x0
> -#define SLI4_ELS_REQUEST64_LOGO		0x1
> +#define SLI4_ELS_REQUEST64_LOGO			0x1
>  #define SLI4_ELS_REQUEST64_FDISC		0x2
>  #define SLI4_ELS_REQUEST64_FLOGIN		0x3
>  #define SLI4_ELS_REQUEST64_PLOGI		0x4
Shouldn't this rather be merged with the previous patch?

> @@ -2004,4 +2004,1730 @@ struct sli4_fc_xri_aborted_cqe {
>  #define SLI4_ELS_REQUEST64_CMD_NON_FABRIC	0x0c
>  #define SLI4_ELS_REQUEST64_CMD_FABRIC		0x0d
>  
> +#define SLI_PAGE_SIZE				(1 << 12)	/* 4096 */
> +#define SLI_SUB_PAGE_MASK			(SLI_PAGE_SIZE - 1)
> +#define SLI_ROUND_PAGE(b)	(((b) + SLI_SUB_PAGE_MASK) & ~SLI_SUB_PAGE_MASK)
> +
> +#define SLI4_BMBX_TIMEOUT_MSEC			30000
> +#define SLI4_FW_READY_TIMEOUT_MSEC		30000
> +
> +#define SLI4_BMBX_DELAY_US			1000	/* 1 ms */
> +#define SLI4_INIT_PORT_DELAY_US			10000	/* 10 ms */
> +
> +static inline u32
> +sli_page_count(size_t bytes, u32 page_size)
> +{
> +	if (!page_size)
> +		return 0;
> +
> +	return (bytes + (page_size - 1)) >> __ffs(page_size);
> +}
> +
> +/*************************************************************************
> + * SLI-4 mailbox command formats and definitions
> + */
> +
> +struct sli4_mbox_command_header {
> +	u8	resvd0;
> +	u8	command;
> +	__le16	status;	/* Port writes to indicate success/fail */
> +};
> +
> +enum {
> +	MBX_CMD_CONFIG_LINK	= 0x07,
> +	MBX_CMD_DUMP		= 0x17,
> +	MBX_CMD_DOWN_LINK	= 0x06,
> +	MBX_CMD_INIT_LINK	= 0x05,
> +	MBX_CMD_INIT_VFI	= 0xa3,
> +	MBX_CMD_INIT_VPI	= 0xa4,
> +	MBX_CMD_POST_XRI	= 0xa7,
> +	MBX_CMD_RELEASE_XRI	= 0xac,
> +	MBX_CMD_READ_CONFIG	= 0x0b,
> +	MBX_CMD_READ_STATUS	= 0x0e,
> +	MBX_CMD_READ_NVPARMS	= 0x02,
> +	MBX_CMD_READ_REV	= 0x11,
> +	MBX_CMD_READ_LNK_STAT	= 0x12,
> +	MBX_CMD_READ_SPARM64	= 0x8d,
> +	MBX_CMD_READ_TOPOLOGY	= 0x95,
> +	MBX_CMD_REG_FCFI	= 0xa0,
> +	MBX_CMD_REG_FCFI_MRQ	= 0xaf,
> +	MBX_CMD_REG_RPI		= 0x93,
> +	MBX_CMD_REG_RX_RQ	= 0xa6,
> +	MBX_CMD_REG_VFI		= 0x9f,
> +	MBX_CMD_REG_VPI		= 0x96,
> +	MBX_CMD_RQST_FEATURES	= 0x9d,
> +	MBX_CMD_SLI_CONFIG	= 0x9b,
> +	MBX_CMD_UNREG_FCFI	= 0xa2,
> +	MBX_CMD_UNREG_RPI	= 0x14,
> +	MBX_CMD_UNREG_VFI	= 0xa1,
> +	MBX_CMD_UNREG_VPI	= 0x97,
> +	MBX_CMD_WRITE_NVPARMS	= 0x03,
> +	MBX_CMD_CFG_AUTO_XFER_RDY = 0xAD,
> +
> +	MBX_STATUS_SUCCESS	= 0x0000,
> +	MBX_STATUS_FAILURE	= 0x0001,
> +	MBX_STATUS_RPI_NOT_REG	= 0x1400,
> +};
> +
Make this two enums, one 'enum sli4_mbx_cmd' and one 'enum sli4_mbx_status'.

> +/* CONFIG_LINK */
> +enum {
> +	SLI4_CFG_LINK_BBSCN = 0xf00,
> +	SLI4_CFG_LINK_CSCN  = 0x1000,
> +};
> +
> +struct sli4_cmd_config_link {
> +	struct sli4_mbox_command_header	hdr;
> +	u8		maxbbc;
> +	u8		rsvd5;
> +	u8		rsvd6;
> +	u8		rsvd7;
> +	u8		alpa;
> +	__le16		n_port_id;
> +	u8		rsvd11;
> +	__le32		rsvd12;
> +	__le32		e_d_tov;
> +	__le32		lp_tov;
> +	__le32		r_a_tov;
> +	__le32		r_t_tov;
> +	__le32		al_tov;
> +	__le32		rsvd36;
> +	__le32		bbscn_dword;
> +};
> +
> +enum {
> +	SLI4_DUMP4_TYPE = 0xf,
> +};

Single enum should rather be converted into a #define ..

> +
> +#define SLI4_WKI_TAG_SAT_TEM 0x1040
> +
> +struct sli4_cmd_dump4 {
> +	struct sli4_mbox_command_header	hdr;
> +	__le32		type_dword;
> +	__le16		wki_selection;
> +	__le16		rsvd10;
> +	__le32		rsvd12;
> +	__le32		returned_byte_cnt;
> +	__le32		resp_data[59];
> +};
> +
> +/* INIT_LINK - initialize the link for a FC port */
> +#define FC_TOPOLOGY_FCAL	0
> +#define FC_TOPOLOGY_P2P		1
> +
> +#define SLI4_INIT_LINK_F_LOOP_BACK	(1 << 0)
> +#define SLI4_INIT_LINK_F_UNFAIR		(1 << 6)
> +#define SLI4_INIT_LINK_F_NO_LIRP	(1 << 7)
> +#define SLI4_INIT_LINK_F_LOOP_VALID_CHK	(1 << 8)
> +#define SLI4_INIT_LINK_F_NO_LISA	(1 << 9)
> +#define SLI4_INIT_LINK_F_FAIL_OVER	(1 << 10)
> +#define SLI4_INIT_LINK_F_NO_AUTOSPEED	(1 << 11)
> +#define SLI4_INIT_LINK_F_PICK_HI_ALPA	(1 << 15)
> +
> +#define SLI4_INIT_LINK_F_P2P_ONLY	1
> +#define SLI4_INIT_LINK_F_FCAL_ONLY	2
> +
> +#define SLI4_INIT_LINK_F_FCAL_FAIL_OVER	0
> +#define SLI4_INIT_LINK_F_P2P_FAIL_OVER	1
> +
> +enum {
> +	SLI4_INIT_LINK_SEL_RESET_AL_PA		= 0xff,
> +	SLI4_INIT_LINK_FLAG_LOOPBACK		= 0x1,
> +	SLI4_INIT_LINK_FLAG_TOPOLOGY		= 0x6,
> +	SLI4_INIT_LINK_FLAG_UNFAIR		= 0x40,
> +	SLI4_INIT_LINK_FLAG_SKIP_LIRP_LILP	= 0x80,
> +	SLI4_INIT_LINK_FLAG_LOOP_VALIDITY	= 0x100,
> +	SLI4_INIT_LINK_FLAG_SKIP_LISA		= 0x200,
> +	SLI4_INIT_LINK_FLAG_EN_TOPO_FAILOVER	= 0x400,
> +	SLI4_INIT_LINK_FLAG_FIXED_SPEED		= 0x800,
> +	SLI4_INIT_LINK_FLAG_SEL_HIGHTEST_AL_PA	= 0x8000,
> +};
> +
Why is this an enum, and the above SLI4_INIT_LINK_F_XXX value are defines?
Please be consistent.

And this applies throughout the remainder of the patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer