Re: [PATCH v7 1/5] xen/arm: Add support for GIC v3

[Vijay Kilari <vijay.kilari@gmail.com>]

On Tue, Jul 22, 2014 at 4:43 PM, Julien Grall <julien.grall@linaro.org> wrote:
>
>
> On 22/07/14 11:43, Ian Campbell wrote:
>>
>> On Tue, 2014-07-22 at 11:01 +0100, Julien Grall wrote:
>>>
>>>
>>> On 22/07/14 10:48, Vijay Kilari wrote:
>>>>>>
>>>>>> +/*
>>>>>> + * Additional registers defined in GIC v3.
>>>>>> + * Common GICD registers are defined in gic.h
>>>>>> + */
>>>>>> +
>>>>>> +#define GICD_STATUSR                 (0x010)
>>>>>> [...][
>>>>>> +#define GICV3_GICD_PIDR0             (0x92)
>>>>>
>>>>>
>>>>> What is the distinction between variables with GIC[DR]_ prefixes and
>>>>> those with GICV3_GIC[DR]_ ones?
>>>>
>>>>
>>>> GICV3 is prefixed for indicating that there are values not the
>>>> addresses.
>>>> In anycase I will remove GICV3 prefixes and postfix _VAL
>>>
>>>
>>> Those value are GICV3 specific. If you drop the prefix we won't know
>>> what are their purpose...
>>
>>
>> I'm not so sure that's the case. We will know it is the value to use for
>> GICD_PIDR0 for any GIC which includes that register, which is at least
>> v3 and v4 right now. Calling it V3 is equally misleading as leaving it
>> out.
>> Given that we've decided to share the #defines across versions I think
>> we should leave the prefix off. The alternative is to make sure
>> everything is prefixed and to duplicate the definitions for each
>> version, which is an approach we previously moved away from I think, I
>> don't see a strong reason to go back on that decision now.
>
>
>
> On GICv2, this field is called ICPIDR0 (same register offset) and is equal
> to 0x90.
>
> If those values are only used for the vgic v3 driver, then they should live
> in the c files and not in the common header.

These PIDR values(not all) are used by both vgic-v3 and gicv3 driver. So those
are kept in common header file.

I plan to keep all defines without GICV3 prefix and add V3
specific one GICD_PIDR2_ARCH_V3_VAL

 #define GICD_PIDR0_VAL               (0x92)
 #define GICD_PIDR1_VAL               (0xb4)
-#define GICD_PIDR2_ARCH_VAL          (0x30)
+#define GICD_PIDR2_ARCH_V3_VAL       (0x3)
 #define GICD_PIDR2_VAL               (0x3b)
 #define GICD_PIDR4_VAL               (0x04)
 #define GICD_PIDR2_ARCH_MASK         (0xf0)
+#define GICD_PIDR2_ARCH_SHIFT        (4)

 #define GICR_PIDR0_VAL               (0x93)
 #define GICR_PIDR1_VAL               GICD_PIDR1_VAL
-#define GICR_PIDR2_ARCH_VAL          GICD_PIDR2_ARCH_VAL
+#define GICR_PIDR2_ARCH_V3_VAL       GICD_PIDR2_ARCH_V3_VAL
 #define GICR_PIDR2_VAL               GICD_PIDR2_VAL
 #define GICR_PIDR4_VAL               GICD_PIDR4_VAL
 #define GICR_PIDR2_ARCH_MASK         GICD_PIDR2_ARCH_MASK
+#define GICR_PIDR2_ARCH_SHIFT        GICD_PIDR2_ARCH_SHIFT