1. Patchew
  2. QEMU
  3. [PATCH 0/3] VIA PM Improvements
  4. View patch

[PATCH 2/3] hw/isa/vt82c686: Allow PM controller to switch to ACPI mode

Bernhard Beschow posted 3 patches 2 years, 3 months ago
[PATCH 2/3] hw/isa/vt82c686: Allow PM controller to switch to ACPI mode
Posted by Bernhard Beschow 2 years, 3 months ago 
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
+
 #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);
-- 
2.39.1
Re: [PATCH 2/3] hw/isa/vt82c686: Allow PM controller to switch to ACPI mode
Posted by BALATON Zoltan 2 years, 3 months ago 
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);
>
Re: [PATCH 2/3] hw/isa/vt82c686: Allow PM controller to switch to ACPI mode
Posted by Philippe Mathieu-Daudé 2 years, 3 months ago 
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")?
Re: [PATCH 2/3] hw/isa/vt82c686: Allow PM controller to switch to ACPI mode
Posted by Igor Mammedov 2 years, 2 months ago 
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")?
>
Re: [PATCH 2/3] hw/isa/vt82c686: Allow PM controller to switch to ACPI mode
Posted by Philippe Mathieu-Daudé 2 years, 3 months ago 
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")?

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.