From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web10.3936.1587639309018037885 for ; Thu, 23 Apr 2020 03:55:09 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=JMcUNsf8; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: philmd@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1587639307; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=avbfwDLUUAKrnHzdvnclfzI338MlL86WkLrSO1AgHr8=; b=JMcUNsf8rIC40jbc7SFA80DLS/1RoCj4z8hzhmlJz2vISfBs9fZ/bqJVAP8o2mZtwn9Igc T64tjm8aaahKFa2Wv5Qf/Nic2T1x0WHB/oV5OYETbSelLVk4v02YPCsMYFeWO7CbkhLL9d NtjIiJefhRXSnIr5EzZw9f4DDKH6CJA= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-351-8bO55n-ONoiWjijiKh3wiA-1; Thu, 23 Apr 2020 06:55:05 -0400 X-MC-Unique: 8bO55n-ONoiWjijiKh3wiA-1 Received: by mail-wr1-f70.google.com with SMTP id 11so2675143wrc.3 for ; Thu, 23 Apr 2020 03:55:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=avbfwDLUUAKrnHzdvnclfzI338MlL86WkLrSO1AgHr8=; b=D/FtjbI5cjpPV9xEMW1mfXphWIcQmLhtZM6xMGwLhrU83quLNc+3IlWayqDRApuvbs dkr0t9V/69Qt4LeU7DWlqQ7RcHqsfiTbyKRqpSekbEN7JjxPC1F7c2WpZjIeDZWzuA7n 7QV7TtXjsrAJ458dYypfaiVor9ySIfHlL+x5Dy4tS2N7PhFz7/uBoGUftGOeZBbCzTs5 dogU+NtF445bv8omxMF1dNSVoNpDh02Oe3caxGJVh2NdsJIawMSDkw74brsVoyh3Unrd shigbx1cTZzihi6jOj3HWqMMaZpjVzjHPSt9YbLoZUYT8tb7NfYnFnxgoVR698R61c5b BtLw== X-Gm-Message-State: AGi0Pub8ZshTEinwRjWyKCEGSoyMWcsaBXDS5ejgT1Po8oHjJF7LxGGQ f5iHDfy0HQ/zCpv5OClgkByMIird6whEMbiyPYFCwKglldpNpkqrUUwFDpXA1H5cBaIJqhJR7fB EEE9I3P97GUWYVw== X-Received: by 2002:a1c:7706:: with SMTP id t6mr3543053wmi.110.1587639304488; Thu, 23 Apr 2020 03:55:04 -0700 (PDT) X-Google-Smtp-Source: APiQypIvRfevssiBsTcKRnSoJHdlT7OFZSApzg65RgzwOvBFUYXy/uSxMQWnzpP/LMSJB4NQIj2hWw== X-Received: by 2002:a1c:7706:: with SMTP id t6mr3543031wmi.110.1587639304233; Thu, 23 Apr 2020 03:55:04 -0700 (PDT) Return-Path: Received: from [192.168.1.39] (116.red-83-42-57.dynamicip.rima-tde.net. [83.42.57.116]) by smtp.gmail.com with ESMTPSA id 33sm3301450wrp.5.2020.04.23.03.55.03 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 23 Apr 2020 03:55:03 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH edk2-platforms 1/1] Silicon/NXP: rework IoAccessLib to use single function pointer struct To: devel@edk2.groups.io, leif@nuviainc.com Cc: Meenakshi Aggarwal , Pankaj Bansal References: <20200423100916.13064-1-leif@nuviainc.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Message-ID: <3abc84d0-4886-efcf-356c-f7fb510a173e@redhat.com> Date: Thu, 23 Apr 2020 12:55:03 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: <20200423100916.13064-1-leif@nuviainc.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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 > 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. > > 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