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

2020-04-20 Thread Paul Zimmerman
Hi Philippe,

On Mon, Apr 20, 2020 at 12:16 AM Philippe Mathieu-Daudé 
wrote:

> Hi Paul,
>
> On 3/29/20 1:17 AM, Paul Zimmerman 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.
> >
> > Signed-off-by: Paul Zimmerman 
> > ---
> >  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
> [...]
> > +static void dwc2_init(DWC2State *s, DeviceState *dev)
> > +{
> > +s->usb_frame_time = NANOSECONDS_PER_SECOND / 1000;  /*
> 100 */
> > +if (NANOSECONDS_PER_SECOND >= USB_HZ_FS) {
> > +s->usb_bit_time = NANOSECONDS_PER_SECOND / USB_HZ_FS;   /* 83.3
> */
> > +} else {
> > +s->usb_bit_time = 1;
> > +}
> > +
> > +s->fi = 11999;
>
> What is this magic number?
>

This is the USB frame interval, in bit times I believe. It's actually
12000-1. Not sure of the reason for the -1, I lifted this code directly
from the hcd-ohci driver and it seems to work ;) The hcd-ohci driver
actually has this value in hex (0x2edf), I changed it to decimal because
I thought it was clearer. I can make a #define for this if you think
that would be better.

> +
> > +memory_region_init(>mem, OBJECT(dev), "dwc2", DWC2_MMIO_SIZE);
> > +memory_region_init_io(>mem_glbreg, OBJECT(dev),
> _mmio_glbreg_ops, s,
> > +  "global", 0x70);
> > +memory_region_init_io(>mem_fszreg, OBJECT(dev),
> _mmio_fszreg_ops, s,
> > +  "hptxfsiz", 0x4);
> > +memory_region_init_io(>mem_hreg0, OBJECT(dev),
> _mmio_hreg0_ops, s,
> > +  "host", 0x44);
> > +memory_region_init_io(>mem_hreg1, OBJECT(dev),
> _mmio_hreg1_ops, s,
> > +  "host channels", 0x20 * NB_CHAN);
> > +memory_region_init_io(>mem_pcgreg, OBJECT(dev),
> _mmio_pcgreg_ops, s,
> > +  "power/clock", 0x8);
> > +memory_region_init_io(>mem_hreg2, OBJECT(dev),
> _mmio_hreg2_ops, s,
> > +  "host fifos", NB_CHAN * 0x1000);
> > +
> > +memory_region_add_subregion(>mem, s->glbregbase, >mem_glbreg);
> > +memory_region_add_subregion(>mem, s->fszregbase, >mem_fszreg);
> > +memory_region_add_subregion(>mem, s->hreg0base, >mem_hreg0);
> > +memory_region_add_subregion(>mem, s->hreg1base, >mem_hreg1);
> > +memory_region_add_subregion(>mem, s->pcgregbase, >mem_pcgreg);
> > +memory_region_add_subregion(>mem, s->hreg2base, >mem_hreg2);
> > +
> > +s->eof_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > +dwc2_frame_boundary, s);
> > +}
> > +
> > +static void dwc2_sysbus_reset(DeviceState *dev)
> > +{
> > +SysBusDevice *d = SYS_BUS_DEVICE(dev);
> > +DWC2State *s = DWC2_USB(d);
> > +
> > +dwc2_reset(s);
> > +}
> > +
> > +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;
>
> No need to use variable for the constant base addresses, use them
> directly in as argument to memory_region_add_subregion().
>

These values are also used in the register read/write functions,
that's why they are in variables.

Since you don't reuse each block, and blocks cover very few registers,
> have you considered using a pair of MRs instead? One of 4KB and the
> other 64KB. hreg2 is the only one particularly different.
>

Because this controller has so many registers, I use arrays
in the DWC2State struct to hold them instead of addressing
them individually. If I only used two MRs, those arrays would
either need be a lot bigger, or the read/write functions would
need some special logic to address the correct array. I thought
the current method was the most straightforward.

Thanks,
Paul


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

2020-04-20 Thread Paul Zimmerman
Hi Philippe,

As the label says, these are the host FIFOs. The controller has a
mode where DMA is not used, and instead the driver reads the data
directly from the FIFOs. I have not implemented this feature yet,
but as I recall one of the BSDs uses this in their driver, so I
plan to implement this in the future.

Thanks,
Paul


On Mon, Apr 20, 2020 at 12:25 AM Philippe Mathieu-Daudé 
wrote:

> On 3/29/20 1:17 AM, Paul Zimmerman 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.
> >
> > Signed-off-by: Paul Zimmerman 
> > ---
> >  hw/usb/hcd-dwc2.c   | 1301 +++
> >  hw/usb/trace-events |   47 ++
> >  2 files changed, 1348 insertions(+)
> >  create mode 100644 hw/usb/hcd-dwc2.c
> [...]
> > +static void dwc2_hreg2_write(void *ptr, hwaddr addr, uint64_t val,
> > + unsigned size)
> > +{
> > +uint64_t orig = val;
> > +
> > +/* TODO - implement FIFOs to support slave mode */
> > +val &= 0x;
> > +trace_usb_dwc2_hreg2_write(addr, addr >> 12, orig, 0, val);
> > +}
> > +
> [...]
>  +
> > +static const MemoryRegionOps dwc2_mmio_hreg2_ops = {
> > +.read = dwc2_hreg2_read,
> > +.write = dwc2_hreg2_write,
> > +.valid.min_access_size = 4,
> > +.valid.max_access_size = 4,
> > +.endianness = DEVICE_LITTLE_ENDIAN,
> > +};
> [...]
> > +static void dwc2_init(DWC2State *s, DeviceState *dev)
> > +{
> > +s->usb_frame_time = NANOSECONDS_PER_SECOND / 1000;  /*
> 100 */
> > +if (NANOSECONDS_PER_SECOND >= USB_HZ_FS) {
> > +s->usb_bit_time = NANOSECONDS_PER_SECOND / USB_HZ_FS;   /* 83.3
> */
> > +} else {
> > +s->usb_bit_time = 1;
> > +}
> > +
> > +s->fi = 11999;
> > +
> > +memory_region_init(>mem, OBJECT(dev), "dwc2", DWC2_MMIO_SIZE);
> > +memory_region_init_io(>mem_glbreg, OBJECT(dev),
> _mmio_glbreg_ops, s,
> > +  "global", 0x70);
> > +memory_region_init_io(>mem_fszreg, OBJECT(dev),
> _mmio_fszreg_ops, s,
> > +  "hptxfsiz", 0x4);
> > +memory_region_init_io(>mem_hreg0, OBJECT(dev),
> _mmio_hreg0_ops, s,
> > +  "host", 0x44);
> > +memory_region_init_io(>mem_hreg1, OBJECT(dev),
> _mmio_hreg1_ops, s,
> > +  "host channels", 0x20 * NB_CHAN);
> > +memory_region_init_io(>mem_pcgreg, OBJECT(dev),
> _mmio_pcgreg_ops, s,
> > +  "power/clock", 0x8);
> > +memory_region_init_io(>mem_hreg2, OBJECT(dev),
> _mmio_hreg2_ops, s,
> > +  "host fifos", NB_CHAN * 0x1000);
> [...]
> What is this region used for? 64KB of packet buffer sram? I'm wondering
> if this shouldn't be created with a memory_region_init_ram() call actually.
>
>


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

2020-04-20 Thread Philippe Mathieu-Daudé
On 3/29/20 1:17 AM, Paul Zimmerman 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.
> 
> Signed-off-by: Paul Zimmerman 
> ---
>  hw/usb/hcd-dwc2.c   | 1301 +++
>  hw/usb/trace-events |   47 ++
>  2 files changed, 1348 insertions(+)
>  create mode 100644 hw/usb/hcd-dwc2.c
[...]
> +static void dwc2_hreg2_write(void *ptr, hwaddr addr, uint64_t val,
> + unsigned size)
> +{
> +uint64_t orig = val;
> +
> +/* TODO - implement FIFOs to support slave mode */
> +val &= 0x;
> +trace_usb_dwc2_hreg2_write(addr, addr >> 12, orig, 0, val);
> +}
> +
[...]
 +
> +static const MemoryRegionOps dwc2_mmio_hreg2_ops = {
> +.read = dwc2_hreg2_read,
> +.write = dwc2_hreg2_write,
> +.valid.min_access_size = 4,
> +.valid.max_access_size = 4,
> +.endianness = DEVICE_LITTLE_ENDIAN,
> +};
[...]
> +static void dwc2_init(DWC2State *s, DeviceState *dev)
> +{
> +s->usb_frame_time = NANOSECONDS_PER_SECOND / 1000;  /* 100 */
> +if (NANOSECONDS_PER_SECOND >= USB_HZ_FS) {
> +s->usb_bit_time = NANOSECONDS_PER_SECOND / USB_HZ_FS;   /* 83.3 */
> +} else {
> +s->usb_bit_time = 1;
> +}
> +
> +s->fi = 11999;
> +
> +memory_region_init(>mem, OBJECT(dev), "dwc2", DWC2_MMIO_SIZE);
> +memory_region_init_io(>mem_glbreg, OBJECT(dev), 
> _mmio_glbreg_ops, s,
> +  "global", 0x70);
> +memory_region_init_io(>mem_fszreg, OBJECT(dev), 
> _mmio_fszreg_ops, s,
> +  "hptxfsiz", 0x4);
> +memory_region_init_io(>mem_hreg0, OBJECT(dev), _mmio_hreg0_ops, 
> s,
> +  "host", 0x44);
> +memory_region_init_io(>mem_hreg1, OBJECT(dev), _mmio_hreg1_ops, 
> s,
> +  "host channels", 0x20 * NB_CHAN);
> +memory_region_init_io(>mem_pcgreg, OBJECT(dev), 
> _mmio_pcgreg_ops, s,
> +  "power/clock", 0x8);
> +memory_region_init_io(>mem_hreg2, OBJECT(dev), _mmio_hreg2_ops, 
> s,
> +  "host fifos", NB_CHAN * 0x1000);
[...]
What is this region used for? 64KB of packet buffer sram? I'm wondering
if this shouldn't be created with a memory_region_init_ram() call actually.




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

2020-04-20 Thread Philippe Mathieu-Daudé
Hi Paul,

On 3/29/20 1:17 AM, Paul Zimmerman 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.
> 
> Signed-off-by: Paul Zimmerman 
> ---
>  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
[...]
> +static void dwc2_init(DWC2State *s, DeviceState *dev)
> +{
> +s->usb_frame_time = NANOSECONDS_PER_SECOND / 1000;  /* 100 */
> +if (NANOSECONDS_PER_SECOND >= USB_HZ_FS) {
> +s->usb_bit_time = NANOSECONDS_PER_SECOND / USB_HZ_FS;   /* 83.3 */
> +} else {
> +s->usb_bit_time = 1;
> +}
> +
> +s->fi = 11999;

What is this magic number?

> +
> +memory_region_init(>mem, OBJECT(dev), "dwc2", DWC2_MMIO_SIZE);
> +memory_region_init_io(>mem_glbreg, OBJECT(dev), 
> _mmio_glbreg_ops, s,
> +  "global", 0x70);
> +memory_region_init_io(>mem_fszreg, OBJECT(dev), 
> _mmio_fszreg_ops, s,
> +  "hptxfsiz", 0x4);
> +memory_region_init_io(>mem_hreg0, OBJECT(dev), _mmio_hreg0_ops, 
> s,
> +  "host", 0x44);
> +memory_region_init_io(>mem_hreg1, OBJECT(dev), _mmio_hreg1_ops, 
> s,
> +  "host channels", 0x20 * NB_CHAN);
> +memory_region_init_io(>mem_pcgreg, OBJECT(dev), 
> _mmio_pcgreg_ops, s,
> +  "power/clock", 0x8);
> +memory_region_init_io(>mem_hreg2, OBJECT(dev), _mmio_hreg2_ops, 
> s,
> +  "host fifos", NB_CHAN * 0x1000);
> +
> +memory_region_add_subregion(>mem, s->glbregbase, >mem_glbreg);
> +memory_region_add_subregion(>mem, s->fszregbase, >mem_fszreg);
> +memory_region_add_subregion(>mem, s->hreg0base, >mem_hreg0);
> +memory_region_add_subregion(>mem, s->hreg1base, >mem_hreg1);
> +memory_region_add_subregion(>mem, s->pcgregbase, >mem_pcgreg);
> +memory_region_add_subregion(>mem, s->hreg2base, >mem_hreg2);
> +
> +s->eof_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +dwc2_frame_boundary, s);
> +}
> +
> +static void dwc2_sysbus_reset(DeviceState *dev)
> +{
> +SysBusDevice *d = SYS_BUS_DEVICE(dev);
> +DWC2State *s = DWC2_USB(d);
> +
> +dwc2_reset(s);
> +}
> +
> +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;

No need to use variable for the constant base addresses, use them
directly in as argument to memory_region_add_subregion().

Since you don't reuse each block, and blocks cover very few registers,
have you considered using a pair of MRs instead? One of 4KB and the
other 64KB. hreg2 is the only one particularly different.

> +s->portnr = NB_PORTS;
> +s->as = _space_memory;
> +
> +dwc2_realize(s, dev, errp);
> +dwc2_init(s, dev);
> +sysbus_init_irq(d, >irq);
> +sysbus_init_mmio(d, >mem);
> +}
[...]



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

2020-04-18 Thread Paul Zimmerman

Hi Peter,

On 4/16/20 8:45 AM, Peter Maydell wrote:

On Sun, 29 Mar 2020 at 00:18, Paul Zimmerman  wrote:


< snip >


+/* 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?


I would prefer not to do that if possible. By using these macros
(which I borrowed from hcd-ehci and hcd-xhci BTW) I am able to use
the existing dwc2 register definition file from the Linux kernel
without modification. To use the macros from registerfields.h it
looks like I would need to write new definitions for all of the
dwc2 registers. I would really like to avoid that.

Thanks,
Paul



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

2020-04-16 Thread Paul Zimmerman

On 4/16/20 9:30 AM, Philippe Mathieu-Daudé wrote:

On 4/16/20 5:47 PM, Peter Maydell wrote:

On Thu, 16 Apr 2020 at 16:45, Peter Maydell  wrote:


On Sun, 29 Mar 2020 at 00:18, Paul Zimmerman  wrote:



+    s->as = _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.)


On closer inspection you're already doing that with the dma_as/
dma_mr. What's this AddressSpace for if it's different?


s->as is not used, probably a leftover (s->dma_as is used).



thanks
-- PMM


Thanks for the reviews guys, I will take all your suggestions into
account before I post the next series.

Thanks,
Paul





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

2020-04-16 Thread Philippe Mathieu-Daudé

On 4/16/20 5:47 PM, Peter Maydell wrote:

On Thu, 16 Apr 2020 at 16:45, Peter Maydell  wrote:


On Sun, 29 Mar 2020 at 00:18, Paul Zimmerman  wrote:



+s->as = _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.)


On closer inspection you're already doing that with the dma_as/
dma_mr. What's this AddressSpace for if it's different?


s->as is not used, probably a leftover (s->dma_as is used).



thanks
-- PMM






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

2020-04-16 Thread Peter Maydell
On Thu, 16 Apr 2020 at 16:45, Peter Maydell  wrote:
>
> On Sun, 29 Mar 2020 at 00:18, Paul Zimmerman  wrote:

> > +s->as = _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.)

On closer inspection you're already doing that with the dma_as/
dma_mr. What's this AddressSpace for if it's different?

thanks
-- PMM



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

2020-04-16 Thread Peter Maydell
On Sun, 29 Mar 2020 at 00:18, Paul Zimmerman  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 
> ---
>  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 00..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 
> + *
> + * 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   1200
> +#define USB_HZ_HS   9600
> +
> +/* 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 = _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",
 _fatal);

> +dwc2_init(s, dev);
> +sysbus_init_irq(d, >irq);
> +sysbus_init_mmio(d, >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(_usb_type_info);
> +}

thanks
-- PMM



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

2020-03-28 Thread Paul Zimmerman
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.

Signed-off-by: Paul Zimmerman 
---
 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 00..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 
+ *
+ * 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   1200
+#define USB_HZ_HS   9600
+
+/* 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))
+
+/* update irq line */
+static inline void dwc2_update_irq(DWC2State *s)
+{
+static int oldlevel;
+int level = 0;
+
+if ((s->gintsts & s->gintmsk) && (s->gahbcfg & GAHBCFG_GLBL_INTR_EN)) {
+level = 1;
+}
+if (level != oldlevel) {
+oldlevel = level;
+trace_usb_dwc2_update_irq(level);
+qemu_set_irq(s->irq, level);
+}
+}
+
+/* flag interrupt condition */
+static inline void dwc2_raise_global_irq(DWC2State *s, uint32_t intr)
+{
+if (!(s->gintsts & intr)) {
+s->gintsts |= intr;
+trace_usb_dwc2_raise_global_irq(intr);
+dwc2_update_irq(s);
+}
+}
+
+static inline void dwc2_lower_global_irq(DWC2State *s, uint32_t intr)
+{
+if (s->gintsts & intr) {
+s->gintsts &= ~intr;
+trace_usb_dwc2_lower_global_irq(intr);
+dwc2_update_irq(s);
+}
+}
+
+static inline void dwc2_raise_host_irq(DWC2State *s, uint32_t host_intr)
+{
+if (!(s->haint & host_intr)) {
+s->haint |= host_intr;
+s->haint &= 0x;
+trace_usb_dwc2_raise_host_irq(host_intr);
+if (s->haint & s->haintmsk) {
+dwc2_raise_global_irq(s, GINTSTS_HCHINT);
+}
+}
+}
+
+static inline void dwc2_lower_host_irq(DWC2State *s, uint32_t host_intr)
+{
+if (s->haint & host_intr) {
+s->haint &= ~host_intr;
+trace_usb_dwc2_lower_host_irq(host_intr);
+if (!(s->haint & s->haintmsk)) {
+dwc2_lower_global_irq(s, GINTSTS_HCHINT);
+}
+}
+}
+
+static inline void dwc2_update_hc_irq(DWC2State *s, int index)
+{
+uint32_t host_intr = 1 << (index >> 3);
+
+if (s->hreg1[index + 2] & s->hreg1[index + 3]) {
+dwc2_raise_host_irq(s, host_intr);
+} else {
+dwc2_lower_host_irq(s, host_intr);
+}
+}
+
+/* set a timer for EOF */
+static void dwc2_eof_timer(DWC2State *s)
+{
+timer_mod(s->eof_timer, s->sof_time + s->usb_frame_time);
+}
+
+/* Set a timer for EOF and generate a SOF event */
+static void dwc2_sof(DWC2State *s)
+{
+s->sof_time += s->usb_frame_time;
+trace_usb_dwc2_sof(s->sof_time);
+dwc2_eof_timer(s);
+dwc2_raise_global_irq(s, GINTSTS_SOF);
+}
+
+/* Do frame processing on frame boundary */
+static void dwc2_frame_boundary(void *opaque)
+{
+DWC2State *s = opaque;
+
+/* Frame boundary, so do EOF stuff here */
+
+/* Increment frame number */
+s->frame_number = (s->frame_number + 1) & 0x;
+s->hfnum = (s->hfnum & ~HFNUM_FRNUM_MASK) |
+