Re: [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

[Leif Lindholm <leif.lindholm@linaro.org>]

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.lindholm@linaro.org]
> > Sent: Monday, December 4, 2017 4:36 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Udit Kumar <udit.kumar@nxp.com>; Meenakshi Aggarwal
> > <meenakshi.aggarwal@nxp.com>; Varun Sethi
> > <V.Sethi@nxp.com>; edk2-devel@lists.01.org; Gao, Liming
> > <liming.gao@intel.com>; Ard Biesheuvel
> > <ard.biesheuvel@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.kumar@nxp.com]
> > > > Sent: Friday, December 1, 2017 9:58 AM
> > > > To: Leif Lindholm <leif.lindholm@linaro.org>;
> > Meenakshi
> > > > Aggarwal <meenakshi.aggarwal@nxp.com>; Varun Sethi
> > > > <V.Sethi@nxp.com>
> > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>;
> > edk2-
> > > > devel@lists.01.org; Gao, Liming
> > <liming.gao@intel.com>;
> > > > Ard Biesheuvel <ard.biesheuvel@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-
> > > > bounces@lists.01.org] On Behalf Of Leif
> > > > > Lindholm
> > > > > Sent: Friday, December 01, 2017 4:28 PM
> > > > > To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> > > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>;
> > > > edk2-devel@lists.01.org;
> > > > > Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel
> > > > > <ard.biesheuvel@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.lindholm@linaro.org]
> > > > > > > Sent: Thursday, November 30, 2017 1:19 AM
> > > > > > > To: Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > > > > > > Cc: Meenakshi Aggarwal
> > > > <meenakshi.aggarwal@nxp.com>; Gao, Liming
> > > > > > > <liming.gao@intel.com>; edk2-
> > devel@lists.01.org;
> > > > Ard Biesheuvel
> > > > > > > <ard.biesheuvel@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-
> > > > bounces@lists.01.org] On
> > > > > > > > > Behalf Of Leif Lindholm
> > > > > > > > > Sent: Wednesday, November 29, 2017 4:51 AM
> > > > > > > > > To: Meenakshi Aggarwal
> > > > <meenakshi.aggarwal@nxp.com>; Gao, Liming
> > > > > > > > > <liming.gao@intel.com>
> > > > > > > > > Cc: Kinney, Michael D
> > > > <michael.d.kinney@intel.com>;
> > > > > > > > > edk2-devel@lists.01.org; Ard Biesheuvel
> > > > > > > > > <ard.biesheuvel@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.gao@intel.com>;
> > > > Meenakshi
> > > > > > > > > Aggarwal
> > > > > > > > > > > <meenakshi.aggarwal@nxp.com>; Ard
> > > > Biesheuvel
> > > > > > > > > > > <ard.biesheuvel@linaro.org>; Kinney,
> > > > Michael D
> > > > > > > > > > > <michael.d.kinney@intel.com>; 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
> > > > > > > > > <meenakshi.aggarwal@nxp.com>; Ard
> > Biesheuvel
> > > > > > > > > > > > <ard.biesheuvel@linaro.org>; Kinney,
> > > > Michael D
> > > > > > > > > > > > <michael.d.kinney@intel.com>; 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
> > > > <ard.biesheuvel@linaro.org>;
> > > > > > > > > Kinney, Michael D
> > > > > > > > > > > > ><michael.d.kinney@intel.com>; edk2-
> > > > > > > > > devel@lists.01.org; Gao, Liming
> > > > > > > > > > > > ><liming.gao@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.biesheuvel@linaro.org>; Kinney,
> > Michael D
> > > > > > > > > > > > >> <michael.d.kinney@intel.com>
> > > > > > > > > > > > >> Cc: Gao, Liming
> > > > <liming.gao@intel.com>; edk2-
> > > > > > > > > devel@ml01.01.org;
> > > > > > > > > > > > >> Meenakshi Aggarwal
> > > > > > > > > <meenakshi.aggarwal@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.biesheuvel@linaro.org]
> > > > > > > > > > > > >> > Sent: Monday, October 17, 2016
> > 1:12
> > > > PM
> > > > > > > > > > > > >> > To: Kinney, Michael D
> > > > > > > > > <michael.d.kinney@intel.com>
> > > > > > > > > > > > >> > Cc: Gao, Liming
> > > > <liming.gao@intel.com>;
> > > > > > > > > Bhupesh Sharma
> > > > > > > > > > > > >> > <bhupesh.sharma@nxp.com>; 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
> > > > > > > > > > > > >> > <michael.d.kinney@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