Re: [edk2-devel] [edk2-platforms][PATCH 4/6] Platform/RPi4: Add switch to select between PL011 and miniUART

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

On 2019.12.18 16:05, Philippe Mathieu-Daudé wrote:
> On 12/18/19 12:41 PM, Pete Batard wrote:
>> The PL011 can be a better choice for the serial console on the RPi4,
>> given that its baud clock is not derived from the CPU clock (which
>> may change under our feet unless we keep it fixed at a low rate), and
>> given the fact that the SBSA/SBBR specs that describe ARM specific
>> architectural requirements for ACPI only permit PL011 based UARTs to
>> begin with.
>>
>> Therefore we add a new PL011_ENABLE build switch to tell the firmware
>> to use PL011 for all console serial I/O, including for TF-A, SPCR and
>> DBG2, as well as toggle the BlueTooth module to use the mini UART.
>>
>> For the time being, the option is disabled by default because it
>> requires an overlay to be enabled in config.txt that pinmuxes the
>> PL011 TX/RX lines to the UART pins on the connector block.
>>
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> ---
> [...]
>> index 65f48ceae688..e5c10c923626 100644
>> --- a/Platform/RaspberryPi/RPi4/AcpiTables/Spcr.aslc
>> +++ b/Platform/RaspberryPi/RPi4/AcpiTables/Spcr.aslc
>> @@ -18,12 +18,19 @@
>>   #define RPI_UART_FLOW_CONTROL_NONE           0
>> +#ifdef PL011_ENABLE
>> +#define RPI_UART_INTERFACE_TYPE              
>> EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_INTERFACE_TYPE_ARM_PL011_UART 
>>
>> +#define RPI_UART_BASE_ADDRESS                FixedPcdGet64 
>> (PcdSerialRegisterBase)
> 
> See comment below.
> 
>> +#define RPI_UART_INTERRUPT                   0x99
>> +#else
>> +#define RPI_UART_INTERFACE_TYPE              
>> EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_INTERFACE_TYPE_BCM2835_UART 
>>
>>   //
>>   // When using the miniUART, PcdSerialRegisterBase points to the 8250 
>> base address,
>>   // which is offset by 0x40 from the actual Bcm2835 base address
>>   //
>>   #define RPI_UART_BASE_ADDRESS                (FixedPcdGet64 
>> (PcdSerialRegisterBase) - 0x40)
>>   #define RPI_UART_INTERRUPT                   0x7D
>> +#endif
>>   STATIC EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE Spcr = {
>>     ACPI_HEADER (
>> @@ -32,7 +39,7 @@ STATIC 
>> EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE Spcr = {
>>       EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_REVISION
>>     ),
>>     // UINT8                                   InterfaceType;
>> -  
>> EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_INTERFACE_TYPE_BCM2835_UART, 
>>
>> +  RPI_UART_INTERFACE_TYPE,
>>     // UINT8                                   Reserved1[3];
>>     {
>>       EFI_ACPI_RESERVED_BYTE,
>> diff --git a/Platform/RaspberryPi/RPi4/AcpiTables/Uart.asl 
>> b/Platform/RaspberryPi/RPi4/AcpiTables/Uart.asl
>> index 15149892f3b0..5b59f2dd3e16 100644
>> --- a/Platform/RaspberryPi/RPi4/AcpiTables/Uart.asl
>> +++ b/Platform/RaspberryPi/RPi4/AcpiTables/Uart.asl
>> @@ -108,7 +108,7 @@ Device(BTH0)
>>     {
>>       Name (RBUF, ResourceTemplate ()
>>       {
>> -      // BT UART: UART0 (PL011)
>> +      // BT UART: URT0 (PL011) or URTM (miniUART)
>>         UARTSerialBus(
>>           115200,        // InitialBaudRate: in BPS
>>           ,              // BitsPerByte: default to 8 bits
>> @@ -133,7 +133,11 @@ Device(BTH0)
>>                          //   no flow control.
>>           16,            // ReceiveBufferSize
>>           16,            // TransmitBufferSize
>> +#ifdef PL011_ENABLE
>> +        "\\_SB.URTM",  // ResourceSource:
>> +#else
>>           "\\_SB.URT0",  // ResourceSource:
>> +#endif
>>                          //   UART bus controller name
>>           ,              // ResourceSourceIndex: assumed to be 0
>>           ,              // ResourceUsage: assumed to be
>> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc 
>> b/Platform/RaspberryPi/RPi4/RPi4.dsc
>> index 1624ebda27d7..ccf5bd5b9ef3 100644
>> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
>> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
>> @@ -38,6 +38,7 @@ [Defines]
>>     DEFINE SECURE_BOOT_ENABLE      = FALSE
>>     DEFINE INCLUDE_TFTP_COMMAND    = FALSE
>>     DEFINE DEBUG_PRINT_ERROR_LEVEL = 0x8000004F
>> +  DEFINE PL011_ENABLE            = FALSE
>>   
>> ################################################################################ 
>>
>>   #
>> @@ -116,10 +117,16 @@ [LibraryClasses.common]
>>     ArmHvcLib|ArmPkg/Library/ArmHvcLib/ArmHvcLib.inf
>>     
>> ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf 
>>
>> +!if $(PL011_ENABLE) == TRUE
>> +  
>> PL011UartClockLib|ArmPlatformPkg/Library/PL011UartClockLib/PL011UartClockLib.inf 
>>
>> +  PL011UartLib|ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.inf
>> +  
>> SerialPortLib|ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf 
>>
>> +!else
>>     PciCf8Lib|MdePkg/Library/BasePciCf8Lib/BasePciCf8Lib.inf
>>     PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
>>     
>> PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf 
>>
>>     
>> SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf 
>>
>> +!endif
>>     # Cryptographic libraries
>>     IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
>> @@ -229,6 +236,12 @@ [BuildOptions]
>>     GCC:*_*_AARCH64_DLINK_FLAGS = -Wl,--fix-cortex-a53-843419
>>     GCC:RELEASE_*_*_CC_FLAGS    = -DMDEPKG_NDEBUG -DNDEBUG
>> +!if $(PL011_ENABLE) == TRUE
>> +  GCC:*_*_*_CC_FLAGS          = -DPL011_ENABLE
>> +  GCC:*_*_*_ASLPP_FLAGS       = -DPL011_ENABLE
>> +  GCC:*_*_*_ASLCC_FLAGS       = -DPL011_ENABLE
>> +!endif
>> +
>>   [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
>>     GCC:*_*_AARCH64_DLINK_FLAGS = -z common-page-size=0x10000
>> @@ -391,6 +404,13 @@ [PcdsFixedAtBuild.common]
>>     gBcm27xxTokenSpaceGuid.PcdBcm27xxPciBusMmioLen|0x3ffffff
>>     gBcm27xxTokenSpaceGuid.PcdBcm27xxPciCpuMmioAdr|0x600000000
>> +!if $(PL011_ENABLE) == TRUE
>> +  ## PL011 UART
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase|0xfe201000
> 
> Can we use relative to gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress 
> instead?
> 
> And instead of RPI_UART_BASE_ADDRESS():
> 
> #define BCM283X_UART_OFFSET          0x00201000
> #define BCM283X_UART_BASE_ADDRESS   (FixedPcdGet64 
> (PcdBcm283xRegistersAddress) \
> 
>                                      + BCM283X_UART_OFFSET)

That's not a bad suggestion but if we do that then I'd rather introduce 
2 new BCM2836_MINI_UART_OFFSET and BCM2836_PL011_UART_OFFSET constants 
in Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h and then 
set BCM283X_UART_BASE_ADDRESS to PcdBcm283xRegistersAddress + one or the 
other.

The whole point was to avoid adding offsets, which may or may not remain 
constant over the evolution of the Bcm283x family and derivatives, into 
individual sources, because then we way have to hunt these constants in 
places we may not even remember they exist.

On the other hand, I wouldn't mind avoiding that confusing 0x40 offset 
we need to substract when using the 8250 UART. So I think I'll go with 
the BCM283X_UART_BASE_ADDRESS + offset route, after adding an extra 
patch to the series that adds the UART offset constants to 
Silicon/.../Bcm2836.h.

Regards,

/Pete

> 
> 
>> +  gArmPlatformTokenSpaceGuid.PL011UartInteger|0
>> +  gArmPlatformTokenSpaceGuid.PL011UartFractional|0
>> +  gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000
>> +!else
>>     ## NS16550 compatible UART
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase|0xfe215040
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE
>> @@ -398,7 +418,10 @@ [PcdsFixedAtBuild.common]
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8
>> +!endif
>> +
>>     gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200
>> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultReceiveFifoDepth|0
> [...]
> 
> 
> 
>