From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-x234.google.com (mail-wr0-x234.google.com [IPv6:2a00:1450:400c:c0c::234]) (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 B7B7C82105 for ; Mon, 13 Feb 2017 04:21:22 -0800 (PST) Received: by mail-wr0-x234.google.com with SMTP id 89so149297235wrr.2 for ; Mon, 13 Feb 2017 04:21:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=C4/LEI1cFKHqSKD/QTGikaJx2uFhu7Z5yMJ5HvYi558=; b=h/6JuyqI3w7If/9G84wdkHjnaHZTiG0fn/6aEt5gCpJQFLH8yFnxIjuCQ3aluz9ThX hOrqKJu6BiaVp/54odJ4sce1SsHtt9U5+0EWhPxvrszxgN7kR7pij5nMkKtGq9litWNW XWinO5oZYw83+rDSzP3/3EFzJ9SCHnanTsA6g= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=C4/LEI1cFKHqSKD/QTGikaJx2uFhu7Z5yMJ5HvYi558=; b=aepaMgwGct4QKYDIzPDJxwMF4lnrFAZFCrly8wOFCZO9RmdU5cHVh3qsKqMRUWZd8+ X6Gzn4B25isnLfHSmXkRCo7KtNUyNGjRiN3F8MD7YWEaSoRwbWdYQxZn1bagfeKlP/M9 w67h1TX2WWbAwjiTQp5KsXvdIhVjaer2TQt5Vr6dn8PO/VpcIPg7VIJn4t5drrFnuRde LHIIHk8izx1D+OtLA49VRpZtUuT0JrSQJvDgPZif+j8W5k/FpUZOSfh8xfLbSl861wHd FymUwFlD9y72EBdZnM4FiEajnbm8dsjmh0rfeA9iyAwYSQD6mEqq/p8WqRuNzYmI9O/4 Tt4g== X-Gm-Message-State: AMke39noetV2iFF1mRafskc8tpC4wSeI+LZiIiWVVZqM/r/DAkanZD1sKPLX4KhpcfcYex2l X-Received: by 10.223.170.195 with SMTP id i3mr21875720wrc.123.1486988481000; Mon, 13 Feb 2017 04:21:21 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id u189sm5154929wmu.1.2017.02.13.04.21.20 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 13 Feb 2017 04:21:20 -0800 (PST) Date: Mon, 13 Feb 2017 12:21:18 +0000 From: Leif Lindholm To: evan.lloyd@arm.com Cc: edk2-devel@ml01.01.org, Ard Biesheuvel , Ryan Harkin Message-ID: <20170213122118.GV16034@bivouac.eciton.net> References: <20170209192623.262044-1-evan.lloyd@arm.com> <20170209192623.262044-3-evan.lloyd@arm.com> MIME-Version: 1.0 In-Reply-To: <20170209192623.262044-3-evan.lloyd@arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH 2/4] ArmPkg/ArmGicDxe: expose HardwareInterrupt2 protocol 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 12:21:23 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Feb 09, 2017 at 07:26:21PM +0000, evan.lloyd@arm.com wrote: > From: Ard Biesheuvel > Ard - can we have some more commit message, please? :) > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > Signed-off-by: Girish Pathak > Signed-off-by: Evan Lloyd > Tested-by: Girish Pathak > --- > 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 > #include > +#include > > 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") >