[PATCH v2 0/5] x86: atom-punit/-pmc s2idle device state checks

[PATCH v2 0/5] x86: atom-punit/-pmc s2idle device state checks

[Hans de Goede]

Hi All,

These patches are an upstream submission of a patch titled:
"Intel Atom suspend: add debug check for S0ix blockers"

Which I have been carrying in my personal kernel tree for years now.
This code originally comes from the latte-l-oss branch of:
https://github.com/MiCode/Xiaomi_Kernel_OpenSource

And has been posted on upstream mailinglists before by
Johannes Stezenbach, whose authorship I have kept for
the 2 base patches and has been reposted by Takashi Iwai
and at one point in time I picked this up from Takashi's
reposting as can be seen from the S-o-b lines. Unfortunately
I cannot find the original postings, so I have no link to
those.

The original version of this added some ugly hooks into
the intel_idle driver which I presume is why these patches
never got anywhere upstream.

With the new acpi_s2idle_dev_ops and acpi_register_lps0_dev()
functionality this functionality can now be implemented cleanly
and that is what this patch-series does.

clk and x86/tip maintainers, it is probably the cleanest if this
entire series is merged through the pdx86 tree (*). Can we have
your ack for merging patch 1/5 resp. 5/5 through the pdx86 tree ?

Regards,

Hans

*) Andy recently mentioned that it might be a good idea to move
some of the arch/x86/platform code to drivers/platform/x86,
arch/x86/platform/atom/punit_atom_debug.c which is a completely
standalone driver definitly is a good candidate for this



Hans de Goede (3):
  clk: x86: Move clk-pmc-atom register defines to
    include/linux/platform_data/x86/pmc_atom.h
  platform/x86: pmc_atom: Annotate d3_sts register bit defines
  platform/x86: pmc_atom: Check state of PMC clocks on s2idle

Johannes Stezenbach (2):
  platform/x86: pmc_atom: Check state of PMC managed devices on s2idle
  x86/platform/atom: Check state of Punit managed devices on s2idle

 arch/x86/platform/atom/punit_atom_debug.c  | 45 ++++++++++++-
 drivers/clk/x86/clk-pmc-atom.c             | 13 +---
 drivers/platform/x86/pmc_atom.c            | 78 ++++++++++++++++++++++
 include/linux/platform_data/x86/pmc_atom.h | 25 +++++--
 4 files changed, 142 insertions(+), 19 deletions(-)

-- 
2.43.0

[PATCH v2 1/5] clk: x86: Move clk-pmc-atom register defines to include/linux/platform_data/x86/pmc_atom.h

[Hans de Goede]

Move the register defines for the Atom (Bay Trail, Cherry Trail) PMC
clocks to include/linux/platform_data/x86/pmc_atom.h.

This is a preparation patch to extend the S0i3 readiness checks
in drivers/platform/x86/pmc_atom.c with checking that the PMC
clocks are off on suspend entry.

Note these are added to include/linux/platform_data/x86/pmc_atom.h rather
then to include/linux/platform_data/x86/clk-pmc-atom.h because the former
already has all the other Atom PMC register defines.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/clk/x86/clk-pmc-atom.c             | 13 +------------
 include/linux/platform_data/x86/pmc_atom.h | 13 +++++++++++++
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c
index 2974dd0ec6f4..5ec9255e33fa 100644
--- a/drivers/clk/x86/clk-pmc-atom.c
+++ b/drivers/clk/x86/clk-pmc-atom.c
@@ -11,23 +11,12 @@
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/platform_data/x86/clk-pmc-atom.h>
+#include <linux/platform_data/x86/pmc_atom.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
 #define PLT_CLK_NAME_BASE	"pmc_plt_clk"
 
-#define PMC_CLK_CTL_OFFSET		0x60
-#define PMC_CLK_CTL_SIZE		4
-#define PMC_CLK_NUM			6
-#define PMC_CLK_CTL_GATED_ON_D3		0x0
-#define PMC_CLK_CTL_FORCE_ON		0x1
-#define PMC_CLK_CTL_FORCE_OFF		0x2
-#define PMC_CLK_CTL_RESERVED		0x3
-#define PMC_MASK_CLK_CTL		GENMASK(1, 0)
-#define PMC_MASK_CLK_FREQ		BIT(2)
-#define PMC_CLK_FREQ_XTAL		(0 << 2)	/* 25 MHz */
-#define PMC_CLK_FREQ_PLL		(1 << 2)	/* 19.2 MHz */
-
 struct clk_plt_fixed {
 	struct clk_hw *clk;
 	struct clk_lookup *lookup;
diff --git a/include/linux/platform_data/x86/pmc_atom.h b/include/linux/platform_data/x86/pmc_atom.h
index b8a701c77fd0..557622ef0390 100644
--- a/include/linux/platform_data/x86/pmc_atom.h
+++ b/include/linux/platform_data/x86/pmc_atom.h
@@ -43,6 +43,19 @@
 				BIT_ORED_DEDICATED_IRQ_GPSC | \
 				BIT_SHARED_IRQ_GPSS)
 
+/* External clk generator settings */
+#define PMC_CLK_CTL_OFFSET		0x60
+#define PMC_CLK_CTL_SIZE		4
+#define PMC_CLK_NUM			6
+#define PMC_CLK_CTL_GATED_ON_D3		0x0
+#define PMC_CLK_CTL_FORCE_ON		0x1
+#define PMC_CLK_CTL_FORCE_OFF		0x2
+#define PMC_CLK_CTL_RESERVED		0x3
+#define PMC_MASK_CLK_CTL		GENMASK(1, 0)
+#define PMC_MASK_CLK_FREQ		BIT(2)
+#define PMC_CLK_FREQ_XTAL		(0 << 2)	/* 25 MHz */
+#define PMC_CLK_FREQ_PLL		(1 << 2)	/* 19.2 MHz */
+
 /* The timers accumulate time spent in sleep state */
 #define	PMC_S0IR_TMR		0x80
 #define	PMC_S0I1_TMR		0x84
-- 
2.43.0

Re: [PATCH v2 1/5] clk: x86: Move clk-pmc-atom register defines to include/linux/platform_data/x86/pmc_atom.h

[Ilpo Järvinen]

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

On Sun, 7 Jan 2024, Hans de Goede wrote:

> Move the register defines for the Atom (Bay Trail, Cherry Trail) PMC
> clocks to include/linux/platform_data/x86/pmc_atom.h.
> 
> This is a preparation patch to extend the S0i3 readiness checks
> in drivers/platform/x86/pmc_atom.c with checking that the PMC
> clocks are off on suspend entry.
> 
> Note these are added to include/linux/platform_data/x86/pmc_atom.h rather
> then to include/linux/platform_data/x86/clk-pmc-atom.h because the former
> already has all the other Atom PMC register defines.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/clk/x86/clk-pmc-atom.c             | 13 +------------
>  include/linux/platform_data/x86/pmc_atom.h | 13 +++++++++++++
>  2 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c
> index 2974dd0ec6f4..5ec9255e33fa 100644
> --- a/drivers/clk/x86/clk-pmc-atom.c
> +++ b/drivers/clk/x86/clk-pmc-atom.c
> @@ -11,23 +11,12 @@
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/platform_data/x86/clk-pmc-atom.h>
> +#include <linux/platform_data/x86/pmc_atom.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
>  #define PLT_CLK_NAME_BASE	"pmc_plt_clk"
>  
> -#define PMC_CLK_CTL_OFFSET		0x60
> -#define PMC_CLK_CTL_SIZE		4
> -#define PMC_CLK_NUM			6
> -#define PMC_CLK_CTL_GATED_ON_D3		0x0
> -#define PMC_CLK_CTL_FORCE_ON		0x1
> -#define PMC_CLK_CTL_FORCE_OFF		0x2
> -#define PMC_CLK_CTL_RESERVED		0x3
> -#define PMC_MASK_CLK_CTL		GENMASK(1, 0)
> -#define PMC_MASK_CLK_FREQ		BIT(2)
> -#define PMC_CLK_FREQ_XTAL		(0 << 2)	/* 25 MHz */
> -#define PMC_CLK_FREQ_PLL		(1 << 2)	/* 19.2 MHz */
> -
>  struct clk_plt_fixed {
>  	struct clk_hw *clk;
>  	struct clk_lookup *lookup;
> diff --git a/include/linux/platform_data/x86/pmc_atom.h b/include/linux/platform_data/x86/pmc_atom.h
> index b8a701c77fd0..557622ef0390 100644
> --- a/include/linux/platform_data/x86/pmc_atom.h
> +++ b/include/linux/platform_data/x86/pmc_atom.h
> @@ -43,6 +43,19 @@
>  				BIT_ORED_DEDICATED_IRQ_GPSC | \
>  				BIT_SHARED_IRQ_GPSS)
>  
> +/* External clk generator settings */
> +#define PMC_CLK_CTL_OFFSET		0x60
> +#define PMC_CLK_CTL_SIZE		4
> +#define PMC_CLK_NUM			6
> +#define PMC_CLK_CTL_GATED_ON_D3		0x0
> +#define PMC_CLK_CTL_FORCE_ON		0x1
> +#define PMC_CLK_CTL_FORCE_OFF		0x2
> +#define PMC_CLK_CTL_RESERVED		0x3
> +#define PMC_MASK_CLK_CTL		GENMASK(1, 0)
> +#define PMC_MASK_CLK_FREQ		BIT(2)
> +#define PMC_CLK_FREQ_XTAL		(0 << 2)	/* 25 MHz */
> +#define PMC_CLK_FREQ_PLL		(1 << 2)	/* 19.2 MHz */

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Noting the last two look like:
#define PMC_CLK_FREQ_XTAL	FIELD_PREP(PMC_MASK_CLK_FREQ, 0) /* 25 MHz */
#define PMC_CLK_FREQ_PLL	FIELD_PREP(PMC_MASK_CLK_FREQ, 1) /* 19.2 MHz */


-- 
 i.

Re: [PATCH v2 1/5] clk: x86: Move clk-pmc-atom register defines to include/linux/platform_data/x86/pmc_atom.h

[Stephen Boyd]

Quoting Hans de Goede (2024-01-07 06:03:06)
> Move the register defines for the Atom (Bay Trail, Cherry Trail) PMC
> clocks to include/linux/platform_data/x86/pmc_atom.h.
> 
> This is a preparation patch to extend the S0i3 readiness checks
> in drivers/platform/x86/pmc_atom.c with checking that the PMC
> clocks are off on suspend entry.
> 
> Note these are added to include/linux/platform_data/x86/pmc_atom.h rather
> then to include/linux/platform_data/x86/clk-pmc-atom.h because the former
> already has all the other Atom PMC register defines.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

[PATCH v2 2/5] platform/x86: pmc_atom: Annotate d3_sts register bit defines

[Hans de Goede]

The include/linux/platform_data/x86/pmc_atom.h d3_sts register bit defines
are named after how these bits are used on Bay Trail devices.

On Cherry Trail (CHT) devices some of these bits have a different meaning
according to the datasheet.

At a comment to the defines for bits which have a different meaning
on Cherry Trail devices.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 include/linux/platform_data/x86/pmc_atom.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/platform_data/x86/pmc_atom.h b/include/linux/platform_data/x86/pmc_atom.h
index 557622ef0390..161e4bc1c9ee 100644
--- a/include/linux/platform_data/x86/pmc_atom.h
+++ b/include/linux/platform_data/x86/pmc_atom.h
@@ -117,14 +117,14 @@
 #define	BIT_SCC_SDIO		BIT(9)
 #define	BIT_SCC_SDCARD		BIT(10)
 #define	BIT_SCC_MIPI		BIT(11)
-#define	BIT_HDA			BIT(12)
+#define	BIT_HDA			BIT(12) /* CHT datasheet: reserved */
 #define	BIT_LPE			BIT(13)
 #define	BIT_OTG			BIT(14)
-#define	BIT_USH			BIT(15)
-#define	BIT_GBE			BIT(16)
-#define	BIT_SATA		BIT(17)
-#define	BIT_USB_EHCI		BIT(18)
-#define	BIT_SEC			BIT(19)
+#define	BIT_USH			BIT(15) /* CHT datasheet: reserved */
+#define	BIT_GBE			BIT(16) /* CHT datasheet: reserved */
+#define	BIT_SATA		BIT(17) /* CHT datasheet: reserved */
+#define	BIT_USB_EHCI		BIT(18) /* CHT datasheet: XHCI!    */
+#define	BIT_SEC			BIT(19) /* BYT datasheet: reserved */
 #define	BIT_PCIE_PORT0		BIT(20)
 #define	BIT_PCIE_PORT1		BIT(21)
 #define	BIT_PCIE_PORT2		BIT(22)
-- 
2.43.0

[PATCH v2 3/5] platform/x86: pmc_atom: Check state of PMC managed devices on s2idle

[Hans de Goede]

From: Johannes Stezenbach <js@sig21.net>

This is a port of "pm: Add pm suspend debug notifier for South 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 South complex
(controlled by PMC) are in a state that allows the SoC to enter S0i3
and prints an error message for any device in D0.

Note the pmc_atom code is enabled by CONFIG_X86_INTEL_LPSS which
already depends on ACPI.

Signed-off-by: Johannes Stezenbach <js@sig21.net>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
[hdegoede: Use acpi_s2idle_dev_ops, ignore fused off blocks, PMIC I2C]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Drop duplicated "pmc_atom: " prefix from pr_err() / pr_dbg() messages
---
 drivers/platform/x86/pmc_atom.c | 67 +++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
index 93a6414c6611..81ad66117365 100644
--- a/drivers/platform/x86/pmc_atom.c
+++ b/drivers/platform/x86/pmc_atom.c
@@ -6,6 +6,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/acpi.h>
 #include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
@@ -17,6 +18,7 @@
 #include <linux/platform_device.h>
 #include <linux/pci.h>
 #include <linux/seq_file.h>
+#include <linux/suspend.h>
 
 struct pmc_bit_map {
 	const char *name;
@@ -448,6 +450,67 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap,
 	return 0;
 }
 
+#ifdef CONFIG_SUSPEND
+static void pmc_dev_state_check(u32 sts, const struct pmc_bit_map *sts_map,
+				u32 fd, const struct pmc_bit_map *fd_map,
+				u32 sts_possible_false_pos)
+{
+	int index;
+
+	for (index = 0; sts_map[index].name; index++) {
+		if (!(fd_map[index].bit_mask & fd) &&
+		    !(sts_map[index].bit_mask & sts)) {
+			if (sts_map[index].bit_mask & sts_possible_false_pos)
+				pm_pr_dbg("%s is in D0 prior to s2idle\n",
+					  sts_map[index].name);
+			else
+				pr_err("%s is in D0 prior to s2idle\n",
+				       sts_map[index].name);
+		}
+	}
+}
+
+static void pmc_s2idle_check(void)
+{
+	struct pmc_dev *pmc = &pmc_device;
+	const struct pmc_reg_map *m = pmc->map;
+	u32 func_dis, func_dis_2;
+	u32 d3_sts_0, d3_sts_1;
+	u32 false_pos_sts_0, false_pos_sts_1;
+
+	func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
+	func_dis_2 = pmc_reg_read(pmc, PMC_FUNC_DIS_2);
+	d3_sts_0 = pmc_reg_read(pmc, PMC_D3_STS_0);
+	d3_sts_1 = pmc_reg_read(pmc, PMC_D3_STS_1);
+
+	/*
+	 * Some blocks are not used on lower-featured versions of the SoC and
+	 * always report D0, add these to false_pos mask to log at debug level.
+	 */
+	if (m->d3_sts_1	== byt_d3_sts_1_map) {
+		/* BYT */
+		false_pos_sts_0 = BIT_GBE | BIT_SATA | BIT_PCIE_PORT0 |
+			BIT_PCIE_PORT1 | BIT_PCIE_PORT2 | BIT_PCIE_PORT3 |
+			BIT_LPSS2_F5_I2C5;
+		false_pos_sts_1 = BIT_SMB | BIT_USH_SS_PHY | BIT_DFX;
+	} else {
+		/* CHT */
+		false_pos_sts_0 = BIT_GBE | BIT_SATA | BIT_LPSS2_F7_I2C7;
+		false_pos_sts_1 = BIT_SMB | BIT_STS_ISH;
+	}
+
+	/* Low part */
+	pmc_dev_state_check(d3_sts_0, m->d3_sts_0, func_dis, m->func_dis, false_pos_sts_0);
+
+	/* High part */
+	pmc_dev_state_check(d3_sts_1, m->d3_sts_1, func_dis_2, m->func_dis_2, false_pos_sts_1);
+}
+
+static struct acpi_s2idle_dev_ops pmc_s2idle_ops = {
+	.check = pmc_s2idle_check,
+};
+#endif
+
 static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	struct pmc_dev *pmc = &pmc_device;
@@ -485,6 +548,10 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
 		dev_warn(&pdev->dev, "platform clocks register failed: %d\n",
 			 ret);
 
+#ifdef CONFIG_SUSPEND
+	acpi_register_lps0_dev(&pmc_s2idle_ops);
+#endif
+
 	pmc->init = true;
 	return ret;
 }
-- 
2.43.0

Re: [PATCH v2 3/5] platform/x86: pmc_atom: Check state of PMC managed devices on s2idle

[Ilpo Järvinen]

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 South IPs"
> from the latte-l-oss branch of:
> from https://github.com/MiCode/Xiaomi_Kernel_OpenSource latte-l-oss

If this is to be included at all, don't place it first but last in the 
commit message. That is, focus on the change itself, not where the patch 
came from.

> With the new acpi_s2idle_dev_ops and acpi_register_lps0_dev()
> functionality this can now finally be ported to the mainline kernel

What is "this" (and no, don't point it to some external patch in some 
random branch).

> without requiring adding non-upstreamable hooks into the cpu_idle
> driver mechanism.

Somehow this entire paragraph (and the one preceeding it) has a flawed way 
to look things, it focuses on stuff that seems largely irrelevant. 
Instead, there should be "a problem statement", what is problem this patch 
is addressing / why.

> This adds a check that all hardware blocks in the South complex
> (controlled by PMC) are in a state that allows the SoC to enter S0i3
> and prints an error message for any device in D0.
> 
> Note the pmc_atom code is enabled by CONFIG_X86_INTEL_LPSS which
> already depends on ACPI.
> 
> Signed-off-by: Johannes Stezenbach <js@sig21.net>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> [hdegoede: Use acpi_s2idle_dev_ops, ignore fused off blocks, PMIC I2C]
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - Drop duplicated "pmc_atom: " prefix from pr_err() / pr_dbg() messages
> ---
>  drivers/platform/x86/pmc_atom.c | 67 +++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
> index 93a6414c6611..81ad66117365 100644
> --- a/drivers/platform/x86/pmc_atom.c
> +++ b/drivers/platform/x86/pmc_atom.c
> @@ -6,6 +6,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +#include <linux/acpi.h>
>  #include <linux/debugfs.h>
>  #include <linux/device.h>
>  #include <linux/dmi.h>
> @@ -17,6 +18,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pci.h>
>  #include <linux/seq_file.h>
> +#include <linux/suspend.h>
>  
>  struct pmc_bit_map {
>  	const char *name;
> @@ -448,6 +450,67 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_SUSPEND
> +static void pmc_dev_state_check(u32 sts, const struct pmc_bit_map *sts_map,
> +				u32 fd, const struct pmc_bit_map *fd_map,
> +				u32 sts_possible_false_pos)
> +{
> +	int index;
> +
> +	for (index = 0; sts_map[index].name; index++) {
> +		if (!(fd_map[index].bit_mask & fd) &&
> +		    !(sts_map[index].bit_mask & sts)) {
> +			if (sts_map[index].bit_mask & sts_possible_false_pos)
> +				pm_pr_dbg("%s is in D0 prior to s2idle\n",
> +					  sts_map[index].name);
> +			else
> +				pr_err("%s is in D0 prior to s2idle\n",
> +				       sts_map[index].name);
> +		}
> +	}
> +}
> +
> +static void pmc_s2idle_check(void)
> +{
> +	struct pmc_dev *pmc = &pmc_device;
> +	const struct pmc_reg_map *m = pmc->map;
> +	u32 func_dis, func_dis_2;
> +	u32 d3_sts_0, d3_sts_1;
> +	u32 false_pos_sts_0, false_pos_sts_1;
> +
> +	func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
> +	func_dis_2 = pmc_reg_read(pmc, PMC_FUNC_DIS_2);
> +	d3_sts_0 = pmc_reg_read(pmc, PMC_D3_STS_0);
> +	d3_sts_1 = pmc_reg_read(pmc, PMC_D3_STS_1);
> +
> +	/*
> +	 * Some blocks are not used on lower-featured versions of the SoC and
> +	 * always report D0, add these to false_pos mask to log at debug level.

Please explain this also in the commit message.

> +	 */
> +	if (m->d3_sts_1	== byt_d3_sts_1_map) {
> +		/* BYT */

Can these be written open into the longer form.

> +		false_pos_sts_0 = BIT_GBE | BIT_SATA | BIT_PCIE_PORT0 |
> +			BIT_PCIE_PORT1 | BIT_PCIE_PORT2 | BIT_PCIE_PORT3 |
> +			BIT_LPSS2_F5_I2C5;
> +		false_pos_sts_1 = BIT_SMB | BIT_USH_SS_PHY | BIT_DFX;
> +	} else {
> +		/* CHT */
> +		false_pos_sts_0 = BIT_GBE | BIT_SATA | BIT_LPSS2_F7_I2C7;
> +		false_pos_sts_1 = BIT_SMB | BIT_STS_ISH;
> +	}

Perhaps move common bits out of the if blocks?

> +
> +	/* Low part */
> +	pmc_dev_state_check(d3_sts_0, m->d3_sts_0, func_dis, m->func_dis, false_pos_sts_0);
> +
> +	/* High part */
> +	pmc_dev_state_check(d3_sts_1, m->d3_sts_1, func_dis_2, m->func_dis_2, false_pos_sts_1);

The variables are called _0 and _1 but the comment talks about "low" and 
"high", could these be made consistent such that variabless end into _lo & 
_hi ?

After making that change, I don't think those comments add any value any 
further value to what is already plainly visible from the code itself.

> +}
> +
> +static struct acpi_s2idle_dev_ops pmc_s2idle_ops = {
> +	.check = pmc_s2idle_check,
> +};
> +#endif
> +
>  static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	struct pmc_dev *pmc = &pmc_device;
> @@ -485,6 +548,10 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		dev_warn(&pdev->dev, "platform clocks register failed: %d\n",
>  			 ret);
>  
> +#ifdef CONFIG_SUSPEND
> +	acpi_register_lps0_dev(&pmc_s2idle_ops);
> +#endif
> +
>  	pmc->init = true;
>  	return ret;
>  }
> 

-- 
 i.

Re: [PATCH v2 3/5] platform/x86: pmc_atom: Check state of PMC managed devices on s2idle

[Hans de Goede]

Hi,

On 1/8/24 12:18, Ilpo Järvinen wrote:
> 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 South IPs"
>> from the latte-l-oss branch of:
>> from https://github.com/MiCode/Xiaomi_Kernel_OpenSource latte-l-oss
> 
> If this is to be included at all, don't place it first but last in the 
> commit message. That is, focus on the change itself, not where the patch 
> came from.
> 
>> With the new acpi_s2idle_dev_ops and acpi_register_lps0_dev()
>> functionality this can now finally be ported to the mainline kernel
> 
> What is "this" (and no, don't point it to some external patch in some 
> random branch).
> 
>> without requiring adding non-upstreamable hooks into the cpu_idle
>> driver mechanism.
> 
> Somehow this entire paragraph (and the one preceeding it) has a flawed way 
> to look things, it focuses on stuff that seems largely irrelevant. 
> Instead, there should be "a problem statement", what is problem this patch 
> is addressing / why.

You are right this really belongs in the cover-letter which already
has it. I have pretty much entirely rewritten the commit message
for the upcoming v3 of this.

Regards,

Hans



> 
>> This adds a check that all hardware blocks in the South complex
>> (controlled by PMC) are in a state that allows the SoC to enter S0i3
>> and prints an error message for any device in D0.
>>
>> Note the pmc_atom code is enabled by CONFIG_X86_INTEL_LPSS which
>> already depends on ACPI.
>>
>> Signed-off-by: Johannes Stezenbach <js@sig21.net>
>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>> [hdegoede: Use acpi_s2idle_dev_ops, ignore fused off blocks, PMIC I2C]
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> - Drop duplicated "pmc_atom: " prefix from pr_err() / pr_dbg() messages
>> ---
>>  drivers/platform/x86/pmc_atom.c | 67 +++++++++++++++++++++++++++++++++
>>  1 file changed, 67 insertions(+)
>>
>> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
>> index 93a6414c6611..81ad66117365 100644
>> --- a/drivers/platform/x86/pmc_atom.c
>> +++ b/drivers/platform/x86/pmc_atom.c
>> @@ -6,6 +6,7 @@
>>  
>>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>  
>> +#include <linux/acpi.h>
>>  #include <linux/debugfs.h>
>>  #include <linux/device.h>
>>  #include <linux/dmi.h>
>> @@ -17,6 +18,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/pci.h>
>>  #include <linux/seq_file.h>
>> +#include <linux/suspend.h>
>>  
>>  struct pmc_bit_map {
>>  	const char *name;
>> @@ -448,6 +450,67 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap,
>>  	return 0;
>>  }
>>  
>> +#ifdef CONFIG_SUSPEND
>> +static void pmc_dev_state_check(u32 sts, const struct pmc_bit_map *sts_map,
>> +				u32 fd, const struct pmc_bit_map *fd_map,
>> +				u32 sts_possible_false_pos)
>> +{
>> +	int index;
>> +
>> +	for (index = 0; sts_map[index].name; index++) {
>> +		if (!(fd_map[index].bit_mask & fd) &&
>> +		    !(sts_map[index].bit_mask & sts)) {
>> +			if (sts_map[index].bit_mask & sts_possible_false_pos)
>> +				pm_pr_dbg("%s is in D0 prior to s2idle\n",
>> +					  sts_map[index].name);
>> +			else
>> +				pr_err("%s is in D0 prior to s2idle\n",
>> +				       sts_map[index].name);
>> +		}
>> +	}
>> +}
>> +
>> +static void pmc_s2idle_check(void)
>> +{
>> +	struct pmc_dev *pmc = &pmc_device;
>> +	const struct pmc_reg_map *m = pmc->map;
>> +	u32 func_dis, func_dis_2;
>> +	u32 d3_sts_0, d3_sts_1;
>> +	u32 false_pos_sts_0, false_pos_sts_1;
>> +
>> +	func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
>> +	func_dis_2 = pmc_reg_read(pmc, PMC_FUNC_DIS_2);
>> +	d3_sts_0 = pmc_reg_read(pmc, PMC_D3_STS_0);
>> +	d3_sts_1 = pmc_reg_read(pmc, PMC_D3_STS_1);
>> +
>> +	/*
>> +	 * Some blocks are not used on lower-featured versions of the SoC and
>> +	 * always report D0, add these to false_pos mask to log at debug level.
> 
> Please explain this also in the commit message.
> 
>> +	 */
>> +	if (m->d3_sts_1	== byt_d3_sts_1_map) {
>> +		/* BYT */
> 
> Can these be written open into the longer form.
> 
>> +		false_pos_sts_0 = BIT_GBE | BIT_SATA | BIT_PCIE_PORT0 |
>> +			BIT_PCIE_PORT1 | BIT_PCIE_PORT2 | BIT_PCIE_PORT3 |
>> +			BIT_LPSS2_F5_I2C5;
>> +		false_pos_sts_1 = BIT_SMB | BIT_USH_SS_PHY | BIT_DFX;
>> +	} else {
>> +		/* CHT */
>> +		false_pos_sts_0 = BIT_GBE | BIT_SATA | BIT_LPSS2_F7_I2C7;
>> +		false_pos_sts_1 = BIT_SMB | BIT_STS_ISH;
>> +	}
> 
> Perhaps move common bits out of the if blocks?
> 
>> +
>> +	/* Low part */
>> +	pmc_dev_state_check(d3_sts_0, m->d3_sts_0, func_dis, m->func_dis, false_pos_sts_0);
>> +
>> +	/* High part */
>> +	pmc_dev_state_check(d3_sts_1, m->d3_sts_1, func_dis_2, m->func_dis_2, false_pos_sts_1);
> 
> The variables are called _0 and _1 but the comment talks about "low" and 
> "high", could these be made consistent such that variabless end into _lo & 
> _hi ?
> 
> After making that change, I don't think those comments add any value any 
> further value to what is already plainly visible from the code itself.

Ack, I'll replace the _0 and _1 with _lo and _hi.

Regards,

Hans



> 
>> +}
>> +
>> +static struct acpi_s2idle_dev_ops pmc_s2idle_ops = {
>> +	.check = pmc_s2idle_check,
>> +};
>> +#endif
>> +
>>  static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  {
>>  	struct pmc_dev *pmc = &pmc_device;
>> @@ -485,6 +548,10 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  		dev_warn(&pdev->dev, "platform clocks register failed: %d\n",
>>  			 ret);
>>  
>> +#ifdef CONFIG_SUSPEND
>> +	acpi_register_lps0_dev(&pmc_s2idle_ops);
>> +#endif
>> +
>>  	pmc->init = true;
>>  	return ret;
>>  }
>>
> 

Re: [PATCH v2 3/5] platform/x86: pmc_atom: Check state of PMC managed devices on s2idle

[Hans de Goede]

Hi,

On 1/8/24 13:25, Hans de Goede wrote:
> Hi,
> 
> On 1/8/24 12:18, Ilpo Järvinen wrote:
>> On Sun, 7 Jan 2024, Hans de Goede wrote:

<snip>

>>> +
>>> +	/* Low part */
>>> +	pmc_dev_state_check(d3_sts_0, m->d3_sts_0, func_dis, m->func_dis, false_pos_sts_0);
>>> +
>>> +	/* High part */
>>> +	pmc_dev_state_check(d3_sts_1, m->d3_sts_1, func_dis_2, m->func_dis_2, false_pos_sts_1);
>>
>> The variables are called _0 and _1 but the comment talks about "low" and 
>> "high", could these be made consistent such that variabless end into _lo & 
>> _hi ?
>>
>> After making that change, I don't think those comments add any value any 
>> further value to what is already plainly visible from the code itself.
> 
> Ack, I'll replace the _0 and _1 with _lo and _hi.

Correction the _0 and _1 actually match the name of the register address
defines, which in turn mach the data sheet names.

so instead of replacing _0 / _1 I have no dropped the /* Low part */
and /* High part */ comments since those are a bit off.

Regards,

Hans

[PATCH v2 4/5] platform/x86: pmc_atom: Check state of PMC clocks on s2idle

[Hans de Goede]

Extend the s2idle check with checking that none of the PMC clocks
is in the forced-on state. If one of the clocks is in forced on
state then S0i3 cannot be reached.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Drop the PMC_CLK_* defines these are defined in
  include/linux/platform_data/x86/pmc_atom.h now
- Drop duplicated "pmc_atom: " prefix from pr_err() message
---
 drivers/platform/x86/pmc_atom.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
index 81ad66117365..d04f635c4075 100644
--- a/drivers/platform/x86/pmc_atom.c
+++ b/drivers/platform/x86/pmc_atom.c
@@ -477,6 +477,7 @@ static void pmc_s2idle_check(void)
 	u32 func_dis, func_dis_2;
 	u32 d3_sts_0, d3_sts_1;
 	u32 false_pos_sts_0, false_pos_sts_1;
+	int i;
 
 	func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
 	func_dis_2 = pmc_reg_read(pmc, PMC_FUNC_DIS_2);
@@ -504,6 +505,16 @@ static void pmc_s2idle_check(void)
 
 	/* High part */
 	pmc_dev_state_check(d3_sts_1, m->d3_sts_1, func_dis_2, m->func_dis_2, false_pos_sts_1);
+
+	/* Check PMC clocks */
+	for (i = 0; i < PMC_CLK_NUM; i++) {
+		u32 ctl = pmc_reg_read(pmc, PMC_CLK_CTL_OFFSET + 4 * i);
+
+		if ((ctl & PMC_MASK_CLK_CTL) != PMC_CLK_CTL_FORCE_ON)
+			continue;
+
+		pr_err("clock %d is ON prior to freeze (ctl 0x%08x)\n", i, ctl);
+	}
 }
 
 static struct acpi_s2idle_dev_ops pmc_s2idle_ops = {
-- 
2.43.0

Re: [PATCH v2 4/5] platform/x86: pmc_atom: Check state of PMC clocks on s2idle

[Ilpo Järvinen]

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

On Sun, 7 Jan 2024, Hans de Goede wrote:

> Extend the s2idle check with checking that none of the PMC clocks
> is in the forced-on state. If one of the clocks is in forced on
> state then S0i3 cannot be reached.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - Drop the PMC_CLK_* defines these are defined in
>   include/linux/platform_data/x86/pmc_atom.h now
> - Drop duplicated "pmc_atom: " prefix from pr_err() message
> ---
>  drivers/platform/x86/pmc_atom.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
> index 81ad66117365..d04f635c4075 100644
> --- a/drivers/platform/x86/pmc_atom.c
> +++ b/drivers/platform/x86/pmc_atom.c
> @@ -477,6 +477,7 @@ static void pmc_s2idle_check(void)
>  	u32 func_dis, func_dis_2;
>  	u32 d3_sts_0, d3_sts_1;
>  	u32 false_pos_sts_0, false_pos_sts_1;
> +	int i;
>  
>  	func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
>  	func_dis_2 = pmc_reg_read(pmc, PMC_FUNC_DIS_2);
> @@ -504,6 +505,16 @@ static void pmc_s2idle_check(void)
>  
>  	/* High part */
>  	pmc_dev_state_check(d3_sts_1, m->d3_sts_1, func_dis_2, m->func_dis_2, false_pos_sts_1);
> +
> +	/* Check PMC clocks */

Kind of obvious comment, how about:

	/* Check PMC clocks don't prevent S0i3 */

Or

	/* Forced-on PMC clock prevents S0i3? */

?

> +	for (i = 0; i < PMC_CLK_NUM; i++) {
> +		u32 ctl = pmc_reg_read(pmc, PMC_CLK_CTL_OFFSET + 4 * i);
> +
> +		if ((ctl & PMC_MASK_CLK_CTL) != PMC_CLK_CTL_FORCE_ON)
> +			continue;
> +
> +		pr_err("clock %d is ON prior to freeze (ctl 0x%08x)\n", i, ctl);
> +	}
>  }
>  
>  static struct acpi_s2idle_dev_ops pmc_s2idle_ops = {
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

Re: [PATCH v2 4/5] platform/x86: pmc_atom: Check state of PMC clocks on s2idle

[Hans de Goede]

Hi,

On 1/8/24 12:27, Ilpo Järvinen wrote:
> On Sun, 7 Jan 2024, Hans de Goede wrote:
> 
>> Extend the s2idle check with checking that none of the PMC clocks
>> is in the forced-on state. If one of the clocks is in forced on
>> state then S0i3 cannot be reached.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> - Drop the PMC_CLK_* defines these are defined in
>>   include/linux/platform_data/x86/pmc_atom.h now
>> - Drop duplicated "pmc_atom: " prefix from pr_err() message
>> ---
>>  drivers/platform/x86/pmc_atom.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
>> index 81ad66117365..d04f635c4075 100644
>> --- a/drivers/platform/x86/pmc_atom.c
>> +++ b/drivers/platform/x86/pmc_atom.c
>> @@ -477,6 +477,7 @@ static void pmc_s2idle_check(void)
>>  	u32 func_dis, func_dis_2;
>>  	u32 d3_sts_0, d3_sts_1;
>>  	u32 false_pos_sts_0, false_pos_sts_1;
>> +	int i;
>>  
>>  	func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
>>  	func_dis_2 = pmc_reg_read(pmc, PMC_FUNC_DIS_2);
>> @@ -504,6 +505,16 @@ static void pmc_s2idle_check(void)
>>  
>>  	/* High part */
>>  	pmc_dev_state_check(d3_sts_1, m->d3_sts_1, func_dis_2, m->func_dis_2, false_pos_sts_1);
>> +
>> +	/* Check PMC clocks */
> 
> Kind of obvious comment, how about:
> 
> 	/* Check PMC clocks don't prevent S0i3 */
> 
> Or
> 
> 	/* Forced-on PMC clock prevents S0i3? */
> 
> ?

Good point. I have gone with the "Forced-on PMC clock prevents S0i3"
comment.

Regards,

Hans


> 
>> +	for (i = 0; i < PMC_CLK_NUM; i++) {
>> +		u32 ctl = pmc_reg_read(pmc, PMC_CLK_CTL_OFFSET + 4 * i);
>> +
>> +		if ((ctl & PMC_MASK_CLK_CTL) != PMC_CLK_CTL_FORCE_ON)
>> +			continue;
>> +
>> +		pr_err("clock %d is ON prior to freeze (ctl 0x%08x)\n", i, ctl);
>> +	}
>>  }
>>  
>>  static struct acpi_s2idle_dev_ops pmc_s2idle_ops = {
>>
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> 

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

[Hans de Goede]

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.

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 */
+		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
 	punit_dbgfs_unregister();
 }
 
-- 
2.43.0

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

[Ilpo Järvinen]

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.

Re: [PATCH v2 0/5] x86: atom-punit/-pmc s2idle device state checks

[Borislav Petkov]

On Sun, Jan 07, 2024 at 03:03:05PM +0100, Hans de Goede wrote:
> clk and x86/tip maintainers, it is probably the cleanest if this
> entire series is merged through the pdx86 tree (*). Can we have
> your ack for merging patch 1/5 resp. 5/5 through the pdx86 tree ?

for patch 5:

Acked-by: Borislav Petkov (AMD) <bp@alien8.de>

> *) Andy recently mentioned that it might be a good idea to move
> some of the arch/x86/platform code to drivers/platform/x86,
> arch/x86/platform/atom/punit_atom_debug.c which is a completely
> standalone driver definitly is a good candidate for this

Moving *all* drivers away from arch/x86/platform/ and to
drivers/platform/x86/ where they really belong sounds like a good idea
to me.

arch/x86/platform should probably be only platform-specific code but not
standalone drivers.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette