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

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

Hi Laszlo


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, February 23, 2018 4:29 PM
> To: Udit Kumar <udit.kumar@nxp.com>; Pankaj Bansal
> <pankaj.bansal@nxp.com>; Leif Lindholm <leif.lindholm@linaro.org>
> Cc: michael.d.kinney@intel.com; edk2-devel@lists.01.org;
> ard.biesheuvel@linaro.org; Meenakshi Aggarwal
> <meenakshi.aggarwal@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-bounces@lists.01.org] On Behalf Of
> >> Laszlo Ersek
> >> Sent: Friday, February 23, 2018 2:51 PM
> >> To: Pankaj Bansal <pankaj.bansal@nxp.com>; Leif Lindholm
> >> <leif.lindholm@linaro.org>
> >> Cc: michael.d.kinney@intel.com; edk2-devel@lists.01.org;
> >> ard.biesheuvel@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-bounces@lists.01.org] On Behalf
> Of
> >>>> Laszlo Ersek
> >>>> Sent: Thursday, February 22, 2018 7:26 PM
> >>>> To: Leif Lindholm <leif.lindholm@linaro.org>
> >>>> Cc: michael.d.kinney@intel.com; edk2-devel@lists.01.org;
> >>>> ard.biesheuvel@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 *)&mOne == 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.
> >>
> >> This can work too, but there is a downside: a large number of IoLib
> >> instances exist in the tree already. If you add the BeMmio* functions to
> >> the existent IoLib class, you'll have to duplicate the implementation to
> >> all instances (identically, I think).
> >>
> >> We've had this debate in the past. Back then it was about IoFifo
> >> routines. I argued for an IoFifo lib class. Ultimately the IoFifo
> >> routines were added to IoLib, and they had to be implemented for many
> >> more library instances than client code would have actually required.
> >> (See the series at 13a50a6fe1dc..2b631390f9f5.) In turn this runs the
> >> risk of adding untested code.
> >>
> >> Regarding the instances for BE CPUs: the name should likely be
> >> BaseIoLibBigEndian or something similar. In lib instance names, the lib
> >> class name is usually prefixed with the firmware phases where the
> >> instance is usable, and hints about the implementation or constraints
> >> are added as a suffix.
> >
> > I see like below
> > CPU 			IP		Call			Lib
> > LE			LE		MMIO       		BaseIoLib
> > LE			BE		SwappedMMIO		BaseIoLibEx
> > BE			BE		MMIO			BaseIoLib
> > BE			LE 		SwappedMMIO
> BaseIoLibEx
> 
> In my opinion, this is wrong. The "Call" column should not vary with CPU
> endianness, it should only vary with "IP" (device) endianness.
> 
> Basically you are putting the entire work on the driver code, to figure
> out whether swapping will be necessary or not. For that, the driver has
> to consult the byte order of *both* the CPU *and* the device, and then
> call "transparent" or "swapped" MMIO functions.
>
> What I am suggesting is that the driver care about device byte order
> only. This will determine whether the driver calls MmioWrite32() or
> BeMmioWrite32().

> In turn, whether or not those functions byte-swap, according to CPU byte
> order, is an internal matter of the libraries.
> 
>   IP  Call           Lib      CPU  Byte-swap within Lib
>   --  -------------  -------  ---  --------------------
>   LE  MmioWrite32    IoLib    LE   no
>   LE  MmioWrite32    IoLib    BE   yes
>   BE  BeMmioWrite32  BeIoLib  LE   yes
>   BE  BeMmioWrite32  BeIoLib  BE   no
> 
> Obviously, the end result is the same; the question is what *concepts*
> the device driver has to care about.

> Under your scheme, every device driver has to care about concepts such
> as "device byte order", "cpu byte order", and "swapping".
> 
> Under my scheme, every device driver has to care about "device byte order".

Thanks for detailing .
May be I was not clear in previous email by saying 

>> I am calling BaseIoLibEx for extend the feature of CPU endianness In this case SwappedMMIO could be MmioToBe or MmioToLe Let this this new 
>> lib BaseIoLibEx decide endianness of CPU based upon Pcd, hob list etc and do conversion if needed.

I think, more or less we are saying same thing.

I am saying for new development will use MmioToBe or MmioToLe based upon IP mode, 
Whereas you are referring as MmioWrite32 and BeMmioWrite32 to be used. 

The difference what I see, between your proposal and mine
You are saying , to do swapping in MmioWrite32 based upon CPU mode.
Whereas I am referring to new APIs. 

Anyway , I am ok with your proposal , which looks more inline w.r.t current edk-2 code 

Thanks
Udit 

> Laszlo