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

[Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>]

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://lists.01.org/mailman/listinfo/edk2-devel