Re: [Qemu-devel] sh4 r2d system emulation - warning: could not add USB device keyboard

[Aurelien Jarno]

On Sat, Apr 18, 2009 at 12:30:01PM +0900, Shin-ichiro KAWASAKI wrote:
> Hi Kristoffer, Thank you for your feed back.
>
> Kristoffer Ericson wrote:
>> Hi,
>>
>> Using kernel from www.assembla.. (6th of april) and getting this at bootup :
>> Warning: could not add USB device keyboard
>>
>> starting with :
>> qemu-system-sh4 -M r2d -kernel r2d_nokernelarg_zImage -hda sh4-disk.img 
>> -m 512M -append "root=/dev/sda1" -serial vc -serial stdio -usb  
>> -usbdevice keyboard 
>>
>> inside boot window I see,
>> can't start sm501-usb.....
>> startup error -75
>
> SM501's usb support patch has not yet been merged into the qemu trunk.
>  http://lists.gnu.org/archive/html/qemu-devel/2009-03/msg00112.html
>
> Here's the new patch which can be applied current svn trunk, rev 7169.

It looks good, I just have a few minor comments. See below.

> Regards,
> Shin-ichiro KAWASAKI
>
>
> Signed-off-by: Shin-ichiro KAWASAKI <address@hidden> 
>
> Index: trunk/hw/r2d.c
> ===================================================================
> --- trunk/hw/r2d.c    (revision 6676)
> +++ trunk/hw/r2d.c    (working copy)
> @@ -222,7 +222,7 @@
>     irq = r2d_fpga_init(0x04000000, sh7750_irl(s));
>     pci = sh_pci_register_bus(r2d_pci_set_irq, r2d_pci_map_irq, irq, 0, 4);
>
> -    sm501_init(0x10000000, SM501_VRAM_SIZE, serial_hds[2]);
> +    sm501_init(0x10000000, SM501_VRAM_SIZE, irq[SM501], serial_hds[2]);
>
>     /* onboard CF (True IDE mode, Master only). */
>     if ((i = drive_get_index(IF_IDE, 0, 0)) != -1)
> Index: trunk/hw/usb-ohci.c
> ===================================================================
> --- trunk/hw/usb-ohci.c       (revision 6676)
> +++ trunk/hw/usb-ohci.c       (working copy)
> @@ -32,6 +32,7 @@
> #include "usb.h"
> #include "pci.h"
> #include "pxa.h"
> +#include "devices.h"
>
> //#define DEBUG_OHCI
> /* Dump packet contents.  */
> @@ -60,7 +61,8 @@
>
> enum ohci_type {
>     OHCI_TYPE_PCI,
> -    OHCI_TYPE_PXA
> +    OHCI_TYPE_PXA,
> +    OHCI_TYPE_SM501,
> };
>
> typedef struct {
> @@ -108,6 +110,9 @@
>     uint32_t hreset;
>     uint32_t htest;
>
> +    /* SM501 local memory offset */
> +    target_phys_addr_t localmem_base;
> +
>     /* Active packets.  */
>     uint32_t old_ctl;
>     USBPacket usb_packet;
> @@ -476,40 +481,67 @@
>     return 1;
> }
>
> -static inline int ohci_read_ed(uint32_t addr, struct ohci_ed *ed)
> +static inline int ohci_read_ed(OHCIState *ohci,
> +                            uint32_t addr, struct ohci_ed *ed)
> {
> -    return get_dwords(addr, (uint32_t *)ed, sizeof(*ed) >> 2);
> +    return get_dwords(addr + ohci->localmem_base,
> +                   (uint32_t *)ed, sizeof(*ed) >> 2);
> }

I think the address translation should be done as close as possible to
the call to cpu_physical_memory_rw(). Could you please move it to
get_dwords() instead, even if it means passing a pointer to the 
OHCIState struct there?

Please also use spaces for indentations of ohci_read_ed's arguments.

That's apply to the similar functions below (get_words(), put_dwords(),
put_words()).

> -static inline int ohci_read_td(uint32_t addr, struct ohci_td *td)
> +static inline int ohci_read_td(OHCIState *ohci,
> +                            uint32_t addr, struct ohci_td *td)
> {
> -    return get_dwords(addr, (uint32_t *)td, sizeof(*td) >> 2);
> +    return get_dwords(addr + ohci->localmem_base,
> +                   (uint32_t *)td, sizeof(*td) >> 2);
> }
>
> -static inline int ohci_read_iso_td(uint32_t addr, struct ohci_iso_td *td)
> +static inline int ohci_read_iso_td(OHCIState *ohci,
> +                                uint32_t addr, struct ohci_iso_td *td)
> {
> -    return (get_dwords(addr, (uint32_t *)td, 4) &&
> -            get_words(addr + 16, td->offset, 8));
> +    return (get_dwords(addr + ohci->localmem_base, (uint32_t *)td, 4) &&
> +            get_words(addr + 16 + ohci->localmem_base, td->offset, 8));
> }
>
> -static inline int ohci_put_ed(uint32_t addr, struct ohci_ed *ed)
> +static inline int ohci_read_hcca(OHCIState *ohci,
> +                              uint32_t addr, struct ohci_hcca *hcca)
> {
> -    return put_dwords(addr, (uint32_t *)ed, sizeof(*ed) >> 2);
> +    cpu_physical_memory_rw(addr + ohci->localmem_base,
> +                        (uint8_t *)hcca, sizeof(*hcca), 0);
> +    return 1;
> }
>
> -static inline int ohci_put_td(uint32_t addr, struct ohci_td *td)
> +static inline int ohci_put_ed(OHCIState *ohci,
> +                           uint32_t addr, struct ohci_ed *ed)
> {
> -    return put_dwords(addr, (uint32_t *)td, sizeof(*td) >> 2);
> +    return put_dwords(addr + ohci->localmem_base,
> +                   (uint32_t *)ed, sizeof(*ed) >> 2);
> }
>
> -static inline int ohci_put_iso_td(uint32_t addr, struct ohci_iso_td *td)
> +static inline int ohci_put_td(OHCIState *ohci,
> +                           uint32_t addr, struct ohci_td *td)
> {
> -    return (put_dwords(addr, (uint32_t *)td, 4) &&
> -            put_words(addr + 16, td->offset, 8));
> +    return put_dwords(addr + ohci->localmem_base,
> +                   (uint32_t *)td, sizeof(*td) >> 2);
> }
>
> +static inline int ohci_put_iso_td(OHCIState *ohci,
> +                               uint32_t addr, struct ohci_iso_td *td)
> +{
> +    return (put_dwords(addr + ohci->localmem_base, (uint32_t *)td, 4) &&
> +            put_words(addr + 16 + ohci->localmem_base, td->offset, 8));
> +}
> +
> +static inline int ohci_put_hcca(OHCIState *ohci,
> +                             uint32_t addr, struct ohci_hcca *hcca)
> +{
> +    cpu_physical_memory_rw(addr + ohci->localmem_base,
> +                        (uint8_t *)hcca, sizeof(*hcca), 1);
> +    return 1;
> +}
> +
> /* Read/Write the contents of a TD from/to main memory.  */
> -static void ohci_copy_td(struct ohci_td *td, uint8_t *buf, int len, int 
> write)
> +static void ohci_copy_td(OHCIState *ohci, struct ohci_td *td,
> +                      uint8_t *buf, int len, int write)
> {
>     uint32_t ptr;
>     uint32_t n;
> @@ -518,17 +550,18 @@
>     n = 0x1000 - (ptr & 0xfff);
>     if (n > len)
>         n = len;
> -    cpu_physical_memory_rw(ptr, buf, n, write);
> +    cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, n, write);
>     if (n == len)
>         return;
>     ptr = td->be & ~0xfffu;
>     buf += n;
> -    cpu_physical_memory_rw(ptr, buf, len - n, write);
> +    cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, len - n, write);
> }
>
> /* Read/Write the contents of an ISO TD from/to main memory.  */
> -static void ohci_copy_iso_td(uint32_t start_addr, uint32_t end_addr,
> -                             uint8_t *buf, int len, int write)
> +static void ohci_copy_iso_td(OHCIState *ohci,
> +                          uint32_t start_addr, uint32_t end_addr,
> +                          uint8_t *buf, int len, int write)
> {
>     uint32_t ptr;
>     uint32_t n;
> @@ -537,12 +570,12 @@
>     n = 0x1000 - (ptr & 0xfff);
>     if (n > len)
>         n = len;
> -    cpu_physical_memory_rw(ptr, buf, n, write);
> +    cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, n, write);
>     if (n == len)
>         return;
>     ptr = end_addr & ~0xfffu;
>     buf += n;
> -    cpu_physical_memory_rw(ptr, buf, len - n, write);
> +    cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, len - n, write);
> }
>
> static void ohci_process_lists(OHCIState *ohci, int completion);
> @@ -579,7 +612,7 @@
>
>     addr = ed->head & OHCI_DPTR_MASK;
>
> -    if (!ohci_read_iso_td(addr, &iso_td)) {
> +    if (!ohci_read_iso_td(ohci, addr, &iso_td)) {
>         printf("usb-ohci: ISO_TD read error at %x\n", addr);
>         return 0;
>     }
> @@ -621,7 +654,7 @@
>         i = OHCI_BM(iso_td.flags, TD_DI);
>         if (i < ohci->done_count)
>             ohci->done_count = i;
> -        ohci_put_iso_td(addr, &iso_td);        +        
> ohci_put_iso_td(ohci, addr, &iso_td);
>         return 0;
>     }
>
> @@ -696,7 +729,7 @@
>     }
>
>     if (len && dir != OHCI_TD_DIR_IN) {
> -        ohci_copy_iso_td(start_addr, end_addr, ohci->usb_buf, len, 0);
> +        ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len, 0);
>     }
>
>     if (completion) {
> @@ -732,7 +765,7 @@
>     /* Writeback */
>     if (dir == OHCI_TD_DIR_IN && ret >= 0 && ret <= len) {
>         /* IN transfer succeeded */
> -        ohci_copy_iso_td(start_addr, end_addr, ohci->usb_buf, ret, 1);
> +        ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret, 1);
>         OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_CC,
>                     OHCI_CC_NOERROR);
>         OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_SIZE, ret);
> @@ -788,7 +821,7 @@
>         if (i < ohci->done_count)
>             ohci->done_count = i;
>     }
> -    ohci_put_iso_td(addr, &iso_td);
> +    ohci_put_iso_td(ohci, addr, &iso_td);
>     return 1;
> }
>
> @@ -818,7 +851,7 @@
> #endif
>         return 1;
>     }
> -    if (!ohci_read_td(addr, &td)) {
> +    if (!ohci_read_td(ohci, addr, &td)) {
>         fprintf(stderr, "usb-ohci: TD read error at %x\n", addr);
>         return 0;
>     }
> @@ -859,7 +892,7 @@
>         }
>
>         if (len && dir != OHCI_TD_DIR_IN && !completion) {
> -            ohci_copy_td(&td, ohci->usb_buf, len, 0);
> +            ohci_copy_td(ohci, &td, ohci->usb_buf, len, 0);
>         }
>     }
>
> @@ -918,7 +951,7 @@
>     }
>     if (ret >= 0) {
>         if (dir == OHCI_TD_DIR_IN) {
> -            ohci_copy_td(&td, ohci->usb_buf, ret, 1);
> +            ohci_copy_td(ohci, &td, ohci->usb_buf, ret, 1);
> #ifdef DEBUG_PACKET
>             dprintf("  data:");
>             for (i = 0; i < ret; i++)
> @@ -987,7 +1020,7 @@
>     i = OHCI_BM(td.flags, TD_DI);
>     if (i < ohci->done_count)
>         ohci->done_count = i;
> -    ohci_put_td(addr, &td);
> +    ohci_put_td(ohci, addr, &td);
>     return OHCI_BM(td.flags, TD_CC) != OHCI_CC_NOERROR;
> }
>
> @@ -1005,7 +1038,7 @@
>         return 0;
>
>     for (cur = head; cur; cur = next_ed) {
> -        if (!ohci_read_ed(cur, &ed)) {
> +        if (!ohci_read_ed(ohci, cur, &ed)) {
>             fprintf(stderr, "usb-ohci: ED read error at %x\n", cur);
>             return 0;
>         }
> @@ -1046,7 +1079,7 @@
>             }
>         }
>
> -        ohci_put_ed(cur, &ed);
> +        ohci_put_ed(ohci, cur, &ed);
>     }
>
>     return active;
> @@ -1087,7 +1120,7 @@
>     OHCIState *ohci = opaque;
>     struct ohci_hcca hcca;
>
> -    cpu_physical_memory_rw(ohci->hcca, (uint8_t *)&hcca, sizeof(hcca), 0);
> +    ohci_read_hcca(ohci, ohci->hcca, &hcca);
>
>     /* Process all the lists at the end of the frame */
>     if (ohci->ctl & OHCI_CTL_PLE) {
> @@ -1131,7 +1164,7 @@
>     ohci_sof(ohci);
>
>     /* Writeback HCCA */
> -    cpu_physical_memory_rw(ohci->hcca, (uint8_t *)&hcca, sizeof(hcca), 1);
> +    ohci_put_hcca(ohci, ohci->hcca, &hcca);
> }
>
> /* Start sending SOF tokens across the USB bus, lists are processed in
> @@ -1620,7 +1653,8 @@
> };
>
> static void usb_ohci_init(OHCIState *ohci, int num_ports, int devfn,
> -            qemu_irq irq, enum ohci_type type, const char *name)
> +                          qemu_irq irq, enum ohci_type type,
> +                       const char *name, uint32_t localmem_base)
> {
>     int i;
>
> @@ -1641,6 +1675,7 @@
>     }
>
>     ohci->mem = cpu_register_io_memory(0, ohci_readfn, ohci_writefn, ohci);
> +    ohci->localmem_base = localmem_base;
>     ohci->name = name;
>
>     ohci->irq = irq;
> @@ -1687,7 +1722,7 @@
>     ohci->pci_dev.config[0x3d] = 0x01; /* interrupt pin 1 */
>
>     usb_ohci_init(&ohci->state, num_ports, devfn, ohci->pci_dev.irq[0],
> -                  OHCI_TYPE_PCI, ohci->pci_dev.name);
> +                  OHCI_TYPE_PCI, ohci->pci_dev.name, 0);
>
>     pci_register_io_region((struct PCIDevice *)ohci, 0, 256,
>                            PCI_ADDRESS_SPACE_MEM, ohci_mapfunc);
> @@ -1699,7 +1734,19 @@
>     OHCIState *ohci = (OHCIState *)qemu_mallocz(sizeof(OHCIState));
>
>     usb_ohci_init(ohci, num_ports, devfn, irq,
> -                  OHCI_TYPE_PXA, "OHCI USB");
> +                  OHCI_TYPE_PXA, "OHCI USB", 0);
>
>     cpu_register_physical_memory(base, 0x1000, ohci->mem);
> }
> +
> +void usb_ohci_init_sm501(uint32_t mmio_base, uint32_t localmem_base,
> +                      int num_ports, int devfn, qemu_irq irq)
> +{
> +    OHCIState *ohci = (OHCIState *)qemu_mallocz(sizeof(OHCIState));
> +
> +    usb_ohci_init(ohci, num_ports, devfn, irq,
> +                  OHCI_TYPE_SM501, "OHCI USB", localmem_base);
> +
> +    cpu_register_physical_memory(mmio_base, 0x1000, ohci->mem);
> +}
> +
> Index: trunk/hw/sm501.c
> ===================================================================
> --- trunk/hw/sm501.c  (revision 6676)
> +++ trunk/hw/sm501.c  (working copy)
> @@ -1055,7 +1055,8 @@
>       sm501_draw_crt(s);
> }
>
> -void sm501_init(uint32_t base, uint32_t local_mem_bytes, CharDriverState 
> *chr)
> +void sm501_init(uint32_t base, uint32_t local_mem_bytes, qemu_irq irq,
> +                CharDriverState *chr)
> {
>     SM501State * s;
>     int sm501_system_config_index;
> @@ -1089,6 +1090,10 @@
>     cpu_register_physical_memory(base + MMIO_BASE_OFFSET + SM501_DC,
>                                  0x1000, sm501_disp_ctrl_index);
>
> +    /* bridge to usb host emulation module */
> +    usb_ohci_init_sm501(base + MMIO_BASE_OFFSET + SM501_USB_HOST, base,  
> +                     2, -1, irq);
> +
>     /* bridge to serial emulation module */
>     if (chr)
>       serial_mm_init(base + MMIO_BASE_OFFSET + SM501_UART0, 2,
> Index: trunk/hw/devices.h
> ===================================================================
> --- trunk/hw/devices.h        (revision 6676)
> +++ trunk/hw/devices.h        (working copy)
> @@ -74,5 +74,10 @@
> qemu_irq tc6393xb_l3v_get(struct tc6393xb_s *s);
>
> /* sm501.c */
> -void sm501_init(uint32_t base, uint32_t local_mem_bytes, CharDriverState 
> *chr);
> +void sm501_init(uint32_t base, uint32_t local_mem_bytes, qemu_irq irq,
> +                CharDriverState *chr);
> +
> +/* usb-ohci.c */
> +void usb_ohci_init_sm501(uint32_t mmio_base, uint32_t localmem_base,
> +                         int num_ports, int devfn, qemu_irq irq);
> #endif
>
>
>

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net