Hi, On 1/8/24 12:18, Ilpo Järvinen wrote: > On Sun, 7 Jan 2024, Hans de Goede wrote: > >> From: Johannes Stezenbach <js@xxxxxxxxx> >> >> 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@xxxxxxxxx> >> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> >> [hdegoede: Use acpi_s2idle_dev_ops, ignore fused off blocks, PMIC I2C] >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> >> --- >> 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; >> } >> >
