[PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[David E. Box]

Adds debugfs access to registers in the Cannon Point PCH PMC that are
useful for debugging #SLP_S0 signal assertion and other low power related
activities. Device pm states are latched in these registers whenever the
package enters C10 and can be read from slp_s0_debug_status. The pm
states may also be latched by writing 1 to slp_s0_dbg_latch which will
immediately capture the current state on the next read of
slp_s0_debug_status. Also while in intel_pmc_core.h clean up spacing.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---

V2:
	Per Andy Shevchenko:
	- Clear latch bit after use
	- Pass pmc_dev as parameter
	- Use DEFINE_SHOW_ATTRIBUTE macro

 drivers/platform/x86/intel_pmc_core.c | 112 ++++++++++++++++++++++++++++++++++
 drivers/platform/x86/intel_pmc_core.h |  27 +++++---
 2 files changed, 132 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 43bbe74..ed40999 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -196,9 +196,64 @@ static const struct pmc_bit_map cnp_pfear_map[] = {
 	{}
 };
 
+static const struct pmc_bit_map cnp_slps0_dbg0_map[] = {
+	{"AUDIO_D3",		BIT(0)},
+	{"OTG_D3",		BIT(1)},
+	{"XHCI_D3",		BIT(2)},
+	{"LPIO_D3",		BIT(3)},
+	{"SDX_D3",		BIT(4)},
+	{"SATA_D3",		BIT(5)},
+	{"UFS0_D3",		BIT(6)},
+	{"UFS1_D3",		BIT(7)},
+	{"EMMC_D3",		BIT(8)},
+};
+
+static const struct pmc_bit_map cnp_slps0_dbg1_map[] = {
+	{"SDIO_PLL_OFF",	BIT(0)},
+	{"USB2_PLL_OFF",	BIT(1)},
+	{"AUDIO_PLL_OFF",	BIT(2)},
+	{"OC_PLL_OFF",		BIT(3)},
+	{"MAIN_PLL_OFF",	BIT(4)},
+	{"XOSC_OFF",		BIT(5)},
+	{"LPC_CLKS_GATED",	BIT(6)},
+	{"PCIE_CLKREQS_IDLE",	BIT(7)},
+	{"AUDIO_ROSC_OFF",	BIT(8)},
+	{"HPET_XOSC_CLK_REQ",	BIT(9)},
+	{"PMC_ROSC_SLOW_CLK",	BIT(10)},
+	{"AON2_ROSC_GATED",	BIT(11)},
+	{"CLKACKS_DEASSERTED",	BIT(12)},
+};
+
+static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
+	{"MPHY_CORE_GATED",	BIT(0)},
+	{"CSME_GATED",		BIT(1)},
+	{"USB2_SUS_GATED",	BIT(2)},
+	{"DYN_FLEX_IO_IDLE",	BIT(3)},
+	{"GBE_NO_LINK",		BIT(4)},
+	{"THERM_SEN_DISABLED",	BIT(5)},
+	{"PCIE_LOW_POWER",	BIT(6)},
+	{"ISH_VNNAON_REQ_ACT",	BIT(7)},
+	{"ISH_VNN_REQ_ACT",	BIT(8)},
+	{"CNV_VNNAON_REQ_ACT",	BIT(9)},
+	{"CNV_VNN_REQ_ACT",	BIT(10)},
+	{"NPK_VNNON_REQ_ACT",	BIT(11)},
+	{"PMSYNC_STATE_IDLE",	BIT(12)},
+	{"ALST_GT_THRES",	BIT(13)},
+	{"PMC_ARC_PG_READY",	BIT(14)},
+};
+
+static const struct slps0_dbg_map cnp_slps0_dbg_maps[] = {
+	{cnp_slps0_dbg0_map, ARRAY_SIZE(cnp_slps0_dbg0_map)},
+	{cnp_slps0_dbg1_map, ARRAY_SIZE(cnp_slps0_dbg1_map)},
+	{cnp_slps0_dbg2_map, ARRAY_SIZE(cnp_slps0_dbg2_map)},
+};
+
 static const struct pmc_reg_map cnp_reg_map = {
 	.pfear_sts = cnp_pfear_map,
 	.slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
+	.slps0_dbg_maps = cnp_slps0_dbg_maps,
+	.slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
+	.slps0_dbg_num = ARRAY_SIZE(cnp_slps0_dbg_maps),
 	.ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
 	.regmap_length = CNP_PMC_MMIO_REG_LEN,
 	.ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
@@ -252,6 +307,8 @@ static int pmc_core_check_read_lock_bit(void)
 }
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
+static bool slps0_dbg_latch;
+
 static void pmc_core_display_map(struct seq_file *s, int index,
 				 u8 pf_reg, const struct pmc_bit_map *pf_map)
 {
@@ -481,6 +538,52 @@ static const struct file_operations pmc_core_ltr_ignore_ops = {
 	.release        = single_release,
 };
 
+static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev, bool reset)
+{
+	const struct pmc_reg_map *map = pmcdev->map;
+	u32 fd;
+
+	mutex_lock(&pmcdev->lock);
+
+	if (!reset && !slps0_dbg_latch)
+		goto out_unlock;
+
+	fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
+	reset ? (fd &= ~CNP_PMC_LATCH_SLPS0_EVENTS) :
+		(fd |= CNP_PMC_LATCH_SLPS0_EVENTS);
+	pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
+
+	slps0_dbg_latch = 0;
+
+out_unlock:
+	mutex_unlock(&pmcdev->lock);
+}
+
+static int pmc_core_slps0_dbg_show(struct seq_file *s, void *unused)
+{
+	struct pmc_dev *pmcdev = s ? s->private : &pmc;
+	const struct slps0_dbg_map *maps = pmcdev->map->slps0_dbg_maps;
+	int num_slps0_dbg_regs = pmcdev->map->slps0_dbg_num;
+	int i, offset;
+	u32 data;
+
+	pmc_core_slps0_dbg_latch(pmcdev, false);
+	offset = pmcdev->map->slps0_dbg_offset;
+	while (num_slps0_dbg_regs--) {
+		data = pmc_core_reg_read(pmcdev, offset);
+		offset += 4;
+		for (i = 0; i < maps->size; i++)
+			seq_printf(s, "SLP_S0_DBG: %-32s\tState: %s\n",
+				   maps->slps0_dbg_sts[i].name,
+				   data & maps->slps0_dbg_sts[i].bit_mask ?
+				   "Yes" : "No");
+		maps++;
+	}
+	pmc_core_slps0_dbg_latch(pmcdev, true);
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);
+
 static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
 {
 	debugfs_remove_recursive(pmcdev->dbgfs_dir);
@@ -514,6 +617,15 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
 				    0444, dir, pmcdev,
 				    &pmc_core_mphy_pg_ops);
 
+	if (pmcdev->map->slps0_dbg_maps) {
+		debugfs_create_file("slp_s0_debug_status", 0444,
+				    dir, pmcdev,
+				    &pmc_core_slps0_dbg_fops);
+
+		debugfs_create_bool("slp_s0_dbg_latch", 0644,
+				    dir, &slps0_dbg_latch);
+	}
+
 	return 0;
 }
 #else
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 5fa5f97..97e4b5a 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -124,27 +124,35 @@ enum ppfear_regs {
 #define SPT_PMC_BIT_MPHY_CMN_LANE3		BIT(3)
 
 /* Cannonlake Power Management Controller register offsets */
-#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET      0x193C
-#define CNP_PMC_LTR_IGNORE_OFFSET              0x1B0C
-#define CNP_PMC_PM_CFG_OFFSET                  0x1818
+#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET	0x193C
+#define CNP_PMC_LTR_IGNORE_OFFSET		0x1B0C
+#define CNP_PMC_PM_CFG_OFFSET			0x1818
+#define CNP_PMC_SLPS0_DBG_OFFSET		0x10B4
 /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */
-#define CNP_PMC_HOST_PPFEAR0A                  0x1D90
+#define CNP_PMC_HOST_PPFEAR0A			0x1D90
 
-#define CNP_PMC_MMIO_REG_LEN                   0x2000
-#define CNP_PPFEAR_NUM_ENTRIES                 8
-#define CNP_PMC_READ_DISABLE_BIT               22
+#define CNP_PMC_MMIO_REG_LEN			0x2000
+#define CNP_PPFEAR_NUM_ENTRIES			8
+#define CNP_PMC_READ_DISABLE_BIT		22
+#define CNP_PMC_LATCH_SLPS0_EVENTS		BIT(31)
 
 struct pmc_bit_map {
 	const char *name;
 	u32 bit_mask;
 };
 
+struct slps0_dbg_map {
+	const struct pmc_bit_map *slps0_dbg_sts;
+	int size;
+};
+
 /**
  * struct pmc_reg_map - Structure used to define parameter unique to a
 			PCH family
  * @pfear_sts:		Maps name of IP block to PPFEAR* bit
  * @mphy_sts:		Maps name of MPHY lane to MPHY status lane status bit
  * @pll_sts:		Maps name of PLL to corresponding bit status
+ * @slps0_dbg_maps:	Array of SLP_S0_DBG* registers containing debug info
  * @slp_s0_offset:	PWRMBASE offset to read SLP_S0 residency
  * @ltr_ignore_offset:	PWRMBASE offset to read/write LTR ignore bit
  * @regmap_length:	Length of memory to map from PWRMBASE address to access
@@ -153,6 +161,8 @@ struct pmc_bit_map {
  *			PPFEAR
  * @pm_cfg_offset:	PWRMBASE offset to PM_CFG register
  * @pm_read_disable_bit: Bit index to read PMC_READ_DISABLE
+ * @slps0_dbg_offset:	PWRMBASE offset to SLP_S0_DEBUG_REG*
+ * @slps0_dbg_num:	Number of SLP_S0_DEBUG_REG registers
  *
  * Each PCH has unique set of register offsets and bit indexes. This structure
  * captures them to have a common implementation.
@@ -161,6 +171,7 @@ struct pmc_reg_map {
 	const struct pmc_bit_map *pfear_sts;
 	const struct pmc_bit_map *mphy_sts;
 	const struct pmc_bit_map *pll_sts;
+	const struct slps0_dbg_map *slps0_dbg_maps;
 	const u32 slp_s0_offset;
 	const u32 ltr_ignore_offset;
 	const int regmap_length;
@@ -168,6 +179,8 @@ struct pmc_reg_map {
 	const int ppfear_buckets;
 	const u32 pm_cfg_offset;
 	const int pm_read_disable_bit;
+	const u32 slps0_dbg_offset;
+	const int slps0_dbg_num;
 };
 
 /**
-- 
2.7.4

Re: [PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[Rajneesh Bhardwaj]

On Thu, May 24, 2018 at 06:10:56PM -0700, David E. Box wrote:

Thanks for sending this, Dave. Few comments below.

> Adds debugfs access to registers in the Cannon Point PCH PMC that are

Please use Cannonlake PCH.

> useful for debugging #SLP_S0 signal assertion and other low power related

assertion failures and other low power debug.

> activities. Device pm states are latched in these registers whenever the
> package enters C10 and can be read from slp_s0_debug_status. The pm
> states may also be latched by writing 1 to slp_s0_dbg_latch which will
> immediately capture the current state on the next read of
> slp_s0_debug_status. Also while in intel_pmc_core.h clean up spacing.
> 

Initially, unless the latch bit is not set the debugfs file does not show
any information as expected but once you write Y to latch file, the debugfs
file continues to show blockers even though you write N back there or unload
- reload the driver. Please fix this.

> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
> 
> V2:
> 	Per Andy Shevchenko:
> 	- Clear latch bit after use
> 	- Pass pmc_dev as parameter
> 	- Use DEFINE_SHOW_ATTRIBUTE macro
> 
>  drivers/platform/x86/intel_pmc_core.c | 112 ++++++++++++++++++++++++++++++++++
>  drivers/platform/x86/intel_pmc_core.h |  27 +++++---
>  2 files changed, 132 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 43bbe74..ed40999 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -196,9 +196,64 @@ static const struct pmc_bit_map cnp_pfear_map[] = {
>  	{}
>  };
>  
> +static const struct pmc_bit_map cnp_slps0_dbg0_map[] = {
> +	{"AUDIO_D3",		BIT(0)},
> +	{"OTG_D3",		BIT(1)},
> +	{"XHCI_D3",		BIT(2)},
> +	{"LPIO_D3",		BIT(3)},
> +	{"SDX_D3",		BIT(4)},
> +	{"SATA_D3",		BIT(5)},
> +	{"UFS0_D3",		BIT(6)},
> +	{"UFS1_D3",		BIT(7)},
> +	{"EMMC_D3",		BIT(8)},
> +};
> +
> +static const struct pmc_bit_map cnp_slps0_dbg1_map[] = {
> +	{"SDIO_PLL_OFF",	BIT(0)},
> +	{"USB2_PLL_OFF",	BIT(1)},
> +	{"AUDIO_PLL_OFF",	BIT(2)},
> +	{"OC_PLL_OFF",		BIT(3)},

Please rename to ISCLK_OC as it is the preferred nomenclature for debug.

> +	{"MAIN_PLL_OFF",	BIT(4)},

Ditto, please use ISCLK_MAIN.

> +	{"XOSC_OFF",		BIT(5)},
> +	{"LPC_CLKS_GATED",	BIT(6)},
> +	{"PCIE_CLKREQS_IDLE",	BIT(7)},
> +	{"AUDIO_ROSC_OFF",	BIT(8)},
> +	{"HPET_XOSC_CLK_REQ",	BIT(9)},
> +	{"PMC_ROSC_SLOW_CLK",	BIT(10)},
> +	{"AON2_ROSC_GATED",	BIT(11)},
> +	{"CLKACKS_DEASSERTED",	BIT(12)},
> +};
> +
> +static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
> +	{"MPHY_CORE_GATED",	BIT(0)},
> +	{"CSME_GATED",		BIT(1)},
> +	{"USB2_SUS_GATED",	BIT(2)},
> +	{"DYN_FLEX_IO_IDLE",	BIT(3)},
> +	{"GBE_NO_LINK",		BIT(4)},
> +	{"THERM_SEN_DISABLED",	BIT(5)},
> +	{"PCIE_LOW_POWER",	BIT(6)},
> +	{"ISH_VNNAON_REQ_ACT",	BIT(7)},
> +	{"ISH_VNN_REQ_ACT",	BIT(8)},
> +	{"CNV_VNNAON_REQ_ACT",	BIT(9)},
> +	{"CNV_VNN_REQ_ACT",	BIT(10)},
> +	{"NPK_VNNON_REQ_ACT",	BIT(11)},
> +	{"PMSYNC_STATE_IDLE",	BIT(12)},
> +	{"ALST_GT_THRES",	BIT(13)},
> +	{"PMC_ARC_PG_READY",	BIT(14)},
> +};
> +
> +static const struct slps0_dbg_map cnp_slps0_dbg_maps[] = {
> +	{cnp_slps0_dbg0_map, ARRAY_SIZE(cnp_slps0_dbg0_map)},
> +	{cnp_slps0_dbg1_map, ARRAY_SIZE(cnp_slps0_dbg1_map)},
> +	{cnp_slps0_dbg2_map, ARRAY_SIZE(cnp_slps0_dbg2_map)},
> +};
> +
>  static const struct pmc_reg_map cnp_reg_map = {
>  	.pfear_sts = cnp_pfear_map,
>  	.slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
> +	.slps0_dbg_maps = cnp_slps0_dbg_maps,
> +	.slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
> +	.slps0_dbg_num = ARRAY_SIZE(cnp_slps0_dbg_maps),
>  	.ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
>  	.regmap_length = CNP_PMC_MMIO_REG_LEN,
>  	.ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
> @@ -252,6 +307,8 @@ static int pmc_core_check_read_lock_bit(void)
>  }
>  
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> +static bool slps0_dbg_latch;
> +
>  static void pmc_core_display_map(struct seq_file *s, int index,
>  				 u8 pf_reg, const struct pmc_bit_map *pf_map)
>  {
> @@ -481,6 +538,52 @@ static const struct file_operations pmc_core_ltr_ignore_ops = {
>  	.release        = single_release,
>  };
>  
> +static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev, bool reset)
> +{
> +	const struct pmc_reg_map *map = pmcdev->map;
> +	u32 fd;
> +
> +	mutex_lock(&pmcdev->lock);
> +
> +	if (!reset && !slps0_dbg_latch)
> +		goto out_unlock;
> +
> +	fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
> +	reset ? (fd &= ~CNP_PMC_LATCH_SLPS0_EVENTS) :
> +		(fd |= CNP_PMC_LATCH_SLPS0_EVENTS);
> +	pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
> +
> +	slps0_dbg_latch = 0;
> +
> +out_unlock:
> +	mutex_unlock(&pmcdev->lock);
> +}
> +
> +static int pmc_core_slps0_dbg_show(struct seq_file *s, void *unused)
> +{
> +	struct pmc_dev *pmcdev = s ? s->private : &pmc;
> +	const struct slps0_dbg_map *maps = pmcdev->map->slps0_dbg_maps;
> +	int num_slps0_dbg_regs = pmcdev->map->slps0_dbg_num;
> +	int i, offset;
> +	u32 data;
> +
> +	pmc_core_slps0_dbg_latch(pmcdev, false);
> +	offset = pmcdev->map->slps0_dbg_offset;
> +	while (num_slps0_dbg_regs--) {
> +		data = pmc_core_reg_read(pmcdev, offset);
> +		offset += 4;
> +		for (i = 0; i < maps->size; i++)
> +			seq_printf(s, "SLP_S0_DBG: %-32s\tState: %s\n",
> +				   maps->slps0_dbg_sts[i].name,
> +				   data & maps->slps0_dbg_sts[i].bit_mask ?
> +				   "Yes" : "No");

In current form, it looks pretty similar to pch_ip_power_gating_status
output. Since it helps in debug, please use Blocks_slp_s0: instead of State: 

> +		maps++;
> +	}
> +	pmc_core_slps0_dbg_latch(pmcdev, true);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);

When the latch bit is not set, can we format the output of this attribute
stating clearly whether the current blockers are "live" or the "last
captured at PC10 entry" ?

> +
>  static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
>  {
>  	debugfs_remove_recursive(pmcdev->dbgfs_dir);
> @@ -514,6 +617,15 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>  				    0444, dir, pmcdev,
>  				    &pmc_core_mphy_pg_ops);
>  
> +	if (pmcdev->map->slps0_dbg_maps) {
> +		debugfs_create_file("slp_s0_debug_status", 0444,

slp_s0_dbg_latch

> +				    dir, pmcdev,
> +				    &pmc_core_slps0_dbg_fops);
> +
> +		debugfs_create_bool("slp_s0_dbg_latch", 0644,
> +				    dir, &slps0_dbg_latch);
> +	}
> +
>  	return 0;
>  }
>  #else
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index 5fa5f97..97e4b5a 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -124,27 +124,35 @@ enum ppfear_regs {
>  #define SPT_PMC_BIT_MPHY_CMN_LANE3		BIT(3)
>  
>  /* Cannonlake Power Management Controller register offsets */
> -#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET      0x193C
> -#define CNP_PMC_LTR_IGNORE_OFFSET              0x1B0C
> -#define CNP_PMC_PM_CFG_OFFSET                  0x1818
> +#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET	0x193C
> +#define CNP_PMC_LTR_IGNORE_OFFSET		0x1B0C
> +#define CNP_PMC_PM_CFG_OFFSET			0x1818
> +#define CNP_PMC_SLPS0_DBG_OFFSET		0x10B4
>  /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */
> -#define CNP_PMC_HOST_PPFEAR0A                  0x1D90
> +#define CNP_PMC_HOST_PPFEAR0A			0x1D90
>  
> -#define CNP_PMC_MMIO_REG_LEN                   0x2000
> -#define CNP_PPFEAR_NUM_ENTRIES                 8
> -#define CNP_PMC_READ_DISABLE_BIT               22
> +#define CNP_PMC_MMIO_REG_LEN			0x2000
> +#define CNP_PPFEAR_NUM_ENTRIES			8
> +#define CNP_PMC_READ_DISABLE_BIT		22
> +#define CNP_PMC_LATCH_SLPS0_EVENTS		BIT(31)
>  
>  struct pmc_bit_map {
>  	const char *name;
>  	u32 bit_mask;
>  };
>  
> +struct slps0_dbg_map {
> +	const struct pmc_bit_map *slps0_dbg_sts;
> +	int size;
> +};
> +
>  /**
>   * struct pmc_reg_map - Structure used to define parameter unique to a
>  			PCH family
>   * @pfear_sts:		Maps name of IP block to PPFEAR* bit
>   * @mphy_sts:		Maps name of MPHY lane to MPHY status lane status bit
>   * @pll_sts:		Maps name of PLL to corresponding bit status
> + * @slps0_dbg_maps:	Array of SLP_S0_DBG* registers containing debug info
>   * @slp_s0_offset:	PWRMBASE offset to read SLP_S0 residency
>   * @ltr_ignore_offset:	PWRMBASE offset to read/write LTR ignore bit
>   * @regmap_length:	Length of memory to map from PWRMBASE address to access
> @@ -153,6 +161,8 @@ struct pmc_bit_map {
>   *			PPFEAR
>   * @pm_cfg_offset:	PWRMBASE offset to PM_CFG register
>   * @pm_read_disable_bit: Bit index to read PMC_READ_DISABLE
> + * @slps0_dbg_offset:	PWRMBASE offset to SLP_S0_DEBUG_REG*
> + * @slps0_dbg_num:	Number of SLP_S0_DEBUG_REG registers
>   *
>   * Each PCH has unique set of register offsets and bit indexes. This structure
>   * captures them to have a common implementation.
> @@ -161,6 +171,7 @@ struct pmc_reg_map {
>  	const struct pmc_bit_map *pfear_sts;
>  	const struct pmc_bit_map *mphy_sts;
>  	const struct pmc_bit_map *pll_sts;
> +	const struct slps0_dbg_map *slps0_dbg_maps;
>  	const u32 slp_s0_offset;
>  	const u32 ltr_ignore_offset;
>  	const int regmap_length;
> @@ -168,6 +179,8 @@ struct pmc_reg_map {
>  	const int ppfear_buckets;
>  	const u32 pm_cfg_offset;
>  	const int pm_read_disable_bit;
> +	const u32 slps0_dbg_offset;
> +	const int slps0_dbg_num;
>  };
>  
>  /**
> -- 
> 2.7.4
> 

-- 
Best Regards,
Rajneesh

Re: [PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[David E. Box]

Hi Rajneesh,

On Mon, 2018-05-28 at 12:30 +0530, Rajneesh Bhardwaj wrote:
> On Thu, May 24, 2018 at 06:10:56PM -0700, David E. Box wrote:
> 
> Thanks for sending this, Dave. Few comments below.
> 
> > Adds debugfs access to registers in the Cannon Point PCH PMC that
> > are
> 
> Please use Cannonlake PCH.
> 
> > useful for debugging #SLP_S0 signal assertion and other low power
> > related
> 
> assertion failures and other low power debug.
> 
> > activities. Device pm states are latched in these registers
> > whenever the
> > package enters C10 and can be read from slp_s0_debug_status. The pm
> > states may also be latched by writing 1 to slp_s0_dbg_latch which
> > will
> > immediately capture the current state on the next read of
> > slp_s0_debug_status. Also while in intel_pmc_core.h clean up
> > spacing.
> > 
> 
> Initially, unless the latch bit is not set the debugfs file does not
> show
> any information as expected but once you write Y to latch file, the
> debugfs
> file continues to show blockers even though you write N back there or
> unload
> - reload the driver. Please fix this.

See comments below

> 
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> > 
> > V2:
> > 	Per Andy Shevchenko:
> > 	- Clear latch bit after use
> > 	- Pass pmc_dev as parameter
> > 	- Use DEFINE_SHOW_ATTRIBUTE macro
> > 
> >  drivers/platform/x86/intel_pmc_core.c | 112
> > ++++++++++++++++++++++++++++++++++
> >  drivers/platform/x86/intel_pmc_core.h |  27 +++++---
> >  2 files changed, 132 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > b/drivers/platform/x86/intel_pmc_core.c
> > index 43bbe74..ed40999 100644
> > --- a/drivers/platform/x86/intel_pmc_core.c
> > +++ b/drivers/platform/x86/intel_pmc_core.c
> > @@ -196,9 +196,64 @@ static const struct pmc_bit_map
> > cnp_pfear_map[] = {
> >  	{}
> >  };
> >  
> > +static const struct pmc_bit_map cnp_slps0_dbg0_map[] = {
> > +	{"AUDIO_D3",		BIT(0)},
> > +	{"OTG_D3",		BIT(1)},
> > +	{"XHCI_D3",		BIT(2)},
> > +	{"LPIO_D3",		BIT(3)},
> > +	{"SDX_D3",		BIT(4)},
> > +	{"SATA_D3",		BIT(5)},
> > +	{"UFS0_D3",		BIT(6)},
> > +	{"UFS1_D3",		BIT(7)},
> > +	{"EMMC_D3",		BIT(8)},
> > +};
> > +
> > +static const struct pmc_bit_map cnp_slps0_dbg1_map[] = {
> > +	{"SDIO_PLL_OFF",	BIT(0)},
> > +	{"USB2_PLL_OFF",	BIT(1)},
> > +	{"AUDIO_PLL_OFF",	BIT(2)},
> > +	{"OC_PLL_OFF",		BIT(3)},
> 
> Please rename to ISCLK_OC as it is the preferred nomenclature for
> debug.

Okay

> 
> > +	{"MAIN_PLL_OFF",	BIT(4)},
> 
> Ditto, please use ISCLK_MAIN.
> 
> > +	{"XOSC_OFF",		BIT(5)},
> > +	{"LPC_CLKS_GATED",	BIT(6)},
> > +	{"PCIE_CLKREQS_IDLE",	BIT(7)},
> > +	{"AUDIO_ROSC_OFF",	BIT(8)},
> > +	{"HPET_XOSC_CLK_REQ",	BIT(9)},
> > +	{"PMC_ROSC_SLOW_CLK",	BIT(10)},
> > +	{"AON2_ROSC_GATED",	BIT(11)},
> > +	{"CLKACKS_DEASSERTED",	BIT(12)},
> > +};
> > +
> > +static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
> > +	{"MPHY_CORE_GATED",	BIT(0)},
> > +	{"CSME_GATED",		BIT(1)},
> > +	{"USB2_SUS_GATED",	BIT(2)},
> > +	{"DYN_FLEX_IO_IDLE",	BIT(3)},
> > +	{"GBE_NO_LINK",		BIT(4)},
> > +	{"THERM_SEN_DISABLED",	BIT(5)},
> > +	{"PCIE_LOW_POWER",	BIT(6)},
> > +	{"ISH_VNNAON_REQ_ACT",	BIT(7)},
> > +	{"ISH_VNN_REQ_ACT",	BIT(8)},
> > +	{"CNV_VNNAON_REQ_ACT",	BIT(9)},
> > +	{"CNV_VNN_REQ_ACT",	BIT(10)},
> > +	{"NPK_VNNON_REQ_ACT",	BIT(11)},
> > +	{"PMSYNC_STATE_IDLE",	BIT(12)},
> > +	{"ALST_GT_THRES",	BIT(13)},
> > +	{"PMC_ARC_PG_READY",	BIT(14)},
> > +};
> > +
> > +static const struct slps0_dbg_map cnp_slps0_dbg_maps[] = {
> > +	{cnp_slps0_dbg0_map, ARRAY_SIZE(cnp_slps0_dbg0_map)},
> > +	{cnp_slps0_dbg1_map, ARRAY_SIZE(cnp_slps0_dbg1_map)},
> > +	{cnp_slps0_dbg2_map, ARRAY_SIZE(cnp_slps0_dbg2_map)},
> > +};
> > +
> >  static const struct pmc_reg_map cnp_reg_map = {
> >  	.pfear_sts = cnp_pfear_map,
> >  	.slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
> > +	.slps0_dbg_maps = cnp_slps0_dbg_maps,
> > +	.slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
> > +	.slps0_dbg_num = ARRAY_SIZE(cnp_slps0_dbg_maps),
> >  	.ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
> >  	.regmap_length = CNP_PMC_MMIO_REG_LEN,
> >  	.ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
> > @@ -252,6 +307,8 @@ static int pmc_core_check_read_lock_bit(void)
> >  }
> >  
> >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> > +static bool slps0_dbg_latch;
> > +
> >  static void pmc_core_display_map(struct seq_file *s, int index,
> >  				 u8 pf_reg, const struct
> > pmc_bit_map *pf_map)
> >  {
> > @@ -481,6 +538,52 @@ static const struct file_operations
> > pmc_core_ltr_ignore_ops = {
> >  	.release        = single_release,
> >  };
> >  
> > +static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev, bool
> > reset)
> > +{
> > +	const struct pmc_reg_map *map = pmcdev->map;
> > +	u32 fd;
> > +
> > +	mutex_lock(&pmcdev->lock);
> > +
> > +	if (!reset && !slps0_dbg_latch)
> > +		goto out_unlock;
> > +
> > +	fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
> > +	reset ? (fd &= ~CNP_PMC_LATCH_SLPS0_EVENTS) :
> > +		(fd |= CNP_PMC_LATCH_SLPS0_EVENTS);
> > +	pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
> > +
> > +	slps0_dbg_latch = 0;
> > +
> > +out_unlock:
> > +	mutex_unlock(&pmcdev->lock);
> > +}
> > +
> > +static int pmc_core_slps0_dbg_show(struct seq_file *s, void
> > *unused)
> > +{
> > +	struct pmc_dev *pmcdev = s ? s->private : &pmc;
> > +	const struct slps0_dbg_map *maps = pmcdev->map-
> > >slps0_dbg_maps;
> > +	int num_slps0_dbg_regs = pmcdev->map->slps0_dbg_num;
> > +	int i, offset;
> > +	u32 data;
> > +
> > +	pmc_core_slps0_dbg_latch(pmcdev, false);
> > +	offset = pmcdev->map->slps0_dbg_offset;
> > +	while (num_slps0_dbg_regs--) {
> > +		data = pmc_core_reg_read(pmcdev, offset);
> > +		offset += 4;
> > +		for (i = 0; i < maps->size; i++)
> > +			seq_printf(s, "SLP_S0_DBG: %-32s\tState:
> > %s\n",
> > +				   maps->slps0_dbg_sts[i].name,
> > +				   data & maps-
> > >slps0_dbg_sts[i].bit_mask ?
> > +				   "Yes" : "No");
> 
> In current form, it looks pretty similar to
> pch_ip_power_gating_status
> output. Since it helps in debug, please use Blocks_slp_s0: instead of
> State: 
> 

That may not be true. What blocks or doesn't can be configured by the
OEM.

> > +		maps++;
> > +	}
> > +	pmc_core_slps0_dbg_latch(pmcdev, true);
> > +	return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);
> 
> When the latch bit is not set, can we format the output of this
> attribute
> stating clearly whether the current blockers are "live" or the "last
> captured at PC10 entry" ?

There is no "live". It's always the last captured state which is why
the status doesn't update the way you expect above. With V2 the latch
is automatically reset after every read.

When the latch isn't set, any updates would indeed be from PC10
transitions, but you can't be sure this isn't the last forced latch
state unless you monitor PC10 residency and assume that the values were
updated when the count changes. Even with the latch, if there's a PC10
transition that occurs after the bit is written but before the
registers are read, the values may be from that transition, not the
latch.

> > +
> >  static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
> >  {
> >  	debugfs_remove_recursive(pmcdev->dbgfs_dir);
> > @@ -514,6 +617,15 @@ static int pmc_core_dbgfs_register(struct
> > pmc_dev *pmcdev)
> >  				    0444, dir, pmcdev,
> >  				    &pmc_core_mphy_pg_ops);
> >  
> > +	if (pmcdev->map->slps0_dbg_maps) {
> > +		debugfs_create_file("slp_s0_debug_status", 0444,
> 
> slp_s0_dbg_latch
> 
> > +				    dir, pmcdev,
> > +				    &pmc_core_slps0_dbg_fops);
> > +
> > +		debugfs_create_bool("slp_s0_dbg_latch", 0644,
> > +				    dir, &slps0_dbg_latch);
> > +	}
> > +
> >  	return 0;
> >  }
> >  #else
> > diff --git a/drivers/platform/x86/intel_pmc_core.h
> > b/drivers/platform/x86/intel_pmc_core.h
> > index 5fa5f97..97e4b5a 100644
> > --- a/drivers/platform/x86/intel_pmc_core.h
> > +++ b/drivers/platform/x86/intel_pmc_core.h
> > @@ -124,27 +124,35 @@ enum ppfear_regs {
> >  #define SPT_PMC_BIT_MPHY_CMN_LANE3		BIT(3)
> >  
> >  /* Cannonlake Power Management Controller register offsets */
> > -#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET      0x193C
> > -#define CNP_PMC_LTR_IGNORE_OFFSET              0x1B0C
> > -#define CNP_PMC_PM_CFG_OFFSET                  0x1818
> > +#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET	0x193C
> > +#define CNP_PMC_LTR_IGNORE_OFFSET		0x1B0C
> > +#define CNP_PMC_PM_CFG_OFFSET			0x1818
> > +#define CNP_PMC_SLPS0_DBG_OFFSET		0x10B4
> >  /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */
> > -#define CNP_PMC_HOST_PPFEAR0A                  0x1D90
> > +#define CNP_PMC_HOST_PPFEAR0A			0x1D90
> >  
> > -#define CNP_PMC_MMIO_REG_LEN                   0x2000
> > -#define CNP_PPFEAR_NUM_ENTRIES                 8
> > -#define CNP_PMC_READ_DISABLE_BIT               22
> > +#define CNP_PMC_MMIO_REG_LEN			0x2000
> > +#define CNP_PPFEAR_NUM_ENTRIES			8
> > +#define CNP_PMC_READ_DISABLE_BIT		22
> > +#define CNP_PMC_LATCH_SLPS0_EVENTS		BIT(31)
> >  
> >  struct pmc_bit_map {
> >  	const char *name;
> >  	u32 bit_mask;
> >  };
> >  
> > +struct slps0_dbg_map {
> > +	const struct pmc_bit_map *slps0_dbg_sts;
> > +	int size;
> > +};
> > +
> >  /**
> >   * struct pmc_reg_map - Structure used to define parameter unique
> > to a
> >  			PCH family
> >   * @pfear_sts:		Maps name of IP block to PPFEAR* bit
> >   * @mphy_sts:		Maps name of MPHY lane to MPHY status
> > lane status bit
> >   * @pll_sts:		Maps name of PLL to corresponding bit
> > status
> > + * @slps0_dbg_maps:	Array of SLP_S0_DBG* registers
> > containing debug info
> >   * @slp_s0_offset:	PWRMBASE offset to read SLP_S0 residency
> >   * @ltr_ignore_offset:	PWRMBASE offset to read/write LTR
> > ignore bit
> >   * @regmap_length:	Length of memory to map from PWRMBASE
> > address to access
> > @@ -153,6 +161,8 @@ struct pmc_bit_map {
> >   *			PPFEAR
> >   * @pm_cfg_offset:	PWRMBASE offset to PM_CFG register
> >   * @pm_read_disable_bit: Bit index to read PMC_READ_DISABLE
> > + * @slps0_dbg_offset:	PWRMBASE offset to SLP_S0_DEBUG_REG*
> > + * @slps0_dbg_num:	Number of SLP_S0_DEBUG_REG registers
> >   *
> >   * Each PCH has unique set of register offsets and bit indexes.
> > This structure
> >   * captures them to have a common implementation.
> > @@ -161,6 +171,7 @@ struct pmc_reg_map {
> >  	const struct pmc_bit_map *pfear_sts;
> >  	const struct pmc_bit_map *mphy_sts;
> >  	const struct pmc_bit_map *pll_sts;
> > +	const struct slps0_dbg_map *slps0_dbg_maps;
> >  	const u32 slp_s0_offset;
> >  	const u32 ltr_ignore_offset;
> >  	const int regmap_length;
> > @@ -168,6 +179,8 @@ struct pmc_reg_map {
> >  	const int ppfear_buckets;
> >  	const u32 pm_cfg_offset;
> >  	const int pm_read_disable_bit;
> > +	const u32 slps0_dbg_offset;
> > +	const int slps0_dbg_num;
> >  };
> >  
> >  /**
> > -- 
> > 2.7.4
> > 
> 
> 

Re: [PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[Rajneesh Bhardwaj]

On Wed, May 30, 2018 at 03:53:12AM -0700, David E. Box wrote:

Hi Dave,

> Hi Rajneesh,
> 
> On Mon, 2018-05-28 at 12:30 +0530, Rajneesh Bhardwaj wrote:
> > On Thu, May 24, 2018 at 06:10:56PM -0700, David E. Box wrote:
> > 
> > Thanks for sending this, Dave. Few comments below.
> > 
> > > Adds debugfs access to registers in the Cannon Point PCH PMC that
> > > are
> > 
> > Please use Cannonlake PCH.
> > 
> > > useful for debugging #SLP_S0 signal assertion and other low power
> > > related
> > 
> > assertion failures and other low power debug.
> > 
> > > activities. Device pm states are latched in these registers
> > > whenever the
> > > package enters C10 and can be read from slp_s0_debug_status. The pm
> > > states may also be latched by writing 1 to slp_s0_dbg_latch which
> > > will
> > > immediately capture the current state on the next read of
> > > slp_s0_debug_status. Also while in intel_pmc_core.h clean up
> > > spacing.
> > > 
> > 
> > Initially, unless the latch bit is not set the debugfs file does not
> > show
> > any information as expected but once you write Y to latch file, the
> > debugfs
> > file continues to show blockers even though you write N back there or
> > unload
> > - reload the driver. Please fix this.
> 
> See comments below
> 
> > 
> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > ---
> > > 
> > > V2:
> > > 	Per Andy Shevchenko:
> > > 	- Clear latch bit after use
> > > 	- Pass pmc_dev as parameter
> > > 	- Use DEFINE_SHOW_ATTRIBUTE macro
> > > 
> > >  drivers/platform/x86/intel_pmc_core.c | 112
> > > ++++++++++++++++++++++++++++++++++
> > >  drivers/platform/x86/intel_pmc_core.h |  27 +++++---
> > >  2 files changed, 132 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > > b/drivers/platform/x86/intel_pmc_core.c
> > > index 43bbe74..ed40999 100644
> > > --- a/drivers/platform/x86/intel_pmc_core.c
> > > +++ b/drivers/platform/x86/intel_pmc_core.c
> > > @@ -196,9 +196,64 @@ static const struct pmc_bit_map
> > > cnp_pfear_map[] = {
> > >  	{}
> > >  };
> > >  
> > > +static const struct pmc_bit_map cnp_slps0_dbg0_map[] = {
> > > +	{"AUDIO_D3",		BIT(0)},
> > > +	{"OTG_D3",		BIT(1)},
> > > +	{"XHCI_D3",		BIT(2)},
> > > +	{"LPIO_D3",		BIT(3)},
> > > +	{"SDX_D3",		BIT(4)},
> > > +	{"SATA_D3",		BIT(5)},
> > > +	{"UFS0_D3",		BIT(6)},
> > > +	{"UFS1_D3",		BIT(7)},
> > > +	{"EMMC_D3",		BIT(8)},
> > > +};
> > > +
> > > +static const struct pmc_bit_map cnp_slps0_dbg1_map[] = {
> > > +	{"SDIO_PLL_OFF",	BIT(0)},
> > > +	{"USB2_PLL_OFF",	BIT(1)},
> > > +	{"AUDIO_PLL_OFF",	BIT(2)},
> > > +	{"OC_PLL_OFF",		BIT(3)},
> > 
> > Please rename to ISCLK_OC as it is the preferred nomenclature for
> > debug.
> 
> Okay
> 
> > 
> > > +	{"MAIN_PLL_OFF",	BIT(4)},
> > 
> > Ditto, please use ISCLK_MAIN.
> > 
> > > +	{"XOSC_OFF",		BIT(5)},
> > > +	{"LPC_CLKS_GATED",	BIT(6)},
> > > +	{"PCIE_CLKREQS_IDLE",	BIT(7)},
> > > +	{"AUDIO_ROSC_OFF",	BIT(8)},
> > > +	{"HPET_XOSC_CLK_REQ",	BIT(9)},
> > > +	{"PMC_ROSC_SLOW_CLK",	BIT(10)},
> > > +	{"AON2_ROSC_GATED",	BIT(11)},
> > > +	{"CLKACKS_DEASSERTED",	BIT(12)},
> > > +};
> > > +
> > > +static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
> > > +	{"MPHY_CORE_GATED",	BIT(0)},
> > > +	{"CSME_GATED",		BIT(1)},
> > > +	{"USB2_SUS_GATED",	BIT(2)},
> > > +	{"DYN_FLEX_IO_IDLE",	BIT(3)},
> > > +	{"GBE_NO_LINK",		BIT(4)},
> > > +	{"THERM_SEN_DISABLED",	BIT(5)},
> > > +	{"PCIE_LOW_POWER",	BIT(6)},
> > > +	{"ISH_VNNAON_REQ_ACT",	BIT(7)},
> > > +	{"ISH_VNN_REQ_ACT",	BIT(8)},
> > > +	{"CNV_VNNAON_REQ_ACT",	BIT(9)},
> > > +	{"CNV_VNN_REQ_ACT",	BIT(10)},
> > > +	{"NPK_VNNON_REQ_ACT",	BIT(11)},
> > > +	{"PMSYNC_STATE_IDLE",	BIT(12)},
> > > +	{"ALST_GT_THRES",	BIT(13)},
> > > +	{"PMC_ARC_PG_READY",	BIT(14)},
> > > +};
> > > +
> > > +static const struct slps0_dbg_map cnp_slps0_dbg_maps[] = {
> > > +	{cnp_slps0_dbg0_map, ARRAY_SIZE(cnp_slps0_dbg0_map)},
> > > +	{cnp_slps0_dbg1_map, ARRAY_SIZE(cnp_slps0_dbg1_map)},
> > > +	{cnp_slps0_dbg2_map, ARRAY_SIZE(cnp_slps0_dbg2_map)},
> > > +};
> > > +
> > >  static const struct pmc_reg_map cnp_reg_map = {
> > >  	.pfear_sts = cnp_pfear_map,
> > >  	.slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
> > > +	.slps0_dbg_maps = cnp_slps0_dbg_maps,
> > > +	.slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
> > > +	.slps0_dbg_num = ARRAY_SIZE(cnp_slps0_dbg_maps),
> > >  	.ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
> > >  	.regmap_length = CNP_PMC_MMIO_REG_LEN,
> > >  	.ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
> > > @@ -252,6 +307,8 @@ static int pmc_core_check_read_lock_bit(void)
> > >  }
> > >  
> > >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> > > +static bool slps0_dbg_latch;
> > > +
> > >  static void pmc_core_display_map(struct seq_file *s, int index,
> > >  				 u8 pf_reg, const struct
> > > pmc_bit_map *pf_map)
> > >  {
> > > @@ -481,6 +538,52 @@ static const struct file_operations
> > > pmc_core_ltr_ignore_ops = {
> > >  	.release        = single_release,
> > >  };
> > >  
> > > +static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev, bool
> > > reset)
> > > +{
> > > +	const struct pmc_reg_map *map = pmcdev->map;
> > > +	u32 fd;
> > > +
> > > +	mutex_lock(&pmcdev->lock);
> > > +
> > > +	if (!reset && !slps0_dbg_latch)
> > > +		goto out_unlock;
> > > +
> > > +	fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
> > > +	reset ? (fd &= ~CNP_PMC_LATCH_SLPS0_EVENTS) :
> > > +		(fd |= CNP_PMC_LATCH_SLPS0_EVENTS);
> > > +	pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
> > > +
> > > +	slps0_dbg_latch = 0;
> > > +
> > > +out_unlock:
> > > +	mutex_unlock(&pmcdev->lock);
> > > +}
> > > +
> > > +static int pmc_core_slps0_dbg_show(struct seq_file *s, void
> > > *unused)
> > > +{
> > > +	struct pmc_dev *pmcdev = s ? s->private : &pmc;
> > > +	const struct slps0_dbg_map *maps = pmcdev->map-
> > > >slps0_dbg_maps;
> > > +	int num_slps0_dbg_regs = pmcdev->map->slps0_dbg_num;
> > > +	int i, offset;
> > > +	u32 data;
> > > +
> > > +	pmc_core_slps0_dbg_latch(pmcdev, false);
> > > +	offset = pmcdev->map->slps0_dbg_offset;
> > > +	while (num_slps0_dbg_regs--) {
> > > +		data = pmc_core_reg_read(pmcdev, offset);
> > > +		offset += 4;
> > > +		for (i = 0; i < maps->size; i++)
> > > +			seq_printf(s, "SLP_S0_DBG: %-32s\tState:
> > > %s\n",
> > > +				   maps->slps0_dbg_sts[i].name,
> > > +				   data & maps-
> > > >slps0_dbg_sts[i].bit_mask ?
> > > +				   "Yes" : "No");
> > 
> > In current form, it looks pretty similar to
> > pch_ip_power_gating_status
> > output. Since it helps in debug, please use Blocks_slp_s0: instead of
> > State: 
> > 
> 
> That may not be true. What blocks or doesn't can be configured by the
> OEM.

IIUC, The IPs exposed by these registers are the ones that the PMC always looks at
before making a decision to assert SLP_S0#. I dont think they can be changed
by OEMs in determining the S0ix flows.

> 
> > > +		maps++;
> > > +	}
> > > +	pmc_core_slps0_dbg_latch(pmcdev, true);
> > > +	return 0;
> > > +}
> > > +DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);
> > 
> > When the latch bit is not set, can we format the output of this
> > attribute
> > stating clearly whether the current blockers are "live" or the "last
> > captured at PC10 entry" ?
> 
> There is no "live". It's always the last captured state which is why
> the status doesn't update the way you expect above. With V2 the latch
> is automatically reset after every read.
> 
> When the latch isn't set, any updates would indeed be from PC10
> transitions, but you can't be sure this isn't the last forced latch
> state unless you monitor PC10 residency and assume that the values were
> updated when the count changes. Even with the latch, if there's a PC10
> transition that occurs after the bit is written but before the
> registers are read, the values may be from that transition, not the
> latch.


All default values are printed as "No". Here, we can rather print a message
that unless latch is set or Package enters c10 (which is unlikely unless
the display is off) the debugfs would not show any blockers.

In the current form this patch prints the output when we read the first time
before setting the latch.

SLP_S0_DBG: AUDIO_D3                            State: No
SLP_S0_DBG: OTG_D3                              State: No
---- xxx ---- snip ---- xxx ----

Once the latch bit is set the SLP_S0_DBG_X registers show proper values but
after explicitely setting latch as "N", the stale values are printed. The
system on which i tested never enters PC10 so once the latch is unset, the
values should be shown as default i.e. NO but that is not always correct as
you mentioned above.


IMO, a better approch would be to set/unset the latch internally when the
user does a read on SLP_S0_DEBUG_X registers rather than exposing a seperate
debugfs file because a write of 0 to the latch bit has no effect but it is a
requirement for the next read.

> 
> > > +
> > >  static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
> > >  {
> > >  	debugfs_remove_recursive(pmcdev->dbgfs_dir);
> > > @@ -514,6 +617,15 @@ static int pmc_core_dbgfs_register(struct
> > > pmc_dev *pmcdev)
> > >  				    0444, dir, pmcdev,
> > >  				    &pmc_core_mphy_pg_ops);
> > >  
> > > +	if (pmcdev->map->slps0_dbg_maps) {
> > > +		debugfs_create_file("slp_s0_debug_status", 0444,
> > 
> > slp_s0_dbg_latch
> > 
> > > +				    dir, pmcdev,
> > > +				    &pmc_core_slps0_dbg_fops);
> > > +
> > > +		debugfs_create_bool("slp_s0_dbg_latch", 0644,
> > > +				    dir, &slps0_dbg_latch);
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >  #else
> > > diff --git a/drivers/platform/x86/intel_pmc_core.h
> > > b/drivers/platform/x86/intel_pmc_core.h
> > > index 5fa5f97..97e4b5a 100644
> > > --- a/drivers/platform/x86/intel_pmc_core.h
> > > +++ b/drivers/platform/x86/intel_pmc_core.h
> > > @@ -124,27 +124,35 @@ enum ppfear_regs {
> > >  #define SPT_PMC_BIT_MPHY_CMN_LANE3		BIT(3)
> > >  
> > >  /* Cannonlake Power Management Controller register offsets */
> > > -#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET      0x193C
> > > -#define CNP_PMC_LTR_IGNORE_OFFSET              0x1B0C
> > > -#define CNP_PMC_PM_CFG_OFFSET                  0x1818
> > > +#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET	0x193C
> > > +#define CNP_PMC_LTR_IGNORE_OFFSET		0x1B0C
> > > +#define CNP_PMC_PM_CFG_OFFSET			0x1818
> > > +#define CNP_PMC_SLPS0_DBG_OFFSET		0x10B4
> > >  /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */
> > > -#define CNP_PMC_HOST_PPFEAR0A                  0x1D90
> > > +#define CNP_PMC_HOST_PPFEAR0A			0x1D90
> > >  
> > > -#define CNP_PMC_MMIO_REG_LEN                   0x2000
> > > -#define CNP_PPFEAR_NUM_ENTRIES                 8
> > > -#define CNP_PMC_READ_DISABLE_BIT               22
> > > +#define CNP_PMC_MMIO_REG_LEN			0x2000
> > > +#define CNP_PPFEAR_NUM_ENTRIES			8
> > > +#define CNP_PMC_READ_DISABLE_BIT		22
> > > +#define CNP_PMC_LATCH_SLPS0_EVENTS		BIT(31)
> > >  
> > >  struct pmc_bit_map {
> > >  	const char *name;
> > >  	u32 bit_mask;
> > >  };
> > >  
> > > +struct slps0_dbg_map {
> > > +	const struct pmc_bit_map *slps0_dbg_sts;
> > > +	int size;
> > > +};
> > > +
> > >  /**
> > >   * struct pmc_reg_map - Structure used to define parameter unique
> > > to a
> > >  			PCH family
> > >   * @pfear_sts:		Maps name of IP block to PPFEAR* bit
> > >   * @mphy_sts:		Maps name of MPHY lane to MPHY status
> > > lane status bit
> > >   * @pll_sts:		Maps name of PLL to corresponding bit
> > > status
> > > + * @slps0_dbg_maps:	Array of SLP_S0_DBG* registers
> > > containing debug info
> > >   * @slp_s0_offset:	PWRMBASE offset to read SLP_S0 residency
> > >   * @ltr_ignore_offset:	PWRMBASE offset to read/write LTR
> > > ignore bit
> > >   * @regmap_length:	Length of memory to map from PWRMBASE
> > > address to access
> > > @@ -153,6 +161,8 @@ struct pmc_bit_map {
> > >   *			PPFEAR
> > >   * @pm_cfg_offset:	PWRMBASE offset to PM_CFG register
> > >   * @pm_read_disable_bit: Bit index to read PMC_READ_DISABLE
> > > + * @slps0_dbg_offset:	PWRMBASE offset to SLP_S0_DEBUG_REG*
> > > + * @slps0_dbg_num:	Number of SLP_S0_DEBUG_REG registers
> > >   *
> > >   * Each PCH has unique set of register offsets and bit indexes.
> > > This structure
> > >   * captures them to have a common implementation.
> > > @@ -161,6 +171,7 @@ struct pmc_reg_map {
> > >  	const struct pmc_bit_map *pfear_sts;
> > >  	const struct pmc_bit_map *mphy_sts;
> > >  	const struct pmc_bit_map *pll_sts;
> > > +	const struct slps0_dbg_map *slps0_dbg_maps;
> > >  	const u32 slp_s0_offset;
> > >  	const u32 ltr_ignore_offset;
> > >  	const int regmap_length;
> > > @@ -168,6 +179,8 @@ struct pmc_reg_map {
> > >  	const int ppfear_buckets;
> > >  	const u32 pm_cfg_offset;
> > >  	const int pm_read_disable_bit;
> > > +	const u32 slps0_dbg_offset;
> > > +	const int slps0_dbg_num;
> > >  };
> > >  
> > >  /**
> > > -- 
> > > 2.7.4
> > > 
> > 
> > 

-- 
Best Regards,
Rajneesh

Re: [PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[David E. Box]

On Wed, 2018-05-30 at 17:03 +0530, Rajneesh Bhardwaj wrote:
> On Wed, May 30, 2018 at 03:53:12AM -0700, David E. Box wrote:
> 
> Hi Dave,
> 
> > Hi Rajneesh,
> > 
> > On Mon, 2018-05-28 at 12:30 +0530, Rajneesh Bhardwaj wrote:
> > > On Thu, May 24, 2018 at 06:10:56PM -0700, David E. Box wrote:
> > > 
> > > Thanks for sending this, Dave. Few comments below.
> > > 
> > > > Adds debugfs access to registers in the Cannon Point PCH PMC
> > > > that
> > > > are
> > > 
> > > Please use Cannonlake PCH.
> > > 
> > > > useful for debugging #SLP_S0 signal assertion and other low
> > > > power
> > > > related
> > > 
> > > assertion failures and other low power debug.
> > > 
> > > > activities. Device pm states are latched in these registers
> > > > whenever the
> > > > package enters C10 and can be read from slp_s0_debug_status.
> > > > The pm
> > > > states may also be latched by writing 1 to slp_s0_dbg_latch
> > > > which
> > > > will
> > > > immediately capture the current state on the next read of
> > > > slp_s0_debug_status. Also while in intel_pmc_core.h clean up
> > > > spacing.
> > > > 
> > > 
> > > Initially, unless the latch bit is not set the debugfs file does
> > > not
> > > show
> > > any information as expected but once you write Y to latch file,
> > > the
> > > debugfs
> > > file continues to show blockers even though you write N back
> > > there or
> > > unload
> > > - reload the driver. Please fix this.
> > 
> > See comments below
> > 
> > > 
> > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > ---
> > > > 
> > > > V2:
> > > > 	Per Andy Shevchenko:
> > > > 	- Clear latch bit after use
> > > > 	- Pass pmc_dev as parameter
> > > > 	- Use DEFINE_SHOW_ATTRIBUTE macro
> > > > 
> > > >  drivers/platform/x86/intel_pmc_core.c | 112
> > > > ++++++++++++++++++++++++++++++++++
> > > >  drivers/platform/x86/intel_pmc_core.h |  27 +++++---
> > > >  2 files changed, 132 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > > > b/drivers/platform/x86/intel_pmc_core.c
> > > > index 43bbe74..ed40999 100644
> > > > --- a/drivers/platform/x86/intel_pmc_core.c
> > > > +++ b/drivers/platform/x86/intel_pmc_core.c
> > > > @@ -196,9 +196,64 @@ static const struct pmc_bit_map
> > > > cnp_pfear_map[] = {
> > > >  	{}
> > > >  };
> > > >  
> > > > +static const struct pmc_bit_map cnp_slps0_dbg0_map[] = {
> > > > +	{"AUDIO_D3",		BIT(0)},
> > > > +	{"OTG_D3",		BIT(1)},
> > > > +	{"XHCI_D3",		BIT(2)},
> > > > +	{"LPIO_D3",		BIT(3)},
> > > > +	{"SDX_D3",		BIT(4)},
> > > > +	{"SATA_D3",		BIT(5)},
> > > > +	{"UFS0_D3",		BIT(6)},
> > > > +	{"UFS1_D3",		BIT(7)},
> > > > +	{"EMMC_D3",		BIT(8)},
> > > > +};
> > > > +
> > > > +static const struct pmc_bit_map cnp_slps0_dbg1_map[] = {
> > > > +	{"SDIO_PLL_OFF",	BIT(0)},
> > > > +	{"USB2_PLL_OFF",	BIT(1)},
> > > > +	{"AUDIO_PLL_OFF",	BIT(2)},
> > > > +	{"OC_PLL_OFF",		BIT(3)},
> > > 
> > > Please rename to ISCLK_OC as it is the preferred nomenclature for
> > > debug.
> > 
> > Okay
> > 
> > > 
> > > > +	{"MAIN_PLL_OFF",	BIT(4)},
> > > 
> > > Ditto, please use ISCLK_MAIN.
> > > 
> > > > +	{"XOSC_OFF",		BIT(5)},
> > > > +	{"LPC_CLKS_GATED",	BIT(6)},
> > > > +	{"PCIE_CLKREQS_IDLE",	BIT(7)},
> > > > +	{"AUDIO_ROSC_OFF",	BIT(8)},
> > > > +	{"HPET_XOSC_CLK_REQ",	BIT(9)},
> > > > +	{"PMC_ROSC_SLOW_CLK",	BIT(10)},
> > > > +	{"AON2_ROSC_GATED",	BIT(11)},
> > > > +	{"CLKACKS_DEASSERTED",	BIT(12)},
> > > > +};
> > > > +
> > > > +static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
> > > > +	{"MPHY_CORE_GATED",	BIT(0)},
> > > > +	{"CSME_GATED",		BIT(1)},
> > > > +	{"USB2_SUS_GATED",	BIT(2)},
> > > > +	{"DYN_FLEX_IO_IDLE",	BIT(3)},
> > > > +	{"GBE_NO_LINK",		BIT(4)},
> > > > +	{"THERM_SEN_DISABLED",	BIT(5)},
> > > > +	{"PCIE_LOW_POWER",	BIT(6)},
> > > > +	{"ISH_VNNAON_REQ_ACT",	BIT(7)},
> > > > +	{"ISH_VNN_REQ_ACT",	BIT(8)},
> > > > +	{"CNV_VNNAON_REQ_ACT",	BIT(9)},
> > > > +	{"CNV_VNN_REQ_ACT",	BIT(10)},
> > > > +	{"NPK_VNNON_REQ_ACT",	BIT(11)},
> > > > +	{"PMSYNC_STATE_IDLE",	BIT(12)},
> > > > +	{"ALST_GT_THRES",	BIT(13)},
> > > > +	{"PMC_ARC_PG_READY",	BIT(14)},
> > > > +};
> > > > +
> > > > +static const struct slps0_dbg_map cnp_slps0_dbg_maps[] = {
> > > > +	{cnp_slps0_dbg0_map, ARRAY_SIZE(cnp_slps0_dbg0_map)},
> > > > +	{cnp_slps0_dbg1_map, ARRAY_SIZE(cnp_slps0_dbg1_map)},
> > > > +	{cnp_slps0_dbg2_map, ARRAY_SIZE(cnp_slps0_dbg2_map)},
> > > > +};
> > > > +
> > > >  static const struct pmc_reg_map cnp_reg_map = {
> > > >  	.pfear_sts = cnp_pfear_map,
> > > >  	.slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
> > > > +	.slps0_dbg_maps = cnp_slps0_dbg_maps,
> > > > +	.slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
> > > > +	.slps0_dbg_num = ARRAY_SIZE(cnp_slps0_dbg_maps),
> > > >  	.ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
> > > >  	.regmap_length = CNP_PMC_MMIO_REG_LEN,
> > > >  	.ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
> > > > @@ -252,6 +307,8 @@ static int
> > > > pmc_core_check_read_lock_bit(void)
> > > >  }
> > > >  
> > > >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> > > > +static bool slps0_dbg_latch;
> > > > +
> > > >  static void pmc_core_display_map(struct seq_file *s, int
> > > > index,
> > > >  				 u8 pf_reg, const struct
> > > > pmc_bit_map *pf_map)
> > > >  {
> > > > @@ -481,6 +538,52 @@ static const struct file_operations
> > > > pmc_core_ltr_ignore_ops = {
> > > >  	.release        = single_release,
> > > >  };
> > > >  
> > > > +static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev,
> > > > bool
> > > > reset)
> > > > +{
> > > > +	const struct pmc_reg_map *map = pmcdev->map;
> > > > +	u32 fd;
> > > > +
> > > > +	mutex_lock(&pmcdev->lock);
> > > > +
> > > > +	if (!reset && !slps0_dbg_latch)
> > > > +		goto out_unlock;
> > > > +
> > > > +	fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
> > > > +	reset ? (fd &= ~CNP_PMC_LATCH_SLPS0_EVENTS) :
> > > > +		(fd |= CNP_PMC_LATCH_SLPS0_EVENTS);
> > > > +	pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
> > > > +
> > > > +	slps0_dbg_latch = 0;
> > > > +
> > > > +out_unlock:
> > > > +	mutex_unlock(&pmcdev->lock);
> > > > +}
> > > > +
> > > > +static int pmc_core_slps0_dbg_show(struct seq_file *s, void
> > > > *unused)
> > > > +{
> > > > +	struct pmc_dev *pmcdev = s ? s->private : &pmc;
> > > > +	const struct slps0_dbg_map *maps = pmcdev->map-
> > > > > slps0_dbg_maps;
> > > > 
> > > > +	int num_slps0_dbg_regs = pmcdev->map->slps0_dbg_num;
> > > > +	int i, offset;
> > > > +	u32 data;
> > > > +
> > > > +	pmc_core_slps0_dbg_latch(pmcdev, false);
> > > > +	offset = pmcdev->map->slps0_dbg_offset;
> > > > +	while (num_slps0_dbg_regs--) {
> > > > +		data = pmc_core_reg_read(pmcdev, offset);
> > > > +		offset += 4;
> > > > +		for (i = 0; i < maps->size; i++)
> > > > +			seq_printf(s, "SLP_S0_DBG: %-
> > > > 32s\tState:
> > > > %s\n",
> > > > +				   maps-
> > > > >slps0_dbg_sts[i].name,
> > > > +				   data & maps-
> > > > > slps0_dbg_sts[i].bit_mask ?
> > > > 
> > > > +				   "Yes" : "No");
> > > 
> > > In current form, it looks pretty similar to
> > > pch_ip_power_gating_status
> > > output. Since it helps in debug, please use Blocks_slp_s0:
> > > instead of
> > > State: 
> > > 
> > 
> > That may not be true. What blocks or doesn't can be configured by
> > the
> > OEM.
> 
> IIUC, The IPs exposed by these registers are the ones that the PMC
> always looks at
> before making a decision to assert SLP_S0#. I dont think they can be
> changed
> by OEMs in determining the S0ix flows.

I confirmed with the architect that the logic operates independently of
other BIOS settings OEMs can make to ignore these same IPs. Reporting
an IP as blocking on a system where the IP is being ignored would be
confusing. The reported state the IP is in remains correct and that is
what this shows.

> 
> > 
> > > > +		maps++;
> > > > +	}
> > > > +	pmc_core_slps0_dbg_latch(pmcdev, true);
> > > > +	return 0;
> > > > +}
> > > > +DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);
> > > 
> > > When the latch bit is not set, can we format the output of this
> > > attribute
> > > stating clearly whether the current blockers are "live" or the
> > > "last
> > > captured at PC10 entry" ?
> > 
> > There is no "live". It's always the last captured state which is
> > why
> > the status doesn't update the way you expect above. With V2 the
> > latch
> > is automatically reset after every read.
> > 
> > When the latch isn't set, any updates would indeed be from PC10
> > transitions, but you can't be sure this isn't the last forced latch
> > state unless you monitor PC10 residency and assume that the values
> > were
> > updated when the count changes. Even with the latch, if there's a
> > PC10
> > transition that occurs after the bit is written but before the
> > registers are read, the values may be from that transition, not the
> > latch.
> 
> 
> All default values are printed as "No". Here, we can rather print a
> message
> that unless latch is set or Package enters c10 (which is unlikely
> unless
> the display is off) the debugfs would not show any blockers.
> 
> In the current form this patch prints the output when we read the
> first time
> before setting the latch.
> 
> SLP_S0_DBG: AUDIO_D3                            State: No
> SLP_S0_DBG: OTG_D3                              State: No
> ---- xxx ---- snip ---- xxx ----
> 
> Once the latch bit is set the SLP_S0_DBG_X registers show proper
> values but
> after explicitely setting latch as "N", the stale values are
> printed. 
> The
> system on which i tested never enters PC10 so once the latch is
> unset, the
> values should be shown as default i.e. NO but that is not always
> correct as
> you mentioned above. 
> 
> 
> IMO, a better approch would be to set/unset the latch internally when
> the
> user does a read on SLP_S0_DEBUG_X registers rather than exposing a
> seperate
> debugfs file because a write of 0 to the latch bit has no effect but
> it is a
> requirement for the next read.

If the latch file is used, the code will already reset it automatically
by writing 0 after each read. Writing "N" is not necessary unless you
wrote "Y" and changed your mind. The reset does not set the values to
all "No". The registers, once updated the first time, always hold the
last saved state. 

If the latch file is not used the registers will only update whenever
the system enters PC10. This works on my system that does enter PC10.

What you suggest would limit the read to forcibly latched values only.
The greatest value of this file IMHO is in being able to see system
state after it enters PC10, a state you cannot guarantee the system
will be in if you only forcibly latch the state with the latch file.

David

Re: [PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[Andy Shevchenko]

On Fri, May 25, 2018 at 4:10 AM, David E. Box
<david.e.box@linux.intel.com> wrote:
> Adds debugfs access to registers in the Cannon Point PCH PMC that are
> useful for debugging #SLP_S0 signal assertion and other low power related
> activities. Device pm states are latched in these registers whenever the
> package enters C10 and can be read from slp_s0_debug_status. The pm
> states may also be latched by writing 1 to slp_s0_dbg_latch which will
> immediately capture the current state on the next read of
> slp_s0_debug_status. Also while in intel_pmc_core.h clean up spacing.
>

Thanks for an update. My comments below.

As far as I understand there is still ongoing discussion about the
approach when and how to show data. So I'll wait a bit for a
settlement between you, guys.

> +static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev, bool reset)
> +{
> +       const struct pmc_reg_map *map = pmcdev->map;
> +       u32 fd;
> +
> +       mutex_lock(&pmcdev->lock);
> +
> +       if (!reset && !slps0_dbg_latch)
> +               goto out_unlock;
> +
> +       fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);

> +       reset ? (fd &= ~CNP_PMC_LATCH_SLPS0_EVENTS) :
> +               (fd |= CNP_PMC_LATCH_SLPS0_EVENTS);

I would rather use classical pattern here, i.e.

if (reset)
 fd ...
else
 fd ...

> +       pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
> +
> +       slps0_dbg_latch = 0;
> +
> +out_unlock:
> +       mutex_unlock(&pmcdev->lock);
> +}

> +       struct pmc_dev *pmcdev = s ? s->private : &pmc;

I'm not sure why do we need such condition.

Simple

... pmcdev = s->private;

is enough.

>  /* Cannonlake Power Management Controller register offsets */
> -#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET      0x193C
> -#define CNP_PMC_LTR_IGNORE_OFFSET              0x1B0C
> -#define CNP_PMC_PM_CFG_OFFSET                  0x1818
> +#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET      0x193C
> +#define CNP_PMC_LTR_IGNORE_OFFSET              0x1B0C
> +#define CNP_PMC_PM_CFG_OFFSET                  0x1818

I have hard time to understand what is the difference here.
Either do another clean up patch (white spaces vs. tabs?) or leave it untouched.

>  /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */
> -#define CNP_PMC_HOST_PPFEAR0A                  0x1D90
> +#define CNP_PMC_HOST_PPFEAR0A                  0x1D90
>
> -#define CNP_PMC_MMIO_REG_LEN                   0x2000
> -#define CNP_PPFEAR_NUM_ENTRIES                 8
> -#define CNP_PMC_READ_DISABLE_BIT               22
> +#define CNP_PMC_MMIO_REG_LEN                   0x2000
> +#define CNP_PPFEAR_NUM_ENTRIES                 8
> +#define CNP_PMC_READ_DISABLE_BIT               22

Ditto.

> +struct slps0_dbg_map {
> +       const struct pmc_bit_map *slps0_dbg_sts;
> +       int size;
> +};

Didn't pay attention to this earlier. Why do we have a separate size
member? What does it define?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[David E. Box]

On Thu, 2018-05-31 at 21:38 +0300, Andy Shevchenko wrote:
> On Fri, May 25, 2018 at 4:10 AM, David E. Box
> <david.e.box@linux.intel.com> wrote:
> > Adds debugfs access to registers in the Cannon Point PCH PMC that
> > are
> > useful for debugging #SLP_S0 signal assertion and other low power
> > related
> > activities. Device pm states are latched in these registers
> > whenever the
> > package enters C10 and can be read from slp_s0_debug_status. The pm
> > states may also be latched by writing 1 to slp_s0_dbg_latch which
> > will
> > immediately capture the current state on the next read of
> > slp_s0_debug_status. Also while in intel_pmc_core.h clean up
> > spacing.
> > 
> 
> Thanks for an update. My comments below.
> 
> As far as I understand there is still ongoing discussion about the
> approach when and how to show data. So I'll wait a bit for a
> settlement between you, guys.
> 
> > +static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev, bool
> > reset)
> > +{
> > +       const struct pmc_reg_map *map = pmcdev->map;
> > +       u32 fd;
> > +
> > +       mutex_lock(&pmcdev->lock);
> > +
> > +       if (!reset && !slps0_dbg_latch)
> > +               goto out_unlock;
> > +
> > +       fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
> > +       reset ? (fd &= ~CNP_PMC_LATCH_SLPS0_EVENTS) :
> > +               (fd |= CNP_PMC_LATCH_SLPS0_EVENTS);
> 
> I would rather use classical pattern here, i.e.
> 
> if (reset)
>  fd ...
> else
>  fd ...
> 
> > +       pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
> > +
> > +       slps0_dbg_latch = 0;
> > +
> > +out_unlock:
> > +       mutex_unlock(&pmcdev->lock);
> > +}
> > +       struct pmc_dev *pmcdev = s ? s->private : &pmc;
> 
> I'm not sure why do we need such condition.
> 
> Simple
> 
> ... pmcdev = s->private;
> 
> is enough.
> 
> >  /* Cannonlake Power Management Controller register offsets */
> > -#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET      0x193C
> > -#define CNP_PMC_LTR_IGNORE_OFFSET              0x1B0C
> > -#define CNP_PMC_PM_CFG_OFFSET                  0x1818
> > +#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET      0x193C
> > +#define CNP_PMC_LTR_IGNORE_OFFSET              0x1B0C
> > +#define CNP_PMC_PM_CFG_OFFSET                  0x1818
> 
> I have hard time to understand what is the difference here.
> Either do another clean up patch (white spaces vs. tabs?) or leave it
> untouched.
> 
> >  /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */
> > -#define CNP_PMC_HOST_PPFEAR0A                  0x1D90
> > +#define CNP_PMC_HOST_PPFEAR0A                  0x1D90
> > 
> > -#define CNP_PMC_MMIO_REG_LEN                   0x2000
> > -#define CNP_PPFEAR_NUM_ENTRIES                 8
> > -#define CNP_PMC_READ_DISABLE_BIT               22
> > +#define CNP_PMC_MMIO_REG_LEN                   0x2000
> > +#define CNP_PPFEAR_NUM_ENTRIES                 8
> > +#define CNP_PMC_READ_DISABLE_BIT               22
> 
> Ditto.
> 
> > +struct slps0_dbg_map {
> > +       const struct pmc_bit_map *slps0_dbg_sts;
> > +       int size;
> > +};
> 
> Didn't pay attention to this earlier. Why do we have a separate size
> member? What does it define?

It holds the size of the pmc_bit_map array, assigned as shown here:

+static const struct slps0_dbg_map cnp_slps0_dbg_maps[] = {
+ {cnp_slps0_dbg0_map, ARRAY_SIZE(cnp_slps0_dbg0_map)},
+ {cnp_slps0_dbg1_map, ARRAY_SIZE(cnp_slps0_dbg1_map)},
+ {cnp_slps0_dbg2_map, ARRAY_SIZE(cnp_slps0_dbg2_map)},
+};


Okay on all other comments

Dave

Re: [PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[Andy Shevchenko]

On Fri, Jun 1, 2018 at 2:04 AM, David E. Box
<david.e.box@linux.intel.com> wrote:
> On Thu, 2018-05-31 at 21:38 +0300, Andy Shevchenko wrote:
>> On Fri, May 25, 2018 at 4:10 AM, David E. Box
>> <david.e.box@linux.intel.com> wrote:

>> > +struct slps0_dbg_map {
>> > +       const struct pmc_bit_map *slps0_dbg_sts;
>> > +       int size;
>> > +};
>>
>> Didn't pay attention to this earlier. Why do we have a separate size
>> member? What does it define?
>
> It holds the size of the pmc_bit_map array, assigned as shown here:
>
> +static const struct slps0_dbg_map cnp_slps0_dbg_maps[] = {
> + {cnp_slps0_dbg0_map, ARRAY_SIZE(cnp_slps0_dbg0_map)},
> + {cnp_slps0_dbg1_map, ARRAY_SIZE(cnp_slps0_dbg1_map)},
> + {cnp_slps0_dbg2_map, ARRAY_SIZE(cnp_slps0_dbg2_map)},
> +};

I see.
Please drop this and use a terminator instead like it's done for the
rest map tables.

-- 
With Best Regards,
Andy Shevchenko

[V3] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[Box, David E]

Adds debugfs access to registers in the Cannon Point PCH PMC that are
useful for debugging #SLP_S0 signal assertion and other low power relate
activities. Device pm states are latched in these registers whenever the
package enters C10 and can be read from slp_s0_debug_status. The pm
states may also be latched by writing 1 to slp_s0_dbg_latch which will
immediately capture the current state on the next read of
slp_s0_debug_status.

Signed-off-by: Box, David E <david.e.box@intel.com>
---
V3:
	- use null terminator in bit_map array
	- replaced ternary operator with if/else
	- Removed space fixes on old code
V2:
	- Clear latch bit after use
	- Pass pmc_dev as parameter
	- Use DEFINE_SHOW_ATTRIBUTE macro

 drivers/platform/x86/intel_pmc_core.c | 120 ++++++++++++++++++++++++++++++++++
 drivers/platform/x86/intel_pmc_core.h |   6 ++
 2 files changed, 126 insertions(+)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 43bbe74743d9..2d272a3e0176 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -196,9 +196,67 @@ static const struct pmc_bit_map cnp_pfear_map[] = {
 	{}
 };
 
+static const struct pmc_bit_map cnp_slps0_dbg0_map[] = {
+	{"AUDIO_D3",		BIT(0)},
+	{"OTG_D3",		BIT(1)},
+	{"XHCI_D3",		BIT(2)},
+	{"LPIO_D3",		BIT(3)},
+	{"SDX_D3",		BIT(4)},
+	{"SATA_D3",		BIT(5)},
+	{"UFS0_D3",		BIT(6)},
+	{"UFS1_D3",		BIT(7)},
+	{"EMMC_D3",		BIT(8)},
+	{}
+};
+
+static const struct pmc_bit_map cnp_slps0_dbg1_map[] = {
+	{"SDIO_PLL_OFF",	BIT(0)},
+	{"USB2_PLL_OFF",	BIT(1)},
+	{"AUDIO_PLL_OFF",	BIT(2)},
+	{"OC_PLL_OFF",		BIT(3)},
+	{"MAIN_PLL_OFF",	BIT(4)},
+	{"XOSC_OFF",		BIT(5)},
+	{"LPC_CLKS_GATED",	BIT(6)},
+	{"PCIE_CLKREQS_IDLE",	BIT(7)},
+	{"AUDIO_ROSC_OFF",	BIT(8)},
+	{"HPET_XOSC_CLK_REQ",	BIT(9)},
+	{"PMC_ROSC_SLOW_CLK",	BIT(10)},
+	{"AON2_ROSC_GATED",	BIT(11)},
+	{"CLKACKS_DEASSERTED",	BIT(12)},
+	{}
+};
+
+static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
+	{"MPHY_CORE_GATED",	BIT(0)},
+	{"CSME_GATED",		BIT(1)},
+	{"USB2_SUS_GATED",	BIT(2)},
+	{"DYN_FLEX_IO_IDLE",	BIT(3)},
+	{"GBE_NO_LINK",		BIT(4)},
+	{"THERM_SEN_DISABLED",	BIT(5)},
+	{"PCIE_LOW_POWER",	BIT(6)},
+	{"ISH_VNNAON_REQ_ACT",	BIT(7)},
+	{"ISH_VNN_REQ_ACT",	BIT(8)},
+	{"CNV_VNNAON_REQ_ACT",	BIT(9)},
+	{"CNV_VNN_REQ_ACT",	BIT(10)},
+	{"NPK_VNNON_REQ_ACT",	BIT(11)},
+	{"PMSYNC_STATE_IDLE",	BIT(12)},
+	{"ALST_GT_THRES",	BIT(13)},
+	{"PMC_ARC_PG_READY",	BIT(14)},
+	{}
+};
+
+static const struct pmc_bit_map *cnp_slps0_dbg_maps[] = {
+	cnp_slps0_dbg0_map,
+	cnp_slps0_dbg1_map,
+	cnp_slps0_dbg2_map,
+	NULL,
+};
+
 static const struct pmc_reg_map cnp_reg_map = {
 	.pfear_sts = cnp_pfear_map,
 	.slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
+	.slps0_dbg_maps = cnp_slps0_dbg_maps,
+	.slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
 	.ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
 	.regmap_length = CNP_PMC_MMIO_REG_LEN,
 	.ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
@@ -252,6 +310,8 @@ static int pmc_core_check_read_lock_bit(void)
 }
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
+static bool slps0_dbg_latch;
+
 static void pmc_core_display_map(struct seq_file *s, int index,
 				 u8 pf_reg, const struct pmc_bit_map *pf_map)
 {
@@ -481,6 +541,57 @@ static const struct file_operations pmc_core_ltr_ignore_ops = {
 	.release        = single_release,
 };
 
+static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev, bool reset)
+{
+	const struct pmc_reg_map *map = pmcdev->map;
+	u32 fd;
+
+	mutex_lock(&pmcdev->lock);
+
+	if (!reset && !slps0_dbg_latch)
+		goto out_unlock;
+
+	fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
+	if (reset)
+		fd &= ~CNP_PMC_LATCH_SLPS0_EVENTS;
+	else
+		fd |= CNP_PMC_LATCH_SLPS0_EVENTS;
+	pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
+
+	slps0_dbg_latch = 0;
+
+out_unlock:
+	mutex_unlock(&pmcdev->lock);
+}
+
+static int pmc_core_slps0_dbg_show(struct seq_file *s, void *unused)
+{
+	struct pmc_dev *pmcdev = s->private;
+	const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
+	const struct pmc_bit_map *map;
+	int offset;
+	u32 data;
+
+	pmc_core_slps0_dbg_latch(pmcdev, false);
+	offset = pmcdev->map->slps0_dbg_offset;
+	while (*maps) {
+		map = *maps;
+		data = pmc_core_reg_read(pmcdev, offset);
+		offset += 4;
+		while (map->name) {
+			seq_printf(s, "SLP_S0_DBG: %-32s\tState: %s\n",
+				   map->name,
+				   data & map->bit_mask ?
+				   "Yes" : "No");
+			++map;
+		}
+		++maps;
+	}
+	pmc_core_slps0_dbg_latch(pmcdev, true);
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);
+
 static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
 {
 	debugfs_remove_recursive(pmcdev->dbgfs_dir);
@@ -514,6 +625,15 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
 				    0444, dir, pmcdev,
 				    &pmc_core_mphy_pg_ops);
 
+	if (pmcdev->map->slps0_dbg_maps) {
+		debugfs_create_file("slp_s0_debug_status", 0444,
+				    dir, pmcdev,
+				    &pmc_core_slps0_dbg_fops);
+
+		debugfs_create_bool("slp_s0_dbg_latch", 0644,
+				    dir, &slps0_dbg_latch);
+	}
+
 	return 0;
 }
 #else
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 5fa5f97870aa..93a7e99e1f8b 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -127,12 +127,14 @@ enum ppfear_regs {
 #define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET      0x193C
 #define CNP_PMC_LTR_IGNORE_OFFSET              0x1B0C
 #define CNP_PMC_PM_CFG_OFFSET                  0x1818
+#define CNP_PMC_SLPS0_DBG_OFFSET		0x10B4
 /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */
 #define CNP_PMC_HOST_PPFEAR0A                  0x1D90
 
 #define CNP_PMC_MMIO_REG_LEN                   0x2000
 #define CNP_PPFEAR_NUM_ENTRIES                 8
 #define CNP_PMC_READ_DISABLE_BIT               22
+#define CNP_PMC_LATCH_SLPS0_EVENTS		BIT(31)
 
 struct pmc_bit_map {
 	const char *name;
@@ -145,6 +147,7 @@ struct pmc_bit_map {
  * @pfear_sts:		Maps name of IP block to PPFEAR* bit
  * @mphy_sts:		Maps name of MPHY lane to MPHY status lane status bit
  * @pll_sts:		Maps name of PLL to corresponding bit status
+ * @slps0_dbg_maps:	Array of SLP_S0_DBG* registers containing debug info
  * @slp_s0_offset:	PWRMBASE offset to read SLP_S0 residency
  * @ltr_ignore_offset:	PWRMBASE offset to read/write LTR ignore bit
  * @regmap_length:	Length of memory to map from PWRMBASE address to access
@@ -153,6 +156,7 @@ struct pmc_bit_map {
  *			PPFEAR
  * @pm_cfg_offset:	PWRMBASE offset to PM_CFG register
  * @pm_read_disable_bit: Bit index to read PMC_READ_DISABLE
+ * @slps0_dbg_offset:	PWRMBASE offset to SLP_S0_DEBUG_REG*
  *
  * Each PCH has unique set of register offsets and bit indexes. This structure
  * captures them to have a common implementation.
@@ -161,6 +165,7 @@ struct pmc_reg_map {
 	const struct pmc_bit_map *pfear_sts;
 	const struct pmc_bit_map *mphy_sts;
 	const struct pmc_bit_map *pll_sts;
+	const struct pmc_bit_map **slps0_dbg_maps;
 	const u32 slp_s0_offset;
 	const u32 ltr_ignore_offset;
 	const int regmap_length;
@@ -168,6 +173,7 @@ struct pmc_reg_map {
 	const int ppfear_buckets;
 	const u32 pm_cfg_offset;
 	const int pm_read_disable_bit;
+	const u32 slps0_dbg_offset;
 };
 
 /**
-- 
2.14.4

Re: [V3] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[Rajneesh Bhardwaj]

On Fri, Jun 08, 2018 at 05:02:37PM -0700, Box, David E wrote:

I am ok with the design and approach and also verified it on a Cannonlake
system. I wont insist for a V4 unless Andy feels a need for respin but there
are minor things that were missed. 

> Adds debugfs access to registers in the Cannon Point PCH PMC that are

Cannonlake PCH

> useful for debugging #SLP_S0 signal assertion and other low power relate

Typo, related.

> activities. Device pm states are latched in these registers whenever the
> package enters C10 and can be read from slp_s0_debug_status. The pm
> states may also be latched by writing 1 to slp_s0_dbg_latch which will
> immediately capture the current state on the next read of
> slp_s0_debug_status.

I had requested for consistency w.r.t. slp_s0_debug vs slp_s0_dbg but its
fine.

Also ISCLK_OC / ISCLK_MAIN suggestion is missing.

> 
> Signed-off-by: Box, David E <david.e.box@intel.com>
> ---
> V3:
> 	- use null terminator in bit_map array
> 	- replaced ternary operator with if/else
> 	- Removed space fixes on old code
> V2:
> 	- Clear latch bit after use
> 	- Pass pmc_dev as parameter
> 	- Use DEFINE_SHOW_ATTRIBUTE macro
> 
>  drivers/platform/x86/intel_pmc_core.c | 120 ++++++++++++++++++++++++++++++++++
>  drivers/platform/x86/intel_pmc_core.h |   6 ++
>  2 files changed, 126 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 43bbe74743d9..2d272a3e0176 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -196,9 +196,67 @@ static const struct pmc_bit_map cnp_pfear_map[] = {
>  	{}
>  };
>  
> +static const struct pmc_bit_map cnp_slps0_dbg0_map[] = {
> +	{"AUDIO_D3",		BIT(0)},
> +	{"OTG_D3",		BIT(1)},
> +	{"XHCI_D3",		BIT(2)},
> +	{"LPIO_D3",		BIT(3)},
> +	{"SDX_D3",		BIT(4)},
> +	{"SATA_D3",		BIT(5)},
> +	{"UFS0_D3",		BIT(6)},
> +	{"UFS1_D3",		BIT(7)},
> +	{"EMMC_D3",		BIT(8)},
> +	{}
> +};
> +
> +static const struct pmc_bit_map cnp_slps0_dbg1_map[] = {
> +	{"SDIO_PLL_OFF",	BIT(0)},
> +	{"USB2_PLL_OFF",	BIT(1)},
> +	{"AUDIO_PLL_OFF",	BIT(2)},
> +	{"OC_PLL_OFF",		BIT(3)},
> +	{"MAIN_PLL_OFF",	BIT(4)},
> +	{"XOSC_OFF",		BIT(5)},
> +	{"LPC_CLKS_GATED",	BIT(6)},
> +	{"PCIE_CLKREQS_IDLE",	BIT(7)},
> +	{"AUDIO_ROSC_OFF",	BIT(8)},
> +	{"HPET_XOSC_CLK_REQ",	BIT(9)},
> +	{"PMC_ROSC_SLOW_CLK",	BIT(10)},
> +	{"AON2_ROSC_GATED",	BIT(11)},
> +	{"CLKACKS_DEASSERTED",	BIT(12)},
> +	{}
> +};
> +
> +static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
> +	{"MPHY_CORE_GATED",	BIT(0)},
> +	{"CSME_GATED",		BIT(1)},
> +	{"USB2_SUS_GATED",	BIT(2)},
> +	{"DYN_FLEX_IO_IDLE",	BIT(3)},
> +	{"GBE_NO_LINK",		BIT(4)},
> +	{"THERM_SEN_DISABLED",	BIT(5)},
> +	{"PCIE_LOW_POWER",	BIT(6)},
> +	{"ISH_VNNAON_REQ_ACT",	BIT(7)},
> +	{"ISH_VNN_REQ_ACT",	BIT(8)},
> +	{"CNV_VNNAON_REQ_ACT",	BIT(9)},
> +	{"CNV_VNN_REQ_ACT",	BIT(10)},
> +	{"NPK_VNNON_REQ_ACT",	BIT(11)},
> +	{"PMSYNC_STATE_IDLE",	BIT(12)},
> +	{"ALST_GT_THRES",	BIT(13)},
> +	{"PMC_ARC_PG_READY",	BIT(14)},
> +	{}
> +};
> +
> +static const struct pmc_bit_map *cnp_slps0_dbg_maps[] = {
> +	cnp_slps0_dbg0_map,
> +	cnp_slps0_dbg1_map,
> +	cnp_slps0_dbg2_map,
> +	NULL,
> +};
> +
>  static const struct pmc_reg_map cnp_reg_map = {
>  	.pfear_sts = cnp_pfear_map,
>  	.slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
> +	.slps0_dbg_maps = cnp_slps0_dbg_maps,
> +	.slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
>  	.ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
>  	.regmap_length = CNP_PMC_MMIO_REG_LEN,
>  	.ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
> @@ -252,6 +310,8 @@ static int pmc_core_check_read_lock_bit(void)
>  }
>  
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> +static bool slps0_dbg_latch;
> +
>  static void pmc_core_display_map(struct seq_file *s, int index,
>  				 u8 pf_reg, const struct pmc_bit_map *pf_map)
>  {
> @@ -481,6 +541,57 @@ static const struct file_operations pmc_core_ltr_ignore_ops = {
>  	.release        = single_release,
>  };
>  
> +static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev, bool reset)
> +{
> +	const struct pmc_reg_map *map = pmcdev->map;
> +	u32 fd;
> +
> +	mutex_lock(&pmcdev->lock);
> +
> +	if (!reset && !slps0_dbg_latch)
> +		goto out_unlock;
> +
> +	fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
> +	if (reset)
> +		fd &= ~CNP_PMC_LATCH_SLPS0_EVENTS;
> +	else
> +		fd |= CNP_PMC_LATCH_SLPS0_EVENTS;
> +	pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
> +
> +	slps0_dbg_latch = 0;
> +
> +out_unlock:
> +	mutex_unlock(&pmcdev->lock);
> +}
> +
> +static int pmc_core_slps0_dbg_show(struct seq_file *s, void *unused)
> +{
> +	struct pmc_dev *pmcdev = s->private;
> +	const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
> +	const struct pmc_bit_map *map;
> +	int offset;
> +	u32 data;
> +
> +	pmc_core_slps0_dbg_latch(pmcdev, false);
> +	offset = pmcdev->map->slps0_dbg_offset;
> +	while (*maps) {
> +		map = *maps;
> +		data = pmc_core_reg_read(pmcdev, offset);
> +		offset += 4;
> +		while (map->name) {
> +			seq_printf(s, "SLP_S0_DBG: %-32s\tState: %s\n",
> +				   map->name,
> +				   data & map->bit_mask ?
> +				   "Yes" : "No");
> +			++map;
> +		}
> +		++maps;
> +	}
> +	pmc_core_slps0_dbg_latch(pmcdev, true);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);
> +
>  static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
>  {
>  	debugfs_remove_recursive(pmcdev->dbgfs_dir);
> @@ -514,6 +625,15 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>  				    0444, dir, pmcdev,
>  				    &pmc_core_mphy_pg_ops);
>  
> +	if (pmcdev->map->slps0_dbg_maps) {
> +		debugfs_create_file("slp_s0_debug_status", 0444,
> +				    dir, pmcdev,
> +				    &pmc_core_slps0_dbg_fops);
> +
> +		debugfs_create_bool("slp_s0_dbg_latch", 0644,
> +				    dir, &slps0_dbg_latch);
> +	}
> +
>  	return 0;
>  }
>  #else
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index 5fa5f97870aa..93a7e99e1f8b 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -127,12 +127,14 @@ enum ppfear_regs {
>  #define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET      0x193C
>  #define CNP_PMC_LTR_IGNORE_OFFSET              0x1B0C
>  #define CNP_PMC_PM_CFG_OFFSET                  0x1818
> +#define CNP_PMC_SLPS0_DBG_OFFSET		0x10B4
>  /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */
>  #define CNP_PMC_HOST_PPFEAR0A                  0x1D90
>  
>  #define CNP_PMC_MMIO_REG_LEN                   0x2000
>  #define CNP_PPFEAR_NUM_ENTRIES                 8
>  #define CNP_PMC_READ_DISABLE_BIT               22
> +#define CNP_PMC_LATCH_SLPS0_EVENTS		BIT(31)
>  
>  struct pmc_bit_map {
>  	const char *name;
> @@ -145,6 +147,7 @@ struct pmc_bit_map {
>   * @pfear_sts:		Maps name of IP block to PPFEAR* bit
>   * @mphy_sts:		Maps name of MPHY lane to MPHY status lane status bit
>   * @pll_sts:		Maps name of PLL to corresponding bit status
> + * @slps0_dbg_maps:	Array of SLP_S0_DBG* registers containing debug info
>   * @slp_s0_offset:	PWRMBASE offset to read SLP_S0 residency
>   * @ltr_ignore_offset:	PWRMBASE offset to read/write LTR ignore bit
>   * @regmap_length:	Length of memory to map from PWRMBASE address to access
> @@ -153,6 +156,7 @@ struct pmc_bit_map {
>   *			PPFEAR
>   * @pm_cfg_offset:	PWRMBASE offset to PM_CFG register
>   * @pm_read_disable_bit: Bit index to read PMC_READ_DISABLE
> + * @slps0_dbg_offset:	PWRMBASE offset to SLP_S0_DEBUG_REG*
>   *
>   * Each PCH has unique set of register offsets and bit indexes. This structure
>   * captures them to have a common implementation.
> @@ -161,6 +165,7 @@ struct pmc_reg_map {
>  	const struct pmc_bit_map *pfear_sts;
>  	const struct pmc_bit_map *mphy_sts;
>  	const struct pmc_bit_map *pll_sts;
> +	const struct pmc_bit_map **slps0_dbg_maps;
>  	const u32 slp_s0_offset;
>  	const u32 ltr_ignore_offset;
>  	const int regmap_length;
> @@ -168,6 +173,7 @@ struct pmc_reg_map {
>  	const int ppfear_buckets;
>  	const u32 pm_cfg_offset;
>  	const int pm_read_disable_bit;
> +	const u32 slps0_dbg_offset;
>  };
>  
>  /**
> -- 
> 2.14.4
> 

-- 
Best Regards,
Rajneesh

Re: [V3] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[Andy Shevchenko]

On Wed, Jun 13, 2018 at 5:07 PM, Rajneesh Bhardwaj
<rajneesh.bhardwaj@intel.com> wrote:
> On Fri, Jun 08, 2018 at 05:02:37PM -0700, Box, David E wrote:
>
> I am ok with the design and approach and also verified it on a Cannonlake
> system. I wont insist for a V4 unless Andy feels a need for respin but there
> are minor things that were missed.

> Also ISCLK_OC / ISCLK_MAIN suggestion is missing.

I missed this in discussion, so, please send v4 with settled fixes.

Meanwhile I keep v3 in my review and testing queue, thanks!


-- 
With Best Regards,
Andy Shevchenko

[PATCH V4] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[David E. Box]

From: <david.e.box@intel.com>

Adds debugfs access to registers in the Cannonlake PCH PMC that are
useful for debugging #SLP_S0 signal assertion and other low power
related activities. Device pm states are latched in these registers
whenever the package enters C10 and can be read from slp_s0_debug_status.
The pm states may also be latched by writing 1 to slp_s0_debug_latch
which will immediately capture the current state on the next read of
slp_s0_debug_status.

Signed-off-by: Box, David E <david.e.box@intel.com>
---
V4:
	- rename slp_s0_dbg string to slp_s0_debug for consistency
	- ADD ISCLK prefix to MAIN_PLL and OC_PLL
V3:
	- use null terminator in bit_map array
	- replaced ternary operator with if/else
	- Removed space fixes on old code
V2:
	- Clear latch bit after use
	- Pass pmc_dev as parameter
	- Use DEFINE_SHOW_ATTRIBUTE macro
 drivers/platform/x86/intel_pmc_core.c | 120 ++++++++++++++++++++++++++++++++++
 drivers/platform/x86/intel_pmc_core.h |   6 ++
 2 files changed, 126 insertions(+)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 43bbe74..d00fee2 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -196,9 +196,67 @@ static const struct pmc_bit_map cnp_pfear_map[] = {
 	{}
 };
 
+static const struct pmc_bit_map cnp_slps0_dbg0_map[] = {
+	{"AUDIO_D3",		BIT(0)},
+	{"OTG_D3",		BIT(1)},
+	{"XHCI_D3",		BIT(2)},
+	{"LPIO_D3",		BIT(3)},
+	{"SDX_D3",		BIT(4)},
+	{"SATA_D3",		BIT(5)},
+	{"UFS0_D3",		BIT(6)},
+	{"UFS1_D3",		BIT(7)},
+	{"EMMC_D3",		BIT(8)},
+	{}
+};
+
+static const struct pmc_bit_map cnp_slps0_dbg1_map[] = {
+	{"SDIO_PLL_OFF",	BIT(0)},
+	{"USB2_PLL_OFF",	BIT(1)},
+	{"AUDIO_PLL_OFF",	BIT(2)},
+	{"ISCLK_OC_PLL_OFF",	BIT(3)},
+	{"ISCLK_MAIN_PLL_OFF",	BIT(4)},
+	{"XOSC_OFF",		BIT(5)},
+	{"LPC_CLKS_GATED",	BIT(6)},
+	{"PCIE_CLKREQS_IDLE",	BIT(7)},
+	{"AUDIO_ROSC_OFF",	BIT(8)},
+	{"HPET_XOSC_CLK_REQ",	BIT(9)},
+	{"PMC_ROSC_SLOW_CLK",	BIT(10)},
+	{"AON2_ROSC_GATED",	BIT(11)},
+	{"CLKACKS_DEASSERTED",	BIT(12)},
+	{}
+};
+
+static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
+	{"MPHY_CORE_GATED",	BIT(0)},
+	{"CSME_GATED",		BIT(1)},
+	{"USB2_SUS_GATED",	BIT(2)},
+	{"DYN_FLEX_IO_IDLE",	BIT(3)},
+	{"GBE_NO_LINK",		BIT(4)},
+	{"THERM_SEN_DISABLED",	BIT(5)},
+	{"PCIE_LOW_POWER",	BIT(6)},
+	{"ISH_VNNAON_REQ_ACT",	BIT(7)},
+	{"ISH_VNN_REQ_ACT",	BIT(8)},
+	{"CNV_VNNAON_REQ_ACT",	BIT(9)},
+	{"CNV_VNN_REQ_ACT",	BIT(10)},
+	{"NPK_VNNON_REQ_ACT",	BIT(11)},
+	{"PMSYNC_STATE_IDLE",	BIT(12)},
+	{"ALST_GT_THRES",	BIT(13)},
+	{"PMC_ARC_PG_READY",	BIT(14)},
+	{}
+};
+
+static const struct pmc_bit_map *cnp_slps0_dbg_maps[] = {
+	cnp_slps0_dbg0_map,
+	cnp_slps0_dbg1_map,
+	cnp_slps0_dbg2_map,
+	NULL,
+};
+
 static const struct pmc_reg_map cnp_reg_map = {
 	.pfear_sts = cnp_pfear_map,
 	.slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
+	.slps0_dbg_maps = cnp_slps0_dbg_maps,
+	.slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
 	.ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
 	.regmap_length = CNP_PMC_MMIO_REG_LEN,
 	.ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
@@ -252,6 +310,8 @@ static int pmc_core_check_read_lock_bit(void)
 }
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
+static bool slps0_dbg_latch;
+
 static void pmc_core_display_map(struct seq_file *s, int index,
 				 u8 pf_reg, const struct pmc_bit_map *pf_map)
 {
@@ -481,6 +541,57 @@ static const struct file_operations pmc_core_ltr_ignore_ops = {
 	.release        = single_release,
 };
 
+static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev, bool reset)
+{
+	const struct pmc_reg_map *map = pmcdev->map;
+	u32 fd;
+
+	mutex_lock(&pmcdev->lock);
+
+	if (!reset && !slps0_dbg_latch)
+		goto out_unlock;
+
+	fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
+	if (reset)
+		fd &= ~CNP_PMC_LATCH_SLPS0_EVENTS;
+	else
+		fd |= CNP_PMC_LATCH_SLPS0_EVENTS;
+	pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
+
+	slps0_dbg_latch = 0;
+
+out_unlock:
+	mutex_unlock(&pmcdev->lock);
+}
+
+static int pmc_core_slps0_dbg_show(struct seq_file *s, void *unused)
+{
+	struct pmc_dev *pmcdev = s->private;
+	const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
+	const struct pmc_bit_map *map;
+	int offset;
+	u32 data;
+
+	pmc_core_slps0_dbg_latch(pmcdev, false);
+	offset = pmcdev->map->slps0_dbg_offset;
+	while (*maps) {
+		map = *maps;
+		data = pmc_core_reg_read(pmcdev, offset);
+		offset += 4;
+		while (map->name) {
+			seq_printf(s, "SLP_S0_DBG: %-32s\tState: %s\n",
+				   map->name,
+				   data & map->bit_mask ?
+				   "Yes" : "No");
+			++map;
+		}
+		++maps;
+	}
+	pmc_core_slps0_dbg_latch(pmcdev, true);
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);
+
 static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
 {
 	debugfs_remove_recursive(pmcdev->dbgfs_dir);
@@ -514,6 +625,15 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
 				    0444, dir, pmcdev,
 				    &pmc_core_mphy_pg_ops);
 
+	if (pmcdev->map->slps0_dbg_maps) {
+		debugfs_create_file("slp_s0_debug_status", 0444,
+				    dir, pmcdev,
+				    &pmc_core_slps0_dbg_fops);
+
+		debugfs_create_bool("slp_s0_debug_latch", 0644,
+				    dir, &slps0_dbg_latch);
+	}
+
 	return 0;
 }
 #else
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 5fa5f97..93a7e99 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -127,12 +127,14 @@ enum ppfear_regs {
 #define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET      0x193C
 #define CNP_PMC_LTR_IGNORE_OFFSET              0x1B0C
 #define CNP_PMC_PM_CFG_OFFSET                  0x1818
+#define CNP_PMC_SLPS0_DBG_OFFSET		0x10B4
 /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */
 #define CNP_PMC_HOST_PPFEAR0A                  0x1D90
 
 #define CNP_PMC_MMIO_REG_LEN                   0x2000
 #define CNP_PPFEAR_NUM_ENTRIES                 8
 #define CNP_PMC_READ_DISABLE_BIT               22
+#define CNP_PMC_LATCH_SLPS0_EVENTS		BIT(31)
 
 struct pmc_bit_map {
 	const char *name;
@@ -145,6 +147,7 @@ struct pmc_bit_map {
  * @pfear_sts:		Maps name of IP block to PPFEAR* bit
  * @mphy_sts:		Maps name of MPHY lane to MPHY status lane status bit
  * @pll_sts:		Maps name of PLL to corresponding bit status
+ * @slps0_dbg_maps:	Array of SLP_S0_DBG* registers containing debug info
  * @slp_s0_offset:	PWRMBASE offset to read SLP_S0 residency
  * @ltr_ignore_offset:	PWRMBASE offset to read/write LTR ignore bit
  * @regmap_length:	Length of memory to map from PWRMBASE address to access
@@ -153,6 +156,7 @@ struct pmc_bit_map {
  *			PPFEAR
  * @pm_cfg_offset:	PWRMBASE offset to PM_CFG register
  * @pm_read_disable_bit: Bit index to read PMC_READ_DISABLE
+ * @slps0_dbg_offset:	PWRMBASE offset to SLP_S0_DEBUG_REG*
  *
  * Each PCH has unique set of register offsets and bit indexes. This structure
  * captures them to have a common implementation.
@@ -161,6 +165,7 @@ struct pmc_reg_map {
 	const struct pmc_bit_map *pfear_sts;
 	const struct pmc_bit_map *mphy_sts;
 	const struct pmc_bit_map *pll_sts;
+	const struct pmc_bit_map **slps0_dbg_maps;
 	const u32 slp_s0_offset;
 	const u32 ltr_ignore_offset;
 	const int regmap_length;
@@ -168,6 +173,7 @@ struct pmc_reg_map {
 	const int ppfear_buckets;
 	const u32 pm_cfg_offset;
 	const int pm_read_disable_bit;
+	const u32 slps0_dbg_offset;
 };
 
 /**
-- 
2.7.4

Re: [PATCH V4] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[Rajneesh Bhardwaj]

On Thu, Jun 14, 2018 at 03:13:02PM -0700, David E. Box wrote:
> From: <david.e.box@intel.com>
> 
> Adds debugfs access to registers in the Cannonlake PCH PMC that are
> useful for debugging #SLP_S0 signal assertion and other low power
> related activities. Device pm states are latched in these registers
> whenever the package enters C10 and can be read from slp_s0_debug_status.
> The pm states may also be latched by writing 1 to slp_s0_debug_latch
> which will immediately capture the current state on the next read of
> slp_s0_debug_status.

Reviewed-and-tested-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>

> 
> Signed-off-by: Box, David E <david.e.box@intel.com>
> ---
> V4:
> 	- rename slp_s0_dbg string to slp_s0_debug for consistency
> 	- ADD ISCLK prefix to MAIN_PLL and OC_PLL
> V3:
> 	- use null terminator in bit_map array
> 	- replaced ternary operator with if/else
> 	- Removed space fixes on old code
> V2:
> 	- Clear latch bit after use
> 	- Pass pmc_dev as parameter
> 	- Use DEFINE_SHOW_ATTRIBUTE macro
>  drivers/platform/x86/intel_pmc_core.c | 120 ++++++++++++++++++++++++++++++++++
>  drivers/platform/x86/intel_pmc_core.h |   6 ++
>  2 files changed, 126 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 43bbe74..d00fee2 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -196,9 +196,67 @@ static const struct pmc_bit_map cnp_pfear_map[] = {
>  	{}
>  };
>  
> +static const struct pmc_bit_map cnp_slps0_dbg0_map[] = {
> +	{"AUDIO_D3",		BIT(0)},
> +	{"OTG_D3",		BIT(1)},
> +	{"XHCI_D3",		BIT(2)},
> +	{"LPIO_D3",		BIT(3)},
> +	{"SDX_D3",		BIT(4)},
> +	{"SATA_D3",		BIT(5)},
> +	{"UFS0_D3",		BIT(6)},
> +	{"UFS1_D3",		BIT(7)},
> +	{"EMMC_D3",		BIT(8)},
> +	{}
> +};
> +
> +static const struct pmc_bit_map cnp_slps0_dbg1_map[] = {
> +	{"SDIO_PLL_OFF",	BIT(0)},
> +	{"USB2_PLL_OFF",	BIT(1)},
> +	{"AUDIO_PLL_OFF",	BIT(2)},
> +	{"ISCLK_OC_PLL_OFF",	BIT(3)},
> +	{"ISCLK_MAIN_PLL_OFF",	BIT(4)},
> +	{"XOSC_OFF",		BIT(5)},
> +	{"LPC_CLKS_GATED",	BIT(6)},
> +	{"PCIE_CLKREQS_IDLE",	BIT(7)},
> +	{"AUDIO_ROSC_OFF",	BIT(8)},
> +	{"HPET_XOSC_CLK_REQ",	BIT(9)},
> +	{"PMC_ROSC_SLOW_CLK",	BIT(10)},
> +	{"AON2_ROSC_GATED",	BIT(11)},
> +	{"CLKACKS_DEASSERTED",	BIT(12)},
> +	{}
> +};
> +
> +static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
> +	{"MPHY_CORE_GATED",	BIT(0)},
> +	{"CSME_GATED",		BIT(1)},
> +	{"USB2_SUS_GATED",	BIT(2)},
> +	{"DYN_FLEX_IO_IDLE",	BIT(3)},
> +	{"GBE_NO_LINK",		BIT(4)},
> +	{"THERM_SEN_DISABLED",	BIT(5)},
> +	{"PCIE_LOW_POWER",	BIT(6)},
> +	{"ISH_VNNAON_REQ_ACT",	BIT(7)},
> +	{"ISH_VNN_REQ_ACT",	BIT(8)},
> +	{"CNV_VNNAON_REQ_ACT",	BIT(9)},
> +	{"CNV_VNN_REQ_ACT",	BIT(10)},
> +	{"NPK_VNNON_REQ_ACT",	BIT(11)},
> +	{"PMSYNC_STATE_IDLE",	BIT(12)},
> +	{"ALST_GT_THRES",	BIT(13)},
> +	{"PMC_ARC_PG_READY",	BIT(14)},
> +	{}
> +};
> +
> +static const struct pmc_bit_map *cnp_slps0_dbg_maps[] = {
> +	cnp_slps0_dbg0_map,
> +	cnp_slps0_dbg1_map,
> +	cnp_slps0_dbg2_map,
> +	NULL,
> +};
> +
>  static const struct pmc_reg_map cnp_reg_map = {
>  	.pfear_sts = cnp_pfear_map,
>  	.slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
> +	.slps0_dbg_maps = cnp_slps0_dbg_maps,
> +	.slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
>  	.ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
>  	.regmap_length = CNP_PMC_MMIO_REG_LEN,
>  	.ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
> @@ -252,6 +310,8 @@ static int pmc_core_check_read_lock_bit(void)
>  }
>  
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> +static bool slps0_dbg_latch;
> +
>  static void pmc_core_display_map(struct seq_file *s, int index,
>  				 u8 pf_reg, const struct pmc_bit_map *pf_map)
>  {
> @@ -481,6 +541,57 @@ static const struct file_operations pmc_core_ltr_ignore_ops = {
>  	.release        = single_release,
>  };
>  
> +static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev, bool reset)
> +{
> +	const struct pmc_reg_map *map = pmcdev->map;
> +	u32 fd;
> +
> +	mutex_lock(&pmcdev->lock);
> +
> +	if (!reset && !slps0_dbg_latch)
> +		goto out_unlock;
> +
> +	fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
> +	if (reset)
> +		fd &= ~CNP_PMC_LATCH_SLPS0_EVENTS;
> +	else
> +		fd |= CNP_PMC_LATCH_SLPS0_EVENTS;
> +	pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
> +
> +	slps0_dbg_latch = 0;
> +
> +out_unlock:
> +	mutex_unlock(&pmcdev->lock);
> +}
> +
> +static int pmc_core_slps0_dbg_show(struct seq_file *s, void *unused)
> +{
> +	struct pmc_dev *pmcdev = s->private;
> +	const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
> +	const struct pmc_bit_map *map;
> +	int offset;
> +	u32 data;
> +
> +	pmc_core_slps0_dbg_latch(pmcdev, false);
> +	offset = pmcdev->map->slps0_dbg_offset;
> +	while (*maps) {
> +		map = *maps;
> +		data = pmc_core_reg_read(pmcdev, offset);
> +		offset += 4;
> +		while (map->name) {
> +			seq_printf(s, "SLP_S0_DBG: %-32s\tState: %s\n",
> +				   map->name,
> +				   data & map->bit_mask ?
> +				   "Yes" : "No");
> +			++map;
> +		}
> +		++maps;
> +	}
> +	pmc_core_slps0_dbg_latch(pmcdev, true);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);
> +
>  static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
>  {
>  	debugfs_remove_recursive(pmcdev->dbgfs_dir);
> @@ -514,6 +625,15 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>  				    0444, dir, pmcdev,
>  				    &pmc_core_mphy_pg_ops);
>  
> +	if (pmcdev->map->slps0_dbg_maps) {
> +		debugfs_create_file("slp_s0_debug_status", 0444,
> +				    dir, pmcdev,
> +				    &pmc_core_slps0_dbg_fops);
> +
> +		debugfs_create_bool("slp_s0_debug_latch", 0644,
> +				    dir, &slps0_dbg_latch);
> +	}
> +
>  	return 0;
>  }
>  #else
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index 5fa5f97..93a7e99 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -127,12 +127,14 @@ enum ppfear_regs {
>  #define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET      0x193C
>  #define CNP_PMC_LTR_IGNORE_OFFSET              0x1B0C
>  #define CNP_PMC_PM_CFG_OFFSET                  0x1818
> +#define CNP_PMC_SLPS0_DBG_OFFSET		0x10B4
>  /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */
>  #define CNP_PMC_HOST_PPFEAR0A                  0x1D90
>  
>  #define CNP_PMC_MMIO_REG_LEN                   0x2000
>  #define CNP_PPFEAR_NUM_ENTRIES                 8
>  #define CNP_PMC_READ_DISABLE_BIT               22
> +#define CNP_PMC_LATCH_SLPS0_EVENTS		BIT(31)
>  
>  struct pmc_bit_map {
>  	const char *name;
> @@ -145,6 +147,7 @@ struct pmc_bit_map {
>   * @pfear_sts:		Maps name of IP block to PPFEAR* bit
>   * @mphy_sts:		Maps name of MPHY lane to MPHY status lane status bit
>   * @pll_sts:		Maps name of PLL to corresponding bit status
> + * @slps0_dbg_maps:	Array of SLP_S0_DBG* registers containing debug info
>   * @slp_s0_offset:	PWRMBASE offset to read SLP_S0 residency
>   * @ltr_ignore_offset:	PWRMBASE offset to read/write LTR ignore bit
>   * @regmap_length:	Length of memory to map from PWRMBASE address to access
> @@ -153,6 +156,7 @@ struct pmc_bit_map {
>   *			PPFEAR
>   * @pm_cfg_offset:	PWRMBASE offset to PM_CFG register
>   * @pm_read_disable_bit: Bit index to read PMC_READ_DISABLE
> + * @slps0_dbg_offset:	PWRMBASE offset to SLP_S0_DEBUG_REG*
>   *
>   * Each PCH has unique set of register offsets and bit indexes. This structure
>   * captures them to have a common implementation.
> @@ -161,6 +165,7 @@ struct pmc_reg_map {
>  	const struct pmc_bit_map *pfear_sts;
>  	const struct pmc_bit_map *mphy_sts;
>  	const struct pmc_bit_map *pll_sts;
> +	const struct pmc_bit_map **slps0_dbg_maps;
>  	const u32 slp_s0_offset;
>  	const u32 ltr_ignore_offset;
>  	const int regmap_length;
> @@ -168,6 +173,7 @@ struct pmc_reg_map {
>  	const int ppfear_buckets;
>  	const u32 pm_cfg_offset;
>  	const int pm_read_disable_bit;
> +	const u32 slps0_dbg_offset;
>  };
>  
>  /**
> -- 
> 2.7.4
> 

-- 
Best Regards,
Rajneesh

Re: [PATCH V4] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[Andy Shevchenko]

On Fri, Jun 15, 2018 at 2:27 PM, Rajneesh Bhardwaj
<rajneesh.bhardwaj@intel.com> wrote:
> On Thu, Jun 14, 2018 at 03:13:02PM -0700, David E. Box wrote:
>> From: <david.e.box@intel.com>
>>
>> Adds debugfs access to registers in the Cannonlake PCH PMC that are
>> useful for debugging #SLP_S0 signal assertion and other low power
>> related activities. Device pm states are latched in these registers
>> whenever the package enters C10 and can be read from slp_s0_debug_status.
>> The pm states may also be latched by writing 1 to slp_s0_debug_latch
>> which will immediately capture the current state on the next read of
>> slp_s0_debug_status.
>
> Reviewed-and-tested-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>

Oops, sorry, it went without your tag.
patchwork seems didn't recognize this kind of combined tags.

>
>>
>> Signed-off-by: Box, David E <david.e.box@intel.com>
>> ---
>> V4:
>>       - rename slp_s0_dbg string to slp_s0_debug for consistency
>>       - ADD ISCLK prefix to MAIN_PLL and OC_PLL
>> V3:
>>       - use null terminator in bit_map array
>>       - replaced ternary operator with if/else
>>       - Removed space fixes on old code
>> V2:
>>       - Clear latch bit after use
>>       - Pass pmc_dev as parameter
>>       - Use DEFINE_SHOW_ATTRIBUTE macro
>>  drivers/platform/x86/intel_pmc_core.c | 120 ++++++++++++++++++++++++++++++++++
>>  drivers/platform/x86/intel_pmc_core.h |   6 ++
>>  2 files changed, 126 insertions(+)
>>
>> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
>> index 43bbe74..d00fee2 100644
>> --- a/drivers/platform/x86/intel_pmc_core.c
>> +++ b/drivers/platform/x86/intel_pmc_core.c
>> @@ -196,9 +196,67 @@ static const struct pmc_bit_map cnp_pfear_map[] = {
>>       {}
>>  };
>>
>> +static const struct pmc_bit_map cnp_slps0_dbg0_map[] = {
>> +     {"AUDIO_D3",            BIT(0)},
>> +     {"OTG_D3",              BIT(1)},
>> +     {"XHCI_D3",             BIT(2)},
>> +     {"LPIO_D3",             BIT(3)},
>> +     {"SDX_D3",              BIT(4)},
>> +     {"SATA_D3",             BIT(5)},
>> +     {"UFS0_D3",             BIT(6)},
>> +     {"UFS1_D3",             BIT(7)},
>> +     {"EMMC_D3",             BIT(8)},
>> +     {}
>> +};
>> +
>> +static const struct pmc_bit_map cnp_slps0_dbg1_map[] = {
>> +     {"SDIO_PLL_OFF",        BIT(0)},
>> +     {"USB2_PLL_OFF",        BIT(1)},
>> +     {"AUDIO_PLL_OFF",       BIT(2)},
>> +     {"ISCLK_OC_PLL_OFF",    BIT(3)},
>> +     {"ISCLK_MAIN_PLL_OFF",  BIT(4)},
>> +     {"XOSC_OFF",            BIT(5)},
>> +     {"LPC_CLKS_GATED",      BIT(6)},
>> +     {"PCIE_CLKREQS_IDLE",   BIT(7)},
>> +     {"AUDIO_ROSC_OFF",      BIT(8)},
>> +     {"HPET_XOSC_CLK_REQ",   BIT(9)},
>> +     {"PMC_ROSC_SLOW_CLK",   BIT(10)},
>> +     {"AON2_ROSC_GATED",     BIT(11)},
>> +     {"CLKACKS_DEASSERTED",  BIT(12)},
>> +     {}
>> +};
>> +
>> +static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
>> +     {"MPHY_CORE_GATED",     BIT(0)},
>> +     {"CSME_GATED",          BIT(1)},
>> +     {"USB2_SUS_GATED",      BIT(2)},
>> +     {"DYN_FLEX_IO_IDLE",    BIT(3)},
>> +     {"GBE_NO_LINK",         BIT(4)},
>> +     {"THERM_SEN_DISABLED",  BIT(5)},
>> +     {"PCIE_LOW_POWER",      BIT(6)},
>> +     {"ISH_VNNAON_REQ_ACT",  BIT(7)},
>> +     {"ISH_VNN_REQ_ACT",     BIT(8)},
>> +     {"CNV_VNNAON_REQ_ACT",  BIT(9)},
>> +     {"CNV_VNN_REQ_ACT",     BIT(10)},
>> +     {"NPK_VNNON_REQ_ACT",   BIT(11)},
>> +     {"PMSYNC_STATE_IDLE",   BIT(12)},
>> +     {"ALST_GT_THRES",       BIT(13)},
>> +     {"PMC_ARC_PG_READY",    BIT(14)},
>> +     {}
>> +};
>> +
>> +static const struct pmc_bit_map *cnp_slps0_dbg_maps[] = {
>> +     cnp_slps0_dbg0_map,
>> +     cnp_slps0_dbg1_map,
>> +     cnp_slps0_dbg2_map,
>> +     NULL,
>> +};
>> +
>>  static const struct pmc_reg_map cnp_reg_map = {
>>       .pfear_sts = cnp_pfear_map,
>>       .slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
>> +     .slps0_dbg_maps = cnp_slps0_dbg_maps,
>> +     .slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
>>       .ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
>>       .regmap_length = CNP_PMC_MMIO_REG_LEN,
>>       .ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
>> @@ -252,6 +310,8 @@ static int pmc_core_check_read_lock_bit(void)
>>  }
>>
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> +static bool slps0_dbg_latch;
>> +
>>  static void pmc_core_display_map(struct seq_file *s, int index,
>>                                u8 pf_reg, const struct pmc_bit_map *pf_map)
>>  {
>> @@ -481,6 +541,57 @@ static const struct file_operations pmc_core_ltr_ignore_ops = {
>>       .release        = single_release,
>>  };
>>
>> +static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev, bool reset)
>> +{
>> +     const struct pmc_reg_map *map = pmcdev->map;
>> +     u32 fd;
>> +
>> +     mutex_lock(&pmcdev->lock);
>> +
>> +     if (!reset && !slps0_dbg_latch)
>> +             goto out_unlock;
>> +
>> +     fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
>> +     if (reset)
>> +             fd &= ~CNP_PMC_LATCH_SLPS0_EVENTS;
>> +     else
>> +             fd |= CNP_PMC_LATCH_SLPS0_EVENTS;
>> +     pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
>> +
>> +     slps0_dbg_latch = 0;
>> +
>> +out_unlock:
>> +     mutex_unlock(&pmcdev->lock);
>> +}
>> +
>> +static int pmc_core_slps0_dbg_show(struct seq_file *s, void *unused)
>> +{
>> +     struct pmc_dev *pmcdev = s->private;
>> +     const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
>> +     const struct pmc_bit_map *map;
>> +     int offset;
>> +     u32 data;
>> +
>> +     pmc_core_slps0_dbg_latch(pmcdev, false);
>> +     offset = pmcdev->map->slps0_dbg_offset;
>> +     while (*maps) {
>> +             map = *maps;
>> +             data = pmc_core_reg_read(pmcdev, offset);
>> +             offset += 4;
>> +             while (map->name) {
>> +                     seq_printf(s, "SLP_S0_DBG: %-32s\tState: %s\n",
>> +                                map->name,
>> +                                data & map->bit_mask ?
>> +                                "Yes" : "No");
>> +                     ++map;
>> +             }
>> +             ++maps;
>> +     }
>> +     pmc_core_slps0_dbg_latch(pmcdev, true);
>> +     return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);
>> +
>>  static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
>>  {
>>       debugfs_remove_recursive(pmcdev->dbgfs_dir);
>> @@ -514,6 +625,15 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>>                                   0444, dir, pmcdev,
>>                                   &pmc_core_mphy_pg_ops);
>>
>> +     if (pmcdev->map->slps0_dbg_maps) {
>> +             debugfs_create_file("slp_s0_debug_status", 0444,
>> +                                 dir, pmcdev,
>> +                                 &pmc_core_slps0_dbg_fops);
>> +
>> +             debugfs_create_bool("slp_s0_debug_latch", 0644,
>> +                                 dir, &slps0_dbg_latch);
>> +     }
>> +
>>       return 0;
>>  }
>>  #else
>> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
>> index 5fa5f97..93a7e99 100644
>> --- a/drivers/platform/x86/intel_pmc_core.h
>> +++ b/drivers/platform/x86/intel_pmc_core.h
>> @@ -127,12 +127,14 @@ enum ppfear_regs {
>>  #define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET      0x193C
>>  #define CNP_PMC_LTR_IGNORE_OFFSET              0x1B0C
>>  #define CNP_PMC_PM_CFG_OFFSET                  0x1818
>> +#define CNP_PMC_SLPS0_DBG_OFFSET             0x10B4
>>  /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */
>>  #define CNP_PMC_HOST_PPFEAR0A                  0x1D90
>>
>>  #define CNP_PMC_MMIO_REG_LEN                   0x2000
>>  #define CNP_PPFEAR_NUM_ENTRIES                 8
>>  #define CNP_PMC_READ_DISABLE_BIT               22
>> +#define CNP_PMC_LATCH_SLPS0_EVENTS           BIT(31)
>>
>>  struct pmc_bit_map {
>>       const char *name;
>> @@ -145,6 +147,7 @@ struct pmc_bit_map {
>>   * @pfear_sts:               Maps name of IP block to PPFEAR* bit
>>   * @mphy_sts:                Maps name of MPHY lane to MPHY status lane status bit
>>   * @pll_sts:         Maps name of PLL to corresponding bit status
>> + * @slps0_dbg_maps:  Array of SLP_S0_DBG* registers containing debug info
>>   * @slp_s0_offset:   PWRMBASE offset to read SLP_S0 residency
>>   * @ltr_ignore_offset:       PWRMBASE offset to read/write LTR ignore bit
>>   * @regmap_length:   Length of memory to map from PWRMBASE address to access
>> @@ -153,6 +156,7 @@ struct pmc_bit_map {
>>   *                   PPFEAR
>>   * @pm_cfg_offset:   PWRMBASE offset to PM_CFG register
>>   * @pm_read_disable_bit: Bit index to read PMC_READ_DISABLE
>> + * @slps0_dbg_offset:        PWRMBASE offset to SLP_S0_DEBUG_REG*
>>   *
>>   * Each PCH has unique set of register offsets and bit indexes. This structure
>>   * captures them to have a common implementation.
>> @@ -161,6 +165,7 @@ struct pmc_reg_map {
>>       const struct pmc_bit_map *pfear_sts;
>>       const struct pmc_bit_map *mphy_sts;
>>       const struct pmc_bit_map *pll_sts;
>> +     const struct pmc_bit_map **slps0_dbg_maps;
>>       const u32 slp_s0_offset;
>>       const u32 ltr_ignore_offset;
>>       const int regmap_length;
>> @@ -168,6 +173,7 @@ struct pmc_reg_map {
>>       const int ppfear_buckets;
>>       const u32 pm_cfg_offset;
>>       const int pm_read_disable_bit;
>> +     const u32 slps0_dbg_offset;
>>  };
>>
>>  /**
>> --
>> 2.7.4
>>
>
> --
> Best Regards,
> Rajneesh



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V4] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers

[Rajneesh Bhardwaj]

On Mon, Jul 02, 2018 at 03:19:22PM +0300, Andy Shevchenko wrote:
> On Fri, Jun 15, 2018 at 2:27 PM, Rajneesh Bhardwaj
> <rajneesh.bhardwaj@intel.com> wrote:
> > On Thu, Jun 14, 2018 at 03:13:02PM -0700, David E. Box wrote:
> >> From: <david.e.box@intel.com>
> >>
> >> Adds debugfs access to registers in the Cannonlake PCH PMC that are
> >> useful for debugging #SLP_S0 signal assertion and other low power
> >> related activities. Device pm states are latched in these registers
> >> whenever the package enters C10 and can be read from slp_s0_debug_status.
> >> The pm states may also be latched by writing 1 to slp_s0_debug_latch
> >> which will immediately capture the current state on the next read of
> >> slp_s0_debug_status.
> >
> > Reviewed-and-tested-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> 
> Oops, sorry, it went without your tag.
> patchwork seems didn't recognize this kind of combined tags.

NP, thats fine.

> 
> >
> >>
> >> Signed-off-by: Box, David E <david.e.box@intel.com>
> >> ---
> >> V4:
> >>       - rename slp_s0_dbg string to slp_s0_debug for consistency
> >>       - ADD ISCLK prefix to MAIN_PLL and OC_PLL
> >> V3:
> >>       - use null terminator in bit_map array
> >>       - replaced ternary operator with if/else
> >>       - Removed space fixes on old code
> >> V2:
> >>       - Clear latch bit after use
> >>       - Pass pmc_dev as parameter
> >>       - Use DEFINE_SHOW_ATTRIBUTE macro
> >>  drivers/platform/x86/intel_pmc_core.c | 120 ++++++++++++++++++++++++++++++++++
> >>  drivers/platform/x86/intel_pmc_core.h |   6 ++
> >>  2 files changed, 126 insertions(+)
> >>
> >> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> >> index 43bbe74..d00fee2 100644
> >> --- a/drivers/platform/x86/intel_pmc_core.c
> >> +++ b/drivers/platform/x86/intel_pmc_core.c
> >> @@ -196,9 +196,67 @@ static const struct pmc_bit_map cnp_pfear_map[] = {
> >>       {}
> >>  };
> >>
> >> +static const struct pmc_bit_map cnp_slps0_dbg0_map[] = {
> >> +     {"AUDIO_D3",            BIT(0)},
> >> +     {"OTG_D3",              BIT(1)},
> >> +     {"XHCI_D3",             BIT(2)},
> >> +     {"LPIO_D3",             BIT(3)},
> >> +     {"SDX_D3",              BIT(4)},
> >> +     {"SATA_D3",             BIT(5)},
> >> +     {"UFS0_D3",             BIT(6)},
> >> +     {"UFS1_D3",             BIT(7)},
> >> +     {"EMMC_D3",             BIT(8)},
> >> +     {}
> >> +};
> >> +
> >> +static const struct pmc_bit_map cnp_slps0_dbg1_map[] = {
> >> +     {"SDIO_PLL_OFF",        BIT(0)},
> >> +     {"USB2_PLL_OFF",        BIT(1)},
> >> +     {"AUDIO_PLL_OFF",       BIT(2)},
> >> +     {"ISCLK_OC_PLL_OFF",    BIT(3)},
> >> +     {"ISCLK_MAIN_PLL_OFF",  BIT(4)},
> >> +     {"XOSC_OFF",            BIT(5)},
> >> +     {"LPC_CLKS_GATED",      BIT(6)},
> >> +     {"PCIE_CLKREQS_IDLE",   BIT(7)},
> >> +     {"AUDIO_ROSC_OFF",      BIT(8)},
> >> +     {"HPET_XOSC_CLK_REQ",   BIT(9)},
> >> +     {"PMC_ROSC_SLOW_CLK",   BIT(10)},
> >> +     {"AON2_ROSC_GATED",     BIT(11)},
> >> +     {"CLKACKS_DEASSERTED",  BIT(12)},
> >> +     {}
> >> +};
> >> +
> >> +static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
> >> +     {"MPHY_CORE_GATED",     BIT(0)},
> >> +     {"CSME_GATED",          BIT(1)},
> >> +     {"USB2_SUS_GATED",      BIT(2)},
> >> +     {"DYN_FLEX_IO_IDLE",    BIT(3)},
> >> +     {"GBE_NO_LINK",         BIT(4)},
> >> +     {"THERM_SEN_DISABLED",  BIT(5)},
> >> +     {"PCIE_LOW_POWER",      BIT(6)},
> >> +     {"ISH_VNNAON_REQ_ACT",  BIT(7)},
> >> +     {"ISH_VNN_REQ_ACT",     BIT(8)},
> >> +     {"CNV_VNNAON_REQ_ACT",  BIT(9)},
> >> +     {"CNV_VNN_REQ_ACT",     BIT(10)},
> >> +     {"NPK_VNNON_REQ_ACT",   BIT(11)},
> >> +     {"PMSYNC_STATE_IDLE",   BIT(12)},
> >> +     {"ALST_GT_THRES",       BIT(13)},
> >> +     {"PMC_ARC_PG_READY",    BIT(14)},
> >> +     {}
> >> +};
> >> +
> >> +static const struct pmc_bit_map *cnp_slps0_dbg_maps[] = {
> >> +     cnp_slps0_dbg0_map,
> >> +     cnp_slps0_dbg1_map,
> >> +     cnp_slps0_dbg2_map,
> >> +     NULL,
> >> +};
> >> +
> >>  static const struct pmc_reg_map cnp_reg_map = {
> >>       .pfear_sts = cnp_pfear_map,
> >>       .slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
> >> +     .slps0_dbg_maps = cnp_slps0_dbg_maps,
> >> +     .slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
> >>       .ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
> >>       .regmap_length = CNP_PMC_MMIO_REG_LEN,
> >>       .ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
> >> @@ -252,6 +310,8 @@ static int pmc_core_check_read_lock_bit(void)
> >>  }
> >>
> >>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> >> +static bool slps0_dbg_latch;
> >> +
> >>  static void pmc_core_display_map(struct seq_file *s, int index,
> >>                                u8 pf_reg, const struct pmc_bit_map *pf_map)
> >>  {
> >> @@ -481,6 +541,57 @@ static const struct file_operations pmc_core_ltr_ignore_ops = {
> >>       .release        = single_release,
> >>  };
> >>
> >> +static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev, bool reset)
> >> +{
> >> +     const struct pmc_reg_map *map = pmcdev->map;
> >> +     u32 fd;
> >> +
> >> +     mutex_lock(&pmcdev->lock);
> >> +
> >> +     if (!reset && !slps0_dbg_latch)
> >> +             goto out_unlock;
> >> +
> >> +     fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
> >> +     if (reset)
> >> +             fd &= ~CNP_PMC_LATCH_SLPS0_EVENTS;
> >> +     else
> >> +             fd |= CNP_PMC_LATCH_SLPS0_EVENTS;
> >> +     pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
> >> +
> >> +     slps0_dbg_latch = 0;
> >> +
> >> +out_unlock:
> >> +     mutex_unlock(&pmcdev->lock);
> >> +}
> >> +
> >> +static int pmc_core_slps0_dbg_show(struct seq_file *s, void *unused)
> >> +{
> >> +     struct pmc_dev *pmcdev = s->private;
> >> +     const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps;
> >> +     const struct pmc_bit_map *map;
> >> +     int offset;
> >> +     u32 data;
> >> +
> >> +     pmc_core_slps0_dbg_latch(pmcdev, false);
> >> +     offset = pmcdev->map->slps0_dbg_offset;
> >> +     while (*maps) {
> >> +             map = *maps;
> >> +             data = pmc_core_reg_read(pmcdev, offset);
> >> +             offset += 4;
> >> +             while (map->name) {
> >> +                     seq_printf(s, "SLP_S0_DBG: %-32s\tState: %s\n",
> >> +                                map->name,
> >> +                                data & map->bit_mask ?
> >> +                                "Yes" : "No");
> >> +                     ++map;
> >> +             }
> >> +             ++maps;
> >> +     }
> >> +     pmc_core_slps0_dbg_latch(pmcdev, true);
> >> +     return 0;
> >> +}
> >> +DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);
> >> +
> >>  static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
> >>  {
> >>       debugfs_remove_recursive(pmcdev->dbgfs_dir);
> >> @@ -514,6 +625,15 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
> >>                                   0444, dir, pmcdev,
> >>                                   &pmc_core_mphy_pg_ops);
> >>
> >> +     if (pmcdev->map->slps0_dbg_maps) {
> >> +             debugfs_create_file("slp_s0_debug_status", 0444,
> >> +                                 dir, pmcdev,
> >> +                                 &pmc_core_slps0_dbg_fops);
> >> +
> >> +             debugfs_create_bool("slp_s0_debug_latch", 0644,
> >> +                                 dir, &slps0_dbg_latch);
> >> +     }
> >> +
> >>       return 0;
> >>  }
> >>  #else
> >> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> >> index 5fa5f97..93a7e99 100644
> >> --- a/drivers/platform/x86/intel_pmc_core.h
> >> +++ b/drivers/platform/x86/intel_pmc_core.h
> >> @@ -127,12 +127,14 @@ enum ppfear_regs {
> >>  #define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET      0x193C
> >>  #define CNP_PMC_LTR_IGNORE_OFFSET              0x1B0C
> >>  #define CNP_PMC_PM_CFG_OFFSET                  0x1818
> >> +#define CNP_PMC_SLPS0_DBG_OFFSET             0x10B4
> >>  /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */
> >>  #define CNP_PMC_HOST_PPFEAR0A                  0x1D90
> >>
> >>  #define CNP_PMC_MMIO_REG_LEN                   0x2000
> >>  #define CNP_PPFEAR_NUM_ENTRIES                 8
> >>  #define CNP_PMC_READ_DISABLE_BIT               22
> >> +#define CNP_PMC_LATCH_SLPS0_EVENTS           BIT(31)
> >>
> >>  struct pmc_bit_map {
> >>       const char *name;
> >> @@ -145,6 +147,7 @@ struct pmc_bit_map {
> >>   * @pfear_sts:               Maps name of IP block to PPFEAR* bit
> >>   * @mphy_sts:                Maps name of MPHY lane to MPHY status lane status bit
> >>   * @pll_sts:         Maps name of PLL to corresponding bit status
> >> + * @slps0_dbg_maps:  Array of SLP_S0_DBG* registers containing debug info
> >>   * @slp_s0_offset:   PWRMBASE offset to read SLP_S0 residency
> >>   * @ltr_ignore_offset:       PWRMBASE offset to read/write LTR ignore bit
> >>   * @regmap_length:   Length of memory to map from PWRMBASE address to access
> >> @@ -153,6 +156,7 @@ struct pmc_bit_map {
> >>   *                   PPFEAR
> >>   * @pm_cfg_offset:   PWRMBASE offset to PM_CFG register
> >>   * @pm_read_disable_bit: Bit index to read PMC_READ_DISABLE
> >> + * @slps0_dbg_offset:        PWRMBASE offset to SLP_S0_DEBUG_REG*
> >>   *
> >>   * Each PCH has unique set of register offsets and bit indexes. This structure
> >>   * captures them to have a common implementation.
> >> @@ -161,6 +165,7 @@ struct pmc_reg_map {
> >>       const struct pmc_bit_map *pfear_sts;
> >>       const struct pmc_bit_map *mphy_sts;
> >>       const struct pmc_bit_map *pll_sts;
> >> +     const struct pmc_bit_map **slps0_dbg_maps;
> >>       const u32 slp_s0_offset;
> >>       const u32 ltr_ignore_offset;
> >>       const int regmap_length;
> >> @@ -168,6 +173,7 @@ struct pmc_reg_map {
> >>       const int ppfear_buckets;
> >>       const u32 pm_cfg_offset;
> >>       const int pm_read_disable_bit;
> >> +     const u32 slps0_dbg_offset;
> >>  };
> >>
> >>  /**
> >> --
> >> 2.7.4
> >>
> >
> > --
> > Best Regards,
> > Rajneesh
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

-- 
Best Regards,
Rajneesh