diff mbox series

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

Message ID 20240107140310.46512-4-hdegoede@redhat.com (mailing list archive)
State Not Applicable, archived
Headers show
Series x86: atom-punit/-pmc s2idle device state checks | expand

Commit Message

Hans de Goede Jan. 7, 2024, 2:03 p.m. UTC
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(+)

Comments

Ilpo Järvinen Jan. 8, 2024, 11:18 a.m. UTC | #1
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;
>  }
>
Hans de Goede Jan. 8, 2024, 12:25 p.m. UTC | #2
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;
>>  }
>>
>
Hans de Goede Jan. 8, 2024, 12:38 p.m. UTC | #3
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
diff mbox series

Patch

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;
 }