From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by mx.groups.io with SMTP id smtpd.web09.13021.1576688386153652553 for ; Wed, 18 Dec 2019 08:59:46 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=IqBJvbCE; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.128.65, mailfrom: pete@akeo.ie) Received: by mail-wm1-f65.google.com with SMTP id d73so2600201wmd.1 for ; Wed, 18 Dec 2019 08:59:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=TuLyXTAKWAlbZACSbmddYDOskkdqajnnARfniye8TBM=; b=IqBJvbCEEYzr+RogkWa2EIPVnXVyjrDhjVYBIrQbvaNd+IzDm732CbuehwNOEiIw4I 6T6uHXCagwCPBNoLVEN4HeKCyoP1NGswpwr2YLkKbPWcwtfUUoDASUIDuwzTL1BtqcRl hG+coAwYvc6Y3cOu9kbYBrTLxto7JyPJCQqy4RAQHBsUV3qn09O+qKHWFwe6LYgPa4uR wv5oVCmDzUIIvb+cgGKGa6xtKpfYLzxMo7RVuSggwWIWGGIgF7z0kv1v9BZL4fBts/H9 fDvRVclHUYkoMEBmqersuMKq5NKZwFAGGQ5LIH2ik+c1RzrjwLX8wz3m6ux6lnS7xjW5 tZuQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=TuLyXTAKWAlbZACSbmddYDOskkdqajnnARfniye8TBM=; b=jZo5eQygLD+m0DcxC5IMXrX7qYzsIpuJQg84VzXBvPip+izFmKiDcTBwJAVCALa5pr h9xu8FWyo2mJDZsBgJ3XwePT/aLXNvvxDxyD2HK9hivepBeslZ8FkzWXjRsZJouNZN51 ZFWJKkG33bcmM9TOHH7CwYtvc4iSolrU0ewooDd1uijVNu/6dSOAG3jWTxAlE2JfQM3P 5TuUMLgfT0nqSgmoDib6tH4TUPUCMSDZLwKgi1sBqag/fowu8GBluj/+zsW3RMjM2QBW STdVnJSFN7CCpztzLNVcXqbSCwyr9I9Wh6bXPhkSoupBJWaVFRCY15u+XQ1LKYXMPx1B BM4Q== X-Gm-Message-State: APjAAAUf76fHH416S3pk/uNCwogQMHN3MueUeHS9Wj4kHroGotKJ7nRr pkLAJYBxkYyt3jBPMB2fUHujgA== X-Google-Smtp-Source: APXvYqyCFC4kbawvihCZhFxzAQlOJNGLo/kVOMuhygSuuemjalyaRZq3BKEGhsdm8BYAeHiIqeGXHg== X-Received: by 2002:a05:600c:1007:: with SMTP id c7mr4640898wmc.158.1576688384544; Wed, 18 Dec 2019 08:59:44 -0800 (PST) Return-Path: Received: from [10.0.0.122] ([84.203.77.210]) by smtp.googlemail.com with ESMTPSA id w8sm7080670wmd.2.2019.12.18.08.59.42 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 18 Dec 2019 08:59:43 -0800 (PST) Subject: Re: [edk2-devel] [edk2-platforms][PATCH 4/6] Platform/RPi4: Add switch to select between PL011 and miniUART To: devel@edk2.groups.io, philmd@redhat.com Cc: ard.biesheuvel@linaro.org, leif.lindholm@linaro.org References: <20191218114156.9036-1-pete@akeo.ie> <20191218114156.9036-5-pete@akeo.ie> <682dfe70-95f4-642f-953f-db20a659c8aa@redhat.com> From: "Pete Batard" Message-ID: Date: Wed, 18 Dec 2019 16:59:41 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.9.1 MIME-Version: 1.0 In-Reply-To: <682dfe70-95f4-642f-953f-db20a659c8aa@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 8bit 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 >> --- > [...] >> 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 > [...] > > > >