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::235; helo=mail-wr0-x235.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr0-x235.google.com (mail-wr0-x235.google.com [IPv6:2a00:1450:400c:c0c::235]) (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 9FD9F21A10960 for ; Wed, 29 Nov 2017 11:44:17 -0800 (PST) Received: by mail-wr0-x235.google.com with SMTP id a41so2620254wra.6 for ; Wed, 29 Nov 2017 11:48:41 -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=OlsTE9agj/UsqdYQUE/Gl4Ak0AZE6RaZDVIdj+o8JBo=; b=Ln7EFrGXHy2TYk3UBivvgZfDcoJfO93r8Sk8/csy031T7b/C2Hj1MRqucUXK43Ewob HCvL7i+2W088oUFMecDggd6qN+LSpA/be/gcZmVz+0bID5/6pIYE8xPb+90FzXfA4Kv0 /fOq/zM3VduSSn9XpxGYuYew/D8DA+I/Y2pFE= 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=OlsTE9agj/UsqdYQUE/Gl4Ak0AZE6RaZDVIdj+o8JBo=; b=Kc+1hvPLLtB7fI1k8OMXNsMoz/munKI/uy0M+2aB9Hh1lMLCuUoTnJFdlOw4uxGye9 YXJZCjSGLXm4zt+Zqe5NpqNZGiOMq874PQXWEz5rmGVh50K3IRdEFLPqAiZt0mch0Ow0 Q8wyJRnYVUH+vWhiM1qmq+YMCLD0CelvAf2AU68xcTubvMF3TDPPkou3OTJIlqGA/ee6 8pgUdBvEmgLvl0JV4kYDFGX356ZWAjA+BPTxXm6VeFJhu5USfqbersc6Wrd6E/okWZTO 3CvMwC0STIGD7IQ566/NV3hXgGaakzol5IlEnLdNrro88cooSrFKu7BW7Ji/wGbxHyy9 A8cA== X-Gm-Message-State: AJaThX4kp9q7dYxrANyLop7T9IriSHMALiSN0mk07bvUc+cHalB6JhHf 2/F3WSfRZuE1ceBlkDtjYRvV7Q== X-Google-Smtp-Source: AGs4zMaMIIg0ZCFOxL+Lkd1RpPf0evN06S4dErLLP7gKUfjFcnIVkKmVwwl5P6WFhll7AdOi+cDATg== X-Received: by 10.223.131.166 with SMTP id 35mr14916wre.84.1511984919742; Wed, 29 Nov 2017 11:48:39 -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 w46sm1167080wrb.86.2017.11.29.11.48.38 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 29 Nov 2017 11:48:38 -0800 (PST) Date: Wed, 29 Nov 2017 19:48:36 +0000 From: Leif Lindholm To: "Kinney, Michael D" Cc: Meenakshi Aggarwal , "Gao, Liming" , "edk2-devel@lists.01.org" , Ard Biesheuvel Message-ID: <20171129194836.dqsbtbqi5budvppl@bivouac.eciton.net> References: <4A89E2EF3DFEDB4C8BFDE51014F606A14B49505E@shsmsx102.ccr.corp.intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E16B65D@SHSMSX104.ccr.corp.intel.com> <20171129125114.i3y446ds4qrkohxl@bivouac.eciton.net> 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 19:44:18 -0000 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit I guess there is no strict rule about a driver only directly accessing one piece of HW? Still, that would be one possible solution: breaking accesses to a separate HW in need of byteswapping out into its own module and letting it override IoLib version there. Regards, Leif On Wed, Nov 29, 2017 at 07:25:05PM +0000, Kinney, Michael D wrote: > Leif, > > I agree that this should be described as byte swapping. > > What about a module that needs to access HW with and > without the bytes swapped? It gets more complex if a > module is linked against several libs and some libs > access HW with bytes swapped and some do not. > > Thanks, > > Mike > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel- > > bounces@lists.01.org] On Behalf Of Leif Lindholm > > Sent: Wednesday, November 29, 2017 4:51 AM > > To: Meenakshi Aggarwal ; > > Gao, Liming > > Cc: Kinney, Michael D ; > > edk2-devel@lists.01.org; Ard Biesheuvel > > > > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add > > support for big-endian MMIO > > > > 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/BaseIoLibIntrinsicSwa > > p.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 > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel