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

["Ard Biesheuvel" <ardb@kernel.org>]

On Thu, 8 Apr 2021 at 13:43, Pete Batard <pete@akeo.ie> wrote:
>
> On 2021.04.08 12:06, Ard Biesheuvel wrote:
> > On Thu, 8 Apr 2021 at 11:47, Pete Batard <pete@akeo.ie> wrote:
> >>
> >> On 2021.04.06 15:04, Mario Bălănică wrote:
> >>> The VPU clock divisor has changed in this commit:
> >>> https://github.com/raspberrypi/firmware/commit/1e5456a,
> >>> thus breaking the mini UART clock divisor calculation on the Pi 4.
> >>>
> >>> Fix this by reading the core clock from the mailbox instead.
> >>>
> >>> Signed-off-by: Mario Bălănică <mariobalanica02@gmail.com>
> >>> ---
> >>>    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
> >>>
> >>
> >> Reviewed-by: Pete Batard <pete@akeo.ie>
> >> Tested-by: Pete Batard <pete@akeo.ie>
> >
> > Thanks Pete.
> >
> > Could anyone get me a version of this patch that is not whitespace
> > mangled? This one does not apply with 'git am'
> >
>
> Done.
>
> The version I sent is the one I applied & tested after I ran it through
> my unmangling script to remove the extra lines.
>
> Not sure if it'll still not be re-mangled by the list processing though.
>

Yes, which is odd given that I am receiving this email directly.

In any case, I still had to perform surgery on the patch to get it to apply.

Pushed as 7fe9704893f1..d2339f3c5f9a; mind double checking if I did
not break anything?

Thanks,
Ard.