Hi Leif.
I agree/accept all the comments, except:
> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: 14 September 2017 17:41
> To: Evan Lloyd <evan.ll...@arm.com>
> Cc: edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheu...@linaro.org>;
> Matteo Carlini <matteo.carl...@arm.com>; nd <n...@arm.com>
> Subject: Re: [PATCH 1/5] ArmPkg: Tidy GIC code before changes.
>
> On Mon, Sep 11, 2017 at 04:23:31PM +0100, evan.ll...@arm.com wrote:
> > From: Evan Lloyd <evan.ll...@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.
...
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
