Re: [PATCH v3 edk2-platforms 01/23] Silicon/Broadcom/Bcm282x: Add interrupt driver

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

Hi Pete,

On Mon, 28 Jan 2019 at 13:44, Pete Batard <pete@akeo.ie> wrote:
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
>  Silicon/Broadcom/Bcm283x/Bcm283x.dec                           |  23 ++
>  Silicon/Broadcom/Bcm283x/Drivers/InterruptDxe/InterruptDxe.c   | 367 ++++++++++++++++++++
>  Silicon/Broadcom/Bcm283x/Drivers/InterruptDxe/InterruptDxe.inf |  48 +++
>  Silicon/Broadcom/Include/IndustryStandard/Bcm2836.h            |  72 ++++
>  4 files changed, 510 insertions(+)
>
> diff --git a/Silicon/Broadcom/Bcm283x/Bcm283x.dec b/Silicon/Broadcom/Bcm283x/Bcm283x.dec
> new file mode 100644
> index 000000000000..8ead5ca87f5d
> --- /dev/null
> +++ b/Silicon/Broadcom/Bcm283x/Bcm283x.dec
> @@ -0,0 +1,23 @@
> +## @file
> +#
> +#  Copyright (c) 2019, Pete Batard <pete@akeo.ie>
> +#
> +#  This program and the accompanying materials are licensed and made available
> +#  under the terms and conditions of the BSD License which accompanies this
> +#  distribution. The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> +#  IMPLIED.
> +#
> +##
> +
> +[Defines]
> +  DEC_SPECIFICATION              = 0x0001001A
> +  PACKAGE_NAME                   = Bcm283xPkg

Please remove the 'Pkg'

> +  PACKAGE_GUID                   = 900C0F44-1152-4FF9-B9C5-933E2918C831
> +  PACKAGE_VERSION                = 1.0
> +
> +[Includes]
> +  Silicon/Broadcom/Include

Please change this to 'Include', and move whatever resides in
Silicon/Broadcom/Include to Silicon/Broadcom/Bcm283x/Include
This ensures that the package itself does not depend on the directory
structure of the repository it resides in.

With that,

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> diff --git a/Silicon/Broadcom/Bcm283x/Drivers/InterruptDxe/InterruptDxe.c b/Silicon/Broadcom/Bcm283x/Drivers/InterruptDxe/InterruptDxe.c
> new file mode 100644
> index 000000000000..9058aa94ffb9
> --- /dev/null
> +++ b/Silicon/Broadcom/Bcm283x/Drivers/InterruptDxe/InterruptDxe.c
> @@ -0,0 +1,367 @@
> +/** @file
> + *
> + *  Copyright (c) 2016, Linaro, Ltd. All rights reserved.
> + *
> + *  This program and the accompanying materials
> + *  are licensed and made available under the terms and conditions of the BSD License
> + *  which accompanies this distribution.  The full text of the license may be found at
> + *  http://opensource.org/licenses/bsd-license.php
> + *
> + *  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> + *  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> + *
> + **/
> +
> +#include <PiDxe.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
> +
> +#include <IndustryStandard/Bcm2836.h>
> +
> +#include <Protocol/Cpu.h>
> +#include <Protocol/HardwareInterrupt.h>
> +
> +//
> +// This currently only implements support for the architected timer interrupts
> +// on the per-CPU interrupt controllers.
> +//
> +#define NUM_IRQS                    (4)
> +
> +#ifdef MDE_CPU_AARCH64
> +#define ARM_ARCH_EXCEPTION_IRQ      EXCEPT_AARCH64_IRQ
> +#else
> +#define ARM_ARCH_EXCEPTION_IRQ      EXCEPT_ARM_IRQ
> +#endif
> +
> +STATIC CONST
> +EFI_PHYSICAL_ADDRESS RegBase = FixedPcdGet32 (PcdInterruptBaseAddress);
> +
> +//
> +// Notifications
> +//
> +STATIC EFI_EVENT                    mExitBootServicesEvent;
> +STATIC HARDWARE_INTERRUPT_HANDLER   mRegisteredInterruptHandlers[NUM_IRQS];
> +
> +/**
> +  Shutdown our hardware
> +
> +  DXE Core will disable interrupts and turn off the timer and disable interrupts
> +  after all the event handlers have run.
> +
> +  @param[in]  Event   The Event that is being processed
> +  @param[in]  Context Event Context
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +ExitBootServicesEvent (
> +  IN EFI_EVENT  Event,
> +  IN VOID       *Context
> +  )
> +{
> +  // Disable all interrupts
> +  MmioWrite32 (RegBase + BCM2836_INTC_TIMER_CONTROL_OFFSET, 0);
> +}
> +
> +/**
> +  Enable interrupt source Source.
> +
> +  @param This     Instance pointer for this protocol
> +  @param Source   Hardware source of the interrupt
> +
> +  @retval EFI_SUCCESS       Source interrupt enabled.
> +  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +EnableInterruptSource (
> +  IN EFI_HARDWARE_INTERRUPT_PROTOCOL    *This,
> +  IN HARDWARE_INTERRUPT_SOURCE          Source
> +  )
> +{
> +  if (Source >= NUM_IRQS) {
> +    ASSERT (FALSE);
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  MmioOr32 (RegBase + BCM2836_INTC_TIMER_CONTROL_OFFSET, 1 << Source);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +
> +/**
> +  Disable interrupt source Source.
> +
> +  @param This     Instance pointer for this protocol
> +  @param Source   Hardware source of the interrupt
> +
> +  @retval EFI_SUCCESS       Source interrupt disabled.
> +  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +DisableInterruptSource (
> +  IN EFI_HARDWARE_INTERRUPT_PROTOCOL    *This,
> +  IN HARDWARE_INTERRUPT_SOURCE          Source
> +  )
> +{
> +  if (Source >= NUM_IRQS) {
> +    ASSERT (FALSE);
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  MmioAnd32 (RegBase + BCM2836_INTC_TIMER_CONTROL_OFFSET, ~(1 << Source));
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Register Handler for the specified interrupt source.
> +
> +  @param This     Instance pointer for this protocol
> +  @param Source   Hardware source of the interrupt
> +  @param Handler  Callback for interrupt. NULL to unregister
> +
> +  @retval EFI_SUCCESS Source was updated to support Handler.
> +  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +RegisterInterruptSource (
> +  IN EFI_HARDWARE_INTERRUPT_PROTOCOL    *This,
> +  IN HARDWARE_INTERRUPT_SOURCE          Source,
> +  IN HARDWARE_INTERRUPT_HANDLER         Handler
> +  )
> +{
> +  if (Source >= NUM_IRQS) {
> +    ASSERT (FALSE);
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  if (Handler == NULL && mRegisteredInterruptHandlers[Source] == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Handler != NULL && mRegisteredInterruptHandlers[Source] != NULL) {
> +    return EFI_ALREADY_STARTED;
> +  }
> +
> +  mRegisteredInterruptHandlers[Source] = Handler;
> +  return EnableInterruptSource (This, Source);
> +}
> +
> +
> +/**
> +  Return current state of interrupt source Source.
> +
> +  @param This     Instance pointer for this protocol
> +  @param Source   Hardware source of the interrupt
> +  @param InterruptState  TRUE: source enabled, FALSE: source disabled.
> +
> +  @retval EFI_SUCCESS       InterruptState is valid
> +  @retval EFI_DEVICE_ERROR  InterruptState is not valid
> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +GetInterruptSourceState (
> +  IN EFI_HARDWARE_INTERRUPT_PROTOCOL    *This,
> +  IN HARDWARE_INTERRUPT_SOURCE          Source,
> +  IN BOOLEAN                            *InterruptState
> +  )
> +{
> +  if (InterruptState == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Source >= NUM_IRQS) {
> +    ASSERT (FALSE);
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  *InterruptState = (MmioRead32 (RegBase + BCM2836_INTC_TIMER_CONTROL_OFFSET) &
> +                     (1 << Source)) != 0;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Signal to the hardware that the End Of Intrrupt state
> +  has been reached.
> +
> +  @param This     Instance pointer for this protocol
> +  @param Source   Hardware source of the interrupt
> +
> +  @retval EFI_SUCCESS       Source interrupt EOI'ed.
> +  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +EndOfInterrupt (
> +  IN EFI_HARDWARE_INTERRUPT_PROTOCOL    *This,
> +  IN HARDWARE_INTERRUPT_SOURCE          Source
> +  )
> +{
> +  return EFI_SUCCESS;
> +}
> +
> +
> +/**
> +  EFI_CPU_INTERRUPT_HANDLER that is called when a processor interrupt occurs.
> +
> +  @param  InterruptType    Defines the type of interrupt or exception that
> +                           occurred on the processor.This parameter is processor
> +                           architecture specific.
> +  @param  SystemContext    A pointer to the processor context when
> +                           the interrupt occurred on the processor.
> +
> +  @return None
> +
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +IrqInterruptHandler (
> +  IN EFI_EXCEPTION_TYPE           InterruptType,
> +  IN EFI_SYSTEM_CONTEXT           SystemContext
> +  )
> +{
> +  HARDWARE_INTERRUPT_HANDLER  InterruptHandler;
> +  HARDWARE_INTERRUPT_SOURCE   Source;
> +  UINT32                      RegVal;
> +
> +  RegVal = MmioRead32 (RegBase + BCM2836_INTC_TIMER_PENDING_OFFSET) &
> +    ((1 << NUM_IRQS) - 1);
> +  Source = HighBitSet32 (RegVal);
> +  if (Source < 0) {
> +    return;
> +  }
> +
> +  InterruptHandler = mRegisteredInterruptHandlers[Source];
> +  if (InterruptHandler != NULL) {
> +    // Call the registered interrupt handler.
> +    InterruptHandler (Source, SystemContext);
> +  }
> +}
> +
> +//
> +// The protocol instance produced by this driver
> +//
> +STATIC EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptProtocol = {
> +  RegisterInterruptSource,
> +  EnableInterruptSource,
> +  DisableInterruptSource,
> +  GetInterruptSourceState,
> +  EndOfInterrupt
> +};
> +
> +STATIC VOID *mCpuArchProtocolNotifyEventRegistration;
> +
> +STATIC
> +VOID
> +EFIAPI
> +CpuArchEventProtocolNotify (
> +  IN  EFI_EVENT       Event,
> +  IN  VOID            *Context
> +  )
> +{
> +  EFI_CPU_ARCH_PROTOCOL   *Cpu;
> +  EFI_STATUS              Status;
> +
> +  //
> +  // Get the CPU protocol that this driver requires.
> +  //
> +  Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID**)&Cpu);
> +  if (EFI_ERROR (Status)) {
> +    return;
> +  }
> +
> +  //
> +  // Unregister the default exception handler.
> +  //
> +  Status = Cpu->RegisterInterruptHandler (Cpu, ARM_ARCH_EXCEPTION_IRQ, NULL);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: Cpu->RegisterInterruptHandler() - %r\n",
> +      __FUNCTION__, Status));
> +    ASSERT (FALSE);
> +    return;
> +  }
> +
> +  //
> +  // Register to receive interrupts
> +  //
> +  Status = Cpu->RegisterInterruptHandler (Cpu, ARM_ARCH_EXCEPTION_IRQ, IrqInterruptHandler);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: Cpu->RegisterInterruptHandler() - %r\n",
> +      __FUNCTION__, Status));
> +    ASSERT (FALSE);
> +    return;
> +  }
> +}
> +
> +
> +/**
> +  Initialize the state information for the CPU Architectural Protocol
> +
> +  @param  ImageHandle   of the loaded driver
> +  @param  SystemTable   Pointer to the System Table
> +
> +  @retval EFI_SUCCESS           Protocol registered
> +  @retval EFI_OUT_OF_RESOURCES  Cannot allocate protocol data structure
> +  @retval EFI_DEVICE_ERROR      Hardware problems
> +
> +**/
> +EFI_STATUS
> +InterruptDxeInitialize (
> +  IN EFI_HANDLE         ImageHandle,
> +  IN EFI_SYSTEM_TABLE   *SystemTable
> +  )
> +{
> +  EFI_STATUS Status;
> +  EFI_EVENT  CpuArchEvent;
> +
> +  // Make sure the Interrupt Controller Protocol is not already installed in the system.
> +  ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
> +
> +  Status = gBS->InstallMultipleProtocolInterfaces (
> +                  &ImageHandle,
> +                  &gHardwareInterruptProtocolGuid,
> +                  &gHardwareInterruptProtocol,
> +                  NULL
> +                );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  //
> +  // Install the interrupt handler as soon as the CPU arch protocol appears.
> +  //
> +  CpuArchEvent = EfiCreateProtocolNotifyEvent (
> +                   &gEfiCpuArchProtocolGuid,
> +                   TPL_CALLBACK,
> +                   CpuArchEventProtocolNotify,
> +                   NULL,
> +                   &mCpuArchProtocolNotifyEventRegistration
> +                 );
> +  ASSERT (CpuArchEvent != NULL);
> +
> +  // Register for an ExitBootServicesEvent
> +  Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_NOTIFY,
> +                  ExitBootServicesEvent, NULL, &mExitBootServicesEvent);
> +
> +  ASSERT_EFI_ERROR (Status);
> +
> +  return Status;
> +}
> diff --git a/Silicon/Broadcom/Bcm283x/Drivers/InterruptDxe/InterruptDxe.inf b/Silicon/Broadcom/Bcm283x/Drivers/InterruptDxe/InterruptDxe.inf
> new file mode 100644
> index 000000000000..5812e48dbb7a
> --- /dev/null
> +++ b/Silicon/Broadcom/Bcm283x/Drivers/InterruptDxe/InterruptDxe.inf
> @@ -0,0 +1,48 @@
> +#/** @file
> +#
> +#  Copyright (c) 2017, Andrei Warkentin <andrey.warkentin@gmail.com>
> +#  Copyright (c) 2016 Linaro, Ltd. All rights reserved.
> +#
> +#  This program and the accompanying materials
> +#  are licensed and made available under the terms and conditions of the BSD License
> +#  which accompanies this distribution.  The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +#
> +#**/
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = InterruptDxe
> +  FILE_GUID                      = 3944f2d7-2e09-4fc0-9e98-008375641453
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = InterruptDxeInitialize
> +
> +[Sources]
> +  InterruptDxe.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  Silicon/Broadcom/Bcm283x/Bcm283x.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  IoLib
> +  UefiBootServicesTableLib
> +  UefiLib
> +  UefiDriverEntryPoint
> +
> +[Protocols]
> +  gHardwareInterruptProtocolGuid  ## PRODUCES
> +  gEfiCpuArchProtocolGuid         ## CONSUMES ## NOTIFY
> +
> +[FixedPcd]
> +  gEmbeddedTokenSpaceGuid.PcdInterruptBaseAddress
> +
> +[Depex]
> +  TRUE
> diff --git a/Silicon/Broadcom/Include/IndustryStandard/Bcm2836.h b/Silicon/Broadcom/Include/IndustryStandard/Bcm2836.h
> new file mode 100644
> index 000000000000..f9fffb764649
> --- /dev/null
> +++ b/Silicon/Broadcom/Include/IndustryStandard/Bcm2836.h
> @@ -0,0 +1,72 @@
> +/** @file
> + *
> + *  Copyright (c) 2017, Andrei Warkentin <andrey.warkentin@gmail.com>
> + *  Copyright (c) 2016, Linaro Limited. All rights reserved.
> + *
> + *  Redistribution and use in source and binary forms, with or without
> + *  modification, are permitted provided that the following conditions are met:
> + *
> + *  Redistributions of source code must retain the above copyright notice, this
> + *  list of conditions and the following disclaimer.
> + *
> + *  Redistributions in binary form must reproduce the above copyright notice,
> + *  this list of conditions and the following disclaimer in the documentation
> + *  and/or other materials provided with the distribution.
> + *
> + *  Neither the name of ARM nor the names of its contributors may be used
> + *  to endorse or promote products derived from this software without specific
> + *  prior written permission.
> + *
> + *  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + *  AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + *  IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + *  ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + *  LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + *  CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + *  SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + *  INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + *  CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + *  ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + *  POSSIBILITY OF SUCH DAMAGE.
> + *
> + **/
> +
> +#ifndef __BCM2836_H__
> +#define __BCM2836_H__
> +
> +/*
> + * Both "core" and SoC perpherals (1M each).
> + */
> +#define BCM2836_SOC_REGISTERS                               0x3f000000
> +#define BCM2836_SOC_REGISTER_LENGTH                         0x02000000
> +
> +/*
> + * Offset between the CPU's view and the VC's view of system memory.
> + */
> +#define BCM2836_DMA_DEVICE_OFFSET                           0xc0000000
> +
> +/* watchdog constants */
> +#define BCM2836_WDOG_BASE_ADDRESS                           0x3f100000
> +#define BCM2836_WDOG_PASSWORD                               0x5a000000
> +#define BCM2836_WDOG_RSTC_OFFSET                            0x0000001c
> +#define BCM2836_WDOG_WDOG_OFFSET                            0x00000024
> +#define BCM2836_WDOG_RSTC_WRCFG_MASK                        0x00000030
> +#define BCM2836_WDOG_RSTC_WRCFG_FULL_RESET                  0x00000020
> +
> +/* mailbox interface constants */
> +#define BCM2836_MBOX_BASE_ADDRESS                           0x3f00b880
> +#define BCM2836_MBOX_READ_OFFSET                            0x00000000
> +#define BCM2836_MBOX_STATUS_OFFSET                          0x00000018
> +#define BCM2836_MBOX_CONFIG_OFFSET                          0x0000001c
> +#define BCM2836_MBOX_WRITE_OFFSET                           0x00000020
> +
> +#define BCM2836_MBOX_STATUS_FULL                            0x1f
> +#define BCM2836_MBOX_STATUS_EMPTY                           0x1e
> +
> +#define BCM2836_MBOX_NUM_CHANNELS                           16
> +
> +/* interrupt controller constants */
> +#define BCM2836_INTC_TIMER_CONTROL_OFFSET                   0x00000040
> +#define BCM2836_INTC_TIMER_PENDING_OFFSET                   0x00000060
> +
> +#endif /*__BCM2836_H__ */
> --
> 2.17.0.windows.1
>