diff mbox series

[v1,1/5] net/ark: update mpu code to match current hardware version

Message ID 20220506212732.28504-1-ed.czeck@atomicrules.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series [v1,1/5] net/ark: update mpu code to match current hardware version |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Ed Czeck May 6, 2022, 9:27 p.m. UTC 
  new version code
remove device-level global operations
remove ark_mpu_reset_stats function

Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
---
 drivers/net/ark/ark_ethdev.c    |  2 --
 drivers/net/ark/ark_ethdev_rx.c |  4 ----
 drivers/net/ark/ark_mpu.c       | 21 ++++-----------------
 drivers/net/ark/ark_mpu.h       | 29 ++---------------------------
 4 files changed, 6 insertions(+), 50 deletions(-)

Comments

Ferruh Yigit May 18, 2022, 12:54 p.m. UTC | #1 
On 5/6/2022 10:27 PM, Ed Czeck wrote:
> new version code
> remove device-level global operations
> remove ark_mpu_reset_stats function
> 

Hi Ed, please find a few comments inline.

Also I assume 'mpu' is an abbreviation, can you please document what it 
stands for in the commit log, like MPU (M.. P.. U..)?
And can you please make it uppercase in the commit title, also if you 
can add them to 'devtools/words-case.txt' (in a separate patch), it will 
be checked next time by './devtools/check-git-log.sh' script.
Same for all patches.

> Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
> ---
>   drivers/net/ark/ark_ethdev.c    |  2 --
>   drivers/net/ark/ark_ethdev_rx.c |  4 ----
>   drivers/net/ark/ark_mpu.c       | 21 ++++-----------------
>   drivers/net/ark/ark_mpu.h       | 29 ++---------------------------
>   4 files changed, 6 insertions(+), 50 deletions(-)
> 

<...>

> @@ -24,10 +24,10 @@ ark_mpu_verify(struct ark_mpu_t *mpu, uint32_t obj_size)
>   {
>   	uint32_t version;
>   
> -	version = mpu->id.vernum & 0x0000fF00;
> -	if ((mpu->id.idnum != 0x2055504d) ||
> -	    (mpu->hw.obj_size != obj_size) ||
> -	    (version != 0x00003100)) {
> +	version = mpu->id.vernum;
> +	if (mpu->id.idnum != ARK_MPU_MODID ||
> +	    version != ARK_MPU_MODVER ||
> +	    mpu->hw.obj_size != obj_size) {

The driver will work with a specific version of the 'MPU'. So a device 
previously working with previous version of the driver, won't work 
anymore after this patch, and will be forced to a (FW/bitstream/?) update.
I am not sure how problematic is this from the stable release 
perspective. cc'ed maintainers.

But at least won't it be good to document this in release notes, and 
perhaps having a table in the driver documentation to list which DPDK 
version requires which HW version can be good, what do you think?
Kevin Traynor May 19, 2022, 10:13 a.m. UTC | #2 
On 18/05/2022 13:54, Ferruh Yigit wrote:
> On 5/6/2022 10:27 PM, Ed Czeck wrote:
>> new version code
>> remove device-level global operations
>> remove ark_mpu_reset_stats function
>>
> 
> Hi Ed, please find a few comments inline.
> 
> Also I assume 'mpu' is an abbreviation, can you please document what it
> stands for in the commit log, like MPU (M.. P.. U..)?
> And can you please make it uppercase in the commit title, also if you
> can add them to 'devtools/words-case.txt' (in a separate patch), it will
> be checked next time by './devtools/check-git-log.sh' script.
> Same for all patches.
> 
>> Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
>> ---
>>    drivers/net/ark/ark_ethdev.c    |  2 --
>>    drivers/net/ark/ark_ethdev_rx.c |  4 ----
>>    drivers/net/ark/ark_mpu.c       | 21 ++++-----------------
>>    drivers/net/ark/ark_mpu.h       | 29 ++---------------------------
>>    4 files changed, 6 insertions(+), 50 deletions(-)
>>
> 
> <...>
> 
>> @@ -24,10 +24,10 @@ ark_mpu_verify(struct ark_mpu_t *mpu, uint32_t obj_size)
>>    {
>>    	uint32_t version;
>>    
>> -	version = mpu->id.vernum & 0x0000fF00;
>> -	if ((mpu->id.idnum != 0x2055504d) ||
>> -	    (mpu->hw.obj_size != obj_size) ||
>> -	    (version != 0x00003100)) {
>> +	version = mpu->id.vernum;
>> +	if (mpu->id.idnum != ARK_MPU_MODID ||
>> +	    version != ARK_MPU_MODVER ||
>> +	    mpu->hw.obj_size != obj_size) {
> 
> The driver will work with a specific version of the 'MPU'. So a device
> previously working with previous version of the driver, won't work
> anymore after this patch, and will be forced to a (FW/bitstream/?) update.
> I am not sure how problematic is this from the stable release
> perspective. cc'ed maintainers.
> 

While it is nice-to-have, stable releases are not guaranteed to work 
with future HW/FW/3rd party software etc.

More importantly, a user should not *have* to make updates outside DPDK 
when moving along a single DPDK LTS stream. e.g. 21.11.1 -> 21.11.2.

> But at least won't it be good to document this in release notes, and
> perhaps having a table in the driver documentation to list which DPDK
> version requires which HW version can be good, what do you think?
> 

I agree documenting the matching versions is good for usability. e.g. 
http://doc.dpdk.org/guides/nics/ice.html#recommended-matching-list
diff mbox series

Patch

diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
index 76b88c62d0..c0578b85ce 100644
--- a/drivers/net/ark/ark_ethdev.c
+++ b/drivers/net/ark/ark_ethdev.c
@@ -524,7 +524,6 @@  ark_config_device(struct rte_eth_dev *dev)
 	num_q = ark_api_num_queues(mpu);
 	ark->rx_queues = num_q;
 	for (i = 0; i < num_q; i++) {
-		ark_mpu_reset(mpu);
 		mpu = RTE_PTR_ADD(mpu, ARK_MPU_QOFFSET);
 	}
 
@@ -536,7 +535,6 @@  ark_config_device(struct rte_eth_dev *dev)
 	num_q = ark_api_num_queues(mpu);
 	ark->tx_queues = num_q;
 	for (i = 0; i < num_q; i++) {
-		ark_mpu_reset(mpu);
 		mpu = RTE_PTR_ADD(mpu, ARK_MPU_QOFFSET);
 	}
 
diff --git a/drivers/net/ark/ark_ethdev_rx.c b/drivers/net/ark/ark_ethdev_rx.c
index 0fbb2603db..85e34d0bb8 100644
--- a/drivers/net/ark/ark_ethdev_rx.c
+++ b/drivers/net/ark/ark_ethdev_rx.c
@@ -91,9 +91,6 @@  eth_ark_rx_hw_setup(struct rte_eth_dev *dev,
 
 	ark_udm_write_addr(queue->udm, phys_addr_prod_index);
 
-	/* advance the valid pointer, but don't start until the queue starts */
-	ark_mpu_reset_stats(queue->mpu);
-
 	/* The seed is the producer index for the HW */
 	ark_mpu_set_producer(queue->mpu, queue->seed_index);
 	dev->data->rx_queue_state[rx_queue_idx] = RTE_ETH_QUEUE_STATE_STOPPED;
@@ -589,7 +586,6 @@  eth_rx_queue_stats_reset(void *vqueue)
 	if (queue == 0)
 		return;
 
-	ark_mpu_reset_stats(queue->mpu);
 	ark_udm_queue_stats_reset(queue->udm);
 }
 
diff --git a/drivers/net/ark/ark_mpu.c b/drivers/net/ark/ark_mpu.c
index b8e94b6ed3..9d5ee7841b 100644
--- a/drivers/net/ark/ark_mpu.c
+++ b/drivers/net/ark/ark_mpu.c
@@ -24,10 +24,10 @@  ark_mpu_verify(struct ark_mpu_t *mpu, uint32_t obj_size)
 {
 	uint32_t version;
 
-	version = mpu->id.vernum & 0x0000fF00;
-	if ((mpu->id.idnum != 0x2055504d) ||
-	    (mpu->hw.obj_size != obj_size) ||
-	    (version != 0x00003100)) {
+	version = mpu->id.vernum;
+	if (mpu->id.idnum != ARK_MPU_MODID ||
+	    version != ARK_MPU_MODVER ||
+	    mpu->hw.obj_size != obj_size) {
 		ARK_PMD_LOG(ERR,
 			    "   MPU module not found as expected %08x"
 			    " \"%c%c%c%c %c%c%c%c\"\n",
@@ -79,16 +79,9 @@  ark_mpu_reset(struct ark_mpu_t *mpu)
 		mpu->cfg.command = MPU_CMD_FORCE_RESET;
 		usleep(10);
 	}
-	ark_mpu_reset_stats(mpu);
 	return mpu->cfg.command != MPU_CMD_IDLE;
 }
 
-void
-ark_mpu_reset_stats(struct ark_mpu_t *mpu)
-{
-	mpu->stats.pci_request = 1;	/* reset stats */
-}
-
 int
 ark_mpu_configure(struct ark_mpu_t *mpu, rte_iova_t ring, uint32_t ring_size,
 		  int is_tx)
@@ -118,12 +111,6 @@  ark_mpu_dump(struct ark_mpu_t *mpu, const char *code, uint16_t qid)
 	ARK_PMD_LOG(DEBUG, "MPU: %s Q: %3u sw_prod %u, hw_cons: %u\n",
 		      code, qid,
 		      mpu->cfg.sw_prod_index, mpu->cfg.hw_cons_index);
-	ARK_PMD_LOG(DEBUG, "MPU: %s state: %d count %d, reserved %d"
-		      "\n",
-		      code,
-		      mpu->debug.state, mpu->debug.count,
-		      mpu->debug.reserved
-		      );
 }
 
 void
diff --git a/drivers/net/ark/ark_mpu.h b/drivers/net/ark/ark_mpu.h
index 92c3e67c86..9d2b70d35f 100644
--- a/drivers/net/ark/ark_mpu.h
+++ b/drivers/net/ark/ark_mpu.h
@@ -15,6 +15,8 @@ 
  * there is minimal documentation.
  */
 
+#define ARK_MPU_MODID 0x2055504d
+#define ARK_MPU_MODVER 0x37313232
 /*
  * MPU hardware structures
  * These are overlay structures to a memory mapped FPGA device.  These
@@ -64,26 +66,6 @@  enum ARK_MPU_COMMAND {
 	MPU_COMMAND_LIMIT = 0xfFFFFFFF
 };
 
-#define ARK_MPU_STATS 0x080
-struct ark_mpu_stats_t {
-	volatile uint64_t pci_request;
-	volatile uint64_t q_empty;
-	volatile uint64_t q_q1;
-	volatile uint64_t q_q2;
-	volatile uint64_t q_q3;
-	volatile uint64_t q_q4;
-	volatile uint64_t q_full;
-};
-
-#define ARK_MPU_DEBUG 0x0C0
-struct ark_mpu_debug_t {
-	volatile uint32_t state;
-	uint32_t reserved;
-	volatile uint32_t count;
-	volatile uint32_t take;
-	volatile uint32_t peek[4];
-};
-
 /*  Consolidated structure */
 struct ark_mpu_t {
 	struct ark_mpu_id_t id;
@@ -93,12 +75,6 @@  struct ark_mpu_t {
 	uint8_t reserved1[(ARK_MPU_CFG - ARK_MPU_HW) -
 			  sizeof(struct ark_mpu_hw_t)];
 	struct ark_mpu_cfg_t cfg;
-	uint8_t reserved2[(ARK_MPU_STATS - ARK_MPU_CFG) -
-			  sizeof(struct ark_mpu_cfg_t)];
-	struct ark_mpu_stats_t stats;
-	uint8_t reserved3[(ARK_MPU_DEBUG - ARK_MPU_STATS) -
-			  sizeof(struct ark_mpu_stats_t)];
-	struct ark_mpu_debug_t debug;
 };
 
 uint16_t ark_api_num_queues(struct ark_mpu_t *mpu);
@@ -113,7 +89,6 @@  int ark_mpu_configure(struct ark_mpu_t *mpu, rte_iova_t ring,
 
 void ark_mpu_dump(struct ark_mpu_t *mpu, const char *msg, uint16_t idx);
 void ark_mpu_dump_setup(struct ark_mpu_t *mpu, uint16_t qid);
-void ark_mpu_reset_stats(struct ark_mpu_t *mpu);
 
 /*  this action is in a performance critical path */
 static inline void