[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 18:09 +0100, Julien Grall wrote: > 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. Writing GICH_LR_PENDING as (1 << 0) as you are suggesting would look like the pending bit was bit 0 of the relevant register, and would not IMHO improve the clarity. > 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. On gic v2 GICH_LRn.[29:28] are the state field. On gic v3 ICH_LRn_EL2.[63:32] are the state field. Both field are defined to have the same values (gic v3 literally says "as for gic v2"), and is defined as: 00 invalid 01 pending 10 active 11 pending and active. which is what the #defines above are. Anyway, whether or not this should be changed I don't think this series is the place to do it, it is big enough and unwieldy enough as it is and Vijay has plenty of stuff on his plate without tacking arbitrary cleanups on to it just because he happens to move the code around. If it bothers you so much then please send a patch *after* the gic v3 support is committed. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |