diff mbox

[dpdk-dev,v3,16/29] net/bnxt: use eal I/O device memory read/write API

Message ID 1484212646-10338-17-git-send-email-jerin.jacob@caviumnetworks.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel compilation success Compilation OK

Commit Message

Jerin Jacob Jan. 12, 2017, 9:17 a.m. UTC 
  From: Santosh Shukla <santosh.shukla@caviumnetworks.com>

Replace the raw I/O device memory read/write access with eal abstraction
for I/O device memory read/write access to fix portability issues across
different architectures.

CC: Stephen Hurd <stephen.hurd@broadcom.com>
CC: Ajit Khaparde <ajit.khaparde@broadcom.com>
Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
 drivers/net/bnxt/bnxt_cpr.h  | 13 ++++++++-----
 drivers/net/bnxt/bnxt_hwrm.c |  7 +++++--
 drivers/net/bnxt/bnxt_txr.h  |  6 +++---
 3 files changed, 16 insertions(+), 10 deletions(-)

Comments

Ferruh Yigit Jan. 12, 2017, 7:10 p.m. UTC | #1 
On 1/12/2017 9:17 AM, Jerin Jacob wrote:
<...>
>  #define B_CP_DB_REARM(cpr, raw_cons)					\
> -		(*(uint32_t *)((cpr)->cp_doorbell) = (DB_CP_REARM_FLAGS | \
> -				RING_CMP(cpr->cp_ring_struct, raw_cons)))
> +	rte_write32((DB_CP_REARM_FLAGS |				\

Just asking, can this be rte_write32_relaxed() since there is explicit
memory barrier defined for B_CP_DIS_DB but not for here?

> +		    RING_CMP(((cpr)->cp_ring_struct), raw_cons)),	\
> +		    ((cpr)->cp_doorbell))
>  
>  #define B_CP_DIS_DB(cpr, raw_cons)					\
> -		rte_smp_wmb();						\
> -		(*(uint32_t *)((cpr)->cp_doorbell) = (DB_CP_FLAGS |	\
> -				RING_CMP(cpr->cp_ring_struct, raw_cons)))
> +	rte_write32((DB_CP_FLAGS |					\
> +		    RING_CMP(((cpr)->cp_ring_struct), raw_cons)),	\
> +		    ((cpr)->cp_doorbell))
>  

<...>

> @@ -80,11 +82,12 @@ static int bnxt_hwrm_send_message_locked(struct bnxt *bp, void *msg,
>  	for (; i < bp->max_req_len; i += 4) {
>  		bar = (uint8_t *)bp->bar0 + i;
>  		*(volatile uint32_t *)bar = 0;

Should this line be removed?

> +		rte_write32(0, bar);
>  	}
>  
>  	/* Ring channel doorbell */
>  	bar = (uint8_t *)bp->bar0 + 0x100;
> -	*(volatile uint32_t *)bar = 1;
> +	rte_write32(1, bar);
>  
>  	/* Poll for the valid bit */
>  	for (i = 0; i < HWRM_CMD_TIMEOUT; i++) {
<...>
Jerin Jacob Jan. 13, 2017, 8:30 a.m. UTC | #2 
On Thu, Jan 12, 2017 at 07:10:24PM +0000, Ferruh Yigit wrote:

Thanks Ferruh for the review.


> On 1/12/2017 9:17 AM, Jerin Jacob wrote:
> <...>
> >  #define B_CP_DB_REARM(cpr, raw_cons)					\
> > -		(*(uint32_t *)((cpr)->cp_doorbell) = (DB_CP_REARM_FLAGS | \
> > -				RING_CMP(cpr->cp_ring_struct, raw_cons)))
> > +	rte_write32((DB_CP_REARM_FLAGS |				\
> 
> Just asking, can this be rte_write32_relaxed() since there is explicit
> memory barrier defined for B_CP_DIS_DB but not for here?

Here is the logic followed across the driver change:

1) If the top level code is in the pattern like "explicit barrier" and then
io write. Then second the io write operation can be relaxed.
2) If the code runs in slow path, To make patch clean and scope limited,
use only nonrelaxed versions.

> 
> > +		    RING_CMP(((cpr)->cp_ring_struct), raw_cons)),	\
> > +		    ((cpr)->cp_doorbell))
> >  
> >  #define B_CP_DIS_DB(cpr, raw_cons)					\
> > -		rte_smp_wmb();						\
> > -		(*(uint32_t *)((cpr)->cp_doorbell) = (DB_CP_FLAGS |	\
> > -				RING_CMP(cpr->cp_ring_struct, raw_cons)))
> > +	rte_write32((DB_CP_FLAGS |					\
> > +		    RING_CMP(((cpr)->cp_ring_struct), raw_cons)),	\
> > +		    ((cpr)->cp_doorbell))
> >  
> 
> <...>
> 
> > @@ -80,11 +82,12 @@ static int bnxt_hwrm_send_message_locked(struct bnxt *bp, void *msg,
> >  	for (; i < bp->max_req_len; i += 4) {
> >  		bar = (uint8_t *)bp->bar0 + i;
> >  		*(volatile uint32_t *)bar = 0;
> 
> Should this line be removed?

Yes. Will be fixed in next revision.

> 
> > +		rte_write32(0, bar);
> >  	}
> >  
> >  	/* Ring channel doorbell */
> >  	bar = (uint8_t *)bp->bar0 + 0x100;
> > -	*(volatile uint32_t *)bar = 1;
> > +	rte_write32(1, bar);
> >  
> >  	/* Poll for the valid bit */
> >  	for (i = 0; i < HWRM_CMD_TIMEOUT; i++) {
> <...>
diff mbox

Patch

diff --git a/drivers/net/bnxt/bnxt_cpr.h b/drivers/net/bnxt/bnxt_cpr.h
index f9f2adb..83e5376 100644
--- a/drivers/net/bnxt/bnxt_cpr.h
+++ b/drivers/net/bnxt/bnxt_cpr.h
@@ -34,6 +34,8 @@ 
 #ifndef _BNXT_CPR_H_
 #define _BNXT_CPR_H_
 
+#include <rte_io.h>
+
 #define CMP_VALID(cmp, raw_cons, ring)					\
 	(!!(((struct cmpl_base *)(cmp))->info3_v & CMPL_BASE_V) ==	\
 	 !((raw_cons) & ((ring)->ring_size)))
@@ -50,13 +52,14 @@ 
 #define DB_CP_FLAGS		(DB_KEY_CP | DB_IDX_VALID | DB_IRQ_DIS)
 
 #define B_CP_DB_REARM(cpr, raw_cons)					\
-		(*(uint32_t *)((cpr)->cp_doorbell) = (DB_CP_REARM_FLAGS | \
-				RING_CMP(cpr->cp_ring_struct, raw_cons)))
+	rte_write32((DB_CP_REARM_FLAGS |				\
+		    RING_CMP(((cpr)->cp_ring_struct), raw_cons)),	\
+		    ((cpr)->cp_doorbell))
 
 #define B_CP_DIS_DB(cpr, raw_cons)					\
-		rte_smp_wmb();						\
-		(*(uint32_t *)((cpr)->cp_doorbell) = (DB_CP_FLAGS |	\
-				RING_CMP(cpr->cp_ring_struct, raw_cons)))
+	rte_write32((DB_CP_FLAGS |					\
+		    RING_CMP(((cpr)->cp_ring_struct), raw_cons)),	\
+		    ((cpr)->cp_doorbell))
 
 struct bnxt_ring;
 struct bnxt_cp_ring_info {
diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
index 07e7124..c182152 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -50,6 +50,8 @@ 
 #include "bnxt_vnic.h"
 #include "hsi_struct_def_dpdk.h"
 
+#include <rte_io.h>
+
 #define HWRM_CMD_TIMEOUT		2000
 
 /*
@@ -72,7 +74,7 @@  static int bnxt_hwrm_send_message_locked(struct bnxt *bp, void *msg,
 	/* Write request msg to hwrm channel */
 	for (i = 0; i < msg_len; i += 4) {
 		bar = (uint8_t *)bp->bar0 + i;
-		*(volatile uint32_t *)bar = *data;
+		rte_write32(*data, bar);
 		data++;
 	}
 
@@ -80,11 +82,12 @@  static int bnxt_hwrm_send_message_locked(struct bnxt *bp, void *msg,
 	for (; i < bp->max_req_len; i += 4) {
 		bar = (uint8_t *)bp->bar0 + i;
 		*(volatile uint32_t *)bar = 0;
+		rte_write32(0, bar);
 	}
 
 	/* Ring channel doorbell */
 	bar = (uint8_t *)bp->bar0 + 0x100;
-	*(volatile uint32_t *)bar = 1;
+	rte_write32(1, bar);
 
 	/* Poll for the valid bit */
 	for (i = 0; i < HWRM_CMD_TIMEOUT; i++) {
diff --git a/drivers/net/bnxt/bnxt_txr.h b/drivers/net/bnxt/bnxt_txr.h
index 4c16101..5b09711 100644
--- a/drivers/net/bnxt/bnxt_txr.h
+++ b/drivers/net/bnxt/bnxt_txr.h
@@ -34,12 +34,12 @@ 
 #ifndef _BNXT_TXR_H_
 #define _BNXT_TXR_H_
 
+#include <rte_io.h>
+
 #define MAX_TX_RINGS	16
 #define BNXT_TX_PUSH_THRESH 92
 
-#define B_TX_DB(db, prod)						\
-		rte_smp_wmb();						\
-		(*(uint32_t *)db = (DB_KEY_TX | prod))
+#define B_TX_DB(db, prod)	rte_write32((DB_KEY_TX | (prod)), db)
 
 struct bnxt_tx_ring_info {
 	uint16_t		tx_prod;