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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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

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