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

[Guenter Roeck <linux@roeck-us.net>]

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