From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 07/16] xen/arm: segregate and split GIC low level functionality Date: Wed, 23 Apr 2014 18:09:43 +0100 Message-ID: <5357F3D7.3030803@linaro.org> References: <1397560675-29861-1-git-send-email-vijay.kilari@gmail.com> <1397560675-29861-8-git-send-email-vijay.kilari@gmail.com> <534D7BDC.1010308@linaro.org> <1398264941.18537.157.camel@kazak.uk.xensource.com> <5357D5BB.6080504@linaro.org> <5357EE99.90507@linaro.org> <1398272629.18537.223.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1398272629.18537.223.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: vijay.kilari@gmail.com, stefano.stabellini@eu.citrix.com, Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org On 04/23/2014 06:03 PM, Ian Campbell wrote: > On Wed, 2014-04-23 at 17:47 +0100, Julien Grall wrote: >> On 04/23/2014 04:01 PM, Julien Grall wrote: >>> On 04/23/2014 03:55 PM, Ian Campbell wrote: >>>> On Tue, 2014-04-15 at 19:35 +0100, Julien Grall wrote: >>>>>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h >>>>>> index eba41ee..2387e38 100644 >>>>>> --- a/xen/include/asm-arm/gic.h >>>>>> +++ b/xen/include/asm-arm/gic.h >>>>>> @@ -43,12 +43,41 @@ >>>>>> #define SGI_TARGET_OTHERS 1 >>>>>> #define SGI_TARGET_SELF 2 >>>>>> >>>>>> +#define GICH_LR_PENDING 1 >>>>>> +#define GICH_LR_ACTIVE 2 >>>>>> + >>>>> >>>>> Prefixing by GICH_ is confusing. I though we were using it for to set >>>>> the value in the hardware. Can you rename it? >>>> >>>> This is code motion of an existing definition, definitely please don't >>>> rename it at the same time. >>>> >>>> But anyway, these are bits relating to the v2 GICH_LR register, which is >>>> the same on v3 as it happens. So I think the names are fine. >>> >>> Reading again the code, you are right. I was confused then of the value >>> and the position at the same time. >> >> After thinking it would be nice to keep the shift in it smth like: >> >> #define GICH_LR_PENDING (1 << 0) >> #define GICH_LR_ACTION (1 << 1) > > That's going to be slightly misleading since those are the actual bit > positions, which are also different on gicv3. > I don't get your point here. This show only the bit position within the field. If this position is also different on GICv3 why this is common code? And enhance GICH_ prefix which may lead to understand it's used to compute HW lrs. -- Julien Grall