From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-he1eur01on0611.outbound.protection.outlook.com [IPv6:2a01:111:f400:fe1e::611]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2E4C7820F6 for ; Thu, 16 Feb 2017 12:27:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector1-arm-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=rhEidnHSpVLCilyCWQabd8TyP3Eiviv+otspihusF6Y=; b=ndr7zQT0nNG3TL6Rckqsy8640a6gklFKwhWBs2KVQrJCU3uzsMpWt5RehfTz5qPXgO+2zbR9WEWU4tQ7XFNXd+w5t8hhCcpbPm09MiRPBgL+cMh99sR2H0bxoBSTP5UqnfHS047tOj/5sr6tP31YFUf4qckfk8c3+proz0LRpr0= Received: from AM5PR0801MB1762.eurprd08.prod.outlook.com (10.169.247.16) by AM5PR0801MB1764.eurprd08.prod.outlook.com (10.169.247.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.888.16; Thu, 16 Feb 2017 20:27:24 +0000 Received: from AM5PR0801MB1762.eurprd08.prod.outlook.com ([10.169.247.16]) by AM5PR0801MB1762.eurprd08.prod.outlook.com ([10.169.247.16]) with mapi id 15.01.0888.034; Thu, 16 Feb 2017 20:27:24 +0000 From: Evan Lloyd To: Leif Lindholm CC: "edk2-devel@ml01.01.org" , "ard.biesheuvel@linaro.org" , "ryan.harkin@linaro.org" Thread-Topic: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions Thread-Index: AQHShfLv5Yv60JzDzU2OIxp8nm+JAKFsF7fw Date: Thu, 16 Feb 2017 20:27:24 +0000 Message-ID: References: <20170209192623.262044-1-evan.lloyd@arm.com> <20170209192623.262044-5-evan.lloyd@arm.com> <20170213121552.GU16034@bivouac.eciton.net> In-Reply-To: <20170213121552.GU16034@bivouac.eciton.net> Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=Evan.Lloyd@arm.com; x-originating-ip: [217.140.96.140] x-ld-processed: f34e5979-57d9-4aaa-ad4d-b122a662184d,ExtAddr x-ms-office365-filtering-correlation-id: b0dca5bb-7d35-48ba-1496-08d456aa37f5 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(22001)(48565401081); SRVR:AM5PR0801MB1764; x-microsoft-exchange-diagnostics: 1; AM5PR0801MB1764; 7:p30uRKICbHKo/HKvisQVK/KhJVufvU3bxD4AVkqpwxOjt/3Tu8XGnTokLBPkVLlb9K2TqdpP7kU59gZeUtqfARHT81CV19eGycL+kTHWWtUs1nnkEl8YQLMTpcO6i+0w/0jYJ4rRSAnNwcPVMpksjPOR8NEZynGnFx+Yk5JkhNOZ1Fo/03P2jmEPT+1NuuKjdpB7KjrmeIAvGtMtCroIBf5wq8/ApdNrz3NFZE05wrfB/kmdNYwjFMQjujiLQc89rECKs7zccje50Gf9KIrHNt3tn9POpzfzUIKsT+GdRNyFEbbg0pNoCAauGEaLIacb2mxwsPEaSNMPK37rFzaxfA== x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(180628864354917)(192374486261705); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040375)(601004)(2401047)(5005006)(8121501046)(10201501046)(3002001)(6055026)(6041248)(20161123560025)(20161123558025)(20161123562025)(20161123564025)(20161123555025)(6072148); SRVR:AM5PR0801MB1764; BCL:0; PCL:0; RULEID:; SRVR:AM5PR0801MB1764; x-forefront-prvs: 0220D4B98D x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(7916002)(39860400002)(39840400002)(39450400003)(39850400002)(39410400002)(40434004)(24454002)(13464003)(189002)(199003)(2900100001)(54356999)(68736007)(50986999)(76176999)(74316002)(4326007)(305945005)(7736002)(2950100002)(7696004)(6916009)(6116002)(102836003)(3846002)(38730400002)(53946003)(53936002)(110136004)(3660700001)(3280700002)(6246003)(5890100001)(2906002)(5660300001)(97736004)(106116001)(105586002)(106356001)(6436002)(99286003)(55016002)(229853002)(6506006)(77096006)(86362001)(575784001)(54906002)(8676002)(66066001)(122556002)(92566002)(81156014)(33656002)(9686003)(25786008)(8936002)(189998001)(101416001)(81166006)(389900003)(44824005)(19627235001); DIR:OUT; SFP:1101; SCL:1; SRVR:AM5PR0801MB1764; H:AM5PR0801MB1762.eurprd08.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Feb 2017 20:27:24.6077 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR0801MB1764 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:27:29 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 =3D { >> 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 >=3D mGicNumInterrupts) { >> + ASSERT(Source < mGicNumInterrupts); >> + return EFI_UNSUPPORTED; >> + } >> + >> + RegOffset =3D Source / 16; >> + Field =3D Source % 16; >> + *RegAddress =3D PcdGet64 (PcdGicDistributorBase) >> + + ARM_GIC_ICDICFR >> + + (4 * RegOffset); >> + *BitNumber =3D (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 =3D 0; >> + BitNumber =3D 0; >> + >> + Status =3D GicGetDistributorIntrCfgBaseAndBitField ( >> + Source, >> + &RegAddress, >> + &BitNumber >> + ); >> + >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + *TriggerType =3D (MmioBitFieldRead32 (RegAddress, BitNumber, >BitNumber) =3D=3D 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) =3D=3D 0) { > *TriggerType =3D EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH; > } else { > *TriggerType =3D 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 i= s 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 =3D 0; >> + UINTN BitNumber =3D 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 !=3D >EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING >> + && TriggerType !=3D >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 =3D GicGetDistributorIntrCfgBaseAndBitField ( >> + Source, >> + &RegAddress, >> + &BitNumber >> + ); >> + >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + Status =3D GicV2GetInterruptSourceState ( >> + (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This, >> + Source, >> + &IntrSourceEnabled >> + ); >> + >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + Value =3D (TriggerType =3D=3D >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 =3D { >> - (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 =3D { >> + (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 =3D 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 =3D { >> 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 >=3D mGicNumInterrupts) { >> + ASSERT(FALSE); >> + return EFI_UNSUPPORTED; >> + } >> + >> + RegOffset =3D Source / 16; >> + Field =3D Source % 16; >> + *RegAddress =3D PcdGet64 (PcdGicDistributorBase) >> + + ARM_GIC_ICDICFR >> + + (4 * RegOffset); >> + *BitNumber =3D (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 =3D 0; >> + UINTN BitNumber =3D 0; >> + EFI_STATUS Status; >> + >> + Status =3D GicGetDistributorIntrCfgBaseAndBitField ( >> + Source, >> + &RegAddress, >> + &BitNumber >> + ); >> + >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + *TriggerType =3D (MmioBitFieldRead32 (RegAddress, BitNumber, >BitNumber) =3D=3D 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 !=3D >EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING >> + && TriggerType !=3D >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 =3D GicGetDistributorIntrCfgBaseAndBitField ( >> + Source, >> + &RegAddress, >> + &BitNumber >> + ); >> + >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + Status =3D GicV3GetInterruptSourceState ( >> + (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This, >> + Source, >> + &IntrSourceEnabled >> + ); >> + >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + Value =3D (TriggerType =3D=3D >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 =3D { >> - (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 =3D { >> + (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 =3D 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 confid= ential and may also be privileged. If you are not the intended recipient, p= lease 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.