hw/isa/vt82c686.c | 47 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 12 deletions(-)
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 <shentey@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;
};
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);
--
2.42.0On 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 <shentey@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? There are also other registers that select between SMI and SCI, do
those need to be taken into account?
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);
>
Am 4. Oktober 2023 12:28:58 UTC schrieb BALATON Zoltan <balaton@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 <shentey@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);
>>
On Thu, 5 Oct 2023, Bernhard Beschow wrote:
> Am 4. Oktober 2023 12:28:58 UTC schrieb BALATON Zoltan <balaton@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 <shentey@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
Where PM function is implemented is just because it was there before or
that's how I went through the functions when cleaning it up and ended up
there but it could be moved, it's not bolted down...
However even if it comes before, we had the pattern before for via-ide and
usb that they can look up function 0 of their own devfn to find the ISA
bridge and sinve we're in vt82b686.c here you can consider these to be
friend classes so pm func could access the ISA irq's directly (or bring
back the via_isa_set_irq func I had before for this). That way it's
simpler and does not need QOM wizardry in ISA function that does not even
model what real chip does so I think this should be implemented in an irq
handler func within the PM object that matches what happens in the real
chip.
> 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.
Yes, if you're now just handling sci with one wpecific mapping then those
can wait until adding smi, just found them now and asked if they need to
be considered for sci now.
Regards,
BALATON Zoltan
Am 5. Oktober 2023 11:26:56 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Thu, 5 Oct 2023, Bernhard Beschow wrote:
>> Am 4. Oktober 2023 12:28:58 UTC schrieb BALATON Zoltan <balaton@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 <shentey@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
>
>Where PM function is implemented is just because it was there before or that's how I went through the functions when cleaning it up and ended up there but it could be moved, it's not bolted down...
>
>However even if it comes before, we had the pattern before for via-ide and usb that they can look up function 0 of their own devfn to find the ISA bridge and sinve we're in vt82b686.c here you can consider these to be friend classes so pm func could access the ISA irq's directly (or bring back the via_isa_set_irq func I had before for this). That way it's simpler and does not need QOM wizardry in ISA function that does not even model what real chip does so I think this should be implemented in an irq handler func within the PM object that matches what happens in the real chip.
This would require container_of() to be used which violates QOM best practices where a device should only know about its (transitive) children. Given the current nesting of devices I prefer to keep the PIC triggering in the ISA function where the PIC resides.
Best regards,
Bernhard
>
>> 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.
>
>Yes, if you're now just handling sci with one wpecific mapping then those can wait until adding smi, just found them now and asked if they need to be considered for sci now.
>
>Regards,
>BALATON Zoltan
On Thu, 5 Oct 2023, Bernhard Beschow wrote:
> Am 5. Oktober 2023 11:26:56 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Thu, 5 Oct 2023, Bernhard Beschow wrote:
>>> Am 4. Oktober 2023 12:28:58 UTC schrieb BALATON Zoltan <balaton@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 <shentey@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
>>
>> Where PM function is implemented is just because it was there before or that's how I went through the functions when cleaning it up and ended up there but it could be moved, it's not bolted down...
>>
>> However even if it comes before, we had the pattern before for via-ide and usb that they can look up function 0 of their own devfn to find the ISA bridge and sinve we're in vt82b686.c here you can consider these to be friend classes so pm func could access the ISA irq's directly (or bring back the via_isa_set_irq func I had before for this). That way it's simpler and does not need QOM wizardry in ISA function that does not even model what real chip does so I think this should be implemented in an irq handler func within the PM object that matches what happens in the real chip.
>
> This would require container_of() to be used which violates QOM best
> practices where a device should only know about its (transitive)
> children. Given the current nesting of devices I prefer to keep the PIC
> triggering in the ISA function where the PIC resides.
No it doesn't. You can get the isa function with
pci_get_function_0(&s->dev) in the PM object where s is pointing to the
ViaPMState. Then if you don't want to poke the ISA object from the PM
object revert the patch that removed via_isa_set_irq that I've added for
such cases but I think you can just access the ISA func from the PM func
in the same file as these are part of the same chip model, only modelled
as QOM objects to separate them in some logical way.
Regards,
BALATON Zoltan
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 <shentey@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;
> };
>
> 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
Missed this before revieweing v1. As this should belong to the PM function
it should not be here but above so maybe Philippe's other suggestion to
use a getter function is clearer then to define that function above in the
via_pm section.
Regards,
BALATON Zoltan
> +
> 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);
>
On 4/10/23 12:57, 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 <shentey@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(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
© 2016 - 2024 Red Hat, Inc.