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

[Hans de Goede <hdegoede@redhat.com>]

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