[v1] Add HardwareInterrupt2 for ARM

[edk2] [PATCH 0/4] [PATCH 0/5] Add HardwareInterrupt2 for ARM

Posted by evan.lloyd@arm.com 7 years, 3 months ago

From: EvanLloyd <evan.lloyd@arm.com>

This v4 series of patches corrects a problem detected on the ARM Juno
platform that is actually generic (at least to ARM GIC platforms).
The HardwareInterrupt protocol had no means of handling characteristics
like Edge/Level triggered and polarity.

A new HardwareInterrupt2 protocol (provided by Ard) is added, and code
changed to utilise the new capabilities.

The code is available for examination on Github at:
    https://github.com/EvanLloyd/tianocore/tree/376_irqtype_v4

v4 responds to further comments from maintainers, being mainly
cosmetic changes.

Note: Significant defects exist in the (original) Watchdog handling,
      and a new patch will follow.

Ard Biesheuvel (3):
  EmbeddedPkg: Introduce HardwareInterrupt2 protocol
  ArmPkg/ArmGicDxe: Expose HardwareInterrupt2 protocol
  ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type

Evan Lloyd (1):
  ArmPkg: Tidy GIC code before changes.

 EmbeddedPkg/EmbeddedPkg.dec                              |   1 +
 ArmPkg/Drivers/ArmGic/ArmGicDxe.inf                      |   3 +-
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf |   6 +-
 ArmPkg/Drivers/ArmGic/ArmGicDxe.h                        |  31 ++-
 ArmPkg/Include/Library/ArmGicLib.h                       |  34 +--
 EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h        | 182 ++++++++++++++++
 ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c                  |  76 +++++--
 ArmPkg/Drivers/ArmGic/ArmGicLib.c                        |  73 +++++--
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c                | 195 ++++++++++++++---
 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c                | 218 +++++++++++++++++---
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c   | 146 +++++++------
 11 files changed, 779 insertions(+), 186 deletions(-)
 create mode 100644 EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h

-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

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

Re: [edk2] [PATCH 0/4] [PATCH 0/5] Add HardwareInterrupt2 for ARM

Posted by Leif Lindholm 7 years, 3 months ago

On Thu, Sep 21, 2017 at 05:23:40PM +0100, evan.lloyd@arm.com wrote:
> From: EvanLloyd <evan.lloyd@arm.com>
> 
> This v4 series of patches corrects a problem detected on the ARM Juno
> platform that is actually generic (at least to ARM GIC platforms).
> The HardwareInterrupt protocol had no means of handling characteristics
> like Edge/Level triggered and polarity.
> 
> A new HardwareInterrupt2 protocol (provided by Ard) is added, and code
> changed to utilise the new capabilities.
> 
> The code is available for examination on Github at:
>     https://github.com/EvanLloyd/tianocore/tree/376_irqtype_v4
> 
> v4 responds to further comments from maintainers, being mainly
> cosmetic changes.
> 
> Note: Significant defects exist in the (original) Watchdog handling,
>       and a new patch will follow.

If you let me move that | before pushing, I'm happy with this set.

Arvind, Daniil, Thomas - any comments?

/
    Leif

> Ard Biesheuvel (3):
>   EmbeddedPkg: Introduce HardwareInterrupt2 protocol
>   ArmPkg/ArmGicDxe: Expose HardwareInterrupt2 protocol
>   ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type
> 
> Evan Lloyd (1):
>   ArmPkg: Tidy GIC code before changes.
> 
>  EmbeddedPkg/EmbeddedPkg.dec                              |   1 +
>  ArmPkg/Drivers/ArmGic/ArmGicDxe.inf                      |   3 +-
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf |   6 +-
>  ArmPkg/Drivers/ArmGic/ArmGicDxe.h                        |  31 ++-
>  ArmPkg/Include/Library/ArmGicLib.h                       |  34 +--
>  EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h        | 182 ++++++++++++++++
>  ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c                  |  76 +++++--
>  ArmPkg/Drivers/ArmGic/ArmGicLib.c                        |  73 +++++--
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c                | 195 ++++++++++++++---
>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c                | 218 +++++++++++++++++---
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c   | 146 +++++++------
>  11 files changed, 779 insertions(+), 186 deletions(-)
>  create mode 100644 EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h
> 
> -- 
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/4] [PATCH 0/5] Add HardwareInterrupt2 for ARM

Posted by Evan Lloyd 7 years, 3 months ago


> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: 21 September 2017 18:56
> To: Evan Lloyd <Evan.Lloyd@arm.com>
> Cc: edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>; Arvind
> Chauhan <Arvind.Chauhan@arm.com>; Daniil Egranov
> <Daniil.Egranov@arm.com>; Thomas Panakamattam Abraham
> <thomas.abraham@arm.com>
> Subject: Re: [PATCH 0/4] [PATCH 0/5] Add HardwareInterrupt2 for ARM
> 
> On Thu, Sep 21, 2017 at 05:23:40PM +0100, evan.lloyd@arm.com wrote:
> > From: EvanLloyd <evan.lloyd@arm.com>
> >
> > This v4 series of patches corrects a problem detected on the ARM Juno
> > platform that is actually generic (at least to ARM GIC platforms).
> > The HardwareInterrupt protocol had no means of handling
> > characteristics like Edge/Level triggered and polarity.
> >
> > A new HardwareInterrupt2 protocol (provided by Ard) is added, and code
> > changed to utilise the new capabilities.
> >
> > The code is available for examination on Github at:
> >     https://github.com/EvanLloyd/tianocore/tree/376_irqtype_v4
> >
> > v4 responds to further comments from maintainers, being mainly
> > cosmetic changes.
> >
> > Note: Significant defects exist in the (original) Watchdog handling,
> >       and a new patch will follow.
> 
> If you let me move that | before pushing, I'm happy with this set.

[[Evan Lloyd]] Very well, if you really feel that strongly. 
How come you have a code style rooted in the 1960s before you were born, while I've move forward to at least the 1990s.  Shouldn't I be the surly, hide-bound, old, luddite dinosaur?

> 
> Arvind, Daniil, Thomas - any comments?
> 
> /
>     Leif
> 
> > Ard Biesheuvel (3):
> >   EmbeddedPkg: Introduce HardwareInterrupt2 protocol
> >   ArmPkg/ArmGicDxe: Expose HardwareInterrupt2 protocol
> >   ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type
> >
> > Evan Lloyd (1):
> >   ArmPkg: Tidy GIC code before changes.
> >
> >  EmbeddedPkg/EmbeddedPkg.dec                              |   1 +
> >  ArmPkg/Drivers/ArmGic/ArmGicDxe.inf                      |   3 +-
> >  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf |   6 +-
> >  ArmPkg/Drivers/ArmGic/ArmGicDxe.h                        |  31 ++-
> >  ArmPkg/Include/Library/ArmGicLib.h                       |  34 +--
> >  EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h        | 182
> ++++++++++++++++
> >  ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c                  |  76 +++++--
> >  ArmPkg/Drivers/ArmGic/ArmGicLib.c                        |  73 +++++--
> >  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c                | 195
> ++++++++++++++---
> >  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c                | 218
> +++++++++++++++++---
> >  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c   | 146
> +++++++------
> >  11 files changed, 779 insertions(+), 186 deletions(-)  create mode
> > 100644 EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h
> >
> > --
> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/4] [PATCH 0/5] Add HardwareInterrupt2 for ARM

Posted by Andrew Fish 7 years, 3 months ago

> On Sep 21, 2017, at 11:39 AM, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: Leif Lindholm [mailto:leif.lindholm@linaro.org <mailto:leif.lindholm@linaro.org>]
>> Sent: 21 September 2017 18:56
>> To: Evan Lloyd <Evan.Lloyd@arm.com <mailto:Evan.Lloyd@arm.com>>
>> Cc: edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org <mailto:ard.biesheuvel@linaro.org>>;
>> Matteo Carlini <Matteo.Carlini@arm.com <mailto:Matteo.Carlini@arm.com>>; nd <nd@arm.com <mailto:nd@arm.com>>; Arvind
>> Chauhan <Arvind.Chauhan@arm.com <mailto:Arvind.Chauhan@arm.com>>; Daniil Egranov
>> <Daniil.Egranov@arm.com <mailto:Daniil.Egranov@arm.com>>; Thomas Panakamattam Abraham
>> <thomas.abraham@arm.com <mailto:thomas.abraham@arm.com>>
>> Subject: Re: [PATCH 0/4] [PATCH 0/5] Add HardwareInterrupt2 for ARM
>> 
>> On Thu, Sep 21, 2017 at 05:23:40PM +0100, evan.lloyd@arm.com wrote:
>>> From: EvanLloyd <evan.lloyd@arm.com>
>>> 
>>> This v4 series of patches corrects a problem detected on the ARM Juno
>>> platform that is actually generic (at least to ARM GIC platforms).
>>> The HardwareInterrupt protocol had no means of handling
>>> characteristics like Edge/Level triggered and polarity.
>>> 
>>> A new HardwareInterrupt2 protocol (provided by Ard) is added, and code
>>> changed to utilise the new capabilities.
>>> 
>>> The code is available for examination on Github at:
>>>    https://github.com/EvanLloyd/tianocore/tree/376_irqtype_v4
>>> 
>>> v4 responds to further comments from maintainers, being mainly
>>> cosmetic changes.
>>> 
>>> Note: Significant defects exist in the (original) Watchdog handling,
>>>      and a new patch will follow.
>> 
>> If you let me move that | before pushing, I'm happy with this set.
> 
> [[Evan Lloyd]] Very well, if you really feel that strongly. 
> How come you have a code style rooted in the 1960s before you were born, while I've move forward to at least the 1990s.  Shouldn't I be the surly, hide-bound, old, luddite dinosaur?
> 

Yikes. There is actually no scientific data that one coding standard is better than another. The data does show having a consistent coding style is what has value. 

I wrote the original coding standard a long time ago.... I remember I was a 4 space indent person, and as a compromise we moved to 2 space indent. It drove me crazy for about 2 weeks and then my brain flipped over and now I prefer 2 spaces, and 4 spaces look strange. Same kind of thing happened when Apple flipped the scroll direction. Folks went crazy, but after a while your brain gets accustomed to the new normal. 

Thanks,

Andrew Fish

>> 
>> Arvind, Daniil, Thomas - any comments?
>> 
>> /
>>    Leif
>> 
>>> Ard Biesheuvel (3):
>>>  EmbeddedPkg: Introduce HardwareInterrupt2 protocol
>>>  ArmPkg/ArmGicDxe: Expose HardwareInterrupt2 protocol
>>>  ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type
>>> 
>>> Evan Lloyd (1):
>>>  ArmPkg: Tidy GIC code before changes.
>>> 
>>> EmbeddedPkg/EmbeddedPkg.dec                              |   1 +
>>> ArmPkg/Drivers/ArmGic/ArmGicDxe.inf                      |   3 +-
>>> ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf |   6 +-
>>> ArmPkg/Drivers/ArmGic/ArmGicDxe.h                        |  31 ++-
>>> ArmPkg/Include/Library/ArmGicLib.h                       |  34 +--
>>> EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h        | 182
>> ++++++++++++++++
>>> ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c                  |  76 +++++--
>>> ArmPkg/Drivers/ArmGic/ArmGicLib.c                        |  73 +++++--
>>> ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c                | 195
>> ++++++++++++++---
>>> ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c                | 218
>> +++++++++++++++++---
>>> ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c   | 146
>> +++++++------
>>> 11 files changed, 779 insertions(+), 186 deletions(-)  create mode
>>> 100644 EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h
>>> 
>>> --
>>> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>>> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

Re: [edk2] [PATCH 0/4] [PATCH 0/5] Add HardwareInterrupt2 for ARM

Posted by Evan Lloyd 7 years, 3 months ago

Hi Andrew.

> From: afish@apple.com [mailto:afish@apple.com] 
> Sent: 21 September 2017 20:15
> To: Evan Lloyd <Evan.Lloyd@arm.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; nd <nd@arm.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org; Thomas Panakamattam Abraham <thomas.abraham@arm.com>; Arvind Chauhan <Arvind.Chauhan@arm.com>
> Subject: Re: [edk2] [PATCH 0/4] [PATCH 0/5] Add HardwareInterrupt2 for ARM
> ?
> 
> If you let me move that | before pushing, I'm happy with this set.
> 
> [[Evan Lloyd]] Very well, if you really feel that strongly. 
> How come you have a code style rooted in the 1960s before you were born, while I've move forward to at least the 1990s.  Shouldn't I be the surly, hide-bound, old, luddite dinosaur?
> 
> 
> Yikes. There is actually no scientific data that one coding standard is better than another. The data does show having a consistent coding style is what has value. 
> 
> I wrote the original coding standard a long time ago.... I remember I was a 4 space indent person, and as a compromise we moved to 2 space indent. It drove me crazy for about 2 weeks and then my brain flipped over and now I prefer 2 spaces, and 4 spaces look strange. Same kind of thing happened when Apple flipped the scroll direction. Folks went crazy, but after a while your brain gets accustomed to the new normal. 
> 
> Thanks,
> 
> Andrew Fish

[[Evan Lloyd]] Reading my comment, one could infer that I overdid calling Leif names.
Now I do do that, frequently, but generally to his face, and, if possible, over beer.
He also knows me well enough to know that I WAS there in the 1960s.

The topic under discussion is the placement of operators when splitting lines,
and the standard has no rules but lots of examples that support my case,
which is that:
    if (   (   (some long condition)
            && (Another long condition))
        || (   (something shortish || something shorter)
            && (another test)
            && (yet another check)))

is clearer than:

    if (((some long condition) &&
         (Another long condition)) ||
        ((something shortish || something shorter) &&
         (another test) &&
         (yet another check)))

A major reason for that is that aligning the operators at the start gives
an extra clue as to how they relate to each other. In the second example
the matching of operators can be obscured.

I hold that a similar case applies to ordinary operators:
    variable = (a * (calculation + with))
               / sums;
rather than:
    variable = (a * (calculation + with)) /
               sums;

Regards,
Evan

PS Outlook insists on messing with the "plain text" format, so this may not look right, sorry.

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