Re: [PATCH v2 5/5] x86/platform/atom: Check state of Punit managed devices on s2idle

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Sun, 7 Jan 2024, Hans de Goede 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.

Same problems with the commit message as in one of the earlier patches.

> 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.
>
> Signed-off-by: Johannes Stezenbach <js@sig21.net>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> [hdegoede: Use acpi_s2idle_dev_ops]
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  arch/x86/platform/atom/punit_atom_debug.c | 45 ++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/atom/punit_atom_debug.c b/arch/x86/platform/atom/punit_atom_debug.c
> index f8ed5f66cd20..03ae08c99155 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>
> @@ -117,6 +120,37 @@ static void punit_dbgfs_unregister(void)
>  	debugfs_remove_recursive(punit_dbg_file);
>  }
>  
> +#if defined(CONFIG_ACPI) && defined(CONFIG_SUSPEND)
> +static const struct punit_device *punit_dev;
> +
> +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 */

Skip MIO, it is on ...

> +		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)
> @@ -131,19 +165,28 @@ MODULE_DEVICE_TABLE(x86cpu, intel_punit_cpu_ids);
>  
>  static int __init punit_atom_debug_init(void)
>  {
> +	struct punit_device *punit_device;
>  	const struct x86_cpu_id *id;
>  
>  	id = x86_match_cpu(intel_punit_cpu_ids);
>  	if (!id)
>  		return -ENODEV;
>  
> -	punit_dbgfs_register((struct punit_device *)id->driver_data);
> +	punit_device = (struct punit_device *)id->driver_data;
> +	punit_dbgfs_register(punit_device);
> +#if defined(CONFIG_ACPI) && defined(CONFIG_SUSPEND)
> +	punit_dev = punit_device;
> +	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

I'd move these into new functions in the existing #if defined(CONFIG_ACPI) 
&& defined(CONFIG_SUSPEND) block and add #else block which has dummy 
functions for them.

>  	punit_dbgfs_unregister();
>  }
>  
> 

-- 
 i.