From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by mx.groups.io with SMTP id smtpd.web11.3701.1588761483250390857 for ; Wed, 06 May 2020 03:38:03 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=IsEH+z0c; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.221.68, mailfrom: pete@akeo.ie) Received: by mail-wr1-f68.google.com with SMTP id j5so1595323wrq.2 for ; Wed, 06 May 2020 03:38:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=QfsuGn6btsjeQIQHREV4HVDTYfbDNjcGBiFWYG0a/P0=; b=IsEH+z0cYvSjPGjTT9kYybFEOKR66BECedQiAdnGOSeh4SKk0qmcxdf6RhtcAy5wZs LYbOWOY/HmOPJsUXm3MA+jiuFBctC9d/JJFDxeQdVRPtbAjn/1Gc0JZvYtNWmDu6D3c4 xa4tcYhEa+fLA2yN2nr6YOjFNkU0zMQzLTGqBfclIx0htAW37oCt7nvZ6MMJqqvz1H1T EdfWhNqM6NWgaqR6KldQRDsTCv659nfsImC0HWebBPgRrRvnmsbByevvO5k9WdVcdnTf ETmpQ2rI9urop9FBUAulEqbMxxqY7S3DsCnW2kpgKb5JdO3gxlOmMtREuwl1n+KkV7B4 wGKg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=QfsuGn6btsjeQIQHREV4HVDTYfbDNjcGBiFWYG0a/P0=; b=HHEvLeQ6Uk3HbveUDUm2LXOJie5Wwz86YBchVEJJgnC/czFGlMtfi49gvAgSqVaAGg yxVpnzpimN1hW/xRxPE6SQRK6QwyrCAU5WfINinA1F5T1il3EZ/GejoPQSVpNfvfHExD ZdhgSGhfuAvxVjv5MAVtaoNhsr3+K9Ct09zGVQc3JHS8f+aNogq126Xt0COfQAWMdMYa bjse2TI8O+L4tuIGd+CHcYDorWy4DNz2dv3Zvq8jxGZHdqkoOrJukTmQ7WOiPV8nNPcH EYlnWoo0WQuhWNAOo/T3TLxRW+1v2XlTS8BPfPi7fHGfWK7RxTvnVaBl6px9lH9j9PaD DnDA== X-Gm-Message-State: AGi0Pube+8mpWUK3JyGfQbMvk610JTHD7Yb5Sp/3WDOucqXWitPZ8CJM vv4wUBFLQcs0Fg1J4tZL8cWBrA== X-Google-Smtp-Source: APiQypIUsLgpaUsxxzNsXDxTwgMJy+Gvwt2Rw2LTGz0ee61sN7v/cZSjB24QhuqLKSt3FRud2j31Mg== X-Received: by 2002:adf:ee03:: with SMTP id y3mr8346303wrn.190.1588761481714; Wed, 06 May 2020 03:38:01 -0700 (PDT) Return-Path: Received: from [10.0.0.122] ([84.203.75.87]) by smtp.googlemail.com with ESMTPSA id c19sm2113420wrb.89.2020.05.06.03.38.00 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 06 May 2020 03:38:01 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH edk2-platforms 4/5] Platform/RaspberryPi3: query firmware for 16550 input clock at boot To: Ard Biesheuvel , 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> <4f8a19c3-80d9-ad1a-4915-69fe13182197@arm.com> From: "Pete Batard" Message-ID: <0a89bbc4-576e-55fe-b4a3-97a0644b174d@akeo.ie> Date: Wed, 6 May 2020 11:38:00 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <4f8a19c3-80d9-ad1a-4915-69fe13182197@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 8bit On 2020.05.06 11:31, Ard Biesheuvel wrote: > On 5/6/20 12:18 PM, Pete Batard wrote: >> One remark below: >> >> 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 >>>> --- >>>> Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S >>>> | 25 +++++++++++++++++++- >>>>   1 file changed, 24 insertions(+), 1 deletion(-) >>>> >>>> diff --git >>>> a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S >>>> b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S >>>> index 91dfe1bb981e..35580e4ed73a 100644 >>>> --- >>>> a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S >>>> +++ >>>> b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S >>>> @@ -3,7 +3,7 @@ >>>>    *  Copyright (c) 2020, Andrei Warkentin >>>>    *  Copyright (c) 2019-2020, Pete Batard >>>>    *  Copyright (c) 2016, Linaro Limited. All rights reserved. >>>> - *  Copyright (c) 2011-2013, ARM Limited. All rights reserved. >>>> + *  Copyright (c) 2011-2020, ARM Limited. All rights reserved. >>>>    * >>>>    *  SPDX-License-Identifier: BSD-2-Clause-Patent >>>>    * >>>> @@ -85,6 +85,14 @@ ASM_FUNC (ArmPlatformPeiBootAction) >>>>       adr     x2, mBoardRevision >>>>       str     w0, [x2] >>>> +#if (RPI_MODEL == 3) >>> >>> As noted by Pete off-list, doing this doesn't work unless we add >>> something like >>> >>> GCC:*_*_*_PP_FLAGS          = -DRPI_MODEL=3 >>> >>> to the [BuildOptions] in RPi3.dsc >>> >>> >>> >>>> +    run     .Lclkinfo_buffer >>>> + >>>> +    ldr     w0, .Lfrequency >>>> +    adrp    x2, _gPcd_BinaryPatch_PcdSerialClockRate >>>> +    str     w0, [x2, :lo12:_gPcd_BinaryPatch_PcdSerialClockRate] >>>> +#endif >>>> + >> >> Since we're modifying a patchable PCD here, shouldn't we add a: >> >> [PatchPcd] >>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate >> >> section in PlatformLib.inf? >> > > 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 > is really overkill IMO. > > Making it patchable on both platforms just to patch it on one is also > unnecessary I think. The current approach will ensure that we catch any > issues at build time, without any major hacks,]. Yeah, I'm also fine with what we have at the moment, and I sure don't want separate .INFs for Pi3 and Pi4. > >> Of course, if we do that, we can't keep PcdSerialClockRate fixed for >> RPi4, as the build process will complain about PCD mismatch. >> >> I also wouldn't mind a comment that explains how one arrives at >> figuring out that "_gPcd_BinaryPatch_PcdSerialClockRate" should be >> used to locate our address (and possibly the addition of :lo12:), >> because I don't think it's going to be that straightforward for people >> reading the code for the first time, though I fear that the >> explanation will boil down to "we need to do it this specific way for >> a gcc aarch64 relocation"... >> > > We don't actually need the adrp/str pair with the lo12 here, I will > replace it with adr. (Just muscle memory) Sounds good, thanks! /Pete > > > >>>>       ret >>>>       .align  4 >>>> @@ -127,6 +135,21 @@ ASM_FUNC (ArmPlatformPeiBootAction) >>>>       .long   0                           // end tag >>>>       .set    .Lrevinfo_size, . - .Lrevinfo_buffer >>>> +#if (RPI_MODEL == 3) >>>> +    .align  4 >>>> +.Lclkinfo_buffer: >>>> +    .long   .Lclkinfo_size >>>> +    .long   0x0 >>>> +    .long   RPI_MBOX_GET_CLOCK_RATE >>>> +    .long   8                           // buf size >>>> +    .long   4                           // input len >>>> +    .long   4                           // clock id: 0x04 = Core/VPU >>>> +.Lfrequency: >>>> +    .long   0                           // frequency >>>> +    .long   0                           // end tag >>>> +    .set    .Lclkinfo_size, . - .Lclkinfo_buffer >>>> +#endif >>>> + >>>>   //UINTN >>>>   //ArmPlatformGetPrimaryCoreMpId ( >>>>   //  VOID >>>> >>> >>> >>> >>> >> >> With addition of "GCC:*_*_*_PP_FLAGS = -DRPI_MODEL=3" in the .dsc: >> Reviewed-by: Pete Batard >> > > > Thanks >