Re: [PATCH v2 3/5] platform/x86: pmc_atom: Check state of PMC 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 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.