Re: [Qemu-arm] [PATCH 1/8] bcm2835_sbm: add BCM2835 mailboxes

[Grégory ESTRADE]

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 <address@hidden> wrote:

On Mon, Dec 21, 2015 at 3:15 PM, Andrew Baumann

<address@hidden> wrote:
> Hi Peter,
>
> Thanks for the review!
>
>> From: Peter Crosthwaite [mailto:address@hidden]
>> Sent: Monday, 21 December 2015 14:49
>> On Thu, Dec 3, 2015 at 10:01 PM, Andrew Baumann
>> <address@hidden> 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 <address@hidden>
>> > ---
>> >  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