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

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

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=https%3A%2F%2Flist
> > s.01.org%2Fmailman%2Flistinfo%2Fedk2-
> > devel&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C695e43cbb51
> > a471cd44608d5376230b0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> > %7C636475817220663612&sdata=hwQh8dfUkVGzm7uQB9%2FirexmYEkdEQ
> > %2Bu97606L%2FHEfg%3D&reserved=0
> > > > _______________________________________________
> > > > edk2-devel mailing list
> > > > edk2-devel@lists.01.org
> > > >
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> > s.01.org%2Fmailman%2Flistinfo%2Fedk2-
> > devel&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C695e43cbb51
> > a471cd44608d5376230b0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> > %7C636475817220663612&sdata=hwQh8dfUkVGzm7uQB9%2FirexmYEkdEQ
> > %2Bu97606L%2FHEfg%3D&reserved=0