Re: [PATCH edk2-platforms v2 18/18] ARM/JunoPkg: Add HDLCD platform library

[Alexei Fedorov <Alexei.Fedorov@arm.com>]

Hi Ard,


You wrote

"Well, the fact that you need to override the BaseMemoryLib
implementation is telling. This means the region is treated as memory
in the code, but mapped with device semantics. So this is not about
performance, but about the face that the UEFI spec stipulates that
unaligned access to memory are allowed on AArch64, and so if the
contents of these regions are dereferenced as memory, they should be
mapped as memory. If they are mapped as device, you should only use
MmioRead32/MmioWrite32 accessors."


In that case could you comment on the following commits:

"The BaseMemoryLib has switch to use BaseMemoryLibOptDxe at OPP, but the flash module is device attributes and have to be alignment accessed. so we change the flash related drivers to use generic BaseMemoryLib which is alignment access. "


Hisilicon/D02: flash related drivers switch to use generic BaseMemoryLib:
https://git.linaro.org/uefi/OpenPlatformPkg.git/commit/Platforms/Hisilicon/D02/Pv660D02.dsc?id=e3c520596d9ebdc525f284a9da95f080af815ec9


Hisilicon/D03: flash related drivers switch to use generic BaseMemoryLib:
https://git.linaro.org/uefi/OpenPlatformPkg.git/commit/Platforms/Hisilicon/D03/D03.dsc?id=337713801cceb684326bfde22975310ca1bd0cc0


Hisilicon/D05: flash related drivers switch to use generic BaseMemoryLib:
https://git.linaro.org/uefi/OpenPlatformPkg.git/commit/Platforms/Hisilicon/D05/D05.dsc?id=0749464022407f11c0c6a46cb8eb293fc74ef236


Alexei.


________________________________
From: edk2-devel <edk2-devel-bounces@lists.01.org> on behalf of Ard Biesheuvel <ard.biesheuvel@linaro.org>
Sent: 09 January 2018 18:26:57
To: Evan Lloyd
Cc: Matteo.Carlini@arm.com@arm.com; leif.lindholm@linaro.org@arm.com; nd@arm.com@arm.com; edk2-devel@lists.01.org; Arvind Chauhan; ard.biesheuvel@linaro.org@arm.com; Thomas Abraham
Subject: Re: [edk2] [PATCH edk2-platforms v2 18/18] ARM/JunoPkg: Add HDLCD platform library

On 9 January 2018 at 18:21, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
>
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: 23 December 2017 16:23
>> To: Evan Lloyd <Evan.Lloyd@arm.com>
>> Cc: edk2-devel@lists.01.org; Arvind Chauhan <Arvind.Chauhan@arm.com>;
>> Daniil Egranov <Daniil.Egranov@arm.com>; Thomas Abraham
>> <thomas.abraham@arm.com>; "ard.biesheuvel@linaro.org"@arm.com;
>> "leif.lindholm@linaro.org"@arm.com;
>> "Matteo.Carlini@arm.com"@arm.com; "nd@arm.com"@arm.com
>> Subject: Re: [PATCH edk2-platforms v2 18/18] ARM/JunoPkg: Add HDLCD
>> platform library
>>
>> On 22 December 2017 at 19:08,  <evan.lloyd@arm.com> wrote:
>> > From: Girish Pathak <girish.pathak@arm.com>
>> >
>> > This change adds the HDLCD platform lib for the Juno plaform. This
>> > library will be instantiated as a LcdPlatformLib to link with
>> > LcdGraphicsOutputDxe for the Juno platform.
>> >
>> > HDLCD platform library depends on the Arm SCMI DXE driver for
>> > communication with the SCP for clock setting. Therefore this change
>> > also enables building of Arm SCMI DXE driver for the Juno platform.
>> >
>> > Contributed-under: TianoCore Contribution Agreement 1.1
>> > Signed-off-by: Girish Pathak <girish.pathak@arm.com>
>>
>> Missing signoff?
> [[Evan Lloyd]] Yes.  Oops.
>
>>
>> > ---
>> >  Platform/ARM/JunoPkg/ArmJuno.dec                                 |   8 +
>> >  Platform/ARM/JunoPkg/ArmJuno.dsc                                 |  29 +
>> >  Platform/ARM/JunoPkg/ArmJuno.fdf                                 |  12 +-
>> >  Platform/ARM/JunoPkg/Library/ArmJunoLib/ArmJunoLib.inf           |   5 +-
>> >  Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJunoLib.inf
>> |  40 ++
>> >  Platform/ARM/JunoPkg/Library/ArmJunoLib/ArmJunoMem.c             |
>> 18 +-
>> >  Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJuno.c      |
>> 559 ++++++++++++++++++++
>> >  7 files changed, 668 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/Platform/ARM/JunoPkg/ArmJuno.dec
>> > b/Platform/ARM/JunoPkg/ArmJuno.dec
>> > index
>> >
>> b733480c3198d135df16ca024b5e85ff350e11c7..cd6710feb2faf0bd17b5ea
>> 39a21d
>> > be5406cd4ffd 100644
>> > --- a/Platform/ARM/JunoPkg/ArmJuno.dec
>> > +++ b/Platform/ARM/JunoPkg/ArmJuno.dec
>> > @@ -53,3 +53,11 @@ [PcdsFixedAtBuild.common]
>> >
>> gArmJunoTokenSpaceGuid.PcdArmMtlMailBoxBase|0x2E000000|UINT64|0
>> x00000025
>> >
>> gArmJunoTokenSpaceGuid.PcdArmMtlMailBoxSize|0x80|UINT32|0x00000
>> 026
>> >
>> > +  # MaxMode must be one number higher than the actual max mode,  #
>> > + i.e. for actual maximum mode 2, set the value to 3.
>> > +  #
>> > +  # Default value zero allows platform to enumerate maximum
>> supported mode.
>> > +  #
>> > +  # For a list of mode numbers look in HdLcdArmJuno.c
>> > +
>> gArmJunoTokenSpaceGuid.PcdArmHdLcdMaxMode|0|UINT32|0x0000001
>> 7
>> > +
>> > diff --git a/Platform/ARM/JunoPkg/ArmJuno.dsc
>> > b/Platform/ARM/JunoPkg/ArmJuno.dsc
>> > index
>> >
>> fe860956a4dc497cac52be70bab3657246a08bd0..9027c5b0728a6941f850
>> 636b3bc3
>> > 15fd33b867fb 100644
>> > --- a/Platform/ARM/JunoPkg/ArmJuno.dsc
>> > +++ b/Platform/ARM/JunoPkg/ArmJuno.dsc
>> > @@ -50,6 +50,11 @@ [LibraryClasses.common]
>> >    # SCMI Mailbox Transport Layer
>> >    ArmMtl|Platform/ARM/JunoPkg/Library/ArmMtl/ArmMtl.inf
>> >
>> > +!ifndef HEADLESS_PLATFORM
>>
>> Wouldn't it make more sense to add a macro ENABLE_HDLCD, rather than
>> inverting the logic?
>
>  [[Evan Lloyd]] We used this to correspond with the ACPI (FADT) terminology, where HEADLESS implies more than just no display (e.g. no keyboard or mouse).
> It would be possible to insert another level to define ENABLE_HDLCD, ENABLE_KEYBOARD, and ENABLE_MOUSE when  HEADLESS_PLATFORM is not defined, but I'm not convinced that would improve clarity.
> For mobile platforms (Juno, say) the default seems obvious, as a mobile without MMI is pretty pointless (unless you contemplate UEFI on IOT?).  For servers, less so, but the option is still available.  It also seems more natural to explicitly exclude support for hardware that is present, rather than defaulting to a crippled state.
>
>

Fair enough.

>>
>> > +
>> >
> ...
>> > +  # SCMI Driver
>> > +  ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf {
>> > +    <LibraryClasses>
>> > +
>> BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>>
>> I take it your trusted SRAM does not tolerate unaligned memcpy() because
>> it is mapped as device memory. Couldn't you map it as non-cacheable
>> memory instead? (I meant to ask in response to the other patch but I forgot)
>>
>
>  [[Evan Lloyd]]  As this is generic code we can't know what the platform SCP interface memory constraints might be, so we've gone for the safest option. (It might be fine for Juno, but future stuff is unknown.)  As far as I can tell, the only advantage of non-cacheable would be to permit unaligned access, which might give a small performance improvement.  My current guess is that the caller's protocol message structures are likely to be properly aligned anyway, so the benefit might be negligible.  The drawback would be that you need to add barriers.
> The bottom line is, we'll change it if you see a significant gain, but there is a cost to re-jig and test.
>

Well, the fact that you need to override the BaseMemoryLib
implementation is telling. This means the region is treated as memory
in the code, but mapped with device semantics. So this is not about
performance, but about the face that the UEFI spec stipulates that
unaligned access to memory are allowed on AArch64, and so if the
contents of these regions are dereferenced as memory, they should be
mapped as memory. If they are mapped as device, you should only use
MmioRead32/MmioWrite32 accessors. Your call.

>>
>> > +  }
>> > +
> ...
>> afb2db0050c65b0d1b2b69c9038e168755c152c1..baa5221cb906ed5d0774
>> 14475da0
>> > 06cf2e5cafc5 100644
>> > --- a/Platform/ARM/JunoPkg/Library/ArmJunoLib/ArmJunoMem.c
>> > +++ b/Platform/ARM/JunoPkg/Library/ArmJunoLib/ArmJunoMem.c
>> > @@ -21,8 +21,10 @@
>> >
> ...
>> >
>> > +  // Frame Buffer Memory
>> > +#if (FixedPcdGet32 (PcdArmLcdDdrFrameBufferSize) != 0)
>>
>> Please use a normal if()
> [[Evan Lloyd]] Will do
>>
> ...
>> > diff --git
>> > a/Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJuno.c
>> > b/Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJuno.c
>> > new file mode 100644
>> > index
>> >
>> 0000000000000000000000000000000000000000..72be0a39846fb0a78e
>> bcf3248b6c
>> > 51377adf4f73
>> > --- /dev/null
>> > +++
>> b/Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJuno.c
>> > @@ -0,0 +1,559 @@
> ...
>> > +
>> > +    ASSERT (PixelFormat == PixelRedGreenBlueReserved8BitPerColor
>> > +      ||  PixelFormat == PixelBlueGreenRedReserved8BitPerColor);
>>
>> Please fix weird indentation
> [[Evan Lloyd]] Will do
>>
>> > +   return EFI_UNSUPPORTED;
> ...
>> > +
>> > +  // Check if memory is already reserved for the frame buffer.
>> > +#if (FixedPcdGet64 (PcdArmLcdDdrFrameBufferBase) != 0)
>>
>> Please don't use CPP conditionals for control flow
> [[Evan Lloyd]] Will do
>>
> ...
>> > --
>> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>> >
>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.