| Message ID | 20231028091606.23700-4-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | VIA PM: Implement basic ACPI support | expand |
On Sat, 28 Oct 2023, Bernhard Beschow wrote: > acpi_update_sci() covers everything pm_update_sci() does. It implements common > ACPI funtionality in a generic fashion. Note that it agnostic to any > Frankenstein usage of the general purpose event registers in other device > models. It just implements a generic mechanism which can be wired to arbitrary > functionality. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/isa/vt82c686.c | 20 ++------------------ > 1 file changed, 2 insertions(+), 18 deletions(-) > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index 60ca781e03..7b44ad9485 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = { > }, > }; > > -static void pm_update_sci(ViaPMState *s) > -{ > - int sci_level, pmsts; > - > - pmsts = acpi_pm1_evt_get_sts(&s->ar); > - sci_level = (((pmsts & s->ar.pm1.evt.en) & > - (ACPI_BITMASK_RT_CLOCK_ENABLE | > - ACPI_BITMASK_POWER_BUTTON_ENABLE | > - ACPI_BITMASK_GLOBAL_LOCK_ENABLE | > - ACPI_BITMASK_TIMER_ENABLE)) != 0); The level calculation in acpi_update_sci() is different than the above. Which one is correct and why? Regards, BALATON Zoltan > - 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)); > -} > - > static void pm_tmr_timer(ACPIREGS *ar) > { > ViaPMState *s = container_of(ar, ViaPMState, ar); > - pm_update_sci(s); > + acpi_update_sci(&s->ar, s->sci_irq); > } > > static void via_pm_reset(DeviceState *d) > @@ -182,7 +166,7 @@ static void via_pm_reset(DeviceState *d) > acpi_pm1_cnt_reset(&s->ar); > acpi_pm_tmr_reset(&s->ar); > acpi_gpe_reset(&s->ar); > - pm_update_sci(s); > + acpi_update_sci(&s->ar, s->sci_irq); > > pm_io_space_update(s); > smb_io_space_update(s); >
Am 28. Oktober 2023 12:59:56 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Sat, 28 Oct 2023, Bernhard Beschow wrote: >> acpi_update_sci() covers everything pm_update_sci() does. It implements common >> ACPI funtionality in a generic fashion. Note that it agnostic to any >> Frankenstein usage of the general purpose event registers in other device >> models. It just implements a generic mechanism which can be wired to arbitrary >> functionality. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/isa/vt82c686.c | 20 ++------------------ >> 1 file changed, 2 insertions(+), 18 deletions(-) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 60ca781e03..7b44ad9485 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = { >> }, >> }; >> >> -static void pm_update_sci(ViaPMState *s) >> -{ >> - int sci_level, pmsts; >> - >> - pmsts = acpi_pm1_evt_get_sts(&s->ar); >> - sci_level = (((pmsts & s->ar.pm1.evt.en) & >> - (ACPI_BITMASK_RT_CLOCK_ENABLE | >> - ACPI_BITMASK_POWER_BUTTON_ENABLE | >> - ACPI_BITMASK_GLOBAL_LOCK_ENABLE | >> - ACPI_BITMASK_TIMER_ENABLE)) != 0); > >The level calculation in acpi_update_sci() is different than the above. Which one is correct and why? acpi_update_sci() just covers the GPE registers in addition which are standard ACPI registers. As written in the commit message these aren't currently mapped by the device model so shouldn't cause any interferences. Best regards, Bernhard > >Regards, >BALATON Zoltan > >> - 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)); >> -} >> - >> static void pm_tmr_timer(ACPIREGS *ar) >> { >> ViaPMState *s = container_of(ar, ViaPMState, ar); >> - pm_update_sci(s); >> + acpi_update_sci(&s->ar, s->sci_irq); >> } >> >> static void via_pm_reset(DeviceState *d) >> @@ -182,7 +166,7 @@ static void via_pm_reset(DeviceState *d) >> acpi_pm1_cnt_reset(&s->ar); >> acpi_pm_tmr_reset(&s->ar); >> acpi_gpe_reset(&s->ar); >> - pm_update_sci(s); >> + acpi_update_sci(&s->ar, s->sci_irq); >> >> pm_io_space_update(s); >> smb_io_space_update(s); >>
On Sat, 28 Oct 2023, Bernhard Beschow wrote: > acpi_update_sci() covers everything pm_update_sci() does. It implements common > ACPI funtionality in a generic fashion. Note that it agnostic to any > Frankenstein usage of the general purpose event registers in other device > models. It just implements a generic mechanism which can be wired to arbitrary > functionality. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/isa/vt82c686.c | 20 ++------------------ > 1 file changed, 2 insertions(+), 18 deletions(-) > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index 60ca781e03..7b44ad9485 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = { > }, > }; > > -static void pm_update_sci(ViaPMState *s) > -{ > - int sci_level, pmsts; > - > - pmsts = acpi_pm1_evt_get_sts(&s->ar); > - sci_level = (((pmsts & s->ar.pm1.evt.en) & > - (ACPI_BITMASK_RT_CLOCK_ENABLE | > - ACPI_BITMASK_POWER_BUTTON_ENABLE | > - ACPI_BITMASK_GLOBAL_LOCK_ENABLE | > - ACPI_BITMASK_TIMER_ENABLE)) != 0); > - 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)); > -} > - > static void pm_tmr_timer(ACPIREGS *ar) > { > ViaPMState *s = container_of(ar, ViaPMState, ar); > - pm_update_sci(s); > + acpi_update_sci(&s->ar, s->sci_irq); To avoid needing an interrupt here maybe you could modify acpi_update_sci() to allow NULL irq then call via_isa_set_irq here so the interrupt routing can be done within the ISA bridge. Regards, BALATON Zoltan > } > > static void via_pm_reset(DeviceState *d) > @@ -182,7 +166,7 @@ static void via_pm_reset(DeviceState *d) > acpi_pm1_cnt_reset(&s->ar); > acpi_pm_tmr_reset(&s->ar); > acpi_gpe_reset(&s->ar); > - pm_update_sci(s); > + acpi_update_sci(&s->ar, s->sci_irq); > > pm_io_space_update(s); > smb_io_space_update(s); >
Am 29. Oktober 2023 00:07:00 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Sat, 28 Oct 2023, Bernhard Beschow wrote: >> acpi_update_sci() covers everything pm_update_sci() does. It implements common >> ACPI funtionality in a generic fashion. Note that it agnostic to any >> Frankenstein usage of the general purpose event registers in other device >> models. It just implements a generic mechanism which can be wired to arbitrary >> functionality. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/isa/vt82c686.c | 20 ++------------------ >> 1 file changed, 2 insertions(+), 18 deletions(-) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 60ca781e03..7b44ad9485 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = { >> }, >> }; >> >> -static void pm_update_sci(ViaPMState *s) >> -{ >> - int sci_level, pmsts; >> - >> - pmsts = acpi_pm1_evt_get_sts(&s->ar); >> - sci_level = (((pmsts & s->ar.pm1.evt.en) & >> - (ACPI_BITMASK_RT_CLOCK_ENABLE | >> - ACPI_BITMASK_POWER_BUTTON_ENABLE | >> - ACPI_BITMASK_GLOBAL_LOCK_ENABLE | >> - ACPI_BITMASK_TIMER_ENABLE)) != 0); >> - 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)); >> -} >> - >> static void pm_tmr_timer(ACPIREGS *ar) >> { >> ViaPMState *s = container_of(ar, ViaPMState, ar); >> - pm_update_sci(s); >> + acpi_update_sci(&s->ar, s->sci_irq); > >To avoid needing an interrupt here maybe you could modify acpi_update_sci() to allow NULL irq then call via_isa_set_irq here so the interrupt routing can be done within the ISA bridge. Why? This function works well as is for other device models. Best regards, Bernhard > >Regards, >BALATON Zoltan > >> } >> >> static void via_pm_reset(DeviceState *d) >> @@ -182,7 +166,7 @@ static void via_pm_reset(DeviceState *d) >> acpi_pm1_cnt_reset(&s->ar); >> acpi_pm_tmr_reset(&s->ar); >> acpi_gpe_reset(&s->ar); >> - pm_update_sci(s); >> + acpi_update_sci(&s->ar, s->sci_irq); >> >> pm_io_space_update(s); >> smb_io_space_update(s); >>
On Sun, 29 Oct 2023, Bernhard Beschow wrote: > Am 29. Oktober 2023 00:07:00 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >> On Sat, 28 Oct 2023, Bernhard Beschow wrote: >>> acpi_update_sci() covers everything pm_update_sci() does. It implements common >>> ACPI funtionality in a generic fashion. Note that it agnostic to any >>> Frankenstein usage of the general purpose event registers in other device >>> models. It just implements a generic mechanism which can be wired to arbitrary >>> functionality. >>> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> hw/isa/vt82c686.c | 20 ++------------------ >>> 1 file changed, 2 insertions(+), 18 deletions(-) >>> >>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>> index 60ca781e03..7b44ad9485 100644 >>> --- a/hw/isa/vt82c686.c >>> +++ b/hw/isa/vt82c686.c >>> @@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = { >>> }, >>> }; >>> >>> -static void pm_update_sci(ViaPMState *s) >>> -{ >>> - int sci_level, pmsts; >>> - >>> - pmsts = acpi_pm1_evt_get_sts(&s->ar); >>> - sci_level = (((pmsts & s->ar.pm1.evt.en) & >>> - (ACPI_BITMASK_RT_CLOCK_ENABLE | >>> - ACPI_BITMASK_POWER_BUTTON_ENABLE | >>> - ACPI_BITMASK_GLOBAL_LOCK_ENABLE | >>> - ACPI_BITMASK_TIMER_ENABLE)) != 0); >>> - 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)); >>> -} >>> - >>> static void pm_tmr_timer(ACPIREGS *ar) >>> { >>> ViaPMState *s = container_of(ar, ViaPMState, ar); >>> - pm_update_sci(s); >>> + acpi_update_sci(&s->ar, s->sci_irq); >> >> To avoid needing an interrupt here maybe you could modify acpi_update_sci() to allow NULL irq then call via_isa_set_irq here so the interrupt routing can be done within the ISA bridge. > > Why? This function works well as is for other device models. To avoid adding another qemu_irq which is not needed otherwise to keep the via model which is already complex relatively simple. Regards, ALATON Zoltan >>> } >>> >>> static void via_pm_reset(DeviceState *d) >>> @@ -182,7 +166,7 @@ static void via_pm_reset(DeviceState *d) >>> acpi_pm1_cnt_reset(&s->ar); >>> acpi_pm_tmr_reset(&s->ar); >>> acpi_gpe_reset(&s->ar); >>> - pm_update_sci(s); >>> + acpi_update_sci(&s->ar, s->sci_irq); >>> >>> pm_io_space_update(s); >>> smb_io_space_update(s); >>> > >
Am 29. Oktober 2023 00:07:00 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Sat, 28 Oct 2023, Bernhard Beschow wrote: >> acpi_update_sci() covers everything pm_update_sci() does. It implements common >> ACPI funtionality in a generic fashion. Note that it agnostic to any >> Frankenstein usage of the general purpose event registers in other device >> models. It just implements a generic mechanism which can be wired to arbitrary >> functionality. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/isa/vt82c686.c | 20 ++------------------ >> 1 file changed, 2 insertions(+), 18 deletions(-) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 60ca781e03..7b44ad9485 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = { >> }, >> }; >> >> -static void pm_update_sci(ViaPMState *s) >> -{ >> - int sci_level, pmsts; >> - >> - pmsts = acpi_pm1_evt_get_sts(&s->ar); >> - sci_level = (((pmsts & s->ar.pm1.evt.en) & >> - (ACPI_BITMASK_RT_CLOCK_ENABLE | >> - ACPI_BITMASK_POWER_BUTTON_ENABLE | >> - ACPI_BITMASK_GLOBAL_LOCK_ENABLE | >> - ACPI_BITMASK_TIMER_ENABLE)) != 0); >> - 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)); >> -} >> - >> static void pm_tmr_timer(ACPIREGS *ar) >> { >> ViaPMState *s = container_of(ar, ViaPMState, ar); >> - pm_update_sci(s); >> + acpi_update_sci(&s->ar, s->sci_irq); > >To avoid needing an interrupt here maybe you could modify acpi_update_sci() to allow NULL irq then call via_isa_set_irq here so the interrupt routing can be done within the ISA bridge. Given the interesting behavior of the amigaone boot loader I'd respin this series with the last two patches only. Best regards, Bernhard > >Regards, >BALATON Zoltan > >> } >> >> static void via_pm_reset(DeviceState *d) >> @@ -182,7 +166,7 @@ static void via_pm_reset(DeviceState *d) >> acpi_pm1_cnt_reset(&s->ar); >> acpi_pm_tmr_reset(&s->ar); >> acpi_gpe_reset(&s->ar); >> - pm_update_sci(s); >> + acpi_update_sci(&s->ar, s->sci_irq); >> >> pm_io_space_update(s); >> smb_io_space_update(s); >>
On Mon, 30 Oct 2023, Bernhard Beschow wrote: > Am 29. Oktober 2023 00:07:00 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >> On Sat, 28 Oct 2023, Bernhard Beschow wrote: >>> acpi_update_sci() covers everything pm_update_sci() does. It implements common >>> ACPI funtionality in a generic fashion. Note that it agnostic to any >>> Frankenstein usage of the general purpose event registers in other device >>> models. It just implements a generic mechanism which can be wired to arbitrary >>> functionality. >>> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> hw/isa/vt82c686.c | 20 ++------------------ >>> 1 file changed, 2 insertions(+), 18 deletions(-) >>> >>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>> index 60ca781e03..7b44ad9485 100644 >>> --- a/hw/isa/vt82c686.c >>> +++ b/hw/isa/vt82c686.c >>> @@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = { >>> }, >>> }; >>> >>> -static void pm_update_sci(ViaPMState *s) >>> -{ >>> - int sci_level, pmsts; >>> - >>> - pmsts = acpi_pm1_evt_get_sts(&s->ar); >>> - sci_level = (((pmsts & s->ar.pm1.evt.en) & >>> - (ACPI_BITMASK_RT_CLOCK_ENABLE | >>> - ACPI_BITMASK_POWER_BUTTON_ENABLE | >>> - ACPI_BITMASK_GLOBAL_LOCK_ENABLE | >>> - ACPI_BITMASK_TIMER_ENABLE)) != 0); >>> - 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)); >>> -} >>> - >>> static void pm_tmr_timer(ACPIREGS *ar) >>> { >>> ViaPMState *s = container_of(ar, ViaPMState, ar); >>> - pm_update_sci(s); >>> + acpi_update_sci(&s->ar, s->sci_irq); >> >> To avoid needing an interrupt here maybe you could modify acpi_update_sci() to allow NULL irq then call via_isa_set_irq here so the interrupt routing can be done within the ISA bridge. > > Given the interesting behavior of the amigaone boot loader I'd respin this series with the last two patches only. I guess you could do that just modeling the register and leave out SMI for now until we resolve the IRQ routing so then it should not conflict. I think you said that nothing uses the interrupt and you just need to be able to trigger poweroff with register write? If you add SMI I think the qemu_irq for that should be in VIAISAState where the other irqs are and PM func should call via_isa_set_irq where it decides to route the SCI to ISA or SMI based on reg values. You can do this in the swich under case PCI_FUNC(of PM device) but if you don't need the IRQ this can be added later in a follow up I think. Regards, BALATON Zoltan
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index 60ca781e03..7b44ad9485 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -145,26 +145,10 @@ static const MemoryRegionOps pm_io_ops = { }, }; -static void pm_update_sci(ViaPMState *s) -{ - int sci_level, pmsts; - - pmsts = acpi_pm1_evt_get_sts(&s->ar); - sci_level = (((pmsts & s->ar.pm1.evt.en) & - (ACPI_BITMASK_RT_CLOCK_ENABLE | - ACPI_BITMASK_POWER_BUTTON_ENABLE | - ACPI_BITMASK_GLOBAL_LOCK_ENABLE | - ACPI_BITMASK_TIMER_ENABLE)) != 0); - 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)); -} - static void pm_tmr_timer(ACPIREGS *ar) { ViaPMState *s = container_of(ar, ViaPMState, ar); - pm_update_sci(s); + acpi_update_sci(&s->ar, s->sci_irq); } static void via_pm_reset(DeviceState *d) @@ -182,7 +166,7 @@ static void via_pm_reset(DeviceState *d) acpi_pm1_cnt_reset(&s->ar); acpi_pm_tmr_reset(&s->ar); acpi_gpe_reset(&s->ar); - pm_update_sci(s); + acpi_update_sci(&s->ar, s->sci_irq); pm_io_space_update(s); smb_io_space_update(s);
acpi_update_sci() covers everything pm_update_sci() does. It implements common ACPI funtionality in a generic fashion. Note that it agnostic to any Frankenstein usage of the general purpose event registers in other device models. It just implements a generic mechanism which can be wired to arbitrary functionality. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/isa/vt82c686.c | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-)