[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 07/16] xen/arm: segregate and split GIC low level functionality
Hello Vijaya, Thank you for the patch. On 04/15/2014 12:17 PM, vijay.kilari@xxxxxxxxx wrote: > +/* Access to the GIC Distributor registers through the fixmap */ > +#define GICD ((volatile uint32_t *) FIXMAP_ADDR(FIXMAP_GICD)) > +#define GICC ((volatile uint32_t *) FIXMAP_ADDR(FIXMAP_GICC1)) > +#define GICH ((volatile uint32_t *) FIXMAP_ADDR(FIXMAP_GICH)) > + > +/* Global state */ > +static struct { > + paddr_t dbase; /* Address of distributor registers */ > + paddr_t cbase; /* Address of CPU interface registers */ > + paddr_t hbase; /* Address of virtual interface registers */ > + paddr_t vbase; /* Address of virtual cpu interface registers */ > + unsigned int lines; /* Number of interrupts (SPIs + PPIs + SGIs) */ > + struct dt_irq maintenance; /* IRQ maintenance */ > + unsigned int cpus; > +} gic; > + > +static struct gic_hw_operations gic_ops; > + > +static uint8_t nr_lrs; As the number of LRs is store in gic_hw_operations, you don't need this one. > + > +/* The GIC mapping of CPU interfaces does not necessarily match the > + * logical CPU numbering. Let's use mapping as returned by the GIC > + * itself > + */ > +static DEFINE_PER_CPU(u8, gic_cpu_id); > + > +/* Maximum cpu interface per GIC */ > +#define NR_GIC_CPU_IF 8 > + > +static unsigned int gicv2_cpu_mask(const cpumask_t *cpumask) > +{ > + unsigned int cpu; > + unsigned int mask = 0; > + cpumask_t possible_mask; > + > + cpumask_and(&possible_mask, cpumask, &cpu_possible_map); > + for_each_cpu(cpu, &possible_mask) I know that coding style error was there before ... can you change it to for_each_cpu( ... ) > + { > + ASSERT(cpu < NR_GIC_CPU_IF); > + mask |= per_cpu(gic_cpu_id, cpu); > + } > + > + return mask; > +} [..] > +static void gicv2_dump_state(struct vcpu *v) > +{ > + int i; > + if ( v == current ) > + { > + for ( i = 0; i < nr_lrs; i++ ) > + printk(" HW_LR[%d]=%x\n", i, GICH[GICH_LR + i]); > + } else { } else { [..] > +static struct dt_irq * gicv2_maintenance_irq(void) > +{ > + return &gic.maintenance; > +} I have a serie which drop dt_struct and changes IRQW routing. It would be nice if you can rebase your code on top of it now. I don't think it will change heavily. See git://xenbits.xen.org/people/julieng/xen-unstable.git branch interrupt-mgmt-v3 [..] > static void gic_irq_enable(struct irq_desc *desc) > { > - int irq = desc->irq; > unsigned long flags; > > spin_lock_irqsave(&desc->lock, flags); > @@ -147,20 +112,19 @@ static void gic_irq_enable(struct irq_desc *desc) > desc->status &= ~IRQ_DISABLED; > dsb(); > /* Enable routing */ > - GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32)); > + gic_hw_ops->gic_irq_ops->enable(desc); This is not the right way to use gic_irq_ops. You should directly assigned this structure to desc->handler. I guess you don't do it because you have the desc->status and gic_lock locking which is common. IHMO, you have to let the GICv{2,3} drivers handling their own lock for the hardware access. > spin_unlock(&gic_lock); > spin_unlock_irqrestore(&desc->lock, flags); > } > > static void gic_irq_disable(struct irq_desc *desc) > { > - int irq = desc->irq; > unsigned long flags; > > spin_lock_irqsave(&desc->lock, flags); > spin_lock(&gic_lock); > /* Disable routing */ > - GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32)); > + gic_hw_ops->gic_irq_ops->disable(desc); > desc->status |= IRQ_DISABLED; > spin_unlock(&gic_lock); > spin_unlock_irqrestore(&desc->lock, flags); Same here. > @@ -186,16 +150,15 @@ static void gic_host_irq_end(struct irq_desc *desc) > { > int irq = desc->irq; > /* Lower the priority */ > - GICC[GICC_EOIR] = irq; > + gic_hw_ops->gic_irq_ops->end(desc); > /* Deactivate */ > - GICC[GICC_DIR] = irq; > + gic_hw_ops->deactivate_irq(irq); > } > static void gic_guest_irq_end(struct irq_desc *desc) > { > - int irq = desc->irq; > /* Lower the priority of the IRQ */ > - GICC[GICC_EOIR] = irq; > + gic_hw_ops->gic_irq_ops->end(desc); > /* Deactivation happens in maintenance interrupt / via GICV */ > } Same here. > @@ -215,6 +178,7 @@ static hw_irq_controller gic_host_irq_type = { > .end = gic_host_irq_end, > .set_affinity = gic_irq_set_affinity, > }; > + Spurious change? > static hw_irq_controller gic_guest_irq_type = { > .typename = "gic", > .startup = gic_irq_startup, > @@ -235,27 +199,7 @@ static void gic_set_irq_properties(unsigned int irq, > bool_t level, > const cpumask_t *cpu_mask, > unsigned int priority) > { > - volatile unsigned char *bytereg; > - uint32_t cfg, edgebit; > - unsigned int mask = gic_cpu_mask(cpu_mask); > - > - /* Set edge / level */ > - cfg = GICD[GICD_ICFGR + irq / 16]; > - edgebit = 2u << (2 * (irq % 16)); > - if ( level ) > - cfg &= ~edgebit; > - else > - cfg |= edgebit; > - GICD[GICD_ICFGR + irq / 16] = cfg; > - > - /* Set target CPU mask (RAZ/WI on uniprocessor) */ > - bytereg = (unsigned char *) (GICD + GICD_ITARGETSR); > - bytereg[irq] = mask; > - > - /* Set priority */ > - bytereg = (unsigned char *) (GICD + GICD_IPRIORITYR); > - bytereg[irq] = priority; > - > + return gic_hw_ops->set_irq_property(irq, level, cpu_mask, priority); You don't need to return as gic_set_irq_properties is a void function. Also why did you drop in plural in the callback name? It should be name set_irq_properties. [..] > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index eba41ee..2387e38 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -43,12 +43,41 @@ > #define SGI_TARGET_OTHERS 1 > #define SGI_TARGET_SELF 2 > > +#define GICH_LR_PENDING 1 > +#define GICH_LR_ACTIVE 2 > + Prefixing by GICH_ is confusing. I though we were using it for to set the value in the hardware. Can you rename it? > +#define GICH_HCR_EN (1 << 0) > +#define GICH_HCR_UIE (1 << 1) > +#define GICH_HCR_LRENPIE (1 << 2) > +#define GICH_HCR_NPIE (1 << 3) > +#define GICH_HCR_VGRP0EIE (1 << 4) > +#define GICH_HCR_VGRP0DIE (1 << 5) > +#define GICH_HCR_VGRP1EIE (1 << 6) > +#define GICH_HCR_VGRP1DIE (1 << 7) > + You dropped this lines in patch #5 and re-add here. Please don't drop them on patch #5. > #ifndef __ASSEMBLY__ > #include <xen/device_tree.h> > +#include <xen/irq.h> > > #define DT_MATCH_GIC DT_MATCH_COMPATIBLE("arm,cortex-a15-gic"), \ > DT_MATCH_COMPATIBLE("arm,cortex-a7-gic") > > +/* > + * Decode LR register content and populate below struct. IHMO, the second part of the description (after the "and") is not useful. [..] > +struct gic_hw_operations { > + /* GIC version */ > + enum gic_version hw_version; > + /* Number of GIC lines supported */ > + unsigned int nr_lines; > + /* Number of LR registers */ > + unsigned int nr_lrs; > + /* Maintenance irq is derived from dt node. Fetch from gic driver */ > + struct dt_irq * (*get_maintenance_irq)(void); > + /* Save GIC registers */ > + void (*save_state)(struct vcpu *); > + /* Restore GIC registers */ > + void (*restore_state)(struct vcpu *); Does restore state will modify the content of vcpu? If not, please add const. > + /* Dump GIC LR register information */ > + void (*dump_state)(struct vcpu *); Same question here. > + /* Map MMIO region of GIC and get GIC address information */ > + int (*gicv_setup)(struct domain *); > + > + /* hw_irq_controller for enable/disable/eoi irq */ The hw_irq_controller should contains every callbacks. (see my previous remark on it). > + hw_irq_controller *gic_irq_ops; const hw_irq_controller *gic_irq_ops; > + > + /* Deactivate/reduce priority of irq */ > + void (*deactivate_irq)(int); > + /* Ack IRQ and return irq id */ > + unsigned int (*ack_irq)(void); > + /* Set IRQ property */ > + void (*set_irq_property)(unsigned int irq, bool_t level, > + const cpumask_t *cpu_mask, > + unsigned int priority); > + /* Send SGI */ > + void (*send_sgi)(const cpumask_t *online_mask, > + enum gic_sgi sgi, uint8_t irqmode); Misaligned? > + /* Disable CPU physical and virtual interfaces */ > + void (*disable_interface)(void); > + /* Update LR with state and priority */ > + void (*update_lr)(int lr, struct pending_irq *, unsigned int state); Update LR won't modify pending_irq, right? If so, please add const. Also you are mixing named and unamed argument. Can you name every arguments? > + /* Update HCR status register */ > + void (*update_hcr_status)(uint32_t flag, bool_t set); > + /* Clear LR register */ > + void (*clear_lr)(int lr); > + /* Read LR register and populate gic_lr structure */ > + void (*read_lr)(int lr, struct gic_lr *); > + /* Write LR register from gic_lr structure */ > + void (*write_lr)(int lr, struct gic_lr *); write_lr won't modify pending_irq, right? If so, please add const. 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 |