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 <[email protected]>
---
drivers/platform/x86/intel_pmc_core.c | 75 +++++++++++++++++++++++++++
drivers/platform/x86/intel_pmc_core.h | 55 +++++++++++++++++---
2 files changed, 123 insertions(+), 7 deletions(-)
diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 2d272a3e0176..972735bd4c75 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -110,10 +110,38 @@ static const struct pmc_bit_map spt_pfear_map[] = {
{},
};
+static const struct pmc_bit_map spt_ltr_show_map[] = {
+ {"IP 0 : LTR_SOUTHPORT_A", SPT_PMC_LTR_SPA},
+ {"IP 1 : LTR_SOUTHPORT_B", SPT_PMC_LTR_SPB},
+ {"IP 2 : LTR_SATA", SPT_PMC_LTR_SATA},
+ {"IP 3 : LTR_GIGABIT_ETHERNET", SPT_PMC_LTR_GBE},
+ {"IP 4 : LTR_XHCI", SPT_PMC_LTR_XHCI},
+ /* IP 5 is reserved */
+ {"IP 6 : LTR_ME", SPT_PMC_LTR_ME},
+ /* EVA is Enterprise Value Add, doesn't really exist on PCH */
+ {"IP 7 : LTR_EVA", SPT_PMC_LTR_EVA},
+ {"IP 8 : LTR_SOUTHPORT_C", SPT_PMC_LTR_SPC},
+ {"IP 9 : LTR_HD_AUDIO", SPT_PMC_LTR_AZ},
+ /* IP 10 is reserved */
+ {"IP 11 : LTR_LPSS", SPT_PMC_LTR_LPSS},
+ {"IP 12 : LTR_SOUTHPORT_D", SPT_PMC_LTR_SPD},
+ {"IP 13 : LTR_SOUTHPORT_E", SPT_PMC_LTR_SPE},
+ {"IP 14 : LTR_CAMERA", SPT_PMC_LTR_CAM},
+ {"IP 15 : LTR_ESPI", SPT_PMC_LTR_ESPI},
+ {"IP 16 : LTR_SCC", SPT_PMC_LTR_SCC},
+ {"IP 17 : LTR_ISH", SPT_PMC_LTR_ISH},
+ /* Below two cannot be 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 +280,39 @@ static const struct pmc_bit_map *cnp_slps0_dbg_maps[] = {
NULL,
};
+static const struct pmc_bit_map cnp_ltr_show_map[] = {
+ {"IP 0 : LTR_SOUTHPORT_A", CNP_PMC_LTR_SPA},
+ {"IP 1 : LTR_SOUTHPORT_B", CNP_PMC_LTR_SPB},
+ {"IP 2 : LTR_SATA", CNP_PMC_LTR_SATA},
+ {"IP 3 : LTR_GIGABIT_ETHERNET", CNP_PMC_LTR_GBE},
+ {"IP 4 : LTR_XHCI", CNP_PMC_LTR_XHCI},
+ /* IP 5 is reserved */
+ {"IP 6 : LTR_ME", CNP_PMC_LTR_ME},
+ /* EVA is Enterprise Value Add, doesn't really exist on PCH */
+ {"IP 7 : LTR_EVA", CNP_PMC_LTR_EVA},
+ {"IP 8 : LTR_SOUTHPORT_C", CNP_PMC_LTR_SPC},
+ {"IP 9 : LTR_HD_AUDIO", CNP_PMC_LTR_AZ},
+ {"IP 10 : LTR_CNV", CNP_PMC_LTR_CNV},
+ {"IP 11 : LTR_LPSS", CNP_PMC_LTR_LPSS},
+ {"IP 12 : LTR_SOUTHPORT_D", CNP_PMC_LTR_SPD},
+ {"IP 13 : LTR_SOUTHPORT_E", CNP_PMC_LTR_SPE},
+ {"IP 14 : LTR_CAMERA", CNP_PMC_LTR_CAM},
+ {"IP 15 : LTR_ESPI", CNP_PMC_LTR_ESPI},
+ {"IP 16 : LTR_SCC", CNP_PMC_LTR_SCC},
+ {"IP 17 : LTR_ISH", CNP_PMC_LTR_ISH},
+ {"IP 18 : LTR_UFSX2", CNP_PMC_LTR_UFSX2},
+ {"IP 19 : LTR_EMMC", CNP_PMC_LTR_EMMC},
+ {"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 +649,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, "%-32s\tRAW LTR: 0x%x\n",
+ 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 +688,9 @@ 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..b24407048fa1 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -45,6 +45,24 @@
#define SPT_PMC_MSG_FULL_STS_BIT 0x18
#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 {
@@ -124,17 +142,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 +187,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 +206,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
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 <[email protected]>
---
drivers/platform/x86/intel_pmc_core.c | 62 ++++++++++++++++++++++++++-
drivers/platform/x86/intel_pmc_core.h | 14 ++++++
2 files changed, 74 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index c1330a03523d..ffa912a8a5f5 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -651,16 +651,74 @@ static int pmc_core_slps0_dbg_show(struct seq_file *s, void *unused)
}
DEFINE_SHOW_ATTRIBUTE(pmc_core_slps0_dbg);
+static void get_ltr_scale(u32 *val)
+{
+ /*
+ * As per PCIE specification supprting 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) {
+ *val = 0;
+ pr_warn("Invalid LTR scale factor.\n");
+ } else {
+ *val = 1U << (5 * (*val));
+ }
+}
+
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 = 0, decoded_non_snoop_ltr = 0;
+ union ltr_payload ltr_data;
+ u32 scale = 0;
int index;
for (index = 0; map[index].name ; index++) {
- seq_printf(s, "%-32s\tRAW LTR: 0x%x\n",
+ ltr_data.raw_data = pmc_core_reg_read(pmcdev,
+ map[index].bit_mask);
+
+ if (ltr_data.bits.non_snoop_req) {
+ scale = ltr_data.bits.non_snoop_scale;
+ get_ltr_scale(&scale);
+ decoded_non_snoop_ltr =
+ ltr_data.bits.non_snoop_val * scale;
+ }
+
+ if (ltr_data.bits.snoop_req) {
+ scale = ltr_data.bits.snoop_scale;
+ get_ltr_scale(&scale);
+ decoded_snoop_ltr =
+ ltr_data.bits.snoop_val * scale;
+ }
+
+ seq_printf(s, "%-24s\tRaw LTR: 0x%-16x\t Non-Snoop LTR (ns): %-16llu\t Snoop LTR (ns): %-16llu\n",
map[index].name,
- pmc_core_reg_read(pmcdev, map[index].bit_mask));
+ ltr_data.raw_data,
+ decoded_non_snoop_ltr,
+ decoded_snoop_ltr);
+
+ decoded_snoop_ltr = decoded_non_snoop_ltr = 0;
}
return 0;
}
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 12663c58f122..bbf9a2790548 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -243,4 +243,18 @@ struct pmc_dev {
struct mutex lock; /* generic mutex lock for PMC Core */
};
+union ltr_payload {
+ u32 raw_data;
+ struct {
+ u32 snoop_val : 10;
+ u32 snoop_scale : 3;
+ u32 snoop_res : 2;
+ u32 snoop_req : 1;
+ u32 non_snoop_val : 10;
+ u32 non_snoop_scale : 3;
+ u32 non_snoop_res : 2;
+ u32 non_snoop_req : 1;
+ } bits;
+};
+
#endif /* PMC_CORE_H */
--
2.17.1
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.
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 <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Souvik Kumar Chakravarty <[email protected]>
Cc: Kuppuswamy Sathyanarayanan <[email protected]>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198779
Acked-by: Matt Turner <[email protected]>
Signed-off-by: Rajneesh Bhardwaj <[email protected]>
---
drivers/platform/x86/intel_telemetry_debugfs.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c b/drivers/platform/x86/intel_telemetry_debugfs.c
index ffd0474b0531..77212e9b22d6 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)
- return -ENODEV;
+ if (err < 0) {
+ pr_debug("Invalid pltconfig, ensure IPC1 device is enabled in BIOS\n");
+ goto exit;
+ }
err = telemetry_debugfs_check_evts();
- if (err < 0)
- return -EINVAL;
+ if (err < 0) {
+ pr_debug("telemetry_debugfs_check_evts failed\n");
+ goto exit;
+ }
register_pm_notifier(&pm_notifier);
@@ -1020,6 +1024,8 @@ static int __init telemetry_debugfs_init(void)
debugfs_conf->telemetry_dbg_dir = NULL;
out_pm:
unregister_pm_notifier(&pm_notifier);
+exit:
+ pr_debug(pr_fmt(DRIVER_NAME) " Failed\n");
return err;
}
--
2.17.1
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 <[email protected]>
---
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 972735bd4c75..c1330a03523d 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -149,6 +149,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 */
@@ -320,6 +321,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)
@@ -566,7 +568,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 b24407048fa1..12663c58f122 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
#define SPT_PMC_LTR_SPA 0x360
@@ -153,6 +153,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
@@ -215,6 +216,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
On Mon, Sep 3, 2018 at 9:06 PM Rajneesh Bhardwaj
<[email protected]> 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.
Thanks for the patch, my comments below.
> +static const struct pmc_bit_map spt_ltr_show_map[] = {
> + {"IP 0 : LTR_SOUTHPORT_A", SPT_PMC_LTR_SPA},
> + {"IP 1 : LTR_SOUTHPORT_B", SPT_PMC_LTR_SPB},
> + {"IP 2 : LTR_SATA", SPT_PMC_LTR_SATA},
> + {"IP 3 : LTR_GIGABIT_ETHERNET", SPT_PMC_LTR_GBE},
> + {"IP 4 : LTR_XHCI", SPT_PMC_LTR_XHCI},
> + /* IP 5 is reserved */
> + {"IP 6 : LTR_ME", SPT_PMC_LTR_ME},
> + /* EVA is Enterprise Value Add, doesn't really exist on PCH */
> + {"IP 7 : LTR_EVA", SPT_PMC_LTR_EVA},
> + {"IP 8 : LTR_SOUTHPORT_C", SPT_PMC_LTR_SPC},
> + {"IP 9 : LTR_HD_AUDIO", SPT_PMC_LTR_AZ},
> + /* IP 10 is reserved */
> + {"IP 11 : LTR_LPSS", SPT_PMC_LTR_LPSS},
> + {"IP 12 : LTR_SOUTHPORT_D", SPT_PMC_LTR_SPD},
> + {"IP 13 : LTR_SOUTHPORT_E", SPT_PMC_LTR_SPE},
> + {"IP 14 : LTR_CAMERA", SPT_PMC_LTR_CAM},
> + {"IP 15 : LTR_ESPI", SPT_PMC_LTR_ESPI},
> + {"IP 16 : LTR_SCC", SPT_PMC_LTR_SCC},
> + {"IP 17 : LTR_ISH", SPT_PMC_LTR_ISH},
> + /* Below two cannot be for LTR_IGNORE */
> + {"LTR_CURRENT_PLATFORM", SPT_PMC_LTR_CUR_PLT},
> + {"LTR_AGGREGATED_SYSTEM", SPT_PMC_LTR_CUR_ASLT},
Before no map has this fancy "IP xx :" prefixes. Please, remove.
> + {},
No need for comma
> +
Redundant.
> +};
> +static const struct pmc_bit_map cnp_ltr_show_map[] = {
Same comments as above.
> +};
> + debugfs_create_file("ltr_show", 0644, dir, pmcdev,
> + &pmc_core_ltr_fops);
One line?
> #define NUM_RETRIES 100
> #define NUM_IP_IGN_ALLOWED 17
+ blank line here.
> +#define SPT_PMC_LTR_CUR_PLT 0x350
--
With Best Regards,
Andy Shevchenko
On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj
<[email protected]> 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.
> +static void get_ltr_scale(u32 *val)
What's wrong to return converted value? Actually the name should
reflect what it does, ie *convert* value.
> +{
> + /*
> + * As per PCIE specification supprting document
supporting
> + * 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) {
> + *val = 0;
> + pr_warn("Invalid LTR scale factor.\n");
> + } else {
> + *val = 1U << (5 * (*val));
> + }
> +}
> +
> 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 = 0, decoded_non_snoop_ltr = 0;
> + union ltr_payload ltr_data;
> + u32 scale = 0;
Redundant assignment.
> int index;
>
> for (index = 0; map[index].name ; index++) {
> - seq_printf(s, "%-32s\tRAW LTR: 0x%x\n",
> + ltr_data.raw_data = pmc_core_reg_read(pmcdev,
> + map[index].bit_mask);
> +
> + if (ltr_data.bits.non_snoop_req) {
> + scale = ltr_data.bits.non_snoop_scale;
> + get_ltr_scale(&scale);
> + decoded_non_snoop_ltr =
> + ltr_data.bits.non_snoop_val * scale;
> + }
> +
> + if (ltr_data.bits.snoop_req) {
> + scale = ltr_data.bits.snoop_scale;
> + get_ltr_scale(&scale);
> + decoded_snoop_ltr =
> + ltr_data.bits.snoop_val * scale;
> + }
> +
> + seq_printf(s, "%-24s\tRaw LTR: 0x%-16x\t Non-Snoop LTR (ns): %-16llu\t Snoop LTR (ns): %-16llu\n",
> map[index].name,
> - pmc_core_reg_read(pmcdev, map[index].bit_mask));
> + ltr_data.raw_data,
> + decoded_non_snoop_ltr,
> + decoded_snoop_ltr);
> +
> + decoded_snoop_ltr = decoded_non_snoop_ltr = 0;
You may do this at the beginning of the loop and get rid of assignment
in the definition block.
> }
> return 0;
> }
> +union ltr_payload {
> + u32 raw_data;
> + struct {
> + u32 snoop_val : 10;
> + u32 snoop_scale : 3;
> + u32 snoop_res : 2;
> + u32 snoop_req : 1;
> + u32 non_snoop_val : 10;
> + u32 non_snoop_scale : 3;
> + u32 non_snoop_res : 2;
> + u32 non_snoop_req : 1;
> + } bits;
> +};
Just use normal masks and shifts.
--
With Best Regards,
Andy Shevchenko
On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj
<[email protected]> 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
IOSS
PSS
> not be obtained and result in a invalid telemetry_plt_config.
What is telemetry_plt_config?
> 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 <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Souvik Kumar Chakravarty <[email protected]>
> Cc: Kuppuswamy Sathyanarayanan <[email protected]>
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198779
>
There should be not a blank line.
> Acked-by: Matt Turner <[email protected]>
> Signed-off-by: Rajneesh Bhardwaj <[email protected]>
> +exit:
> + pr_debug(pr_fmt(DRIVER_NAME) " Failed\n");
Completely useless.
Device core does it in generic way.
--
With Best Regards,
Andy Shevchenko
On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj
<[email protected]> wrote:
>
> Cannonlake PCH allows us to ignore LTR from more IPs than Sunrisepoint
> PCH so make the LTR ignore platform specific.
>
This looks fine to me.
> Signed-off-by: Rajneesh Bhardwaj <[email protected]>
> ---
> 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 972735bd4c75..c1330a03523d 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -149,6 +149,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 */
> @@ -320,6 +321,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)
> @@ -566,7 +568,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 b24407048fa1..12663c58f122 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
> #define SPT_PMC_LTR_SPA 0x360
> @@ -153,6 +153,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
> @@ -215,6 +216,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
On 26-Sep-18 7:18 PM, Andy Shevchenko wrote:
> On Mon, Sep 3, 2018 at 9:06 PM Rajneesh Bhardwaj
> <[email protected]> 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.
> Thanks for the patch, my comments below.
Hi Andy,
Thanks for the review, my answers below.
>
>> +static const struct pmc_bit_map spt_ltr_show_map[] = {
>> + {"IP 0 : LTR_SOUTHPORT_A", SPT_PMC_LTR_SPA},
>> + {"IP 1 : LTR_SOUTHPORT_B", SPT_PMC_LTR_SPB},
>> + {"IP 2 : LTR_SATA", SPT_PMC_LTR_SATA},
>> + {"IP 3 : LTR_GIGABIT_ETHERNET", SPT_PMC_LTR_GBE},
>> + {"IP 4 : LTR_XHCI", SPT_PMC_LTR_XHCI},
>> + /* IP 5 is reserved */
>> + {"IP 6 : LTR_ME", SPT_PMC_LTR_ME},
>> + /* EVA is Enterprise Value Add, doesn't really exist on PCH */
>> + {"IP 7 : LTR_EVA", SPT_PMC_LTR_EVA},
>> + {"IP 8 : LTR_SOUTHPORT_C", SPT_PMC_LTR_SPC},
>> + {"IP 9 : LTR_HD_AUDIO", SPT_PMC_LTR_AZ},
>> + /* IP 10 is reserved */
>> + {"IP 11 : LTR_LPSS", SPT_PMC_LTR_LPSS},
>> + {"IP 12 : LTR_SOUTHPORT_D", SPT_PMC_LTR_SPD},
>> + {"IP 13 : LTR_SOUTHPORT_E", SPT_PMC_LTR_SPE},
>> + {"IP 14 : LTR_CAMERA", SPT_PMC_LTR_CAM},
>> + {"IP 15 : LTR_ESPI", SPT_PMC_LTR_ESPI},
>> + {"IP 16 : LTR_SCC", SPT_PMC_LTR_SCC},
>> + {"IP 17 : LTR_ISH", SPT_PMC_LTR_ISH},
>> + /* Below two cannot be for LTR_IGNORE */
>> + {"LTR_CURRENT_PLATFORM", SPT_PMC_LTR_CUR_PLT},
>> + {"LTR_AGGREGATED_SYSTEM", SPT_PMC_LTR_CUR_ASLT},
> Before no map has this fancy "IP xx :" prefixes. Please, remove.
The users of the driver often ask for IP Numbers while performing
LTR_IGNORE operation so this is deliberately added. Please consider it.
>
>> + {},
> No need for comma
Ok.
>> +
> Redundant.
OK
>
>> +};
>> +static const struct pmc_bit_map cnp_ltr_show_map[] = {
> Same comments as above.
>
>> +};
>> + debugfs_create_file("ltr_show", 0644, dir, pmcdev,
>> + &pmc_core_ltr_fops);
> One line?
IIRC, it was crossing the limit. I will check again and if possible
would change it.
>
>> #define NUM_RETRIES 100
>> #define NUM_IP_IGN_ALLOWED 17
> + blank line here.
Sure.
>
>> +#define SPT_PMC_LTR_CUR_PLT 0x350
On 26-Sep-18 7:23 PM, Andy Shevchenko wrote:
> On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj
> <[email protected]> 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.
>> +static void get_ltr_scale(u32 *val)
> What's wrong to return converted value? Actually the name should
> reflect what it does, ie *convert* value.
I can change it as per your suggestion.
>
>> +{
>> + /*
>> + * As per PCIE specification supprting document
> supporting
oops. Will fix.
>
>> + * 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) {
>> + *val = 0;
>> + pr_warn("Invalid LTR scale factor.\n");
>> + } else {
>> + *val = 1U << (5 * (*val));
>> + }
>> +}
>> +
>> 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 = 0, decoded_non_snoop_ltr = 0;
>> + union ltr_payload ltr_data;
>> + u32 scale = 0;
> Redundant assignment.
Ok
>
>> int index;
>>
>> for (index = 0; map[index].name ; index++) {
>> - seq_printf(s, "%-32s\tRAW LTR: 0x%x\n",
>> + ltr_data.raw_data = pmc_core_reg_read(pmcdev,
>> + map[index].bit_mask);
>> +
>> + if (ltr_data.bits.non_snoop_req) {
>> + scale = ltr_data.bits.non_snoop_scale;
>> + get_ltr_scale(&scale);
>> + decoded_non_snoop_ltr =
>> + ltr_data.bits.non_snoop_val * scale;
>> + }
>> +
>> + if (ltr_data.bits.snoop_req) {
>> + scale = ltr_data.bits.snoop_scale;
>> + get_ltr_scale(&scale);
>> + decoded_snoop_ltr =
>> + ltr_data.bits.snoop_val * scale;
>> + }
>> +
>> + seq_printf(s, "%-24s\tRaw LTR: 0x%-16x\t Non-Snoop LTR (ns): %-16llu\t Snoop LTR (ns): %-16llu\n",
>> map[index].name,
>> - pmc_core_reg_read(pmcdev, map[index].bit_mask));
>> + ltr_data.raw_data,
>> + decoded_non_snoop_ltr,
>> + decoded_snoop_ltr);
>> +
>> + decoded_snoop_ltr = decoded_non_snoop_ltr = 0;
> You may do this at the beginning of the loop and get rid of assignment
> in the definition block.
Fine.
>
>> }
>> return 0;
>> }
>> +union ltr_payload {
>> + u32 raw_data;
>> + struct {
>> + u32 snoop_val : 10;
>> + u32 snoop_scale : 3;
>> + u32 snoop_res : 2;
>> + u32 snoop_req : 1;
>> + u32 non_snoop_val : 10;
>> + u32 non_snoop_scale : 3;
>> + u32 non_snoop_res : 2;
>> + u32 non_snoop_req : 1;
>> + } bits;
>> +};
> Just use normal masks and shifts.
I chose union over masks and shifts to reduce code size and ensured
correct endian-ness. Just for my understanding, can you please let me
know why you feel masks/shift are better suited here?
>
On 26-Sep-18 7:26 PM, Andy Shevchenko wrote:
> On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj
> <[email protected]> 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
> IOSS
> PSS
Fine.
>
>> not be obtained and result in a invalid telemetry_plt_config.
> What is telemetry_plt_config?
Internal data structure that holds platform config, 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 <[email protected]>
>> Cc: Len Brown <[email protected]>
>> Cc: Souvik Kumar Chakravarty <[email protected]>
>> Cc: Kuppuswamy Sathyanarayanan <[email protected]>
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198779
>>
> There should be not a blank line.
OK.
>
>> Acked-by: Matt Turner <[email protected]>
>> Signed-off-by: Rajneesh Bhardwaj <[email protected]>
>> +exit:
>> + pr_debug(pr_fmt(DRIVER_NAME) " Failed\n");
> Completely useless.
>
> Device core does it in generic way.
If i remove this print then perhaps there is no need of this patch.
Reason to print this is that the platform driver / core driver does not
show any error. In-fact they are even loaded in module table. OTOH, this
debugfs interface fails. This is very confusing to the users if they
check the lsmod output so i feel this print might help.
>
Thank you.
On 26-Sep-18 7:27 PM, Andy Shevchenko wrote:
> On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj
> <[email protected]> wrote:
>> Cannonlake PCH allows us to ignore LTR from more IPs than Sunrisepoint
>> PCH so make the LTR ignore platform specific.
>>
> This looks fine to me.
>
>> Signed-off-by: Rajneesh Bhardwaj <[email protected]>
>> ---
>> 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 972735bd4c75..c1330a03523d 100644
>> --- a/drivers/platform/x86/intel_pmc_core.c
>> +++ b/drivers/platform/x86/intel_pmc_core.c
>> @@ -149,6 +149,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 */
>> @@ -320,6 +321,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)
>> @@ -566,7 +568,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 b24407048fa1..12663c58f122 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
>> #define SPT_PMC_LTR_SPA 0x360
>> @@ -153,6 +153,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
>> @@ -215,6 +216,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
>>
>
On Wed, Sep 26, 2018 at 5:15 PM Bhardwaj, Rajneesh
<[email protected]> wrote:
> On 26-Sep-18 7:18 PM, Andy Shevchenko wrote:
> > On Mon, Sep 3, 2018 at 9:06 PM Rajneesh Bhardwaj
> > <[email protected]> wrote:
> >> + {"IP 16 : LTR_SCC", SPT_PMC_LTR_SCC},
> >> + {"IP 17 : LTR_ISH", SPT_PMC_LTR_ISH},
> > Before no map has this fancy "IP xx :" prefixes. Please, remove.
> The users of the driver often ask for IP Numbers while performing
> LTR_IGNORE operation so this is deliberately added. Please consider it.
You may enumerate during printing.
--
With Best Regards,
Andy Shevchenko
On Wed, Sep 26, 2018 at 5:24 PM Bhardwaj, Rajneesh
<[email protected]> wrote:
> On 26-Sep-18 7:26 PM, Andy Shevchenko wrote:
> > On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj
> > <[email protected]> wrote:
> >> not be obtained and result in a invalid telemetry_plt_config.
> > What is telemetry_plt_config?
>
> Internal data structure that holds platform config, maintained by the
> telemetry platform driver.
You need to spell if for the reader.
> >> 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.
> >> +exit:
> >> + pr_debug(pr_fmt(DRIVER_NAME) " Failed\n");
> > Completely useless.
> >
> > Device core does it in generic way.
>
> If i remove this print then perhaps there is no need of this patch.
Maybe.
> Reason to print this is that the platform driver / core driver does not
> show any error.
If the code fails and returns 0 — it's a bug in error reporting inside the code.
> In-fact they are even loaded in module table. OTOH, this
> debugfs interface fails. This is very confusing to the users if they
> check the lsmod output so i feel this print might help.
Again, device core *already has* this and even more (it prints also a
return code!).
--
With Best Regards,
Andy Shevchenko
On Wed, Sep 26, 2018 at 5:19 PM Bhardwaj, Rajneesh
<[email protected]> wrote:
> On 26-Sep-18 7:23 PM, Andy Shevchenko wrote:
> > On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj
> > <[email protected]> wrote:
> >> +static void get_ltr_scale(u32 *val)
> > What's wrong to return converted value? Actually the name should
> > reflect what it does, ie *convert* value.
>
> I can change it as per your suggestion.
Please do.
> >> +union ltr_payload {
> >> + u32 raw_data;
> >> + struct {
> >> + u32 snoop_val : 10;
> >> + u32 snoop_scale : 3;
> >> + u32 snoop_res : 2;
> >> + u32 snoop_req : 1;
> >> + u32 non_snoop_val : 10;
> >> + u32 non_snoop_scale : 3;
> >> + u32 non_snoop_res : 2;
> >> + u32 non_snoop_req : 1;
> >> + } bits;
> >> +};
> > Just use normal masks and shifts.
>
> I chose union over masks and shifts to reduce code size and ensured
> correct endian-ness.
How do you ensure endianess in union if you do nothing to it here? It
just would reflect CPU endianess.
> Just for my understanding, can you please let me
> know why you feel masks/shift are better suited here?
First of all, in the very same driver shifts and masks / standalone
bits are already in use.
Like you mentioned an endianess, it would make it more clear here,
though it's still require to get a value in a proper one in the first
place.
On top of that, a compiler which might generate an awful code out of
bits defined as above.
Btw, there are helpers for that like those in bitfield.h.
--
With Best Regards,
Andy Shevchenko
On Fri, Sep 28, 2018 at 02:44:06PM +0530, Bhardwaj, Rajneesh wrote:
Resending it since previous message had few HTML contents so it was not
delivered to the list.
>
>
> On 26-Sep-18 10:48 PM, Andy Shevchenko wrote:
> >On Wed, Sep 26, 2018 at 5:24 PM Bhardwaj, Rajneesh
> ><[email protected]> wrote:
> >>On 26-Sep-18 7:26 PM, Andy Shevchenko wrote:
> >>>On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj
> >>><[email protected]> wrote:
> >>>>not be obtained and result in a invalid telemetry_plt_config.
> >>>What is telemetry_plt_config?
> >>Internal data structure that holds platform config, maintained by the
> >>telemetry platform driver.
> >You need to spell if for the reader.
>
> Sure, thanks for the suggestion. I will do it if you agree to my explanation
> below and require v2.
>
> >
> >>>>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.
> >>>>+exit:
> >>>>+ pr_debug(pr_fmt(DRIVER_NAME) " Failed\n");
> >>>Completely useless.
> >>>
> >>>Device core does it in generic way.
> >>If i remove this print then perhaps there is no need of this patch.
> >Maybe.
> >
> >
> >
> >>Reason to print this is that the platform driver / core driver does not
> >>show any error.
> >If the code fails and returns 0 — it's a bug in error reporting inside the code.
>
Below are my comments as per previous mail.
> The existing Telemetry ecosystem for Atom consists of IPC driver, telem core
> driver and telem platform driver but there are no consumers of the APIs
> except the ones in telem debugfs driver. Here in this case, not all drivers
> that make up the telemetry infra return failure. Here is how the system
> looks w/wo IPC1 setting in the BIOS.
>
> When IPC is present
>
> root@raj-glk:~# lsmod | grep -i telem
>
> intel_telemetry_debugfs245760
>
> intel_telemetry_pltdrv204800
>
> intel_punit_ipc163841 intel_telemetry_pltdrv
>
> intel_telemetry_core163842 intel_telemetry_debugfs,intel_telemetry_pltdrv
>
> intel_pmc_ipc204802 intel_telemetry_debugfs,intel_telemetry_pltdrv
>
> When IPC is missing
>
> root@raj-glk:~# lsmod | grep -i telem
>
> intel_telemetry_pltdrv204800
>
> intel_punit_ipc163841 intel_telemetry_pltdrv
>
> intel_telemetry_core163841 intel_telemetry_pltdrv
>
> intel_pmc_ipc204801 intel_telemetry_pltdrv
>
>
> If we look at the dmesg log, we see "intel_telemetry_core Init". So one
> might think that the driver has loaded fine since user may not know that
> telemetry is more than just one driver. Such things are often reported by
> users of this driver.
> So IMHO, keeping this error helps user triage the problem easily. I can
> change to pr_info you'd like it.
>
> >
> >>In-fact they are even loaded in module table. OTOH, this
> >>debugfs interface fails. This is very confusing to the users if they
> >>check the lsmod output so i feel this print might help.
> >Again, device core *already has* this and even more (it prints also a
> >return code!).
> >
>
--
Best Regards,
Rajneesh