Re: [Qemu-devel] [PATCH v4 5/8] bcm2836_control: add bcm2836 ARM control logic

[Andrew Baumann]

Hi Peter,

> From: Peter Crosthwaite [mailto:address@hidden
> Sent: Thursday, 28 January 2016 20:38
> 
> On Fri, Jan 15, 2016 at 3:58 PM, Andrew Baumann
> <address@hidden> wrote:
> > This module is specific to the bcm2836 (Pi2). It implements the top
> > level interrupt controller, and mailboxes used for inter-processor
> > synchronisation.
[...]
> > +    for (i = 0; i < BCM2836_NCORES; i++) {
> > +        /* handle local timer interrupts for this core */
> > +        if (s->timerirqs[i]) {
> > +            assert(s->timerirqs[i] < (1 << IRQ_MAILBOX0)); /* sanity check 
> > */
> > +            for (j = 0; j < IRQ_MAILBOX0; j++) {
> 
> I think <= IRQ_CNTVIRQ is cleaner, as it keeps "MAILBOX" out of the timer
> code.

Ok.

[...]
> > +typedef struct BCM2836ControlState {
> > +    /*< private >*/
> > +    SysBusDevice busdev;
> > +    /*< public >*/
> > +    MemoryRegion iomem;
> > +
> 
> > +    /* interrupt status registers (not directly visible to user) */
> > +    bool gpu_irq, gpu_fiq;
> > +    uint8_t timerirqs[BCM2836_NCORES];
> > +
> 
> This ...
> 
> > +    /* mailboxes */
> > +    uint32_t mailboxes[BCM2836_NCORES * BCM2836_MBPERCORE];
> > +
> > +    /* interrupt routing/control registers */
> > +    uint8_t route_gpu_irq, route_gpu_fiq;
> > +    uint32_t timercontrol[BCM2836_NCORES];
> > +    uint32_t mailboxcontrol[BCM2836_NCORES];
> > +
> 
> > +    /* interrupt source registers, post-routing (visible) */
> > +    uint32_t irqsrc[BCM2836_NCORES];
> > +    uint32_t fiqsrc[BCM2836_NCORES];
> > +
> 
> And this are absent from the VMSD, but after some thought they don't
> need to be as they are pure functions of the input pin state that is
> always refreshable from other state no? I would these together with a
> brief comment as to the above, and keep the migratable state (genuine
> device state) all together.

Yes, that was exactly the intention. I'll comment/revise as you suggest.

> Reviewed-by: Peter Crosthwaite <address@hidden>

Thanks for the review,
Andrew