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

[Udit Kumar <udit.kumar@nxp.com>]

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