| Message ID | 20231231163322.9492-5-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | x86: atom-punit/-pmc s2idle device state checks | expand |
On Sun, Dec 31, 2023 at 6:33 PM Hans de Goede <hdegoede@redhat.com> wrote: > > From: Johannes Stezenbach <js@sig21.net> > > This is a port of "pm: Add pm suspend debug notifier for North IPs" > from the latte-l-oss branch of: > from https://github.com/MiCode/Xiaomi_Kernel_OpenSource latte-l-oss > > With the new acpi_s2idle_dev_ops and acpi_register_lps0_dev() > functionality this can now finally be ported to the mainline kernel > without requiring adding non-upstreamable hooks into the cpu_idle > driver mechanism. > > This adds a check that all hardware blocks in the North complex > (controlled by Punit) are in a state that allows the SoC to enter S0i3 > and prints an error message for any device in D0. ... > static void punit_dbgfs_register(struct punit_device *punit_device) > { > + punit_dev = punit_device; This is not the correct (semantically) place for this. Instead, optionally introduce a local variable in the punit_atom_debug_init() and assign the global one there. Also it seems that you may move this global variable under ifdeffery (and hence its assignment) and have less stale bytes in the object file. (With this said, it seems that local variables are plausible to have.) > }
Hi Andy, On 1/2/24 01:07, Andy Shevchenko wrote: > On Sun, Dec 31, 2023 at 6:33 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> From: Johannes Stezenbach <js@sig21.net> >> >> This is a port of "pm: Add pm suspend debug notifier for North IPs" >> from the latte-l-oss branch of: >> from https://github.com/MiCode/Xiaomi_Kernel_OpenSource latte-l-oss >> >> With the new acpi_s2idle_dev_ops and acpi_register_lps0_dev() >> functionality this can now finally be ported to the mainline kernel >> without requiring adding non-upstreamable hooks into the cpu_idle >> driver mechanism. >> >> This adds a check that all hardware blocks in the North complex >> (controlled by Punit) are in a state that allows the SoC to enter S0i3 >> and prints an error message for any device in D0. > > ... > >> static void punit_dbgfs_register(struct punit_device *punit_device) >> { >> + punit_dev = punit_device; > > This is not the correct (semantically) place for this. > > Instead, optionally introduce a local variable in the > punit_atom_debug_init() and assign the global one there. Also it seems > that you may move this global variable under ifdeffery (and hence its > assignment) and have less stale bytes in the object file. (With this > said, it seems that local variables are plausible to have.) Thank you for the reviews. I agree with all your review remarks and I'll submit a v2 series addressing all of them soon. Regards, Hans
diff --git a/arch/x86/platform/atom/punit_atom_debug.c b/arch/x86/platform/atom/punit_atom_debug.c index f8ed5f66cd20..efa4b188300d 100644 --- a/arch/x86/platform/atom/punit_atom_debug.c +++ b/arch/x86/platform/atom/punit_atom_debug.c @@ -7,6 +7,9 @@ * Copyright (c) 2015, Intel Corporation. */ +#define pr_fmt(fmt) "punit_atom: " fmt + +#include <linux/acpi.h> #include <linux/module.h> #include <linux/init.h> #include <linux/device.h> @@ -102,10 +105,12 @@ static int punit_dev_state_show(struct seq_file *seq_file, void *unused) } DEFINE_SHOW_ATTRIBUTE(punit_dev_state); +static const struct punit_device *punit_dev; static struct dentry *punit_dbg_file; static void punit_dbgfs_register(struct punit_device *punit_device) { + punit_dev = punit_device; punit_dbg_file = debugfs_create_dir("punit_atom", NULL); debugfs_create_file("dev_power_state", 0444, punit_dbg_file, @@ -117,6 +122,35 @@ static void punit_dbgfs_unregister(void) debugfs_remove_recursive(punit_dbg_file); } +#if defined(CONFIG_ACPI) && defined(CONFIG_SUSPEND) +static void punit_s2idle_check(void) +{ + const struct punit_device *punit_devp; + u32 punit_pwr_status, dstate; + int status; + + for (punit_devp = punit_dev; punit_devp->name; punit_devp++) { + /* Skip MIO this is on till the very last moment */ + if (punit_devp->reg == MIO_SS_PM) + continue; + + status = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, + punit_devp->reg, &punit_pwr_status); + if (status) { + pr_err("%s read failed\n", punit_devp->name); + } else { + dstate = (punit_pwr_status >> punit_devp->sss_pos) & 3; + if (!dstate) + pr_err("%s is in D0 prior to s2idle\n", punit_devp->name); + } + } +} + +static struct acpi_s2idle_dev_ops punit_s2idle_ops = { + .check = punit_s2idle_check, +}; +#endif + #define X86_MATCH(model, data) \ X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6, INTEL_FAM6_##model, \ X86_FEATURE_MWAIT, data) @@ -138,12 +172,18 @@ static int __init punit_atom_debug_init(void) return -ENODEV; punit_dbgfs_register((struct punit_device *)id->driver_data); +#if defined(CONFIG_ACPI) && defined(CONFIG_SUSPEND) + acpi_register_lps0_dev(&punit_s2idle_ops); +#endif return 0; } static void __exit punit_atom_debug_exit(void) { +#if defined(CONFIG_ACPI) && defined(CONFIG_SUSPEND) + acpi_unregister_lps0_dev(&punit_s2idle_ops); +#endif punit_dbgfs_unregister(); }