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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 12/18/19 5:59 PM, Pete Batard wrote:
> 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.

If the family evolves, we can handle the new case or copy/paste again.

I insist with this modularity because then with little work we can 
support the RPi1, using:
gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress|0x20000000

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

If you don't mind doing the work, I believe this is the best way.

Thanks!

Phil.