* [PATCH edk2-platforms 1/1] Silicon/NXP: rework IoAccessLib to use single function pointer struct
@ 2020-04-23 10:09 Leif Lindholm
2020-04-23 10:55 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-04-23 14:16 ` Pankaj Bansal
0 siblings, 2 replies; 4+ messages in thread
From: Leif Lindholm @ 2020-04-23 10:09 UTC (permalink / raw)
To: devel; +Cc: Meenakshi Aggarwal, Pankaj Bansal
For whatever reason, the function pointer structs (and their retrieval
functions) were split up on data size boundary - separate for 16-bit,
32-bit, and 64-bit.
This makes the already tedious boilerplate require to deal with these
hardware quirks even worse, so unify them into a single function pointer
struct and retrieval function.
Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
Meenakshi, Pankaj - I have no hardware to verify that this patch works,
but it builds cleanlyi and isn't exactly complex.
I will need R-b from at least one of you, and ideally Tested-by as well.
Silicon/NXP/Include/Library/IoAccessLib.h | 52 ++----------
Silicon/NXP/Library/IoAccessLib/IoAccessLib.c | 82 ++++---------------
2 files changed, 22 insertions(+), 112 deletions(-)
diff --git a/Silicon/NXP/Include/Library/IoAccessLib.h b/Silicon/NXP/Include/Library/IoAccessLib.h
index 0b708d544fa7..0f5b19dcf149 100644
--- a/Silicon/NXP/Include/Library/IoAccessLib.h
+++ b/Silicon/NXP/Include/Library/IoAccessLib.h
@@ -15,40 +15,26 @@
/// Structure to have pointer to R/W
/// Mmio operations for 16 bits.
///
-typedef struct _MMIO_OPERATIONS_16 {
+typedef struct _MMIO_OPERATIONS {
UINT16 (*Read16) (UINTN Address);
UINT16 (*Write16) (UINTN Address, UINT16 Value);
UINT16 (*Or16) (UINTN Address, UINT16 OrData);
UINT16 (*And16) (UINTN Address, UINT16 AndData);
UINT16 (*AndThenOr16) (UINTN Address, UINT16 AndData, UINT16 OrData);
-} MMIO_OPERATIONS_16;
-
-///
-/// Structure to have pointer to R/W
-/// Mmio operations for 32 bits.
-///
-typedef struct _MMIO_OPERATIONS_32 {
UINT32 (*Read32) (UINTN Address);
UINT32 (*Write32) (UINTN Address, UINT32 Value);
UINT32 (*Or32) (UINTN Address, UINT32 OrData);
UINT32 (*And32) (UINTN Address, UINT32 AndData);
UINT32 (*AndThenOr32) (UINTN Address, UINT32 AndData, UINT32 OrData);
-} MMIO_OPERATIONS_32;
-
-///
-/// Structure to have pointer to R/W
-/// Mmio operations for 64 bits.
-///
-typedef struct _MMIO_OPERATIONS_64 {
UINT64 (*Read64) (UINTN Address);
UINT64 (*Write64) (UINTN Address, UINT64 Value);
UINT64 (*Or64) (UINTN Address, UINT64 OrData);
UINT64 (*And64) (UINTN Address, UINT64 AndData);
UINT64 (*AndThenOr64) (UINTN Address, UINT64 AndData, UINT64 OrData);
-} MMIO_OPERATIONS_64;
+} MMIO_OPERATIONS;
/**
- Function to return pointer to 16 bit Mmio operations.
+ Function to return pointer to Mmio operations.
@param Swap Flag to tell if Swap is needed or not
on Mmio Operations.
@@ -56,36 +42,8 @@ typedef struct _MMIO_OPERATIONS_64 {
@return Pointer to Mmio Operations.
**/
-MMIO_OPERATIONS_16 *
-GetMmioOperations16 (
- IN BOOLEAN Swap
- );
-
-/**
- Function to return pointer to 32 bit Mmio operations.
-
- @param Swap Flag to tell if Swap is needed or not
- on Mmio Operations.
-
- @return Pointer to Mmio Operations.
-
-**/
-MMIO_OPERATIONS_32 *
-GetMmioOperations32 (
- IN BOOLEAN Swap
- );
-
-/**
- Function to return pointer to 64 bit Mmio operations.
-
- @param Swap Flag to tell if Swap is needed or not
- on Mmio Operations.
-
- @return Pointer to Mmio Operations.
-
-**/
-MMIO_OPERATIONS_64 *
-GetMmioOperations64 (
+MMIO_OPERATIONS *
+GetMmioOperations (
IN BOOLEAN Swap
);
diff --git a/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c b/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
index 6ed83d019a6e..33039afda40f 100644
--- a/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
+++ b/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
@@ -301,39 +301,17 @@ SwapMmioAnd64 (
return MmioAnd64 (Address, SwapBytes64 (AndData));
}
-STATIC MMIO_OPERATIONS_16 SwappingFunctions16 = {
+STATIC MMIO_OPERATIONS SwappingFunctions = {
SwapMmioRead16,
SwapMmioWrite16,
SwapMmioOr16,
SwapMmioAnd16,
SwapMmioAndThenOr16,
-};
-
-STATIC MMIO_OPERATIONS_16 NonSwappingFunctions16 = {
- MmioRead16,
- MmioWrite16,
- MmioOr16,
- MmioAnd16,
- MmioAndThenOr16,
-};
-
-STATIC MMIO_OPERATIONS_32 SwappingFunctions32 = {
SwapMmioRead32,
SwapMmioWrite32,
SwapMmioOr32,
SwapMmioAnd32,
SwapMmioAndThenOr32,
-};
-
-STATIC MMIO_OPERATIONS_32 NonSwappingFunctions32 = {
- MmioRead32,
- MmioWrite32,
- MmioOr32,
- MmioAnd32,
- MmioAndThenOr32,
-};
-
-STATIC MMIO_OPERATIONS_64 SwappingFunctions64 = {
SwapMmioRead64,
SwapMmioWrite64,
SwapMmioOr64,
@@ -341,7 +319,17 @@ STATIC MMIO_OPERATIONS_64 SwappingFunctions64 = {
SwapMmioAndThenOr64,
};
-STATIC MMIO_OPERATIONS_64 NonSwappingFunctions64 = {
+STATIC MMIO_OPERATIONS NonSwappingFunctions = {
+ MmioRead16,
+ MmioWrite16,
+ MmioOr16,
+ MmioAnd16,
+ MmioAndThenOr16,
+ MmioRead32,
+ MmioWrite32,
+ MmioOr32,
+ MmioAnd32,
+ MmioAndThenOr32,
MmioRead64,
MmioWrite64,
MmioOr64,
@@ -350,7 +338,7 @@ STATIC MMIO_OPERATIONS_64 NonSwappingFunctions64 = {
};
/**
- Function to return pointer to 16 bit Mmio operations.
+ Function to return pointer to Mmio operations.
@param Swap Flag to tell if Swap is needed or not
on Mmio Operations.
@@ -358,47 +346,11 @@ STATIC MMIO_OPERATIONS_64 NonSwappingFunctions64 = {
@return Pointer to Mmio Operations.
**/
-MMIO_OPERATIONS_16 *
-GetMmioOperations16 (BOOLEAN Swap) {
+MMIO_OPERATIONS *
+GetMmioOperations (BOOLEAN Swap) {
if (Swap) {
- return &SwappingFunctions16;
+ return &SwappingFunctions;
} else {
- return &NonSwappingFunctions16;
- }
-}
-
-/**
- Function to return pointer to 32 bit Mmio operations.
-
- @param Swap Flag to tell if Swap is needed or not
- on Mmio Operations.
-
- @return Pointer to Mmio Operations.
-
-**/
-MMIO_OPERATIONS_32 *
-GetMmioOperations32 (BOOLEAN Swap) {
- if (Swap) {
- return &SwappingFunctions32;
- } else {
- return &NonSwappingFunctions32;
- }
-}
-
-/**
- Function to return pointer to 64 bit Mmio operations.
-
- @param Swap Flag to tell if Swap is needed or not
- on Mmio Operations.
-
- @return Pointer to Mmio Operations.
-
-**/
-MMIO_OPERATIONS_64 *
-GetMmioOperations64 (BOOLEAN Swap) {
- if (Swap) {
- return &SwappingFunctions64;
- } else {
- return &NonSwappingFunctions64;
+ return &NonSwappingFunctions;
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH edk2-platforms 1/1] Silicon/NXP: rework IoAccessLib to use single function pointer struct
2020-04-23 10:09 [PATCH edk2-platforms 1/1] Silicon/NXP: rework IoAccessLib to use single function pointer struct Leif Lindholm
@ 2020-04-23 10:55 ` Philippe Mathieu-Daudé
2020-04-23 14:16 ` Pankaj Bansal
1 sibling, 0 replies; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-23 10:55 UTC (permalink / raw)
To: devel, leif; +Cc: Meenakshi Aggarwal, Pankaj Bansal
On 4/23/20 12:09 PM, Leif Lindholm wrote:
> For whatever reason, the function pointer structs (and their retrieval
> functions) were split up on data size boundary - separate for 16-bit,
> 32-bit, and 64-bit.
>
> This makes the already tedious boilerplate require to deal with these
> hardware quirks even worse, so unify them into a single function pointer
> struct and retrieval function.
>
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
>
> Meenakshi, Pankaj - I have no hardware to verify that this patch works,
> but it builds cleanlyi and isn't exactly complex.
> I will need R-b from at least one of you, and ideally Tested-by as well.
>
> Silicon/NXP/Include/Library/IoAccessLib.h | 52 ++----------
> Silicon/NXP/Library/IoAccessLib/IoAccessLib.c | 82 ++++---------------
> 2 files changed, 22 insertions(+), 112 deletions(-)
>
> diff --git a/Silicon/NXP/Include/Library/IoAccessLib.h b/Silicon/NXP/Include/Library/IoAccessLib.h
> index 0b708d544fa7..0f5b19dcf149 100644
> --- a/Silicon/NXP/Include/Library/IoAccessLib.h
> +++ b/Silicon/NXP/Include/Library/IoAccessLib.h
> @@ -15,40 +15,26 @@
> /// Structure to have pointer to R/W
> /// Mmio operations for 16 bits.
> ///
> -typedef struct _MMIO_OPERATIONS_16 {
> +typedef struct _MMIO_OPERATIONS {
> UINT16 (*Read16) (UINTN Address);
> UINT16 (*Write16) (UINTN Address, UINT16 Value);
> UINT16 (*Or16) (UINTN Address, UINT16 OrData);
> UINT16 (*And16) (UINTN Address, UINT16 AndData);
> UINT16 (*AndThenOr16) (UINTN Address, UINT16 AndData, UINT16 OrData);
> -} MMIO_OPERATIONS_16;
> -
> -///
> -/// Structure to have pointer to R/W
> -/// Mmio operations for 32 bits.
> -///
> -typedef struct _MMIO_OPERATIONS_32 {
> UINT32 (*Read32) (UINTN Address);
> UINT32 (*Write32) (UINTN Address, UINT32 Value);
> UINT32 (*Or32) (UINTN Address, UINT32 OrData);
> UINT32 (*And32) (UINTN Address, UINT32 AndData);
> UINT32 (*AndThenOr32) (UINTN Address, UINT32 AndData, UINT32 OrData);
> -} MMIO_OPERATIONS_32;
> -
> -///
> -/// Structure to have pointer to R/W
> -/// Mmio operations for 64 bits.
> -///
> -typedef struct _MMIO_OPERATIONS_64 {
> UINT64 (*Read64) (UINTN Address);
> UINT64 (*Write64) (UINTN Address, UINT64 Value);
> UINT64 (*Or64) (UINTN Address, UINT64 OrData);
> UINT64 (*And64) (UINTN Address, UINT64 AndData);
> UINT64 (*AndThenOr64) (UINTN Address, UINT64 AndData, UINT64 OrData);
> -} MMIO_OPERATIONS_64;
> +} MMIO_OPERATIONS;
>
> /**
> - Function to return pointer to 16 bit Mmio operations.
> + Function to return pointer to Mmio operations.
>
> @param Swap Flag to tell if Swap is needed or not
> on Mmio Operations.
> @@ -56,36 +42,8 @@ typedef struct _MMIO_OPERATIONS_64 {
> @return Pointer to Mmio Operations.
>
> **/
> -MMIO_OPERATIONS_16 *
> -GetMmioOperations16 (
> - IN BOOLEAN Swap
> - );
> -
> -/**
> - Function to return pointer to 32 bit Mmio operations.
> -
> - @param Swap Flag to tell if Swap is needed or not
> - on Mmio Operations.
> -
> - @return Pointer to Mmio Operations.
> -
> -**/
> -MMIO_OPERATIONS_32 *
> -GetMmioOperations32 (
> - IN BOOLEAN Swap
> - );
> -
> -/**
> - Function to return pointer to 64 bit Mmio operations.
> -
> - @param Swap Flag to tell if Swap is needed or not
> - on Mmio Operations.
> -
> - @return Pointer to Mmio Operations.
> -
> -**/
> -MMIO_OPERATIONS_64 *
> -GetMmioOperations64 (
> +MMIO_OPERATIONS *
> +GetMmioOperations (
> IN BOOLEAN Swap
> );
>
> diff --git a/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c b/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
> index 6ed83d019a6e..33039afda40f 100644
> --- a/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
> +++ b/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
> @@ -301,39 +301,17 @@ SwapMmioAnd64 (
> return MmioAnd64 (Address, SwapBytes64 (AndData));
> }
>
> -STATIC MMIO_OPERATIONS_16 SwappingFunctions16 = {
> +STATIC MMIO_OPERATIONS SwappingFunctions = {
> SwapMmioRead16,
> SwapMmioWrite16,
> SwapMmioOr16,
> SwapMmioAnd16,
> SwapMmioAndThenOr16,
> -};
> -
> -STATIC MMIO_OPERATIONS_16 NonSwappingFunctions16 = {
> - MmioRead16,
> - MmioWrite16,
> - MmioOr16,
> - MmioAnd16,
> - MmioAndThenOr16,
> -};
> -
> -STATIC MMIO_OPERATIONS_32 SwappingFunctions32 = {
> SwapMmioRead32,
> SwapMmioWrite32,
> SwapMmioOr32,
> SwapMmioAnd32,
> SwapMmioAndThenOr32,
> -};
> -
> -STATIC MMIO_OPERATIONS_32 NonSwappingFunctions32 = {
> - MmioRead32,
> - MmioWrite32,
> - MmioOr32,
> - MmioAnd32,
> - MmioAndThenOr32,
> -};
> -
> -STATIC MMIO_OPERATIONS_64 SwappingFunctions64 = {
> SwapMmioRead64,
> SwapMmioWrite64,
> SwapMmioOr64,
> @@ -341,7 +319,17 @@ STATIC MMIO_OPERATIONS_64 SwappingFunctions64 = {
> SwapMmioAndThenOr64,
> };
>
> -STATIC MMIO_OPERATIONS_64 NonSwappingFunctions64 = {
> +STATIC MMIO_OPERATIONS NonSwappingFunctions = {
> + MmioRead16,
> + MmioWrite16,
> + MmioOr16,
> + MmioAnd16,
> + MmioAndThenOr16,
> + MmioRead32,
> + MmioWrite32,
> + MmioOr32,
> + MmioAnd32,
> + MmioAndThenOr32,
> MmioRead64,
> MmioWrite64,
> MmioOr64,
> @@ -350,7 +338,7 @@ STATIC MMIO_OPERATIONS_64 NonSwappingFunctions64 = {
> };
>
> /**
> - Function to return pointer to 16 bit Mmio operations.
> + Function to return pointer to Mmio operations.
>
> @param Swap Flag to tell if Swap is needed or not
> on Mmio Operations.
> @@ -358,47 +346,11 @@ STATIC MMIO_OPERATIONS_64 NonSwappingFunctions64 = {
> @return Pointer to Mmio Operations.
>
> **/
> -MMIO_OPERATIONS_16 *
> -GetMmioOperations16 (BOOLEAN Swap) {
> +MMIO_OPERATIONS *
> +GetMmioOperations (BOOLEAN Swap) {
> if (Swap) {
> - return &SwappingFunctions16;
> + return &SwappingFunctions;
> } else {
> - return &NonSwappingFunctions16;
> - }
> -}
> -
> -/**
> - Function to return pointer to 32 bit Mmio operations.
> -
> - @param Swap Flag to tell if Swap is needed or not
> - on Mmio Operations.
> -
> - @return Pointer to Mmio Operations.
> -
> -**/
> -MMIO_OPERATIONS_32 *
> -GetMmioOperations32 (BOOLEAN Swap) {
> - if (Swap) {
> - return &SwappingFunctions32;
> - } else {
> - return &NonSwappingFunctions32;
> - }
> -}
> -
> -/**
> - Function to return pointer to 64 bit Mmio operations.
> -
> - @param Swap Flag to tell if Swap is needed or not
> - on Mmio Operations.
> -
> - @return Pointer to Mmio Operations.
> -
> -**/
> -MMIO_OPERATIONS_64 *
> -GetMmioOperations64 (BOOLEAN Swap) {
> - if (Swap) {
> - return &SwappingFunctions64;
> - } else {
> - return &NonSwappingFunctions64;
> + return &NonSwappingFunctions;
> }
> }
>
Nice simplification.
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH edk2-platforms 1/1] Silicon/NXP: rework IoAccessLib to use single function pointer struct
2020-04-23 10:09 [PATCH edk2-platforms 1/1] Silicon/NXP: rework IoAccessLib to use single function pointer struct Leif Lindholm
2020-04-23 10:55 ` [edk2-devel] " Philippe Mathieu-Daudé
@ 2020-04-23 14:16 ` Pankaj Bansal
2020-04-24 11:57 ` Leif Lindholm
1 sibling, 1 reply; 4+ messages in thread
From: Pankaj Bansal @ 2020-04-23 14:16 UTC (permalink / raw)
To: Leif Lindholm, devel@edk2.groups.io; +Cc: Meenakshi Aggarwal
> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Thursday, April 23, 2020 3:39 PM
> To: devel@edk2.groups.io
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Pankaj Bansal
> <pankaj.bansal@nxp.com>
> Subject: [PATCH edk2-platforms 1/1] Silicon/NXP: rework IoAccessLib to use
> single function pointer struct
>
> For whatever reason, the function pointer structs (and their retrieval
> functions) were split up on data size boundary - separate for 16-bit,
> 32-bit, and 64-bit.
>
> This makes the already tedious boilerplate require to deal with these
> hardware quirks even worse, so unify them into a single function pointer
> struct and retrieval function.
>
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
>
> Meenakshi, Pankaj - I have no hardware to verify that this patch works,
> but it builds cleanlyi and isn't exactly complex.
> I will need R-b from at least one of you, and ideally Tested-by as well.
>
Reviewed-by: Pankaj Bansal <pankaj.bansal@nxp.com>
Tested-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> Silicon/NXP/Include/Library/IoAccessLib.h | 52 ++----------
> Silicon/NXP/Library/IoAccessLib/IoAccessLib.c | 82 ++++---------------
> 2 files changed, 22 insertions(+), 112 deletions(-)
>
> diff --git a/Silicon/NXP/Include/Library/IoAccessLib.h
> b/Silicon/NXP/Include/Library/IoAccessLib.h
> index 0b708d544fa7..0f5b19dcf149 100644
> --- a/Silicon/NXP/Include/Library/IoAccessLib.h
> +++ b/Silicon/NXP/Include/Library/IoAccessLib.h
> @@ -15,40 +15,26 @@
> /// Structure to have pointer to R/W
> /// Mmio operations for 16 bits.
> ///
> -typedef struct _MMIO_OPERATIONS_16 {
> +typedef struct _MMIO_OPERATIONS {
> UINT16 (*Read16) (UINTN Address);
> UINT16 (*Write16) (UINTN Address, UINT16 Value);
> UINT16 (*Or16) (UINTN Address, UINT16 OrData);
> UINT16 (*And16) (UINTN Address, UINT16 AndData);
> UINT16 (*AndThenOr16) (UINTN Address, UINT16 AndData, UINT16 OrData);
> -} MMIO_OPERATIONS_16;
> -
> -///
> -/// Structure to have pointer to R/W
> -/// Mmio operations for 32 bits.
> -///
> -typedef struct _MMIO_OPERATIONS_32 {
> UINT32 (*Read32) (UINTN Address);
> UINT32 (*Write32) (UINTN Address, UINT32 Value);
> UINT32 (*Or32) (UINTN Address, UINT32 OrData);
> UINT32 (*And32) (UINTN Address, UINT32 AndData);
> UINT32 (*AndThenOr32) (UINTN Address, UINT32 AndData, UINT32 OrData);
> -} MMIO_OPERATIONS_32;
> -
> -///
> -/// Structure to have pointer to R/W
> -/// Mmio operations for 64 bits.
> -///
> -typedef struct _MMIO_OPERATIONS_64 {
> UINT64 (*Read64) (UINTN Address);
> UINT64 (*Write64) (UINTN Address, UINT64 Value);
> UINT64 (*Or64) (UINTN Address, UINT64 OrData);
> UINT64 (*And64) (UINTN Address, UINT64 AndData);
> UINT64 (*AndThenOr64) (UINTN Address, UINT64 AndData, UINT64 OrData);
> -} MMIO_OPERATIONS_64;
> +} MMIO_OPERATIONS;
>
> /**
> - Function to return pointer to 16 bit Mmio operations.
> + Function to return pointer to Mmio operations.
>
> @param Swap Flag to tell if Swap is needed or not
> on Mmio Operations.
> @@ -56,36 +42,8 @@ typedef struct _MMIO_OPERATIONS_64 {
> @return Pointer to Mmio Operations.
>
> **/
> -MMIO_OPERATIONS_16 *
> -GetMmioOperations16 (
> - IN BOOLEAN Swap
> - );
> -
> -/**
> - Function to return pointer to 32 bit Mmio operations.
> -
> - @param Swap Flag to tell if Swap is needed or not
> - on Mmio Operations.
> -
> - @return Pointer to Mmio Operations.
> -
> -**/
> -MMIO_OPERATIONS_32 *
> -GetMmioOperations32 (
> - IN BOOLEAN Swap
> - );
> -
> -/**
> - Function to return pointer to 64 bit Mmio operations.
> -
> - @param Swap Flag to tell if Swap is needed or not
> - on Mmio Operations.
> -
> - @return Pointer to Mmio Operations.
> -
> -**/
> -MMIO_OPERATIONS_64 *
> -GetMmioOperations64 (
> +MMIO_OPERATIONS *
> +GetMmioOperations (
> IN BOOLEAN Swap
> );
>
> diff --git a/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
> b/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
> index 6ed83d019a6e..33039afda40f 100644
> --- a/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
> +++ b/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
> @@ -301,39 +301,17 @@ SwapMmioAnd64 (
> return MmioAnd64 (Address, SwapBytes64 (AndData));
> }
>
> -STATIC MMIO_OPERATIONS_16 SwappingFunctions16 = {
> +STATIC MMIO_OPERATIONS SwappingFunctions = {
> SwapMmioRead16,
> SwapMmioWrite16,
> SwapMmioOr16,
> SwapMmioAnd16,
> SwapMmioAndThenOr16,
> -};
> -
> -STATIC MMIO_OPERATIONS_16 NonSwappingFunctions16 = {
> - MmioRead16,
> - MmioWrite16,
> - MmioOr16,
> - MmioAnd16,
> - MmioAndThenOr16,
> -};
> -
> -STATIC MMIO_OPERATIONS_32 SwappingFunctions32 = {
> SwapMmioRead32,
> SwapMmioWrite32,
> SwapMmioOr32,
> SwapMmioAnd32,
> SwapMmioAndThenOr32,
> -};
> -
> -STATIC MMIO_OPERATIONS_32 NonSwappingFunctions32 = {
> - MmioRead32,
> - MmioWrite32,
> - MmioOr32,
> - MmioAnd32,
> - MmioAndThenOr32,
> -};
> -
> -STATIC MMIO_OPERATIONS_64 SwappingFunctions64 = {
> SwapMmioRead64,
> SwapMmioWrite64,
> SwapMmioOr64,
> @@ -341,7 +319,17 @@ STATIC MMIO_OPERATIONS_64 SwappingFunctions64
> = {
> SwapMmioAndThenOr64,
> };
>
> -STATIC MMIO_OPERATIONS_64 NonSwappingFunctions64 = {
> +STATIC MMIO_OPERATIONS NonSwappingFunctions = {
> + MmioRead16,
> + MmioWrite16,
> + MmioOr16,
> + MmioAnd16,
> + MmioAndThenOr16,
> + MmioRead32,
> + MmioWrite32,
> + MmioOr32,
> + MmioAnd32,
> + MmioAndThenOr32,
> MmioRead64,
> MmioWrite64,
> MmioOr64,
> @@ -350,7 +338,7 @@ STATIC MMIO_OPERATIONS_64
> NonSwappingFunctions64 = {
> };
>
> /**
> - Function to return pointer to 16 bit Mmio operations.
> + Function to return pointer to Mmio operations.
>
> @param Swap Flag to tell if Swap is needed or not
> on Mmio Operations.
> @@ -358,47 +346,11 @@ STATIC MMIO_OPERATIONS_64
> NonSwappingFunctions64 = {
> @return Pointer to Mmio Operations.
>
> **/
> -MMIO_OPERATIONS_16 *
> -GetMmioOperations16 (BOOLEAN Swap) {
> +MMIO_OPERATIONS *
> +GetMmioOperations (BOOLEAN Swap) {
> if (Swap) {
> - return &SwappingFunctions16;
> + return &SwappingFunctions;
> } else {
> - return &NonSwappingFunctions16;
> - }
> -}
> -
> -/**
> - Function to return pointer to 32 bit Mmio operations.
> -
> - @param Swap Flag to tell if Swap is needed or not
> - on Mmio Operations.
> -
> - @return Pointer to Mmio Operations.
> -
> -**/
> -MMIO_OPERATIONS_32 *
> -GetMmioOperations32 (BOOLEAN Swap) {
> - if (Swap) {
> - return &SwappingFunctions32;
> - } else {
> - return &NonSwappingFunctions32;
> - }
> -}
> -
> -/**
> - Function to return pointer to 64 bit Mmio operations.
> -
> - @param Swap Flag to tell if Swap is needed or not
> - on Mmio Operations.
> -
> - @return Pointer to Mmio Operations.
> -
> -**/
> -MMIO_OPERATIONS_64 *
> -GetMmioOperations64 (BOOLEAN Swap) {
> - if (Swap) {
> - return &SwappingFunctions64;
> - } else {
> - return &NonSwappingFunctions64;
> + return &NonSwappingFunctions;
> }
> }
> --
> 2.20.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH edk2-platforms 1/1] Silicon/NXP: rework IoAccessLib to use single function pointer struct
2020-04-23 14:16 ` Pankaj Bansal
@ 2020-04-24 11:57 ` Leif Lindholm
0 siblings, 0 replies; 4+ messages in thread
From: Leif Lindholm @ 2020-04-24 11:57 UTC (permalink / raw)
To: Pankaj Bansal; +Cc: devel@edk2.groups.io, Meenakshi Aggarwal
On Thu, Apr 23, 2020 at 14:16:28 +0000, Pankaj Bansal wrote:
>
>
> > -----Original Message-----
> > From: Leif Lindholm <leif@nuviainc.com>
> > Sent: Thursday, April 23, 2020 3:39 PM
> > To: devel@edk2.groups.io
> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Pankaj Bansal
> > <pankaj.bansal@nxp.com>
> > Subject: [PATCH edk2-platforms 1/1] Silicon/NXP: rework IoAccessLib to use
> > single function pointer struct
> >
> > For whatever reason, the function pointer structs (and their retrieval
> > functions) were split up on data size boundary - separate for 16-bit,
> > 32-bit, and 64-bit.
> >
> > This makes the already tedious boilerplate require to deal with these
> > hardware quirks even worse, so unify them into a single function pointer
> > struct and retrieval function.
> >
> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> > Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> >
> > Meenakshi, Pankaj - I have no hardware to verify that this patch works,
> > but it builds cleanlyi and isn't exactly complex.
> > I will need R-b from at least one of you, and ideally Tested-by as well.
> >
>
> Reviewed-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> Tested-by: Pankaj Bansal <pankaj.bansal@nxp.com>
Thanks!
Pushed as ce617e442c70.
/
Leif
> > Silicon/NXP/Include/Library/IoAccessLib.h | 52 ++----------
> > Silicon/NXP/Library/IoAccessLib/IoAccessLib.c | 82 ++++---------------
> > 2 files changed, 22 insertions(+), 112 deletions(-)
> >
> > diff --git a/Silicon/NXP/Include/Library/IoAccessLib.h
> > b/Silicon/NXP/Include/Library/IoAccessLib.h
> > index 0b708d544fa7..0f5b19dcf149 100644
> > --- a/Silicon/NXP/Include/Library/IoAccessLib.h
> > +++ b/Silicon/NXP/Include/Library/IoAccessLib.h
> > @@ -15,40 +15,26 @@
> > /// Structure to have pointer to R/W
> > /// Mmio operations for 16 bits.
> > ///
> > -typedef struct _MMIO_OPERATIONS_16 {
> > +typedef struct _MMIO_OPERATIONS {
> > UINT16 (*Read16) (UINTN Address);
> > UINT16 (*Write16) (UINTN Address, UINT16 Value);
> > UINT16 (*Or16) (UINTN Address, UINT16 OrData);
> > UINT16 (*And16) (UINTN Address, UINT16 AndData);
> > UINT16 (*AndThenOr16) (UINTN Address, UINT16 AndData, UINT16 OrData);
> > -} MMIO_OPERATIONS_16;
> > -
> > -///
> > -/// Structure to have pointer to R/W
> > -/// Mmio operations for 32 bits.
> > -///
> > -typedef struct _MMIO_OPERATIONS_32 {
> > UINT32 (*Read32) (UINTN Address);
> > UINT32 (*Write32) (UINTN Address, UINT32 Value);
> > UINT32 (*Or32) (UINTN Address, UINT32 OrData);
> > UINT32 (*And32) (UINTN Address, UINT32 AndData);
> > UINT32 (*AndThenOr32) (UINTN Address, UINT32 AndData, UINT32 OrData);
> > -} MMIO_OPERATIONS_32;
> > -
> > -///
> > -/// Structure to have pointer to R/W
> > -/// Mmio operations for 64 bits.
> > -///
> > -typedef struct _MMIO_OPERATIONS_64 {
> > UINT64 (*Read64) (UINTN Address);
> > UINT64 (*Write64) (UINTN Address, UINT64 Value);
> > UINT64 (*Or64) (UINTN Address, UINT64 OrData);
> > UINT64 (*And64) (UINTN Address, UINT64 AndData);
> > UINT64 (*AndThenOr64) (UINTN Address, UINT64 AndData, UINT64 OrData);
> > -} MMIO_OPERATIONS_64;
> > +} MMIO_OPERATIONS;
> >
> > /**
> > - Function to return pointer to 16 bit Mmio operations.
> > + Function to return pointer to Mmio operations.
> >
> > @param Swap Flag to tell if Swap is needed or not
> > on Mmio Operations.
> > @@ -56,36 +42,8 @@ typedef struct _MMIO_OPERATIONS_64 {
> > @return Pointer to Mmio Operations.
> >
> > **/
> > -MMIO_OPERATIONS_16 *
> > -GetMmioOperations16 (
> > - IN BOOLEAN Swap
> > - );
> > -
> > -/**
> > - Function to return pointer to 32 bit Mmio operations.
> > -
> > - @param Swap Flag to tell if Swap is needed or not
> > - on Mmio Operations.
> > -
> > - @return Pointer to Mmio Operations.
> > -
> > -**/
> > -MMIO_OPERATIONS_32 *
> > -GetMmioOperations32 (
> > - IN BOOLEAN Swap
> > - );
> > -
> > -/**
> > - Function to return pointer to 64 bit Mmio operations.
> > -
> > - @param Swap Flag to tell if Swap is needed or not
> > - on Mmio Operations.
> > -
> > - @return Pointer to Mmio Operations.
> > -
> > -**/
> > -MMIO_OPERATIONS_64 *
> > -GetMmioOperations64 (
> > +MMIO_OPERATIONS *
> > +GetMmioOperations (
> > IN BOOLEAN Swap
> > );
> >
> > diff --git a/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
> > b/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
> > index 6ed83d019a6e..33039afda40f 100644
> > --- a/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
> > +++ b/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
> > @@ -301,39 +301,17 @@ SwapMmioAnd64 (
> > return MmioAnd64 (Address, SwapBytes64 (AndData));
> > }
> >
> > -STATIC MMIO_OPERATIONS_16 SwappingFunctions16 = {
> > +STATIC MMIO_OPERATIONS SwappingFunctions = {
> > SwapMmioRead16,
> > SwapMmioWrite16,
> > SwapMmioOr16,
> > SwapMmioAnd16,
> > SwapMmioAndThenOr16,
> > -};
> > -
> > -STATIC MMIO_OPERATIONS_16 NonSwappingFunctions16 = {
> > - MmioRead16,
> > - MmioWrite16,
> > - MmioOr16,
> > - MmioAnd16,
> > - MmioAndThenOr16,
> > -};
> > -
> > -STATIC MMIO_OPERATIONS_32 SwappingFunctions32 = {
> > SwapMmioRead32,
> > SwapMmioWrite32,
> > SwapMmioOr32,
> > SwapMmioAnd32,
> > SwapMmioAndThenOr32,
> > -};
> > -
> > -STATIC MMIO_OPERATIONS_32 NonSwappingFunctions32 = {
> > - MmioRead32,
> > - MmioWrite32,
> > - MmioOr32,
> > - MmioAnd32,
> > - MmioAndThenOr32,
> > -};
> > -
> > -STATIC MMIO_OPERATIONS_64 SwappingFunctions64 = {
> > SwapMmioRead64,
> > SwapMmioWrite64,
> > SwapMmioOr64,
> > @@ -341,7 +319,17 @@ STATIC MMIO_OPERATIONS_64 SwappingFunctions64
> > = {
> > SwapMmioAndThenOr64,
> > };
> >
> > -STATIC MMIO_OPERATIONS_64 NonSwappingFunctions64 = {
> > +STATIC MMIO_OPERATIONS NonSwappingFunctions = {
> > + MmioRead16,
> > + MmioWrite16,
> > + MmioOr16,
> > + MmioAnd16,
> > + MmioAndThenOr16,
> > + MmioRead32,
> > + MmioWrite32,
> > + MmioOr32,
> > + MmioAnd32,
> > + MmioAndThenOr32,
> > MmioRead64,
> > MmioWrite64,
> > MmioOr64,
> > @@ -350,7 +338,7 @@ STATIC MMIO_OPERATIONS_64
> > NonSwappingFunctions64 = {
> > };
> >
> > /**
> > - Function to return pointer to 16 bit Mmio operations.
> > + Function to return pointer to Mmio operations.
> >
> > @param Swap Flag to tell if Swap is needed or not
> > on Mmio Operations.
> > @@ -358,47 +346,11 @@ STATIC MMIO_OPERATIONS_64
> > NonSwappingFunctions64 = {
> > @return Pointer to Mmio Operations.
> >
> > **/
> > -MMIO_OPERATIONS_16 *
> > -GetMmioOperations16 (BOOLEAN Swap) {
> > +MMIO_OPERATIONS *
> > +GetMmioOperations (BOOLEAN Swap) {
> > if (Swap) {
> > - return &SwappingFunctions16;
> > + return &SwappingFunctions;
> > } else {
> > - return &NonSwappingFunctions16;
> > - }
> > -}
> > -
> > -/**
> > - Function to return pointer to 32 bit Mmio operations.
> > -
> > - @param Swap Flag to tell if Swap is needed or not
> > - on Mmio Operations.
> > -
> > - @return Pointer to Mmio Operations.
> > -
> > -**/
> > -MMIO_OPERATIONS_32 *
> > -GetMmioOperations32 (BOOLEAN Swap) {
> > - if (Swap) {
> > - return &SwappingFunctions32;
> > - } else {
> > - return &NonSwappingFunctions32;
> > - }
> > -}
> > -
> > -/**
> > - Function to return pointer to 64 bit Mmio operations.
> > -
> > - @param Swap Flag to tell if Swap is needed or not
> > - on Mmio Operations.
> > -
> > - @return Pointer to Mmio Operations.
> > -
> > -**/
> > -MMIO_OPERATIONS_64 *
> > -GetMmioOperations64 (BOOLEAN Swap) {
> > - if (Swap) {
> > - return &SwappingFunctions64;
> > - } else {
> > - return &NonSwappingFunctions64;
> > + return &NonSwappingFunctions;
> > }
> > }
> > --
> > 2.20.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-24 11:57 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-23 10:09 [PATCH edk2-platforms 1/1] Silicon/NXP: rework IoAccessLib to use single function pointer struct Leif Lindholm
2020-04-23 10:55 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-04-23 14:16 ` Pankaj Bansal
2020-04-24 11:57 ` Leif Lindholm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox