On Sat, 14 Sep 2024 10:25:45 +0800
maobibo <maob...@loongson.cn> wrote:
> On 2024/9/13 下午8:41, Igor Mammedov wrote:
> > On Wed, 11 Sep 2024 11:09:21 +0800
> > Bibo Mao <maob...@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 <maob...@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);
then one has to got and check what ACPI_GED_SLP_EN is
and why it's that specific value.
while keeping it as is would be consistent with slp_typ
line right above it.
But it's stylistic, I don't really care wrt it.
>
> >
> >> + 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.
I'd avoid renaming, if one need to reference spec we usually add
a comment about field/value that points to earliest spec where it
was introduced and chapter in it. Also for fields we also add
a comment with _verbatim_ field name from spec, so that one
can copy/past/search it in spec when needed.
>
> 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"
> >
>
