Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions

[Evan Lloyd <Evan.Lloyd@arm.com>]

Hi Ard.
Your comments make sense and we will comply (including "Please no spaces after casts").
However, I can find nothing requiring no space after casts in the CCS.
It does have some casts in example code:
5.7.2.3 has "if ((INTN)foo >= 0)" with no space
5.7.2.4 has "if (( LogEntryArray[Index].Handle == (EFI_PHYSICAL_ADDRESS) (UINTN) Handle)" with spaces (and others).

By the standard edk2 process we should determine which is the most popular and insist on the reverse.  :-)

Regards,
Evan

>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: 13 February 2017 13:05
>To: Evan Lloyd
>Cc: edk2-devel@lists.01.org; Leif Lindholm; ryan.harkin@linaro.org
>Subject: Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType
>functions
>
>(apologies for the delayed [and now somewhat redundant] response, this
>sat in my outbox since this morning)
>
>On 9 February 2017 at 19:26,  <evan.lloyd@arm.com> wrote:
>> From: Girish Pathak <girish.pathak@arm.com>
>>
>> This change implements GetTriggerType and SetTriggerType functions
>> in ArmGicV2Dxe (GicV2GetTriggerType/GicV2SetTriggerType)
>> and ArmGicV3Dxe (GicV3GetTriggerType/GicV3SetTriggerType)
>>
>> SetTriggerType configures the interrupt mode of an interrupt
>> as edge sensitive or level sensitive.
>>
>> GetTriggerType function returns the mode of an interrupt.
>>
>> The requirement for this change derives from a problem detected on
>ARM
>> Juno boards, but the change is of generic relevance.
>>
>> NOTE: At this point the GICv3 code is not tested.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> 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>
>
>It's probably best to reorder this patch with #3, or perhaps fold it
>into #2 entirely.
>
>> ---
>>  ArmPkg/Include/Library/ArmGicLib.h        |   4 +
>>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 165
>++++++++++++++++++--
>>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 159
>+++++++++++++++++--
>>  3 files changed, 308 insertions(+), 20 deletions(-)
>>
>> diff --git a/ArmPkg/Include/Library/ArmGicLib.h
>b/ArmPkg/Include/Library/ArmGicLib.h
>> index
>4364f3ffef464596f64cf59881d703cf54cf0ddd..6610f356c20e73d84ff3ba51995
>6b426d97ef1eb 100644
>> --- a/ArmPkg/Include/Library/ArmGicLib.h
>> +++ b/ArmPkg/Include/Library/ArmGicLib.h
>> @@ -51,6 +51,10 @@
>>  #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_LEVEL_TRIGGERED  0x0 // Level triggered
>interrupt
>> +#define ARM_GIC_ICDICFR_EDGE_TRIGGERED   0x1 // Edge triggered
>interrupt
>> +
>>  //
>>  // GIC Redistributor
>>  //
>> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>> index
>8c4d66125e2e8c7af9898f336ee742ed0aebf058..1f47403c6cdc7e8c0f6ac65d3
>b95a562da6a2d32 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;
>> @@ -193,19 +194,95 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL
>gHardwareInterruptV2Protocol = {
>>    GicV2EndOfInterrupt
>>  };
>>
>> +/**
>> +  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 BitNumber    Bit number in the register to set/reset.
>> +
>> +  @retval EFI_SUCCESS       Source interrupt supported.
>> +  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
>> +**/
>>  STATIC
>>  EFI_STATUS
>> +GicGetDistributorIntrCfgBaseAndBitField (
>> +  IN HARDWARE_INTERRUPT_SOURCE             Source,
>> +  OUT UINTN                               *RegAddress,
>> +  OUT UINTN                               *BitNumber
>> +  )
>> +{
>> +  UINTN                  RegOffset;
>> +  UINTN                  Field;
>> +
>> +  if (Source >= mGicNumInterrupts) {
>> +    ASSERT(Source < mGicNumInterrupts);
>> +    return EFI_UNSUPPORTED;
>> +  }
>> +
>> +  RegOffset = Source / 16;
>> +  Field = Source % 16;
>> +  *RegAddress = PcdGet64 (PcdGicDistributorBase)
>> +                  + ARM_GIC_ICDICFR
>> +                  + (4 * RegOffset);
>> +  *BitNumber = (Field * 2) + 1;
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +/**
>> +  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.
>> +**/
>> +EFI_STATUS
>
>STATIC ?
>
>>  EFIAPI
>>  GicV2GetTriggerType (
>>    IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL      *This,
>> -  IN  HARDWARE_INTERRUPT_SOURCE             Source,
>> +  IN  HARDWARE_INTERRUPT_SOURCE              Source,
>>    OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  *TriggerType
>>    )
>>  {
>> +  UINTN                   RegAddress;
>> +  UINTN                   BitNumber;
>> +  EFI_STATUS              Status;
>> +
>> +  RegAddress = 0;
>> +  BitNumber  = 0;
>> +
>> +  Status = GicGetDistributorIntrCfgBaseAndBitField (
>> +              Source,
>> +              &RegAddress,
>> +              &BitNumber
>> +              );
>> +
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  *TriggerType = (MmioBitFieldRead32 (RegAddress, BitNumber,
>BitNumber) == 0)
>> +                 ?  EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH
>> +                 :  EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
>> +
>>    return EFI_SUCCESS;
>>  }
>>
>> -STATIC
>
>?
>
>> +/**
>> +  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.
>> +**/
>>  EFI_STATUS
>>  EFIAPI
>>  GicV2SetTriggerType (
>> @@ -214,20 +291,83 @@ GicV2SetTriggerType (
>>    IN  EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  TriggerType
>>    )
>>  {
>> +  UINTN                   RegAddress = 0;
>> +  UINTN                   BitNumber = 0;
>> +  UINT32                  Value;
>> +  EFI_STATUS              Status;
>> +  BOOLEAN                 IntrSourceEnabled;
>> +
>> +  if (TriggerType !=
>EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
>> +     && TriggerType !=
>EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) {
>> +          DEBUG ((EFI_D_ERROR, "Invalid interrupt trigger type: %d\n", \
>> +                  TriggerType));
>> +          ASSERT (FALSE);
>> +          return EFI_UNSUPPORTED;
>> +  }
>> +
>> +  Status = GicGetDistributorIntrCfgBaseAndBitField (
>> +             Source,
>> +             &RegAddress,
>> +             &BitNumber
>> +             );
>> +
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  Status = GicV2GetInterruptSourceState (
>> +             (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
>> +             Source,
>> +             &IntrSourceEnabled
>> +             );
>> +
>> +  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 (IntrSourceEnabled) {
>> +    GicV2DisableInterruptSource (
>> +      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
>> +      Source
>> +      );
>> +  }
>> +
>> +  MmioAndThenOr32 (
>> +    RegAddress,
>> +    ~(0x1 << BitNumber),
>> +    Value << BitNumber
>> +    );
>> +  //
>> +  // Restore interrupt state
>> +  //
>> +  if (IntrSourceEnabled) {
>> +    GicV2EnableInterruptSource (
>> +      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
>> +      Source
>> +      );
>> +  }
>> +
>>    return EFI_SUCCESS;
>>  }
>>
>> -STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL
>gHardwareInterrupt2V2Protocol = {
>> -  (HARDWARE_INTERRUPT2_REGISTER)RegisterInterruptSource,
>> -  (HARDWARE_INTERRUPT2_ENABLE)GicV2EnableInterruptSource,
>> -  (HARDWARE_INTERRUPT2_DISABLE)GicV2DisableInterruptSource,
>> -
>(HARDWARE_INTERRUPT2_INTERRUPT_STATE)GicV2GetInterruptSourceSta
>te,
>> -  (HARDWARE_INTERRUPT2_END_OF_INTERRUPT)GicV2EndOfInterrupt,
>> +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,
>
>Please no spaces after casts
>
>>    GicV2GetTriggerType,
>>    GicV2SetTriggerType
>>  };
>>
>> -
>
>Spurious whitespace change
>
>>  /**
>>    Shutdown our hardware
>>
>> @@ -346,8 +486,11 @@ GicV2DxeInitialize (
>>    ArmGicEnableDistributor (mGicDistributorBase);
>>
>>    Status = InstallAndRegisterInterruptService (
>> -             &gHardwareInterruptV2Protocol,
>&gHardwareInterrupt2V2Protocol,
>> -             GicV2IrqInterruptHandler, GicV2ExitBootServicesEvent);
>> +             &gHardwareInterruptV2Protocol,
>> +             &gHardwareInterrupt2V2Protocol,
>> +             GicV2IrqInterruptHandler,
>> +             GicV2ExitBootServicesEvent
>> +             );
>>
>
>Spurious whitespace change
>
>>    return Status;
>>  }
>> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> index
>02deeef78b6d7737172a5992c6decac43cfdd64a..a0383ecd7738750f73a225381
>1403d6ed0d2fd51 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;
>> @@ -184,8 +185,54 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL
>gHardwareInterruptV3Protocol = {
>>    GicV3EndOfInterrupt
>>  };
>>
>> +/**
>> +  Calculate GICD_ICFGRn base address and corresponding bit
>> +  field Int_config[1] in the GIC distributor register.
>> +
>> +  @param Source       Hardware source of the interrupt.
>> +  @param RegAddress   Corresponding GICD_ICFGRn base address.
>> +  @param BitNumber    Bit number in the register to set/reset.
>> +
>> +  @retval EFI_SUCCESS       Source interrupt supported.
>> +  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
>> +**/
>>  STATIC
>>  EFI_STATUS
>> +GicGetDistributorIntrCfgBaseAndBitField (
>> +  IN HARDWARE_INTERRUPT_SOURCE              Source,
>> +  OUT UINTN                                *RegAddress,
>> +  OUT UINTN                                *BitNumber
>> +  )
>> +{
>> +  UINTN                   RegOffset;
>> +  UINTN                   Field;
>> +
>> +  if (Source >= mGicNumInterrupts) {
>> +    ASSERT(FALSE);
>> +    return EFI_UNSUPPORTED;
>> +  }
>> +
>> +  RegOffset = Source / 16;
>> +  Field = Source % 16;
>> +  *RegAddress = PcdGet64 (PcdGicDistributorBase)
>> +                  + ARM_GIC_ICDICFR
>> +                  + (4 * RegOffset);
>> +  *BitNumber = (Field * 2) + 1;
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +/**
>> +  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.
>> +**/
>> +EFI_STATUS
>
>STATIC ?
>
>>  EFIAPI
>>  GicV3GetTriggerType (
>>    IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL      *This,
>> @@ -193,10 +240,37 @@ GicV3GetTriggerType (
>>    OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  *TriggerType
>>    )
>>  {
>> +  UINTN                   RegAddress = 0;
>> +  UINTN                   BitNumber = 0;
>> +  EFI_STATUS              Status;
>> +
>> +  Status = GicGetDistributorIntrCfgBaseAndBitField (
>> +             Source,
>> +             &RegAddress,
>> +             &BitNumber
>> +             );
>> +
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  *TriggerType = (MmioBitFieldRead32 (RegAddress, BitNumber,
>BitNumber) == 0)
>> +                 ?  EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH
>> +                 :  EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
>> +
>>    return EFI_SUCCESS;
>>  }
>>
>> -STATIC
>> +/**
>> +  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.
>> +**/
>>  EFI_STATUS
>>  EFIAPI
>>  GicV3SetTriggerType (
>> @@ -205,15 +279,79 @@ GicV3SetTriggerType (
>>    IN  EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  TriggerType
>>    )
>>  {
>> +  UINTN                   RegAddress;
>> +  UINTN                   BitNumber;
>> +  UINT32                  Value;
>> +  EFI_STATUS              Status;
>> +  BOOLEAN                 IntrSourceEnabled;
>> +
>> +  if (TriggerType !=
>EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
>> +     && TriggerType !=
>EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) {
>> +          DEBUG ((EFI_D_ERROR, "Invalid interrupt trigger type: %d\n", \
>> +                 TriggerType));
>> +          ASSERT (FALSE);
>> +          return EFI_UNSUPPORTED;
>> +  }
>> +
>> +  Status = GicGetDistributorIntrCfgBaseAndBitField (
>> +             Source,
>> +             &RegAddress,
>> +             &BitNumber
>> +             );
>> +
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  Status = GicV3GetInterruptSourceState (
>> +             (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
>> +             Source,
>> +             &IntrSourceEnabled
>> +             );
>> +
>> +  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 (IntrSourceEnabled) {
>> +    GicV3DisableInterruptSource (
>> +      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
>> +      Source
>> +      );
>> +  }
>> +
>> +  MmioAndThenOr32 (
>> +    RegAddress,
>> +    ~(0x1 << BitNumber),
>> +    Value << BitNumber
>> +    );
>> +  //
>> +  // Restore interrupt state
>> +  //
>> +  if (IntrSourceEnabled) {
>> +    GicV3EnableInterruptSource (
>> +      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
>> +      Source
>> +      );
>> +  }
>> +
>>    return EFI_SUCCESS;
>>  }
>>
>> -STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL
>gHardwareInterrupt2V3Protocol = {
>> -  (HARDWARE_INTERRUPT2_REGISTER)RegisterInterruptSource,
>> -  (HARDWARE_INTERRUPT2_ENABLE)GicV3EnableInterruptSource,
>> -  (HARDWARE_INTERRUPT2_DISABLE)GicV3DisableInterruptSource,
>> -
>(HARDWARE_INTERRUPT2_INTERRUPT_STATE)GicV3GetInterruptSourceSta
>te,
>> -  (HARDWARE_INTERRUPT2_END_OF_INTERRUPT)GicV3EndOfInterrupt,
>> +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
>>  };
>> @@ -365,8 +503,11 @@ GicV3DxeInitialize (
>>    ArmGicEnableDistributor (mGicDistributorBase);
>>
>>    Status = InstallAndRegisterInterruptService (
>> -             &gHardwareInterruptV3Protocol,
>&gHardwareInterrupt2V3Protocol,
>> -             GicV3IrqInterruptHandler, GicV3ExitBootServicesEvent);
>> +             &gHardwareInterruptV3Protocol,
>> +             &gHardwareInterrupt2V3Protocol,
>> +             GicV3IrqInterruptHandler,
>> +             GicV3ExitBootServicesEvent
>> +             );
>>
>>    return Status;
>>  }
>> --
>> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.