From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-x22f.google.com (mail-lf0-x22f.google.com [IPv6:2a00:1450:4010:c07::22f]) (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 9A4F981F3A for ; Thu, 16 Feb 2017 12:42:25 -0800 (PST) Received: by mail-lf0-x22f.google.com with SMTP id x1so13896845lff.0 for ; Thu, 16 Feb 2017 12:42:25 -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=5swqJG7fnFJIXd67TAJ7cN2hfYoS+Z/GAN+i0v2+zRM=; b=YkNPPyXD1pQWBqra2NxzMkZpyoQdblBoIbXN/Q+sGzkF9TeLcI+1eIbwh0eZkgeqPj EAjzJvgx1O2mY5yay7WowUq+BFGu2uSG6blwfWXrlaLJkFlCREnB87ceKYkp1YXpZKU+ nm6fTVwlT2GV23qxTjGK0xjhyqbQVBgLqlipM= 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=5swqJG7fnFJIXd67TAJ7cN2hfYoS+Z/GAN+i0v2+zRM=; b=XFQjv4SQolrwokp42KtqGU7Su2ug1SmUsn1fxtMo0Q4hKCH3TVCRWVkSAUPRiAGTD1 tNGPosySrQPkN7jYvLItcvgnZo5SSOaYXHiMHVkSkN1WzHiOkOwtIxzh8uzMVw4MjA6j hEZ6OKPJZxu/JHhuJXA7SR459G36fyhAXrHmkC+LAFbNwX/6fjy3EA0z2yHVJjrWvBIH +K/1aIE/aO12H3H9Rsv+MA1LdDCmkC6I4aqkGXKG4yy8yvMFGb5CNeG6uwp7bINxFVqN dDRkZzIpYjzOTnEwZrHffCCnlUPEybrpq/qgUkathzfYDyrBFUAG01KqoW1TSkziMOU+ 5MKQ== X-Gm-Message-State: AMke39kITq1jbvLtXy5tzqLU/1135OYQI7yVwhEBTT0Rs2tThl+HLKJga4H6zbq7x6B5Sh2wPSZoKCb6gd9dceqv X-Received: by 10.25.99.134 with SMTP id v6mr1330510lfi.170.1487277743508; Thu, 16 Feb 2017 12:42:23 -0800 (PST) MIME-Version: 1.0 Received: by 10.25.207.72 with HTTP; Thu, 16 Feb 2017 12:42:22 -0800 (PST) Received: by 10.25.207.72 with HTTP; Thu, 16 Feb 2017 12:42:22 -0800 (PST) In-Reply-To: References: <20170209192623.262044-1-evan.lloyd@arm.com> <20170209192623.262044-5-evan.lloyd@arm.com> <20170213121552.GU16034@bivouac.eciton.net> From: Ryan Harkin Date: Thu, 16 Feb 2017 20:42:22 +0000 Message-ID: To: Evan Lloyd Cc: Ard Biesheuvel , Leif Lindholm , "edk2-devel@ml01.01.org" X-Content-Filtered-By: Mailman/MimeDel 2.1.21 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: Thu, 16 Feb 2017 20:42:26 -0000 Content-Type: text/plain; charset=UTF-8 On 16 Feb 2017 20:27, "Evan Lloyd" 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 > >> > >> 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 > > > >(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.