Hi Phil,
On Tue, Apr 28, 2020 at 12:06 AM Philippe Mathieu-Daudé <f4...@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 <pauld...@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 <pauld...@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 <pauld...@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.
>
