From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.3543.1588761100677763769 for ; Wed, 06 May 2020 03:31:40 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 565F730E; Wed, 6 May 2020 03:31:40 -0700 (PDT) Received: from [192.168.1.81] (unknown [10.37.8.179]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 184643F71F; Wed, 6 May 2020 03:31:38 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH edk2-platforms 4/5] Platform/RaspberryPi3: query firmware for 16550 input clock at boot To: Pete Batard , devel@edk2.groups.io Cc: leif@nuviainc.com, andrey.warkentin@gmail.com References: <20200505145029.29826-1-ard.biesheuvel@arm.com> <20200505145029.29826-5-ard.biesheuvel@arm.com> <02940c75-5a92-9fca-449a-7a90ccfc8362@arm.com> <2144540f-1b10-de24-95fa-09a0b2601cbb@akeo.ie> From: "Ard Biesheuvel" Message-ID: <4f8a19c3-80d9-ad1a-4915-69fe13182197@arm.com> Date: Wed, 6 May 2020 12:31:36 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <2144540f-1b10-de24-95fa-09a0b2601cbb@akeo.ie> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 5/6/20 12:18 PM, Pete Batard wrote: > One remark below: >=20 > On 2020.05.05 19:11, Ard Biesheuvel wrote: >> On 5/5/20 4:50 PM, Ard Biesheuvel wrote: >>> Query the firmware for the clock rate that is used to drive the >>> 16550 baud clock, so that we can program the correct baud rate. >>> >>> Co-authored-by: Pete Batard >>> Co-authored-by: Andrei Warkentin >>> Co-authored-by: Ard Biesheuvel >>> Signed-off-by: Pete Batard >>> Signed-off-by: Ard Biesheuvel >>> --- >>> =20 >>> Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S= =20 >>> | 25 +++++++++++++++++++- >>> =C2=A0 1 file changed, 24 insertions(+), 1 deletion(-) >>> >>> diff --git=20 >>> a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S= b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S=20 >>> >>> index 91dfe1bb981e..35580e4ed73a 100644 >>> ---=20 >>> a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S >>> +++=20 >>> b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S >>> @@ -3,7 +3,7 @@ >>> =C2=A0=C2=A0 *=C2=A0 Copyright (c) 2020, Andrei Warkentin >>> =C2=A0=C2=A0 *=C2=A0 Copyright (c) 2019-2020, Pete Batard >>> =C2=A0=C2=A0 *=C2=A0 Copyright (c) 2016, Linaro Limited. All rights re= served. >>> - *=C2=A0 Copyright (c) 2011-2013, ARM Limited. All rights reserved. >>> + *=C2=A0 Copyright (c) 2011-2020, ARM Limited. All rights reserved. >>> =C2=A0=C2=A0 * >>> =C2=A0=C2=A0 *=C2=A0 SPDX-License-Identifier: BSD-2-Clause-Patent >>> =C2=A0=C2=A0 * >>> @@ -85,6 +85,14 @@ ASM_FUNC (ArmPlatformPeiBootAction) >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 adr=C2=A0=C2=A0=C2=A0=C2=A0 x2, mBoardR= evision >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 str=C2=A0=C2=A0=C2=A0=C2=A0 w0, [x2] >>> +#if (RPI_MODEL =3D=3D 3) >> >> As noted by Pete off-list, doing this doesn't work unless we add=20 >> something like >> >> GCC:*_*_*_PP_FLAGS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 =3D -DRPI_MODEL=3D3 >> >> to the [BuildOptions] in RPi3.dsc >> >> >> >>> +=C2=A0=C2=A0=C2=A0 run=C2=A0=C2=A0=C2=A0=C2=A0 .Lclkinfo_buffer >>> + >>> +=C2=A0=C2=A0=C2=A0 ldr=C2=A0=C2=A0=C2=A0=C2=A0 w0, .Lfrequency >>> +=C2=A0=C2=A0=C2=A0 adrp=C2=A0=C2=A0=C2=A0 x2, _gPcd_BinaryPatch_PcdSe= rialClockRate >>> +=C2=A0=C2=A0=C2=A0 str=C2=A0=C2=A0=C2=A0=C2=A0 w0, [x2, :lo12:_gPcd_B= inaryPatch_PcdSerialClockRate] >>> +#endif >>> + >=20 > Since we're modifying a patchable PCD here, shouldn't we add a: >=20 > [PatchPcd] > =C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate >=20 > section in PlatformLib.inf? >=20 We should add it, but we can add it to the [Pcd] section. Alternatively, we could have different .INFs for RPi3 and RPi4, but that= =20 is really overkill IMO. Making it patchable on both platforms just to patch it on one is also=20 unnecessary I think. The current approach will ensure that we catch any=20 issues at build time, without any major hacks,]. > Of course, if we do that, we can't keep PcdSerialClockRate fixed for=20 > RPi4, as the build process will complain about PCD mismatch. >=20 > I also wouldn't mind a comment that explains how one arrives at figuring= = =20 > out that "_gPcd_BinaryPatch_PcdSerialClockRate" should be used to locate= = =20 > our address (and possibly the addition of :lo12:), because I don't think= = =20 > it's going to be that straightforward for people reading the code for=20 > the first time, though I fear that the explanation will boil down to "we= = =20 > need to do it this specific way for a gcc aarch64 relocation"... >=20 We don't actually need the adrp/str pair with the lo12 here, I will=20 replace it with adr. (Just muscle memory) >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .align=C2=A0 4 >>> @@ -127,6 +135,21 @@ ASM_FUNC (ArmPlatformPeiBootAction) >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .long=C2=A0=C2=A0 0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // end = tag >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .set=C2=A0=C2=A0=C2=A0 .Lrevinfo_size, = . - .Lrevinfo_buffer >>> +#if (RPI_MODEL =3D=3D 3) >>> +=C2=A0=C2=A0=C2=A0 .align=C2=A0 4 >>> +.Lclkinfo_buffer: >>> +=C2=A0=C2=A0=C2=A0 .long=C2=A0=C2=A0 .Lclkinfo_size >>> +=C2=A0=C2=A0=C2=A0 .long=C2=A0=C2=A0 0x0 >>> +=C2=A0=C2=A0=C2=A0 .long=C2=A0=C2=A0 RPI_MBOX_GET_CLOCK_RATE >>> +=C2=A0=C2=A0=C2=A0 .long=C2=A0=C2=A0 8=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // buf size >>> +=C2=A0=C2=A0=C2=A0 .long=C2=A0=C2=A0 4=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // input len >>> +=C2=A0=C2=A0=C2=A0 .long=C2=A0=C2=A0 4=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // clock id: 0x04 = =3D Core/VPU >>> +.Lfrequency: >>> +=C2=A0=C2=A0=C2=A0 .long=C2=A0=C2=A0 0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // frequency >>> +=C2=A0=C2=A0=C2=A0 .long=C2=A0=C2=A0 0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // end tag >>> +=C2=A0=C2=A0=C2=A0 .set=C2=A0=C2=A0=C2=A0 .Lclkinfo_size, . - .Lclkin= fo_buffer >>> +#endif >>> + >>> =C2=A0 //UINTN >>> =C2=A0 //ArmPlatformGetPrimaryCoreMpId ( >>> =C2=A0 //=C2=A0 VOID >>> >> >> >>=20 >> >=20 > With addition of "GCC:*_*_*_PP_FLAGS =3D -DRPI_MODEL=3D3" in the .dsc: > Reviewed-by: Pete Batard >=20 Thanks