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 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
