On Tue, 8 Oct 2024 11:22:33 -0400
Annie Li <annie...@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...@oracle.com> wrote:
> >
> >> The control method sleep button is added, as well as its GPE event
> >> handler.
> >>
> >> Co-developed-by: Miguel Luis<miguel.l...@oracle.com>
> >> Signed-off-by: Annie Li<annie...@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...@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...@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
