[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v1 08/10] xen/arm: Add support for GIC v3
Hello Vijay, Thank you for the patch. On 03/19/2014 02:17 PM, vijay.kilari@xxxxxxxxx wrote: > From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx> > > Add support for GIC v3 specification. > This driver assumes that ARE and SRE > is enable by default. Can you expand what mean ARE and SRE here? Can you explain how do you configure the GICv3 ... [..] > +struct gic_state_data { > + uint32_t gic_hcr, gic_vmcr; > + uint32_t gic_apr0[4]; > + uint32_t gic_apr1[4]; > + uint64_t gic_lr[16]; > +}; > + > +#define GICD ((volatile unsigned char *) gic.map_dbase) You only need to cast into unsigned char *. IO read/write will take care of the volatile. > +/* Only one region is implemented which is enough for 0-31 cpus */ > +#define GICR ((volatile unsigned char *) gic.rdist_regions[0].map_rdist_base) > + > +/* per-cpu re-distributor base */ > +static DEFINE_PER_CPU(paddr_t, rbase); > +static DEFINE_PER_CPU(paddr_t, phy_rbase); > + > +static unsigned nr_lrs; > +static uint32_t nr_priorities; > + > +/* The GIC mapping of CPU interfaces does not necessarily match the > + * logical CPU numbering. Let's use mapping as returned by the GIC > + * itself > + */ > + > +#define gic_data_rdist_rd_base() (this_cpu(rbase)) > +#define gic_data_rdist_sgi_base() (gic_data_rdist_rd_base() + SZ_64K) > + > +static inline u64 read_cpuid_mpidr(void) > +{ > + return READ_SYSREG(MPIDR_EL1); MPDIR_EL1 is already replicated in current_cpu_data.mpidr.bits [..] > +static void gic_enable_sre(void) > +{ > + uint32_t val; > + Can you explain in a comment what you are enabling here .... > + val = READ_SYSREG32(ICC_SRE_EL2); > + val |= GICC_SRE_EL2_SRE | GICC_SRE_EL2_ENEL1 | GICC_SRE_EL2_DFB | > GICC_SRE_EL2_DIB; > + WRITE_SYSREG32(val, ICC_SRE_EL2); > + isb(); > +} > + > +/* Wait for completion of a distributor change */ > +static void gic_do_wait_for_rwp(paddr_t base) > +{ > + u32 val; > + do { > + val = readl_relaxed((void *)base + GICD_CTLR); > + val = readl_relaxed(GICD + GICD_CTLR); > + val = GICD[GICD_CTLR]; > + cpu_relax(); > + } while (val & GICD_CTLR_RWP); > +} > + > +static void gic_dist_wait_for_rwp(void) > +{ > + gic_do_wait_for_rwp((paddr_t)GICD); > +} > + > +static void gic_redist_wait_for_rwp(void) > +{ > + gic_do_wait_for_rwp(gic_data_rdist_rd_base()); > +} > + > +static void gic_wait_for_rwp(int irq) > +{ > + if (irq < 32) > + gic_redist_wait_for_rwp(); > + else > + gic_dist_wait_for_rwp(); > +} > + > +static unsigned int gic_mask_cpu(const cpumask_t *cpumask) > +{ > + unsigned int cpu; > + cpumask_t possible_mask; > + > + cpumask_and(&possible_mask, cpumask, &cpu_possible_map); > + cpu = cpumask_any(&possible_mask); > + return cpu; > +} > + > +static unsigned int gic_nr_lines(void) > +{ > + return gic.lines; > +} > + > +static unsigned int gic_nr_lrs(void) > +{ > + return nr_lrs; > +} > + > +static void write_aprn_regs(struct gic_state_data *d) > +{ > + switch(nr_priorities) Coding style switch ( .. ) > + { > + case 7: case align to the { Please check all the file against CODING_STYLE. I won't shout anymore on every coding style error in this mail. [..] > +static int gic_state_init(struct vcpu *v) > +{ > + v->arch.gic_state = (struct gic_state_data *)xzalloc(struct > gic_state_data); Don't need to cast. > + if(!v->arch.gic_state) > + return -ENOMEM; > + return 0; > +} > + > +static void save_state(struct vcpu *v) > +{ > + int i; > + struct gic_state_data *d; > + d = (struct gic_state_data *)v->arch.gic_state; > + > + /* No need for spinlocks here because interrupts are disabled around > + * this call and it only accesses struct vcpu fields that cannot be > + * accessed simultaneously by another pCPU. > + */ > + for ( i=0; i<nr_lrs; i++) > + d->gic_lr[i] = gich_read_lr(i); You are introducing a helper to read/write lr. How the compiler handle it? Will it optimize? For me it seems very slow... > +static u64 gic_mpidr_to_affinity(u64 mpidr) > +{ > + /* Make sure we don't broadcast the interrupt */ > + return mpidr & ~GICD_IROUTER_SPI_MODE_ANY; > +} > + > +/* > + * - needs to be called with gic.lock held > + * - needs to be called with a valid cpu_mask, ie each cpu in the mask has Please add assert in the function to check that the function is effectively called with this requirements. [..] > + > + edgebit = 2u << (2 * (irq % 16)); > + if ( level ) > + cfg &= ~edgebit; > + else > + cfg |= edgebit; > + > + if (irq < 16) > + writel_relaxed(cfg, (void *)gic_data_rdist_sgi_base() + GICR_ICFGR0); > + else if (irq < 32) > + writel_relaxed(cfg, (void *)gic_data_rdist_sgi_base() + GICR_ICFGR1); > + else > + writel_relaxed(cfg, GICD + GICD_ICFGR + (irq / 16) * 4); > + > + Spurious line > + /* need to check if ARE is set to access IROUTER */ > + affinity = gic_mpidr_to_affinity(cpu_logical_map(cpu)); > + if (irq > 31) > + writeq_relaxed(affinity, (GICD + GICD_IROUTER + irq * 8)); > + > + /* Set priority */ > + if (irq < 32) > + { > + rebase = gic_data_rdist_sgi_base(); HARD tab. > + writeb_relaxed(priority, (void *)rebase + GICR_IPRIORITYR0 + irq); > + } > + else > + { > + writeb_relaxed(priority, GICD + GICD_IPRIORITYR + irq); > + Spurious line [..] > +static void __cpuinit gic_cpu_init(void) > +{ > + int i; > + paddr_t rbase_sgi; > + > + /* Register ourselves with the rest of the world */ > + if (gic_populate_rdist()) > + return; > + > + gic_enable_redist(); > + > + rbase_sgi = gic_data_rdist_sgi_base(); > + > + /* > + * Set priority on PPI and SGI interrupts > + */ > + for (i = 0; i < 16; i += 4) > + writel_relaxed((GIC_PRI_IPI<<24 | GIC_PRI_IPI<<16 | GIC_PRI_IPI<<8 | > GIC_PRI_IPI), (void *)rbase_sgi + GICR_IPRIORITYR0 + (i / 4) * 4); > + //writel_relaxed(0x0, (void *)rbase + GICR_IPRIORITYR0 + (i / 4) * > 4); > + //writel_relaxed(0xa0a0a0a0, (void *)rbase + GICR_IPRIORITYR0 + (i / > 4) * 4); Please remove this comment. [..] > +/* Set up the GIC */ > +void __init gicv3_init(void) > +{ > + static const struct dt_device_match gic_ids[] __initconst = > + { > + DT_MATCH_GIC_V3, > + { /* sentinel */ }, > + }; > + struct dt_device_node *node; > + struct rdist_region *rdist_regs; > + int res, i; > + uint32_t reg; > + > + node = dt_find_interrupt_controller(gic_ids); > + if ( !node ) > + panic("Unable to find compatible GIC in the device tree"); > + > + dt_device_set_used_by(node, DOMID_XEN); > + > + res = dt_device_get_address(node, 0, &gic.dbase, &gic.dbase_size); > + if ( res || !gic.dbase || (gic.dbase & ~PAGE_MASK) || (gic.dbase_size & > ~PAGE_MASK) ) > + panic("GIC: Cannot find a valid address for the distributor"); > + > + gic.map_dbase = ioremap_nocache(gic.dbase, gic.dbase_size); ioremap_nocache can fail. Please check the return. > + > + reg = readl_relaxed(GICD + GICD_PIDR0); > + if ((reg & 0xff) != GICD_PIDR0_GICv3) > + panic("GIC: no distributor detected, giving up\n"); > + > + gic.hw_version = GIC_VERSION_V3; > + > + if (!dt_property_read_u32(node, "#redistributor-regions", > &gic.rdist_count)) > + gic.rdist_count = 1; > + > + rdist_regs = xzalloc_array(struct rdist_region, gic.rdist_count); > + if (!rdist_regs) > + panic("GIC: no distributor detected, giving up\n"); > + > + for (i = 0; i < gic.rdist_count; i++) { > + u64 rdist_base, rdist_size; HARD tab. > + > + res = dt_device_get_address(node, 1 + i, &rdist_base, &rdist_size); > + if ( res || !rdist_base) > + printk("No rdist base found\n"); You are setup rdist_base and rdist_base_size to and unknown value if an error occurred. Should not Xen panic? > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index e0859ae..291e34c 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -688,6 +688,11 @@ static void maintenance_interrupt(int irq, void *dev_id, > struct cpu_user_regs *r > /* Set up the GIC */ > void __init gic_init(void) > { > + static const struct dt_device_match gicv3_ids[] __initconst = > + { > + DT_MATCH_GIC_V3, > + { /* sentinel */ }, > + }; As I said on patch #6, you should use the device API. It will help use when a new GIC driver will be introduced. [..] > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 38df789..15e83e8 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -142,6 +142,10 @@ struct arch_domain > /* Base address for guest GIC */ > paddr_t dbase; /* Distributor base address */ > paddr_t cbase; /* CPU base address */ > + paddr_t dbase_size; /* Distributor base size */ > + paddr_t rbase; /* Re-Distributor base address */ > + paddr_t rbase_size; /* Re-Distributor size */ > + uint32_t rdist_stride; Theses values should go in a GICv3 specific structure. It's *NOT* common code. > } vgic; > > struct vuart { > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index 2de6c6a..b6fbd5e 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -18,6 +18,8 @@ > #ifndef __ASM_ARM_GIC_H__ > #define __ASM_ARM_GIC_H__ > > +#define SZ_64K 0x00010000 Please don't hardcode SZ_64K like that. It should also go in common code. [..] > +/* > + * The minimum GICC_BPR is required to be in the range 0-3. We set > + * GICC_BPR to 0 but we must expect that it might be 3. This means we > + * can rely on premption between the following ranges: > + * 0xf0..0xff > + * 0xe0..0xdf > + * 0xc0..0xcf > + * 0xb0..0xbf > + * 0xa0..0xaf > + * 0x90..0x9f > + * 0x80..0x8f > + * > + * Priorities within a range will not preempt each other. > + * > + * A GIC must support a mimimum of 16 priority levels. > + */ > +#define GIC_PRI_LOWEST 0xf0 > +#define GIC_PRI_IRQ 0xa0 > +#define GIC_PRI_IPI 0x90 /* IPIs must preempt normal interrupts */ > +#define GIC_PRI_HIGHEST 0x80 /* Higher priorities belong to Secure-World */ > + Hmmm you let this part on common header patch #6. Why do you move know? Regards, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |