Re: [Xen-devel] [PATCH v3 13/16] xen/arm: Add support for GIC v3

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

Hi Julien,

On Thu, Apr 17, 2014 at 4:47 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> On 04/17/2014 12:00 PM, Vijay Kilari wrote:
>> On Thu, Apr 17, 2014 at 3:27 PM, Julien Grall <julien.grall@xxxxxxxxxx> 
>> wrote:
>>> On 04/15/2014 12:17 PM, vijay.kilari@xxxxxxxxx wrote:
>>>> +#define GICD_CTLR       (0x000)
>>>> +#define GICD_TYPER      (0x004)
>>>> +#define GICD_IIDR       (0x008)
>>>> +#define GICD_STATUSR    (0x010)
>>>> +#define GICD_SETSPI_NSR (0x040)
>>>> +#define GICD_CLRSPI_NSR (0x048)
>>>> +#define GICD_SETSPI_SR  (0x050)
>>>> +#define GICD_CLRSPI_SR  (0x058)
>>>> +#define GICD_IGROUPR    (0x080)
>>>> +#define GICD_IGROUPRN   (0x0FC)
>>>> +#define GICD_ISENABLER  (0x100)
>>>> +#define GICD_ISENABLERN (0x17C)
>>>> +#define GICD_ICENABLER  (0x180)
>>>> +#define GICD_ICENABLERN (0x1fC)
>>>> +#define GICD_ISPENDR    (0x200)
>>>> +#define GICD_ISPENDRN   (0x27C)
>>>> +#define GICD_ICPENDR    (0x280)
>>>> +#define GICD_ICPENDRN   (0x2FC)
>>>> +#define GICD_ISACTIVER  (0x300)
>>>> +#define GICD_ISACTIVERN (0x37C)
>>>> +#define GICD_ICACTIVER  (0x380)
>>>> +#define GICD_ICACTIVERN (0x3FC)
>>>> +#define GICD_IPRIORITYR (0x400)
>>>> +#define GICD_IPRIORITYRN (0x7F8)
>>>> +#define GICD_ICFGR      (0xC00)
>>>> +#define GICD_ICFGRN     (0xCFC)
>>>> +#define GICD_NSACR      (0xE00)
>>>> +#define GICD_NSACRN     (0xEFC)
>>>> +#define GICD_SGIR       (0xF00)
>>>> +#define GICD_CPENDSGIR  (0xF10)
>>>> +#define GICD_CPENDSGIRN (0xF1C)
>>>> +#define GICD_SPENDSGIR  (0xF20)
>>>> +#define GICD_SPENDSGIRN (0xF2C)
>>>> +#define GICD_IROUTER    (0x6000)
>>>> +#define GICD_IROUTERN   (0x7FF8)
>>>> +#define GICD_PIDR0      (0xFFE0)
>>>> +#define GICD_PIDR2      (0xFFE8)
>>>> +#define GICD_PIDR7      (0xFFDC)
>>>
>>> Most of this registers are the same as GICv2 except /4 right?
>>>
>>> If so, it might be interesting to drop the /4 in the GICv2 so we will be
>>> able to share most of the VGIC distr code.
>>>
>> Few registers are common. There are additional registers in V3 like
>> IROUTER, PIDR.
>
> You have some strange conception of few :). About 20 registers are
> common with GICv2.
>
>> The GICv2 defines are also used by gic-v2 driver as well which expects
>> register_address/4.
>> So changing this impacts existing GIC v2 driver as well.
>
> I would prefer to modify GIC v2 driver rather than forking too much the
> VGIC code. The change in GIC v2 is not so difficult...

   Is it ok if we can get rid of usage fixmap in GICv2 driver to
access GICD/GICH/GICC regions
and use ioremap_nocache() similar to GICv3 driver?.

>
> It will save us lots of debugging time later. If we keep your solution,
> if we find a bug in the vGICv2 distributor we have to check vGICv3
> {re,distributor}. That would mean fixing the same bug in 3 places...
>
> With my solution, you will be able to handle in common code the
> registers valid on both configuration. Then calling vgic callbacks for
> GICv3 specific registers.
>
> It will also help to support vGICv2 on GICv3.
>
> --
> Julien Grall

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