| Message ID | 20230129213418.87978-3-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VIA PM Improvements | expand |
On Sun, 29 Jan 2023, Bernhard Beschow wrote: > Adds missing functionality the real hardware supports. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/isa/vt82c686.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index 2189be6f20..b0765d4ed8 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -37,6 +37,9 @@ > #include "qemu/timer.h" > #include "trace.h" > > +#define ACPI_ENABLE 0xf1 > +#define ACPI_DISABLE 0xf0 Are these some standard in which case they should be in a shared acpi header or chip specific, then we could just put it in a comment, see below. > + > #define TYPE_VIA_PM "via-pm" > OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM) > > @@ -50,6 +53,19 @@ struct ViaPMState { > qemu_irq irq; > }; > > +static void via_pm_apm_ctrl_changed(uint32_t val, void *arg) > +{ > + ViaPMState *s = arg; > + > + /* ACPI specs 3.0, 4.7.2.5 */ > + acpi_pm1_cnt_update(&s->ar, val == ACPI_ENABLE, val == ACPI_DISABLE); > + if (val == ACPI_ENABLE || val == ACPI_DISABLE) { > + return; > + } > + > + qemu_log_mask(LOG_UNIMP, "%s: unimplemented value 0x%x", __func__, val); Maybe a switch is more readable here: switch(val) { case 0xf1: /* Enable */ case 0xf0: /* Disable */ acpi_pm1_cnt_update(&s->ar, val & 1, val & 1); break; default: qemu_log_mask(LOG_UNIMP, "%s: unimplemented value 0x%x", __func__, val); } Or maybe not, it can be personal preference. (Why does acpi_pm1_cnt_update() take two arguments for a bool value? Can these be both true or false at the same time?) Regards, BALATON Zoltan > +} > + > static void pm_io_space_update(ViaPMState *s) > { > uint32_t pmbase = pci_get_long(s->dev.config + 0x48) & 0xff80UL; > @@ -193,7 +209,7 @@ static void via_pm_realize(PCIDevice *dev, Error **errp) > memory_region_add_subregion(pci_address_space_io(dev), 0, &s->smb.io); > memory_region_set_enabled(&s->smb.io, false); > > - apm_init(dev, &s->apm, NULL, s); > + apm_init(dev, &s->apm, via_pm_apm_ctrl_changed, s); > > memory_region_init_io(&s->io, OBJECT(dev), &pm_io_ops, s, "via-pm", 128); > memory_region_add_subregion(pci_address_space_io(dev), 0, &s->io); >
On 31/1/23 15:54, BALATON Zoltan wrote: > On Sun, 29 Jan 2023, Bernhard Beschow wrote: >> Adds missing functionality the real hardware supports. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/isa/vt82c686.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 2189be6f20..b0765d4ed8 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -37,6 +37,9 @@ >> #include "qemu/timer.h" >> #include "trace.h" > Why does > acpi_pm1_cnt_update() take two arguments for a bool value? Can these be > both true or false at the same time? No, this is a one-bit so boolean is enough... Maybe unfinished refactor from commit eaba51c573 ("acpi, acpi_piix, vt82c686: factor out PM1_CNT logic")?
Forgot to Cc ACPI maintainers. On 6/2/23 09:00, Philippe Mathieu-Daudé wrote: > On 31/1/23 15:54, BALATON Zoltan wrote: >> On Sun, 29 Jan 2023, Bernhard Beschow wrote: >>> Adds missing functionality the real hardware supports. >>> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> hw/isa/vt82c686.c | 18 +++++++++++++++++- >>> 1 file changed, 17 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>> index 2189be6f20..b0765d4ed8 100644 >>> --- a/hw/isa/vt82c686.c >>> +++ b/hw/isa/vt82c686.c >>> @@ -37,6 +37,9 @@ >>> #include "qemu/timer.h" >>> #include "trace.h" > >> Why does acpi_pm1_cnt_update() take two arguments for a bool value? >> Can these be both true or false at the same time? > > No, this is a one-bit so boolean is enough... > > Maybe unfinished refactor from commit eaba51c573 ("acpi, acpi_piix, > vt82c686: factor out PM1_CNT logic")?
On Mon, 6 Feb 2023 09:00:37 +0100 Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > On 31/1/23 15:54, BALATON Zoltan wrote: > > On Sun, 29 Jan 2023, Bernhard Beschow wrote: > >> Adds missing functionality the real hardware supports. > >> > >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> > >> --- > >> hw/isa/vt82c686.c | 18 +++++++++++++++++- > >> 1 file changed, 17 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > >> index 2189be6f20..b0765d4ed8 100644 > >> --- a/hw/isa/vt82c686.c > >> +++ b/hw/isa/vt82c686.c > >> @@ -37,6 +37,9 @@ > >> #include "qemu/timer.h" > >> #include "trace.h" > > > Why does > > acpi_pm1_cnt_update() take two arguments for a bool value? Can these be > > both true or false at the same time? > > No, this is a one-bit so boolean is enough... one boolean would be fine unless they were both false? > > Maybe unfinished refactor from commit eaba51c573 ("acpi, acpi_piix, > vt82c686: factor out PM1_CNT logic")? >
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index 2189be6f20..b0765d4ed8 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -37,6 +37,9 @@ #include "qemu/timer.h" #include "trace.h" +#define ACPI_ENABLE 0xf1 +#define ACPI_DISABLE 0xf0 + #define TYPE_VIA_PM "via-pm" OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM) @@ -50,6 +53,19 @@ struct ViaPMState { qemu_irq irq; }; +static void via_pm_apm_ctrl_changed(uint32_t val, void *arg) +{ + ViaPMState *s = arg; + + /* ACPI specs 3.0, 4.7.2.5 */ + acpi_pm1_cnt_update(&s->ar, val == ACPI_ENABLE, val == ACPI_DISABLE); + if (val == ACPI_ENABLE || val == ACPI_DISABLE) { + return; + } + + qemu_log_mask(LOG_UNIMP, "%s: unimplemented value 0x%x", __func__, val); +} + static void pm_io_space_update(ViaPMState *s) { uint32_t pmbase = pci_get_long(s->dev.config + 0x48) & 0xff80UL; @@ -193,7 +209,7 @@ static void via_pm_realize(PCIDevice *dev, Error **errp) memory_region_add_subregion(pci_address_space_io(dev), 0, &s->smb.io); memory_region_set_enabled(&s->smb.io, false); - apm_init(dev, &s->apm, NULL, s); + apm_init(dev, &s->apm, via_pm_apm_ctrl_changed, s); memory_region_init_io(&s->io, OBJECT(dev), &pm_io_ops, s, "via-pm", 128); memory_region_add_subregion(pci_address_space_io(dev), 0, &s->io);
Adds missing functionality the real hardware supports. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/isa/vt82c686.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)