> -----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 *)&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

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. 

Thanks
Udit 

> Also, if you want to support BE CPUs with separate IoLib instances, I'm
> afraid that's going to lead to the combinatorial explosion that Leif
> characterized as "burst[ing] an aneurysm". I think using the (seemingly
> dynamic) "mOne" approach I suggested above, a smart compiler can
> eliminate all the branches at build time.
> 
> Anyway, I don't insist; I just commented on an RFC.
> 
> 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&data=02%7C01%7Cudit.kumar%40nxp.com%7C932b04d97a2648845d9
> 208d57a9ece38%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6365
> 49744881765464&sdata=wHi1Jd5Ar3AlM9sIWI9osdHR%2FF3%2Bx%2BrDEK7U
> AqECXDY%3D&reserved=0
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to