* [PATCH 1/4] EmbeddedPkg: introduce HardwareInterrupt2 protocol
2017-02-09 19:26 [PATCH 0/4] HardwareInterrupt2 protocol evan.lloyd
@ 2017-02-09 19:26 ` evan.lloyd
2017-02-13 12:26 ` Leif Lindholm
2017-02-09 19:26 ` [PATCH 2/4] ArmPkg/ArmGicDxe: expose " evan.lloyd
` (3 subsequent siblings)
4 siblings, 1 reply; 24+ messages in thread
From: evan.lloyd @ 2017-02-09 19:26 UTC (permalink / raw)
To: edk2-devel; +Cc: Ard Biesheuvel, Leif Lindholm, Ryan Harkin
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
The existing HardwareInterrupt protocol lacks the method to configure
the level/edge and polarity properties of an interrupt. So introduce a
new protocol HardwareInterrupt2, and add some new members that allow the
manipulation of those properties.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Girish Pathak <girish.pathak@arm.com>
Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
Tested-by: Girish Pathak <girish.pathak@arm.com>
---
EmbeddedPkg/EmbeddedPkg.dec | 1 +
EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h | 181 ++++++++++++++++++++
2 files changed, 182 insertions(+)
diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 2c2cf41103c282103c2a83ad9a105d0f6ac2e9a3..2464f715e68c8e0eb1214c0170fb040830b88f06 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -58,6 +58,7 @@ [Guids.common]
[Protocols.common]
gHardwareInterruptProtocolGuid = { 0x2890B3EA, 0x053D, 0x1643, { 0xAD, 0x0C, 0xD6, 0x48, 0x08, 0xDA, 0x3F, 0xF1 } }
+ gHardwareInterrupt2ProtocolGuid = { 0x32898322, 0x2da1, 0x474a, { 0xba, 0xaa, 0xf3, 0xf7, 0xcf, 0x56, 0x94, 0x70 } }
gEfiDebugSupportPeriodicCallbackProtocolGuid = { 0x9546e07c, 0x2cbb, 0x4c88, { 0x98, 0x6c, 0xcd, 0x34, 0x10, 0x86, 0xf0, 0x44 } }
gEfiEblAddCommandProtocolGuid = { 0xaeda2428, 0x9a22, 0x4637, { 0x9b, 0x21, 0x54, 0x5e, 0x28, 0xfb, 0xb8, 0x29 } }
gEmbeddedDeviceGuid = { 0xbf4b9d10, 0x13ec, 0x43dd, { 0x88, 0x80, 0xe9, 0xb, 0x71, 0x8f, 0x27, 0xde } }
diff --git a/EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h b/EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h
new file mode 100644
index 0000000000000000000000000000000000000000..61acdaac4341951b8c1916858490a815cf94dc99
--- /dev/null
+++ b/EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h
@@ -0,0 +1,181 @@
+/** @file
+
+ Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
+
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions of the BSD License
+ which accompanies this distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef __HARDWARE_INTERRUPT2_H__
+#define __HARDWARE_INTERRUPT2_H__
+
+#include <Protocol/HardwareInterrupt.h>
+
+// 22838932-1a2d-4a47-aaba-f3f7cf569470
+
+#define EFI_HARDWARE_INTERRUPT2_PROTOCOL_GUID \
+ { 0x32898322, 0x2da1, 0x474a, { 0xba, 0xaa, 0xf3, 0xf7, 0xcf, 0x56, 0x94, 0x70 } }
+
+typedef enum {
+ EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_LOW,
+ EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH,
+ EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_FALLING,
+ EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING,
+} EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE;
+
+typedef struct _EFI_HARDWARE_INTERRUPT2_PROTOCOL EFI_HARDWARE_INTERRUPT2_PROTOCOL;
+
+/**
+ Register Handler for the specified interrupt source.
+
+ @param This Instance pointer for this protocol
+ @param Source Hardware source of the interrupt
+ @param Handler Callback for interrupt. NULL to unregister
+
+ @retval EFI_SUCCESS Source was updated to support Handler.
+ @retval EFI_DEVICE_ERROR Hardware could not be programmed.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *HARDWARE_INTERRUPT2_REGISTER) (
+ IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+ IN HARDWARE_INTERRUPT_SOURCE Source,
+ IN HARDWARE_INTERRUPT_HANDLER Handler
+ );
+
+
+/**
+ Enable interrupt source Source.
+
+ @param This Instance pointer for this protocol
+ @param Source Hardware source of the interrupt
+
+ @retval EFI_SUCCESS Source interrupt enabled.
+ @retval EFI_DEVICE_ERROR Hardware could not be programmed.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *HARDWARE_INTERRUPT2_ENABLE) (
+ IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+ IN HARDWARE_INTERRUPT_SOURCE Source
+ );
+
+
+
+/**
+ Disable interrupt source Source.
+
+ @param This Instance pointer for this protocol
+ @param Source Hardware source of the interrupt
+
+ @retval EFI_SUCCESS Source interrupt disabled.
+ @retval EFI_DEVICE_ERROR Hardware could not be programmed.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *HARDWARE_INTERRUPT2_DISABLE) (
+ IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+ IN HARDWARE_INTERRUPT_SOURCE Source
+ );
+
+
+/**
+ Return current state of interrupt source Source.
+
+ @param This Instance pointer for this protocol
+ @param Source Hardware source of the interrupt
+ @param InterruptState TRUE: source enabled, FALSE: source disabled.
+
+ @retval EFI_SUCCESS InterruptState is valid
+ @retval EFI_DEVICE_ERROR InterruptState is not valid
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *HARDWARE_INTERRUPT2_INTERRUPT_STATE) (
+ IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+ IN HARDWARE_INTERRUPT_SOURCE Source,
+ IN BOOLEAN *InterruptState
+ );
+
+/**
+ Signal to the hardware that the End Of Intrrupt state
+ has been reached.
+
+ @param This Instance pointer for this protocol
+ @param Source Hardware source of the interrupt
+
+ @retval EFI_SUCCESS Source interrupt EOI'ed.
+ @retval EFI_DEVICE_ERROR Hardware could not be programmed.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *HARDWARE_INTERRUPT2_END_OF_INTERRUPT) (
+ IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+ IN HARDWARE_INTERRUPT_SOURCE Source
+ );
+
+/**
+ Return the configured trigger type for an interrupt source
+
+ @param This Instance pointer for this protocol
+ @param Source Hardware source of the interrupt
+ @param TriggerType The configured trigger type
+
+ @retval EFI_SUCCESS Operation successful
+ @retval EFI_DEVICE_ERROR Information could not be returned
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *HARDWARE_INTERRUPT2_GET_TRIGGER_TYPE) (
+ IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+ IN HARDWARE_INTERRUPT_SOURCE Source,
+ OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE *TriggerType
+ );
+
+
+/**
+ Configure the trigger type for an interrupt source
+
+ @param This Instance pointer for this protocol
+ @param Source Hardware source of the interrupt
+ @param TriggerType The trigger type to configure
+
+ @retval EFI_SUCCESS Operation successful
+ @retval EFI_DEVICE_ERROR Hardware could not be programmed.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *HARDWARE_INTERRUPT2_SET_TRIGGER_TYPE) (
+ IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+ IN HARDWARE_INTERRUPT_SOURCE Source,
+ IN EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType
+ );
+
+struct _EFI_HARDWARE_INTERRUPT2_PROTOCOL {
+ HARDWARE_INTERRUPT2_REGISTER RegisterInterruptSource;
+ HARDWARE_INTERRUPT2_ENABLE EnableInterruptSource;
+ HARDWARE_INTERRUPT2_DISABLE DisableInterruptSource;
+ HARDWARE_INTERRUPT2_INTERRUPT_STATE GetInterruptSourceState;
+ HARDWARE_INTERRUPT2_END_OF_INTERRUPT EndOfInterrupt;
+
+ // v2 members
+ HARDWARE_INTERRUPT2_GET_TRIGGER_TYPE GetTriggerType;
+ HARDWARE_INTERRUPT2_SET_TRIGGER_TYPE SetTriggerType;
+};
+
+extern EFI_GUID gHardwareInterrupt2ProtocolGuid;
+
+#endif
--
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 1/4] EmbeddedPkg: introduce HardwareInterrupt2 protocol
2017-02-09 19:26 ` [PATCH 1/4] EmbeddedPkg: introduce " evan.lloyd
@ 2017-02-13 12:26 ` Leif Lindholm
0 siblings, 0 replies; 24+ messages in thread
From: Leif Lindholm @ 2017-02-13 12:26 UTC (permalink / raw)
To: evan.lloyd; +Cc: edk2-devel, Ard Biesheuvel, Ryan Harkin
On Thu, Feb 09, 2017 at 07:26:20PM +0000, evan.lloyd@arm.com wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> The existing HardwareInterrupt protocol lacks the method to configure
> the level/edge and polarity properties of an interrupt. So introduce a
> new protocol HardwareInterrupt2, and add some new members that allow the
> manipulation of those properties.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> Tested-by: Girish Pathak <girish.pathak@arm.com>
> ---
> EmbeddedPkg/EmbeddedPkg.dec | 1 +
> EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h | 181 ++++++++++++++++++++
> 2 files changed, 182 insertions(+)
>
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index 2c2cf41103c282103c2a83ad9a105d0f6ac2e9a3..2464f715e68c8e0eb1214c0170fb040830b88f06 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -58,6 +58,7 @@ [Guids.common]
>
> [Protocols.common]
> gHardwareInterruptProtocolGuid = { 0x2890B3EA, 0x053D, 0x1643, { 0xAD, 0x0C, 0xD6, 0x48, 0x08, 0xDA, 0x3F, 0xF1 } }
> + gHardwareInterrupt2ProtocolGuid = { 0x32898322, 0x2da1, 0x474a, { 0xba, 0xaa, 0xf3, 0xf7, 0xcf, 0x56, 0x94, 0x70 } }
> gEfiDebugSupportPeriodicCallbackProtocolGuid = { 0x9546e07c, 0x2cbb, 0x4c88, { 0x98, 0x6c, 0xcd, 0x34, 0x10, 0x86, 0xf0, 0x44 } }
> gEfiEblAddCommandProtocolGuid = { 0xaeda2428, 0x9a22, 0x4637, { 0x9b, 0x21, 0x54, 0x5e, 0x28, 0xfb, 0xb8, 0x29 } }
> gEmbeddedDeviceGuid = { 0xbf4b9d10, 0x13ec, 0x43dd, { 0x88, 0x80, 0xe9, 0xb, 0x71, 0x8f, 0x27, 0xde } }
> diff --git a/EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h b/EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..61acdaac4341951b8c1916858490a815cf94dc99
> --- /dev/null
> +++ b/EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h
> @@ -0,0 +1,181 @@
> +/** @file
> +
> + Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
-2017 by now, I guess.
> +
> + This program and the accompanying materials
> + are licensed and made available under the terms and conditions of the BSD License
> + which accompanies this distribution. The full text of the license may be found at
> + http://opensource.org/licenses/bsd-license.php
> +
> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef __HARDWARE_INTERRUPT2_H__
> +#define __HARDWARE_INTERRUPT2_H__
> +
> +#include <Protocol/HardwareInterrupt.h>
> +
> +// 22838932-1a2d-4a47-aaba-f3f7cf569470
> +
> +#define EFI_HARDWARE_INTERRUPT2_PROTOCOL_GUID \
> + { 0x32898322, 0x2da1, 0x474a, { 0xba, 0xaa, 0xf3, 0xf7, 0xcf, 0x56, 0x94, 0x70 } }
> +
> +typedef enum {
> + EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_LOW,
> + EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH,
> + EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_FALLING,
> + EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING,
> +} EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE;
> +
> +typedef struct _EFI_HARDWARE_INTERRUPT2_PROTOCOL EFI_HARDWARE_INTERRUPT2_PROTOCOL;
> +
> +/**
> + Register Handler for the specified interrupt source.
> +
> + @param This Instance pointer for this protocol
> + @param Source Hardware source of the interrupt
> + @param Handler Callback for interrupt. NULL to unregister
> +
> + @retval EFI_SUCCESS Source was updated to support Handler.
> + @retval EFI_DEVICE_ERROR Hardware could not be programmed.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_REGISTER) (
> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> + IN HARDWARE_INTERRUPT_SOURCE Source,
Arguments horizontally misaligned?
> + IN HARDWARE_INTERRUPT_HANDLER Handler
> + );
> +
> +
> +/**
> + Enable interrupt source Source.
> +
> + @param This Instance pointer for this protocol
> + @param Source Hardware source of the interrupt
> +
> + @retval EFI_SUCCESS Source interrupt enabled.
> + @retval EFI_DEVICE_ERROR Hardware could not be programmed.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_ENABLE) (
> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> + IN HARDWARE_INTERRUPT_SOURCE Source
Arguments horizontally misaligned?
> + );
> +
> +
> +
> +/**
> + Disable interrupt source Source.
> +
> + @param This Instance pointer for this protocol
> + @param Source Hardware source of the interrupt
> +
> + @retval EFI_SUCCESS Source interrupt disabled.
> + @retval EFI_DEVICE_ERROR Hardware could not be programmed.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_DISABLE) (
> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> + IN HARDWARE_INTERRUPT_SOURCE Source
Arguments horizontally misaligned?
> + );
> +
> +
> +/**
> + Return current state of interrupt source Source.
> +
> + @param This Instance pointer for this protocol
> + @param Source Hardware source of the interrupt
> + @param InterruptState TRUE: source enabled, FALSE: source disabled.
> +
> + @retval EFI_SUCCESS InterruptState is valid
> + @retval EFI_DEVICE_ERROR InterruptState is not valid
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_INTERRUPT_STATE) (
> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> + IN HARDWARE_INTERRUPT_SOURCE Source,
Arguments horizontally misaligned?
> + IN BOOLEAN *InterruptState
> + );
> +
> +/**
> + Signal to the hardware that the End Of Intrrupt state
> + has been reached.
> +
> + @param This Instance pointer for this protocol
> + @param Source Hardware source of the interrupt
> +
> + @retval EFI_SUCCESS Source interrupt EOI'ed.
> + @retval EFI_DEVICE_ERROR Hardware could not be programmed.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_END_OF_INTERRUPT) (
> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
Did someone clone HARDWARE_INTERRUPT_PROTOCOL templates, inserted a 2
and forgot to delete a space? :)
(I'll stop commenting, but please address throughout.)
/
Leif
> + IN HARDWARE_INTERRUPT_SOURCE Source
> + );
> +
> +/**
> + Return the configured trigger type for an interrupt source
> +
> + @param This Instance pointer for this protocol
> + @param Source Hardware source of the interrupt
> + @param TriggerType The configured trigger type
> +
> + @retval EFI_SUCCESS Operation successful
> + @retval EFI_DEVICE_ERROR Information could not be returned
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_GET_TRIGGER_TYPE) (
> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> + IN HARDWARE_INTERRUPT_SOURCE Source,
> + OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE *TriggerType
> + );
> +
> +
> +/**
> + Configure the trigger type for an interrupt source
> +
> + @param This Instance pointer for this protocol
> + @param Source Hardware source of the interrupt
> + @param TriggerType The trigger type to configure
> +
> + @retval EFI_SUCCESS Operation successful
> + @retval EFI_DEVICE_ERROR Hardware could not be programmed.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_SET_TRIGGER_TYPE) (
> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> + IN HARDWARE_INTERRUPT_SOURCE Source,
> + IN EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType
> + );
> +
> +struct _EFI_HARDWARE_INTERRUPT2_PROTOCOL {
> + HARDWARE_INTERRUPT2_REGISTER RegisterInterruptSource;
> + HARDWARE_INTERRUPT2_ENABLE EnableInterruptSource;
> + HARDWARE_INTERRUPT2_DISABLE DisableInterruptSource;
> + HARDWARE_INTERRUPT2_INTERRUPT_STATE GetInterruptSourceState;
> + HARDWARE_INTERRUPT2_END_OF_INTERRUPT EndOfInterrupt;
> +
> + // v2 members
> + HARDWARE_INTERRUPT2_GET_TRIGGER_TYPE GetTriggerType;
> + HARDWARE_INTERRUPT2_SET_TRIGGER_TYPE SetTriggerType;
> +};
> +
> +extern EFI_GUID gHardwareInterrupt2ProtocolGuid;
> +
> +#endif
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/4] ArmPkg/ArmGicDxe: expose HardwareInterrupt2 protocol
2017-02-09 19:26 [PATCH 0/4] HardwareInterrupt2 protocol evan.lloyd
2017-02-09 19:26 ` [PATCH 1/4] EmbeddedPkg: introduce " evan.lloyd
@ 2017-02-09 19:26 ` evan.lloyd
2017-02-13 12:21 ` Leif Lindholm
2017-02-09 19:26 ` [PATCH 3/4] ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type evan.lloyd
` (2 subsequent siblings)
4 siblings, 1 reply; 24+ messages in thread
From: evan.lloyd @ 2017-02-09 19:26 UTC (permalink / raw)
To: edk2-devel; +Cc: Ard Biesheuvel, Leif Lindholm, Ryan Harkin
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Girish Pathak <girish.pathak@arm.com>
Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
Tested-by: Girish Pathak <girish.pathak@arm.com>
---
ArmPkg/Drivers/ArmGic/ArmGicDxe.inf | 1 +
ArmPkg/Drivers/ArmGic/ArmGicDxe.h | 2 ++
ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c | 2 ++
ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 38 +++++++++++++++++++-
ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 37 ++++++++++++++++++-
5 files changed, 78 insertions(+), 2 deletions(-)
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
index e554301c4b28022c805f69242cf6ee979d19abc2..69390638a9afb6aeccad510e7b572450568c1409 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
+++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
@@ -48,6 +48,7 @@ [LibraryClasses]
[Protocols]
gHardwareInterruptProtocolGuid
+ gHardwareInterrupt2ProtocolGuid
gEfiCpuArchProtocolGuid
[Pcd.common]
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
index af33aa90b00c6775e10a831d63ed707394862362..2633e1ea194fa67511861a4165d54dad99a6f39b 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
+++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
@@ -24,6 +24,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Protocol/Cpu.h>
#include <Protocol/HardwareInterrupt.h>
+#include <Protocol/HardwareInterrupt2.h>
extern UINTN mGicNumInterrupts;
extern HARDWARE_INTERRUPT_HANDLER *gRegisteredInterruptHandlers;
@@ -34,6 +35,7 @@ extern HARDWARE_INTERRUPT_HANDLER *gRegisteredInterruptHandlers;
EFI_STATUS
InstallAndRegisterInterruptService (
IN EFI_HARDWARE_INTERRUPT_PROTOCOL *InterruptProtocol,
+ IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *Interrupt2Protocol,
IN EFI_CPU_INTERRUPT_HANDLER InterruptHandler,
IN EFI_EVENT_NOTIFY ExitBootServicesEvent
);
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
index be77b8361c5af033fd2889cdb48902af867f321d..ef6746f1ad7afba5bba30fc17774987cf17121b6 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
@@ -88,6 +88,7 @@ RegisterInterruptSource (
EFI_STATUS
InstallAndRegisterInterruptService (
IN EFI_HARDWARE_INTERRUPT_PROTOCOL *InterruptProtocol,
+ IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *Interrupt2Protocol,
IN EFI_CPU_INTERRUPT_HANDLER InterruptHandler,
IN EFI_EVENT_NOTIFY ExitBootServicesEvent
)
@@ -104,6 +105,7 @@ InstallAndRegisterInterruptService (
Status = gBS->InstallMultipleProtocolInterfaces (
&gHardwareInterruptHandle,
&gHardwareInterruptProtocolGuid, InterruptProtocol,
+ &gHardwareInterrupt2ProtocolGuid, Interrupt2Protocol,
NULL
);
if (EFI_ERROR (Status)) {
diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
index b9ecd5543a3e2e0b00fffbcf5543a60567bb5dde..8c4d66125e2e8c7af9898f336ee742ed0aebf058 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
@@ -193,6 +193,41 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol = {
GicV2EndOfInterrupt
};
+STATIC
+EFI_STATUS
+EFIAPI
+GicV2GetTriggerType (
+ IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+ IN HARDWARE_INTERRUPT_SOURCE Source,
+ OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE *TriggerType
+ )
+{
+ return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+GicV2SetTriggerType (
+ IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+ IN HARDWARE_INTERRUPT_SOURCE Source,
+ IN EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType
+ )
+{
+ 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,
+ GicV2GetTriggerType,
+ GicV2SetTriggerType
+};
+
+
/**
Shutdown our hardware
@@ -311,7 +346,8 @@ GicV2DxeInitialize (
ArmGicEnableDistributor (mGicDistributorBase);
Status = InstallAndRegisterInterruptService (
- &gHardwareInterruptV2Protocol, 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 8af97a93b1889b33978a7c7fb2a8417c83139142..02deeef78b6d7737172a5992c6decac43cfdd64a 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
@@ -184,6 +184,40 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV3Protocol = {
GicV3EndOfInterrupt
};
+STATIC
+EFI_STATUS
+EFIAPI
+GicV3GetTriggerType (
+ IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+ IN HARDWARE_INTERRUPT_SOURCE Source,
+ OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE *TriggerType
+ )
+{
+ return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+GicV3SetTriggerType (
+ IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+ IN HARDWARE_INTERRUPT_SOURCE Source,
+ IN EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType
+ )
+{
+ 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,
+ GicV3GetTriggerType,
+ GicV3SetTriggerType
+};
+
/**
Shutdown our hardware
@@ -331,7 +365,8 @@ GicV3DxeInitialize (
ArmGicEnableDistributor (mGicDistributorBase);
Status = InstallAndRegisterInterruptService (
- &gHardwareInterruptV3Protocol, GicV3IrqInterruptHandler, GicV3ExitBootServicesEvent);
+ &gHardwareInterruptV3Protocol, &gHardwareInterrupt2V3Protocol,
+ GicV3IrqInterruptHandler, GicV3ExitBootServicesEvent);
return Status;
}
--
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 2/4] ArmPkg/ArmGicDxe: expose HardwareInterrupt2 protocol
2017-02-09 19:26 ` [PATCH 2/4] ArmPkg/ArmGicDxe: expose " evan.lloyd
@ 2017-02-13 12:21 ` Leif Lindholm
2017-02-13 12:26 ` Ard Biesheuvel
0 siblings, 1 reply; 24+ messages in thread
From: Leif Lindholm @ 2017-02-13 12:21 UTC (permalink / raw)
To: evan.lloyd; +Cc: edk2-devel, Ard Biesheuvel, Ryan Harkin
On Thu, Feb 09, 2017 at 07:26:21PM +0000, evan.lloyd@arm.com wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
Ard - can we have some more commit message, please? :)
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> Tested-by: Girish Pathak <girish.pathak@arm.com>
> ---
> ArmPkg/Drivers/ArmGic/ArmGicDxe.inf | 1 +
> ArmPkg/Drivers/ArmGic/ArmGicDxe.h | 2 ++
> ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c | 2 ++
> ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 38 +++++++++++++++++++-
> ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 37 ++++++++++++++++++-
> 5 files changed, 78 insertions(+), 2 deletions(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
> index e554301c4b28022c805f69242cf6ee979d19abc2..69390638a9afb6aeccad510e7b572450568c1409 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
> @@ -48,6 +48,7 @@ [LibraryClasses]
>
> [Protocols]
> gHardwareInterruptProtocolGuid
> + gHardwareInterrupt2ProtocolGuid
> gEfiCpuArchProtocolGuid
>
> [Pcd.common]
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
> index af33aa90b00c6775e10a831d63ed707394862362..2633e1ea194fa67511861a4165d54dad99a6f39b 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
> @@ -24,6 +24,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>
> #include <Protocol/Cpu.h>
> #include <Protocol/HardwareInterrupt.h>
> +#include <Protocol/HardwareInterrupt2.h>
>
> extern UINTN mGicNumInterrupts;
> extern HARDWARE_INTERRUPT_HANDLER *gRegisteredInterruptHandlers;
> @@ -34,6 +35,7 @@ extern HARDWARE_INTERRUPT_HANDLER *gRegisteredInterruptHandlers;
> EFI_STATUS
> InstallAndRegisterInterruptService (
> IN EFI_HARDWARE_INTERRUPT_PROTOCOL *InterruptProtocol,
> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *Interrupt2Protocol,
> IN EFI_CPU_INTERRUPT_HANDLER InterruptHandler,
> IN EFI_EVENT_NOTIFY ExitBootServicesEvent
> );
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> index be77b8361c5af033fd2889cdb48902af867f321d..ef6746f1ad7afba5bba30fc17774987cf17121b6 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> @@ -88,6 +88,7 @@ RegisterInterruptSource (
> EFI_STATUS
> InstallAndRegisterInterruptService (
> IN EFI_HARDWARE_INTERRUPT_PROTOCOL *InterruptProtocol,
> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *Interrupt2Protocol,
> IN EFI_CPU_INTERRUPT_HANDLER InterruptHandler,
> IN EFI_EVENT_NOTIFY ExitBootServicesEvent
> )
> @@ -104,6 +105,7 @@ InstallAndRegisterInterruptService (
> Status = gBS->InstallMultipleProtocolInterfaces (
> &gHardwareInterruptHandle,
> &gHardwareInterruptProtocolGuid, InterruptProtocol,
> + &gHardwareInterrupt2ProtocolGuid, Interrupt2Protocol,
> NULL
> );
> if (EFI_ERROR (Status)) {
> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> index b9ecd5543a3e2e0b00fffbcf5543a60567bb5dde..8c4d66125e2e8c7af9898f336ee742ed0aebf058 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> @@ -193,6 +193,41 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol = {
> GicV2EndOfInterrupt
> };
>
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +GicV2GetTriggerType (
> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> + IN HARDWARE_INTERRUPT_SOURCE Source,
> + OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE *TriggerType
> + )
> +{
> + return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +GicV2SetTriggerType (
> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> + IN HARDWARE_INTERRUPT_SOURCE Source,
> + IN EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType
> + )
> +{
> + return EFI_SUCCESS;
> +}
> +
> +STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V2Protocol = {
So, this one gets its STATIC revoked in 4/4 - should it just be left
out from the start?
> + (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
>
> @@ -311,7 +346,8 @@ GicV2DxeInitialize (
> ArmGicEnableDistributor (mGicDistributorBase);
>
> Status = InstallAndRegisterInterruptService (
> - &gHardwareInterruptV2Protocol, GicV2IrqInterruptHandler, GicV2ExitBootServicesEvent);
> + &gHardwareInterruptV2Protocol, &gHardwareInterrupt2V2Protocol,
> + GicV2IrqInterruptHandler, GicV2ExitBootServicesEvent);
And arguably, since this is the functional change, you could do the
cosmetic change (1 per line) which Girish tried in 4/4.
>
> return Status;
> }
> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> index 8af97a93b1889b33978a7c7fb2a8417c83139142..02deeef78b6d7737172a5992c6decac43cfdd64a 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> @@ -184,6 +184,40 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV3Protocol = {
> GicV3EndOfInterrupt
> };
>
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +GicV3GetTriggerType (
> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> + IN HARDWARE_INTERRUPT_SOURCE Source,
> + OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE *TriggerType
> + )
> +{
> + return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +GicV3SetTriggerType (
> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> + IN HARDWARE_INTERRUPT_SOURCE Source,
> + IN EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType
> + )
> +{
> + return EFI_SUCCESS;
> +}
> +
> +STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V3Protocol = {
Same comment on STATIC. Leave out?
> + (HARDWARE_INTERRUPT2_REGISTER)RegisterInterruptSource,
> + (HARDWARE_INTERRUPT2_ENABLE)GicV3EnableInterruptSource,
> + (HARDWARE_INTERRUPT2_DISABLE)GicV3DisableInterruptSource,
> + (HARDWARE_INTERRUPT2_INTERRUPT_STATE)GicV3GetInterruptSourceState,
> + (HARDWARE_INTERRUPT2_END_OF_INTERRUPT)GicV3EndOfInterrupt,
> + GicV3GetTriggerType,
> + GicV3SetTriggerType
> +};
> +
> /**
> Shutdown our hardware
>
> @@ -331,7 +365,8 @@ GicV3DxeInitialize (
> ArmGicEnableDistributor (mGicDistributorBase);
>
> Status = InstallAndRegisterInterruptService (
> - &gHardwareInterruptV3Protocol, GicV3IrqInterruptHandler, GicV3ExitBootServicesEvent);
> + &gHardwareInterruptV3Protocol, &gHardwareInterrupt2V3Protocol,
> + GicV3IrqInterruptHandler, GicV3ExitBootServicesEvent);
And same comment on 1 per line.
>
> return Status;
> }
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2/4] ArmPkg/ArmGicDxe: expose HardwareInterrupt2 protocol
2017-02-13 12:21 ` Leif Lindholm
@ 2017-02-13 12:26 ` Ard Biesheuvel
0 siblings, 0 replies; 24+ messages in thread
From: Ard Biesheuvel @ 2017-02-13 12:26 UTC (permalink / raw)
To: Leif Lindholm; +Cc: evan.lloyd, edk2-devel, Ryan Harkin
> On 13 Feb 2017, at 13:21, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
>> On Thu, Feb 09, 2017 at 07:26:21PM +0000, evan.lloyd@arm.com wrote:
>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>
> Ard - can we have some more commit message, please? :)
>
This patch was only a PoC, and needs to be merged with 4/4 imo.
The STATICs should be kept, and the other cosmetic changes could also be dropped afaict
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
>> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
>> Tested-by: Girish Pathak <girish.pathak@arm.com>
>> ---
>> ArmPkg/Drivers/ArmGic/ArmGicDxe.inf | 1 +
>> ArmPkg/Drivers/ArmGic/ArmGicDxe.h | 2 ++
>> ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c | 2 ++
>> ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 38 +++++++++++++++++++-
>> ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 37 ++++++++++++++++++-
>> 5 files changed, 78 insertions(+), 2 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
>> index e554301c4b28022c805f69242cf6ee979d19abc2..69390638a9afb6aeccad510e7b572450568c1409 100644
>> --- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
>> +++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
>> @@ -48,6 +48,7 @@ [LibraryClasses]
>>
>> [Protocols]
>> gHardwareInterruptProtocolGuid
>> + gHardwareInterrupt2ProtocolGuid
>> gEfiCpuArchProtocolGuid
>>
>> [Pcd.common]
>> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
>> index af33aa90b00c6775e10a831d63ed707394862362..2633e1ea194fa67511861a4165d54dad99a6f39b 100644
>> --- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
>> +++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
>> @@ -24,6 +24,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>
>> #include <Protocol/Cpu.h>
>> #include <Protocol/HardwareInterrupt.h>
>> +#include <Protocol/HardwareInterrupt2.h>
>>
>> extern UINTN mGicNumInterrupts;
>> extern HARDWARE_INTERRUPT_HANDLER *gRegisteredInterruptHandlers;
>> @@ -34,6 +35,7 @@ extern HARDWARE_INTERRUPT_HANDLER *gRegisteredInterruptHandlers;
>> EFI_STATUS
>> InstallAndRegisterInterruptService (
>> IN EFI_HARDWARE_INTERRUPT_PROTOCOL *InterruptProtocol,
>> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *Interrupt2Protocol,
>> IN EFI_CPU_INTERRUPT_HANDLER InterruptHandler,
>> IN EFI_EVENT_NOTIFY ExitBootServicesEvent
>> );
>> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
>> index be77b8361c5af033fd2889cdb48902af867f321d..ef6746f1ad7afba5bba30fc17774987cf17121b6 100644
>> --- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
>> +++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
>> @@ -88,6 +88,7 @@ RegisterInterruptSource (
>> EFI_STATUS
>> InstallAndRegisterInterruptService (
>> IN EFI_HARDWARE_INTERRUPT_PROTOCOL *InterruptProtocol,
>> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *Interrupt2Protocol,
>> IN EFI_CPU_INTERRUPT_HANDLER InterruptHandler,
>> IN EFI_EVENT_NOTIFY ExitBootServicesEvent
>> )
>> @@ -104,6 +105,7 @@ InstallAndRegisterInterruptService (
>> Status = gBS->InstallMultipleProtocolInterfaces (
>> &gHardwareInterruptHandle,
>> &gHardwareInterruptProtocolGuid, InterruptProtocol,
>> + &gHardwareInterrupt2ProtocolGuid, Interrupt2Protocol,
>> NULL
>> );
>> if (EFI_ERROR (Status)) {
>> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>> index b9ecd5543a3e2e0b00fffbcf5543a60567bb5dde..8c4d66125e2e8c7af9898f336ee742ed0aebf058 100644
>> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>> @@ -193,6 +193,41 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol = {
>> GicV2EndOfInterrupt
>> };
>>
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +GicV2GetTriggerType (
>> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
>> + IN HARDWARE_INTERRUPT_SOURCE Source,
>> + OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE *TriggerType
>> + )
>> +{
>> + return EFI_SUCCESS;
>> +}
>> +
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +GicV2SetTriggerType (
>> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
>> + IN HARDWARE_INTERRUPT_SOURCE Source,
>> + IN EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType
>> + )
>> +{
>> + return EFI_SUCCESS;
>> +}
>> +
>> +STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V2Protocol = {
>
> So, this one gets its STATIC revoked in 4/4 - should it just be left
> out from the start?
>
>> + (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
>>
>> @@ -311,7 +346,8 @@ GicV2DxeInitialize (
>> ArmGicEnableDistributor (mGicDistributorBase);
>>
>> Status = InstallAndRegisterInterruptService (
>> - &gHardwareInterruptV2Protocol, GicV2IrqInterruptHandler, GicV2ExitBootServicesEvent);
>> + &gHardwareInterruptV2Protocol, &gHardwareInterrupt2V2Protocol,
>> + GicV2IrqInterruptHandler, GicV2ExitBootServicesEvent);
>
> And arguably, since this is the functional change, you could do the
> cosmetic change (1 per line) which Girish tried in 4/4.
>
>>
>> return Status;
>> }
>> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> index 8af97a93b1889b33978a7c7fb2a8417c83139142..02deeef78b6d7737172a5992c6decac43cfdd64a 100644
>> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>> @@ -184,6 +184,40 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV3Protocol = {
>> GicV3EndOfInterrupt
>> };
>>
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +GicV3GetTriggerType (
>> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
>> + IN HARDWARE_INTERRUPT_SOURCE Source,
>> + OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE *TriggerType
>> + )
>> +{
>> + return EFI_SUCCESS;
>> +}
>> +
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +GicV3SetTriggerType (
>> + IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
>> + IN HARDWARE_INTERRUPT_SOURCE Source,
>> + IN EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType
>> + )
>> +{
>> + return EFI_SUCCESS;
>> +}
>> +
>> +STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V3Protocol = {
>
> Same comment on STATIC. Leave out?
>
>> + (HARDWARE_INTERRUPT2_REGISTER)RegisterInterruptSource,
>> + (HARDWARE_INTERRUPT2_ENABLE)GicV3EnableInterruptSource,
>> + (HARDWARE_INTERRUPT2_DISABLE)GicV3DisableInterruptSource,
>> + (HARDWARE_INTERRUPT2_INTERRUPT_STATE)GicV3GetInterruptSourceState,
>> + (HARDWARE_INTERRUPT2_END_OF_INTERRUPT)GicV3EndOfInterrupt,
>> + GicV3GetTriggerType,
>> + GicV3SetTriggerType
>> +};
>> +
>> /**
>> Shutdown our hardware
>>
>> @@ -331,7 +365,8 @@ GicV3DxeInitialize (
>> ArmGicEnableDistributor (mGicDistributorBase);
>>
>> Status = InstallAndRegisterInterruptService (
>> - &gHardwareInterruptV3Protocol, GicV3IrqInterruptHandler, GicV3ExitBootServicesEvent);
>> + &gHardwareInterruptV3Protocol, &gHardwareInterrupt2V3Protocol,
>> + GicV3IrqInterruptHandler, GicV3ExitBootServicesEvent);
>
> And same comment on 1 per line.
>
>>
>> return Status;
>> }
>> --
>> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/4] ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type
2017-02-09 19:26 [PATCH 0/4] HardwareInterrupt2 protocol evan.lloyd
2017-02-09 19:26 ` [PATCH 1/4] EmbeddedPkg: introduce " evan.lloyd
2017-02-09 19:26 ` [PATCH 2/4] ArmPkg/ArmGicDxe: expose " evan.lloyd
@ 2017-02-09 19:26 ` evan.lloyd
2017-02-13 12:30 ` Leif Lindholm
2017-02-09 19:26 ` [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions evan.lloyd
2017-02-13 15:51 ` [PATCH 0/4] HardwareInterrupt2 protocol Evan Lloyd
4 siblings, 1 reply; 24+ messages in thread
From: evan.lloyd @ 2017-02-09 19:26 UTC (permalink / raw)
To: edk2-devel; +Cc: Ard Biesheuvel, Leif Lindholm, Ryan Harkin
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Utilise the new HardwareInterrupt2 protocol to adjust the
Edje/Level characteristics of the Watchdog interrupt.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Girish Pathak <girish.pathak@arm.com>
Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
Tested-by: Girish Pathak <girish.pathak@arm.com>
---
ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf | 4 +--
ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 27 ++++++++++++--------
2 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
index fece14cc18315cd15510680c438288687b60c018..51d5c0042d84333b9fe66547c99a8d8ed987f175 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
@@ -47,7 +47,7 @@ [Pcd.common]
[Protocols]
gEfiWatchdogTimerArchProtocolGuid
- gHardwareInterruptProtocolGuid
+ gHardwareInterrupt2ProtocolGuid
[Depex]
- gHardwareInterruptProtocolGuid
+ gHardwareInterrupt2ProtocolGuid
diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 54a1625a32137556b58fa93ddf7fbe4d0f22c786..9ba9bf79c961b52dc9b448039a9186e069fc29f7 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -25,7 +25,8 @@
#include <Library/ArmGenericTimerCounterLib.h>
#include <Protocol/WatchdogTimer.h>
-#include <Protocol/HardwareInterrupt.h>
+
+#include <Protocol/HardwareInterrupt2.h>
#include "GenericWatchdog.h"
@@ -41,7 +42,7 @@ UINTN mTimerFrequencyHz = 0;
// It is therefore stored here. 0 means the timer is not running.
UINT64 mNumTimerTicks = 0;
-EFI_HARDWARE_INTERRUPT_PROTOCOL *mInterruptProtocol;
+EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
EFI_STATUS
WatchdogWriteOffsetRegister (
@@ -320,7 +321,7 @@ GenericWatchdogEntry (
if (!EFI_ERROR (Status)) {
// Install interrupt handler
Status = gBS->LocateProtocol (
- &gHardwareInterruptProtocolGuid,
+ &gHardwareInterrupt2ProtocolGuid,
NULL,
(VOID **)&mInterruptProtocol
);
@@ -331,13 +332,19 @@ GenericWatchdogEntry (
WatchdogInterruptHandler
);
if (!EFI_ERROR (Status)) {
- // Install the Timer Architectural Protocol onto a new handle
- Handle = NULL;
- Status = gBS->InstallMultipleProtocolInterfaces (
- &Handle,
- &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
- NULL
- );
+ Status = mInterruptProtocol->SetTriggerType (
+ mInterruptProtocol,
+ FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
+ EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING);
+ if (!EFI_ERROR (Status)) {
+ // Install the Timer Architectural Protocol onto a new handle
+ Handle = NULL;
+ Status = gBS->InstallMultipleProtocolInterfaces (
+ &Handle,
+ &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
+ NULL
+ );
+ }
}
}
}
--
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 3/4] ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type
2017-02-09 19:26 ` [PATCH 3/4] ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type evan.lloyd
@ 2017-02-13 12:30 ` Leif Lindholm
0 siblings, 0 replies; 24+ messages in thread
From: Leif Lindholm @ 2017-02-13 12:30 UTC (permalink / raw)
To: evan.lloyd; +Cc: edk2-devel, Ard Biesheuvel, Ryan Harkin
On Thu, Feb 09, 2017 at 07:26:22PM +0000, evan.lloyd@arm.com wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Utilise the new HardwareInterrupt2 protocol to adjust the
> Edje/Level characteristics of the Watchdog interrupt.
Edge.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> Tested-by: Girish Pathak <girish.pathak@arm.com>
> ---
> ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf | 4 +--
> ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 27 ++++++++++++--------
> 2 files changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
> index fece14cc18315cd15510680c438288687b60c018..51d5c0042d84333b9fe66547c99a8d8ed987f175 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
> @@ -47,7 +47,7 @@ [Pcd.common]
>
> [Protocols]
> gEfiWatchdogTimerArchProtocolGuid
> - gHardwareInterruptProtocolGuid
> + gHardwareInterrupt2ProtocolGuid
>
> [Depex]
> - gHardwareInterruptProtocolGuid
> + gHardwareInterrupt2ProtocolGuid
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> index 54a1625a32137556b58fa93ddf7fbe4d0f22c786..9ba9bf79c961b52dc9b448039a9186e069fc29f7 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> @@ -25,7 +25,8 @@
> #include <Library/ArmGenericTimerCounterLib.h>
>
> #include <Protocol/WatchdogTimer.h>
> -#include <Protocol/HardwareInterrupt.h>
> +
> +#include <Protocol/HardwareInterrupt2.h>
If Protocol/HardwareInterrupt.h is dropped, can we move this before
Protocol/WatchdogTimer.h?
>
> #include "GenericWatchdog.h"
>
> @@ -41,7 +42,7 @@ UINTN mTimerFrequencyHz = 0;
> // It is therefore stored here. 0 means the timer is not running.
> UINT64 mNumTimerTicks = 0;
>
> -EFI_HARDWARE_INTERRUPT_PROTOCOL *mInterruptProtocol;
> +EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
>
> EFI_STATUS
> WatchdogWriteOffsetRegister (
> @@ -320,7 +321,7 @@ GenericWatchdogEntry (
> if (!EFI_ERROR (Status)) {
> // Install interrupt handler
> Status = gBS->LocateProtocol (
> - &gHardwareInterruptProtocolGuid,
> + &gHardwareInterrupt2ProtocolGuid,
> NULL,
> (VOID **)&mInterruptProtocol
> );
> @@ -331,13 +332,19 @@ GenericWatchdogEntry (
> WatchdogInterruptHandler
> );
> if (!EFI_ERROR (Status)) {
> - // Install the Timer Architectural Protocol onto a new handle
> - Handle = NULL;
> - Status = gBS->InstallMultipleProtocolInterfaces (
> - &Handle,
> - &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
> - NULL
> - );
> + Status = mInterruptProtocol->SetTriggerType (
> + mInterruptProtocol,
I believe this is an incorrect level of indentation.
> + FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
> + EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING);
> + if (!EFI_ERROR (Status)) {
> + // Install the Timer Architectural Protocol onto a new handle
> + Handle = NULL;
> + Status = gBS->InstallMultipleProtocolInterfaces (
> + &Handle,
> + &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
> + NULL
> + );
> + }
> }
> }
> }
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
2017-02-09 19:26 [PATCH 0/4] HardwareInterrupt2 protocol evan.lloyd
` (2 preceding siblings ...)
2017-02-09 19:26 ` [PATCH 3/4] ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type evan.lloyd
@ 2017-02-09 19:26 ` evan.lloyd
2017-02-13 12:15 ` Leif Lindholm
2017-02-13 13:05 ` Ard Biesheuvel
2017-02-13 15:51 ` [PATCH 0/4] HardwareInterrupt2 protocol Evan Lloyd
4 siblings, 2 replies; 24+ messages in thread
From: evan.lloyd @ 2017-02-09 19:26 UTC (permalink / raw)
To: edk2-devel; +Cc: Ard Biesheuvel, Leif Lindholm, Ryan Harkin
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")
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
2017-02-09 19:26 ` [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions evan.lloyd
@ 2017-02-13 12:15 ` Leif Lindholm
2017-02-16 20:27 ` Evan Lloyd
2017-02-13 13:05 ` Ard Biesheuvel
1 sibling, 1 reply; 24+ messages in thread
From: Leif Lindholm @ 2017-02-13 12:15 UTC (permalink / raw)
To: evan.lloyd; +Cc: edk2-devel, Ard Biesheuvel, Ryan Harkin
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")
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
2017-02-13 12:15 ` Leif Lindholm
@ 2017-02-16 20:27 ` Evan Lloyd
2017-02-16 20:42 ` Ryan Harkin
0 siblings, 1 reply; 24+ messages in thread
From: Evan Lloyd @ 2017-02-16 20:27 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel@ml01.01.org, ard.biesheuvel@linaro.org,
ryan.harkin@linaro.org
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.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
2017-02-16 20:27 ` Evan Lloyd
@ 2017-02-16 20:42 ` Ryan Harkin
2017-02-17 12:06 ` Evan Lloyd
0 siblings, 1 reply; 24+ messages in thread
From: Ryan Harkin @ 2017-02-16 20:42 UTC (permalink / raw)
To: Evan Lloyd; +Cc: Ard Biesheuvel, Leif Lindholm, edk2-devel@ml01.01.org
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.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
2017-02-16 20:42 ` Ryan Harkin
@ 2017-02-17 12:06 ` Evan Lloyd
2017-02-17 12:30 ` Ryan Harkin
2017-02-24 14:06 ` Leif Lindholm
0 siblings, 2 replies; 24+ messages in thread
From: Evan Lloyd @ 2017-02-17 12:06 UTC (permalink / raw)
To: ryan.harkin@linaro.org
Cc: ard.biesheuvel@linaro.org, Leif Lindholm, edk2-devel@ml01.01.org
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.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
2017-02-17 12:06 ` Evan Lloyd
@ 2017-02-17 12:30 ` Ryan Harkin
2017-02-17 15:08 ` Alexei Fedorov
2017-02-24 14:06 ` Leif Lindholm
1 sibling, 1 reply; 24+ messages in thread
From: Ryan Harkin @ 2017-02-17 12:30 UTC (permalink / raw)
To: Evan Lloyd
Cc: ard.biesheuvel@linaro.org, Leif Lindholm, edk2-devel@ml01.01.org
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.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
2017-02-17 12:30 ` Ryan Harkin
@ 2017-02-17 15:08 ` Alexei Fedorov
2017-02-17 18:18 ` Ard Biesheuvel
0 siblings, 1 reply; 24+ messages in thread
From: Alexei Fedorov @ 2017-02-17 15:08 UTC (permalink / raw)
To: ryan.harkin@linaro.org, Evan Lloyd
Cc: edk2-devel@ml01.01.org, Leif Lindholm, ard.biesheuvel@linaro.org
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.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
2017-02-17 15:08 ` Alexei Fedorov
@ 2017-02-17 18:18 ` Ard Biesheuvel
0 siblings, 0 replies; 24+ messages in thread
From: Ard Biesheuvel @ 2017-02-17 18:18 UTC (permalink / raw)
To: Alexei Fedorov
Cc: ryan.harkin@linaro.org, Evan Lloyd, edk2-devel@ml01.01.org,
Leif Lindholm
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
2017-02-17 12:06 ` Evan Lloyd
2017-02-17 12:30 ` Ryan Harkin
@ 2017-02-24 14:06 ` Leif Lindholm
1 sibling, 0 replies; 24+ messages in thread
From: Leif Lindholm @ 2017-02-24 14:06 UTC (permalink / raw)
To: Evan Lloyd
Cc: ryan.harkin@linaro.org, ard.biesheuvel@linaro.org,
edk2-devel@ml01.01.org
(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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
2017-02-09 19:26 ` [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions evan.lloyd
2017-02-13 12:15 ` Leif Lindholm
@ 2017-02-13 13:05 ` Ard Biesheuvel
2017-02-16 20:16 ` Evan Lloyd
1 sibling, 1 reply; 24+ messages in thread
From: Ard Biesheuvel @ 2017-02-13 13:05 UTC (permalink / raw)
To: Evan Lloyd; +Cc: edk2-devel@lists.01.org, Leif Lindholm, Ryan Harkin
(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")
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
2017-02-13 13:05 ` Ard Biesheuvel
@ 2017-02-16 20:16 ` Evan Lloyd
2017-02-16 20:46 ` Ard Biesheuvel
0 siblings, 1 reply; 24+ messages in thread
From: Evan Lloyd @ 2017-02-16 20:16 UTC (permalink / raw)
To: ard.biesheuvel@linaro.org
Cc: edk2-devel@lists.01.org, Leif Lindholm, ryan.harkin@linaro.org
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.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
2017-02-16 20:16 ` Evan Lloyd
@ 2017-02-16 20:46 ` Ard Biesheuvel
2017-02-17 11:53 ` Evan Lloyd
0 siblings, 1 reply; 24+ messages in thread
From: Ard Biesheuvel @ 2017-02-16 20:46 UTC (permalink / raw)
To: Evan Lloyd; +Cc: edk2-devel@lists.01.org, Leif Lindholm, ryan.harkin@linaro.org
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.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
2017-02-16 20:46 ` Ard Biesheuvel
@ 2017-02-17 11:53 ` Evan Lloyd
2017-02-24 11:26 ` Leif Lindholm
0 siblings, 1 reply; 24+ messages in thread
From: Evan Lloyd @ 2017-02-17 11:53 UTC (permalink / raw)
To: ard.biesheuvel@linaro.org
Cc: edk2-devel@lists.01.org, Leif Lindholm, ryan.harkin@linaro.org
>-----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.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
2017-02-17 11:53 ` Evan Lloyd
@ 2017-02-24 11:26 ` Leif Lindholm
0 siblings, 0 replies; 24+ messages in thread
From: Leif Lindholm @ 2017-02-24 11:26 UTC (permalink / raw)
To: Evan Lloyd
Cc: ard.biesheuvel@linaro.org, edk2-devel@lists.01.org,
ryan.harkin@linaro.org
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] HardwareInterrupt2 protocol
2017-02-09 19:26 [PATCH 0/4] HardwareInterrupt2 protocol evan.lloyd
` (3 preceding siblings ...)
2017-02-09 19:26 ` [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions evan.lloyd
@ 2017-02-13 15:51 ` Evan Lloyd
2017-02-13 17:15 ` Leif Lindholm
4 siblings, 1 reply; 24+ messages in thread
From: Evan Lloyd @ 2017-02-13 15:51 UTC (permalink / raw)
To: edk2-devel@ml01.01.org
Cc: ryan.harkin@linaro.org, Leif Lindholm, ard.biesheuvel@linaro.org
I'd like to make something clear here. We found a problem and solicited opinion.
Ard very helpfully proposed a solution, complete with a rapid response example patch.
However, it was code he wrote quickly to propose a solution - and I imagine he didn't expect it to go directly into the code line.
We submitted his patches as sent, because we felt we should not take credit for his excellent work.
So any problems here are our fault, not Ard's - and he didn't get to review things before submission.
I'll tidy things up, and try again.
Evan
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>evan.lloyd@arm.com
>Sent: 09 February 2017 19:26
>To: edk2-devel@ml01.01.org
>Cc: ryan.harkin@linaro.org; Leif Lindholm; ard.biesheuvel@linaro.org
>Subject: [edk2] [PATCH 0/4] HardwareInterrupt2 protocol
>
>From: Evan Lloyd <evan.lloyd@arm.com>
>
>This series of patches corrects a problem detected on the ARM Juno
>platform that is actually generic (at least to ARM GIC platforms).
>The HardwareInterrupt protocol had no means of handling characteristics
>like Edge/Level triggered and polarity.
>
>A new HardwareInterrupt2 protocol (provided by Ard) is added,
>and code changed to utilise the new capabilities.
>
>The code is available for examination on Github at:
> https://github.com/EvanLloyd/tianocore/tree/376_irqtype_v1
>
>Ard Biesheuvel (3):
> EmbeddedPkg: introduce HardwareInterrupt2 protocol
> ArmPkg/ArmGicDxe: expose HardwareInterrupt2 protocol
> ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type
>
>Girish Pathak (1):
> ArmPkg:Provide GetTriggerType/SetTriggerType functions
>
> EmbeddedPkg/EmbeddedPkg.dec | 1 +
> ArmPkg/Drivers/ArmGic/ArmGicDxe.inf | 1 +
> ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf | 4 +-
> ArmPkg/Drivers/ArmGic/ArmGicDxe.h | 2 +
> ArmPkg/Include/Library/ArmGicLib.h | 4 +
> EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h | 181
>++++++++++++++++++++
> ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c | 2 +
> ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 181
>+++++++++++++++++++-
> ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 178
>++++++++++++++++++-
> ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 27 +--
> 10 files changed, 567 insertions(+), 14 deletions(-)
> create mode 100644
>EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h
>
>--
>Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>
>_______________________________________________
>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.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 0/4] HardwareInterrupt2 protocol
2017-02-13 15:51 ` [PATCH 0/4] HardwareInterrupt2 protocol Evan Lloyd
@ 2017-02-13 17:15 ` Leif Lindholm
0 siblings, 0 replies; 24+ messages in thread
From: Leif Lindholm @ 2017-02-13 17:15 UTC (permalink / raw)
To: Evan Lloyd
Cc: edk2-devel@ml01.01.org, ryan.harkin@linaro.org,
ard.biesheuvel@linaro.org
Hi Evan,
No, this is all fine - you've submitted his originals as "From: Ard
Biesheuvel", which retains him as original author of 1-3.
It's completely up to Ard whether he wants to get involved or not at
this point. The v1 of the series was sent out by you, so I expect any
v2 to come from you as well :)
This may be a slightly unusual situation for the edk2-devel mailing
list, but it's just business as usual on many Linux kernel lists.
Regards,
Leif
On Mon, Feb 13, 2017 at 03:51:58PM +0000, Evan Lloyd wrote:
> I'd like to make something clear here. We found a problem and
> solicited opinion. Ard very helpfully proposed a solution, complete
> with a rapid response example patch.
> However, it was code he wrote quickly to propose a solution - and I
> imagine he didn't expect it to go directly into the code line. We
> submitted his patches as sent, because we felt we should not take
> credit for his excellent work. So any problems here are our fault,
> not Ard's - and he didn't get to review things before submission.
> I'll tidy things up, and try again.
>
> Evan
>
> >-----Original Message-----
> >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> >evan.lloyd@arm.com
> >Sent: 09 February 2017 19:26
> >To: edk2-devel@ml01.01.org
> >Cc: ryan.harkin@linaro.org; Leif Lindholm; ard.biesheuvel@linaro.org
> >Subject: [edk2] [PATCH 0/4] HardwareInterrupt2 protocol
> >
> >From: Evan Lloyd <evan.lloyd@arm.com>
> >
> >This series of patches corrects a problem detected on the ARM Juno
> >platform that is actually generic (at least to ARM GIC platforms).
> >The HardwareInterrupt protocol had no means of handling characteristics
> >like Edge/Level triggered and polarity.
> >
> >A new HardwareInterrupt2 protocol (provided by Ard) is added,
> >and code changed to utilise the new capabilities.
> >
> >The code is available for examination on Github at:
> > https://github.com/EvanLloyd/tianocore/tree/376_irqtype_v1
> >
> >Ard Biesheuvel (3):
> > EmbeddedPkg: introduce HardwareInterrupt2 protocol
> > ArmPkg/ArmGicDxe: expose HardwareInterrupt2 protocol
> > ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type
> >
> >Girish Pathak (1):
> > ArmPkg:Provide GetTriggerType/SetTriggerType functions
> >
> > EmbeddedPkg/EmbeddedPkg.dec | 1 +
> > ArmPkg/Drivers/ArmGic/ArmGicDxe.inf | 1 +
> > ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf | 4 +-
> > ArmPkg/Drivers/ArmGic/ArmGicDxe.h | 2 +
> > ArmPkg/Include/Library/ArmGicLib.h | 4 +
> > EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h | 181
> >++++++++++++++++++++
> > ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c | 2 +
> > ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 181
> >+++++++++++++++++++-
> > ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 178
> >++++++++++++++++++-
> > ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 27 +--
> > 10 files changed, 567 insertions(+), 14 deletions(-)
> > create mode 100644
> >EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h
> >
> >--
> >Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >
> >_______________________________________________
> >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.
^ permalink raw reply [flat|nested] 24+ messages in thread