Re: [Qemu-devel] [PATCH v2 02/20] arm: add Faraday FUSBH200 EHCI controller

[Gerd Hoffmann]

  Hi,

>> +static uint64_t
>> +ehci_faraday_read(void *ptr, hwaddr addr, unsigned size)
>> +{
>> +    hwaddr off = 0x34 + addr;
>> +
>> +    switch (off) {
>> +    case 0x34:  /* fusbh200: EOF/Async. Sleep Timer Register */
>> +        return 0x00000041;
>> +    case 0x40:  /* fusbh200: Bus Monitor Control/Status Register */
>> +        /* High-Speed, VBUS valid, interrupt level-high active */
>> +        return (2 << 9) | (1 << 8) | (1 << 3);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void
>> +ehci_faraday_write(void *ptr, hwaddr addr, uint64_t val, unsigned size)
>> +{
>> +}
>> +
>> +static const MemoryRegionOps ehci_mmio_faraday_ops = {
>> +    .read = ehci_faraday_read,
>> +    .write = ehci_faraday_write,
>> +    .valid.min_access_size = 4,
>> +    .valid.max_access_size = 4,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +};

This should go to hcd-ehci-sysbus.c

>> +    memory_region_init_io(&s->mem_faraday, &ehci_mmio_faraday_ops, s,
>> +                          "faraday", 0x4c);
> 
> I don't think this is good design... Can't you do the Faraday part from
> your own instance_init / initfn / realizefn function?

Yes, please.  Just add your own usb_ehci_sysbus_${board}_initfn, call
usb_ehci_sysbus_initfn for the common stuff, then add your
hardware-specific memory region.

> The define -> class/state movements look fine to me as continuation of
> our previous effort. Optionally this patch could be split into two -
> preparing the fields and updating all define users, and then introduce
> your new model.

Agree, splitting would be nice: one patch making portbase + nports
runtime configurable, one patch adding the new ehci controller.

cheers,
  Gerd