From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by mx.groups.io with SMTP id smtpd.web11.3941.1587729434965259543 for ; Fri, 24 Apr 2020 04:57:15 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=1QQXGIA/; spf=pass (domain: nuviainc.com, ip: 209.85.128.67, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f67.google.com with SMTP id v8so9832788wma.0 for ; Fri, 24 Apr 2020 04:57:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=liod4WQ44Yzpi1nm3idV02iaprTNaIR/cZGMRd2u3mI=; b=1QQXGIA/AMIbsMi0BGe9f3iXH9Vty8PDDUCiCZTeDEiHf3RsPmFaW+oA1S/za9bqrn pcz4b6xLBKelbVxSk7eez1O10YZORJE+kpCw8tLX8p16CUcxmgufArMdgP5+0yjKNbPU qHs7HvdXBQYvYoE9iUyrZyOvVPi0sKPGUSYlyvHMrqmM/4VC7oRpCYuPq1bi/7mxU4JV 3mcHMmd53hM3fDHjMf7AMcsfr7FXNfxDiy1+KNbGLlJsqlp+MVStWSp71lW23BnyGM6A MgAcMITC28kbSMjdbE/aPC69SMe3YxX8dO/obdljXSIHBa15ur4kVZbOR/0zbulGaMo3 ZFkw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=liod4WQ44Yzpi1nm3idV02iaprTNaIR/cZGMRd2u3mI=; b=Gz/WWO/IePESFoGSpQ2Hu0AqYZyq0BdESui2ntkdG0qgX5wk2mnSne7uHkHJj8U7OQ 6RXVb9Po+86TzEOVXrwMzlgrJt9ietfgSvZXDyEj9O6ApvstcOIBi8BWiKUmiSqg+Gv+ wQ3ELBTRsgU2QX1A8X9DPjkcxvfCCzepyrPDfTDNk3SjqqA0GTSGlGHgyA31y9b5dfrd YZNf4U9F6y8MsMvBM5SSeo5fDma8WXFmrGD/MOX075gnkZFAY3+8z0n1CEf63Vjhy2oY IRhkDtU5qu34VSVjh1wkIw8v1ICS8vfHos/ByrXeEXpzRI9rKk7sm+FRv0emnQ4VNNsr HPnA== X-Gm-Message-State: AGi0PuYMORew5BKUNn1LOnMONUSE0zco0HZ1H/ctuw6KWF7qPF+I9dYH r+neWXKS9jM3pDIs+PeHzsMbKg== X-Google-Smtp-Source: APiQypIJEZ6qpAIvbKOGpfWZX5nwXb0XKoRr84CI3CPtLbd0VUE9dP0MjOQxk0kbGaUWAjvNdF4StA== X-Received: by 2002:a1c:3884:: with SMTP id f126mr10203797wma.91.1587729433568; Fri, 24 Apr 2020 04:57:13 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id t20sm2487716wmi.2.2020.04.24.04.57.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 24 Apr 2020 04:57:13 -0700 (PDT) Date: Fri, 24 Apr 2020 12:57:11 +0100 From: "Leif Lindholm" To: Pankaj Bansal Cc: "devel@edk2.groups.io" , Meenakshi Aggarwal Subject: Re: [PATCH edk2-platforms 1/1] Silicon/NXP: rework IoAccessLib to use single function pointer struct Message-ID: <20200424115711.GL14075@vanye> References: <20200423100916.13064-1-leif@nuviainc.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Apr 23, 2020 at 14:16:28 +0000, Pankaj Bansal wrote: > > > > -----Original Message----- > > From: Leif Lindholm > > Sent: Thursday, April 23, 2020 3:39 PM > > To: devel@edk2.groups.io > > Cc: Meenakshi Aggarwal ; Pankaj Bansal > > > > 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 > > Cc: Pankaj Bansal > > Signed-off-by: Leif Lindholm > > --- > > > > 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 > Tested-by: Pankaj Bansal 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 >