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>
---
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..6610f356c20e73d84ff3ba519956b426d97ef1eb 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..1f47403c6cdc7e8c0f6ac65d3b95a562da6a2d32 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
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)GicV2GetInterruptSourceState,
- (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,
GicV2GetTriggerType,
GicV2SetTriggerType
};
-
/**
Shutdown our hardware
@@ -346,8 +486,11 @@ GicV2DxeInitialize (
ArmGicEnableDistributor (mGicDistributorBase);
Status = InstallAndRegisterInterruptService (
- &gHardwareInterruptV2Protocol, &gHardwareInterrupt2V2Protocol,
- GicV2IrqInterruptHandler, GicV2ExitBootServicesEvent);
+ &gHardwareInterruptV2Protocol,
+ &gHardwareInterrupt2V2Protocol,
+ GicV2IrqInterruptHandler,
+ GicV2ExitBootServicesEvent
+ );
return Status;
}
diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
index 02deeef78b6d7737172a5992c6decac43cfdd64a..a0383ecd7738750f73a2253811403d6ed0d2fd51 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
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)GicV3GetInterruptSourceState,
- (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")
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On Thu, Feb 09, 2017 at 07:26:23PM +0000, 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>
(Tested-by: is usually considered to be implicit from the code author
- its value comes when added by someone else.)
> ---
> 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..6610f356c20e73d84ff3ba519956b426d97ef1eb 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..1f47403c6cdc7e8c0f6ac65d3b95a562da6a2d32 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 (
I don't see Interrupt generally truncated to Intr in this driver.
Since what is being looked for is the the ICFG, why not just call it
GicGetDistributorICfgBaseAndBitField?
> + 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;
A lot of magic values above - can this be improved with some added
#defines in ArmGicLib.h?
> +
> + 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
> EFIAPI
> GicV2GetTriggerType (
> IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> - IN HARDWARE_INTERRUPT_SOURCE Source,
> + IN HARDWARE_INTERRUPT_SOURCE Source,
Cosmetic whitespace change only.
> 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;
> +
Ternaries are excellent when they increase code readability.
I am not convinced that is the case here.
Consider:
if (MmioBitFieldRead32 (RegAddress, BitNumber, BitNumber) == 0) {
*TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH;
} else {
*TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
}
?
The versions generate identical code with gcc at -O1 and above.
> 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;
"Interrupt" isn't truncated in variable/function names elsewhere in
this function. If you want to abbreviate the name - how about just
calling it SourceEnabled?
> +
> + if (TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
> + && TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) {
Should that && not line up with the 'T' rather than the '('?
> + 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;
Again, consider if/else.
> +
> + //
> + // 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)GicV2GetInterruptSourceState,
> - (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,
Apart from the dropped STATIC, This is a cosmetic whitespace-only
change that also seems incorrect to me.
> GicV2GetTriggerType,
> GicV2SetTriggerType
> };
>
> -
> /**
> Shutdown our hardware
>
> @@ -346,8 +486,11 @@ GicV2DxeInitialize (
> ArmGicEnableDistributor (mGicDistributorBase);
>
> Status = InstallAndRegisterInterruptService (
> - &gHardwareInterruptV2Protocol, &gHardwareInterrupt2V2Protocol,
> - GicV2IrqInterruptHandler, GicV2ExitBootServicesEvent);
> + &gHardwareInterruptV2Protocol,
> + &gHardwareInterrupt2V2Protocol,
> + GicV2IrqInterruptHandler,
> + GicV2ExitBootServicesEvent
This is a whitespace-only change.
> + );
>
> return Status;
> }
> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> index 02deeef78b6d7737172a5992c6decac43cfdd64a..a0383ecd7738750f73a2253811403d6ed0d2fd51 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 (
ICfg?
> + 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;
A lot of magic numbers above. Can they be improved with some added
#defines in ArmGicLib.h?
> +
> + 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
> 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;
> +
Consider if/else?
> 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;
Consider SourceEnabled?
> +
> + if (TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
> + && TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) {
Line up with 'T' instead of '('?
> + DEBUG ((EFI_D_ERROR, "Invalid interrupt trigger type: %d\n", \
> + TriggerType));
This lines up differently from in the v2 driver (which I would argue
gets it right).
> + 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;
Consider if/else?
> +
> + //
> + // 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)GicV3GetInterruptSourceState,
> - (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,
Apart from dropped STATIC, a whitespace-only change.
> GicV3GetTriggerType,
> GicV3SetTriggerType
> };
> @@ -365,8 +503,11 @@ GicV3DxeInitialize (
> ArmGicEnableDistributor (mGicDistributorBase);
>
> Status = InstallAndRegisterInterruptService (
> - &gHardwareInterruptV3Protocol, &gHardwareInterrupt2V3Protocol,
> - GicV3IrqInterruptHandler, GicV3ExitBootServicesEvent);
> + &gHardwareInterruptV3Protocol,
> + &gHardwareInterrupt2V3Protocol,
> + GicV3IrqInterruptHandler,
> + GicV3ExitBootServicesEvent
> + );
Whitespace-only change.
>
> return Status;
> }
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Leif.
We accept all the comments except that about ternaries.
Response inline.
>-----Original Message-----
>From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>Sent: 13 February 2017 12:16
>To: Evan Lloyd
>Cc: edk2-devel@ml01.01.org; ard.biesheuvel@linaro.org;
>ryan.harkin@linaro.org
>Subject: Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType
>functions
>
>On Thu, Feb 09, 2017 at 07:26:23PM +0000, 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>
>
>(Tested-by: is usually considered to be implicit from the code author
>- its value comes when added by someone else.)
>
>> ---
>> 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 (
>
>I don't see Interrupt generally truncated to Intr in this driver.
>Since what is being looked for is the the ICFG, why not just call it
>GicGetDistributorICfgBaseAndBitField?
>
>> + 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;
>
>A lot of magic values above - can this be improved with some added
>#defines in ArmGicLib.h?
>
>> +
>> + 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
>> EFIAPI
>> GicV2GetTriggerType (
>> IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
>> - IN HARDWARE_INTERRUPT_SOURCE Source,
>> + IN HARDWARE_INTERRUPT_SOURCE Source,
>
>Cosmetic whitespace change only.
>
>> 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;
>> +
>
>Ternaries are excellent when they increase code readability.
>I am not convinced that is the case here.
>
>Consider:
>
> if (MmioBitFieldRead32 (RegAddress, BitNumber, BitNumber) == 0) {
> *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH;
> } else {
> *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
> }
>
>?
>
>The versions generate identical code with gcc at -O1 and above.
Firstly, I'm not sure why 5 lines of code is more readable than 3.
My main point though is that the ternary expression clearly indicates that all we are doing is setting *TriggerType.
The multiple assignment "if" requires examination to determine that there is nothing else going on. (Because otherwise why wouldn't you use a ternary?)
I'm about to submit v2 without this, in the hope that I've made the case.
>
>> 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;
>
>"Interrupt" isn't truncated in variable/function names elsewhere in
>this function. If you want to abbreviate the name - how about just
>calling it SourceEnabled?
>
>> +
>> + if (TriggerType !=
>EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
>> + && TriggerType !=
>EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) {
>
>Should that && not line up with the 'T' rather than the '('?
>
>> + 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;
>
>Again, consider if/else.
>
>> +
>> + //
>> + // 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,
>
>Apart from the dropped STATIC, This is a cosmetic whitespace-only
>change that also seems incorrect to me.
>
>> GicV2GetTriggerType,
>> GicV2SetTriggerType
>> };
>>
>> -
>> /**
>> Shutdown our hardware
>>
>> @@ -346,8 +486,11 @@ GicV2DxeInitialize (
>> ArmGicEnableDistributor (mGicDistributorBase);
>>
>> Status = InstallAndRegisterInterruptService (
>> - &gHardwareInterruptV2Protocol,
>&gHardwareInterrupt2V2Protocol,
>> - GicV2IrqInterruptHandler, GicV2ExitBootServicesEvent);
>> + &gHardwareInterruptV2Protocol,
>> + &gHardwareInterrupt2V2Protocol,
>> + GicV2IrqInterruptHandler,
>> + GicV2ExitBootServicesEvent
>
>This is a whitespace-only 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 (
>
>ICfg?
>
>> + 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;
>
>A lot of magic numbers above. Can they be improved with some added
>#defines in ArmGicLib.h?
>
>> +
>> + 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
>> 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;
>> +
>
>Consider if/else?
>
>> 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;
>
>Consider SourceEnabled?
>
>> +
>> + if (TriggerType !=
>EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
>> + && TriggerType !=
>EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) {
>
>Line up with 'T' instead of '('?
>
>> + DEBUG ((EFI_D_ERROR, "Invalid interrupt trigger type: %d\n", \
>> + TriggerType));
>
>This lines up differently from in the v2 driver (which I would argue
>gets it right).
>
>> + 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;
>
>Consider if/else?
>
>> +
>> + //
>> + // 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,
>
>Apart from dropped STATIC, a whitespace-only change.
>
>> GicV3GetTriggerType,
>> GicV3SetTriggerType
>> };
>> @@ -365,8 +503,11 @@ GicV3DxeInitialize (
>> ArmGicEnableDistributor (mGicDistributorBase);
>>
>> Status = InstallAndRegisterInterruptService (
>> - &gHardwareInterruptV3Protocol,
>&gHardwareInterrupt2V3Protocol,
>> - GicV3IrqInterruptHandler, GicV3ExitBootServicesEvent);
>> + &gHardwareInterruptV3Protocol,
>> + &gHardwareInterrupt2V3Protocol,
>> + GicV3IrqInterruptHandler,
>> + GicV3ExitBootServicesEvent
>> + );
>
>Whitespace-only change.
>
>>
>> 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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 16 Feb 2017 20:27, "Evan Lloyd" <Evan.Lloyd@arm.com> wrote:
>
> Hi Leif.
> We accept all the comments except that about ternaries.
> Response inline.
>
> >-----Original Message-----
> >From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> >Sent: 13 February 2017 12:16
> >To: Evan Lloyd
> >Cc: edk2-devel@ml01.01.org; ard.biesheuvel@linaro.org;
> >ryan.harkin@linaro.org
> >Subject: Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType
> >functions
> >
> >On Thu, Feb 09, 2017 at 07:26:23PM +0000, 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>
> >
> >(Tested-by: is usually considered to be implicit from the code author
> >- its value comes when added by someone else.)
> >
> >> ---
> >> 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 (
> >
> >I don't see Interrupt generally truncated to Intr in this driver.
> >Since what is being looked for is the the ICFG, why not just call it
> >GicGetDistributorICfgBaseAndBitField?
> >
> >> + 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;
> >
> >A lot of magic values above - can this be improved with some added
> >#defines in ArmGicLib.h?
> >
> >> +
> >> + 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
> >> EFIAPI
> >> GicV2GetTriggerType (
> >> IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> >> - IN HARDWARE_INTERRUPT_SOURCE Source,
> >> + IN HARDWARE_INTERRUPT_SOURCE Source,
> >
> >Cosmetic whitespace change only.
> >
> >> 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;
> >> +
> >
> >Ternaries are excellent when they increase code readability.
> >I am not convinced that is the case here.
> >
> >Consider:
> >
> > if (MmioBitFieldRead32 (RegAddress, BitNumber, BitNumber) == 0) {
> > *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH;
> > } else {
> > *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
> > }
> >
> >?
> >
> >The versions generate identical code with gcc at -O1 and above.
>
> Firstly, I'm not sure why 5 lines of code is more readable than 3.
> My main point though is that the ternary expression clearly indicates
that all we are doing is setting *TriggerType.
> The multiple assignment "if" requires examination to determine that there
is nothing else going on. (Because otherwise why wouldn't you use a
ternary?)
> I'm about to submit v2 without this, in the hope that I've made the case.
>
I find your argument unconvincing and would prefer an "if" clause.
> >
> >> 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;
> >
> >"Interrupt" isn't truncated in variable/function names elsewhere in
> >this function. If you want to abbreviate the name - how about just
> >calling it SourceEnabled?
> >
> >> +
> >> + if (TriggerType !=
> >EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
> >> + && TriggerType !=
> >EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) {
> >
> >Should that && not line up with the 'T' rather than the '('?
> >
> >> + 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;
> >
> >Again, consider if/else.
> >
> >> +
> >> + //
> >> + // 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,
> >
> >Apart from the dropped STATIC, This is a cosmetic whitespace-only
> >change that also seems incorrect to me.
> >
> >> GicV2GetTriggerType,
> >> GicV2SetTriggerType
> >> };
> >>
> >> -
> >> /**
> >> Shutdown our hardware
> >>
> >> @@ -346,8 +486,11 @@ GicV2DxeInitialize (
> >> ArmGicEnableDistributor (mGicDistributorBase);
> >>
> >> Status = InstallAndRegisterInterruptService (
> >> - &gHardwareInterruptV2Protocol,
> >&gHardwareInterrupt2V2Protocol,
> >> - GicV2IrqInterruptHandler, GicV2ExitBootServicesEvent);
> >> + &gHardwareInterruptV2Protocol,
> >> + &gHardwareInterrupt2V2Protocol,
> >> + GicV2IrqInterruptHandler,
> >> + GicV2ExitBootServicesEvent
> >
> >This is a whitespace-only 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 (
> >
> >ICfg?
> >
> >> + 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;
> >
> >A lot of magic numbers above. Can they be improved with some added
> >#defines in ArmGicLib.h?
> >
> >> +
> >> + 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
> >> 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;
> >> +
> >
> >Consider if/else?
> >
> >> 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;
> >
> >Consider SourceEnabled?
> >
> >> +
> >> + if (TriggerType !=
> >EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
> >> + && TriggerType !=
> >EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) {
> >
> >Line up with 'T' instead of '('?
> >
> >> + DEBUG ((EFI_D_ERROR, "Invalid interrupt trigger type:
%d\n", \
> >> + TriggerType));
> >
> >This lines up differently from in the v2 driver (which I would argue
> >gets it right).
> >
> >> + 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;
> >
> >Consider if/else?
> >
> >> +
> >> + //
> >> + // 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,
> >
> >Apart from dropped STATIC, a whitespace-only change.
> >
> >> GicV3GetTriggerType,
> >> GicV3SetTriggerType
> >> };
> >> @@ -365,8 +503,11 @@ GicV3DxeInitialize (
> >> ArmGicEnableDistributor (mGicDistributorBase);
> >>
> >> Status = InstallAndRegisterInterruptService (
> >> - &gHardwareInterruptV3Protocol,
> >&gHardwareInterrupt2V3Protocol,
> >> - GicV3IrqInterruptHandler, GicV3ExitBootServicesEvent);
> >> + &gHardwareInterruptV3Protocol,
> >> + &gHardwareInterrupt2V3Protocol,
> >> + GicV3IrqInterruptHandler,
> >> + GicV3ExitBootServicesEvent
> >> + );
> >
> >Whitespace-only change.
> >
> >>
> >> 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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Ryan.
From: Ryan Harkin [mailto:ryan.harkin@linaro.org]
Sent: 16 February 2017 20:42
To: Evan Lloyd
Cc: ard.biesheuvel@linaro.org; Leif Lindholm; edk2-devel@ml01.01.org
Subject: RE: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
On 16 Feb 2017 20:27, "Evan Lloyd" <Evan.Lloyd@arm.com<mailto:Evan.Lloyd@arm.com>> wrote:
>
> Hi Leif.
> We accept all the comments except that about ternaries.
> Response inline.
>
…
> >> +
> >> + *TriggerType = (MmioBitFieldRead32 (RegAddress, BitNumber,
> >BitNumber) == 0)
> >> + ? EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH
> >> + : EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
> >> +
> >
> >Ternaries are excellent when they increase code readability.
> >I am not convinced that is the case here.
> >
> >Consider:
> >
> > if (MmioBitFieldRead32 (RegAddress, BitNumber, BitNumber) == 0) {
> > *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH;
> > } else {
> > *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
> > }
> >
> >?
> >
> >The versions generate identical code with gcc at -O1 and above.
>
> Firstly, I'm not sure why 5 lines of code is more readable than 3.
> My main point though is that the ternary expression clearly indicates that all we are doing is setting *TriggerType.
> The multiple assignment "if" requires examination to determine that there is nothing else going on. (Because otherwise why wouldn't you use a ternary?)
> I'm about to submit v2 without this, in the hope that I've made the case.
>
I find your argument unconvincing and would prefer an "if" clause.
That is fine, lots of people have irrational prejudices. ;-)
Do you have a counter argument though?
Leif points out that “Ternaries are excellent when they increase code readability.”
The debate would thus seem to be a subjective assessment of “readability”.
What criteria should we use to identify when a ternary is more readable, and when not?
And how do we ensure consistency across all EDK2 maintainers?
Evan
…
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 17 February 2017 at 12:06, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
> Hi Ryan.
>
>
>
> From: Ryan Harkin [mailto:ryan.harkin@linaro.org]
> Sent: 16 February 2017 20:42
> To: Evan Lloyd
> Cc: ard.biesheuvel@linaro.org; Leif Lindholm; edk2-devel@ml01.01.org
> Subject: RE: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType
> functions
>
>
>
>
> On 16 Feb 2017 20:27, "Evan Lloyd" <Evan.Lloyd@arm.com> wrote:
>>
>> Hi Leif.
>> We accept all the comments except that about ternaries.
>> Response inline.
>>
> …
>
>> >> +
>> >> + *TriggerType = (MmioBitFieldRead32 (RegAddress, BitNumber,
>> >BitNumber) == 0)
>> >> + ? EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH
>> >> + : EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
>> >> +
>> >
>> >Ternaries are excellent when they increase code readability.
>> >I am not convinced that is the case here.
>> >
>> >Consider:
>> >
>> > if (MmioBitFieldRead32 (RegAddress, BitNumber, BitNumber) == 0) {
>> > *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH;
>> > } else {
>> > *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
>> > }
>> >
>> >?
>> >
>> >The versions generate identical code with gcc at -O1 and above.
>>
>> Firstly, I'm not sure why 5 lines of code is more readable than 3.
>> My main point though is that the ternary expression clearly indicates
>> that all we are doing is setting *TriggerType.
>> The multiple assignment "if" requires examination to determine that there
>> is nothing else going on. (Because otherwise why wouldn't you use a
>> ternary?)
>> I'm about to submit v2 without this, in the hope that I've made the case.
>>
>
> I find your argument unconvincing and would prefer an "if" clause.
>
> That is fine, lots of people have irrational prejudices. ;-)
>
> Do you have a counter argument though?
>
I don't think I need a 3rd argument. Like Leif, I don't think that
ternary adds clarity and think an '"if" statement would be easier to
decipher.
> Leif points out that “Ternaries are excellent when they increase code
> readability.”
>
> The debate would thus seem to be a subjective assessment of “readability”.
>
Indeed it is.
> What criteria should we use to identify when a ternary is more readable, and
> when not?
>
> And how do we ensure consistency across all EDK2 maintainers?
>
None of that is down to me.
Cheers,
Ryan.
> Evan
>
>
>
> …
>
>
>
> 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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Please take a look at the following code in GenericWatchdogEntry():
Status = mInterruptProtocol->RegisterInterruptSource (
mInterruptProtocol,
FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
WatchdogInterruptHandler
);
if (!EFI_ERROR (Status)) {
Status = mInterruptProtocol->SetTriggerType (
mInterruptProtocol,
FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING);
Why can't we extend EFI_HARDWARE_INTERRUPT2_PROTOCOL RegisterInterruptSource() function API to have an extra EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType parameter to set interrupt type & get rid of the second call?
Alexei.
________________________________
From: edk2-devel <edk2-devel-bounces@lists.01.org> on behalf of Ryan Harkin <ryan.harkin@linaro.org>
Sent: 17 February 2017 12:30:13
To: Evan Lloyd
Cc: edk2-devel@ml01.01.org; Leif Lindholm; ard.biesheuvel@linaro.org
Subject: Re: [edk2] [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
On 17 February 2017 at 12:06, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
> Hi Ryan.
>
>
>
> From: Ryan Harkin [mailto:ryan.harkin@linaro.org]
> Sent: 16 February 2017 20:42
> To: Evan Lloyd
> Cc: ard.biesheuvel@linaro.org; Leif Lindholm; edk2-devel@ml01.01.org
> Subject: RE: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType
> functions
>
>
>
>
> On 16 Feb 2017 20:27, "Evan Lloyd" <Evan.Lloyd@arm.com> wrote:
>>
>> Hi Leif.
>> We accept all the comments except that about ternaries.
>> Response inline.
>>
> …
>
>> >> +
>> >> + *TriggerType = (MmioBitFieldRead32 (RegAddress, BitNumber,
>> >BitNumber) == 0)
>> >> + ? EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH
>> >> + : EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
>> >> +
>> >
>> >Ternaries are excellent when they increase code readability.
>> >I am not convinced that is the case here.
>> >
>> >Consider:
>> >
>> > if (MmioBitFieldRead32 (RegAddress, BitNumber, BitNumber) == 0) {
>> > *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH;
>> > } else {
>> > *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
>> > }
>> >
>> >?
>> >
>> >The versions generate identical code with gcc at -O1 and above.
>>
>> Firstly, I'm not sure why 5 lines of code is more readable than 3.
>> My main point though is that the ternary expression clearly indicates
>> that all we are doing is setting *TriggerType.
>> The multiple assignment "if" requires examination to determine that there
>> is nothing else going on. (Because otherwise why wouldn't you use a
>> ternary?)
>> I'm about to submit v2 without this, in the hope that I've made the case.
>>
>
> I find your argument unconvincing and would prefer an "if" clause.
>
> That is fine, lots of people have irrational prejudices. ;-)
>
> Do you have a counter argument though?
>
I don't think I need a 3rd argument. Like Leif, I don't think that
ternary adds clarity and think an '"if" statement would be easier to
decipher.
> Leif points out that “Ternaries are excellent when they increase code
> readability.”
>
> The debate would thus seem to be a subjective assessment of “readability”.
>
Indeed it is.
> What criteria should we use to identify when a ternary is more readable, and
> when not?
>
> And how do we ensure consistency across all EDK2 maintainers?
>
None of that is down to me.
Cheers,
Ryan.
> Evan
>
>
>
> …
>
>
>
> 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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 17 February 2017 at 15:08, Alexei Fedorov <Alexei.Fedorov@arm.com> wrote:
> Please take a look at the following code in GenericWatchdogEntry():
>
>
> Status = mInterruptProtocol->RegisterInterruptSource (
> mInterruptProtocol,
> FixedPcdGet32
> (PcdGenericWatchdogEl2IntrNum),
> WatchdogInterruptHandler
> );
> if (!EFI_ERROR (Status)) {
> Status = mInterruptProtocol->SetTriggerType (
> mInterruptProtocol,
> FixedPcdGet32
> (PcdGenericWatchdogEl2IntrNum),
>
> EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING);
>
>
> Why can't we extend EFI_HARDWARE_INTERRUPT2_PROTOCOL
> RegisterInterruptSource() function API to have an extra
> EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType parameter to set interrupt
> type & get rid of the second call?
>
We could do that, yes. I tried to keep the members shared between the
old and the new version of the protocol identical, so that the latter
could potentially be cast to the former. I did not investigate
extensively whether there is any point to doing that, though. it just
seemed like a sensible thing to do for two-way compatibility
> ________________________________
> From: edk2-devel <edk2-devel-bounces@lists.01.org> on behalf of Ryan Harkin
> <ryan.harkin@linaro.org>
> Sent: 17 February 2017 12:30:13
> To: Evan Lloyd
> Cc: edk2-devel@ml01.01.org; Leif Lindholm; ard.biesheuvel@linaro.org
> Subject: Re: [edk2] [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType
> functions
>
> On 17 February 2017 at 12:06, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
>> Hi Ryan.
>>
>>
>>
>> From: Ryan Harkin [mailto:ryan.harkin@linaro.org]
>> Sent: 16 February 2017 20:42
>> To: Evan Lloyd
>> Cc: ard.biesheuvel@linaro.org; Leif Lindholm; edk2-devel@ml01.01.org
>> Subject: RE: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType
>> functions
>>
>>
>>
>>
>> On 16 Feb 2017 20:27, "Evan Lloyd" <Evan.Lloyd@arm.com> wrote:
>>>
>>> Hi Leif.
>>> We accept all the comments except that about ternaries.
>>> Response inline.
>>>
>> …
>>
>>> >> +
>>> >> + *TriggerType = (MmioBitFieldRead32 (RegAddress, BitNumber,
>>> >BitNumber) == 0)
>>> >> + ? EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH
>>> >> + : EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
>>> >> +
>>> >
>>> >Ternaries are excellent when they increase code readability.
>>> >I am not convinced that is the case here.
>>> >
>>> >Consider:
>>> >
>>> > if (MmioBitFieldRead32 (RegAddress, BitNumber, BitNumber) == 0) {
>>> > *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH;
>>> > } else {
>>> > *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
>>> > }
>>> >
>>> >?
>>> >
>>> >The versions generate identical code with gcc at -O1 and above.
>>>
>>> Firstly, I'm not sure why 5 lines of code is more readable than 3.
>>> My main point though is that the ternary expression clearly indicates
>>> that all we are doing is setting *TriggerType.
>>> The multiple assignment "if" requires examination to determine that there
>>> is nothing else going on. (Because otherwise why wouldn't you use a
>>> ternary?)
>>> I'm about to submit v2 without this, in the hope that I've made the case.
>>>
>>
>> I find your argument unconvincing and would prefer an "if" clause.
>>
>> That is fine, lots of people have irrational prejudices. ;-)
>>
>> Do you have a counter argument though?
>>
>
> I don't think I need a 3rd argument. Like Leif, I don't think that
> ternary adds clarity and think an '"if" statement would be easier to
> decipher.
>
>
>> Leif points out that “Ternaries are excellent when they increase code
>> readability.”
>>
>> The debate would thus seem to be a subjective assessment of “readability”.
>>
>
> Indeed it is.
>
>
>> What criteria should we use to identify when a ternary is more readable,
>> and
>> when not?
>>
>> And how do we ensure consistency across all EDK2 maintainers?
>>
>
> None of that is down to me.
>
> Cheers,
> Ryan.
>
>> Evan
>>
>>
>>
>> …
>>
>>
>>
>> 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.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
(Intentionally sending my responses out of order)
Hi Evan,
On Fri, Feb 17, 2017 at 12:06:30PM +0000, Evan Lloyd wrote:
> Leif points out that “Ternaries are excellent when they increase
> code readability.”
>
> The debate would thus seem to be a subjective assessment of
> “readability”.
Indeed. With the asymmetric weight distribution towards readability by
the maintainer.
> What criteria should we use to identify when a ternary is more
> readable, and when not?
My view of readability of ternaries is that it comes down to the
amount of information decoding required.
The Tianocore coding style actually works against ternaries a bit, in
that the resulting long variable/function/macro names frequently cause
them to spill onto multiple lines, which usually (but not always)
decrease readability.
The sequence in question
---
*TriggerType = (MmioBitFieldRead32 (RegAddress, BitNumber, BitNumber) == 0)
? EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH
: EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
---
looks like it's extracting a bitfield and assigning it to
*TriggerType, until you get to the == 0, where it looks like it's
assigning whether or not the value of an extracted bitfield is equal
to 0, and you have to get to the next line before you even spot that
it's a ternary. That's twice I have to re-evaluate _what_ it is I am
reviewing before I get to actually reviewing it.
At which point it becomes clear that the "bitfield extraction" is
actually being used as a roundabout binary AND operation. Another
re-evaluation of the sequence.
Had it been written as
---
*TriggerType = (MmioRead32 (RegAddress) & (1U << BitNumber) == 0)
---
it wouldn't have slowed me down as much, and I might not even have
objected, only grumbled a bit under my breath.
The if-form, while 2 lines longer, contains the two lines
} else {
and
}
, which are completely trivial. So while two lines are added, they do
not add two lines to review. And it makes it completely clear from the
get-go that what I am reviewing is a test assigning one value or
another depending on the result of the test.
It also means (although it too would be improved by replacing the
BitField form with the binary AND) it at least isolates the test
statement.
Examples of ternary use that I approve of:
From Linux kernel:
arch/arm64/mm/dump.c
.name = (CONFIG_PGTABLE_LEVELS > 3) ? "PUD" : "PGD",
From EDK2:
MdeModulePkg/Bus/Isa/Ps2MouseDxe/CommPs2.c
MouseDev->State.RightButton = (UINT8) (RButton ? TRUE : FALSE);
MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
WholeFileSize = IS_FFS_FILE2 (FfsHeader) ? FFS_FILE2_SIZE (FfsHeader): FFS_FILE_SIZE (FfsHeader);
I guess we can sum it up in an oversimplified way as "where the
distance between the '=' and the '?' is short enough to make it
immediately clear that we're dealing with a ternary, and the test is
immediately obvious even to someone not familiar with the component in
question.
Nested ternaries are _always_ an abomination, even if trivial.
> And how do we ensure consistency across all EDK2 maintainers?
This is on my list for things to bring into the discussion in
Budapest.
Regards,
Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
(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..6610f356c20e73d84ff3ba519956b426d97ef1eb 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..1f47403c6cdc7e8c0f6ac65d3b95a562da6a2d32 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)GicV2GetInterruptSourceState,
> - (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..a0383ecd7738750f73a2253811403d6ed0d2fd51 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)GicV3GetInterruptSourceState,
> - (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")
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 16 February 2017 at 20:16, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
> 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. :-)
>
This comes up now and again on the mailing list, and there is
consensus that, due to the high precedence of the cast operator,
putting a space after it is counter intuitive. So please omit them,
and certainly don't add spaces after casts when it is the only change
made on those particular lines.
Thanks,
Ard.
>>-----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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: 16 February 2017 20:47
>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
>
>On 16 February 2017 at 20:16, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
>> 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. :-)
>>
>
>This comes up now and again on the mailing list, and there is
>consensus that, due to the high precedence of the cast operator,
>putting a space after it is counter intuitive. So please omit them,
>and certainly don't add spaces after casts when it is the only change
>made on those particular lines.
>
>Thanks,
>Ard.
>
As stated, no argument. It is not something we feel strongly about either way, and as you do, we'll comply.
What I do feel strongly about is that the CSS should provide a guide as to how to write code that will not get rejected.
I raised the comment with a view to getting a rule about casts into the CSS, because otherwise developers suffer at the whim of individual maintainers, with some preferring one style and others another.
If, as you say, there is a consensus, surely that should translate into a CSS statement of that consensus?
Who should I badger to get that done?
>
>
>>>-----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..6610f356c20e73d84ff3ba519
>95
>>>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..1f47403c6cdc7e8c0f6ac65
>d3
>>>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)GicV2GetInterruptSource
>Sta
>>>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..a0383ecd7738750f73a225
>381
>>>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)GicV3GetInterruptSource
>Sta
>>>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.
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Evan,
Sorry for delay in responding, I was off sick last week, and am only
catching up on backlog (and emergencies) now.
On Fri, Feb 17, 2017 at 11:53:30AM +0000, Evan Lloyd wrote:
> As stated, no argument. It is not something we feel strongly about
> either way, and as you do, we'll comply.
> What I do feel strongly about is that the CSS should provide a guide
> as to how to write code that will not get rejected.
> I raised the comment with a view to getting a rule about casts into
> the CSS, because otherwise developers suffer at the whim of
> individual maintainers, with some preferring one style and others
> another.
> If, as you say, there is a consensus, surely that should translate
> into a CSS statement of that consensus?
> Who should I badger to get that done?
Basically, I was planning to sit down with Ard and Laszlo at the
upcoming Linaro Connect in Budapest and try to write down a lot of
these things that are either:
* Generally agreed upon, but not documented.
* Inconsistently documented.
* Not documented at all.
And then start working on getting this all written down properly in
their proper locations, sending out proposals for modifications to the
CSS, Contributions.txt, and so on, to edk2-devel.
Or in summary, badger me if you haven't seen anything by early April.
/
Leif
> >>>-----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..6610f356c20e73d84ff3ba519
> >95
> >>>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..1f47403c6cdc7e8c0f6ac65
> >d3
> >>>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)GicV2GetInterruptSource
> >Sta
> >>>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..a0383ecd7738750f73a225
> >381
> >>>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)GicV3GetInterruptSource
> >Sta
> >>>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.
> 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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.