Re: [PATCH v3] hw/isa/vt82c686: Respect SCI interrupt assignment

[Philippe Mathieu-Daudé]

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.