public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Leif Lindholm <leif.lindholm@linaro.org>
To: evan.lloyd@arm.com
Cc: edk2-devel@lists.01.org,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Matteo Carlini <Matteo.Carlini@arm.com>,
	nd@arm.com
Subject: Re: [PATCH 3/5] ArmPkg/ArmGicDxe: Expose HardwareInterrupt2 protocol
Date: Thu, 14 Sep 2017 18:00:25 +0100	[thread overview]
Message-ID: <20170914170025.ywvthusrop42uukd@bivouac.eciton.net> (raw)
In-Reply-To: <20170911152335.72672-4-evan.lloyd@arm.com>

On Mon, Sep 11, 2017 at 04:23:33PM +0100, evan.lloyd@arm.com wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> The existing HardwareInterrupt protocol lacked a means to configure the
> level/edge properties of an interrupt.  The new HardwareInterrupt2
> protocol introduced this capability.
> This patch updates the GIC drivers to provide the new interfaces.
> The changes comprise:
>   Update to use HardwareInterrupt2 protocol
>   Additions to register info in ArmGicLib.h
>   Added new functionality (GetTriggerType and SetTriggerType)
> 
> The requirement for this change derives from a problem detected on ARM
> Juno boards, but the change is of generic (ARM) relevance.
> 
> This commit is in response to review on the mailing list and, as
> suggested there, rolls Girish's updates onto Ard's original example.
> 
> NOTE: At this point the GICv3 code is not tested.

Is this still the case?
I don't see how the code can be merged while that holds true.

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> Tested-by: Girish Pathak <girish.pathak@arm.com>
> ---
>  ArmPkg/Drivers/ArmGic/ArmGicDxe.inf       |   3 +-
>  ArmPkg/Drivers/ArmGic/ArmGicDxe.h         |  25 +++-
>  ArmPkg/Include/Library/ArmGicLib.h        |  12 +-
>  ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c   |  44 ++++++-
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 139 +++++++++++++++++++-
>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 138 ++++++++++++++++++-
>  6 files changed, 354 insertions(+), 7 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
> index e554301c4b28022c805f69242cf6ee979d19abc2..d5921533fb68fa32c3e0705b05930700ee81da07 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
> @@ -1,7 +1,7 @@
>  #/** @file
>  #
>  #  Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
> -#  Copyright (c) 2012 - 2015, ARM Ltd. All rights reserved.<BR>
> +#  Copyright (c) 2012 - 2017, ARM Ltd. All rights reserved.<BR>
>  #
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD License
> @@ -48,6 +48,7 @@ [LibraryClasses]
>  
>  [Protocols]
>    gHardwareInterruptProtocolGuid
> +  gHardwareInterrupt2ProtocolGuid
>    gEfiCpuArchProtocolGuid
>  
>  [Pcd.common]
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
> index 1018f2004e75d879a72c2d6bf37b64051e720d12..cefa4c2d4e4a05c54e51642db0f471e9a338afb6 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
> @@ -1,6 +1,6 @@
>  /*++
>  
> -Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
> +Copyright (c) 2013-2017, ARM Ltd. All rights reserved.<BR>
>  
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
> @@ -24,6 +24,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  
>  #include <Protocol/Cpu.h>
>  #include <Protocol/HardwareInterrupt.h>
> +#include <Protocol/HardwareInterrupt2.h>
>  
>  extern UINTN                        mGicNumInterrupts;
>  extern HARDWARE_INTERRUPT_HANDLER  *gRegisteredInterruptHandlers;
> @@ -34,6 +35,7 @@ extern HARDWARE_INTERRUPT_HANDLER  *gRegisteredInterruptHandlers;
>  EFI_STATUS
>  InstallAndRegisterInterruptService (
>    IN EFI_HARDWARE_INTERRUPT_PROTOCOL   *InterruptProtocol,
> +  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL  *Interrupt2Protocol,
>    IN EFI_CPU_INTERRUPT_HANDLER          InterruptHandler,
>    IN EFI_EVENT_NOTIFY                   ExitBootServicesEvent
>    );
> @@ -64,4 +66,25 @@ GicV3DxeInitialize (
>    IN EFI_SYSTEM_TABLE   *SystemTable
>    );
>  
> +
> +// Shared code
> +
> +/**
> +  Calculate GICD_ICFGRn base address and corresponding bit
> +  field Int_config[1] of the GIC distributor register.
> +
> +  @param Source       Hardware source of the interrupt.
> +  @param RegAddress   Corresponding GICD_ICFGRn base address.
> +  @param Config1Bit   Bit number of F Int_config[1] bit in the register.
> +
> +  @retval EFI_SUCCESS       Source interrupt supported.
> +  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
> +**/
> +EFI_STATUS
> +GicGetDistributorIcfgBaseAndBit (
> +  IN HARDWARE_INTERRUPT_SOURCE             Source,
> +  OUT UINTN                               *RegAddress,
> +  OUT UINTN                               *Config1Bit
> +  );
> +
>  #endif
> diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
> index f7b546895d116f81c65a853fcdb067ec7601b2da..1c8d8cf6a7c39e2b5e2e36feb3e5433f29f488e2 100644
> --- a/ArmPkg/Include/Library/ArmGicLib.h
> +++ b/ArmPkg/Include/Library/ArmGicLib.h
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +*  Copyright (c) 2011-2017, ARM Limited. All rights reserved.
>  *
>  *  This program and the accompanying materials
>  *  are licensed and made available under the terms and conditions of the BSD License
> @@ -51,10 +51,18 @@
>  #define ARM_GIC_ICDDCR_ARE      (1 << 4) // Affinity Routing Enable (ARE)
>  #define ARM_GIC_ICDDCR_DS       (1 << 6) // Disable Security (DS)
>  
> +// GICD_ICDICFR bits
> +#define ARM_GIC_ICDICFR_WIDTH            32   // ICDICFR is a 32 bit register
> +#define ARM_GIC_ICDICFR_BYTES            (ARM_GIC_ICDICFR_WIDTH / 8)
> +#define ARM_GIC_ICDICFR_F_WIDTH          2    // Each F field is 2 bits
> +#define ARM_GIC_ICDICFR_F_STRIDE         16   // (32/2) F fields per register
> +#define ARM_GIC_ICDICFR_F_CONFIG1_BIT    1    // Bit number within F field
> +#define ARM_GIC_ICDICFR_LEVEL_TRIGGERED  0x0  // Level triggered interrupt
> +#define ARM_GIC_ICDICFR_EDGE_TRIGGERED   0x1  // Edge triggered interrupt
> +
>  
>  // GIC Redistributor
>  
> -

Spurious whitespace change.

>  #define ARM_GICR_CTLR_FRAME_SIZE    SIZE_64KB
>  #define ARM_GICR_SGI_PPI_FRAME_SIZE SIZE_64KB
>  
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> index 88cb455b75bb8e8cb22157643a392403ce93129d..13a7fae07b856025ab1c0eac97d07ec4c4df20a9 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> @@ -1,6 +1,6 @@
>  /*++
>  
> -Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
> +Copyright (c) 2013-2017, ARM Ltd. All rights reserved.<BR>
>  
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
> @@ -43,6 +43,46 @@ UINTN mGicNumInterrupts                 = 0;
>  
>  HARDWARE_INTERRUPT_HANDLER  *gRegisteredInterruptHandlers = NULL;
>  
> +
> +/**
> +  Calculate GICD_ICFGRn base address and corresponding bit
> +  field Int_config[1] of the GIC distributor register.
> +
> +  @param Source       Hardware source of the interrupt.
> +  @param RegAddress   Corresponding GICD_ICFGRn base address.
> +  @param Config1Bit   Bit number of F Int_config[1] bit in the register.
> +
> +  @retval EFI_SUCCESS       Source interrupt supported.
> +  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
> +**/
> +EFI_STATUS
> +GicGetDistributorIcfgBaseAndBit (
> +  IN HARDWARE_INTERRUPT_SOURCE             Source,
> +  OUT UINTN                               *RegAddress,
> +  OUT UINTN                               *Config1Bit
> +  )
> +{
> +  UINTN                  RegIndex;
> +  UINTN                  Field;
> +
> +  if (Source >= mGicNumInterrupts) {
> +    ASSERT(Source < mGicNumInterrupts);
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  RegIndex = Source / ARM_GIC_ICDICFR_F_STRIDE;  // NOTE: truncation is significant
> +  Field = Source % ARM_GIC_ICDICFR_F_STRIDE;
> +  *RegAddress = PcdGet64 (PcdGicDistributorBase)
> +                  + ARM_GIC_ICDICFR
> +                  + (ARM_GIC_ICDICFR_BYTES * RegIndex);

Should these continuation lines really be indented?

> +  *Config1Bit = ((Field * ARM_GIC_ICDICFR_F_WIDTH)
> +                 + ARM_GIC_ICDICFR_F_CONFIG1_BIT);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +
> +
>  /**
>    Register Handler for the specified interrupt source.
>  
> @@ -88,6 +128,7 @@ RegisterInterruptSource (
>  EFI_STATUS
>  InstallAndRegisterInterruptService (
>    IN EFI_HARDWARE_INTERRUPT_PROTOCOL   *InterruptProtocol,
> +  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL  *Interrupt2Protocol,
>    IN EFI_CPU_INTERRUPT_HANDLER          InterruptHandler,
>    IN EFI_EVENT_NOTIFY                   ExitBootServicesEvent
>    )
> @@ -105,6 +146,7 @@ InstallAndRegisterInterruptService (
>    Status = gBS->InstallMultipleProtocolInterfaces (
>                    &gHardwareInterruptHandle,
>                    &gHardwareInterruptProtocolGuid, InterruptProtocol,
> +                  &gHardwareInterrupt2ProtocolGuid, Interrupt2Protocol,
>                    NULL
>                    );
>    if (EFI_ERROR (Status)) {
> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> index 50ec90207b515d849cbf64f0a4b0d639b3868e60..41db2277132d47dda1a047b73eaf63323b5b9aaf 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> @@ -29,6 +29,7 @@ Abstract:
>  #define ARM_GIC_DEFAULT_PRIORITY  0x80
>  
>  extern EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol;
> +extern EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V2Protocol;
>  
>  STATIC UINT32 mGicInterruptInterfaceBase;
>  STATIC UINT32 mGicDistributorBase;
> @@ -184,7 +185,7 @@ GicV2IrqInterruptHandler (
>      // Call the registered interrupt handler.
>      InterruptHandler (GicInterrupt, SystemContext);
>    } else {
> -    DEBUG ((EFI_D_ERROR, "Spurious GIC interrupt: 0x%x\n", GicInterrupt));
> +    DEBUG ((DEBUG_ERROR, "Spurious GIC interrupt: 0x%x\n", GicInterrupt));

I don't see anything changing here other than changing the EFI_D_ERROR
to DEBUG_ERROR. Preferably leave such non-functional changes out of
functional patches, even if it leaves the file inconsistent.
If a DEBUG line has a functional modification, clearly it should be
updated to the new form at the same time.

>      GicV2EndOfInterrupt (&gHardwareInterruptV2Protocol, GicInterrupt);
>    }
>  }
> @@ -200,6 +201,141 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol = {
>    GicV2EndOfInterrupt
>  };
>  
> +/**
> +  Get interrupt trigger type of an interrupt
> +
> +  @param This          Instance pointer for this protocol
> +  @param Source        Hardware source of the interrupt.
> +  @param TriggerType   Returns interrupt trigger type.
> +
> +  @retval EFI_SUCCESS       Source interrupt supported.
> +  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +GicV2GetTriggerType (
> +  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL      *This,
> +  IN  HARDWARE_INTERRUPT_SOURCE              Source,
> +  OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  *TriggerType
> +  )
> +{
> +  UINTN                   RegAddress;
> +  UINTN                   Config1Bit;
> +  EFI_STATUS              Status;
> +
> +  Status = GicGetDistributorIcfgBaseAndBit (
> +              Source,
> +              &RegAddress,
> +              &Config1Bit
> +              );
> +
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (MmioBitFieldRead32 (RegAddress, Config1Bit, Config1Bit) == 0) {

Why not

  if (MmioRead32 (RegAddress) & (1 << Config1Bit) == 0) {

?

> +     *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH;
> +  } else {
> +     *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Set interrupt trigger type of an interrupt
> +
> +  @param This          Instance pointer for this protocol
> +  @param Source        Hardware source of the interrupt.
> +  @param TriggerType   Interrupt trigger type.
> +
> +  @retval EFI_SUCCESS       Source interrupt supported.
> +  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +GicV2SetTriggerType (
> +  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL      *This,
> +  IN  HARDWARE_INTERRUPT_SOURCE             Source,
> +  IN  EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  TriggerType
> +  )
> +{
> +  UINTN                   RegAddress;
> +  UINTN                   Config1Bit;
> +  UINT32                  Value;
> +  EFI_STATUS              Status;
> +  BOOLEAN                 SourceEnabled;
> +
> +  if (TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
> +     && TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) {

One more space indentation.
Could benefit from added parentheses:

  if ((TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING) &&
      (TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH)) {

> +          DEBUG ((DEBUG_ERROR, "Invalid interrupt trigger type: %d\n", \
> +                  TriggerType));
> +          ASSERT (FALSE);
> +          return EFI_UNSUPPORTED;
> +  }
> +
> +  Status = GicGetDistributorIcfgBaseAndBit (
> +             Source,
> +             &RegAddress,
> +             &Config1Bit
> +             );
> +
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = GicV2GetInterruptSourceState (
> +             (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
> +             Source,
> +             &SourceEnabled
> +             );
> +
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Value = (TriggerType == EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING)
> +          ?  ARM_GIC_ICDICFR_EDGE_TRIGGERED
> +          :  ARM_GIC_ICDICFR_LEVEL_TRIGGERED;
> +
> +  // Before changing the value, we must disable the interrupt,
> +  // otherwise GIC behavior is UNPREDICTABLE.
> +  if (SourceEnabled) {
> +    GicV2DisableInterruptSource (
> +      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
> +      Source
> +      );
> +  }
> +
> +  MmioAndThenOr32 (
> +    RegAddress,
> +    ~(0x1 << Config1Bit),
> +    Value << Config1Bit
> +    );
> +
> +  // Restore interrupt state
> +  if (SourceEnabled) {
> +    GicV2EnableInterruptSource (
> +      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
> +      Source
> +      );
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V2Protocol = {
> +  (HARDWARE_INTERRUPT2_REGISTER)RegisterInterruptSource,
> +  (HARDWARE_INTERRUPT2_ENABLE)GicV2EnableInterruptSource,
> +  (HARDWARE_INTERRUPT2_DISABLE)GicV2DisableInterruptSource,
> +  (HARDWARE_INTERRUPT2_INTERRUPT_STATE)GicV2GetInterruptSourceState,
> +  (HARDWARE_INTERRUPT2_END_OF_INTERRUPT)GicV2EndOfInterrupt,
> +  GicV2GetTriggerType,
> +  GicV2SetTriggerType
> +};
> +
>  /**
>    Shutdown our hardware
>  
> @@ -324,6 +460,7 @@ GicV2DxeInitialize (
>  
>    Status = InstallAndRegisterInterruptService (
>               &gHardwareInterruptV2Protocol,
> +             &gHardwareInterrupt2V2Protocol,
>               GicV2IrqInterruptHandler,
>               GicV2ExitBootServicesEvent
>               );
> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> index 69b2d8d794e151e25f06cbea079e2796d9793a43..0c1d5b53119e8cad7ae67c57e9efaa51c1386be6 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> @@ -19,6 +19,7 @@
>  #define ARM_GIC_DEFAULT_PRIORITY  0x80
>  
>  extern EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV3Protocol;
> +extern EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V3Protocol;
>  
>  STATIC UINTN mGicDistributorBase;
>  STATIC UINTN mGicRedistributorsBase;
> @@ -177,7 +178,7 @@ GicV3IrqInterruptHandler (
>      // Call the registered interrupt handler.
>      InterruptHandler (GicInterrupt, SystemContext);
>    } else {
> -    DEBUG ((EFI_D_ERROR, "Spurious GIC interrupt: 0x%x\n", GicInterrupt));
> +    DEBUG ((DEBUG_ERROR, "Spurious GIC interrupt: 0x%x\n", GicInterrupt));

Non-functional change, please leave out.

>      GicV3EndOfInterrupt (&gHardwareInterruptV3Protocol, GicInterrupt);
>    }
>  }
> @@ -193,6 +194,140 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV3Protocol = {
>    GicV3EndOfInterrupt
>  };
>  
> +/**
> +  Get interrupt trigger type of an interrupt
> +
> +  @param This          Instance pointer for this protocol
> +  @param Source        Hardware source of the interrupt.
> +  @param TriggerType   Returns interrupt trigger type.
> +
> +  @retval EFI_SUCCESS       Source interrupt supported.
> +  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +GicV3GetTriggerType (
> +  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL      *This,
> +  IN  HARDWARE_INTERRUPT_SOURCE             Source,
> +  OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  *TriggerType
> +  )
> +{
> +  UINTN                   RegAddress;
> +  UINTN                   Config1Bit;
> +  EFI_STATUS              Status;
> +
> +  Status = GicGetDistributorIcfgBaseAndBit (
> +             Source,
> +             &RegAddress,
> +             &Config1Bit
> +             );
> +
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (MmioBitFieldRead32 (RegAddress, Config1Bit, Config1Bit) == 0) {

  if (MmioRead32 (RegAddress) & (1 << Config1Bit) == 0) {

?

> +     *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH;
> +  } else {
> +     *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Set interrupt trigger type of an interrupt
> +
> +  @param This          Instance pointer for this protocol
> +  @param Source        Hardware source of the interrupt.
> +  @param TriggerType   Interrupt trigger type.
> +
> +  @retval EFI_SUCCESS       Source interrupt supported.
> +  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +GicV3SetTriggerType (
> +  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL      *This,
> +  IN  HARDWARE_INTERRUPT_SOURCE             Source,
> +  IN  EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  TriggerType
> +  )
> +{
> +  UINTN                   RegAddress;
> +  UINTN                   Config1Bit;
> +  UINT32                  Value;
> +  EFI_STATUS              Status;
> +  BOOLEAN                 SourceEnabled;
> +
> +  if (TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
> +     && TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) {

Missing indentation space, and could do with added parentheses).

/
    Leif

> +          DEBUG ((DEBUG_ERROR, "Invalid interrupt trigger type: %d\n", \
> +                 TriggerType));
> +          ASSERT (FALSE);
> +          return EFI_UNSUPPORTED;
> +  }
> +
> +  Status = GicGetDistributorIcfgBaseAndBit (
> +             Source,
> +             &RegAddress,
> +             &Config1Bit
> +             );
> +
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = GicV3GetInterruptSourceState (
> +             (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
> +             Source,
> +             &SourceEnabled
> +             );
> +
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Value = (TriggerType == EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING)
> +          ?  ARM_GIC_ICDICFR_EDGE_TRIGGERED
> +          :  ARM_GIC_ICDICFR_LEVEL_TRIGGERED;
> +
> +  // Before changing the value, we must disable the interrupt,
> +  // otherwise GIC behavior is UNPREDICTABLE.
> +  if (SourceEnabled) {
> +    GicV3DisableInterruptSource (
> +      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
> +      Source
> +      );
> +  }
> +
> +  MmioAndThenOr32 (
> +    RegAddress,
> +    ~(0x1 << Config1Bit),
> +    Value << Config1Bit
> +    );
> +  // Restore interrupt state
> +  if (SourceEnabled) {
> +    GicV3EnableInterruptSource (
> +      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
> +      Source
> +      );
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V3Protocol = {
> +  (HARDWARE_INTERRUPT2_REGISTER)RegisterInterruptSource,
> +  (HARDWARE_INTERRUPT2_ENABLE)GicV3EnableInterruptSource,
> +  (HARDWARE_INTERRUPT2_DISABLE)GicV3DisableInterruptSource,
> +  (HARDWARE_INTERRUPT2_INTERRUPT_STATE)GicV3GetInterruptSourceState,
> +  (HARDWARE_INTERRUPT2_END_OF_INTERRUPT)GicV3EndOfInterrupt,
> +  GicV3GetTriggerType,
> +  GicV3SetTriggerType
> +};
> +
>  /**
>    Shutdown our hardware
>  
> @@ -354,6 +489,7 @@ GicV3DxeInitialize (
>  
>    Status = InstallAndRegisterInterruptService (
>               &gHardwareInterruptV3Protocol,
> +             &gHardwareInterrupt2V3Protocol,
>               GicV3IrqInterruptHandler,
>               GicV3ExitBootServicesEvent
>               );
> -- 
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> 


  reply	other threads:[~2017-09-14 16:57 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-11 15:23 [PATCH 0/5] Add HardwareInterrupt2 for ARM evan.lloyd
2017-09-11 15:23 ` [PATCH 1/5] ArmPkg: Tidy GIC code before changes evan.lloyd
2017-09-14 16:41   ` Leif Lindholm
2017-09-21 15:34     ` Evan Lloyd
2017-09-21 17:43       ` Leif Lindholm
2017-09-11 15:23 ` [PATCH 2/5] EmbeddedPkg: Introduce HardwareInterrupt2 protocol evan.lloyd
2017-09-14 16:42   ` Leif Lindholm
2017-09-15  9:21     ` Alexei Fedorov
2017-09-11 15:23 ` [PATCH 3/5] ArmPkg/ArmGicDxe: Expose " evan.lloyd
2017-09-14 17:00   ` Leif Lindholm [this message]
2017-09-11 15:23 ` [PATCH 4/5] ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type evan.lloyd
2017-09-14 17:06   ` Leif Lindholm
2017-09-11 15:23 ` [PATCH 5/5] ArmPkg: Tidy up GenericWatchdogDxe.c evan.lloyd
2017-09-14 17:10   ` Leif Lindholm
  -- strict thread matches above, loose matches on Subject: below --
2017-02-16 22:14 [PATCH 0/5] HardwareInterrupt2 protocol evan.lloyd
2017-02-16 22:14 ` [PATCH 3/5] ArmPkg/ArmGicDxe: Expose " evan.lloyd

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170914170025.ywvthusrop42uukd@bivouac.eciton.net \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox