| Message ID | 20221117225822.16154-4-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | Report percentage of time in hardware sleep state | expand |
Hi Mario, comments below. On Thu, Nov 17, 2022 at 5:58 PM Mario Limonciello <mario.limonciello@amd.com> wrote: > > `check_counters` is a stateful variable for indicating whether or > not to be checking if counters incremented on resume from s2idle. > > As the module already has code to gate whether to check the counters > that will fail the suspend when this is enabled, use that instead. > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > RFC v3->v4: > * No changes > --- > drivers/platform/x86/intel/pmc/core.c | 7 ++----- > drivers/platform/x86/intel/pmc/core.h | 1 - > 2 files changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c > index 17ec5825d13d..adc2cae4db28 100644 > --- a/drivers/platform/x86/intel/pmc/core.c > +++ b/drivers/platform/x86/intel/pmc/core.c > @@ -2059,8 +2059,6 @@ static __maybe_unused int pmc_core_suspend(struct device *dev) > { > struct pmc_dev *pmcdev = dev_get_drvdata(dev); > > - pmcdev->check_counters = false; > - > /* No warnings on S0ix failures */ > if (!warn_on_s0ix_failures) > return 0; > @@ -2077,7 +2075,6 @@ static __maybe_unused int pmc_core_suspend(struct device *dev) > if (pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter)) > return -EIO; > > - pmcdev->check_counters = true; > return 0; > } > > @@ -2113,10 +2110,10 @@ static __maybe_unused int pmc_core_resume(struct device *dev) > const struct pmc_bit_map **maps = pmcdev->map->lpm_sts; > int offset = pmcdev->map->lpm_status_offset; > > - if (!pmcdev->check_counters) > + if (!pmc_core_is_s0ix_failed(pmcdev)) Will this break the "CPU did not enter SLP_S0!!!" warning? As far as I can tell, If an Intel system uses S3 instead of S0ix, pmcdev->s0ix_counter will not get updated in the suspend callback. In the resume callback, the counter check in pmc_core_is_s0ix_failed() no longer makes any sense. It either fails all the time (if pmcdev->s0ix_counter was inited with a non- zero value) or succeeds all the time (if pmcdev->s0ix_counter was zero-inited). > return 0; > > - if (!pmc_core_is_s0ix_failed(pmcdev)) > + if (!warn_on_s0ix_failures) > return 0; > > if (pmc_core_is_pc10_failed(pmcdev)) { > diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h > index 7a059e02c265..5687e91e884c 100644 > --- a/drivers/platform/x86/intel/pmc/core.h > +++ b/drivers/platform/x86/intel/pmc/core.h > @@ -316,7 +316,6 @@ struct pmc_reg_map { > * @pmc_xram_read_bit: flag to indicate whether PMC XRAM shadow registers > * used to read MPHY PG and PLL status are available > * @mutex_lock: mutex to complete one transcation > - * @check_counters: On resume, check if counters are getting incremented > * @pc10_counter: PC10 residency counter > * @s0ix_counter: S0ix residency (step adjusted) > * @num_lpm_modes: Count of enabled modes > -- > 2.34.1 >
[Public] > -----Original Message----- > From: Sven van Ashbrook <svenva@chromium.org> > Sent: Wednesday, November 23, 2022 11:47 > To: Limonciello, Mario <Mario.Limonciello@amd.com> > Cc: Rafael J . Wysocki <rafael@kernel.org>; Rajneesh Bhardwaj > <irenic.rajneesh@gmail.com>; David E Box <david.e.box@intel.com>; Raul > Rangel <rrangel@chromium.org>; linux-pm@vger.kernel.org; platform- > driver-x86@vger.kernel.org; Pavel Machek <pavel@ucw.cz>; Len Brown > <len.brown@intel.com>; John Stultz <jstultz@google.com>; Thomas > Gleixner <tglx@linutronix.de>; Stephen Boyd <sboyd@kernel.org>; S-k, > Shyam-sundar <Shyam-sundar.S-k@amd.com>; Rajat Jain > <rajatja@google.com>; Hans de Goede <hdegoede@redhat.com>; linux- > kernel@vger.kernel.org; Mark Gross <markgross@kernel.org> > Subject: Re: [RFC v4 3/5] platform/x86/intel/pmc: core: Drop check_counters > > Hi Mario, comments below. > > On Thu, Nov 17, 2022 at 5:58 PM Mario Limonciello > <mario.limonciello@amd.com> wrote: > > > > `check_counters` is a stateful variable for indicating whether or > > not to be checking if counters incremented on resume from s2idle. > > > > As the module already has code to gate whether to check the counters > > that will fail the suspend when this is enabled, use that instead. > > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > --- > > RFC v3->v4: > > * No changes > > --- > > drivers/platform/x86/intel/pmc/core.c | 7 ++----- > > drivers/platform/x86/intel/pmc/core.h | 1 - > > 2 files changed, 2 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/platform/x86/intel/pmc/core.c > b/drivers/platform/x86/intel/pmc/core.c > > index 17ec5825d13d..adc2cae4db28 100644 > > --- a/drivers/platform/x86/intel/pmc/core.c > > +++ b/drivers/platform/x86/intel/pmc/core.c > > @@ -2059,8 +2059,6 @@ static __maybe_unused int > pmc_core_suspend(struct device *dev) > > { > > struct pmc_dev *pmcdev = dev_get_drvdata(dev); > > > > - pmcdev->check_counters = false; > > - > > /* No warnings on S0ix failures */ > > if (!warn_on_s0ix_failures) > > return 0; > > @@ -2077,7 +2075,6 @@ static __maybe_unused int > pmc_core_suspend(struct device *dev) > > if (pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter)) > > return -EIO; > > > > - pmcdev->check_counters = true; > > return 0; > > } > > > > @@ -2113,10 +2110,10 @@ static __maybe_unused int > pmc_core_resume(struct device *dev) > > const struct pmc_bit_map **maps = pmcdev->map->lpm_sts; > > int offset = pmcdev->map->lpm_status_offset; > > > > - if (!pmcdev->check_counters) > > + if (!pmc_core_is_s0ix_failed(pmcdev)) > > Will this break the "CPU did not enter SLP_S0!!!" warning? > > As far as I can tell, > If an Intel system uses S3 instead of S0ix, pmcdev->s0ix_counter will > not get updated in the > suspend callback. In the resume callback, the counter check in > pmc_core_is_s0ix_failed() > no longer makes any sense. It either fails all the time (if > pmcdev->s0ix_counter was inited with a non- > zero value) or succeeds all the time (if pmcdev->s0ix_counter was zero- > inited). Ah yeah; So this patch probably doesn't make sense as is. It would mean either needing to check pm_suspend_via_firmware() in the resume callback or just skipping it. I'll drop it in the next revision of this. > > > return 0; > > > > - if (!pmc_core_is_s0ix_failed(pmcdev)) > > + if (!warn_on_s0ix_failures) > > return 0; > > > > if (pmc_core_is_pc10_failed(pmcdev)) { > > diff --git a/drivers/platform/x86/intel/pmc/core.h > b/drivers/platform/x86/intel/pmc/core.h > > index 7a059e02c265..5687e91e884c 100644 > > --- a/drivers/platform/x86/intel/pmc/core.h > > +++ b/drivers/platform/x86/intel/pmc/core.h > > @@ -316,7 +316,6 @@ struct pmc_reg_map { > > * @pmc_xram_read_bit: flag to indicate whether PMC XRAM shadow > registers > > * used to read MPHY PG and PLL status are available > > * @mutex_lock: mutex to complete one transcation > > - * @check_counters: On resume, check if counters are getting > incremented > > * @pc10_counter: PC10 residency counter > > * @s0ix_counter: S0ix residency (step adjusted) > > * @num_lpm_modes: Count of enabled modes > > -- > > 2.34.1 > >
diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c index 17ec5825d13d..adc2cae4db28 100644 --- a/drivers/platform/x86/intel/pmc/core.c +++ b/drivers/platform/x86/intel/pmc/core.c @@ -2059,8 +2059,6 @@ static __maybe_unused int pmc_core_suspend(struct device *dev) { struct pmc_dev *pmcdev = dev_get_drvdata(dev); - pmcdev->check_counters = false; - /* No warnings on S0ix failures */ if (!warn_on_s0ix_failures) return 0; @@ -2077,7 +2075,6 @@ static __maybe_unused int pmc_core_suspend(struct device *dev) if (pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter)) return -EIO; - pmcdev->check_counters = true; return 0; } @@ -2113,10 +2110,10 @@ static __maybe_unused int pmc_core_resume(struct device *dev) const struct pmc_bit_map **maps = pmcdev->map->lpm_sts; int offset = pmcdev->map->lpm_status_offset; - if (!pmcdev->check_counters) + if (!pmc_core_is_s0ix_failed(pmcdev)) return 0; - if (!pmc_core_is_s0ix_failed(pmcdev)) + if (!warn_on_s0ix_failures) return 0; if (pmc_core_is_pc10_failed(pmcdev)) { diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h index 7a059e02c265..5687e91e884c 100644 --- a/drivers/platform/x86/intel/pmc/core.h +++ b/drivers/platform/x86/intel/pmc/core.h @@ -316,7 +316,6 @@ struct pmc_reg_map { * @pmc_xram_read_bit: flag to indicate whether PMC XRAM shadow registers * used to read MPHY PG and PLL status are available * @mutex_lock: mutex to complete one transcation - * @check_counters: On resume, check if counters are getting incremented * @pc10_counter: PC10 residency counter * @s0ix_counter: S0ix residency (step adjusted) * @num_lpm_modes: Count of enabled modes
`check_counters` is a stateful variable for indicating whether or not to be checking if counters incremented on resume from s2idle. As the module already has code to gate whether to check the counters that will fail the suspend when this is enabled, use that instead. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- RFC v3->v4: * No changes --- drivers/platform/x86/intel/pmc/core.c | 7 ++----- drivers/platform/x86/intel/pmc/core.h | 1 - 2 files changed, 2 insertions(+), 6 deletions(-)