From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x22d.google.com (mail-wm0-x22d.google.com [IPv6:2a00:1450:400c:c09::22d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C03CB21E1B76D for ; Thu, 21 Sep 2017 10:40:13 -0700 (PDT) Received: by mail-wm0-x22d.google.com with SMTP id q124so3980382wmb.0 for ; Thu, 21 Sep 2017 10:43:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=k7poWYW3ogNhUN+atTNEp20HypXV78jI2t8hHq9lDHo=; b=YMLsGVeN1QCBZywvgE81zCUIFwhFdIdmSGE+o7MNrGdrZfUY5fAGsw1/Hfk3RzkEco x6QqzpRm04tlcResmipPeYs51lbAWy0ezalRMEvBqiqg7ZgF2nR7AnT+D/EcWjaLtLgv cH9SwBVY1qfCRjPnaDY9Z9qYPi/YXY1NyRg3c= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=k7poWYW3ogNhUN+atTNEp20HypXV78jI2t8hHq9lDHo=; b=odXF4i2bw99NOMwBRqgB3sC2N2WuLF0BsaLDYrcY2I6xo7b+HFe9Ssc5TM8BCx3kBE tixc/xgIoNsWnxnUaaxaunCsw53Ovr1YzyI62oeMOn/yQ+w51T/Xdm8X8LglHbQSAMRE L5WvgQIbA0pDy+DpXQHK7SkcBK51qqvjrW+BfzCtq0llL3Y5eK3W7LZrSXvuvMHaMgAi 16qWIRptONDfBY3gIf6FX9L6MAK4MioFhntU2qyVcC1nsswNscwad9yOiba3NRHgDfvy Uomv7SjmQ0X0E034D+zFrdPsxY0Pv8BGb4OMEb1bmVDoTY3gc/F/fnG8wLkRgEVnfook 8aPA== X-Gm-Message-State: AHPjjUgNzpCOpl+ArAbI+eRRle1SFHEINsuR8geHm4keT75QrNOxM+tV il3Q0jzToBmrn5AfwlAZvIRmWg== X-Google-Smtp-Source: AOwi7QCyIxVS8YeRb/O/5QLiM7Ucjgyoq+BD0aPkEg4oA0WKlVVWBJQ/rs6CGm9/Eq1lu0LF/i8RDA== X-Received: by 10.28.174.67 with SMTP id x64mr1833122wme.82.1506015799273; Thu, 21 Sep 2017 10:43:19 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id b47sm2565508wra.73.2017.09.21.10.43.17 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 21 Sep 2017 10:43:18 -0700 (PDT) Date: Thu, 21 Sep 2017 18:43:16 +0100 From: Leif Lindholm To: Evan Lloyd Cc: "edk2-devel@lists.01.org" , Ard Biesheuvel , Matteo Carlini , nd Message-ID: <20170921174316.wn75y3x77z472rc4@bivouac.eciton.net> References: <20170911152335.72672-1-evan.lloyd@arm.com> <20170911152335.72672-2-evan.lloyd@arm.com> <20170914164109.foiwrmrdbm7aftd6@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH 1/5] ArmPkg: Tidy GIC code before changes. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Sep 2017 17:40:14 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Sep 21, 2017 at 03:34:07PM +0000, Evan Lloyd wrote: > 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 > > Cc: edk2-devel@lists.01.org; Ard Biesheuvel ; > > Matteo Carlini ; nd > > 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 > > > > ... > > > > > // 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. The example of 5.2.1.6 already violates the rule of 5.2.2.11. But the whole problem is that there is no explicit rule about prefix/suffix, which leads to... > I can change it if you insist, but it will be a clear instance of a > maintainer's personal prejudice overriding the coding standard. If you can point me to a rule I have missed, then yes, you would be absolutely correct. But I can only find uses, of both variants, in examples explaining other rules. If not, I am doing exactly what I am meant to be doing. Since the maintainer is the one who a) has to review patches without prior understanding of what the author was thinking and b) the one who needs to review future modifications to that code, it would be highly irresponsible to not ask for the code to be presented in the form most clear to them, where this does not conflict with the coding style. > I strongly believe prefix format is much clearer, especially for > compound conditions with nesting. Then why this prefix format not uniformly used in the patch? Further down, there is --- - CpuTarget = MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3); + CpuTarget = MpId & + (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3); --- Now, a) I think that's very clear and b) any other way pushes the length of the second line over 80 characters. But it does mean you are not adhering to the rules you are arguing for. > > > > > 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. Yes, this is much better. Another reason why I keep banging on about macros - they let you add descriptive semantics that makes the code easier to understand for someone unfamiliar with it. Regards, Leif