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;
>>
>