[edk2-devel] [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Fix mini UART baud divisor calculation
Pete Batard
pete at akeo.ie
Sat Apr 3 14:35:36 UTC 2021
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 at kernel.org
> <mailto:ardb at kernel.org>> a scris:
>
> On Fri, 2 Apr 2021 at 19:52, Mario Bălănică
> <mariobalanica02 at gmail.com <mailto:mariobalanica02 at 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 at gmail.com
> <mailto:mariobalanica02 at 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
> >
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73667): https://edk2.groups.io/g/devel/message/73667
Mute This Topic: https://groups.io/mt/81808942/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list