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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Sounds good.

Mike

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Monday, December 4, 2017 7:54 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
> 
> 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