Re: [PATCH 1/3] hw/arm: Add EHCI/OHCI controllers to Allwinner R40 and Bananapi board

[Philippe Mathieu-Daudé]

Hi,

On 13/1/24 20:16, Guenter Roeck wrote:
> Allwinner R40 supports two USB host ports shared between a USB 2.0 EHCI
> host controller and a USB 1.1 OHCI host controller. Add support for both
> of them.
>
> If machine USB support is not enabled, create unimplemented devices
> for the USB memory ranges to avoid crashes when booting Linux.

I never really understood the reason for machine_usb() and had on my
TODO to do some archeology research to figure it out since quite some
time. Having to map an UnimpDevice due to CLI options seems like an
anti-pattern when the device is indeed implemented in the repository.

IIUC MachineState was introduced by a big rework during 2014-2015,
and ::usb became a property.

Reading git-history and mailing list threads, apparently it was not
easy for the mac99 machine (read whole thread):
https://lore.kernel.org/qemu-devel/1420550957-22337-2-git-send-email-marcel@redhat.com/
A bit later commit c6e765035b ("powerpc: fix -machine usb=no for
newworld and pseries machines") introduced the ::usb_disabled property
to deal with CLI -nodefaults with these 2 specific machines.

(-nodefaults is yet another CLI option I never really understood but
take it as legacy heritage with biggest maintenance headaches).


The R40 is a SoC photolithographied on silicon as a single piece
with a set of in-silicon peripherals, we can not remove peripherals
from this HW. We can not remove its USB controller.

What we can do is play at the external devices connected on buses
exposed by the SoC.

I can understand it is convenient for CLI users to start a machine
with a console and a keyboard, so if a machine provides a USB bus,
then it is created with a USB to UART converter and a USB keyboard.

So I'd interpret '-machine usb=off' as "if this machine has a USB
controller providing a USB bus which is not exposed outside of the
machine", but certainly not as "any mmio-mapped USB controller is
removed from chipsets".


To the extend I can understand floppy/cdrom/sdcard drives can be
inserted into available controllers used as buses, but I don't
really understand how to do that with serial/parallel.


> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>   docs/system/arm/bananapi_m2u.rst |  2 +-
>   hw/arm/Kconfig                   |  2 +
>   hw/arm/allwinner-r40.c           | 70 +++++++++++++++++++++++++++++++-
>   include/hw/arm/allwinner-r40.h   |  9 ++++
>   4 files changed, 80 insertions(+), 3 deletions(-)


> @@ -407,6 +427,37 @@ static void allwinner_r40_realize(DeviceState *dev, Error 
> **errp)
>       sysbus_realize(SYS_BUS_DEVICE(&s->ccu), &error_fatal);
>       sysbus_mmio_map(SYS_BUS_DEVICE(&s->ccu), 0, s->memmap[AW_R40_DEV_CCU]);
>   
> +    /* USB */
> +    if (machine_usb(current_machine)) {
> +        int i;
> +
> +        for (i = 0; i < AW_R40_NUM_USB; i++) {
> +            g_autofree char *bus = g_strdup_printf("usb-bus.%d", i);
> +
> +            object_property_set_bool(OBJECT(&s->ehci[i]), "companion-enable",
> +                                     true, &error_fatal);
> +            sysbus_realize(SYS_BUS_DEVICE(&s->ehci[i]), &error_fatal);
> +            sysbus_mmio_map(SYS_BUS_DEVICE(&s->ehci[i]), 0,
> +                            allwinner_r40_memmap[i ? AW_R40_DEV_EHCI2
> +                                                   : AW_R40_DEV_EHCI1]);
> +            sysbus_connect_irq(SYS_BUS_DEVICE(&s->ehci[i]), 0,
> +                               qdev_get_gpio_in(DEVICE(&s->gic),
> +                                                i ? AW_R40_GIC_SPI_EHCI2
> +                                                  : AW_R40_GIC_SPI_EHCI1));
> +
> +            object_property_set_str(OBJECT(&s->ohci[i]), "masterbus", bus,
> +                                    &error_fatal);
> +            sysbus_realize(SYS_BUS_DEVICE(&s->ohci[i]), &error_fatal);
> +            sysbus_mmio_map(SYS_BUS_DEVICE(&s->ohci[i]), 0,
> +                            allwinner_r40_memmap[i ? AW_R40_DEV_OHCI2
> +                                                   : AW_R40_DEV_OHCI1]);
> +            sysbus_connect_irq(SYS_BUS_DEVICE(&s->ohci[i]), 0,
> +                               qdev_get_gpio_in(DEVICE(&s->gic),
> +                                                i ? AW_R40_GIC_SPI_OHCI2
> +                                                  : AW_R40_GIC_SPI_OHCI1));
> +        }
> +    }
> +
>       /* SD/MMC */
>       for (int i = 0; i < AW_R40_NUM_MMCS; i++) {
>           qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->gic),
> @@ -498,6 +549,21 @@ static void allwinner_r40_realize(DeviceState *dev, Error 
> **errp)
>                                       r40_unimplemented[i].base,
>                                       r40_unimplemented[i].size);
>       }
> +    if (!machine_usb(current_machine)) {
> +        /* unimplemented if USB is not enabled */
> +        create_unimplemented_device("usb-ehci1",
> +                                    allwinner_r40_memmap[AW_R40_DEV_EHCI1],
> +                                    0x800);
> +        create_unimplemented_device("usb-ehci2",
> +                                    allwinner_r40_memmap[AW_R40_DEV_EHCI2],
> +                                    0x800);
> +        create_unimplemented_device("usb-ohci1",
> +                                    allwinner_r40_memmap[AW_R40_DEV_OHCI1],
> +                                    0x800);
> +        create_unimplemented_device("usb-ohci2",
> +                                    allwinner_r40_memmap[AW_R40_DEV_OHCI2],
> +                                    0x800);
> +    }
>   }