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....@intel.com>; Meenakshi Aggarwal > <meenakshi.aggar...@nxp.com>; Ard Biesheuvel > <ard.biesheu...@linaro.org>; Kinney, Michael D > <michael.d.kin...@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-boun...@lists.01.org] On Behalf Of > > Gao, Liming > > Sent: Monday, October 16, 2017 8:48 AM > > To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>; Ard Biesheuvel > > <ard.biesheu...@linaro.org>; Kinney, Michael D > > <michael.d.kin...@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.aggar...@nxp.com] > > >Sent: Friday, October 13, 2017 2:07 PM > > >To: Ard Biesheuvel <ard.biesheu...@linaro.org>; Kinney, Michael D > > ><michael.d.kin...@intel.com>; edk2-devel@lists.01.org; Gao, Liming > > ><liming....@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.biesheu...@linaro.org>; Kinney, Michael D > > >> <michael.d.kin...@intel.com> > > >> Cc: Gao, Liming <liming....@intel.com>; edk2-de...@ml01.01.org; > > >> Meenakshi Aggarwal <meenakshi.aggar...@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.biesheu...@linaro.org] > > >> > Sent: Monday, October 17, 2016 1:12 PM > > >> > To: Kinney, Michael D <michael.d.kin...@intel.com> > > >> > Cc: Gao, Liming <liming....@intel.com>; Bhupesh Sharma > > >> > <bhupesh.sha...@nxp.com>; edk2-de...@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.kin...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel