Re: [PATCH v5 4/7] dwc-hsotg (dwc2) USB host controller emulation

[Peter Maydell]

On Tue, 12 May 2020 at 07:50, 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
> which 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
> which has a description of the controller registers starting on
> page 130.
>
> Thanks to Felippe Mathieu-Daude for providing a cleaner method
> of implementing the memory regions for the controller registers.
>
> +
> +        if (pid != USB_TOKEN_IN) {
> +            trace_usb_dwc2_memory_read(hcdma, tlen);
> +            if (dma_memory_read(&s->dma_as, hcdma,
> +                                s->usb_buf[chan], tlen) != MEMTX_OK) {
> +                fprintf(stderr, "%s: dma_memory_read failed\n", __func__);

Don't report things with fprintf(stderr, ...), please. Other
options:

 * use qemu_log_mask() with a suitable mask; in particular for
   "guest just did something silly" we have LOG_GUEST_ERROR,
   and for "guest tried to do something we haven't implemented"
   we have LOG_GUEST_ERROR
 * use a tracepoint
 * for "this can't happen unless some other part of QEMU is
   buggy", just assert()
 * don't print or log anything if the emulation is just doing
   what the h/w would do in some situation

In this case LOG_GUEST_ERROR seems plausible. Ideally we'd do
whatever the hardware does on memory read failures (maybe it
logs this in a status bit somewhere ?)

I've noted a few other fprintf uses below but all of them
should be changed, please.

> +            }
> +        }
> +
> +        usb_packet_init(&p->packet);
> +        usb_packet_setup(&p->packet, pid, ep, 0, hcdma,
> +                         pid != USB_TOKEN_IN, true);
> +        usb_packet_addbuf(&p->packet, s->usb_buf[chan], tlen);
> +        p->async = DWC2_ASYNC_NONE;
> +        usb_handle_packet(dev, &p->packet);
> +    } else {
> +        tlen = p->len;
> +    }
> +
> +    stsidx = -p->packet.status;
> +    assert(stsidx < sizeof(pstatus) / sizeof(*pstatus));
> +    actual = p->packet.actual_length;
> +    trace_usb_dwc2_packet_status(pstatus[stsidx], actual);
> +
> +babble:
> +    if (p->packet.status != USB_RET_SUCCESS &&
> +            p->packet.status != USB_RET_NAK &&
> +            p->packet.status != USB_RET_STALL &&
> +            p->packet.status != USB_RET_ASYNC) {
> +        fprintf(stderr, "%s: packet status %s actual %d\n", __func__,
> +                pstatus[stsidx], actual);

This one seems like maybe it should be a tracepoint.

> +    }
> +

> +    case GRSTCTL:
> +        val |= GRSTCTL_AHBIDLE;
> +        val &= ~GRSTCTL_DMAREQ;
> +        if (!(old & GRSTCTL_TXFFLSH) && (val & GRSTCTL_TXFFLSH)) {
> +                /* TODO - TX fifo flush */

Maybe LOG_UNIMP these so we know if the guest tries to use missing
functionality ?

> +        }
> +        if (!(old & GRSTCTL_RXFFLSH) && (val & GRSTCTL_RXFFLSH)) {
> +                /* TODO - RX fifo flush */
> +        }
> +        if (!(old & GRSTCTL_IN_TKNQ_FLSH) && (val & GRSTCTL_IN_TKNQ_FLSH)) {
> +                /* TODO - device IN token queue flush */
> +        }
> +        if (!(old & GRSTCTL_FRMCNTRRST) && (val & GRSTCTL_FRMCNTRRST)) {
> +                /* TODO - host frame counter reset */
> +        }
> +        if (!(old & GRSTCTL_HSFTRST) && (val & GRSTCTL_HSFTRST)) {
> +                /* TODO - ? soft reset */
> +        }
> +        if (!(old & GRSTCTL_CSFTRST) && (val & GRSTCTL_CSFTRST)) {
> +                /* TODO - core soft reset */
> +        }

> +    case HFNUM:
> +    case HPTXSTS:
> +    case HAINT:
> +        fprintf(stderr, "%s: write to read-only register\n", __func__);

This kind of message should be a LOG_GUEST_ERROR.

> +        return;


> +static void dwc2_reset(DeviceState *dev)
> +{
> +    DWC2State *s = DWC2_USB(dev);
> +    int i;
> +
> +    trace_usb_dwc2_reset();
> +    timer_del(s->frame_timer);
> +    qemu_bh_cancel(s->async_bh);
> +
> +    if (s->uport.dev && s->uport.dev->attached) {
> +        usb_detach(&s->uport);
> +    }
> +
> +    dwc2_bus_stop(s);


> +    dwc2_update_irq(s);

A device that uses single-phase reset shouldn't try to change
outbound IRQ lines from its reset function (because the device
on the other end might have already reset before this device,
or might reset after this device, and it doesn't necessarily
handle the irq line change correctly). If you need to
update IRQ lines in reset, you can use three-phase-reset
(see docs/devel/reset.rst).

thanks
-- PMM