[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 07/16] xen/arm: segregate and split GIC low level functionality



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 not the actual
bit positions, but rather the offset into the field. The overall offset
is also different on gicv3, whereas the individual bits within the field
are the same.

This series is big enough as it is, no need to be bike shedding this
sort of thing, especially when this is only moving it to start with.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.