From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::22e; helo=mail-it0-x22e.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x22e.google.com (mail-it0-x22e.google.com [IPv6:2607:f8b0:4001:c0b::22e]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id AC7C52220D1EA for ; Tue, 9 Jan 2018 10:21:47 -0800 (PST) Received: by mail-it0-x22e.google.com with SMTP id d137so13036214itc.2 for ; Tue, 09 Jan 2018 10:26:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=jueb9iGDWCb0kFAZCB8nZdlE124K8g3Kl+5pH7hXmVE=; b=jUY/1i+e3r1aJzplB3Y3XrLyIAuwYx6l1qvaG4CkkX3VimK3gIvOr+KCiYkQZknX4c bp5srAXoIYHCcONJ2BF7xIJSYi2uHQrX6lcjMGBKa4PSh1wAYotlYDG0eUnwcCm0lSYB I8oxkGeFCH9FEYxCFWXA2EBOy8GoAqmg+i+x8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=jueb9iGDWCb0kFAZCB8nZdlE124K8g3Kl+5pH7hXmVE=; b=If6i/mp5kE3uxgkzg/sB0EfLlpuCvgL4dyCWlA2s3mdVH7lgrsnanr5ieqUqCtOqpZ SJXGFswby+8MmNUesT8v40eaUZme7aPeq+F+feoO/fGQ7CUwF/+fulit6Tqh2yeQZltw pDk9+F+LIInqIWE3X4nQlbbCtdzBh3ogNrZITfbMlHAdbpqHub3ERYzHW9hmR1UakTes NnvPxDn/UAbQO82/xsUgxCHHWXniJsOFyoNpVrr9KBz7gElIIjZDBNGgx3iAIWurXp5J NP1CchVU82++Xu0pc9WfLKQ+SQAkZhS7VwmexDhqN/pRQTKokwrritb6b69SmDsDXv+y /40w== X-Gm-Message-State: AKwxytccPVxiSqFJCAIRjrDx0QjV11eKQrzvMK1My+RylN77XA9qg2cW +jv3rnwTurqCDdQZgGg7DkAJ0jr164op45X2Tn8F5Q== X-Google-Smtp-Source: ACJfBoviyJkKP3SdD1HDe5CD8t8NChoXiDttMR2+2lXWVaDSA42cqze81rEpw2XvReKYAgKNrdVAZ9TFrcgu6kfrWRI= X-Received: by 10.36.185.22 with SMTP id w22mr6160177ite.58.1515522417672; Tue, 09 Jan 2018 10:26:57 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.37.197 with HTTP; Tue, 9 Jan 2018 10:26:57 -0800 (PST) In-Reply-To: References: <20171222190821.12440-1-evan.lloyd@arm.com> <20171222190821.12440-19-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Tue, 9 Jan 2018 18:26:57 +0000 Message-ID: To: Evan Lloyd Cc: "edk2-devel@lists.01.org" , Arvind Chauhan , Daniil Egranov , Thomas Abraham , "ard.biesheuvel@linaro.org@arm.com" <"ard.biesheuvel@linaro.org"@arm.com>, "leif.lindholm@linaro.org@arm.com" <"leif.lindholm@linaro.org"@arm.com>, "Matteo.Carlini@arm.com@arm.com" <"Matteo.Carlini@arm.com"@arm.com>, "nd@arm.com@arm.com" <"nd@arm.com"@arm.com> Subject: Re: [PATCH edk2-platforms v2 18/18] ARM/JunoPkg: Add HDLCD platform library X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Jan 2018 18:21:48 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On 9 January 2018 at 18:21, Evan Lloyd wrote: > > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: 23 December 2017 16:23 >> To: Evan Lloyd >> Cc: edk2-devel@lists.01.org; Arvind Chauhan ; >> Daniil Egranov ; Thomas Abraham >> ; "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, wrote: >> > From: Girish Pathak >> > >> > 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 >> >> Missing signoff? > [[Evan Lloyd]] Yes. Oops. > >> >> > --- >> > Platform/ARM/JunoPkg/ArmJuno.dec | = 8 + >> > Platform/ARM/JunoPkg/ArmJuno.dsc | 2= 9 + >> > Platform/ARM/JunoPkg/ArmJuno.fdf | 1= 2 +- >> > 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) terminolo= gy, where HEADLESS implies more than just no display (e.g. no keyboard or m= ouse). > It would be possible to insert another level to define ENABLE_HDLCD, ENAB= LE_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 w= ithout MMI is pretty pointless (unless you contemplate UEFI on IOT?). For = servers, less so, but the option is still available. It also seems more na= tural to explicitly exclude support for hardware that is present, rather th= an defaulting to a crippled state. > > Fair enough. >> >> > + >> > > ... >> > + # SCMI Driver >> > + ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf { >> > + >> > + >> 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 for= got) >> > > [[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 opt= ion. (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 gues= s is that the caller's protocol message structures are likely to be properl= y aligned anyway, so the benefit might be negligible. The drawback would b= e that you need to add barriers. > The bottom line is, we'll change it if you see a significant gain, but th= ere 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) !=3D 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 =3D=3D PixelRedGreenBlueReserved8BitPerColor >> > + || PixelFormat =3D=3D 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) !=3D 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 conf= idential and may also be privileged. If you are not the intended recipient,= please notify the sender immediately and do not disclose the contents to a= ny other person, use it for any purpose, or store or copy the information i= n any medium. Thank you.