Am 4. Oktober 2023 12:08:02 UTC schrieb BALATON Zoltan <bala...@eik.bme.hu>:
>On Tue, 3 Oct 2023, Bernhard Beschow wrote:
>> According to the datasheet, SCI interrupts of the power management function
>> aren't triggered through the PCI pins but rather directly to the integrated
>> PIC.
>> The routing is configurable through the ACPI interrupt select register at
>> offset
>> 42 in the PCI configuration space of the ISA function.
>
>This should be config reg 42 of the PM function 4 not ISA function 0 but the
>code seems to do it right just this description is wrong.
Notice via_isa_set_pm_irq() using ViaISAState for routing the SCI to the PIC.
Hence, the description seems correct to me.
>
>>
>> Signed-off-by: Bernhard Beschow <shen...@gmail.com>
>
>You could cc me on vt82c686 too as now I have two machines using these (one is
>not yet upstream).
Usually I let git-publish handle the cc which derives it from the MAINTAINERS
file. You could add yourself there to get cc'ed automatically.
I'm curious what the other machine not yet upstreamed is?
>
>> ---
>> hw/isa/vt82c686.c | 43 +++++++++++++++++++++++++++++++------------
>> 1 file changed, 31 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 57bdfb4e78..2988ad1eda 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -46,6 +46,8 @@ struct ViaPMState {
>> ACPIREGS ar;
>> APMState apm;
>> PMSMBus smb;
>> +
>
>No empty line needed here.
Here I took inspiration from struct PIIX4PMState which separates the qemu_irqs,
too. The long term plan is to also add an smi_irq attribute in order to bring
both device models to feature parity. So I'd rather leave it as is.
> It you want to, you could add an empty line after the first member and rename
> that to parent_obj as per new convention while you're changing this object
> state.
I didn't want to add this style fix in this single commit series. I think this
would deserve its own commit where all device models in this file are fixed.
>
>> + 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[]) {
>> @@ -568,9 +567,25 @@ 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 + 0x42) & 0xf;
>> +
>> + 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);
>
>No need to add local variable for single use unless you expect to have more of
>these later but for single use you caould just use DEVICE(obj or s) in the
>call below.
I have one more user on my pc-via branch for wiring the ISA interrupts.
Best regards,
Bernhard
>
>Other than these small nits:
>
>Reviewed-by: BALATON Zoltan <bala...@eik.bme.hu>
>
>>
>> object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>> object_initialize_child(obj, "ide", &s->ide, TYPE_VIA_IDE);
>> @@ -578,6 +593,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 +721,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);
>>
