From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Evan Lloyd <evan.lloyd@arm.com>
Cc: "edk2-devel@lists.01.org" <edk2-devel@ml01.01.org>,
Leif Lindholm <leif.lindholm@linaro.org>,
Ryan Harkin <ryan.harkin@linaro.org>
Subject: Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions
Date: Mon, 13 Feb 2017 13:05:27 +0000 [thread overview]
Message-ID: <CAKv+Gu-ktztcwN57APZbD_==nwEde8Uf1z_whJbY3Pqua7bAJw@mail.gmail.com> (raw)
In-Reply-To: <20170209192623.262044-5-evan.lloyd@arm.com>
(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")
>
next prev parent reply other threads:[~2017-02-13 13:05 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
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-13 12:26 ` Leif Lindholm
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
2017-02-09 19:26 ` [PATCH 3/4] ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type 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 12:15 ` Leif Lindholm
2017-02-16 20:27 ` Evan Lloyd
2017-02-16 20:42 ` Ryan Harkin
2017-02-17 12:06 ` Evan Lloyd
2017-02-17 12:30 ` Ryan Harkin
2017-02-17 15:08 ` Alexei Fedorov
2017-02-17 18:18 ` Ard Biesheuvel
2017-02-24 14:06 ` Leif Lindholm
2017-02-13 13:05 ` Ard Biesheuvel [this message]
2017-02-16 20:16 ` Evan Lloyd
2017-02-16 20:46 ` Ard Biesheuvel
2017-02-17 11:53 ` Evan Lloyd
2017-02-24 11:26 ` Leif Lindholm
2017-02-13 15:51 ` [PATCH 0/4] HardwareInterrupt2 protocol Evan Lloyd
2017-02-13 17:15 ` Leif Lindholm
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKv+Gu-ktztcwN57APZbD_==nwEde8Uf1z_whJbY3Pqua7bAJw@mail.gmail.com' \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox