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

[BALATON Zoltan]

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);