From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Leif Lindholm <leif.lindholm@linaro.org>
Cc: evan.lloyd@arm.com, edk2-devel@ml01.01.org,
Ryan Harkin <ryan.harkin@linaro.org>
Subject: Re: [PATCH 2/4] ArmPkg/ArmGicDxe: expose HardwareInterrupt2 protocol
Date: Mon, 13 Feb 2017 13:26:49 +0100 [thread overview]
Message-ID: <588E0A4A-89A9-4136-B8CE-59FEC94DDAE7@linaro.org> (raw)
In-Reply-To: <20170213122118.GV16034@bivouac.eciton.net>
> On 13 Feb 2017, at 13:21, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
>> On Thu, Feb 09, 2017 at 07:26:21PM +0000, evan.lloyd@arm.com wrote:
>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>
> Ard - can we have some more commit message, please? :)
>
This patch was only a PoC, and needs to be merged with 4/4 imo.
The STATICs should be kept, and the other cosmetic changes could also be dropped afaict
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> 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>
>> ---
>> 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 <Protocol/Cpu.h>
>> #include <Protocol/HardwareInterrupt.h>
>> +#include <Protocol/HardwareInterrupt2.h>
>>
>> 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")
>>
next prev parent reply other threads:[~2017-02-13 12:26 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 [this message]
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
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=588E0A4A-89A9-4136-B8CE-59FEC94DDAE7@linaro.org \
--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