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

[Evan Lloyd <Evan.Lloyd@arm.com>]

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


>
> > +
> >
...
> > +  # 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.

>
> > +  }
> > +
...
> 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.