From: "Kinney, Michael D" <michael.d.kinney@intel.com>
To: Leif Lindholm <leif.lindholm@linaro.org>,
Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>,
"Gao, Liming" <liming.gao@intel.com>,
"Kinney, Michael D" <michael.d.kinney@intel.com>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO
Date: Wed, 29 Nov 2017 19:25:05 +0000 [thread overview]
Message-ID: <E92EE9817A31E24EB0585FDF735412F5A7DC7DA7@ORSMSX113.amr.corp.intel.com> (raw)
In-Reply-To: <20171129125114.i3y446ds4qrkohxl@bivouac.eciton.net>
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://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
next prev parent reply other threads:[~2017-11-29 19:20 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-14 9:33 [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO Bhupesh Sharma
2016-10-14 12:06 ` Laszlo Ersek
2016-10-14 13:17 ` Bhupesh Sharma
2016-10-14 13:32 ` Laszlo Ersek
2016-10-17 3:10 ` Gao, Liming
2016-10-17 4:10 ` Kinney, Michael D
2016-10-17 7:42 ` Ard Biesheuvel
2016-10-17 9:57 ` Bhupesh Sharma
2017-10-13 6:07 ` Meenakshi Aggarwal
2017-10-16 3:17 ` Gao, Liming
2017-10-23 7:07 ` Udit Kumar
2017-11-27 6:06 ` Meenakshi Aggarwal
2017-11-27 11:07 ` Leif Lindholm
2017-11-29 12:51 ` Leif Lindholm
2017-11-29 19:25 ` Kinney, Michael D [this message]
2017-11-29 19:48 ` Leif Lindholm
2017-11-30 4:15 ` Meenakshi Aggarwal
2017-12-01 10:57 ` Leif Lindholm
2017-12-01 17:57 ` Udit Kumar
2017-12-01 22:41 ` Kinney, Michael D
2017-12-04 6:18 ` Meenakshi Aggarwal
2017-12-04 12:36 ` Leif Lindholm
2017-12-04 15:31 ` Kinney, Michael D
2017-12-04 15:54 ` Leif Lindholm
2017-12-04 16:19 ` Kinney, Michael D
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=E92EE9817A31E24EB0585FDF735412F5A7DC7DA7@ORSMSX113.amr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox