From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f51.google.com (mail-wr1-f51.google.com [209.85.221.51]) by mx.groups.io with SMTP id smtpd.web10.30742.1617460539887109120 for ; Sat, 03 Apr 2021 07:35:40 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=0gxgSHCG; spf=pass (domain: akeo.ie, ip: 209.85.221.51, mailfrom: pete@akeo.ie) Received: by mail-wr1-f51.google.com with SMTP id e18so7006785wrt.6 for ; Sat, 03 Apr 2021 07:35:39 -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=gUuwTl5tjGJWc2ToWk2Tbk3lPLSi6I7iaWBzDJO+uso=; b=0gxgSHCGSNFsJJYcuBFE9ngHYjKFwT+tvn/G6t/fUYZVilIUfX61BAgRnIpJiP1NiF riUvcYN11e40Amb5U0VbLM/OoX72DP9sXLKmprJRtJ6DD9g6owI7RMTk//5JfQdkX5DO dCupvEZmZ6O31ttzXyJcptM5lYyB8jkY5uRkiYTjGSmY4+JgXD1VdtUBjYAPEBoU+snZ lwh/3XAmKs4wlDphtyAZAl0VMzUZQroY7IU46P0JHv+Y/B2ajBbE+ps2KAp9JIQoXkeE Vcu2eZuEa245yvbQkJaft73ikh/T3crWosB80pIUKXjJpYl7+lSCmp68Mxf27qhOJTrt Y9Fg== 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=gUuwTl5tjGJWc2ToWk2Tbk3lPLSi6I7iaWBzDJO+uso=; b=KLxJZsGODv+/aWM8CHAZ8k6KXd34vLDRvHUJGgymE1ulRl/mB7RaGCwYajH9ezP3F+ mrwb2bgKA5zbIVw96WA3fReZuwSX6QcxHakWQLO++5UT1iT8mRCGPcHslQlqoaXbhwnp zHPdz9oxOh4rhViG4IJ1N+p0AlDjV3qYV+S6Z/nj21H1bcRHLKiSsVfdAZg7jZqitBEA JanlIeO39Vj3Jlspux8X7LAsBRWm1QAj3zCHsvaHptSxzAjQjOdyyu14TDtss8izB93H 1ZD8/MBzP8xAo63jGDFbkcscbjEqf1WXj88xCMKABEC8KUbATWVCWK2MnDb9Wvbsm2MN UUzQ== X-Gm-Message-State: AOAM5309lG+MgVK20ePmbKMZ8dc+FKnYaBvsV4hjXYCCaEbJhy3jYHOM 423D+sIHJNl01P/av/yeOb1nXw== X-Google-Smtp-Source: ABdhPJzg4A8XAYfpBZcUdoe3ht4A0Oh7t3lYX26uWgM2LBsFwL37j+eAdiYMFKTVcIc6/Monti2e8Q== X-Received: by 2002:a5d:47c4:: with SMTP id o4mr20235836wrc.138.1617460538340; Sat, 03 Apr 2021 07:35:38 -0700 (PDT) Return-Path: Received: from [10.0.0.122] ([84.203.60.64]) by smtp.googlemail.com with ESMTPSA id 1sm42371392wmj.0.2021.04.03.07.35.37 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 03 Apr 2021 07:35:37 -0700 (PDT) Subject: Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Fix mini UART baud divisor calculation To: =?UTF-8?B?TWFyaW8gQsSDbMSDbmljxIM=?= , Ard Biesheuvel Cc: devel@edk2.groups.io, Ard Biesheuvel , Leif Lindholm References: <20210402175148.1485-1-mariobalanica02@gmail.com> From: "Pete Batard" Message-ID: <6ebf4605-b011-a430-968b-d0f85cf91769@akeo.ie> Date: Sat, 3 Apr 2021 15:35:36 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 8bit Hi Mario, On 2021.04.03 13:47, Mario Bălănică wrote: > It works with the old firmware as well. Are you sure of this? You may have to go quite far back to find start4.elf versions where this might not work... The whole reason behind reading the base clock and divisor from MMIO, rather than use a fixed base clock, is actually because we got stung with the Raspberry Pi Foundation changing start4.elf on a whim, and thereby breaking the serial divisor computation. So we moved away from using the approach proposed in this patch (i.e. assuming a fixed PcdSerialClockRate) as we found that it didn't work for all cases. I'm also wondering why there are no mentions of clock rates in the firmware commit message you linked to. Do you have other information, from the Raspberry Pi Foundation developers, where they indicate that they plan to keep the base clock for miniUART fixed to 500 MHz from now on? But more importantly, I'd like to understand the issue we are trying to solve with this patch. Does the current serial computation create issues? Because, if that is not the case, I think I'd rather keep the dynamic approach of reading the base clock and VPU divisor, since it might be more future-proof than relying on a fixed value provided in the .dsc. Especially, we don't know if the Raspberry Pi Foundation may not introduce a new Rapsberry Pi 4 revision tomorrow (like they did with the Pi400), where the fixed 500 MHz base clock rate would not apply... So I'd like to understand better how this patch came about, and what it is we are trying to achieve with it. Regards, /Pete > > sâm., 3 apr. 2021, 10:36 Ard Biesheuvel > a scris: > > On Fri, 2 Apr 2021 at 19:52, Mario Bălănică > > wrote: > > > > The baud rate divisor calculation for mini UART on BCM2711 is the > same > > as on older models since this commit: > > https://github.com/raspberrypi/firmware/commit/1e5456a > > > > > Signed-off-by: Mario Bălănică > > > Doesn't this make the new build incompatible with the old firmware? Is > there a way to support both? > > > > --- > > > Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c >  | 15 +++------------ > > > Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S > |  4 ---- > >  Platform/RaspberryPi/RPi4/RPi4.dsc >      |  6 ++++-- > >  3 files changed, 7 insertions(+), 18 deletions(-) > > > > diff --git > a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c > b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c > > index 5e83bbf022eb..d2f983bf0a9f 100644 > > --- > a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c > > +++ > b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c > > @@ -34,25 +34,16 @@ SerialPortGetDivisor ( > >    UINT32  SerialBaudRate > >  ) > >  { > > -  UINT64              BaseClockRate; > > +  UINT32              BaseClockRate; > >    UINT32              Divisor; > > > > -  // > > -  // On the Raspberry Pi, the clock to use for the > 16650-compatible UART > > -  // is the base clock divided by the 12.12 fixed point VPU > clock divisor. > > -  // > > -  BaseClockRate = (UINT64)PcdGet32 (PcdSerialClockRate); > > -#if (RPI_MODEL == 4) > > -  Divisor = MmioRead32(BCM2836_CM_BASE + > BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF; > > -  if (Divisor != 0) > > -    BaseClockRate = (BaseClockRate << 12) / Divisor; > > -#endif > > +  BaseClockRate = PcdGet32 (PcdSerialClockRate); > > > >    // > >    // As per the BCM2xxx datasheets: > >    // baudrate = system_clock_freq / (8 * (divisor + 1)). > >    // > > -  Divisor = (UINT32)BaseClockRate / (SerialBaudRate * 8); > > +  Divisor = BaseClockRate / (SerialBaudRate * 8); > >    if (Divisor != 0) { > >      Divisor--; > >    } > > diff --git > a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S > b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S > > index 58351e4fb8cc..7008aaf8aa4c 100644 > > --- > a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S > > +++ > b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S > > @@ -85,13 +85,11 @@ ASM_FUNC (ArmPlatformPeiBootAction) > >      adr     x2, mBoardRevision > >      str     w0, [x2] > > > > -#if (RPI_MODEL == 3) > >      run     .Lclkinfo_buffer > > > >      ldr     w0, .Lfrequency > >      adr     x2, _gPcd_BinaryPatch_PcdSerialClockRate > >      str     w0, [x2] > > -#endif > > > >      ret > > > > @@ -135,7 +133,6 @@ ASM_FUNC (ArmPlatformPeiBootAction) > >      .long   0                           // end tag > >      .set    .Lrevinfo_size, . - .Lrevinfo_buffer > > > > -#if (RPI_MODEL == 3) > >      .align  4 > >  .Lclkinfo_buffer: > >      .long   .Lclkinfo_size > > @@ -148,7 +145,6 @@ ASM_FUNC (ArmPlatformPeiBootAction) > >      .long   0                           // frequency > >      .long   0                           // end tag > >      .set    .Lclkinfo_size, . - .Lclkinfo_buffer > > -#endif > > > >  //UINTN > >  //ArmPlatformGetPrimaryCoreMpId ( > > diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc > b/Platform/RaspberryPi/RPi4/RPi4.dsc > > index 2c05c31118d2..ff802d8347ea 100644 > > --- a/Platform/RaspberryPi/RPi4/RPi4.dsc > > +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc > > @@ -429,7 +429,6 @@ [PcdsFixedAtBuild.common] > >    gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000 > >    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE > >    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride|4 > > -  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|1000000000 > >    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27 > >    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8 > >    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200 > > @@ -465,6 +464,9 @@ [PcdsFixedAtBuild.common] > >    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2" > >    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE > > > > +[PcdsPatchableInModule] > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000 > > + > >  [PcdsDynamicHii.common.DEFAULT] > > > >    # > > @@ -621,7 +623,7 @@ [Components.common] > >    MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf > >    MdeModulePkg/Universal/SerialDxe/SerialDxe.inf { > >      > > - > SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf > > + > SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf > >    } > >    Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.inf > >    EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.inf > > -- > > 2.29.2.windows.2 > > >