From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c0c::22a; helo=mail-wr0-x22a.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr0-x22a.google.com (mail-wr0-x22a.google.com [IPv6:2a00:1450:400c:c0c::22a]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8C802221660DA for ; Wed, 29 Nov 2017 04:46:54 -0800 (PST) Received: by mail-wr0-x22a.google.com with SMTP id z34so3275352wrz.10 for ; Wed, 29 Nov 2017 04:51:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=PFfEA6sXZyZ+l3o7hSZcseBSSZlG35bSeduxhPDxTBM=; b=jukmiIrhuyfo56wMsVYpEy8cqNb+LPiYGAGixAE1ieNeEmXxcTYk8xFwCEp2hEIi76 cH6FWcBD2jjQNqmCVe2S49YI5ltq2kEYrLOZlSA7gIktoWj/8XJoC/gmHz/twn3yH2Sd c6c2MmYevDOOd8UjkxRhOwXxi05jFriGsB5FQ= 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:content-transfer-encoding :in-reply-to:user-agent; bh=PFfEA6sXZyZ+l3o7hSZcseBSSZlG35bSeduxhPDxTBM=; b=j589f8c5Fa8AXuW47/NcEJYwM0aSbQEhXvoaFTqRAgr7bTr9qXT5HKGx4vbeLPT+5F DBhMKSKizt5U6VQgoucYe5b0pZ4/vkuVEiFKY8mW6304lsqDY3kWo9t85XV+KuSYl10r 0P7AWPXa7t3t3OoccIkS8U7HtygnacX8sKSfr4eXsozipqCnjC0KAiDZlQM3cq+KbKBY pE6IBTxzVBDO1dZP2s4Kfx36xr08Z8IJXGjbY/z8OFszphKlWipxUGrBCW7Cd24zhrAo 2bKAXDMpOPvxaz/lZx/SQk6rgyZ2ezQh/zp08h8LB8oVKG7xcxQ9s1Wop3lemyKTapNf urCQ== X-Gm-Message-State: AJaThX7ccIQpR1glW4XmYB05/CU+W4X2muRZSVOSZez2zWQa2v+pcS5t ysK1/Rzf3RULqnskv6pggIzAtQ== X-Google-Smtp-Source: AGs4zMYN/z18sG+JMCKgJa7Zpm72PhjXrp63BQA9BTUYrv3X7dhQiKqDZs3YluneKrGvxC9J3y/jOA== X-Received: by 10.223.181.132 with SMTP id c4mr2372556wre.42.1511959876928; Wed, 29 Nov 2017 04:51:16 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id w21sm874005wra.30.2017.11.29.04.51.15 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 29 Nov 2017 04:51:15 -0800 (PST) Date: Wed, 29 Nov 2017 12:51:14 +0000 From: Leif Lindholm To: Meenakshi Aggarwal , "Gao, Liming" Cc: "edk2-devel@lists.01.org" , Udit Kumar , Ard Biesheuvel , "Kinney, Michael D" Message-ID: <20171129125114.i3y446ds4qrkohxl@bivouac.eciton.net> References: <1476437615-29534-1-git-send-email-bhupesh.sharma@nxp.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14B49505E@shsmsx102.ccr.corp.intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E16B65D@SHSMSX104.ccr.corp.intel.com> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Nov 2017 12:46:55 -0000 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit Hi Meenakshi, I finally got around to looking at the watchdog code (that uses this library), and that has convinced me the best solution is to do what Liming proposed. Looking at this snippet: > +STATIC > +UINT16 > +EFIAPI > +WdogRead ( > +  IN  UINTN     Address > +  ) > +{ > +  if (FixedPcdGetBool (PcdWdogBigEndian)) { > +    return BeMmioRead16 (Address); > +  } else { > +    return MmioRead16(Address); > +  } This is actually a pretty good demonstration of the arguments that were made with regards to how the BeIoLib could be just another implementation of IoLib. You would then just do (in your .dsc):   Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf {     IoLib|   } This causes the big-endian version of the library to be used for this component. This makes these Wdog functions and the Pcd redundant, and the rest of the code can use MmioRead16()/MmioWrite16() directly. But then, it is not really a big-endian or litte-endian version of the library we need. We always know which endianness we are building for. What we need is a byteswapping flavour of IoLib. So Liming, what if we do something like adding a MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSwap.inf --- [Defines] INF_VERSION = 0x0001001A BASE_NAME = BaseIoLibIntrinsicSwap MODULE_UNI_FILE = BaseIoLibIntrinsic.uni FILE_GUID = d4a60d44-3688-4a50-b2d0-5c6fc2422523 MODULE_TYPE = BASE VERSION_STRING = 1.0 LIBRARY_CLASS = IoLib # # VALID_ARCHITECTURES = IA32 X64 EBC IPF ARM AARCH64 # [Sources] BaseIoLibIntrinsicInternal.h IoHighLevel.c IoLib.c IoLibEbc.c # Asserts on all i/o port accesses IoLibMmioBuffer.c [Packages] MdePkg/MdePkg.dec [LibraryClasses] DebugLib BaseLib [BuildOptions] GCC:*_*_*_CC_FLAGS = -DSWAP_BYTES --- And then add #ifdef SWAP_BYTES return SwapBytesXX (Value); #else return Value; #fi for the read operations and #ifdef SWAP_BYTES *(type)Address = SwapBytesXX (Value); #else *(type)Address = Value; #fi for the write operations in IoLib.c? / Leif On Mon, Nov 27, 2017 at 06:06:33AM +0000, Meenakshi Aggarwal wrote: > Hi Leif, > > This is regarding Big-Endian Library patch ([PATCH v2 1/9] > Platform/NXP: Add support for Big Endian Mmio APIs) > > We have started this discussion before and the suggestion was to > create a separate .inf file keeping APIs name same > e.g. MmioRead/MmioWrite in MdePkg. > > But we can't go with this approach (reason mentioned by Udit). > > So please suggest if we should keep this library under Platform/NXP > or I send a new patch moving this library in MdePkg. > > But we have to keep a different name for Big Endian MMIO APIs. > > > Thanks, > Meenakshi > > > > -----Original Message----- > > From: Udit Kumar > > Sent: Monday, October 23, 2017 12:38 PM > > To: Gao, Liming ; Meenakshi Aggarwal > > ; Ard Biesheuvel > > ; Kinney, Michael D > > ; edk2-devel@lists.01.org > > Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for big-endian > > MMIO > > > > Hi Meenakshi/Liming, > > My 2 cents, around this. > > > > 1) > > Having a new lib for BE read might not be helpful for us, e.g. a IP which is in > > BE mode access the UART for print or system registers which are in LE, then > > with new Lib, we will get all read/write in BE mode > > > > 2) > > Especially for our IPs, which are changing from BE to LE depending on > > platform. > > As said before, having BE read lib with API name of MmioRead32 etc, will not > > help (I guess Meenakshi already seen some problems around this) Adding a > > new lib with MmioRead32BE API name could help, but IP driver we need to > > take care of IP mode either by Pcd or #define, to select MmioRead32 or > > MmioRead32BE. > > This conditional compile needs to be done for all IPs (which works in BE/LE > > mode on different platforms). > > > > My preferred way of implementation to use one function in IP driver, And > > based on IP mode, do the switch. > > > > New Lib could have function like below > > MmioRead32Generic(IN UINTN Address, BOOL IsIPBE) { > > UINT32 Value; > > > > ASSERT ((Address & 3) == 0); > > Value = *(volatile UINT32*)Address; > > If(IsIPBE) > > Value = SwapBytes32(Value); > > return Value; > > } > > > > And IP driver can use it > > MmioRead32Generic(ADDR, > > FixedPcdGet(This_IP_Mode_For_This_platform) > > > > Comments are welcome. > > > > Regards > > Udit > > > > > -----Original Message----- > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > > Gao, Liming > > > Sent: Monday, October 16, 2017 8:48 AM > > > To: Meenakshi Aggarwal ; Ard Biesheuvel > > > ; Kinney, Michael D > > > ; edk2-devel@lists.01.org > > > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for > > > big-endian MMIO > > > > > > Meenakshi: > > > I suggest to introduce new IoLib library instance, not to add new IoLib > > APIs. > > > New IoLib library instance will perform IO operation as the big > > > endian. You can update MdePkg/Library/BaseIoLibIntrinsic instance, add > > > new source file and new INF for it. > > > > > > UINT32 > > > EFIAPI > > > MmioRead32 ( > > > IN UINTN Address > > > ) > > > { > > > UINT32 Value; > > > > > > ASSERT ((Address & 3) == 0); > > > Value = *(volatile UINT32*)Address; > > > return SwapBytes32(Value); > > > } > > > > > > Thanks > > > Liming > > > >-----Original Message----- > > > >From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] > > > >Sent: Friday, October 13, 2017 2:07 PM > > > >To: Ard Biesheuvel ; Kinney, Michael D > > > >; edk2-devel@lists.01.org; Gao, Liming > > > > > > > >Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for > > > >big-endian MMIO > > > > > > > >Hi All, > > > > > > > > > > > >It’s a pretty old discussion, we have left the upstreaming of NXP > > > >package in between because of some other work, but have started it > > > >again > > > now. > > > > > > > > > > > >Issue : Few NXP modules support Big Endian MMIOs as these are ported > > > >from PowerPC. > > > > > > > >Solution suggested : Create a separate library for BE MMIO APIs. > > > > > > > > > > > >So what I have done is, I have created a separate library to support > > > >BE MMIO APIs and currently keeping it to my package. > > > >This library is basically a wrapper over existing MMIO APIs. > > > > > > > >UINT32 > > > >EFIAPI > > > >BeMmioRead32 ( > > > > IN UINTN Address > > > > ) > > > >{ > > > > UINT32 Value; > > > > > > > > Value = MmioRead32(Address); > > > > > > > > return SwapBytes32(Value); > > > >} > > > > > > > > > > > >Need your opinion on below optinos: > > > > > > > >1. Will this be a good idea to make this library a part of MdePkg? OR > > > > > > > >2. Add a new file e.g. IoBeMmio.c like IoHighLevel.c in > > > >MdePkg/Library/BaseIoLibIntrinsic/ > > > > And made these APIs a part of IoLib itself. OR > > > > > > > >3. Keep this library internal to NXP package. > > > > > > > > > > > >Please provide your inputs. > > > > > > > > > > > >Thanks & Regards, > > > >Meenakshi > > > > > > > >> -----Original Message----- > > > >> From: Bhupesh Sharma > > > >> Sent: Monday, October 17, 2016 3:28 PM > > > >> To: Ard Biesheuvel ; Kinney, Michael D > > > >> > > > >> Cc: Gao, Liming ; edk2-devel@ml01.01.org; > > > >> Meenakshi Aggarwal > > > >> Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for > > > >> big-endian MMIO > > > >> > > > >> Hi Ard, > > > >> > > > >> > -----Original Message----- > > > >> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > > > >> > Sent: Monday, October 17, 2016 1:12 PM > > > >> > To: Kinney, Michael D > > > >> > Cc: Gao, Liming ; Bhupesh Sharma > > > >> > ; edk2-devel@ml01.01.org > > > >> > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support for > > > >> > big- endian MMIO > > > >> > > > > >> > On 17 October 2016 at 05:10, Kinney, Michael D > > > >> > wrote: > > > >> > > Bhupesh, > > > >> > > > > > >> > > It is also possible to add an ARM specific PCD to select > > > >> > > endianness and update > > > >> > > MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c to use that PCD in > > > >> > > MmioRead/Write() APIs in that file to support both endian types. > > > >> > > You can use the SwapBytesxx() functions from BaseLib(as > > > >> > Laszlo > > > >> > > suggested) based on the setting of this ARM specific PCD. > > > >> > > > > > >> > > Modules that link against this lib can select endianness by > > > >> > > setting PCD in the scope of that module. > > > >> > > > > > >> > > The IPF version of IoLib uses an IPF specific PCD to translate > > > >> > > I/O port accesses to MMIO accesses. So there is already an > > > >> > > example of an arch specific PCD in this lib instance. > > > >> > > > > > >> > > > > >> > This is not a platform wide thing, it is a per-device property > > > >> > whether the MMIO occurs in big endian or little endian manner. > > > >> > > > > >> > So I think Liming's suggestion makes sense: create an IoLib > > > >> > implementation that performs the byte swapping, and selectively > > > >> > incorporate it into drivers that require it using > > > >> > > > > >> > BeMmioDeviceDxe.inf { > > > >> > > > > >> > IoLib|SomePkg/Library/BigEndianIoLib.inf > > > >> > } > > > >> > > > >> That's correct. I think creating a separate IoLib for byte-swapping > > > >> makes sense. > > > >> > > > >> We will rework the patch accordingly. > > > >> > > > >> Regards, > > > >> Bhupesh > > > _______________________________________________ > > > edk2-devel mailing list > > > edk2-devel@lists.01.org > > > https://lists.01.org/mailman/listinfo/edk2-devel