| Message ID | 20240927183906.1248-3-annie.li@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support ACPI Control Method Sleep button | expand |
On Fri, 27 Sep 2024 14:38:57 -0400 Annie Li <annie.li@oracle.com> wrote: > The control method sleep button is added, as well as its GPE event > handler. > > Co-developed-by: Miguel Luis <miguel.luis@oracle.com> > Signed-off-by: Annie Li <annie.li@oracle.com> > --- > hw/acpi/control_method_device.c | 54 +++++++++++++++++++++++++ > hw/acpi/meson.build | 1 + > include/hw/acpi/control_method_device.h | 25 ++++++++++++ > 3 files changed, 80 insertions(+) > > diff --git a/hw/acpi/control_method_device.c b/hw/acpi/control_method_device.c > new file mode 100644 > index 0000000000..f52c190352 > --- /dev/null > +++ b/hw/acpi/control_method_device.c > @@ -0,0 +1,54 @@ > +/* > + * Control method devices > + * > + * Copyright (c) 2023 Oracle and/or its affiliates. > + * > + * > + * Authors: > + * Annie Li <annie.li@oracle.com> > + * > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. replace it with SPDX-License-Identifier like it's done elsewhere > + * > + */ > + > +#include "qemu/osdep.h" > +#include "hw/acpi/control_method_device.h" > +#include "hw/mem/nvdimm.h" > + > +void acpi_dsdt_add_sleep_button(Aml *scope) > +{ > + Aml *dev = aml_device("\\_SB."ACPI_SLEEP_BUTTON_DEVICE); drop "\\_SB." here and below as well, > + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C0E"))); > + Aml *pkg = aml_package(2); > + aml_append(pkg, aml_int(0x01)); > + aml_append(pkg, aml_int(0x04)); > + aml_append(dev, aml_name_decl("_PRW", pkg)); > + aml_append(dev, aml_operation_region("\\Boo", AML_SYSTEM_IO, use some sensible name for opreg > + aml_int(0x201), 0x1)); > + Aml *field = aml_field("\\Boo", AML_BYTE_ACC, AML_NOLOCK, > + AML_WRITE_AS_ZEROS); > + aml_append(field, aml_named_field("SBP", 1)); > + aml_append(field, aml_named_field("SBW", 1)); > + aml_append(dev, field); > + aml_append(scope, dev); > +} also above and below lacks any documentation, add comments for relevant spec references, like we do with other ACPI functions. Also perhaps, it's out of order, reviewer has not clue where from above registers come and how it is supposed to work. if you invented those registers, there should be a preceding doc patch that documents them. Suggest to reorder after patch that implements above registers in hw, and also comment here where to look for them. > + > +void acpi_dsdt_add_sleep_gpe_event_handler(Aml *scope) > +{ > + Aml *method = aml_method("_L07", 0, AML_NOTSERIALIZED); > + Aml *condition = aml_if(aml_name("\\_SB.SLPB.SBP")); > + aml_append(condition, aml_store(aml_int(1), aml_name("\\_SB.SLPB.SBP"))); > + aml_append(condition, > + aml_notify(aml_name("\\_SB."ACPI_SLEEP_BUTTON_DEVICE), > + aml_int(0x80))); > + aml_append(method, condition); > + condition = aml_if(aml_name("\\_SB.SLPB.SBW")); > + aml_append(condition, aml_store(aml_int(1), aml_name("\\_SB.SLPB.SBW"))); > + aml_append(condition, > + aml_notify(aml_name("\\_SB."ACPI_SLEEP_BUTTON_DEVICE), > + aml_int(0x2))); > + aml_append(method, condition); > + aml_append(scope, method); > +} > diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build > index fa5c07db90..0b4f1b432d 100644 > --- a/hw/acpi/meson.build > +++ b/hw/acpi/meson.build > @@ -17,6 +17,7 @@ acpi_ss.add(when: 'CONFIG_ACPI_PCI', if_true: files('pci.c')) > acpi_ss.add(when: 'CONFIG_ACPI_CXL', if_true: files('cxl.c'), if_false: files('cxl-stub.c')) > acpi_ss.add(when: 'CONFIG_ACPI_VMGENID', if_true: files('vmgenid.c')) > acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('generic_event_device.c')) > +acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('control_method_device.c')) > acpi_ss.add(when: 'CONFIG_ACPI_HMAT', if_true: files('hmat.c')) > acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false: files('ghes-stub.c')) > acpi_ss.add(when: 'CONFIG_ACPI_PIIX4', if_true: files('piix4.c')) > diff --git a/include/hw/acpi/control_method_device.h b/include/hw/acpi/control_method_device.h > new file mode 100644 > index 0000000000..87f8d6fd59 > --- /dev/null > +++ b/include/hw/acpi/control_method_device.h > @@ -0,0 +1,25 @@ > +/* > + * Control method devices > + * > + * Copyright (c) 2023 Oracle and/or its affiliates. > + * > + * > + * Authors: > + * Annie Li <annie.li@oracle.com> > + * > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + * > + */ > + > + > +#ifndef HW_ACPI_CONTROL_METHOD_DEVICE_H > +#define HW_ACPI_CONTROL_NETHOD_DEVICE_H > + > +#define ACPI_SLEEP_BUTTON_DEVICE "SLPB" > + > +void acpi_dsdt_add_sleep_button(Aml *scope); > +void acpi_dsdt_add_sleep_gpe_event_handler(Aml *scope); > + > +#endif
On 10/7/2024 8:59 AM, Igor Mammedov wrote: > On Fri, 27 Sep 2024 14:38:57 -0400 > Annie Li<annie.li@oracle.com> wrote: > >> The control method sleep button is added, as well as its GPE event >> handler. >> >> Co-developed-by: Miguel Luis<miguel.luis@oracle.com> >> Signed-off-by: Annie Li<annie.li@oracle.com> >> --- >> hw/acpi/control_method_device.c | 54 +++++++++++++++++++++++++ >> hw/acpi/meson.build | 1 + >> include/hw/acpi/control_method_device.h | 25 ++++++++++++ >> 3 files changed, 80 insertions(+) >> >> diff --git a/hw/acpi/control_method_device.c b/hw/acpi/control_method_device.c >> new file mode 100644 >> index 0000000000..f52c190352 >> --- /dev/null >> +++ b/hw/acpi/control_method_device.c >> @@ -0,0 +1,54 @@ >> +/* >> + * Control method devices >> + * >> + * Copyright (c) 2023 Oracle and/or its affiliates. >> + * >> + * >> + * Authors: >> + * Annie Li<annie.li@oracle.com> >> + * >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or later. >> + * See the COPYING file in the top-level directory. > replace it with SPDX-License-Identifier like it's done elsewhere Nod > >> + * >> + */ >> + >> +#include "qemu/osdep.h" >> +#include "hw/acpi/control_method_device.h" >> +#include "hw/mem/nvdimm.h" >> + >> +void acpi_dsdt_add_sleep_button(Aml *scope) >> +{ >> + Aml *dev = aml_device("\\_SB."ACPI_SLEEP_BUTTON_DEVICE); > drop "\\_SB." here and below as well, Make sense here since the scope parameter is "_SB", but for the "_SB" in acpi_dsdt_add_sleep_gpe_event_handler where the scope parameter is "_GPE", I suppose "\\_SB." is necessary. > >> + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C0E"))); >> + Aml *pkg = aml_package(2); >> + aml_append(pkg, aml_int(0x01)); >> + aml_append(pkg, aml_int(0x04)); >> + aml_append(dev, aml_name_decl("_PRW", pkg)); >> + aml_append(dev, aml_operation_region("\\Boo", AML_SYSTEM_IO, > use some sensible name for opreg will do >> + aml_int(0x201), 0x1)); >> + Aml *field = aml_field("\\Boo", AML_BYTE_ACC, AML_NOLOCK, >> + AML_WRITE_AS_ZEROS); >> + aml_append(field, aml_named_field("SBP", 1)); >> + aml_append(field, aml_named_field("SBW", 1)); >> + aml_append(dev, field); >> + aml_append(scope, dev); >> +} > also above and below lacks any documentation, > add comments for relevant spec references, like we do with other ACPI > functions. Also perhaps, it's out of order, reviewer has not clue > where from above registers come and how it is supposed to work. > > if you invented those registers, there should be a preceding doc patch > that documents them. > > Suggest to reorder after patch that implements above registers in hw, > and also comment here where to look for them. Thanks for the feedback. Thanks Annie >> + >> +void acpi_dsdt_add_sleep_gpe_event_handler(Aml *scope) >> +{ >> + Aml *method = aml_method("_L07", 0, AML_NOTSERIALIZED); >> + Aml *condition = aml_if(aml_name("\\_SB.SLPB.SBP")); >> + aml_append(condition, aml_store(aml_int(1), aml_name("\\_SB.SLPB.SBP"))); >> + aml_append(condition, >> + aml_notify(aml_name("\\_SB."ACPI_SLEEP_BUTTON_DEVICE), >> + aml_int(0x80))); >> + aml_append(method, condition); >> + condition = aml_if(aml_name("\\_SB.SLPB.SBW")); >> + aml_append(condition, aml_store(aml_int(1), aml_name("\\_SB.SLPB.SBW"))); >> + aml_append(condition, >> + aml_notify(aml_name("\\_SB."ACPI_SLEEP_BUTTON_DEVICE), >> + aml_int(0x2))); >> + aml_append(method, condition); >> + aml_append(scope, method); >> +} >> diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build >> index fa5c07db90..0b4f1b432d 100644 >> --- a/hw/acpi/meson.build >> +++ b/hw/acpi/meson.build >> @@ -17,6 +17,7 @@ acpi_ss.add(when: 'CONFIG_ACPI_PCI', if_true: files('pci.c')) >> acpi_ss.add(when: 'CONFIG_ACPI_CXL', if_true: files('cxl.c'), if_false: files('cxl-stub.c')) >> acpi_ss.add(when: 'CONFIG_ACPI_VMGENID', if_true: files('vmgenid.c')) >> acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('generic_event_device.c')) >> +acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('control_method_device.c')) >> acpi_ss.add(when: 'CONFIG_ACPI_HMAT', if_true: files('hmat.c')) >> acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false: files('ghes-stub.c')) >> acpi_ss.add(when: 'CONFIG_ACPI_PIIX4', if_true: files('piix4.c')) >> diff --git a/include/hw/acpi/control_method_device.h b/include/hw/acpi/control_method_device.h >> new file mode 100644 >> index 0000000000..87f8d6fd59 >> --- /dev/null >> +++ b/include/hw/acpi/control_method_device.h >> @@ -0,0 +1,25 @@ >> +/* >> + * Control method devices >> + * >> + * Copyright (c) 2023 Oracle and/or its affiliates. >> + * >> + * >> + * Authors: >> + * Annie Li<annie.li@oracle.com> >> + * >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or later. >> + * See the COPYING file in the top-level directory. >> + * >> + */ >> + >> + >> +#ifndef HW_ACPI_CONTROL_METHOD_DEVICE_H >> +#define HW_ACPI_CONTROL_NETHOD_DEVICE_H >> + >> +#define ACPI_SLEEP_BUTTON_DEVICE "SLPB" >> + >> +void acpi_dsdt_add_sleep_button(Aml *scope); >> +void acpi_dsdt_add_sleep_gpe_event_handler(Aml *scope); >> + >> +#endif
On Tue, 8 Oct 2024 11:22:33 -0400 Annie Li <annie.li@oracle.com> wrote: > On 10/7/2024 8:59 AM, Igor Mammedov wrote: > > On Fri, 27 Sep 2024 14:38:57 -0400 > > Annie Li<annie.li@oracle.com> wrote: > > > >> The control method sleep button is added, as well as its GPE event > >> handler. > >> > >> Co-developed-by: Miguel Luis<miguel.luis@oracle.com> > >> Signed-off-by: Annie Li<annie.li@oracle.com> > >> --- > >> hw/acpi/control_method_device.c | 54 +++++++++++++++++++++++++ > >> hw/acpi/meson.build | 1 + > >> include/hw/acpi/control_method_device.h | 25 ++++++++++++ > >> 3 files changed, 80 insertions(+) > >> > >> diff --git a/hw/acpi/control_method_device.c b/hw/acpi/control_method_device.c > >> new file mode 100644 > >> index 0000000000..f52c190352 > >> --- /dev/null > >> +++ b/hw/acpi/control_method_device.c > >> @@ -0,0 +1,54 @@ > >> +/* > >> + * Control method devices > >> + * > >> + * Copyright (c) 2023 Oracle and/or its affiliates. > >> + * > >> + * > >> + * Authors: > >> + * Annie Li<annie.li@oracle.com> > >> + * > >> + * > >> + * This work is licensed under the terms of the GNU GPL, version 2 or later. > >> + * See the COPYING file in the top-level directory. > > replace it with SPDX-License-Identifier like it's done elsewhere > Nod > > > >> + * > >> + */ > >> + > >> +#include "qemu/osdep.h" > >> +#include "hw/acpi/control_method_device.h" > >> +#include "hw/mem/nvdimm.h" > >> + > >> +void acpi_dsdt_add_sleep_button(Aml *scope) > >> +{ > >> + Aml *dev = aml_device("\\_SB."ACPI_SLEEP_BUTTON_DEVICE); > > drop "\\_SB." here and below as well, > Make sense here since the scope parameter is "_SB", but for the "_SB" > in acpi_dsdt_add_sleep_gpe_event_handler where the scope parameter is > "_GPE", I suppose "\\_SB." is necessary. Check the spec onhow name resolution in ACPI works. Anyways you likely won't have dedicated acpi_dsdt_add_sleep_gpe_event_handler(), on respin > > > >> + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C0E"))); > >> + Aml *pkg = aml_package(2); > >> + aml_append(pkg, aml_int(0x01)); > >> + aml_append(pkg, aml_int(0x04)); > >> + aml_append(dev, aml_name_decl("_PRW", pkg)); > >> + aml_append(dev, aml_operation_region("\\Boo", AML_SYSTEM_IO, > > use some sensible name for opreg > will do > >> + aml_int(0x201), 0x1)); > >> + Aml *field = aml_field("\\Boo", AML_BYTE_ACC, AML_NOLOCK, > >> + AML_WRITE_AS_ZEROS); > >> + aml_append(field, aml_named_field("SBP", 1)); > >> + aml_append(field, aml_named_field("SBW", 1)); > >> + aml_append(dev, field); > >> + aml_append(scope, dev); > >> +} > > also above and below lacks any documentation, > > add comments for relevant spec references, like we do with other ACPI > > functions. Also perhaps, it's out of order, reviewer has not clue > > where from above registers come and how it is supposed to work. > > > > if you invented those registers, there should be a preceding doc patch > > that documents them. > > > > Suggest to reorder after patch that implements above registers in hw, > > and also comment here where to look for them. > > Thanks for the feedback. > > Thanks > Annie > > >> + > >> +void acpi_dsdt_add_sleep_gpe_event_handler(Aml *scope) > >> +{ > >> + Aml *method = aml_method("_L07", 0, AML_NOTSERIALIZED); > >> + Aml *condition = aml_if(aml_name("\\_SB.SLPB.SBP")); > >> + aml_append(condition, aml_store(aml_int(1), aml_name("\\_SB.SLPB.SBP"))); > >> + aml_append(condition, > >> + aml_notify(aml_name("\\_SB."ACPI_SLEEP_BUTTON_DEVICE), > >> + aml_int(0x80))); > >> + aml_append(method, condition); > >> + condition = aml_if(aml_name("\\_SB.SLPB.SBW")); > >> + aml_append(condition, aml_store(aml_int(1), aml_name("\\_SB.SLPB.SBW"))); > >> + aml_append(condition, > >> + aml_notify(aml_name("\\_SB."ACPI_SLEEP_BUTTON_DEVICE), > >> + aml_int(0x2))); > >> + aml_append(method, condition); > >> + aml_append(scope, method); > >> +} > >> diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build > >> index fa5c07db90..0b4f1b432d 100644 > >> --- a/hw/acpi/meson.build > >> +++ b/hw/acpi/meson.build > >> @@ -17,6 +17,7 @@ acpi_ss.add(when: 'CONFIG_ACPI_PCI', if_true: files('pci.c')) > >> acpi_ss.add(when: 'CONFIG_ACPI_CXL', if_true: files('cxl.c'), if_false: files('cxl-stub.c')) > >> acpi_ss.add(when: 'CONFIG_ACPI_VMGENID', if_true: files('vmgenid.c')) > >> acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('generic_event_device.c')) > >> +acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('control_method_device.c')) > >> acpi_ss.add(when: 'CONFIG_ACPI_HMAT', if_true: files('hmat.c')) > >> acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false: files('ghes-stub.c')) > >> acpi_ss.add(when: 'CONFIG_ACPI_PIIX4', if_true: files('piix4.c')) > >> diff --git a/include/hw/acpi/control_method_device.h b/include/hw/acpi/control_method_device.h > >> new file mode 100644 > >> index 0000000000..87f8d6fd59 > >> --- /dev/null > >> +++ b/include/hw/acpi/control_method_device.h > >> @@ -0,0 +1,25 @@ > >> +/* > >> + * Control method devices > >> + * > >> + * Copyright (c) 2023 Oracle and/or its affiliates. > >> + * > >> + * > >> + * Authors: > >> + * Annie Li<annie.li@oracle.com> > >> + * > >> + * > >> + * This work is licensed under the terms of the GNU GPL, version 2 or later. > >> + * See the COPYING file in the top-level directory. > >> + * > >> + */ > >> + > >> + > >> +#ifndef HW_ACPI_CONTROL_METHOD_DEVICE_H > >> +#define HW_ACPI_CONTROL_NETHOD_DEVICE_H > >> + > >> +#define ACPI_SLEEP_BUTTON_DEVICE "SLPB" > >> + > >> +void acpi_dsdt_add_sleep_button(Aml *scope); > >> +void acpi_dsdt_add_sleep_gpe_event_handler(Aml *scope); > >> + > >> +#endif
diff --git a/hw/acpi/control_method_device.c b/hw/acpi/control_method_device.c new file mode 100644 index 0000000000..f52c190352 --- /dev/null +++ b/hw/acpi/control_method_device.c @@ -0,0 +1,54 @@ +/* + * Control method devices + * + * Copyright (c) 2023 Oracle and/or its affiliates. + * + * + * Authors: + * Annie Li <annie.li@oracle.com> + * + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ + +#include "qemu/osdep.h" +#include "hw/acpi/control_method_device.h" +#include "hw/mem/nvdimm.h" + +void acpi_dsdt_add_sleep_button(Aml *scope) +{ + Aml *dev = aml_device("\\_SB."ACPI_SLEEP_BUTTON_DEVICE); + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C0E"))); + Aml *pkg = aml_package(2); + aml_append(pkg, aml_int(0x01)); + aml_append(pkg, aml_int(0x04)); + aml_append(dev, aml_name_decl("_PRW", pkg)); + aml_append(dev, aml_operation_region("\\Boo", AML_SYSTEM_IO, + aml_int(0x201), 0x1)); + Aml *field = aml_field("\\Boo", AML_BYTE_ACC, AML_NOLOCK, + AML_WRITE_AS_ZEROS); + aml_append(field, aml_named_field("SBP", 1)); + aml_append(field, aml_named_field("SBW", 1)); + aml_append(dev, field); + aml_append(scope, dev); +} + +void acpi_dsdt_add_sleep_gpe_event_handler(Aml *scope) +{ + Aml *method = aml_method("_L07", 0, AML_NOTSERIALIZED); + Aml *condition = aml_if(aml_name("\\_SB.SLPB.SBP")); + aml_append(condition, aml_store(aml_int(1), aml_name("\\_SB.SLPB.SBP"))); + aml_append(condition, + aml_notify(aml_name("\\_SB."ACPI_SLEEP_BUTTON_DEVICE), + aml_int(0x80))); + aml_append(method, condition); + condition = aml_if(aml_name("\\_SB.SLPB.SBW")); + aml_append(condition, aml_store(aml_int(1), aml_name("\\_SB.SLPB.SBW"))); + aml_append(condition, + aml_notify(aml_name("\\_SB."ACPI_SLEEP_BUTTON_DEVICE), + aml_int(0x2))); + aml_append(method, condition); + aml_append(scope, method); +} diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build index fa5c07db90..0b4f1b432d 100644 --- a/hw/acpi/meson.build +++ b/hw/acpi/meson.build @@ -17,6 +17,7 @@ acpi_ss.add(when: 'CONFIG_ACPI_PCI', if_true: files('pci.c')) acpi_ss.add(when: 'CONFIG_ACPI_CXL', if_true: files('cxl.c'), if_false: files('cxl-stub.c')) acpi_ss.add(when: 'CONFIG_ACPI_VMGENID', if_true: files('vmgenid.c')) acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('generic_event_device.c')) +acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('control_method_device.c')) acpi_ss.add(when: 'CONFIG_ACPI_HMAT', if_true: files('hmat.c')) acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false: files('ghes-stub.c')) acpi_ss.add(when: 'CONFIG_ACPI_PIIX4', if_true: files('piix4.c')) diff --git a/include/hw/acpi/control_method_device.h b/include/hw/acpi/control_method_device.h new file mode 100644 index 0000000000..87f8d6fd59 --- /dev/null +++ b/include/hw/acpi/control_method_device.h @@ -0,0 +1,25 @@ +/* + * Control method devices + * + * Copyright (c) 2023 Oracle and/or its affiliates. + * + * + * Authors: + * Annie Li <annie.li@oracle.com> + * + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ + + +#ifndef HW_ACPI_CONTROL_METHOD_DEVICE_H +#define HW_ACPI_CONTROL_NETHOD_DEVICE_H + +#define ACPI_SLEEP_BUTTON_DEVICE "SLPB" + +void acpi_dsdt_add_sleep_button(Aml *scope); +void acpi_dsdt_add_sleep_gpe_event_handler(Aml *scope); + +#endif
The control method sleep button is added, as well as its GPE event handler. Co-developed-by: Miguel Luis <miguel.luis@oracle.com> Signed-off-by: Annie Li <annie.li@oracle.com> --- hw/acpi/control_method_device.c | 54 +++++++++++++++++++++++++ hw/acpi/meson.build | 1 + include/hw/acpi/control_method_device.h | 25 ++++++++++++ 3 files changed, 80 insertions(+)