hw/isa/vt82c686.c | 48 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 36 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 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);
/* 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);
--
2.42.0
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);
>
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);
>>
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.
© 2016 - 2024 Red Hat, Inc.