diff mbox

[dpdk-dev,v3,15/29] crypto/qat: use eal I/O device memory read/write API

Message ID 1484212646-10338-16-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: John Griffin <john.griffin@intel.com>
CC: Fiona Trahe <fiona.trahe@intel.com>
CC: Deepak Kumar Jain <deepak.k.jain@intel.com>
Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
 drivers/crypto/qat/qat_adf/adf_transport_access_macros.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Ferruh Yigit Jan. 12, 2017, 7:09 p.m. UTC | #1 
Hi Jerin,

On 1/12/2017 9:17 AM, Jerin Jacob wrote:
<...>

> +#include <rte_io.h>
> +
>  /* CSR write macro */
> -#define ADF_CSR_WR(csrAddr, csrOffset, val) \
> -	(void)((*((volatile uint32_t *)(((uint8_t *)csrAddr) + csrOffset)) \
> -			= (val)))
> +#define ADF_CSR_WR(csrAddr, csrOffset, val)		\
> +	rte_write32(val, (((uint8_t *)csrAddr) + csrOffset))

For IA, this update introduces an extra compiler barrier (rte_io_wmb()),
which is indeed not a must, is this correct?

If so, does it make sense to override these functions for x86, and make
rte_writeX = rte_writeX_relaxed
rte_readX = rte_readX_relaxed

>  
>  /* CSR read macro */
> -#define ADF_CSR_RD(csrAddr, csrOffset) \
> -	(*((volatile uint32_t *)(((uint8_t *)csrAddr) + csrOffset)))
> +#define ADF_CSR_RD(csrAddr, csrOffset)			\
> +	rte_read32((((uint8_t *)csrAddr) + csrOffset))

This patchset both introduces new rte_readX/rte_writeX functions, also
applies them into drivers.

While applying them, it changes the behavior.
Like above code was doing a read, but after update it does read and
read_memory_barrier.

What do you think this patchset updates usage in a manner that keeps
behavior exact same. Like using rte_read32_relaxed for this case.
And doing architecture related updates in a different patchset?

This both makes easy to see architecture specific updates, and makes
easy to trace any possible performance issues by this patchset.

>  
>  #define ADF_BANK_INT_SRC_SEL_MASK_0 0x4444444CUL
>  #define ADF_BANK_INT_SRC_SEL_MASK_X 0x44444444UL
>
diff mbox

Patch

diff --git a/drivers/crypto/qat/qat_adf/adf_transport_access_macros.h b/drivers/crypto/qat/qat_adf/adf_transport_access_macros.h
index 47f1c91..d218f85 100644
--- a/drivers/crypto/qat/qat_adf/adf_transport_access_macros.h
+++ b/drivers/crypto/qat/qat_adf/adf_transport_access_macros.h
@@ -47,14 +47,15 @@ 
 #ifndef ADF_TRANSPORT_ACCESS_MACROS_H
 #define ADF_TRANSPORT_ACCESS_MACROS_H
 
+#include <rte_io.h>
+
 /* CSR write macro */
-#define ADF_CSR_WR(csrAddr, csrOffset, val) \
-	(void)((*((volatile uint32_t *)(((uint8_t *)csrAddr) + csrOffset)) \
-			= (val)))
+#define ADF_CSR_WR(csrAddr, csrOffset, val)		\
+	rte_write32(val, (((uint8_t *)csrAddr) + csrOffset))
 
 /* CSR read macro */
-#define ADF_CSR_RD(csrAddr, csrOffset) \
-	(*((volatile uint32_t *)(((uint8_t *)csrAddr) + csrOffset)))
+#define ADF_CSR_RD(csrAddr, csrOffset)			\
+	rte_read32((((uint8_t *)csrAddr) + csrOffset))
 
 #define ADF_BANK_INT_SRC_SEL_MASK_0 0x4444444CUL
 #define ADF_BANK_INT_SRC_SEL_MASK_X 0x44444444UL