From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x231.google.com (mail-it0-x231.google.com [IPv6:2607:f8b0:4001:c0b::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id F2B3681FBC for ; Mon, 13 Feb 2017 05:05:28 -0800 (PST) Received: by mail-it0-x231.google.com with SMTP id 203so185642980ith.0 for ; Mon, 13 Feb 2017 05:05:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=I3n/J9Bb1XVqFg9fNr8Bp62K0M0m2aOCA4/0VXP1xE0=; b=JZjgxwwuUhzVorLCpoByIoz3NdrVPMCFreHcGM/JcGwQWEq+ietBABs9s8jKjeUzV7 kIomcKSE57NxF1v4X6eG7Ta2EzW7xYvE1Jq2iG4JI6G9rkZIID4VfKiyipkkEbiYO/QE k3U0wj8ZaxqWRQXY9ERo8RL9lNEoVejR3CEaI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=I3n/J9Bb1XVqFg9fNr8Bp62K0M0m2aOCA4/0VXP1xE0=; b=hopKcTzRKiWoTqFXuerBntIGOicAmmLMjaBJ/5QjgE0nT7JomgO5NML2asJYzt3N98 AWVftsAzK+BOItcSo2zteflq4qaM/RBcWCSkzEx+DWxzwQ4fOLXVmC2/iWrCgDeS4WRr H2tHP2t9jWusiamkgZflE5p0u6uwJJHwqiAYUSwbdJo5HkBOX+lZPWs9we0bY2Ylzis8 4ajzpkL2UbBfXlbhiEZT/oSZHqt4IRbBLNkB6dMKpUNhh+929z46Et0eX0kkFP72Dt8N 0XIJan3SpXCrMmxsRa2xV5x/E5TmWdEgwhr4X8pWuj7Jv7+n/eEbXr+wGT87+c+POTL7 FLjQ== X-Gm-Message-State: AIkVDXJpr9k5G411uSRqtVyjU7ao1gWkXPvdEXJZ41dNRNwjvnzb1CfuVDdIwdjgvfFsySjHTw0aenHXi+USZmSI X-Received: by 10.36.23.74 with SMTP id 71mr38375697ith.37.1486991127967; Mon, 13 Feb 2017 05:05:27 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.144.139 with HTTP; Mon, 13 Feb 2017 05:05:27 -0800 (PST) In-Reply-To: <20170209192623.262044-5-evan.lloyd@arm.com> References: <20170209192623.262044-1-evan.lloyd@arm.com> <20170209192623.262044-5-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Mon, 13 Feb 2017 13:05:27 +0000 Message-ID: To: Evan Lloyd Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Ryan Harkin Subject: Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Feb 2017 13:05:29 -0000 Content-Type: text/plain; charset=UTF-8 (apologies for the delayed [and now somewhat redundant] response, this sat in my outbox since this morning) On 9 February 2017 at 19:26, wrote: > From: Girish Pathak > > 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 > Signed-off-by: Evan Lloyd > Tested-by: Girish Pathak 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") >