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...@kernel.org <mailto:a...@kernel.org>> a scris:On Fri, 2 Apr 2021 at 19:52, Mario Bălănică <mariobalanic...@gmail.com <mailto:mariobalanic...@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ă <mariobalanic...@gmail.com <mailto:mariobalanic...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
