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