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

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

[Mario Bălănică]

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ă <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
-- 
2.29.2.windows.2

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

[Ard Biesheuvel]

On Fri, 2 Apr 2021 at 19:52, Mario Bălănică <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
>
> Signed-off-by: Mario Bălănică <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
>

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

[Mario Bălănică]

[-- Attachment #1: Type: text/plain, Size: 5279 bytes --]

It works with the old firmware as well.

sâm., 3 apr. 2021, 10:36 Ard Biesheuvel <ardb@kernel.org> a scris:

> On Fri, 2 Apr 2021 at 19:52, Mario Bălănică <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
> >
> > Signed-off-by: Mario Bălănică <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
> >
>

[-- Attachment #2: Type: text/html, Size: 6632 bytes --]

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

[Pete Batard]

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
>      >
> 

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

[Mario Bălănică]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

Hi Pete,

I've submitted this patch because the mini UART serial console produces garbage since UEFI v1.25 (due to the wrong baud rate).
It has been tested on the firmware shipped with UEFI v1.24 and it works fine. I don't see any reason to test it on older versions than this.

The core clock is not fixed. I've made PcdSerialClockRate patchable here:

> 
> @@ -465,6 +464,9 @@ [PcdsFixedAtBuild.common]
> gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2"
> gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
> 
> +[PcdsPatchableInModule]
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000
> +
> 

500 MHz is the default clock rate (assuming it hasn't been changed in config.txt).
The PCD gets patched in both the PEI and DXE phases to the value read from mailbox.

[-- Attachment #2: Type: text/html, Size: 1331 bytes --]

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

[Pete Batard]

On 2021.04.03 16:17, Mario Bălănică wrote:
> I've submitted this patch because the mini UART serial console produces 
> garbage since UEFI v1.25 (due to the wrong baud rate).

OK.

When you fix something like this, please mention that there was a 
regression that requires fixing, in the commit message.

> It has been tested on the firmware shipped with UEFI v1.24 and it works 
> fine. I don't see any reason to test it on older versions than this.

Well, my worry is that you are removing this part:

 > -#if (RPI_MODEL == 4)
 > -  Divisor = MmioRead32(BCM2836_CM_BASE + 
BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF;
 > -  if (Divisor != 0)
 > -    BaseClockRate = (BaseClockRate << 12) / Divisor;
 > -#endif

which was added due to serial issues with older versions of the firmware 
(much older than the one included in 1.24), to make it compatible with 
versions of start4.elf where the Pi Foundation had suffled the serial 
clock rates yet again.

I guess if we can't make both newer versions and these older versions 
work with the same code, we have to stick with what works for newer 
ones. But I'm still wondering what changed in the Pi firmware to make 
applying this divisor computation fail, especially as this was supposed 
to alleviate the precise serial baudrate issue you are trying to fix.

> The core clock is not fixed. I've made PcdSerialClockRate patchable here:
> 
>     @@ -465,6 +464,9 @@ [PcdsFixedAtBuild.common]
>     gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2"
>     gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
> 
>     +[PcdsPatchableInModule]
>     + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000
>     +
> 
> 500 MHz is the default clock rate (assuming it hasn't been changed in 
> config.txt).
> The PCD gets patched in both the PEI and DXE phases to the value read 
> from mailbox.

Yeah, I had missed that.

I suppose we can go with what you suggest. But I'm still worried that 
the Pi Foundation are going to shuffle their clocks again in a future 
firmware, and that we'll be modifying this code yet again, as your patch 
is pretty much reverting the code to what we used to have... until we 
found it got broken by a newly introduced Pi Foundation firmware.

Regards,

/Pete

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

[Mario Bălănică]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

> 
> > -#if (RPI_MODEL == 4)
> > -  Divisor = MmioRead32(BCM2836_CM_BASE +
> BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF;
> > -  if (Divisor != 0)
> > -    BaseClockRate = (BaseClockRate << 12) / Divisor;
> > -#endif
> 

Keeping this doesn't interfere with the rest of the patch. I've removed it only because it's useless on the latest firmware.

[-- Attachment #2: Type: text/html, Size: 896 bytes --]

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

[Pete Batard]

On 2021.04.03 17:15, Mario Bălănică wrote:
>      > -#if (RPI_MODEL == 4)
>      > -  Divisor = MmioRead32(BCM2836_CM_BASE +
>     BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF;
>      > -  if (Divisor != 0)
>      > -    BaseClockRate = (BaseClockRate << 12) / Divisor;
>      > -#endif
> 
> Keeping this doesn't interfere with the rest of the patch. I've removed 
> it only because it's useless on the latest firmware.

Okay, then can you please send a v2 of your patch that leaves this in? I 
believe this divisor arithmetic is still needed with older versions of 
start4.elf, and we may have people using choosing to use an older 
start4.elf for whatever reason.

In general, if it doesn't interfere with your proposal, it's better to 
leave existing code in, as overzealous code removal may have unintended 
consequences, unless you are confident that the code is superfluous.

Also make sure you update the commit message to mention what issue you 
are fixing.

Thanks,

/Pete

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

[Mario Bălănică]

[-- Attachment #1: Type: text/plain, Size: 146 bytes --]

Can you tell me which commit in the firmware repo breaks the mini UART divisor calculation, so that I can test the final changes against it too?

[-- Attachment #2: Type: text/html, Size: 146 bytes --]

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

[Mario Bălănică]

[-- Attachment #1: Type: text/plain, Size: 200 bytes --]

I assume this is the commit: https://github.com/raspberrypi/firmware/commit/11e3c314bc2b64f7d862bac00ff3d9f42f3c5a50 as per https://github.com/raspberrypi/firmware/issues/1445#issuecomment-708460807

[-- Attachment #2: Type: text/html, Size: 463 bytes --]

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

[Pete Batard]

On 2021.04.03 17:55, Mario Bălănică wrote:
> Can you tell me which commit in the firmware repo breaks the mini UART 
> divisor calculation, so that I can test the final changes against it too?

That would be one of the start4.elf/fixup4.dat combination around the 
time when the commit that added the divisor was introduced. You should 
be able to find that commit through git blame and then work your way down.

If you need more information than that, I can try to dig it up, but it 
won't happen until at least Tuesday.

Regards,

/Pete

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

[Mario Bălănică]

[-- Attachment #1: Type: text/plain, Size: 689 bytes --]

Okay, I've tested the patch with the firmware from the commit mentioned above and it doesn't work. The VPU clock divisor bit needs PcdSerialClockRate to be set to 1 GHz (so that the quotient becomes equal to the core clock).

The mailbox on the other hand always returns 200 MHz for the core clock. This is definitely a bug, which got fixed it in a later commit.

There isn't any reliable way to support the firmwares with a broken mailbox interface as well in this case, and I really doubt anyone would want to use them with the UEFI in the first place. Your build scripts always include the latest version.

I can resend the patch with a more detailed commit message if you wish.

[-- Attachment #2: Type: text/html, Size: 719 bytes --]

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

[Pete Batard]

Hi Mario,

Please re-send a v2 with a commit message that explains the issue that 
is being fixed. Thanks.

For the record, I ran some tests that show that the divisor was changed 
from 1 to 2 with the latest Pi Foundation firmware (which is confirmed 
by the fact that if you switch your baudrate from 115200 to 57600, 
you'll get the expected output).

I'm not sure what game the Pi Foundation are playing at this stage, coz 
they sure don't seem to be following any official logic with how they 
actually set the serial clock, and our attempts at second guessing that 
logic are no longer working. So I guess the best we can do is go with 
whatever gets us output for the latest firmware, and hope they're not 
going to break that anytime soon (which is still better than the TF-A 
approach, where they got so fed up with this cat and mouse game that 
they dropped UART initialization altogether [1])

Regards,

/Pete

[1] 
https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/plat/rpi?h=v2.3&id=0eda713b9bd65222155900aacf3a67805351f88f

On 2021.04.03 23:45, Mario Bălănică wrote:
> Okay, I've tested the patch with the firmware from the commit mentioned 
> above and it doesn't work. The VPU clock divisor bit needs 
> PcdSerialClockRate to be set to 1 GHz (so that the quotient becomes 
> equal to the core clock).
> 
> The mailbox on the other hand always returns 200 MHz for the core clock. 
> This is definitely a bug, which got fixed it in a later commit.
> 
> There isn't any reliable way to support the firmwares with a broken 
> mailbox interface as well in this case, and I really doubt anyone would 
> want to use them with the UEFI in the first place. Your build scripts 
> always include the latest version.
> 
> I can resend the patch with a more detailed commit message if you wish.