Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Fix mini UART baud divisor calculation

["Pete Batard" <pete@akeo.ie>]

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 <ardb@kernel.org 
> <mailto:ardb@kernel.org>> a scris:
> 
>     On Fri, 2 Apr 2021 at 19:52, Mario Bălănică
>     <mariobalanica02@gmail.com <mailto:mariobalanica02@gmail.com>> 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
>     <https://github.com/raspberrypi/firmware/commit/1e5456a>
>      >
>      > Signed-off-by: Mario Bălănică <mariobalanica02@gmail.com
>     <mailto:mariobalanica02@gmail.com>>
> 
>     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 {
>      >      <LibraryClasses>
>      > -     
>     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
>      >
>