Am 4. Oktober 2023 12:28:58 UTC schrieb BALATON Zoltan <bala...@eik.bme.hu>:
>On Wed, 4 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 ISA function.
>>
>> Signed-off-by: Bernhard Beschow <shen...@gmail.com>
>>
>> ---
>>
>> v2:
>> * Introduce named constants for the ACPI interrupt select register at offset
>> 0x42 (Phil)
>> ---
>> hw/isa/vt82c686.c | 47 +++++++++++++++++++++++++++++++++++------------
>> 1 file changed, 35 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 57bdfb4e78..93ffaaf706 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -46,6 +46,8 @@ struct ViaPMState {
>> ACPIREGS ar;
>> APMState apm;
>> PMSMBus smb;
>> +
>> + qemu_irq irq;
>
>Is it better to name this sci_irq because there seems to be an smi_irq too?
>Also there seems to be no SCI pin but there's an SMI pin so does this sci_irq
>need to be forwaeded to the ISA bridge and exposed as a qemu_gpio or could it
>just set the ISA IRQ within its own handler in the via_pm object?
Triggering the PIC in the PM function seems more complicated since ISA function
embeds PM function and also PM function is implemented before ISA function, so
this would create nesting problems in the code. Either way, both approaches
need to sneak into the other PCI function's data, so I'd keep
via_isa_set_pm_irq() and just update the constants.
> There are also other registers that select between SMI and SCI, do those need
> to be taken into account?
Maybe later. The current code already made the decision to use SCI by
triggering the PIC, so I'd declare this to be out of scope for now.
Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan
>
>> };
>>
>> static void pm_io_space_update(ViaPMState *s)
>> @@ -148,18 +150,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->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 +204,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(DEVICE(obj), &s->irq, 1);
>> +}
>> +
>> typedef struct via_pm_init_info {
>> uint16_t device_id;
>> } ViaPMInitInfo;
>> @@ -238,6 +236,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[]) {
>> @@ -545,6 +544,9 @@ static const TypeInfo vt8231_superio_info = {
>> #define TYPE_VIA_ISA "via-isa"
>> OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
>>
>> +#define VIA_ISA_SCI_SELECT_OFS 0x42
>> +#define VIA_ISA_SCI_SELECT_MASK 0xf
>> +
>> struct ViaISAState {
>> PCIDevice dev;
>> qemu_irq cpu_intr;
>> @@ -568,9 +570,26 @@ static const VMStateDescription vmstate_via = {
>> }
>> };
>>
>> +static void via_isa_set_pm_irq(void *opaque, int n, int level)
>> +{
>> + ViaISAState *s = opaque;
>> + uint8_t irq = pci_get_byte(s->pm.dev.config + VIA_ISA_SCI_SELECT_OFS)
>> + & VIA_ISA_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 +597,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 +725,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(DEVICE(&s->pm), 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);
>>
