2019-03-13 22:22:10

by Rajat Jain

[permalink] [raw]
Subject: [PATCH 1/2] platform/x86: intel_pmc_core: Convert to a platform_driver

Convert the intel_pmc_core driver to a platform driver. There is no
functional change. Some code that tries to determine what kind of
CPU this is, has been moved code is moved from pmc_core_probe() to
pmc_core_init().

Signed-off-by: Rajat Jain <[email protected]>
---
This is rebased off
git://git.infradead.org/linux-platform-drivers-x86.git/for-next

drivers/platform/x86/intel_pmc_core.c | 93 ++++++++++++++++++++-------
1 file changed, 68 insertions(+), 25 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index f2c621b55f49..55578d07610c 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -19,6 +19,7 @@
#include <linux/io.h>
#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/platform_device.h>
#include <linux/uaccess.h>

#include <asm/cpu_device_id.h>
@@ -854,12 +855,59 @@ static const struct dmi_system_id pmc_core_dmi_table[] = {
{}
};

-static int __init pmc_core_probe(void)
+static int pmc_core_probe(struct platform_device *pdev)
{
- struct pmc_dev *pmcdev = &pmc;
+ struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
+ int err;
+
+ pmcdev->regbase = ioremap(pmcdev->base_addr,
+ pmcdev->map->regmap_length);
+ if (!pmcdev->regbase)
+ return -ENOMEM;
+
+ mutex_init(&pmcdev->lock);
+ pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
+
+ err = pmc_core_dbgfs_register(pmcdev);
+ if (err < 0) {
+ dev_warn(&pdev->dev, "debugfs register failed.\n");
+ iounmap(pmcdev->regbase);
+ return err;
+ }
+
+ dmi_check_system(pmc_core_dmi_table);
+ dev_info(&pdev->dev, " initialized\n");
+ return 0;
+}
+
+static int pmc_core_remove(struct platform_device *pdev)
+{
+ struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
+
+ pmc_core_dbgfs_unregister(pmcdev);
+ mutex_destroy(&pmcdev->lock);
+ iounmap(pmcdev->regbase);
+ return 0;
+}
+
+static struct platform_driver pmc_core_driver = {
+ .driver = {
+ .name = "pmc_core",
+ },
+ .probe = pmc_core_probe,
+ .remove = pmc_core_remove,
+};
+
+static struct platform_device pmc_core_device = {
+ .name = "pmc_core",
+};
+
+static int __init pmc_core_init(void)
+{
+ int ret;
const struct x86_cpu_id *cpu_id;
+ struct pmc_dev *pmcdev = &pmc;
u64 slp_s0_addr;
- int err;

cpu_id = x86_match_cpu(intel_pmc_core_ids);
if (!cpu_id)
@@ -880,36 +928,31 @@ static int __init pmc_core_probe(void)
else
pmcdev->base_addr = slp_s0_addr - pmcdev->map->slp_s0_offset;

- pmcdev->regbase = ioremap(pmcdev->base_addr,
- pmcdev->map->regmap_length);
- if (!pmcdev->regbase)
- return -ENOMEM;
+ platform_set_drvdata(&pmc_core_device, pmcdev);

- mutex_init(&pmcdev->lock);
- pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
+ ret = platform_device_register(&pmc_core_device);
+ if (ret)
+ return ret;

- err = pmc_core_dbgfs_register(pmcdev);
- if (err < 0) {
- pr_warn(" debugfs register failed.\n");
- iounmap(pmcdev->regbase);
- return err;
- }
+ ret = platform_driver_register(&pmc_core_driver);
+ if (ret)
+ goto out_remove_dev;

- dmi_check_system(pmc_core_dmi_table);
- pr_info(" initialized\n");
return 0;
+
+out_remove_dev:
+ platform_device_unregister(&pmc_core_device);
+ return ret;
}
-module_init(pmc_core_probe)

-static void __exit pmc_core_remove(void)
+static void __init pmc_core_exit(void)
{
- struct pmc_dev *pmcdev = &pmc;
-
- pmc_core_dbgfs_unregister(pmcdev);
- mutex_destroy(&pmcdev->lock);
- iounmap(pmcdev->regbase);
+ platform_driver_unregister(&pmc_core_driver);
+ platform_device_unregister(&pmc_core_device);
}
-module_exit(pmc_core_remove)
+
+module_init(pmc_core_init);
+module_exit(pmc_core_exit);

MODULE_LICENSE("GPL v2");
MODULE_DESCRIPTION("Intel PMC Core Driver");
--
2.21.0.360.g471c308f928-goog



2019-03-13 22:22:24

by Rajat Jain

[permalink] [raw]
Subject: [PATCH 2/2] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

Add a module parameter which when enabled, will check on resume, if the
last S0ix attempt was successful. If not, the driver would provide
helpful debug information (which gets latched during the failed suspend
attempt) to debug the S0ix failure.

This information is very useful to debug S0ix failures. Specially since
the latched debug information will be lost (over-written) if the system
attempts to go into runtime (or imminent) S0ix again after that failed
suspend attempt.

Signed-off-by: Rajat Jain <[email protected]>
---
drivers/platform/x86/intel_pmc_core.c | 86 +++++++++++++++++++++++++++
drivers/platform/x86/intel_pmc_core.h | 7 +++
2 files changed, 93 insertions(+)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 55578d07610c..b1f4405a27ce 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -20,6 +20,7 @@
#include <linux/module.h>
#include <linux/pci.h>
#include <linux/platform_device.h>
+#include <linux/suspend.h>
#include <linux/uaccess.h>

#include <asm/cpu_device_id.h>
@@ -890,9 +891,94 @@ static int pmc_core_remove(struct platform_device *pdev)
return 0;
}

+#ifdef CONFIG_PM_SLEEP
+
+static bool warn_on_s0ix_failures;
+module_param(warn_on_s0ix_failures, bool, 0644);
+MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix failures");
+
+static int pmc_core_suspend(struct device *dev)
+{
+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
+
+ /* Save PC10 and S0ix residency for checking later */
+ if (warn_on_s0ix_failures &&
+ !rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter) &&
+ !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
+ pmcdev->check_counters = true;
+ else
+ pmcdev->check_counters = false;
+
+ return 0;
+}
+
+static inline bool pc10_failed(struct pmc_dev *pmcdev)
+{
+ u64 pc10_counter;
+
+ if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
+ pc10_counter == pmcdev->pc10_counter)
+ return true;
+ else
+ return false;
+}
+
+static inline bool s0ix_failed(struct pmc_dev *pmcdev)
+{
+ u64 s0ix_counter;
+
+ if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
+ s0ix_counter == pmcdev->s0ix_counter)
+ return true;
+ else
+ return false;
+}
+
+static int pmc_core_resume(struct device *dev)
+{
+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
+
+ if (!pmcdev->check_counters)
+ return 0;
+
+ if (pc10_failed(pmcdev)) {
+ dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
+ pmcdev->pc10_counter);
+ } else if (s0ix_failed(pmcdev)) {
+
+ const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
+ const struct pmc_bit_map *map;
+ int offset = pmcdev->map->slps0_dbg_offset;
+ u32 data;
+
+ dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
+ pmcdev->s0ix_counter);
+ while (*maps) {
+ map = *maps;
+ data = pmc_core_reg_read(pmcdev, offset);
+ offset += 4;
+ while (map->name) {
+ dev_warn(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
+ map->name,
+ data & map->bit_mask ? "Yes" : "No");
+ ++map;
+ }
+ ++maps;
+ }
+ }
+ return 0;
+}
+
+#endif
+
+const struct dev_pm_ops pmc_core_pm_ops = {
+ SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend, pmc_core_resume)
+};
+
static struct platform_driver pmc_core_driver = {
.driver = {
.name = "pmc_core",
+ .pm = &pmc_core_pm_ops
},
.probe = pmc_core_probe,
.remove = pmc_core_remove,
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 88d9c0653a5f..fdee5772e532 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -241,6 +241,9 @@ 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)
*
* pmc_dev contains info about power management controller device.
*/
@@ -253,6 +256,10 @@ struct pmc_dev {
#endif /* CONFIG_DEBUG_FS */
int pmc_xram_read_bit;
struct mutex lock; /* generic mutex lock for PMC Core */
+
+ bool check_counters; /* Check for counter increments on resume */
+ u64 pc10_counter;
+ u64 s0ix_counter;
};

#endif /* PMC_CORE_H */
--
2.21.0.360.g471c308f928-goog


2019-03-16 08:31:22

by Rajneesh Bhardwaj

[permalink] [raw]
Subject: Re: [PATCH 1/2] platform/x86: intel_pmc_core: Convert to a platform_driver

On Wed, Mar 13, 2019 at 03:21:23PM -0700, Rajat Jain wrote:
> Convert the intel_pmc_core driver to a platform driver. There is no
> functional change. Some code that tries to determine what kind of
> CPU this is, has been moved code is moved from pmc_core_probe() to

Possible typo here.

> pmc_core_init().
>
> Signed-off-by: Rajat Jain <[email protected]>

Thanks for sending this. This is certainly useful to support suspend-resume
functionality for this driver which is otherwise only possible with PM
notifiers otherwise and that is not desirable. Initially this was a PCI
driver and after design discussion it was converted to module. I would like
to consult Andy and Srinivas for their opinion about binding it to actual
platform bus instead of the virtual bus as in its current form. In one of the
internal versions, we used a known acpi PNP HID.

> ---
> This is rebased off
> git://git.infradead.org/linux-platform-drivers-x86.git/for-next
>
> drivers/platform/x86/intel_pmc_core.c | 93 ++++++++++++++++++++-------
> 1 file changed, 68 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index f2c621b55f49..55578d07610c 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -19,6 +19,7 @@
> #include <linux/io.h>
> #include <linux/module.h>
> #include <linux/pci.h>
> +#include <linux/platform_device.h>
> #include <linux/uaccess.h>
>
> #include <asm/cpu_device_id.h>
> @@ -854,12 +855,59 @@ static const struct dmi_system_id pmc_core_dmi_table[] = {
> {}
> };
>
> -static int __init pmc_core_probe(void)
> +static int pmc_core_probe(struct platform_device *pdev)
> {
> - struct pmc_dev *pmcdev = &pmc;
> + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> + int err;
> +
> + pmcdev->regbase = ioremap(pmcdev->base_addr,
> + pmcdev->map->regmap_length);
> + if (!pmcdev->regbase)
> + return -ENOMEM;
> +
> + mutex_init(&pmcdev->lock);
> + pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
> +
> + err = pmc_core_dbgfs_register(pmcdev);
> + if (err < 0) {
> + dev_warn(&pdev->dev, "debugfs register failed.\n");
> + iounmap(pmcdev->regbase);
> + return err;
> + }
> +
> + dmi_check_system(pmc_core_dmi_table);
> + dev_info(&pdev->dev, " initialized\n");
> + return 0;
> +}
> +
> +static int pmc_core_remove(struct platform_device *pdev)
> +{
> + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> +
> + pmc_core_dbgfs_unregister(pmcdev);
> + mutex_destroy(&pmcdev->lock);
> + iounmap(pmcdev->regbase);
> + return 0;
> +}
> +
> +static struct platform_driver pmc_core_driver = {
> + .driver = {
> + .name = "pmc_core",
> + },
> + .probe = pmc_core_probe,
> + .remove = pmc_core_remove,
> +};
> +
> +static struct platform_device pmc_core_device = {
> + .name = "pmc_core",
> +};
> +
> +static int __init pmc_core_init(void)
> +{
> + int ret;

Please use reverse x-mas tree style.

> const struct x86_cpu_id *cpu_id;
> + struct pmc_dev *pmcdev = &pmc;
> u64 slp_s0_addr;
> - int err;
>
> cpu_id = x86_match_cpu(intel_pmc_core_ids);
> if (!cpu_id)
> @@ -880,36 +928,31 @@ static int __init pmc_core_probe(void)
> else
> pmcdev->base_addr = slp_s0_addr - pmcdev->map->slp_s0_offset;
>
> - pmcdev->regbase = ioremap(pmcdev->base_addr,
> - pmcdev->map->regmap_length);
> - if (!pmcdev->regbase)
> - return -ENOMEM;
> + platform_set_drvdata(&pmc_core_device, pmcdev);
>
> - mutex_init(&pmcdev->lock);
> - pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
> + ret = platform_device_register(&pmc_core_device);
> + if (ret)
> + return ret;
>
> - err = pmc_core_dbgfs_register(pmcdev);
> - if (err < 0) {
> - pr_warn(" debugfs register failed.\n");
> - iounmap(pmcdev->regbase);
> - return err;
> - }
> + ret = platform_driver_register(&pmc_core_driver);
> + if (ret)
> + goto out_remove_dev;
>
> - dmi_check_system(pmc_core_dmi_table);
> - pr_info(" initialized\n");
> return 0;
> +
> +out_remove_dev:
> + platform_device_unregister(&pmc_core_device);
> + return ret;
> }
> -module_init(pmc_core_probe)
>
> -static void __exit pmc_core_remove(void)
> +static void __init pmc_core_exit(void)
> {
> - struct pmc_dev *pmcdev = &pmc;
> -
> - pmc_core_dbgfs_unregister(pmcdev);
> - mutex_destroy(&pmcdev->lock);
> - iounmap(pmcdev->regbase);
> + platform_driver_unregister(&pmc_core_driver);
> + platform_device_unregister(&pmc_core_device);
> }
> -module_exit(pmc_core_remove)
> +
> +module_init(pmc_core_init);
> +module_exit(pmc_core_exit);
>
> MODULE_LICENSE("GPL v2");
> MODULE_DESCRIPTION("Intel PMC Core Driver");
> --
> 2.21.0.360.g471c308f928-goog
>

--
Best Regards,
Rajneesh

2019-03-16 08:43:59

by Rajneesh Bhardwaj

[permalink] [raw]
Subject: Re: [PATCH 2/2] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

On Wed, Mar 13, 2019 at 03:21:24PM -0700, Rajat Jain wrote:
> Add a module parameter which when enabled, will check on resume, if the
> last S0ix attempt was successful. If not, the driver would provide
> helpful debug information (which gets latched during the failed suspend
> attempt) to debug the S0ix failure.

+ Srinivas and David.

>
> This information is very useful to debug S0ix failures. Specially since
> the latched debug information will be lost (over-written) if the system
> attempts to go into runtime (or imminent) S0ix again after that failed
> suspend attempt.
>
> Signed-off-by: Rajat Jain <[email protected]>
> ---
> drivers/platform/x86/intel_pmc_core.c | 86 +++++++++++++++++++++++++++
> drivers/platform/x86/intel_pmc_core.h | 7 +++
> 2 files changed, 93 insertions(+)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 55578d07610c..b1f4405a27ce 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -20,6 +20,7 @@
> #include <linux/module.h>
> #include <linux/pci.h>
> #include <linux/platform_device.h>
> +#include <linux/suspend.h>
> #include <linux/uaccess.h>
>
> #include <asm/cpu_device_id.h>
> @@ -890,9 +891,94 @@ static int pmc_core_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_PM_SLEEP
> +
> +static bool warn_on_s0ix_failures;
> +module_param(warn_on_s0ix_failures, bool, 0644);
> +MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix failures");
> +
> +static int pmc_core_suspend(struct device *dev)
> +{
> + struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> +
> + /* Save PC10 and S0ix residency for checking later */
> + if (warn_on_s0ix_failures &&
> + !rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter) &&
> + !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
> + pmcdev->check_counters = true;
> + else
> + pmcdev->check_counters = false;
> +
> + return 0;
> +}
> +
> +static inline bool pc10_failed(struct pmc_dev *pmcdev)
> +{
> + u64 pc10_counter;
> +
> + if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
> + pc10_counter == pmcdev->pc10_counter)
> + return true;
> + else
> + return false;
> +}
> +
> +static inline bool s0ix_failed(struct pmc_dev *pmcdev)
> +{
> + u64 s0ix_counter;
> +
> + if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
> + s0ix_counter == pmcdev->s0ix_counter)
> + return true;
> + else
> + return false;
> +}
> +
> +static int pmc_core_resume(struct device *dev)
> +{
> + struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> +
> + if (!pmcdev->check_counters)
> + return 0;
> +
> + if (pc10_failed(pmcdev)) {
> + dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
> + pmcdev->pc10_counter);
> + } else if (s0ix_failed(pmcdev)) {
> +
> + const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;

slps0_dbg feature is not available on Skylake and Kabylake. PCH IP power
gating status is available on all.

> + const struct pmc_bit_map *map;
> + int offset = pmcdev->map->slps0_dbg_offset;
> + u32 data;
> +
> + dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
> + pmcdev->s0ix_counter);

I feel this dev_warn may be frowned upon and should be possible kept behind
something like pm_debug_messages_on though module param helps mitigate it.

> + while (*maps) {
> + map = *maps;
> + data = pmc_core_reg_read(pmcdev, offset);
> + offset += 4;
> + while (map->name) {
> + dev_warn(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
> + map->name,
> + data & map->bit_mask ? "Yes" : "No");
> + ++map;
> + }
> + ++maps;
> + }
> + }
> + return 0;
> +}
> +
> +#endif
> +
> +const struct dev_pm_ops pmc_core_pm_ops = {
> + SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend, pmc_core_resume)
> +};
> +
> static struct platform_driver pmc_core_driver = {
> .driver = {
> .name = "pmc_core",
> + .pm = &pmc_core_pm_ops
> },
> .probe = pmc_core_probe,
> .remove = pmc_core_remove,
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index 88d9c0653a5f..fdee5772e532 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -241,6 +241,9 @@ 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)
> *
> * pmc_dev contains info about power management controller device.
> */
> @@ -253,6 +256,10 @@ struct pmc_dev {
> #endif /* CONFIG_DEBUG_FS */
> int pmc_xram_read_bit;
> struct mutex lock; /* generic mutex lock for PMC Core */
> +
> + bool check_counters; /* Check for counter increments on resume */
> + u64 pc10_counter;
> + u64 s0ix_counter;
> };
>
> #endif /* PMC_CORE_H */
> --
> 2.21.0.360.g471c308f928-goog
>

--
Best Regards,
Rajneesh

2019-03-18 09:46:50

by Somayaji, Vishwanath

[permalink] [raw]
Subject: RE: [PATCH 2/2] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure



>-----Original Message-----
>From: Rajat Jain <[email protected]>
>Sent: Thursday, March 14, 2019 3:51 AM
>To: Bhardwaj, Rajneesh <[email protected]>; Somayaji, Vishwanath
><[email protected]>; Darren Hart <[email protected]>; Andy
>Shevchenko <[email protected]>; [email protected]; linux-
>[email protected]
>Cc: Rajat Jain <[email protected]>; [email protected];
>[email protected]; [email protected]
>Subject: [PATCH 2/2] platform/x86: intel_pmc_core: Allow to dump debug
>registers on S0ix failure
>
>Add a module parameter which when enabled, will check on resume, if the
>last S0ix attempt was successful. If not, the driver would provide
>helpful debug information (which gets latched during the failed suspend
>attempt) to debug the S0ix failure.
>
>This information is very useful to debug S0ix failures. Specially since
>the latched debug information will be lost (over-written) if the system
>attempts to go into runtime (or imminent) S0ix again after that failed
>suspend attempt.
>
>Signed-off-by: Rajat Jain <[email protected]>
>---
> drivers/platform/x86/intel_pmc_core.c | 86 +++++++++++++++++++++++++++
> drivers/platform/x86/intel_pmc_core.h | 7 +++
> 2 files changed, 93 insertions(+)
>
>diff --git a/drivers/platform/x86/intel_pmc_core.c
>b/drivers/platform/x86/intel_pmc_core.c
>index 55578d07610c..b1f4405a27ce 100644
>--- a/drivers/platform/x86/intel_pmc_core.c
>+++ b/drivers/platform/x86/intel_pmc_core.c
>@@ -20,6 +20,7 @@
> #include <linux/module.h>
> #include <linux/pci.h>
> #include <linux/platform_device.h>
>+#include <linux/suspend.h>
> #include <linux/uaccess.h>
>
> #include <asm/cpu_device_id.h>
>@@ -890,9 +891,94 @@ static int pmc_core_remove(struct platform_device
>*pdev)
> return 0;
> }
>
>+#ifdef CONFIG_PM_SLEEP
>+
>+static bool warn_on_s0ix_failures;
>+module_param(warn_on_s0ix_failures, bool, 0644);
>+MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix
>failures");
>+
>+static int pmc_core_suspend(struct device *dev)
>+{
>+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
>+
>+ /* Save PC10 and S0ix residency for checking later */
>+ if (warn_on_s0ix_failures &&
>+ !rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter)
>&&
>+ !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
>+ pmcdev->check_counters = true;
>+ else
>+ pmcdev->check_counters = false;
>+
>+ return 0;
>+}
>+
>+static inline bool pc10_failed(struct pmc_dev *pmcdev)
>+{
>+ u64 pc10_counter;
>+
>+ if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
>+ pc10_counter == pmcdev->pc10_counter)
>+ return true;
>+ else
>+ return false;
>+}
>+
>+static inline bool s0ix_failed(struct pmc_dev *pmcdev)
>+{
>+ u64 s0ix_counter;
>+
>+ if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
>+ s0ix_counter == pmcdev->s0ix_counter)
>+ return true;
>+ else
>+ return false;
>+}
>+
>+static int pmc_core_resume(struct device *dev)
>+{
>+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
>+
>+ if (!pmcdev->check_counters)
>+ return 0;
>+
>+ if (pc10_failed(pmcdev)) {
>+ dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
>+ pmcdev->pc10_counter);
>+ } else if (s0ix_failed(pmcdev)) {
>+
>+ const struct pmc_bit_map **maps = pmcdev->map-
>>slps0_dbg_maps;
>+ const struct pmc_bit_map *map;
>+ int offset = pmcdev->map->slps0_dbg_offset;
>+ u32 data;
>+
>+ dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
>+ pmcdev->s0ix_counter);
>+ while (*maps) {
>+ map = *maps;
>+ data = pmc_core_reg_read(pmcdev, offset);
>+ offset += 4;
>+ while (map->name) {
>+ dev_warn(dev, "SLP_S0_DBG: %-32s\tState:
>%s\n",
>+ map->name,
>+ data & map->bit_mask ? "Yes" : "No");
>+ ++map;
>+ }
>+ ++maps;
>+ }
>+ }
>+ return 0;
>+}
>+
>+#endif
>+
>+const struct dev_pm_ops pmc_core_pm_ops = {
>+ SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend,
>pmc_core_resume)
These PM Ops routines will be called not just in s2idle scenario, but also in other suspend scenarios like s2ram, s2disk. However actual functionalities served by these routines are relevant only for s2idle.
That means we will end up having false errors in s2ram/s2disk scenarios as PC10/s0ix counters wont increment in those scenarios.

Vishwa
>+};
>+
> static struct platform_driver pmc_core_driver = {
> .driver = {
> .name = "pmc_core",
>+ .pm = &pmc_core_pm_ops
> },
> .probe = pmc_core_probe,
> .remove = pmc_core_remove,
>diff --git a/drivers/platform/x86/intel_pmc_core.h
>b/drivers/platform/x86/intel_pmc_core.h
>index 88d9c0653a5f..fdee5772e532 100644
>--- a/drivers/platform/x86/intel_pmc_core.h
>+++ b/drivers/platform/x86/intel_pmc_core.h
>@@ -241,6 +241,9 @@ 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)
> *
> * pmc_dev contains info about power management controller device.
> */
>@@ -253,6 +256,10 @@ struct pmc_dev {
> #endif /* CONFIG_DEBUG_FS */
> int pmc_xram_read_bit;
> struct mutex lock; /* generic mutex lock for PMC Core */
>+
>+ bool check_counters; /* Check for counter increments on resume */
>+ u64 pc10_counter;
>+ u64 s0ix_counter;
> };
>
> #endif /* PMC_CORE_H */
>--
>2.21.0.360.g471c308f928-goog

2019-03-18 15:07:18

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH 1/2] platform/x86: intel_pmc_core: Convert to a platform_driver

On Sat, Mar 16, 2019 at 1:30 AM Rajneesh Bhardwaj
<[email protected]> wrote:
>
> On Wed, Mar 13, 2019 at 03:21:23PM -0700, Rajat Jain wrote:
> > Convert the intel_pmc_core driver to a platform driver. There is no
> > functional change. Some code that tries to determine what kind of
> > CPU this is, has been moved code is moved from pmc_core_probe() to
>
> Possible typo here.

Ummm, you mean grammar error I guess? Sure, I will rephrase.

>
> > pmc_core_init().
> >
> > Signed-off-by: Rajat Jain <[email protected]>
>
> Thanks for sending this. This is certainly useful to support suspend-resume
> functionality for this driver which is otherwise only possible with PM
> notifiers otherwise and that is not desirable. Initially this was a PCI
> driver and after design discussion it was converted to module. I would like
> to consult Andy and Srinivas for their opinion about binding it to actual
> platform bus instead of the virtual bus as in its current form. In one of the
> internal versions, we used a known acpi PNP HID.

Sure, if there is an established ACPI PNP HID, then we could bind it
using that, on platforms where we are still developing BIOS /
coreboot. However, this might not be possible for shipping systems
(Kabylake / skylake) where there is no plan to change the BIOS.

>
> > ---
> > This is rebased off
> > git://git.infradead.org/linux-platform-drivers-x86.git/for-next
> >
> > drivers/platform/x86/intel_pmc_core.c | 93 ++++++++++++++++++++-------
> > 1 file changed, 68 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> > index f2c621b55f49..55578d07610c 100644
> > --- a/drivers/platform/x86/intel_pmc_core.c
> > +++ b/drivers/platform/x86/intel_pmc_core.c
> > @@ -19,6 +19,7 @@
> > #include <linux/io.h>
> > #include <linux/module.h>
> > #include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > #include <linux/uaccess.h>
> >
> > #include <asm/cpu_device_id.h>
> > @@ -854,12 +855,59 @@ static const struct dmi_system_id pmc_core_dmi_table[] = {
> > {}
> > };
> >
> > -static int __init pmc_core_probe(void)
> > +static int pmc_core_probe(struct platform_device *pdev)
> > {
> > - struct pmc_dev *pmcdev = &pmc;
> > + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> > + int err;
> > +
> > + pmcdev->regbase = ioremap(pmcdev->base_addr,
> > + pmcdev->map->regmap_length);
> > + if (!pmcdev->regbase)
> > + return -ENOMEM;
> > +
> > + mutex_init(&pmcdev->lock);
> > + pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
> > +
> > + err = pmc_core_dbgfs_register(pmcdev);
> > + if (err < 0) {
> > + dev_warn(&pdev->dev, "debugfs register failed.\n");
> > + iounmap(pmcdev->regbase);
> > + return err;
> > + }
> > +
> > + dmi_check_system(pmc_core_dmi_table);
> > + dev_info(&pdev->dev, " initialized\n");
> > + return 0;
> > +}
> > +
> > +static int pmc_core_remove(struct platform_device *pdev)
> > +{
> > + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> > +
> > + pmc_core_dbgfs_unregister(pmcdev);
> > + mutex_destroy(&pmcdev->lock);
> > + iounmap(pmcdev->regbase);
> > + return 0;
> > +}
> > +
> > +static struct platform_driver pmc_core_driver = {
> > + .driver = {
> > + .name = "pmc_core",
> > + },
> > + .probe = pmc_core_probe,
> > + .remove = pmc_core_remove,
> > +};
> > +
> > +static struct platform_device pmc_core_device = {
> > + .name = "pmc_core",
> > +};
> > +
> > +static int __init pmc_core_init(void)
> > +{
> > + int ret;
>
> Please use reverse x-mas tree style.

OK, will do.
>
> > const struct x86_cpu_id *cpu_id;
> > + struct pmc_dev *pmcdev = &pmc;
> > u64 slp_s0_addr;
> > - int err;
> >
> > cpu_id = x86_match_cpu(intel_pmc_core_ids);
> > if (!cpu_id)
> > @@ -880,36 +928,31 @@ static int __init pmc_core_probe(void)
> > else
> > pmcdev->base_addr = slp_s0_addr - pmcdev->map->slp_s0_offset;
> >
> > - pmcdev->regbase = ioremap(pmcdev->base_addr,
> > - pmcdev->map->regmap_length);
> > - if (!pmcdev->regbase)
> > - return -ENOMEM;
> > + platform_set_drvdata(&pmc_core_device, pmcdev);
> >
> > - mutex_init(&pmcdev->lock);
> > - pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
> > + ret = platform_device_register(&pmc_core_device);
> > + if (ret)
> > + return ret;
> >
> > - err = pmc_core_dbgfs_register(pmcdev);
> > - if (err < 0) {
> > - pr_warn(" debugfs register failed.\n");
> > - iounmap(pmcdev->regbase);
> > - return err;
> > - }
> > + ret = platform_driver_register(&pmc_core_driver);
> > + if (ret)
> > + goto out_remove_dev;
> >
> > - dmi_check_system(pmc_core_dmi_table);
> > - pr_info(" initialized\n");
> > return 0;
> > +
> > +out_remove_dev:
> > + platform_device_unregister(&pmc_core_device);
> > + return ret;
> > }
> > -module_init(pmc_core_probe)
> >
> > -static void __exit pmc_core_remove(void)
> > +static void __init pmc_core_exit(void)
> > {
> > - struct pmc_dev *pmcdev = &pmc;
> > -
> > - pmc_core_dbgfs_unregister(pmcdev);
> > - mutex_destroy(&pmcdev->lock);
> > - iounmap(pmcdev->regbase);
> > + platform_driver_unregister(&pmc_core_driver);
> > + platform_device_unregister(&pmc_core_device);
> > }
> > -module_exit(pmc_core_remove)
> > +
> > +module_init(pmc_core_init);
> > +module_exit(pmc_core_exit);
> >
> > MODULE_LICENSE("GPL v2");
> > MODULE_DESCRIPTION("Intel PMC Core Driver");
> > --
> > 2.21.0.360.g471c308f928-goog
> >
>
> --
> Best Regards,
> Rajneesh

2019-03-18 15:14:20

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH 2/2] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

On Sat, Mar 16, 2019 at 1:43 AM Rajneesh Bhardwaj
<[email protected]> wrote:
>
> On Wed, Mar 13, 2019 at 03:21:24PM -0700, Rajat Jain wrote:
> > Add a module parameter which when enabled, will check on resume, if the
> > last S0ix attempt was successful. If not, the driver would provide
> > helpful debug information (which gets latched during the failed suspend
> > attempt) to debug the S0ix failure.
>
> + Srinivas and David.
>
> >
> > This information is very useful to debug S0ix failures. Specially since
> > the latched debug information will be lost (over-written) if the system
> > attempts to go into runtime (or imminent) S0ix again after that failed
> > suspend attempt.
> >
> > Signed-off-by: Rajat Jain <[email protected]>
> > ---
> > drivers/platform/x86/intel_pmc_core.c | 86 +++++++++++++++++++++++++++
> > drivers/platform/x86/intel_pmc_core.h | 7 +++
> > 2 files changed, 93 insertions(+)
> >
> > diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> > index 55578d07610c..b1f4405a27ce 100644
> > --- a/drivers/platform/x86/intel_pmc_core.c
> > +++ b/drivers/platform/x86/intel_pmc_core.c
> > @@ -20,6 +20,7 @@
> > #include <linux/module.h>
> > #include <linux/pci.h>
> > #include <linux/platform_device.h>
> > +#include <linux/suspend.h>
> > #include <linux/uaccess.h>
> >
> > #include <asm/cpu_device_id.h>
> > @@ -890,9 +891,94 @@ static int pmc_core_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_PM_SLEEP
> > +
> > +static bool warn_on_s0ix_failures;
> > +module_param(warn_on_s0ix_failures, bool, 0644);
> > +MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix failures");
> > +
> > +static int pmc_core_suspend(struct device *dev)
> > +{
> > + struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> > +
> > + /* Save PC10 and S0ix residency for checking later */
> > + if (warn_on_s0ix_failures &&
> > + !rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter) &&
> > + !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
> > + pmcdev->check_counters = true;
> > + else
> > + pmcdev->check_counters = false;
> > +
> > + return 0;
> > +}
> > +
> > +static inline bool pc10_failed(struct pmc_dev *pmcdev)
> > +{
> > + u64 pc10_counter;
> > +
> > + if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
> > + pc10_counter == pmcdev->pc10_counter)
> > + return true;
> > + else
> > + return false;
> > +}
> > +
> > +static inline bool s0ix_failed(struct pmc_dev *pmcdev)
> > +{
> > + u64 s0ix_counter;
> > +
> > + if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
> > + s0ix_counter == pmcdev->s0ix_counter)
> > + return true;
> > + else
> > + return false;
> > +}
> > +
> > +static int pmc_core_resume(struct device *dev)
> > +{
> > + struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> > +
> > + if (!pmcdev->check_counters)
> > + return 0;
> > +
> > + if (pc10_failed(pmcdev)) {
> > + dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
> > + pmcdev->pc10_counter);
> > + } else if (s0ix_failed(pmcdev)) {
> > +
> > + const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
>
> slps0_dbg feature is not available on Skylake and Kabylake. PCH IP power
> gating status is available on all.

Right, so the slps0_dbg_maps field is only populated for the platforms
that have the slps0_dbg feature. So it seems like the below code would
work well for both kind of platforms.

>
> > + const struct pmc_bit_map *map;
> > + int offset = pmcdev->map->slps0_dbg_offset;
> > + u32 data;
> > +
> > + dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
> > + pmcdev->s0ix_counter);
>
> I feel this dev_warn may be frowned upon and should be possible kept behind
> something like pm_debug_messages_on though module param helps mitigate it.

Right, in my mind, as the parameter name suggests, the whole intent of
the "warn_on_s0ix_failures" is to generate noticable warnings. Since
these warnings are off by default, and thus will only be generated
some one turns on the parameter, thus indicating he is interested in
being warned for failures.

Thanks,

Rajat

>
> > + while (*maps) {
> > + map = *maps;
> > + data = pmc_core_reg_read(pmcdev, offset);
> > + offset += 4;
> > + while (map->name) {
> > + dev_warn(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
> > + map->name,
> > + data & map->bit_mask ? "Yes" : "No");
> > + ++map;
> > + }
> > + ++maps;
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > +#endif
> > +
> > +const struct dev_pm_ops pmc_core_pm_ops = {
> > + SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend, pmc_core_resume)
> > +};
> > +
> > static struct platform_driver pmc_core_driver = {
> > .driver = {
> > .name = "pmc_core",
> > + .pm = &pmc_core_pm_ops
> > },
> > .probe = pmc_core_probe,
> > .remove = pmc_core_remove,
> > diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> > index 88d9c0653a5f..fdee5772e532 100644
> > --- a/drivers/platform/x86/intel_pmc_core.h
> > +++ b/drivers/platform/x86/intel_pmc_core.h
> > @@ -241,6 +241,9 @@ 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)
> > *
> > * pmc_dev contains info about power management controller device.
> > */
> > @@ -253,6 +256,10 @@ struct pmc_dev {
> > #endif /* CONFIG_DEBUG_FS */
> > int pmc_xram_read_bit;
> > struct mutex lock; /* generic mutex lock for PMC Core */
> > +
> > + bool check_counters; /* Check for counter increments on resume */
> > + u64 pc10_counter;
> > + u64 s0ix_counter;
> > };
> >
> > #endif /* PMC_CORE_H */
> > --
> > 2.21.0.360.g471c308f928-goog
> >
>
> --
> Best Regards,
> Rajneesh

2019-03-18 15:20:14

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH 2/2] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

On Mon, Mar 18, 2019 at 2:31 AM Somayaji, Vishwanath
<[email protected]> wrote:
>
>
>
> >-----Original Message-----
> >From: Rajat Jain <[email protected]>
> >Sent: Thursday, March 14, 2019 3:51 AM
> >To: Bhardwaj, Rajneesh <[email protected]>; Somayaji, Vishwanath
> ><[email protected]>; Darren Hart <[email protected]>; Andy
> >Shevchenko <[email protected]>; [email protected]; linux-
> >[email protected]
> >Cc: Rajat Jain <[email protected]>; [email protected];
> >[email protected]; [email protected]
> >Subject: [PATCH 2/2] platform/x86: intel_pmc_core: Allow to dump debug
> >registers on S0ix failure
> >
> >Add a module parameter which when enabled, will check on resume, if the
> >last S0ix attempt was successful. If not, the driver would provide
> >helpful debug information (which gets latched during the failed suspend
> >attempt) to debug the S0ix failure.
> >
> >This information is very useful to debug S0ix failures. Specially since
> >the latched debug information will be lost (over-written) if the system
> >attempts to go into runtime (or imminent) S0ix again after that failed
> >suspend attempt.
> >
> >Signed-off-by: Rajat Jain <[email protected]>
> >---
> > drivers/platform/x86/intel_pmc_core.c | 86 +++++++++++++++++++++++++++
> > drivers/platform/x86/intel_pmc_core.h | 7 +++
> > 2 files changed, 93 insertions(+)
> >
> >diff --git a/drivers/platform/x86/intel_pmc_core.c
> >b/drivers/platform/x86/intel_pmc_core.c
> >index 55578d07610c..b1f4405a27ce 100644
> >--- a/drivers/platform/x86/intel_pmc_core.c
> >+++ b/drivers/platform/x86/intel_pmc_core.c
> >@@ -20,6 +20,7 @@
> > #include <linux/module.h>
> > #include <linux/pci.h>
> > #include <linux/platform_device.h>
> >+#include <linux/suspend.h>
> > #include <linux/uaccess.h>
> >
> > #include <asm/cpu_device_id.h>
> >@@ -890,9 +891,94 @@ static int pmc_core_remove(struct platform_device
> >*pdev)
> > return 0;
> > }
> >
> >+#ifdef CONFIG_PM_SLEEP
> >+
> >+static bool warn_on_s0ix_failures;
> >+module_param(warn_on_s0ix_failures, bool, 0644);
> >+MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix
> >failures");
> >+
> >+static int pmc_core_suspend(struct device *dev)
> >+{
> >+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> >+
> >+ /* Save PC10 and S0ix residency for checking later */
> >+ if (warn_on_s0ix_failures &&
> >+ !rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter)
> >&&
> >+ !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
> >+ pmcdev->check_counters = true;
> >+ else
> >+ pmcdev->check_counters = false;
> >+
> >+ return 0;
> >+}
> >+
> >+static inline bool pc10_failed(struct pmc_dev *pmcdev)
> >+{
> >+ u64 pc10_counter;
> >+
> >+ if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
> >+ pc10_counter == pmcdev->pc10_counter)
> >+ return true;
> >+ else
> >+ return false;
> >+}
> >+
> >+static inline bool s0ix_failed(struct pmc_dev *pmcdev)
> >+{
> >+ u64 s0ix_counter;
> >+
> >+ if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
> >+ s0ix_counter == pmcdev->s0ix_counter)
> >+ return true;
> >+ else
> >+ return false;
> >+}
> >+
> >+static int pmc_core_resume(struct device *dev)
> >+{
> >+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> >+
> >+ if (!pmcdev->check_counters)
> >+ return 0;
> >+
> >+ if (pc10_failed(pmcdev)) {
> >+ dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
> >+ pmcdev->pc10_counter);
> >+ } else if (s0ix_failed(pmcdev)) {
> >+
> >+ const struct pmc_bit_map **maps = pmcdev->map-
> >>slps0_dbg_maps;
> >+ const struct pmc_bit_map *map;
> >+ int offset = pmcdev->map->slps0_dbg_offset;
> >+ u32 data;
> >+
> >+ dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
> >+ pmcdev->s0ix_counter);
> >+ while (*maps) {
> >+ map = *maps;
> >+ data = pmc_core_reg_read(pmcdev, offset);
> >+ offset += 4;
> >+ while (map->name) {
> >+ dev_warn(dev, "SLP_S0_DBG: %-32s\tState:
> >%s\n",
> >+ map->name,
> >+ data & map->bit_mask ? "Yes" : "No");
> >+ ++map;
> >+ }
> >+ ++maps;
> >+ }
> >+ }
> >+ return 0;
> >+}
> >+
> >+#endif
> >+
> >+const struct dev_pm_ops pmc_core_pm_ops = {
> >+ SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend,
> >pmc_core_resume)
> These PM Ops routines will be called not just in s2idle scenario, but also in other suspend scenarios like s2ram, s2disk. However actual functionalities served by these routines are relevant only for s2idle.
> That means we will end up having false errors in s2ram/s2disk scenarios as PC10/s0ix counters wont increment in those scenarios.

Yes, you are right. Currently there is no API for a driver to know
whether the *current suspend* attempt is targeting S0ix or S3. I was
hoping that the pm_suspend_via_s2idle() might tell us that but that is
not true. Note that this issue is mitigated by the expectation that
this parameter (warn_on_s0ix_failures) will only be enabled only on
platforms that use S0ix.

However, if this is a concern and there is a string sentiment around
it, I am happy to throw in a patch that adds such an API in the pm
core and uses it (I have a patch ready).

Thanks,

Rajat

>
> Vishwa
> >+};
> >+
> > static struct platform_driver pmc_core_driver = {
> > .driver = {
> > .name = "pmc_core",
> >+ .pm = &pmc_core_pm_ops
> > },
> > .probe = pmc_core_probe,
> > .remove = pmc_core_remove,
> >diff --git a/drivers/platform/x86/intel_pmc_core.h
> >b/drivers/platform/x86/intel_pmc_core.h
> >index 88d9c0653a5f..fdee5772e532 100644
> >--- a/drivers/platform/x86/intel_pmc_core.h
> >+++ b/drivers/platform/x86/intel_pmc_core.h
> >@@ -241,6 +241,9 @@ 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)
> > *
> > * pmc_dev contains info about power management controller device.
> > */
> >@@ -253,6 +256,10 @@ struct pmc_dev {
> > #endif /* CONFIG_DEBUG_FS */
> > int pmc_xram_read_bit;
> > struct mutex lock; /* generic mutex lock for PMC Core */
> >+
> >+ bool check_counters; /* Check for counter increments on resume */
> >+ u64 pc10_counter;
> >+ u64 s0ix_counter;
> > };
> >
> > #endif /* PMC_CORE_H */
> >--
> >2.21.0.360.g471c308f928-goog
>

2019-03-18 16:14:56

by Rajneesh Bhardwaj

[permalink] [raw]
Subject: Re: [PATCH 2/2] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

On Mon, Mar 18, 2019 at 08:18:56AM -0700, Rajat Jain wrote:
> On Mon, Mar 18, 2019 at 2:31 AM Somayaji, Vishwanath
> <[email protected]> wrote:
> >
> >
> >
> > >-----Original Message-----
> > >From: Rajat Jain <[email protected]>
> > >Sent: Thursday, March 14, 2019 3:51 AM
> > >To: Bhardwaj, Rajneesh <[email protected]>; Somayaji, Vishwanath
> > ><[email protected]>; Darren Hart <[email protected]>; Andy
> > >Shevchenko <[email protected]>; [email protected]; linux-
> > >[email protected]
> > >Cc: Rajat Jain <[email protected]>; [email protected];
> > >[email protected]; [email protected]
> > >Subject: [PATCH 2/2] platform/x86: intel_pmc_core: Allow to dump debug
> > >registers on S0ix failure
> > >
> > >Add a module parameter which when enabled, will check on resume, if the
> > >last S0ix attempt was successful. If not, the driver would provide
> > >helpful debug information (which gets latched during the failed suspend
> > >attempt) to debug the S0ix failure.
> > >
> > >This information is very useful to debug S0ix failures. Specially since
> > >the latched debug information will be lost (over-written) if the system
> > >attempts to go into runtime (or imminent) S0ix again after that failed
> > >suspend attempt.
> > >
> > >Signed-off-by: Rajat Jain <[email protected]>
> > >---
> > > drivers/platform/x86/intel_pmc_core.c | 86 +++++++++++++++++++++++++++
> > > drivers/platform/x86/intel_pmc_core.h | 7 +++
> > > 2 files changed, 93 insertions(+)
> > >
> > >diff --git a/drivers/platform/x86/intel_pmc_core.c
> > >b/drivers/platform/x86/intel_pmc_core.c
> > >index 55578d07610c..b1f4405a27ce 100644
> > >--- a/drivers/platform/x86/intel_pmc_core.c
> > >+++ b/drivers/platform/x86/intel_pmc_core.c
> > >@@ -20,6 +20,7 @@
> > > #include <linux/module.h>
> > > #include <linux/pci.h>
> > > #include <linux/platform_device.h>
> > >+#include <linux/suspend.h>
> > > #include <linux/uaccess.h>
> > >
> > > #include <asm/cpu_device_id.h>
> > >@@ -890,9 +891,94 @@ static int pmc_core_remove(struct platform_device
> > >*pdev)
> > > return 0;
> > > }
> > >
> > >+#ifdef CONFIG_PM_SLEEP
> > >+
> > >+static bool warn_on_s0ix_failures;
> > >+module_param(warn_on_s0ix_failures, bool, 0644);
> > >+MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix
> > >failures");
> > >+
> > >+static int pmc_core_suspend(struct device *dev)
> > >+{
> > >+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> > >+
> > >+ /* Save PC10 and S0ix residency for checking later */
> > >+ if (warn_on_s0ix_failures &&
> > >+ !rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter)
> > >&&
> > >+ !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
> > >+ pmcdev->check_counters = true;
> > >+ else
> > >+ pmcdev->check_counters = false;
> > >+
> > >+ return 0;
> > >+}
> > >+
> > >+static inline bool pc10_failed(struct pmc_dev *pmcdev)
> > >+{
> > >+ u64 pc10_counter;
> > >+
> > >+ if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
> > >+ pc10_counter == pmcdev->pc10_counter)
> > >+ return true;
> > >+ else
> > >+ return false;
> > >+}
> > >+
> > >+static inline bool s0ix_failed(struct pmc_dev *pmcdev)
> > >+{
> > >+ u64 s0ix_counter;
> > >+
> > >+ if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
> > >+ s0ix_counter == pmcdev->s0ix_counter)
> > >+ return true;
> > >+ else
> > >+ return false;
> > >+}
> > >+
> > >+static int pmc_core_resume(struct device *dev)
> > >+{
> > >+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> > >+
> > >+ if (!pmcdev->check_counters)
> > >+ return 0;
> > >+
> > >+ if (pc10_failed(pmcdev)) {
> > >+ dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
> > >+ pmcdev->pc10_counter);
> > >+ } else if (s0ix_failed(pmcdev)) {
> > >+
> > >+ const struct pmc_bit_map **maps = pmcdev->map-
> > >>slps0_dbg_maps;
> > >+ const struct pmc_bit_map *map;
> > >+ int offset = pmcdev->map->slps0_dbg_offset;
> > >+ u32 data;
> > >+
> > >+ dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
> > >+ pmcdev->s0ix_counter);
> > >+ while (*maps) {
> > >+ map = *maps;
> > >+ data = pmc_core_reg_read(pmcdev, offset);
> > >+ offset += 4;
> > >+ while (map->name) {
> > >+ dev_warn(dev, "SLP_S0_DBG: %-32s\tState:
> > >%s\n",
> > >+ map->name,
> > >+ data & map->bit_mask ? "Yes" : "No");
> > >+ ++map;
> > >+ }
> > >+ ++maps;
> > >+ }
> > >+ }
> > >+ return 0;
> > >+}
> > >+
> > >+#endif
> > >+
> > >+const struct dev_pm_ops pmc_core_pm_ops = {
> > >+ SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend,
> > >pmc_core_resume)
> > These PM Ops routines will be called not just in s2idle scenario, but also in other suspend scenarios like s2ram, s2disk. However actual functionalities served by these routines are relevant only for s2idle.
> > That means we will end up having false errors in s2ram/s2disk scenarios as PC10/s0ix counters wont increment in those scenarios.
>
> Yes, you are right. Currently there is no API for a driver to know
> whether the *current suspend* attempt is targeting S0ix or S3. I was
> hoping that the pm_suspend_via_s2idle() might tell us that but that is
> not true. Note that this issue is mitigated by the expectation that
> this parameter (warn_on_s0ix_failures) will only be enabled only on
> platforms that use S0ix.

Maybe we can use ACPI_FADT_LOW_POWER_S0 also as a condition to dump this
data though callback is best way to check in my opinion.

Adding Srinivas, David and Rafael.

>
> However, if this is a concern and there is a string sentiment around
> it, I am happy to throw in a patch that adds such an API in the pm
> core and uses it (I have a patch ready).




>
> Thanks,
>
> Rajat
>
> >
> > Vishwa
> > >+};
> > >+
> > > static struct platform_driver pmc_core_driver = {
> > > .driver = {
> > > .name = "pmc_core",
> > >+ .pm = &pmc_core_pm_ops
> > > },
> > > .probe = pmc_core_probe,
> > > .remove = pmc_core_remove,
> > >diff --git a/drivers/platform/x86/intel_pmc_core.h
> > >b/drivers/platform/x86/intel_pmc_core.h
> > >index 88d9c0653a5f..fdee5772e532 100644
> > >--- a/drivers/platform/x86/intel_pmc_core.h
> > >+++ b/drivers/platform/x86/intel_pmc_core.h
> > >@@ -241,6 +241,9 @@ 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)
> > > *
> > > * pmc_dev contains info about power management controller device.
> > > */
> > >@@ -253,6 +256,10 @@ struct pmc_dev {
> > > #endif /* CONFIG_DEBUG_FS */
> > > int pmc_xram_read_bit;
> > > struct mutex lock; /* generic mutex lock for PMC Core */
> > >+
> > >+ bool check_counters; /* Check for counter increments on resume */
> > >+ u64 pc10_counter;
> > >+ u64 s0ix_counter;
> > > };
> > >
> > > #endif /* PMC_CORE_H */
> > >--
> > >2.21.0.360.g471c308f928-goog
> >

--
Best Regards,
Rajneesh

2019-03-20 01:05:32

by Rajat Jain

[permalink] [raw]
Subject: [PATCH 1/2] platform/x86: intel_pmc_core: Convert to a platform_driver

Convert the intel_pmc_core driver to a platform driver. There is no
functional change. Some code that tried to determine the kind of
CPU, has been moved from pmc_core_probe() to pmc_core_init().

Signed-off-by: Rajat Jain <[email protected]>
---
v2: Rephrase the commit log. No code changes.

drivers/platform/x86/intel_pmc_core.c | 93 ++++++++++++++++++++-------
1 file changed, 68 insertions(+), 25 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index f2c621b55f49..55578d07610c 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -19,6 +19,7 @@
#include <linux/io.h>
#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/platform_device.h>
#include <linux/uaccess.h>

#include <asm/cpu_device_id.h>
@@ -854,12 +855,59 @@ static const struct dmi_system_id pmc_core_dmi_table[] = {
{}
};

-static int __init pmc_core_probe(void)
+static int pmc_core_probe(struct platform_device *pdev)
{
- struct pmc_dev *pmcdev = &pmc;
+ struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
+ int err;
+
+ pmcdev->regbase = ioremap(pmcdev->base_addr,
+ pmcdev->map->regmap_length);
+ if (!pmcdev->regbase)
+ return -ENOMEM;
+
+ mutex_init(&pmcdev->lock);
+ pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
+
+ err = pmc_core_dbgfs_register(pmcdev);
+ if (err < 0) {
+ dev_warn(&pdev->dev, "debugfs register failed.\n");
+ iounmap(pmcdev->regbase);
+ return err;
+ }
+
+ dmi_check_system(pmc_core_dmi_table);
+ dev_info(&pdev->dev, " initialized\n");
+ return 0;
+}
+
+static int pmc_core_remove(struct platform_device *pdev)
+{
+ struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
+
+ pmc_core_dbgfs_unregister(pmcdev);
+ mutex_destroy(&pmcdev->lock);
+ iounmap(pmcdev->regbase);
+ return 0;
+}
+
+static struct platform_driver pmc_core_driver = {
+ .driver = {
+ .name = "pmc_core",
+ },
+ .probe = pmc_core_probe,
+ .remove = pmc_core_remove,
+};
+
+static struct platform_device pmc_core_device = {
+ .name = "pmc_core",
+};
+
+static int __init pmc_core_init(void)
+{
+ int ret;
const struct x86_cpu_id *cpu_id;
+ struct pmc_dev *pmcdev = &pmc;
u64 slp_s0_addr;
- int err;

cpu_id = x86_match_cpu(intel_pmc_core_ids);
if (!cpu_id)
@@ -880,36 +928,31 @@ static int __init pmc_core_probe(void)
else
pmcdev->base_addr = slp_s0_addr - pmcdev->map->slp_s0_offset;

- pmcdev->regbase = ioremap(pmcdev->base_addr,
- pmcdev->map->regmap_length);
- if (!pmcdev->regbase)
- return -ENOMEM;
+ platform_set_drvdata(&pmc_core_device, pmcdev);

- mutex_init(&pmcdev->lock);
- pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
+ ret = platform_device_register(&pmc_core_device);
+ if (ret)
+ return ret;

- err = pmc_core_dbgfs_register(pmcdev);
- if (err < 0) {
- pr_warn(" debugfs register failed.\n");
- iounmap(pmcdev->regbase);
- return err;
- }
+ ret = platform_driver_register(&pmc_core_driver);
+ if (ret)
+ goto out_remove_dev;

- dmi_check_system(pmc_core_dmi_table);
- pr_info(" initialized\n");
return 0;
+
+out_remove_dev:
+ platform_device_unregister(&pmc_core_device);
+ return ret;
}
-module_init(pmc_core_probe)

-static void __exit pmc_core_remove(void)
+static void __init pmc_core_exit(void)
{
- struct pmc_dev *pmcdev = &pmc;
-
- pmc_core_dbgfs_unregister(pmcdev);
- mutex_destroy(&pmcdev->lock);
- iounmap(pmcdev->regbase);
+ platform_driver_unregister(&pmc_core_driver);
+ platform_device_unregister(&pmc_core_device);
}
-module_exit(pmc_core_remove)
+
+module_init(pmc_core_init);
+module_exit(pmc_core_exit);

MODULE_LICENSE("GPL v2");
MODULE_DESCRIPTION("Intel PMC Core Driver");
--
2.21.0.225.g810b269d1ac-goog


2019-03-20 01:07:06

by Rajat Jain

[permalink] [raw]
Subject: [PATCH 2/2] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

Add a module parameter which when enabled, will check on resume, if the
last S0ix attempt was successful. If not, the driver would warn and provide
helpful debug information (which gets latched during the failed suspend
attempt) to debug the S0ix failure.

This information is very useful to debug S0ix failures. Specially since
the latched debug information will be lost (over-written) if the system
attempts to go into runtime (or imminent) S0ix again after that failed
suspend attempt.

Signed-off-by: Rajat Jain <[email protected]>
---
v2: Use pm_suspend_via_firmware() to enable the check only for S0ix
(I think).

drivers/platform/x86/intel_pmc_core.c | 86 +++++++++++++++++++++++++++
drivers/platform/x86/intel_pmc_core.h | 7 +++
2 files changed, 93 insertions(+)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 55578d07610c..0ab893fac4bb 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -20,6 +20,7 @@
#include <linux/module.h>
#include <linux/pci.h>
#include <linux/platform_device.h>
+#include <linux/suspend.h>
#include <linux/uaccess.h>

#include <asm/cpu_device_id.h>
@@ -890,9 +891,94 @@ static int pmc_core_remove(struct platform_device *pdev)
return 0;
}

+#ifdef CONFIG_PM_SLEEP
+
+static bool warn_on_s0ix_failures;
+module_param(warn_on_s0ix_failures, bool, 0644);
+MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix failures");
+
+static int pmc_core_suspend(struct device *dev)
+{
+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
+
+ /* Save PC10 and S0ix residency for checking later */
+ if (warn_on_s0ix_failures && !pm_suspend_via_firmware() &&
+ !rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter) &&
+ !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
+ pmcdev->check_counters = true;
+ else
+ pmcdev->check_counters = false;
+
+ return 0;
+}
+
+static inline bool pc10_failed(struct pmc_dev *pmcdev)
+{
+ u64 pc10_counter;
+
+ if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
+ pc10_counter == pmcdev->pc10_counter)
+ return true;
+ else
+ return false;
+}
+
+static inline bool s0ix_failed(struct pmc_dev *pmcdev)
+{
+ u64 s0ix_counter;
+
+ if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
+ s0ix_counter == pmcdev->s0ix_counter)
+ return true;
+ else
+ return false;
+}
+
+static int pmc_core_resume(struct device *dev)
+{
+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
+
+ if (!pmcdev->check_counters)
+ return 0;
+
+ if (pc10_failed(pmcdev)) {
+ dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
+ pmcdev->pc10_counter);
+ } else if (s0ix_failed(pmcdev)) {
+
+ const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
+ const struct pmc_bit_map *map;
+ int offset = pmcdev->map->slps0_dbg_offset;
+ u32 data;
+
+ dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
+ pmcdev->s0ix_counter);
+ while (*maps) {
+ map = *maps;
+ data = pmc_core_reg_read(pmcdev, offset);
+ offset += 4;
+ while (map->name) {
+ dev_warn(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
+ map->name,
+ data & map->bit_mask ? "Yes" : "No");
+ ++map;
+ }
+ ++maps;
+ }
+ }
+ return 0;
+}
+
+#endif
+
+const struct dev_pm_ops pmc_core_pm_ops = {
+ SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend, pmc_core_resume)
+};
+
static struct platform_driver pmc_core_driver = {
.driver = {
.name = "pmc_core",
+ .pm = &pmc_core_pm_ops
},
.probe = pmc_core_probe,
.remove = pmc_core_remove,
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 88d9c0653a5f..fdee5772e532 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -241,6 +241,9 @@ 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)
*
* pmc_dev contains info about power management controller device.
*/
@@ -253,6 +256,10 @@ struct pmc_dev {
#endif /* CONFIG_DEBUG_FS */
int pmc_xram_read_bit;
struct mutex lock; /* generic mutex lock for PMC Core */
+
+ bool check_counters; /* Check for counter increments on resume */
+ u64 pc10_counter;
+ u64 s0ix_counter;
};

#endif /* PMC_CORE_H */
--
2.21.0.225.g810b269d1ac-goog


2019-03-20 10:38:42

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH 2/2] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

On Monday, March 18, 2019 5:01:06 PM CET Rajneesh Bhardwaj wrote:
> On Mon, Mar 18, 2019 at 08:18:56AM -0700, Rajat Jain wrote:
> > On Mon, Mar 18, 2019 at 2:31 AM Somayaji, Vishwanath
> > <[email protected]> wrote:
> > >
> > >
> > >
> > > >-----Original Message-----
> > > >From: Rajat Jain <[email protected]>
> > > >Sent: Thursday, March 14, 2019 3:51 AM
> > > >To: Bhardwaj, Rajneesh <[email protected]>; Somayaji, Vishwanath
> > > ><[email protected]>; Darren Hart <[email protected]>; Andy
> > > >Shevchenko <[email protected]>; [email protected]; linux-
> > > >[email protected]
> > > >Cc: Rajat Jain <[email protected]>; [email protected];
> > > >[email protected]; [email protected]
> > > >Subject: [PATCH 2/2] platform/x86: intel_pmc_core: Allow to dump debug
> > > >registers on S0ix failure
> > > >
> > > >Add a module parameter which when enabled, will check on resume, if the
> > > >last S0ix attempt was successful. If not, the driver would provide
> > > >helpful debug information (which gets latched during the failed suspend
> > > >attempt) to debug the S0ix failure.
> > > >
> > > >This information is very useful to debug S0ix failures. Specially since
> > > >the latched debug information will be lost (over-written) if the system
> > > >attempts to go into runtime (or imminent) S0ix again after that failed
> > > >suspend attempt.
> > > >
> > > >Signed-off-by: Rajat Jain <[email protected]>
> > > >---
> > > > drivers/platform/x86/intel_pmc_core.c | 86 +++++++++++++++++++++++++++
> > > > drivers/platform/x86/intel_pmc_core.h | 7 +++
> > > > 2 files changed, 93 insertions(+)
> > > >
> > > >diff --git a/drivers/platform/x86/intel_pmc_core.c
> > > >b/drivers/platform/x86/intel_pmc_core.c
> > > >index 55578d07610c..b1f4405a27ce 100644
> > > >--- a/drivers/platform/x86/intel_pmc_core.c
> > > >+++ b/drivers/platform/x86/intel_pmc_core.c
> > > >@@ -20,6 +20,7 @@
> > > > #include <linux/module.h>
> > > > #include <linux/pci.h>
> > > > #include <linux/platform_device.h>
> > > >+#include <linux/suspend.h>
> > > > #include <linux/uaccess.h>
> > > >
> > > > #include <asm/cpu_device_id.h>
> > > >@@ -890,9 +891,94 @@ static int pmc_core_remove(struct platform_device
> > > >*pdev)
> > > > return 0;
> > > > }
> > > >
> > > >+#ifdef CONFIG_PM_SLEEP
> > > >+
> > > >+static bool warn_on_s0ix_failures;
> > > >+module_param(warn_on_s0ix_failures, bool, 0644);
> > > >+MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix
> > > >failures");
> > > >+
> > > >+static int pmc_core_suspend(struct device *dev)
> > > >+{
> > > >+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> > > >+
> > > >+ /* Save PC10 and S0ix residency for checking later */
> > > >+ if (warn_on_s0ix_failures &&
> > > >+ !rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter)
> > > >&&
> > > >+ !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
> > > >+ pmcdev->check_counters = true;
> > > >+ else
> > > >+ pmcdev->check_counters = false;
> > > >+
> > > >+ return 0;
> > > >+}
> > > >+
> > > >+static inline bool pc10_failed(struct pmc_dev *pmcdev)
> > > >+{
> > > >+ u64 pc10_counter;
> > > >+
> > > >+ if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
> > > >+ pc10_counter == pmcdev->pc10_counter)
> > > >+ return true;
> > > >+ else
> > > >+ return false;
> > > >+}
> > > >+
> > > >+static inline bool s0ix_failed(struct pmc_dev *pmcdev)
> > > >+{
> > > >+ u64 s0ix_counter;
> > > >+
> > > >+ if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
> > > >+ s0ix_counter == pmcdev->s0ix_counter)
> > > >+ return true;
> > > >+ else
> > > >+ return false;
> > > >+}
> > > >+
> > > >+static int pmc_core_resume(struct device *dev)
> > > >+{
> > > >+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> > > >+
> > > >+ if (!pmcdev->check_counters)
> > > >+ return 0;
> > > >+
> > > >+ if (pc10_failed(pmcdev)) {
> > > >+ dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
> > > >+ pmcdev->pc10_counter);
> > > >+ } else if (s0ix_failed(pmcdev)) {
> > > >+
> > > >+ const struct pmc_bit_map **maps = pmcdev->map-
> > > >>slps0_dbg_maps;
> > > >+ const struct pmc_bit_map *map;
> > > >+ int offset = pmcdev->map->slps0_dbg_offset;
> > > >+ u32 data;
> > > >+
> > > >+ dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
> > > >+ pmcdev->s0ix_counter);
> > > >+ while (*maps) {
> > > >+ map = *maps;
> > > >+ data = pmc_core_reg_read(pmcdev, offset);
> > > >+ offset += 4;
> > > >+ while (map->name) {
> > > >+ dev_warn(dev, "SLP_S0_DBG: %-32s\tState:
> > > >%s\n",
> > > >+ map->name,
> > > >+ data & map->bit_mask ? "Yes" : "No");
> > > >+ ++map;
> > > >+ }
> > > >+ ++maps;
> > > >+ }
> > > >+ }
> > > >+ return 0;
> > > >+}
> > > >+
> > > >+#endif
> > > >+
> > > >+const struct dev_pm_ops pmc_core_pm_ops = {
> > > >+ SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend,
> > > >pmc_core_resume)
> > > These PM Ops routines will be called not just in s2idle scenario, but also in other suspend scenarios like s2ram, s2disk. However actual functionalities served by these routines are relevant only for s2idle.
> > > That means we will end up having false errors in s2ram/s2disk scenarios as PC10/s0ix counters wont increment in those scenarios.
> >
> > Yes, you are right. Currently there is no API for a driver to know
> > whether the *current suspend* attempt is targeting S0ix or S3.

As a matter of fact, if pm_suspend_via_firmware() returns "true", then
S0ix cannot be the target.

However, you cannot say whether or not S0ix is the target if
pm_suspend_via_firmware() returns "false".

I guess that is the problem here?

> > I was hoping that the pm_suspend_via_s2idle() might tell us that but
> > that is not true. Note that this issue is mitigated by the expectation
> > that this parameter (warn_on_s0ix_failures) will only be enabled only on
> > platforms that use S0ix.
>
> Maybe we can use ACPI_FADT_LOW_POWER_S0 also as a condition to dump this
> data though callback is best way to check in my opinion.
>
> Adding Srinivas, David and Rafael.

You cannot really say whether or not a platform is going to use S0ix, as
S0ix technically is an extension of the idle path.

What happens is that the last non-idle CPU requests C10 and the SoC
decides how to handle that request. There are many possible things it
may do then.

AFAICS, ACPI_FADT_LOW_POWER_S0 set doesn't need to mean that S0ix is possible
(user space may disable C10 via sysfs, for example), but at least it shouldn't
be unset in that case, so it can be used as an indicator of S0ix availability.

> > However, if this is a concern and there is a string sentiment around
> > it, I am happy to throw in a patch that adds such an API in the pm
> > core and uses it (I have a patch ready).

That API is there already AFAICS.


2019-03-20 19:06:05

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH 2/2] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

Hi Rafael,
On Wed, Mar 20, 2019 at 3:37 AM Rafael J. Wysocki <[email protected]> wrote:
>
> On Monday, March 18, 2019 5:01:06 PM CET Rajneesh Bhardwaj wrote:
> > On Mon, Mar 18, 2019 at 08:18:56AM -0700, Rajat Jain wrote:
> > > On Mon, Mar 18, 2019 at 2:31 AM Somayaji, Vishwanath
> > > <[email protected]> wrote:
> > > >
> > > >
> > > >
> > > > >-----Original Message-----
> > > > >From: Rajat Jain <[email protected]>
> > > > >Sent: Thursday, March 14, 2019 3:51 AM
> > > > >To: Bhardwaj, Rajneesh <[email protected]>; Somayaji, Vishwanath
> > > > ><[email protected]>; Darren Hart <[email protected]>; Andy
> > > > >Shevchenko <[email protected]>; [email protected]; linux-
> > > > >[email protected]
> > > > >Cc: Rajat Jain <[email protected]>; [email protected];
> > > > >[email protected]; [email protected]
> > > > >Subject: [PATCH 2/2] platform/x86: intel_pmc_core: Allow to dump debug
> > > > >registers on S0ix failure
> > > > >
> > > > >Add a module parameter which when enabled, will check on resume, if the
> > > > >last S0ix attempt was successful. If not, the driver would provide
> > > > >helpful debug information (which gets latched during the failed suspend
> > > > >attempt) to debug the S0ix failure.
> > > > >
> > > > >This information is very useful to debug S0ix failures. Specially since
> > > > >the latched debug information will be lost (over-written) if the system
> > > > >attempts to go into runtime (or imminent) S0ix again after that failed
> > > > >suspend attempt.
> > > > >
> > > > >Signed-off-by: Rajat Jain <[email protected]>
> > > > >---
> > > > > drivers/platform/x86/intel_pmc_core.c | 86 +++++++++++++++++++++++++++
> > > > > drivers/platform/x86/intel_pmc_core.h | 7 +++
> > > > > 2 files changed, 93 insertions(+)
> > > > >
> > > > >diff --git a/drivers/platform/x86/intel_pmc_core.c
> > > > >b/drivers/platform/x86/intel_pmc_core.c
> > > > >index 55578d07610c..b1f4405a27ce 100644
> > > > >--- a/drivers/platform/x86/intel_pmc_core.c
> > > > >+++ b/drivers/platform/x86/intel_pmc_core.c
> > > > >@@ -20,6 +20,7 @@
> > > > > #include <linux/module.h>
> > > > > #include <linux/pci.h>
> > > > > #include <linux/platform_device.h>
> > > > >+#include <linux/suspend.h>
> > > > > #include <linux/uaccess.h>
> > > > >
> > > > > #include <asm/cpu_device_id.h>
> > > > >@@ -890,9 +891,94 @@ static int pmc_core_remove(struct platform_device
> > > > >*pdev)
> > > > > return 0;
> > > > > }
> > > > >
> > > > >+#ifdef CONFIG_PM_SLEEP
> > > > >+
> > > > >+static bool warn_on_s0ix_failures;
> > > > >+module_param(warn_on_s0ix_failures, bool, 0644);
> > > > >+MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix
> > > > >failures");
> > > > >+
> > > > >+static int pmc_core_suspend(struct device *dev)
> > > > >+{
> > > > >+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> > > > >+
> > > > >+ /* Save PC10 and S0ix residency for checking later */
> > > > >+ if (warn_on_s0ix_failures &&
> > > > >+ !rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter)
> > > > >&&
> > > > >+ !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
> > > > >+ pmcdev->check_counters = true;
> > > > >+ else
> > > > >+ pmcdev->check_counters = false;
> > > > >+
> > > > >+ return 0;
> > > > >+}
> > > > >+
> > > > >+static inline bool pc10_failed(struct pmc_dev *pmcdev)
> > > > >+{
> > > > >+ u64 pc10_counter;
> > > > >+
> > > > >+ if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
> > > > >+ pc10_counter == pmcdev->pc10_counter)
> > > > >+ return true;
> > > > >+ else
> > > > >+ return false;
> > > > >+}
> > > > >+
> > > > >+static inline bool s0ix_failed(struct pmc_dev *pmcdev)
> > > > >+{
> > > > >+ u64 s0ix_counter;
> > > > >+
> > > > >+ if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
> > > > >+ s0ix_counter == pmcdev->s0ix_counter)
> > > > >+ return true;
> > > > >+ else
> > > > >+ return false;
> > > > >+}
> > > > >+
> > > > >+static int pmc_core_resume(struct device *dev)
> > > > >+{
> > > > >+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> > > > >+
> > > > >+ if (!pmcdev->check_counters)
> > > > >+ return 0;
> > > > >+
> > > > >+ if (pc10_failed(pmcdev)) {
> > > > >+ dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
> > > > >+ pmcdev->pc10_counter);
> > > > >+ } else if (s0ix_failed(pmcdev)) {
> > > > >+
> > > > >+ const struct pmc_bit_map **maps = pmcdev->map-
> > > > >>slps0_dbg_maps;
> > > > >+ const struct pmc_bit_map *map;
> > > > >+ int offset = pmcdev->map->slps0_dbg_offset;
> > > > >+ u32 data;
> > > > >+
> > > > >+ dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
> > > > >+ pmcdev->s0ix_counter);
> > > > >+ while (*maps) {
> > > > >+ map = *maps;
> > > > >+ data = pmc_core_reg_read(pmcdev, offset);
> > > > >+ offset += 4;
> > > > >+ while (map->name) {
> > > > >+ dev_warn(dev, "SLP_S0_DBG: %-32s\tState:
> > > > >%s\n",
> > > > >+ map->name,
> > > > >+ data & map->bit_mask ? "Yes" : "No");
> > > > >+ ++map;
> > > > >+ }
> > > > >+ ++maps;
> > > > >+ }
> > > > >+ }
> > > > >+ return 0;
> > > > >+}
> > > > >+
> > > > >+#endif
> > > > >+
> > > > >+const struct dev_pm_ops pmc_core_pm_ops = {
> > > > >+ SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend,
> > > > >pmc_core_resume)
> > > > These PM Ops routines will be called not just in s2idle scenario, but also in other suspend scenarios like s2ram, s2disk. However actual functionalities served by these routines are relevant only for s2idle.
> > > > That means we will end up having false errors in s2ram/s2disk scenarios as PC10/s0ix counters wont increment in those scenarios.
> > >
> > > Yes, you are right. Currently there is no API for a driver to know
> > > whether the *current suspend* attempt is targeting S0ix or S3.
>
> As a matter of fact, if pm_suspend_via_firmware() returns "true", then
> S0ix cannot be the target.
>
> However, you cannot say whether or not S0ix is the target if
> pm_suspend_via_firmware() returns "false".
>
> I guess that is the problem here?

Yes, I had sent in a v2 yesterday using pm_suspend_via_firmware() but
I forgot to copy you. Can you please take a look at if this is better:
https://patchwork.kernel.org/patch/10860657/

>
> > > I was hoping that the pm_suspend_via_s2idle() might tell us that but
> > > that is not true. Note that this issue is mitigated by the expectation
> > > that this parameter (warn_on_s0ix_failures) will only be enabled only on
> > > platforms that use S0ix.
> >
> > Maybe we can use ACPI_FADT_LOW_POWER_S0 also as a condition to dump this
> > data though callback is best way to check in my opinion.
> >
> > Adding Srinivas, David and Rafael.
>
> You cannot really say whether or not a platform is going to use S0ix, as
> S0ix technically is an extension of the idle path.
>
> What happens is that the last non-idle CPU requests C10 and the SoC
> decides how to handle that request. There are many possible things it
> may do then.
>
> AFAICS, ACPI_FADT_LOW_POWER_S0 set doesn't need to mean that S0ix is possible
> (user space may disable C10 via sysfs, for example), but at least it shouldn't
> be unset in that case, so it can be used as an indicator of S0ix availability.

So, it seems to be the suggestion is to check for
ACPI_FADT_LOW_POWER_S0 in addition to pm_suspend_via_firmware()? If I
understand it right, the ACPI_FADT_LOW_POWER_S0 indicates the platform
capability of going into S0ix. So may be it makes more sense to check
for that flag at the init time. Does anyone feel that this driver
needs to be loaded on systems that do not have ACPI_FADT_LOW_POWER_S0?

>
> > > However, if this is a concern and there is a string sentiment around
> > > it, I am happy to throw in a patch that adds such an API in the pm
> > > core and uses it (I have a patch ready).
>
> That API is there already AFAICS.
>

I assume you mean pm_suspend_via_firmware()? I've used that in my v2,
can you please see if that makes it more acceptable?

Thanks,

Rajat

2019-03-23 00:31:37

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH 1/2] platform/x86: intel_pmc_core: Convert to a platform_driver

Hi Rajneesh,



On Fri, Mar 22, 2019 at 12:56 PM Bhardwaj, Rajneesh
<[email protected]> wrote:
>
> Some suggestions below
>
> On 18-Mar-19 8:36 PM, Rajat Jain wrote:
>
> On Sat, Mar 16, 2019 at 1:30 AM Rajneesh Bhardwaj
> <[email protected]> wrote:
>
> On Wed, Mar 13, 2019 at 03:21:23PM -0700, Rajat Jain wrote:
>
> Convert the intel_pmc_core driver to a platform driver. There is no
> functional change. Some code that tries to determine what kind of
> CPU this is, has been moved code is moved from pmc_core_probe() to
>
> Possible typo here.
>
> Ummm, you mean grammar error I guess? Sure, I will rephrase.
>
> pmc_core_init().
>
> Signed-off-by: Rajat Jain <[email protected]>
>
> Thanks for sending this. This is certainly useful to support suspend-resume
> functionality for this driver which is otherwise only possible with PM
> notifiers otherwise and that is not desirable. Initially this was a PCI
> driver and after design discussion it was converted to module. I would like
> to consult Andy and Srinivas for their opinion about binding it to actual
> platform bus instead of the virtual bus as in its current form. In one of the
> internal versions, we used a known acpi PNP HID.
>
> Sure, if there is an established ACPI PNP HID, then we could bind it
> using that, on platforms where we are still developing BIOS /
> coreboot. However, this might not be possible for shipping systems
> (Kabylake / skylake) where there is no plan to change the BIOS.
>
> In one of our internal patches, i had used HID of power engine plugin. IIRC, During my testing it was working on KBL, CNL with UEFI BIOS but i highly recommend testing it.
>
> ---8<----8<-----
>
> +static const struct acpi_device_id pmc_acpi_ids[] = {
>
> + {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
>
> + { }
>
> };

We do not have this device in any of our ACPI tables today. If Intel
can confirm that this is a well known HID to be used for attaching
this driver, we can start putting it on our platform's ACPI going
forward (Whiskeylake, Cometlake, Cannonlake, Icelake ...). But I
believe we also need to have this driver attach with the device on
older platforms (Skylake, Kabylake, Amberlake) that are already
shipping, and running a Non UEFI BIOS (that may not have this HID
since it is not published).

Currently the intel_pmc_core driver attaches itself to the following
table of CPU families, without regard to whether it has that HID in
the ACPI or not:

static const struct x86_cpu_id intel_pmc_core_ids[] = {
INTEL_CPU_FAM6(SKYLAKE_MOBILE, spt_reg_map),
INTEL_CPU_FAM6(SKYLAKE_DESKTOP, spt_reg_map),
INTEL_CPU_FAM6(KABYLAKE_MOBILE, spt_reg_map),
INTEL_CPU_FAM6(KABYLAKE_DESKTOP, spt_reg_map),
INTEL_CPU_FAM6(CANNONLAKE_MOBILE, cnp_reg_map),
INTEL_CPU_FAM6(ICELAKE_MOBILE, icl_reg_map),
{}
};

So to avoid a regression, I suggest that we still maintain the above
table (may be eliminate few entries) and always attach if the CPU is
among the table, and if the CPU is not among the table, use the ACPI
HID to attach. I propose to attach to at least Skylake and Kabylake
systems using the table above, and for Canonlake and Icelake and
newer, we can rely on BIOS providing the ACPI HID. Of course I do not
know if all non-Google Canonlake/Icelake platforms will have this HID
in their BIOS. If we are not sure, we can include Canonlake and
Icelake also in that list, an. Please let me know what do you think.

Thanks,

Rajat

>
>
>
> -builtin_pci_driver(intel_pmc_core_driver);
>
> +static struct platform_driver pmc_plat_driver = {
>
> + .remove = pmc_plat_remove,
>
> + .probe = pmc_plat_probe,
>
> + .driver = {
>
> + .name = "pmc_core_driver",
>
> + .acpi_match_table = ACPI_PTR(pmc_acpi_ids),
>
> + },
>
> +};
>
> ---
> This is rebased off
> git://git.infradead.org/linux-platform-drivers-x86.git/for-next
>
> drivers/platform/x86/intel_pmc_core.c | 93 ++++++++++++++++++++-------
> 1 file changed, 68 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index f2c621b55f49..55578d07610c 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -19,6 +19,7 @@
> #include <linux/io.h>
> #include <linux/module.h>
> #include <linux/pci.h>
> +#include <linux/platform_device.h>
> #include <linux/uaccess.h>
>
> #include <asm/cpu_device_id.h>
> @@ -854,12 +855,59 @@ static const struct dmi_system_id pmc_core_dmi_table[] = {
> {}
> };
>
> -static int __init pmc_core_probe(void)
> +static int pmc_core_probe(struct platform_device *pdev)
> {
> - struct pmc_dev *pmcdev = &pmc;
> + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> + int err;
> +
> + pmcdev->regbase = ioremap(pmcdev->base_addr,
> + pmcdev->map->regmap_length);
> + if (!pmcdev->regbase)
> + return -ENOMEM;
> +
> + mutex_init(&pmcdev->lock);
> + pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
> +
> + err = pmc_core_dbgfs_register(pmcdev);
> + if (err < 0) {
> + dev_warn(&pdev->dev, "debugfs register failed.\n");
> + iounmap(pmcdev->regbase);
> + return err;
> + }
> +
> + dmi_check_system(pmc_core_dmi_table);
> + dev_info(&pdev->dev, " initialized\n");
> + return 0;
> +}
> +
> +static int pmc_core_remove(struct platform_device *pdev)
> +{
> + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> +
> + pmc_core_dbgfs_unregister(pmcdev);
> + mutex_destroy(&pmcdev->lock);
> + iounmap(pmcdev->regbase);
> + return 0;
> +}
> +
> +static struct platform_driver pmc_core_driver = {
> + .driver = {
> + .name = "pmc_core",
> + },
> + .probe = pmc_core_probe,
> + .remove = pmc_core_remove,
> +};
> +
> +static struct platform_device pmc_core_device = {
> + .name = "pmc_core",
> +};
> +
> +static int __init pmc_core_init(void)
> +{
> + int ret;
>
> Please use reverse x-mas tree style.
>
> OK, will do.
>
> const struct x86_cpu_id *cpu_id;
> + struct pmc_dev *pmcdev = &pmc;
> u64 slp_s0_addr;
> - int err;
>
> cpu_id = x86_match_cpu(intel_pmc_core_ids);
> if (!cpu_id)
> @@ -880,36 +928,31 @@ static int __init pmc_core_probe(void)
> else
> pmcdev->base_addr = slp_s0_addr - pmcdev->map->slp_s0_offset;
>
> - pmcdev->regbase = ioremap(pmcdev->base_addr,
> - pmcdev->map->regmap_length);
> - if (!pmcdev->regbase)
> - return -ENOMEM;
> + platform_set_drvdata(&pmc_core_device, pmcdev);
>
> - mutex_init(&pmcdev->lock);
> - pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
> + ret = platform_device_register(&pmc_core_device);
> + if (ret)
> + return ret;
>
> - err = pmc_core_dbgfs_register(pmcdev);
> - if (err < 0) {
> - pr_warn(" debugfs register failed.\n");
> - iounmap(pmcdev->regbase);
> - return err;
> - }
> + ret = platform_driver_register(&pmc_core_driver);
> + if (ret)
> + goto out_remove_dev;
>
> - dmi_check_system(pmc_core_dmi_table);
> - pr_info(" initialized\n");
> return 0;
> +
> +out_remove_dev:
> + platform_device_unregister(&pmc_core_device);
> + return ret;
> }
> -module_init(pmc_core_probe)
>
> -static void __exit pmc_core_remove(void)
> +static void __init pmc_core_exit(void)
> {
> - struct pmc_dev *pmcdev = &pmc;
> -
> - pmc_core_dbgfs_unregister(pmcdev);
> - mutex_destroy(&pmcdev->lock);
> - iounmap(pmcdev->regbase);
> + platform_driver_unregister(&pmc_core_driver);
> + platform_device_unregister(&pmc_core_device);
> }
> -module_exit(pmc_core_remove)
> +
> +module_init(pmc_core_init);
> +module_exit(pmc_core_exit);
>
> MODULE_LICENSE("GPL v2");
> MODULE_DESCRIPTION("Intel PMC Core Driver");
> --
> 2.21.0.360.g471c308f928-goog
>
> --
> Best Regards,
> Rajneesh

2019-03-25 10:24:38

by Bhardwaj, Rajneesh

[permalink] [raw]
Subject: Re: [PATCH 1/2] platform/x86: intel_pmc_core: Convert to a platform_driver

Hi Rajat

On 23-Mar-19 6:00 AM, Rajat Jain wrote:
> Hi Rajneesh,
>
>
>
> On Fri, Mar 22, 2019 at 12:56 PM Bhardwaj, Rajneesh
> <[email protected]> wrote:
>> Some suggestions below
>>
>> On 18-Mar-19 8:36 PM, Rajat Jain wrote:
>>
>> On Sat, Mar 16, 2019 at 1:30 AM Rajneesh Bhardwaj
>> <[email protected]> wrote:
>>
>> On Wed, Mar 13, 2019 at 03:21:23PM -0700, Rajat Jain wrote:
>>
>> Convert the intel_pmc_core driver to a platform driver. There is no
>> functional change. Some code that tries to determine what kind of
>> CPU this is, has been moved code is moved from pmc_core_probe() to
>>
>> Possible typo here.
>>
>> Ummm, you mean grammar error I guess? Sure, I will rephrase.
>>
>> pmc_core_init().
>>
>> Signed-off-by: Rajat Jain <[email protected]>
>>
>> Thanks for sending this. This is certainly useful to support suspend-resume
>> functionality for this driver which is otherwise only possible with PM
>> notifiers otherwise and that is not desirable. Initially this was a PCI
>> driver and after design discussion it was converted to module. I would like
>> to consult Andy and Srinivas for their opinion about binding it to actual
>> platform bus instead of the virtual bus as in its current form. In one of the
>> internal versions, we used a known acpi PNP HID.
>>
>> Sure, if there is an established ACPI PNP HID, then we could bind it
>> using that, on platforms where we are still developing BIOS /
>> coreboot. However, this might not be possible for shipping systems
>> (Kabylake / skylake) where there is no plan to change the BIOS.
>>
>> In one of our internal patches, i had used HID of power engine plugin. IIRC, During my testing it was working on KBL, CNL with UEFI BIOS but i highly recommend testing it.
>>
>> ---8<----8<-----
>>
>> +static const struct acpi_device_id pmc_acpi_ids[] = {
>>
>> + {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
>>
>> + { }
>>
>> };
> We do not have this device in any of our ACPI tables today. If Intel
> can confirm that this is a well known HID to be used for attaching
> this driver, we can start putting it on our platform's ACPI going
> forward (Whiskeylake, Cometlake, Cannonlake, Icelake ...). But I
> believe we also need to have this driver attach with the device on
> older platforms (Skylake, Kabylake, Amberlake) that are already
> shipping, and running a Non UEFI BIOS (that may not have this HID
> since it is not published).
>
> Currently the intel_pmc_core driver attaches itself to the following
> table of CPU families, without regard to whether it has that HID in
> the ACPI or not:
>
> static const struct x86_cpu_id intel_pmc_core_ids[] = {
> INTEL_CPU_FAM6(SKYLAKE_MOBILE, spt_reg_map),
> INTEL_CPU_FAM6(SKYLAKE_DESKTOP, spt_reg_map),
> INTEL_CPU_FAM6(KABYLAKE_MOBILE, spt_reg_map),
> INTEL_CPU_FAM6(KABYLAKE_DESKTOP, spt_reg_map),
> INTEL_CPU_FAM6(CANNONLAKE_MOBILE, cnp_reg_map),
> INTEL_CPU_FAM6(ICELAKE_MOBILE, icl_reg_map),
> {}
> };

In the past i tried one hybrid approach i.e. PCI and Platform driver at
the same time. Based on that, i feel that this idea of spilling probe
like this may not be the best option. The ACPI CID that i suggested is
available on most Intel Core Platforms that i have worked on and i can
help you in verifying it with UEFI BIOS if you want. Meanwhile, please
see this https://patchwork.kernel.org/patch/9806565/ it gives some
background about this ACPI ID and also points to the LPIT spec.

>
> So to avoid a regression, I suggest that we still maintain the above
> table (may be eliminate few entries) and always attach if the CPU is
> among the table, and if the CPU is not among the table, use the ACPI
> HID to attach. I propose to attach to at least Skylake and Kabylake
> systems using the table above, and for Canonlake and Icelake and
> newer, we can rely on BIOS providing the ACPI HID. Of course I do not
> know if all non-Google Canonlake/Icelake platforms will have this HID
> in their BIOS. If we are not sure, we can include Canonlake and
> Icelake also in that list, an. Please let me know what do you think.

If Coreboot firmware can not be updated for the shipping devices, then
can Chromium kernel take the suggested deviation which i think gets
updated OTA periodically? For upstream,  I am waiting to hear from
Rafael, Andi, David and Srinivas for their inputs.

>
> Thanks,
>
> Rajat
>
>>
>>
>> -builtin_pci_driver(intel_pmc_core_driver);
>>
>> +static struct platform_driver pmc_plat_driver = {
>>
>> + .remove = pmc_plat_remove,
>>
>> + .probe = pmc_plat_probe,
>>
>> + .driver = {
>>
>> + .name = "pmc_core_driver",
>>
>> + .acpi_match_table = ACPI_PTR(pmc_acpi_ids),
>>
>> + },
>>
>> +};
>>
>> ---
>> This is rebased off
>> git://git.infradead.org/linux-platform-drivers-x86.git/for-next
>>
>> drivers/platform/x86/intel_pmc_core.c | 93 ++++++++++++++++++++-------
>> 1 file changed, 68 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
>> index f2c621b55f49..55578d07610c 100644
>> --- a/drivers/platform/x86/intel_pmc_core.c
>> +++ b/drivers/platform/x86/intel_pmc_core.c
>> @@ -19,6 +19,7 @@
>> #include <linux/io.h>
>> #include <linux/module.h>
>> #include <linux/pci.h>
>> +#include <linux/platform_device.h>
>> #include <linux/uaccess.h>
>>
>> #include <asm/cpu_device_id.h>
>> @@ -854,12 +855,59 @@ static const struct dmi_system_id pmc_core_dmi_table[] = {
>> {}
>> };
>>
>> -static int __init pmc_core_probe(void)
>> +static int pmc_core_probe(struct platform_device *pdev)
>> {
>> - struct pmc_dev *pmcdev = &pmc;
>> + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
>> + int err;
>> +
>> + pmcdev->regbase = ioremap(pmcdev->base_addr,
>> + pmcdev->map->regmap_length);
>> + if (!pmcdev->regbase)
>> + return -ENOMEM;
>> +
>> + mutex_init(&pmcdev->lock);
>> + pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
>> +
>> + err = pmc_core_dbgfs_register(pmcdev);
>> + if (err < 0) {
>> + dev_warn(&pdev->dev, "debugfs register failed.\n");
>> + iounmap(pmcdev->regbase);
>> + return err;
>> + }
>> +
>> + dmi_check_system(pmc_core_dmi_table);
>> + dev_info(&pdev->dev, " initialized\n");
>> + return 0;
>> +}
>> +
>> +static int pmc_core_remove(struct platform_device *pdev)
>> +{
>> + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
>> +
>> + pmc_core_dbgfs_unregister(pmcdev);
>> + mutex_destroy(&pmcdev->lock);
>> + iounmap(pmcdev->regbase);
>> + return 0;
>> +}
>> +
>> +static struct platform_driver pmc_core_driver = {
>> + .driver = {
>> + .name = "pmc_core",
>> + },
>> + .probe = pmc_core_probe,
>> + .remove = pmc_core_remove,
>> +};
>> +
>> +static struct platform_device pmc_core_device = {
>> + .name = "pmc_core",
>> +};
>> +
>> +static int __init pmc_core_init(void)
>> +{
>> + int ret;
>>
>> Please use reverse x-mas tree style.
>>
>> OK, will do.
>>
>> const struct x86_cpu_id *cpu_id;
>> + struct pmc_dev *pmcdev = &pmc;
>> u64 slp_s0_addr;
>> - int err;
>>
>> cpu_id = x86_match_cpu(intel_pmc_core_ids);
>> if (!cpu_id)
>> @@ -880,36 +928,31 @@ static int __init pmc_core_probe(void)
>> else
>> pmcdev->base_addr = slp_s0_addr - pmcdev->map->slp_s0_offset;
>>
>> - pmcdev->regbase = ioremap(pmcdev->base_addr,
>> - pmcdev->map->regmap_length);
>> - if (!pmcdev->regbase)
>> - return -ENOMEM;
>> + platform_set_drvdata(&pmc_core_device, pmcdev);
>>
>> - mutex_init(&pmcdev->lock);
>> - pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
>> + ret = platform_device_register(&pmc_core_device);
>> + if (ret)
>> + return ret;
>>
>> - err = pmc_core_dbgfs_register(pmcdev);
>> - if (err < 0) {
>> - pr_warn(" debugfs register failed.\n");
>> - iounmap(pmcdev->regbase);
>> - return err;
>> - }
>> + ret = platform_driver_register(&pmc_core_driver);
>> + if (ret)
>> + goto out_remove_dev;
>>
>> - dmi_check_system(pmc_core_dmi_table);
>> - pr_info(" initialized\n");
>> return 0;
>> +
>> +out_remove_dev:
>> + platform_device_unregister(&pmc_core_device);
>> + return ret;
>> }
>> -module_init(pmc_core_probe)
>>
>> -static void __exit pmc_core_remove(void)
>> +static void __init pmc_core_exit(void)
>> {
>> - struct pmc_dev *pmcdev = &pmc;
>> -
>> - pmc_core_dbgfs_unregister(pmcdev);
>> - mutex_destroy(&pmcdev->lock);
>> - iounmap(pmcdev->regbase);
>> + platform_driver_unregister(&pmc_core_driver);
>> + platform_device_unregister(&pmc_core_device);
>> }
>> -module_exit(pmc_core_remove)
>> +
>> +module_init(pmc_core_init);
>> +module_exit(pmc_core_exit);
>>
>> MODULE_LICENSE("GPL v2");
>> MODULE_DESCRIPTION("Intel PMC Core Driver");
>> --
>> 2.21.0.360.g471c308f928-goog
>>
>> --
>> Best Regards,
>> Rajneesh

2019-03-26 01:44:04

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH 1/2] platform/x86: intel_pmc_core: Convert to a platform_driver

Hi Rajneesh,


On Mon, Mar 25, 2019 at 3:23 AM Bhardwaj, Rajneesh
<[email protected]> wrote:
>
> Hi Rajat
>
> On 23-Mar-19 6:00 AM, Rajat Jain wrote:
> > Hi Rajneesh,
> >
> >
> >
> > On Fri, Mar 22, 2019 at 12:56 PM Bhardwaj, Rajneesh
> > <[email protected]> wrote:
> >> Some suggestions below
> >>
> >> On 18-Mar-19 8:36 PM, Rajat Jain wrote:
> >>
> >> On Sat, Mar 16, 2019 at 1:30 AM Rajneesh Bhardwaj
> >> <[email protected]> wrote:
> >>
> >> On Wed, Mar 13, 2019 at 03:21:23PM -0700, Rajat Jain wrote:
> >>
> >> Convert the intel_pmc_core driver to a platform driver. There is no
> >> functional change. Some code that tries to determine what kind of
> >> CPU this is, has been moved code is moved from pmc_core_probe() to
> >>
> >> Possible typo here.
> >>
> >> Ummm, you mean grammar error I guess? Sure, I will rephrase.
> >>
> >> pmc_core_init().
> >>
> >> Signed-off-by: Rajat Jain <[email protected]>
> >>
> >> Thanks for sending this. This is certainly useful to support suspend-resume
> >> functionality for this driver which is otherwise only possible with PM
> >> notifiers otherwise and that is not desirable. Initially this was a PCI
> >> driver and after design discussion it was converted to module. I would like
> >> to consult Andy and Srinivas for their opinion about binding it to actual
> >> platform bus instead of the virtual bus as in its current form. In one of the
> >> internal versions, we used a known acpi PNP HID.
> >>
> >> Sure, if there is an established ACPI PNP HID, then we could bind it
> >> using that, on platforms where we are still developing BIOS /
> >> coreboot. However, this might not be possible for shipping systems
> >> (Kabylake / skylake) where there is no plan to change the BIOS.
> >>
> >> In one of our internal patches, i had used HID of power engine plugin. IIRC, During my testing it was working on KBL, CNL with UEFI BIOS but i highly recommend testing it.
> >>
> >> ---8<----8<-----
> >>
> >> +static const struct acpi_device_id pmc_acpi_ids[] = {
> >>
> >> + {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
> >>
> >> + { }
> >>
> >> };
> > We do not have this device in any of our ACPI tables today. If Intel
> > can confirm that this is a well known HID to be used for attaching
> > this driver, we can start putting it on our platform's ACPI going
> > forward (Whiskeylake, Cometlake, Cannonlake, Icelake ...). But I
> > believe we also need to have this driver attach with the device on
> > older platforms (Skylake, Kabylake, Amberlake) that are already
> > shipping, and running a Non UEFI BIOS (that may not have this HID
> > since it is not published).
> >
> > Currently the intel_pmc_core driver attaches itself to the following
> > table of CPU families, without regard to whether it has that HID in
> > the ACPI or not:
> >
> > static const struct x86_cpu_id intel_pmc_core_ids[] = {
> > INTEL_CPU_FAM6(SKYLAKE_MOBILE, spt_reg_map),
> > INTEL_CPU_FAM6(SKYLAKE_DESKTOP, spt_reg_map),
> > INTEL_CPU_FAM6(KABYLAKE_MOBILE, spt_reg_map),
> > INTEL_CPU_FAM6(KABYLAKE_DESKTOP, spt_reg_map),
> > INTEL_CPU_FAM6(CANNONLAKE_MOBILE, cnp_reg_map),
> > INTEL_CPU_FAM6(ICELAKE_MOBILE, icl_reg_map),
> > {}
> > };
>
> In the past i tried one hybrid approach i.e. PCI and Platform driver at
> the same time. Based on that, i feel that this idea of spilling probe
> like this may not be the best option. The ACPI CID that i suggested is
> available on most Intel Core Platforms that i have worked on and i can
> help you in verifying it with UEFI BIOS if you want. Meanwhile, please
> see this https://patchwork.kernel.org/patch/9806565/ it gives some
> background about this ACPI ID and also points to the LPIT spec.
>
> >
> > So to avoid a regression, I suggest that we still maintain the above
> > table (may be eliminate few entries) and always attach if the CPU is
> > among the table, and if the CPU is not among the table, use the ACPI
> > HID to attach. I propose to attach to at least Skylake and Kabylake
> > systems using the table above, and for Canonlake and Icelake and
> > newer, we can rely on BIOS providing the ACPI HID. Of course I do not
> > know if all non-Google Canonlake/Icelake platforms will have this HID
> > in their BIOS. If we are not sure, we can include Canonlake and
> > Icelake also in that list, an. Please let me know what do you think.
>
> If Coreboot firmware can not be updated for the shipping devices, then
> can Chromium kernel take the suggested deviation which i think gets
> updated OTA periodically? For upstream, I am waiting to hear from
> Rafael, Andi, David and Srinivas for their inputs.

So if everyone here thinks we should completely switch to using the
ACPI HID "INT33A1" for attaching to the device, sure, we can do that.
Yes, for Chromeos, we can put in a work around internally that ensures
that shipping chromebooks (Kabylake etc) can work fine without that
ACPI ID. What I do not know is if that will cause any regressions
outside of Chromeos. Can you discuss with Rafael, Andy, Srinivas
internally and let me know on how they'd like to proceed on this.

The other option is to apply this patch as-is so we know that there is
no "functional change" and thus no possible regression (so the driver
continues to attach to those and only those systems that it used to,
before this patch). And then introduce the ACPI ID Change as a new
independent patch.

Let me know.

Thanks,

Rajat

>
> >
> > Thanks,
> >
> > Rajat
> >
> >>
> >>
> >> -builtin_pci_driver(intel_pmc_core_driver);
> >>
> >> +static struct platform_driver pmc_plat_driver = {
> >>
> >> + .remove = pmc_plat_remove,
> >>
> >> + .probe = pmc_plat_probe,
> >>
> >> + .driver = {
> >>
> >> + .name = "pmc_core_driver",
> >>
> >> + .acpi_match_table = ACPI_PTR(pmc_acpi_ids),
> >>
> >> + },
> >>
> >> +};
> >>
> >> ---
> >> This is rebased off
> >> git://git.infradead.org/linux-platform-drivers-x86.git/for-next
> >>
> >> drivers/platform/x86/intel_pmc_core.c | 93 ++++++++++++++++++++-------
> >> 1 file changed, 68 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> >> index f2c621b55f49..55578d07610c 100644
> >> --- a/drivers/platform/x86/intel_pmc_core.c
> >> +++ b/drivers/platform/x86/intel_pmc_core.c
> >> @@ -19,6 +19,7 @@
> >> #include <linux/io.h>
> >> #include <linux/module.h>
> >> #include <linux/pci.h>
> >> +#include <linux/platform_device.h>
> >> #include <linux/uaccess.h>
> >>
> >> #include <asm/cpu_device_id.h>
> >> @@ -854,12 +855,59 @@ static const struct dmi_system_id pmc_core_dmi_table[] = {
> >> {}
> >> };
> >>
> >> -static int __init pmc_core_probe(void)
> >> +static int pmc_core_probe(struct platform_device *pdev)
> >> {
> >> - struct pmc_dev *pmcdev = &pmc;
> >> + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> >> + int err;
> >> +
> >> + pmcdev->regbase = ioremap(pmcdev->base_addr,
> >> + pmcdev->map->regmap_length);
> >> + if (!pmcdev->regbase)
> >> + return -ENOMEM;
> >> +
> >> + mutex_init(&pmcdev->lock);
> >> + pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
> >> +
> >> + err = pmc_core_dbgfs_register(pmcdev);
> >> + if (err < 0) {
> >> + dev_warn(&pdev->dev, "debugfs register failed.\n");
> >> + iounmap(pmcdev->regbase);
> >> + return err;
> >> + }
> >> +
> >> + dmi_check_system(pmc_core_dmi_table);
> >> + dev_info(&pdev->dev, " initialized\n");
> >> + return 0;
> >> +}
> >> +
> >> +static int pmc_core_remove(struct platform_device *pdev)
> >> +{
> >> + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> >> +
> >> + pmc_core_dbgfs_unregister(pmcdev);
> >> + mutex_destroy(&pmcdev->lock);
> >> + iounmap(pmcdev->regbase);
> >> + return 0;
> >> +}
> >> +
> >> +static struct platform_driver pmc_core_driver = {
> >> + .driver = {
> >> + .name = "pmc_core",
> >> + },
> >> + .probe = pmc_core_probe,
> >> + .remove = pmc_core_remove,
> >> +};
> >> +
> >> +static struct platform_device pmc_core_device = {
> >> + .name = "pmc_core",
> >> +};
> >> +
> >> +static int __init pmc_core_init(void)
> >> +{
> >> + int ret;
> >>
> >> Please use reverse x-mas tree style.
> >>
> >> OK, will do.
> >>
> >> const struct x86_cpu_id *cpu_id;
> >> + struct pmc_dev *pmcdev = &pmc;
> >> u64 slp_s0_addr;
> >> - int err;
> >>
> >> cpu_id = x86_match_cpu(intel_pmc_core_ids);
> >> if (!cpu_id)
> >> @@ -880,36 +928,31 @@ static int __init pmc_core_probe(void)
> >> else
> >> pmcdev->base_addr = slp_s0_addr - pmcdev->map->slp_s0_offset;
> >>
> >> - pmcdev->regbase = ioremap(pmcdev->base_addr,
> >> - pmcdev->map->regmap_length);
> >> - if (!pmcdev->regbase)
> >> - return -ENOMEM;
> >> + platform_set_drvdata(&pmc_core_device, pmcdev);
> >>
> >> - mutex_init(&pmcdev->lock);
> >> - pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
> >> + ret = platform_device_register(&pmc_core_device);
> >> + if (ret)
> >> + return ret;
> >>
> >> - err = pmc_core_dbgfs_register(pmcdev);
> >> - if (err < 0) {
> >> - pr_warn(" debugfs register failed.\n");
> >> - iounmap(pmcdev->regbase);
> >> - return err;
> >> - }
> >> + ret = platform_driver_register(&pmc_core_driver);
> >> + if (ret)
> >> + goto out_remove_dev;
> >>
> >> - dmi_check_system(pmc_core_dmi_table);
> >> - pr_info(" initialized\n");
> >> return 0;
> >> +
> >> +out_remove_dev:
> >> + platform_device_unregister(&pmc_core_device);
> >> + return ret;
> >> }
> >> -module_init(pmc_core_probe)
> >>
> >> -static void __exit pmc_core_remove(void)
> >> +static void __init pmc_core_exit(void)
> >> {
> >> - struct pmc_dev *pmcdev = &pmc;
> >> -
> >> - pmc_core_dbgfs_unregister(pmcdev);
> >> - mutex_destroy(&pmcdev->lock);
> >> - iounmap(pmcdev->regbase);
> >> + platform_driver_unregister(&pmc_core_driver);
> >> + platform_device_unregister(&pmc_core_device);
> >> }
> >> -module_exit(pmc_core_remove)
> >> +
> >> +module_init(pmc_core_init);
> >> +module_exit(pmc_core_exit);
> >>
> >> MODULE_LICENSE("GPL v2");
> >> MODULE_DESCRIPTION("Intel PMC Core Driver");
> >> --
> >> 2.21.0.360.g471c308f928-goog
> >>
> >> --
> >> Best Regards,
> >> Rajneesh

2019-03-29 03:42:12

by srinivas pandruvada

[permalink] [raw]
Subject: Re: [PATCH 1/2] platform/x86: intel_pmc_core: Convert to a platform_driver

On Mon, 2019-03-25 at 18:41 -0700, Rajat Jain wrote:
> Hi Rajneesh,
>
>
> On Mon, Mar 25, 2019 at 3:23 AM Bhardwaj, Rajneesh
> <[email protected]> wrote:
> >
> > Hi Rajat
> >
> > On 23-Mar-19 6:00 AM, Rajat Jain wrote:
> > > Hi Rajneesh,
> > >
> > >
> > >
> > > On Fri, Mar 22, 2019 at 12:56 PM Bhardwaj, Rajneesh
> > > <[email protected]> wrote:
> > > > Some suggestions below
> > > >
> > > > On 18-Mar-19 8:36 PM, Rajat Jain wrote:
> > > >
> > > > On Sat, Mar 16, 2019 at 1:30 AM Rajneesh Bhardwaj
> > > > <[email protected]> wrote:
> > > >
> > > > On Wed, Mar 13, 2019 at 03:21:23PM -0700, Rajat Jain wrote:
> > > >
> > > > Convert the intel_pmc_core driver to a platform driver. There
> > > > is no
> > > > functional change. Some code that tries to determine what kind
> > > > of
> > > > CPU this is, has been moved code is moved from pmc_core_probe()
> > > > to
> > > >
> > > > Possible typo here.
> > > >
> > > > Ummm, you mean grammar error I guess? Sure, I will rephrase.
> > > >
> > > > pmc_core_init().
> > > >
> > > > Signed-off-by: Rajat Jain <[email protected]>
> > > >
> > > > Thanks for sending this. This is certainly useful to support
> > > > suspend-resume
> > > > functionality for this driver which is otherwise only possible
> > > > with PM
> > > > notifiers otherwise and that is not desirable. Initially this
> > > > was a PCI
> > > > driver and after design discussion it was converted to module.
> > > > I would like
> > > > to consult Andy and Srinivas for their opinion about binding it
> > > > to actual
> > > > platform bus instead of the virtual bus as in its current form.
> > > > In one of the
> > > > internal versions, we used a known acpi PNP HID.
> > > >
> > > > Sure, if there is an established ACPI PNP HID, then we could
> > > > bind it
> > > > using that, on platforms where we are still developing BIOS /
> > > > coreboot. However, this might not be possible for shipping
> > > > systems
> > > > (Kabylake / skylake) where there is no plan to change the BIOS.
> > > >
> > > > In one of our internal patches, i had used HID of power engine
> > > > plugin. IIRC, During my testing it was working on KBL, CNL with
> > > > UEFI BIOS but i highly recommend testing it.
> > > >
> > > > ---8<----8<-----
> > > >
> > > > +static const struct acpi_device_id pmc_acpi_ids[] = {
> > > >
> > > > + {"INT33A1", 0}, /* _HID for Intel Power Engine,
> > > > _CID PNP0D80*/
> > > >
> > > > + { }
> > > >
> > > > };
> > >
> > > We do not have this device in any of our ACPI tables today. If
> > > Intel
> > > can confirm that this is a well known HID to be used for
> > > attaching
> > > this driver, we can start putting it on our platform's ACPI going
> > > forward (Whiskeylake, Cometlake, Cannonlake, Icelake ...). But I
> > > believe we also need to have this driver attach with the device
> > > on
> > > older platforms (Skylake, Kabylake, Amberlake) that are already
> > > shipping, and running a Non UEFI BIOS (that may not have this HID
> > > since it is not published).
> > >
> > > Currently the intel_pmc_core driver attaches itself to the
> > > following
> > > table of CPU families, without regard to whether it has that HID
> > > in
> > > the ACPI or not:
> > >
> > > static const struct x86_cpu_id intel_pmc_core_ids[] = {
> > > INTEL_CPU_FAM6(SKYLAKE_MOBILE, spt_reg_map),
> > > INTEL_CPU_FAM6(SKYLAKE_DESKTOP, spt_reg_map),
> > > INTEL_CPU_FAM6(KABYLAKE_MOBILE, spt_reg_map),
> > > INTEL_CPU_FAM6(KABYLAKE_DESKTOP, spt_reg_map),
> > > INTEL_CPU_FAM6(CANNONLAKE_MOBILE, cnp_reg_map),
> > > INTEL_CPU_FAM6(ICELAKE_MOBILE, icl_reg_map),
> > > {}
> > > };
> >
> > In the past i tried one hybrid approach i.e. PCI and Platform
> > driver at
> > the same time. Based on that, i feel that this idea of spilling
> > probe
> > like this may not be the best option. The ACPI CID that i suggested
> > is
> > available on most Intel Core Platforms that i have worked on and i
> > can
> > help you in verifying it with UEFI BIOS if you want. Meanwhile,
> > please
> > see this https://patchwork.kernel.org/patch/9806565/ it gives some
> > background about this ACPI ID and also points to the LPIT spec.
> >
> > >
> > > So to avoid a regression, I suggest that we still maintain the
> > > above
> > > table (may be eliminate few entries) and always attach if the CPU
> > > is
> > > among the table, and if the CPU is not among the table, use the
> > > ACPI
> > > HID to attach. I propose to attach to at least Skylake and
> > > Kabylake
> > > systems using the table above, and for Canonlake and Icelake and
> > > newer, we can rely on BIOS providing the ACPI HID. Of course I do
> > > not
> > > know if all non-Google Canonlake/Icelake platforms will have this
> > > HID
> > > in their BIOS. If we are not sure, we can include Canonlake and
> > > Icelake also in that list, an. Please let me know what do you
> > > think.
> >
> > If Coreboot firmware can not be updated for the shipping devices,
> > then
> > can Chromium kernel take the suggested deviation which i think gets
> > updated OTA periodically? For upstream, I am waiting to hear from
> > Rafael, Andi, David and Srinivas for their inputs.
>
> So if everyone here thinks we should completely switch to using the
> ACPI HID "INT33A1" for attaching to the device, sure, we can do that.
> Yes, for Chromeos, we can put in a work around internally that
> ensures
> that shipping chromebooks (Kabylake etc) can work fine without that
> ACPI ID. What I do not know is if that will cause any regressions
> outside of Chromeos. Can you discuss with Rafael, Andy, Srinivas
> internally and let me know on how they'd like to proceed on this.
>
> The other option is to apply this patch as-is so we know that there
> is
> no "functional change" and thus no possible regression (so the driver
> continues to attach to those and only those systems that s it used
> to,
> before this patch). And then introduce the ACPI ID Change as a new
> independent patch.
Use INT33A1 to enumerate, if this id doesn't exist then fallback to the
cpuid style. The aim should be that we don't have to add any more CPU
model to enumerate. But most probably register map will differ so we
still may end up adding some CPU model relationship.


Thanks,
Srinivas


>
> Let me know.
>
> Thanks,
>
> Rajat
>
> >
> > >
> > > Thanks,
> > >
> > > Rajat
> > >
> > > >
> > > >
> > > > -builtin_pci_driver(intel_pmc_core_driver);
> > > >
> > > > +static struct platform_driver pmc_plat_driver = {
> > > >
> > > > + .remove = pmc_plat_remove,
> > > >
> > > > + .probe = pmc_plat_probe,
> > > >
> > > > + .driver = {
> > > >
> > > > + .name = "pmc_core_driver",
> > > >
> > > > + .acpi_match_table =
> > > > ACPI_PTR(pmc_acpi_ids),
> > > >
> > > > + },
> > > >
> > > > +};
> > > >
> > > > ---
> > > > This is rebased off
> > > > git://git.infradead.org/linux-platform-drivers-x86.git/for-next
> > > >
> > > > drivers/platform/x86/intel_pmc_core.c | 93
> > > > ++++++++++++++++++++-------
> > > > 1 file changed, 68 insertions(+), 25 deletions(-)
> > > >
> > > > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > > > b/drivers/platform/x86/intel_pmc_core.c
> > > > index f2c621b55f49..55578d07610c 100644
> > > > --- a/drivers/platform/x86/intel_pmc_core.c
> > > > +++ b/drivers/platform/x86/intel_pmc_core.c
> > > > @@ -19,6 +19,7 @@
> > > > #include <linux/io.h>
> > > > #include <linux/module.h>
> > > > #include <linux/pci.h>
> > > > +#include <linux/platform_device.h>
> > > > #include <linux/uaccess.h>
> > > >
> > > > #include <asm/cpu_device_id.h>
> > > > @@ -854,12 +855,59 @@ static const struct dmi_system_id
> > > > pmc_core_dmi_table[] = {
> > > > {}
> > > > };
> > > >
> > > > -static int __init pmc_core_probe(void)
> > > > +static int pmc_core_probe(struct platform_device *pdev)
> > > > {
> > > > - struct pmc_dev *pmcdev = &pmc;
> > > > + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> > > > + int err;
> > > > +
> > > > + pmcdev->regbase = ioremap(pmcdev->base_addr,
> > > > + pmcdev->map->regmap_length);
> > > > + if (!pmcdev->regbase)
> > > > + return -ENOMEM;
> > > > +
> > > > + mutex_init(&pmcdev->lock);
> > > > + pmcdev->pmc_xram_read_bit =
> > > > pmc_core_check_read_lock_bit();
> > > > +
> > > > + err = pmc_core_dbgfs_register(pmcdev);
> > > > + if (err < 0) {
> > > > + dev_warn(&pdev->dev, "debugfs register
> > > > failed.\n");
> > > > + iounmap(pmcdev->regbase);
> > > > + return err;
> > > > + }
> > > > +
> > > > + dmi_check_system(pmc_core_dmi_table);
> > > > + dev_info(&pdev->dev, " initialized\n");
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int pmc_core_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> > > > +
> > > > + pmc_core_dbgfs_unregister(pmcdev);
> > > > + mutex_destroy(&pmcdev->lock);
> > > > + iounmap(pmcdev->regbase);
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static struct platform_driver pmc_core_driver = {
> > > > + .driver = {
> > > > + .name = "pmc_core",
> > > > + },
> > > > + .probe = pmc_core_probe,
> > > > + .remove = pmc_core_remove,
> > > > +};
> > > > +
> > > > +static struct platform_device pmc_core_device = {
> > > > + .name = "pmc_core",
> > > > +};
> > > > +
> > > > +static int __init pmc_core_init(void)
> > > > +{
> > > > + int ret;
> > > >
> > > > Please use reverse x-mas tree style.
> > > >
> > > > OK, will do.
> > > >
> > > > const struct x86_cpu_id *cpu_id;
> > > > + struct pmc_dev *pmcdev = &pmc;
> > > > u64 slp_s0_addr;
> > > > - int err;
> > > >
> > > > cpu_id = x86_match_cpu(intel_pmc_core_ids);
> > > > if (!cpu_id)
> > > > @@ -880,36 +928,31 @@ static int __init pmc_core_probe(void)
> > > > else
> > > > pmcdev->base_addr = slp_s0_addr - pmcdev->map-
> > > > >slp_s0_offset;
> > > >
> > > > - pmcdev->regbase = ioremap(pmcdev->base_addr,
> > > > - pmcdev->map->regmap_length);
> > > > - if (!pmcdev->regbase)
> > > > - return -ENOMEM;
> > > > + platform_set_drvdata(&pmc_core_device, pmcdev);
> > > >
> > > > - mutex_init(&pmcdev->lock);
> > > > - pmcdev->pmc_xram_read_bit =
> > > > pmc_core_check_read_lock_bit();
> > > > + ret = platform_device_register(&pmc_core_device);
> > > > + if (ret)
> > > > + return ret;
> > > >
> > > > - err = pmc_core_dbgfs_register(pmcdev);
> > > > - if (err < 0) {
> > > > - pr_warn(" debugfs register failed.\n");
> > > > - iounmap(pmcdev->regbase);
> > > > - return err;
> > > > - }
> > > > + ret = platform_driver_register(&pmc_core_driver);
> > > > + if (ret)
> > > > + goto out_remove_dev;
> > > >
> > > > - dmi_check_system(pmc_core_dmi_table);
> > > > - pr_info(" initialized\n");
> > > > return 0;
> > > > +
> > > > +out_remove_dev:
> > > > + platform_device_unregister(&pmc_core_device);
> > > > + return ret;
> > > > }
> > > > -module_init(pmc_core_probe)
> > > >
> > > > -static void __exit pmc_core_remove(void)
> > > > +static void __init pmc_core_exit(void)
> > > > {
> > > > - struct pmc_dev *pmcdev = &pmc;
> > > > -
> > > > - pmc_core_dbgfs_unregister(pmcdev);
> > > > - mutex_destroy(&pmcdev->lock);
> > > > - iounmap(pmcdev->regbase);
> > > > + platform_driver_unregister(&pmc_core_driver);
> > > > + platform_device_unregister(&pmc_core_device);
> > > > }
> > > > -module_exit(pmc_core_remove)
> > > > +
> > > > +module_init(pmc_core_init);
> > > > +module_exit(pmc_core_exit);
> > > >
> > > > MODULE_LICENSE("GPL v2");
> > > > MODULE_DESCRIPTION("Intel PMC Core Driver");
> > > > --
> > > > 2.21.0.360.g471c308f928-goog
> > > >
> > > > --
> > > > Best Regards,
> > > > Rajneesh


2019-03-29 05:31:27

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH 1/2] platform/x86: intel_pmc_core: Convert to a platform_driver

Hi Srinivas,


On Thu, Mar 28, 2019 at 8:41 PM Srinivas Pandruvada
<[email protected]> wrote:
>
> On Mon, 2019-03-25 at 18:41 -0700, Rajat Jain wrote:
> > Hi Rajneesh,
> >
> >
> > On Mon, Mar 25, 2019 at 3:23 AM Bhardwaj, Rajneesh
> > <[email protected]> wrote:
> > >
> > > Hi Rajat
> > >
> > > On 23-Mar-19 6:00 AM, Rajat Jain wrote:
> > > > Hi Rajneesh,
> > > >
> > > >
> > > >
> > > > On Fri, Mar 22, 2019 at 12:56 PM Bhardwaj, Rajneesh
> > > > <[email protected]> wrote:
> > > > > Some suggestions below
> > > > >
> > > > > On 18-Mar-19 8:36 PM, Rajat Jain wrote:
> > > > >
> > > > > On Sat, Mar 16, 2019 at 1:30 AM Rajneesh Bhardwaj
> > > > > <[email protected]> wrote:
> > > > >
> > > > > On Wed, Mar 13, 2019 at 03:21:23PM -0700, Rajat Jain wrote:
> > > > >
> > > > > Convert the intel_pmc_core driver to a platform driver. There
> > > > > is no
> > > > > functional change. Some code that tries to determine what kind
> > > > > of
> > > > > CPU this is, has been moved code is moved from pmc_core_probe()
> > > > > to
> > > > >
> > > > > Possible typo here.
> > > > >
> > > > > Ummm, you mean grammar error I guess? Sure, I will rephrase.
> > > > >
> > > > > pmc_core_init().
> > > > >
> > > > > Signed-off-by: Rajat Jain <[email protected]>
> > > > >
> > > > > Thanks for sending this. This is certainly useful to support
> > > > > suspend-resume
> > > > > functionality for this driver which is otherwise only possible
> > > > > with PM
> > > > > notifiers otherwise and that is not desirable. Initially this
> > > > > was a PCI
> > > > > driver and after design discussion it was converted to module.
> > > > > I would like
> > > > > to consult Andy and Srinivas for their opinion about binding it
> > > > > to actual
> > > > > platform bus instead of the virtual bus as in its current form.
> > > > > In one of the
> > > > > internal versions, we used a known acpi PNP HID.
> > > > >
> > > > > Sure, if there is an established ACPI PNP HID, then we could
> > > > > bind it
> > > > > using that, on platforms where we are still developing BIOS /
> > > > > coreboot. However, this might not be possible for shipping
> > > > > systems
> > > > > (Kabylake / skylake) where there is no plan to change the BIOS.
> > > > >
> > > > > In one of our internal patches, i had used HID of power engine
> > > > > plugin. IIRC, During my testing it was working on KBL, CNL with
> > > > > UEFI BIOS but i highly recommend testing it.
> > > > >
> > > > > ---8<----8<-----
> > > > >
> > > > > +static const struct acpi_device_id pmc_acpi_ids[] = {
> > > > >
> > > > > + {"INT33A1", 0}, /* _HID for Intel Power Engine,
> > > > > _CID PNP0D80*/
> > > > >
> > > > > + { }
> > > > >
> > > > > };
> > > >
> > > > We do not have this device in any of our ACPI tables today. If
> > > > Intel
> > > > can confirm that this is a well known HID to be used for
> > > > attaching
> > > > this driver, we can start putting it on our platform's ACPI going
> > > > forward (Whiskeylake, Cometlake, Cannonlake, Icelake ...). But I
> > > > believe we also need to have this driver attach with the device
> > > > on
> > > > older platforms (Skylake, Kabylake, Amberlake) that are already
> > > > shipping, and running a Non UEFI BIOS (that may not have this HID
> > > > since it is not published).
> > > >
> > > > Currently the intel_pmc_core driver attaches itself to the
> > > > following
> > > > table of CPU families, without regard to whether it has that HID
> > > > in
> > > > the ACPI or not:
> > > >
> > > > static const struct x86_cpu_id intel_pmc_core_ids[] = {
> > > > INTEL_CPU_FAM6(SKYLAKE_MOBILE, spt_reg_map),
> > > > INTEL_CPU_FAM6(SKYLAKE_DESKTOP, spt_reg_map),
> > > > INTEL_CPU_FAM6(KABYLAKE_MOBILE, spt_reg_map),
> > > > INTEL_CPU_FAM6(KABYLAKE_DESKTOP, spt_reg_map),
> > > > INTEL_CPU_FAM6(CANNONLAKE_MOBILE, cnp_reg_map),
> > > > INTEL_CPU_FAM6(ICELAKE_MOBILE, icl_reg_map),
> > > > {}
> > > > };
> > >
> > > In the past i tried one hybrid approach i.e. PCI and Platform
> > > driver at
> > > the same time. Based on that, i feel that this idea of spilling
> > > probe
> > > like this may not be the best option. The ACPI CID that i suggested
> > > is
> > > available on most Intel Core Platforms that i have worked on and i
> > > can
> > > help you in verifying it with UEFI BIOS if you want. Meanwhile,
> > > please
> > > see this https://patchwork.kernel.org/patch/9806565/ it gives some
> > > background about this ACPI ID and also points to the LPIT spec.
> > >
> > > >
> > > > So to avoid a regression, I suggest that we still maintain the
> > > > above
> > > > table (may be eliminate few entries) and always attach if the CPU
> > > > is
> > > > among the table, and if the CPU is not among the table, use the
> > > > ACPI
> > > > HID to attach. I propose to attach to at least Skylake and
> > > > Kabylake
> > > > systems using the table above, and for Canonlake and Icelake and
> > > > newer, we can rely on BIOS providing the ACPI HID. Of course I do
> > > > not
> > > > know if all non-Google Canonlake/Icelake platforms will have this
> > > > HID
> > > > in their BIOS. If we are not sure, we can include Canonlake and
> > > > Icelake also in that list, an. Please let me know what do you
> > > > think.
> > >
> > > If Coreboot firmware can not be updated for the shipping devices,
> > > then
> > > can Chromium kernel take the suggested deviation which i think gets
> > > updated OTA periodically? For upstream, I am waiting to hear from
> > > Rafael, Andi, David and Srinivas for their inputs.
> >
> > So if everyone here thinks we should completely switch to using the
> > ACPI HID "INT33A1" for attaching to the device, sure, we can do that.
> > Yes, for Chromeos, we can put in a work around internally that
> > ensures
> > that shipping chromebooks (Kabylake etc) can work fine without that
> > ACPI ID. What I do not know is if that will cause any regressions
> > outside of Chromeos. Can you discuss with Rafael, Andy, Srinivas
> > internally and let me know on how they'd like to proceed on this.
> >
> > The other option is to apply this patch as-is so we know that there
> > is
> > no "functional change" and thus no possible regression (so the driver
> > continues to attach to those and only those systems that s it used
> > to,
> > before this patch). And then introduce the ACPI ID Change as a new
> > independent patch.
> Use INT33A1 to enumerate, if this id doesn't exist then fallback to the
> cpuid style. The aim should be that we don't have to add any more CPU
> model to enumerate. But most probably register map will differ so we
> still may end up adding some CPU model relationship.

Thanks for the guidance. Just to reconfirm my understanding of your suggestion:

Here is the suggestive code Rajneesh provided, and I intend to do it similarly:

+static const struct acpi_device_id pmc_acpi_ids[] = {
+ {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
+ { }
+};

+static struct platform_driver pmc_plat_driver = {
+ .remove = pmc_plat_remove,
+ .probe = pmc_plat_probe,
+ .driver = {
+ .name = "pmc_core_driver",
+ .acpi_match_table = ACPI_PTR(pmc_acpi_ids),
+ },
+};

My understanding is that with this, the kernel would use
acpi_match_table to automatically create the platform_device on a
platform where ACPI tables contain the INT33A1 HID, and thus we don't
need to create the platform_device in module_init time on such
platforms. So are you saying that during init, I should check if the
ACPI HID INT33A1 is not present on the platform, then use the cpu_id
table to create the platform_device? Thus newer platforms will not
need an entry in the table.

Thanks,

Rajat


>
>
> Thanks,
> Srinivas
>
>
> >
> > Let me know.
> >
> > Thanks,
> >
> > Rajat
> >
> > >
> > > >
> > > > Thanks,
> > > >
> > > > Rajat
> > > >
> > > > >
> > > > >
> > > > > -builtin_pci_driver(intel_pmc_core_driver);
> > > > >
> > > > > +static struct platform_driver pmc_plat_driver = {
> > > > >
> > > > > + .remove = pmc_plat_remove,
> > > > >
> > > > > + .probe = pmc_plat_probe,
> > > > >
> > > > > + .driver = {
> > > > >
> > > > > + .name = "pmc_core_driver",
> > > > >
> > > > > + .acpi_match_table =
> > > > > ACPI_PTR(pmc_acpi_ids),
> > > > >
> > > > > + },
> > > > >
> > > > > +};
> > > > >
> > > > > ---
> > > > > This is rebased off
> > > > > git://git.infradead.org/linux-platform-drivers-x86.git/for-next
> > > > >
> > > > > drivers/platform/x86/intel_pmc_core.c | 93
> > > > > ++++++++++++++++++++-------
> > > > > 1 file changed, 68 insertions(+), 25 deletions(-)
> > > > >
> > > > > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > > > > b/drivers/platform/x86/intel_pmc_core.c
> > > > > index f2c621b55f49..55578d07610c 100644
> > > > > --- a/drivers/platform/x86/intel_pmc_core.c
> > > > > +++ b/drivers/platform/x86/intel_pmc_core.c
> > > > > @@ -19,6 +19,7 @@
> > > > > #include <linux/io.h>
> > > > > #include <linux/module.h>
> > > > > #include <linux/pci.h>
> > > > > +#include <linux/platform_device.h>
> > > > > #include <linux/uaccess.h>
> > > > >
> > > > > #include <asm/cpu_device_id.h>
> > > > > @@ -854,12 +855,59 @@ static const struct dmi_system_id
> > > > > pmc_core_dmi_table[] = {
> > > > > {}
> > > > > };
> > > > >
> > > > > -static int __init pmc_core_probe(void)
> > > > > +static int pmc_core_probe(struct platform_device *pdev)
> > > > > {
> > > > > - struct pmc_dev *pmcdev = &pmc;
> > > > > + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> > > > > + int err;
> > > > > +
> > > > > + pmcdev->regbase = ioremap(pmcdev->base_addr,
> > > > > + pmcdev->map->regmap_length);
> > > > > + if (!pmcdev->regbase)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + mutex_init(&pmcdev->lock);
> > > > > + pmcdev->pmc_xram_read_bit =
> > > > > pmc_core_check_read_lock_bit();
> > > > > +
> > > > > + err = pmc_core_dbgfs_register(pmcdev);
> > > > > + if (err < 0) {
> > > > > + dev_warn(&pdev->dev, "debugfs register
> > > > > failed.\n");
> > > > > + iounmap(pmcdev->regbase);
> > > > > + return err;
> > > > > + }
> > > > > +
> > > > > + dmi_check_system(pmc_core_dmi_table);
> > > > > + dev_info(&pdev->dev, " initialized\n");
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static int pmc_core_remove(struct platform_device *pdev)
> > > > > +{
> > > > > + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> > > > > +
> > > > > + pmc_core_dbgfs_unregister(pmcdev);
> > > > > + mutex_destroy(&pmcdev->lock);
> > > > > + iounmap(pmcdev->regbase);
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static struct platform_driver pmc_core_driver = {
> > > > > + .driver = {
> > > > > + .name = "pmc_core",
> > > > > + },
> > > > > + .probe = pmc_core_probe,
> > > > > + .remove = pmc_core_remove,
> > > > > +};
> > > > > +
> > > > > +static struct platform_device pmc_core_device = {
> > > > > + .name = "pmc_core",
> > > > > +};
> > > > > +
> > > > > +static int __init pmc_core_init(void)
> > > > > +{
> > > > > + int ret;
> > > > >
> > > > > Please use reverse x-mas tree style.
> > > > >
> > > > > OK, will do.
> > > > >
> > > > > const struct x86_cpu_id *cpu_id;
> > > > > + struct pmc_dev *pmcdev = &pmc;
> > > > > u64 slp_s0_addr;
> > > > > - int err;
> > > > >
> > > > > cpu_id = x86_match_cpu(intel_pmc_core_ids);
> > > > > if (!cpu_id)
> > > > > @@ -880,36 +928,31 @@ static int __init pmc_core_probe(void)
> > > > > else
> > > > > pmcdev->base_addr = slp_s0_addr - pmcdev->map-
> > > > > >slp_s0_offset;
> > > > >
> > > > > - pmcdev->regbase = ioremap(pmcdev->base_addr,
> > > > > - pmcdev->map->regmap_length);
> > > > > - if (!pmcdev->regbase)
> > > > > - return -ENOMEM;
> > > > > + platform_set_drvdata(&pmc_core_device, pmcdev);
> > > > >
> > > > > - mutex_init(&pmcdev->lock);
> > > > > - pmcdev->pmc_xram_read_bit =
> > > > > pmc_core_check_read_lock_bit();
> > > > > + ret = platform_device_register(&pmc_core_device);
> > > > > + if (ret)
> > > > > + return ret;
> > > > >
> > > > > - err = pmc_core_dbgfs_register(pmcdev);
> > > > > - if (err < 0) {
> > > > > - pr_warn(" debugfs register failed.\n");
> > > > > - iounmap(pmcdev->regbase);
> > > > > - return err;
> > > > > - }
> > > > > + ret = platform_driver_register(&pmc_core_driver);
> > > > > + if (ret)
> > > > > + goto out_remove_dev;
> > > > >
> > > > > - dmi_check_system(pmc_core_dmi_table);
> > > > > - pr_info(" initialized\n");
> > > > > return 0;
> > > > > +
> > > > > +out_remove_dev:
> > > > > + platform_device_unregister(&pmc_core_device);
> > > > > + return ret;
> > > > > }
> > > > > -module_init(pmc_core_probe)
> > > > >
> > > > > -static void __exit pmc_core_remove(void)
> > > > > +static void __init pmc_core_exit(void)
> > > > > {
> > > > > - struct pmc_dev *pmcdev = &pmc;
> > > > > -
> > > > > - pmc_core_dbgfs_unregister(pmcdev);
> > > > > - mutex_destroy(&pmcdev->lock);
> > > > > - iounmap(pmcdev->regbase);
> > > > > + platform_driver_unregister(&pmc_core_driver);
> > > > > + platform_device_unregister(&pmc_core_device);
> > > > > }
> > > > > -module_exit(pmc_core_remove)
> > > > > +
> > > > > +module_init(pmc_core_init);
> > > > > +module_exit(pmc_core_exit);
> > > > >
> > > > > MODULE_LICENSE("GPL v2");
> > > > > MODULE_DESCRIPTION("Intel PMC Core Driver");
> > > > > --
> > > > > 2.21.0.360.g471c308f928-goog
> > > > >
> > > > > --
> > > > > Best Regards,
> > > > > Rajneesh
>

2019-03-29 15:55:54

by srinivas pandruvada

[permalink] [raw]
Subject: Re: [PATCH 1/2] platform/x86: intel_pmc_core: Convert to a platform_driver

On Thu, 2019-03-28 at 22:29 -0700, Rajat Jain wrote:
> Hi Srinivas,
>
>

[...]

> So if everyone here thinks we should completely switch to using
> > > the
> > > ACPI HID "INT33A1" for attaching to the device, sure, we can do
> > > that.
> > > Yes, for Chromeos, we can put in a work around internally that
> > > ensures
> > > that shipping chromebooks (Kabylake etc) can work fine without
> > > that
> > > ACPI ID. What I do not know is if that will cause any regressions
> > > outside of Chromeos. Can you discuss with Rafael, Andy, Srinivas
> > > internally and let me know on how they'd like to proceed on this.
> > >
> > > The other option is to apply this patch as-is so we know that
> > > there
> > > is
> > > no "functional change" and thus no possible regression (so the
> > > driver
> > > continues to attach to those and only those systems that s it
> > > used
> > > to,
> > > before this patch). And then introduce the ACPI ID Change as a
> > > new
> > > independent patch.
> >
> > Use INT33A1 to enumerate, if this id doesn't exist then fallback to
> > the
> > cpuid style. The aim should be that we don't have to add any more
> > CPU
> > model to enumerate. But most probably register map will differ so
> > we
> > still may end up adding some CPU model relationship.
>
> Thanks for the guidance. Just to reconfirm my understanding of your
> suggestion:
>
> Here is the suggestive code Rajneesh provided, and I intend to do it
> similarly:
>
> +static const struct acpi_device_id pmc_acpi_ids[] = {
> + {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID
> PNP0D80*/
> + { }
> +};
>
> +static struct platform_driver pmc_plat_driver = {
> + .remove = pmc_plat_remove,
> + .probe = pmc_plat_probe,
> + .driver = {
> + .name = "pmc_core_driver",
> + .acpi_match_table =
> ACPI_PTR(pmc_acpi_ids),
> + },
> +};
>
> My understanding is that with this, the kernel would use
> acpi_match_table to automatically create the platform_device on a
> platform where ACPI tables contain the INT33A1 HID, and thus we don't
> need to create the platform_device in module_init time on such
> platforms.
Yes. There will be /sys/bus/platform/devices/INT33A1:00.


> So are you saying that during init, I should check if the
> ACPI HID INT33A1 is not present on the platform, then use the cpu_id
> table to create the platform_device? Thus newer platforms will not
> need an entry in the table.
Yes. Preferably in a different file as Andy would like. So the the
current driver only implements platform driver for INT33A1. The other driver which will enumerate on CPUID and create INT33A1 platform device if there is no ACPI match via acpi_match_device() or similar API, for INT33A1. When you create a platform device the pmc driver will be probed.

Thanks,
Srinivas



2019-04-04 01:01:57

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH 1/2] platform/x86: intel_pmc_core: Convert to a platform_driver

On Fri, Mar 29, 2019 at 8:53 AM Srinivas Pandruvada
<[email protected]> wrote:
>
> On Thu, 2019-03-28 at 22:29 -0700, Rajat Jain wrote:
> > Hi Srinivas,
> >
> >
>
> [...]
>
> > So if everyone here thinks we should completely switch to using
> > > > the
> > > > ACPI HID "INT33A1" for attaching to the device, sure, we can do
> > > > that.
> > > > Yes, for Chromeos, we can put in a work around internally that
> > > > ensures
> > > > that shipping chromebooks (Kabylake etc) can work fine without
> > > > that
> > > > ACPI ID. What I do not know is if that will cause any regressions
> > > > outside of Chromeos. Can you discuss with Rafael, Andy, Srinivas
> > > > internally and let me know on how they'd like to proceed on this.
> > > >
> > > > The other option is to apply this patch as-is so we know that
> > > > there
> > > > is
> > > > no "functional change" and thus no possible regression (so the
> > > > driver
> > > > continues to attach to those and only those systems that s it
> > > > used
> > > > to,
> > > > before this patch). And then introduce the ACPI ID Change as a
> > > > new
> > > > independent patch.
> > >
> > > Use INT33A1 to enumerate, if this id doesn't exist then fallback to
> > > the
> > > cpuid style.

OK, I got to down to implement this and have a few questions.

The intel_pmc_driver does need some way to determine whether to choose
between spt_reg_map / cnp_reg_map / icl_reg_map. How should it choose
between these?

1) For ACPI based enumeration, I don't think the ACPI nodes on the
current systems will have anything to offer to help the driver
differentiate between these. So it will still need to choose one of
the above maps based on the CPU model. I assume that is OK. I expect
something like:

switch (boot_cpu_data.x86_model) {
case INTEL_FAM6_SKYLAKE_MOBILE:
case INTEL_FAM6_SKYLAKE_DESKTOP:
case INTEL_FAM6_KABYLAKE_MOBILE:
case INTEL_FAM6_KABYLAKE_DESKTOP:
pmcdev->map = &spt_reg_map;
break;

case INTEL_FAM6_CANONLAKE_DESKTOP:
pmcdev->map = &cnp_reg_map;
break;

case INTEL_FAM6_ICELAKE_DESKTOP:
pmcdev->map = &icl_reg_map;
break;
default:
/* Which map should we use by default? */
}


2) For the ACPI based enumeration, what should be the default map
chosen by the driver, for CPUs other than what is in that list today
(Or do we expect to keep adding to this switch/case for new CPU
models)?

3) For the fallback option, i.e. manually creating platform devices
using a new file, yes, we can pass a value in platform_data to
indicate which map to choose from, which is what I think should be
done. However, I am wondering if all this fallback might be an
overkill / over engineering. Do we know of any platforms that do not
have this ACPI device, and still are actually using this
intel_pmc_core driver? If not, may be we don't need this fallback
option?

Thanks,

Rajat

> > > The aim should be that we don't have to add any more
> > > CPU
> > > model to enumerate. But most probably register map will differ so
> > > we
> > > still may end up adding some CPU model relationship.
> >
> > Thanks for the guidance. Just to reconfirm my understanding of your
> > suggestion:
> >
> > Here is the suggestive code Rajneesh provided, and I intend to do it
> > similarly:
> >
> > +static const struct acpi_device_id pmc_acpi_ids[] = {
> > + {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID
> > PNP0D80*/
> > + { }
> > +};
> >
> > +static struct platform_driver pmc_plat_driver = {
> > + .remove = pmc_plat_remove,
> > + .probe = pmc_plat_probe,
> > + .driver = {
> > + .name = "pmc_core_driver",
> > + .acpi_match_table =
> > ACPI_PTR(pmc_acpi_ids),
> > + },
> > +};
> >
> > My understanding is that with this, the kernel would use
> > acpi_match_table to automatically create the platform_device on a
> > platform where ACPI tables contain the INT33A1 HID, and thus we don't
> > need to create the platform_device in module_init time on such
> > platforms.
> Yes. There will be /sys/bus/platform/devices/INT33A1:00.
>
>
> > So are you saying that during init, I should check if the
> > ACPI HID INT33A1 is not present on the platform, then use the cpu_id
> > table to create the platform_device? Thus newer platforms will not
> > need an entry in the table.
> Yes. Preferably in a different file as Andy would like. So the the
> current driver only implements platform driver for INT33A1. The other driver which will enumerate on CPUID and create INT33A1 platform device if there is no ACPI match via acpi_match_device() or similar API, for INT33A1. When you create a platform device the pmc driver will be probed.
>
> Thanks,
> Srinivas
>
>

2019-04-05 20:37:03

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v3 1/3] platform/x86: intel_pmc_core: Convert to a platform_driver

Convert the intel_pmc_core driver to a platform driver, and attach using
the ACPI enumeration method (via the ACPI device "INT33A1").

Signed-off-by: Rajat Jain <[email protected]>
---
v3: Don't instantiate the platform_device. Use ACPI enumeration.
v2: Rephrase the commit log. No code changes.

drivers/platform/x86/intel_pmc_core.c | 92 ++++++++++++++++++---------
1 file changed, 63 insertions(+), 29 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index f2c621b55f49..331889a57f73 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -19,6 +19,7 @@
#include <linux/io.h>
#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/platform_device.h>
#include <linux/uaccess.h>

#include <asm/cpu_device_id.h>
@@ -806,18 +807,6 @@ static inline void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
}
#endif /* CONFIG_DEBUG_FS */

-static const struct x86_cpu_id intel_pmc_core_ids[] = {
- INTEL_CPU_FAM6(SKYLAKE_MOBILE, spt_reg_map),
- INTEL_CPU_FAM6(SKYLAKE_DESKTOP, spt_reg_map),
- INTEL_CPU_FAM6(KABYLAKE_MOBILE, spt_reg_map),
- INTEL_CPU_FAM6(KABYLAKE_DESKTOP, spt_reg_map),
- INTEL_CPU_FAM6(CANNONLAKE_MOBILE, cnp_reg_map),
- INTEL_CPU_FAM6(ICELAKE_MOBILE, icl_reg_map),
- {}
-};
-
-MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids);
-
static const struct pci_device_id pmc_pci_ids[] = {
{ PCI_VDEVICE(INTEL, SPT_PMC_PCI_DEVICE_ID), 0},
{ 0, },
@@ -854,26 +843,50 @@ static const struct dmi_system_id pmc_core_dmi_table[] = {
{}
};

-static int __init pmc_core_probe(void)
+static int pmc_core_probe(struct platform_device *pdev)
{
+ static bool device_initialized;
struct pmc_dev *pmcdev = &pmc;
- const struct x86_cpu_id *cpu_id;
u64 slp_s0_addr;
int err;

- cpu_id = x86_match_cpu(intel_pmc_core_ids);
- if (!cpu_id)
+ if (device_initialized)
return -ENODEV;

- pmcdev->map = (struct pmc_reg_map *)cpu_id->driver_data;
+ switch (boot_cpu_data.x86_model) {
+ case INTEL_FAM6_SKYLAKE_MOBILE:
+ case INTEL_FAM6_SKYLAKE_DESKTOP:
+ case INTEL_FAM6_KABYLAKE_MOBILE:
+ case INTEL_FAM6_KABYLAKE_DESKTOP:

- /*
- * Coffeelake has CPU ID of Kabylake and Cannonlake PCH. So here
- * Sunrisepoint PCH regmap can't be used. Use Cannonlake PCH regmap
- * in this case.
- */
- if (pmcdev->map == &spt_reg_map && !pci_dev_present(pmc_pci_ids))
+ pmcdev->map = &spt_reg_map;
+
+ /*
+ * Special case: Coffeelake has CPU ID of Kabylake, but has
+ * Cannonlake PCH. So need to use cnp_reg_map instead of
+ * spt_reg_map for this special case. The PMC core PCI device
+ * on Coffeelake is hidden, so use that as the deciding factor.
+ */
+ if (!pci_dev_present(pmc_pci_ids))
+ pmcdev->map = &cnp_reg_map;
+
+ break;
+
+ case INTEL_FAM6_CANNONLAKE_MOBILE:
pmcdev->map = &cnp_reg_map;
+ break;
+
+ case INTEL_FAM6_ICELAKE_MOBILE:
+ pmcdev->map = &icl_reg_map;
+ break;
+ default:
+ /*
+ * Which map should we use by default? If not specified
+ * explicitly, assume Icelake by default for now.
+ */
+ pmcdev->map = &icl_reg_map;
+ break;
+ }

if (lpit_read_residency_count_address(&slp_s0_addr))
pmcdev->base_addr = PMC_BASE_ADDR_DEFAULT;
@@ -890,26 +903,47 @@ static int __init pmc_core_probe(void)

err = pmc_core_dbgfs_register(pmcdev);
if (err < 0) {
- pr_warn(" debugfs register failed.\n");
+ dev_warn(&pdev->dev, "debugfs register failed.\n");
iounmap(pmcdev->regbase);
return err;
}

dmi_check_system(pmc_core_dmi_table);
- pr_info(" initialized\n");
+ platform_set_drvdata(pdev, pmcdev);
+
+ dev_info(&pdev->dev, " initialized\n");
+ device_initialized = true;
+
return 0;
}
-module_init(pmc_core_probe)

-static void __exit pmc_core_remove(void)
+static int pmc_core_remove(struct platform_device *pdev)
{
- struct pmc_dev *pmcdev = &pmc;
+ struct pmc_dev *pmcdev = platform_get_drvdata(pdev);

+ platform_set_drvdata(pdev, NULL);
pmc_core_dbgfs_unregister(pmcdev);
mutex_destroy(&pmcdev->lock);
iounmap(pmcdev->regbase);
+ return 0;
}
-module_exit(pmc_core_remove)
+
+static const struct acpi_device_id pmc_core_acpi_ids[] = {
+ {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, pmc_core_acpi_ids);
+
+static struct platform_driver pmc_core_driver = {
+ .driver = {
+ .name = "pmc_core",
+ .acpi_match_table = ACPI_PTR(pmc_core_acpi_ids),
+ },
+ .probe = pmc_core_probe,
+ .remove = pmc_core_remove,
+};
+
+module_platform_driver(pmc_core_driver);

MODULE_LICENSE("GPL v2");
MODULE_DESCRIPTION("Intel PMC Core Driver");
--
2.21.0.392.gf8f6787159e-goog

2019-04-05 20:37:07

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v3 2/3] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

Add a module parameter which when enabled, will check on resume, if the
last S0ix attempt was successful. If not, the driver would warn and provide
helpful debug information (which gets latched during the failed suspend
attempt) to debug the S0ix failure.

This information is very useful to debug S0ix failures. Specially since
the latched debug information will be lost (over-written) if the system
attempts to go into runtime (or imminent) S0ix again after that failed
suspend attempt.

Signed-off-by: Rajat Jain <[email protected]>
---
v3: No changes
v2: Use pm_suspend_via_firmware() to enable the check only for S0ix

drivers/platform/x86/intel_pmc_core.c | 86 +++++++++++++++++++++++++++
drivers/platform/x86/intel_pmc_core.h | 7 +++
2 files changed, 93 insertions(+)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 331889a57f73..d9561a1c620d 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -20,6 +20,7 @@
#include <linux/module.h>
#include <linux/pci.h>
#include <linux/platform_device.h>
+#include <linux/suspend.h>
#include <linux/uaccess.h>

#include <asm/cpu_device_id.h>
@@ -928,6 +929,90 @@ static int pmc_core_remove(struct platform_device *pdev)
return 0;
}

+#ifdef CONFIG_PM_SLEEP
+
+static bool warn_on_s0ix_failures;
+module_param(warn_on_s0ix_failures, bool, 0644);
+MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix failures");
+
+static int pmc_core_suspend(struct device *dev)
+{
+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
+
+ /* Save PC10 and S0ix residency for checking later */
+ if (warn_on_s0ix_failures && !pm_suspend_via_firmware() &&
+ !rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter) &&
+ !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
+ pmcdev->check_counters = true;
+ else
+ pmcdev->check_counters = false;
+
+ return 0;
+}
+
+static inline bool pc10_failed(struct pmc_dev *pmcdev)
+{
+ u64 pc10_counter;
+
+ if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
+ pc10_counter == pmcdev->pc10_counter)
+ return true;
+ else
+ return false;
+}
+
+static inline bool s0ix_failed(struct pmc_dev *pmcdev)
+{
+ u64 s0ix_counter;
+
+ if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
+ s0ix_counter == pmcdev->s0ix_counter)
+ return true;
+ else
+ return false;
+}
+
+static int pmc_core_resume(struct device *dev)
+{
+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
+
+ if (!pmcdev->check_counters)
+ return 0;
+
+ if (pc10_failed(pmcdev)) {
+ dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
+ pmcdev->pc10_counter);
+ } else if (s0ix_failed(pmcdev)) {
+
+ const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
+ const struct pmc_bit_map *map;
+ int offset = pmcdev->map->slps0_dbg_offset;
+ u32 data;
+
+ dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
+ pmcdev->s0ix_counter);
+ while (*maps) {
+ map = *maps;
+ data = pmc_core_reg_read(pmcdev, offset);
+ offset += 4;
+ while (map->name) {
+ dev_warn(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
+ map->name,
+ data & map->bit_mask ? "Yes" : "No");
+ ++map;
+ }
+ ++maps;
+ }
+ }
+ return 0;
+}
+
+#endif
+
+static const struct dev_pm_ops pmc_core_pm_ops = {
+ SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend, pmc_core_resume)
+};
+
static const struct acpi_device_id pmc_core_acpi_ids[] = {
{"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
{ }
@@ -938,6 +1023,7 @@ static struct platform_driver pmc_core_driver = {
.driver = {
.name = "pmc_core",
.acpi_match_table = ACPI_PTR(pmc_core_acpi_ids),
+ .pm = &pmc_core_pm_ops,
},
.probe = pmc_core_probe,
.remove = pmc_core_remove,
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 88d9c0653a5f..fdee5772e532 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -241,6 +241,9 @@ 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)
*
* pmc_dev contains info about power management controller device.
*/
@@ -253,6 +256,10 @@ struct pmc_dev {
#endif /* CONFIG_DEBUG_FS */
int pmc_xram_read_bit;
struct mutex lock; /* generic mutex lock for PMC Core */
+
+ bool check_counters; /* Check for counter increments on resume */
+ u64 pc10_counter;
+ u64 s0ix_counter;
};

#endif /* PMC_CORE_H */
--
2.21.0.392.gf8f6787159e-goog

2019-04-05 20:37:33

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v3 3/3] platform/x86: intel_pmc_core: Instantiate pmc_core device on legacy platforms

Add code to instantiate the pmc_core platform device and thus attach to
the driver, if the ACPI device for the same ("INT33A1") is not present
in a system where it should be. This was discussed here:
https://www.mail-archive.com/[email protected]/msg1966991.html

Signed-off-by: Rajat Jain <[email protected]>
---
v3: (first version of *this* patch -to go with rest of v3 patchset)
v2: (does not exist)
v1: (does not exist)

drivers/platform/x86/Makefile | 2 +-
.../platform/x86/intel_pmc_core_plat_init.c | 64 +++++++++++++++++++
2 files changed, 65 insertions(+), 1 deletion(-)
create mode 100644 drivers/platform/x86/intel_pmc_core_plat_init.c

diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 86cb76677bc8..83d0bb9a14bb 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -89,7 +89,7 @@ obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU) += intel_bxtwc_tmu.o
obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \
intel_telemetry_pltdrv.o \
intel_telemetry_debugfs.o
-obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o
+obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o intel_pmc_core_plat_init.o
obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o
obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o
diff --git a/drivers/platform/x86/intel_pmc_core_plat_init.c b/drivers/platform/x86/intel_pmc_core_plat_init.c
new file mode 100644
index 000000000000..45be89e7586c
--- /dev/null
+++ b/drivers/platform/x86/intel_pmc_core_plat_init.c
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Intel PMC Core platform init
+ * Copyright (c) 2019, Google Inc.
+ * Author - Rajat Jain
+ *
+ * This code instantiates platform devices for intel_pmc_core driver, only
+ * on supported platforms that may not have the ACPI devices in the ACPI tables.
+ * No new platforms should be added here, because we expect that new platforms
+ * should all have the ACPI device, which is the preferred way of enumeration.
+ */
+
+#include <linux/acpi.h>
+#include <linux/platform_device.h>
+
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
+
+static struct platform_device pmc_core_device = {
+ .name = "pmc_core",
+};
+
+static int dummy;
+/*
+ * INTEL_CPU_FAM6 macro won't take NULL for driver_data (uses &driver_data),
+ * thus provide a dummy driver_data.
+ */
+static const struct x86_cpu_id intel_pmc_core_ids[] = {
+ INTEL_CPU_FAM6(SKYLAKE_MOBILE, dummy),
+ INTEL_CPU_FAM6(SKYLAKE_DESKTOP, dummy),
+ INTEL_CPU_FAM6(KABYLAKE_MOBILE, dummy),
+ INTEL_CPU_FAM6(KABYLAKE_DESKTOP, dummy),
+ INTEL_CPU_FAM6(CANNONLAKE_MOBILE, dummy),
+ INTEL_CPU_FAM6(ICELAKE_MOBILE, dummy),
+ {}
+};
+MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids);
+
+static int __init pmc_core_platform_init(void)
+{
+ int ret;
+
+ /* Skip creating the platform device if ACPI already has a device */
+ if (acpi_dev_present("INT33A1", NULL, -1) ||
+ !x86_match_cpu(intel_pmc_core_ids))
+ return -ENODEV;
+
+ ret = platform_device_register(&pmc_core_device);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static void __exit pmc_core_platform_exit(void)
+{
+ if (!acpi_dev_present("INT33A1", NULL, -1) &&
+ x86_match_cpu(intel_pmc_core_ids))
+ platform_device_unregister(&pmc_core_device);
+}
+
+module_init(pmc_core_platform_init);
+module_exit(pmc_core_platform_exit);
--
2.21.0.392.gf8f6787159e-goog

2019-04-08 16:53:31

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 1/3] platform/x86: intel_pmc_core: Convert to a platform_driver

On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <[email protected]> wrote:
>
> Convert the intel_pmc_core driver to a platform driver, and attach using
> the ACPI enumeration method (via the ACPI device "INT33A1").
>
> Signed-off-by: Rajat Jain <[email protected]>

> -static const struct x86_cpu_id intel_pmc_core_ids[] = {
> - INTEL_CPU_FAM6(SKYLAKE_MOBILE, spt_reg_map),
> - INTEL_CPU_FAM6(SKYLAKE_DESKTOP, spt_reg_map),
> - INTEL_CPU_FAM6(KABYLAKE_MOBILE, spt_reg_map),
> - INTEL_CPU_FAM6(KABYLAKE_DESKTOP, spt_reg_map),
> - INTEL_CPU_FAM6(CANNONLAKE_MOBILE, cnp_reg_map),
> - INTEL_CPU_FAM6(ICELAKE_MOBILE, icl_reg_map),
> - {}
> -};
> -
> -MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids);
> -

> +static int pmc_core_probe(struct platform_device *pdev)
> {
> + static bool device_initialized;
> struct pmc_dev *pmcdev = &pmc;
> - const struct x86_cpu_id *cpu_id;
> u64 slp_s0_addr;
> int err;
>
> - cpu_id = x86_match_cpu(intel_pmc_core_ids);
> - if (!cpu_id)
> + if (device_initialized)
> return -ENODEV;
>
> - pmcdev->map = (struct pmc_reg_map *)cpu_id->driver_data;

> + switch (boot_cpu_data.x86_model) {

I didn't get why this should be boot CPU?
Otherwise, leave the structure and leave the x86_match_cpu() call.

> + case INTEL_FAM6_SKYLAKE_MOBILE:
> + case INTEL_FAM6_SKYLAKE_DESKTOP:
> + case INTEL_FAM6_KABYLAKE_MOBILE:
> + case INTEL_FAM6_KABYLAKE_DESKTOP:
>
> - /*
> - * Coffeelake has CPU ID of Kabylake and Cannonlake PCH. So here
> - * Sunrisepoint PCH regmap can't be used. Use Cannonlake PCH regmap
> - * in this case.
> - */
> - if (pmcdev->map == &spt_reg_map && !pci_dev_present(pmc_pci_ids))
> + pmcdev->map = &spt_reg_map;
> +
> + /*
> + * Special case: Coffeelake has CPU ID of Kabylake, but has
> + * Cannonlake PCH. So need to use cnp_reg_map instead of
> + * spt_reg_map for this special case. The PMC core PCI device
> + * on Coffeelake is hidden, so use that as the deciding factor.
> + */
> + if (!pci_dev_present(pmc_pci_ids))
> + pmcdev->map = &cnp_reg_map;
> +
> + break;
> +
> + case INTEL_FAM6_CANNONLAKE_MOBILE:
> pmcdev->map = &cnp_reg_map;
> + break;
> +
> + case INTEL_FAM6_ICELAKE_MOBILE:
> + pmcdev->map = &icl_reg_map;
> + break;
> + default:
> + /*
> + * Which map should we use by default? If not specified
> + * explicitly, assume Icelake by default for now.
> + */
> + pmcdev->map = &icl_reg_map;
> + break;
> + }
>
> if (lpit_read_residency_count_address(&slp_s0_addr))
> pmcdev->base_addr = PMC_BASE_ADDR_DEFAULT;
> @@ -890,26 +903,47 @@ static int __init pmc_core_probe(void)
>
> err = pmc_core_dbgfs_register(pmcdev);
> if (err < 0) {
> - pr_warn(" debugfs register failed.\n");
> + dev_warn(&pdev->dev, "debugfs register failed.\n");
> iounmap(pmcdev->regbase);
> return err;
> }
>
> dmi_check_system(pmc_core_dmi_table);
> - pr_info(" initialized\n");
> + platform_set_drvdata(pdev, pmcdev);
> +
> + dev_info(&pdev->dev, " initialized\n");
> + device_initialized = true;
> +
> return 0;
> }
> -module_init(pmc_core_probe)
>
> -static void __exit pmc_core_remove(void)
> +static int pmc_core_remove(struct platform_device *pdev)
> {
> - struct pmc_dev *pmcdev = &pmc;
> + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
>
> + platform_set_drvdata(pdev, NULL);
> pmc_core_dbgfs_unregister(pmcdev);
> mutex_destroy(&pmcdev->lock);
> iounmap(pmcdev->regbase);
> + return 0;
> }
> -module_exit(pmc_core_remove)
> +
> +static const struct acpi_device_id pmc_core_acpi_ids[] = {
> + {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
> + { }
> +};
> +MODULE_DEVICE_TABLE(acpi, pmc_core_acpi_ids);
> +
> +static struct platform_driver pmc_core_driver = {
> + .driver = {
> + .name = "pmc_core",
> + .acpi_match_table = ACPI_PTR(pmc_core_acpi_ids),
> + },
> + .probe = pmc_core_probe,
> + .remove = pmc_core_remove,
> +};
> +
> +module_platform_driver(pmc_core_driver);
>
> MODULE_LICENSE("GPL v2");
> MODULE_DESCRIPTION("Intel PMC Core Driver");
> --
> 2.21.0.392.gf8f6787159e-goog
>


--
With Best Regards,
Andy Shevchenko

2019-04-08 17:03:37

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <[email protected]> wrote:
>
> Add a module parameter which when enabled, will check on resume, if the
> last S0ix attempt was successful. If not, the driver would warn and provide
> helpful debug information (which gets latched during the failed suspend
> attempt) to debug the S0ix failure.
>
> This information is very useful to debug S0ix failures. Specially since
> the latched debug information will be lost (over-written) if the system
> attempts to go into runtime (or imminent) S0ix again after that failed
> suspend attempt.

> +#ifdef CONFIG_PM_SLEEP
> +
> +static bool warn_on_s0ix_failures;
> +module_param(warn_on_s0ix_failures, bool, 0644);
> +MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix failures");
> +
> +static int pmc_core_suspend(struct device *dev)
> +{
> + struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> +
> + /* Save PC10 and S0ix residency for checking later */

> + if (warn_on_s0ix_failures && !pm_suspend_via_firmware() &&
> + !rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter) &&
> + !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
> + pmcdev->check_counters = true;

Perhaps something like

pmcdev->check_counters = false;
/* User doesn't want to be warned */
if (!warn_on...)
return 0;
/* We do suspend via firmware */
if (...)
return 0;
...

?

> + else
> + pmcdev->check_counters = false;
> +
> + return 0;
> +}
> +
> +static inline bool pc10_failed(struct pmc_dev *pmcdev)

To be or not to be? :-)
Perhaps names of the functions should be

pmc_code_is_pc10_failed()

and so on

> +{
> + u64 pc10_counter;
> +
> + if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
> + pc10_counter == pmcdev->pc10_counter)
> + return true;

> + else

Redundant.

> + return false;
> +}
> +
> +static inline bool s0ix_failed(struct pmc_dev *pmcdev)
> +{
> + u64 s0ix_counter;
> +
> + if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
> + s0ix_counter == pmcdev->s0ix_counter)
> + return true;

> + else

Ditto.

> + return false;
> +}
> +
> +static int pmc_core_resume(struct device *dev)
> +{
> + struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> +
> + if (!pmcdev->check_counters)
> + return 0;
> +
> + if (pc10_failed(pmcdev)) {
> + dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
> + pmcdev->pc10_counter);
> + } else if (s0ix_failed(pmcdev)) {
> +
> + const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
> + const struct pmc_bit_map *map;
> + int offset = pmcdev->map->slps0_dbg_offset;
> + u32 data;
> +
> + dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
> + pmcdev->s0ix_counter);
> + while (*maps) {
> + map = *maps;
> + data = pmc_core_reg_read(pmcdev, offset);
> + offset += 4;
> + while (map->name) {
> + dev_warn(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
> + map->name,
> + data & map->bit_mask ? "Yes" : "No");
> + ++map;
> + }
> + ++maps;
> + }

Can't we utilize existing print helpers?

> + }
> + return 0;
> +}
> +
> +#endif

--
With Best Regards,
Andy Shevchenko

2019-04-08 18:56:47

by Rajneesh Bhardwaj

[permalink] [raw]
Subject: Re: [PATCH v3 3/3] platform/x86: intel_pmc_core: Instantiate pmc_core device on legacy platforms

On Fri, Apr 05, 2019 at 01:35:58PM -0700, Rajat Jain wrote:
> Add code to instantiate the pmc_core platform device and thus attach to
> the driver, if the ACPI device for the same ("INT33A1") is not present
> in a system where it should be. This was discussed here:
> https://www.mail-archive.com/[email protected]/msg1966991.html
>
> Signed-off-by: Rajat Jain <[email protected]>
> ---
> v3: (first version of *this* patch -to go with rest of v3 patchset)
> v2: (does not exist)
> v1: (does not exist)
>
> drivers/platform/x86/Makefile | 2 +-
> .../platform/x86/intel_pmc_core_plat_init.c | 64 +++++++++++++++++++
> 2 files changed, 65 insertions(+), 1 deletion(-)
> create mode 100644 drivers/platform/x86/intel_pmc_core_plat_init.c

Can we call this intel_pmc_core_plat_drv.c instead?

>
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 86cb76677bc8..83d0bb9a14bb 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -89,7 +89,7 @@ obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU) += intel_bxtwc_tmu.o
> obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \
> intel_telemetry_pltdrv.o \
> intel_telemetry_debugfs.o
> -obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o
> +obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o intel_pmc_core_plat_init.o

ditto

> obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
> obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o
> obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o
> diff --git a/drivers/platform/x86/intel_pmc_core_plat_init.c b/drivers/platform/x86/intel_pmc_core_plat_init.c
> new file mode 100644
> index 000000000000..45be89e7586c
> --- /dev/null
> +++ b/drivers/platform/x86/intel_pmc_core_plat_init.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Intel PMC Core platform init
> + * Copyright (c) 2019, Google Inc.

I'd like to consult Andy and Darren if we should also have Intel Copyright
too?

> + * Author - Rajat Jain
> + *
> + * This code instantiates platform devices for intel_pmc_core driver, only
> + * on supported platforms that may not have the ACPI devices in the ACPI tables.
> + * No new platforms should be added here, because we expect that new platforms
> + * should all have the ACPI device, which is the preferred way of enumeration.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> +
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +
> +static struct platform_device pmc_core_device = {
> + .name = "pmc_core",
> +};
> +
> +static int dummy;
> +/*
> + * INTEL_CPU_FAM6 macro won't take NULL for driver_data (uses &driver_data),
> + * thus provide a dummy driver_data.
> + */
> +static const struct x86_cpu_id intel_pmc_core_ids[] = {
> + INTEL_CPU_FAM6(SKYLAKE_MOBILE, dummy),
> + INTEL_CPU_FAM6(SKYLAKE_DESKTOP, dummy),
> + INTEL_CPU_FAM6(KABYLAKE_MOBILE, dummy),
> + INTEL_CPU_FAM6(KABYLAKE_DESKTOP, dummy),
> + INTEL_CPU_FAM6(CANNONLAKE_MOBILE, dummy),
> + INTEL_CPU_FAM6(ICELAKE_MOBILE, dummy),
> + {}
> +};
> +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids);
> +
> +static int __init pmc_core_platform_init(void)
> +{
> + int ret;
> +
> + /* Skip creating the platform device if ACPI already has a device */
> + if (acpi_dev_present("INT33A1", NULL, -1) ||
> + !x86_match_cpu(intel_pmc_core_ids))
> + return -ENODEV;
> +
> + ret = platform_device_register(&pmc_core_device);
> + if (ret)
> + return ret;

we can just return platform_devie_register (...) and avoid extra lines and
variable.

> +
> + return 0;
> +}
> +
> +static void __exit pmc_core_platform_exit(void)
> +{
> + if (!acpi_dev_present("INT33A1", NULL, -1) &&
> + x86_match_cpu(intel_pmc_core_ids))
> + platform_device_unregister(&pmc_core_device);
> +}
> +
> +module_init(pmc_core_platform_init);
> +module_exit(pmc_core_platform_exit);
> --
> 2.21.0.392.gf8f6787159e-goog
>

--
Best Regards,
Rajneesh

2019-04-08 18:59:38

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH v3 3/3] platform/x86: intel_pmc_core: Instantiate pmc_core device on legacy platforms

On Mon, Apr 8, 2019 at 11:15 AM Rajneesh Bhardwaj
<[email protected]> wrote:
>
> On Fri, Apr 05, 2019 at 01:35:58PM -0700, Rajat Jain wrote:
> > Add code to instantiate the pmc_core platform device and thus attach to
> > the driver, if the ACPI device for the same ("INT33A1") is not present
> > in a system where it should be. This was discussed here:
> > https://www.mail-archive.com/[email protected]/msg1966991.html
> >
> > Signed-off-by: Rajat Jain <[email protected]>
> > ---
> > v3: (first version of *this* patch -to go with rest of v3 patchset)
> > v2: (does not exist)
> > v1: (does not exist)
> >
> > drivers/platform/x86/Makefile | 2 +-
> > .../platform/x86/intel_pmc_core_plat_init.c | 64 +++++++++++++++++++
> > 2 files changed, 65 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/platform/x86/intel_pmc_core_plat_init.c
>
> Can we call this intel_pmc_core_plat_drv.c instead?

Sure.

>
> >
> > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> > index 86cb76677bc8..83d0bb9a14bb 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -89,7 +89,7 @@ obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU) += intel_bxtwc_tmu.o
> > obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \
> > intel_telemetry_pltdrv.o \
> > intel_telemetry_debugfs.o
> > -obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o
> > +obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o intel_pmc_core_plat_init.o
>
> ditto

Sure.

>
> > obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
> > obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o
> > obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o
> > diff --git a/drivers/platform/x86/intel_pmc_core_plat_init.c b/drivers/platform/x86/intel_pmc_core_plat_init.c
> > new file mode 100644
> > index 000000000000..45be89e7586c
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel_pmc_core_plat_init.c
> > @@ -0,0 +1,64 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Intel PMC Core platform init
> > + * Copyright (c) 2019, Google Inc.
>
> I'd like to consult Andy and Darren if we should also have Intel Copyright
> too?
>
> > + * Author - Rajat Jain
> > + *
> > + * This code instantiates platform devices for intel_pmc_core driver, only
> > + * on supported platforms that may not have the ACPI devices in the ACPI tables.
> > + * No new platforms should be added here, because we expect that new platforms
> > + * should all have the ACPI device, which is the preferred way of enumeration.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <asm/cpu_device_id.h>
> > +#include <asm/intel-family.h>
> > +
> > +static struct platform_device pmc_core_device = {
> > + .name = "pmc_core",
> > +};
> > +
> > +static int dummy;
> > +/*
> > + * INTEL_CPU_FAM6 macro won't take NULL for driver_data (uses &driver_data),
> > + * thus provide a dummy driver_data.
> > + */
> > +static const struct x86_cpu_id intel_pmc_core_ids[] = {
> > + INTEL_CPU_FAM6(SKYLAKE_MOBILE, dummy),
> > + INTEL_CPU_FAM6(SKYLAKE_DESKTOP, dummy),
> > + INTEL_CPU_FAM6(KABYLAKE_MOBILE, dummy),
> > + INTEL_CPU_FAM6(KABYLAKE_DESKTOP, dummy),
> > + INTEL_CPU_FAM6(CANNONLAKE_MOBILE, dummy),
> > + INTEL_CPU_FAM6(ICELAKE_MOBILE, dummy),
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids);
> > +
> > +static int __init pmc_core_platform_init(void)
> > +{
> > + int ret;
> > +
> > + /* Skip creating the platform device if ACPI already has a device */
> > + if (acpi_dev_present("INT33A1", NULL, -1) ||
> > + !x86_match_cpu(intel_pmc_core_ids))
> > + return -ENODEV;
> > +
> > + ret = platform_device_register(&pmc_core_device);
> > + if (ret)
> > + return ret;
>
> we can just return platform_devie_register (...) and avoid extra lines and
> variable.

Sure.

>
> > +
> > + return 0;
> > +}
> > +
> > +static void __exit pmc_core_platform_exit(void)
> > +{
> > + if (!acpi_dev_present("INT33A1", NULL, -1) &&
> > + x86_match_cpu(intel_pmc_core_ids))
> > + platform_device_unregister(&pmc_core_device);
> > +}
> > +
> > +module_init(pmc_core_platform_init);
> > +module_exit(pmc_core_platform_exit);
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
> --
> Best Regards,
> Rajneesh

2019-04-08 19:02:42

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

On Mon, Apr 8, 2019 at 10:02 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <[email protected]> wrote:
> >
> > Add a module parameter which when enabled, will check on resume, if the
> > last S0ix attempt was successful. If not, the driver would warn and provide
> > helpful debug information (which gets latched during the failed suspend
> > attempt) to debug the S0ix failure.
> >
> > This information is very useful to debug S0ix failures. Specially since
> > the latched debug information will be lost (over-written) if the system
> > attempts to go into runtime (or imminent) S0ix again after that failed
> > suspend attempt.
>
> > +#ifdef CONFIG_PM_SLEEP
> > +
> > +static bool warn_on_s0ix_failures;
> > +module_param(warn_on_s0ix_failures, bool, 0644);
> > +MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix failures");
> > +
> > +static int pmc_core_suspend(struct device *dev)
> > +{
> > + struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> > +
> > + /* Save PC10 and S0ix residency for checking later */
>
> > + if (warn_on_s0ix_failures && !pm_suspend_via_firmware() &&
> > + !rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter) &&
> > + !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
> > + pmcdev->check_counters = true;
>
> Perhaps something like
>
> pmcdev->check_counters = false;
> /* User doesn't want to be warned */
> if (!warn_on...)
> return 0;
> /* We do suspend via firmware */
> if (...)
> return 0;
> ...
>
> ?

I guess what you mean is one conditional per line. Sure, I will do that.

>
> > + else
> > + pmcdev->check_counters = false;
> > +
> > + return 0;
> > +}
> > +
> > +static inline bool pc10_failed(struct pmc_dev *pmcdev)
>
> To be or not to be? :-)
> Perhaps names of the functions should be
>
> pmc_code_is_pc10_failed()
>
> and so on

I think the suggestion is to use pmc_core_* as the function names. OK,
I will rename the functions to:

pmc_core_pc10_failed()
and
pmc_core_s0ix_failed()


>
> > +{
> > + u64 pc10_counter;
> > +
> > + if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
> > + pc10_counter == pmcdev->pc10_counter)
> > + return true;
>
> > + else
>
> Redundant.

OK, I'll remove the "else" part here.

>
> > + return false;
> > +}
> > +
> > +static inline bool s0ix_failed(struct pmc_dev *pmcdev)
> > +{
> > + u64 s0ix_counter;
> > +
> > + if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
> > + s0ix_counter == pmcdev->s0ix_counter)
> > + return true;
>
> > + else
>
> Ditto.

OK, I'll remove the "else" part here.

>
> > + return false;
> > +}
> > +
> > +static int pmc_core_resume(struct device *dev)
> > +{
> > + struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> > +
> > + if (!pmcdev->check_counters)
> > + return 0;
> > +
> > + if (pc10_failed(pmcdev)) {
> > + dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
> > + pmcdev->pc10_counter);
> > + } else if (s0ix_failed(pmcdev)) {
> > +
> > + const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
> > + const struct pmc_bit_map *map;
> > + int offset = pmcdev->map->slps0_dbg_offset;
> > + u32 data;
> > +
> > + dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
> > + pmcdev->s0ix_counter);
> > + while (*maps) {
> > + map = *maps;
> > + data = pmc_core_reg_read(pmcdev, offset);
> > + offset += 4;
> > + while (map->name) {
> > + dev_warn(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
> > + map->name,
> > + data & map->bit_mask ? "Yes" : "No");
> > + ++map;
> > + }
> > + ++maps;
> > + }
>
> Can't we utilize existing print helpers?

I didn't quite see any existing print helpers in this file. I took
this code from pmc_core_slps0_dbg_show(), and I think although I can
abstract out this code into a static function, the calling code need
to use seq_printf(s,...) and dev_warn(dev,...) respectively. - so
might be overkill (did not feel that the benefits were worth it).
Please let me know if you have any suggestions and will be happy to
use them.

Thanks,

Rajat


>
> > + }
> > + return 0;
> > +}
> > +
> > +#endif
>
> --
> With Best Regards,
> Andy Shevchenko

2019-04-08 19:08:22

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 1/3] platform/x86: intel_pmc_core: Convert to a platform_driver

On Mon, Apr 8, 2019 at 9:43 PM Rajat Jain <[email protected]> wrote:
> On Mon, Apr 8, 2019 at 9:51 AM Andy Shevchenko
> <[email protected]> wrote:
> > On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <[email protected]> wrote:

> > > + switch (boot_cpu_data.x86_model) {
> >
> > I didn't get why this should be boot CPU?
> > Otherwise, leave the structure and leave the x86_match_cpu() call.
>
> I didn't quite understand the concern. The x86_match_cpu() also uses
> the same boot_cpu_data that I've used, am I missing something?

It's a detail of implementation, and instead of continue using nice
helpers, you open coded the similar.
Why?

--
With Best Regards,
Andy Shevchenko

2019-04-08 19:12:15

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

On Mon, Apr 8, 2019 at 11:41 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Mon, Apr 8, 2019 at 9:36 PM Rajat Jain <[email protected]> wrote:
> > On Mon, Apr 8, 2019 at 10:02 AM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <[email protected]> wrote:
>
> > > Perhaps something like
> > >
> > > pmcdev->check_counters = false;
> > > /* User doesn't want to be warned */
> > > if (!warn_on...)
> > > return 0;
> > > /* We do suspend via firmware */
> > > if (...)
> > > return 0;
> > > ...
> > >
> > > ?
> >
> > I guess what you mean is one conditional per line. Sure, I will do that.
>
> Yes
>
> > > > +static inline bool pc10_failed(struct pmc_dev *pmcdev)
> > >
> > > To be or not to be? :-)
> > > Perhaps names of the functions should be
> > >
> > > pmc_code_is_pc10_failed()
> > >
> > > and so on
> >
> > I think the suggestion is to use pmc_core_* as the function names. OK,
> > I will rename the functions to:
> >
> > pmc_core_pc10_failed()
> > and
> > pmc_core_s0ix_failed()
>
> And verb "to be". See above.
>
> > > Can't we utilize existing print helpers?
> >
> > I didn't quite see any existing print helpers in this file. I took
> > this code from pmc_core_slps0_dbg_show(), and I think although I can
> > abstract out this code into a static function, the calling code need
> > to use seq_printf(s,...) and dev_warn(dev,...) respectively. - so
> > might be overkill (did not feel that the benefits were worth it).
> > Please let me know if you have any suggestions and will be happy to
> > use them.
>
> Instead of adding module parameter and doing these prints, perhaps
> introduce another debugfs node.

Uh, I actually did wanted to print them at the resume time in kernel
logs, because I think this is something kernel developers would be
responsible for debugging and thus would be great to have this
included within the kernel logs. User space tools may differ on
different distros and may or may not be looking for S0ix failures, and
particularly may not dump this new debugfs attribute.

The other thing is that seemingly this could also help in situations
where debugfs is not configured?

Thanks,

Rajat


>
> --
> With Best Regards,
> Andy Shevchenko

2019-04-08 19:20:11

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 3/3] platform/x86: intel_pmc_core: Instantiate pmc_core device on legacy platforms

On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <[email protected]> wrote:
>
> Add code to instantiate the pmc_core platform device and thus attach to
> the driver, if the ACPI device for the same ("INT33A1") is not present
> in a system where it should be. This was discussed here:
> https://www.mail-archive.com/[email protected]/msg1966991.html
>



> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> +
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +
> +static struct platform_device pmc_core_device = {
> + .name = "pmc_core",
> +};
> +

> +static int dummy;
> +/*
> + * INTEL_CPU_FAM6 macro won't take NULL for driver_data (uses &driver_data),
> + * thus provide a dummy driver_data.
> + */

You may use pmc_core_device instead, right?

> +static const struct x86_cpu_id intel_pmc_core_ids[] = {
> + INTEL_CPU_FAM6(SKYLAKE_MOBILE, dummy),
> + INTEL_CPU_FAM6(SKYLAKE_DESKTOP, dummy),
> + INTEL_CPU_FAM6(KABYLAKE_MOBILE, dummy),
> + INTEL_CPU_FAM6(KABYLAKE_DESKTOP, dummy),
> + INTEL_CPU_FAM6(CANNONLAKE_MOBILE, dummy),
> + INTEL_CPU_FAM6(ICELAKE_MOBILE, dummy),
> + {}
> +};
> +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids);
> +
> +static int __init pmc_core_platform_init(void)
> +{
> + int ret;
> +
> + /* Skip creating the platform device if ACPI already has a device */
> + if (acpi_dev_present("INT33A1", NULL, -1) ||
> + !x86_match_cpu(intel_pmc_core_ids))
> + return -ENODEV;

Split it to one conditional per line.

> +
> + ret = platform_device_register(&pmc_core_device);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static void __exit pmc_core_platform_exit(void)
> +{

> + if (!acpi_dev_present("INT33A1", NULL, -1) &&
> + x86_match_cpu(intel_pmc_core_ids))

Redundant check.

> + platform_device_unregister(&pmc_core_device);
> +}

--
With Best Regards,
Andy Shevchenko

2019-04-08 19:41:19

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH v3 1/3] platform/x86: intel_pmc_core: Convert to a platform_driver

Hi,

Thank you for the review.

On Mon, Apr 8, 2019 at 9:51 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <[email protected]> wrote:
> >
> > Convert the intel_pmc_core driver to a platform driver, and attach using
> > the ACPI enumeration method (via the ACPI device "INT33A1").
> >
> > Signed-off-by: Rajat Jain <[email protected]>
>
> > -static const struct x86_cpu_id intel_pmc_core_ids[] = {
> > - INTEL_CPU_FAM6(SKYLAKE_MOBILE, spt_reg_map),
> > - INTEL_CPU_FAM6(SKYLAKE_DESKTOP, spt_reg_map),
> > - INTEL_CPU_FAM6(KABYLAKE_MOBILE, spt_reg_map),
> > - INTEL_CPU_FAM6(KABYLAKE_DESKTOP, spt_reg_map),
> > - INTEL_CPU_FAM6(CANNONLAKE_MOBILE, cnp_reg_map),
> > - INTEL_CPU_FAM6(ICELAKE_MOBILE, icl_reg_map),
> > - {}
> > -};
> > -
> > -MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids);
> > -
>
> > +static int pmc_core_probe(struct platform_device *pdev)
> > {
> > + static bool device_initialized;
> > struct pmc_dev *pmcdev = &pmc;
> > - const struct x86_cpu_id *cpu_id;
> > u64 slp_s0_addr;
> > int err;
> >
> > - cpu_id = x86_match_cpu(intel_pmc_core_ids);
> > - if (!cpu_id)
> > + if (device_initialized)
> > return -ENODEV;
> >
> > - pmcdev->map = (struct pmc_reg_map *)cpu_id->driver_data;
>
> > + switch (boot_cpu_data.x86_model) {
>
> I didn't get why this should be boot CPU?
> Otherwise, leave the structure and leave the x86_match_cpu() call.

I didn't quite understand the concern. The x86_match_cpu() also uses
the same boot_cpu_data that I've used, am I missing something?

Thanks,

Rajat


>
> > + case INTEL_FAM6_SKYLAKE_MOBILE:
> > + case INTEL_FAM6_SKYLAKE_DESKTOP:
> > + case INTEL_FAM6_KABYLAKE_MOBILE:
> > + case INTEL_FAM6_KABYLAKE_DESKTOP:
> >
> > - /*
> > - * Coffeelake has CPU ID of Kabylake and Cannonlake PCH. So here
> > - * Sunrisepoint PCH regmap can't be used. Use Cannonlake PCH regmap
> > - * in this case.
> > - */
> > - if (pmcdev->map == &spt_reg_map && !pci_dev_present(pmc_pci_ids))
> > + pmcdev->map = &spt_reg_map;
> > +
> > + /*
> > + * Special case: Coffeelake has CPU ID of Kabylake, but has
> > + * Cannonlake PCH. So need to use cnp_reg_map instead of
> > + * spt_reg_map for this special case. The PMC core PCI device
> > + * on Coffeelake is hidden, so use that as the deciding factor.
> > + */
> > + if (!pci_dev_present(pmc_pci_ids))
> > + pmcdev->map = &cnp_reg_map;
> > +
> > + break;
> > +
> > + case INTEL_FAM6_CANNONLAKE_MOBILE:
> > pmcdev->map = &cnp_reg_map;
> > + break;
> > +
> > + case INTEL_FAM6_ICELAKE_MOBILE:
> > + pmcdev->map = &icl_reg_map;
> > + break;
> > + default:
> > + /*
> > + * Which map should we use by default? If not specified
> > + * explicitly, assume Icelake by default for now.
> > + */
> > + pmcdev->map = &icl_reg_map;
> > + break;
> > + }
> >
> > if (lpit_read_residency_count_address(&slp_s0_addr))
> > pmcdev->base_addr = PMC_BASE_ADDR_DEFAULT;
> > @@ -890,26 +903,47 @@ static int __init pmc_core_probe(void)
> >
> > err = pmc_core_dbgfs_register(pmcdev);
> > if (err < 0) {
> > - pr_warn(" debugfs register failed.\n");
> > + dev_warn(&pdev->dev, "debugfs register failed.\n");
> > iounmap(pmcdev->regbase);
> > return err;
> > }
> >
> > dmi_check_system(pmc_core_dmi_table);
> > - pr_info(" initialized\n");
> > + platform_set_drvdata(pdev, pmcdev);
> > +
> > + dev_info(&pdev->dev, " initialized\n");
> > + device_initialized = true;
> > +
> > return 0;
> > }
> > -module_init(pmc_core_probe)
> >
> > -static void __exit pmc_core_remove(void)
> > +static int pmc_core_remove(struct platform_device *pdev)
> > {
> > - struct pmc_dev *pmcdev = &pmc;
> > + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> >
> > + platform_set_drvdata(pdev, NULL);
> > pmc_core_dbgfs_unregister(pmcdev);
> > mutex_destroy(&pmcdev->lock);
> > iounmap(pmcdev->regbase);
> > + return 0;
> > }
> > -module_exit(pmc_core_remove)
> > +
> > +static const struct acpi_device_id pmc_core_acpi_ids[] = {
> > + {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(acpi, pmc_core_acpi_ids);
> > +
> > +static struct platform_driver pmc_core_driver = {
> > + .driver = {
> > + .name = "pmc_core",
> > + .acpi_match_table = ACPI_PTR(pmc_core_acpi_ids),
> > + },
> > + .probe = pmc_core_probe,
> > + .remove = pmc_core_remove,
> > +};
> > +
> > +module_platform_driver(pmc_core_driver);
> >
> > MODULE_LICENSE("GPL v2");
> > MODULE_DESCRIPTION("Intel PMC Core Driver");
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko

2019-04-08 20:36:45

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH v3 3/3] platform/x86: intel_pmc_core: Instantiate pmc_core device on legacy platforms

On Mon, Apr 8, 2019 at 10:07 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <[email protected]> wrote:
> >
> > Add code to instantiate the pmc_core platform device and thus attach to
> > the driver, if the ACPI device for the same ("INT33A1") is not present
> > in a system where it should be. This was discussed here:
> > https://www.mail-archive.com/[email protected]/msg1966991.html
> >
>
>
>
> > +#include <linux/acpi.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <asm/cpu_device_id.h>
> > +#include <asm/intel-family.h>
> > +
> > +static struct platform_device pmc_core_device = {
> > + .name = "pmc_core",
> > +};
> > +
>
> > +static int dummy;
> > +/*
> > + * INTEL_CPU_FAM6 macro won't take NULL for driver_data (uses &driver_data),
> > + * thus provide a dummy driver_data.
> > + */
>
> You may use pmc_core_device instead, right?

sure, will do.

>
> > +static const struct x86_cpu_id intel_pmc_core_ids[] = {
> > + INTEL_CPU_FAM6(SKYLAKE_MOBILE, dummy),
> > + INTEL_CPU_FAM6(SKYLAKE_DESKTOP, dummy),
> > + INTEL_CPU_FAM6(KABYLAKE_MOBILE, dummy),
> > + INTEL_CPU_FAM6(KABYLAKE_DESKTOP, dummy),
> > + INTEL_CPU_FAM6(CANNONLAKE_MOBILE, dummy),
> > + INTEL_CPU_FAM6(ICELAKE_MOBILE, dummy),
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids);
> > +
> > +static int __init pmc_core_platform_init(void)
> > +{
> > + int ret;
> > +
> > + /* Skip creating the platform device if ACPI already has a device */
> > + if (acpi_dev_present("INT33A1", NULL, -1) ||
> > + !x86_match_cpu(intel_pmc_core_ids))
> > + return -ENODEV;
>
> Split it to one conditional per line.

sure, will do.

>
> > +
> > + ret = platform_device_register(&pmc_core_device);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static void __exit pmc_core_platform_exit(void)
> > +{
>
> > + if (!acpi_dev_present("INT33A1", NULL, -1) &&
> > + x86_match_cpu(intel_pmc_core_ids))
>
> Redundant check.

will fix.

>
> > + platform_device_unregister(&pmc_core_device);
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko

2019-04-08 20:44:03

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

On Mon, Apr 8, 2019 at 9:36 PM Rajat Jain <[email protected]> wrote:
> On Mon, Apr 8, 2019 at 10:02 AM Andy Shevchenko
> <[email protected]> wrote:
> > On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <[email protected]> wrote:

> > Perhaps something like
> >
> > pmcdev->check_counters = false;
> > /* User doesn't want to be warned */
> > if (!warn_on...)
> > return 0;
> > /* We do suspend via firmware */
> > if (...)
> > return 0;
> > ...
> >
> > ?
>
> I guess what you mean is one conditional per line. Sure, I will do that.

Yes

> > > +static inline bool pc10_failed(struct pmc_dev *pmcdev)
> >
> > To be or not to be? :-)
> > Perhaps names of the functions should be
> >
> > pmc_code_is_pc10_failed()
> >
> > and so on
>
> I think the suggestion is to use pmc_core_* as the function names. OK,
> I will rename the functions to:
>
> pmc_core_pc10_failed()
> and
> pmc_core_s0ix_failed()

And verb "to be". See above.

> > Can't we utilize existing print helpers?
>
> I didn't quite see any existing print helpers in this file. I took
> this code from pmc_core_slps0_dbg_show(), and I think although I can
> abstract out this code into a static function, the calling code need
> to use seq_printf(s,...) and dev_warn(dev,...) respectively. - so
> might be overkill (did not feel that the benefits were worth it).
> Please let me know if you have any suggestions and will be happy to
> use them.

Instead of adding module parameter and doing these prints, perhaps
introduce another debugfs node.

--
With Best Regards,
Andy Shevchenko

2019-04-08 20:45:29

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH v3 1/3] platform/x86: intel_pmc_core: Convert to a platform_driver

On Mon, Apr 8, 2019 at 11:44 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Mon, Apr 8, 2019 at 9:43 PM Rajat Jain <[email protected]> wrote:
> > On Mon, Apr 8, 2019 at 9:51 AM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <[email protected]> wrote:
>
> > > > + switch (boot_cpu_data.x86_model) {
> > >
> > > I didn't get why this should be boot CPU?
> > > Otherwise, leave the structure and leave the x86_match_cpu() call.
> >
> > I didn't quite understand the concern. The x86_match_cpu() also uses
> > the same boot_cpu_data that I've used, am I missing something?
>
> It's a detail of implementation, and instead of continue using nice
> helpers, you open coded the similar.
> Why?

OK, sure. I will put back the intel_pmc_core_ids structure and the
x86_match_cpu().

Thanks,

Rajat

>
> --
> With Best Regards,
> Andy Shevchenko

2019-04-08 20:59:41

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

On Mon, Apr 8, 2019 at 9:58 PM Rajat Jain <[email protected]> wrote:
> On Mon, Apr 8, 2019 at 11:41 AM Andy Shevchenko
> <[email protected]> wrote:
> > On Mon, Apr 8, 2019 at 9:36 PM Rajat Jain <[email protected]> wrote:

> > Instead of adding module parameter and doing these prints, perhaps
> > introduce another debugfs node.
>
> Uh, I actually did wanted to print them at the resume time in kernel
> logs, because I think this is something kernel developers would be
> responsible for debugging and thus would be great to have this
> included within the kernel logs. User space tools may differ on
> different distros and may or may not be looking for S0ix failures, and
> particularly may not dump this new debugfs attribute.
>
> The other thing is that seemingly this could also help in situations
> where debugfs is not configured?

The keywords above "developers", "debugging", so, the concept of this
driver is almost all about debugging, debugfs is not for users or esp.
user space tools.
So, I don't see any issue here with creating another node and use it
for *debugging* whenever it's needed.

--
With Best Regards,
Andy Shevchenko

2019-04-09 19:40:11

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

On Mon, Apr 8, 2019 at 12:34 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Mon, Apr 8, 2019 at 9:58 PM Rajat Jain <[email protected]> wrote:
> > On Mon, Apr 8, 2019 at 11:41 AM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Mon, Apr 8, 2019 at 9:36 PM Rajat Jain <[email protected]> wrote:
>
> > > Instead of adding module parameter and doing these prints, perhaps
> > > introduce another debugfs node.
> >
> > Uh, I actually did wanted to print them at the resume time in kernel
> > logs, because I think this is something kernel developers would be
> > responsible for debugging and thus would be great to have this
> > included within the kernel logs. User space tools may differ on
> > different distros and may or may not be looking for S0ix failures, and
> > particularly may not dump this new debugfs attribute.
> >
> > The other thing is that seemingly this could also help in situations
> > where debugfs is not configured?
>
> The keywords above "developers", "debugging", so, the concept of this
> driver is almost all about debugging, debugfs is not for users or esp.
> user space tools.
> So, I don't see any issue here with creating another node and use it
> for *debugging* whenever it's needed.

Hi Andy,

In the past, we did see random suspend (S0ix) failures after the
devices were released to users (S0ix failures reasons can be a lot so
was not possible to catch all in lab testing). Rajneesh (@Intel) is
aware of the context. In most of these cases, it was very difficult to
reproduce the issue in developer environment, so really difficult to
arrive at what component was causing those failures without these
logs. So I'd still like to make one last request to allow to these
debugs in the kernel at resume time, to make this feature very useful
(it still doesn't change anything for anyone not enabling the module
parameter). However if you still have a strong opinion against this,
I'll add it as a new debugfs attribute in the new re-spin of the
patch.

Thanks & Best Regards,

Rajat

>
> --
> With Best Regards,
> Andy Shevchenko

2019-04-11 00:32:15

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v4 1/3] platform/x86: intel_pmc_core: Convert to a platform_driver

Convert the intel_pmc_core driver to a platform driver, and attach using
the ACPI enumeration method (via the ACPI device "INT33A1").

Change-Id: I2c2c9bbff48a6548f5c96e18d766ffc4633762c0
Signed-off-by: Rajat Jain <[email protected]>
---
v4: put back the x86_match_cpu() method.
v3: Don't instantiate the platform_device. Use ACPI enumeration.
v2: Rephrase the commit log. No code changes.

drivers/platform/x86/intel_pmc_core.c | 42 ++++++++++++++++++++++-----
1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 9908d233305e..8da886e17681 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -19,6 +19,7 @@
#include <linux/io.h>
#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/platform_device.h>
#include <linux/uaccess.h>

#include <asm/cpu_device_id.h>
@@ -854,13 +855,17 @@ static const struct dmi_system_id pmc_core_dmi_table[] = {
{}
};

-static int __init pmc_core_probe(void)
+static int pmc_core_probe(struct platform_device *pdev)
{
+ static bool device_initialized;
struct pmc_dev *pmcdev = &pmc;
const struct x86_cpu_id *cpu_id;
u64 slp_s0_addr;
int err;

+ if (device_initialized)
+ return -ENODEV;
+
cpu_id = x86_match_cpu(intel_pmc_core_ids);
if (!cpu_id)
return -ENODEV;
@@ -888,28 +893,49 @@ static int __init pmc_core_probe(void)
mutex_init(&pmcdev->lock);
pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();

+ dmi_check_system(pmc_core_dmi_table);
+ platform_set_drvdata(pdev, pmcdev);
+
err = pmc_core_dbgfs_register(pmcdev);
if (err < 0) {
- pr_warn(" debugfs register failed.\n");
+ dev_warn(&pdev->dev, "debugfs register failed.\n");
iounmap(pmcdev->regbase);
return err;
}

- dmi_check_system(pmc_core_dmi_table);
- pr_info(" initialized\n");
+ dev_info(&pdev->dev, " initialized\n");
+ device_initialized = true;
+
return 0;
}
-module_init(pmc_core_probe)

-static void __exit pmc_core_remove(void)
+static int pmc_core_remove(struct platform_device *pdev)
{
- struct pmc_dev *pmcdev = &pmc;
+ struct pmc_dev *pmcdev = platform_get_drvdata(pdev);

+ platform_set_drvdata(pdev, NULL);
pmc_core_dbgfs_unregister(pmcdev);
mutex_destroy(&pmcdev->lock);
iounmap(pmcdev->regbase);
+ return 0;
}
-module_exit(pmc_core_remove)
+
+static const struct acpi_device_id pmc_core_acpi_ids[] = {
+ {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, pmc_core_acpi_ids);
+
+static struct platform_driver pmc_core_driver = {
+ .driver = {
+ .name = "pmc_core",
+ .acpi_match_table = ACPI_PTR(pmc_core_acpi_ids),
+ },
+ .probe = pmc_core_probe,
+ .remove = pmc_core_remove,
+};
+
+module_platform_driver(pmc_core_driver);

MODULE_LICENSE("GPL v2");
MODULE_DESCRIPTION("Intel PMC Core Driver");
--
2.21.0.392.gf8f6787159e-goog

2019-04-11 00:32:22

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v4 2/3] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

Add a module parameter which when enabled, will check on resume, if the
last S0ix attempt was successful. If not, the driver would warn and provide
helpful debug information (which gets latched during the failed suspend
attempt) to debug the S0ix failure.

This information is very useful to debug S0ix failures. Specially since
the latched debug information will be lost (over-written) if the system
attempts to go into runtime (or imminent) S0ix again after that failed
suspend attempt.

Signed-off-by: Rajat Jain <[email protected]>
---
v4: Use 1 condition per if statement, rename some functions.
v3: No changes
v2: Use pm_suspend_via_firmware() to enable the check only for S0ix

drivers/platform/x86/intel_pmc_core.c | 93 +++++++++++++++++++++++++++
drivers/platform/x86/intel_pmc_core.h | 7 ++
2 files changed, 100 insertions(+)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 8da886e17681..b1f099de5cb3 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -20,6 +20,7 @@
#include <linux/module.h>
#include <linux/pci.h>
#include <linux/platform_device.h>
+#include <linux/suspend.h>
#include <linux/uaccess.h>

#include <asm/cpu_device_id.h>
@@ -920,6 +921,97 @@ static int pmc_core_remove(struct platform_device *pdev)
return 0;
}

+#ifdef CONFIG_PM_SLEEP
+
+static bool warn_on_s0ix_failures;
+module_param(warn_on_s0ix_failures, bool, 0644);
+MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix failures");
+
+static 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;
+
+ /* Check if the syspend will actually use S0ix */
+ if (pm_suspend_via_firmware())
+ return 0;
+
+ /* Save PC10 and S0ix residency for checking later */
+ if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter) &&
+ !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
+ pmcdev->check_counters = true;
+
+ return 0;
+}
+
+static inline bool pmc_core_is_pc10_failed(struct pmc_dev *pmcdev)
+{
+ u64 pc10_counter;
+
+ if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
+ pc10_counter == pmcdev->pc10_counter)
+ return true;
+
+ return false;
+}
+
+static inline bool pmc_core_is_s0ix_failed(struct pmc_dev *pmcdev)
+{
+ u64 s0ix_counter;
+
+ if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
+ s0ix_counter == pmcdev->s0ix_counter)
+ return true;
+
+ return false;
+}
+
+static int pmc_core_resume(struct device *dev)
+{
+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
+
+ if (!pmcdev->check_counters)
+ return 0;
+
+ if (pmc_core_is_pc10_failed(pmcdev)) {
+ dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
+ pmcdev->pc10_counter);
+ } else if (pmc_core_is_s0ix_failed(pmcdev)) {
+
+ const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
+ const struct pmc_bit_map *map;
+ int offset = pmcdev->map->slps0_dbg_offset;
+ u32 data;
+
+ dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
+ pmcdev->s0ix_counter);
+ while (*maps) {
+ map = *maps;
+ data = pmc_core_reg_read(pmcdev, offset);
+ offset += 4;
+ while (map->name) {
+ dev_warn(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
+ map->name,
+ data & map->bit_mask ? "Yes" : "No");
+ ++map;
+ }
+ ++maps;
+ }
+ }
+ return 0;
+}
+
+#endif
+
+static const struct dev_pm_ops pmc_core_pm_ops = {
+ SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend, pmc_core_resume)
+};
+
static const struct acpi_device_id pmc_core_acpi_ids[] = {
{"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
{ }
@@ -930,6 +1022,7 @@ static struct platform_driver pmc_core_driver = {
.driver = {
.name = "pmc_core",
.acpi_match_table = ACPI_PTR(pmc_core_acpi_ids),
+ .pm = &pmc_core_pm_ops,
},
.probe = pmc_core_probe,
.remove = pmc_core_remove,
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 88d9c0653a5f..fdee5772e532 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -241,6 +241,9 @@ 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)
*
* pmc_dev contains info about power management controller device.
*/
@@ -253,6 +256,10 @@ struct pmc_dev {
#endif /* CONFIG_DEBUG_FS */
int pmc_xram_read_bit;
struct mutex lock; /* generic mutex lock for PMC Core */
+
+ bool check_counters; /* Check for counter increments on resume */
+ u64 pc10_counter;
+ u64 s0ix_counter;
};

#endif /* PMC_CORE_H */
--
2.21.0.392.gf8f6787159e-goog

2019-04-11 00:34:38

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v4 3/3] platform/x86: intel_pmc_core: Instantiate pmc_core device on legacy platforms

Add code to instantiate the pmc_core platform device and thus attach to
the driver, if the ACPI device for the same ("INT33A1") is not present
in a system where it should be. This was discussed here:
https://www.mail-archive.com/[email protected]/msg1966991.html

Change-Id: I62e0909b010c13614feec36e2c72aed984b61734
Signed-off-by: Rajat Jain <[email protected]>
---
v4: Rename file, remove dummy arg, 1 conditional per if statement,
simplify init / exit calls..
v3: (first version of *this* patch -to go with rest of v3 patchset)
v2: (does not exist)
v1: (does not exist)

drivers/platform/x86/Makefile | 2 +-
.../platform/x86/intel_pmc_core_plat_drv.c | 60 +++++++++++++++++++
2 files changed, 61 insertions(+), 1 deletion(-)
create mode 100644 drivers/platform/x86/intel_pmc_core_plat_drv.c

diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 86cb76677bc8..7041a88c34c7 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -89,7 +89,7 @@ obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU) += intel_bxtwc_tmu.o
obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \
intel_telemetry_pltdrv.o \
intel_telemetry_debugfs.o
-obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o
+obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o intel_pmc_core_plat_drv.o
obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o
obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o
diff --git a/drivers/platform/x86/intel_pmc_core_plat_drv.c b/drivers/platform/x86/intel_pmc_core_plat_drv.c
new file mode 100644
index 000000000000..7f5af5bd7f1f
--- /dev/null
+++ b/drivers/platform/x86/intel_pmc_core_plat_drv.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Intel PMC Core platform init
+ * Copyright (c) 2019, Google Inc.
+ * Author - Rajat Jain
+ *
+ * This code instantiates platform devices for intel_pmc_core driver, only
+ * on supported platforms that may not have the ACPI devices in the ACPI tables.
+ * No new platforms should be added here, because we expect that new platforms
+ * should all have the ACPI device, which is the preferred way of enumeration.
+ */
+
+#include <linux/acpi.h>
+#include <linux/platform_device.h>
+
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
+
+static struct platform_device pmc_core_device = {
+ .name = "pmc_core",
+};
+
+/*
+ * intel_pmc_core_platform_ids is the list of platforms where we want to
+ * instantiate the platform_device if not already instantiated. This is
+ * different than intel_pmc_core_ids in intel_pmc_core.c which is the
+ * list of platforms that the driver supports for pmc_core device. The
+ * other list may grow, but this list should not.
+ */
+static const struct x86_cpu_id intel_pmc_core_platform_ids[] = {
+ INTEL_CPU_FAM6(SKYLAKE_MOBILE, pmc_core_device),
+ INTEL_CPU_FAM6(SKYLAKE_DESKTOP, pmc_core_device),
+ INTEL_CPU_FAM6(KABYLAKE_MOBILE, pmc_core_device),
+ INTEL_CPU_FAM6(KABYLAKE_DESKTOP, pmc_core_device),
+ INTEL_CPU_FAM6(CANNONLAKE_MOBILE, pmc_core_device),
+ INTEL_CPU_FAM6(ICELAKE_MOBILE, pmc_core_device),
+ {}
+};
+MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_platform_ids);
+
+static int __init pmc_core_platform_init(void)
+{
+ /* Skip creating the platform device if ACPI already has a device */
+ if (acpi_dev_present("INT33A1", NULL, -1))
+ return -ENODEV;
+
+ if (!x86_match_cpu(intel_pmc_core_platform_ids))
+ return -ENODEV;
+
+ return platform_device_register(&pmc_core_device);
+}
+
+static void __exit pmc_core_platform_exit(void)
+{
+ platform_device_unregister(&pmc_core_device);
+}
+
+module_init(pmc_core_platform_init);
+module_exit(pmc_core_platform_exit);
--
2.21.0.392.gf8f6787159e-goog

2019-04-11 00:38:38

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v5 1/3] platform/x86: intel_pmc_core: Convert to a platform_driver

Convert the intel_pmc_core driver to a platform driver, and attach using
the ACPI enumeration method (via the ACPI device "INT33A1").

Signed-off-by: Rajat Jain <[email protected]>
---
v5: Remove the gerrit ID from commit log
v4: put back the x86_match_cpu() method.
v3: Don't instantiate the platform_device. Use ACPI enumeration.
v2: Rephrase the commit log. No code changes.

drivers/platform/x86/intel_pmc_core.c | 42 ++++++++++++++++++++++-----
1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 9908d233305e..8da886e17681 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -19,6 +19,7 @@
#include <linux/io.h>
#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/platform_device.h>
#include <linux/uaccess.h>

#include <asm/cpu_device_id.h>
@@ -854,13 +855,17 @@ static const struct dmi_system_id pmc_core_dmi_table[] = {
{}
};

-static int __init pmc_core_probe(void)
+static int pmc_core_probe(struct platform_device *pdev)
{
+ static bool device_initialized;
struct pmc_dev *pmcdev = &pmc;
const struct x86_cpu_id *cpu_id;
u64 slp_s0_addr;
int err;

+ if (device_initialized)
+ return -ENODEV;
+
cpu_id = x86_match_cpu(intel_pmc_core_ids);
if (!cpu_id)
return -ENODEV;
@@ -888,28 +893,49 @@ static int __init pmc_core_probe(void)
mutex_init(&pmcdev->lock);
pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();

+ dmi_check_system(pmc_core_dmi_table);
+ platform_set_drvdata(pdev, pmcdev);
+
err = pmc_core_dbgfs_register(pmcdev);
if (err < 0) {
- pr_warn(" debugfs register failed.\n");
+ dev_warn(&pdev->dev, "debugfs register failed.\n");
iounmap(pmcdev->regbase);
return err;
}

- dmi_check_system(pmc_core_dmi_table);
- pr_info(" initialized\n");
+ dev_info(&pdev->dev, " initialized\n");
+ device_initialized = true;
+
return 0;
}
-module_init(pmc_core_probe)

-static void __exit pmc_core_remove(void)
+static int pmc_core_remove(struct platform_device *pdev)
{
- struct pmc_dev *pmcdev = &pmc;
+ struct pmc_dev *pmcdev = platform_get_drvdata(pdev);

+ platform_set_drvdata(pdev, NULL);
pmc_core_dbgfs_unregister(pmcdev);
mutex_destroy(&pmcdev->lock);
iounmap(pmcdev->regbase);
+ return 0;
}
-module_exit(pmc_core_remove)
+
+static const struct acpi_device_id pmc_core_acpi_ids[] = {
+ {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, pmc_core_acpi_ids);
+
+static struct platform_driver pmc_core_driver = {
+ .driver = {
+ .name = "pmc_core",
+ .acpi_match_table = ACPI_PTR(pmc_core_acpi_ids),
+ },
+ .probe = pmc_core_probe,
+ .remove = pmc_core_remove,
+};
+
+module_platform_driver(pmc_core_driver);

MODULE_LICENSE("GPL v2");
MODULE_DESCRIPTION("Intel PMC Core Driver");
--
2.21.0.392.gf8f6787159e-goog

2019-04-11 00:38:47

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v5 3/3] platform/x86: intel_pmc_core: Instantiate pmc_core device on legacy platforms

Add code to instantiate the pmc_core platform device and thus attach to
the driver, if the ACPI device for the same ("INT33A1") is not present
in a system where it should be. This was discussed here:
https://www.mail-archive.com/[email protected]/msg1966991.html

Signed-off-by: Rajat Jain <[email protected]>
---
v5: Remove the gerrit id from commit log
v4: Rename file, remove dummy arg, 1 conditional per if statement,
simplify init / exit calls..
v3: (first version of *this* patch -to go with rest of v3 patchset)
v2: (does not exist)
v1: (does not exist)

drivers/platform/x86/Makefile | 2 +-
.../platform/x86/intel_pmc_core_plat_drv.c | 60 +++++++++++++++++++
2 files changed, 61 insertions(+), 1 deletion(-)
create mode 100644 drivers/platform/x86/intel_pmc_core_plat_drv.c

diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 86cb76677bc8..7041a88c34c7 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -89,7 +89,7 @@ obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU) += intel_bxtwc_tmu.o
obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \
intel_telemetry_pltdrv.o \
intel_telemetry_debugfs.o
-obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o
+obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o intel_pmc_core_plat_drv.o
obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o
obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o
diff --git a/drivers/platform/x86/intel_pmc_core_plat_drv.c b/drivers/platform/x86/intel_pmc_core_plat_drv.c
new file mode 100644
index 000000000000..7f5af5bd7f1f
--- /dev/null
+++ b/drivers/platform/x86/intel_pmc_core_plat_drv.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Intel PMC Core platform init
+ * Copyright (c) 2019, Google Inc.
+ * Author - Rajat Jain
+ *
+ * This code instantiates platform devices for intel_pmc_core driver, only
+ * on supported platforms that may not have the ACPI devices in the ACPI tables.
+ * No new platforms should be added here, because we expect that new platforms
+ * should all have the ACPI device, which is the preferred way of enumeration.
+ */
+
+#include <linux/acpi.h>
+#include <linux/platform_device.h>
+
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
+
+static struct platform_device pmc_core_device = {
+ .name = "pmc_core",
+};
+
+/*
+ * intel_pmc_core_platform_ids is the list of platforms where we want to
+ * instantiate the platform_device if not already instantiated. This is
+ * different than intel_pmc_core_ids in intel_pmc_core.c which is the
+ * list of platforms that the driver supports for pmc_core device. The
+ * other list may grow, but this list should not.
+ */
+static const struct x86_cpu_id intel_pmc_core_platform_ids[] = {
+ INTEL_CPU_FAM6(SKYLAKE_MOBILE, pmc_core_device),
+ INTEL_CPU_FAM6(SKYLAKE_DESKTOP, pmc_core_device),
+ INTEL_CPU_FAM6(KABYLAKE_MOBILE, pmc_core_device),
+ INTEL_CPU_FAM6(KABYLAKE_DESKTOP, pmc_core_device),
+ INTEL_CPU_FAM6(CANNONLAKE_MOBILE, pmc_core_device),
+ INTEL_CPU_FAM6(ICELAKE_MOBILE, pmc_core_device),
+ {}
+};
+MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_platform_ids);
+
+static int __init pmc_core_platform_init(void)
+{
+ /* Skip creating the platform device if ACPI already has a device */
+ if (acpi_dev_present("INT33A1", NULL, -1))
+ return -ENODEV;
+
+ if (!x86_match_cpu(intel_pmc_core_platform_ids))
+ return -ENODEV;
+
+ return platform_device_register(&pmc_core_device);
+}
+
+static void __exit pmc_core_platform_exit(void)
+{
+ platform_device_unregister(&pmc_core_device);
+}
+
+module_init(pmc_core_platform_init);
+module_exit(pmc_core_platform_exit);
--
2.21.0.392.gf8f6787159e-goog

2019-04-11 00:38:48

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v5 2/3] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

Add a module parameter which when enabled, will check on resume, if the
last S0ix attempt was successful. If not, the driver would warn and provide
helpful debug information (which gets latched during the failed suspend
attempt) to debug the S0ix failure.

This information is very useful to debug S0ix failures. Specially since
the latched debug information will be lost (over-written) if the system
attempts to go into runtime (or imminent) S0ix again after that failed
suspend attempt.

Signed-off-by: Rajat Jain <[email protected]>
---
v5: Remove the gerrit id from commit log
v4: Use 1 condition per if statement, rename some functions.
v3: No changes
v2: Use pm_suspend_via_firmware() to enable the check only for S0ix

drivers/platform/x86/intel_pmc_core.c | 93 +++++++++++++++++++++++++++
drivers/platform/x86/intel_pmc_core.h | 7 ++
2 files changed, 100 insertions(+)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 8da886e17681..b1f099de5cb3 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -20,6 +20,7 @@
#include <linux/module.h>
#include <linux/pci.h>
#include <linux/platform_device.h>
+#include <linux/suspend.h>
#include <linux/uaccess.h>

#include <asm/cpu_device_id.h>
@@ -920,6 +921,97 @@ static int pmc_core_remove(struct platform_device *pdev)
return 0;
}

+#ifdef CONFIG_PM_SLEEP
+
+static bool warn_on_s0ix_failures;
+module_param(warn_on_s0ix_failures, bool, 0644);
+MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix failures");
+
+static 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;
+
+ /* Check if the syspend will actually use S0ix */
+ if (pm_suspend_via_firmware())
+ return 0;
+
+ /* Save PC10 and S0ix residency for checking later */
+ if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter) &&
+ !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
+ pmcdev->check_counters = true;
+
+ return 0;
+}
+
+static inline bool pmc_core_is_pc10_failed(struct pmc_dev *pmcdev)
+{
+ u64 pc10_counter;
+
+ if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
+ pc10_counter == pmcdev->pc10_counter)
+ return true;
+
+ return false;
+}
+
+static inline bool pmc_core_is_s0ix_failed(struct pmc_dev *pmcdev)
+{
+ u64 s0ix_counter;
+
+ if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
+ s0ix_counter == pmcdev->s0ix_counter)
+ return true;
+
+ return false;
+}
+
+static int pmc_core_resume(struct device *dev)
+{
+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
+
+ if (!pmcdev->check_counters)
+ return 0;
+
+ if (pmc_core_is_pc10_failed(pmcdev)) {
+ dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
+ pmcdev->pc10_counter);
+ } else if (pmc_core_is_s0ix_failed(pmcdev)) {
+
+ const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
+ const struct pmc_bit_map *map;
+ int offset = pmcdev->map->slps0_dbg_offset;
+ u32 data;
+
+ dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
+ pmcdev->s0ix_counter);
+ while (*maps) {
+ map = *maps;
+ data = pmc_core_reg_read(pmcdev, offset);
+ offset += 4;
+ while (map->name) {
+ dev_warn(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
+ map->name,
+ data & map->bit_mask ? "Yes" : "No");
+ ++map;
+ }
+ ++maps;
+ }
+ }
+ return 0;
+}
+
+#endif
+
+static const struct dev_pm_ops pmc_core_pm_ops = {
+ SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend, pmc_core_resume)
+};
+
static const struct acpi_device_id pmc_core_acpi_ids[] = {
{"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
{ }
@@ -930,6 +1022,7 @@ static struct platform_driver pmc_core_driver = {
.driver = {
.name = "pmc_core",
.acpi_match_table = ACPI_PTR(pmc_core_acpi_ids),
+ .pm = &pmc_core_pm_ops,
},
.probe = pmc_core_probe,
.remove = pmc_core_remove,
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 88d9c0653a5f..fdee5772e532 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -241,6 +241,9 @@ 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)
*
* pmc_dev contains info about power management controller device.
*/
@@ -253,6 +256,10 @@ struct pmc_dev {
#endif /* CONFIG_DEBUG_FS */
int pmc_xram_read_bit;
struct mutex lock; /* generic mutex lock for PMC Core */
+
+ bool check_counters; /* Check for counter increments on resume */
+ u64 pc10_counter;
+ u64 s0ix_counter;
};

#endif /* PMC_CORE_H */
--
2.21.0.392.gf8f6787159e-goog

2019-04-11 13:42:54

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v5 2/3] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

On Thu, Apr 11, 2019 at 3:38 AM Rajat Jain <[email protected]> wrote:
>
> Add a module parameter which when enabled, will check on resume, if the
> last S0ix attempt was successful. If not, the driver would warn and provide
> helpful debug information (which gets latched during the failed suspend
> attempt) to debug the S0ix failure.
>
> This information is very useful to debug S0ix failures. Specially since
> the latched debug information will be lost (over-written) if the system
> attempts to go into runtime (or imminent) S0ix again after that failed
> suspend attempt.

> +static 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;
> +
> + /* Check if the syspend will actually use S0ix */
> + if (pm_suspend_via_firmware())
> + return 0;
> +
> + /* Save PC10 and S0ix residency for checking later */

> + if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter) &&
> + !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))

Split it.

> + pmcdev->check_counters = true;
> +
> + return 0;
> +}
> +
> +static inline bool pmc_core_is_pc10_failed(struct pmc_dev *pmcdev)
> +{
> + u64 pc10_counter;
> +
> + if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
> + pc10_counter == pmcdev->pc10_counter)
> + return true;

Split this as well.

> +
> + return false;
> +}
> +
> +static inline bool pmc_core_is_s0ix_failed(struct pmc_dev *pmcdev)
> +{
> + u64 s0ix_counter;
> +
> + if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
> + s0ix_counter == pmcdev->s0ix_counter)
> + return true;

And this.

> +
> + return false;
> +}
> +
> +static int pmc_core_resume(struct device *dev)
> +{
> + struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> +
> + if (!pmcdev->check_counters)
> + return 0;
> +
> + if (pmc_core_is_pc10_failed(pmcdev)) {
> + dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
> + pmcdev->pc10_counter);
> + } else if (pmc_core_is_s0ix_failed(pmcdev)) {

> +

Redundant.

> + const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
> + const struct pmc_bit_map *map;
> + int offset = pmcdev->map->slps0_dbg_offset;
> + u32 data;
> +
> + dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
> + pmcdev->s0ix_counter);
> + while (*maps) {
> + map = *maps;
> + data = pmc_core_reg_read(pmcdev, offset);
> + offset += 4;
> + while (map->name) {
> + dev_warn(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
> + map->name,
> + data & map->bit_mask ? "Yes" : "No");

> + ++map;

map++;

> + }
> + ++maps;

maps++;

> + }

This is quite noisy. You need to print only what is important. I don't
think polluting dmesg with piles of these kind of messages is a good
idea.
Also, it is more likely should be done on debug level (except may be
one or two messages with really important information).

> + }
> + return 0;
> +}
> +
> +#endif

--
With Best Regards,
Andy Shevchenko

2019-04-11 13:47:31

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v5 3/3] platform/x86: intel_pmc_core: Instantiate pmc_core device on legacy platforms

On Thu, Apr 11, 2019 at 3:38 AM Rajat Jain <[email protected]> wrote:
>
> Add code to instantiate the pmc_core platform device and thus attach to
> the driver, if the ACPI device for the same ("INT33A1") is not present
> in a system where it should be. This was discussed here:
> https://www.mail-archive.com/[email protected]/msg1966991.html
>
> Signed-off-by: Rajat Jain <[email protected]>
> ---
> v5: Remove the gerrit id from commit log
> v4: Rename file, remove dummy arg, 1 conditional per if statement,
> simplify init / exit calls..
> v3: (first version of *this* patch -to go with rest of v3 patchset)
> v2: (does not exist)
> v1: (does not exist)
>
> drivers/platform/x86/Makefile | 2 +-
> .../platform/x86/intel_pmc_core_plat_drv.c | 60 +++++++++++++++++++
> 2 files changed, 61 insertions(+), 1 deletion(-)
> create mode 100644 drivers/platform/x86/intel_pmc_core_plat_drv.c
>
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 86cb76677bc8..7041a88c34c7 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -89,7 +89,7 @@ obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU) += intel_bxtwc_tmu.o
> obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \
> intel_telemetry_pltdrv.o \
> intel_telemetry_debugfs.o
> -obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o
> +obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o intel_pmc_core_plat_drv.o
> obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
> obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o
> obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o
> diff --git a/drivers/platform/x86/intel_pmc_core_plat_drv.c b/drivers/platform/x86/intel_pmc_core_plat_drv.c
> new file mode 100644
> index 000000000000..7f5af5bd7f1f
> --- /dev/null
> +++ b/drivers/platform/x86/intel_pmc_core_plat_drv.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Intel PMC Core platform init
> + * Copyright (c) 2019, Google Inc.
> + * Author - Rajat Jain
> + *
> + * This code instantiates platform devices for intel_pmc_core driver, only
> + * on supported platforms that may not have the ACPI devices in the ACPI tables.
> + * No new platforms should be added here, because we expect that new platforms
> + * should all have the ACPI device, which is the preferred way of enumeration.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> +
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +
> +static struct platform_device pmc_core_device = {
> + .name = "pmc_core",

This name is too generic. Better to use the current module name, i.e.
"intel_pmc_core".

However, see my comment to the first patch in the series.

> +};
> +
> +/*
> + * intel_pmc_core_platform_ids is the list of platforms where we want to
> + * instantiate the platform_device if not already instantiated. This is
> + * different than intel_pmc_core_ids in intel_pmc_core.c which is the
> + * list of platforms that the driver supports for pmc_core device. The
> + * other list may grow, but this list should not.
> + */
> +static const struct x86_cpu_id intel_pmc_core_platform_ids[] = {
> + INTEL_CPU_FAM6(SKYLAKE_MOBILE, pmc_core_device),
> + INTEL_CPU_FAM6(SKYLAKE_DESKTOP, pmc_core_device),
> + INTEL_CPU_FAM6(KABYLAKE_MOBILE, pmc_core_device),
> + INTEL_CPU_FAM6(KABYLAKE_DESKTOP, pmc_core_device),
> + INTEL_CPU_FAM6(CANNONLAKE_MOBILE, pmc_core_device),
> + INTEL_CPU_FAM6(ICELAKE_MOBILE, pmc_core_device),
> + {}
> +};
> +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_platform_ids);
> +
> +static int __init pmc_core_platform_init(void)
> +{
> + /* Skip creating the platform device if ACPI already has a device */
> + if (acpi_dev_present("INT33A1", NULL, -1))
> + return -ENODEV;
> +
> + if (!x86_match_cpu(intel_pmc_core_platform_ids))
> + return -ENODEV;
> +
> + return platform_device_register(&pmc_core_device);
> +}
> +
> +static void __exit pmc_core_platform_exit(void)
> +{
> + platform_device_unregister(&pmc_core_device);
> +}
> +
> +module_init(pmc_core_platform_init);
> +module_exit(pmc_core_platform_exit);
> --
> 2.21.0.392.gf8f6787159e-goog
>


--
With Best Regards,
Andy Shevchenko

2019-04-11 13:47:33

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] platform/x86: intel_pmc_core: Convert to a platform_driver

On Thu, Apr 11, 2019 at 3:37 AM Rajat Jain <[email protected]> wrote:
>
> Convert the intel_pmc_core driver to a platform driver, and attach using
> the ACPI enumeration method (via the ACPI device "INT33A1").

> cpu_id = x86_match_cpu(intel_pmc_core_ids);
> if (!cpu_id)
> return -ENODEV;
> @@ -888,28 +893,49 @@ static int __init pmc_core_probe(void)
> mutex_init(&pmcdev->lock);
> pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
>
> + dmi_check_system(pmc_core_dmi_table);
> + platform_set_drvdata(pdev, pmcdev);
> +
> err = pmc_core_dbgfs_register(pmcdev);
> if (err < 0) {
> - pr_warn(" debugfs register failed.\n");
> + dev_warn(&pdev->dev, "debugfs register failed.\n");
> iounmap(pmcdev->regbase);
> return err;
> }
>
> - dmi_check_system(pmc_core_dmi_table);
> - pr_info(" initialized\n");

> + dev_info(&pdev->dev, " initialized\n");
> + device_initialized = true;

First you do something, then print it's done, and not other way around.

> +
> return 0;
> }
> -module_init(pmc_core_probe)
>
> -static void __exit pmc_core_remove(void)
> +static int pmc_core_remove(struct platform_device *pdev)
> {
> - struct pmc_dev *pmcdev = &pmc;
> + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
>
> + platform_set_drvdata(pdev, NULL);
> pmc_core_dbgfs_unregister(pmcdev);
> mutex_destroy(&pmcdev->lock);
> iounmap(pmcdev->regbase);
> + return 0;
> }
> -module_exit(pmc_core_remove)
> +
> +static const struct acpi_device_id pmc_core_acpi_ids[] = {
> + {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
> + { }
> +};
> +MODULE_DEVICE_TABLE(acpi, pmc_core_acpi_ids);
> +
> +static struct platform_driver pmc_core_driver = {
> + .driver = {
> + .name = "pmc_core",
> + .acpi_match_table = ACPI_PTR(pmc_core_acpi_ids),
> + },
> + .probe = pmc_core_probe,
> + .remove = pmc_core_remove,
> +};
> +
> +module_platform_driver(pmc_core_driver);

So, this patch has a bisectability issue. After it AFAIU some
platforms will not be able to enumerate the device.

To avoid such you have to reconsider logic behind this conversion, i.e.
1. Split the driver to core part and current initialization mechanism
2. Add platform driver as a separate module.

--
With Best Regards,
Andy Shevchenko

2019-04-17 23:04:05

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v6 1/3] platform/x86: intel_pmc_core: Convert to a platform_driver

Convert the intel_pmc_core driver to a platform driver. There is no
functional change to the driver, or to the way the devices are
instantiated.

Signed-off-by: Rajat Jain <[email protected]>
---
v6: Let the way devices are instantiated exactly the same as before this
patch (so no functional change other than converting to a platform
device). Use "intel_pmc_core" platform device (no ACPI
instantiation)
v5: Remove the gerrit ID from commit log
v4: put back the x86_match_cpu() method.
v3: Don't instantiate the platform_device. Use ACPI enumeration.
v2: Rephrase the commit log. No code changes.

drivers/platform/x86/intel_pmc_core.c | 65 +++++++++++++++++++++++----
1 file changed, 57 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 9908d233305e..153e79cc4d5b 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -19,6 +19,7 @@
#include <linux/io.h>
#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/platform_device.h>
#include <linux/uaccess.h>

#include <asm/cpu_device_id.h>
@@ -854,13 +855,17 @@ static const struct dmi_system_id pmc_core_dmi_table[] = {
{}
};

-static int __init pmc_core_probe(void)
+static int pmc_core_probe(struct platform_device *pdev)
{
+ static bool device_initialized;
struct pmc_dev *pmcdev = &pmc;
const struct x86_cpu_id *cpu_id;
u64 slp_s0_addr;
int err;

+ if (device_initialized)
+ return -ENODEV;
+
cpu_id = x86_match_cpu(intel_pmc_core_ids);
if (!cpu_id)
return -ENODEV;
@@ -886,30 +891,74 @@ static int __init pmc_core_probe(void)
return -ENOMEM;

mutex_init(&pmcdev->lock);
+ platform_set_drvdata(pdev, pmcdev);
pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
+ dmi_check_system(pmc_core_dmi_table);

err = pmc_core_dbgfs_register(pmcdev);
if (err < 0) {
- pr_warn(" debugfs register failed.\n");
+ dev_warn(&pdev->dev, "debugfs register failed.\n");
iounmap(pmcdev->regbase);
return err;
}

- dmi_check_system(pmc_core_dmi_table);
- pr_info(" initialized\n");
+ device_initialized = true;
+ dev_info(&pdev->dev, " initialized\n");
+
return 0;
}
-module_init(pmc_core_probe)

-static void __exit pmc_core_remove(void)
+static int pmc_core_remove(struct platform_device *pdev)
{
- struct pmc_dev *pmcdev = &pmc;
+ struct pmc_dev *pmcdev = platform_get_drvdata(pdev);

pmc_core_dbgfs_unregister(pmcdev);
+ platform_set_drvdata(pdev, NULL);
mutex_destroy(&pmcdev->lock);
iounmap(pmcdev->regbase);
+ return 0;
+}
+
+static struct platform_driver pmc_core_driver = {
+ .driver = {
+ .name = "intel_pmc_core",
+ },
+ .probe = pmc_core_probe,
+ .remove = pmc_core_remove,
+};
+
+static struct platform_device pmc_core_device = {
+ .name = "intel_pmc_core",
+};
+
+static int __init pmc_core_init(void)
+{
+ int ret;
+
+ if (!x86_match_cpu(intel_pmc_core_ids))
+ return -ENODEV;
+
+ ret = platform_driver_register(&pmc_core_driver);
+ if (ret)
+ return ret;
+
+ ret = platform_device_register(&pmc_core_device);
+ if (ret) {
+ platform_driver_unregister(&pmc_core_driver);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void __exit pmc_core_exit(void)
+{
+ platform_device_unregister(&pmc_core_device);
+ platform_driver_unregister(&pmc_core_driver);
}
-module_exit(pmc_core_remove)
+
+module_init(pmc_core_init)
+module_exit(pmc_core_exit)

MODULE_LICENSE("GPL v2");
MODULE_DESCRIPTION("Intel PMC Core Driver");
--
2.21.0.392.gf8f6787159e-goog

2019-04-17 23:04:06

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v6 2/3] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

Add a module parameter which when enabled, will check on resume, if the
last S0ix attempt was successful. If not, the driver would warn and provide
helpful debug information (which gets latched during the failed suspend
attempt) to debug the S0ix failure.

This information is very useful to debug S0ix failures. Specially since
the latched debug information will be lost (over-written) if the system
attempts to go into runtime (or imminent) S0ix again after that failed
suspend attempt.

Signed-off-by: Rajat Jain <[email protected]>
---
v6: Some code restructuring, single conditional per if stmt.
Change from dev_warn to dev_dbg.
v5: Remove the gerrit id from commit log
v4: Use 1 condition per if statement, rename some functions.
v3: No changes
v2: Use pm_suspend_via_firmware() to enable the check only for S0ix

drivers/platform/x86/intel_pmc_core.c | 105 ++++++++++++++++++++++++++
drivers/platform/x86/intel_pmc_core.h | 7 ++
2 files changed, 112 insertions(+)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 153e79cc4d5b..1d902230ba61 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -20,6 +20,7 @@
#include <linux/module.h>
#include <linux/pci.h>
#include <linux/platform_device.h>
+#include <linux/suspend.h>
#include <linux/uaccess.h>

#include <asm/cpu_device_id.h>
@@ -919,9 +920,113 @@ static int pmc_core_remove(struct platform_device *pdev)
return 0;
}

+#ifdef CONFIG_PM_SLEEP
+
+static bool warn_on_s0ix_failures;
+module_param(warn_on_s0ix_failures, bool, 0644);
+MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix failures");
+
+static 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;
+
+ /* Check if the syspend will actually use S0ix */
+ if (pm_suspend_via_firmware())
+ return 0;
+
+ /* Save PC10 residency for checking later */
+ if (rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter))
+ return -EIO;
+
+ /* Save S0ix residency for checking later */
+ if (pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
+ return -EIO;
+
+ pmcdev->check_counters = true;
+ return 0;
+}
+
+static inline bool pmc_core_is_pc10_failed(struct pmc_dev *pmcdev)
+{
+ u64 pc10_counter;
+
+ if (rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter))
+ return false;
+
+ if (pc10_counter == pmcdev->pc10_counter)
+ return true;
+
+ return false;
+}
+
+static inline bool pmc_core_is_s0ix_failed(struct pmc_dev *pmcdev)
+{
+ u64 s0ix_counter;
+
+ if (pmc_core_dev_state_get(pmcdev, &s0ix_counter))
+ return false;
+
+ if (s0ix_counter == pmcdev->s0ix_counter)
+ return true;
+
+ return false;
+}
+
+static int pmc_core_resume(struct device *dev)
+{
+ struct pmc_dev *pmcdev = dev_get_drvdata(dev);
+ const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
+ int offset = pmcdev->map->slps0_dbg_offset;
+ const struct pmc_bit_map *map;
+ u32 data;
+
+ if (!pmcdev->check_counters)
+ return 0;
+
+ if (!pmc_core_is_s0ix_failed(pmcdev))
+ return 0;
+
+ if (pmc_core_is_pc10_failed(pmcdev)) {
+ /* S0ix failed because of PC10 entry failure */
+ dev_info(dev, "CPU did not enter PC10!!! (PC10 cnt=0x%llx)\n",
+ pmcdev->pc10_counter);
+ return 0;
+ }
+
+ /* The real interesting case - S0ix failed - lets ask PMC why. */
+ dev_warn(dev, "CPU did not enter SLP_S0!!! (S0ix cnt=%llu)\n",
+ pmcdev->s0ix_counter);
+ while (*maps) {
+ map = *maps;
+ data = pmc_core_reg_read(pmcdev, offset);
+ offset += 4;
+ while (map->name) {
+ dev_dbg(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
+ map->name,
+ data & map->bit_mask ? "Yes" : "No");
+ map++;
+ }
+ maps++;
+ }
+ return 0;
+}
+
+#endif
+
+static const struct dev_pm_ops pmc_core_pm_ops = {
+ SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend, pmc_core_resume)
+};
+
static struct platform_driver pmc_core_driver = {
.driver = {
.name = "intel_pmc_core",
+ .pm = &pmc_core_pm_ops,
},
.probe = pmc_core_probe,
.remove = pmc_core_remove,
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 88d9c0653a5f..fdee5772e532 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -241,6 +241,9 @@ 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)
*
* pmc_dev contains info about power management controller device.
*/
@@ -253,6 +256,10 @@ struct pmc_dev {
#endif /* CONFIG_DEBUG_FS */
int pmc_xram_read_bit;
struct mutex lock; /* generic mutex lock for PMC Core */
+
+ bool check_counters; /* Check for counter increments on resume */
+ u64 pc10_counter;
+ u64 s0ix_counter;
};

#endif /* PMC_CORE_H */
--
2.21.0.392.gf8f6787159e-goog

2019-04-17 23:05:07

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v6 3/3] platform/x86: intel_pmc_core: Attach using APCI HID "INT33A1"

Most modern platforms already have the ACPI device "INT33A1" that could
be used to attach to the driver. Switch the driver to using that and
thus make the intel_pmc_core.c a pure platform_driver.

Some of the legacy platforms though, may still not have this ACPI device
in their ACPI tables. Thus for such platforms, move the code to manually
instantiate a platform_device into a new file of its own. This would
instantiate the intel_pmc_core platform device and thus attach to
the driver, if the ACPI device for the same ("INT33A1") is not present
in a system where it should be. This was discussed here:
https://www.mail-archive.com/[email protected]/msg1966991.html

Signed-off-by: Rajat Jain <[email protected]>
---
v6: Squash some of the changes of first patch into this patch to work
around the bisectability issue.
v5: Remove the gerrit id from commit log
v4: Rename file, remove dummy arg, 1 conditional per if statement,
simplify init / exit calls..
v3: (first version of *this* patch -to go with rest of v3 patchset)
v2: (does not exist)
v1: (does not exist)

drivers/platform/x86/Makefile | 2 +-
drivers/platform/x86/intel_pmc_core.c | 40 +++----------
.../platform/x86/intel_pmc_core_plat_drv.c | 60 +++++++++++++++++++
3 files changed, 69 insertions(+), 33 deletions(-)
create mode 100644 drivers/platform/x86/intel_pmc_core_plat_drv.c

diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 86cb76677bc8..7041a88c34c7 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -89,7 +89,7 @@ obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU) += intel_bxtwc_tmu.o
obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \
intel_telemetry_pltdrv.o \
intel_telemetry_debugfs.o
-obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o
+obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o intel_pmc_core_plat_drv.o
obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o
obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o
diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 1d902230ba61..f20d08ad39ea 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -1023,47 +1023,23 @@ static const struct dev_pm_ops pmc_core_pm_ops = {
SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend, pmc_core_resume)
};

+static const struct acpi_device_id pmc_core_acpi_ids[] = {
+ {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, pmc_core_acpi_ids);
+
static struct platform_driver pmc_core_driver = {
.driver = {
.name = "intel_pmc_core",
+ .acpi_match_table = ACPI_PTR(pmc_core_acpi_ids),
.pm = &pmc_core_pm_ops,
},
.probe = pmc_core_probe,
.remove = pmc_core_remove,
};

-static struct platform_device pmc_core_device = {
- .name = "intel_pmc_core",
-};
-
-static int __init pmc_core_init(void)
-{
- int ret;
-
- if (!x86_match_cpu(intel_pmc_core_ids))
- return -ENODEV;
-
- ret = platform_driver_register(&pmc_core_driver);
- if (ret)
- return ret;
-
- ret = platform_device_register(&pmc_core_device);
- if (ret) {
- platform_driver_unregister(&pmc_core_driver);
- return ret;
- }
-
- return 0;
-}
-
-static void __exit pmc_core_exit(void)
-{
- platform_device_unregister(&pmc_core_device);
- platform_driver_unregister(&pmc_core_driver);
-}
-
-module_init(pmc_core_init)
-module_exit(pmc_core_exit)
+module_platform_driver(pmc_core_driver);

MODULE_LICENSE("GPL v2");
MODULE_DESCRIPTION("Intel PMC Core Driver");
diff --git a/drivers/platform/x86/intel_pmc_core_plat_drv.c b/drivers/platform/x86/intel_pmc_core_plat_drv.c
new file mode 100644
index 000000000000..20be3eaeb722
--- /dev/null
+++ b/drivers/platform/x86/intel_pmc_core_plat_drv.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Intel PMC Core platform init
+ * Copyright (c) 2019, Google Inc.
+ * Author - Rajat Jain
+ *
+ * This code instantiates platform devices for intel_pmc_core driver, only
+ * on supported platforms that may not have the ACPI devices in the ACPI tables.
+ * No new platforms should be added here, because we expect that new platforms
+ * should all have the ACPI device, which is the preferred way of enumeration.
+ */
+
+#include <linux/acpi.h>
+#include <linux/platform_device.h>
+
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
+
+static struct platform_device pmc_core_device = {
+ .name = "intel_pmc_core",
+};
+
+/*
+ * intel_pmc_core_platform_ids is the list of platforms where we want to
+ * instantiate the platform_device if not already instantiated. This is
+ * different than intel_pmc_core_ids in intel_pmc_core.c which is the
+ * list of platforms that the driver supports for pmc_core device. The
+ * other list may grow, but this list should not.
+ */
+static const struct x86_cpu_id intel_pmc_core_platform_ids[] = {
+ INTEL_CPU_FAM6(SKYLAKE_MOBILE, pmc_core_device),
+ INTEL_CPU_FAM6(SKYLAKE_DESKTOP, pmc_core_device),
+ INTEL_CPU_FAM6(KABYLAKE_MOBILE, pmc_core_device),
+ INTEL_CPU_FAM6(KABYLAKE_DESKTOP, pmc_core_device),
+ INTEL_CPU_FAM6(CANNONLAKE_MOBILE, pmc_core_device),
+ INTEL_CPU_FAM6(ICELAKE_MOBILE, pmc_core_device),
+ {}
+};
+MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_platform_ids);
+
+static int __init pmc_core_platform_init(void)
+{
+ /* Skip creating the platform device if ACPI already has a device */
+ if (acpi_dev_present("INT33A1", NULL, -1))
+ return -ENODEV;
+
+ if (!x86_match_cpu(intel_pmc_core_platform_ids))
+ return -ENODEV;
+
+ return platform_device_register(&pmc_core_device);
+}
+
+static void __exit pmc_core_platform_exit(void)
+{
+ platform_device_unregister(&pmc_core_device);
+}
+
+module_init(pmc_core_platform_init);
+module_exit(pmc_core_platform_exit);
--
2.21.0.392.gf8f6787159e-goog

2019-04-17 23:05:53

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] platform/x86: intel_pmc_core: Convert to a platform_driver

On Thu, Apr 11, 2019 at 6:44 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Apr 11, 2019 at 3:37 AM Rajat Jain <[email protected]> wrote:
> >
> > Convert the intel_pmc_core driver to a platform driver, and attach using
> > the ACPI enumeration method (via the ACPI device "INT33A1").
>
> > cpu_id = x86_match_cpu(intel_pmc_core_ids);
> > if (!cpu_id)
> > return -ENODEV;
> > @@ -888,28 +893,49 @@ static int __init pmc_core_probe(void)
> > mutex_init(&pmcdev->lock);
> > pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
> >
> > + dmi_check_system(pmc_core_dmi_table);
> > + platform_set_drvdata(pdev, pmcdev);
> > +
> > err = pmc_core_dbgfs_register(pmcdev);
> > if (err < 0) {
> > - pr_warn(" debugfs register failed.\n");
> > + dev_warn(&pdev->dev, "debugfs register failed.\n");
> > iounmap(pmcdev->regbase);
> > return err;
> > }
> >
> > - dmi_check_system(pmc_core_dmi_table);
> > - pr_info(" initialized\n");
>
> > + dev_info(&pdev->dev, " initialized\n");
> > + device_initialized = true;
>
> First you do something, then print it's done, and not other way around.

done

>
> > +
> > return 0;
> > }
> > -module_init(pmc_core_probe)
> >
> > -static void __exit pmc_core_remove(void)
> > +static int pmc_core_remove(struct platform_device *pdev)
> > {
> > - struct pmc_dev *pmcdev = &pmc;
> > + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> >
> > + platform_set_drvdata(pdev, NULL);
> > pmc_core_dbgfs_unregister(pmcdev);
> > mutex_destroy(&pmcdev->lock);
> > iounmap(pmcdev->regbase);
> > + return 0;
> > }
> > -module_exit(pmc_core_remove)
> > +
> > +static const struct acpi_device_id pmc_core_acpi_ids[] = {
> > + {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(acpi, pmc_core_acpi_ids);
> > +
> > +static struct platform_driver pmc_core_driver = {
> > + .driver = {
> > + .name = "pmc_core",
> > + .acpi_match_table = ACPI_PTR(pmc_core_acpi_ids),
> > + },
> > + .probe = pmc_core_probe,
> > + .remove = pmc_core_remove,
> > +};
> > +
> > +module_platform_driver(pmc_core_driver);
>
> So, this patch has a bisectability issue. After it AFAIU some
> platforms will not be able to enumerate the device.
>
> To avoid such you have to reconsider logic behind this conversion, i.e.
> 1. Split the driver to core part and current initialization mechanism
> 2. Add platform driver as a separate module.

done.


>
> --
> With Best Regards,
> Andy Shevchenko

2019-04-17 23:07:24

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH v5 2/3] platform/x86: intel_pmc_core: Allow to dump debug registers on S0ix failure

On Thu, Apr 11, 2019 at 6:40 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Apr 11, 2019 at 3:38 AM Rajat Jain <[email protected]> wrote:
> >
> > Add a module parameter which when enabled, will check on resume, if the
> > last S0ix attempt was successful. If not, the driver would warn and provide
> > helpful debug information (which gets latched during the failed suspend
> > attempt) to debug the S0ix failure.
> >
> > This information is very useful to debug S0ix failures. Specially since
> > the latched debug information will be lost (over-written) if the system
> > attempts to go into runtime (or imminent) S0ix again after that failed
> > suspend attempt.
>
> > +static 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;
> > +
> > + /* Check if the syspend will actually use S0ix */
> > + if (pm_suspend_via_firmware())
> > + return 0;
> > +
> > + /* Save PC10 and S0ix residency for checking later */
>
> > + if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter) &&
> > + !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
>
> Split it.

done

>
> > + pmcdev->check_counters = true;
> > +
> > + return 0;
> > +}
> > +
> > +static inline bool pmc_core_is_pc10_failed(struct pmc_dev *pmcdev)
> > +{
> > + u64 pc10_counter;
> > +
> > + if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) &&
> > + pc10_counter == pmcdev->pc10_counter)
> > + return true;
>
> Split this as well.

done
>
> > +
> > + return false;
> > +}
> > +
> > +static inline bool pmc_core_is_s0ix_failed(struct pmc_dev *pmcdev)
> > +{
> > + u64 s0ix_counter;
> > +
> > + if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) &&
> > + s0ix_counter == pmcdev->s0ix_counter)
> > + return true;
>
> And this.

done
>
> > +
> > + return false;
> > +}
> > +
> > +static int pmc_core_resume(struct device *dev)
> > +{
> > + struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> > +
> > + if (!pmcdev->check_counters)
> > + return 0;
> > +
> > + if (pmc_core_is_pc10_failed(pmcdev)) {
> > + dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
> > + pmcdev->pc10_counter);
> > + } else if (pmc_core_is_s0ix_failed(pmcdev)) {
>
> > +
>
> Redundant.

I did not quite understand the comment, but I have restructured this
and I think this concerned will be addressed.

>
> > + const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
> > + const struct pmc_bit_map *map;
> > + int offset = pmcdev->map->slps0_dbg_offset;
> > + u32 data;
> > +
> > + dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n",
> > + pmcdev->s0ix_counter);
> > + while (*maps) {
> > + map = *maps;
> > + data = pmc_core_reg_read(pmcdev, offset);
> > + offset += 4;
> > + while (map->name) {
> > + dev_warn(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
> > + map->name,
> > + data & map->bit_mask ? "Yes" : "No");
>
> > + ++map;
>
> map++;

done
>
> > + }
> > + ++maps;
>
> maps++;

done
>
> > + }
>
> This is quite noisy. You need to print only what is important. I don't
> think polluting dmesg with piles of these kind of messages is a good
> idea.
> Also, it is more likely should be done on debug level (except may be
> one or two messages with really important information).

Changed it to dev_dbg in my latest patch. I do not know if a subset of
this information will be helpful to Intel to debug S0ix failures. This
is something I'd like to defer to Rajneesh. I'd be happy to cut it
short if it can still get the info Intel needs to debug S0ix failures.




>
> > + }
> > + return 0;
> > +}
> > +
> > +#endif
>
> --
> With Best Regards,
> Andy Shevchenko

2019-04-17 23:09:00

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH v5 3/3] platform/x86: intel_pmc_core: Instantiate pmc_core device on legacy platforms

On Thu, Apr 11, 2019 at 6:46 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Apr 11, 2019 at 3:38 AM Rajat Jain <[email protected]> wrote:
> >
> > Add code to instantiate the pmc_core platform device and thus attach to
> > the driver, if the ACPI device for the same ("INT33A1") is not present
> > in a system where it should be. This was discussed here:
> > https://www.mail-archive.com/[email protected]/msg1966991.html
> >
> > Signed-off-by: Rajat Jain <[email protected]>
> > ---
> > v5: Remove the gerrit id from commit log
> > v4: Rename file, remove dummy arg, 1 conditional per if statement,
> > simplify init / exit calls..
> > v3: (first version of *this* patch -to go with rest of v3 patchset)
> > v2: (does not exist)
> > v1: (does not exist)
> >
> > drivers/platform/x86/Makefile | 2 +-
> > .../platform/x86/intel_pmc_core_plat_drv.c | 60 +++++++++++++++++++
> > 2 files changed, 61 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/platform/x86/intel_pmc_core_plat_drv.c
> >
> > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> > index 86cb76677bc8..7041a88c34c7 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -89,7 +89,7 @@ obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU) += intel_bxtwc_tmu.o
> > obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \
> > intel_telemetry_pltdrv.o \
> > intel_telemetry_debugfs.o
> > -obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o
> > +obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o intel_pmc_core_plat_drv.o
> > obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
> > obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o
> > obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o
> > diff --git a/drivers/platform/x86/intel_pmc_core_plat_drv.c b/drivers/platform/x86/intel_pmc_core_plat_drv.c
> > new file mode 100644
> > index 000000000000..7f5af5bd7f1f
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel_pmc_core_plat_drv.c
> > @@ -0,0 +1,60 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Intel PMC Core platform init
> > + * Copyright (c) 2019, Google Inc.
> > + * Author - Rajat Jain
> > + *
> > + * This code instantiates platform devices for intel_pmc_core driver, only
> > + * on supported platforms that may not have the ACPI devices in the ACPI tables.
> > + * No new platforms should be added here, because we expect that new platforms
> > + * should all have the ACPI device, which is the preferred way of enumeration.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <asm/cpu_device_id.h>
> > +#include <asm/intel-family.h>
> > +
> > +static struct platform_device pmc_core_device = {
> > + .name = "pmc_core",
>
> This name is too generic. Better to use the current module name, i.e.
> "intel_pmc_core".

done


>
> However, see my comment to the first patch in the series.
>
> > +};
> > +
> > +/*
> > + * intel_pmc_core_platform_ids is the list of platforms where we want to
> > + * instantiate the platform_device if not already instantiated. This is
> > + * different than intel_pmc_core_ids in intel_pmc_core.c which is the
> > + * list of platforms that the driver supports for pmc_core device. The
> > + * other list may grow, but this list should not.
> > + */
> > +static const struct x86_cpu_id intel_pmc_core_platform_ids[] = {
> > + INTEL_CPU_FAM6(SKYLAKE_MOBILE, pmc_core_device),
> > + INTEL_CPU_FAM6(SKYLAKE_DESKTOP, pmc_core_device),
> > + INTEL_CPU_FAM6(KABYLAKE_MOBILE, pmc_core_device),
> > + INTEL_CPU_FAM6(KABYLAKE_DESKTOP, pmc_core_device),
> > + INTEL_CPU_FAM6(CANNONLAKE_MOBILE, pmc_core_device),
> > + INTEL_CPU_FAM6(ICELAKE_MOBILE, pmc_core_device),
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_platform_ids);
> > +
> > +static int __init pmc_core_platform_init(void)
> > +{
> > + /* Skip creating the platform device if ACPI already has a device */
> > + if (acpi_dev_present("INT33A1", NULL, -1))
> > + return -ENODEV;
> > +
> > + if (!x86_match_cpu(intel_pmc_core_platform_ids))
> > + return -ENODEV;
> > +
> > + return platform_device_register(&pmc_core_device);
> > +}
> > +
> > +static void __exit pmc_core_platform_exit(void)
> > +{
> > + platform_device_unregister(&pmc_core_device);
> > +}
> > +
> > +module_init(pmc_core_platform_init);
> > +module_exit(pmc_core_platform_exit);
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko

2019-05-06 09:41:26

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v6 3/3] platform/x86: intel_pmc_core: Attach using APCI HID "INT33A1"

On Thu, Apr 18, 2019 at 2:02 AM Rajat Jain <[email protected]> wrote:
>
> Most modern platforms already have the ACPI device "INT33A1" that could
> be used to attach to the driver. Switch the driver to using that and
> thus make the intel_pmc_core.c a pure platform_driver.
>
> Some of the legacy platforms though, may still not have this ACPI device
> in their ACPI tables. Thus for such platforms, move the code to manually
> instantiate a platform_device into a new file of its own. This would
> instantiate the intel_pmc_core platform device and thus attach to
> the driver, if the ACPI device for the same ("INT33A1") is not present
> in a system where it should be. This was discussed here:
> https://www.mail-archive.com/[email protected]/msg1966991.html
>

Pushed to my review and testing queue, thanks!

> Signed-off-by: Rajat Jain <[email protected]>
> ---
> v6: Squash some of the changes of first patch into this patch to work
> around the bisectability issue.
> v5: Remove the gerrit id from commit log
> v4: Rename file, remove dummy arg, 1 conditional per if statement,
> simplify init / exit calls..
> v3: (first version of *this* patch -to go with rest of v3 patchset)
> v2: (does not exist)
> v1: (does not exist)
>
> drivers/platform/x86/Makefile | 2 +-
> drivers/platform/x86/intel_pmc_core.c | 40 +++----------
> .../platform/x86/intel_pmc_core_plat_drv.c | 60 +++++++++++++++++++
> 3 files changed, 69 insertions(+), 33 deletions(-)
> create mode 100644 drivers/platform/x86/intel_pmc_core_plat_drv.c
>
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 86cb76677bc8..7041a88c34c7 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -89,7 +89,7 @@ obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU) += intel_bxtwc_tmu.o
> obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \
> intel_telemetry_pltdrv.o \
> intel_telemetry_debugfs.o
> -obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o
> +obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o intel_pmc_core_plat_drv.o
> obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
> obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o
> obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 1d902230ba61..f20d08ad39ea 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -1023,47 +1023,23 @@ static const struct dev_pm_ops pmc_core_pm_ops = {
> SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend, pmc_core_resume)
> };
>
> +static const struct acpi_device_id pmc_core_acpi_ids[] = {
> + {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
> + { }
> +};
> +MODULE_DEVICE_TABLE(acpi, pmc_core_acpi_ids);
> +
> static struct platform_driver pmc_core_driver = {
> .driver = {
> .name = "intel_pmc_core",
> + .acpi_match_table = ACPI_PTR(pmc_core_acpi_ids),
> .pm = &pmc_core_pm_ops,
> },
> .probe = pmc_core_probe,
> .remove = pmc_core_remove,
> };
>
> -static struct platform_device pmc_core_device = {
> - .name = "intel_pmc_core",
> -};
> -
> -static int __init pmc_core_init(void)
> -{
> - int ret;
> -
> - if (!x86_match_cpu(intel_pmc_core_ids))
> - return -ENODEV;
> -
> - ret = platform_driver_register(&pmc_core_driver);
> - if (ret)
> - return ret;
> -
> - ret = platform_device_register(&pmc_core_device);
> - if (ret) {
> - platform_driver_unregister(&pmc_core_driver);
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> -static void __exit pmc_core_exit(void)
> -{
> - platform_device_unregister(&pmc_core_device);
> - platform_driver_unregister(&pmc_core_driver);
> -}
> -
> -module_init(pmc_core_init)
> -module_exit(pmc_core_exit)
> +module_platform_driver(pmc_core_driver);
>
> MODULE_LICENSE("GPL v2");
> MODULE_DESCRIPTION("Intel PMC Core Driver");
> diff --git a/drivers/platform/x86/intel_pmc_core_plat_drv.c b/drivers/platform/x86/intel_pmc_core_plat_drv.c
> new file mode 100644
> index 000000000000..20be3eaeb722
> --- /dev/null
> +++ b/drivers/platform/x86/intel_pmc_core_plat_drv.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Intel PMC Core platform init
> + * Copyright (c) 2019, Google Inc.
> + * Author - Rajat Jain
> + *
> + * This code instantiates platform devices for intel_pmc_core driver, only
> + * on supported platforms that may not have the ACPI devices in the ACPI tables.
> + * No new platforms should be added here, because we expect that new platforms
> + * should all have the ACPI device, which is the preferred way of enumeration.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> +
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +
> +static struct platform_device pmc_core_device = {
> + .name = "intel_pmc_core",
> +};
> +
> +/*
> + * intel_pmc_core_platform_ids is the list of platforms where we want to
> + * instantiate the platform_device if not already instantiated. This is
> + * different than intel_pmc_core_ids in intel_pmc_core.c which is the
> + * list of platforms that the driver supports for pmc_core device. The
> + * other list may grow, but this list should not.
> + */
> +static const struct x86_cpu_id intel_pmc_core_platform_ids[] = {
> + INTEL_CPU_FAM6(SKYLAKE_MOBILE, pmc_core_device),
> + INTEL_CPU_FAM6(SKYLAKE_DESKTOP, pmc_core_device),
> + INTEL_CPU_FAM6(KABYLAKE_MOBILE, pmc_core_device),
> + INTEL_CPU_FAM6(KABYLAKE_DESKTOP, pmc_core_device),
> + INTEL_CPU_FAM6(CANNONLAKE_MOBILE, pmc_core_device),
> + INTEL_CPU_FAM6(ICELAKE_MOBILE, pmc_core_device),
> + {}
> +};
> +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_platform_ids);
> +
> +static int __init pmc_core_platform_init(void)
> +{
> + /* Skip creating the platform device if ACPI already has a device */
> + if (acpi_dev_present("INT33A1", NULL, -1))
> + return -ENODEV;
> +
> + if (!x86_match_cpu(intel_pmc_core_platform_ids))
> + return -ENODEV;
> +
> + return platform_device_register(&pmc_core_device);
> +}
> +
> +static void __exit pmc_core_platform_exit(void)
> +{
> + platform_device_unregister(&pmc_core_device);
> +}
> +
> +module_init(pmc_core_platform_init);
> +module_exit(pmc_core_platform_exit);
> --
> 2.21.0.392.gf8f6787159e-goog
>


--
With Best Regards,
Andy Shevchenko