Re: [PATCH 1/5] ArmPkg: Tidy GIC code before changes.

[Evan Lloyd <Evan.Lloyd@arm.com>]

Hi Leif.
I agree/accept all the comments, except:

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: 14 September 2017 17:41
> 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>
> Subject: Re: [PATCH 1/5] ArmPkg: Tidy GIC code before changes.
> 
> On Mon, Sep 11, 2017 at 04:23:31PM +0100, evan.lloyd@arm.com wrote:
> > From: Evan Lloyd <evan.lloyd@arm.com>
> >
...
> 
> >    // whereas Affinity3 is defined at [32:39] in MPIDR
> > -  CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 |
> > ARM_CORE_AFF2)) | ((MpId & ARM_CORE_AFF3) >> 8);
> > +  CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 |
> ARM_CORE_AFF2))
> > +                | ((MpId & ARM_CORE_AFF3) >> 8);
> 
> I can't find an explicit rule on this, but my preference aligns with what
> examples I can see in the Coding Style: moving that lone '|' to the end of the
> preceding line:
> 
>   CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 |
> ARM_CORE_AFF2)) |
>                 ((MpId & ARM_CORE_AFF3) >> 8);

[[Evan Lloyd]] 5.2.1.6 and 5.7.2.4 have multiple examples of prefix style, 
  with 5.2.2.11 and 5.7.2.3 providing only 2 instances of a line suffix operator.
  I can change it if you insist, but it will be a clear instance of a 
  maintainer's personal prejudice overriding the coding standard. I strongly
  believe prefix format is much clearer, especially for compound conditions
  with nesting.

> 
> >    if (Revision == ARM_GIC_ARCH_REVISION_3) {
...
> >      // Write set-enable register
> > -    MmioWrite32 (GicCpuRedistributorBase +
> ARM_GICR_CTLR_FRAME_SIZE + ARM_GICR_ISENABLER + (4 * RegOffset), 1
> << RegShift);
> > +    MmioWrite32 (
> > +      (GicCpuRedistributorBase
> > +        + ARM_GICR_CTLR_FRAME_SIZE
> > +        + ARM_GICR_ISENABLER
> > +        + (4 * RegOffset)),
> > +      1 << RegShift
> > +      );
> 
> Maybe rewrite as
> 
> #define SET_ENABLE_ADDRESS(base,offset) ((base) +
> ARM_GICR_CTLR_FRAME_SIZE + \
>                                          ARM_GICR_ISENABLER + (4 * offset))
> 
> (further up)
> 
> then
> 
>     MmioWrite32 (
>       SET_ENABLE_ADDRESS (GicCpuRedistributorBase, RegOffset),
>       1 << RegShift
>       );
> 
> ?

[[Evan Lloyd]] Agree, but I called the macros ISENABLER_ADDRESS and ISENABLER_ADDRESS
(using the register names) because SET_ seemed to imply writing something in this context.

...