On Tue, Apr 28, 2020 at 12:06 AM Philippe Mathieu-Daudé <
address@hidden> 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 <address@hidden>
> ---
> 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 <address@hidden>
> + *
> + * 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 <address@hidden>
> + *
> + * 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.