Re: [PATCH v1 1/2] MdePkg: Definitions for Extended Interrupt Flags

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Mike,

I will send an updated patch with the suggested changes. I will also drop the IS_xxx macros.

Regards,

Sami Mujawar

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com> 
Sent: 24 September 2020 11:07 PM
To: Sami Mujawar <Sami.Mujawar@arm.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: gaoliming@byosoft.com.cn; Liu, Zhiguang <zhiguang.liu@intel.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; Pierre Gondois <Pierre.Gondois@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Ben Adderson <Ben.Adderson@arm.com>; nd <nd@arm.com>
Subject: RE: [PATCH v1 1/2] MdePkg: Definitions for Extended Interrupt Flags

Hi Sami,

The set of define values looks more complex than needed for single bit field checks.

Perhaps we can simplify to just defining the bit locations and the IS_ macros check
for 0 or non-zer0 results?

#define EFI_ACPI_EXTENDED_INTERRUPT_FLAG_PRODUCER_CONSUMER_MASK   BIT0
#define EFI_ACPI_EXTENDED_INTERRUPT_FLAG_MODE_MASK                BIT1
#define EFI_ACPI_EXTENDED_INTERRUPT_FLAG_POLARITY_MASK            BIT2
#define EFI_ACPI_EXTENDED_INTERRUPT_FLAG_SHARABLE_MASK            BIT3
#define EFI_ACPI_EXTENDED_INTERRUPT_FLAG_WAKE_CAPABLITY_MASK      BIT4

#define IS_EXTENDED_INTERRUPT_CONSUMER(Flag)                      \
  (((Flag) & EFI_ACPI_EXTENDED_INTERRUPT_FLAG_PRODUCER_CONSUMER_MASK) != 0)

#define IS_EXTENDED_INTERRUPT_EDGE_TRIGGERED(Flag)                \
  (((Flag) & EFI_ACPI_EXTENDED_INTERRUPT_FLAG_MODE_MASK) != 0)

Are the IS_ macros really required?  I do not see those for other bit
fields in ACPI structs.

Thanks,

Mike

> -----Original Message-----
> From: Sami Mujawar <sami.mujawar@arm.com>
> Sent: Tuesday, September 22, 2020 7:08 AM
> To: devel@edk2.groups.io
> Cc: Sami Mujawar <sami.mujawar@arm.com>; Kinney, Michael D <michael.d.kinney@intel.com>; gaoliming@byosoft.com.cn; Liu, Zhiguang
> <zhiguang.liu@intel.com>; Alexei.Fedorov@arm.com; pierre.gondois@arm.com; Matteo.Carlini@arm.com; Ben.Adderson@arm.com; nd@arm.com
> Subject: [PATCH v1 1/2] MdePkg: Definitions for Extended Interrupt Flags
> 
> Add Interrupt Vector Flag definitions for Extended Interrupt
> Descriptor, and macros to test the flags.
> Ref: ACPI specification 6.4.3.6
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>  MdePkg/Include/IndustryStandard/Acpi10.h | 85 ++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/MdePkg/Include/IndustryStandard/Acpi10.h b/MdePkg/Include/IndustryStandard/Acpi10.h
> index adeb5ae8c219f31d2403fc7aa217bfb4e1e44694..fa3f0694b9cf80bf9c1a325099a970b9cf8c1426 100644
> --- a/MdePkg/Include/IndustryStandard/Acpi10.h
> +++ b/MdePkg/Include/IndustryStandard/Acpi10.h
> @@ -2,6 +2,7 @@
>    ACPI 1.0b definitions from the ACPI Specification, revision 1.0b
> 
>  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2020, Arm Limited. All rights reserved.<BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
>  **/
> 
> @@ -377,6 +378,90 @@ typedef struct {
>  #define   EFI_ACPI_MEMORY_NON_WRITABLE                  0x00
> 
>  //
> +// Interrupt Vector Flags definitions for Extended Interrupt Descriptor
> +// Ref ACPI specification 6.4.3.6
> +//
> +#define EFI_ACPI_EXTENDED_INTERRUPT_FLAG_PRODUCER_CONSUMER_MASK   BIT0
> +#define   EFI_ACPI_EXTENDED_INTERRUPT_FLAG_PRODUCER               0
> +#define   EFI_ACPI_EXTENDED_INTERRUPT_FLAG_CONSUMER               BIT0
> +
> +#define EFI_ACPI_EXTENDED_INTERRUPT_FLAG_MODE_MASK                BIT1
> +#define   EFI_ACPI_EXTENDED_INTERRUPT_FLAG_LEVEL_TRIGGERED        0
> +#define   EFI_ACPI_EXTENDED_INTERRUPT_FLAG_EDGE_TRIGGERED         BIT1
> +
> +#define EFI_ACPI_EXTENDED_INTERRUPT_FLAG_POLARITY_MASK            BIT2
> +#define   EFI_ACPI_EXTENDED_INTERRUPT_FLAG_ACTIVE_HIGH            0
> +#define   EFI_ACPI_EXTENDED_INTERRUPT_FLAG_ACTIVE_LOW             BIT2
> +
> +#define EFI_ACPI_EXTENDED_INTERRUPT_FLAG_SHARABLE_MASK            BIT3
> +#define   EFI_ACPI_EXTENDED_INTERRUPT_FLAG_EXCLUSIVE              0
> +#define   EFI_ACPI_EXTENDED_INTERRUPT_FLAG_SHARED                 BIT3
> +
> +#define EFI_ACPI_EXTENDED_INTERRUPT_FLAG_WAKE_CAPABLITY_MASK      BIT4
> +#define   EFI_ACPI_EXTENDED_INTERRUPT_FLAG_NOT_WAKE_CAPABLE       0
> +#define   EFI_ACPI_EXTENDED_INTERRUPT_FLAG_WAKE_CAPABLE           BIT4> +
> +/* Helper macros to test Extended Interrupt Resource descriptor flags.
> +*/
> +
> +/** Test the Extended Interrupt flags to determine if the Device
> +    is an Interrupt Consumer or Producer.
> +
> +  @param [in]  Flag       Extended Interrupt Resource descriptor flag.
> +
> +  @retval TRUE            Device is Interrupt Consumer.
> +  @retval FALSE           Device is Interrupt Producer.
> +*/
> +#define IS_EXTENDED_INTERRUPT_CONSUMER(Flag)                      \
> +  (((Flag) & EFI_ACPI_EXTENDED_INTERRUPT_FLAG_CONSUMER) ==        \
> +    EFI_ACPI_EXTENDED_INTERRUPT_FLAG_CONSUMER)
> +
> +/** Test if the Extended Interrupt is Edge or Level triggered.
> +
> +  @param [in]  Flag       Extended Interrupt Resource descriptor flag.
> +
> +  @retval TRUE            Interrupt is Edge triggered.
> +  @retval FALSE           Interrupt is Level triggered.
> +*/
> +#define IS_EXTENDED_INTERRUPT_EDGE_TRIGGERED(Flag)                \
> +  (((Flag) & EFI_ACPI_EXTENDED_INTERRUPT_FLAG_EDGE_TRIGGERED) ==  \
> +    EFI_ACPI_EXTENDED_INTERRUPT_FLAG_EDGE_TRIGGERED)> +
> +/** Test if the Extended Interrupt is Active Low or Active High.
> +
> +  @param [in]  Flag       Extended Interrupt Resource descriptor flag.
> +
> +  @retval TRUE            Interrupt is Active Low.
> +  @retval FALSE           Interrupt is Active High.
> +*/
> +#define IS_EXTENDED_INTERRUPT_ACTIVE_LOW(Flag)                    \
> +  (((Flag) & EFI_ACPI_EXTENDED_INTERRUPT_FLAG_ACTIVE_LOW) ==      \
> +    EFI_ACPI_EXTENDED_INTERRUPT_FLAG_ACTIVE_LOW)
> +
> +/** Test if the Extended Interrupt is Shared or Exclusive.
> +
> +  @param [in]  Flag       Extended Interrupt Resource descriptor flag.
> +
> +  @retval TRUE            Interrupt is Shared.
> +  @retval FALSE           Interrupt is Exclusive.
> +*/
> +#define IS_EXTENDED_INTERRUPT_SHARED(Flag)                        \
> +  (((Flag) & EFI_ACPI_EXTENDED_INTERRUPT_FLAG_SHARED) ==          \
> +    EFI_ACPI_EXTENDED_INTERRUPT_FLAG_SHARED)
> +
> +/** Test the Extended Interrupt flags to determine if the Device
> +    is Wake capable or not.
> +
> +  @param [in]  Flag       Extended Interrupt Resource descriptor flag.
> +
> +  @retval TRUE            Interrupt is Wake Capable.
> +  @retval FALSE           Interrupt is not Wake Capable.
> +*/
> +#define IS_EXTENDED_INTERRUPT_WAKE_CAPABLE(Flag)                  \
> +  (((Flag) & EFI_ACPI_EXTENDED_INTERRUPT_FLAG_WAKE_CAPABLE) ==    \
> +    EFI_ACPI_EXTENDED_INTERRUPT_FLAG_WAKE_CAPABLE)
> +
> +//
>  // Ensure proper structure formats
>  //
>  #pragma pack(1)
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'