Re: [PATCH 05/42] hw/isa/piix3: Create USB controller in host device

From: Bernhard Beschow <shentey@gmail.com>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>, qemu-devel@nongnu.org
Cc: "Hervé Poussineau" <hpoussin@reactos.org>,
	"Aurelien Jarno" <aurelien@aurel32.net>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Eduardo Habkost" <eduardo@habkost.net>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"Jiaxun Yang" <jiaxun.yang@flygoat.com>,
	"Ani Sinha" <ani@anisinha.ca>,
	"Igor Mammedov" <imammedo@redhat.com>,
	"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
	"Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [PATCH 05/42] hw/isa/piix3: Create USB controller in host device
Date: Sun, 18 Sep 2022 21:46:13 +0000	[thread overview]
Message-ID: <9807B008-53A1-443B-80AC-BAD060AB8D7E@gmail.com> (raw)
In-Reply-To: <803efdb8-e85a-7322-1f49-0f81c486275f@amsat.org>

Am 1. September 2022 19:13:22 UTC schrieb "Philippe Mathieu-Daudé" <f4bug@amsat.org>:
>On 1/9/22 18:25, Bernhard Beschow wrote:
>> The USB controller is an integral part of PIIX3 (function 2). So create
>> it as part of the south bridge.
>> 
>> Note that the USB function is optional in QEMU. This is why it gets
>> object_initialize_child()'ed in realize rather than in instance_init.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/i386/pc_piix.c             |  6 ++----
>>   hw/isa/Kconfig                |  1 +
>>   hw/isa/piix3.c                | 17 +++++++++++++++++
>>   include/hw/southbridge/piix.h |  4 ++++
>>   4 files changed, 24 insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index b08d946992..76ac8b2035 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -219,6 +219,8 @@ static void pc_init1(MachineState *machine,
>>           pcms->bus = pci_bus;
>>             pci_dev = pci_new_multifunction(-1, true, type);
>> +        object_property_set_bool(OBJECT(pci_dev), "has-usb",
>> +                                 machine_usb(machine), &error_abort);
>>           pci_realize_and_unref(pci_dev, pci_bus, &error_fatal);
>>           piix3 = PIIX3_PCI_DEVICE(pci_dev);
>>           piix3->pic = x86ms->gsi;
>> @@ -297,10 +299,6 @@ static void pc_init1(MachineState *machine,
>>       }
>>   #endif
>>   -    if (pcmc->pci_enabled && machine_usb(machine)) {
>> -        pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
>> -    }
>> -
>>       if (pcmc->pci_enabled && x86_machine_is_acpi_enabled(X86_MACHINE(pcms))) {
>>           PCIDevice *piix4_pm;
>>   diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
>> index 6e8f9cac54..f02eca3c3e 100644
>> --- a/hw/isa/Kconfig
>> +++ b/hw/isa/Kconfig
>> @@ -36,6 +36,7 @@ config PIIX3
>>       select I8257
>>       select ISA_BUS
>>       select MC146818RTC
>> +    select USB_UHCI
>>     config PIIX4
>>       bool
>> diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
>> index 96ab7107e2..27052a5546 100644
>> --- a/hw/isa/piix3.c
>> +++ b/hw/isa/piix3.c
>> @@ -297,6 +297,7 @@ static const MemoryRegionOps rcr_ops = {
>>   static void pci_piix3_realize(PCIDevice *dev, Error **errp)
>>   {
>>       PIIX3State *d = PIIX3_PCI_DEVICE(dev);
>> +    PCIBus *pci_bus = pci_get_bus(dev);
>>       ISABus *isa_bus;
>>         isa_bus = isa_bus_new(DEVICE(d), get_system_memory(),
>> @@ -319,6 +320,16 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
>>       if (!qdev_realize(DEVICE(&d->rtc), BUS(isa_bus), errp)) {
>>           return;
>>       }
>> +
>> +    /* USB */
>> +    if (d->has_usb) {
>> +        object_initialize_child(OBJECT(dev), "uhci", &d->uhci,
>> +                                "piix3-usb-uhci");
>> +        qdev_prop_set_int32(DEVICE(&d->uhci), "addr", dev->devfn + 2);
>> +        if (!qdev_realize(DEVICE(&d->uhci), BUS(pci_bus), errp)) {
>> +            return;
>> +        }
>> +    }
>>   }
>>     static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
>> @@ -341,6 +352,11 @@ static void pci_piix3_init(Object *obj)
>>       object_initialize_child(obj, "rtc", &d->rtc, TYPE_MC146818_RTC);
>>   }
>>   +static Property pci_piix3_props[] = {
>> +    DEFINE_PROP_BOOL("has-usb", PIIX3State, has_usb, true),
>
>Maybe s/has-usb/usb-enabled/?

I consisered that. I chose has-foo for now since I took inspiration from the board. Also, "enabled" may refer to a device being present but its enable bit is cleared in PCI configuration space, c.f. vt82c686. Let me know if you still suggest a change here, in which case I'll consult you for naming suggestions once I consolidate piix and vt82c686 ;)

Best regards,
Bernhard
>
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>   static void pci_piix3_class_init(ObjectClass *klass, void *data)
>>   {
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -359,6 +375,7 @@ static void pci_piix3_class_init(ObjectClass *klass, void *data)
>>        * pc_piix.c's pc_init1()
>>        */
>>       dc->user_creatable = false;
>> +    device_class_set_props(dc, pci_piix3_props);
>>       adevc->build_dev_aml = build_pci_isa_aml;
>>   }
>>   diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
>> index b1fa08dd2b..5367917182 100644
>> --- a/include/hw/southbridge/piix.h
>> +++ b/include/hw/southbridge/piix.h
>> @@ -15,6 +15,7 @@
>>   #include "hw/pci/pci.h"
>>   #include "qom/object.h"
>>   #include "hw/rtc/mc146818rtc.h"
>> +#include "hw/usb/hcd-uhci.h"
>>     /* PIRQRC[A:D]: PIRQx Route Control Registers */
>>   #define PIIX_PIRQCA 0x60
>> @@ -54,12 +55,15 @@ struct PIIXState {
>>       int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
>>         RTCState rtc;
>> +    UHCIState uhci;
>>         /* Reset Control Register contents */
>>       uint8_t rcr;
>>         /* IO memory region for Reset Control Register (PIIX_RCR_IOPORT) */
>>       MemoryRegion rcr_mem;
>> +
>> +    bool has_usb;
>>   };
>>   typedef struct PIIXState PIIX3State;
>>   
>