Re: [RFC V2 PATCH 02/11] acpi: Implement control method sleep button

[Annie Li <annie.li@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 5420 bytes --]


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

[-- Attachment #2: Type: text/html, Size: 7704 bytes --]