[PATCH 0/9] intel_pmc_core: Add sub-state requirements and mode latching support

[PATCH 0/9] intel_pmc_core: Add sub-state requirements and mode latching support

[David E. Box]

- Patch 1 and 2 remove the use of the global struct pmc_dev
- Patches 3-7 add support for reading low power mode sub-state
  requirements, latching sub-state status on different low power mode
  events, and displaying the sub-state residency in microseconds
- Patch 8 adds missing LTR IPs for TGL
- Patch 9 adds support for ADL-P which is based on TGL

Applied on top of latest 5.12-rc2 based hans-review/review-hans

David E. Box (4):
  platform/x86: intel_pmc_core: Don't use global pmcdev in quirks
  platform/x86: intel_pmc_core: Remove global struct pmc_dev
  platform/x86: intel_pmc_core: Add option to set/clear LPM mode
  platform/x86: intel_pmc_core: Add support for Alder Lake PCH-P

Gayatri Kammela (5):
  platform/x86: intel_pmc_core: Handle sub-states generically
  platform/x86: intel_pmc_core: Show LPM residency in microseconds
  platform/x86: intel_pmc_core: Get LPM requirements for Tiger Lake
  platform/x86: intel_pmc_core: Add requirements file to debugfs
  platform/x86: intel_pmc_core: Add LTR registers for Tiger Lake

 drivers/platform/x86/intel_pmc_core.c | 359 +++++++++++++++++++++++---
 drivers/platform/x86/intel_pmc_core.h |  47 +++-
 2 files changed, 370 insertions(+), 36 deletions(-)

-- 
2.25.1

[PATCH 1/9] platform/x86: intel_pmc_core: Don't use global pmcdev in quirks

[David E. Box]

The DMI callbacks, used for quirks, currently access the PMC by getting
the address a global pmc_dev struct. Instead, have the callbacks set a
global quirk specific variable. In probe, after calling dmi_check_system(),
pass pmc_dev to a function that will handle each quirk if its variable
condition is met. This allows removing the global pmc_dev later.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_core.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index b5888aeb4bcf..260d49dca1ad 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -1186,9 +1186,15 @@ static const struct pci_device_id pmc_pci_ids[] = {
  * the platform BIOS enforces 24Mhz crystal to shutdown
  * before PMC can assert SLP_S0#.
  */
+static bool xtal_ignore;
 static int quirk_xtal_ignore(const struct dmi_system_id *id)
 {
-	struct pmc_dev *pmcdev = &pmc;
+	xtal_ignore = true;
+	return 0;
+}
+
+static void pmc_core_xtal_ignore(struct pmc_dev *pmcdev)
+{
 	u32 value;
 
 	value = pmc_core_reg_read(pmcdev, pmcdev->map->pm_vric1_offset);
@@ -1197,7 +1203,6 @@ static int quirk_xtal_ignore(const struct dmi_system_id *id)
 	/* Low Voltage Mode Enable */
 	value &= ~SPT_PMC_VRIC1_SLPS0LVEN;
 	pmc_core_reg_write(pmcdev, pmcdev->map->pm_vric1_offset, value);
-	return 0;
 }
 
 static const struct dmi_system_id pmc_core_dmi_table[]  = {
@@ -1212,6 +1217,14 @@ static const struct dmi_system_id pmc_core_dmi_table[]  = {
 	{}
 };
 
+static void pmc_core_do_dmi_quirks(struct pmc_dev *pmcdev)
+{
+	dmi_check_system(pmc_core_dmi_table);
+
+	if (xtal_ignore)
+		pmc_core_xtal_ignore(pmcdev);
+}
+
 static int pmc_core_probe(struct platform_device *pdev)
 {
 	static bool device_initialized;
@@ -1253,7 +1266,7 @@ static int pmc_core_probe(struct platform_device *pdev)
 	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);
+	pmc_core_do_dmi_quirks(pmcdev);
 
 	/*
 	 * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when
-- 
2.25.1

Re: [PATCH 1/9] platform/x86: intel_pmc_core: Don't use global pmcdev in quirks

[Hans de Goede]

Hi,

On 4/1/21 5:05 AM, David E. Box wrote:
> The DMI callbacks, used for quirks, currently access the PMC by getting
> the address a global pmc_dev struct. Instead, have the callbacks set a
> global quirk specific variable. In probe, after calling dmi_check_system(),
> pass pmc_dev to a function that will handle each quirk if its variable
> condition is met. This allows removing the global pmc_dev later.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/platform/x86/intel_pmc_core.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index b5888aeb4bcf..260d49dca1ad 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -1186,9 +1186,15 @@ static const struct pci_device_id pmc_pci_ids[] = {
>   * the platform BIOS enforces 24Mhz crystal to shutdown
>   * before PMC can assert SLP_S0#.
>   */
> +static bool xtal_ignore;
>  static int quirk_xtal_ignore(const struct dmi_system_id *id)
>  {
> -	struct pmc_dev *pmcdev = &pmc;
> +	xtal_ignore = true;
> +	return 0;
> +}
> +
> +static void pmc_core_xtal_ignore(struct pmc_dev *pmcdev)
> +{
>  	u32 value;
>  
>  	value = pmc_core_reg_read(pmcdev, pmcdev->map->pm_vric1_offset);
> @@ -1197,7 +1203,6 @@ static int quirk_xtal_ignore(const struct dmi_system_id *id)
>  	/* Low Voltage Mode Enable */
>  	value &= ~SPT_PMC_VRIC1_SLPS0LVEN;
>  	pmc_core_reg_write(pmcdev, pmcdev->map->pm_vric1_offset, value);
> -	return 0;
>  }
>  
>  static const struct dmi_system_id pmc_core_dmi_table[]  = {
> @@ -1212,6 +1217,14 @@ static const struct dmi_system_id pmc_core_dmi_table[]  = {
>  	{}
>  };
>  
> +static void pmc_core_do_dmi_quirks(struct pmc_dev *pmcdev)
> +{
> +	dmi_check_system(pmc_core_dmi_table);
> +
> +	if (xtal_ignore)
> +		pmc_core_xtal_ignore(pmcdev);
> +}
> +
>  static int pmc_core_probe(struct platform_device *pdev)
>  {
>  	static bool device_initialized;
> @@ -1253,7 +1266,7 @@ static int pmc_core_probe(struct platform_device *pdev)
>  	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);
> +	pmc_core_do_dmi_quirks(pmcdev);
>  
>  	/*
>  	 * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when
> 

Re: [PATCH 1/9] platform/x86: intel_pmc_core: Don't use global pmcdev in quirks

[Rajneesh Bhardwaj]

Reviewed-by: Rajneesh Bhardwaj <irenic.rajneesh@gmail.com>

On Wed, Mar 31, 2021 at 11:06 PM David E. Box
<david.e.box@linux.intel.com> wrote:
>
> The DMI callbacks, used for quirks, currently access the PMC by getting
> the address a global pmc_dev struct. Instead, have the callbacks set a
> global quirk specific variable. In probe, after calling dmi_check_system(),
> pass pmc_dev to a function that will handle each quirk if its variable
> condition is met. This allows removing the global pmc_dev later.
>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index b5888aeb4bcf..260d49dca1ad 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -1186,9 +1186,15 @@ static const struct pci_device_id pmc_pci_ids[] = {
>   * the platform BIOS enforces 24Mhz crystal to shutdown
>   * before PMC can assert SLP_S0#.
>   */
> +static bool xtal_ignore;
>  static int quirk_xtal_ignore(const struct dmi_system_id *id)
>  {
> -       struct pmc_dev *pmcdev = &pmc;
> +       xtal_ignore = true;
> +       return 0;
> +}
> +
> +static void pmc_core_xtal_ignore(struct pmc_dev *pmcdev)
> +{
>         u32 value;
>
>         value = pmc_core_reg_read(pmcdev, pmcdev->map->pm_vric1_offset);
> @@ -1197,7 +1203,6 @@ static int quirk_xtal_ignore(const struct dmi_system_id *id)
>         /* Low Voltage Mode Enable */
>         value &= ~SPT_PMC_VRIC1_SLPS0LVEN;
>         pmc_core_reg_write(pmcdev, pmcdev->map->pm_vric1_offset, value);
> -       return 0;
>  }
>
>  static const struct dmi_system_id pmc_core_dmi_table[]  = {
> @@ -1212,6 +1217,14 @@ static const struct dmi_system_id pmc_core_dmi_table[]  = {
>         {}
>  };
>
> +static void pmc_core_do_dmi_quirks(struct pmc_dev *pmcdev)
> +{
> +       dmi_check_system(pmc_core_dmi_table);
> +
> +       if (xtal_ignore)
> +               pmc_core_xtal_ignore(pmcdev);
> +}
> +
>  static int pmc_core_probe(struct platform_device *pdev)
>  {
>         static bool device_initialized;
> @@ -1253,7 +1266,7 @@ static int pmc_core_probe(struct platform_device *pdev)
>         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);
> +       pmc_core_do_dmi_quirks(pmcdev);
>
>         /*
>          * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when
> --
> 2.25.1
>


-- 
Thanks,
Rajneesh

[PATCH 2/9] platform/x86: intel_pmc_core: Remove global struct pmc_dev

[David E. Box]

The intel_pmc_core driver did not always bind to a device which meant it
lacked a struct device that could be used to maintain driver data. So a
global instance of struct pmc_dev was used for this purpose and functions
accessed this directly. Since the driver now binds to an ACPI device,
remove the global pmc_dev in favor of one that is allocated during probe.
Modify users of the global to obtain the object by argument instead.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_core.c | 41 ++++++++++++++-------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 260d49dca1ad..5ca40fe3da59 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -31,8 +31,6 @@
 
 #include "intel_pmc_core.h"
 
-static struct pmc_dev pmc;
-
 /* PKGC MSRs are common across Intel Core SoCs */
 static const struct pmc_bit_map msr_map[] = {
 	{"Package C2",                  MSR_PKG_C2_RESIDENCY},
@@ -617,9 +615,8 @@ static int pmc_core_dev_state_get(void *data, u64 *val)
 
 DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get, NULL, "%llu\n");
 
-static int pmc_core_check_read_lock_bit(void)
+static int pmc_core_check_read_lock_bit(struct pmc_dev *pmcdev)
 {
-	struct pmc_dev *pmcdev = &pmc;
 	u32 value;
 
 	value = pmc_core_reg_read(pmcdev, pmcdev->map->pm_cfg_offset);
@@ -744,28 +741,26 @@ static int pmc_core_ppfear_show(struct seq_file *s, void *unused)
 DEFINE_SHOW_ATTRIBUTE(pmc_core_ppfear);
 
 /* This function should return link status, 0 means ready */
-static int pmc_core_mtpmc_link_status(void)
+static int pmc_core_mtpmc_link_status(struct pmc_dev *pmcdev)
 {
-	struct pmc_dev *pmcdev = &pmc;
 	u32 value;
 
 	value = pmc_core_reg_read(pmcdev, SPT_PMC_PM_STS_OFFSET);
 	return value & BIT(SPT_PMC_MSG_FULL_STS_BIT);
 }
 
-static int pmc_core_send_msg(u32 *addr_xram)
+static int pmc_core_send_msg(struct pmc_dev *pmcdev, u32 *addr_xram)
 {
-	struct pmc_dev *pmcdev = &pmc;
 	u32 dest;
 	int timeout;
 
 	for (timeout = NUM_RETRIES; timeout > 0; timeout--) {
-		if (pmc_core_mtpmc_link_status() == 0)
+		if (pmc_core_mtpmc_link_status(pmcdev) == 0)
 			break;
 		msleep(5);
 	}
 
-	if (timeout <= 0 && pmc_core_mtpmc_link_status())
+	if (timeout <= 0 && pmc_core_mtpmc_link_status(pmcdev))
 		return -EBUSY;
 
 	dest = (*addr_xram & MTPMC_MASK) | (1U << 1);
@@ -791,7 +786,7 @@ static int pmc_core_mphy_pg_show(struct seq_file *s, void *unused)
 
 	mutex_lock(&pmcdev->lock);
 
-	if (pmc_core_send_msg(&mphy_core_reg_low) != 0) {
+	if (pmc_core_send_msg(pmcdev, &mphy_core_reg_low) != 0) {
 		err = -EBUSY;
 		goto out_unlock;
 	}
@@ -799,7 +794,7 @@ static int pmc_core_mphy_pg_show(struct seq_file *s, void *unused)
 	msleep(10);
 	val_low = pmc_core_reg_read(pmcdev, SPT_PMC_MFPMC_OFFSET);
 
-	if (pmc_core_send_msg(&mphy_core_reg_high) != 0) {
+	if (pmc_core_send_msg(pmcdev, &mphy_core_reg_high) != 0) {
 		err = -EBUSY;
 		goto out_unlock;
 	}
@@ -842,7 +837,7 @@ static int pmc_core_pll_show(struct seq_file *s, void *unused)
 	mphy_common_reg  = (SPT_PMC_MPHY_COM_STS_0 << 16);
 	mutex_lock(&pmcdev->lock);
 
-	if (pmc_core_send_msg(&mphy_common_reg) != 0) {
+	if (pmc_core_send_msg(pmcdev, &mphy_common_reg) != 0) {
 		err = -EBUSY;
 		goto out_unlock;
 	}
@@ -863,9 +858,8 @@ static int pmc_core_pll_show(struct seq_file *s, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(pmc_core_pll);
 
-static int pmc_core_send_ltr_ignore(u32 value)
+static int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value)
 {
-	struct pmc_dev *pmcdev = &pmc;
 	const struct pmc_reg_map *map = pmcdev->map;
 	u32 reg;
 	int err = 0;
@@ -891,6 +885,8 @@ static ssize_t pmc_core_ltr_ignore_write(struct file *file,
 					 const char __user *userbuf,
 					 size_t count, loff_t *ppos)
 {
+	struct seq_file *s = file->private_data;
+	struct pmc_dev *pmcdev = s->private;
 	u32 buf_size, value;
 	int err;
 
@@ -900,7 +896,7 @@ static ssize_t pmc_core_ltr_ignore_write(struct file *file,
 	if (err)
 		return err;
 
-	err = pmc_core_send_ltr_ignore(value);
+	err = pmc_core_send_ltr_ignore(pmcdev, value);
 
 	return err == 0 ? count : err;
 }
@@ -1228,13 +1224,19 @@ static void pmc_core_do_dmi_quirks(struct pmc_dev *pmcdev)
 static int pmc_core_probe(struct platform_device *pdev)
 {
 	static bool device_initialized;
-	struct pmc_dev *pmcdev = &pmc;
+	struct pmc_dev *pmcdev;
 	const struct x86_cpu_id *cpu_id;
 	u64 slp_s0_addr;
 
 	if (device_initialized)
 		return -ENODEV;
 
+	pmcdev = devm_kzalloc(&pdev->dev, sizeof(*pmcdev), GFP_KERNEL);
+	if (!pmcdev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, pmcdev);
+
 	cpu_id = x86_match_cpu(intel_pmc_core_ids);
 	if (!cpu_id)
 		return -ENODEV;
@@ -1264,8 +1266,7 @@ static int pmc_core_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	mutex_init(&pmcdev->lock);
-	platform_set_drvdata(pdev, pmcdev);
-	pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
+	pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
 	pmc_core_do_dmi_quirks(pmcdev);
 
 	/*
@@ -1274,7 +1275,7 @@ static int pmc_core_probe(struct platform_device *pdev)
 	 */
 	if (pmcdev->map == &tgl_reg_map) {
 		dev_dbg(&pdev->dev, "ignoring GBE LTR\n");
-		pmc_core_send_ltr_ignore(3);
+		pmc_core_send_ltr_ignore(pmcdev, 3);
 	}
 
 	pmc_core_dbgfs_register(pmcdev);
-- 
2.25.1

Re: [PATCH 2/9] platform/x86: intel_pmc_core: Remove global struct pmc_dev

[Hans de Goede]

Hi,

On 4/1/21 5:05 AM, David E. Box wrote:
> The intel_pmc_core driver did not always bind to a device which meant it
> lacked a struct device that could be used to maintain driver data. So a
> global instance of struct pmc_dev was used for this purpose and functions
> accessed this directly. Since the driver now binds to an ACPI device,
> remove the global pmc_dev in favor of one that is allocated during probe.
> Modify users of the global to obtain the object by argument instead.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>  drivers/platform/x86/intel_pmc_core.c | 41 ++++++++++++++-------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 260d49dca1ad..5ca40fe3da59 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -31,8 +31,6 @@
>  
>  #include "intel_pmc_core.h"
>  
> -static struct pmc_dev pmc;
> -
>  /* PKGC MSRs are common across Intel Core SoCs */
>  static const struct pmc_bit_map msr_map[] = {
>  	{"Package C2",                  MSR_PKG_C2_RESIDENCY},
> @@ -617,9 +615,8 @@ static int pmc_core_dev_state_get(void *data, u64 *val)
>  
>  DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get, NULL, "%llu\n");
>  
> -static int pmc_core_check_read_lock_bit(void)
> +static int pmc_core_check_read_lock_bit(struct pmc_dev *pmcdev)
>  {
> -	struct pmc_dev *pmcdev = &pmc;
>  	u32 value;
>  
>  	value = pmc_core_reg_read(pmcdev, pmcdev->map->pm_cfg_offset);
> @@ -744,28 +741,26 @@ static int pmc_core_ppfear_show(struct seq_file *s, void *unused)
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_ppfear);
>  
>  /* This function should return link status, 0 means ready */
> -static int pmc_core_mtpmc_link_status(void)
> +static int pmc_core_mtpmc_link_status(struct pmc_dev *pmcdev)
>  {
> -	struct pmc_dev *pmcdev = &pmc;
>  	u32 value;
>  
>  	value = pmc_core_reg_read(pmcdev, SPT_PMC_PM_STS_OFFSET);
>  	return value & BIT(SPT_PMC_MSG_FULL_STS_BIT);
>  }
>  
> -static int pmc_core_send_msg(u32 *addr_xram)
> +static int pmc_core_send_msg(struct pmc_dev *pmcdev, u32 *addr_xram)
>  {
> -	struct pmc_dev *pmcdev = &pmc;
>  	u32 dest;
>  	int timeout;
>  
>  	for (timeout = NUM_RETRIES; timeout > 0; timeout--) {
> -		if (pmc_core_mtpmc_link_status() == 0)
> +		if (pmc_core_mtpmc_link_status(pmcdev) == 0)
>  			break;
>  		msleep(5);
>  	}
>  
> -	if (timeout <= 0 && pmc_core_mtpmc_link_status())
> +	if (timeout <= 0 && pmc_core_mtpmc_link_status(pmcdev))
>  		return -EBUSY;
>  
>  	dest = (*addr_xram & MTPMC_MASK) | (1U << 1);
> @@ -791,7 +786,7 @@ static int pmc_core_mphy_pg_show(struct seq_file *s, void *unused)
>  
>  	mutex_lock(&pmcdev->lock);
>  
> -	if (pmc_core_send_msg(&mphy_core_reg_low) != 0) {
> +	if (pmc_core_send_msg(pmcdev, &mphy_core_reg_low) != 0) {
>  		err = -EBUSY;
>  		goto out_unlock;
>  	}
> @@ -799,7 +794,7 @@ static int pmc_core_mphy_pg_show(struct seq_file *s, void *unused)
>  	msleep(10);
>  	val_low = pmc_core_reg_read(pmcdev, SPT_PMC_MFPMC_OFFSET);
>  
> -	if (pmc_core_send_msg(&mphy_core_reg_high) != 0) {
> +	if (pmc_core_send_msg(pmcdev, &mphy_core_reg_high) != 0) {
>  		err = -EBUSY;
>  		goto out_unlock;
>  	}
> @@ -842,7 +837,7 @@ static int pmc_core_pll_show(struct seq_file *s, void *unused)
>  	mphy_common_reg  = (SPT_PMC_MPHY_COM_STS_0 << 16);
>  	mutex_lock(&pmcdev->lock);
>  
> -	if (pmc_core_send_msg(&mphy_common_reg) != 0) {
> +	if (pmc_core_send_msg(pmcdev, &mphy_common_reg) != 0) {
>  		err = -EBUSY;
>  		goto out_unlock;
>  	}
> @@ -863,9 +858,8 @@ static int pmc_core_pll_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_pll);
>  
> -static int pmc_core_send_ltr_ignore(u32 value)
> +static int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value)
>  {
> -	struct pmc_dev *pmcdev = &pmc;
>  	const struct pmc_reg_map *map = pmcdev->map;
>  	u32 reg;
>  	int err = 0;
> @@ -891,6 +885,8 @@ static ssize_t pmc_core_ltr_ignore_write(struct file *file,
>  					 const char __user *userbuf,
>  					 size_t count, loff_t *ppos)
>  {
> +	struct seq_file *s = file->private_data;
> +	struct pmc_dev *pmcdev = s->private;
>  	u32 buf_size, value;
>  	int err;
>  
> @@ -900,7 +896,7 @@ static ssize_t pmc_core_ltr_ignore_write(struct file *file,
>  	if (err)
>  		return err;
>  
> -	err = pmc_core_send_ltr_ignore(value);
> +	err = pmc_core_send_ltr_ignore(pmcdev, value);
>  
>  	return err == 0 ? count : err;
>  }
> @@ -1228,13 +1224,19 @@ static void pmc_core_do_dmi_quirks(struct pmc_dev *pmcdev)
>  static int pmc_core_probe(struct platform_device *pdev)
>  {
>  	static bool device_initialized;
> -	struct pmc_dev *pmcdev = &pmc;
> +	struct pmc_dev *pmcdev;
>  	const struct x86_cpu_id *cpu_id;
>  	u64 slp_s0_addr;
>  
>  	if (device_initialized)
>  		return -ENODEV;
>  
> +	pmcdev = devm_kzalloc(&pdev->dev, sizeof(*pmcdev), GFP_KERNEL);
> +	if (!pmcdev)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, pmcdev);
> +
>  	cpu_id = x86_match_cpu(intel_pmc_core_ids);
>  	if (!cpu_id)
>  		return -ENODEV;
> @@ -1264,8 +1266,7 @@ static int pmc_core_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	mutex_init(&pmcdev->lock);
> -	platform_set_drvdata(pdev, pmcdev);
> -	pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
> +	pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
>  	pmc_core_do_dmi_quirks(pmcdev);
>  
>  	/*
> @@ -1274,7 +1275,7 @@ static int pmc_core_probe(struct platform_device *pdev)
>  	 */
>  	if (pmcdev->map == &tgl_reg_map) {
>  		dev_dbg(&pdev->dev, "ignoring GBE LTR\n");
> -		pmc_core_send_ltr_ignore(3);
> +		pmc_core_send_ltr_ignore(pmcdev, 3);
>  	}
>  
>  	pmc_core_dbgfs_register(pmcdev);
> 

Re: [PATCH 2/9] platform/x86: intel_pmc_core: Remove global struct pmc_dev

[Rajneesh Bhardwaj]

Reviewed-by: Rajneesh Bhardwaj <irenic.rajneesh@gmail.com>

On Wed, Mar 31, 2021 at 11:06 PM David E. Box
<david.e.box@linux.intel.com> wrote:
>
> The intel_pmc_core driver did not always bind to a device which meant it
> lacked a struct device that could be used to maintain driver data. So a
> global instance of struct pmc_dev was used for this purpose and functions
> accessed this directly. Since the driver now binds to an ACPI device,
> remove the global pmc_dev in favor of one that is allocated during probe.
> Modify users of the global to obtain the object by argument instead.
>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 41 ++++++++++++++-------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 260d49dca1ad..5ca40fe3da59 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -31,8 +31,6 @@
>
>  #include "intel_pmc_core.h"
>
> -static struct pmc_dev pmc;
> -
>  /* PKGC MSRs are common across Intel Core SoCs */
>  static const struct pmc_bit_map msr_map[] = {
>         {"Package C2",                  MSR_PKG_C2_RESIDENCY},
> @@ -617,9 +615,8 @@ static int pmc_core_dev_state_get(void *data, u64 *val)
>
>  DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get, NULL, "%llu\n");
>
> -static int pmc_core_check_read_lock_bit(void)
> +static int pmc_core_check_read_lock_bit(struct pmc_dev *pmcdev)
>  {
> -       struct pmc_dev *pmcdev = &pmc;
>         u32 value;
>
>         value = pmc_core_reg_read(pmcdev, pmcdev->map->pm_cfg_offset);
> @@ -744,28 +741,26 @@ static int pmc_core_ppfear_show(struct seq_file *s, void *unused)
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_ppfear);
>
>  /* This function should return link status, 0 means ready */
> -static int pmc_core_mtpmc_link_status(void)
> +static int pmc_core_mtpmc_link_status(struct pmc_dev *pmcdev)
>  {
> -       struct pmc_dev *pmcdev = &pmc;
>         u32 value;
>
>         value = pmc_core_reg_read(pmcdev, SPT_PMC_PM_STS_OFFSET);
>         return value & BIT(SPT_PMC_MSG_FULL_STS_BIT);
>  }
>
> -static int pmc_core_send_msg(u32 *addr_xram)
> +static int pmc_core_send_msg(struct pmc_dev *pmcdev, u32 *addr_xram)
>  {
> -       struct pmc_dev *pmcdev = &pmc;
>         u32 dest;
>         int timeout;
>
>         for (timeout = NUM_RETRIES; timeout > 0; timeout--) {
> -               if (pmc_core_mtpmc_link_status() == 0)
> +               if (pmc_core_mtpmc_link_status(pmcdev) == 0)
>                         break;
>                 msleep(5);
>         }
>
> -       if (timeout <= 0 && pmc_core_mtpmc_link_status())
> +       if (timeout <= 0 && pmc_core_mtpmc_link_status(pmcdev))
>                 return -EBUSY;
>
>         dest = (*addr_xram & MTPMC_MASK) | (1U << 1);
> @@ -791,7 +786,7 @@ static int pmc_core_mphy_pg_show(struct seq_file *s, void *unused)
>
>         mutex_lock(&pmcdev->lock);
>
> -       if (pmc_core_send_msg(&mphy_core_reg_low) != 0) {
> +       if (pmc_core_send_msg(pmcdev, &mphy_core_reg_low) != 0) {
>                 err = -EBUSY;
>                 goto out_unlock;
>         }
> @@ -799,7 +794,7 @@ static int pmc_core_mphy_pg_show(struct seq_file *s, void *unused)
>         msleep(10);
>         val_low = pmc_core_reg_read(pmcdev, SPT_PMC_MFPMC_OFFSET);
>
> -       if (pmc_core_send_msg(&mphy_core_reg_high) != 0) {
> +       if (pmc_core_send_msg(pmcdev, &mphy_core_reg_high) != 0) {
>                 err = -EBUSY;
>                 goto out_unlock;
>         }
> @@ -842,7 +837,7 @@ static int pmc_core_pll_show(struct seq_file *s, void *unused)
>         mphy_common_reg  = (SPT_PMC_MPHY_COM_STS_0 << 16);
>         mutex_lock(&pmcdev->lock);
>
> -       if (pmc_core_send_msg(&mphy_common_reg) != 0) {
> +       if (pmc_core_send_msg(pmcdev, &mphy_common_reg) != 0) {
>                 err = -EBUSY;
>                 goto out_unlock;
>         }
> @@ -863,9 +858,8 @@ static int pmc_core_pll_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_pll);
>
> -static int pmc_core_send_ltr_ignore(u32 value)
> +static int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value)
>  {
> -       struct pmc_dev *pmcdev = &pmc;
>         const struct pmc_reg_map *map = pmcdev->map;
>         u32 reg;
>         int err = 0;
> @@ -891,6 +885,8 @@ static ssize_t pmc_core_ltr_ignore_write(struct file *file,
>                                          const char __user *userbuf,
>                                          size_t count, loff_t *ppos)
>  {
> +       struct seq_file *s = file->private_data;
> +       struct pmc_dev *pmcdev = s->private;
>         u32 buf_size, value;
>         int err;
>
> @@ -900,7 +896,7 @@ static ssize_t pmc_core_ltr_ignore_write(struct file *file,
>         if (err)
>                 return err;
>
> -       err = pmc_core_send_ltr_ignore(value);
> +       err = pmc_core_send_ltr_ignore(pmcdev, value);
>
>         return err == 0 ? count : err;
>  }
> @@ -1228,13 +1224,19 @@ static void pmc_core_do_dmi_quirks(struct pmc_dev *pmcdev)
>  static int pmc_core_probe(struct platform_device *pdev)
>  {
>         static bool device_initialized;
> -       struct pmc_dev *pmcdev = &pmc;
> +       struct pmc_dev *pmcdev;
>         const struct x86_cpu_id *cpu_id;
>         u64 slp_s0_addr;
>
>         if (device_initialized)
>                 return -ENODEV;
>
> +       pmcdev = devm_kzalloc(&pdev->dev, sizeof(*pmcdev), GFP_KERNEL);
> +       if (!pmcdev)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, pmcdev);
> +
>         cpu_id = x86_match_cpu(intel_pmc_core_ids);
>         if (!cpu_id)
>                 return -ENODEV;
> @@ -1264,8 +1266,7 @@ static int pmc_core_probe(struct platform_device *pdev)
>                 return -ENOMEM;
>
>         mutex_init(&pmcdev->lock);
> -       platform_set_drvdata(pdev, pmcdev);
> -       pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit();
> +       pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
>         pmc_core_do_dmi_quirks(pmcdev);
>
>         /*
> @@ -1274,7 +1275,7 @@ static int pmc_core_probe(struct platform_device *pdev)
>          */
>         if (pmcdev->map == &tgl_reg_map) {
>                 dev_dbg(&pdev->dev, "ignoring GBE LTR\n");
> -               pmc_core_send_ltr_ignore(3);
> +               pmc_core_send_ltr_ignore(pmcdev, 3);
>         }
>
>         pmc_core_dbgfs_register(pmcdev);
> --
> 2.25.1
>


-- 
Thanks,
Rajneesh

[PATCH 3/9] platform/x86: intel_pmc_core: Handle sub-states generically

[David E. Box]

From: Gayatri Kammela <gayatri.kammela@intel.com>

The current implementation of pmc_core_substate_res_show() is written
specifically for Tiger Lake. However, new platforms will also have
sub-states and may support different modes. Therefore rewrite the code to
handle sub-states generically.

Read the number and type of enabled states from the PMC. Use the Low
Power Mode (LPM) priority register to store the states in order from
shallowest to deepest for displaying. Add a for_each macro to simplify
this. While changing the sub-state display it makes sense to show only the
"enabled" sub-states instead of showing all possible ones. After this
patch, the debugfs file looks like this:

Substate   Residency
S0i2.0     0
S0i3.0     0
S0i2.1     9329279
S0i3.1     0
S0i3.2     0

Suggested-by: David E. Box <david.e.box@linux.intel.com>
Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_core.c | 59 ++++++++++++++++++++++-----
 drivers/platform/x86/intel_pmc_core.h | 18 +++++++-
 2 files changed, 64 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 5ca40fe3da59..ce300c2942d0 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -577,8 +577,9 @@ static const struct pmc_reg_map tgl_reg_map = {
 	.pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
 	.pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
 	.ltr_ignore_max = TGL_NUM_IP_IGN_ALLOWED,
-	.lpm_modes = tgl_lpm_modes,
+	.lpm_num_maps = TGL_LPM_NUM_MAPS,
 	.lpm_en_offset = TGL_LPM_EN_OFFSET,
+	.lpm_priority_offset = TGL_LPM_PRI_OFFSET,
 	.lpm_residency_offset = TGL_LPM_RESIDENCY_OFFSET,
 	.lpm_sts = tgl_lpm_maps,
 	.lpm_status_offset = TGL_LPM_STATUS_OFFSET,
@@ -1028,18 +1029,14 @@ DEFINE_SHOW_ATTRIBUTE(pmc_core_ltr);
 static int pmc_core_substate_res_show(struct seq_file *s, void *unused)
 {
 	struct pmc_dev *pmcdev = s->private;
-	const char **lpm_modes = pmcdev->map->lpm_modes;
 	u32 offset = pmcdev->map->lpm_residency_offset;
-	u32 lpm_en;
-	int index;
+	int i, mode;
 
-	lpm_en = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_en_offset);
-	seq_printf(s, "status substate residency\n");
-	for (index = 0; lpm_modes[index]; index++) {
-		seq_printf(s, "%7s %7s %-15u\n",
-			   BIT(index) & lpm_en ? "Enabled" : " ",
-			   lpm_modes[index], pmc_core_reg_read(pmcdev, offset));
-		offset += 4;
+	seq_printf(s, "%-10s %-15s\n", "Substate", "Residency");
+
+	pmc_for_each_mode(i, mode, pmcdev) {
+		seq_printf(s, "%-10s %-15u\n", pmc_lpm_modes[mode],
+			   pmc_core_reg_read(pmcdev, offset + (4 * mode)));
 	}
 
 	return 0;
@@ -1091,6 +1088,45 @@ static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(pmc_core_pkgc);
 
+static void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev)
+{
+	u8 lpm_priority[LPM_MAX_NUM_MODES];
+	u32 lpm_en;
+	int mode, i, p;
+
+	/* Use LPM Maps to indicate support for substates */
+	if (!pmcdev->map->lpm_num_maps)
+		return;
+
+	lpm_en = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_en_offset);
+	pmcdev->num_modes = hweight32(lpm_en);
+
+	/* Each byte contains information for 2 modes (7:4 and 3:0) */
+	for (mode = 0; mode < LPM_MAX_NUM_MODES; mode += 2) {
+		u8 priority = pmc_core_reg_read_byte(pmcdev,
+				pmcdev->map->lpm_priority_offset + (mode / 2));
+		int pri0 = GENMASK(3, 0) & priority;
+		int pri1 = (GENMASK(7, 4) & priority) >> 4;
+
+		lpm_priority[pri0] = mode;
+		lpm_priority[pri1] = mode + 1;
+	}
+
+	/*
+	 * Loop though all modes from lowest to highest priority,
+	 * and capture all enabled modes in order
+	 */
+	i = 0;
+	for (p = LPM_MAX_NUM_MODES - 1; p >= 0; p--) {
+		int mode = lpm_priority[p];
+
+		if (!(BIT(mode) & lpm_en))
+			continue;
+
+		pmcdev->lpm_en_modes[i++] = mode;
+	}
+}
+
 static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
 {
 	debugfs_remove_recursive(pmcdev->dbgfs_dir);
@@ -1267,6 +1303,7 @@ static int pmc_core_probe(struct platform_device *pdev)
 
 	mutex_init(&pmcdev->lock);
 	pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
+	pmc_core_get_low_power_modes(pmcdev);
 	pmc_core_do_dmi_quirks(pmcdev);
 
 	/*
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index f33cd2c34835..5a4e3a49f5b1 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -187,6 +187,8 @@ enum ppfear_regs {
 #define ICL_PMC_LTR_WIGIG			0x1BFC
 #define ICL_PMC_SLP_S0_RES_COUNTER_STEP		0x64
 
+#define LPM_MAX_NUM_MODES			8
+
 #define TGL_NUM_IP_IGN_ALLOWED			22
 #define TGL_PMC_SLP_S0_RES_COUNTER_STEP		0x7A
 
@@ -199,8 +201,10 @@ enum ppfear_regs {
 /* Tigerlake Low Power Mode debug registers */
 #define TGL_LPM_STATUS_OFFSET			0x1C3C
 #define TGL_LPM_LIVE_STATUS_OFFSET		0x1C5C
+#define TGL_LPM_PRI_OFFSET			0x1C7C
+#define TGL_LPM_NUM_MAPS			6
 
-const char *tgl_lpm_modes[] = {
+const char *pmc_lpm_modes[] = {
 	"S0i2.0",
 	"S0i2.1",
 	"S0i2.2",
@@ -258,8 +262,9 @@ struct pmc_reg_map {
 	const u32 ltr_ignore_max;
 	const u32 pm_vric1_offset;
 	/* Low Power Mode registers */
-	const char **lpm_modes;
+	const int lpm_num_maps;
 	const u32 lpm_en_offset;
+	const u32 lpm_priority_offset;
 	const u32 lpm_residency_offset;
 	const u32 lpm_status_offset;
 	const u32 lpm_live_status_offset;
@@ -278,6 +283,8 @@ struct pmc_reg_map {
  * @check_counters:	On resume, check if counters are getting incremented
  * @pc10_counter:	PC10 residency counter
  * @s0ix_counter:	S0ix residency (step adjusted)
+ * @num_modes:		Count of enabled modes
+ * @lpm_en_modes:	Array of enabled modes from lowest to highest priority
  *
  * pmc_dev contains info about power management controller device.
  */
@@ -292,6 +299,13 @@ struct pmc_dev {
 	bool check_counters; /* Check for counter increments on resume */
 	u64 pc10_counter;
 	u64 s0ix_counter;
+	int num_modes;
+	int lpm_en_modes[LPM_MAX_NUM_MODES];
 };
 
+#define pmc_for_each_mode(i, mode, pmcdev)		\
+	for (i = 0, mode = pmcdev->lpm_en_modes[i];	\
+	     i < pmcdev->num_modes;			\
+	     i++, mode = pmcdev->lpm_en_modes[i])
+
 #endif /* PMC_CORE_H */
-- 
2.25.1

Re: [PATCH 3/9] platform/x86: intel_pmc_core: Handle sub-states generically

[Hans de Goede]

Hi,

On 4/1/21 5:05 AM, David E. Box wrote:
> From: Gayatri Kammela <gayatri.kammela@intel.com>
> 
> The current implementation of pmc_core_substate_res_show() is written
> specifically for Tiger Lake. However, new platforms will also have
> sub-states and may support different modes. Therefore rewrite the code to
> handle sub-states generically.
> 
> Read the number and type of enabled states from the PMC. Use the Low
> Power Mode (LPM) priority register to store the states in order from
> shallowest to deepest for displaying. Add a for_each macro to simplify
> this. While changing the sub-state display it makes sense to show only the
> "enabled" sub-states instead of showing all possible ones. After this
> patch, the debugfs file looks like this:
> 
> Substate   Residency
> S0i2.0     0
> S0i3.0     0
> S0i2.1     9329279
> S0i3.1     0
> S0i3.2     0
> 
> Suggested-by: David E. Box <david.e.box@linux.intel.com>
> Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>  drivers/platform/x86/intel_pmc_core.c | 59 ++++++++++++++++++++++-----
>  drivers/platform/x86/intel_pmc_core.h | 18 +++++++-
>  2 files changed, 64 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 5ca40fe3da59..ce300c2942d0 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -577,8 +577,9 @@ static const struct pmc_reg_map tgl_reg_map = {
>  	.pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
>  	.pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
>  	.ltr_ignore_max = TGL_NUM_IP_IGN_ALLOWED,
> -	.lpm_modes = tgl_lpm_modes,
> +	.lpm_num_maps = TGL_LPM_NUM_MAPS,
>  	.lpm_en_offset = TGL_LPM_EN_OFFSET,
> +	.lpm_priority_offset = TGL_LPM_PRI_OFFSET,
>  	.lpm_residency_offset = TGL_LPM_RESIDENCY_OFFSET,
>  	.lpm_sts = tgl_lpm_maps,
>  	.lpm_status_offset = TGL_LPM_STATUS_OFFSET,
> @@ -1028,18 +1029,14 @@ DEFINE_SHOW_ATTRIBUTE(pmc_core_ltr);
>  static int pmc_core_substate_res_show(struct seq_file *s, void *unused)
>  {
>  	struct pmc_dev *pmcdev = s->private;
> -	const char **lpm_modes = pmcdev->map->lpm_modes;
>  	u32 offset = pmcdev->map->lpm_residency_offset;
> -	u32 lpm_en;
> -	int index;
> +	int i, mode;
>  
> -	lpm_en = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_en_offset);
> -	seq_printf(s, "status substate residency\n");
> -	for (index = 0; lpm_modes[index]; index++) {
> -		seq_printf(s, "%7s %7s %-15u\n",
> -			   BIT(index) & lpm_en ? "Enabled" : " ",
> -			   lpm_modes[index], pmc_core_reg_read(pmcdev, offset));
> -		offset += 4;
> +	seq_printf(s, "%-10s %-15s\n", "Substate", "Residency");
> +
> +	pmc_for_each_mode(i, mode, pmcdev) {
> +		seq_printf(s, "%-10s %-15u\n", pmc_lpm_modes[mode],
> +			   pmc_core_reg_read(pmcdev, offset + (4 * mode)));
>  	}
>  
>  	return 0;
> @@ -1091,6 +1088,45 @@ static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_pkgc);
>  
> +static void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev)
> +{
> +	u8 lpm_priority[LPM_MAX_NUM_MODES];
> +	u32 lpm_en;
> +	int mode, i, p;
> +
> +	/* Use LPM Maps to indicate support for substates */
> +	if (!pmcdev->map->lpm_num_maps)
> +		return;
> +
> +	lpm_en = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_en_offset);
> +	pmcdev->num_modes = hweight32(lpm_en);
> +
> +	/* Each byte contains information for 2 modes (7:4 and 3:0) */
> +	for (mode = 0; mode < LPM_MAX_NUM_MODES; mode += 2) {
> +		u8 priority = pmc_core_reg_read_byte(pmcdev,
> +				pmcdev->map->lpm_priority_offset + (mode / 2));
> +		int pri0 = GENMASK(3, 0) & priority;
> +		int pri1 = (GENMASK(7, 4) & priority) >> 4;
> +
> +		lpm_priority[pri0] = mode;
> +		lpm_priority[pri1] = mode + 1;
> +	}
> +
> +	/*
> +	 * Loop though all modes from lowest to highest priority,
> +	 * and capture all enabled modes in order
> +	 */
> +	i = 0;
> +	for (p = LPM_MAX_NUM_MODES - 1; p >= 0; p--) {
> +		int mode = lpm_priority[p];
> +
> +		if (!(BIT(mode) & lpm_en))
> +			continue;
> +
> +		pmcdev->lpm_en_modes[i++] = mode;
> +	}
> +}
> +
>  static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
>  {
>  	debugfs_remove_recursive(pmcdev->dbgfs_dir);
> @@ -1267,6 +1303,7 @@ static int pmc_core_probe(struct platform_device *pdev)
>  
>  	mutex_init(&pmcdev->lock);
>  	pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
> +	pmc_core_get_low_power_modes(pmcdev);
>  	pmc_core_do_dmi_quirks(pmcdev);
>  
>  	/*
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index f33cd2c34835..5a4e3a49f5b1 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -187,6 +187,8 @@ enum ppfear_regs {
>  #define ICL_PMC_LTR_WIGIG			0x1BFC
>  #define ICL_PMC_SLP_S0_RES_COUNTER_STEP		0x64
>  
> +#define LPM_MAX_NUM_MODES			8
> +
>  #define TGL_NUM_IP_IGN_ALLOWED			22
>  #define TGL_PMC_SLP_S0_RES_COUNTER_STEP		0x7A
>  
> @@ -199,8 +201,10 @@ enum ppfear_regs {
>  /* Tigerlake Low Power Mode debug registers */
>  #define TGL_LPM_STATUS_OFFSET			0x1C3C
>  #define TGL_LPM_LIVE_STATUS_OFFSET		0x1C5C
> +#define TGL_LPM_PRI_OFFSET			0x1C7C
> +#define TGL_LPM_NUM_MAPS			6
>  
> -const char *tgl_lpm_modes[] = {
> +const char *pmc_lpm_modes[] = {
>  	"S0i2.0",
>  	"S0i2.1",
>  	"S0i2.2",
> @@ -258,8 +262,9 @@ struct pmc_reg_map {
>  	const u32 ltr_ignore_max;
>  	const u32 pm_vric1_offset;
>  	/* Low Power Mode registers */
> -	const char **lpm_modes;
> +	const int lpm_num_maps;
>  	const u32 lpm_en_offset;
> +	const u32 lpm_priority_offset;
>  	const u32 lpm_residency_offset;
>  	const u32 lpm_status_offset;
>  	const u32 lpm_live_status_offset;
> @@ -278,6 +283,8 @@ struct pmc_reg_map {
>   * @check_counters:	On resume, check if counters are getting incremented
>   * @pc10_counter:	PC10 residency counter
>   * @s0ix_counter:	S0ix residency (step adjusted)
> + * @num_modes:		Count of enabled modes
> + * @lpm_en_modes:	Array of enabled modes from lowest to highest priority
>   *
>   * pmc_dev contains info about power management controller device.
>   */
> @@ -292,6 +299,13 @@ struct pmc_dev {
>  	bool check_counters; /* Check for counter increments on resume */
>  	u64 pc10_counter;
>  	u64 s0ix_counter;
> +	int num_modes;
> +	int lpm_en_modes[LPM_MAX_NUM_MODES];
>  };
>  
> +#define pmc_for_each_mode(i, mode, pmcdev)		\
> +	for (i = 0, mode = pmcdev->lpm_en_modes[i];	\
> +	     i < pmcdev->num_modes;			\
> +	     i++, mode = pmcdev->lpm_en_modes[i])
> +
>  #endif /* PMC_CORE_H */
> 

Re: [PATCH 3/9] platform/x86: intel_pmc_core: Handle sub-states generically

[Rajneesh Bhardwaj]

A minor suggestion, num_modes should be called num_lpm_modes since
it's a pmcdev's property.

Acked-by: Rajneesh Bhardwaj <irenic.rajneesh@gmail.com>


On Wed, Mar 31, 2021 at 11:06 PM David E. Box
<david.e.box@linux.intel.com> wrote:
>
> From: Gayatri Kammela <gayatri.kammela@intel.com>
>
> The current implementation of pmc_core_substate_res_show() is written
> specifically for Tiger Lake. However, new platforms will also have
> sub-states and may support different modes. Therefore rewrite the code to
> handle sub-states generically.
>
> Read the number and type of enabled states from the PMC. Use the Low
> Power Mode (LPM) priority register to store the states in order from
> shallowest to deepest for displaying. Add a for_each macro to simplify
> this. While changing the sub-state display it makes sense to show only the
> "enabled" sub-states instead of showing all possible ones. After this
> patch, the debugfs file looks like this:
>
> Substate   Residency
> S0i2.0     0
> S0i3.0     0
> S0i2.1     9329279
> S0i3.1     0
> S0i3.2     0
>
> Suggested-by: David E. Box <david.e.box@linux.intel.com>
> Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 59 ++++++++++++++++++++++-----
>  drivers/platform/x86/intel_pmc_core.h | 18 +++++++-
>  2 files changed, 64 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 5ca40fe3da59..ce300c2942d0 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -577,8 +577,9 @@ static const struct pmc_reg_map tgl_reg_map = {
>         .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
>         .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
>         .ltr_ignore_max = TGL_NUM_IP_IGN_ALLOWED,
> -       .lpm_modes = tgl_lpm_modes,
> +       .lpm_num_maps = TGL_LPM_NUM_MAPS,
>         .lpm_en_offset = TGL_LPM_EN_OFFSET,
> +       .lpm_priority_offset = TGL_LPM_PRI_OFFSET,
>         .lpm_residency_offset = TGL_LPM_RESIDENCY_OFFSET,
>         .lpm_sts = tgl_lpm_maps,
>         .lpm_status_offset = TGL_LPM_STATUS_OFFSET,
> @@ -1028,18 +1029,14 @@ DEFINE_SHOW_ATTRIBUTE(pmc_core_ltr);
>  static int pmc_core_substate_res_show(struct seq_file *s, void *unused)
>  {
>         struct pmc_dev *pmcdev = s->private;
> -       const char **lpm_modes = pmcdev->map->lpm_modes;
>         u32 offset = pmcdev->map->lpm_residency_offset;
> -       u32 lpm_en;
> -       int index;
> +       int i, mode;
>
> -       lpm_en = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_en_offset);
> -       seq_printf(s, "status substate residency\n");
> -       for (index = 0; lpm_modes[index]; index++) {
> -               seq_printf(s, "%7s %7s %-15u\n",
> -                          BIT(index) & lpm_en ? "Enabled" : " ",
> -                          lpm_modes[index], pmc_core_reg_read(pmcdev, offset));
> -               offset += 4;
> +       seq_printf(s, "%-10s %-15s\n", "Substate", "Residency");
> +
> +       pmc_for_each_mode(i, mode, pmcdev) {
> +               seq_printf(s, "%-10s %-15u\n", pmc_lpm_modes[mode],
> +                          pmc_core_reg_read(pmcdev, offset + (4 * mode)));
>         }
>
>         return 0;
> @@ -1091,6 +1088,45 @@ static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_pkgc);
>
> +static void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev)
> +{
> +       u8 lpm_priority[LPM_MAX_NUM_MODES];
> +       u32 lpm_en;
> +       int mode, i, p;
> +
> +       /* Use LPM Maps to indicate support for substates */
> +       if (!pmcdev->map->lpm_num_maps)
> +               return;
> +
> +       lpm_en = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_en_offset);
> +       pmcdev->num_modes = hweight32(lpm_en);
> +
> +       /* Each byte contains information for 2 modes (7:4 and 3:0) */
> +       for (mode = 0; mode < LPM_MAX_NUM_MODES; mode += 2) {
> +               u8 priority = pmc_core_reg_read_byte(pmcdev,
> +                               pmcdev->map->lpm_priority_offset + (mode / 2));
> +               int pri0 = GENMASK(3, 0) & priority;
> +               int pri1 = (GENMASK(7, 4) & priority) >> 4;
> +
> +               lpm_priority[pri0] = mode;
> +               lpm_priority[pri1] = mode + 1;
> +       }
> +
> +       /*
> +        * Loop though all modes from lowest to highest priority,
> +        * and capture all enabled modes in order
> +        */
> +       i = 0;
> +       for (p = LPM_MAX_NUM_MODES - 1; p >= 0; p--) {
> +               int mode = lpm_priority[p];
> +
> +               if (!(BIT(mode) & lpm_en))
> +                       continue;
> +
> +               pmcdev->lpm_en_modes[i++] = mode;
> +       }
> +}
> +
>  static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
>  {
>         debugfs_remove_recursive(pmcdev->dbgfs_dir);
> @@ -1267,6 +1303,7 @@ static int pmc_core_probe(struct platform_device *pdev)
>
>         mutex_init(&pmcdev->lock);
>         pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
> +       pmc_core_get_low_power_modes(pmcdev);
>         pmc_core_do_dmi_quirks(pmcdev);
>
>         /*
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index f33cd2c34835..5a4e3a49f5b1 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -187,6 +187,8 @@ enum ppfear_regs {
>  #define ICL_PMC_LTR_WIGIG                      0x1BFC
>  #define ICL_PMC_SLP_S0_RES_COUNTER_STEP                0x64
>
> +#define LPM_MAX_NUM_MODES                      8
> +
>  #define TGL_NUM_IP_IGN_ALLOWED                 22
>  #define TGL_PMC_SLP_S0_RES_COUNTER_STEP                0x7A
>
> @@ -199,8 +201,10 @@ enum ppfear_regs {
>  /* Tigerlake Low Power Mode debug registers */
>  #define TGL_LPM_STATUS_OFFSET                  0x1C3C
>  #define TGL_LPM_LIVE_STATUS_OFFSET             0x1C5C
> +#define TGL_LPM_PRI_OFFSET                     0x1C7C
> +#define TGL_LPM_NUM_MAPS                       6
>
> -const char *tgl_lpm_modes[] = {
> +const char *pmc_lpm_modes[] = {
>         "S0i2.0",
>         "S0i2.1",
>         "S0i2.2",
> @@ -258,8 +262,9 @@ struct pmc_reg_map {
>         const u32 ltr_ignore_max;
>         const u32 pm_vric1_offset;
>         /* Low Power Mode registers */
> -       const char **lpm_modes;
> +       const int lpm_num_maps;
>         const u32 lpm_en_offset;
> +       const u32 lpm_priority_offset;
>         const u32 lpm_residency_offset;
>         const u32 lpm_status_offset;
>         const u32 lpm_live_status_offset;
> @@ -278,6 +283,8 @@ struct pmc_reg_map {
>   * @check_counters:    On resume, check if counters are getting incremented
>   * @pc10_counter:      PC10 residency counter
>   * @s0ix_counter:      S0ix residency (step adjusted)
> + * @num_modes:         Count of enabled modes
> + * @lpm_en_modes:      Array of enabled modes from lowest to highest priority
>   *
>   * pmc_dev contains info about power management controller device.
>   */
> @@ -292,6 +299,13 @@ struct pmc_dev {
>         bool check_counters; /* Check for counter increments on resume */
>         u64 pc10_counter;
>         u64 s0ix_counter;
> +       int num_modes;
> +       int lpm_en_modes[LPM_MAX_NUM_MODES];
>  };
>
> +#define pmc_for_each_mode(i, mode, pmcdev)             \
> +       for (i = 0, mode = pmcdev->lpm_en_modes[i];     \
> +            i < pmcdev->num_modes;                     \
> +            i++, mode = pmcdev->lpm_en_modes[i])
> +
>  #endif /* PMC_CORE_H */
> --
> 2.25.1
>


-- 
Thanks,
Rajneesh

[PATCH 4/9] platform/x86: intel_pmc_core: Show LPM residency in microseconds

[David E. Box]

From: Gayatri Kammela <gayatri.kammela@intel.com>

Modify the low power mode (LPM or sub-state) residency counters to display
in microseconds just like the slp_s0_residency counter. The granularity of
the counter is approximately 30.5us per tick. Double this value then divide
by two to maintain accuracy.

Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_core.c | 14 ++++++++++++--
 drivers/platform/x86/intel_pmc_core.h |  3 +++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index ce300c2942d0..ba0db301f07b 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -578,6 +578,7 @@ static const struct pmc_reg_map tgl_reg_map = {
 	.pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
 	.ltr_ignore_max = TGL_NUM_IP_IGN_ALLOWED,
 	.lpm_num_maps = TGL_LPM_NUM_MAPS,
+	.lpm_res_counter_step_x2 = TGL_PMC_LPM_RES_COUNTER_STEP_X2,
 	.lpm_en_offset = TGL_LPM_EN_OFFSET,
 	.lpm_priority_offset = TGL_LPM_PRI_OFFSET,
 	.lpm_residency_offset = TGL_LPM_RESIDENCY_OFFSET,
@@ -1026,17 +1027,26 @@ static int pmc_core_ltr_show(struct seq_file *s, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(pmc_core_ltr);
 
+static inline u64 adjust_lpm_residency(struct pmc_dev *pmcdev, u32 offset,
+				       const int lpm_adj_x2)
+{
+	u64 lpm_res = pmc_core_reg_read(pmcdev, offset);
+
+	return GET_X2_COUNTER((u64)lpm_adj_x2 * lpm_res);
+}
+
 static int pmc_core_substate_res_show(struct seq_file *s, void *unused)
 {
 	struct pmc_dev *pmcdev = s->private;
+	const int lpm_adj_x2 = pmcdev->map->lpm_res_counter_step_x2;
 	u32 offset = pmcdev->map->lpm_residency_offset;
 	int i, mode;
 
 	seq_printf(s, "%-10s %-15s\n", "Substate", "Residency");
 
 	pmc_for_each_mode(i, mode, pmcdev) {
-		seq_printf(s, "%-10s %-15u\n", pmc_lpm_modes[mode],
-			   pmc_core_reg_read(pmcdev, offset + (4 * mode)));
+		seq_printf(s, "%-10s %-15llu\n", pmc_lpm_modes[mode],
+			   adjust_lpm_residency(pmcdev, offset + (4 * mode), lpm_adj_x2));
 	}
 
 	return 0;
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 5a4e3a49f5b1..3800c1ba6fb7 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -188,9 +188,11 @@ enum ppfear_regs {
 #define ICL_PMC_SLP_S0_RES_COUNTER_STEP		0x64
 
 #define LPM_MAX_NUM_MODES			8
+#define GET_X2_COUNTER(v)			((v) >> 1)
 
 #define TGL_NUM_IP_IGN_ALLOWED			22
 #define TGL_PMC_SLP_S0_RES_COUNTER_STEP		0x7A
+#define TGL_PMC_LPM_RES_COUNTER_STEP_X2		61	/* 30.5us * 2 */
 
 /*
  * Tigerlake Power Management Controller register offsets
@@ -263,6 +265,7 @@ struct pmc_reg_map {
 	const u32 pm_vric1_offset;
 	/* Low Power Mode registers */
 	const int lpm_num_maps;
+	const int lpm_res_counter_step_x2;
 	const u32 lpm_en_offset;
 	const u32 lpm_priority_offset;
 	const u32 lpm_residency_offset;
-- 
2.25.1

Re: [PATCH 4/9] platform/x86: intel_pmc_core: Show LPM residency in microseconds

[Hans de Goede]

Hi,

On 4/1/21 5:05 AM, David E. Box wrote:
> From: Gayatri Kammela <gayatri.kammela@intel.com>
> 
> Modify the low power mode (LPM or sub-state) residency counters to display
> in microseconds just like the slp_s0_residency counter. The granularity of
> the counter is approximately 30.5us per tick. Double this value then divide
> by two to maintain accuracy.
> 
> Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>  drivers/platform/x86/intel_pmc_core.c | 14 ++++++++++++--
>  drivers/platform/x86/intel_pmc_core.h |  3 +++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index ce300c2942d0..ba0db301f07b 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -578,6 +578,7 @@ static const struct pmc_reg_map tgl_reg_map = {
>  	.pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
>  	.ltr_ignore_max = TGL_NUM_IP_IGN_ALLOWED,
>  	.lpm_num_maps = TGL_LPM_NUM_MAPS,
> +	.lpm_res_counter_step_x2 = TGL_PMC_LPM_RES_COUNTER_STEP_X2,
>  	.lpm_en_offset = TGL_LPM_EN_OFFSET,
>  	.lpm_priority_offset = TGL_LPM_PRI_OFFSET,
>  	.lpm_residency_offset = TGL_LPM_RESIDENCY_OFFSET,
> @@ -1026,17 +1027,26 @@ static int pmc_core_ltr_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_ltr);
>  
> +static inline u64 adjust_lpm_residency(struct pmc_dev *pmcdev, u32 offset,
> +				       const int lpm_adj_x2)
> +{
> +	u64 lpm_res = pmc_core_reg_read(pmcdev, offset);
> +
> +	return GET_X2_COUNTER((u64)lpm_adj_x2 * lpm_res);
> +}
> +
>  static int pmc_core_substate_res_show(struct seq_file *s, void *unused)
>  {
>  	struct pmc_dev *pmcdev = s->private;
> +	const int lpm_adj_x2 = pmcdev->map->lpm_res_counter_step_x2;
>  	u32 offset = pmcdev->map->lpm_residency_offset;
>  	int i, mode;
>  
>  	seq_printf(s, "%-10s %-15s\n", "Substate", "Residency");
>  
>  	pmc_for_each_mode(i, mode, pmcdev) {
> -		seq_printf(s, "%-10s %-15u\n", pmc_lpm_modes[mode],
> -			   pmc_core_reg_read(pmcdev, offset + (4 * mode)));
> +		seq_printf(s, "%-10s %-15llu\n", pmc_lpm_modes[mode],
> +			   adjust_lpm_residency(pmcdev, offset + (4 * mode), lpm_adj_x2));
>  	}
>  
>  	return 0;
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index 5a4e3a49f5b1..3800c1ba6fb7 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -188,9 +188,11 @@ enum ppfear_regs {
>  #define ICL_PMC_SLP_S0_RES_COUNTER_STEP		0x64
>  
>  #define LPM_MAX_NUM_MODES			8
> +#define GET_X2_COUNTER(v)			((v) >> 1)
>  
>  #define TGL_NUM_IP_IGN_ALLOWED			22
>  #define TGL_PMC_SLP_S0_RES_COUNTER_STEP		0x7A
> +#define TGL_PMC_LPM_RES_COUNTER_STEP_X2		61	/* 30.5us * 2 */
>  
>  /*
>   * Tigerlake Power Management Controller register offsets
> @@ -263,6 +265,7 @@ struct pmc_reg_map {
>  	const u32 pm_vric1_offset;
>  	/* Low Power Mode registers */
>  	const int lpm_num_maps;
> +	const int lpm_res_counter_step_x2;
>  	const u32 lpm_en_offset;
>  	const u32 lpm_priority_offset;
>  	const u32 lpm_residency_offset;
> 

Re: [PATCH 4/9] platform/x86: intel_pmc_core: Show LPM residency in microseconds

[Rajneesh Bhardwaj]

Reviewed-by: Rajneesh Bhardwaj <irenic.rajneesh@gmail.com>

On Wed, Mar 31, 2021 at 11:06 PM David E. Box
<david.e.box@linux.intel.com> wrote:
>
> From: Gayatri Kammela <gayatri.kammela@intel.com>
>
> Modify the low power mode (LPM or sub-state) residency counters to display
> in microseconds just like the slp_s0_residency counter. The granularity of
> the counter is approximately 30.5us per tick. Double this value then divide
> by two to maintain accuracy.
>
> Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 14 ++++++++++++--
>  drivers/platform/x86/intel_pmc_core.h |  3 +++
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index ce300c2942d0..ba0db301f07b 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -578,6 +578,7 @@ static const struct pmc_reg_map tgl_reg_map = {
>         .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
>         .ltr_ignore_max = TGL_NUM_IP_IGN_ALLOWED,
>         .lpm_num_maps = TGL_LPM_NUM_MAPS,
> +       .lpm_res_counter_step_x2 = TGL_PMC_LPM_RES_COUNTER_STEP_X2,
>         .lpm_en_offset = TGL_LPM_EN_OFFSET,
>         .lpm_priority_offset = TGL_LPM_PRI_OFFSET,
>         .lpm_residency_offset = TGL_LPM_RESIDENCY_OFFSET,
> @@ -1026,17 +1027,26 @@ static int pmc_core_ltr_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_ltr);
>
> +static inline u64 adjust_lpm_residency(struct pmc_dev *pmcdev, u32 offset,
> +                                      const int lpm_adj_x2)
> +{
> +       u64 lpm_res = pmc_core_reg_read(pmcdev, offset);
> +
> +       return GET_X2_COUNTER((u64)lpm_adj_x2 * lpm_res);
> +}
> +
>  static int pmc_core_substate_res_show(struct seq_file *s, void *unused)
>  {
>         struct pmc_dev *pmcdev = s->private;
> +       const int lpm_adj_x2 = pmcdev->map->lpm_res_counter_step_x2;
>         u32 offset = pmcdev->map->lpm_residency_offset;
>         int i, mode;
>
>         seq_printf(s, "%-10s %-15s\n", "Substate", "Residency");
>
>         pmc_for_each_mode(i, mode, pmcdev) {
> -               seq_printf(s, "%-10s %-15u\n", pmc_lpm_modes[mode],
> -                          pmc_core_reg_read(pmcdev, offset + (4 * mode)));
> +               seq_printf(s, "%-10s %-15llu\n", pmc_lpm_modes[mode],
> +                          adjust_lpm_residency(pmcdev, offset + (4 * mode), lpm_adj_x2));
>         }
>
>         return 0;
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index 5a4e3a49f5b1..3800c1ba6fb7 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -188,9 +188,11 @@ enum ppfear_regs {
>  #define ICL_PMC_SLP_S0_RES_COUNTER_STEP                0x64
>
>  #define LPM_MAX_NUM_MODES                      8
> +#define GET_X2_COUNTER(v)                      ((v) >> 1)
>
>  #define TGL_NUM_IP_IGN_ALLOWED                 22
>  #define TGL_PMC_SLP_S0_RES_COUNTER_STEP                0x7A
> +#define TGL_PMC_LPM_RES_COUNTER_STEP_X2                61      /* 30.5us * 2 */
>
>  /*
>   * Tigerlake Power Management Controller register offsets
> @@ -263,6 +265,7 @@ struct pmc_reg_map {
>         const u32 pm_vric1_offset;
>         /* Low Power Mode registers */
>         const int lpm_num_maps;
> +       const int lpm_res_counter_step_x2;
>         const u32 lpm_en_offset;
>         const u32 lpm_priority_offset;
>         const u32 lpm_residency_offset;
> --
> 2.25.1
>


-- 
Thanks,
Rajneesh

[PATCH 5/9] platform/x86: intel_pmc_core: Get LPM requirements for Tiger Lake

[David E. Box]

From: Gayatri Kammela <gayatri.kammela@intel.com>

Platforms that support low power modes (LPM) such as Tiger Lake maintain
requirements for each sub-state that a readable in the PMC. However, unlike
LPM status registers, requirement registers are not memory mapped but are
available from an ACPI _DSM. Collect the requirements for Tiger Lake using
the _DSM method and store in a buffer.

Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
Co-developed-by: David E. Box <david.e.box@linux.intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_core.c | 49 +++++++++++++++++++++++++++
 drivers/platform/x86/intel_pmc_core.h |  2 ++
 2 files changed, 51 insertions(+)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index ba0db301f07b..0ec26a4c715e 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -23,7 +23,9 @@
 #include <linux/slab.h>
 #include <linux/suspend.h>
 #include <linux/uaccess.h>
+#include <linux/uuid.h>
 
+#include <acpi/acpi_bus.h>
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
 #include <asm/msr.h>
@@ -31,6 +33,9 @@
 
 #include "intel_pmc_core.h"
 
+#define ACPI_S0IX_DSM_UUID		"57a6512e-3979-4e9d-9708-ff13b2508972"
+#define ACPI_GET_LOW_MODE_REGISTERS	1
+
 /* PKGC MSRs are common across Intel Core SoCs */
 static const struct pmc_bit_map msr_map[] = {
 	{"Package C2",                  MSR_PKG_C2_RESIDENCY},
@@ -587,6 +592,46 @@ static const struct pmc_reg_map tgl_reg_map = {
 	.lpm_live_status_offset = TGL_LPM_LIVE_STATUS_OFFSET,
 };
 
+static void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev)
+{
+	struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
+	const int num_maps = pmcdev->map->lpm_num_maps;
+	size_t lpm_size = LPM_MAX_NUM_MODES * num_maps * 4;
+	union acpi_object *out_obj;
+	struct acpi_device *adev;
+	guid_t s0ix_dsm_guid;
+	u32 *lpm_req_regs;
+
+	adev = ACPI_COMPANION(&pdev->dev);
+	if (!adev)
+		return;
+
+	lpm_req_regs = devm_kzalloc(&pdev->dev, lpm_size * sizeof(u32),
+				     GFP_KERNEL);
+	if (!lpm_req_regs)
+		return;
+
+	guid_parse(ACPI_S0IX_DSM_UUID, &s0ix_dsm_guid);
+
+	out_obj = acpi_evaluate_dsm(adev->handle, &s0ix_dsm_guid, 0,
+				    ACPI_GET_LOW_MODE_REGISTERS, NULL);
+	if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
+		u32 *addr = (u32 *)out_obj->buffer.pointer;
+		int size = out_obj->buffer.length;
+
+		if (size != lpm_size)
+			return;
+
+		memcpy_fromio(lpm_req_regs, addr, lpm_size);
+	} else
+		acpi_handle_debug(adev->handle,
+				  "_DSM function 0 evaluation failed\n");
+
+	ACPI_FREE(out_obj);
+
+	pmcdev->lpm_req_regs = lpm_req_regs;
+}
+
 static inline u32 pmc_core_reg_read(struct pmc_dev *pmcdev, int reg_offset)
 {
 	return readl(pmcdev->regbase + reg_offset);
@@ -1312,10 +1357,14 @@ static int pmc_core_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	mutex_init(&pmcdev->lock);
+
 	pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
 	pmc_core_get_low_power_modes(pmcdev);
 	pmc_core_do_dmi_quirks(pmcdev);
 
+	if (pmcdev->map == &tgl_reg_map)
+		pmc_core_get_tgl_lpm_reqs(pdev);
+
 	/*
 	 * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when
 	 * a cable is attached. Tell the PMC to ignore it.
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 3800c1ba6fb7..81d797feed33 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -288,6 +288,7 @@ struct pmc_reg_map {
  * @s0ix_counter:	S0ix residency (step adjusted)
  * @num_modes:		Count of enabled modes
  * @lpm_en_modes:	Array of enabled modes from lowest to highest priority
+ * @lpm_req_regs:	List of substate requirements
  *
  * pmc_dev contains info about power management controller device.
  */
@@ -304,6 +305,7 @@ struct pmc_dev {
 	u64 s0ix_counter;
 	int num_modes;
 	int lpm_en_modes[LPM_MAX_NUM_MODES];
+	u32 *lpm_req_regs;
 };
 
 #define pmc_for_each_mode(i, mode, pmcdev)		\
-- 
2.25.1

Re: [PATCH 5/9] platform/x86: intel_pmc_core: Get LPM requirements for Tiger Lake

[Hans de Goede]

Hi,

On 4/1/21 5:05 AM, David E. Box wrote:
> From: Gayatri Kammela <gayatri.kammela@intel.com>
> 
> Platforms that support low power modes (LPM) such as Tiger Lake maintain
> requirements for each sub-state that a readable in the PMC. However, unlike
> LPM status registers, requirement registers are not memory mapped but are
> available from an ACPI _DSM. Collect the requirements for Tiger Lake using
> the _DSM method and store in a buffer.
> 
> Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
> Co-developed-by: David E. Box <david.e.box@linux.intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 49 +++++++++++++++++++++++++++
>  drivers/platform/x86/intel_pmc_core.h |  2 ++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index ba0db301f07b..0ec26a4c715e 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -23,7 +23,9 @@
>  #include <linux/slab.h>
>  #include <linux/suspend.h>
>  #include <linux/uaccess.h>
> +#include <linux/uuid.h>
>  
> +#include <acpi/acpi_bus.h>
>  #include <asm/cpu_device_id.h>
>  #include <asm/intel-family.h>
>  #include <asm/msr.h>
> @@ -31,6 +33,9 @@
>  
>  #include "intel_pmc_core.h"
>  
> +#define ACPI_S0IX_DSM_UUID		"57a6512e-3979-4e9d-9708-ff13b2508972"
> +#define ACPI_GET_LOW_MODE_REGISTERS	1
> +
>  /* PKGC MSRs are common across Intel Core SoCs */
>  static const struct pmc_bit_map msr_map[] = {
>  	{"Package C2",                  MSR_PKG_C2_RESIDENCY},
> @@ -587,6 +592,46 @@ static const struct pmc_reg_map tgl_reg_map = {
>  	.lpm_live_status_offset = TGL_LPM_LIVE_STATUS_OFFSET,
>  };
>  
> +static void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev)
> +{
> +	struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> +	const int num_maps = pmcdev->map->lpm_num_maps;
> +	size_t lpm_size = LPM_MAX_NUM_MODES * num_maps * 4;
> +	union acpi_object *out_obj;
> +	struct acpi_device *adev;
> +	guid_t s0ix_dsm_guid;
> +	u32 *lpm_req_regs;
> +
> +	adev = ACPI_COMPANION(&pdev->dev);
> +	if (!adev)
> +		return;
> +
> +	lpm_req_regs = devm_kzalloc(&pdev->dev, lpm_size * sizeof(u32),
> +				     GFP_KERNEL);
> +	if (!lpm_req_regs)
> +		return;
> +
> +	guid_parse(ACPI_S0IX_DSM_UUID, &s0ix_dsm_guid);
> +
> +	out_obj = acpi_evaluate_dsm(adev->handle, &s0ix_dsm_guid, 0,
> +				    ACPI_GET_LOW_MODE_REGISTERS, NULL);

Since you are using ACPI functions here, maybe change:

	depends on PCI

In the config INTEL_PMC_CORE Kconfig entry to:

	depends on PCI && ACPI

Note all functions you use are stubbed when !ACPI, so this
should build fine without this, but it will turn this function
into a no-op. If you prefer not to add the depends on ACPI that
is fine too.



> +	if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
> +		u32 *addr = (u32 *)out_obj->buffer.pointer;
> +		int size = out_obj->buffer.length;
> +
> +		if (size != lpm_size)

	You're leaking lpm_req_regs here (sort of) maybe devm_free it here ?

> +			return;
> +
> +		memcpy_fromio(lpm_req_regs, addr, lpm_size);

This is wrong, the memory in an ACPI buffer is not IO-mem it is normal memory.

> +	} else
> +		acpi_handle_debug(adev->handle,
> +				  "_DSM function 0 evaluation failed\n");
> +
> +	ACPI_FREE(out_obj);
> +
> +	pmcdev->lpm_req_regs = lpm_req_regs;

You do this even if the "if (out_obj && out_obj->type == ACPI_TYPE_BUFFER)"
check above failed, making pmcdev->lpm_req_regs point to a block of
memory filled with zeros. That does not seem right.

> +}
> +
>  static inline u32 pmc_core_reg_read(struct pmc_dev *pmcdev, int reg_offset)
>  {
>  	return readl(pmcdev->regbase + reg_offset);
> @@ -1312,10 +1357,14 @@ static int pmc_core_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	mutex_init(&pmcdev->lock);
> +
>  	pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
>  	pmc_core_get_low_power_modes(pmcdev);
>  	pmc_core_do_dmi_quirks(pmcdev);
>  
> +	if (pmcdev->map == &tgl_reg_map)
> +		pmc_core_get_tgl_lpm_reqs(pdev);
> +
>  	/*
>  	 * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when
>  	 * a cable is attached. Tell the PMC to ignore it.
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index 3800c1ba6fb7..81d797feed33 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -288,6 +288,7 @@ struct pmc_reg_map {
>   * @s0ix_counter:	S0ix residency (step adjusted)
>   * @num_modes:		Count of enabled modes
>   * @lpm_en_modes:	Array of enabled modes from lowest to highest priority
> + * @lpm_req_regs:	List of substate requirements
>   *
>   * pmc_dev contains info about power management controller device.
>   */
> @@ -304,6 +305,7 @@ struct pmc_dev {
>  	u64 s0ix_counter;
>  	int num_modes;
>  	int lpm_en_modes[LPM_MAX_NUM_MODES];
> +	u32 *lpm_req_regs;
>  };
>  
>  #define pmc_for_each_mode(i, mode, pmcdev)		\
> 

Regards,

Hams

Re: [PATCH 5/9] platform/x86: intel_pmc_core: Get LPM requirements for Tiger Lake

[Rajneesh Bhardwaj]

On Wed, Mar 31, 2021 at 11:06 PM David E. Box
<david.e.box@linux.intel.com> wrote:
>
> From: Gayatri Kammela <gayatri.kammela@intel.com>
>
> Platforms that support low power modes (LPM) such as Tiger Lake maintain
> requirements for each sub-state that a readable in the PMC. However, unlike
> LPM status registers, requirement registers are not memory mapped but are
> available from an ACPI _DSM. Collect the requirements for Tiger Lake using
> the _DSM method and store in a buffer.
>
> Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
> Co-developed-by: David E. Box <david.e.box@linux.intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 49 +++++++++++++++++++++++++++
>  drivers/platform/x86/intel_pmc_core.h |  2 ++
>  2 files changed, 51 insertions(+)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index ba0db301f07b..0ec26a4c715e 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -23,7 +23,9 @@
>  #include <linux/slab.h>
>  #include <linux/suspend.h>
>  #include <linux/uaccess.h>
> +#include <linux/uuid.h>
>
> +#include <acpi/acpi_bus.h>
>  #include <asm/cpu_device_id.h>
>  #include <asm/intel-family.h>
>  #include <asm/msr.h>
> @@ -31,6 +33,9 @@
>
>  #include "intel_pmc_core.h"
>
> +#define ACPI_S0IX_DSM_UUID             "57a6512e-3979-4e9d-9708-ff13b2508972"
> +#define ACPI_GET_LOW_MODE_REGISTERS    1
> +
>  /* PKGC MSRs are common across Intel Core SoCs */
>  static const struct pmc_bit_map msr_map[] = {
>         {"Package C2",                  MSR_PKG_C2_RESIDENCY},
> @@ -587,6 +592,46 @@ static const struct pmc_reg_map tgl_reg_map = {
>         .lpm_live_status_offset = TGL_LPM_LIVE_STATUS_OFFSET,
>  };
>
> +static void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev)
> +{
> +       struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> +       const int num_maps = pmcdev->map->lpm_num_maps;
> +       size_t lpm_size = LPM_MAX_NUM_MODES * num_maps * 4;
> +       union acpi_object *out_obj;
> +       struct acpi_device *adev;
> +       guid_t s0ix_dsm_guid;
> +       u32 *lpm_req_regs;
> +
> +       adev = ACPI_COMPANION(&pdev->dev);
> +       if (!adev)
> +               return;
> +
> +       lpm_req_regs = devm_kzalloc(&pdev->dev, lpm_size * sizeof(u32),
> +                                    GFP_KERNEL);
> +       if (!lpm_req_regs)
> +               return;
> +
> +       guid_parse(ACPI_S0IX_DSM_UUID, &s0ix_dsm_guid);
> +
> +       out_obj = acpi_evaluate_dsm(adev->handle, &s0ix_dsm_guid, 0,
> +                                   ACPI_GET_LOW_MODE_REGISTERS, NULL);
> +       if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
> +               u32 *addr = (u32 *)out_obj->buffer.pointer;
> +               int size = out_obj->buffer.length;
> +
> +               if (size != lpm_size)
> +                       return;
> +
> +               memcpy_fromio(lpm_req_regs, addr, lpm_size);

this is not __iomem* so why are you using this? Simple memcpy should work here.

> +       } else
> +               acpi_handle_debug(adev->handle,
> +                                 "_DSM function 0 evaluation failed\n");
> +
> +       ACPI_FREE(out_obj);
> +
> +       pmcdev->lpm_req_regs = lpm_req_regs;
> +}
> +
>  static inline u32 pmc_core_reg_read(struct pmc_dev *pmcdev, int reg_offset)
>  {
>         return readl(pmcdev->regbase + reg_offset);
> @@ -1312,10 +1357,14 @@ static int pmc_core_probe(struct platform_device *pdev)
>                 return -ENOMEM;
>
>         mutex_init(&pmcdev->lock);
> +
>         pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
>         pmc_core_get_low_power_modes(pmcdev);
>         pmc_core_do_dmi_quirks(pmcdev);
>
> +       if (pmcdev->map == &tgl_reg_map)
> +               pmc_core_get_tgl_lpm_reqs(pdev);
> +
>         /*
>          * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when
>          * a cable is attached. Tell the PMC to ignore it.
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index 3800c1ba6fb7..81d797feed33 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -288,6 +288,7 @@ struct pmc_reg_map {
>   * @s0ix_counter:      S0ix residency (step adjusted)
>   * @num_modes:         Count of enabled modes
>   * @lpm_en_modes:      Array of enabled modes from lowest to highest priority
> + * @lpm_req_regs:      List of substate requirements
>   *
>   * pmc_dev contains info about power management controller device.
>   */
> @@ -304,6 +305,7 @@ struct pmc_dev {
>         u64 s0ix_counter;
>         int num_modes;
>         int lpm_en_modes[LPM_MAX_NUM_MODES];
> +       u32 *lpm_req_regs;
>  };
>
>  #define pmc_for_each_mode(i, mode, pmcdev)             \
> --
> 2.25.1
>


-- 
Thanks,
Rajneesh

[PATCH 6/9] platform/x86: intel_pmc_core: Add requirements file to debugfs

[David E. Box]

From: Gayatri Kammela <gayatri.kammela@intel.com>

Add the debugfs file, substate_requirements, to view the low power mode
(LPM) requirements for each enabled mode alongside the last latched status
of the condition.

After this patch, the new file will look like this:

                    Element |    S0i2.0 |    S0i3.0 |    S0i2.1 |    S0i3.1 |    S0i3.2 |    Status |
            USB2PLL_OFF_STS |  Required |  Required |  Required |  Required |  Required |           |
PCIe/USB3.1_Gen2PLL_OFF_STS |  Required |  Required |  Required |  Required |  Required |           |
       PCIe_Gen3PLL_OFF_STS |  Required |  Required |  Required |  Required |  Required |       Yes |
            OPIOPLL_OFF_STS |  Required |  Required |  Required |  Required |  Required |       Yes |
              OCPLL_OFF_STS |  Required |  Required |  Required |  Required |  Required |       Yes |
            MainPLL_OFF_STS |           |  Required |           |  Required |  Required |           |

Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
Co-developed-by: David E. Box <david.e.box@linux.intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_core.c | 86 +++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 0ec26a4c715e..0b47a1da5f49 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -1122,6 +1122,86 @@ static int pmc_core_substate_l_sts_regs_show(struct seq_file *s, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_l_sts_regs);
 
+static void pmc_core_substate_req_header_show(struct seq_file *s)
+{
+	struct pmc_dev *pmcdev = s->private;
+	int i, mode;
+
+	seq_printf(s, "%30s |", "Element");
+	pmc_for_each_mode(i, mode, pmcdev)
+		seq_printf(s, " %9s |", pmc_lpm_modes[mode]);
+
+	seq_printf(s, " %9s |\n", "Status");
+}
+
+static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
+{
+	struct pmc_dev *pmcdev = s->private;
+	const struct pmc_bit_map **maps = pmcdev->map->lpm_sts;
+	const struct pmc_bit_map *map;
+	const int num_maps = pmcdev->map->lpm_num_maps;
+	u32 sts_offset = pmcdev->map->lpm_status_offset;
+	u32 *lpm_req_regs = pmcdev->lpm_req_regs;
+	int mp;
+
+	/* Display the header */
+	pmc_core_substate_req_header_show(s);
+
+	/* Loop over maps */
+	for (mp = 0; mp < num_maps; mp++) {
+		u32 req_mask = 0;
+		u32 lpm_status;
+		int mode, idx, i, len = 32;
+
+		/*
+		 * Capture the requirements and create a mask so that we only
+		 * show an element if it's required for at least one of the
+		 * enabled low power modes
+		 */
+		pmc_for_each_mode(idx, mode, pmcdev)
+			req_mask |= lpm_req_regs[mp + (mode * num_maps)];
+
+		/* Get the last latched status for this map */
+		lpm_status = pmc_core_reg_read(pmcdev, sts_offset + (mp * 4));
+
+		/*  Loop over elements in this map */
+		map = maps[mp];
+		for (i = 0; map[i].name && i < len; i++) {
+			u32 bit_mask = map[i].bit_mask;
+
+			if (!(bit_mask & req_mask))
+				/*
+				 * Not required for any enabled states
+				 * so don't display
+				 */
+				continue;
+
+			/* Display the element name in the first column */
+			seq_printf(s, "%30s |", map[i].name);
+
+			/* Loop over the enabled states and display if required */
+			pmc_for_each_mode(idx, mode, pmcdev) {
+				if (lpm_req_regs[mp + (mode * num_maps)] & bit_mask)
+					seq_printf(s, " %9s |",
+						   "Required");
+				else
+					seq_printf(s, " %9s |", " ");
+			}
+
+			/* In Status column, show the last captured state of this agent */
+			if (lpm_status & bit_mask)
+				seq_printf(s, " %9s |", "Yes");
+			else
+				seq_printf(s, " %9s |", " ");
+
+			seq_puts(s, "\n");
+		}
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_req_regs);
+
 static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
 {
 	struct pmc_dev *pmcdev = s->private;
@@ -1241,6 +1321,12 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
 				    pmcdev->dbgfs_dir, pmcdev,
 				    &pmc_core_substate_l_sts_regs_fops);
 	}
+
+	if (pmcdev->lpm_req_regs) {
+		debugfs_create_file("substate_requirements", 0444,
+				    pmcdev->dbgfs_dir, pmcdev,
+				    &pmc_core_substate_req_regs_fops);
+	}
 }
 
 static const struct x86_cpu_id intel_pmc_core_ids[] = {
-- 
2.25.1

Re: [PATCH 6/9] platform/x86: intel_pmc_core: Add requirements file to debugfs

[Hans de Goede]

Hi,

On 4/1/21 5:05 AM, David E. Box wrote:
> From: Gayatri Kammela <gayatri.kammela@intel.com>
> 
> Add the debugfs file, substate_requirements, to view the low power mode
> (LPM) requirements for each enabled mode alongside the last latched status
> of the condition.
> 
> After this patch, the new file will look like this:
> 
>                     Element |    S0i2.0 |    S0i3.0 |    S0i2.1 |    S0i3.1 |    S0i3.2 |    Status |
>             USB2PLL_OFF_STS |  Required |  Required |  Required |  Required |  Required |           |
> PCIe/USB3.1_Gen2PLL_OFF_STS |  Required |  Required |  Required |  Required |  Required |           |
>        PCIe_Gen3PLL_OFF_STS |  Required |  Required |  Required |  Required |  Required |       Yes |
>             OPIOPLL_OFF_STS |  Required |  Required |  Required |  Required |  Required |       Yes |
>               OCPLL_OFF_STS |  Required |  Required |  Required |  Required |  Required |       Yes |
>             MainPLL_OFF_STS |           |  Required |           |  Required |  Required |           |
> 
> Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
> Co-developed-by: David E. Box <david.e.box@linux.intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>  drivers/platform/x86/intel_pmc_core.c | 86 +++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 0ec26a4c715e..0b47a1da5f49 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -1122,6 +1122,86 @@ static int pmc_core_substate_l_sts_regs_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_l_sts_regs);
>  
> +static void pmc_core_substate_req_header_show(struct seq_file *s)
> +{
> +	struct pmc_dev *pmcdev = s->private;
> +	int i, mode;
> +
> +	seq_printf(s, "%30s |", "Element");
> +	pmc_for_each_mode(i, mode, pmcdev)
> +		seq_printf(s, " %9s |", pmc_lpm_modes[mode]);
> +
> +	seq_printf(s, " %9s |\n", "Status");
> +}
> +
> +static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
> +{
> +	struct pmc_dev *pmcdev = s->private;
> +	const struct pmc_bit_map **maps = pmcdev->map->lpm_sts;
> +	const struct pmc_bit_map *map;
> +	const int num_maps = pmcdev->map->lpm_num_maps;
> +	u32 sts_offset = pmcdev->map->lpm_status_offset;
> +	u32 *lpm_req_regs = pmcdev->lpm_req_regs;
> +	int mp;
> +
> +	/* Display the header */
> +	pmc_core_substate_req_header_show(s);
> +
> +	/* Loop over maps */
> +	for (mp = 0; mp < num_maps; mp++) {
> +		u32 req_mask = 0;
> +		u32 lpm_status;
> +		int mode, idx, i, len = 32;
> +
> +		/*
> +		 * Capture the requirements and create a mask so that we only
> +		 * show an element if it's required for at least one of the
> +		 * enabled low power modes
> +		 */
> +		pmc_for_each_mode(idx, mode, pmcdev)
> +			req_mask |= lpm_req_regs[mp + (mode * num_maps)];
> +
> +		/* Get the last latched status for this map */
> +		lpm_status = pmc_core_reg_read(pmcdev, sts_offset + (mp * 4));
> +
> +		/*  Loop over elements in this map */
> +		map = maps[mp];
> +		for (i = 0; map[i].name && i < len; i++) {
> +			u32 bit_mask = map[i].bit_mask;
> +
> +			if (!(bit_mask & req_mask))
> +				/*
> +				 * Not required for any enabled states
> +				 * so don't display
> +				 */
> +				continue;
> +
> +			/* Display the element name in the first column */
> +			seq_printf(s, "%30s |", map[i].name);
> +
> +			/* Loop over the enabled states and display if required */
> +			pmc_for_each_mode(idx, mode, pmcdev) {
> +				if (lpm_req_regs[mp + (mode * num_maps)] & bit_mask)
> +					seq_printf(s, " %9s |",
> +						   "Required");
> +				else
> +					seq_printf(s, " %9s |", " ");
> +			}
> +
> +			/* In Status column, show the last captured state of this agent */
> +			if (lpm_status & bit_mask)
> +				seq_printf(s, " %9s |", "Yes");
> +			else
> +				seq_printf(s, " %9s |", " ");
> +
> +			seq_puts(s, "\n");
> +		}
> +	}
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_req_regs);
> +
>  static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
>  {
>  	struct pmc_dev *pmcdev = s->private;
> @@ -1241,6 +1321,12 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>  				    pmcdev->dbgfs_dir, pmcdev,
>  				    &pmc_core_substate_l_sts_regs_fops);
>  	}
> +
> +	if (pmcdev->lpm_req_regs) {
> +		debugfs_create_file("substate_requirements", 0444,
> +				    pmcdev->dbgfs_dir, pmcdev,
> +				    &pmc_core_substate_req_regs_fops);
> +	}
>  }
>  
>  static const struct x86_cpu_id intel_pmc_core_ids[] = {
> 

Re: [PATCH 6/9] platform/x86: intel_pmc_core: Add requirements file to debugfs

[Rajneesh Bhardwaj]

On Wed, Mar 31, 2021 at 11:06 PM David E. Box
<david.e.box@linux.intel.com> wrote:
>
> From: Gayatri Kammela <gayatri.kammela@intel.com>
>
> Add the debugfs file, substate_requirements, to view the low power mode
> (LPM) requirements for each enabled mode alongside the last latched status
> of the condition.
>
> After this patch, the new file will look like this:
>
>                     Element |    S0i2.0 |    S0i3.0 |    S0i2.1 |    S0i3.1 |    S0i3.2 |    Status |
>             USB2PLL_OFF_STS |  Required |  Required |  Required |  Required |  Required |           |
> PCIe/USB3.1_Gen2PLL_OFF_STS |  Required |  Required |  Required |  Required |  Required |           |
>        PCIe_Gen3PLL_OFF_STS |  Required |  Required |  Required |  Required |  Required |       Yes |
>             OPIOPLL_OFF_STS |  Required |  Required |  Required |  Required |  Required |       Yes |
>               OCPLL_OFF_STS |  Required |  Required |  Required |  Required |  Required |       Yes |
>             MainPLL_OFF_STS |           |  Required |           |  Required |  Required |           |
>
> Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
> Co-developed-by: David E. Box <david.e.box@linux.intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 86 +++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 0ec26a4c715e..0b47a1da5f49 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -1122,6 +1122,86 @@ static int pmc_core_substate_l_sts_regs_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_l_sts_regs);
>
> +static void pmc_core_substate_req_header_show(struct seq_file *s)
> +{
> +       struct pmc_dev *pmcdev = s->private;
> +       int i, mode;
> +
> +       seq_printf(s, "%30s |", "Element");
> +       pmc_for_each_mode(i, mode, pmcdev)
> +               seq_printf(s, " %9s |", pmc_lpm_modes[mode]);
> +
> +       seq_printf(s, " %9s |\n", "Status");
> +}
> +
> +static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
> +{
> +       struct pmc_dev *pmcdev = s->private;
> +       const struct pmc_bit_map **maps = pmcdev->map->lpm_sts;
> +       const struct pmc_bit_map *map;
> +       const int num_maps = pmcdev->map->lpm_num_maps;
> +       u32 sts_offset = pmcdev->map->lpm_status_offset;
> +       u32 *lpm_req_regs = pmcdev->lpm_req_regs;
> +       int mp;
> +
> +       /* Display the header */
> +       pmc_core_substate_req_header_show(s);
> +
> +       /* Loop over maps */
> +       for (mp = 0; mp < num_maps; mp++) {
> +               u32 req_mask = 0;
> +               u32 lpm_status;
> +               int mode, idx, i, len = 32;
> +
> +               /*
> +                * Capture the requirements and create a mask so that we only
> +                * show an element if it's required for at least one of the
> +                * enabled low power modes
> +                */
> +               pmc_for_each_mode(idx, mode, pmcdev)
> +                       req_mask |= lpm_req_regs[mp + (mode * num_maps)];
> +
> +               /* Get the last latched status for this map */
> +               lpm_status = pmc_core_reg_read(pmcdev, sts_offset + (mp * 4));
> +
> +               /*  Loop over elements in this map */
> +               map = maps[mp];
> +               for (i = 0; map[i].name && i < len; i++) {
> +                       u32 bit_mask = map[i].bit_mask;
> +
> +                       if (!(bit_mask & req_mask))
> +                               /*
> +                                * Not required for any enabled states
> +                                * so don't display
> +                                */
> +                               continue;
> +
> +                       /* Display the element name in the first column */
> +                       seq_printf(s, "%30s |", map[i].name);
> +
> +                       /* Loop over the enabled states and display if required */
> +                       pmc_for_each_mode(idx, mode, pmcdev) {
> +                               if (lpm_req_regs[mp + (mode * num_maps)] & bit_mask)
> +                                       seq_printf(s, " %9s |",
> +                                                  "Required");
> +                               else
> +                                       seq_printf(s, " %9s |", " ");
> +                       }
> +
> +                       /* In Status column, show the last captured state of this agent */
> +                       if (lpm_status & bit_mask)
> +                               seq_printf(s, " %9s |", "Yes");
> +                       else
> +                               seq_printf(s, " %9s |", " ");

Why is this left blank, maybe NA (Not Available)?

> +
> +                       seq_puts(s, "\n");
> +               }
> +       }
> +
> +       return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_req_regs);
> +
>  static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
>  {
>         struct pmc_dev *pmcdev = s->private;
> @@ -1241,6 +1321,12 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>                                     pmcdev->dbgfs_dir, pmcdev,
>                                     &pmc_core_substate_l_sts_regs_fops);
>         }
> +
> +       if (pmcdev->lpm_req_regs) {
> +               debugfs_create_file("substate_requirements", 0444,
> +                                   pmcdev->dbgfs_dir, pmcdev,
> +                                   &pmc_core_substate_req_regs_fops);
> +       }
>  }
>
>  static const struct x86_cpu_id intel_pmc_core_ids[] = {
> --
> 2.25.1
>


-- 
Thanks,
Rajneesh

Re: [PATCH 6/9] platform/x86: intel_pmc_core: Add requirements file to debugfs

[David E. Box]

On Wed, 2021-04-07 at 11:45 -0400, Rajneesh Bhardwaj wrote:
> On Wed, Mar 31, 2021 at 11:06 PM David E. Box
> <david.e.box@linux.intel.com> wrote:
> > 
> > From: Gayatri Kammela <gayatri.kammela@intel.com>
> > 
> > Add the debugfs file, substate_requirements, to view the low power
> > mode
> > (LPM) requirements for each enabled mode alongside the last latched
> > status
> > of the condition.
> > 
> > After this patch, the new file will look like this:
> > 
> >                     Element |    S0i2.0 |    S0i3.0 |    S0i2.1
> > |    S0i3.1 |    S0i3.2 |    Status |
> >             USB2PLL_OFF_STS |  Required |  Required |  Required | 
> > Required |  Required |           |
> > PCIe/USB3.1_Gen2PLL_OFF_STS |  Required |  Required |  Required | 
> > Required |  Required |           |
> >        PCIe_Gen3PLL_OFF_STS |  Required |  Required |  Required | 
> > Required |  Required |       Yes |
> >             OPIOPLL_OFF_STS |  Required |  Required |  Required | 
> > Required |  Required |       Yes |
> >               OCPLL_OFF_STS |  Required |  Required |  Required | 
> > Required |  Required |       Yes |
> >             MainPLL_OFF_STS |           |  Required |           | 
> > Required |  Required |           |
> > 
> > Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
> > Co-developed-by: David E. Box <david.e.box@linux.intel.com>
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> >  drivers/platform/x86/intel_pmc_core.c | 86
> > +++++++++++++++++++++++++++
> >  1 file changed, 86 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > b/drivers/platform/x86/intel_pmc_core.c
> > index 0ec26a4c715e..0b47a1da5f49 100644
> > --- a/drivers/platform/x86/intel_pmc_core.c
> > +++ b/drivers/platform/x86/intel_pmc_core.c
> > @@ -1122,6 +1122,86 @@ static int
> > pmc_core_substate_l_sts_regs_show(struct seq_file *s, void *unused)
> >  }
> >  DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_l_sts_regs);
> > 
> > +static void pmc_core_substate_req_header_show(struct seq_file *s)
> > +{
> > +       struct pmc_dev *pmcdev = s->private;
> > +       int i, mode;
> > +
> > +       seq_printf(s, "%30s |", "Element");
> > +       pmc_for_each_mode(i, mode, pmcdev)
> > +               seq_printf(s, " %9s |", pmc_lpm_modes[mode]);
> > +
> > +       seq_printf(s, " %9s |\n", "Status");
> > +}
> > +
> > +static int pmc_core_substate_req_regs_show(struct seq_file *s,
> > void *unused)
> > +{
> > +       struct pmc_dev *pmcdev = s->private;
> > +       const struct pmc_bit_map **maps = pmcdev->map->lpm_sts;
> > +       const struct pmc_bit_map *map;
> > +       const int num_maps = pmcdev->map->lpm_num_maps;
> > +       u32 sts_offset = pmcdev->map->lpm_status_offset;
> > +       u32 *lpm_req_regs = pmcdev->lpm_req_regs;
> > +       int mp;
> > +
> > +       /* Display the header */
> > +       pmc_core_substate_req_header_show(s);
> > +
> > +       /* Loop over maps */
> > +       for (mp = 0; mp < num_maps; mp++) {
> > +               u32 req_mask = 0;
> > +               u32 lpm_status;
> > +               int mode, idx, i, len = 32;
> > +
> > +               /*
> > +                * Capture the requirements and create a mask so
> > that we only
> > +                * show an element if it's required for at least
> > one of the
> > +                * enabled low power modes
> > +                */
> > +               pmc_for_each_mode(idx, mode, pmcdev)
> > +                       req_mask |= lpm_req_regs[mp + (mode *
> > num_maps)];
> > +
> > +               /* Get the last latched status for this map */
> > +               lpm_status = pmc_core_reg_read(pmcdev, sts_offset +
> > (mp * 4));
> > +
> > +               /*  Loop over elements in this map */
> > +               map = maps[mp];
> > +               for (i = 0; map[i].name && i < len; i++) {
> > +                       u32 bit_mask = map[i].bit_mask;
> > +
> > +                       if (!(bit_mask & req_mask))
> > +                               /*
> > +                                * Not required for any enabled
> > states
> > +                                * so don't display
> > +                                */
> > +                               continue;
> > +
> > +                       /* Display the element name in the first
> > column */
> > +                       seq_printf(s, "%30s |", map[i].name);
> > +
> > +                       /* Loop over the enabled states and display
> > if required */
> > +                       pmc_for_each_mode(idx, mode, pmcdev) {
> > +                               if (lpm_req_regs[mp + (mode *
> > num_maps)] & bit_mask)
> > +                                       seq_printf(s, " %9s |",
> > +                                                  "Required");
> > +                               else
> > +                                       seq_printf(s, " %9s |", "
> > ");
> > +                       }
> > +
> > +                       /* In Status column, show the last captured
> > state of this agent */
> > +                       if (lpm_status & bit_mask)
> > +                               seq_printf(s, " %9s |", "Yes");
> > +                       else
> > +                               seq_printf(s, " %9s |", " ");
> 
> Why is this left blank, maybe NA (Not Available)?

The last column shows that last latched state of that agent. So if
anything it would be "Not Seen". But a blank space makes it visually
easier to parse.

David

> 
> > +
> > +                       seq_puts(s, "\n");
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_req_regs);
> > +
> >  static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
> >  {
> >         struct pmc_dev *pmcdev = s->private;
> > @@ -1241,6 +1321,12 @@ static void pmc_core_dbgfs_register(struct
> > pmc_dev *pmcdev)
> >                                     pmcdev->dbgfs_dir, pmcdev,
> >                                    
> > &pmc_core_substate_l_sts_regs_fops);
> >         }
> > +
> > +       if (pmcdev->lpm_req_regs) {
> > +               debugfs_create_file("substate_requirements", 0444,
> > +                                   pmcdev->dbgfs_dir, pmcdev,
> > +                                  
> > &pmc_core_substate_req_regs_fops);
> > +       }
> >  }
> > 
> >  static const struct x86_cpu_id intel_pmc_core_ids[] = {
> > --
> > 2.25.1
> > 
> 
> 

[PATCH 7/9] platform/x86: intel_pmc_core: Add option to set/clear LPM mode

[David E. Box]

By default the Low Power Mode (LPM or sub-state) status registers will
latch condition status on every entry into Package C10. This is
configurable in the PMC to allow latching on any achievable sub-state. Add
a debugfs file to support this.

Also add the option to clear the status registers to 0. Clearing the status
registers before testing removes ambiguity around when the current values
were set.

The new file, latch_lpm_mode, looks like this:

	[c10] S0i2.0 S0i3.0 S0i2.1 S0i3.1 S0i3.2 clear

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_core.c | 94 +++++++++++++++++++++++++++
 drivers/platform/x86/intel_pmc_core.h | 20 ++++++
 2 files changed, 114 insertions(+)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 0b47a1da5f49..458c0056e7a1 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -584,6 +584,8 @@ static const struct pmc_reg_map tgl_reg_map = {
 	.ltr_ignore_max = TGL_NUM_IP_IGN_ALLOWED,
 	.lpm_num_maps = TGL_LPM_NUM_MAPS,
 	.lpm_res_counter_step_x2 = TGL_PMC_LPM_RES_COUNTER_STEP_X2,
+	.etr3_offset = TGL_ETR3_OFFSET,
+	.lpm_sts_latch_en_offset = TGL_LPM_STS_LATCH_EN_OFFSET,
 	.lpm_en_offset = TGL_LPM_EN_OFFSET,
 	.lpm_priority_offset = TGL_LPM_PRI_OFFSET,
 	.lpm_residency_offset = TGL_LPM_RESIDENCY_OFFSET,
@@ -1202,6 +1204,95 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_req_regs);
 
+static int pmc_core_lpm_latch_mode_show(struct seq_file *s, void *unused)
+{
+	struct pmc_dev *pmcdev = s->private;
+	bool c10;
+	u32 reg;
+	int idx, mode;
+
+	reg = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_sts_latch_en_offset);
+	if (reg & BIT(LPM_STS_LATCH_MODE_BIT)) {
+		seq_puts(s, "c10");
+		c10 = false;
+	} else {
+		seq_puts(s, "[c10]");
+		c10 = true;
+	}
+
+	pmc_for_each_mode(idx, mode, pmcdev) {
+		if ((BIT(mode) & reg) && !c10)
+			seq_printf(s, " [%s]", pmc_lpm_modes[mode]);
+		else
+			seq_printf(s, " %s", pmc_lpm_modes[mode]);
+	}
+
+	seq_puts(s, " clear\n");
+
+	return 0;
+}
+
+static ssize_t pmc_core_lpm_latch_mode_write(struct file *file,
+					     const char __user *userbuf,
+					     size_t count, loff_t *ppos)
+{
+	struct seq_file *s = file->private_data;
+	struct pmc_dev *pmcdev = s->private;
+	bool clear = false, c10 = false;
+	unsigned char buf[10] = {0};
+	size_t ret;
+	int mode;
+	u32 reg;
+
+	ret = simple_write_to_buffer(buf, 10, ppos, userbuf, count);
+	if (ret < 0)
+		return ret;
+
+	mode = sysfs_match_string(pmc_lpm_modes, buf);
+	if (mode < 0) {
+		if (strncmp("clear", buf, 5) == 0)
+			clear = true;
+		else if (strncmp("c10", buf, 3) == 0)
+			c10 = true;
+		else
+			return mode;
+	}
+
+	if (clear) {
+		mutex_lock(&pmcdev->lock);
+
+		reg = pmc_core_reg_read(pmcdev, pmcdev->map->etr3_offset);
+		reg |= BIT(ETR3_CLEAR_LPM_EVENTS_BIT);
+		pmc_core_reg_write(pmcdev, pmcdev->map->etr3_offset, reg);
+
+		mutex_unlock(&pmcdev->lock);
+
+		return count;
+	}
+
+	if (c10) {
+		mutex_lock(&pmcdev->lock);
+
+		reg = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_sts_latch_en_offset);
+		reg &= ~BIT(LPM_STS_LATCH_MODE_BIT);
+		pmc_core_reg_write(pmcdev, pmcdev->map->lpm_sts_latch_en_offset, reg);
+
+		mutex_unlock(&pmcdev->lock);
+
+		return count;
+	}
+
+	/*
+	 * For LPM mode latching we set the latch enable bit and selected mode
+	 * and clear everything else.
+	 */
+	reg = BIT(LPM_STS_LATCH_MODE_BIT) | BIT(mode);
+	pmc_core_reg_write(pmcdev, pmcdev->map->lpm_sts_latch_en_offset, reg);
+
+	return count;
+}
+DEFINE_PMC_CORE_ATTR_WRITE(pmc_core_lpm_latch_mode);
+
 static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
 {
 	struct pmc_dev *pmcdev = s->private;
@@ -1320,6 +1411,9 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
 		debugfs_create_file("substate_live_status_registers", 0444,
 				    pmcdev->dbgfs_dir, pmcdev,
 				    &pmc_core_substate_l_sts_regs_fops);
+		debugfs_create_file("lpm_latch_mode", 0644,
+				    pmcdev->dbgfs_dir, pmcdev,
+				    &pmc_core_lpm_latch_mode_fops);
 	}
 
 	if (pmcdev->lpm_req_regs) {
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 81d797feed33..f41f61aa7008 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -189,6 +189,8 @@ enum ppfear_regs {
 
 #define LPM_MAX_NUM_MODES			8
 #define GET_X2_COUNTER(v)			((v) >> 1)
+#define ETR3_CLEAR_LPM_EVENTS_BIT		28
+#define LPM_STS_LATCH_MODE_BIT			31
 
 #define TGL_NUM_IP_IGN_ALLOWED			22
 #define TGL_PMC_SLP_S0_RES_COUNTER_STEP		0x7A
@@ -197,6 +199,8 @@ enum ppfear_regs {
 /*
  * Tigerlake Power Management Controller register offsets
  */
+#define TGL_ETR3_OFFSET				0x1048
+#define TGL_LPM_STS_LATCH_EN_OFFSET		0x1C34
 #define TGL_LPM_EN_OFFSET			0x1C78
 #define TGL_LPM_RESIDENCY_OFFSET		0x1C80
 
@@ -266,6 +270,8 @@ struct pmc_reg_map {
 	/* Low Power Mode registers */
 	const int lpm_num_maps;
 	const int lpm_res_counter_step_x2;
+	const u32 etr3_offset;
+	const u32 lpm_sts_latch_en_offset;
 	const u32 lpm_en_offset;
 	const u32 lpm_priority_offset;
 	const u32 lpm_residency_offset;
@@ -313,4 +319,18 @@ struct pmc_dev {
 	     i < pmcdev->num_modes;			\
 	     i++, mode = pmcdev->lpm_en_modes[i])
 
+#define DEFINE_PMC_CORE_ATTR_WRITE(__name)				\
+static int __name ## _open(struct inode *inode, struct file *file)	\
+{									\
+	return single_open(file, __name ## _show, inode->i_private);	\
+}									\
+									\
+static const struct file_operations __name ## _fops = {			\
+	.owner		= THIS_MODULE,					\
+	.open		= __name ## _open,				\
+	.read		= seq_read,					\
+	.write		= __name ## _write,				\
+	.release	= single_release,				\
+}
+
 #endif /* PMC_CORE_H */
-- 
2.25.1

Re: [PATCH 7/9] platform/x86: intel_pmc_core: Add option to set/clear LPM mode

[Hans de Goede]

Hi,

On 4/1/21 5:05 AM, David E. Box wrote:
> By default the Low Power Mode (LPM or sub-state) status registers will
> latch condition status on every entry into Package C10. This is
> configurable in the PMC to allow latching on any achievable sub-state. Add
> a debugfs file to support this.
> 
> Also add the option to clear the status registers to 0. Clearing the status
> registers before testing removes ambiguity around when the current values
> were set.
> 
> The new file, latch_lpm_mode, looks like this:
> 
> 	[c10] S0i2.0 S0i3.0 S0i2.1 S0i3.1 S0i3.2 clear
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 94 +++++++++++++++++++++++++++
>  drivers/platform/x86/intel_pmc_core.h | 20 ++++++
>  2 files changed, 114 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 0b47a1da5f49..458c0056e7a1 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -584,6 +584,8 @@ static const struct pmc_reg_map tgl_reg_map = {
>  	.ltr_ignore_max = TGL_NUM_IP_IGN_ALLOWED,
>  	.lpm_num_maps = TGL_LPM_NUM_MAPS,
>  	.lpm_res_counter_step_x2 = TGL_PMC_LPM_RES_COUNTER_STEP_X2,
> +	.etr3_offset = TGL_ETR3_OFFSET,
> +	.lpm_sts_latch_en_offset = TGL_LPM_STS_LATCH_EN_OFFSET,
>  	.lpm_en_offset = TGL_LPM_EN_OFFSET,
>  	.lpm_priority_offset = TGL_LPM_PRI_OFFSET,
>  	.lpm_residency_offset = TGL_LPM_RESIDENCY_OFFSET,
> @@ -1202,6 +1204,95 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_req_regs);
>  
> +static int pmc_core_lpm_latch_mode_show(struct seq_file *s, void *unused)
> +{
> +	struct pmc_dev *pmcdev = s->private;
> +	bool c10;
> +	u32 reg;
> +	int idx, mode;
> +
> +	reg = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_sts_latch_en_offset);
> +	if (reg & BIT(LPM_STS_LATCH_MODE_BIT)) {
> +		seq_puts(s, "c10");
> +		c10 = false;
> +	} else {
> +		seq_puts(s, "[c10]");
> +		c10 = true;
> +	}
> +
> +	pmc_for_each_mode(idx, mode, pmcdev) {
> +		if ((BIT(mode) & reg) && !c10)
> +			seq_printf(s, " [%s]", pmc_lpm_modes[mode]);
> +		else
> +			seq_printf(s, " %s", pmc_lpm_modes[mode]);
> +	}

So this either shows [c10] selected, or it shows which s0ix modes
are selected, that is a bit weird.

I realize this is a debugfs interface, but can we still get some docs
in this, at a minimum some more explanation in the commit message ?


> +
> +	seq_puts(s, " clear\n");
> +
> +	return 0;
> +}
> +
> +static ssize_t pmc_core_lpm_latch_mode_write(struct file *file,
> +					     const char __user *userbuf,
> +					     size_t count, loff_t *ppos)
> +{
> +	struct seq_file *s = file->private_data;
> +	struct pmc_dev *pmcdev = s->private;
> +	bool clear = false, c10 = false;
> +	unsigned char buf[10] = {0};
> +	size_t ret;
> +	int mode;
> +	u32 reg;
> +
> +	ret = simple_write_to_buffer(buf, 10, ppos, userbuf, count);
> +	if (ret < 0)
> +		return ret;

You are using C-string functions on buf below, so you must guarantee
that it is 0 terminated. To guarantee the buffer's size must be 1 larger
then the size which you pass to simple_write_to_buffer()

> +
> +	mode = sysfs_match_string(pmc_lpm_modes, buf);
> +	if (mode < 0) {
> +		if (strncmp("clear", buf, 5) == 0)
> +			clear = true;
> +		else if (strncmp("c10", buf, 3) == 0)
> +			c10 = true;

Ugh, no. Now it will not just accept "clear" and "clear\n" but
also "clear foobar everything else I write now does not matter"
as "clear" string. This misuse of strncmp for sysfs / debugfs write
functions seems to be some sort of anti-pattern in the kernel.

Please use sysfs_streq() here so that only "clear" and "clear\n"
are accepted (and the same for the "c10" check).



> +		else
> +			return mode;
> +	}
> +
> +	if (clear) {
> +		mutex_lock(&pmcdev->lock);
> +
> +		reg = pmc_core_reg_read(pmcdev, pmcdev->map->etr3_offset);
> +		reg |= BIT(ETR3_CLEAR_LPM_EVENTS_BIT);
> +		pmc_core_reg_write(pmcdev, pmcdev->map->etr3_offset, reg);
> +
> +		mutex_unlock(&pmcdev->lock);
> +
> +		return count;
> +	}
> +
> +	if (c10) {
> +		mutex_lock(&pmcdev->lock);
> +
> +		reg = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_sts_latch_en_offset);
> +		reg &= ~BIT(LPM_STS_LATCH_MODE_BIT);
> +		pmc_core_reg_write(pmcdev, pmcdev->map->lpm_sts_latch_en_offset, reg);
> +
> +		mutex_unlock(&pmcdev->lock);
> +
> +		return count;
> +	}
> +
> +	/*
> +	 * For LPM mode latching we set the latch enable bit and selected mode
> +	 * and clear everything else.
> +	 */
> +	reg = BIT(LPM_STS_LATCH_MODE_BIT) | BIT(mode);
> +	pmc_core_reg_write(pmcdev, pmcdev->map->lpm_sts_latch_en_offset, reg);
> +
> +	return count;
> +}
> +DEFINE_PMC_CORE_ATTR_WRITE(pmc_core_lpm_latch_mode);
> +
>  static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
>  {
>  	struct pmc_dev *pmcdev = s->private;
> @@ -1320,6 +1411,9 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>  		debugfs_create_file("substate_live_status_registers", 0444,
>  				    pmcdev->dbgfs_dir, pmcdev,
>  				    &pmc_core_substate_l_sts_regs_fops);
> +		debugfs_create_file("lpm_latch_mode", 0644,
> +				    pmcdev->dbgfs_dir, pmcdev,
> +				    &pmc_core_lpm_latch_mode_fops);
>  	}
>  
>  	if (pmcdev->lpm_req_regs) {
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index 81d797feed33..f41f61aa7008 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -189,6 +189,8 @@ enum ppfear_regs {
>  
>  #define LPM_MAX_NUM_MODES			8
>  #define GET_X2_COUNTER(v)			((v) >> 1)
> +#define ETR3_CLEAR_LPM_EVENTS_BIT		28
> +#define LPM_STS_LATCH_MODE_BIT			31
>  
>  #define TGL_NUM_IP_IGN_ALLOWED			22
>  #define TGL_PMC_SLP_S0_RES_COUNTER_STEP		0x7A
> @@ -197,6 +199,8 @@ enum ppfear_regs {
>  /*
>   * Tigerlake Power Management Controller register offsets
>   */
> +#define TGL_ETR3_OFFSET				0x1048
> +#define TGL_LPM_STS_LATCH_EN_OFFSET		0x1C34
>  #define TGL_LPM_EN_OFFSET			0x1C78
>  #define TGL_LPM_RESIDENCY_OFFSET		0x1C80
>  
> @@ -266,6 +270,8 @@ struct pmc_reg_map {
>  	/* Low Power Mode registers */
>  	const int lpm_num_maps;
>  	const int lpm_res_counter_step_x2;
> +	const u32 etr3_offset;
> +	const u32 lpm_sts_latch_en_offset;
>  	const u32 lpm_en_offset;
>  	const u32 lpm_priority_offset;
>  	const u32 lpm_residency_offset;
> @@ -313,4 +319,18 @@ struct pmc_dev {
>  	     i < pmcdev->num_modes;			\
>  	     i++, mode = pmcdev->lpm_en_modes[i])
>  
> +#define DEFINE_PMC_CORE_ATTR_WRITE(__name)				\
> +static int __name ## _open(struct inode *inode, struct file *file)	\
> +{									\
> +	return single_open(file, __name ## _show, inode->i_private);	\
> +}									\
> +									\
> +static const struct file_operations __name ## _fops = {			\
> +	.owner		= THIS_MODULE,					\
> +	.open		= __name ## _open,				\
> +	.read		= seq_read,					\
> +	.write		= __name ## _write,				\
> +	.release	= single_release,				\
> +}
> +
>  #endif /* PMC_CORE_H */
> 

Regards,

Hans

Re: [PATCH 7/9] platform/x86: intel_pmc_core: Add option to set/clear LPM mode

[David E. Box]

Hi Hans.

Ack on your comments for this series. Questions answered below.

On Wed, 2021-04-07 at 16:37 +0200, Hans de Goede wrote:
> Hi,
> 
> On 4/1/21 5:05 AM, David E. Box wrote:
> > By default the Low Power Mode (LPM or sub-state) status registers
> > will
> > latch condition status on every entry into Package C10. This is
> > configurable in the PMC to allow latching on any achievable sub-
> > state. Add
> > a debugfs file to support this.
> > 
> > Also add the option to clear the status registers to 0. Clearing
> > the status
> > registers before testing removes ambiguity around when the current
> > values
> > were set.
> > 
> > The new file, latch_lpm_mode, looks like this:
> > 
> >         [c10] S0i2.0 S0i3.0 S0i2.1 S0i3.1 S0i3.2 clear
> > 
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> >  drivers/platform/x86/intel_pmc_core.c | 94
> > +++++++++++++++++++++++++++
> >  drivers/platform/x86/intel_pmc_core.h | 20 ++++++
> >  2 files changed, 114 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > b/drivers/platform/x86/intel_pmc_core.c
> > index 0b47a1da5f49..458c0056e7a1 100644
> > --- a/drivers/platform/x86/intel_pmc_core.c
> > +++ b/drivers/platform/x86/intel_pmc_core.c
> > @@ -584,6 +584,8 @@ static const struct pmc_reg_map tgl_reg_map = {
> >         .ltr_ignore_max = TGL_NUM_IP_IGN_ALLOWED,
> >         .lpm_num_maps = TGL_LPM_NUM_MAPS,
> >         .lpm_res_counter_step_x2 = TGL_PMC_LPM_RES_COUNTER_STEP_X2,
> > +       .etr3_offset = TGL_ETR3_OFFSET,
> > +       .lpm_sts_latch_en_offset = TGL_LPM_STS_LATCH_EN_OFFSET,
> >         .lpm_en_offset = TGL_LPM_EN_OFFSET,
> >         .lpm_priority_offset = TGL_LPM_PRI_OFFSET,
> >         .lpm_residency_offset = TGL_LPM_RESIDENCY_OFFSET,
> > @@ -1202,6 +1204,95 @@ static int
> > pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
> >  }
> >  DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_req_regs);
> >  
> > +static int pmc_core_lpm_latch_mode_show(struct seq_file *s, void
> > *unused)
> > +{
> > +       struct pmc_dev *pmcdev = s->private;
> > +       bool c10;
> > +       u32 reg;
> > +       int idx, mode;
> > +
> > +       reg = pmc_core_reg_read(pmcdev, pmcdev->map-
> > >lpm_sts_latch_en_offset);
> > +       if (reg & BIT(LPM_STS_LATCH_MODE_BIT)) {
> > +               seq_puts(s, "c10");
> > +               c10 = false;
> > +       } else {
> > +               seq_puts(s, "[c10]");
> > +               c10 = true;
> > +       }
> > +
> > +       pmc_for_each_mode(idx, mode, pmcdev) {
> > +               if ((BIT(mode) & reg) && !c10)
> > +                       seq_printf(s, " [%s]",
> > pmc_lpm_modes[mode]);
> > +               else
> > +                       seq_printf(s, " %s", pmc_lpm_modes[mode]);
> > +       }
> 
> So this either shows [c10] selected, or it shows which s0ix modes
> are selected, that is a bit weird.

One bit controls whether c10 or substate latching is enabled. A
different register sets the substate to latch and applies iff substate
latching is enabled. If c10 latching is enabled, any selected substates
are ignored by hardware so we don't show them. In the write function
when a substate is selected, substate latching is also enabled.

> 
> I realize this is a debugfs interface, but can we still get some docs
> in this, at a minimum some more explanation in the commit message ?

I'll describe this better in the commit message. We also maintain S0ix
documentation on 01.org. I'll add a patch that links to this site in
Documentation.

Thanks

David

> 
> 
> > +
> > +       seq_puts(s, " clear\n");
> > +
> > +       return 0;
> > +}
> > +
> > +static ssize_t pmc_core_lpm_latch_mode_write(struct file *file,
> > +                                            const char __user
> > *userbuf,
> > +                                            size_t count, loff_t
> > *ppos)
> > +{
> > +       struct seq_file *s = file->private_data;
> > +       struct pmc_dev *pmcdev = s->private;
> > +       bool clear = false, c10 = false;
> > +       unsigned char buf[10] = {0};
> > +       size_t ret;
> > +       int mode;
> > +       u32 reg;
> > +
> > +       ret = simple_write_to_buffer(buf, 10, ppos, userbuf,
> > count);
> > +       if (ret < 0)
> > +               return ret;
> 
> You are using C-string functions on buf below, so you must guarantee
> that it is 0 terminated. To guarantee the buffer's size must be 1
> larger
> then the size which you pass to simple_write_to_buffer()
> 
> > +
> > +       mode = sysfs_match_string(pmc_lpm_modes, buf);
> > +       if (mode < 0) {
> > +               if (strncmp("clear", buf, 5) == 0)
> > +                       clear = true;
> > +               else if (strncmp("c10", buf, 3) == 0)
> > +                       c10 = true;
> 
> Ugh, no. Now it will not just accept "clear" and "clear\n" but
> also "clear foobar everything else I write now does not matter"
> as "clear" string. This misuse of strncmp for sysfs / debugfs write
> functions seems to be some sort of anti-pattern in the kernel.
> 
> Please use sysfs_streq() here so that only "clear" and "clear\n"
> are accepted (and the same for the "c10" check).
> 
> 
> 
> > +               else
> > +                       return mode;
> > +       }
> > +
> > +       if (clear) {
> > +               mutex_lock(&pmcdev->lock);
> > +
> > +               reg = pmc_core_reg_read(pmcdev, pmcdev->map-
> > >etr3_offset);
> > +               reg |= BIT(ETR3_CLEAR_LPM_EVENTS_BIT);
> > +               pmc_core_reg_write(pmcdev, pmcdev->map-
> > >etr3_offset, reg);
> > +
> > +               mutex_unlock(&pmcdev->lock);
> > +
> > +               return count;
> > +       }
> > +
> > +       if (c10) {
> > +               mutex_lock(&pmcdev->lock);
> > +
> > +               reg = pmc_core_reg_read(pmcdev, pmcdev->map-
> > >lpm_sts_latch_en_offset);
> > +               reg &= ~BIT(LPM_STS_LATCH_MODE_BIT);
> > +               pmc_core_reg_write(pmcdev, pmcdev->map-
> > >lpm_sts_latch_en_offset, reg);
> > +
> > +               mutex_unlock(&pmcdev->lock);
> > +
> > +               return count;
> > +       }
> > +
> > +       /*
> > +        * For LPM mode latching we set the latch enable bit and
> > selected mode
> > +        * and clear everything else.
> > +        */
> > +       reg = BIT(LPM_STS_LATCH_MODE_BIT) | BIT(mode);
> > +       pmc_core_reg_write(pmcdev, pmcdev->map-
> > >lpm_sts_latch_en_offset, reg);
> > +
> > +       return count;
> > +}
> > +DEFINE_PMC_CORE_ATTR_WRITE(pmc_core_lpm_latch_mode);
> > +
> >  static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
> >  {
> >         struct pmc_dev *pmcdev = s->private;
> > @@ -1320,6 +1411,9 @@ static void pmc_core_dbgfs_register(struct
> > pmc_dev *pmcdev)
> >                 debugfs_create_file("substate_live_status_registers
> > ", 0444,
> >                                     pmcdev->dbgfs_dir, pmcdev,
> >                                    
> > &pmc_core_substate_l_sts_regs_fops);
> > +               debugfs_create_file("lpm_latch_mode", 0644,
> > +                                   pmcdev->dbgfs_dir, pmcdev,
> > +                                   &pmc_core_lpm_latch_mode_fops);
> >         }
> >  
> >         if (pmcdev->lpm_req_regs) {
> > diff --git a/drivers/platform/x86/intel_pmc_core.h
> > b/drivers/platform/x86/intel_pmc_core.h
> > index 81d797feed33..f41f61aa7008 100644
> > --- a/drivers/platform/x86/intel_pmc_core.h
> > +++ b/drivers/platform/x86/intel_pmc_core.h
> > @@ -189,6 +189,8 @@ enum ppfear_regs {
> >  
> >  #define LPM_MAX_NUM_MODES                      8
> >  #define GET_X2_COUNTER(v)                      ((v) >> 1)
> > +#define ETR3_CLEAR_LPM_EVENTS_BIT              28
> > +#define LPM_STS_LATCH_MODE_BIT                 31
> >  
> >  #define TGL_NUM_IP_IGN_ALLOWED                 22
> >  #define TGL_PMC_SLP_S0_RES_COUNTER_STEP                0x7A
> > @@ -197,6 +199,8 @@ enum ppfear_regs {
> >  /*
> >   * Tigerlake Power Management Controller register offsets
> >   */
> > +#define TGL_ETR3_OFFSET                                0x1048
> > +#define TGL_LPM_STS_LATCH_EN_OFFSET            0x1C34
> >  #define TGL_LPM_EN_OFFSET                      0x1C78
> >  #define TGL_LPM_RESIDENCY_OFFSET               0x1C80
> >  
> > @@ -266,6 +270,8 @@ struct pmc_reg_map {
> >         /* Low Power Mode registers */
> >         const int lpm_num_maps;
> >         const int lpm_res_counter_step_x2;
> > +       const u32 etr3_offset;
> > +       const u32 lpm_sts_latch_en_offset;
> >         const u32 lpm_en_offset;
> >         const u32 lpm_priority_offset;
> >         const u32 lpm_residency_offset;
> > @@ -313,4 +319,18 @@ struct pmc_dev {
> >              i < pmcdev->num_modes;                     \
> >              i++, mode = pmcdev->lpm_en_modes[i])
> >  
> > +#define
> > DEFINE_PMC_CORE_ATTR_WRITE(__name)                             \
> > +static int __name ## _open(struct inode *inode, struct file
> > *file)     \
> > +{                                                                 
> >      \
> > +       return single_open(file, __name ## _show, inode-
> > >i_private);    \
> > +}                                                                 
> >      \
> > +                                                                  
> >      \
> > +static const struct file_operations __name ## _fops =
> > {                        \
> > +       .owner          =
> > THIS_MODULE,                                  \
> > +       .open           = __name ##
> > _open,                              \
> > +       .read           =
> > seq_read,                                     \
> > +       .write          = __name ##
> > _write,                             \
> > +       .release        =
> > single_release,                               \
> > +}
> > +
> >  #endif /* PMC_CORE_H */
> > 
> 
> Regards,
> 
> Hans
> 

[PATCH 8/9] platform/x86: intel_pmc_core: Add LTR registers for Tiger Lake

[David E. Box]

From: Gayatri Kammela <gayatri.kammela@intel.com>

Just like Ice Lake, Tiger Lake uses Cannon Lake's LTR information
and supports a few additional registers. Hence add the LTR registers
specific to Tiger Lake to the cnp_ltr_show_map[].

Also adjust the number of LTR IPs for Tiger Lake to the correct amount.

Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_core.c | 2 ++
 drivers/platform/x86/intel_pmc_core.h | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 458c0056e7a1..9168062c927e 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -383,6 +383,8 @@ static const struct pmc_bit_map cnp_ltr_show_map[] = {
 	 * a list of core SoCs using this.
 	 */
 	{"WIGIG",		ICL_PMC_LTR_WIGIG},
+	{"THC0",                TGL_PMC_LTR_THC0},
+	{"THC1",                TGL_PMC_LTR_THC1},
 	/* Below two cannot be used for LTR_IGNORE */
 	{"CURRENT_PLATFORM",	CNP_PMC_LTR_CUR_PLT},
 	{"AGGREGATED_SYSTEM",	CNP_PMC_LTR_CUR_ASLT},
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index f41f61aa7008..634130b589a2 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -192,8 +192,10 @@ enum ppfear_regs {
 #define ETR3_CLEAR_LPM_EVENTS_BIT		28
 #define LPM_STS_LATCH_MODE_BIT			31
 
-#define TGL_NUM_IP_IGN_ALLOWED			22
 #define TGL_PMC_SLP_S0_RES_COUNTER_STEP		0x7A
+#define TGL_PMC_LTR_THC0			0x1C04
+#define TGL_PMC_LTR_THC1			0x1C08
+#define TGL_NUM_IP_IGN_ALLOWED			23
 #define TGL_PMC_LPM_RES_COUNTER_STEP_X2		61	/* 30.5us * 2 */
 
 /*
-- 
2.25.1

Re: [PATCH 8/9] platform/x86: intel_pmc_core: Add LTR registers for Tiger Lake

[Hans de Goede]

Hi,

On 4/1/21 5:05 AM, David E. Box wrote:
> From: Gayatri Kammela <gayatri.kammela@intel.com>
> 
> Just like Ice Lake, Tiger Lake uses Cannon Lake's LTR information
> and supports a few additional registers. Hence add the LTR registers
> specific to Tiger Lake to the cnp_ltr_show_map[].
> 
> Also adjust the number of LTR IPs for Tiger Lake to the correct amount.
> 
> Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/platform/x86/intel_pmc_core.c | 2 ++
>  drivers/platform/x86/intel_pmc_core.h | 4 +++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 458c0056e7a1..9168062c927e 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -383,6 +383,8 @@ static const struct pmc_bit_map cnp_ltr_show_map[] = {
>  	 * a list of core SoCs using this.
>  	 */
>  	{"WIGIG",		ICL_PMC_LTR_WIGIG},
> +	{"THC0",                TGL_PMC_LTR_THC0},
> +	{"THC1",                TGL_PMC_LTR_THC1},
>  	/* Below two cannot be used for LTR_IGNORE */
>  	{"CURRENT_PLATFORM",	CNP_PMC_LTR_CUR_PLT},
>  	{"AGGREGATED_SYSTEM",	CNP_PMC_LTR_CUR_ASLT},
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index f41f61aa7008..634130b589a2 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -192,8 +192,10 @@ enum ppfear_regs {
>  #define ETR3_CLEAR_LPM_EVENTS_BIT		28
>  #define LPM_STS_LATCH_MODE_BIT			31
>  
> -#define TGL_NUM_IP_IGN_ALLOWED			22
>  #define TGL_PMC_SLP_S0_RES_COUNTER_STEP		0x7A
> +#define TGL_PMC_LTR_THC0			0x1C04
> +#define TGL_PMC_LTR_THC1			0x1C08
> +#define TGL_NUM_IP_IGN_ALLOWED			23
>  #define TGL_PMC_LPM_RES_COUNTER_STEP_X2		61	/* 30.5us * 2 */
>  
>  /*
> 

Re: [PATCH 8/9] platform/x86: intel_pmc_core: Add LTR registers for Tiger Lake

[Rajneesh Bhardwaj]

Acked-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>

On Wed, Mar 31, 2021 at 11:06 PM David E. Box
<david.e.box@linux.intel.com> wrote:
>
> From: Gayatri Kammela <gayatri.kammela@intel.com>
>
> Just like Ice Lake, Tiger Lake uses Cannon Lake's LTR information
> and supports a few additional registers. Hence add the LTR registers
> specific to Tiger Lake to the cnp_ltr_show_map[].
>
> Also adjust the number of LTR IPs for Tiger Lake to the correct amount.
>
> Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 2 ++
>  drivers/platform/x86/intel_pmc_core.h | 4 +++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 458c0056e7a1..9168062c927e 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -383,6 +383,8 @@ static const struct pmc_bit_map cnp_ltr_show_map[] = {
>          * a list of core SoCs using this.
>          */
>         {"WIGIG",               ICL_PMC_LTR_WIGIG},
> +       {"THC0",                TGL_PMC_LTR_THC0},
> +       {"THC1",                TGL_PMC_LTR_THC1},
>         /* Below two cannot be used for LTR_IGNORE */
>         {"CURRENT_PLATFORM",    CNP_PMC_LTR_CUR_PLT},
>         {"AGGREGATED_SYSTEM",   CNP_PMC_LTR_CUR_ASLT},
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index f41f61aa7008..634130b589a2 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -192,8 +192,10 @@ enum ppfear_regs {
>  #define ETR3_CLEAR_LPM_EVENTS_BIT              28
>  #define LPM_STS_LATCH_MODE_BIT                 31
>
> -#define TGL_NUM_IP_IGN_ALLOWED                 22
>  #define TGL_PMC_SLP_S0_RES_COUNTER_STEP                0x7A
> +#define TGL_PMC_LTR_THC0                       0x1C04
> +#define TGL_PMC_LTR_THC1                       0x1C08
> +#define TGL_NUM_IP_IGN_ALLOWED                 23
>  #define TGL_PMC_LPM_RES_COUNTER_STEP_X2                61      /* 30.5us * 2 */
>
>  /*
> --
> 2.25.1
>


-- 
Thanks,
Rajneesh

Re: [PATCH 8/9] platform/x86: intel_pmc_core: Add LTR registers for Tiger Lake

[Rajneesh Bhardwaj]

Please ignore the typo in my previous email and use this tag instead.

Acked-by: Rajneesh Bhardwaj <irenic.rajneesh@gmail.com>

On Wed, Apr 7, 2021 at 11:48 AM Rajneesh Bhardwaj
<irenic.rajneesh@gmail.com> wrote:
>
> Acked-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
>
> On Wed, Mar 31, 2021 at 11:06 PM David E. Box
> <david.e.box@linux.intel.com> wrote:
> >
> > From: Gayatri Kammela <gayatri.kammela@intel.com>
> >
> > Just like Ice Lake, Tiger Lake uses Cannon Lake's LTR information
> > and supports a few additional registers. Hence add the LTR registers
> > specific to Tiger Lake to the cnp_ltr_show_map[].
> >
> > Also adjust the number of LTR IPs for Tiger Lake to the correct amount.
> >
> > Signed-off-by: Gayatri Kammela <gayatri.kammela@intel.com>
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> >  drivers/platform/x86/intel_pmc_core.c | 2 ++
> >  drivers/platform/x86/intel_pmc_core.h | 4 +++-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> > index 458c0056e7a1..9168062c927e 100644
> > --- a/drivers/platform/x86/intel_pmc_core.c
> > +++ b/drivers/platform/x86/intel_pmc_core.c
> > @@ -383,6 +383,8 @@ static const struct pmc_bit_map cnp_ltr_show_map[] = {
> >          * a list of core SoCs using this.
> >          */
> >         {"WIGIG",               ICL_PMC_LTR_WIGIG},
> > +       {"THC0",                TGL_PMC_LTR_THC0},
> > +       {"THC1",                TGL_PMC_LTR_THC1},
> >         /* Below two cannot be used for LTR_IGNORE */
> >         {"CURRENT_PLATFORM",    CNP_PMC_LTR_CUR_PLT},
> >         {"AGGREGATED_SYSTEM",   CNP_PMC_LTR_CUR_ASLT},
> > diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> > index f41f61aa7008..634130b589a2 100644
> > --- a/drivers/platform/x86/intel_pmc_core.h
> > +++ b/drivers/platform/x86/intel_pmc_core.h
> > @@ -192,8 +192,10 @@ enum ppfear_regs {
> >  #define ETR3_CLEAR_LPM_EVENTS_BIT              28
> >  #define LPM_STS_LATCH_MODE_BIT                 31
> >
> > -#define TGL_NUM_IP_IGN_ALLOWED                 22
> >  #define TGL_PMC_SLP_S0_RES_COUNTER_STEP                0x7A
> > +#define TGL_PMC_LTR_THC0                       0x1C04
> > +#define TGL_PMC_LTR_THC1                       0x1C08
> > +#define TGL_NUM_IP_IGN_ALLOWED                 23
> >  #define TGL_PMC_LPM_RES_COUNTER_STEP_X2                61      /* 30.5us * 2 */
> >
> >  /*
> > --
> > 2.25.1
> >
>
>
> --
> Thanks,
> Rajneesh



-- 
Thanks,
Rajneesh

[PATCH 9/9] platform/x86: intel_pmc_core: Add support for Alder Lake PCH-P

[David E. Box]

Alder PCH-P is based on Tiger Lake PCH.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 9168062c927e..88d582df829f 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -1440,6 +1440,7 @@ static const struct x86_cpu_id intel_pmc_core_ids[] = {
 	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT,	&tgl_reg_map),
 	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_L,	&icl_reg_map),
 	X86_MATCH_INTEL_FAM6_MODEL(ROCKETLAKE,		&tgl_reg_map),
+	X86_MATCH_INTEL_FAM6_MODEL(ALDERLAKE_L,		&tgl_reg_map),
 	{}
 };
 
-- 
2.25.1

Re: [PATCH 9/9] platform/x86: intel_pmc_core: Add support for Alder Lake PCH-P

[Hans de Goede]

Hi,

On 4/1/21 5:05 AM, David E. Box wrote:
> Alder PCH-P is based on Tiger Lake PCH.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/platform/x86/intel_pmc_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 9168062c927e..88d582df829f 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -1440,6 +1440,7 @@ static const struct x86_cpu_id intel_pmc_core_ids[] = {
>  	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT,	&tgl_reg_map),
>  	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_L,	&icl_reg_map),
>  	X86_MATCH_INTEL_FAM6_MODEL(ROCKETLAKE,		&tgl_reg_map),
> +	X86_MATCH_INTEL_FAM6_MODEL(ALDERLAKE_L,		&tgl_reg_map),
>  	{}
>  };
>  
> 

Re: [PATCH 9/9] platform/x86: intel_pmc_core: Add support for Alder Lake PCH-P

[Rajneesh Bhardwaj]

Acked-by: Rajenesh Bhardwaj <irenic.rajneesh@gmail.com>

On Wed, Mar 31, 2021 at 11:06 PM David E. Box
<david.e.box@linux.intel.com> wrote:
>
> Alder PCH-P is based on Tiger Lake PCH.
>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 9168062c927e..88d582df829f 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -1440,6 +1440,7 @@ static const struct x86_cpu_id intel_pmc_core_ids[] = {
>         X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT,        &tgl_reg_map),
>         X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_L,      &icl_reg_map),
>         X86_MATCH_INTEL_FAM6_MODEL(ROCKETLAKE,          &tgl_reg_map),
> +       X86_MATCH_INTEL_FAM6_MODEL(ALDERLAKE_L,         &tgl_reg_map),
>         {}
>  };
>
> --
> 2.25.1
>


-- 
Thanks,
Rajneesh

Re: [PATCH 0/9] intel_pmc_core: Add sub-state requirements and mode latching support

[Hans de Goede]

Hi,

On 4/1/21 5:05 AM, David E. Box wrote:
> - Patch 1 and 2 remove the use of the global struct pmc_dev
> - Patches 3-7 add support for reading low power mode sub-state
>   requirements, latching sub-state status on different low power mode
>   events, and displaying the sub-state residency in microseconds
> - Patch 8 adds missing LTR IPs for TGL
> - Patch 9 adds support for ADL-P which is based on TGL
> 
> Applied on top of latest 5.12-rc2 based hans-review/review-hans

Thnak you for this series, this mostly is fine, a few small remarks
on patch 5/9 and 7/9 if you can send a v2 addressing those, then
this is ready for merging.

Regards,

Hans


> 
> David E. Box (4):
>   platform/x86: intel_pmc_core: Don't use global pmcdev in quirks
>   platform/x86: intel_pmc_core: Remove global struct pmc_dev
>   platform/x86: intel_pmc_core: Add option to set/clear LPM mode
>   platform/x86: intel_pmc_core: Add support for Alder Lake PCH-P
> 
> Gayatri Kammela (5):
>   platform/x86: intel_pmc_core: Handle sub-states generically
>   platform/x86: intel_pmc_core: Show LPM residency in microseconds
>   platform/x86: intel_pmc_core: Get LPM requirements for Tiger Lake
>   platform/x86: intel_pmc_core: Add requirements file to debugfs
>   platform/x86: intel_pmc_core: Add LTR registers for Tiger Lake
> 
>  drivers/platform/x86/intel_pmc_core.c | 359 +++++++++++++++++++++++---
>  drivers/platform/x86/intel_pmc_core.h |  47 +++-
>  2 files changed, 370 insertions(+), 36 deletions(-)
>