[PATCH v2 0/2] Add FDT table support with acpi ged pm register

[PATCH v2 0/2] Add FDT table support with acpi ged pm register

[Bibo Mao]

ACPI ged is used for power management on LoongArch virt platform, in
general it is parsed from acpi table. However if system boot directly from
elf kernel, no UEFI bios is provided and acpi table cannot be used also.

Here acpi ged pm register is exposed with FDT table, it is compatbile
with syscon method in FDT table, only that acpi ged pm register is accessed
with 8-bit mode, rather with 32-bit mode.

---
v1 ... v2:
  1. Modify name of macro for acpi ged register from ACPI spec, and also add
     comments for macro definition.
---
Bibo Mao (2):
  acpi: ged: Add macro for acpi sleep control register
  hw/loongarch/virt: Add FDT table support with acpi ged pm register

 hw/acpi/generic_event_device.c         |  6 ++--
 hw/i386/acpi-microvm.c                 |  2 +-
 hw/loongarch/acpi-build.c              |  2 +-
 hw/loongarch/virt.c                    | 39 ++++++++++++++++++++++++++
 include/hw/acpi/generic_event_device.h |  9 ++++--
 5 files changed, 51 insertions(+), 7 deletions(-)


base-commit: a66f28df650166ae8b50c992eea45e7b247f4143
-- 
2.39.3

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

[Bibo Mao]

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;
+        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));
     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"
-- 
2.39.3

[PATCH v2 2/2] hw/loongarch/virt: Add FDT table support with acpi ged pm register

[Bibo Mao]

ACPI ged is used for power management on LoongArch virt platform, in
general it is parsed from acpi table. However if system boot directly from
elf kernel, no UEFI bios is provided and acpi table cannot be used also.

Here acpi ged pm register is exposed with FDT table, it is compatbile
with syscon method in FDT table, only that acpi ged pm register is accessed
with 8-bit mode, rather with 32-bit mode.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 hw/loongarch/virt.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 29040422aa..e849d079cb 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -279,6 +279,44 @@ static void fdt_add_rtc_node(LoongArchVirtMachineState *lvms,
     g_free(nodename);
 }
 
+static void fdt_add_ged_reset(LoongArchVirtMachineState *lvms)
+{
+    char *name;
+    uint32_t ged_handle;
+    MachineState *ms = MACHINE(lvms);
+    hwaddr base = VIRT_GED_REG_ADDR;
+    hwaddr size = ACPI_GED_REG_COUNT;
+
+    ged_handle = qemu_fdt_alloc_phandle(ms->fdt);
+    name = g_strdup_printf("/ged@%" PRIx64, base);
+    qemu_fdt_add_subnode(ms->fdt, name);
+    qemu_fdt_setprop_string(ms->fdt, name, "compatible", "syscon");
+    qemu_fdt_setprop_cells(ms->fdt, name, "reg", 0x0, base, 0x0, size);
+    /* 8 bit registers */
+    qemu_fdt_setprop_cell(ms->fdt, name, "reg-shift", 0);
+    qemu_fdt_setprop_cell(ms->fdt, name, "reg-io-width", 1);
+    qemu_fdt_setprop_cell(ms->fdt, name, "phandle", ged_handle);
+    ged_handle = qemu_fdt_get_phandle(ms->fdt, name);
+    g_free(name);
+
+    name = g_strdup_printf("/reboot");
+    qemu_fdt_add_subnode(ms->fdt, name);
+    qemu_fdt_setprop_string(ms->fdt, name, "compatible", "syscon-reboot");
+    qemu_fdt_setprop_cell(ms->fdt, name, "regmap", ged_handle);
+    qemu_fdt_setprop_cell(ms->fdt, name, "offset", ACPI_GED_REG_RESET);
+    qemu_fdt_setprop_cell(ms->fdt, name, "value", ACPI_GED_RESET_VALUE);
+    g_free(name);
+
+    name = g_strdup_printf("/poweroff");
+    qemu_fdt_add_subnode(ms->fdt, name);
+    qemu_fdt_setprop_string(ms->fdt, name, "compatible", "syscon-poweroff");
+    qemu_fdt_setprop_cell(ms->fdt, name, "regmap", ged_handle);
+    qemu_fdt_setprop_cell(ms->fdt, name, "offset", ACPI_GED_REG_SLEEP_CTL);
+    qemu_fdt_setprop_cell(ms->fdt, name, "value", ACPI_GED_SLP_EN |
+                          (ACPI_GED_SLP_TYPx_S5 << ACPI_GED_SLP_TYPx_POS));
+    g_free(name);
+}
+
 static void fdt_add_uart_node(LoongArchVirtMachineState *lvms,
                               uint32_t *pch_pic_phandle)
 {
@@ -724,6 +762,7 @@ static void virt_devices_init(DeviceState *pch_pic,
                          qdev_get_gpio_in(pch_pic,
                          VIRT_RTC_IRQ - VIRT_GSI_BASE));
     fdt_add_rtc_node(lvms, pch_pic_phandle);
+    fdt_add_ged_reset(lvms);
 
     /* acpi ged */
     lvms->acpi_ged = create_acpi_ged(pch_pic, lvms);
-- 
2.39.3

Re: [PATCH v2 0/2] Add FDT table support with acpi ged pm register

[gaosong]

在 2024/9/11 上午11:09, Bibo Mao 写道:
> ACPI ged is used for power management on LoongArch virt platform, in
> general it is parsed from acpi table. However if system boot directly from
> elf kernel, no UEFI bios is provided and acpi table cannot be used also.
> 
> Here acpi ged pm register is exposed with FDT table, it is compatbile
> with syscon method in FDT table, only that acpi ged pm register is accessed
> with 8-bit mode, rather with 32-bit mode.
> 
> ---
> v1 ... v2:
>    1. Modify name of macro for acpi ged register from ACPI spec, and also add
>       comments for macro definition.
> ---
> Bibo Mao (2):
>    acpi: ged: Add macro for acpi sleep control register
>    hw/loongarch/virt: Add FDT table support with acpi ged pm register
> 


>   hw/acpi/generic_event_device.c         |  6 ++--
>   hw/i386/acpi-microvm.c                 |  2 +-
>   hw/loongarch/acpi-build.c              |  2 +-
>   hw/loongarch/virt.c                    | 39 ++++++++++++++++++++++++++
>   include/hw/acpi/generic_event_device.h |  9 ++++--
>   5 files changed, 51 insertions(+), 7 deletions(-)
> 
> 
> base-commit: a66f28df650166ae8b50c992eea45e7b247f4143
> Reviewed-by: Song Gao <gaosong@loongson.cn>

Applied series to loongarch-next

Thanks
Song Gao

Re: [PATCH v2 0/2] Add FDT table support with acpi ged pm register

[maobibo]

Song,

On 2024/9/12 下午7:35, gaosong wrote:
> 
> 
> 在 2024/9/11 上午11:09, Bibo Mao 写道:
>> ACPI ged is used for power management on LoongArch virt platform, in
>> general it is parsed from acpi table. However if system boot directly 
>> from
>> elf kernel, no UEFI bios is provided and acpi table cannot be used also.
>>
>> Here acpi ged pm register is exposed with FDT table, it is compatbile
>> with syscon method in FDT table, only that acpi ged pm register is 
>> accessed
>> with 8-bit mode, rather with 32-bit mode.
>>
>> ---
>> v1 ... v2:
>>    1. Modify name of macro for acpi ged register from ACPI spec, and 
>> also add
>>       comments for macro definition.
>> ---
>> Bibo Mao (2):
>>    acpi: ged: Add macro for acpi sleep control register
>>    hw/loongarch/virt: Add FDT table support with acpi ged pm register
>>
> 
> 
>>   hw/acpi/generic_event_device.c         |  6 ++--
>>   hw/i386/acpi-microvm.c                 |  2 +-
>>   hw/loongarch/acpi-build.c              |  2 +-
>>   hw/loongarch/virt.c                    | 39 ++++++++++++++++++++++++++
>>   include/hw/acpi/generic_event_device.h |  9 ++++--
>>   5 files changed, 51 insertions(+), 7 deletions(-)
>>
>>
>> base-commit: a66f28df650166ae8b50c992eea45e7b247f4143
>> Reviewed-by: Song Gao <gaosong@loongson.cn>
> 
> Applied series to loongarch-next

It is not urgent and it needs approval from other maintainers :)

Regards
Bibo Mao
> 
> Thanks
> Song Gao
> 
> 

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

[Igor Mammedov]

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.

> +        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?

>      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"

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"
> 

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

[Igor Mammedov]

On Sat, 14 Sep 2024 10:25:45 +0800
maobibo <maobibo@loongson.cn> wrote:

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

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"  
> >   
> 

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

[maobibo]

On 2024/9/17 下午3:44, Igor Mammedov wrote:
> On Sat, 14 Sep 2024 10:25:45 +0800
> maobibo <maobibo@loongson.cn> wrote:
> 
>> 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);
> 
> 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.
Got it, thanks for your detailed explanation.
Will refresh it in the next patch.

Regards
Bibo Mao
> 
> 
>>
>> 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"
>>>    
>>