Re: [PATCH v2 4/6] dwc-hsotg USB host controller emulation

[Peter Maydell]

On Sun, 29 Mar 2020 at 00:18, Paul Zimmerman <address@hidden> wrote:
>
> Add the dwc-hsotg (dwc2) USB host controller emulation code.
> Based on hw/usb/hcd-ehci.c and hw/usb/hcd-ohci.c.
>
> Note that to use this with the dwc-otg driver in the Raspbian
> kernel, you must pass the option "dwc_otg.fiq_fsm_enable=0" on
> the kernel command line.
>
> Emulation of slave mode and of descriptor-DMA mode has not been
> implemented yet. These modes are seldom used.
>
> I have used some on-line sources of information while developing
> this emulation, including:
>
> http://www.capital-micro.com/PDF/CME-M7_Family_User_Guide_EN.pdf
> has a pretty complete description of the controller starting on
> page 370.
>
> https://sourceforge.net/p/wive-ng/wive-ng-mt/ci/master/tree/docs/DataSheets/RT3050_5x_V2.0_081408_0902.pdf
> has a description of the controller registers starting on page
> 130.

Ooh, these reference URLs are very helpful. Could you put
them in a comment at the top of the C file as well as in the
commit message, please?

> Signed-off-by: Paul Zimmerman <address@hidden>
> ---
>  hw/usb/hcd-dwc2.c   | 1301 +++++++++++++++++++++++++++++++++++++++++++
>  hw/usb/trace-events |   47 ++
>  2 files changed, 1348 insertions(+)
>  create mode 100644 hw/usb/hcd-dwc2.c
>
> diff --git a/hw/usb/hcd-dwc2.c b/hw/usb/hcd-dwc2.c
> new file mode 100644
> index 0000000000..fd85543f4d
> --- /dev/null
> +++ b/hw/usb/hcd-dwc2.c
> @@ -0,0 +1,1301 @@
> +/*
> + * dwc-hsotg (dwc2) USB host controller emulation
> + *
> + * Based on hw/usb/hcd-ehci.c and hw/usb/hcd-ohci.c
> + *
> + * 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/usb/dwc2-regs.h"
> +#include "hw/usb/hcd-dwc2.h"
> +#include "trace.h"
> +#include "qemu/error-report.h"
> +#include "qemu/main-loop.h"
> +
> +#define USB_HZ_FS       12000000
> +#define USB_HZ_HS       96000000
> +
> +/* nifty macros from Arnon's EHCI version  */
> +#define get_field(data, field) \
> +    (((data) & field##_MASK) >> field##_SHIFT)
> +
> +#define set_field(data, newval, field) do { \
> +    uint32_t val = *data; \
> +    val &= ~field##_MASK; \
> +    val |= ((newval) << field##_SHIFT) & field##_MASK; \
> +    *data = val; \
> +} while (0)
> +
> +#define get_bit(data, bitmask) \
> +    (!!((data) & bitmask))

Could you use the standard field definition, extract, and deposit
macros from include/hw/registerfields.h, please?

> +static void dwc2_sysbus_realize(DeviceState *dev, Error **errp)
> +{
> +    SysBusDevice *d = SYS_BUS_DEVICE(dev);
> +    DWC2State *s = DWC2_USB(dev);
> +
> +    s->glbregbase = 0;
> +    s->fszregbase = 0x0100;
> +    s->hreg0base = 0x0400;
> +    s->hreg1base = 0x0500;
> +    s->pcgregbase = 0x0e00;
> +    s->hreg2base = 0x1000;
> +    s->portnr = NB_PORTS;
> +    s->as = &address_space_memory;

Ideally this should be a device property. (hw/dma/pl080.c
has an example of how to declare a TYPE_MEMORY_REGION
property and then create an AddressSpace from it in
the realize method. hw/arm/versatilepb.c and hw/arm/mps2-tz.c
show the other end, using object_property_set_link() to pass
the appropriate MemoryRegion to the device before realizing it.)

> +
> +    dwc2_realize(s, dev, errp);

Why have you divided the realize function up into
dwc2_sysbus_realize() and dwc2_realize() and
dwc2_init()? The usual expectation would be that
there is (if you need it) an instance_init called
dwc2_init() and a realize called dwc2_realize(),
so using these names for functions that are just
called from the realize method is a bit confusing.
    object_property_set_link(OBJECT(dev), OBJECT(sysmem), "downstream",
                             &error_fatal);

> +    dwc2_init(s, dev);
> +    sysbus_init_irq(d, &s->irq);
> +    sysbus_init_mmio(d, &s->mem);
> +}
> +
> +static void dwc2_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = dwc2_sysbus_realize;
> +    dc->reset = dwc2_sysbus_reset;
> +    set_bit(DEVICE_CATEGORY_USB, dc->categories);

Could you provide a VMStateDescription for dc->vmsd, please?

> +}
> +
> +static const TypeInfo dwc2_usb_type_info = {
> +    .name          = TYPE_DWC2_USB,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(DWC2State),
> +    .class_init    = dwc2_class_init,
> +};
> +
> +static void dwc2_usb_register_types(void)
> +{
> +    type_register_static(&dwc2_usb_type_info);
> +}

thanks
-- PMM