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

[Philippe Mathieu-Daudé]

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.