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.

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(-)

diff --git a/docs/system/arm/bananapi_m2u.rst b/docs/system/arm/bananapi_m2u.rst
index b09ba5c548..e77c425e2c 100644
--- a/docs/system/arm/bananapi_m2u.rst
+++ b/docs/system/arm/bananapi_m2u.rst
@@ -23,6 +23,7 @@ The Banana Pi M2U machine supports the following devices:
  * GMAC ethernet
  * Clock Control Unit
  * TWI (I2C)
+ * USB 2.0
 
 Limitations
 """""""""""
@@ -33,7 +34,6 @@ Currently, Banana Pi M2U does *not* support the following features:
 - Audio output
 - Hardware Watchdog
 - Real Time Clock
-- USB 2.0 interfaces
 
 Also see the 'unimplemented' array in the Allwinner R40 SoC module
 for a complete list of unimplemented I/O devices: ``./hw/arm/allwinner-r40.c``
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 39d255425b..6b508780d3 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -415,6 +415,8 @@ config ALLWINNER_R40
     select ARM_TIMER
     select ARM_GIC
     select UNIMP
+    select USB_OHCI
+    select USB_EHCI_SYSBUS
     select SD
 
 config RASPI
diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
index a0d367c60d..f42b0fa0ce 100644
--- a/hw/arm/allwinner-r40.c
+++ b/hw/arm/allwinner-r40.c
@@ -23,6 +23,7 @@
 #include "qemu/bswap.h"
 #include "qemu/module.h"
 #include "qemu/units.h"
+#include "hw/boards.h"
 #include "hw/qdev-core.h"
 #include "hw/sysbus.h"
 #include "hw/char/serial.h"
@@ -45,6 +46,10 @@ const hwaddr allwinner_r40_memmap[] = {
     [AW_R40_DEV_MMC1]       = 0x01c10000,
     [AW_R40_DEV_MMC2]       = 0x01c11000,
     [AW_R40_DEV_MMC3]       = 0x01c12000,
+    [AW_R40_DEV_EHCI1]      = 0x01c19000,
+    [AW_R40_DEV_OHCI1]      = 0x01c19400,
+    [AW_R40_DEV_EHCI2]      = 0x01c1c000,
+    [AW_R40_DEV_OHCI2]      = 0x01c1c400,
     [AW_R40_DEV_CCU]        = 0x01c20000,
     [AW_R40_DEV_PIT]        = 0x01c20c00,
     [AW_R40_DEV_UART0]      = 0x01c28000,
@@ -89,9 +94,9 @@ static struct AwR40Unimplemented r40_unimplemented[] = {
     { "crypto",     0x01c15000, 4 * KiB },
     { "spi2",       0x01c17000, 4 * KiB },
     { "sata",       0x01c18000, 4 * KiB },
-    { "usb1-host",  0x01c19000, 4 * KiB },
+    { "usb1-phy",   0x01c19800, 2 * KiB },
     { "sid",        0x01c1b000, 4 * KiB },
-    { "usb2-host",  0x01c1c000, 4 * KiB },
+    { "usb2-phy",   0x01c1c800, 2 * KiB },
     { "cs1",        0x01c1d000, 4 * KiB },
     { "spi3",       0x01c1f000, 4 * KiB },
     { "rtc",        0x01c20400, 1 * KiB },
@@ -181,6 +186,10 @@ enum {
     AW_R40_GIC_SPI_MMC2      = 34,
     AW_R40_GIC_SPI_MMC3      = 35,
     AW_R40_GIC_SPI_EMAC      = 55,
+    AW_R40_GIC_SPI_OHCI1     = 64,
+    AW_R40_GIC_SPI_OHCI2     = 65,
+    AW_R40_GIC_SPI_EHCI1     = 76,
+    AW_R40_GIC_SPI_EHCI2     = 78,
     AW_R40_GIC_SPI_GMAC      = 85,
 };
 
@@ -276,6 +285,17 @@ static void allwinner_r40_init(Object *obj)
                                 TYPE_AW_SDHOST_SUN50I_A64);
     }
 
+    if (machine_usb(current_machine)) {
+        int i;
+
+        for (i = 0; i < AW_R40_NUM_USB; i++) {
+            object_initialize_child(obj, "ehci[*]", &s->ehci[i],
+                                    TYPE_PLATFORM_EHCI);
+            object_initialize_child(obj, "ohci[*]", &s->ohci[i],
+                                    TYPE_SYSBUS_OHCI);
+        }
+    }
+
     object_initialize_child(obj, "twi0", &s->i2c0, TYPE_AW_I2C_SUN6I);
 
     object_initialize_child(obj, "emac", &s->emac, TYPE_AW_EMAC);
@@ -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);
+    }
 }
 
 static void allwinner_r40_class_init(ObjectClass *oc, void *data)
diff --git a/include/hw/arm/allwinner-r40.h b/include/hw/arm/allwinner-r40.h
index 6e1ac9d4c1..ae82822d42 100644
--- a/include/hw/arm/allwinner-r40.h
+++ b/include/hw/arm/allwinner-r40.h
@@ -30,6 +30,8 @@
 #include "hw/i2c/allwinner-i2c.h"
 #include "hw/net/allwinner_emac.h"
 #include "hw/net/allwinner-sun8i-emac.h"
+#include "hw/usb/hcd-ohci.h"
+#include "hw/usb/hcd-ehci.h"
 #include "target/arm/cpu.h"
 #include "sysemu/block-backend.h"
 
@@ -44,6 +46,10 @@ enum {
     AW_R40_DEV_MMC1,
     AW_R40_DEV_MMC2,
     AW_R40_DEV_MMC3,
+    AW_R40_DEV_EHCI1,
+    AW_R40_DEV_OHCI1,
+    AW_R40_DEV_EHCI2,
+    AW_R40_DEV_OHCI2,
     AW_R40_DEV_CCU,
     AW_R40_DEV_PIT,
     AW_R40_DEV_UART0,
@@ -88,6 +94,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AwR40State, AW_R40)
  * which are currently emulated by the R40 SoC code.
  */
 #define AW_R40_NUM_MMCS         4
+#define AW_R40_NUM_USB          2
 #define AW_R40_NUM_UARTS        8
 
 struct AwR40State {
@@ -106,6 +113,8 @@ struct AwR40State {
     AwSRAMCState sramc;
     AwA10PITState timer;
     AwSdHostState mmc[AW_R40_NUM_MMCS];
+    EHCISysBusState ehci[AW_R40_NUM_USB];
+    OHCISysBusState ohci[AW_R40_NUM_USB];
     AwR40ClockCtlState ccu;
     AwR40DramCtlState dramc;
     AWI2CState i2c0;
-- 
2.39.2

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);
> +    }
>   }

On 1/15/24 03:02, Philippe Mathieu-Daudé wrote:
> 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.
> 

Me not either. I copied the code from aw_a10_init(), trying to use the
same pattern. I am perfectly fine with making it unconditional, but then
I would argue that it should be unconditional for Allwinner A10 as well
(not that I really care much, just for consistency).

The "-usb" option says "enable on-board USB host controller (if not
enabled by default)". Unfortunately, that doesn't tell me much,
and most specifically it doesn't tell me how to enable it by default.
One option I can think of would be to enable it on the machine level,
i.e., from bananapi_m2u.c, but then, again, I don't see if/how
that is done for other boards. Any suggestions ?

Of course, I could discuss this with the person who implemented this
code for A10, but it turns out that was me, for no good reason than
that I tried to follow the pattern I had seen elsewhere without really
understanding what I was doing.

So should I drop the conditional from H40 and send a separate patch
to drop it from the A10 code as well, following your line of argument ?
Or drop it and leave A10 alone ?

Thanks,
Guenter

On Mon, Jan 15, 2024 at 08:12:29AM -0800, Guenter Roeck wrote:
> On 1/15/24 03:02, Philippe Mathieu-Daudé wrote:
> > On 13/1/24 20:16, Guenter Roeck wrote:
> > > 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.
> 
> Me not either. I copied the code from aw_a10_init(), trying to use the
> same pattern. I am perfectly fine with making it unconditional, but then
> I would argue that it should be unconditional for Allwinner A10 as well
> (not that I really care much, just for consistency).
> 
> The "-usb" option says "enable on-board USB host controller (if not
> enabled by default)". Unfortunately, that doesn't tell me much,
> and most specifically it doesn't tell me how to enable it by default.
> One option I can think of would be to enable it on the machine level,
> i.e., from bananapi_m2u.c, but then, again, I don't see if/how
> that is done for other boards. Any suggestions ?

The -usb switch is there as long as I remember (which goes back to qemu
0.1x days).  I suspect it was introduced in the early usb emulation
days, where usb emulation was more fragile and turning it on by default
didn't look like a good plan.

usb emulation in modern qemu should be stable enough that enabling it by
default should not be much of a problem.  ohci/uhci/ehci emulation is
somewhat expensive due to polling being wired into the hardware design,
but as long as there are no usb devices connected this should not be
much of a concern either as the guest drivers usually put the usb host
adapter to sleep in that case (which saves power on physical hardware
and stops the polling timer in qemu).

So, turning on usb support by default makes sense to me when emulating
boards, where guests expect the usb controllers being present at
specific MMIO addresses, so mapping UnimpDevice is not needed.

In case guests detect hardware via generated device tree / generated
ACPI tables / pci bus scan (i.e. arm virt + microvm + pc + q35) it IMHO
makes sense to keep current behavior.

take care,
  Gerd

Am 16. Januar 2024 10:13:37 UTC schrieb Gerd Hoffmann <kraxel@redhat.com>:
>On Mon, Jan 15, 2024 at 08:12:29AM -0800, Guenter Roeck wrote:
>> On 1/15/24 03:02, Philippe Mathieu-Daudé wrote:
>> > On 13/1/24 20:16, Guenter Roeck wrote:
>> > > 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.
>> 
>> Me not either. I copied the code from aw_a10_init(), trying to use the
>> same pattern. I am perfectly fine with making it unconditional, but then
>> I would argue that it should be unconditional for Allwinner A10 as well
>> (not that I really care much, just for consistency).
>> 
>> The "-usb" option says "enable on-board USB host controller (if not
>> enabled by default)". Unfortunately, that doesn't tell me much,
>> and most specifically it doesn't tell me how to enable it by default.
>> One option I can think of would be to enable it on the machine level,
>> i.e., from bananapi_m2u.c, but then, again, I don't see if/how
>> that is done for other boards. Any suggestions ?
>
>The -usb switch is there as long as I remember (which goes back to qemu
>0.1x days).  I suspect it was introduced in the early usb emulation
>days, where usb emulation was more fragile and turning it on by default
>didn't look like a good plan.
>
>usb emulation in modern qemu should be stable enough that enabling it by
>default should not be much of a problem.  ohci/uhci/ehci emulation is
>somewhat expensive due to polling being wired into the hardware design,
>but as long as there are no usb devices connected this should not be
>much of a concern either as the guest drivers usually put the usb host
>adapter to sleep in that case (which saves power on physical hardware
>and stops the polling timer in qemu).
>
>So, turning on usb support by default makes sense to me when emulating
>boards, where guests expect the usb controllers being present at
>specific MMIO addresses, so mapping UnimpDevice is not needed.

Good idea, this might make upstreaming my VIA-south-bridges-in-the-pc-machine work simpler.

>
>In case guests detect hardware via generated device tree / generated
>ACPI tables / pci bus scan (i.e. arm virt + microvm + pc + q35) it IMHO
>makes sense to keep current behavior.

I remember similar discussions with Phil during PIIX consolidation where the motivation was to make the code simpler by removing options. That is, if a piece of silicon contains multiple functions, the QEMU model should always instantiate all (available) functions rather than providing options for cherry-picking.

Especially in highly integrated devices (such as south bridges) there might be interconnections between functions that are hard to handle when some are optional. For example, the ACPI function in the VIA south bridges detect activity from all other functions. In both PIIX and VIA, the USB function can be (de)activated by software from the ISA function (not implemented yet).

The latter would allow the current behavior of the pc machine(s) to be implemented while avoiding the cherry picking problem: The BIOS could switch the USB function on or off depending on fw_cfg (please correct me if I'm wrong). I think this would't even need compat machinery. Of course, support would need to be added in SeaBIOS and -- I guess -- Tianocore. I'd like to look into that but, as usual, no guarantees when this will arrive.

Best regards,
Bernhard

>
>take care,
>  Gerd
>
>

> >In case guests detect hardware via generated device tree / generated
> >ACPI tables / pci bus scan (i.e. arm virt + microvm + pc + q35) it IMHO
> >makes sense to keep current behavior.
> 
> I remember similar discussions with Phil during PIIX consolidation
> where the motivation was to make the code simpler by removing options.
> That is, if a piece of silicon contains multiple functions, the QEMU
> model should always instantiate all (available) functions rather than
> providing options for cherry-picking.
> 
> Especially in highly integrated devices (such as south bridges) there
> might be interconnections between functions that are hard to handle
> when some are optional. For example, the ACPI function in the VIA
> south bridges detect activity from all other functions. In both PIIX
> and VIA, the USB function can be (de)activated by software from the
> ISA function (not implemented yet).
> 
> The latter would allow the current behavior of the pc machine(s) to be
> implemented while avoiding the cherry picking problem: The BIOS could
> switch the USB function on or off depending on fw_cfg (please correct
> me if I'm wrong). I think this would't even need compat machinery. Of
> course, support would need to be added in SeaBIOS and -- I guess --
> Tianocore. I'd like to look into that but, as usual, no guarantees
> when this will arrive.

Well, our chipsets are quite old.  piix has uhci, q35 has uhci+ehci.
Modern usb is xhci, which also is more virtualization friendly hardware
design.  Also qemu usb configuration is much simpler in case there is
only one usb bus (xhci) instead of two (xhci + chipset) because you
don't have to explicitly assign usb devices to a specific usb bus if
there is only one.  So I don't like the idea to have the chipset usb bus
present by default.

We have a similar problem in the LPC bridges.  They likewise have bits
in the pci config space to enable/disable ISA devices such as floppy,
serial port, parallel port.  These bits are implemented, but read-only.
So it is possible to read them to figure whenever a given device is
present, but it is not possible to set them to enable/disable devices.
Before we switched the ACPI DSDT to be 100% generated we actually had
some AML functions reading those bits to figure whenever these ISA
devices are present or not.

I'd suggest to follow that design pattern for the config bits in the
south bridge ACPI function.

take care,
  Gerd

On 15/1/24 17:12, Guenter Roeck wrote:
> On 1/15/24 03:02, Philippe Mathieu-Daudé wrote:
>> 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.
>>
> 
> Me not either. I copied the code from aw_a10_init(), trying to use the
> same pattern. I am perfectly fine with making it unconditional, but then
> I would argue that it should be unconditional for Allwinner A10 as well
> (not that I really care much, just for consistency).

Certainly, but I'd rather have a global pattern cleanup, not just
Allwinner based machines. Looking at the repository:

$ git grep -w machine_usb hw/
hw/arm/allwinner-a10.c:82:    if (machine_usb(current_machine)) {
hw/arm/allwinner-a10.c:168:    if (machine_usb(current_machine)) {
hw/arm/nseries.c:1356:    if (machine_usb(machine)) {
hw/arm/realview.c:288:        if (machine_usb(machine)) {
hw/arm/realview.c-289-            pci_create_simple(pci_bus, -1, 
"pci-ohci");
hw/arm/versatilepb.c:276:    if (machine_usb(machine)) {
hw/arm/versatilepb.c-277-        pci_create_simple(pci_bus, -1, "pci-ohci");
hw/core/machine.c:1175:bool machine_usb(MachineState *machine)
hw/i386/acpi-microvm.c:88:    if (machine_usb(MACHINE(mms))) {
hw/i386/acpi-microvm.c-89-        xhci_sysbus_build_aml(scope, 
MICROVM_XHCI_BASE, MICROVM_XHCI_IRQ);
hw/i386/microvm.c:218:    if (x86_machine_is_acpi_enabled(x86ms) && 
machine_usb(MACHINE(mms))) {
hw/i386/microvm.c-225-        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 
MICROVM_XHCI_BASE);
hw/i386/pc_piix.c:267: 
machine_usb(machine), &error_abort);
hw/i386/pc_q35.c:321:    if (machine_usb(machine)) {
hw/i386/pc_q35.c-323-        ehci_create_ich9_with_companions(host_bus, 
0x1d);
hw/ppc/mac_oldworld.c:300:    if (machine_usb(machine)) {
hw/ppc/mac_oldworld.c-301-        pci_create_simple(pci_bus, -1, 
"pci-ohci");

I'd classify that as "USB controller over MMIO / over some bus (PCI)".

The "plug a PCI-to-USB card by default" seems a valid use case (except
for Q35 which is a Frankenstein case, ICH9 chipset is like ARM SoC,
USB bus is always there).

IMHO all the MMIO uses should be corrected.

> The "-usb" option says "enable on-board USB host controller (if not
> enabled by default)". Unfortunately, that doesn't tell me much,
> and most specifically it doesn't tell me how to enable it by default.
> One option I can think of would be to enable it on the machine level,
> i.e., from bananapi_m2u.c, but then, again, I don't see if/how
> that is done for other boards. Any suggestions ?
> 
> Of course, I could discuss this with the person who implemented this
> code for A10, but it turns out that was me, for no good reason than
> that I tried to follow the pattern I had seen elsewhere without really
> understanding what I was doing.
> 
> So should I drop the conditional from H40 and send a separate patch
> to drop it from the A10 code as well, following your line of argument ?
> Or drop it and leave A10 alone ?

Well your patch isn't invalid, so:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

I'm just worried we are taking a bad path here and would rather avoid
it if possible...

Regards,

Phil.

On 1/15/24 08:30, Philippe Mathieu-Daudé wrote:
> On 15/1/24 17:12, Guenter Roeck wrote:
>> On 1/15/24 03:02, Philippe Mathieu-Daudé wrote:
>>> 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.
>>>
>>
>> Me not either. I copied the code from aw_a10_init(), trying to use the
>> same pattern. I am perfectly fine with making it unconditional, but then
>> I would argue that it should be unconditional for Allwinner A10 as well
>> (not that I really care much, just for consistency).
> 
> Certainly, but I'd rather have a global pattern cleanup, not just
> Allwinner based machines. Looking at the repository:
> 
> $ git grep -w machine_usb hw/
> hw/arm/allwinner-a10.c:82:    if (machine_usb(current_machine)) {
> hw/arm/allwinner-a10.c:168:    if (machine_usb(current_machine)) {
> hw/arm/nseries.c:1356:    if (machine_usb(machine)) {
> hw/arm/realview.c:288:        if (machine_usb(machine)) {
> hw/arm/realview.c-289-            pci_create_simple(pci_bus, -1, "pci-ohci");
> hw/arm/versatilepb.c:276:    if (machine_usb(machine)) {
> hw/arm/versatilepb.c-277-        pci_create_simple(pci_bus, -1, "pci-ohci");
> hw/core/machine.c:1175:bool machine_usb(MachineState *machine)
> hw/i386/acpi-microvm.c:88:    if (machine_usb(MACHINE(mms))) {
> hw/i386/acpi-microvm.c-89-        xhci_sysbus_build_aml(scope, MICROVM_XHCI_BASE, MICROVM_XHCI_IRQ);
> hw/i386/microvm.c:218:    if (x86_machine_is_acpi_enabled(x86ms) && machine_usb(MACHINE(mms))) {
> hw/i386/microvm.c-225-        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, MICROVM_XHCI_BASE);
> hw/i386/pc_piix.c:267: machine_usb(machine), &error_abort);
> hw/i386/pc_q35.c:321:    if (machine_usb(machine)) {
> hw/i386/pc_q35.c-323-        ehci_create_ich9_with_companions(host_bus, 0x1d);
> hw/ppc/mac_oldworld.c:300:    if (machine_usb(machine)) {
> hw/ppc/mac_oldworld.c-301-        pci_create_simple(pci_bus, -1, "pci-ohci");
> 
> I'd classify that as "USB controller over MMIO / over some bus (PCI)".
> 
> The "plug a PCI-to-USB card by default" seems a valid use case (except
> for Q35 which is a Frankenstein case, ICH9 chipset is like ARM SoC,
> USB bus is always there).
> 
> IMHO all the MMIO uses should be corrected.
> 
>> The "-usb" option says "enable on-board USB host controller (if not
>> enabled by default)". Unfortunately, that doesn't tell me much,
>> and most specifically it doesn't tell me how to enable it by default.
>> One option I can think of would be to enable it on the machine level,
>> i.e., from bananapi_m2u.c, but then, again, I don't see if/how
>> that is done for other boards. Any suggestions ?
>>
>> Of course, I could discuss this with the person who implemented this
>> code for A10, but it turns out that was me, for no good reason than
>> that I tried to follow the pattern I had seen elsewhere without really
>> understanding what I was doing.
>>
>> So should I drop the conditional from H40 and send a separate patch
>> to drop it from the A10 code as well, following your line of argument ?
>> Or drop it and leave A10 alone ?
> 
> Well your patch isn't invalid, so:
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

I'll send v2 anyway, especially taking your other patch series into account.

Thanks,
Guenter