[v3,19/27] net/nfp: refact the nsp module

Message ID 20230915091551.1459606-20-chaoyong.he@corigine.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series refact the nfpcore module |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Chaoyong He Sept. 15, 2023, 9:15 a.m. UTC
  Move the definition of data structure into the implement file.
Also sync the logic from kernel driver and remove the unneeded header
file include statements.

Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
---
 drivers/net/nfp/nfp_ethdev.c           |   2 +-
 drivers/net/nfp/nfpcore/nfp_nsp.c      | 390 +++++++++++++++++++------
 drivers/net/nfp/nfpcore/nfp_nsp.h      | 140 ++++-----
 drivers/net/nfp/nfpcore/nfp_nsp_cmds.c |   4 -
 drivers/net/nfp/nfpcore/nfp_nsp_eth.c  |  79 ++---
 5 files changed, 398 insertions(+), 217 deletions(-)
  

Comments

Ferruh Yigit Sept. 18, 2023, 12:31 p.m. UTC | #1
On 9/15/2023 10:15 AM, Chaoyong He wrote:
> Move the definition of data structure into the implement file.
> Also sync the logic from kernel driver and remove the unneeded header
> file include statements.
> 
> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>

<...>

> @@ -381,6 +478,43 @@ nfp_nsp_command_buf(struct nfp_nsp *nsp,
>  	return ret;
>  }
>  
> +#define SZ_1M 0x00100000
> +#define SZ_4K 0x00001000
> +
> +static int
> +nfp_nsp_command_buf(struct nfp_nsp *nsp,
> +		struct nfp_nsp_command_buf_arg *arg)
> +{
> +	int err;
> +	uint64_t reg;
> +	uint32_t size;
> +	uint32_t max_size;
>

One of the previous patches, patch 3/27, updates variable type
'unsigned int max_size;' -> 'size_t max_size;'

Now it is updated again to 'uint32_t max_size;', I assume by mistake
that previous patch updated by this one didn't.

If it is by mistake can you please check all patches that moves code, if
they are aligned with previous cleanups.
Or if this is intentional, can you please update the first patch to set
the variable correct at first place?
  
Ferruh Yigit Sept. 18, 2023, 12:36 p.m. UTC | #2
On 9/18/2023 1:31 PM, Ferruh Yigit wrote:
> On 9/15/2023 10:15 AM, Chaoyong He wrote:
>> Move the definition of data structure into the implement file.
>> Also sync the logic from kernel driver and remove the unneeded header
>> file include statements.
>>
>> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
>> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> 
> <...>
> 
>> @@ -381,6 +478,43 @@ nfp_nsp_command_buf(struct nfp_nsp *nsp,
>>  	return ret;
>>  }
>>  
>> +#define SZ_1M 0x00100000
>> +#define SZ_4K 0x00001000
>> +
>> +static int
>> +nfp_nsp_command_buf(struct nfp_nsp *nsp,
>> +		struct nfp_nsp_command_buf_arg *arg)
>> +{
>> +	int err;
>> +	uint64_t reg;
>> +	uint32_t size;
>> +	uint32_t max_size;
>>
> 
> One of the previous patches, patch 3/27, updates variable type
> 'unsigned int max_size;' -> 'size_t max_size;'
> 
> Now it is updated again to 'uint32_t max_size;', I assume by mistake
> that previous patch updated by this one didn't.
> 
> If it is by mistake can you please check all patches that moves code, if
> they are aligned with previous cleanups.
> Or if this is intentional, can you please update the first patch to set
> the variable correct at first place?
> 

This comment is for v4, I replied to wrong version.
  

Patch

diff --git a/drivers/net/nfp/nfp_ethdev.c b/drivers/net/nfp/nfp_ethdev.c
index 2e43055fd5..9243191de3 100644
--- a/drivers/net/nfp/nfp_ethdev.c
+++ b/drivers/net/nfp/nfp_ethdev.c
@@ -661,7 +661,7 @@  nfp_net_init(struct rte_eth_dev *eth_dev)
 static int
 nfp_fw_upload(struct rte_pci_device *dev, struct nfp_nsp *nsp, char *card)
 {
-	struct nfp_cpp *cpp = nsp->cpp;
+	struct nfp_cpp *cpp = nfp_nsp_cpp(nsp);
 	void *fw_buf;
 	char fw_name[125];
 	char serial[40];
diff --git a/drivers/net/nfp/nfpcore/nfp_nsp.c b/drivers/net/nfp/nfpcore/nfp_nsp.c
index 8e65064b10..75d13cb84f 100644
--- a/drivers/net/nfp/nfpcore/nfp_nsp.c
+++ b/drivers/net/nfp/nfpcore/nfp_nsp.c
@@ -3,20 +3,127 @@ 
  * All rights reserved.
  */
 
-#define NFP_SUBSYS "nfp_nsp"
-
-#include <stdio.h>
-#include <time.h>
+#include "nfp_nsp.h"
 
 #include <rte_common.h>
 
-#include "nfp_cpp.h"
 #include "nfp_logs.h"
-#include "nfp_nsp.h"
 #include "nfp_platform.h"
 #include "nfp_resource.h"
 
-int
+/* Offsets relative to the CSR base */
+#define NSP_STATUS              0x00
+#define   NSP_STATUS_MAGIC      GENMASK_ULL(63, 48)
+#define   NSP_STATUS_MAJOR      GENMASK_ULL(47, 44)
+#define   NSP_STATUS_MINOR      GENMASK_ULL(43, 32)
+#define   NSP_STATUS_CODE       GENMASK_ULL(31, 16)
+#define   NSP_STATUS_RESULT     GENMASK_ULL(15, 8)
+#define   NSP_STATUS_BUSY       RTE_BIT64(0)
+
+#define NSP_COMMAND             0x08
+#define   NSP_COMMAND_OPTION    GENMASK_ULL(63, 32)
+#define   NSP_COMMAND_CODE      GENMASK_ULL(31, 16)
+#define   NSP_COMMAND_DMA_BUF   RTE_BIT64(1)
+#define   NSP_COMMAND_START     RTE_BIT64(0)
+
+/* CPP address to retrieve the data from */
+#define NSP_BUFFER              0x10
+#define   NSP_BUFFER_CPP        GENMASK_ULL(63, 40)
+#define   NSP_BUFFER_ADDRESS    GENMASK_ULL(39, 0)
+
+#define NSP_DFLT_BUFFER         0x18
+#define   NSP_DFLT_BUFFER_CPP          GENMASK_ULL(63, 40)
+#define   NSP_DFLT_BUFFER_ADDRESS      GENMASK_ULL(39, 0)
+
+#define NSP_DFLT_BUFFER_CONFIG  0x20
+#define   NSP_DFLT_BUFFER_SIZE_4KB     GENMASK_ULL(15, 8)
+#define   NSP_DFLT_BUFFER_SIZE_MB      GENMASK_ULL(7, 0)
+
+#define NSP_MAGIC               0xab10
+#define NSP_MAJOR               0
+#define NSP_MINOR               8
+
+#define NSP_CODE_MAJOR          GENMASK_ULL(15, 12)
+#define NSP_CODE_MINOR          GENMASK_ULL(11, 0)
+
+#define NFP_FW_LOAD_RET_MAJOR   GENMASK_ULL(15, 8)
+#define NFP_FW_LOAD_RET_MINOR   GENMASK_ULL(23, 16)
+
+enum nfp_nsp_cmd {
+	SPCODE_NOOP             = 0, /* No operation */
+	SPCODE_SOFT_RESET       = 1, /* Soft reset the NFP */
+	SPCODE_FW_DEFAULT       = 2, /* Load default (UNDI) FW */
+	SPCODE_PHY_INIT         = 3, /* Initialize the PHY */
+	SPCODE_MAC_INIT         = 4, /* Initialize the MAC */
+	SPCODE_PHY_RXADAPT      = 5, /* Re-run PHY RX Adaptation */
+	SPCODE_FW_LOAD          = 6, /* Load fw from buffer, len in option */
+	SPCODE_ETH_RESCAN       = 7, /* Rescan ETHs, write ETH_TABLE to buf */
+	SPCODE_ETH_CONTROL      = 8, /* Update media config from buffer */
+	SPCODE_NSP_WRITE_FLASH  = 11, /* Load and flash image from buffer */
+	SPCODE_NSP_SENSORS      = 12, /* Read NSP sensor(s) */
+	SPCODE_NSP_IDENTIFY     = 13, /* Read NSP version */
+	SPCODE_FW_STORED        = 16, /* If no FW loaded, load flash app FW */
+	SPCODE_HWINFO_LOOKUP    = 17, /* Lookup HWinfo with overwrites etc. */
+	SPCODE_HWINFO_SET       = 18, /* Set HWinfo entry */
+	SPCODE_FW_LOADED        = 19, /* Is application firmware loaded */
+	SPCODE_VERSIONS         = 21, /* Report FW versions */
+	SPCODE_READ_SFF_EEPROM  = 22, /* Read module EEPROM */
+	SPCODE_READ_MEDIA       = 23, /* Get the supported/advertised media for a port */
+};
+
+static const struct {
+	uint32_t code;
+	const char *msg;
+} nsp_errors[] = {
+	{ 6010, "could not map to phy for port" },
+	{ 6011, "not an allowed rate/lanes for port" },
+	{ 6012, "not an allowed rate/lanes for port" },
+	{ 6013, "high/low error, change other port first" },
+	{ 6014, "config not found in flash" },
+};
+
+struct nfp_nsp {
+	struct nfp_cpp *cpp;
+	struct nfp_resource *res;
+	struct {
+		uint16_t major;
+		uint16_t minor;
+	} ver;
+
+	/** Eth table config state */
+	bool modified;
+	uint32_t idx;
+	void *entries;
+};
+
+/* NFP command argument structure */
+struct nfp_nsp_command_arg {
+	uint16_t code;         /**< NFP SP Command Code */
+	bool dma;              /**< @buf points to a host buffer, not NSP buffer */
+	bool error_quiet;      /**< Don't print command error/warning */
+	uint32_t timeout_sec;  /**< Timeout value to wait for completion in seconds */
+	uint32_t option;       /**< NSP Command Argument */
+	uint64_t buf;          /**< NSP Buffer Address */
+	/** Callback for interpreting option if error occurred */
+	void (*error_cb)(struct nfp_nsp *state, uint32_t ret_val);
+};
+
+/* NFP command with buffer argument structure */
+struct nfp_nsp_command_buf_arg {
+	struct nfp_nsp_command_arg arg;  /**< NFP command argument structure */
+	const void *in_buf;              /**< Buffer with data for input */
+	void *out_buf;                   /**< Buffer for output data */
+	uint32_t in_size;                /**< Size of @in_buf */
+	uint32_t out_size;               /**< Size of @out_buf */
+};
+
+struct nfp_cpp *
+nfp_nsp_cpp(struct nfp_nsp *state)
+{
+	return state->cpp;
+}
+
+bool
 nfp_nsp_config_modified(struct nfp_nsp *state)
 {
 	return state->modified;
@@ -24,7 +131,7 @@  nfp_nsp_config_modified(struct nfp_nsp *state)
 
 void
 nfp_nsp_config_set_modified(struct nfp_nsp *state,
-		int modified)
+		bool modified)
 {
 	state->modified = modified;
 }
@@ -66,7 +173,7 @@  nfp_nsp_print_extended_error(uint32_t ret_val)
 		return;
 
 	for (i = 0; i < RTE_DIM(nsp_errors); i++)
-		if (ret_val == (uint32_t)nsp_errors[i].code)
+		if (ret_val == nsp_errors[i].code)
 			PMD_DRV_LOG(ERR, "err msg: %s", nsp_errors[i].msg);
 }
 
@@ -222,11 +329,8 @@  nfp_nsp_wait_reg(struct nfp_cpp *cpp,
  *   - -ETIMEDOUT if the NSP took longer than @timeout_sec seconds to complete
  */
 static int
-nfp_nsp_command(struct nfp_nsp *state,
-		uint16_t code,
-		uint32_t option,
-		uint32_t buff_cpp,
-		uint64_t buff_addr)
+nfp_nsp_command_real(struct nfp_nsp *state,
+		const struct nfp_nsp_command_arg *arg)
 {
 	int err;
 	uint64_t reg;
@@ -250,22 +354,14 @@  nfp_nsp_command(struct nfp_nsp *state,
 		return err;
 	}
 
-	if (!FIELD_FIT(NSP_BUFFER_CPP, buff_cpp >> 8) ||
-			!FIELD_FIT(NSP_BUFFER_ADDRESS, buff_addr)) {
-		PMD_DRV_LOG(ERR, "Host buffer out of reach %08x %" PRIx64,
-				buff_cpp, buff_addr);
-		return -EINVAL;
-	}
-
-	err = nfp_cpp_writeq(cpp, nsp_cpp, nsp_buffer,
-			FIELD_PREP(NSP_BUFFER_CPP, buff_cpp >> 8) |
-			FIELD_PREP(NSP_BUFFER_ADDRESS, buff_addr));
+	err = nfp_cpp_writeq(cpp, nsp_cpp, nsp_buffer, arg->buf);
 	if (err < 0)
 		return err;
 
 	err = nfp_cpp_writeq(cpp, nsp_cpp, nsp_command,
-			FIELD_PREP(NSP_COMMAND_OPTION, option) |
-			FIELD_PREP(NSP_COMMAND_CODE, code) |
+			FIELD_PREP(NSP_COMMAND_OPTION, arg->option) |
+			FIELD_PREP(NSP_COMMAND_CODE, arg->code) |
+			FIELD_PREP(NSP_COMMAND_DMA_BUF, arg->dma) |
 			FIELD_PREP(NSP_COMMAND_START, 1));
 	if (err < 0)
 		return err;
@@ -275,7 +371,7 @@  nfp_nsp_command(struct nfp_nsp *state,
 			NSP_COMMAND_START, 0);
 	if (err != 0) {
 		PMD_DRV_LOG(ERR, "Error %d waiting for code %#04x to start",
-				err, code);
+				err, arg->code);
 		return err;
 	}
 
@@ -284,7 +380,7 @@  nfp_nsp_command(struct nfp_nsp *state,
 			NSP_STATUS_BUSY, 0);
 	if (err != 0) {
 		PMD_DRV_LOG(ERR, "Error %d waiting for code %#04x to complete",
-				err, code);
+				err, arg->code);
 		return err;
 	}
 
@@ -296,84 +392,85 @@  nfp_nsp_command(struct nfp_nsp *state,
 
 	err = FIELD_GET(NSP_STATUS_RESULT, reg);
 	if (err != 0) {
-		PMD_DRV_LOG(ERR, "Result (error) code set: %d (%d) command: %d",
-				-err, (int)ret_val, code);
-		nfp_nsp_print_extended_error(ret_val);
+		if (!arg->error_quiet)
+			PMD_DRV_LOG(WARNING, "Result (error) code set: %d (%d) command: %d",
+					-err, (int)ret_val, arg->code);
+
+		if (arg->error_cb != 0)
+			arg->error_cb(state, ret_val);
+		else
+			nfp_nsp_print_extended_error(ret_val);
+
 		return -err;
 	}
 
 	return ret_val;
 }
 
-#define SZ_1M 0x00100000
+static int
+nfp_nsp_command(struct nfp_nsp *state,
+		uint16_t code)
+{
+	const struct nfp_nsp_command_arg arg = {
+		.code = code,
+	};
+
+	return nfp_nsp_command_real(state, &arg);
+}
 
 static int
-nfp_nsp_command_buf(struct nfp_nsp *nsp,
-		uint16_t code, uint32_t option,
-		const void *in_buf,
-		unsigned int in_size,
-		void *out_buf,
-		unsigned int out_size)
+nfp_nsp_command_buf_def(struct nfp_nsp *nsp,
+		struct nfp_nsp_command_buf_arg *arg)
 {
 	int err;
 	int ret;
 	uint64_t reg;
-	size_t max_size;
 	uint32_t cpp_id;
 	uint64_t cpp_buf;
 	struct nfp_cpp *cpp = nsp->cpp;
 
-	if (nsp->ver.minor < 13) {
-		PMD_DRV_LOG(ERR, "NSP: Code 0x%04x with buffer not supported ABI %hu.%hu)",
-				code, nsp->ver.major, nsp->ver.minor);
-		return -EOPNOTSUPP;
-	}
-
-	err = nfp_cpp_readq(cpp, nfp_resource_cpp_id(nsp->res),
-			nfp_resource_address(nsp->res) + NSP_DFLT_BUFFER_CONFIG,
-			&reg);
-	if (err < 0)
-		return err;
-
-	max_size = RTE_MAX(in_size, out_size);
-	if (FIELD_GET(NSP_DFLT_BUFFER_SIZE_MB, reg) * SZ_1M < max_size) {
-		PMD_DRV_LOG(ERR, "NSP: default buffer too small for command 0x%04x (%llu < %lu)",
-				code, FIELD_GET(NSP_DFLT_BUFFER_SIZE_MB, reg) * SZ_1M, max_size);
-		return -EINVAL;
-	}
-
 	err = nfp_cpp_readq(cpp, nfp_resource_cpp_id(nsp->res),
 			nfp_resource_address(nsp->res) + NSP_DFLT_BUFFER,
 			&reg);
 	if (err < 0)
 		return err;
 
-	cpp_id = FIELD_GET(NSP_BUFFER_CPP, reg) << 8;
-	cpp_buf = FIELD_GET(NSP_BUFFER_ADDRESS, reg);
+	cpp_id = FIELD_GET(NSP_DFLT_BUFFER_CPP, reg) << 8;
+	cpp_buf = FIELD_GET(NSP_DFLT_BUFFER_ADDRESS, reg);
 
-	if (in_buf != NULL && in_size > 0) {
-		err = nfp_cpp_write(cpp, cpp_id, cpp_buf, in_buf, in_size);
+	if (arg->in_buf != NULL && arg->in_size > 0) {
+		err = nfp_cpp_write(cpp, cpp_id, cpp_buf,
+				arg->in_buf, arg->in_size);
 		if (err < 0)
 			return err;
 	}
 
 	/* Zero out remaining part of the buffer */
-	if (out_buf != NULL && out_size > 0 && out_size > in_size) {
-		memset(out_buf, 0, out_size - in_size);
-		err = nfp_cpp_write(cpp, cpp_id, cpp_buf + in_size, out_buf,
-				out_size - in_size);
+	if (arg->out_buf != NULL && arg->out_size > arg->in_size) {
+		err = nfp_cpp_write(cpp, cpp_id, cpp_buf + arg->in_size,
+				arg->out_buf, arg->out_size - arg->in_size);
 		if (err < 0)
 			return err;
 	}
 
-	ret = nfp_nsp_command(nsp, code, option, cpp_id, cpp_buf);
+	if (!FIELD_FIT(NSP_BUFFER_CPP, cpp_id >> 8) ||
+			!FIELD_FIT(NSP_BUFFER_ADDRESS, cpp_buf)) {
+		PMD_DRV_LOG(ERR, "Buffer out of reach %#08x %#016lx",
+				cpp_id, cpp_buf);
+		return -EINVAL;
+	}
+
+	arg->arg.buf = FIELD_PREP(NSP_BUFFER_CPP, cpp_id >> 8) |
+			FIELD_PREP(NSP_BUFFER_ADDRESS, cpp_buf);
+	ret = nfp_nsp_command_real(nsp, &arg->arg);
 	if (ret < 0) {
 		PMD_DRV_LOG(ERR, "NSP command failed");
 		return ret;
 	}
 
-	if (out_buf != NULL && out_size > 0) {
-		err = nfp_cpp_read(cpp, cpp_id, cpp_buf, out_buf, out_size);
+	if (arg->out_buf != NULL && arg->out_size > 0) {
+		err = nfp_cpp_read(cpp, cpp_id, cpp_buf,
+				arg->out_buf, arg->out_size);
 		if (err < 0)
 			return err;
 	}
@@ -381,6 +478,43 @@  nfp_nsp_command_buf(struct nfp_nsp *nsp,
 	return ret;
 }
 
+#define SZ_1M 0x00100000
+#define SZ_4K 0x00001000
+
+static int
+nfp_nsp_command_buf(struct nfp_nsp *nsp,
+		struct nfp_nsp_command_buf_arg *arg)
+{
+	int err;
+	uint64_t reg;
+	uint32_t size;
+	uint32_t max_size;
+	struct nfp_cpp *cpp = nsp->cpp;
+
+	if (nsp->ver.minor < 13) {
+		PMD_DRV_LOG(ERR, "NSP: Code %#04x with buffer not supported ABI %hu.%hu)",
+				arg->arg.code, nsp->ver.major, nsp->ver.minor);
+		return -EOPNOTSUPP;
+	}
+
+	err = nfp_cpp_readq(cpp, nfp_resource_cpp_id(nsp->res),
+			nfp_resource_address(nsp->res) + NSP_DFLT_BUFFER_CONFIG,
+			&reg);
+	if (err < 0)
+		return err;
+
+	max_size = RTE_MAX(arg->in_size, arg->out_size);
+	size = FIELD_GET(NSP_DFLT_BUFFER_SIZE_MB, reg) * SZ_1M +
+			FIELD_GET(NSP_DFLT_BUFFER_SIZE_4KB, reg) * SZ_4K;
+	if (size < max_size) {
+		PMD_DRV_LOG(ERR, "NSP: default buffer too small for command %#04x (%u < %u)",
+				arg->arg.code, size, max_size);
+		return -EINVAL;
+	}
+
+	return nfp_nsp_command_buf_def(nsp, arg);
+}
+
 int
 nfp_nsp_wait(struct nfp_nsp *state)
 {
@@ -392,7 +526,7 @@  nfp_nsp_wait(struct nfp_nsp *state)
 	wait.tv_nsec = 25000000;    /* 25ms */
 
 	for (;;) {
-		err = nfp_nsp_command(state, SPCODE_NOOP, 0, 0, 0);
+		err = nfp_nsp_command(state, SPCODE_NOOP);
 		if (err != -EAGAIN)
 			break;
 
@@ -413,13 +547,57 @@  nfp_nsp_wait(struct nfp_nsp *state)
 int
 nfp_nsp_device_soft_reset(struct nfp_nsp *state)
 {
-	return nfp_nsp_command(state, SPCODE_SOFT_RESET, 0, 0, 0);
+	return nfp_nsp_command(state, SPCODE_SOFT_RESET);
 }
 
 int
 nfp_nsp_mac_reinit(struct nfp_nsp *state)
 {
-	return nfp_nsp_command(state, SPCODE_MAC_INIT, 0, 0, 0);
+	return nfp_nsp_command(state, SPCODE_MAC_INIT);
+}
+
+static void
+nfp_nsp_load_fw_extended_msg(struct nfp_nsp *state,
+		uint32_t ret_val)
+{
+	uint32_t minor;
+	uint32_t major;
+	static const char * const major_msg[] = {
+		/* 0 */ "Firmware from driver loaded",
+		/* 1 */ "Firmware from flash loaded",
+		/* 2 */ "Firmware loading failure",
+	};
+	static const char * const minor_msg[] = {
+		/*  0 */ "",
+		/*  1 */ "no named partition on flash",
+		/*  2 */ "error reading from flash",
+		/*  3 */ "can not deflate",
+		/*  4 */ "not a trusted file",
+		/*  5 */ "can not parse FW file",
+		/*  6 */ "MIP not found in FW file",
+		/*  7 */ "null firmware name in MIP",
+		/*  8 */ "FW version none",
+		/*  9 */ "FW build number none",
+		/* 10 */ "no FW selection policy HWInfo key found",
+		/* 11 */ "static FW selection policy",
+		/* 12 */ "FW version has precedence",
+		/* 13 */ "different FW application load requested",
+		/* 14 */ "development build",
+	};
+
+	major = FIELD_GET(NFP_FW_LOAD_RET_MAJOR, ret_val);
+	minor = FIELD_GET(NFP_FW_LOAD_RET_MINOR, ret_val);
+
+	if (!nfp_nsp_has_stored_fw_load(state))
+		return;
+
+	if (major >= RTE_DIM(major_msg))
+		PMD_DRV_LOG(INFO, "FW loading status: %x", ret_val);
+	else if (minor >= RTE_DIM(minor_msg))
+		PMD_DRV_LOG(INFO, "%s, reason code: %d", major_msg[major], minor);
+	else
+		PMD_DRV_LOG(INFO, "%s%c %s", major_msg[major],
+				minor != 0 ? ',' : '.', minor_msg[minor]);
 }
 
 int
@@ -427,8 +605,24 @@  nfp_nsp_load_fw(struct nfp_nsp *state,
 		void *buf,
 		size_t size)
 {
-	return nfp_nsp_command_buf(state, SPCODE_FW_LOAD, size, buf, size,
-			NULL, 0);
+	int ret;
+	struct nfp_nsp_command_buf_arg load_fw = {
+		{
+			.code     = SPCODE_FW_LOAD,
+			.option   = size,
+			.error_cb = nfp_nsp_load_fw_extended_msg,
+		},
+		.in_buf  = buf,
+		.in_size = size,
+	};
+
+	ret = nfp_nsp_command_buf(state, &load_fw);
+	if (ret < 0)
+		return ret;
+
+	nfp_nsp_load_fw_extended_msg(state, ret);
+
+	return 0;
 }
 
 int
@@ -436,8 +630,16 @@  nfp_nsp_read_eth_table(struct nfp_nsp *state,
 		void *buf,
 		size_t size)
 {
-	return nfp_nsp_command_buf(state, SPCODE_ETH_RESCAN, size, NULL, 0,
-			buf, size);
+	struct nfp_nsp_command_buf_arg eth_rescan = {
+		{
+			.code   = SPCODE_ETH_RESCAN,
+			.option = size,
+		},
+		.out_buf  = buf,
+		.out_size = size,
+	};
+
+	return nfp_nsp_command_buf(state, &eth_rescan);
 }
 
 int
@@ -445,8 +647,16 @@  nfp_nsp_write_eth_table(struct nfp_nsp *state,
 		const void *buf,
 		size_t size)
 {
-	return nfp_nsp_command_buf(state, SPCODE_ETH_CONTROL, size, buf, size,
-			NULL, 0);
+	struct nfp_nsp_command_buf_arg eth_ctrl = {
+		{
+			.code   = SPCODE_ETH_CONTROL,
+			.option = size,
+		},
+		.in_buf  = buf,
+		.in_size = size,
+	};
+
+	return nfp_nsp_command_buf(state, &eth_ctrl);
 }
 
 int
@@ -454,8 +664,16 @@  nfp_nsp_read_identify(struct nfp_nsp *state,
 		void *buf,
 		size_t size)
 {
-	return nfp_nsp_command_buf(state, SPCODE_NSP_IDENTIFY, size, NULL, 0,
-			buf, size);
+	struct nfp_nsp_command_buf_arg identify = {
+		{
+			.code   = SPCODE_NSP_IDENTIFY,
+			.option = size,
+		},
+		.out_buf  = buf,
+		.out_size = size,
+	};
+
+	return nfp_nsp_command_buf(state, &identify);
 }
 
 int
@@ -464,6 +682,14 @@  nfp_nsp_read_sensors(struct nfp_nsp *state,
 		void *buf,
 		size_t size)
 {
-	return nfp_nsp_command_buf(state, SPCODE_NSP_SENSORS, sensor_mask, NULL,
-			0, buf, size);
+	struct nfp_nsp_command_buf_arg sensors = {
+		{
+			.code   = SPCODE_NSP_SENSORS,
+			.option = sensor_mask,
+		},
+		.out_buf  = buf,
+		.out_size = size,
+	};
+
+	return nfp_nsp_command_buf(state, &sensors);
 }
diff --git a/drivers/net/nfp/nfpcore/nfp_nsp.h b/drivers/net/nfp/nfpcore/nfp_nsp.h
index 14986a9130..fe52dffeb7 100644
--- a/drivers/net/nfp/nfpcore/nfp_nsp.h
+++ b/drivers/net/nfp/nfpcore/nfp_nsp.h
@@ -7,78 +7,8 @@ 
 #define __NSP_NSP_H__
 
 #include "nfp_cpp.h"
-#include "nfp_nsp.h"
-
-/* Offsets relative to the CSR base */
-#define NSP_STATUS              0x00
-#define   NSP_STATUS_MAGIC      GENMASK_ULL(63, 48)
-#define   NSP_STATUS_MAJOR      GENMASK_ULL(47, 44)
-#define   NSP_STATUS_MINOR      GENMASK_ULL(43, 32)
-#define   NSP_STATUS_CODE       GENMASK_ULL(31, 16)
-#define   NSP_STATUS_RESULT     GENMASK_ULL(15, 8)
-#define   NSP_STATUS_BUSY       RTE_BIT64(0)
-
-#define NSP_COMMAND             0x08
-#define   NSP_COMMAND_OPTION    GENMASK_ULL(63, 32)
-#define   NSP_COMMAND_CODE      GENMASK_ULL(31, 16)
-#define   NSP_COMMAND_START     RTE_BIT64(0)
-
-/* CPP address to retrieve the data from */
-#define NSP_BUFFER              0x10
-#define   NSP_BUFFER_CPP        GENMASK_ULL(63, 40)
-#define   NSP_BUFFER_PCIE       GENMASK_ULL(39, 38)
-#define   NSP_BUFFER_ADDRESS    GENMASK_ULL(37, 0)
-
-#define NSP_DFLT_BUFFER         0x18
-
-#define NSP_DFLT_BUFFER_CONFIG 0x20
-#define   NSP_DFLT_BUFFER_SIZE_MB    GENMASK_ULL(7, 0)
-
-#define NSP_MAGIC               0xab10
-#define NSP_MAJOR               0
-#define NSP_MINOR               8
-
-#define NSP_CODE_MAJOR          GENMASK(15, 12)
-#define NSP_CODE_MINOR          GENMASK(11, 0)
-
-enum nfp_nsp_cmd {
-	SPCODE_NOOP             = 0, /* No operation */
-	SPCODE_SOFT_RESET       = 1, /* Soft reset the NFP */
-	SPCODE_FW_DEFAULT       = 2, /* Load default (UNDI) FW */
-	SPCODE_PHY_INIT         = 3, /* Initialize the PHY */
-	SPCODE_MAC_INIT         = 4, /* Initialize the MAC */
-	SPCODE_PHY_RXADAPT      = 5, /* Re-run PHY RX Adaptation */
-	SPCODE_FW_LOAD          = 6, /* Load fw from buffer, len in option */
-	SPCODE_ETH_RESCAN       = 7, /* Rescan ETHs, write ETH_TABLE to buf */
-	SPCODE_ETH_CONTROL      = 8, /* Update media config from buffer */
-	SPCODE_NSP_SENSORS      = 12, /* Read NSP sensor(s) */
-	SPCODE_NSP_IDENTIFY     = 13, /* Read NSP version */
-};
-
-static const struct {
-	int code;
-	const char *msg;
-} nsp_errors[] = {
-	{ 6010, "could not map to phy for port" },
-	{ 6011, "not an allowed rate/lanes for port" },
-	{ 6012, "not an allowed rate/lanes for port" },
-	{ 6013, "high/low error, change other port first" },
-	{ 6014, "config not found in flash" },
-};
 
-struct nfp_nsp {
-	struct nfp_cpp *cpp;
-	struct nfp_resource *res;
-	struct {
-		uint16_t major;
-		uint16_t minor;
-	} ver;
-
-	/* Eth table config state */
-	int modified;
-	unsigned int idx;
-	void *entries;
-};
+struct nfp_nsp;
 
 struct nfp_nsp *nfp_nsp_open(struct nfp_cpp *cpp);
 void nfp_nsp_close(struct nfp_nsp *state);
@@ -92,18 +22,61 @@  int nfp_nsp_read_identify(struct nfp_nsp *state, void *buf, size_t size);
 int nfp_nsp_read_sensors(struct nfp_nsp *state, uint32_t sensor_mask,
 		void *buf, size_t size);
 
-static inline int
+static inline bool
 nfp_nsp_has_mac_reinit(struct nfp_nsp *state)
 {
 	return nfp_nsp_get_abi_ver_minor(state) > 20;
 }
 
+static inline bool
+nfp_nsp_has_stored_fw_load(struct nfp_nsp *state)
+{
+	return nfp_nsp_get_abi_ver_minor(state) > 23;
+}
+
+static inline bool
+nfp_nsp_has_hwinfo_lookup(struct nfp_nsp *state)
+{
+	return nfp_nsp_get_abi_ver_minor(state) > 24;
+}
+
+static inline bool
+nfp_nsp_has_hwinfo_set(struct nfp_nsp *state)
+{
+	return nfp_nsp_get_abi_ver_minor(state) > 25;
+}
+
+static inline bool
+nfp_nsp_has_fw_loaded(struct nfp_nsp *state)
+{
+	return nfp_nsp_get_abi_ver_minor(state) > 25;
+}
+
+static inline bool
+nfp_nsp_has_versions(struct nfp_nsp *state)
+{
+	return nfp_nsp_get_abi_ver_minor(state) > 27;
+}
+
+static inline bool
+nfp_nsp_has_read_module_eeprom(struct nfp_nsp *state)
+{
+	return nfp_nsp_get_abi_ver_minor(state) > 28;
+}
+
+static inline bool
+nfp_nsp_has_read_media(struct nfp_nsp *state)
+{
+	return nfp_nsp_get_abi_ver_minor(state) > 33;
+}
+
 enum nfp_eth_interface {
 	NFP_INTERFACE_NONE      = 0,
 	NFP_INTERFACE_SFP       = 1,
 	NFP_INTERFACE_SFPP      = 10,
 	NFP_INTERFACE_SFP28     = 28,
 	NFP_INTERFACE_QSFP      = 40,
+	NFP_INTERFACE_RJ45      = 45,
 	NFP_INTERFACE_CXP       = 100,
 	NFP_INTERFACE_QSFP28    = 112,
 };
@@ -151,6 +124,7 @@  struct nfp_eth_table {
 		enum nfp_eth_media media; /**< Media type of the @interface */
 
 		enum nfp_eth_fec fec;     /**< Forward Error Correction mode */
+		enum nfp_eth_fec act_fec; /**< Active Forward Error Correction mode */
 		enum nfp_eth_aneg aneg;   /**< Auto negotiation mode */
 
 		struct rte_ether_addr mac_addr;  /**< Interface MAC address */
@@ -159,17 +133,18 @@  struct nfp_eth_table {
 		/** Id of interface within port (for split ports) */
 		uint8_t label_subport;
 
-		int enabled;     /**< Enable port */
-		int tx_enabled;  /**< Enable TX */
-		int rx_enabled;  /**< Enable RX */
+		bool enabled;     /**< Enable port */
+		bool tx_enabled;  /**< Enable TX */
+		bool rx_enabled;  /**< Enable RX */
+		bool supp_aneg;   /**< Support auto negotiation */
 
-		int override_changed;  /**< Media reconfig pending */
+		bool override_changed;  /**< Media reconfig pending */
 
 		uint8_t port_type;    /**< One of %PORT_* */
 		/** Sum of lanes of all subports of this port */
 		uint32_t port_lanes;
 
-		int is_split;   /**< Split port */
+		bool is_split;   /**< Split port */
 
 		uint32_t fec_modes_supported;  /**< Bitmap of FEC modes supported */
 	} ports[]; /**< Table of ports */
@@ -177,8 +152,8 @@  struct nfp_eth_table {
 
 struct nfp_eth_table *nfp_eth_read_ports(struct nfp_cpp *cpp);
 
-int nfp_eth_set_mod_enable(struct nfp_cpp *cpp, uint32_t idx, int enable);
-int nfp_eth_set_configured(struct nfp_cpp *cpp, uint32_t idx, int configed);
+int nfp_eth_set_mod_enable(struct nfp_cpp *cpp, uint32_t idx, bool enable);
+int nfp_eth_set_configured(struct nfp_cpp *cpp, uint32_t idx, bool configured);
 int nfp_eth_set_fec(struct nfp_cpp *cpp, uint32_t idx, enum nfp_eth_fec mode);
 
 int nfp_nsp_read_eth_table(struct nfp_nsp *state, void *buf, size_t size);
@@ -187,12 +162,13 @@  int nfp_nsp_write_eth_table(struct nfp_nsp *state, const void *buf,
 void nfp_nsp_config_set_state(struct nfp_nsp *state, void *entries,
 		uint32_t idx);
 void nfp_nsp_config_clear_state(struct nfp_nsp *state);
-void nfp_nsp_config_set_modified(struct nfp_nsp *state, int modified);
+void nfp_nsp_config_set_modified(struct nfp_nsp *state, bool modified);
 void *nfp_nsp_config_entries(struct nfp_nsp *state);
-int nfp_nsp_config_modified(struct nfp_nsp *state);
+struct nfp_cpp *nfp_nsp_cpp(struct nfp_nsp *state);
+bool nfp_nsp_config_modified(struct nfp_nsp *state);
 uint32_t nfp_nsp_config_idx(struct nfp_nsp *state);
 
-static inline int
+static inline bool
 nfp_eth_can_support_fec(struct nfp_eth_table_port *eth_port)
 {
 	return eth_port->fec_modes_supported != 0;
diff --git a/drivers/net/nfp/nfpcore/nfp_nsp_cmds.c b/drivers/net/nfp/nfpcore/nfp_nsp_cmds.c
index 429f639fa2..86956f4330 100644
--- a/drivers/net/nfp/nfpcore/nfp_nsp_cmds.c
+++ b/drivers/net/nfp/nfpcore/nfp_nsp_cmds.c
@@ -3,12 +3,8 @@ 
  * All rights reserved.
  */
 
-#include <stdio.h>
-#include <rte_byteorder.h>
-#include "nfp_cpp.h"
 #include "nfp_logs.h"
 #include "nfp_nsp.h"
-#include "nfp_nffw.h"
 
 struct nsp_identify {
 	uint8_t version[40];
diff --git a/drivers/net/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/nfp/nfpcore/nfp_nsp_eth.c
index dd17d0f5f6..90ecdaaa52 100644
--- a/drivers/net/nfp/nfpcore/nfp_nsp_eth.c
+++ b/drivers/net/nfp/nfpcore/nfp_nsp_eth.c
@@ -3,10 +3,6 @@ 
  * All rights reserved.
  */
 
-#include <stdio.h>
-#include <rte_common.h>
-#include <rte_byteorder.h>
-#include "nfp_cpp.h"
 #include "nfp_logs.h"
 #include "nfp_nsp.h"
 #include "nfp_platform.h"
@@ -21,6 +17,7 @@ 
 #define NSP_ETH_PORT_PHYLABEL           GENMASK_ULL(59, 54)
 #define NSP_ETH_PORT_FEC_SUPP_BASER     RTE_BIT64(60)
 #define NSP_ETH_PORT_FEC_SUPP_RS        RTE_BIT64(61)
+#define NSP_ETH_PORT_SUPP_ANEG          RTE_BIT64(63)
 
 #define NSP_ETH_PORT_LANES_MASK         rte_cpu_to_le_64(NSP_ETH_PORT_LANES)
 
@@ -34,6 +31,7 @@ 
 #define NSP_ETH_STATE_OVRD_CHNG         RTE_BIT64(22)
 #define NSP_ETH_STATE_ANEG              GENMASK_ULL(25, 23)
 #define NSP_ETH_STATE_FEC               GENMASK_ULL(27, 26)
+#define NSP_ETH_STATE_ACT_FEC           GENMASK_ULL(29, 28)
 
 #define NSP_ETH_CTRL_CONFIGURED         RTE_BIT64(0)
 #define NSP_ETH_CTRL_ENABLED            RTE_BIT64(1)
@@ -54,26 +52,12 @@ 
 #define PORT_NONE               0xef
 #define PORT_OTHER              0xff
 
-#define SPEED_10                10
-#define SPEED_100               100
-#define SPEED_1000              1000
-#define SPEED_2500              2500
-#define SPEED_5000              5000
-#define SPEED_10000             10000
-#define SPEED_14000             14000
-#define SPEED_20000             20000
-#define SPEED_25000             25000
-#define SPEED_40000             40000
-#define SPEED_50000             50000
-#define SPEED_56000             56000
-#define SPEED_100000            100000
-
 enum nfp_eth_raw {
 	NSP_ETH_RAW_PORT = 0,
 	NSP_ETH_RAW_STATE,
 	NSP_ETH_RAW_MAC,
 	NSP_ETH_RAW_CONTROL,
-	NSP_ETH_NUM_RAW
+	NSP_ETH_NUM_RAW,
 };
 
 enum nfp_eth_rate {
@@ -100,12 +84,12 @@  static const struct {
 	enum nfp_eth_rate rate;
 	uint32_t speed;
 } nsp_eth_rate_tbl[] = {
-	{ RATE_INVALID, 0, },
-	{ RATE_10M,     SPEED_10, },
-	{ RATE_100M,    SPEED_100, },
-	{ RATE_1G,      SPEED_1000, },
-	{ RATE_10G,     SPEED_10000, },
-	{ RATE_25G,     SPEED_25000, },
+	{ RATE_INVALID, RTE_ETH_SPEED_NUM_NONE, },
+	{ RATE_10M,     RTE_ETH_SPEED_NUM_10M, },
+	{ RATE_100M,    RTE_ETH_SPEED_NUM_100M, },
+	{ RATE_1G,      RTE_ETH_SPEED_NUM_1G, },
+	{ RATE_10G,     RTE_ETH_SPEED_NUM_10G, },
+	{ RATE_25G,     RTE_ETH_SPEED_NUM_25G, },
 };
 
 static uint32_t
@@ -193,7 +177,14 @@  nfp_eth_port_translate(struct nfp_nsp *nsp,
 	if (dst->fec_modes_supported != 0)
 		dst->fec_modes_supported |= NFP_FEC_AUTO | NFP_FEC_DISABLED;
 
-	dst->fec = 1 << FIELD_GET(NSP_ETH_STATE_FEC, state);
+	dst->fec = FIELD_GET(NSP_ETH_STATE_FEC, state);
+	dst->act_fec = dst->fec;
+
+	if (nfp_nsp_get_abi_ver_minor(nsp) < 33)
+		return;
+
+	dst->act_fec = FIELD_GET(NSP_ETH_STATE_ACT_FEC, state);
+	dst->supp_aneg = FIELD_GET(NSP_ETH_PORT_SUPP_ANEG, port);
 }
 
 static void
@@ -222,7 +213,7 @@  nfp_eth_calc_port_geometry(struct nfp_eth_table *table)
 						table->ports[i].label_port,
 						table->ports[i].label_subport);
 
-			table->ports[i].is_split = 1;
+			table->ports[i].is_split = true;
 		}
 	}
 }
@@ -233,6 +224,9 @@  nfp_eth_calc_port_type(struct nfp_eth_table_port *entry)
 	if (entry->interface == NFP_INTERFACE_NONE) {
 		entry->port_type = PORT_NONE;
 		return;
+	} else if (entry->interface == NFP_INTERFACE_RJ45) {
+		entry->port_type = PORT_TP;
+		return;
 	}
 
 	if (entry->media == NFP_MEDIA_FIBRE)
@@ -251,7 +245,6 @@  nfp_eth_read_ports_real(struct nfp_nsp *nsp)
 	uint32_t table_sz;
 	struct nfp_eth_table *table;
 	union eth_table_entry *entries;
-	const struct rte_ether_addr *mac;
 
 	entries = rte_zmalloc(NULL, NSP_ETH_TABLE_SIZE, 0);
 	if (entries == NULL)
@@ -263,16 +256,9 @@  nfp_eth_read_ports_real(struct nfp_nsp *nsp)
 		goto err;
 	}
 
-	/*
-	 * The NFP3800 NIC support 8 ports, but only 2 ports are valid,
-	 * the rest 6 ports mac are all 0, ensure we don't use these port
-	 */
-	for (i = 0; i < NSP_ETH_MAX_COUNT; i++) {
-		mac = (const struct rte_ether_addr *)entries[i].mac_addr;
-		if ((entries[i].port & NSP_ETH_PORT_LANES_MASK) != 0 &&
-				!rte_is_zero_ether_addr(mac))
+	for (i = 0; i < NSP_ETH_MAX_COUNT; i++)
+		if ((entries[i].port & NSP_ETH_PORT_LANES_MASK) != 0)
 			cnt++;
-	}
 
 	/*
 	 * Some versions of flash will give us 0 instead of port count. For
@@ -292,11 +278,8 @@  nfp_eth_read_ports_real(struct nfp_nsp *nsp)
 
 	table->count = cnt;
 	for (i = 0, j = 0; i < NSP_ETH_MAX_COUNT; i++) {
-		mac = (const struct rte_ether_addr *)entries[i].mac_addr;
-		if ((entries[i].port & NSP_ETH_PORT_LANES_MASK) != 0 &&
-				!rte_is_zero_ether_addr(mac))
-			nfp_eth_port_translate(nsp, &entries[i], i,
-					&table->ports[j++]);
+		if ((entries[i].port & NSP_ETH_PORT_LANES_MASK) != 0)
+			nfp_eth_port_translate(nsp, &entries[i], i, &table->ports[j++]);
 	}
 
 	nfp_eth_calc_port_geometry(table);
@@ -437,7 +420,7 @@  nfp_eth_config_commit_end(struct nfp_nsp *nsp)
 int
 nfp_eth_set_mod_enable(struct nfp_cpp *cpp,
 		uint32_t idx,
-		int enable)
+		bool enable)
 {
 	uint64_t reg;
 	struct nfp_nsp *nsp;
@@ -445,7 +428,7 @@  nfp_eth_set_mod_enable(struct nfp_cpp *cpp,
 
 	nsp = nfp_eth_config_start(cpp, idx);
 	if (nsp == NULL)
-		return -1;
+		return -EIO;
 
 	entries = nfp_nsp_config_entries(nsp);
 
@@ -457,7 +440,7 @@  nfp_eth_set_mod_enable(struct nfp_cpp *cpp,
 		reg |= FIELD_PREP(NSP_ETH_CTRL_ENABLED, enable);
 		entries[idx].control = rte_cpu_to_le_64(reg);
 
-		nfp_nsp_config_set_modified(nsp, 1);
+		nfp_nsp_config_set_modified(nsp, true);
 	}
 
 	return nfp_eth_config_commit_end(nsp);
@@ -481,7 +464,7 @@  nfp_eth_set_mod_enable(struct nfp_cpp *cpp,
 int
 nfp_eth_set_configured(struct nfp_cpp *cpp,
 		uint32_t idx,
-		int configured)
+		bool configured)
 {
 	uint64_t reg;
 	struct nfp_nsp *nsp;
@@ -510,7 +493,7 @@  nfp_eth_set_configured(struct nfp_cpp *cpp,
 		reg |= FIELD_PREP(NSP_ETH_CTRL_CONFIGURED, configured);
 		entries[idx].control = rte_cpu_to_le_64(reg);
 
-		nfp_nsp_config_set_modified(nsp, 1);
+		nfp_nsp_config_set_modified(nsp, true);
 	}
 
 	return nfp_eth_config_commit_end(nsp);
@@ -548,7 +531,7 @@  nfp_eth_set_bit_config(struct nfp_nsp *nsp,
 
 	entries[idx].control |= rte_cpu_to_le_64(ctrl_bit);
 
-	nfp_nsp_config_set_modified(nsp, 1);
+	nfp_nsp_config_set_modified(nsp, true);
 
 	return 0;
 }