Re: [Qemu-devel] [Qemu-ppc][PATCH v8 1/2] Add USB option in machine options

[Alexander Graf]

On 14.08.2012, at 19:49, Li Zhang <address@hidden> wrote:

> When -usb option is used, global varible usb_enabled is set.
> And all the plafrom will create one USB controller according
> to this variable. In fact, global varibles make code hard
> to read.
> 
> So this patch is to remove global variable usb_enabled and
> add USB option in machine options. All the plaforms will get
> USB option value from machine options.
> 
> USB option of machine options will be set either by:
>  * -usb
>  * -machine type=pseries,usb=on
> 
> Both these ways can work now. They both set USB option in
> machine options. In the future, the first way will be removed.
> 
> Signed-off-by: Li Zhang <address@hidden>
> ---
> v7->v8 : 
>  * Declare usb_enabled() and set_usb_option() in sysemu.h
>  * Separate USB enablement on sPAPR platform.
> 
> hw/nseries.c      |    2 +-
> hw/pc_piix.c      |    2 +-
> hw/ppc_newworld.c |    4 ++--
> hw/ppc_oldworld.c |    2 +-
> hw/ppc_prep.c     |    2 +-
> hw/pxa2xx.c       |    4 ++--
> hw/realview.c     |    2 +-
> hw/versatilepb.c  |    2 +-
> qemu-config.c     |    4 ++++
> sysemu.h          |    5 ++++-
> vl.c              |   34 ++++++++++++++++++++++++++++------
> 11 files changed, 46 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/nseries.c b/hw/nseries.c
> index 4df2670..c67e95a 100644
> --- a/hw/nseries.c
> +++ b/hw/nseries.c
> @@ -1322,7 +1322,7 @@ static void n8x0_init(ram_addr_t ram_size, const char 
> *boot_device,
>     n8x0_dss_setup(s);
>     n8x0_cbus_setup(s);
>     n8x0_uart_setup(s);
> -    if (usb_enabled)
> +    if (usb_enabled(false))
>         n8x0_usb_setup(s);
> 
>     if (kernel_filename) {
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 0c0096f..b662192 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -267,7 +267,7 @@ static void pc_init1(MemoryRegion *system_memory,
>     pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
>                  floppy, idebus[0], idebus[1], rtc_state);
> 
> -    if (pci_enabled && usb_enabled) {
> +    if (pci_enabled && usb_enabled(false)) {
>         pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
>     }
> 
> diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
> index 4e2a6e6..fe92491 100644
> --- a/hw/ppc_newworld.c
> +++ b/hw/ppc_newworld.c
> @@ -350,7 +350,7 @@ static void ppc_core99_init (ram_addr_t ram_size,
> 
>     /* cuda also initialize ADB */
>     if (machine_arch == ARCH_MAC99_U3) {
> -        usb_enabled = 1;
> +         set_usb_option(true);

Semantically, this one sets the default usb_enabled to true. Just make the 
usb_enabled() call get its default value as true when arch == mac99_u3.

>     }
>     cuda_init(&cuda_mem, pic[0x19]);
> 
> @@ -360,7 +360,7 @@ static void ppc_core99_init (ram_addr_t ram_size,
>     macio_init(pci_bus, PCI_DEVICE_ID_APPLE_UNI_N_KEYL, 0, pic_mem,
>                dbdma_mem, cuda_mem, NULL, 3, ide_mem, escc_bar);
> 
> -    if (usb_enabled) {
> +    if (usb_enabled(true)) {

Why true?

>         pci_create_simple(pci_bus, -1, "pci-ohci");
>     }
> 
> diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
> index f2c6908..a62b3d8 100644
> --- a/hw/ppc_oldworld.c
> +++ b/hw/ppc_oldworld.c
> @@ -286,7 +286,7 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
>     macio_init(pci_bus, PCI_DEVICE_ID_APPLE_343S1201, 1, pic_mem,
>                dbdma_mem, cuda_mem, nvr, 2, ide_mem, escc_bar);
> 
> -    if (usb_enabled) {
> +    if (usb_enabled(true)) {

Why true? Same for all occurencex of true as default below. Today, we always 
default to false, no?

>         pci_create_simple(pci_bus, -1, "pci-ohci");
>     }
> 
> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
> index be2b268..84ead22 100644
> --- a/hw/ppc_prep.c
> +++ b/hw/ppc_prep.c
> @@ -661,7 +661,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
>     memory_region_add_subregion(sysmem, 0xFEFF0000, xcsr);
> #endif
> 
> -    if (usb_enabled) {
> +    if (usb_enabled(true)) {
>         pci_create_simple(pci_bus, -1, "pci-ohci");
>     }
> 
> diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
> index d5f1420..28eb92e 100644
> --- a/hw/pxa2xx.c
> +++ b/hw/pxa2xx.c
> @@ -2108,7 +2108,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
>         s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>     }
> 
> -    if (usb_enabled) {
> +    if (usb_enabled(true)) {
>         sysbus_create_simple("sysbus-ohci", 0x4c000000,
>                         qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>     }
> @@ -2239,7 +2239,7 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, 
> unsigned int sdram_size)
>         s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>     }
> 
> -    if (usb_enabled) {
> +    if (usb_enabled(false)) {
>         sysbus_create_simple("sysbus-ohci", 0x4c000000,
>                         qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>     }
> diff --git a/hw/realview.c b/hw/realview.c
> index 19db4d0..a53f327 100644
> --- a/hw/realview.c
> +++ b/hw/realview.c
> @@ -227,7 +227,7 @@ static void realview_init(ram_addr_t ram_size,
>         sysbus_connect_irq(busdev, 2, pic[50]);
>         sysbus_connect_irq(busdev, 3, pic[51]);
>         pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
> -        if (usb_enabled) {
> +        if (usb_enabled(true)) {
>             pci_create_simple(pci_bus, -1, "pci-ohci");
>         }
>         n = drive_get_max_bus(IF_SCSI);
> diff --git a/hw/versatilepb.c b/hw/versatilepb.c
> index 4fd5d9b..f76671c 100644
> --- a/hw/versatilepb.c
> +++ b/hw/versatilepb.c
> @@ -247,7 +247,7 @@ static void versatile_init(ram_addr_t ram_size,
>             pci_nic_init_nofail(nd, "rtl8139", NULL);
>         }
>     }
> -    if (usb_enabled) {
> +    if (usb_enabled(false)) {
>         pci_create_simple(pci_bus, -1, "pci-ohci");
>     }
>     n = drive_get_max_bus(IF_SCSI);
> diff --git a/qemu-config.c b/qemu-config.c
> index 5c3296b..b86ee36 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -595,6 +595,10 @@ static QemuOptsList qemu_machine_opts = {
>             .name = "dt_compatible",
>             .type = QEMU_OPT_STRING,
>             .help = "Overrides the \"compatible\" property of the dt root 
> node",
> +        },{
> +            .name = "usb",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "Set on/off to enable/disable usb",
>         },
>         { /* End of list */ }
>     },
> diff --git a/sysemu.h b/sysemu.h
> index 4669348..3492fef 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -118,7 +118,6 @@ extern const char *keyboard_layout;
> extern int win2k_install_hack;
> extern int alt_grab;
> extern int ctrl_grab;
> -extern int usb_enabled;
> extern int smp_cpus;
> extern int max_cpus;
> extern int cursor_hide;
> @@ -188,4 +187,8 @@ void register_devices(void);
> void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>                           const char *suffix);
> char *get_boot_devices_list(uint32_t *size);
> +
> +bool usb_enabled(bool default_usb);
> +void set_usb_option(bool usb_option);

I don't think we need an exported helper to set the option. Machines only ever 
want to be able to override the default.


Alex

> +
> #endif
> diff --git a/vl.c b/vl.c
> index d01256a..bf51249 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -198,7 +198,6 @@ CharDriverState *serial_hds[MAX_SERIAL_PORTS];
> CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
> CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
> int win2k_install_hack = 0;
> -int usb_enabled = 0;
> int singlestep = 0;
> int smp_cpus = 1;
> int max_cpus = 0;
> @@ -765,6 +764,27 @@ static int bt_parse(const char *opt)
>     return 1;
> }
> 
> +/*********Qemu USB setting******/
> +bool usb_enabled(bool default_usb)
> +{
> +    QemuOpts *mach_opts;
> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
> +    if (mach_opts) {
> +        return qemu_opt_get_bool(mach_opts, "usb", default_usb);
> +    }
> +    return default_usb;
> +}
> +
> +void set_usb_option(bool usb_option)
> +{
> +    QemuOpts *mach_opts;
> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
> +    if (mach_opts) {
> +        qemu_opt_set_bool(mach_opts, "usb", usb_option);
> +    }
> +}
> +
> +
> /***********************************************************/
> /* QEMU Block devices */
> 
> @@ -1053,8 +1073,9 @@ static int usb_device_add(const char *devname)
>     const char *p;
>     USBDevice *dev = NULL;
> 
> -    if (!usb_enabled)
> +    if (!usb_enabled(false)) {
>         return -1;
> +    }
> 
>     /* drivers with .usbdevice_name entry in USBDeviceInfo */
>     dev = usbdevice_create(devname);
> @@ -1090,8 +1111,9 @@ static int usb_device_del(const char *devname)
>     if (strstart(devname, "host:", &p))
>         return usb_host_device_close(p);
> 
> -    if (!usb_enabled)
> +    if (!usb_enabled(false)) {
>         return -1;
> +    }
> 
>     p = strchr(devname, '.');
>     if (!p)
> @@ -3027,10 +3049,10 @@ int main(int argc, char **argv, char **envp)
>                 }
>                 break;
>             case QEMU_OPTION_usb:
> -                usb_enabled = 1;
> +                set_usb_option(true);
>                 break;
>             case QEMU_OPTION_usbdevice:
> -                usb_enabled = 1;
> +                set_usb_option(true);
>                 add_device_config(DEV_USB, optarg);
>                 break;
>             case QEMU_OPTION_device:
> @@ -3578,7 +3600,7 @@ int main(int argc, char **argv, char **envp)
>     current_machine = machine;
> 
>     /* init USB devices */
> -    if (usb_enabled) {
> +    if (usb_enabled(false)) {
>         if (foreach_device_config(DEV_USB, usb_parse) < 0)
>             exit(1);
>     }
> -- 
> 1.7.7.6
>