Re: [PATCH v2 1/2] acpi: ged: Add macro for acpi sleep control register

[maobibo]

On 2024/9/13 下午8:41, Igor Mammedov wrote:
> On Wed, 11 Sep 2024 11:09:21 +0800
> Bibo Mao <maobibo@loongson.cn> wrote:
>
> > Macro definition is added for acpi sleep control register, so that
> > ged emulation driver can use this, also it can be used in FDT table if
> > ged is exposed with FDT table.
> >
> > Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> > ---
> >   hw/acpi/generic_event_device.c         | 6 +++---
> >   hw/i386/acpi-microvm.c                 | 2 +-
> >   hw/loongarch/acpi-build.c              | 2 +-
> >   include/hw/acpi/generic_event_device.h | 9 +++++++--
> >   4 files changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> > index 15b4c3ebbf..94992e6119 100644
> > --- a/hw/acpi/generic_event_device.c
> > +++ b/hw/acpi/generic_event_device.c
> > @@ -201,9 +201,9 @@ static void ged_regs_write(void *opaque, hwaddr addr, 
> > uint64_t data,
> >   
> >       switch (addr) {
> >       case ACPI_GED_REG_SLEEP_CTL:
> > -        slp_typ = (data >> 2) & 0x07;
> > -        slp_en  = (data >> 5) & 0x01;
> > -        if (slp_en && slp_typ == 5) {
> > +        slp_typ = (data & ACPI_GED_SLP_TYPx_MASK) >> ACPI_GED_SLP_TYPx_POS;
> this makes a bit more complex expression once macros are expanded,
> but doesn't really helps to clarity.
>
> If I have to touch/share this code, I'd replace magic numbers above
> with corresponding simple numeric macro but keep the same expressions.
That sounds reasonable, it is better to keep the same expression such as:
    slp_typ = (data >> ACPI_GED_SLP_TYPx_POS) & ACPI_GED_SLP_TYPx_MASK;

However what about for this sentence?
    slp_en  = (data >> 5) & 0x01;
I think the modification like this is better
    slp_en  = !!(data & ACPI_GED_SLP_EN);

> > +        slp_en  = !!(data & ACPI_GED_SLP_EN);
> > +        if (slp_en && slp_typ == ACPI_GED_SLP_TYPx_S5) {
> >               qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> >           }
> >           return;
> > diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> > index 279da6b4aa..1e424076d2 100644
> > --- a/hw/i386/acpi-microvm.c
> > +++ b/hw/i386/acpi-microvm.c
> > @@ -131,7 +131,7 @@ build_dsdt_microvm(GArray *table_data, BIOSLinker *linker,
> >       /* ACPI 5.0: Table 7-209 System State Package */
> >       scope = aml_scope("\\");
> >       pkg = aml_package(4);
> > -    aml_append(pkg, aml_int(ACPI_GED_SLP_TYP_S5));
> > +    aml_append(pkg, aml_int(ACPI_GED_SLP_TYPx_S5));
>
> what's the point of renaming this?
ACPI spec set name with SLP_TYPx. I am ok with both, it seems less 
modification is better.

Regards
Bibo Mao
> >       aml_append(pkg, aml_int(0)); /* ignored */
> >       aml_append(pkg, aml_int(0)); /* reserved */
> >       aml_append(pkg, aml_int(0)); /* reserved */
> > diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c
> > index 2638f87434..974519a347 100644
> > --- a/hw/loongarch/acpi-build.c
> > +++ b/hw/loongarch/acpi-build.c
> > @@ -418,7 +418,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, 
> > MachineState *machine)
> >       /* System State Package */
> >       scope = aml_scope("\\");
> >       pkg = aml_package(4);
> > -    aml_append(pkg, aml_int(ACPI_GED_SLP_TYP_S5));
> > +    aml_append(pkg, aml_int(ACPI_GED_SLP_TYPx_S5));
> >       aml_append(pkg, aml_int(0)); /* ignored */
> >       aml_append(pkg, aml_int(0)); /* reserved */
> >       aml_append(pkg, aml_int(0)); /* reserved */
> > diff --git a/include/hw/acpi/generic_event_device.h 
> > b/include/hw/acpi/generic_event_device.h
> > index 40af3550b5..41741e94ea 100644
> > --- a/include/hw/acpi/generic_event_device.h
> > +++ b/include/hw/acpi/generic_event_device.h
> > @@ -81,8 +81,13 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
> >   /* ACPI_GED_REG_RESET value for reset*/
> >   #define ACPI_GED_RESET_VALUE       0x42
> >   
> > -/* ACPI_GED_REG_SLEEP_CTL.SLP_TYP value for S5 (aka poweroff) */
> > -#define ACPI_GED_SLP_TYP_S5        0x05
> > +/* [ACPI 5.0+ FADT] Sleep Control Register */
> > +/* 3-bit field defines the type of hardware sleep state */
> > +#define ACPI_GED_SLP_TYPx_POS      0x2
> > +#define ACPI_GED_SLP_TYPx_MASK     (0x07 << ACPI_GED_SLP_TYPx_POS)
> > +#define ACPI_GED_SLP_TYPx_S5       0x05  /* System \_S5 State (Soft Off) */
> > +/* Write-only, Set this bit causes system to enter SLP_TYPx sleeping state */
> > +#define ACPI_GED_SLP_EN            0x20
> >   
> >   #define GED_DEVICE      "GED"
> >   #define AML_GED_EVT_REG "EREG"