Sounds good. Mike
> -----Original Message----- > From: Leif Lindholm [mailto:leif.lindh...@linaro.org] > Sent: Monday, December 4, 2017 7:54 AM > To: Kinney, Michael D <michael.d.kin...@intel.com> > Cc: Udit Kumar <udit.ku...@nxp.com>; Meenakshi Aggarwal > <meenakshi.aggar...@nxp.com>; Varun Sethi > <v.se...@nxp.com>; edk2-devel@lists.01.org; Gao, Liming > <liming....@intel.com>; Ard Biesheuvel > <ard.biesheu...@linaro.org> > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add support > for big-endian MMIO > > Hi Mike, > > This separate library would only be necessary > specifically because > an additional library to the default platform IoLib was > needed. > So while it's another dependency, it would likely reduce > image size. > > Which is why I was leaning towards a wrapper. > > This also means the actual hw-dependent bit gets > abstracted away from > the portable and predictable byteswapping. Which always > makes me > slightly more comfortable. > > If you're OK with the concept, I can throw an RFC > together. > > Best Regards, > > Leif > > On Mon, Dec 04, 2017 at 03:31:10PM +0000, Kinney, Michael > D wrote: > > Leif, > > > > I may make more sense to add to MdePkg as a peer to > IoLib. > > This minimizes the package dependencies for Si modules. > > > > I am ok as a wrapper on top of IoLib. Whatever reduces > > code duplication and reduces maintenance. > > > > Best regards, > > > > Mike > > > > > -----Original Message----- > > > From: Leif Lindholm [mailto:leif.lindh...@linaro.org] > > > Sent: Monday, December 4, 2017 4:36 AM > > > To: Kinney, Michael D <michael.d.kin...@intel.com> > > > Cc: Udit Kumar <udit.ku...@nxp.com>; Meenakshi > Aggarwal > > > <meenakshi.aggar...@nxp.com>; Varun Sethi > > > <v.se...@nxp.com>; edk2-devel@lists.01.org; Gao, > Liming > > > <liming....@intel.com>; Ard Biesheuvel > > > <ard.biesheu...@linaro.org> > > > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: Add > support > > > for big-endian MMIO > > > > > > Mike, > > > > > > In that case - do you think it should be added to > > > MdeModulePkg? > > > > > > Should it be implemented simply as a wrapper on IoLib > > > (depending on > > > it)? > > > > > > / > > > Leif > > > > > > On Fri, Dec 01, 2017 at 10:41:26PM +0000, Kinney, > Michael > > > D wrote: > > > > Udit, > > > > > > > > I agree with your concern. > > > > > > > > I am in favor of adding new APIs that perform the > > > > byte swap operations. > > > > > > > > Mike > > > > > > > > > -----Original Message----- > > > > > From: Udit Kumar [mailto:udit.ku...@nxp.com] > > > > > Sent: Friday, December 1, 2017 9:58 AM > > > > > To: Leif Lindholm <leif.lindh...@linaro.org>; > > > Meenakshi > > > > > Aggarwal <meenakshi.aggar...@nxp.com>; Varun > Sethi > > > > > <v.se...@nxp.com> > > > > > Cc: Kinney, Michael D > <michael.d.kin...@intel.com>; > > > edk2- > > > > > de...@lists.01.org; Gao, Liming > > > <liming....@intel.com>; > > > > > Ard Biesheuvel <ard.biesheu...@linaro.org> > > > > > Subject: RE: [edk2] [PATCH 1/1] MdePkg/IoLib: Add > > > support > > > > > for big-endian MMIO > > > > > > > > > > Thanks Leif, > > > > > > > > > > > It may require a little bit more of up-front > work, > > > but > > > > > the end result will be a > > > > > > platform port that works with the intended edk2 > > > design > > > > > principles rather than > > > > > > > > > > Yes, this will be re-design/code for us, breaking > all > > > > > software pieces into > > > > > smaller blocks and exposing protocol from the > same. > > > > > This could be managed. > > > > > > > > > > But how do you see, if there is such dependency > oN > > > lib. > > > > > Say a driver which is in BE mode, and is using > > > DebugLib > > > > > (BaseDebugLibSerialPort) > > > > > And DebugLib uses SerialPortLib, which is in LE > mode > > > > > > > > > > Thanks > > > > > Udit > > > > > > > > > > > -----Original Message----- > > > > > > From: edk2-devel [mailto:edk2-devel- > > > > > boun...@lists.01.org] On Behalf Of Leif > > > > > > Lindholm > > > > > > Sent: Friday, December 01, 2017 4:28 PM > > > > > > To: Meenakshi Aggarwal > <meenakshi.aggar...@nxp.com> > > > > > > Cc: Kinney, Michael D > <michael.d.kin...@intel.com>; > > > > > edk2-devel@lists.01.org; > > > > > > Gao, Liming <liming....@intel.com>; Ard > Biesheuvel > > > > > > <ard.biesheu...@linaro.org> > > > > > > Subject: Re: [edk2] [PATCH 1/1] MdePkg/IoLib: > Add > > > > > support for big-endian > > > > > > MMIO > > > > > > > > > > > > On Thu, Nov 30, 2017 at 04:15:38AM +0000, > Meenakshi > > > > > Aggarwal wrote: > > > > > > > Hi Leif, Mike, > > > > > > > > > > > > > > > > > > > > > NXP boards, at present, have few controllers > with > > > big > > > > > endian and other > > > > > > > with little endian memory access. > > > > > > > > > > > > Sure, this is not a problem. > > > > > > > > > > > > > Maximum controllers depend on SocLib library > for > > > > > clock information and > > > > > > > endianness for SocLib and controllers is > > > different. > > > > > > > > > > > > OK, I see in SocLib you have something called a > Gur > > > > > that is read once (and again > > > > > > does a special per-device Pcd dance for runtime > > > > > selection between > > > > > > byteswapping Mmio and plain Mmio). > > > > > > > > > > > > > So this option will not work for us, > > > > > > > You would then just do (in your .dsc): > > > > > > > > Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf { > > > > > > > IoLib|<path to BeIoLib .inf> > > > > > > > } > > > > > > > > > > > > > > Because all libraries included by WatchDog > will > > > then > > > > > access BE mode Mmio > > > > > > APIs. > > > > > > > > > > > > Which libraries performing Mmio accesses are > you > > > > > intending to include in your > > > > > > watchdog driver? You have submitted none as > part of > > > > > this series. > > > > > > > > > > > > > And breaking code into separate modules with > > > > > different access will be > > > > > > > very difficult. > > > > > > > > > > > > It may require a little bit more of up-front > work, > > > but > > > > > the end result will be a > > > > > > platform port that works with the intended edk2 > > > design > > > > > principles rather than > > > > > > against them. And that will reduce the overall > > > effort > > > > > (not to mention code > > > > > > duplication). > > > > > > > > > > > > From the patches you have sent, the only > required > > > > > change I see (if a > > > > > > byteswapping IoLib was added to edk2) would be > to > > > > > create a tiny driver for this > > > > > > "Gur" device that installs a protocol > containing a > > > > > single function for reading > > > > > > from that device's register space. That driver > can > > > be > > > > > built against the swapping > > > > > > or non-swapping IoLib as appropriate. > > > > > > > > > > > > > Watchdog is not the only module which need BE > > > Mmio > > > > > APIs, we have MMC > > > > > > > and other controllers also with same > requirement. > > > > > > > > > > > > And the same solutions are possible everywhere. > > > > > > > > > > > > Best Regards, > > > > > > > > > > > > Leif > > > > > > > > > > > > > We need BE Mmio APIs with a different name. > > > > > > > > > > > > > > Please see the possibility. > > > > > > > > > > > > > > Thanks & Regards, > > > > > > > Meenakshi > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: Leif Lindholm > > > > > [mailto:leif.lindh...@linaro.org] > > > > > > > > Sent: Thursday, November 30, 2017 1:19 AM > > > > > > > > To: Kinney, Michael D > > > <michael.d.kin...@intel.com> > > > > > > > > Cc: Meenakshi Aggarwal > > > > > <meenakshi.aggar...@nxp.com>; Gao, Liming > > > > > > > > <liming....@intel.com>; edk2- > > > de...@lists.01.org; > > > > > Ard Biesheuvel > > > > > > > > <ard.biesheu...@linaro.org> > > > > > > > > Subject: Re: [edk2] [PATCH 1/1] > MdePkg/IoLib: > > > Add > > > > > support for > > > > > > > > big-endian MMIO > > > > > > > > > > > > > > > > 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- > > > > > boun...@lists.01.org] On > > > > > > > > > > Behalf Of Leif Lindholm > > > > > > > > > > Sent: Wednesday, November 29, 2017 4:51 > AM > > > > > > > > > > To: Meenakshi Aggarwal > > > > > <meenakshi.aggar...@nxp.com>; Gao, Liming > > > > > > > > > > <liming....@intel.com> > > > > > > > > > > Cc: Kinney, Michael D > > > > > <michael.d.kin...@intel.com>; > > > > > > > > > > edk2-devel@lists.01.org; Ard Biesheuvel > > > > > > > > > > <ard.biesheu...@linaro.org> > > > > > > > > > > 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|<path to BeIoLib .inf> > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > This causes the big-endian version of > the > > > > > library to be used for > > > > > > > > > > this component. This makes these > > > Wdog<access> > > > > > 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 > <liming....@intel.com>; > > > > > Meenakshi > > > > > > > > > > Aggarwal > > > > > > > > > > > > <meenakshi.aggar...@nxp.com>; Ard > > > > > Biesheuvel > > > > > > > > > > > > <ard.biesheu...@linaro.org>; > Kinney, > > > > > Michael D > > > > > > > > > > > > <michael.d.kin...@intel.com>; edk2- > > > > > > > > > > de...@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- > > > > > > > > > > boun...@lists.01.org] On Behalf Of > > > > > > > > > > > > > Gao, Liming > > > > > > > > > > > > > Sent: Monday, October 16, 2017 > 8:48 > > > AM > > > > > > > > > > > > > To: Meenakshi Aggarwal > > > > > > > > > > <meenakshi.aggar...@nxp.com>; Ard > > > Biesheuvel > > > > > > > > > > > > > <ard.biesheu...@linaro.org>; > Kinney, > > > > > Michael D > > > > > > > > > > > > > <michael.d.kin...@intel.com>; > edk2- > > > > > > > > > > de...@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.aggar...@nxp.com] > > > > > > > > > > > > > >Sent: Friday, October 13, 2017 > 2:07 > > > PM > > > > > > > > > > > > > >To: Ard Biesheuvel > > > > > <ard.biesheu...@linaro.org>; > > > > > > > > > > Kinney, Michael D > > > > > > > > > > > > > ><michael.d.kin...@intel.com>; > edk2- > > > > > > > > > > de...@lists.01.org; Gao, Liming > > > > > > > > > > > > > ><liming....@intel.com> > > > > > > > > > > > > > >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 > > > > > > > > > > <ard.biesheu...@linaro.org>; Kinney, > > > Michael D > > > > > > > > > > > > > >> <michael.d.kin...@intel.com> > > > > > > > > > > > > > >> Cc: Gao, Liming > > > > > <liming....@intel.com>; edk2- > > > > > > > > > > de...@ml01.01.org; > > > > > > > > > > > > > >> Meenakshi Aggarwal > > > > > > > > > > <meenakshi.aggar...@nxp.com> > > > > > > > > > > > > > >> Subject: RE: [edk2] [PATCH > 1/1] > > > > > MdePkg/IoLib: > > > > > > > > > > Add support for > > > > > > > > > > > > > >> big-endian MMIO > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> Hi Ard, > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > -----Original Message----- > > > > > > > > > > > > > >> > From: Ard Biesheuvel > > > > > > > > > > [mailto:ard.biesheu...@linaro.org] > > > > > > > > > > > > > >> > Sent: Monday, October 17, > 2016 > > > 1:12 > > > > > PM > > > > > > > > > > > > > >> > To: Kinney, Michael D > > > > > > > > > > <michael.d.kin...@intel.com> > > > > > > > > > > > > > >> > Cc: Gao, Liming > > > > > <liming....@intel.com>; > > > > > > > > > > Bhupesh Sharma > > > > > > > > > > > > > >> > <bhupesh.sha...@nxp.com>; > edk2- > > > > > > > > > > de...@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 > > > > > > > > > > > > > >> > <michael.d.kin...@intel.com> > > > 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 { > > > > > > > > > > > > > >> > <LibraryClasses> > > > > > > > > > > > > > >> > > > > > > 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://emea01.safelinks.protection.outlook.com/?url=http > > > > > s%3A%2F%2Fl > > > > > > > > ist > > > > > > > > s.01.org%2Fmailman%2Flistinfo%2Fedk2- > > > > > > > > > > > > > > > > > > > > > > > devel&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C695e4 > > > > > 3cbb51 > > > > > > > > > > > > > > > > > > > > > > > a471cd44608d5376230b0%7C686ea1d3bc2b4c6fa92cd99c5c301635% > > > > > 7C0%7C0 > > > > > > > > > > > > > > > > > %7C636475817220663612&sdata=hwQh8dfUkVGzm7uQB9%2FirexmYEk > > > > > dEQ > > > > > > > > %2Bu97606L%2FHEfg%3D&reserved=0 > > > > > > > > > > > > > _______________________________________________ > > > > > > > > > > edk2-devel mailing list > > > > > > > > > > edk2-devel@lists.01.org > > > > > > > > > > > > > > > > > > > > > > > > > > > https://emea01.safelinks.protection.outlook.com/?url=http > > > > > s%3A%2F%2Fl > > > > > > > > ist > > > > > > > > s.01.org%2Fmailman%2Flistinfo%2Fedk2- > > > > > > > > > > > > > > > > > > > > > > > devel&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C695e4 > > > > > 3cbb51 > > > > > > > > > > > > > > > > > > > > > > > a471cd44608d5376230b0%7C686ea1d3bc2b4c6fa92cd99c5c301635% > > > > > 7C0%7C0 > > > > > > > > > > > > > > > > > %7C636475817220663612&sdata=hwQh8dfUkVGzm7uQB9%2FirexmYEk > > > > > dEQ > > > > > > > > %2Bu97606L%2FHEfg%3D&reserved=0 > > > > > > _______________________________________________ > > > > > > edk2-devel mailing list > > > > > > edk2-devel@lists.01.org > > > > > > > > > > > > > > > https://emea01.safelinks.protection.outlook.com/?url=http > > > > > s%3A%2F%2Flists.01 > > > > > > .org%2Fmailman%2Flistinfo%2Fedk2- > > > > > > > > > > > > > > > devel&data=02%7C01%7Cudit.kumar%40nxp.com%7Cd06018336a064 > > > > > 8fa8e440 > > > > > > > > > > > > > > > 8d538aa5f19%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C > > > > > 6364772 > > > > > > > > > > > > > > > 26761253124&sdata=bAU%2BOidbzbKw76OOq5%2FUplwQbo8iMnE3alH > > > > > Y4ptAX > > > > > > MU%3D&reserved=0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
