[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 Vijay, A couple of things I've noticed while reviewing this series. On 04/15/2014 12:17 PM, vijay.kilari@xxxxxxxxx wrote: > +static void gicv2_write_lr(int lr, struct gic_lr *lr_reg) > +{ > + uint32_t lrv = 0; Newline here. > + lrv = ( ((lr_reg->pirq & GICH_LR_PHYSICAL_MASK) << > GICH_LR_PHYSICAL_SHIFT) | > + ((lr_reg->virq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT) > | > + ((uint32_t)(lr_reg->priority & GICH_LR_PRIORITY_MASK) << > GICH_LR_PRIORITY_SHIFT) | > + ((uint32_t)(lr_reg->state & GICH_LR_STATE_MASK) << > GICH_LR_STATE_SHIFT) | > + ((uint32_t)(lr_reg->hw_status & GICH_LR_HW_MASK) << > GICH_LR_HW_SHIFT) | > + ((uint32_t)(lr_reg->grp & GICH_LR_GRP_MASK) << > GICH_LR_GRP_SHIFT) ); > + > + GICH[GICH_LR + lr] = lrv; > +} [..] > +struct gic_hw_operations { Some of these functions require locks. You have to specified here which lock is required. That will avoid headache for people who are planning to implement new GIC driver. > + /* 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 *); > + /* Dump GIC LR register information */ > + void (*dump_state)(struct vcpu *); > + /* Map MMIO region of GIC and get GIC address information */ > + int (*gicv_setup)(struct domain *); > + > + /* hw_irq_controller for enable/disable/eoi irq */ > + 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); This name of this function is not clear for me. I would rename into read_irq_and_ack (maybe the and_ack is not necessary). > + /* 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); > + /* 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 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 *); > + /* Read VMCR priority */ > + unsigned int (*read_vmcr_priority)(void); > + /* Secondary CPU init */ > + void (*secondary_init)(void); I would return an int here. I saw a panic on GICv3, and I don't think it's acceptable for secondary CPUs. I can be used during CPU hotplug. 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 |