Hi, The "sbm" is a typo. It was meant to be "smb": Semaphore MailBoxes... Sorry about that.
Regards, Gregory On Tue, Dec 22, 2015 at 12:15 AM, Andrew Baumann < andrew.baum...@microsoft.com> wrote: > 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 >
