Re: [PATCH v4 1/7] raspi: add BCM2835 SOC MPHI emulation

[Paul Zimmerman <pauldzim@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 14032 bytes --]

Hi Phil,

On Tue, Apr 28, 2020 at 12:06 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Hi Paul,
>
> On 4/28/20 4:22 AM, Paul Zimmerman wrote:
> > Add BCM2835 SOC MPHI (Message-based Parallel Host Interface)
> > emulation. It is very basic, only providing the FIQ interrupt
> > needed to allow the dwc-otg USB host controller driver in the
> > Raspbian kernel to function.
> >
> > Signed-off-by: Paul Zimmerman <pauldzim@gmail.com>
> > ---
> >  hw/arm/bcm2835_peripherals.c         |  17 +++
> >  hw/misc/Makefile.objs                |   1 +
> >  hw/misc/bcm2835_mphi.c               | 190 +++++++++++++++++++++++++++
> >  include/hw/arm/bcm2835_peripherals.h |   2 +
> >  include/hw/misc/bcm2835_mphi.h       |  48 +++++++
> >  5 files changed, 258 insertions(+)
> >  create mode 100644 hw/misc/bcm2835_mphi.c
> >  create mode 100644 include/hw/misc/bcm2835_mphi.h
> >
> > diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
> > index edcaa4916d..5e2c832d95 100644
> > --- a/hw/arm/bcm2835_peripherals.c
> > +++ b/hw/arm/bcm2835_peripherals.c
> > @@ -124,6 +124,10 @@ static void bcm2835_peripherals_init(Object *obj)
> >      sysbus_init_child_obj(obj, "gpio", &s->gpio, sizeof(s->gpio),
> >                            TYPE_BCM2835_GPIO);
> >
> > +    /* Mphi */
> > +    sysbus_init_child_obj(obj, "mphi", &s->mphi, sizeof(s->mphi),
> > +                          TYPE_BCM2835_MPHI);
> > +
> >      object_property_add_const_link(OBJECT(&s->gpio), "sdbus-sdhci",
> >                                     OBJECT(&s->sdhci.sdbus),
> &error_abort);
> >      object_property_add_const_link(OBJECT(&s->gpio), "sdbus-sdhost",
> > @@ -368,6 +372,19 @@ static void bcm2835_peripherals_realize(DeviceState
> *dev, Error **errp)
> >          return;
> >      }
> >
> > +    /* Mphi */
> > +    object_property_set_bool(OBJECT(&s->mphi), true, "realized", &err);
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +        return;
> > +    }
> > +
> > +    memory_region_add_subregion(&s->peri_mr, MPHI_OFFSET,
> > +                sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->mphi), 0));
> > +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->mphi), 0,
> > +        qdev_get_gpio_in_named(DEVICE(&s->ic), BCM2835_IC_GPU_IRQ,
> > +                               INTERRUPT_HOSTPORT));
> > +
> >      create_unimp(s, &s->armtmr, "bcm2835-sp804",
> ARMCTRL_TIMER0_1_OFFSET, 0x40);
> >      create_unimp(s, &s->cprman, "bcm2835-cprman", CPRMAN_OFFSET,
> 0x1000);
> >      create_unimp(s, &s->a2w, "bcm2835-a2w", A2W_OFFSET, 0x1000);
> > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> > index 68aae2eabb..91085cc21b 100644
> > --- a/hw/misc/Makefile.objs
> > +++ b/hw/misc/Makefile.objs
> > @@ -57,6 +57,7 @@ common-obj-$(CONFIG_OMAP) += omap_l4.o
> >  common-obj-$(CONFIG_OMAP) += omap_sdrc.o
> >  common-obj-$(CONFIG_OMAP) += omap_tap.o
> >  common-obj-$(CONFIG_RASPI) += bcm2835_mbox.o
> > +common-obj-$(CONFIG_RASPI) += bcm2835_mphi.o
> >  common-obj-$(CONFIG_RASPI) += bcm2835_property.o
> >  common-obj-$(CONFIG_RASPI) += bcm2835_rng.o
> >  common-obj-$(CONFIG_RASPI) += bcm2835_thermal.o
> > diff --git a/hw/misc/bcm2835_mphi.c b/hw/misc/bcm2835_mphi.c
> > new file mode 100644
> > index 0000000000..66fc4a9cd3
> > --- /dev/null
> > +++ b/hw/misc/bcm2835_mphi.c
> > @@ -0,0 +1,190 @@
> > +/*
> > + * BCM2835 SOC MPHI emulation
> > + *
> > + * Very basic emulation, only providing the FIQ interrupt needed to
> > + * allow the dwc-otg USB host controller driver in the Raspbian kernel
> > + * to function.
> > + *
> > + * Copyright (c) 2020 Paul Zimmerman <pauldzim@gmail.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > +#include "hw/misc/bcm2835_mphi.h"
> > +#include "migration/vmstate.h"
> > +#include "qemu/error-report.h"
> > +#include "qemu/main-loop.h"
> > +
> > +static inline void mphi_raise_irq(BCM2835MphiState *s)
> > +{
> > +    qemu_set_irq(s->irq, 1);
> > +}
> > +
> > +static inline void mphi_lower_irq(BCM2835MphiState *s)
> > +{
> > +    qemu_set_irq(s->irq, 0);
> > +}
> > +
> > +static uint64_t mphi_reg_read(void *ptr, hwaddr addr, unsigned size)
> > +{
> > +    BCM2835MphiState *s = ptr;
> > +    uint32_t reg = s->regbase + addr;
> > +    uint32_t val = 0;
> > +
> > +    switch (reg) {
> > +    case 0x28:  /* outdda */
> > +        val = s->outdda;
> > +        break;
> > +    case 0x2c:  /* outddb */
> > +        val = s->outddb;
> > +        break;
> > +    case 0x4c:  /* ctrl */
> > +        val = s->ctrl;
> > +        val |= 1 << 17;
> > +        break;
> > +    case 0x50:  /* intstat */
> > +        val = s->intstat;
> > +        break;
> > +    case 0x1f0: /* swirq_set */
> > +        val = s->swirq_set;
> > +        break;
> > +    case 0x1f4: /* swirq_clr */
> > +        val = s->swirq_clr;
> > +        break;
>
> I'm surprised this register is read.
>

Maybe it’s not, I’ll check the driver code.


> > +    default:
> > +        break;
> > +    }
> > +
> > +    return val;
> > +}
> > +
> > +static void mphi_reg_write(void *ptr, hwaddr addr, uint64_t val,
> unsigned size)
> > +{
> > +    BCM2835MphiState *s = ptr;
> > +    uint32_t reg = s->regbase + addr;
> > +    int do_irq = 0;
> > +
> > +    val &= 0xffffffff;
>
> Using '.impl.max_access_size = 4' (see below) this line is not necessary.
>

Ok. I just copied this code from one of the USB drivers, I’ll have a
look into this.


> > +
> > +    switch (reg) {
> > +    case 0x28:  /* outdda */
> > +        s->outdda = val;
> > +        break;
> > +    case 0x2c:  /* outddb */
> > +        s->outddb = val;
> > +        if (val & (1 << 29)) {
> > +            do_irq = 1;
> > +        }
>
> Any idea what outdda/b means?
>

The Raspbian driver has a comment about triggering a fake DMA transfer,
so something to do with DMA. I should say that this emulation was 100%
developed without any documentation, just reverse engineered from what
the driver does.


> > +        break;
> > +    case 0x4c:  /* ctrl */
> > +        s->ctrl = val;
> > +        if (val & (1 << 16)) {
>
> Any idea what are bits 16/17 for?
>
> > +            do_irq = -1;
> > +        }
>
> I suppose this register is INT_ENA (inverted?)
>
> > +        break;
> > +    case 0x50:  /* intstat */
> > +        s->intstat = val;
> > +        if (val & ((1 << 16) | (1 << 29))) {
> > +            do_irq = -1;
>
> I suppose writting INT_STAT acknowledges interrupts.
>
> > +        }
> > +        break;
>
> Here ...
>
> > +    case 0x1f0: /* swirq_set */
> > +        s->swirq_set = val;
> > +        do_irq = 1;
> > +        break;
> > +    case 0x1f4: /* swirq_clr */
> > +        s->swirq_clr = val;
> > +        do_irq = -1;
> > +        break;
>
> ... we access the same register, 's->swirq'.
>
> 0x1f0 sets the bits:
>
>   s->swirq = val;
>
> 0x1f4 clears the bits:
>
>   s->swirq &= ~val;
>
> Then you could simplify with qemu_set_irq(s->irq, ... && s->swirq);
>

Yep I think you’re right, I’ll do that.


> > +    default:
>
> Please log unimplemented accesses:
>
>        qemu_log_mask(LOG_UNIMP, ...);
>

Ah, I was wondering about the right way to do that, thanks.


> > +        break;
>
>            return?
>
> > +    }
> > +
> > +    if (do_irq > 0) {
> > +        mphi_raise_irq(s);
> > +    } else if (do_irq < 0) {
> > +        mphi_lower_irq(s);
> > +    }
> > +}
> > +
> > +static const MemoryRegionOps mphi_mmio_ops = {
> > +    .read = mphi_reg_read,
> > +    .write = mphi_reg_write,
> > +    .valid.min_access_size = 4,
> > +    .valid.max_access_size = 4,
>
> I don't know what are the valid bus access sizes, but per your
> implementation you want:
>
>        .impl.min_access_size = 4,
>        .impl.max_access_size = 4,
>
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > +};
> > +
> > +static void mphi_reset(DeviceState *dev)
> > +{
> > +}
> > +
> > +static void mphi_realize(DeviceState *dev, Error **errp)
> > +{
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > +    BCM2835MphiState *s = BCM2835_MPHI(dev);
> > +
> > +    sysbus_init_irq(sbd, &s->irq);
> > +}
> > +
> > +static void mphi_init(Object *obj)
> > +{
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> > +    BCM2835MphiState *s = BCM2835_MPHI(obj);
> > +
> > +    s->regbase = 0;
> > +    memory_region_init(&s->mem, obj, "mphi", MPHI_MMIO_SIZE);
> > +    memory_region_init_io(&s->mem_reg, obj, &mphi_mmio_ops, s,
> "global", 0x200);
> > +    memory_region_add_subregion(&s->mem, s->regbase, &s->mem_reg);
>
> I'm not sure why you use a container. You can simplify using a single:
>
>        memory_region_init_io(&s->iomem, obj, &mphi_mmio_ops, s, "mphi",
> MPHI_MMIO_SIZE);


OK


> > +    sysbus_init_mmio(sbd, &s->mem);
> > +}
> > +
> > +const VMStateDescription vmstate_mphi_state = {
> > +    .name = "mphi",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT16(regbase, BCM2835MphiState),
> > +        VMSTATE_UINT32(outdda, BCM2835MphiState),
> > +        VMSTATE_UINT32(outddb, BCM2835MphiState),
> > +        VMSTATE_UINT32(ctrl, BCM2835MphiState),
> > +        VMSTATE_UINT32(intstat, BCM2835MphiState),
> > +        VMSTATE_UINT32(swirq_set, BCM2835MphiState),
> > +        VMSTATE_UINT32(swirq_clr, BCM2835MphiState),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static void mphi_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->realize = mphi_realize;
> > +    dc->reset = mphi_reset;
> > +    dc->vmsd = &vmstate_mphi_state;
> > +}
> > +
> > +static const TypeInfo bcm2835_mphi_type_info = {
> > +    .name          = TYPE_BCM2835_MPHI,
> > +    .parent        = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size = sizeof(BCM2835MphiState),
> > +    .instance_init = mphi_init,
> > +    .class_init    = mphi_class_init,
> > +};
> > +
> > +static void bcm2835_mphi_register_types(void)
> > +{
> > +    type_register_static(&bcm2835_mphi_type_info);
> > +}
> > +
> > +type_init(bcm2835_mphi_register_types)
> > diff --git a/include/hw/arm/bcm2835_peripherals.h
> b/include/hw/arm/bcm2835_peripherals.h
> > index 2e8655a7c2..7a7a8f6141 100644
> > --- a/include/hw/arm/bcm2835_peripherals.h
> > +++ b/include/hw/arm/bcm2835_peripherals.h
> > @@ -21,6 +21,7 @@
> >  #include "hw/misc/bcm2835_property.h"
> >  #include "hw/misc/bcm2835_rng.h"
> >  #include "hw/misc/bcm2835_mbox.h"
> > +#include "hw/misc/bcm2835_mphi.h"
> >  #include "hw/misc/bcm2835_thermal.h"
> >  #include "hw/sd/sdhci.h"
> >  #include "hw/sd/bcm2835_sdhost.h"
> > @@ -42,6 +43,7 @@ typedef struct BCM2835PeripheralState {
> >      qemu_irq irq, fiq;
> >
> >      BCM2835SystemTimerState systmr;
> > +    BCM2835MphiState mphi;
> >      UnimplementedDeviceState armtmr;
> >      UnimplementedDeviceState cprman;
> >      UnimplementedDeviceState a2w;
> > diff --git a/include/hw/misc/bcm2835_mphi.h
> b/include/hw/misc/bcm2835_mphi.h
> > new file mode 100644
> > index 0000000000..6d070b04a5
> > --- /dev/null
> > +++ b/include/hw/misc/bcm2835_mphi.h
> > @@ -0,0 +1,48 @@
> > +/*
> > + * BCM2835 SOC MPHI state definitions
> > + *
> > + * Copyright (c) 2020 Paul Zimmerman <pauldzim@gmail.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#ifndef HW_MISC_BCM2835_MPHI_H
> > +#define HW_MISC_BCM2835_MPHI_H
> > +
> > +#include "hw/irq.h"
> > +#include "hw/sysbus.h"
> > +#include "sysemu/dma.h"
>
> sysemu/dma.h not used.
>

Will remove.

>
> > +
> > +#define MPHI_MMIO_SIZE      0x1000
> > +
> > +typedef struct BCM2835MphiState BCM2835MphiState;
> > +
> > +struct BCM2835MphiState {
> > +    SysBusDevice parent_obj;
> > +    qemu_irq irq;
> > +    MemoryRegion mem;
> > +    MemoryRegion mem_reg;
> > +    uint16_t regbase;
>
> You can remove regbase.
>

Right, thanks,


> > +
> > +    uint32_t outdda;
> > +    uint32_t outddb;
> > +    uint32_t ctrl;
> > +    uint32_t intstat;
> > +    uint32_t swirq_set;
> > +    uint32_t swirq_clr;
>
> As suggested previously, you can use a single 'swirq' register.
>
> > +};
> > +
> > +#define TYPE_BCM2835_MPHI   "bcm2835-mphi"
> > +
> > +#define BCM2835_MPHI(obj) \
> > +    OBJECT_CHECK(BCM2835MphiState, (obj), TYPE_BCM2835_MPHI)
> > +
> > +#endif
> >
>
> I made a lot of picky comments, mostly to simplify, but this patch is in
> good shape otherwise (being aware we have no documentation on this
> device). Maybe Peter/Gerd are OK to accept it as it (as it is your first
> contribution).
>
> Thanks for the review!

Paul


Regards,
>
> Phil.
>

[-- Attachment #2: Type: text/html, Size: 21196 bytes --]