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

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

[Hans de Goede]

Hi All,

These 2 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 go 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.

x86/tip maintainers, it is probably the cleanest if I merge
this entire series through the pdx86 tree (*). Can I have your
ack for merging patch 4/4 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 (2):
  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  | 40 ++++++++++
 drivers/platform/x86/pmc_atom.c            | 86 ++++++++++++++++++++++
 include/linux/platform_data/x86/pmc_atom.h | 12 +--
 3 files changed, 132 insertions(+), 6 deletions(-)

-- 
2.43.0

[PATCH 1/4] 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 b8a701c77fd0..5eec0e22acdf 100644
--- a/include/linux/platform_data/x86/pmc_atom.h
+++ b/include/linux/platform_data/x86/pmc_atom.h
@@ -104,14 +104,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 2/4] 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>
---
 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..e14d489fa6f9 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("pmc_atom: %s is in D0 prior to s2idle\n",
+					  sts_map[index].name);
+			else
+				pr_err("pmc_atom: %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 lvl.
+	 */
+	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

[PATCH 3/4] 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>
---
 drivers/platform/x86/pmc_atom.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
index e14d489fa6f9..375010ef61ae 100644
--- a/drivers/platform/x86/pmc_atom.c
+++ b/drivers/platform/x86/pmc_atom.c
@@ -20,6 +20,14 @@
 #include <linux/seq_file.h>
 #include <linux/suspend.h>
 
+#define PMC_CLK_CTL_OFFSET		0x60
+#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)
+
 struct pmc_bit_map {
 	const char *name;
 	u32 bit_mask;
@@ -477,6 +485,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 +513,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("pmc_atom: clk %d is ON prior to freeze (ctl %08x)\n", i, ctl);
+	}
 }
 
 static struct acpi_s2idle_dev_ops pmc_s2idle_ops = {
-- 
2.43.0

[PATCH 4/4] 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 | 40 +++++++++++++++++++++++
 1 file changed, 40 insertions(+)

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();
 }
 
-- 
2.43.0

Re: [PATCH 2/4] platform/x86: pmc_atom: Check state of PMC managed devices on s2idle

[Andy Shevchenko]

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 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.

...

> +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("pmc_atom: %s is in D0 prior to s2idle\n",
> +                                         sts_map[index].name);

Please, drop the prefix, we have pr_fmt() for this already defined.

> +                       else
> +                               pr_err("pmc_atom: %s is in D0 prior to s2idle\n",
> +                                      sts_map[index].name);

Ditto.

> +               }
> +       }
> +}

...

> +       /*
> +        * 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 lvl.

lvl --> level

> +        */

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Dec 31, 2023 at 6:33 PM Hans de Goede <hdegoede@redhat.com> 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

Perhaps "forced-on state"?

> state then S0i3 cannot be reached.

...

> +#define PMC_CLK_CTL_OFFSET             0x60
> +#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)

Please, move these to include/linux/platform_data/x86/clk-pmc-atom.h
from drivers/clk/x86/clk-pmc-atom.c and use the former. Otherwise it's
a big dup of the existing stuff.

...

> +               pr_err("pmc_atom: clk %d is ON prior to freeze (ctl %08x)\n", i, ctl);

No prefix.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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.)

>  }

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

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