[PATCH v1 1/1] DynamicTables: Fix DT PCI interrupt flags parsing

[PATCH v1 1/1] DynamicTables: Fix DT PCI interrupt flags parsing

[PierreGondois]

From: Pierre Gondois <pierre.gondois@arm.com>

Device Tree PCI interrupt flags use the convention described at
linux/Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml

The 3rd cell is the flags, encoded as follows:
  bits[3:0] trigger type and level flags.
  1 = low-to-high edge triggered
  2 = high-to-low edge triggered (invalid for SPIs)
  4 = active high level-sensitive
  8 = active low level-sensitive (invalid for SPIs).

Fix the incorrect code.

Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
---
 .../Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c     | 2 +-
 DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtUtility.h        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index a34018151f2d..d5b1c153e98f 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -449,7 +449,7 @@ GeneratePrt (
     if ((Index > 0)   &&
         (IrqMapInfo->IntcInterrupt.Interrupt >= 32)   &&
         (IrqMapInfo->IntcInterrupt.Interrupt < 1020)  &&
-        ((IrqMapInfo->IntcInterrupt.Flags & 0x3) != BIT0))
+        ((IrqMapInfo->IntcInterrupt.Flags & 0x3) != 0))
     {
       Status = EFI_INVALID_PARAMETER;
       ASSERT_EFI_ERROR (Status);
diff --git a/DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtUtility.h b/DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtUtility.h
index f2f425632b10..3f5d131d9ae5 100644
--- a/DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtUtility.h
+++ b/DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtUtility.h
@@ -60,7 +60,7 @@
 #define SPI_OFFSET  (32U)
 #define DT_PPI_IRQ  (1U)
 #define DT_SPI_IRQ  (0U)
-#define DT_IRQ_IS_EDGE_TRIGGERED(x)  ((((x) & (BIT0 | BIT2)) != 0))
+#define DT_IRQ_IS_EDGE_TRIGGERED(x)  ((((x) & (BIT0 | BIT1)) != 0))
 #define DT_IRQ_IS_ACTIVE_LOW(x)      ((((x) & (BIT1 | BIT3)) != 0))
 #define IRQ_TYPE_OFFSET    (0U)
 #define IRQ_NUMBER_OFFSET  (1U)
-- 
2.25.1

Re: [edk2-devel] [PATCH v1 1/1] DynamicTables: Fix DT PCI interrupt flags parsing

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]

Hi Pierre,

Please find my response inline marked [SAMI]

Regards,

Sami Mujawar

On Wed, Apr 27, 2022 at 07:49 AM, PierreGondois wrote:

> 
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPci=
> 
> eGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtP=
> 
> cieGenerator.c
> index a34018151f2d..d5b1c153e98f 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera=
> 
> tor.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera=
> 
> tor.c
> @@ -449,7 +449,7 @@ GeneratePrt (
> if ((Index > 0) &&
> (IrqMapInfo->IntcInterrupt.Interrupt >=3D 32) &&
> (IrqMapInfo->IntcInterrupt.Interrupt < 1020) &&
> - ((IrqMapInfo->IntcInterrupt.Flags & 0x3) !=3D BIT0))
> + ((IrqMapInfo->IntcInterrupt.Flags & 0x3) !=3D 0))

[SAMI] The BSA 1.0 says " Each legacy interrupt SPI must be programmed as level-sensitive ".
Considering this, I think the above check should be ((IrqMapInfo->IntcInterrupt.Flags & 0xB) != 0).
If you agree, please let me know I will make that change locally before pushing.
[/SAMI]

> 
> {
> Status =3D EFI_INVALID_PARAMETER;
> ASSERT_EFI_ERROR (Status);
> diff --git a/DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtUtility.h b/D=
> 
> ynamicTablesPkg/Library/FdtHwInfoParserLib/FdtUtility.h

[-- Attachment #2: Type: text/html, Size: 2074 bytes --]

Re: [edk2-devel] [PATCH v1 1/1] DynamicTables: Fix DT PCI interrupt flags parsing

[PierreGondois]

Hi Sami

On 7/18/22 14:27, Sami Mujawar wrote:
> Hi Pierre,
> 
> Please find my response inline marked [SAMI]
> 
> Regards,
> 
> Sami Mujawar
> 
> On Wed, Apr 27, 2022 at 07:49 AM, PierreGondois wrote:
> 
>     diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPci=
>     eGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtP=
>     cieGenerator.c
>     index a34018151f2d..d5b1c153e98f 100644
>     --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera=
>     tor.c
>     +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera=
>     tor.c
>     @@ -449,7 +449,7 @@ GeneratePrt (
>     if ((Index > 0) &&
>     (IrqMapInfo->IntcInterrupt.Interrupt >=3D 32) &&
>     (IrqMapInfo->IntcInterrupt.Interrupt < 1020) &&
>     - ((IrqMapInfo->IntcInterrupt.Flags & 0x3) !=3D BIT0))
>     + ((IrqMapInfo->IntcInterrupt.Flags & 0x3) !=3D 0))
> 
> [SAMI] The BSA 1.0 says "Each legacy interrupt SPI must be programmed as level-sensitive".
> Considering this, I think the above check should be ((IrqMapInfo->IntcInterrupt.Flags & 0xB) != 0).
> If you agree, please let me know I will make that change locally before pushing.
> [/SAMI]

[Pierre]
Yes indeed,
Thanks for the review and changing this without requiring a v2,

Regards,
Pierre

[/Pierre]

> 
>     {
>     Status =3D EFI_INVALID_PARAMETER;
>     ASSERT_EFI_ERROR (Status);
>     diff --git a/DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtUtility.h b/D=
>     ynamicTablesPkg/Library/FdtHwInfoParserLib/FdtUtility.h
> 

Re: [edk2-devel] [PATCH v1 1/1] DynamicTables: Fix DT PCI interrupt flags parsing

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 183 bytes --]

Hi Pierre,

I will make that change locally before pushing.
Otherwise this patch looks good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

[-- Attachment #2: Type: text/html, Size: 226 bytes --]

Re: [PATCH v1 1/1] DynamicTables: Fix DT PCI interrupt flags parsing

[Sami Mujawar]

Pushed as 039bdb4d3e96..fc4a132c0e9d

Regards,

Sami Mujawar

On 27/04/2022 03:49 pm, Pierre.Gondois@arm.com wrote:
> From: Pierre Gondois <pierre.gondois@arm.com>
>
> Device Tree PCI interrupt flags use the convention described at
> linux/Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml
>
> The 3rd cell is the flags, encoded as follows:
>    bits[3:0] trigger type and level flags.
>    1 = low-to-high edge triggered
>    2 = high-to-low edge triggered (invalid for SPIs)
>    4 = active high level-sensitive
>    8 = active low level-sensitive (invalid for SPIs).
>
> Fix the incorrect code.
>
> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
> ---
>   .../Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c     | 2 +-
>   DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtUtility.h        | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> index a34018151f2d..d5b1c153e98f 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> @@ -449,7 +449,7 @@ GeneratePrt (
>       if ((Index > 0)   &&
>           (IrqMapInfo->IntcInterrupt.Interrupt >= 32)   &&
>           (IrqMapInfo->IntcInterrupt.Interrupt < 1020)  &&
> -        ((IrqMapInfo->IntcInterrupt.Flags & 0x3) != BIT0))
> +        ((IrqMapInfo->IntcInterrupt.Flags & 0x3) != 0))
>       {
>         Status = EFI_INVALID_PARAMETER;
>         ASSERT_EFI_ERROR (Status);
> diff --git a/DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtUtility.h b/DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtUtility.h
> index f2f425632b10..3f5d131d9ae5 100644
> --- a/DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtUtility.h
> +++ b/DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtUtility.h
> @@ -60,7 +60,7 @@
>   #define SPI_OFFSET  (32U)
>   #define DT_PPI_IRQ  (1U)
>   #define DT_SPI_IRQ  (0U)
> -#define DT_IRQ_IS_EDGE_TRIGGERED(x)  ((((x) & (BIT0 | BIT2)) != 0))
> +#define DT_IRQ_IS_EDGE_TRIGGERED(x)  ((((x) & (BIT0 | BIT1)) != 0))
>   #define DT_IRQ_IS_ACTIVE_LOW(x)      ((((x) & (BIT1 | BIT3)) != 0))
>   #define IRQ_TYPE_OFFSET    (0U)
>   #define IRQ_NUMBER_OFFSET  (1U)
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.