Message ID | 20231005115159.81202-1-shentey@gmail.com |
---|---|
State | New |
Headers | show |
Series | [v3] hw/isa/vt82c686: Respect SCI interrupt assignment | expand |
On Thu, 5 Oct 2023, Bernhard Beschow wrote: > According to the datasheet, SCI interrupts of the power management function > aren't routed through the PCI pins but rather directly to the integrated PIC. > The routing is configurable through the ACPI interrupt select register at offset > 0x42 in the PCI configuration space of the power management function. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > --- > > v3: > * Rename SCI irq attribute to sci_irq (Zoltan) > * Fix confusion about location of ACPI interrupt select register (Zoltan) > * Model SCI as named GPIO (Bernhard) > * Perform upcast via macro rather than sub structure selection (Bernhard) > > v2: > * Introduce named constants for the ACPI interrupt select register at offset > 0x42 (Phil) > --- > hw/isa/vt82c686.c | 48 +++++++++++++++++++++++++++++++++++------------ > 1 file changed, 36 insertions(+), 12 deletions(-) > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index 57bdfb4e78..aeb9434a46 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -40,12 +40,17 @@ > #define TYPE_VIA_PM "via-pm" > OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM) > > +#define VIA_PM_SCI_SELECT_OFS 0x42 > +#define VIA_PM_SCI_SELECT_MASK 0xf > + > struct ViaPMState { > PCIDevice dev; > MemoryRegion io; > ACPIREGS ar; > APMState apm; > PMSMBus smb; > + > + qemu_irq sci_irq; > }; > > static void pm_io_space_update(ViaPMState *s) > @@ -148,18 +153,7 @@ static void pm_update_sci(ViaPMState *s) > ACPI_BITMASK_POWER_BUTTON_ENABLE | > ACPI_BITMASK_GLOBAL_LOCK_ENABLE | > ACPI_BITMASK_TIMER_ENABLE)) != 0); > - if (pci_get_byte(s->dev.config + PCI_INTERRUPT_PIN)) { > - /* > - * FIXME: > - * Fix device model that realizes this PM device and remove > - * this work around. > - * The device model should wire SCI and setup > - * PCI_INTERRUPT_PIN properly. > - * If PIN# = 0(interrupt pin isn't used), don't raise SCI as > - * work around. > - */ > - pci_set_irq(&s->dev, sci_level); > - } > + qemu_set_irq(s->sci_irq, sci_level); I still think this it more complex that it should be and what's in via_isa_set_pm_irq() below should be here instead and drop all the named gpio wizardry that's just unneeded complication here. Regards. BALATON Zoltan > /* schedule a timer interruption if needed */ > acpi_pm_tmr_update(&s->ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) && > !(pmsts & ACPI_BITMASK_TIMER_STATUS)); > @@ -213,6 +207,13 @@ static void via_pm_realize(PCIDevice *dev, Error **errp) > acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2, false); > } > > +static void via_pm_init(Object *obj) > +{ > + ViaPMState *s = VIA_PM(obj); > + > + qdev_init_gpio_out_named(DEVICE(obj), &s->sci_irq, "sci", 1); > +} > + > typedef struct via_pm_init_info { > uint16_t device_id; > } ViaPMInitInfo; > @@ -238,6 +239,7 @@ static void via_pm_class_init(ObjectClass *klass, void *data) > static const TypeInfo via_pm_info = { > .name = TYPE_VIA_PM, > .parent = TYPE_PCI_DEVICE, > + .instance_init = via_pm_init, > .instance_size = sizeof(ViaPMState), > .abstract = true, > .interfaces = (InterfaceInfo[]) { > @@ -568,9 +570,27 @@ static const VMStateDescription vmstate_via = { > } > }; > > +static void via_isa_set_pm_irq(void *opaque, int n, int level) > +{ > + ViaISAState *s = opaque; > + PCIDevice *pci_dev = PCI_DEVICE(&s->pm); > + uint8_t irq = pci_get_byte(pci_dev->config + VIA_PM_SCI_SELECT_OFS) > + & VIA_PM_SCI_SELECT_MASK; > + > + if (irq == 2) { > + qemu_log_mask(LOG_GUEST_ERROR, "IRQ 2 for PM controller is reserved"); > + return; > + } > + > + if (irq != 0) { > + qemu_set_irq(s->isa_irqs_in[irq], level); > + } > +} > + > static void via_isa_init(Object *obj) > { > ViaISAState *s = VIA_ISA(obj); > + DeviceState *dev = DEVICE(s); > > object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC); > object_initialize_child(obj, "ide", &s->ide, TYPE_VIA_IDE); > @@ -578,6 +598,8 @@ static void via_isa_init(Object *obj) > object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI); > object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97); > object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97); > + > + qdev_init_gpio_in_named(dev, via_isa_set_pm_irq, "sci", 1); > } > > static const TypeInfo via_isa_info = { > @@ -704,6 +726,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp) > if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) { > return; > } > + qdev_connect_gpio_out_named(DEVICE(&s->pm), "sci", 0, > + qdev_get_gpio_in_named(DEVICE(d), "sci", 0)); > > /* Function 5: AC97 Audio */ > qdev_prop_set_int32(DEVICE(&s->ac97), "addr", d->devfn + 5); >
On 5/10/23 14:45, BALATON Zoltan wrote: > On Thu, 5 Oct 2023, Bernhard Beschow wrote: >> According to the datasheet, SCI interrupts of the power management >> function >> aren't routed through the PCI pins but rather directly to the >> integrated PIC. >> The routing is configurable through the ACPI interrupt select register >> at offset >> 0x42 in the PCI configuration space of the power management function. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> >> --- >> >> v3: >> * Rename SCI irq attribute to sci_irq (Zoltan) >> * Fix confusion about location of ACPI interrupt select register (Zoltan) >> * Model SCI as named GPIO (Bernhard) >> * Perform upcast via macro rather than sub structure selection (Bernhard) >> >> v2: >> * Introduce named constants for the ACPI interrupt select register at >> offset >> 0x42 (Phil) >> --- >> hw/isa/vt82c686.c | 48 +++++++++++++++++++++++++++++++++++------------ >> 1 file changed, 36 insertions(+), 12 deletions(-) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 57bdfb4e78..aeb9434a46 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -40,12 +40,17 @@ >> #define TYPE_VIA_PM "via-pm" >> OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM) >> >> +#define VIA_PM_SCI_SELECT_OFS 0x42 >> +#define VIA_PM_SCI_SELECT_MASK 0xf >> + >> struct ViaPMState { >> PCIDevice dev; >> MemoryRegion io; >> ACPIREGS ar; >> APMState apm; >> PMSMBus smb; >> + >> + qemu_irq sci_irq; >> }; >> >> static void pm_io_space_update(ViaPMState *s) >> @@ -148,18 +153,7 @@ static void pm_update_sci(ViaPMState *s) >> ACPI_BITMASK_POWER_BUTTON_ENABLE | >> ACPI_BITMASK_GLOBAL_LOCK_ENABLE | >> ACPI_BITMASK_TIMER_ENABLE)) != 0); >> - if (pci_get_byte(s->dev.config + PCI_INTERRUPT_PIN)) { >> - /* >> - * FIXME: >> - * Fix device model that realizes this PM device and remove >> - * this work around. >> - * The device model should wire SCI and setup >> - * PCI_INTERRUPT_PIN properly. >> - * If PIN# = 0(interrupt pin isn't used), don't raise SCI as >> - * work around. >> - */ >> - pci_set_irq(&s->dev, sci_level); >> - } >> + qemu_set_irq(s->sci_irq, sci_level); > > I still think this it more complex that it should be and what's in > via_isa_set_pm_irq() below should be here instead and drop all the named > gpio wizardry that's just unneeded complication here. Zoltan, I'm not sure I get what you mean. Do you mind respining a v4 of Bernhard's patch? Regards, Phil.
Am 5. Oktober 2023 12:45:02 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Thu, 5 Oct 2023, Bernhard Beschow wrote: >> According to the datasheet, SCI interrupts of the power management function >> aren't routed through the PCI pins but rather directly to the integrated PIC. >> The routing is configurable through the ACPI interrupt select register at offset >> 0x42 in the PCI configuration space of the power management function. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> >> --- >> >> v3: >> * Rename SCI irq attribute to sci_irq (Zoltan) >> * Fix confusion about location of ACPI interrupt select register (Zoltan) >> * Model SCI as named GPIO (Bernhard) >> * Perform upcast via macro rather than sub structure selection (Bernhard) >> >> v2: >> * Introduce named constants for the ACPI interrupt select register at offset >> 0x42 (Phil) >> --- >> hw/isa/vt82c686.c | 48 +++++++++++++++++++++++++++++++++++------------ >> 1 file changed, 36 insertions(+), 12 deletions(-) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 57bdfb4e78..aeb9434a46 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -40,12 +40,17 @@ >> #define TYPE_VIA_PM "via-pm" >> OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM) >> >> +#define VIA_PM_SCI_SELECT_OFS 0x42 >> +#define VIA_PM_SCI_SELECT_MASK 0xf >> + >> struct ViaPMState { >> PCIDevice dev; >> MemoryRegion io; >> ACPIREGS ar; >> APMState apm; >> PMSMBus smb; >> + >> + qemu_irq sci_irq; >> }; >> >> static void pm_io_space_update(ViaPMState *s) >> @@ -148,18 +153,7 @@ static void pm_update_sci(ViaPMState *s) >> ACPI_BITMASK_POWER_BUTTON_ENABLE | >> ACPI_BITMASK_GLOBAL_LOCK_ENABLE | >> ACPI_BITMASK_TIMER_ENABLE)) != 0); >> - if (pci_get_byte(s->dev.config + PCI_INTERRUPT_PIN)) { >> - /* >> - * FIXME: >> - * Fix device model that realizes this PM device and remove >> - * this work around. >> - * The device model should wire SCI and setup >> - * PCI_INTERRUPT_PIN properly. >> - * If PIN# = 0(interrupt pin isn't used), don't raise SCI as >> - * work around. >> - */ >> - pci_set_irq(&s->dev, sci_level); >> - } >> + qemu_set_irq(s->sci_irq, sci_level); > >I still think this it more complex that it should be and what's in via_isa_set_pm_irq() below should be here instead and drop all the named gpio wizardry that's just unneeded complication here. The bigger picture of this patch is to render pm_update_sci() redundant to acpi_update_sci() and use that. It wants a qemu_irq as one of its parameters which this patch provides. There is one obstacle before acpi_update_sci() can be reused but that should be fixable. I'll send a v5 later. Best regards, Bernhard > >Regards. >BALATON Zoltan > >> /* schedule a timer interruption if needed */ >> acpi_pm_tmr_update(&s->ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) && >> !(pmsts & ACPI_BITMASK_TIMER_STATUS)); >> @@ -213,6 +207,13 @@ static void via_pm_realize(PCIDevice *dev, Error **errp) >> acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2, false); >> } >> >> +static void via_pm_init(Object *obj) >> +{ >> + ViaPMState *s = VIA_PM(obj); >> + >> + qdev_init_gpio_out_named(DEVICE(obj), &s->sci_irq, "sci", 1); >> +} >> + >> typedef struct via_pm_init_info { >> uint16_t device_id; >> } ViaPMInitInfo; >> @@ -238,6 +239,7 @@ static void via_pm_class_init(ObjectClass *klass, void *data) >> static const TypeInfo via_pm_info = { >> .name = TYPE_VIA_PM, >> .parent = TYPE_PCI_DEVICE, >> + .instance_init = via_pm_init, >> .instance_size = sizeof(ViaPMState), >> .abstract = true, >> .interfaces = (InterfaceInfo[]) { >> @@ -568,9 +570,27 @@ static const VMStateDescription vmstate_via = { >> } >> }; >> >> +static void via_isa_set_pm_irq(void *opaque, int n, int level) >> +{ >> + ViaISAState *s = opaque; >> + PCIDevice *pci_dev = PCI_DEVICE(&s->pm); >> + uint8_t irq = pci_get_byte(pci_dev->config + VIA_PM_SCI_SELECT_OFS) >> + & VIA_PM_SCI_SELECT_MASK; >> + >> + if (irq == 2) { >> + qemu_log_mask(LOG_GUEST_ERROR, "IRQ 2 for PM controller is reserved"); >> + return; >> + } >> + >> + if (irq != 0) { >> + qemu_set_irq(s->isa_irqs_in[irq], level); >> + } >> +} >> + >> static void via_isa_init(Object *obj) >> { >> ViaISAState *s = VIA_ISA(obj); >> + DeviceState *dev = DEVICE(s); >> >> object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC); >> object_initialize_child(obj, "ide", &s->ide, TYPE_VIA_IDE); >> @@ -578,6 +598,8 @@ static void via_isa_init(Object *obj) >> object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI); >> object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97); >> object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97); >> + >> + qdev_init_gpio_in_named(dev, via_isa_set_pm_irq, "sci", 1); >> } >> >> static const TypeInfo via_isa_info = { >> @@ -704,6 +726,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp) >> if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) { >> return; >> } >> + qdev_connect_gpio_out_named(DEVICE(&s->pm), "sci", 0, >> + qdev_get_gpio_in_named(DEVICE(d), "sci", 0)); >> >> /* Function 5: AC97 Audio */ >> qdev_prop_set_int32(DEVICE(&s->ac97), "addr", d->devfn + 5); >>
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index 57bdfb4e78..aeb9434a46 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -40,12 +40,17 @@ #define TYPE_VIA_PM "via-pm" OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM) +#define VIA_PM_SCI_SELECT_OFS 0x42 +#define VIA_PM_SCI_SELECT_MASK 0xf + struct ViaPMState { PCIDevice dev; MemoryRegion io; ACPIREGS ar; APMState apm; PMSMBus smb; + + qemu_irq sci_irq; }; static void pm_io_space_update(ViaPMState *s) @@ -148,18 +153,7 @@ static void pm_update_sci(ViaPMState *s) ACPI_BITMASK_POWER_BUTTON_ENABLE | ACPI_BITMASK_GLOBAL_LOCK_ENABLE | ACPI_BITMASK_TIMER_ENABLE)) != 0); - if (pci_get_byte(s->dev.config + PCI_INTERRUPT_PIN)) { - /* - * FIXME: - * Fix device model that realizes this PM device and remove - * this work around. - * The device model should wire SCI and setup - * PCI_INTERRUPT_PIN properly. - * If PIN# = 0(interrupt pin isn't used), don't raise SCI as - * work around. - */ - pci_set_irq(&s->dev, sci_level); - } + qemu_set_irq(s->sci_irq, sci_level); /* schedule a timer interruption if needed */ acpi_pm_tmr_update(&s->ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) && !(pmsts & ACPI_BITMASK_TIMER_STATUS)); @@ -213,6 +207,13 @@ static void via_pm_realize(PCIDevice *dev, Error **errp) acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2, false); } +static void via_pm_init(Object *obj) +{ + ViaPMState *s = VIA_PM(obj); + + qdev_init_gpio_out_named(DEVICE(obj), &s->sci_irq, "sci", 1); +} + typedef struct via_pm_init_info { uint16_t device_id; } ViaPMInitInfo; @@ -238,6 +239,7 @@ static void via_pm_class_init(ObjectClass *klass, void *data) static const TypeInfo via_pm_info = { .name = TYPE_VIA_PM, .parent = TYPE_PCI_DEVICE, + .instance_init = via_pm_init, .instance_size = sizeof(ViaPMState), .abstract = true, .interfaces = (InterfaceInfo[]) { @@ -568,9 +570,27 @@ static const VMStateDescription vmstate_via = { } }; +static void via_isa_set_pm_irq(void *opaque, int n, int level) +{ + ViaISAState *s = opaque; + PCIDevice *pci_dev = PCI_DEVICE(&s->pm); + uint8_t irq = pci_get_byte(pci_dev->config + VIA_PM_SCI_SELECT_OFS) + & VIA_PM_SCI_SELECT_MASK; + + if (irq == 2) { + qemu_log_mask(LOG_GUEST_ERROR, "IRQ 2 for PM controller is reserved"); + return; + } + + if (irq != 0) { + qemu_set_irq(s->isa_irqs_in[irq], level); + } +} + static void via_isa_init(Object *obj) { ViaISAState *s = VIA_ISA(obj); + DeviceState *dev = DEVICE(s); object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC); object_initialize_child(obj, "ide", &s->ide, TYPE_VIA_IDE); @@ -578,6 +598,8 @@ static void via_isa_init(Object *obj) object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI); object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97); object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97); + + qdev_init_gpio_in_named(dev, via_isa_set_pm_irq, "sci", 1); } static const TypeInfo via_isa_info = { @@ -704,6 +726,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp) if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) { return; } + qdev_connect_gpio_out_named(DEVICE(&s->pm), "sci", 0, + qdev_get_gpio_in_named(DEVICE(d), "sci", 0)); /* Function 5: AC97 Audio */ qdev_prop_set_int32(DEVICE(&s->ac97), "addr", d->devfn + 5);