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

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 5/6/20 12:18 PM, Pete Batard wrote:
> 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)?
> 

I'd prefer not to. That way, we can easily spot problems when the 
patching code is incorporated inadvertently.

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