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

["Leif Lindholm" <leif@nuviainc.com>]

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
>