[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 13/15] xen/arm: Add support for GIC v3
On Fri, 2014-04-04 at 17:26 +0530, vijay.kilari@xxxxxxxxx wrote: > +#define gic_data_rdist_rd_base() (this_cpu(rbase)) > +#define gic_data_rdist_sgi_base() (gic_data_rdist_rd_base() + SZ_64K) Macros should be SHOUTY and ideally a lot shorter. Since we have GICD[...] etc how about GICR[...]? And for the SGI bit just use GICR[GICR_SGI_...] (where GICR_SGI_ is 64K). > + > +static inline u64 read_cpuid_mpidr(void) > +{ > + return READ_SYSREG(MPIDR_EL1); > +} No need for this trivial wrapper. > +/* Wait for completion of a distributor change */ > +static void gic_do_wait_for_rwp(paddr_t base) base here is a virtual address not a physical one, no? Hence the cast you have below. > +{ > + u32 val; > + do { > + val = readl_relaxed((void *)base + GICD_CTLR); > + val = readl_relaxed(GICD + GICD_CTLR); ??? > + cpu_relax(); This doesn't currently yield, but in principal it could, which would mean a delay even if the termination condition was true. Perhaps for(;;) { val = readl... if (val & ... ) break; cpu_relax(); } ??? > + } while ( val & GICD_CTLR_RWP ); > +} > + > +static void gic_dist_wait_for_rwp(void) > +{ > + gic_do_wait_for_rwp((paddr_t)GICD); Ugly and seemingly unnecessary cast. > +} > + > +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 ) Either NR_LOCAL_IRQS or suitable gic v3 specific #define please (and rename NR_LOCAL_IRQS to a v2 name) > + gic_redist_wait_for_rwp(); > + else > + gic_dist_wait_for_rwp(); > +} > + > + > +static void write_aprn_regs(struct gic_state_data *d) > +{ > + switch ( nr_priorities ) > + { > + case 7: > + WRITE_SYSREG32(d->v3.gic_apr0[2], ICH_AP0R2_EL2); > + WRITE_SYSREG32(d->v3.gic_apr1[2], ICH_AP1R2_EL2); + /* Fall-thru */ when doing so deliberately please. Is it harmful/illegal to write to AP0R2 etc if priorities < 7? > + case 6: > + WRITE_SYSREG32(d->v3.gic_apr0[1], ICH_AP0R1_EL2); > + WRITE_SYSREG32(d->v3.gic_apr1[1], ICH_AP1R1_EL2); > + case 5: > + WRITE_SYSREG32(d->v3.gic_apr0[0], ICH_AP0R0_EL2); > + WRITE_SYSREG32(d->v3.gic_apr1[0], ICH_AP1R0_EL2); > + break; > + default: > + panic("Write Undefined active priorities \n"); I think these limits should be checked at init time with a panic there and this should be come an assertion. > + } > +} > + > +static void read_aprn_regs(struct gic_state_data *d) > +{ Same comments here as for write. > +static void gic_enable_irq(int irq) > +{ > + uint32_t enabler; > + > + /* Enable routing */ > + if ( irq > 31 ) > + { > + enabler = readl_relaxed(GICD + GICD_ISENABLER + (irq / 32) * 4); > + writel_relaxed(enabler | (1u << (irq % 32)), GICD + GICD_ISENABLER + > (irq / 32) * 4); &GICD[GICD_IS_ENABLER +...] ? > + } > + else > + { > + enabler = readl_relaxed((void *)gic_data_rdist_sgi_base() + > GICR_ISENABLER0); > + writel_relaxed(enabler | (1u << irq), (void > *)gic_data_rdist_sgi_base() + GICR_ISENABLER0); No casts please, just get the type right to start with. Per the comments at the macro definition this could be &GIRC[GICR_SGI_ISENABLR0]. I think both halves of this if would benefit from using some temporary variables for clarity, or at least enabler = read... enabler |= thing writel(enabler, ...) > +/* > + * - needs to be called with gic.lock held > + * - needs to be called with a valid cpu_mask, ie each cpu in the mask has > + * already called gic_cpu_init > + */ > +static void gic_set_irq_property(unsigned int irq, bool_t level, > + const cpumask_t *cpu_mask, > + unsigned int priority) > +{ > + uint32_t cfg, edgebit; > + u64 affinity; > + unsigned int cpu = gic_mask_cpu(cpu_mask); > + paddr_t rebase; > + > + > + /* Set edge / level */ > + if ( irq < 16 ) > + /* SGI's are always edge-triggered not need to call GICD_ICFGR0 */ > + cfg = readl_relaxed((void *)gic_data_rdist_sgi_base() + GICR_ICFGR0); casts and names throughout this function. [...] > +static void __init gic_dist_init(void) > +{ > + uint32_t type; > + u64 affinity; > + int i; > + > + /* Disable the distributor */ > + writel_relaxed(0, GICD + GICD_CTLR); Does using readl/writel_relaxed buy you anything over using a GICD macro with a volatile in it like the v2 code does? > + > + type = readl_relaxed(GICD + GICD_TYPER); > + gic.lines = 32 * ((type & GICD_TYPE_LINES) + 1); > + > + printk("GIC: %d lines, (IID %8.8x).\n", > + gic.lines, readl_relaxed(GICD + GICD_IIDR)); > + > + /* Default all global IRQs to level, active low */ > + for ( i = 32; i < gic.lines; i += 16 ) > + writel_relaxed(0, GICD + GICD_ICFGR + (i / 16) * 4); > + > + /* Default priority for global interrupts */ > + for ( i = 32; i < gic.lines; i += 4 ) > + writel_relaxed((GIC_PRI_IRQ<<24 | GIC_PRI_IRQ<<16 | GIC_PRI_IRQ<<8 | > GIC_PRI_IRQ), GICD + GICD_IPRIORITYR + (i / 4) * 4); Watch out for long lines please, try and keep to 80 columns. > + > + /* Disable all global interrupts */ > + for ( i = 32; i < gic.lines; i += 32 ) > + writel_relaxed(0xffffffff, GICD + GICD_ICENABLER + (i / 32) * 4); > + > + gic_dist_wait_for_rwp(); > + > + /* Turn on the distributor */ > + writel_relaxed(GICD_CTL_ENABLE | GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G1A > | GICD_CTLR_ENABLE_G1, GICD + GICD_CTLR); > + > + /* Route all global IRQs to this CPU */ > + affinity = gic_mpidr_to_affinity(read_cpuid_mpidr()); > + for ( i = 32; i < gic.lines; i++ ) > + writeq_relaxed(affinity, GICD + GICD_IROUTER + i * 8); > +} > + > +static void gic_enable_redist(void) > +{ > + paddr_t rbase; > + u32 val; > + > + rbase = this_cpu(rbase); > + > + /* Wake up this CPU redistributor */ > + val = readl_relaxed((void *)rbase + GICR_WAKER); > + val &= ~GICR_WAKER_ProcessorSleep; > + writel_relaxed(val, (void *)rbase + GICR_WAKER); > + > + do { > + val = readl_relaxed((void *)rbase + GICR_WAKER); > + cpu_relax(); > + } while ( val & GICR_WAKER_ChildrenAsleep ); > +} > + > +static int __init gic_populate_rdist(void) > +{ > + u64 mpidr = cpu_logical_map(smp_processor_id()); > + u64 typer; > + u64 aff; > + int i; > + uint32_t reg; > + > + aff = mpidr & ((1 << 24) - 1); > + aff |= (mpidr >> 8) & (0xffUL << 24); > + > + for ( i = 0; i < gic.rdist_count; i++ ) > + { > + uint32_t ptr = 0; > + > + reg = readl_relaxed(GICR + ptr + GICR_PIDR0); > + if ( (reg & 0xff) != GICR_PIDR0_GICv3 ) { > + printk("No redistributor present @%x\n", ptr); Debug message? > + break; > + } > + > + do { > + typer = readq_relaxed(GICR + ptr + GICR_TYPER); > + if ( (typer >> 32) == aff ) > + { > + this_cpu(rbase) = (u64)GICR + ptr; Cast? > + printk("CPU%d: found redistributor %llx region %d\n", > + smp_processor_id(), (unsigned long long) mpidr, > i); > + return 0; > + } > + > + if ( gic.rdist_stride ) { > + ptr += gic.rdist_stride; > + } else { > + ptr += SZ_64K * 2; /* Skip RD_base + SGI_base */ Whatever initialises rdist_stride should default it to SZ_64K if that is indeed the default. > + if ( typer & GICR_TYPER_VLPIS ) > + ptr += SZ_64K * 2; /* Skip VLPI_base + reserved page */ > + } > + } while ( !(typer & GICR_TYPER_LAST) ); > + } > + > + /* We couldn't even deal with ourselves... */ > + printk("CPU%d: mpidr %lx has no re-distributor!\n", > + smp_processor_id(), (unsigned long)mpidr); No casts, either use PRIx64 or make the type correct. (I'm going to stop mentioning casts, please eliminate them all or explain why they are needed). > + /* > + * 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); Long lines. > + > +static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask, > + u64 cluster_id) > +{ > + int cpu = *base_cpu; > + u64 mpidr = cpu_logical_map(cpu); > + u16 tlist = 0; > + > + while ( cpu < nr_cpu_ids ) > + { > + /* > + * If we ever get a cluster of more than 16 CPUs, just > + * scream and skip that CPU. I don't see any screaming. We would want to know if this was happening, wouldn't we? > +static void gic_update_lr(int lr, struct pending_irq *p, unsigned int state) > +{ > + u64 grp = GICH_LR_GRP1; > + u64 val = 0; > + > + BUG_ON(lr >= nr_lrs); > + BUG_ON(lr < 0); > + > + val = ((((u64)state) & 0x3) << GICH_LR_STATE_SHIFT) | grp | > + ((((u64)p->priority) & 0xff) << GICH_LR_PRIORITY_SHIFT) | > + (((u64)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); > + > + if ( p->desc != NULL ) > + val |= GICH_LR_HW |(((u64) p->desc->irq & GICH_LR_PHYSICAL_MASK) << > GICH_LR_PHYSICAL_SHIFT); > + > + gich_write_lr(lr, val); > +} > + > +static void gic_clear_lr(int lr) > +{ > + gich_write_lr(lr, 0); > +} > + > +static void gic_read_lr(int lr, struct gic_lr *lr_reg) I can't find struct gic_lr anywhere. > +{ > + u64 lrv; > + lrv = gich_read_lr(lr); > + lr_reg->pirq = (lrv >> GICH_LR_PHYSICAL_SHIFT) & GICH_LR_PHYSICAL_MASK; > + lr_reg->virq = (lrv >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK; > + lr_reg->priority = (lrv >> GICH_LR_PRIORITY_SHIFT) & > GICH_LR_PRIORITY_MASK; > + lr_reg->state = (lrv >> GICH_LR_STATE_SHIFT) & GICH_LR_STATE_MASK; > + lr_reg->hw_status = (lrv >> GICH_LR_HW_SHIFT) & GICH_LR_HW_MASK; > + lr_reg->grp = (lrv >> GICH_LR_GRP_SHIFT) & GICH_LR_GRP_MASK; If you want to be able to do field-by-field accesses then please do what the page table code does and use a union and bit fields. See lpae_pt_t. typedef union __packed { uint64_t bits; struct { unsigned long pirq:4; unsugned long virq:4; etc, including explicit padding }; } gicv3_lr; Then: gicv3 lrv = {.bits = gich_read_lr(lr)}; > +static struct dt_irq * gic_maintenance_irq(void) > +{ > + return &gic.maintenance; > +} > + > +static void gic_hcr_status(uint32_t flag, uint8_t status) > +{ > + if ( status ) status is actually "bool_t set" ? > + WRITE_SYSREG32((READ_SYSREG32(ICH_HCR_EL2) | flag), ICH_HCR_EL2); > + else > + WRITE_SYSREG32((READ_SYSREG32(ICH_HCR_EL2) & (~flag)), ICH_HCR_EL2); > +} > + > + rdist_regs = xzalloc_array(struct rdist_region, gic.rdist_count); I thought I saw a comment at the top saying that only a single region was supported? Shouldn't this be checked somewhere, or even better fixed. Is there a limit on gic.rdist_count? How large is it? Can rdist_regs be a static array? Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |