Indeed, this header is generated from the Broadcom's Linux drivers headers, with a Perl script.
On Tue, Dec 22, 2015 at 12:33 AM, Peter Crosthwaite < crosthwaitepe...@gmail.com> wrote: > On Mon, Dec 21, 2015 at 3:15 PM, 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? > > > > Yes, its probably one of the more recent language standards though. > QEMU does use to more modern features liberally. > > > [...] > >> > --- /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... > > > > It looks very autogenerated and seems pretty nasty on the repetition. > > As implementers of the hardware, it is much rarer to need these > repetitious defs than the software users on the other side. "Do > something specific with CPU#3's Mbox#5" is going to appear in > software, but hardware implementers generally don't have a choice to > implement things specifically and it usually ends up being looped and > the exploded defs are never used. If there are only a handful of > genuinely single defs needed, can they be fished out? > > Regards, > Peter > > >> > --- /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 >
