Hi Peter, Thanks for the review!
> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com] > Sent: Monday, 21 December 2015 14:49 > On Thu, Dec 3, 2015 at 10:01 PM, Andrew Baumann > <andrew.baum...@microsoft.com> wrote: > > This adds the system mailboxes which are used to communicate with a > > number of GPU peripherals on Pi/Pi2. > > > > Signed-off-by: Andrew Baumann <andrew.baum...@microsoft.com> > > --- > > default-configs/arm-softmmu.mak | 1 + > > hw/misc/Makefile.objs | 1 + > > hw/misc/bcm2835_sbm.c | 280 ++++++++++++++++++++ > > > include/hw/arm/bcm2835_arm_control.h | 481 > +++++++++++++++++++++++++++++++++++ > > Do we need this file as of this patch? Yes, for ARM_MC_IHAVEDATAIRQEN and other related defines. [...] > > +static void bcm2835_sbm_update(BCM2835SbmState *s) > > What does Sbm stand for? If it is acronym is should be all caps in camel case. I'm not sure -- it comes from Gregory's code; maybe he can comment? Where this device gets instantiated, there is a comment of /* Semaphores / Doorbells / Mailboxes */ ... but only mailboxes are implemented here. I'm guessing maybe it's intended as "system mailboxes". I can rename it. > > +{ > > + int set; > > bool. > > > + int done, n; > > + uint32_t value; > > + > > + /* Avoid unwanted recursive calls */ > > + s->mbox_irq_disabled = 1; > > + > > + /* Get pending responses and put them in the vc->arm mbox */ > > + done = 0; > > + while (!done) { > > + done = 1; > > + if (s->mbox[0].status & ARM_MS_FULL) { > > + /* vc->arm mbox full, exit */ > > break here. > > > + } else { > > so you can drop the else and get back a level of indent. > > > + for (n = 0; n < MBOX_CHAN_COUNT; n++) { > > + if (s->available[n]) { > > + value = ldl_phys(&s->mbox_as, n<<4); > > + if (value != MBOX_INVALID_DATA) { > > + mbox_push(&s->mbox[0], value); > > + } else { > > + /* Hmmm... */ > > Needs more comment :) Gregory -- help! :) [...] > > + s->available[irq] = level; > > + if (!s->mbox_irq_disabled) { > > I don't think you need this. IO devices are not thread safe and > core-code knows it. So you don't need to worry about interruption and > re-entry of your IRQ handler. I will have to put a debugger on this to confirm, but I think the issue is that we are using a private memory region and GPIOs to route mailbox data and interrupts to the relevant sub peripherals. The ldl_phys invokes another device emulation (such as bcm2835_property.c in this series), which may cause it to signal an interrupt back to us via qemu_set_irq which will recursively run our handler. We want that IRQ to be recorded but "squashed". [...] > > + > > + switch (offset) { > > + case 0x80: /* MAIL0_READ */ > > + case 0x84: > > + case 0x88: > > + case 0x8c: > > case 0x80..0x8c Woah! Is that standard C? [...] > > --- /dev/null > > +++ b/include/hw/arm/bcm2835_arm_control.h > > @@ -0,0 +1,481 @@ > > +/* > > + * linux/arch/arm/mach-bcm2708/arm_control.h [...] > When you have a regular structure like this, you should collapse it > down using some arithmatic: Notice that this file comes from Linux. I know it's not pretty, but can we please keep it as-is, for comparison purposes? I'm not sure there's much value in cleaning it up locally... > > --- /dev/null > > +++ b/include/hw/misc/bcm2835_sbm.h > > @@ -0,0 +1,37 @@ > > +/* > > + * Raspberry Pi emulation (c) 2012 Gregory Estrade > > + * This code is licensed under the GNU GPLv2 and later. > > + */ > > + > > +#ifndef BCM2835_SBM_H > > +#define BCM2835_SBM_H > > + > > +#include "hw/sysbus.h" > > +#include "exec/address-spaces.h" > > +#include "hw/arm/bcm2835_mbox.h" > > + > > +#define TYPE_BCM2835_SBM "bcm2835_sbm" > > +#define BCM2835_SBM(obj) \ > > + OBJECT_CHECK(BCM2835SbmState, (obj), TYPE_BCM2835_SBM) > > + > > +typedef struct { > > + MemoryRegion *mbox_mr; > > + AddressSpace mbox_as; > > I can't see where these two fields are used. A quick scan shows that > the SbmState's copy is uses for all DMA ops. If these are needed, > mbox_init should pass a pointer to the the SbmState then this saves > the pointer and navigates as needed back to the container structure to > get the AS. You're right. They're uninitialised/unused detritus from a previous refactor. I'll remove them. Thanks, Andrew