diff mbox

[edk2] ArmPkg/ArmGicLib: manage GICv3 SPI state at the distributor

Message ID 1467912167-4303-1-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit 28f8d28faabf50a82ef8d137308592c64ea9e2b6
Headers show

Commit Message

Ard Biesheuvel July 7, 2016, 5:22 p.m. UTC 
Unlike SGIs and PPIs, which are private to the CPU and are managed at
the redistributor level (which is also a per-CPU construct), shared
interrupts (SPIs) are shared between all CPUs, and therefore managed at
the distributor level.

Reported-by: Narinder Dhillon <ndhillonv2@gmail.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c | 25 +++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Ard Biesheuvel July 13, 2016, 1:29 p.m. UTC | #1 
On 7 July 2016 at 19:22, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Unlike SGIs and PPIs, which are private to the CPU and are managed at

> the redistributor level (which is also a per-CPU construct), shared

> interrupts (SPIs) are shared between all CPUs, and therefore managed at

> the distributor level.

>

> Reported-by: Narinder Dhillon <ndhillonv2@gmail.com>

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


Ping?

> ---

>  ArmPkg/Drivers/ArmGic/ArmGicLib.c | 25 +++++++++++++++++---

>  1 file changed, 22 insertions(+), 3 deletions(-)

>

> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c

> index 248e896c4b94..73795ed4e56c 100644

> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c

> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c

> @@ -20,6 +20,19 @@

>  #include <Library/PcdLib.h>

>

>  /**

> + *

> + * Return whether the Source interrupt index refers to a shared interrupt (SPI)

> + */

> +STATIC

> +BOOLEAN

> +SourceIsSpi (

> +  IN UINTN  Source

> +  )

> +{

> +  return Source >= 32 && Source < 1020;

> +}

> +

> +/**

>   * Return the base address of the GIC redistributor for the current CPU

>   *

>   * @param Revision  GIC Revision. The GIC redistributor might have a different

> @@ -183,7 +196,9 @@ ArmGicEnableInterrupt (

>    RegShift = Source % 32;

>

>    Revision = ArmGicGetSupportedArchRevision ();

> -  if ((Revision == ARM_GIC_ARCH_REVISION_2) || FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {

> +  if ((Revision == ARM_GIC_ARCH_REVISION_2) ||

> +      FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||

> +      SourceIsSpi (Source)) {

>      // Write set-enable register

>      MmioWrite32 (GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset), 1 << RegShift);

>    } else {

> @@ -216,7 +231,9 @@ ArmGicDisableInterrupt (

>    RegShift = Source % 32;

>

>    Revision = ArmGicGetSupportedArchRevision ();

> -  if ((Revision == ARM_GIC_ARCH_REVISION_2) || FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {

> +  if ((Revision == ARM_GIC_ARCH_REVISION_2) ||

> +      FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||

> +      SourceIsSpi (Source)) {

>      // Write clear-enable register

>      MmioWrite32 (GicDistributorBase + ARM_GIC_ICDICER + (4 * RegOffset), 1 << RegShift);

>    } else {

> @@ -249,7 +266,9 @@ ArmGicIsInterruptEnabled (

>    RegShift = Source % 32;

>

>    Revision = ArmGicGetSupportedArchRevision ();

> -  if ((Revision == ARM_GIC_ARCH_REVISION_2) || FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {

> +  if ((Revision == ARM_GIC_ARCH_REVISION_2) ||

> +      FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||

> +      SourceIsSpi (Source)) {

>      Interrupts = ((MmioRead32 (GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset)) & (1 << RegShift)) != 0);

>    } else {

>      GicCpuRedistributorBase = GicGetCpuRedistributorBase (GicRedistributorBase, Revision);

> --

> 2.7.4

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm July 13, 2016, 2:32 p.m. UTC | #2 
On Wed, Jul 13, 2016 at 03:29:03PM +0200, Ard Biesheuvel wrote:
> On 7 July 2016 at 19:22, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> > Unlike SGIs and PPIs, which are private to the CPU and are managed at

> > the redistributor level (which is also a per-CPU construct), shared

> > interrupts (SPIs) are shared between all CPUs, and therefore managed at

> > the distributor level.

> >

> > Reported-by: Narinder Dhillon <ndhillonv2@gmail.com>

> > Contributed-under: TianoCore Contribution Agreement 1.0

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> 

> Ping?


Sorry, this one slipped between the cracks.

It would have helped this bear of very little brain had the commit
message mentioned "(just as on GICv2)". Could you add that before
pushing? If so:

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


> > ---

> >  ArmPkg/Drivers/ArmGic/ArmGicLib.c | 25 +++++++++++++++++---

> >  1 file changed, 22 insertions(+), 3 deletions(-)

> >

> > diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c

> > index 248e896c4b94..73795ed4e56c 100644

> > --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c

> > +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c

> > @@ -20,6 +20,19 @@

> >  #include <Library/PcdLib.h>

> >

> >  /**

> > + *

> > + * Return whether the Source interrupt index refers to a shared interrupt (SPI)

> > + */

> > +STATIC

> > +BOOLEAN

> > +SourceIsSpi (

> > +  IN UINTN  Source

> > +  )

> > +{

> > +  return Source >= 32 && Source < 1020;

> > +}

> > +

> > +/**

> >   * Return the base address of the GIC redistributor for the current CPU

> >   *

> >   * @param Revision  GIC Revision. The GIC redistributor might have a different

> > @@ -183,7 +196,9 @@ ArmGicEnableInterrupt (

> >    RegShift = Source % 32;

> >

> >    Revision = ArmGicGetSupportedArchRevision ();

> > -  if ((Revision == ARM_GIC_ARCH_REVISION_2) || FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {

> > +  if ((Revision == ARM_GIC_ARCH_REVISION_2) ||

> > +      FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||

> > +      SourceIsSpi (Source)) {

> >      // Write set-enable register

> >      MmioWrite32 (GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset), 1 << RegShift);

> >    } else {

> > @@ -216,7 +231,9 @@ ArmGicDisableInterrupt (

> >    RegShift = Source % 32;

> >

> >    Revision = ArmGicGetSupportedArchRevision ();

> > -  if ((Revision == ARM_GIC_ARCH_REVISION_2) || FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {

> > +  if ((Revision == ARM_GIC_ARCH_REVISION_2) ||

> > +      FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||

> > +      SourceIsSpi (Source)) {

> >      // Write clear-enable register

> >      MmioWrite32 (GicDistributorBase + ARM_GIC_ICDICER + (4 * RegOffset), 1 << RegShift);

> >    } else {

> > @@ -249,7 +266,9 @@ ArmGicIsInterruptEnabled (

> >    RegShift = Source % 32;

> >

> >    Revision = ArmGicGetSupportedArchRevision ();

> > -  if ((Revision == ARM_GIC_ARCH_REVISION_2) || FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {

> > +  if ((Revision == ARM_GIC_ARCH_REVISION_2) ||

> > +      FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||

> > +      SourceIsSpi (Source)) {

> >      Interrupts = ((MmioRead32 (GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset)) & (1 << RegShift)) != 0);

> >    } else {

> >      GicCpuRedistributorBase = GicGetCpuRedistributorBase (GicRedistributorBase, Revision);

> > --

> > 2.7.4

> >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel July 13, 2016, 2:38 p.m. UTC | #3 
On 13 July 2016 at 16:32, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Jul 13, 2016 at 03:29:03PM +0200, Ard Biesheuvel wrote:

>> On 7 July 2016 at 19:22, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>> > Unlike SGIs and PPIs, which are private to the CPU and are managed at

>> > the redistributor level (which is also a per-CPU construct), shared

>> > interrupts (SPIs) are shared between all CPUs, and therefore managed at

>> > the distributor level.

>> >

>> > Reported-by: Narinder Dhillon <ndhillonv2@gmail.com>

>> > Contributed-under: TianoCore Contribution Agreement 1.0

>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>

>> Ping?

>

> Sorry, this one slipped between the cracks.

>

> It would have helped this bear of very little brain had the commit

> message mentioned "(just as on GICv2)". Could you add that before

> pushing? If so:

>

> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

>


Pushed, thanks.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 248e896c4b94..73795ed4e56c 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -20,6 +20,19 @@ 
 #include <Library/PcdLib.h>
 
 /**
+ *
+ * Return whether the Source interrupt index refers to a shared interrupt (SPI)
+ */
+STATIC
+BOOLEAN
+SourceIsSpi (
+  IN UINTN  Source
+  )
+{
+  return Source >= 32 && Source < 1020;
+}
+
+/**
  * Return the base address of the GIC redistributor for the current CPU
  *
  * @param Revision  GIC Revision. The GIC redistributor might have a different
@@ -183,7 +196,9 @@  ArmGicEnableInterrupt (
   RegShift = Source % 32;
 
   Revision = ArmGicGetSupportedArchRevision ();
-  if ((Revision == ARM_GIC_ARCH_REVISION_2) || FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {
+  if ((Revision == ARM_GIC_ARCH_REVISION_2) ||
+      FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
+      SourceIsSpi (Source)) {
     // Write set-enable register
     MmioWrite32 (GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset), 1 << RegShift);
   } else {
@@ -216,7 +231,9 @@  ArmGicDisableInterrupt (
   RegShift = Source % 32;
 
   Revision = ArmGicGetSupportedArchRevision ();
-  if ((Revision == ARM_GIC_ARCH_REVISION_2) || FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {
+  if ((Revision == ARM_GIC_ARCH_REVISION_2) ||
+      FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
+      SourceIsSpi (Source)) {
     // Write clear-enable register
     MmioWrite32 (GicDistributorBase + ARM_GIC_ICDICER + (4 * RegOffset), 1 << RegShift);
   } else {
@@ -249,7 +266,9 @@  ArmGicIsInterruptEnabled (
   RegShift = Source % 32;
 
   Revision = ArmGicGetSupportedArchRevision ();
-  if ((Revision == ARM_GIC_ARCH_REVISION_2) || FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {
+  if ((Revision == ARM_GIC_ARCH_REVISION_2) ||
+      FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
+      SourceIsSpi (Source)) {
     Interrupts = ((MmioRead32 (GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset)) & (1 << RegShift)) != 0);
   } else {
     GicCpuRedistributorBase = GicGetCpuRedistributorBase (GicRedistributorBase, Revision);