Re: [PATCH 1/1] MdePkg/IoLib: Add support for big-endian MMIO

[Laszlo Ersek <lersek@redhat.com>]

On 10/14/16 15:17, Bhupesh Sharma wrote:
> Hi Laszlo,
> 
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Friday, October 14, 2016 5:37 PM
>>
>> On 10/14/16 11:33, Bhupesh Sharma wrote:
>>> Various IPs on NXP/FSL SoCs having ARM64 cores have big-endian MMIO
>>> interfaces.
>>>
>>> This implies that a byte-swap operation is needed to read/write such
>>> BE MMIO registers from the LE ARM64 cores.
>>>
>>> This patch adds the support for the same.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@nxp.com>
>>> ---
>>>  MdePkg/Include/Library/IoLib.h               | 364
>> ++++++++++++++++++++
>>>  MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c | 479
>>> +++++++++++++++++++++++++++
>>>  2 files changed, 843 insertions(+)
>>
>> I think this is both overkill and incomplete, at the same time :)
>>
>> - Incomplete because only one IoLib instance gets the implementation.
> 
> Agree, but do we have an example of similar BE MMIO IPs on other
> Architectures/soc? I was only aware of NXP/FSL having such MMIO
> interfaces as the IPs have been reused from PPC SoC, which used
> to have a BE core and hence did not require a SwapByte.

For example, the fw_cfg device of QEMU is big endian. OvmfPkg and
ArmVirtPkg call SwapBytesXX as necessary, in combination with MmioWriteXX.

In general, I think it's expected that a library declared under MdePkg
will not cause build failures (unresolved symbols) in any platform code,
once the library is resolved correctly in the platform DSC.

>  
>> - Overkill because you can easily use the SwapBytes16, SwapBytes32,
>> SwapBytes64 functions -- also from BaseLib --, for transforming
>> MmioWrite arguments and MmioRead results.
>>
> 
> Yes, but that means at every IP driver needs to especially carry such
> arguments and transform the results.

Indeed.

> That might be an overkill.

I think it should be possible to factor out these functions to a
separate library, or non-standard protocol, that is central enough for
the platform or device in question, but not core enough for putting into
MdePkg.

> We already have similar implementations MMIO implementations in Linux for e.g.
> http://lxr.free-electrons.com/source/include/asm-generic/io.h#L642

Hmmm. That does detract from the value of my "overkill" argument. So I
guess I'll have to defer to the MdePkg maintainers on that.

Regarding my "incomplete" argument, it still stands. I think it's
logically impossible to introduce a library class that is simultaneously
- central enough to merit a place under MdePkg, but
- not central enough to receive implementations (library instances) for
all the platforms supported by edk2 at the moment.

Thanks
Laszlo