[PATCH v2 1/4] platform/x86: intel_pmc_core: Show Latency Tolerance info

[PATCH v2 1/4] platform/x86: intel_pmc_core: Show Latency Tolerance info

[Rajneesh Bhardwaj]

This adds support to show the Latency Tolerance Reporting for the IPs on
the PCH as reported by the PMC. The format shown here is raw LTR data
payload that can further be decoded as per the PCI specification.

This also fixes some minor alignment issues in the header file by
removing spaces and converting to tabs at some places.

Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>
---
Changes in v2:
 * Removed IP # from map and displaying IP # while printing.
 * Other style fixes as per Andy's suggestion.

 drivers/platform/x86/intel_pmc_core.c | 73 +++++++++++++++++++++++++++
 drivers/platform/x86/intel_pmc_core.h | 56 +++++++++++++++++---
 2 files changed, 122 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 2d272a3e0176..217a822a8da1 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -110,10 +110,37 @@ static const struct pmc_bit_map spt_pfear_map[] = {
 	{},
 };
 
+static const struct pmc_bit_map spt_ltr_show_map[] = {
+	{"LTR_SOUTHPORT_A",		SPT_PMC_LTR_SPA},
+	{"LTR_SOUTHPORT_B",		SPT_PMC_LTR_SPB},
+	{"LTR_SATA",			SPT_PMC_LTR_SATA},
+	{"LTR_GIGABIT_ETHERNET",	SPT_PMC_LTR_GBE},
+	{"LTR_XHCI",			SPT_PMC_LTR_XHCI},
+	/* IP 5 is reserved */
+	{"LTR_ME",			SPT_PMC_LTR_ME},
+	/* EVA is Enterprise Value Add, doesn't really exist on PCH */
+	{"LTR_EVA",			SPT_PMC_LTR_EVA},
+	{"LTR_SOUTHPORT_C",		SPT_PMC_LTR_SPC},
+	{"LTR_HD_AUDIO",		SPT_PMC_LTR_AZ},
+	/* IP 10 is reserved */
+	{"LTR_LPSS",			SPT_PMC_LTR_LPSS},
+	{"LTR_SOUTHPORT_D",		SPT_PMC_LTR_SPD},
+	{"LTR_SOUTHPORT_E",		SPT_PMC_LTR_SPE},
+	{"LTR_CAMERA",			SPT_PMC_LTR_CAM},
+	{"LTR_ESPI",			SPT_PMC_LTR_ESPI},
+	{"LTR_SCC",			SPT_PMC_LTR_SCC},
+	{"LTR_ISH",			SPT_PMC_LTR_ISH},
+	/* Below two cannot be used for LTR_IGNORE */
+	{"LTR_CURRENT_PLATFORM",	SPT_PMC_LTR_CUR_PLT},
+	{"LTR_AGGREGATED_SYSTEM",	SPT_PMC_LTR_CUR_ASLT},
+	{}
+};
+
 static const struct pmc_reg_map spt_reg_map = {
 	.pfear_sts = spt_pfear_map,
 	.mphy_sts = spt_mphy_map,
 	.pll_sts = spt_pll_map,
+	.ltr_show_sts = spt_ltr_show_map,
 	.slp_s0_offset = SPT_PMC_SLP_S0_RES_COUNTER_OFFSET,
 	.ltr_ignore_offset = SPT_PMC_LTR_IGNORE_OFFSET,
 	.regmap_length = SPT_PMC_MMIO_REG_LEN,
@@ -252,10 +279,39 @@ static const struct pmc_bit_map *cnp_slps0_dbg_maps[] = {
 	NULL,
 };
 
+static const struct pmc_bit_map cnp_ltr_show_map[] = {
+	{"LTR_SOUTHPORT_A",		CNP_PMC_LTR_SPA},
+	{"LTR_SOUTHPORT_B",		CNP_PMC_LTR_SPB},
+	{"LTR_SATA",			CNP_PMC_LTR_SATA},
+	{"LTR_GIGABIT_ETHERNET",	CNP_PMC_LTR_GBE},
+	{"LTR_XHCI",			CNP_PMC_LTR_XHCI},
+	/* IP 5 is reserved */
+	{"LTR_ME",			CNP_PMC_LTR_ME},
+	/* EVA is Enterprise Value Add, doesn't really exist on PCH */
+	{"LTR_EVA",			CNP_PMC_LTR_EVA},
+	{"LTR_SOUTHPORT_C",		CNP_PMC_LTR_SPC},
+	{"LTR_HD_AUDIO",		CNP_PMC_LTR_AZ},
+	{"LTR_CNV",			CNP_PMC_LTR_CNV},
+	{"LTR_LPSS",			CNP_PMC_LTR_LPSS},
+	{"LTR_SOUTHPORT_D",		CNP_PMC_LTR_SPD},
+	{"LTR_SOUTHPORT_E",		CNP_PMC_LTR_SPE},
+	{"LTR_CAMERA",			CNP_PMC_LTR_CAM},
+	{"LTR_ESPI",			CNP_PMC_LTR_ESPI},
+	{"LTR_SCC",			CNP_PMC_LTR_SCC},
+	{"LTR_ISH",			CNP_PMC_LTR_ISH},
+	{"LTR_UFSX2",			CNP_PMC_LTR_UFSX2},
+	{"LTR_EMMC",			CNP_PMC_LTR_EMMC},
+	/* Below two cannot be used for LTR_IGNORE */
+	{"LTR_CURRENT_PLATFORM",	CNP_PMC_LTR_CUR_PLT},
+	{"LTR_AGGREGATED_SYSTEM",	CNP_PMC_LTR_CUR_ASLT},
+	{}
+};
+
 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,
+	.ltr_show_sts = cnp_ltr_show_map,
 	.slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
 	.ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
 	.regmap_length = CNP_PMC_MMIO_REG_LEN,
@@ -592,6 +648,21 @@ static int pmc_core_slps0_dbg_show(struct seq_file *s, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);
 
+static int pmc_core_ltr_show(struct seq_file *s, void *unused)
+{
+	struct pmc_dev *pmcdev = s->private;
+	const struct pmc_bit_map *map = pmcdev->map->ltr_show_sts;
+	int index;
+
+	for (index = 0; map[index].name ; index++) {
+		seq_printf(s, "IP %-2d :%-32s\tRAW LTR: 0x%x\n", index,
+			   map[index].name,
+			   pmc_core_reg_read(pmcdev, map[index].bit_mask));
+	}
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(pmc_core_ltr);
+
 static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
 {
 	debugfs_remove_recursive(pmcdev->dbgfs_dir);
@@ -616,6 +687,8 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
 	debugfs_create_file("ltr_ignore", 0644, dir, pmcdev,
 			    &pmc_core_ltr_ignore_ops);
 
+	debugfs_create_file("ltr_show", 0644, dir, pmcdev, &pmc_core_ltr_fops);
+
 	if (pmcdev->map->pll_sts)
 		debugfs_create_file("pll_status", 0444, dir, pmcdev,
 				    &pmc_core_pll_ops);
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 93a7e99e1f8b..7a00436e337d 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -46,6 +46,25 @@
 #define NUM_RETRIES				100
 #define NUM_IP_IGN_ALLOWED			17
 
+#define SPT_PMC_LTR_CUR_PLT			0x350
+#define SPT_PMC_LTR_CUR_ASLT			0x354
+#define SPT_PMC_LTR_SPA				0x360
+#define SPT_PMC_LTR_SPB				0x364
+#define SPT_PMC_LTR_SATA			0x368
+#define SPT_PMC_LTR_GBE				0x36C
+#define SPT_PMC_LTR_XHCI			0x370
+#define SPT_PMC_LTR_ME				0x378
+#define SPT_PMC_LTR_EVA				0x37C
+#define SPT_PMC_LTR_SPC				0x380
+#define SPT_PMC_LTR_AZ				0x384
+#define SPT_PMC_LTR_LPSS			0x38C
+#define SPT_PMC_LTR_CAM				0x390
+#define SPT_PMC_LTR_SPD				0x394
+#define SPT_PMC_LTR_SPE				0x398
+#define SPT_PMC_LTR_ESPI			0x39C
+#define SPT_PMC_LTR_SCC				0x3A0
+#define SPT_PMC_LTR_ISH				0x3A4
+
 /* Sunrise Point: PGD PFET Enable Ack Status Registers */
 enum ppfear_regs {
 	SPT_PMC_XRAM_PPFEAR0A = 0x590,
@@ -124,17 +143,38 @@ 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)
+#define CNP_PMC_LTR_CUR_PLT			0x1B50
+#define CNP_PMC_LTR_CUR_ASLT			0x1B54
+#define CNP_PMC_LTR_SPA				0x1B60
+#define CNP_PMC_LTR_SPB				0x1B64
+#define CNP_PMC_LTR_SATA			0x1B68
+#define CNP_PMC_LTR_GBE				0x1B6C
+#define CNP_PMC_LTR_XHCI			0x1B70
+#define CNP_PMC_LTR_ME				0x1B78
+#define CNP_PMC_LTR_EVA				0x1B7C
+#define CNP_PMC_LTR_SPC				0x1B80
+#define CNP_PMC_LTR_AZ				0x1B84
+#define CNP_PMC_LTR_LPSS			0x1B8C
+#define CNP_PMC_LTR_CAM				0x1B90
+#define CNP_PMC_LTR_SPD				0x1B94
+#define CNP_PMC_LTR_SPE				0x1B98
+#define CNP_PMC_LTR_ESPI			0x1B9C
+#define CNP_PMC_LTR_SCC				0x1BA0
+#define CNP_PMC_LTR_ISH				0x1BA4
+#define CNP_PMC_LTR_CNV				0x1BF0
+#define CNP_PMC_LTR_EMMC			0x1BF4
+#define CNP_PMC_LTR_UFSX2			0x1BF8
 
 struct pmc_bit_map {
 	const char *name;
@@ -148,6 +188,7 @@ struct pmc_bit_map {
  * @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
+ * @ltr_show_sts:	Maps PCH IP Names to their MMIO register offsets
  * @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
@@ -166,6 +207,7 @@ struct pmc_reg_map {
 	const struct pmc_bit_map *mphy_sts;
 	const struct pmc_bit_map *pll_sts;
 	const struct pmc_bit_map **slps0_dbg_maps;
+	const struct pmc_bit_map *ltr_show_sts;
 	const u32 slp_s0_offset;
 	const u32 ltr_ignore_offset;
 	const int regmap_length;
-- 
2.17.1

[PATCH v2 2/4] platform/x86: intel_pmc_core: Fix LTR IGNORE Max offset

[Rajneesh Bhardwaj]

Cannonlake PCH allows us to ignore LTR from more IPs than Sunrisepoint
PCH so make the LTR ignore platform specific.

Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_core.c | 4 +++-
 drivers/platform/x86/intel_pmc_core.h | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 217a822a8da1..c616cfedf2be 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -148,6 +148,7 @@ static const struct pmc_reg_map spt_reg_map = {
 	.ppfear_buckets = SPT_PPFEAR_NUM_ENTRIES,
 	.pm_cfg_offset = SPT_PMC_PM_CFG_OFFSET,
 	.pm_read_disable_bit = SPT_PMC_READ_DISABLE_BIT,
+	.ltr_ignore_max = SPT_NUM_IP_IGN_ALLOWED,
 };
 
 /* Cannonlake: PGD PFET Enable Ack Status Register(s) bitmap */
@@ -319,6 +320,7 @@ static const struct pmc_reg_map cnp_reg_map = {
 	.ppfear_buckets = CNP_PPFEAR_NUM_ENTRIES,
 	.pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
 	.pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
+	.ltr_ignore_max = CNP_NUM_IP_IGN_ALLOWED,
 };
 
 static inline u8 pmc_core_reg_read_byte(struct pmc_dev *pmcdev, int offset)
@@ -565,7 +567,7 @@ static ssize_t pmc_core_ltr_ignore_write(struct file *file, const char __user
 		goto out_unlock;
 	}
 
-	if (val > NUM_IP_IGN_ALLOWED) {
+	if (val > map->ltr_ignore_max) {
 		err = -EINVAL;
 		goto out_unlock;
 	}
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 7a00436e337d..7f8181057ec8 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -44,7 +44,7 @@
 #define SPT_PMC_READ_DISABLE_BIT		0x16
 #define SPT_PMC_MSG_FULL_STS_BIT		0x18
 #define NUM_RETRIES				100
-#define NUM_IP_IGN_ALLOWED			17
+#define SPT_NUM_IP_IGN_ALLOWED			17
 
 #define SPT_PMC_LTR_CUR_PLT			0x350
 #define SPT_PMC_LTR_CUR_ASLT			0x354
@@ -154,6 +154,7 @@ enum ppfear_regs {
 #define CNP_PPFEAR_NUM_ENTRIES			8
 #define CNP_PMC_READ_DISABLE_BIT		22
 #define CNP_PMC_LATCH_SLPS0_EVENTS		BIT(31)
+#define CNP_NUM_IP_IGN_ALLOWED			19
 #define CNP_PMC_LTR_CUR_PLT			0x1B50
 #define CNP_PMC_LTR_CUR_ASLT			0x1B54
 #define CNP_PMC_LTR_SPA				0x1B60
@@ -216,6 +217,7 @@ struct pmc_reg_map {
 	const u32 pm_cfg_offset;
 	const int pm_read_disable_bit;
 	const u32 slps0_dbg_offset;
+	const u32 ltr_ignore_max;
 };
 
 /**
-- 
2.17.1

Re: [PATCH v2 2/4] platform/x86: intel_pmc_core: Fix LTR IGNORE Max offset

[Andy Shevchenko]

On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj
<rajneesh.bhardwaj@linux.intel.com> wrote:
>
> Cannonlake PCH allows us to ignore LTR from more IPs than Sunrisepoint
> PCH so make the LTR ignore platform specific.
>

Pushed to my reviewing and testing queue, thanks!

> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 4 +++-
>  drivers/platform/x86/intel_pmc_core.h | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 217a822a8da1..c616cfedf2be 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -148,6 +148,7 @@ static const struct pmc_reg_map spt_reg_map = {
>         .ppfear_buckets = SPT_PPFEAR_NUM_ENTRIES,
>         .pm_cfg_offset = SPT_PMC_PM_CFG_OFFSET,
>         .pm_read_disable_bit = SPT_PMC_READ_DISABLE_BIT,
> +       .ltr_ignore_max = SPT_NUM_IP_IGN_ALLOWED,
>  };
>
>  /* Cannonlake: PGD PFET Enable Ack Status Register(s) bitmap */
> @@ -319,6 +320,7 @@ static const struct pmc_reg_map cnp_reg_map = {
>         .ppfear_buckets = CNP_PPFEAR_NUM_ENTRIES,
>         .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
>         .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
> +       .ltr_ignore_max = CNP_NUM_IP_IGN_ALLOWED,
>  };
>
>  static inline u8 pmc_core_reg_read_byte(struct pmc_dev *pmcdev, int offset)
> @@ -565,7 +567,7 @@ static ssize_t pmc_core_ltr_ignore_write(struct file *file, const char __user
>                 goto out_unlock;
>         }
>
> -       if (val > NUM_IP_IGN_ALLOWED) {
> +       if (val > map->ltr_ignore_max) {
>                 err = -EINVAL;
>                 goto out_unlock;
>         }
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index 7a00436e337d..7f8181057ec8 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -44,7 +44,7 @@
>  #define SPT_PMC_READ_DISABLE_BIT               0x16
>  #define SPT_PMC_MSG_FULL_STS_BIT               0x18
>  #define NUM_RETRIES                            100
> -#define NUM_IP_IGN_ALLOWED                     17
> +#define SPT_NUM_IP_IGN_ALLOWED                 17
>
>  #define SPT_PMC_LTR_CUR_PLT                    0x350
>  #define SPT_PMC_LTR_CUR_ASLT                   0x354
> @@ -154,6 +154,7 @@ enum ppfear_regs {
>  #define CNP_PPFEAR_NUM_ENTRIES                 8
>  #define CNP_PMC_READ_DISABLE_BIT               22
>  #define CNP_PMC_LATCH_SLPS0_EVENTS             BIT(31)
> +#define CNP_NUM_IP_IGN_ALLOWED                 19
>  #define CNP_PMC_LTR_CUR_PLT                    0x1B50
>  #define CNP_PMC_LTR_CUR_ASLT                   0x1B54
>  #define CNP_PMC_LTR_SPA                                0x1B60
> @@ -216,6 +217,7 @@ struct pmc_reg_map {
>         const u32 pm_cfg_offset;
>         const int pm_read_disable_bit;
>         const u32 slps0_dbg_offset;
> +       const u32 ltr_ignore_max;
>  };
>
>  /**
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 3/4] platform/x86: intel_pmc_core: Decode Snoop / Non Snoop LTR

[Rajneesh Bhardwaj]

The LTR values follow PCIE LTR encoding format and can be decoded as per
https://pcisig.com/sites/default/files/specification_documents/ECN_LatencyTolnReporting_14Aug08.pdf

This adds support to translate the raw LTR values as read from the PMC
to meaningful values in nanosecond units of time.

Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>
---
Changes in v2:
 * Get rid of union and bitfields to decode LTR and use FIELD_GET macro
 * Change get_ltr_scale to convert_ltr_scale.
 * Other style fixes and misc. improvements suggested by Andy for v1.

 drivers/platform/x86/intel_pmc_core.c | 64 +++++++++++++++++++++++++--
 drivers/platform/x86/intel_pmc_core.h |  5 +++
 2 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index c616cfedf2be..fbcab53456f3 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -21,6 +21,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/acpi.h>
+#include <linux/bitfield.h>
 #include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/io.h>
@@ -650,16 +651,73 @@ static int pmc_core_slps0_dbg_show(struct seq_file *s, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);
 
+static u32 convert_ltr_scale(u32 val)
+{
+	u32 scale = 0;
+	/*
+	 * As per PCIE specification supporting document
+	 * ECN_LatencyTolnReporting_14Aug08.pdf the Latency
+	 * Tolerance Reporting data payload is encoded in a
+	 * 3 bit scale and 10 bit value fields. Values are
+	 * multiplied by the indicated scale to yield an absolute time
+	 * value, expressible in a range from 1 nanosecond to
+	 * 2^25*(2^10-1) = 34,326,183,936 nanoseconds.
+	 *
+	 * scale encoding is as follows:
+	 *
+	 * ----------------------------------------------
+	 * |scale factor	|	Multiplier (ns)	|
+	 * ----------------------------------------------
+	 * |	0		|	1		|
+	 * |	1		|	32		|
+	 * |	2		|	1024		|
+	 * |	3		|	32768		|
+	 * |	4		|	1048576		|
+	 * |	5		|	33554432	|
+	 * |	6		|	Invalid		|
+	 * |	7		|	Invalid		|
+	 * ----------------------------------------------
+	 */
+	if (val > 5)
+		pr_warn("Invalid LTR scale factor.\n");
+	else
+		scale = 1U << (5 * (val));
+
+	return scale;
+}
+
 static int pmc_core_ltr_show(struct seq_file *s, void *unused)
 {
 	struct pmc_dev *pmcdev = s->private;
 	const struct pmc_bit_map *map = pmcdev->map->ltr_show_sts;
+	u64 decoded_snoop_ltr, decoded_non_snoop_ltr;
+	u32 ltr_raw_data, scale, val;
+	u16 snoop_ltr, nonsnoop_ltr;
 	int index;
 
 	for (index = 0; map[index].name ; index++) {
-		seq_printf(s, "IP %-2d :%-32s\tRAW LTR: 0x%x\n", index,
-			   map[index].name,
-			   pmc_core_reg_read(pmcdev, map[index].bit_mask));
+		decoded_snoop_ltr = decoded_non_snoop_ltr = 0;
+		ltr_raw_data = pmc_core_reg_read(pmcdev,
+						 map[index].bit_mask);
+		snoop_ltr = ltr_raw_data & ~MTPMC_MASK;
+		nonsnoop_ltr = (ltr_raw_data >> 0x10) & ~MTPMC_MASK;
+
+		if (FIELD_GET(LTR_REQ_NONSNOOP, ltr_raw_data)) {
+			scale = FIELD_GET(LTR_DECODED_SCALE, nonsnoop_ltr);
+			val = FIELD_GET(LTR_DECODED_VAL, nonsnoop_ltr);
+			decoded_non_snoop_ltr = val * convert_ltr_scale(scale);
+		}
+
+		if (FIELD_GET(LTR_REQ_SNOOP, ltr_raw_data)) {
+			scale = FIELD_GET(LTR_DECODED_SCALE, snoop_ltr);
+			val = FIELD_GET(LTR_DECODED_VAL, snoop_ltr);
+			decoded_snoop_ltr = val * convert_ltr_scale(scale);
+		}
+
+		seq_printf(s, "IP %-2d :%-24s\tRaw LTR: 0x%-16x\t Non-Snoop LTR (ns): %-16llu\t Snoop LTR (ns): %-16llu\n",
+			   index, map[index].name, ltr_raw_data,
+			   decoded_non_snoop_ltr,
+			   decoded_snoop_ltr);
 	}
 	return 0;
 }
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 7f8181057ec8..cc49cd4c86e9 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -177,6 +177,11 @@ enum ppfear_regs {
 #define CNP_PMC_LTR_EMMC			0x1BF4
 #define CNP_PMC_LTR_UFSX2			0x1BF8
 
+#define LTR_REQ_NONSNOOP			BIT(31)
+#define LTR_REQ_SNOOP				BIT(15)
+#define LTR_DECODED_VAL				GENMASK(9, 0)
+#define LTR_DECODED_SCALE			GENMASK(12, 10)
+
 struct pmc_bit_map {
 	const char *name;
 	u32 bit_mask;
-- 
2.17.1

Re: [PATCH v2 3/4] platform/x86: intel_pmc_core: Decode Snoop / Non Snoop LTR

[Andy Shevchenko]

On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj
<rajneesh.bhardwaj@linux.intel.com> wrote:
>
> The LTR values follow PCIE LTR encoding format and can be decoded as per
> https://pcisig.com/sites/default/files/specification_documents/ECN_LatencyTolnReporting_14Aug08.pdf
>
> This adds support to translate the raw LTR values as read from the PMC
> to meaningful values in nanosecond units of time.

While I have pushed this to my review and testing queue, it needs a
bit more work. See my comments below.

> +static u32 convert_ltr_scale(u32 val)
> +{

> +       u32 scale = 0;

Redundant, see below.

> +       /*
> +        * As per PCIE specification supporting document
> +        * ECN_LatencyTolnReporting_14Aug08.pdf the Latency
> +        * Tolerance Reporting data payload is encoded in a
> +        * 3 bit scale and 10 bit value fields. Values are
> +        * multiplied by the indicated scale to yield an absolute time
> +        * value, expressible in a range from 1 nanosecond to
> +        * 2^25*(2^10-1) = 34,326,183,936 nanoseconds.
> +        *
> +        * scale encoding is as follows:
> +        *
> +        * ----------------------------------------------
> +        * |scale factor        |       Multiplier (ns) |
> +        * ----------------------------------------------
> +        * |    0               |       1               |
> +        * |    1               |       32              |
> +        * |    2               |       1024            |
> +        * |    3               |       32768           |
> +        * |    4               |       1048576         |
> +        * |    5               |       33554432        |
> +        * |    6               |       Invalid         |
> +        * |    7               |       Invalid         |
> +        * ----------------------------------------------
> +        */

> +       if (val > 5)

> +               pr_warn("Invalid LTR scale factor.\n");

if (...) {
 pr_warn(...); // Btw, Does it recoverable state? What user will get
with returned 0 as a multiplier?
 return 0; // Btw, is 0 fits better than ~0? How hw would behave with
this value?
}

> +       else
> +               scale = 1U << (5 * (val));
> +
> +       return scale;

return 1U << (5 * val);

> +}

>         for (index = 0; map[index].name ; index++) {
> -               seq_printf(s, "IP %-2d :%-32s\tRAW LTR: 0x%x\n", index,
> -                          map[index].name,
> -                          pmc_core_reg_read(pmcdev, map[index].bit_mask));

We use 32 characters for the names. Here are two minor issues:
- inconsistency with the rest
- ping-pong style of programming (you changed 32 to 24 in the same
series where you introduced 32 in the first place).


> +               decoded_snoop_ltr = decoded_non_snoop_ltr = 0;
> +               ltr_raw_data = pmc_core_reg_read(pmcdev,
> +                                                map[index].bit_mask);
> +               snoop_ltr = ltr_raw_data & ~MTPMC_MASK;
> +               nonsnoop_ltr = (ltr_raw_data >> 0x10) & ~MTPMC_MASK;
> +
> +               if (FIELD_GET(LTR_REQ_NONSNOOP, ltr_raw_data)) {
> +                       scale = FIELD_GET(LTR_DECODED_SCALE, nonsnoop_ltr);
> +                       val = FIELD_GET(LTR_DECODED_VAL, nonsnoop_ltr);
> +                       decoded_non_snoop_ltr = val * convert_ltr_scale(scale);
> +               }
> +
> +               if (FIELD_GET(LTR_REQ_SNOOP, ltr_raw_data)) {
> +                       scale = FIELD_GET(LTR_DECODED_SCALE, snoop_ltr);
> +                       val = FIELD_GET(LTR_DECODED_VAL, snoop_ltr);
> +                       decoded_snoop_ltr = val * convert_ltr_scale(scale);
> +               }
> +

> +               seq_printf(s, "IP %-2d :%-24s\tRaw LTR: 0x%-16x\t Non-Snoop LTR (ns): %-16llu\t Snoop LTR (ns): %-16llu\n",

Here 0x%-16x would look a bit strange and difficult to parse. 0x%016x
much better.
After you remove the index, it would give you 4 more characters,
though it 4 less than 8 you got from reducing 32 to 24.

OTOH, those long texts perhaps may be compressed somehow, at least
remove LTR duplicating from the last two. Remove spaces after '\t' as
well.

> +                          index, map[index].name, ltr_raw_data,
> +                          decoded_non_snoop_ltr,
> +                          decoded_snoop_ltr);
>         }
>         return 0;
>  }

> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -177,6 +177,11 @@ enum ppfear_regs {

It might be good idea to include linux/bits.h here.

> +#define LTR_REQ_NONSNOOP                       BIT(31)
> +#define LTR_REQ_SNOOP                          BIT(15)
> +#define LTR_DECODED_VAL                                GENMASK(9, 0)
> +#define LTR_DECODED_SCALE                      GENMASK(12, 10)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] platform/x86: intel_pmc_core: Decode Snoop / Non Snoop LTR

[Bhardwaj, Rajneesh]

Hi Andy,

Thanks for your review. My comments below.

If you agree then i can quickly send v3 addressing all suggestions so we 
can make it in time for 4.20 merge window.


On 19-Oct-18 6:04 PM, Andy Shevchenko wrote:
> On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj
> <rajneesh.bhardwaj@linux.intel.com> wrote:
>> The LTR values follow PCIE LTR encoding format and can be decoded as per
>> https://pcisig.com/sites/default/files/specification_documents/ECN_LatencyTolnReporting_14Aug08.pdf
>>
>> This adds support to translate the raw LTR values as read from the PMC
>> to meaningful values in nanosecond units of time.
> While I have pushed this to my review and testing queue, it needs a
> bit more work. See my comments below.
>
>> +static u32 convert_ltr_scale(u32 val)
>> +{
>> +       u32 scale = 0;
> Redundant, see below.
>
>> +       /*
>> +        * As per PCIE specification supporting document
>> +        * ECN_LatencyTolnReporting_14Aug08.pdf the Latency
>> +        * Tolerance Reporting data payload is encoded in a
>> +        * 3 bit scale and 10 bit value fields. Values are
>> +        * multiplied by the indicated scale to yield an absolute time
>> +        * value, expressible in a range from 1 nanosecond to
>> +        * 2^25*(2^10-1) = 34,326,183,936 nanoseconds.
>> +        *
>> +        * scale encoding is as follows:
>> +        *
>> +        * ----------------------------------------------
>> +        * |scale factor        |       Multiplier (ns) |
>> +        * ----------------------------------------------
>> +        * |    0               |       1               |
>> +        * |    1               |       32              |
>> +        * |    2               |       1024            |
>> +        * |    3               |       32768           |
>> +        * |    4               |       1048576         |
>> +        * |    5               |       33554432        |
>> +        * |    6               |       Invalid         |
>> +        * |    7               |       Invalid         |
>> +        * ----------------------------------------------
>> +        */
>> +       if (val > 5)
>> +               pr_warn("Invalid LTR scale factor.\n");
> if (...) {
>   pr_warn(...); // Btw, Does it recoverable state? What user will get
> with returned 0 as a multiplier?
>   return 0; // Btw, is 0 fits better than ~0? How hw would behave with
> this value?
> }

We show 0 LTR for invalid scale as PMC output is junk sometimes.


>
>> +       else
>> +               scale = 1U << (5 * (val));
>> +
>> +       return scale;
> return 1U << (5 * val);

We intend to return 0 so for invalid LTR scale. This will make retuen 
non zero and we dont want that.

>
>> +}
>>          for (index = 0; map[index].name ; index++) {
>> -               seq_printf(s, "IP %-2d :%-32s\tRAW LTR: 0x%x\n", index,
>> -                          map[index].name,
>> -                          pmc_core_reg_read(pmcdev, map[index].bit_mask));
> We use 32 characters for the names. Here are two minor issues:
> - inconsistency with the rest

intentional.

> - ping-pong style of programming (you changed 32 to 24 in the same
> series where you introduced 32 in the first place).

This is because the formatted output looks better with this width. I 
used 32 for the earlier patch because its consistent with rest and also 
does not look bad on screen.

>
>
>> +               decoded_snoop_ltr = decoded_non_snoop_ltr = 0;
>> +               ltr_raw_data = pmc_core_reg_read(pmcdev,
>> +                                                map[index].bit_mask);
>> +               snoop_ltr = ltr_raw_data & ~MTPMC_MASK;
>> +               nonsnoop_ltr = (ltr_raw_data >> 0x10) & ~MTPMC_MASK;
>> +
>> +               if (FIELD_GET(LTR_REQ_NONSNOOP, ltr_raw_data)) {
>> +                       scale = FIELD_GET(LTR_DECODED_SCALE, nonsnoop_ltr);
>> +                       val = FIELD_GET(LTR_DECODED_VAL, nonsnoop_ltr);
>> +                       decoded_non_snoop_ltr = val * convert_ltr_scale(scale);
>> +               }
>> +
>> +               if (FIELD_GET(LTR_REQ_SNOOP, ltr_raw_data)) {
>> +                       scale = FIELD_GET(LTR_DECODED_SCALE, snoop_ltr);
>> +                       val = FIELD_GET(LTR_DECODED_VAL, snoop_ltr);
>> +                       decoded_snoop_ltr = val * convert_ltr_scale(scale);
>> +               }
>> +
>> +               seq_printf(s, "IP %-2d :%-24s\tRaw LTR: 0x%-16x\t Non-Snoop LTR (ns): %-16llu\t Snoop LTR (ns): %-16llu\n",
> Here 0x%-16x would look a bit strange and difficult to parse. 0x%016x
> much better.

Sure, I can test how it looks with 0x%016x and modify it.

> After you remove the index, it would give you 4 more characters,
> though it 4 less than 8 you got from reducing 32 to 24.

I plan to keep the index as is. Reason for this is mentioned in previous 
patch that introduces this index.

>
> OTOH, those long texts perhaps may be compressed somehow, at least
> remove LTR duplicating from the last two. Remove spaces after '\t' as
> well.

Noted.

>
>> +                          index, map[index].name, ltr_raw_data,
>> +                          decoded_non_snoop_ltr,
>> +                          decoded_snoop_ltr);
>>          }
>>          return 0;
>>   }
>> --- a/drivers/platform/x86/intel_pmc_core.h
>> +++ b/drivers/platform/x86/intel_pmc_core.h
>> @@ -177,6 +177,11 @@ enum ppfear_regs {
> It might be good idea to include linux/bits.h here.

Certainly. Luckily 0day bot didnt complain about randconfigs etc so is 
it really needed as it will increase size?

>
>> +#define LTR_REQ_NONSNOOP                       BIT(31)
>> +#define LTR_REQ_SNOOP                          BIT(15)
>> +#define LTR_DECODED_VAL                                GENMASK(9, 0)
>> +#define LTR_DECODED_SCALE                      GENMASK(12, 10)

Re: [PATCH v2 3/4] platform/x86: intel_pmc_core: Decode Snoop / Non Snoop LTR

[Andy Shevchenko]

On Tue, Oct 30, 2018 at 9:40 AM Bhardwaj, Rajneesh
<rajneesh.bhardwaj@linux.intel.com> wrote:

> Thanks for your review. My comments below.
>
> If you agree then i can quickly send v3 addressing all suggestions so we
> can make it in time for 4.20 merge window.

I don't like `quickly` part — usual way to make the last minute mistakes.
How long does testing take?

> On 19-Oct-18 6:04 PM, Andy Shevchenko wrote:
> > On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj
> > <rajneesh.bhardwaj@linux.intel.com> wrote:
> >> The LTR values follow PCIE LTR encoding format and can be decoded as per
> >> https://pcisig.com/sites/default/files/specification_documents/ECN_LatencyTolnReporting_14Aug08.pdf
> >>
> >> This adds support to translate the raw LTR values as read from the PMC
> >> to meaningful values in nanosecond units of time.
> > While I have pushed this to my review and testing queue, it needs a
> > bit more work. See my comments below.

> >> +       u32 scale = 0;
> > Redundant, see below.

> >> +       if (val > 5)
> >> +               pr_warn("Invalid LTR scale factor.\n");
> > if (...) {
> >   pr_warn(...); // Btw, Does it recoverable state? What user will get
> > with returned 0 as a multiplier?
> >   return 0; // Btw, is 0 fits better than ~0? How hw would behave with
> > this value?
> > }
>
> We show 0 LTR for invalid scale as PMC output is junk sometimes.

> >> +       else
> >> +               scale = 1U << (5 * (val));
> >> +
> >> +       return scale;
> > return 1U << (5 * val);
>
> We intend to return 0 so for invalid LTR scale. This will make retuen
> non zero and we dont want that.

And my example, if being read carefully, doesn't alter that.

> >> +}
> >>          for (index = 0; map[index].name ; index++) {
> >> -               seq_printf(s, "IP %-2d :%-32s\tRAW LTR: 0x%x\n", index,
> >> -                          map[index].name,
> >> -                          pmc_core_reg_read(pmcdev, map[index].bit_mask));
> > We use 32 characters for the names. Here are two minor issues:
> > - inconsistency with the rest
>
> intentional.

I understand that, but this is not like the rest is printed.
OK, this is bikeshedding, it's your call, but keep in mind the
avoidance of ping-pong programming.

> > - ping-pong style of programming (you changed 32 to 24 in the same
> > series where you introduced 32 in the first place).
>
> This is because the formatted output looks better with this width. I
> used 32 for the earlier patch because its consistent with rest and also
> does not look bad on screen.

See how it looks like with my proposals below.

> > After you remove the index, it would give you 4 more characters,
> > though it 4 less than 8 you got from reducing 32 to 24.
>
> I plan to keep the index as is. Reason for this is mentioned in previous
> patch that introduces this index.

No, please don't. We have a numerous userspace tools which are doing
this pretty well.

> >> --- a/drivers/platform/x86/intel_pmc_core.h
> >> +++ b/drivers/platform/x86/intel_pmc_core.h
> >> @@ -177,6 +177,11 @@ enum ppfear_regs {
> > It might be good idea to include linux/bits.h here.
>
> Certainly. Luckily 0day bot didnt complain about randconfigs etc so is
> it really needed as it will increase size?

You are lucky because of ordering of inclusions. This is fragile.
Since you are using macros from bits.h better to explicitly show this
dependency.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 4/4] platform/x86: intel_telemetry: report debugfs failure

[Rajneesh Bhardwaj]

On some Goldmont based systems such as ASRock J3455M the BIOS may not
enable the IPC1 device that provides access to the PMC and PUNIT. In
such scenarios, the IOSS and PSS resources from the platform device can
not be obtained and result in a invalid telemetry_plt_config which is an
internal data structure that holds platform config and is maintained by
the telemetry platform driver.

This is also applicable to the platforms where the BIOS supports IPC1
device under debug configurations but IPC1 is disabled by user or the
policy.

This change allows user to know the reason for not seeing entries under
/sys/kernel/debug/telemetry/* when there is no apparent failure at boot.

Cc: Matt Turner <matt.turner@intel.com>
Cc: Len Brown <len.brown@intel.com>
Cc: Souvik Kumar Chakravarty <souvik.k.chakravarty@intel.com>
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com>

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198779
Acked-by: Matt Turner <matt.turner@intel.com>
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>
---
Changes in v2:
 * Removed print and out label both as suggested by Andy.
 * changed to pr_info.
 * Other minor style fixes.


 drivers/platform/x86/intel_telemetry_debugfs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c b/drivers/platform/x86/intel_telemetry_debugfs.c
index ffd0474b0531..1423fa8710fd 100644
--- a/drivers/platform/x86/intel_telemetry_debugfs.c
+++ b/drivers/platform/x86/intel_telemetry_debugfs.c
@@ -951,12 +951,16 @@ static int __init telemetry_debugfs_init(void)
 	debugfs_conf = (struct telemetry_debugfs_conf *)id->driver_data;
 
 	err = telemetry_pltconfig_valid();
-	if (err < 0)
+	if (err < 0) {
+		pr_info("Invalid pltconfig, ensure IPC1 device is enabled in BIOS\n");
 		return -ENODEV;
+	}
 
 	err = telemetry_debugfs_check_evts();
-	if (err < 0)
+	if (err < 0) {
+		pr_info("telemetry_debugfs_check_evts failed\n");
 		return -EINVAL;
+	}
 
 	register_pm_notifier(&pm_notifier);
 
-- 
2.17.1

Re: [PATCH v2 4/4] platform/x86: intel_telemetry: report debugfs failure

[Andy Shevchenko]

On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj
<rajneesh.bhardwaj@linux.intel.com> wrote:
>
> On some Goldmont based systems such as ASRock J3455M the BIOS may not
> enable the IPC1 device that provides access to the PMC and PUNIT. In
> such scenarios, the IOSS and PSS resources from the platform device can
> not be obtained and result in a invalid telemetry_plt_config which is an
> internal data structure that holds platform config and is maintained by
> the telemetry platform driver.
>
> This is also applicable to the platforms where the BIOS supports IPC1
> device under debug configurations but IPC1 is disabled by user or the
> policy.
>
> This change allows user to know the reason for not seeing entries under
> /sys/kernel/debug/telemetry/* when there is no apparent failure at boot.
>

Pushed to my review and testing queue, thanks!

P.S. I appended one more patch against this file, please check if it's okay.

> Cc: Matt Turner <matt.turner@intel.com>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Souvik Kumar Chakravarty <souvik.k.chakravarty@intel.com>
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com>
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198779
> Acked-by: Matt Turner <matt.turner@intel.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>
> ---
> Changes in v2:
>  * Removed print and out label both as suggested by Andy.
>  * changed to pr_info.
>  * Other minor style fixes.
>
>
>  drivers/platform/x86/intel_telemetry_debugfs.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c b/drivers/platform/x86/intel_telemetry_debugfs.c
> index ffd0474b0531..1423fa8710fd 100644
> --- a/drivers/platform/x86/intel_telemetry_debugfs.c
> +++ b/drivers/platform/x86/intel_telemetry_debugfs.c
> @@ -951,12 +951,16 @@ static int __init telemetry_debugfs_init(void)
>         debugfs_conf = (struct telemetry_debugfs_conf *)id->driver_data;
>
>         err = telemetry_pltconfig_valid();
> -       if (err < 0)
> +       if (err < 0) {
> +               pr_info("Invalid pltconfig, ensure IPC1 device is enabled in BIOS\n");
>                 return -ENODEV;
> +       }
>
>         err = telemetry_debugfs_check_evts();
> -       if (err < 0)
> +       if (err < 0) {
> +               pr_info("telemetry_debugfs_check_evts failed\n");
>                 return -EINVAL;
> +       }
>
>         register_pm_notifier(&pm_notifier);
>
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/4] platform/x86: intel_telemetry: report debugfs failure

[Bhardwaj, Rajneesh]

On 19-Oct-18 6:09 PM, Andy Shevchenko wrote:
> On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj
> <rajneesh.bhardwaj@linux.intel.com> wrote:
>> On some Goldmont based systems such as ASRock J3455M the BIOS may not
>> enable the IPC1 device that provides access to the PMC and PUNIT. In
>> such scenarios, the IOSS and PSS resources from the platform device can
>> not be obtained and result in a invalid telemetry_plt_config which is an
>> internal data structure that holds platform config and is maintained by
>> the telemetry platform driver.
>>
>> This is also applicable to the platforms where the BIOS supports IPC1
>> device under debug configurations but IPC1 is disabled by user or the
>> policy.
>>
>> This change allows user to know the reason for not seeing entries under
>> /sys/kernel/debug/telemetry/* when there is no apparent failure at boot.
>>
> Pushed to my review and testing queue, thanks!
>
> P.S. I appended one more patch against this file, please check if it's okay.

Thank you Andy. I will check it when Infradead is online.

>
>> Cc: Matt Turner <matt.turner@intel.com>
>> Cc: Len Brown <len.brown@intel.com>
>> Cc: Souvik Kumar Chakravarty <souvik.k.chakravarty@intel.com>
>> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com>
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198779
>> Acked-by: Matt Turner <matt.turner@intel.com>
>> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>
>> ---
>> Changes in v2:
>>   * Removed print and out label both as suggested by Andy.
>>   * changed to pr_info.
>>   * Other minor style fixes.
>>
>>
>>   drivers/platform/x86/intel_telemetry_debugfs.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c b/drivers/platform/x86/intel_telemetry_debugfs.c
>> index ffd0474b0531..1423fa8710fd 100644
>> --- a/drivers/platform/x86/intel_telemetry_debugfs.c
>> +++ b/drivers/platform/x86/intel_telemetry_debugfs.c
>> @@ -951,12 +951,16 @@ static int __init telemetry_debugfs_init(void)
>>          debugfs_conf = (struct telemetry_debugfs_conf *)id->driver_data;
>>
>>          err = telemetry_pltconfig_valid();
>> -       if (err < 0)
>> +       if (err < 0) {
>> +               pr_info("Invalid pltconfig, ensure IPC1 device is enabled in BIOS\n");
>>                  return -ENODEV;
>> +       }
>>
>>          err = telemetry_debugfs_check_evts();
>> -       if (err < 0)
>> +       if (err < 0) {
>> +               pr_info("telemetry_debugfs_check_evts failed\n");
>>                  return -EINVAL;
>> +       }
>>
>>          register_pm_notifier(&pm_notifier);
>>
>> --
>> 2.17.1
>>
>

Re: [PATCH v2 4/4] platform/x86: intel_telemetry: report debugfs failure

[Andy Shevchenko]

On Tue, Oct 30, 2018 at 9:41 AM Bhardwaj, Rajneesh
<rajneesh.bhardwaj@linux.intel.com> wrote:
>
>
>
> On 19-Oct-18 6:09 PM, Andy Shevchenko wrote:
> > On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj
> > <rajneesh.bhardwaj@linux.intel.com> wrote:
> >> On some Goldmont based systems such as ASRock J3455M the BIOS may not
> >> enable the IPC1 device that provides access to the PMC and PUNIT. In
> >> such scenarios, the IOSS and PSS resources from the platform device can
> >> not be obtained and result in a invalid telemetry_plt_config which is an
> >> internal data structure that holds platform config and is maintained by
> >> the telemetry platform driver.
> >>
> >> This is also applicable to the platforms where the BIOS supports IPC1
> >> device under debug configurations but IPC1 is disabled by user or the
> >> policy.
> >>
> >> This change allows user to know the reason for not seeing entries under
> >> /sys/kernel/debug/telemetry/* when there is no apparent failure at boot.
> >>
> > Pushed to my review and testing queue, thanks!
> >
> > P.S. I appended one more patch against this file, please check if it's okay.
>
> Thank you Andy. I will check it when Infradead is online.

You may check our mirror on GH:
https://github.com/dvhart/linux-pdx86

>
> >
> >> Cc: Matt Turner <matt.turner@intel.com>
> >> Cc: Len Brown <len.brown@intel.com>
> >> Cc: Souvik Kumar Chakravarty <souvik.k.chakravarty@intel.com>
> >> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com>
> >>
> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198779
> >> Acked-by: Matt Turner <matt.turner@intel.com>
> >> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>
> >> ---
> >> Changes in v2:
> >>   * Removed print and out label both as suggested by Andy.
> >>   * changed to pr_info.
> >>   * Other minor style fixes.
> >>
> >>
> >>   drivers/platform/x86/intel_telemetry_debugfs.c | 8 ++++++--
> >>   1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c b/drivers/platform/x86/intel_telemetry_debugfs.c
> >> index ffd0474b0531..1423fa8710fd 100644
> >> --- a/drivers/platform/x86/intel_telemetry_debugfs.c
> >> +++ b/drivers/platform/x86/intel_telemetry_debugfs.c
> >> @@ -951,12 +951,16 @@ static int __init telemetry_debugfs_init(void)
> >>          debugfs_conf = (struct telemetry_debugfs_conf *)id->driver_data;
> >>
> >>          err = telemetry_pltconfig_valid();
> >> -       if (err < 0)
> >> +       if (err < 0) {
> >> +               pr_info("Invalid pltconfig, ensure IPC1 device is enabled in BIOS\n");
> >>                  return -ENODEV;
> >> +       }
> >>
> >>          err = telemetry_debugfs_check_evts();
> >> -       if (err < 0)
> >> +       if (err < 0) {
> >> +               pr_info("telemetry_debugfs_check_evts failed\n");
> >>                  return -EINVAL;
> >> +       }
> >>
> >>          register_pm_notifier(&pm_notifier);
> >>
> >> --
> >> 2.17.1
> >>
> >
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] platform/x86: intel_pmc_core: Show Latency Tolerance info

[Andy Shevchenko]

On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj
<rajneesh.bhardwaj@linux.intel.com> wrote:
>
> This adds support to show the Latency Tolerance Reporting for the IPs on
> the PCH as reported by the PMC. The format shown here is raw LTR data
> payload that can further be decoded as per the PCI specification.
>
> This also fixes some minor alignment issues in the header file by
> removing spaces and converting to tabs at some places.
>

I have pushed slightly modified variant of this patch to my review and
testing queue.
Though this series needs a bit more work.

I see inconsistency now with the output with the rest of nodes there.
I don't care much, though some can be addressed for no regression.

For example, index printing. Please, remove it. I completely forgot
about userspace powerful tools. At least two very old and nice can
enumerate lines for you.
Obviously the index printing is redundant.

> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>
> ---
> Changes in v2:
>  * Removed IP # from map and displaying IP # while printing.
>  * Other style fixes as per Andy's suggestion.
>
>  drivers/platform/x86/intel_pmc_core.c | 73 +++++++++++++++++++++++++++
>  drivers/platform/x86/intel_pmc_core.h | 56 +++++++++++++++++---
>  2 files changed, 122 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 2d272a3e0176..217a822a8da1 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -110,10 +110,37 @@ static const struct pmc_bit_map spt_pfear_map[] = {
>         {},
>  };
>
> +static const struct pmc_bit_map spt_ltr_show_map[] = {
> +       {"LTR_SOUTHPORT_A",             SPT_PMC_LTR_SPA},
> +       {"LTR_SOUTHPORT_B",             SPT_PMC_LTR_SPB},
> +       {"LTR_SATA",                    SPT_PMC_LTR_SATA},
> +       {"LTR_GIGABIT_ETHERNET",        SPT_PMC_LTR_GBE},
> +       {"LTR_XHCI",                    SPT_PMC_LTR_XHCI},
> +       /* IP 5 is reserved */
> +       {"LTR_ME",                      SPT_PMC_LTR_ME},
> +       /* EVA is Enterprise Value Add, doesn't really exist on PCH */
> +       {"LTR_EVA",                     SPT_PMC_LTR_EVA},
> +       {"LTR_SOUTHPORT_C",             SPT_PMC_LTR_SPC},
> +       {"LTR_HD_AUDIO",                SPT_PMC_LTR_AZ},
> +       /* IP 10 is reserved */
> +       {"LTR_LPSS",                    SPT_PMC_LTR_LPSS},
> +       {"LTR_SOUTHPORT_D",             SPT_PMC_LTR_SPD},
> +       {"LTR_SOUTHPORT_E",             SPT_PMC_LTR_SPE},
> +       {"LTR_CAMERA",                  SPT_PMC_LTR_CAM},
> +       {"LTR_ESPI",                    SPT_PMC_LTR_ESPI},
> +       {"LTR_SCC",                     SPT_PMC_LTR_SCC},
> +       {"LTR_ISH",                     SPT_PMC_LTR_ISH},
> +       /* Below two cannot be used for LTR_IGNORE */
> +       {"LTR_CURRENT_PLATFORM",        SPT_PMC_LTR_CUR_PLT},
> +       {"LTR_AGGREGATED_SYSTEM",       SPT_PMC_LTR_CUR_ASLT},
> +       {}
> +};
> +
>  static const struct pmc_reg_map spt_reg_map = {
>         .pfear_sts = spt_pfear_map,
>         .mphy_sts = spt_mphy_map,
>         .pll_sts = spt_pll_map,
> +       .ltr_show_sts = spt_ltr_show_map,
>         .slp_s0_offset = SPT_PMC_SLP_S0_RES_COUNTER_OFFSET,
>         .ltr_ignore_offset = SPT_PMC_LTR_IGNORE_OFFSET,
>         .regmap_length = SPT_PMC_MMIO_REG_LEN,
> @@ -252,10 +279,39 @@ static const struct pmc_bit_map *cnp_slps0_dbg_maps[] = {
>         NULL,
>  };
>
> +static const struct pmc_bit_map cnp_ltr_show_map[] = {
> +       {"LTR_SOUTHPORT_A",             CNP_PMC_LTR_SPA},
> +       {"LTR_SOUTHPORT_B",             CNP_PMC_LTR_SPB},
> +       {"LTR_SATA",                    CNP_PMC_LTR_SATA},
> +       {"LTR_GIGABIT_ETHERNET",        CNP_PMC_LTR_GBE},
> +       {"LTR_XHCI",                    CNP_PMC_LTR_XHCI},
> +       /* IP 5 is reserved */
> +       {"LTR_ME",                      CNP_PMC_LTR_ME},
> +       /* EVA is Enterprise Value Add, doesn't really exist on PCH */
> +       {"LTR_EVA",                     CNP_PMC_LTR_EVA},
> +       {"LTR_SOUTHPORT_C",             CNP_PMC_LTR_SPC},
> +       {"LTR_HD_AUDIO",                CNP_PMC_LTR_AZ},
> +       {"LTR_CNV",                     CNP_PMC_LTR_CNV},
> +       {"LTR_LPSS",                    CNP_PMC_LTR_LPSS},
> +       {"LTR_SOUTHPORT_D",             CNP_PMC_LTR_SPD},
> +       {"LTR_SOUTHPORT_E",             CNP_PMC_LTR_SPE},
> +       {"LTR_CAMERA",                  CNP_PMC_LTR_CAM},
> +       {"LTR_ESPI",                    CNP_PMC_LTR_ESPI},
> +       {"LTR_SCC",                     CNP_PMC_LTR_SCC},
> +       {"LTR_ISH",                     CNP_PMC_LTR_ISH},
> +       {"LTR_UFSX2",                   CNP_PMC_LTR_UFSX2},
> +       {"LTR_EMMC",                    CNP_PMC_LTR_EMMC},
> +       /* Below two cannot be used for LTR_IGNORE */
> +       {"LTR_CURRENT_PLATFORM",        CNP_PMC_LTR_CUR_PLT},
> +       {"LTR_AGGREGATED_SYSTEM",       CNP_PMC_LTR_CUR_ASLT},
> +       {}
> +};
> +
>  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,
> +       .ltr_show_sts = cnp_ltr_show_map,
>         .slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
>         .ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
>         .regmap_length = CNP_PMC_MMIO_REG_LEN,
> @@ -592,6 +648,21 @@ static int pmc_core_slps0_dbg_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);
>
> +static int pmc_core_ltr_show(struct seq_file *s, void *unused)
> +{
> +       struct pmc_dev *pmcdev = s->private;
> +       const struct pmc_bit_map *map = pmcdev->map->ltr_show_sts;
> +       int index;
> +
> +       for (index = 0; map[index].name ; index++) {
> +               seq_printf(s, "IP %-2d :%-32s\tRAW LTR: 0x%x\n", index,
> +                          map[index].name,
> +                          pmc_core_reg_read(pmcdev, map[index].bit_mask));
> +       }
> +       return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pmc_core_ltr);
> +
>  static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
>  {
>         debugfs_remove_recursive(pmcdev->dbgfs_dir);
> @@ -616,6 +687,8 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>         debugfs_create_file("ltr_ignore", 0644, dir, pmcdev,
>                             &pmc_core_ltr_ignore_ops);
>
> +       debugfs_create_file("ltr_show", 0644, dir, pmcdev, &pmc_core_ltr_fops);
> +
>         if (pmcdev->map->pll_sts)
>                 debugfs_create_file("pll_status", 0444, dir, pmcdev,
>                                     &pmc_core_pll_ops);
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index 93a7e99e1f8b..7a00436e337d 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -46,6 +46,25 @@
>  #define NUM_RETRIES                            100
>  #define NUM_IP_IGN_ALLOWED                     17
>
> +#define SPT_PMC_LTR_CUR_PLT                    0x350
> +#define SPT_PMC_LTR_CUR_ASLT                   0x354
> +#define SPT_PMC_LTR_SPA                                0x360
> +#define SPT_PMC_LTR_SPB                                0x364
> +#define SPT_PMC_LTR_SATA                       0x368
> +#define SPT_PMC_LTR_GBE                                0x36C
> +#define SPT_PMC_LTR_XHCI                       0x370
> +#define SPT_PMC_LTR_ME                         0x378
> +#define SPT_PMC_LTR_EVA                                0x37C
> +#define SPT_PMC_LTR_SPC                                0x380
> +#define SPT_PMC_LTR_AZ                         0x384
> +#define SPT_PMC_LTR_LPSS                       0x38C
> +#define SPT_PMC_LTR_CAM                                0x390
> +#define SPT_PMC_LTR_SPD                                0x394
> +#define SPT_PMC_LTR_SPE                                0x398
> +#define SPT_PMC_LTR_ESPI                       0x39C
> +#define SPT_PMC_LTR_SCC                                0x3A0
> +#define SPT_PMC_LTR_ISH                                0x3A4
> +
>  /* Sunrise Point: PGD PFET Enable Ack Status Registers */
>  enum ppfear_regs {
>         SPT_PMC_XRAM_PPFEAR0A = 0x590,
> @@ -124,17 +143,38 @@ 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)
> +#define CNP_PMC_LTR_CUR_PLT                    0x1B50
> +#define CNP_PMC_LTR_CUR_ASLT                   0x1B54
> +#define CNP_PMC_LTR_SPA                                0x1B60
> +#define CNP_PMC_LTR_SPB                                0x1B64
> +#define CNP_PMC_LTR_SATA                       0x1B68
> +#define CNP_PMC_LTR_GBE                                0x1B6C
> +#define CNP_PMC_LTR_XHCI                       0x1B70
> +#define CNP_PMC_LTR_ME                         0x1B78
> +#define CNP_PMC_LTR_EVA                                0x1B7C
> +#define CNP_PMC_LTR_SPC                                0x1B80
> +#define CNP_PMC_LTR_AZ                         0x1B84
> +#define CNP_PMC_LTR_LPSS                       0x1B8C
> +#define CNP_PMC_LTR_CAM                                0x1B90
> +#define CNP_PMC_LTR_SPD                                0x1B94
> +#define CNP_PMC_LTR_SPE                                0x1B98
> +#define CNP_PMC_LTR_ESPI                       0x1B9C
> +#define CNP_PMC_LTR_SCC                                0x1BA0
> +#define CNP_PMC_LTR_ISH                                0x1BA4
> +#define CNP_PMC_LTR_CNV                                0x1BF0
> +#define CNP_PMC_LTR_EMMC                       0x1BF4
> +#define CNP_PMC_LTR_UFSX2                      0x1BF8
>
>  struct pmc_bit_map {
>         const char *name;
> @@ -148,6 +188,7 @@ struct pmc_bit_map {
>   * @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
> + * @ltr_show_sts:      Maps PCH IP Names to their MMIO register offsets
>   * @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
> @@ -166,6 +207,7 @@ struct pmc_reg_map {
>         const struct pmc_bit_map *mphy_sts;
>         const struct pmc_bit_map *pll_sts;
>         const struct pmc_bit_map **slps0_dbg_maps;
> +       const struct pmc_bit_map *ltr_show_sts;
>         const u32 slp_s0_offset;
>         const u32 ltr_ignore_offset;
>         const int regmap_length;
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] platform/x86: intel_pmc_core: Show Latency Tolerance info

[Bhardwaj, Rajneesh]

+ Srinivas


On 19-Oct-18 5:42 PM, Andy Shevchenko wrote:
> On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj
> <rajneesh.bhardwaj@linux.intel.com> wrote:
>> This adds support to show the Latency Tolerance Reporting for the IPs on
>> the PCH as reported by the PMC. The format shown here is raw LTR data
>> payload that can further be decoded as per the PCI specification.
>>
>> This also fixes some minor alignment issues in the header file by
>> removing spaces and converting to tabs at some places.
>>
> I have pushed slightly modified variant of this patch to my review and
> testing queue.
> Though this series needs a bit more work.

Thanks again for your review Andy. I see that the infradead server is 
down at the moment so i haven't seen your modifications yet.
http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/review-andy 
says gateway timeout.

>
> I see inconsistency now with the output with the rest of nodes there.

index printing is not needed for those nodes hence i never added.

> I don't care much, though some can be addressed for no regression.
>
> For example, index printing. Please, remove it. I completely forgot
> about userspace powerful tools. At least two very old and nice can
> enumerate lines for you.
> Obviously the index printing is redundant.

Index printing is required here (for LTR Show and LTR Ignore) because it 
paves an obvious and easy way for the users of this driver to know the 
IP number to be used for LTR ignore. This was specifically requested by 
some customer and Srinivas asked me to implement this so adding him for 
his inputs.

I am also planning to add some documentation for this driver later so 
please consider this in current form.

>
>> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>
>> ---
>> Changes in v2:
>>   * Removed IP # from map and displaying IP # while printing.
>>   * Other style fixes as per Andy's suggestion.
>>
>>   drivers/platform/x86/intel_pmc_core.c | 73 +++++++++++++++++++++++++++
>>   drivers/platform/x86/intel_pmc_core.h | 56 +++++++++++++++++---
>>   2 files changed, 122 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
>> index 2d272a3e0176..217a822a8da1 100644
>> --- a/drivers/platform/x86/intel_pmc_core.c
>> +++ b/drivers/platform/x86/intel_pmc_core.c
>> @@ -110,10 +110,37 @@ static const struct pmc_bit_map spt_pfear_map[] = {
>>          {},
>>   };
>>
>> +static const struct pmc_bit_map spt_ltr_show_map[] = {
>> +       {"LTR_SOUTHPORT_A",             SPT_PMC_LTR_SPA},
>> +       {"LTR_SOUTHPORT_B",             SPT_PMC_LTR_SPB},
>> +       {"LTR_SATA",                    SPT_PMC_LTR_SATA},
>> +       {"LTR_GIGABIT_ETHERNET",        SPT_PMC_LTR_GBE},
>> +       {"LTR_XHCI",                    SPT_PMC_LTR_XHCI},
>> +       /* IP 5 is reserved */
>> +       {"LTR_ME",                      SPT_PMC_LTR_ME},
>> +       /* EVA is Enterprise Value Add, doesn't really exist on PCH */
>> +       {"LTR_EVA",                     SPT_PMC_LTR_EVA},
>> +       {"LTR_SOUTHPORT_C",             SPT_PMC_LTR_SPC},
>> +       {"LTR_HD_AUDIO",                SPT_PMC_LTR_AZ},
>> +       /* IP 10 is reserved */
>> +       {"LTR_LPSS",                    SPT_PMC_LTR_LPSS},
>> +       {"LTR_SOUTHPORT_D",             SPT_PMC_LTR_SPD},
>> +       {"LTR_SOUTHPORT_E",             SPT_PMC_LTR_SPE},
>> +       {"LTR_CAMERA",                  SPT_PMC_LTR_CAM},
>> +       {"LTR_ESPI",                    SPT_PMC_LTR_ESPI},
>> +       {"LTR_SCC",                     SPT_PMC_LTR_SCC},
>> +       {"LTR_ISH",                     SPT_PMC_LTR_ISH},
>> +       /* Below two cannot be used for LTR_IGNORE */
>> +       {"LTR_CURRENT_PLATFORM",        SPT_PMC_LTR_CUR_PLT},
>> +       {"LTR_AGGREGATED_SYSTEM",       SPT_PMC_LTR_CUR_ASLT},
>> +       {}
>> +};
>> +
>>   static const struct pmc_reg_map spt_reg_map = {
>>          .pfear_sts = spt_pfear_map,
>>          .mphy_sts = spt_mphy_map,
>>          .pll_sts = spt_pll_map,
>> +       .ltr_show_sts = spt_ltr_show_map,
>>          .slp_s0_offset = SPT_PMC_SLP_S0_RES_COUNTER_OFFSET,
>>          .ltr_ignore_offset = SPT_PMC_LTR_IGNORE_OFFSET,
>>          .regmap_length = SPT_PMC_MMIO_REG_LEN,
>> @@ -252,10 +279,39 @@ static const struct pmc_bit_map *cnp_slps0_dbg_maps[] = {
>>          NULL,
>>   };
>>
>> +static const struct pmc_bit_map cnp_ltr_show_map[] = {
>> +       {"LTR_SOUTHPORT_A",             CNP_PMC_LTR_SPA},
>> +       {"LTR_SOUTHPORT_B",             CNP_PMC_LTR_SPB},
>> +       {"LTR_SATA",                    CNP_PMC_LTR_SATA},
>> +       {"LTR_GIGABIT_ETHERNET",        CNP_PMC_LTR_GBE},
>> +       {"LTR_XHCI",                    CNP_PMC_LTR_XHCI},
>> +       /* IP 5 is reserved */
>> +       {"LTR_ME",                      CNP_PMC_LTR_ME},
>> +       /* EVA is Enterprise Value Add, doesn't really exist on PCH */
>> +       {"LTR_EVA",                     CNP_PMC_LTR_EVA},
>> +       {"LTR_SOUTHPORT_C",             CNP_PMC_LTR_SPC},
>> +       {"LTR_HD_AUDIO",                CNP_PMC_LTR_AZ},
>> +       {"LTR_CNV",                     CNP_PMC_LTR_CNV},
>> +       {"LTR_LPSS",                    CNP_PMC_LTR_LPSS},
>> +       {"LTR_SOUTHPORT_D",             CNP_PMC_LTR_SPD},
>> +       {"LTR_SOUTHPORT_E",             CNP_PMC_LTR_SPE},
>> +       {"LTR_CAMERA",                  CNP_PMC_LTR_CAM},
>> +       {"LTR_ESPI",                    CNP_PMC_LTR_ESPI},
>> +       {"LTR_SCC",                     CNP_PMC_LTR_SCC},
>> +       {"LTR_ISH",                     CNP_PMC_LTR_ISH},
>> +       {"LTR_UFSX2",                   CNP_PMC_LTR_UFSX2},
>> +       {"LTR_EMMC",                    CNP_PMC_LTR_EMMC},
>> +       /* Below two cannot be used for LTR_IGNORE */
>> +       {"LTR_CURRENT_PLATFORM",        CNP_PMC_LTR_CUR_PLT},
>> +       {"LTR_AGGREGATED_SYSTEM",       CNP_PMC_LTR_CUR_ASLT},
>> +       {}
>> +};
>> +
>>   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,
>> +       .ltr_show_sts = cnp_ltr_show_map,
>>          .slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
>>          .ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
>>          .regmap_length = CNP_PMC_MMIO_REG_LEN,
>> @@ -592,6 +648,21 @@ static int pmc_core_slps0_dbg_show(struct seq_file *s, void *unused)
>>   }
>>   DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);
>>
>> +static int pmc_core_ltr_show(struct seq_file *s, void *unused)
>> +{
>> +       struct pmc_dev *pmcdev = s->private;
>> +       const struct pmc_bit_map *map = pmcdev->map->ltr_show_sts;
>> +       int index;
>> +
>> +       for (index = 0; map[index].name ; index++) {
>> +               seq_printf(s, "IP %-2d :%-32s\tRAW LTR: 0x%x\n", index,
>> +                          map[index].name,
>> +                          pmc_core_reg_read(pmcdev, map[index].bit_mask));
>> +       }
>> +       return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(pmc_core_ltr);
>> +
>>   static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
>>   {
>>          debugfs_remove_recursive(pmcdev->dbgfs_dir);
>> @@ -616,6 +687,8 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
>>          debugfs_create_file("ltr_ignore", 0644, dir, pmcdev,
>>                              &pmc_core_ltr_ignore_ops);
>>
>> +       debugfs_create_file("ltr_show", 0644, dir, pmcdev, &pmc_core_ltr_fops);
>> +
>>          if (pmcdev->map->pll_sts)
>>                  debugfs_create_file("pll_status", 0444, dir, pmcdev,
>>                                      &pmc_core_pll_ops);
>> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
>> index 93a7e99e1f8b..7a00436e337d 100644
>> --- a/drivers/platform/x86/intel_pmc_core.h
>> +++ b/drivers/platform/x86/intel_pmc_core.h
>> @@ -46,6 +46,25 @@
>>   #define NUM_RETRIES                            100
>>   #define NUM_IP_IGN_ALLOWED                     17
>>
>> +#define SPT_PMC_LTR_CUR_PLT                    0x350
>> +#define SPT_PMC_LTR_CUR_ASLT                   0x354
>> +#define SPT_PMC_LTR_SPA                                0x360
>> +#define SPT_PMC_LTR_SPB                                0x364
>> +#define SPT_PMC_LTR_SATA                       0x368
>> +#define SPT_PMC_LTR_GBE                                0x36C
>> +#define SPT_PMC_LTR_XHCI                       0x370
>> +#define SPT_PMC_LTR_ME                         0x378
>> +#define SPT_PMC_LTR_EVA                                0x37C
>> +#define SPT_PMC_LTR_SPC                                0x380
>> +#define SPT_PMC_LTR_AZ                         0x384
>> +#define SPT_PMC_LTR_LPSS                       0x38C
>> +#define SPT_PMC_LTR_CAM                                0x390
>> +#define SPT_PMC_LTR_SPD                                0x394
>> +#define SPT_PMC_LTR_SPE                                0x398
>> +#define SPT_PMC_LTR_ESPI                       0x39C
>> +#define SPT_PMC_LTR_SCC                                0x3A0
>> +#define SPT_PMC_LTR_ISH                                0x3A4
>> +
>>   /* Sunrise Point: PGD PFET Enable Ack Status Registers */
>>   enum ppfear_regs {
>>          SPT_PMC_XRAM_PPFEAR0A = 0x590,
>> @@ -124,17 +143,38 @@ 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)
>> +#define CNP_PMC_LTR_CUR_PLT                    0x1B50
>> +#define CNP_PMC_LTR_CUR_ASLT                   0x1B54
>> +#define CNP_PMC_LTR_SPA                                0x1B60
>> +#define CNP_PMC_LTR_SPB                                0x1B64
>> +#define CNP_PMC_LTR_SATA                       0x1B68
>> +#define CNP_PMC_LTR_GBE                                0x1B6C
>> +#define CNP_PMC_LTR_XHCI                       0x1B70
>> +#define CNP_PMC_LTR_ME                         0x1B78
>> +#define CNP_PMC_LTR_EVA                                0x1B7C
>> +#define CNP_PMC_LTR_SPC                                0x1B80
>> +#define CNP_PMC_LTR_AZ                         0x1B84
>> +#define CNP_PMC_LTR_LPSS                       0x1B8C
>> +#define CNP_PMC_LTR_CAM                                0x1B90
>> +#define CNP_PMC_LTR_SPD                                0x1B94
>> +#define CNP_PMC_LTR_SPE                                0x1B98
>> +#define CNP_PMC_LTR_ESPI                       0x1B9C
>> +#define CNP_PMC_LTR_SCC                                0x1BA0
>> +#define CNP_PMC_LTR_ISH                                0x1BA4
>> +#define CNP_PMC_LTR_CNV                                0x1BF0
>> +#define CNP_PMC_LTR_EMMC                       0x1BF4
>> +#define CNP_PMC_LTR_UFSX2                      0x1BF8
>>
>>   struct pmc_bit_map {
>>          const char *name;
>> @@ -148,6 +188,7 @@ struct pmc_bit_map {
>>    * @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
>> + * @ltr_show_sts:      Maps PCH IP Names to their MMIO register offsets
>>    * @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
>> @@ -166,6 +207,7 @@ struct pmc_reg_map {
>>          const struct pmc_bit_map *mphy_sts;
>>          const struct pmc_bit_map *pll_sts;
>>          const struct pmc_bit_map **slps0_dbg_maps;
>> +       const struct pmc_bit_map *ltr_show_sts;
>>          const u32 slp_s0_offset;
>>          const u32 ltr_ignore_offset;
>>          const int regmap_length;
>> --
>> 2.17.1
>>
>

Re: [PATCH v2 1/4] platform/x86: intel_pmc_core: Show Latency Tolerance info

[Andy Shevchenko]

On Tue, Oct 30, 2018 at 9:25 AM Bhardwaj, Rajneesh
<rajneesh.bhardwaj@linux.intel.com> wrote:
>
> + Srinivas
>
>
> On 19-Oct-18 5:42 PM, Andy Shevchenko wrote:
> > On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj
> > <rajneesh.bhardwaj@linux.intel.com> wrote:
> >> This adds support to show the Latency Tolerance Reporting for the IPs on
> >> the PCH as reported by the PMC. The format shown here is raw LTR data
> >> payload that can further be decoded as per the PCI specification.
> >>
> >> This also fixes some minor alignment issues in the header file by
> >> removing spaces and converting to tabs at some places.
> >>
> > I have pushed slightly modified variant of this patch to my review and
> > testing queue.
> > Though this series needs a bit more work.
>
> Thanks again for your review Andy. I see that the infradead server is
> down at the moment so i haven't seen your modifications yet.
> http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/review-andy
> says gateway timeout.

Oops, yesterday it worked perfectly.

> > I see inconsistency now with the output with the rest of nodes there.
>
> index printing is not needed for those nodes hence i never added.
>
> > I don't care much, though some can be addressed for no regression.
> >
> > For example, index printing. Please, remove it. I completely forgot
> > about userspace powerful tools. At least two very old and nice can
> > enumerate lines for you.
> > Obviously the index printing is redundant.
>
> Index printing is required here (for LTR Show and LTR Ignore) because it
> paves an obvious and easy way for the users of this driver to know the
> IP number to be used for LTR ignore. This was specifically requested by
> some customer and Srinivas asked me to implement this so adding him for
> his inputs.

So, why it should be in kernel? When user prints this, they usually
call `cat /.../file`, right?
Is it too hard to call `cat -n /.../file` instead? The benefit of such
approach is that it's independent on the file we are printing.

(Note, `grep -n <PATTERN> /.../file` does the same`)

For more variants
https://stackoverflow.com/questions/8206370/add-numbers-to-the-beginning-of-every-line-in-a-file

> I am also planning to add some documentation for this driver later so
> please consider this in current form.

Good.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] platform/x86: intel_pmc_core: Show Latency Tolerance info

[Srinivas Pandruvada]

[...]
> > > For example, index printing. Please, remove it. I completely
> > > forgot
> > > about userspace powerful tools. At least two very old and nice
> > > can
> > > enumerate lines for you.
> > > Obviously the index printing is redundant.
> > 
> > Index printing is required here (for LTR Show and LTR Ignore)
> > because it
> > paves an obvious and easy way for the users of this driver to know
> > the
> > IP number to be used for LTR ignore. This was specifically
> > requested by
> > some customer and Srinivas asked me to implement this so adding him
> > for
> > his inputs.
> 
> So, why it should be in kernel? When user prints this, they usually
> call `cat /.../file`, right?
> Is it too hard to call `cat -n /.../file` instead? The benefit of
> such
> approach is that it's independent on the file we are printing.
> 
> (Note, `grep -n <PATTERN> /.../file` does the same`)
> 
> For more variants
> 
https://stackoverflow.com/questions/8206370/add-numbers-to-the-beginning-of-every-line-in-a-file
> 
We get copy/paste data from some serial terminals from systems which
don't have traditional linux shell or busy box. Not sure if they can do
cat "-n" option or have this command at all. So line number helps. They
can't even store output as as file as this is RO file system.

But I am not as sticky on this.

Thanks,
Srinivas

Re: [PATCH v2 1/4] platform/x86: intel_pmc_core: Show Latency Tolerance info

[Andy Shevchenko]

On Tue, Oct 30, 2018 at 8:03 PM Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:

> > > Index printing is required here (for LTR Show and LTR Ignore)
> > > because it
> > > paves an obvious and easy way for the users of this driver to know
> > > the
> > > IP number to be used for LTR ignore. This was specifically
> > > requested by
> > > some customer and Srinivas asked me to implement this so adding him
> > > for
> > > his inputs.
> >
> > So, why it should be in kernel? When user prints this, they usually
> > call `cat /.../file`, right?
> > Is it too hard to call `cat -n /.../file` instead? The benefit of
> > such
> > approach is that it's independent on the file we are printing.
> >
> > (Note, `grep -n <PATTERN> /.../file` does the same`)
> >
> > For more variants
> >
> https://stackoverflow.com/questions/8206370/add-numbers-to-the-beginning-of-every-line-in-a-file
> >
> We get copy/paste data from some serial terminals from systems which
> don't have traditional linux shell or busy box. Not sure if they can do
> cat "-n" option or have this command at all. So line number helps. They
> can't even store output as as file as this is RO file system.

Hmm... I'm not following this. If there is serial connection where at
least you may see things, how it's guaranteed that it will not print
more enough to rewrite the DTE's input buffer?
On the other hand if you copy the data to the other system which, I
bet, has `cat -n` available, not a problem either.

So, the use case here, AFAICS, if you have a debug log enabled and
it's spitted out like SysRq where you can see, but not able to copy
and it's guaranteed not to overflow on the screen / output device.

> But I am not as sticky on this.

Since it's a debugfs and not any ABI implied, I'm fine with it to
have, but I would like to understand the real use case of it (and this
definitely should be reflected in the commit message).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] platform/x86: intel_pmc_core: Show Latency Tolerance info

[Srinivas Pandruvada]

On Tue, 2018-10-30 at 20:33 +0200, Andy Shevchenko wrote:
> On Tue, Oct 30, 2018 at 8:03 PM Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> 
> > > > Index printing is required here (for LTR Show and LTR Ignore)
> > > > because it
> > > > paves an obvious and easy way for the users of this driver to
> > > > know
> > > > the
> > > > IP number to be used for LTR ignore. This was specifically
> > > > requested by
> > > > some customer and Srinivas asked me to implement this so adding
> > > > him
> > > > for
> > > > his inputs.
> > > 
> > > So, why it should be in kernel? When user prints this, they
> > > usually
> > > call `cat /.../file`, right?
> > > Is it too hard to call `cat -n /.../file` instead? The benefit of
> > > such
> > > approach is that it's independent on the file we are printing.
> > > 
> > > (Note, `grep -n <PATTERN> /.../file` does the same`)
> > > 
> > > For more variants
> > > 
> > 
> > 
https://stackoverflow.com/questions/8206370/add-numbers-to-the-beginning-of-every-line-in-a-file
> > > 
> > 
> > We get copy/paste data from some serial terminals from systems
> > which
> > don't have traditional linux shell or busy box. Not sure if they
> > can do
> > cat "-n" option or have this command at all. So line number helps.
> > They
> > can't even store output as as file as this is RO file system.
> 
> Hmm... I'm not following this. If there is serial connection where at
> least you may see things, how it's guaranteed that it will not print
> more enough to rewrite the DTE's input buffer?
No guarantee, This is just best effort.
We get something like this from emails:

Device	S-state	  Status   Sysfs node BR1A	  S4	*disabled  pci:
0000:00:01.0 BR1B	  S4	*disabled BR2A	  S4	*disabled  pci:
0000:00:02.0

Any line marker helps. But again this is not a hard requirement.
There will always be argument, that this can be done in other ways.

For sake of time discussing this:

Rajneesh,
Please get rid of index printing.

> On the other hand if you copy the data to the other system which, I
> bet, has `cat -n` available, not a problem either.
> 
> So, the use case here, AFAICS, if you have a debug log enabled and
> it's spitted out like SysRq where you can see, but not able to copy
> and it's guaranteed not to overflow on the screen / output device.
> 
> > But I am not as sticky on this.
> 
> Since it's a debugfs and not any ABI implied, I'm fine with it to
> have, but I would like to understand the real use case of it (and
> this
> definitely should be reflected in the commit message).
>