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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.