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

[Leif Lindholm <leif.lindholm@linaro.org>]

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 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?

> - Have some function pointers (global variables, or members of a global
> structure) that point to either IoLib or BeIoLib functions, dependent on
> the detected device byte order. A module can be linked against both
> IoLib and BeIoLib.
> 
> - Use those function pointers to talk to the device.

Yes, that does sound like the best solution for the code in the driver
regardless.

/
    Leif