Re: [PATCH edk2-platforms 1/1] Silicon/NXP: rework IoAccessLib to use single function pointer struct

["Pankaj Bansal" <pankaj.bansal@nxp.com>]

> -----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