Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Leif Lindholm]

On Fri, Feb 23, 2018 at 11:47:28AM +0100, Laszlo Ersek wrote:
> >> I think the solution that saves the most on the *source* code size
> >> is:
> >> - introduce the BeIoLib class
> >> - duplicate the MMIO functions from BaseIoLibIntrinsic to the one
> >>   BeIoLib instance that we introduce
> >> - modify the MMIO functions in *both* lib instances (original LE, and
> >>   new BE), like this:
> >>
> >>   - If the CPU architecture is known to be bound to a single
> >> endianness, then hardcode the appropriate operation. This can be
> >> done with preprocessor macros, or with the architecture support
> >> of INF files / separate source files. For example, on IA32 and
> >> X64, the IoLib instance should work transparently,
> >> unconditionally, and the BeIoLib instance should byte-swap,
> >> unconditionally.
> >>
> >>   - On other CPU arches, all the wider-than-byte MMIO functions, in
> >> *both* lib instances should do something like this:
> >>
> >> //
> >> // at file scope
> >> //
> >> STATIC CONST UINT16 mOne = 1;
> >>
> >> //
> >> // at function scope
> >> //
> >> if (*(CONST UINT8 *) == 1) {
> >>   //
> >>   // CPU in LE mode:
> >>   // - work transparently in the IoLib instance
> >>   // - byte-swap in the BeIoLib instance
> >>   //
> >> } else {
> >>   //
> >>   // CPU in BE mode:
> >>   // - byte-swap in the IoLib instance
> >>   // - work transparently in the BeIoLib instance
> >>   //
> >> }
> >
> > You meant, sort of cpu_to_le and cpu_to_be sort of APIs
> 
> I'm lost. I don't know how to put it any clearer. Let me try with actual
> code.
> 
> (a) Suggested for "MdePkg/Library/BaseIoLibIntrinsic/IoLib.c", which is
> used on IA32 and X64, therefore CPU byte order is little endian only:
> 
> > UINT32
> > EFIAPI
> > MmioWrite32 (
> >   IN  UINTN Address,
> >   IN  UINT32Value
> >   )
> > {
> >   ASSERT ((Address & 3) == 0);
> >
> >   MemoryFence ();
> >   *(volatile UINT32*)Address = Value;
> >   MemoryFence ();
> >
> >   return Value;
> > }
> 
> In other words, no change to the current implementation.
> 
> 
> (b) Suggested for "MdePkg/Library/BaseBeIoLibIntrinsic/IoLib.c", also to
> be used on IA32 and X64. Because the CPU byte order is LE only, this
> variant will byte-swap unconditionally.
> 
> > UINT32
> > EFIAPI
> > BeMmioWrite32 (
> >   IN  UINTN Address,
> >   IN  UINT32Value
> >   )
> > {
> >   ASSERT ((Address & 3) == 0);
> >
> >   MemoryFence ();
> >   *(volatile UINT32*)Address = SwapBytes32 (Value);
> >   MemoryFence ();
> >
> >   return Value;
> > }
> 
> (c) Suggested for "MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c", which
> is used on ARM and AARCH64. And here I'll assume that the CPU byte order
> on those can be either LE or BE.

We badly need to reduce the number of architecture-specific libraries
for doing mmio accesses from C code, rather than increasing them.

So, I realise I've confused the situation somewhat here by talking
about big-endian CPUs. We are not looking to support big-endian CPUs
today. All I wanted was to make sure we don't unneccesarily build in
assumptions in the codebase about things that could change in the
future with fairly minor changes to the specifications.

What I do see as an absolute is that a single _UEFI_ architecture
could never be more than one possible endianness. I.e., if someone
wanted to (and this _really_ isn't me being *nudge* *nudge*, *wink*
*wink*) bring in a BE-port of AArch64, that wouldn't be AARCH64, that
would be AARCH64BE.

Which also means I don't think there is any need for any sort of
runtime detection of this.

Your proposal of a set of function-pointers in the driver being mapped
to appropriate device endianness on initialization seems sufficient to
resolve the situation posed. And the situation feels sufficiently
esoteric to motivate that level of clunkiness.

Regards,

Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Laszlo Ersek]

On 02/23/18 12:48, Pankaj Bansal wrote:
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Friday, February 23, 2018 4:52 PM
>> To: Pankaj Bansal <pankaj.ban...@nxp.com>; Udit Kumar
>> <udit.ku...@nxp.com>; Leif Lindholm <leif.lindh...@linaro.org>
>> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
>> ard.biesheu...@linaro.org; Meenakshi Aggarwal
>> <meenakshi.aggar...@nxp.com>
>> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
>> for Big Endian Mmio APIs
>>
>> On 02/23/18 12:04, Pankaj Bansal wrote:
>>
>>> However Laszlo, with the method you suggest (using STATIC CONST
>>> UINT16 mOne), would it not add delay in each Mmio Operation ?
>>>
>>> I am concerned about boot delay using this approach.
>> The condition can be evaluated at compile time, so I expect optimizing
>> compilers to eliminate the dead branch.
>>
>> Assuming the condition cannot be eliminated at build time, what is your
>> concern: the single byte access, or the branch instruction?
>>
>> I don't think the single byte access matters. (If you tried to replace that 
>> with a
>> HOB or PCD lookup, it could only be worse.)
>>
>> I also doubt the branch should be a concern. You could replace the "if"
>> (or the ternary operator "?:") with function pointers that you set e.g.
>> in a constructor function. But I think an "if" with an invariable
>> (constant) controlling expression is at least as friendly towards branch
>> predictors as a function pointer variable (through which you might be
>> *forced* to make a real function call).
>>
>> Personally I wouldn't worry.
>>
> 
> I think you are right about smart compiler eliminating the branches at build 
> time.
> I just pointed this out because we call Mmio/BeMmio APIs when accessing Nor 
> flash for variable read/write.
> As these are called so many time during boot, I did not want any delay to be 
> added to these APIs than necessary.
> Now that you have pointed it out, I don't think any significant delay will be 
> added to these APIs.

In addition to that, physical flash access is likely so slow anyway that
a few additional instructions should be lost in the noise, generally
speaking.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Pankaj Bansal]

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Friday, February 23, 2018 4:52 PM
> To: Pankaj Bansal <pankaj.ban...@nxp.com>; Udit Kumar
> <udit.ku...@nxp.com>; Leif Lindholm <leif.lindh...@linaro.org>
> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> ard.biesheu...@linaro.org; Meenakshi Aggarwal
> <meenakshi.aggar...@nxp.com>
> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
> for Big Endian Mmio APIs
> 
> On 02/23/18 12:04, Pankaj Bansal wrote:
> 
> > However Laszlo, with the method you suggest (using STATIC CONST
> > UINT16 mOne), would it not add delay in each Mmio Operation ?
> >
> > I am concerned about boot delay using this approach.
> The condition can be evaluated at compile time, so I expect optimizing
> compilers to eliminate the dead branch.
> 
> Assuming the condition cannot be eliminated at build time, what is your
> concern: the single byte access, or the branch instruction?
> 
> I don't think the single byte access matters. (If you tried to replace that 
> with a
> HOB or PCD lookup, it could only be worse.)
> 
> I also doubt the branch should be a concern. You could replace the "if"
> (or the ternary operator "?:") with function pointers that you set e.g.
> in a constructor function. But I think an "if" with an invariable
> (constant) controlling expression is at least as friendly towards branch
> predictors as a function pointer variable (through which you might be
> *forced* to make a real function call).
> 
> Personally I wouldn't worry.
> 

I think you are right about smart compiler eliminating the branches at build 
time.
I just pointed this out because we call Mmio/BeMmio APIs when accessing Nor 
flash for variable read/write.
As these are called so many time during boot, I did not want any delay to be 
added to these APIs than necessary.
Now that you have pointed it out, I don't think any significant delay will be 
added to these APIs.

> 
> Anyway: please do not mistake my willingness / preference to go into such
> detail for having high stakes at this. If you go an entirely different route, 
> I'm
> OK with that too. I felt I was asked for my opinion and I tried to express it 
> in
> detail, that's all.

Any comments/suggestions/opinions are always appreciated from you or from any 
edk2 mailing list member.

> 
> Thanks
> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Udit Kumar]

Hi Laszlo/Leif,

For short term, is this ok to keep this lib under Silicon/NXP, here we are 
assuming  
CPU will always on be LE mode whereas IP can vary between LE/BE mode ?

For long term, we can discuss on APIs/name of Lib/Function name etc
We will update our code, as per agreement.

For me, Suggested approach is ok as well to keep CPU endianness in ARM package.
but need views from Ard/Leif here.

Thanks
Udit

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Friday, February 23, 2018 4:17 PM
> To: Udit Kumar <udit.ku...@nxp.com>; Leif Lindholm
> <leif.lindh...@linaro.org>
> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> ard.biesheu...@linaro.org
> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
> for Big Endian Mmio APIs
> 
> On 02/23/18 11:25, Udit Kumar wrote:
> >
> >
> >> -Original Message-
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> >> Of Laszlo Ersek
> >> Sent: Thursday, February 22, 2018 7:26 PM
> >> To: Leif Lindholm <leif.lindh...@linaro.org>
> >> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> >> ard.biesheu...@linaro.org
> >> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add
> >> support for Big Endian Mmio APIs
> >>
> >> On 02/22/18 12:52, Leif Lindholm wrote:
> >>> On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:
> >>
> >>>>> But that brings back the complication as to how we have a driver
> >>>>> that needs an LE IO library to write output, and a BE IO library
> >>>>> to manipulate the hardware.
> >>>>
> >>>> Can you please explain the "write output" use case more precisely?
> >>>>
> >>>> My thinking would be this:
> >>>>
> >>>> - Use the IoLib class directly for "writing output" in little
> >>>> endian byte order (which is still unclear to me sorry).
> >>>
> >>> If the IoLib class is mapped to a an instance that byte-swaps
> >>> (hereto referred to as BeIoLib if IoLibSwap is unsuitable), would we
> >>> not then end up mapping the non-swapping, currently implemented in
> >>> BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up
> >>> needing to duplicated all IoLib implementation .infs to provide an
> >>> IoLib and a BeIoLib for each?
> >>>
> >>> It's at that point I burst an aneurysm.
> >>> Am I overthinking/underthinking this?
> >>
> >> We need two library classes, one for talking to LE devices and
> >> another to BE devices. These should be usable in a given module at
> >> the same time, as Ard says.
> >>
> >> Both library classes need to work on both LE and BE CPUs (working
> >> from your suggestion that UEFI might grow BE CPU support at some
> >> point). Whether that is implemented by dumb, separate library
> >> instances (yielding in total 2*2=4 library instances), or by smart,
> >> CPU-endianness-agnostic library instances (in total, 2), is a
> >> different question.
> >>
> >> Note that such "smarts" could be less than trivial to implement:
> >> - check CPU endianness in each library API?
> >> - or check in the lib constructor only, and flip some function
> >>   pointers?
> >> - use a dynamic PCD for caching CPU endianness?
> >> - use a HOB for the same?
> >> - use a lib global variable (for caching only on the module level)?
> >>
> >> I think the solution that saves the most on the *source* code size
> >> is:
> >> - introduce the BeIoLib class
> >> - duplicate the MMIO functions from BaseIoLibIntrinsic to the one
> >>   BeIoLib instance that we introduce
> >> - modify the MMIO functions in *both* lib instances (original LE, and
> >>   new BE), like this:
> >>
> >>   - If the CPU architecture is known to be bound to a single
> >> endianness, then hardcode the appropriate operation. This can be
> >> done with preprocessor macros, or with the architecture support
> >> of INF files / separate source files. For example, on IA32 and
> >> X64, the IoLib instance should work transparently,
> >> unconditionally, and the BeIoLib instance should byte-swap,
> >> unconditionally.
> >>
> >>   - On other CPU arches, all the wider-than-byte MMIO functions, in
> >> *both* lib instances should 

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Laszlo Ersek]

On 02/23/18 12:04, Pankaj Bansal wrote:

> However Laszlo, with the method you suggest (using STATIC CONST
> UINT16 mOne), would it not add delay in each Mmio Operation ?
> 
> I am concerned about boot delay using this approach.
The condition can be evaluated at compile time, so I expect optimizing
compilers to eliminate the dead branch.

Assuming the condition cannot be eliminated at build time, what is your
concern: the single byte access, or the branch instruction?

I don't think the single byte access matters. (If you tried to replace
that with a HOB or PCD lookup, it could only be worse.)

I also doubt the branch should be a concern. You could replace the "if"
(or the ternary operator "?:") with function pointers that you set e.g.
in a constructor function. But I think an "if" with an invariable
(constant) controlling expression is at least as friendly towards branch
predictors as a function pointer variable (through which you might be
*forced* to make a real function call).

Personally I wouldn't worry.


Anyway: please do not mistake my willingness / preference to go into
such detail for having high stakes at this. If you go an entirely
different route, I'm OK with that too. I felt I was asked for my opinion
and I tried to express it in detail, that's all.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Udit Kumar]

Hi Laszlo


> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Friday, February 23, 2018 4:29 PM
> To: Udit Kumar <udit.ku...@nxp.com>; Pankaj Bansal
> <pankaj.ban...@nxp.com>; Leif Lindholm <leif.lindh...@linaro.org>
> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> ard.biesheu...@linaro.org; Meenakshi Aggarwal
> <meenakshi.aggar...@nxp.com>
> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
> for Big Endian Mmio APIs
> 
> On 02/23/18 11:39, Udit Kumar wrote:
> >
> >
> >> -Original Message-
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> >> Laszlo Ersek
> >> Sent: Friday, February 23, 2018 2:51 PM
> >> To: Pankaj Bansal <pankaj.ban...@nxp.com>; Leif Lindholm
> >> <leif.lindh...@linaro.org>
> >> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> >> ard.biesheu...@linaro.org
> >> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add
> support
> >> for Big Endian Mmio APIs
> >>
> >> On 02/23/18 09:40, Pankaj Bansal wrote:
> >>> Hi All
> >>>
> >>>> -Original Message-
> >>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> Of
> >>>> Laszlo Ersek
> >>>> Sent: Thursday, February 22, 2018 7:26 PM
> >>>> To: Leif Lindholm <leif.lindh...@linaro.org>
> >>>> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> >>>> ard.biesheu...@linaro.org
> >>>> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add
> >> support
> >>>> for Big Endian Mmio APIs
> >>>>
> >>>> On 02/22/18 12:52, Leif Lindholm wrote:
> >>>>> On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:
> >>>>
> >>>>>>> But that brings back the complication as to how we have a driver
> >>>>>>> that needs an LE IO library to write output, and a BE IO library to
> >>>>>>> manipulate the hardware.
> >>>>>>
> >>>>>> Can you please explain the "write output" use case more precisely?
> >>>>>>
> >>>>>> My thinking would be this:
> >>>>>>
> >>>>>> - Use the IoLib class directly for "writing output" in little endian
> >>>>>> byte order (which is still unclear to me sorry).
> >>>>>
> >>>>> If the IoLib class is mapped to a an instance that byte-swaps (hereto
> >>>>> referred to as BeIoLib if IoLibSwap is unsuitable), would we not then
> >>>>> end up mapping the non-swapping, currently implemented in
> >>>>> BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up
> >>>>> needing to duplicated all IoLib implementation .infs to provide an
> >>>>> IoLib and a BeIoLib for each?
> >>>>>
> >>>>> It's at that point I burst an aneurysm.
> >>>>> Am I overthinking/underthinking this?
> >>>>
> >>>> We need two library classes, one for talking to LE devices and another
> to
> >> BE
> >>>> devices. These should be usable in a given module at the same time, as
> >> Ard
> >>>> says.
> >>>>
> >>>> Both library classes need to work on both LE and BE CPUs (working
> from
> >> your
> >>>> suggestion that UEFI might grow BE CPU support at some point).
> >>>> Whether that is implemented by dumb, separate library instances
> >> (yielding in
> >>>> total 2*2=4 library instances), or by smart, CPU-endianness-agnostic
> >> library
> >>>> instances (in total, 2), is a different question.
> >>>>
> >>>> Note that such "smarts" could be less than trivial to implement:
> >>>> - check CPU endianness in each library API?
> >>>> - or check in the lib constructor only, and flip some function pointers?
> >>>> - use a dynamic PCD for caching CPU endianness?
> >>>> - use a HOB for the same?
> >>>> - use a lib global variable (for caching only on the module level)?
> >>>>
> >>>> I think the solution that saves the most on the *source* code size is:
> >>>> - introduce the BeIoLib class
> >>>> - duplicate the MMIO f

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Pankaj Bansal]

Hi All

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Friday, February 23, 2018 4:29 PM
> To: Udit Kumar <udit.ku...@nxp.com>; Pankaj Bansal
> <pankaj.ban...@nxp.com>; Leif Lindholm <leif.lindh...@linaro.org>
> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> ard.biesheu...@linaro.org; Meenakshi Aggarwal
> <meenakshi.aggar...@nxp.com>
> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
> for Big Endian Mmio APIs
> 
> On 02/23/18 11:39, Udit Kumar wrote:
> >
> >
> >> -Original Message-
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> >> Of Laszlo Ersek
> >> Sent: Friday, February 23, 2018 2:51 PM
> >> To: Pankaj Bansal <pankaj.ban...@nxp.com>; Leif Lindholm
> >> <leif.lindh...@linaro.org>
> >> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> >> ard.biesheu...@linaro.org
> >> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add
> >> support for Big Endian Mmio APIs
> >>
> >> On 02/23/18 09:40, Pankaj Bansal wrote:
> >>> Hi All
> >>>
> >>>> -Original Message-
> >>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> >>>> Of Laszlo Ersek
> >>>> Sent: Thursday, February 22, 2018 7:26 PM
> >>>> To: Leif Lindholm <leif.lindh...@linaro.org>
> >>>> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> >>>> ard.biesheu...@linaro.org
> >>>> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add
> >> support
> >>>> for Big Endian Mmio APIs
> >>>>
> >>>> On 02/22/18 12:52, Leif Lindholm wrote:
> >>>>> On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:
> >>>>
> >>>>>>> But that brings back the complication as to how we have a driver
> >>>>>>> that needs an LE IO library to write output, and a BE IO library
> >>>>>>> to manipulate the hardware.
> >>>>>>
> >>>>>> Can you please explain the "write output" use case more precisely?
> >>>>>>
> >>>>>> My thinking would be this:
> >>>>>>
> >>>>>> - Use the IoLib class directly for "writing output" in little
> >>>>>> endian byte order (which is still unclear to me sorry).
> >>>>>
> >>>>> If the IoLib class is mapped to a an instance that byte-swaps
> >>>>> (hereto referred to as BeIoLib if IoLibSwap is unsuitable), would
> >>>>> we not then end up mapping the non-swapping, currently
> implemented
> >>>>> in BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up
> >>>>> needing to duplicated all IoLib implementation .infs to provide an
> >>>>> IoLib and a BeIoLib for each?
> >>>>>
> >>>>> It's at that point I burst an aneurysm.
> >>>>> Am I overthinking/underthinking this?
> >>>>
> >>>> We need two library classes, one for talking to LE devices and
> >>>> another to
> >> BE
> >>>> devices. These should be usable in a given module at the same time,
> >>>> as
> >> Ard
> >>>> says.
> >>>>
> >>>> Both library classes need to work on both LE and BE CPUs (working
> >>>> from
> >> your
> >>>> suggestion that UEFI might grow BE CPU support at some point).
> >>>> Whether that is implemented by dumb, separate library instances
> >> (yielding in
> >>>> total 2*2=4 library instances), or by smart,
> >>>> CPU-endianness-agnostic
> >> library
> >>>> instances (in total, 2), is a different question.
> >>>>
> >>>> Note that such "smarts" could be less than trivial to implement:
> >>>> - check CPU endianness in each library API?
> >>>> - or check in the lib constructor only, and flip some function pointers?
> >>>> - use a dynamic PCD for caching CPU endianness?
> >>>> - use a HOB for the same?
> >>>> - use a lib global variable (for caching only on the module level)?
> >>>>
> >>>> I think the solution that saves the most on the *source* code size is:
> &g

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Laszlo Ersek]

On 02/23/18 11:39, Udit Kumar wrote:
> 
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Friday, February 23, 2018 2:51 PM
>> To: Pankaj Bansal <pankaj.ban...@nxp.com>; Leif Lindholm
>> <leif.lindh...@linaro.org>
>> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
>> ard.biesheu...@linaro.org
>> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
>> for Big Endian Mmio APIs
>>
>> On 02/23/18 09:40, Pankaj Bansal wrote:
>>> Hi All
>>>
>>>> -Original Message-
>>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>>>> Laszlo Ersek
>>>> Sent: Thursday, February 22, 2018 7:26 PM
>>>> To: Leif Lindholm <leif.lindh...@linaro.org>
>>>> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
>>>> ard.biesheu...@linaro.org
>>>> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add
>> support
>>>> for Big Endian Mmio APIs
>>>>
>>>> On 02/22/18 12:52, Leif Lindholm wrote:
>>>>> On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:
>>>>
>>>>>>> But that brings back the complication as to how we have a driver
>>>>>>> that needs an LE IO library to write output, and a BE IO library to
>>>>>>> manipulate the hardware.
>>>>>>
>>>>>> Can you please explain the "write output" use case more precisely?
>>>>>>
>>>>>> My thinking would be this:
>>>>>>
>>>>>> - Use the IoLib class directly for "writing output" in little endian
>>>>>> byte order (which is still unclear to me sorry).
>>>>>
>>>>> If the IoLib class is mapped to a an instance that byte-swaps (hereto
>>>>> referred to as BeIoLib if IoLibSwap is unsuitable), would we not then
>>>>> end up mapping the non-swapping, currently implemented in
>>>>> BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up
>>>>> needing to duplicated all IoLib implementation .infs to provide an
>>>>> IoLib and a BeIoLib for each?
>>>>>
>>>>> It's at that point I burst an aneurysm.
>>>>> Am I overthinking/underthinking this?
>>>>
>>>> We need two library classes, one for talking to LE devices and another to
>> BE
>>>> devices. These should be usable in a given module at the same time, as
>> Ard
>>>> says.
>>>>
>>>> Both library classes need to work on both LE and BE CPUs (working from
>> your
>>>> suggestion that UEFI might grow BE CPU support at some point).
>>>> Whether that is implemented by dumb, separate library instances
>> (yielding in
>>>> total 2*2=4 library instances), or by smart, CPU-endianness-agnostic
>> library
>>>> instances (in total, 2), is a different question.
>>>>
>>>> Note that such "smarts" could be less than trivial to implement:
>>>> - check CPU endianness in each library API?
>>>> - or check in the lib constructor only, and flip some function pointers?
>>>> - use a dynamic PCD for caching CPU endianness?
>>>> - use a HOB for the same?
>>>> - use a lib global variable (for caching only on the module level)?
>>>>
>>>> I think the solution that saves the most on the *source* code size is:
>>>> - introduce the BeIoLib class
>>>> - duplicate the MMIO functions from BaseIoLibIntrinsic to the one
>>>>   BeIoLib instance that we introduce
>>>> - modify the MMIO functions in *both* lib instances (original LE, and
>>>>   new BE), like this:
>>>>
>>>>   - If the CPU architecture is known to be bound to a single endianness,
>>>> then hardcode the appropriate operation. This can be done with
>>>> preprocessor macros, or with the architecture support of INF files /
>>>> separate source files. For example, on IA32 and X64, the IoLib
>>>> instance should work transparently, unconditionally, and the BeIoLib
>>>> instance should byte-swap, unconditionally.
>>>>
>>>>   - On other CPU arches, all the wider-than-byte MMIO functions, in
>>>> *both* lib instances should do something like this:
>>>>
>>>>

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Laszlo Ersek]

On 02/23/18 11:25, Udit Kumar wrote:
>
>
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
>> Of Laszlo Ersek
>> Sent: Thursday, February 22, 2018 7:26 PM
>> To: Leif Lindholm <leif.lindh...@linaro.org>
>> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
>> ard.biesheu...@linaro.org
>> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add
>> support for Big Endian Mmio APIs
>>
>> On 02/22/18 12:52, Leif Lindholm wrote:
>>> On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:
>>
>>>>> But that brings back the complication as to how we have a driver
>>>>> that needs an LE IO library to write output, and a BE IO library
>>>>> to manipulate the hardware.
>>>>
>>>> Can you please explain the "write output" use case more precisely?
>>>>
>>>> My thinking would be this:
>>>>
>>>> - Use the IoLib class directly for "writing output" in little
>>>> endian byte order (which is still unclear to me sorry).
>>>
>>> If the IoLib class is mapped to a an instance that byte-swaps
>>> (hereto referred to as BeIoLib if IoLibSwap is unsuitable), would we
>>> not then end up mapping the non-swapping, currently implemented in
>>> BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up
>>> needing to duplicated all IoLib implementation .infs to provide an
>>> IoLib and a BeIoLib for each?
>>>
>>> It's at that point I burst an aneurysm.
>>> Am I overthinking/underthinking this?
>>
>> We need two library classes, one for talking to LE devices and
>> another to BE devices. These should be usable in a given module at
>> the same time, as Ard says.
>>
>> Both library classes need to work on both LE and BE CPUs (working
>> from your suggestion that UEFI might grow BE CPU support at some
>> point). Whether that is implemented by dumb, separate library
>> instances (yielding in total 2*2=4 library instances), or by smart,
>> CPU-endianness-agnostic library instances (in total, 2), is a
>> different question.
>>
>> Note that such "smarts" could be less than trivial to implement:
>> - check CPU endianness in each library API?
>> - or check in the lib constructor only, and flip some function
>>   pointers?
>> - use a dynamic PCD for caching CPU endianness?
>> - use a HOB for the same?
>> - use a lib global variable (for caching only on the module level)?
>>
>> I think the solution that saves the most on the *source* code size
>> is:
>> - introduce the BeIoLib class
>> - duplicate the MMIO functions from BaseIoLibIntrinsic to the one
>>   BeIoLib instance that we introduce
>> - modify the MMIO functions in *both* lib instances (original LE, and
>>   new BE), like this:
>>
>>   - If the CPU architecture is known to be bound to a single
>> endianness, then hardcode the appropriate operation. This can be
>> done with preprocessor macros, or with the architecture support
>> of INF files / separate source files. For example, on IA32 and
>> X64, the IoLib instance should work transparently,
>> unconditionally, and the BeIoLib instance should byte-swap,
>> unconditionally.
>>
>>   - On other CPU arches, all the wider-than-byte MMIO functions, in
>> *both* lib instances should do something like this:
>>
>> //
>> // at file scope
>> //
>> STATIC CONST UINT16 mOne = 1;
>>
>> //
>> // at function scope
>> //
>> if (*(CONST UINT8 *) == 1) {
>>   //
>>   // CPU in LE mode:
>>   // - work transparently in the IoLib instance
>>   // - byte-swap in the BeIoLib instance
>>   //
>> } else {
>>   //
>>   // CPU in BE mode:
>>   // - byte-swap in the IoLib instance
>>   // - work transparently in the BeIoLib instance
>>   //
>> }
>
> You meant, sort of cpu_to_le and cpu_to_be sort of APIs

I'm lost. I don't know how to put it any clearer. Let me try with actual
code.

(a) Suggested for "MdePkg/Library/BaseIoLibIntrinsic/IoLib.c", which is
used on IA32 and X64, therefore CPU byte order is little endian only:

> UINT32
> EFIAPI
> MmioWrite32 (
>   IN  UINTN Address,
>   IN  UINT32Value
>   )
> {
>   ASSERT ((Address & 3) == 0);
>
>   MemoryFence ();
>   *(volatile UINT32*)Address = V

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Udit Kumar]

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Friday, February 23, 2018 2:51 PM
> To: Pankaj Bansal <pankaj.ban...@nxp.com>; Leif Lindholm
> <leif.lindh...@linaro.org>
> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> ard.biesheu...@linaro.org
> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
> for Big Endian Mmio APIs
> 
> On 02/23/18 09:40, Pankaj Bansal wrote:
> > Hi All
> >
> >> -Original Message-
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> >> Laszlo Ersek
> >> Sent: Thursday, February 22, 2018 7:26 PM
> >> To: Leif Lindholm <leif.lindh...@linaro.org>
> >> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> >> ard.biesheu...@linaro.org
> >> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add
> support
> >> for Big Endian Mmio APIs
> >>
> >> On 02/22/18 12:52, Leif Lindholm wrote:
> >>> On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:
> >>
> >>>>> But that brings back the complication as to how we have a driver
> >>>>> that needs an LE IO library to write output, and a BE IO library to
> >>>>> manipulate the hardware.
> >>>>
> >>>> Can you please explain the "write output" use case more precisely?
> >>>>
> >>>> My thinking would be this:
> >>>>
> >>>> - Use the IoLib class directly for "writing output" in little endian
> >>>> byte order (which is still unclear to me sorry).
> >>>
> >>> If the IoLib class is mapped to a an instance that byte-swaps (hereto
> >>> referred to as BeIoLib if IoLibSwap is unsuitable), would we not then
> >>> end up mapping the non-swapping, currently implemented in
> >>> BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up
> >>> needing to duplicated all IoLib implementation .infs to provide an
> >>> IoLib and a BeIoLib for each?
> >>>
> >>> It's at that point I burst an aneurysm.
> >>> Am I overthinking/underthinking this?
> >>
> >> We need two library classes, one for talking to LE devices and another to
> BE
> >> devices. These should be usable in a given module at the same time, as
> Ard
> >> says.
> >>
> >> Both library classes need to work on both LE and BE CPUs (working from
> your
> >> suggestion that UEFI might grow BE CPU support at some point).
> >> Whether that is implemented by dumb, separate library instances
> (yielding in
> >> total 2*2=4 library instances), or by smart, CPU-endianness-agnostic
> library
> >> instances (in total, 2), is a different question.
> >>
> >> Note that such "smarts" could be less than trivial to implement:
> >> - check CPU endianness in each library API?
> >> - or check in the lib constructor only, and flip some function pointers?
> >> - use a dynamic PCD for caching CPU endianness?
> >> - use a HOB for the same?
> >> - use a lib global variable (for caching only on the module level)?
> >>
> >> I think the solution that saves the most on the *source* code size is:
> >> - introduce the BeIoLib class
> >> - duplicate the MMIO functions from BaseIoLibIntrinsic to the one
> >>   BeIoLib instance that we introduce
> >> - modify the MMIO functions in *both* lib instances (original LE, and
> >>   new BE), like this:
> >>
> >>   - If the CPU architecture is known to be bound to a single endianness,
> >> then hardcode the appropriate operation. This can be done with
> >> preprocessor macros, or with the architecture support of INF files /
> >> separate source files. For example, on IA32 and X64, the IoLib
> >> instance should work transparently, unconditionally, and the BeIoLib
> >> instance should byte-swap, unconditionally.
> >>
> >>   - On other CPU arches, all the wider-than-byte MMIO functions, in
> >> *both* lib instances should do something like this:
> >>
> >> //
> >> // at file scope
> >> //
> >> STATIC CONST UINT16 mOne = 1;
> >>
> >> //
> >> // at function scope
> >> //
> >> if (*(CONST UINT8 *) == 1) {
> >>   //
> >>   // CPU in LE mode:
> >>  

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Udit Kumar]

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, February 22, 2018 7:26 PM
> To: Leif Lindholm <leif.lindh...@linaro.org>
> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> ard.biesheu...@linaro.org
> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
> for Big Endian Mmio APIs
> 
> On 02/22/18 12:52, Leif Lindholm wrote:
> > On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:
> 
> >>> But that brings back the complication as to how we have a driver
> >>> that needs an LE IO library to write output, and a BE IO library to
> >>> manipulate the hardware.
> >>
> >> Can you please explain the "write output" use case more precisely?
> >>
> >> My thinking would be this:
> >>
> >> - Use the IoLib class directly for "writing output" in little endian
> >> byte order (which is still unclear to me sorry).
> >
> > If the IoLib class is mapped to a an instance that byte-swaps (hereto
> > referred to as BeIoLib if IoLibSwap is unsuitable), would we not then
> > end up mapping the non-swapping, currently implemented in
> > BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up
> > needing to duplicated all IoLib implementation .infs to provide an
> > IoLib and a BeIoLib for each?
> >
> > It's at that point I burst an aneurysm.
> > Am I overthinking/underthinking this?
> 
> We need two library classes, one for talking to LE devices and another to BE
> devices. These should be usable in a given module at the same time, as Ard
> says.
> 
> Both library classes need to work on both LE and BE CPUs (working from
> your suggestion that UEFI might grow BE CPU support at some point).
> Whether that is implemented by dumb, separate library instances (yielding
> in total 2*2=4 library instances), or by smart, CPU-endianness-agnostic
> library instances (in total, 2), is a different question.
> 
> Note that such "smarts" could be less than trivial to implement:
> - check CPU endianness in each library API?
> - or check in the lib constructor only, and flip some function pointers?
> - use a dynamic PCD for caching CPU endianness?
> - use a HOB for the same?
> - use a lib global variable (for caching only on the module level)?
> 
> I think the solution that saves the most on the *source* code size is:
> - introduce the BeIoLib class
> - duplicate the MMIO functions from BaseIoLibIntrinsic to the one
>   BeIoLib instance that we introduce
> - modify the MMIO functions in *both* lib instances (original LE, and
>   new BE), like this:
> 
>   - If the CPU architecture is known to be bound to a single endianness,
> then hardcode the appropriate operation. This can be done with
> preprocessor macros, or with the architecture support of INF files /
> separate source files. For example, on IA32 and X64, the IoLib
> instance should work transparently, unconditionally, and the BeIoLib
> instance should byte-swap, unconditionally.
> 
>   - On other CPU arches, all the wider-than-byte MMIO functions, in
> *both* lib instances should do something like this:
> 
> //
> // at file scope
> //
> STATIC CONST UINT16 mOne = 1;
> 
> //
> // at function scope
> //
> if (*(CONST UINT8 *) == 1) {
>   //
>   // CPU in LE mode:
>   // - work transparently in the IoLib instance
>   // - byte-swap in the BeIoLib instance
>   //
> } else {
>   //
>   // CPU in BE mode:
>   // - byte-swap in the IoLib instance
>   // - work transparently in the BeIoLib instance
>   //
> }

You meant, sort of cpu_to_le and cpu_to_be sort of APIs 
Thanks
Udit
 
> Thanks,
> Laszlo
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.
> 01.org%2Fmailman%2Flistinfo%2Fedk2-
> devel=02%7C01%7Cudit.kumar%40nxp.com%7C930d96bb226d4491df2
> d08d579fc1717%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63654
> 9046016636482=0uPLjiDP60oNVSdbh44gostMx2id%2BzdLYjk8t%2BLwJ
> tU%3D=0
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Laszlo Ersek]

On 02/23/18 10:47, Meenakshi Aggarwal wrote:
> 
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Friday, February 23, 2018 2:51 PM
>> To: Pankaj Bansal <pankaj.ban...@nxp.com>; Leif Lindholm
>> <leif.lindh...@linaro.org>
>> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
>> ard.biesheu...@linaro.org
>> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
>> for Big Endian Mmio APIs
>>
>> On 02/23/18 09:40, Pankaj Bansal wrote:
>>> Hi All
>>>
>>>> -Original Message-
>>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>>>> Laszlo Ersek
>>>> Sent: Thursday, February 22, 2018 7:26 PM
>>>> To: Leif Lindholm <leif.lindh...@linaro.org>
>>>> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
>>>> ard.biesheu...@linaro.org
>>>> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add
>> support
>>>> for Big Endian Mmio APIs
>>>>
>>>> On 02/22/18 12:52, Leif Lindholm wrote:
>>>>> On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:
>>>>
>>>>>>> But that brings back the complication as to how we have a driver
>>>>>>> that needs an LE IO library to write output, and a BE IO library to
>>>>>>> manipulate the hardware.
>>>>>>
>>>>>> Can you please explain the "write output" use case more precisely?
>>>>>>
>>>>>> My thinking would be this:
>>>>>>
>>>>>> - Use the IoLib class directly for "writing output" in little endian
>>>>>> byte order (which is still unclear to me sorry).
>>>>>
>>>>> If the IoLib class is mapped to a an instance that byte-swaps (hereto
>>>>> referred to as BeIoLib if IoLibSwap is unsuitable), would we not then
>>>>> end up mapping the non-swapping, currently implemented in
>>>>> BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up
>>>>> needing to duplicated all IoLib implementation .infs to provide an
>>>>> IoLib and a BeIoLib for each?
>>>>>
>>>>> It's at that point I burst an aneurysm.
>>>>> Am I overthinking/underthinking this?
>>>>
>>>> We need two library classes, one for talking to LE devices and another to
>> BE
>>>> devices. These should be usable in a given module at the same time, as
>> Ard
>>>> says.
>>>>
>>>> Both library classes need to work on both LE and BE CPUs (working from
>> your
>>>> suggestion that UEFI might grow BE CPU support at some point).
>>>> Whether that is implemented by dumb, separate library instances
>> (yielding in
>>>> total 2*2=4 library instances), or by smart, CPU-endianness-agnostic
>> library
>>>> instances (in total, 2), is a different question.
>>>>
>>>> Note that such "smarts" could be less than trivial to implement:
>>>> - check CPU endianness in each library API?
>>>> - or check in the lib constructor only, and flip some function pointers?
>>>> - use a dynamic PCD for caching CPU endianness?
>>>> - use a HOB for the same?
>>>> - use a lib global variable (for caching only on the module level)?
>>>>
>>>> I think the solution that saves the most on the *source* code size is:
>>>> - introduce the BeIoLib class
>>>> - duplicate the MMIO functions from BaseIoLibIntrinsic to the one
>>>>   BeIoLib instance that we introduce
>>>> - modify the MMIO functions in *both* lib instances (original LE, and
>>>>   new BE), like this:
>>>>
>>>>   - If the CPU architecture is known to be bound to a single endianness,
>>>> then hardcode the appropriate operation. This can be done with
>>>> preprocessor macros, or with the architecture support of INF files /
>>>> separate source files. For example, on IA32 and X64, the IoLib
>>>> instance should work transparently, unconditionally, and the BeIoLib
>>>> instance should byte-swap, unconditionally.
>>>>
>>>>   - On other CPU arches, all the wider-than-byte MMIO functions, in
>>>> *both* lib instances should do something like this:
>>>>
>

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Meenakshi Aggarwal]

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Friday, February 23, 2018 2:51 PM
> To: Pankaj Bansal <pankaj.ban...@nxp.com>; Leif Lindholm
> <leif.lindh...@linaro.org>
> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> ard.biesheu...@linaro.org
> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
> for Big Endian Mmio APIs
> 
> On 02/23/18 09:40, Pankaj Bansal wrote:
> > Hi All
> >
> >> -Original Message-
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> >> Laszlo Ersek
> >> Sent: Thursday, February 22, 2018 7:26 PM
> >> To: Leif Lindholm <leif.lindh...@linaro.org>
> >> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> >> ard.biesheu...@linaro.org
> >> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add
> support
> >> for Big Endian Mmio APIs
> >>
> >> On 02/22/18 12:52, Leif Lindholm wrote:
> >>> On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:
> >>
> >>>>> But that brings back the complication as to how we have a driver
> >>>>> that needs an LE IO library to write output, and a BE IO library to
> >>>>> manipulate the hardware.
> >>>>
> >>>> Can you please explain the "write output" use case more precisely?
> >>>>
> >>>> My thinking would be this:
> >>>>
> >>>> - Use the IoLib class directly for "writing output" in little endian
> >>>> byte order (which is still unclear to me sorry).
> >>>
> >>> If the IoLib class is mapped to a an instance that byte-swaps (hereto
> >>> referred to as BeIoLib if IoLibSwap is unsuitable), would we not then
> >>> end up mapping the non-swapping, currently implemented in
> >>> BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up
> >>> needing to duplicated all IoLib implementation .infs to provide an
> >>> IoLib and a BeIoLib for each?
> >>>
> >>> It's at that point I burst an aneurysm.
> >>> Am I overthinking/underthinking this?
> >>
> >> We need two library classes, one for talking to LE devices and another to
> BE
> >> devices. These should be usable in a given module at the same time, as
> Ard
> >> says.
> >>
> >> Both library classes need to work on both LE and BE CPUs (working from
> your
> >> suggestion that UEFI might grow BE CPU support at some point).
> >> Whether that is implemented by dumb, separate library instances
> (yielding in
> >> total 2*2=4 library instances), or by smart, CPU-endianness-agnostic
> library
> >> instances (in total, 2), is a different question.
> >>
> >> Note that such "smarts" could be less than trivial to implement:
> >> - check CPU endianness in each library API?
> >> - or check in the lib constructor only, and flip some function pointers?
> >> - use a dynamic PCD for caching CPU endianness?
> >> - use a HOB for the same?
> >> - use a lib global variable (for caching only on the module level)?
> >>
> >> I think the solution that saves the most on the *source* code size is:
> >> - introduce the BeIoLib class
> >> - duplicate the MMIO functions from BaseIoLibIntrinsic to the one
> >>   BeIoLib instance that we introduce
> >> - modify the MMIO functions in *both* lib instances (original LE, and
> >>   new BE), like this:
> >>
> >>   - If the CPU architecture is known to be bound to a single endianness,
> >> then hardcode the appropriate operation. This can be done with
> >> preprocessor macros, or with the architecture support of INF files /
> >> separate source files. For example, on IA32 and X64, the IoLib
> >> instance should work transparently, unconditionally, and the BeIoLib
> >> instance should byte-swap, unconditionally.
> >>
> >>   - On other CPU arches, all the wider-than-byte MMIO functions, in
> >> *both* lib instances should do something like this:
> >>
> >> //
> >> // at file scope
> >> //
> >> STATIC CONST UINT16 mOne = 1;
> >>
> >> //
> >> // at function scope
> >> //
> >> if (*(CONST UINT8 *) == 1) {
> >>   //
> >>   // CPU in LE mode:

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Laszlo Ersek]

On 02/23/18 09:40, Pankaj Bansal wrote:
> Hi All
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Thursday, February 22, 2018 7:26 PM
>> To: Leif Lindholm <leif.lindh...@linaro.org>
>> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
>> ard.biesheu...@linaro.org
>> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
>> for Big Endian Mmio APIs
>>
>> On 02/22/18 12:52, Leif Lindholm wrote:
>>> On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:
>>
>>>>> But that brings back the complication as to how we have a driver
>>>>> that needs an LE IO library to write output, and a BE IO library to
>>>>> manipulate the hardware.
>>>>
>>>> Can you please explain the "write output" use case more precisely?
>>>>
>>>> My thinking would be this:
>>>>
>>>> - Use the IoLib class directly for "writing output" in little endian
>>>> byte order (which is still unclear to me sorry).
>>>
>>> If the IoLib class is mapped to a an instance that byte-swaps (hereto
>>> referred to as BeIoLib if IoLibSwap is unsuitable), would we not then
>>> end up mapping the non-swapping, currently implemented in
>>> BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up
>>> needing to duplicated all IoLib implementation .infs to provide an
>>> IoLib and a BeIoLib for each?
>>>
>>> It's at that point I burst an aneurysm.
>>> Am I overthinking/underthinking this?
>>
>> We need two library classes, one for talking to LE devices and another to BE
>> devices. These should be usable in a given module at the same time, as Ard
>> says.
>>
>> Both library classes need to work on both LE and BE CPUs (working from your
>> suggestion that UEFI might grow BE CPU support at some point).
>> Whether that is implemented by dumb, separate library instances (yielding in
>> total 2*2=4 library instances), or by smart, CPU-endianness-agnostic library
>> instances (in total, 2), is a different question.
>>
>> Note that such "smarts" could be less than trivial to implement:
>> - check CPU endianness in each library API?
>> - or check in the lib constructor only, and flip some function pointers?
>> - use a dynamic PCD for caching CPU endianness?
>> - use a HOB for the same?
>> - use a lib global variable (for caching only on the module level)?
>>
>> I think the solution that saves the most on the *source* code size is:
>> - introduce the BeIoLib class
>> - duplicate the MMIO functions from BaseIoLibIntrinsic to the one
>>   BeIoLib instance that we introduce
>> - modify the MMIO functions in *both* lib instances (original LE, and
>>   new BE), like this:
>>
>>   - If the CPU architecture is known to be bound to a single endianness,
>> then hardcode the appropriate operation. This can be done with
>> preprocessor macros, or with the architecture support of INF files /
>> separate source files. For example, on IA32 and X64, the IoLib
>> instance should work transparently, unconditionally, and the BeIoLib
>> instance should byte-swap, unconditionally.
>>
>>   - On other CPU arches, all the wider-than-byte MMIO functions, in
>> *both* lib instances should do something like this:
>>
>> //
>> // at file scope
>> //
>> STATIC CONST UINT16 mOne = 1;
>>
>> //
>> // at function scope
>> //
>> if (*(CONST UINT8 *) == 1) {
>>   //
>>   // CPU in LE mode:
>>   // - work transparently in the IoLib instance
>>   // - byte-swap in the BeIoLib instance
>>   //
>> } else {
>>   //
>>   // CPU in BE mode:
>>   // - byte-swap in the IoLib instance
>>   // - work transparently in the BeIoLib instance
>>   //
>> }
> 
> I suggest this approach :
> 
> 1. Add BeMmio* functions in existing IoLib. BeMmio* functions will swap the 
> input before write and swap output after read and so on.
> Mmio* functions will not perform any byte swapping
> 2. create second instance (a copy) of this IoLib for CPUs that are Big 
> Endian. We can call it BigEndianIoLib.
>  In this library Mmio* functions will swap the input before write and 
> swap output after read and so on.
>  BeMmio* functions will not perform any byte swapping.
> 3. Include the instance of IoL

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Pankaj Bansal]

Hi All

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, February 22, 2018 7:26 PM
> To: Leif Lindholm <leif.lindh...@linaro.org>
> Cc: michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> ard.biesheu...@linaro.org
> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
> for Big Endian Mmio APIs
> 
> On 02/22/18 12:52, Leif Lindholm wrote:
> > On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:
> 
> >>> But that brings back the complication as to how we have a driver
> >>> that needs an LE IO library to write output, and a BE IO library to
> >>> manipulate the hardware.
> >>
> >> Can you please explain the "write output" use case more precisely?
> >>
> >> My thinking would be this:
> >>
> >> - Use the IoLib class directly for "writing output" in little endian
> >> byte order (which is still unclear to me sorry).
> >
> > If the IoLib class is mapped to a an instance that byte-swaps (hereto
> > referred to as BeIoLib if IoLibSwap is unsuitable), would we not then
> > end up mapping the non-swapping, currently implemented in
> > BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up
> > needing to duplicated all IoLib implementation .infs to provide an
> > IoLib and a BeIoLib for each?
> >
> > It's at that point I burst an aneurysm.
> > Am I overthinking/underthinking this?
> 
> We need two library classes, one for talking to LE devices and another to BE
> devices. These should be usable in a given module at the same time, as Ard
> says.
> 
> Both library classes need to work on both LE and BE CPUs (working from your
> suggestion that UEFI might grow BE CPU support at some point).
> Whether that is implemented by dumb, separate library instances (yielding in
> total 2*2=4 library instances), or by smart, CPU-endianness-agnostic library
> instances (in total, 2), is a different question.
> 
> Note that such "smarts" could be less than trivial to implement:
> - check CPU endianness in each library API?
> - or check in the lib constructor only, and flip some function pointers?
> - use a dynamic PCD for caching CPU endianness?
> - use a HOB for the same?
> - use a lib global variable (for caching only on the module level)?
> 
> I think the solution that saves the most on the *source* code size is:
> - introduce the BeIoLib class
> - duplicate the MMIO functions from BaseIoLibIntrinsic to the one
>   BeIoLib instance that we introduce
> - modify the MMIO functions in *both* lib instances (original LE, and
>   new BE), like this:
> 
>   - If the CPU architecture is known to be bound to a single endianness,
> then hardcode the appropriate operation. This can be done with
> preprocessor macros, or with the architecture support of INF files /
> separate source files. For example, on IA32 and X64, the IoLib
> instance should work transparently, unconditionally, and the BeIoLib
> instance should byte-swap, unconditionally.
> 
>   - On other CPU arches, all the wider-than-byte MMIO functions, in
> *both* lib instances should do something like this:
> 
> //
> // at file scope
> //
> STATIC CONST UINT16 mOne = 1;
> 
> //
> // at function scope
> //
> if (*(CONST UINT8 *) == 1) {
>   //
>   // CPU in LE mode:
>   // - work transparently in the IoLib instance
>   // - byte-swap in the BeIoLib instance
>   //
> } else {
>   //
>   // CPU in BE mode:
>   // - byte-swap in the IoLib instance
>   // - work transparently in the BeIoLib instance
>   //
> }

I suggest this approach :

1. Add BeMmio* functions in existing IoLib. BeMmio* functions will swap the 
input before write and swap output after read and so on.
Mmio* functions will not perform any byte swapping
2. create second instance (a copy) of this IoLib for CPUs that are Big Endian. 
We can call it BigEndianIoLib.
 In this library Mmio* functions will swap the input before write and swap 
output after read and so on.
 BeMmio* functions will not perform any byte swapping.
3. Include the instance of IoLib in dsc file based on cpu endianness that the 
platform wants to use. i.e.
If BIG_ENDIAN == FALSE
   IoLib | ..\..\..\IoLib
   Else
  IoLib | ..\..\..\BigEndianIoLib
4. The devices that are Big endian in platform will always call BeMmio* 
functions. They need not check CPU endianness.
5. The devices that are Little endian in platform will always call Mmio* 
functions. They need not check CPU endianness.

> 
> Thank

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Laszlo Ersek]

On 02/22/18 12:52, Leif Lindholm wrote:
> On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:

>>> But that brings back the complication as to how we have a driver that
>>> needs an LE IO library to write output, and a BE IO library to
>>> manipulate the hardware.
>>
>> Can you please explain the "write output" use case more precisely?
>>
>> My thinking would be this:
>>
>> - Use the IoLib class directly for "writing output" in little endian
>> byte order (which is still unclear to me sorry).
> 
> If the IoLib class is mapped to a an instance that byte-swaps (hereto
> referred to as BeIoLib if IoLibSwap is unsuitable), would we not then
> end up mapping the non-swapping, currently implemented in
> BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up
> needing to duplicated all IoLib implementation .infs to provide an
> IoLib and a BeIoLib for each?
> 
> It's at that point I burst an aneurysm.
> Am I overthinking/underthinking this?

We need two library classes, one for talking to LE devices and another
to BE devices. These should be usable in a given module at the same
time, as Ard says.

Both library classes need to work on both LE and BE CPUs (working from
your suggestion that UEFI might grow BE CPU support at some point).
Whether that is implemented by dumb, separate library instances
(yielding in total 2*2=4 library instances), or by smart,
CPU-endianness-agnostic library instances (in total, 2), is a different
question.

Note that such "smarts" could be less than trivial to implement:
- check CPU endianness in each library API?
- or check in the lib constructor only, and flip some function pointers?
- use a dynamic PCD for caching CPU endianness?
- use a HOB for the same?
- use a lib global variable (for caching only on the module level)?

I think the solution that saves the most on the *source* code size is:
- introduce the BeIoLib class
- duplicate the MMIO functions from BaseIoLibIntrinsic to the one
  BeIoLib instance that we introduce
- modify the MMIO functions in *both* lib instances (original LE, and
  new BE), like this:

  - If the CPU architecture is known to be bound to a single endianness,
then hardcode the appropriate operation. This can be done with
preprocessor macros, or with the architecture support of INF files /
separate source files. For example, on IA32 and X64, the IoLib
instance should work transparently, unconditionally, and the BeIoLib
instance should byte-swap, unconditionally.

  - On other CPU arches, all the wider-than-byte MMIO functions, in
*both* lib instances should do something like this:

//
// at file scope
//
STATIC CONST UINT16 mOne = 1;

//
// at function scope
//
if (*(CONST UINT8 *) == 1) {
  //
  // CPU in LE mode:
  // - work transparently in the IoLib instance
  // - byte-swap in the BeIoLib instance
  //
} else {
  //
  // CPU in BE mode:
  // - byte-swap in the IoLib instance
  // - work transparently in the BeIoLib instance
  //
}

Thanks,
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Leif Lindholm]

On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:
> >> - As long as the specs are LE-only, "endianness opposite to the
> >> executing processor" is needless complication / speculative generality
> >> in my eyes.
> > 
> > HTON/NTOH?
> 
> I don't understand this reference; host-to-net and net-to-host seem to
> support my suggestion. "net" is always BE, and "host" is whatever it is.
> So the API names are the same, but they work differently, dependent on
> CPU byte order. (Apologies if you mean something else by HTON/NTOH.)

Well, they work differently depending on whether host and net byte
order differ or not :)

> > The specs are not LE-only.
> > PI _is_ (at this point in time) LE-only.
> > UEFI leaves this entirely to architectural bindings.
> 
> I disagree; from UEFI 2.7:
> 
> 1 Introduction
> 1.9 Conventions Used in this Document
> 1.9.1 Data Structure Descriptions
> 
> Supported processors are “little endian” machines. This distinction
> means that the low-order byte of a multibyte data item in memory is
> at the lowest address, while the high-order byte is at the highest
> address. Some supported 64-bit processors may be configured for both
> “little endian” and “big endian” operation. All implementations
> designed to conform to this specification use “little endian”
> operation.

Interestingly, that paragraph never appeared when I searched through
the spec... (but I see it now you point it out).

Nevertheless, the spec is reflecting the current state of things. This
is UEFI, not ULEEFI, and is a BE-only architecture was ever added, the
spec would need to change.

> >> - Even if we supported multiple endiannesses on the CPU front, the API
> >> names should reflect the *device* byte order, not the CPU byte order.
> >> Think of the case when the same platform device is integrated on board
> >> B1 whose CPU is LE, and on board B2 whose CPU is BE.
> > 
> > The actual watchdog code in this series, and comments made on the
> > list, suggests that there exists variants of this _device_ with BE
> > or LE byte order.
> > 
> > If this is not the case, then yes, I agree that BE-naming makes sense.
> 
> In my opinion, even if the same device is seen in the wild with both
> byte orders, the library class (the function names) should be designed
> independently of CPU byte order. The device driver should pick one set
> of functions dependent on device byte order (if indeed both byte orders
> are seen on variants of the device). How those APIs map to CPU byte
> order is a library instance detail.
> 
> > So, Meenakshi - can you confirm that the Watchdog driver is expected
> > to be used against devices in both BE and LE mode?
> > 
> > If it is the case, maybe this library would make more sense as the
> > non-standard protocol you suggested in
> > https://www.mail-archive.com/edk2-devel@lists.01.org/msg17869.html
> > ?
> 
> Hmmm. Looking back at that email of mine, I think my "incomplete"
> argument no longer stands. Jordan and Mike have explained to me since
> that BaseLib (the class) has several APIs that are simply
> un-implementable on various CPUs, for example it has a number of
> Itanium-specific functions. Drivers that are not tied to IPF should
> simply not call them. This is supposed to be by design.
> 
> Having skimmed your RFC, I also think I may have been wrong about
> "overkill" originally. If there are many BE registers, swapping bytes
> all the time explicitly is annoying, and people can mess up the word
> size. So right now I feel that having these APIs in a new lib class
> should be OK, as long as the function names are clear.

OK.

> (Clearly I've been wrong already on this topic, see above, so do take my
> points with a grain of salt :) )
> 
> >> If we name the APIs
> >> after the CPU byte order, then the same driver source code will be
> >> misleading on one of the boards. Whereas, if we name the APIs after
> >> device byte order, then the driver source code will be correct
> >> regardless of board / CPU, and only the internal workings of the APIs
> >> should change. For example, on a BE CPU / platform, the "normal" (LE)
> >> IoLib class should be resolved to an instance that byte-swaps
> >> internally, and the BE IoLib class should be resolved to an instance
> >> that is transparent internally.
> > 
> > Right.
> > 
> > But that brings back the complication as to how we have a driver that
> > needs an LE IO library to write output, and a BE IO library to
> > manipulate the hardware.
> 
> Can you please explain the "write output" use case more precisely?
> 
> My thinking would be this:
> 
> - Use the IoLib class directly for "writing output" in little endian
> byte order (which is still unclear to me sorry).

If the IoLib class is mapped to a an instance that byte-swaps (hereto
referred to as BeIoLib if IoLibSwap is unsuitable), would we not then
end up mapping the non-swapping, currently implemented in
BaseLibIoIntrinsic, variant as BeIoLib? Or if 

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Laszlo Ersek]

On 02/21/18 19:58, Leif Lindholm wrote:
> On Wed, Feb 21, 2018 at 05:06:02PM +0100, Laszlo Ersek wrote:
>> On 02/21/18 16:46, Leif Lindholm wrote:
>>> Apologies for dropping the ball on this series during my sabbatical.
>>>
>>> For this particular patch, I would still like to see a core library
>>> provide the needed functionality. I just sent out an RFC of a possible
>>> implementation.
>>>
>>> Regardless, a key point is that this isn't about "big-endian", it is
>>> about endianness opposite to the executing processor.
>>
>> I commented on just this aspect under your RFC. I think I disagree, for
>> two reasons:
>>
>> - As long as the specs are LE-only, "endianness opposite to the
>> executing processor" is needless complication / speculative generality
>> in my eyes.
> 
> HTON/NTOH?

I don't understand this reference; host-to-net and net-to-host seem to
support my suggestion. "net" is always BE, and "host" is whatever it is.
So the API names are the same, but they work differently, dependent on
CPU byte order. (Apologies if you mean something else by HTON/NTOH.)


> The specs are not LE-only.
> PI _is_ (at this point in time) LE-only.
> UEFI leaves this entirely to architectural bindings.

I disagree; from UEFI 2.7:

1 Introduction
1.9 Conventions Used in this Document
1.9.1 Data Structure Descriptions

Supported processors are “little endian” machines. This distinction
means that the low-order byte of a multibyte data item in memory is
at the lowest address, while the high-order byte is at the highest
address. Some supported 64-bit processors may be configured for both
“little endian” and “big endian” operation. All implementations
designed to conform to this specification use “little endian”
operation.


> For PI, this is mentioned in a single paragraph, repeated 4 times in
> the PI 1.6 specification (due to it merging what was previously
> separate documents).

The same paragraph is present in UEFI 2.7.


>> - Even if we supported multiple endiannesses on the CPU front, the API
>> names should reflect the *device* byte order, not the CPU byte order.
>> Think of the case when the same platform device is integrated on board
>> B1 whose CPU is LE, and on board B2 whose CPU is BE.
> 
> The actual watchdog code in this series, and comments made on the
> list, suggests that there exists variants of this _device_ with BE
> or LE byte order.
> 
> If this is not the case, then yes, I agree that BE-naming makes sense.

In my opinion, even if the same device is seen in the wild with both
byte orders, the library class (the function names) should be designed
independently of CPU byte order. The device driver should pick one set
of functions dependent on device byte order (if indeed both byte orders
are seen on variants of the device). How those APIs map to CPU byte
order is a library instance detail.


> So, Meenakshi - can you confirm that the Watchdog driver is expected
> to be used against devices in both BE and LE mode?
> 
> If it is the case, maybe this library would make more sense as the
> non-standard protocol you suggested in
> https://www.mail-archive.com/edk2-devel@lists.01.org/msg17869.html
> ?

Hmmm. Looking back at that email of mine, I think my "incomplete"
argument no longer stands. Jordan and Mike have explained to me since
that BaseLib (the class) has several APIs that are simply
un-implementable on various CPUs, for example it has a number of
Itanium-specific functions. Drivers that are not tied to IPF should
simply not call them. This is supposed to be by design.

Having skimmed your RFC, I also think I may have been wrong about
"overkill" originally. If there are many BE registers, swapping bytes
all the time explicitly is annoying, and people can mess up the word
size. So right now I feel that having these APIs in a new lib class
should be OK, as long as the function names are clear.

(Clearly I've been wrong already on this topic, see above, so do take my
points with a grain of salt :) )

>> If we name the APIs
>> after the CPU byte order, then the same driver source code will be
>> misleading on one of the boards. Whereas, if we name the APIs after
>> device byte order, then the driver source code will be correct
>> regardless of board / CPU, and only the internal workings of the APIs
>> should change. For example, on a BE CPU / platform, the "normal" (LE)
>> IoLib class should be resolved to an instance that byte-swaps
>> internally, and the BE IoLib class should be resolved to an instance
>> that is transparent internally.
> 
> Right.
> 
> But that brings back the complication as to how we have a driver that
> needs an LE IO library to write output, and a BE IO library to
> manipulate the hardware.

Can you please explain the "write output" use case more precisely?

My thinking would be this:

- Use the IoLib class directly for "writing output" in little endian
byte order (which is still unclear to me sorry).

- Have some function pointers (global 

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Udit Kumar]

> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Wednesday, February 21, 2018 9:16 PM
> Subject: Re: [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big
> Endian Mmio APIs
> 
> Apologies for dropping the ball on this series during my sabbatical.
> 
> For this particular patch, I would still like to see a core library provide 
> the
> needed functionality. I just sent out an RFC of a possible implementation.
> 
> Regardless, a key point is that this isn't about "big-endian", it is about
> endianness opposite to the executing processor.

Ok.
But We should know endianness of CPU and associated IP at the start. 
Most of time  We are in little endian mode, therefore we had BeMmioxx.

Anyway for me this is ok to rename as SwapMmioxx, if you are making
CPU endianness variant as well.

> /
> Leif
> 
> On Fri, Feb 16, 2018 at 02:19:57PM +0530, Meenakshi wrote:
> > From: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> >
> > This library add supports for BE read/write and other MMIO helper
> > function.
> > In this data swapped after reading from MMIO and before write using
> > MMIO.
> > It can be used by any module with BE address space.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> > ---
> >  Silicon/NXP/Include/Library/BeIoLib.h   | 332
> ++
> >  Silicon/NXP/Library/BeIoLib/BeIoLib.c   | 400
> 
> >  Silicon/NXP/Library/BeIoLib/BeIoLib.inf |  31 +++
> >  3 files changed, 763 insertions(+)
> >  create mode 100644 Silicon/NXP/Include/Library/BeIoLib.h
> >  create mode 100644 Silicon/NXP/Library/BeIoLib/BeIoLib.c
> >  create mode 100644 Silicon/NXP/Library/BeIoLib/BeIoLib.inf
> >
> > diff --git a/Silicon/NXP/Include/Library/BeIoLib.h
> > b/Silicon/NXP/Include/Library/BeIoLib.h
> > new file mode 100644
> > index 000..a58883a
> > --- /dev/null
> > +++ b/Silicon/NXP/Include/Library/BeIoLib.h
> > @@ -0,0 +1,332 @@
> > +/** BeIoLib.h
> > + *
> > + *  Copyright 2017 NXP
> > + *
> > + *  This program and the accompanying materials
> > + *  are licensed and made available under the terms and conditions of
> > +the BSD License
> > + *  which accompanies this distribution.  The full text of the
> > +license may be found at
> > + *
> >
> +https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fope
> > +nsource.org%2Flicenses%2Fbsd-
> license.php=02%7C01%7Cudit.kumar%40
> >
> +nxp.com%7Cf911978f84484376f56e08d5794237e8%7C686ea1d3bc2b4c6fa92
> cd99c
> >
> +5c301635%7C0%7C0%7C636548247671415204=GTzQcn%2FbZmfV41%
> 2BwBELVN
> > +n1OUYJ3zz2ARFG6kF2rvcg%3D=0
> > + *
> > + *  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > +BASIS,
> > + *  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> > + *
> > + **/
> > +
> > +#ifndef __BE_IOLIB_H__
> > +#define __BE_IOLIB_H__
> > +
> > +#include 
> > +
> > +/**
> > +  MmioRead8 for Big-Endian modules.
> > +
> > +  @param  Address The MMIO register to read.
> > +
> > +  @return The value read.
> > +
> > +**/
> > +UINT8
> > +EFIAPI
> > +BeMmioRead8 (
> > +  IN  UINTN Address
> > +  );
> > +
> > +/**
> > +  MmioRead16 for Big-Endian modules.
> > +
> > +  @param  Address The MMIO register to read.
> > +
> > +  @return The value read.
> > +
> > +**/
> > +UINT16
> > +EFIAPI
> > +BeMmioRead16 (
> > +  IN  UINTN Address
> > +  );
> > +
> > +/**
> > +  MmioRead32 for Big-Endian modules.
> > +
> > +  @param  Address The MMIO register to read.
> > +
> > +  @return The value read.
> > +
> > +**/
> > +UINT32
> > +EFIAPI
> > +BeMmioRead32 (
> > +  IN  UINTN Address
> > +  );
> > +
> > +/**
> > +  MmioRead64 for Big-Endian modules.
> > +
> > +  @param  Address The MMIO register to read.
> > +
> > +  @return The value read.
> > +
> > +**/
> > +UINT64
> > +EFIAPI
> > +BeMmioRead64 (
> > +  IN  UINTN Address
> > +  );
> > +
> > +/**
> > +  MmioWrite8 for Big-Endian modules.
> > +
> > +  @param  Address The MMIO register to write.
> > +  @param  Value   The value to write to the MMIO register.
> > +
> > +**/
> > +UINT8
> >

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Meenakshi Aggarwal]

> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Thursday, February 22, 2018 12:28 AM
> To: Laszlo Ersek <ler...@redhat.com>
> Cc: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>;
> michael.d.kin...@intel.com; edk2-devel@lists.01.org;
> ard.biesheu...@linaro.org
> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
> for Big Endian Mmio APIs
> 
> On Wed, Feb 21, 2018 at 05:06:02PM +0100, Laszlo Ersek wrote:
> > On 02/21/18 16:46, Leif Lindholm wrote:
> > > Apologies for dropping the ball on this series during my sabbatical.
> > >
> > > For this particular patch, I would still like to see a core library
> > > provide the needed functionality. I just sent out an RFC of a possible
> > > implementation.
> > >
> > > Regardless, a key point is that this isn't about "big-endian", it is
> > > about endianness opposite to the executing processor.
> >
> > I commented on just this aspect under your RFC. I think I disagree, for
> > two reasons:
> >
> > - As long as the specs are LE-only, "endianness opposite to the
> > executing processor" is needless complication / speculative generality
> > in my eyes.
> 
> HTON/NTOH?
> 
> The specs are not LE-only.
> PI _is_ (at this point in time) LE-only.
> UEFI leaves this entirely to architectural bindings.
> 
> For PI, this is mentioned in a single paragraph, repeated 4 times in
> the PI 1.6 specification (due to it merging what was previously
> separate documents).
> 
> > - Even if we supported multiple endiannesses on the CPU front, the API
> > names should reflect the *device* byte order, not the CPU byte order.
> > Think of the case when the same platform device is integrated on board
> > B1 whose CPU is LE, and on board B2 whose CPU is BE.
> 
> The actual watchdog code in this series, and comments made on the
> list, suggests that there exists variants of this _device_ with BE
> or LE byte order.
> 
> If this is not the case, then yes, I agree that BE-naming makes sense.
> 
> So, Meenakshi - can you confirm that the Watchdog driver is expected
> to be used against devices in both BE and LE mode?
> 
Yes Leif,
Watchdog is BE in this series of patches and will be LE for coming SoCs.
In this series, IFC is the example which is in BE mode for LS1043 and LS1046
and LE for LS2088.

> If it is the case, maybe this library would make more sense as the
> non-standard protocol you suggested in
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw
> ww.mail-archive.com%2Fedk2-
> devel%40lists.01.org%2Fmsg17869.html=02%7C01%7Cmeenakshi.aggar
> wal%40nxp.com%7C3ba61129eca0405466d508d5795d1454%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C636548363050169302=pIcASmlSK
> CD0jsCvEMkKk2ikjSuD19lTa8xErcxaH4Y%3D=0
> ?
> 
> > If we name the APIs
> > after the CPU byte order, then the same driver source code will be
> > misleading on one of the boards. Whereas, if we name the APIs after
> > device byte order, then the driver source code will be correct
> > regardless of board / CPU, and only the internal workings of the APIs
> > should change. For example, on a BE CPU / platform, the "normal" (LE)
> > IoLib class should be resolved to an instance that byte-swaps
> > internally, and the BE IoLib class should be resolved to an instance
> > that is transparent internally.
> 
> Right.
> 
> But that brings back the complication as to how we have a driver that
> needs an LE IO library to write output, and a BE IO library to
> manipulate the hardware.
> 
Yes it is complicated, but we have similar situation. 
> /
> Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Leif Lindholm]

On Wed, Feb 21, 2018 at 05:06:02PM +0100, Laszlo Ersek wrote:
> On 02/21/18 16:46, Leif Lindholm wrote:
> > Apologies for dropping the ball on this series during my sabbatical.
> > 
> > For this particular patch, I would still like to see a core library
> > provide the needed functionality. I just sent out an RFC of a possible
> > implementation.
> > 
> > Regardless, a key point is that this isn't about "big-endian", it is
> > about endianness opposite to the executing processor.
> 
> I commented on just this aspect under your RFC. I think I disagree, for
> two reasons:
> 
> - As long as the specs are LE-only, "endianness opposite to the
> executing processor" is needless complication / speculative generality
> in my eyes.

HTON/NTOH?

The specs are not LE-only.
PI _is_ (at this point in time) LE-only.
UEFI leaves this entirely to architectural bindings.

For PI, this is mentioned in a single paragraph, repeated 4 times in
the PI 1.6 specification (due to it merging what was previously
separate documents).

> - Even if we supported multiple endiannesses on the CPU front, the API
> names should reflect the *device* byte order, not the CPU byte order.
> Think of the case when the same platform device is integrated on board
> B1 whose CPU is LE, and on board B2 whose CPU is BE.

The actual watchdog code in this series, and comments made on the
list, suggests that there exists variants of this _device_ with BE
or LE byte order.

If this is not the case, then yes, I agree that BE-naming makes sense.

So, Meenakshi - can you confirm that the Watchdog driver is expected
to be used against devices in both BE and LE mode?

If it is the case, maybe this library would make more sense as the
non-standard protocol you suggested in
https://www.mail-archive.com/edk2-devel@lists.01.org/msg17869.html
?

> If we name the APIs
> after the CPU byte order, then the same driver source code will be
> misleading on one of the boards. Whereas, if we name the APIs after
> device byte order, then the driver source code will be correct
> regardless of board / CPU, and only the internal workings of the APIs
> should change. For example, on a BE CPU / platform, the "normal" (LE)
> IoLib class should be resolved to an instance that byte-swaps
> internally, and the BE IoLib class should be resolved to an instance
> that is transparent internally.

Right.

But that brings back the complication as to how we have a driver that
needs an LE IO library to write output, and a BE IO library to
manipulate the hardware.

/
Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Laszlo Ersek]

On 02/21/18 16:46, Leif Lindholm wrote:
> Apologies for dropping the ball on this series during my sabbatical.
> 
> For this particular patch, I would still like to see a core library
> provide the needed functionality. I just sent out an RFC of a possible
> implementation.
> 
> Regardless, a key point is that this isn't about "big-endian", it is
> about endianness opposite to the executing processor.

I commented on just this aspect under your RFC. I think I disagree, for
two reasons:

- As long as the specs are LE-only, "endianness opposite to the
executing processor" is needless complication / speculative generality
in my eyes.

- Even if we supported multiple endiannesses on the CPU front, the API
names should reflect the *device* byte order, not the CPU byte order.
Think of the case when the same platform device is integrated on board
B1 whose CPU is LE, and on board B2 whose CPU is BE. If we name the APIs
after the CPU byte order, then the same driver source code will be
misleading on one of the boards. Whereas, if we name the APIs after
device byte order, then the driver source code will be correct
regardless of board / CPU, and only the internal workings of the APIs
should change. For example, on a BE CPU / platform, the "normal" (LE)
IoLib class should be resolved to an instance that byte-swaps
internally, and the BE IoLib class should be resolved to an instance
that is transparent internally.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Leif Lindholm]

Apologies for dropping the ball on this series during my sabbatical.

For this particular patch, I would still like to see a core library
provide the needed functionality. I just sent out an RFC of a possible
implementation.

Regardless, a key point is that this isn't about "big-endian", it is
about endianness opposite to the executing processor.

/
Leif

On Fri, Feb 16, 2018 at 02:19:57PM +0530, Meenakshi wrote:
> From: Meenakshi Aggarwal 
> 
> This library add supports for BE read/write and other
> MMIO helper function.
> In this data swapped after reading from MMIO and before
> write using MMIO.
> It can be used by any module with BE address space.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Meenakshi Aggarwal 
> ---
>  Silicon/NXP/Include/Library/BeIoLib.h   | 332 ++
>  Silicon/NXP/Library/BeIoLib/BeIoLib.c   | 400 
> 
>  Silicon/NXP/Library/BeIoLib/BeIoLib.inf |  31 +++
>  3 files changed, 763 insertions(+)
>  create mode 100644 Silicon/NXP/Include/Library/BeIoLib.h
>  create mode 100644 Silicon/NXP/Library/BeIoLib/BeIoLib.c
>  create mode 100644 Silicon/NXP/Library/BeIoLib/BeIoLib.inf
> 
> diff --git a/Silicon/NXP/Include/Library/BeIoLib.h 
> b/Silicon/NXP/Include/Library/BeIoLib.h
> new file mode 100644
> index 000..a58883a
> --- /dev/null
> +++ b/Silicon/NXP/Include/Library/BeIoLib.h
> @@ -0,0 +1,332 @@
> +/** BeIoLib.h
> + *
> + *  Copyright 2017 NXP
> + *
> + *  This program and the accompanying materials
> + *  are licensed and made available under the terms and conditions of the 
> BSD License
> + *  which accompanies this distribution.  The full text of the license may 
> be found at
> + *  http://opensource.org/licenses/bsd-license.php
> + *
> + *  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> + *  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> + *
> + **/
> +
> +#ifndef __BE_IOLIB_H__
> +#define __BE_IOLIB_H__
> +
> +#include 
> +
> +/**
> +  MmioRead8 for Big-Endian modules.
> +
> +  @param  Address The MMIO register to read.
> +
> +  @return The value read.
> +
> +**/
> +UINT8
> +EFIAPI
> +BeMmioRead8 (
> +  IN  UINTN Address
> +  );
> +
> +/**
> +  MmioRead16 for Big-Endian modules.
> +
> +  @param  Address The MMIO register to read.
> +
> +  @return The value read.
> +
> +**/
> +UINT16
> +EFIAPI
> +BeMmioRead16 (
> +  IN  UINTN Address
> +  );
> +
> +/**
> +  MmioRead32 for Big-Endian modules.
> +
> +  @param  Address The MMIO register to read.
> +
> +  @return The value read.
> +
> +**/
> +UINT32
> +EFIAPI
> +BeMmioRead32 (
> +  IN  UINTN Address
> +  );
> +
> +/**
> +  MmioRead64 for Big-Endian modules.
> +
> +  @param  Address The MMIO register to read.
> +
> +  @return The value read.
> +
> +**/
> +UINT64
> +EFIAPI
> +BeMmioRead64 (
> +  IN  UINTN Address
> +  );
> +
> +/**
> +  MmioWrite8 for Big-Endian modules.
> +
> +  @param  Address The MMIO register to write.
> +  @param  Value   The value to write to the MMIO register.
> +
> +**/
> +UINT8
> +EFIAPI
> +BeMmioWrite8 (
> +  IN  UINTN Address,
> +  IN  UINT8 Value
> +  );
> +
> +/**
> +  MmioWrite16 for Big-Endian modules.
> +
> +  @param  Address The MMIO register to write.
> +  @param  Value   The value to write to the MMIO register.
> +
> +**/
> +UINT16
> +EFIAPI
> +BeMmioWrite16 (
> +  IN  UINTN Address,
> +  IN  UINT16Value
> +  );
> +
> +/**
> +  MmioWrite32 for Big-Endian modules.
> +
> +  @param  Address The MMIO register to write.
> +  @param  Value   The value to write to the MMIO register.
> +
> +**/
> +UINT32
> +EFIAPI
> +BeMmioWrite32 (
> +  IN  UINTN Address,
> +  IN  UINT32Value
> +  );
> +
> +/**
> +  MmioWrite64 for Big-Endian modules.
> +
> +  @param  Address The MMIO register to write.
> +  @param  Value   The value to write to the MMIO register.
> +
> +**/
> +UINT64
> +EFIAPI
> +BeMmioWrite64 (
> +  IN  UINTN Address,
> +  IN  UINT64Value
> +  );
> +
> +/**
> +  MmioAndThenOr8 for Big-Endian modules.
> +
> +  @param  Address The MMIO register to write.
> +  @param  AndData The value to AND with the read value from the MMIO 
> register.
> +  @param  OrData  The value to OR with the result of the AND operation.
> +
> +  @return The value written back to the MMIO register.
> +
> +**/
> +UINT8
> +EFIAPI
> +BeMmioAndThenOr8 (
> +  IN  UINTN Address,
> +  IN  UINT8 AndData,
> +  IN  UINT8 OrData
> +  );
> +
> +/**
> +  MmioAndThenOr16 for Big-Endian modules.
> +
> +  @param  Address The MMIO register to write.
> +  @param  AndData The value to AND with the read value from the MMIO 
> register.
> +  @param  OrData  The value to OR with the result of the AND operation.
> +
> +  @return The value written back to the MMIO register.
> +
> +**/
> +UINT16
> +EFIAPI
> +BeMmioAndThenOr16 (
> +  IN  UINTN Address,
> +  IN  UINT16AndData,
> +  IN  

[edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Meenakshi]

From: Meenakshi Aggarwal 

This library add supports for BE read/write and other
MMIO helper function.
In this data swapped after reading from MMIO and before
write using MMIO.
It can be used by any module with BE address space.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Meenakshi Aggarwal 
---
 Silicon/NXP/Include/Library/BeIoLib.h   | 332 ++
 Silicon/NXP/Library/BeIoLib/BeIoLib.c   | 400 
 Silicon/NXP/Library/BeIoLib/BeIoLib.inf |  31 +++
 3 files changed, 763 insertions(+)
 create mode 100644 Silicon/NXP/Include/Library/BeIoLib.h
 create mode 100644 Silicon/NXP/Library/BeIoLib/BeIoLib.c
 create mode 100644 Silicon/NXP/Library/BeIoLib/BeIoLib.inf

diff --git a/Silicon/NXP/Include/Library/BeIoLib.h 
b/Silicon/NXP/Include/Library/BeIoLib.h
new file mode 100644
index 000..a58883a
--- /dev/null
+++ b/Silicon/NXP/Include/Library/BeIoLib.h
@@ -0,0 +1,332 @@
+/** BeIoLib.h
+ *
+ *  Copyright 2017 NXP
+ *
+ *  This program and the accompanying materials
+ *  are licensed and made available under the terms and conditions of the BSD 
License
+ *  which accompanies this distribution.  The full text of the license may be 
found at
+ *  http://opensource.org/licenses/bsd-license.php
+ *
+ *  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+ *
+ **/
+
+#ifndef __BE_IOLIB_H__
+#define __BE_IOLIB_H__
+
+#include 
+
+/**
+  MmioRead8 for Big-Endian modules.
+
+  @param  Address The MMIO register to read.
+
+  @return The value read.
+
+**/
+UINT8
+EFIAPI
+BeMmioRead8 (
+  IN  UINTN Address
+  );
+
+/**
+  MmioRead16 for Big-Endian modules.
+
+  @param  Address The MMIO register to read.
+
+  @return The value read.
+
+**/
+UINT16
+EFIAPI
+BeMmioRead16 (
+  IN  UINTN Address
+  );
+
+/**
+  MmioRead32 for Big-Endian modules.
+
+  @param  Address The MMIO register to read.
+
+  @return The value read.
+
+**/
+UINT32
+EFIAPI
+BeMmioRead32 (
+  IN  UINTN Address
+  );
+
+/**
+  MmioRead64 for Big-Endian modules.
+
+  @param  Address The MMIO register to read.
+
+  @return The value read.
+
+**/
+UINT64
+EFIAPI
+BeMmioRead64 (
+  IN  UINTN Address
+  );
+
+/**
+  MmioWrite8 for Big-Endian modules.
+
+  @param  Address The MMIO register to write.
+  @param  Value   The value to write to the MMIO register.
+
+**/
+UINT8
+EFIAPI
+BeMmioWrite8 (
+  IN  UINTN Address,
+  IN  UINT8 Value
+  );
+
+/**
+  MmioWrite16 for Big-Endian modules.
+
+  @param  Address The MMIO register to write.
+  @param  Value   The value to write to the MMIO register.
+
+**/
+UINT16
+EFIAPI
+BeMmioWrite16 (
+  IN  UINTN Address,
+  IN  UINT16Value
+  );
+
+/**
+  MmioWrite32 for Big-Endian modules.
+
+  @param  Address The MMIO register to write.
+  @param  Value   The value to write to the MMIO register.
+
+**/
+UINT32
+EFIAPI
+BeMmioWrite32 (
+  IN  UINTN Address,
+  IN  UINT32Value
+  );
+
+/**
+  MmioWrite64 for Big-Endian modules.
+
+  @param  Address The MMIO register to write.
+  @param  Value   The value to write to the MMIO register.
+
+**/
+UINT64
+EFIAPI
+BeMmioWrite64 (
+  IN  UINTN Address,
+  IN  UINT64Value
+  );
+
+/**
+  MmioAndThenOr8 for Big-Endian modules.
+
+  @param  Address The MMIO register to write.
+  @param  AndData The value to AND with the read value from the MMIO register.
+  @param  OrData  The value to OR with the result of the AND operation.
+
+  @return The value written back to the MMIO register.
+
+**/
+UINT8
+EFIAPI
+BeMmioAndThenOr8 (
+  IN  UINTN Address,
+  IN  UINT8 AndData,
+  IN  UINT8 OrData
+  );
+
+/**
+  MmioAndThenOr16 for Big-Endian modules.
+
+  @param  Address The MMIO register to write.
+  @param  AndData The value to AND with the read value from the MMIO register.
+  @param  OrData  The value to OR with the result of the AND operation.
+
+  @return The value written back to the MMIO register.
+
+**/
+UINT16
+EFIAPI
+BeMmioAndThenOr16 (
+  IN  UINTN Address,
+  IN  UINT16AndData,
+  IN  UINT16OrData
+  );
+
+/**
+  MmioAndThenOr32 for Big-Endian modules.
+
+  @param  Address The MMIO register to write.
+  @param  AndData The value to AND with the read value from the MMIO register.
+  @param  OrData  The value to OR with the result of the AND operation.
+
+  @return The value written back to the MMIO register.
+
+**/
+UINT32
+EFIAPI
+BeMmioAndThenOr32 (
+  IN  UINTN Address,
+  IN  UINT32AndData,
+  IN  UINT32OrData
+  );
+
+/**
+  MmioAndThenOr64 for Big-Endian modules.
+
+  @param  Address The MMIO register to write.
+  @param  AndData The value to AND with the read value from the MMIO register.
+  @param  OrData  The value to OR with the result of the AND operation.
+
+  @return The value written back to the MMIO register.
+
+**/
+UINT64
+EFIAPI
+BeMmioAndThenOr64 (
+  IN