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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-07-22 at 17:00 +0530, Vijay Kilari wrote:
> On Tue, Jul 22, 2014 at 4:43 PM, Julien Grall <julien.grall@xxxxxxxxxx> 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 think all that should be in the header file are the usual masks and
offsets used to decode the register and specific names defines to match
against. e.g. for PIDR2:

        #define GICD_PIDR2_ARCH_REV_OFFSET (4)
        #define GICD_PIDR2_ARCH_REV_MASK    (0xf << GICD_PIDR2_ARCH_REV_OFFSET)
        #define GICD_PIDR2_USES_JEP_MASK    0x08
        #define GICD_PIDR2_JEP_ID_MASK      0x07
        ...

The gic code should use these to extract things it needs.

It would normally be OK to also define:
        #define GICD_PIDR2_ARCH_GICV1 1
        #define GICD_PIDR2_ARCH_GICV2 2
        #define GICD_PIDR2_ARCH_GICV3 3
to compare against, but in this case since there is a 1:1 correspondence
there doesn't seem to be much point.

The vgic code might use these to construct a value, or might have a
local define or constant with a suitable comment.

Ian.


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