Re: [edk2-devel] [PATCH edk2-platforms 3/5] Platform/RaspberryPi: fix 16550 divisor calculation logic

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

One general remark below:

On 2020.05.05 19:10, Ard Biesheuvel wrote:
> On 5/5/20 4:50 PM, Ard Biesheuvel wrote:
>> The 16550 'miniUART' on the Raspberry Pi gets its input clock from
>> different sources on RPi3 and RPi3. Fix the logic that derives the
> 
> This should be 'Rpi3 and RPi4'
> 
>> divisor for the 16550 baud clock on the respective platforms.
>>
>> While at it, make the input clock PCD patchable for RPi3 so we can
>> manipulate it at runtime in a future patch.

For the sake of harmonization between platforms, wouldn't we want to 
also declare the PCD patchable on RPi4 as well (even though we obviously 
aren't going to patch it for the time being)?

There is some plan of factorizing the DSC content, so if that doesn't 
hurt us, I'd propose we try to keep the Pi3 and Pi4 version as close as 
possible.

This also ties in with my remark for the next patch.

>>
>> Co-authored-by: Pete Batard <pete@akeo.ie>
>> Co-authored-by: Andrei Warkentin <andrey.warkentin@gmail.com>
>> Co-authored-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>>   Platform/RaspberryPi/RPi3/RPi3.dsc                                 
>> |  4 +++-
>>   Platform/RaspberryPi/RPi4/RPi4.dsc                                 
>> |  2 +-
>>   Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c | 
>> 14 ++++++++------
>>   3 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc 
>> b/Platform/RaspberryPi/RPi3/RPi3.dsc
>> index d7218219fc5a..96b27400eef8 100644
>> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
>> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
>> @@ -409,7 +409,6 @@ [PcdsFixedAtBuild.common]
>>     gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride|4
>> -  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8
>>     gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200
>> @@ -441,6 +440,9 @@ [PcdsFixedAtBuild.common]
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2"
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
>> +[PcdsPatchableInModule]
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|250000000
>> +
>>   [PcdsDynamicHii.common.DEFAULT]
>>     #
>> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc 
>> b/Platform/RaspberryPi/RPi4/RPi4.dsc
>> index 4fb015b077e6..5d8bd88d7e34 100644
>> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
>> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
>> @@ -420,7 +420,7 @@ [PcdsFixedAtBuild.common]
>>     gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride|4
>> -  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|1000000000
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8
>>     gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200
>> diff --git 
>> a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c 
>> b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
>> index b1d17d3fa04a..5e83bbf022eb 100644
>> --- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
>> +++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
>> @@ -41,18 +41,20 @@ SerialPortGetDivisor (
>>     // 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) * 4;
>> +  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
>>     //
>> -  // Now calculate divisor for baud generator
>> -  //    Ref_Clk_Rate / Baud_Rate / 16
>> +  // As per the BCM2xxx datasheets:
>> +  // baudrate = system_clock_freq / (8 * (divisor + 1)).
>>     //
>> -  Divisor = (UINT32)BaseClockRate / (SerialBaudRate * 16);
>> -  if (((UINT32)BaseClockRate % (SerialBaudRate * 16)) >= 
>> SerialBaudRate * 8) {
>> -    Divisor++;
>> +  Divisor = (UINT32)BaseClockRate / (SerialBaudRate * 8);
>> +  if (Divisor != 0) {
>> +    Divisor--;
>>     }
>>     return Divisor;
>>   }
>>
> 
> 
> 
> 

Reviewed-by: Pete Batard <pete@akeo.ie>