2021-04-17 03:14:06

by David E. Box

Subject: [PATCH V2 0/9] intel_pmc_core: Add sub-state requirements and mode

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

Applied on top of latest hans-review/review-hans

Patches that changed in V2:
Patch 3: Variable name change
Patch 5: Do proper cleanup after fail
Patch 7: Debugfs write function fixes

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

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

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


base-commit: 823b31517ad3196324322804ee365d5fcff704d6
--
2.25.1

2021-04-17 03:14:09

by David E. Box

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

From: Gayatri Kammela <[email protected]>

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

Signed-off-by: Gayatri Kammela <[email protected]>
Signed-off-by: David E. Box <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Reviewed-by: Rajneesh Bhardwaj <[email protected]>
---

V2: No change

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

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index c02f63c00ecc..0e59a84b51bf 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -580,6 +580,7 @@ static const struct pmc_reg_map tgl_reg_map = {
.pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
.ltr_ignore_max = TGL_NUM_IP_IGN_ALLOWED,
.lpm_num_maps = TGL_LPM_NUM_MAPS,
+ .lpm_res_counter_step_x2 = TGL_PMC_LPM_RES_COUNTER_STEP_X2,
.lpm_en_offset = TGL_LPM_EN_OFFSET,
.lpm_priority_offset = TGL_LPM_PRI_OFFSET,
.lpm_residency_offset = TGL_LPM_RESIDENCY_OFFSET,
@@ -1138,17 +1139,26 @@ static int pmc_core_ltr_show(struct seq_file *s, void *unused)
}
DEFINE_SHOW_ATTRIBUTE(pmc_core_ltr);

+static inline u64 adjust_lpm_residency(struct pmc_dev *pmcdev, u32 offset,
+ const int lpm_adj_x2)
+{
+ u64 lpm_res = pmc_core_reg_read(pmcdev, offset);
+
+ return GET_X2_COUNTER((u64)lpm_adj_x2 * lpm_res);
+}
+
static int pmc_core_substate_res_show(struct seq_file *s, void *unused)
{
struct pmc_dev *pmcdev = s->private;
+ const int lpm_adj_x2 = pmcdev->map->lpm_res_counter_step_x2;
u32 offset = pmcdev->map->lpm_residency_offset;
int i, mode;

seq_printf(s, "%-10s %-15s\n", "Substate", "Residency");

pmc_for_each_mode(i, mode, pmcdev) {
- seq_printf(s, "%-10s %-15u\n", pmc_lpm_modes[mode],
- pmc_core_reg_read(pmcdev, offset + (4 * mode)));
+ seq_printf(s, "%-10s %-15llu\n", pmc_lpm_modes[mode],
+ adjust_lpm_residency(pmcdev, offset + (4 * mode), lpm_adj_x2));
}

return 0;
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 2ffe0eba36e1..aa44fd5399cc 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -188,9 +188,11 @@ enum ppfear_regs {
#define ICL_PMC_SLP_S0_RES_COUNTER_STEP 0x64

#define LPM_MAX_NUM_MODES 8
+#define GET_X2_COUNTER(v) ((v) >> 1)

#define TGL_NUM_IP_IGN_ALLOWED 22
#define TGL_PMC_SLP_S0_RES_COUNTER_STEP 0x7A
+#define TGL_PMC_LPM_RES_COUNTER_STEP_X2 61 /* 30.5us * 2 */

/*
* Tigerlake Power Management Controller register offsets
@@ -268,6 +270,7 @@ struct pmc_reg_map {
const u32 pm_vric1_offset;
/* Low Power Mode registers */
const int lpm_num_maps;
+ const int lpm_res_counter_step_x2;
const u32 lpm_en_offset;
const u32 lpm_priority_offset;
const u32 lpm_residency_offset;
--
2.25.1

2021-04-17 03:14:21

by David E. Box

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

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

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

The new file, latch_lpm_mode, looks like this:

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

Signed-off-by: David E. Box <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
---

V2: - Rebase on Tamar/Tomas global reset patch that already adds
Extended Test Register 3
- In write function, make sure count is 1 less than buffer to reserve
space for '\0'
- Use sysfs_streq to properly compare the input string

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

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 684f13f0c4a5..97cf3384c4c0 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -586,6 +586,7 @@ static const struct pmc_reg_map tgl_reg_map = {
.ltr_ignore_max = TGL_NUM_IP_IGN_ALLOWED,
.lpm_num_maps = TGL_LPM_NUM_MAPS,
.lpm_res_counter_step_x2 = TGL_PMC_LPM_RES_COUNTER_STEP_X2,
+ .lpm_sts_latch_en_offset = TGL_LPM_STS_LATCH_EN_OFFSET,
.lpm_en_offset = TGL_LPM_EN_OFFSET,
.lpm_priority_offset = TGL_LPM_PRI_OFFSET,
.lpm_residency_offset = TGL_LPM_RESIDENCY_OFFSET,
@@ -1321,6 +1322,114 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
}
DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_req_regs);

+static int pmc_core_lpm_latch_mode_show(struct seq_file *s, void *unused)
+{
+ struct pmc_dev *pmcdev = s->private;
+ bool c10;
+ u32 reg;
+ int idx, mode;
+
+ reg = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_sts_latch_en_offset);
+ if (reg & LPM_STS_LATCH_MODE) {
+ seq_puts(s, "c10");
+ c10 = false;
+ } else {
+ seq_puts(s, "[c10]");
+ c10 = true;
+ }
+
+ pmc_for_each_mode(idx, mode, pmcdev) {
+ if ((BIT(mode) & reg) && !c10)
+ seq_printf(s, " [%s]", pmc_lpm_modes[mode]);
+ else
+ seq_printf(s, " %s", pmc_lpm_modes[mode]);
+ }
+
+ seq_puts(s, " clear\n");
+
+ return 0;
+}
+
+static ssize_t pmc_core_lpm_latch_mode_write(struct file *file,
+ const char __user *userbuf,
+ size_t count, loff_t *ppos)
+{
+ struct seq_file *s = file->private_data;
+ struct pmc_dev *pmcdev = s->private;
+ bool clear = false, c10 = false;
+ unsigned char buf[8];
+ size_t ret;
+ int idx, m, mode;
+ u32 reg;
+
+ if (count > sizeof(buf) - 1)
+ return -EINVAL;
+
+ ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);
+ if (ret < 0)
+ return ret;
+
+ buf[count] = '\0';
+
+ /*
+ * Allowed strings are:
+ * Any enabled substate, e.g. 'S0i2.0'
+ * 'c10'
+ * 'clear'
+ */
+ mode = sysfs_match_string(pmc_lpm_modes, buf);
+
+ /* Check string matches enabled mode */
+ pmc_for_each_mode(idx, m, pmcdev)
+ if (mode == m)
+ break;
+
+ if (mode != m || mode < 0) {
+ if (sysfs_streq(buf, "clear"))
+ clear = true;
+ else if (sysfs_streq(buf, "c10"))
+ c10 = true;
+ else
+ return -EINVAL;
+ }
+
+ if (clear) {
+ mutex_lock(&pmcdev->lock);
+
+ reg = pmc_core_reg_read(pmcdev, pmcdev->map->etr3_offset);
+ reg |= ETR3_CLEAR_LPM_EVENTS;
+ pmc_core_reg_write(pmcdev, pmcdev->map->etr3_offset, reg);
+
+ mutex_unlock(&pmcdev->lock);
+
+ return count;
+ }
+
+ if (c10) {
+ mutex_lock(&pmcdev->lock);
+
+ reg = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_sts_latch_en_offset);
+ reg &= ~LPM_STS_LATCH_MODE;
+ pmc_core_reg_write(pmcdev, pmcdev->map->lpm_sts_latch_en_offset, reg);
+
+ mutex_unlock(&pmcdev->lock);
+
+ return count;
+ }
+
+ /*
+ * For LPM mode latching we set the latch enable bit and selected mode
+ * and clear everything else.
+ */
+ reg = LPM_STS_LATCH_MODE | BIT(mode);
+ mutex_lock(&pmcdev->lock);
+ pmc_core_reg_write(pmcdev, pmcdev->map->lpm_sts_latch_en_offset, reg);
+ mutex_unlock(&pmcdev->lock);
+
+ return count;
+}
+DEFINE_PMC_CORE_ATTR_WRITE(pmc_core_lpm_latch_mode);
+
static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
{
struct pmc_dev *pmcdev = s->private;
@@ -1439,6 +1548,9 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
debugfs_create_file("substate_live_status_registers", 0444,
pmcdev->dbgfs_dir, pmcdev,
&pmc_core_substate_l_sts_regs_fops);
+ debugfs_create_file("lpm_latch_mode", 0644,
+ pmcdev->dbgfs_dir, pmcdev,
+ &pmc_core_lpm_latch_mode_fops);
}

if (pmcdev->lpm_req_regs) {
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 64fb368f40f6..c45805671c4a 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -189,6 +189,7 @@ enum ppfear_regs {

#define LPM_MAX_NUM_MODES 8
#define GET_X2_COUNTER(v) ((v) >> 1)
+#define LPM_STS_LATCH_MODE BIT(31)

#define TGL_NUM_IP_IGN_ALLOWED 22
#define TGL_PMC_SLP_S0_RES_COUNTER_STEP 0x7A
@@ -197,6 +198,7 @@ enum ppfear_regs {
/*
* Tigerlake Power Management Controller register offsets
*/
+#define TGL_LPM_STS_LATCH_EN_OFFSET 0x1C34
#define TGL_LPM_EN_OFFSET 0x1C78
#define TGL_LPM_RESIDENCY_OFFSET 0x1C80

@@ -211,6 +213,9 @@ enum ppfear_regs {
#define ETR3_CF9GR BIT(20)
#define ETR3_CF9LOCK BIT(31)

+/* Extended Test Mode Register LPM bits (TGL and later */
+#define ETR3_CLEAR_LPM_EVENTS BIT(28)
+
const char *pmc_lpm_modes[] = {
"S0i2.0",
"S0i2.1",
@@ -271,6 +276,7 @@ struct pmc_reg_map {
/* Low Power Mode registers */
const int lpm_num_maps;
const int lpm_res_counter_step_x2;
+ const u32 lpm_sts_latch_en_offset;
const u32 lpm_en_offset;
const u32 lpm_priority_offset;
const u32 lpm_residency_offset;
@@ -319,4 +325,18 @@ struct pmc_dev {
i < pmcdev->num_lpm_modes; \
i++, mode = pmcdev->lpm_en_modes[i])

+#define DEFINE_PMC_CORE_ATTR_WRITE(__name) \
+static int __name ## _open(struct inode *inode, struct file *file) \
+{ \
+ return single_open(file, __name ## _show, inode->i_private); \
+} \
+ \
+static const struct file_operations __name ## _fops = { \
+ .owner = THIS_MODULE, \
+ .open = __name ## _open, \
+ .read = seq_read, \
+ .write = __name ## _write, \
+ .release = single_release, \
+}
+
#endif /* PMC_CORE_H */
--
2.25.1

2021-04-17 03:14:21

by David E. Box

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

From: Gayatri Kammela <[email protected]>

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

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

Signed-off-by: Gayatri Kammela <[email protected]>
Signed-off-by: David E. Box <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Acked-by: Rajneesh Bhardwaj <[email protected]>
---

V2: No change

drivers/platform/x86/intel_pmc_core.c | 2 ++
drivers/platform/x86/intel_pmc_core.h | 4 +++-
2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 97cf3384c4c0..786b67171ddc 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -383,6 +383,8 @@ static const struct pmc_bit_map cnp_ltr_show_map[] = {
* a list of core SoCs using this.
*/
{"WIGIG", ICL_PMC_LTR_WIGIG},
+ {"THC0", TGL_PMC_LTR_THC0},
+ {"THC1", TGL_PMC_LTR_THC1},
/* Below two cannot be used for LTR_IGNORE */
{"CURRENT_PLATFORM", CNP_PMC_LTR_CUR_PLT},
{"AGGREGATED_SYSTEM", CNP_PMC_LTR_CUR_ASLT},
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index c45805671c4a..e8dae9c6c45f 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -191,8 +191,10 @@ enum ppfear_regs {
#define GET_X2_COUNTER(v) ((v) >> 1)
#define LPM_STS_LATCH_MODE BIT(31)

-#define TGL_NUM_IP_IGN_ALLOWED 22
#define TGL_PMC_SLP_S0_RES_COUNTER_STEP 0x7A
+#define TGL_PMC_LTR_THC0 0x1C04
+#define TGL_PMC_LTR_THC1 0x1C08
+#define TGL_NUM_IP_IGN_ALLOWED 23
#define TGL_PMC_LPM_RES_COUNTER_STEP_X2 61 /* 30.5us * 2 */

/*
--
2.25.1

2021-04-17 03:14:27

by David E. Box

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

Alder PCH-P is based on Tiger Lake PCH.

Signed-off-by: David E. Box <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Acked-by: Rajneesh Bhardwaj <[email protected]>
---

V2: No change

drivers/platform/x86/intel_pmc_core.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 786b67171ddc..900aa5e40a0f 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -1577,6 +1577,7 @@ static const struct x86_cpu_id intel_pmc_core_ids[] = {
X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT, &tgl_reg_map),
X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_L, &icl_reg_map),
X86_MATCH_INTEL_FAM6_MODEL(ROCKETLAKE, &tgl_reg_map),
+ X86_MATCH_INTEL_FAM6_MODEL(ALDERLAKE_L, &tgl_reg_map),
{}
};

--
2.25.1

2021-04-17 03:14:55

by David E. Box

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

From: Gayatri Kammela <[email protected]>

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

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

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

Suggested-by: David E. Box <[email protected]>
Signed-off-by: Gayatri Kammela <[email protected]>
Signed-off-by: David E. Box <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Acked-by: Rajneesh Bhardwaj <[email protected]>
---

V2: Renamed num_modes to num_lpm_modes as suggested by Rajneesh

drivers/platform/x86/intel_pmc_core.c | 59 ++++++++++++++++++++++-----
drivers/platform/x86/intel_pmc_core.h | 18 +++++++-
2 files changed, 64 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index e8474d171d23..c02f63c00ecc 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -579,8 +579,9 @@ static const struct pmc_reg_map tgl_reg_map = {
.pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
.pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
.ltr_ignore_max = TGL_NUM_IP_IGN_ALLOWED,
- .lpm_modes = tgl_lpm_modes,
+ .lpm_num_maps = TGL_LPM_NUM_MAPS,
.lpm_en_offset = TGL_LPM_EN_OFFSET,
+ .lpm_priority_offset = TGL_LPM_PRI_OFFSET,
.lpm_residency_offset = TGL_LPM_RESIDENCY_OFFSET,
.lpm_sts = tgl_lpm_maps,
.lpm_status_offset = TGL_LPM_STATUS_OFFSET,
@@ -1140,18 +1141,14 @@ DEFINE_SHOW_ATTRIBUTE(pmc_core_ltr);
static int pmc_core_substate_res_show(struct seq_file *s, void *unused)
{
struct pmc_dev *pmcdev = s->private;
- const char **lpm_modes = pmcdev->map->lpm_modes;
u32 offset = pmcdev->map->lpm_residency_offset;
- u32 lpm_en;
- int index;
+ int i, mode;

- lpm_en = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_en_offset);
- seq_printf(s, "status substate residency\n");
- for (index = 0; lpm_modes[index]; index++) {
- seq_printf(s, "%7s %7s %-15u\n",
- BIT(index) & lpm_en ? "Enabled" : " ",
- lpm_modes[index], pmc_core_reg_read(pmcdev, offset));
- offset += 4;
+ seq_printf(s, "%-10s %-15s\n", "Substate", "Residency");
+
+ pmc_for_each_mode(i, mode, pmcdev) {
+ seq_printf(s, "%-10s %-15u\n", pmc_lpm_modes[mode],
+ pmc_core_reg_read(pmcdev, offset + (4 * mode)));
}

return 0;
@@ -1203,6 +1200,45 @@ static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
}
DEFINE_SHOW_ATTRIBUTE(pmc_core_pkgc);

+static void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev)
+{
+ u8 lpm_priority[LPM_MAX_NUM_MODES];
+ u32 lpm_en;
+ int mode, i, p;
+
+ /* Use LPM Maps to indicate support for substates */
+ if (!pmcdev->map->lpm_num_maps)
+ return;
+
+ lpm_en = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_en_offset);
+ pmcdev->num_lpm_modes = hweight32(lpm_en);
+
+ /* Each byte contains information for 2 modes (7:4 and 3:0) */
+ for (mode = 0; mode < LPM_MAX_NUM_MODES; mode += 2) {
+ u8 priority = pmc_core_reg_read_byte(pmcdev,
+ pmcdev->map->lpm_priority_offset + (mode / 2));
+ int pri0 = GENMASK(3, 0) & priority;
+ int pri1 = (GENMASK(7, 4) & priority) >> 4;
+
+ lpm_priority[pri0] = mode;
+ lpm_priority[pri1] = mode + 1;
+ }
+
+ /*
+ * Loop though all modes from lowest to highest priority,
+ * and capture all enabled modes in order
+ */
+ i = 0;
+ for (p = LPM_MAX_NUM_MODES - 1; p >= 0; p--) {
+ int mode = lpm_priority[p];
+
+ if (!(BIT(mode) & lpm_en))
+ continue;
+
+ pmcdev->lpm_en_modes[i++] = mode;
+ }
+}
+
static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
{
debugfs_remove_recursive(pmcdev->dbgfs_dir);
@@ -1379,6 +1415,7 @@ static int pmc_core_probe(struct platform_device *pdev)

mutex_init(&pmcdev->lock);
pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
+ pmc_core_get_low_power_modes(pmcdev);
pmc_core_do_dmi_quirks(pmcdev);

/*
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index 98ebdfe57138..2ffe0eba36e1 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -187,6 +187,8 @@ enum ppfear_regs {
#define ICL_PMC_LTR_WIGIG 0x1BFC
#define ICL_PMC_SLP_S0_RES_COUNTER_STEP 0x64

+#define LPM_MAX_NUM_MODES 8
+
#define TGL_NUM_IP_IGN_ALLOWED 22
#define TGL_PMC_SLP_S0_RES_COUNTER_STEP 0x7A

@@ -199,13 +201,15 @@ enum ppfear_regs {
/* Tigerlake Low Power Mode debug registers */
#define TGL_LPM_STATUS_OFFSET 0x1C3C
#define TGL_LPM_LIVE_STATUS_OFFSET 0x1C5C
+#define TGL_LPM_PRI_OFFSET 0x1C7C
+#define TGL_LPM_NUM_MAPS 6

/* Extended Test Mode Register 3 (CNL and later) */
#define ETR3_OFFSET 0x1048
#define ETR3_CF9GR BIT(20)
#define ETR3_CF9LOCK BIT(31)

-const char *tgl_lpm_modes[] = {
+const char *pmc_lpm_modes[] = {
"S0i2.0",
"S0i2.1",
"S0i2.2",
@@ -263,8 +267,9 @@ struct pmc_reg_map {
const u32 ltr_ignore_max;
const u32 pm_vric1_offset;
/* Low Power Mode registers */
- const char **lpm_modes;
+ const int lpm_num_maps;
const u32 lpm_en_offset;
+ const u32 lpm_priority_offset;
const u32 lpm_residency_offset;
const u32 lpm_status_offset;
const u32 lpm_live_status_offset;
@@ -284,6 +289,8 @@ struct pmc_reg_map {
* @check_counters: On resume, check if counters are getting incremented
* @pc10_counter: PC10 residency counter
* @s0ix_counter: S0ix residency (step adjusted)
+ * @num_lpm_modes: Count of enabled modes
+ * @lpm_en_modes: Array of enabled modes from lowest to highest priority
*
* pmc_dev contains info about power management controller device.
*/
@@ -298,6 +305,13 @@ struct pmc_dev {
bool check_counters; /* Check for counter increments on resume */
u64 pc10_counter;
u64 s0ix_counter;
+ int num_lpm_modes;
+ int lpm_en_modes[LPM_MAX_NUM_MODES];
};

+#define pmc_for_each_mode(i, mode, pmcdev) \
+ for (i = 0, mode = pmcdev->lpm_en_modes[i]; \
+ i < pmcdev->num_lpm_modes; \
+ i++, mode = pmcdev->lpm_en_modes[i])
+
#endif /* PMC_CORE_H */
--
2.25.1

2021-04-17 03:15:14

by David E. Box

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

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

Signed-off-by: David E. Box <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Reviewed-by: Rajneesh Bhardwaj <[email protected]>
---

V2: No change

drivers/platform/x86/intel_pmc_core.c | 41 ++++++++++++++-------------
1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 07657532ccdb..e8474d171d23 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -31,8 +31,6 @@

#include "intel_pmc_core.h"

-static struct pmc_dev pmc;
-
/* PKGC MSRs are common across Intel Core SoCs */
static const struct pmc_bit_map msr_map[] = {
{"Package C2", MSR_PKG_C2_RESIDENCY},
@@ -729,9 +727,8 @@ static int pmc_core_dev_state_get(void *data, u64 *val)

DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get, NULL, "%llu\n");

-static int pmc_core_check_read_lock_bit(void)
+static int pmc_core_check_read_lock_bit(struct pmc_dev *pmcdev)
{
- struct pmc_dev *pmcdev = &pmc;
u32 value;

value = pmc_core_reg_read(pmcdev, pmcdev->map->pm_cfg_offset);
@@ -856,28 +853,26 @@ static int pmc_core_ppfear_show(struct seq_file *s, void *unused)
DEFINE_SHOW_ATTRIBUTE(pmc_core_ppfear);

/* This function should return link status, 0 means ready */
-static int pmc_core_mtpmc_link_status(void)
+static int pmc_core_mtpmc_link_status(struct pmc_dev *pmcdev)
{
- struct pmc_dev *pmcdev = &pmc;
u32 value;

value = pmc_core_reg_read(pmcdev, SPT_PMC_PM_STS_OFFSET);
return value & BIT(SPT_PMC_MSG_FULL_STS_BIT);
}

-static int pmc_core_send_msg(u32 *addr_xram)
+static int pmc_core_send_msg(struct pmc_dev *pmcdev, u32 *addr_xram)
{
- struct pmc_dev *pmcdev = &pmc;
u32 dest;
int timeout;

for (timeout = NUM_RETRIES; timeout > 0; timeout--) {
- if (pmc_core_mtpmc_link_status() == 0)
+ if (pmc_core_mtpmc_link_status(pmcdev) == 0)
break;
msleep(5);
}

- if (timeout <= 0 && pmc_core_mtpmc_link_status())
+ if (timeout <= 0 && pmc_core_mtpmc_link_status(pmcdev))
return -EBUSY;

dest = (*addr_xram & MTPMC_MASK) | (1U << 1);
@@ -903,7 +898,7 @@ static int pmc_core_mphy_pg_show(struct seq_file *s, void *unused)

mutex_lock(&pmcdev->lock);

- if (pmc_core_send_msg(&mphy_core_reg_low) != 0) {
+ if (pmc_core_send_msg(pmcdev, &mphy_core_reg_low) != 0) {
err = -EBUSY;
goto out_unlock;
}
@@ -911,7 +906,7 @@ static int pmc_core_mphy_pg_show(struct seq_file *s, void *unused)
msleep(10);
val_low = pmc_core_reg_read(pmcdev, SPT_PMC_MFPMC_OFFSET);

- if (pmc_core_send_msg(&mphy_core_reg_high) != 0) {
+ if (pmc_core_send_msg(pmcdev, &mphy_core_reg_high) != 0) {
err = -EBUSY;
goto out_unlock;
}
@@ -954,7 +949,7 @@ static int pmc_core_pll_show(struct seq_file *s, void *unused)
mphy_common_reg = (SPT_PMC_MPHY_COM_STS_0 << 16);
mutex_lock(&pmcdev->lock);

- if (pmc_core_send_msg(&mphy_common_reg) != 0) {
+ if (pmc_core_send_msg(pmcdev, &mphy_common_reg) != 0) {
err = -EBUSY;
goto out_unlock;
}
@@ -975,9 +970,8 @@ static int pmc_core_pll_show(struct seq_file *s, void *unused)
}
DEFINE_SHOW_ATTRIBUTE(pmc_core_pll);

-static int pmc_core_send_ltr_ignore(u32 value)
+static int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value)
{
- struct pmc_dev *pmcdev = &pmc;
const struct pmc_reg_map *map = pmcdev->map;
u32 reg;
int err = 0;
@@ -1003,6 +997,8 @@ static ssize_t pmc_core_ltr_ignore_write(struct file *file,
const char __user *userbuf,
size_t count, loff_t *ppos)
{
+ struct seq_file *s = file->private_data;
+ struct pmc_dev *pmcdev = s->private;
u32 buf_size, value;
int err;

@@ -1012,7 +1008,7 @@ static ssize_t pmc_core_ltr_ignore_write(struct file *file,
if (err)
return err;

- err = pmc_core_send_ltr_ignore(value);
+ err = pmc_core_send_ltr_ignore(pmcdev, value);

return err == 0 ? count : err;
}
@@ -1340,13 +1336,19 @@ static void pmc_core_do_dmi_quirks(struct pmc_dev *pmcdev)
static int pmc_core_probe(struct platform_device *pdev)
{
static bool device_initialized;
- struct pmc_dev *pmcdev = &pmc;
+ struct pmc_dev *pmcdev;
const struct x86_cpu_id *cpu_id;
u64 slp_s0_addr;

if (device_initialized)
return -ENODEV;

+ pmcdev = devm_kzalloc(&pdev->dev, sizeof(*pmcdev), GFP_KERNEL);
+ if (!pmcdev)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, pmcdev);
+
cpu_id = x86_match_cpu(intel_pmc_core_ids);
if (!cpu_id)
return -ENODEV;
@@ -1376,8 +1378,7 @@ static int pmc_core_probe(struct platform_device *pdev)
return -ENOMEM;

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

/*
@@ -1386,7 +1387,7 @@ static int pmc_core_probe(struct platform_device *pdev)
*/
if (pmcdev->map == &tgl_reg_map) {
dev_dbg(&pdev->dev, "ignoring GBE LTR\n");
- pmc_core_send_ltr_ignore(3);
+ pmc_core_send_ltr_ignore(pmcdev, 3);
}

pmc_core_dbgfs_register(pmcdev);
--
2.25.1

2021-04-17 03:15:25

by David E. Box

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

From: Gayatri Kammela <[email protected]>

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

Signed-off-by: Gayatri Kammela <[email protected]>
Co-developed-by: David E. Box <[email protected]>
Signed-off-by: David E. Box <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
---

V2: - Move buffer allocation so that it does not need to be freed
(which was missing anyway) when an error is encountered.
- Use label to free out_obj after errors
- Use memcpy instead of memcpy_fromio for ACPI memory

drivers/platform/x86/intel_pmc_core.c | 56 +++++++++++++++++++++++++++
drivers/platform/x86/intel_pmc_core.h | 2 +
2 files changed, 58 insertions(+)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 0e59a84b51bf..97efe9a6bd01 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -23,7 +23,9 @@
#include <linux/slab.h>
#include <linux/suspend.h>
#include <linux/uaccess.h>
+#include <linux/uuid.h>

+#include <acpi/acpi_bus.h>
#include <asm/cpu_device_id.h>
#include <asm/intel-family.h>
#include <asm/msr.h>
@@ -31,6 +33,9 @@

#include "intel_pmc_core.h"

+#define ACPI_S0IX_DSM_UUID "57a6512e-3979-4e9d-9708-ff13b2508972"
+#define ACPI_GET_LOW_MODE_REGISTERS 1
+
/* PKGC MSRs are common across Intel Core SoCs */
static const struct pmc_bit_map msr_map[] = {
{"Package C2", MSR_PKG_C2_RESIDENCY},
@@ -590,6 +595,53 @@ static const struct pmc_reg_map tgl_reg_map = {
.etr3_offset = ETR3_OFFSET,
};

+static void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev)
+{
+ struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
+ const int num_maps = pmcdev->map->lpm_num_maps;
+ size_t lpm_size = LPM_MAX_NUM_MODES * num_maps * 4;
+ union acpi_object *out_obj;
+ struct acpi_device *adev;
+ guid_t s0ix_dsm_guid;
+ u32 *lpm_req_regs, *addr;
+
+ adev = ACPI_COMPANION(&pdev->dev);
+ if (!adev)
+ return;
+
+ guid_parse(ACPI_S0IX_DSM_UUID, &s0ix_dsm_guid);
+
+ out_obj = acpi_evaluate_dsm(adev->handle, &s0ix_dsm_guid, 0,
+ ACPI_GET_LOW_MODE_REGISTERS, NULL);
+ if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
+ int size = out_obj->buffer.length;
+
+ if (size != lpm_size) {
+ acpi_handle_debug(adev->handle,
+ "_DSM returned unexpected buffer size,"
+ " have %d, expect %ld\n", size, lpm_size);
+ goto free_acpi_obj;
+ }
+ } else {
+ acpi_handle_debug(adev->handle,
+ "_DSM function 0 evaluation failed\n");
+ goto free_acpi_obj;
+ }
+
+ addr = (u32 *)out_obj->buffer.pointer;
+
+ lpm_req_regs = devm_kzalloc(&pdev->dev, lpm_size * sizeof(u32),
+ GFP_KERNEL);
+ if (!lpm_req_regs)
+ goto free_acpi_obj;
+
+ memcpy(lpm_req_regs, addr, lpm_size);
+ pmcdev->lpm_req_regs = lpm_req_regs;
+
+free_acpi_obj:
+ ACPI_FREE(out_obj);
+}
+
static inline u32 pmc_core_reg_read(struct pmc_dev *pmcdev, int reg_offset)
{
return readl(pmcdev->regbase + reg_offset);
@@ -1424,10 +1476,14 @@ static int pmc_core_probe(struct platform_device *pdev)
return -ENOMEM;

mutex_init(&pmcdev->lock);
+
pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
pmc_core_get_low_power_modes(pmcdev);
pmc_core_do_dmi_quirks(pmcdev);

+ if (pmcdev->map == &tgl_reg_map)
+ pmc_core_get_tgl_lpm_reqs(pdev);
+
/*
* On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when
* a cable is attached. Tell the PMC to ignore it.
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index aa44fd5399cc..64fb368f40f6 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -294,6 +294,7 @@ struct pmc_reg_map {
* @s0ix_counter: S0ix residency (step adjusted)
* @num_lpm_modes: Count of enabled modes
* @lpm_en_modes: Array of enabled modes from lowest to highest priority
+ * @lpm_req_regs: List of substate requirements
*
* pmc_dev contains info about power management controller device.
*/
@@ -310,6 +311,7 @@ struct pmc_dev {
u64 s0ix_counter;
int num_lpm_modes;
int lpm_en_modes[LPM_MAX_NUM_MODES];
+ u32 *lpm_req_regs;
};

#define pmc_for_each_mode(i, mode, pmcdev) \
--
2.25.1

2021-04-17 05:56:38

by kernel test robot

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

Hi "David,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on 823b31517ad3196324322804ee365d5fcff704d6]

url: https://github.com/0day-ci/linux/commits/David-E-Box/intel_pmc_core-Add-sub-state-requirements-and-mode/20210417-111530
base: 823b31517ad3196324322804ee365d5fcff704d6
config: i386-randconfig-a001-20210417 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/703038f16e99686bf2538222cee482f823bfa60f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review David-E-Box/intel_pmc_core-Add-sub-state-requirements-and-mode/20210417-111530
git checkout 703038f16e99686bf2538222cee482f823bfa60f
# save the attached .config to linux build tree
make W=1 W=1 ARCH=i386

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from drivers/platform/x86/intel_pmc_core.c:14:
drivers/platform/x86/intel_pmc_core.c: In function 'pmc_core_get_tgl_lpm_reqs':
>> drivers/platform/x86/intel_pmc_core.c:621:5: warning: format '%ld' expects argument of type 'long int', but argument 5 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
621 | "_DSM returned unexpected buffer size,"
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
622 | " have %d, expect %ld\n", size, lpm_size);
| ~~~~~~~~
| |
| size_t {aka unsigned int}
include/linux/acpi.h:1073:42: note: in definition of macro 'acpi_handle_debug'
1073 | acpi_handle_printk(KERN_DEBUG, handle, fmt, ##__VA_ARGS__); \
| ^~~
drivers/platform/x86/intel_pmc_core.c:622:25: note: format string is defined here
622 | " have %d, expect %ld\n", size, lpm_size);
| ~~^
| |
| long int
| %d


vim +621 drivers/platform/x86/intel_pmc_core.c

597
598 static void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev)
599 {
600 struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
601 const int num_maps = pmcdev->map->lpm_num_maps;
602 size_t lpm_size = LPM_MAX_NUM_MODES * num_maps * 4;
603 union acpi_object *out_obj;
604 struct acpi_device *adev;
605 guid_t s0ix_dsm_guid;
606 u32 *lpm_req_regs, *addr;
607
608 adev = ACPI_COMPANION(&pdev->dev);
609 if (!adev)
610 return;
611
612 guid_parse(ACPI_S0IX_DSM_UUID, &s0ix_dsm_guid);
613
614 out_obj = acpi_evaluate_dsm(adev->handle, &s0ix_dsm_guid, 0,
615 ACPI_GET_LOW_MODE_REGISTERS, NULL);
616 if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
617 int size = out_obj->buffer.length;
618
619 if (size != lpm_size) {
620 acpi_handle_debug(adev->handle,
> 621 "_DSM returned unexpected buffer size,"
622 " have %d, expect %ld\n", size, lpm_size);
623 goto free_acpi_obj;
624 }
625 } else {
626 acpi_handle_debug(adev->handle,
627 "_DSM function 0 evaluation failed\n");
628 goto free_acpi_obj;
629 }
630
631 addr = (u32 *)out_obj->buffer.pointer;
632
633 lpm_req_regs = devm_kzalloc(&pdev->dev, lpm_size * sizeof(u32),
634 GFP_KERNEL);
635 if (!lpm_req_regs)
636 goto free_acpi_obj;
637
638 memcpy(lpm_req_regs, addr, lpm_size);
639 pmcdev->lpm_req_regs = lpm_req_regs;
640
641 free_acpi_obj:
642 ACPI_FREE(out_obj);
643 }
644

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

2021-04-17 06:29:55

by kernel test robot

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

Hi "David,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on 823b31517ad3196324322804ee365d5fcff704d6]

url: https://github.com/0day-ci/linux/commits/David-E-Box/intel_pmc_core-Add-sub-state-requirements-and-mode/20210417-111530
base: 823b31517ad3196324322804ee365d5fcff704d6
config: i386-randconfig-a012-20210416 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/703038f16e99686bf2538222cee482f823bfa60f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review David-E-Box/intel_pmc_core-Add-sub-state-requirements-and-mode/20210417-111530
git checkout 703038f16e99686bf2538222cee482f823bfa60f
# save the attached .config to linux build tree
make W=1 W=1 ARCH=i386

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/platform/x86/intel_pmc_core.c: In function 'pmc_core_get_tgl_lpm_reqs':
>> <command-line>: warning: format '%ld' expects argument of type 'long int', but argument 5 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
drivers/platform/x86/intel_pmc_core.c:12:21: note: in expansion of macro 'KBUILD_MODNAME'
12 | #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
| ^~~~~~~~~~~~~~
include/linux/dynamic_debug.h:129:15: note: in expansion of macro 'pr_fmt'
129 | func(&id, ##__VA_ARGS__); \
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:147:2: note: in expansion of macro '__dynamic_func_call'
147 | __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/acpi.h:1067:2: note: in expansion of macro '_dynamic_func_call'
1067 | _dynamic_func_call(fmt, __acpi_handle_debug, \
| ^~~~~~~~~~~~~~~~~~
drivers/platform/x86/intel_pmc_core.c:620:4: note: in expansion of macro 'acpi_handle_debug'
620 | acpi_handle_debug(adev->handle,
| ^~~~~~~~~~~~~~~~~

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

2021-04-17 08:53:49

by Hans de Goede

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

Hi David,

On 4/17/21 5:12 AM, David E. Box wrote:
> From: Gayatri Kammela <[email protected]>
>
> Platforms that support low power modes (LPM) such as Tiger Lake maintain
> requirements for each sub-state that a readable in the PMC. However, unlike
> LPM status registers, requirement registers are not memory mapped but are
> available from an ACPI _DSM. Collect the requirements for Tiger Lake using
> the _DSM method and store in a buffer.
>
> Signed-off-by: Gayatri Kammela <[email protected]>
> Co-developed-by: David E. Box <[email protected]>
> Signed-off-by: David E. Box <[email protected]>
> Reviewed-by: Hans de Goede <[email protected]>

Erm, I did not give my "Reviewed-by: Hans de Goede <[email protected]>"
for this patch, because it still needed some work.

Next time please only add my Reviewed-by to patches where I
explicitly replied with a Reviewed-by.

The same goes for patch 7/9

Regards,

Hans



> ---
>
> V2: - Move buffer allocation so that it does not need to be freed
> (which was missing anyway) when an error is encountered.
> - Use label to free out_obj after errors
> - Use memcpy instead of memcpy_fromio for ACPI memory
>
> drivers/platform/x86/intel_pmc_core.c | 56 +++++++++++++++++++++++++++
> drivers/platform/x86/intel_pmc_core.h | 2 +
> 2 files changed, 58 insertions(+)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 0e59a84b51bf..97efe9a6bd01 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -23,7 +23,9 @@
> #include <linux/slab.h>
> #include <linux/suspend.h>
> #include <linux/uaccess.h>
> +#include <linux/uuid.h>
>
> +#include <acpi/acpi_bus.h>
> #include <asm/cpu_device_id.h>
> #include <asm/intel-family.h>
> #include <asm/msr.h>
> @@ -31,6 +33,9 @@
>
> #include "intel_pmc_core.h"
>
> +#define ACPI_S0IX_DSM_UUID "57a6512e-3979-4e9d-9708-ff13b2508972"
> +#define ACPI_GET_LOW_MODE_REGISTERS 1
> +
> /* PKGC MSRs are common across Intel Core SoCs */
> static const struct pmc_bit_map msr_map[] = {
> {"Package C2", MSR_PKG_C2_RESIDENCY},
> @@ -590,6 +595,53 @@ static const struct pmc_reg_map tgl_reg_map = {
> .etr3_offset = ETR3_OFFSET,
> };
>
> +static void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev)
> +{
> + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> + const int num_maps = pmcdev->map->lpm_num_maps;
> + size_t lpm_size = LPM_MAX_NUM_MODES * num_maps * 4;
> + union acpi_object *out_obj;
> + struct acpi_device *adev;
> + guid_t s0ix_dsm_guid;
> + u32 *lpm_req_regs, *addr;
> +
> + adev = ACPI_COMPANION(&pdev->dev);
> + if (!adev)
> + return;
> +
> + guid_parse(ACPI_S0IX_DSM_UUID, &s0ix_dsm_guid);
> +
> + out_obj = acpi_evaluate_dsm(adev->handle, &s0ix_dsm_guid, 0,
> + ACPI_GET_LOW_MODE_REGISTERS, NULL);
> + if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
> + int size = out_obj->buffer.length;
> +
> + if (size != lpm_size) {
> + acpi_handle_debug(adev->handle,
> + "_DSM returned unexpected buffer size,"
> + " have %d, expect %ld\n", size, lpm_size);
> + goto free_acpi_obj;
> + }
> + } else {
> + acpi_handle_debug(adev->handle,
> + "_DSM function 0 evaluation failed\n");
> + goto free_acpi_obj;
> + }
> +
> + addr = (u32 *)out_obj->buffer.pointer;
> +
> + lpm_req_regs = devm_kzalloc(&pdev->dev, lpm_size * sizeof(u32),
> + GFP_KERNEL);
> + if (!lpm_req_regs)
> + goto free_acpi_obj;
> +
> + memcpy(lpm_req_regs, addr, lpm_size);
> + pmcdev->lpm_req_regs = lpm_req_regs;
> +
> +free_acpi_obj:
> + ACPI_FREE(out_obj);
> +}
> +
> static inline u32 pmc_core_reg_read(struct pmc_dev *pmcdev, int reg_offset)
> {
> return readl(pmcdev->regbase + reg_offset);
> @@ -1424,10 +1476,14 @@ static int pmc_core_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> mutex_init(&pmcdev->lock);
> +
> pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
> pmc_core_get_low_power_modes(pmcdev);
> pmc_core_do_dmi_quirks(pmcdev);
>
> + if (pmcdev->map == &tgl_reg_map)
> + pmc_core_get_tgl_lpm_reqs(pdev);
> +
> /*
> * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when
> * a cable is attached. Tell the PMC to ignore it.
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index aa44fd5399cc..64fb368f40f6 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -294,6 +294,7 @@ struct pmc_reg_map {
> * @s0ix_counter: S0ix residency (step adjusted)
> * @num_lpm_modes: Count of enabled modes
> * @lpm_en_modes: Array of enabled modes from lowest to highest priority
> + * @lpm_req_regs: List of substate requirements
> *
> * pmc_dev contains info about power management controller device.
> */
> @@ -310,6 +311,7 @@ struct pmc_dev {
> u64 s0ix_counter;
> int num_lpm_modes;
> int lpm_en_modes[LPM_MAX_NUM_MODES];
> + u32 *lpm_req_regs;
> };
>
> #define pmc_for_each_mode(i, mode, pmcdev) \
>

2021-04-17 09:04:20

by Hans de Goede

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

Hi,

On 4/17/21 5:12 AM, David E. Box wrote:
> From: Gayatri Kammela <[email protected]>
>
> Platforms that support low power modes (LPM) such as Tiger Lake maintain
> requirements for each sub-state that a readable in the PMC. However, unlike
> LPM status registers, requirement registers are not memory mapped but are
> available from an ACPI _DSM. Collect the requirements for Tiger Lake using
> the _DSM method and store in a buffer.
>
> Signed-off-by: Gayatri Kammela <[email protected]>
> Co-developed-by: David E. Box <[email protected]>
> Signed-off-by: David E. Box <[email protected]>
> Reviewed-by: Hans de Goede <[email protected]>
> ---
>
> V2: - Move buffer allocation so that it does not need to be freed
> (which was missing anyway) when an error is encountered.
> - Use label to free out_obj after errors
> - Use memcpy instead of memcpy_fromio for ACPI memory
>
> drivers/platform/x86/intel_pmc_core.c | 56 +++++++++++++++++++++++++++
> drivers/platform/x86/intel_pmc_core.h | 2 +
> 2 files changed, 58 insertions(+)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 0e59a84b51bf..97efe9a6bd01 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -23,7 +23,9 @@
> #include <linux/slab.h>
> #include <linux/suspend.h>
> #include <linux/uaccess.h>
> +#include <linux/uuid.h>
>
> +#include <acpi/acpi_bus.h>
> #include <asm/cpu_device_id.h>
> #include <asm/intel-family.h>
> #include <asm/msr.h>
> @@ -31,6 +33,9 @@
>
> #include "intel_pmc_core.h"
>
> +#define ACPI_S0IX_DSM_UUID "57a6512e-3979-4e9d-9708-ff13b2508972"
> +#define ACPI_GET_LOW_MODE_REGISTERS 1
> +
> /* PKGC MSRs are common across Intel Core SoCs */
> static const struct pmc_bit_map msr_map[] = {
> {"Package C2", MSR_PKG_C2_RESIDENCY},
> @@ -590,6 +595,53 @@ static const struct pmc_reg_map tgl_reg_map = {
> .etr3_offset = ETR3_OFFSET,
> };
>
> +static void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev)
> +{
> + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> + const int num_maps = pmcdev->map->lpm_num_maps;
> + size_t lpm_size = LPM_MAX_NUM_MODES * num_maps * 4;

The type of lpm_size should be an u32, so that it matches
the type of out_obj->buffer.length.

> + union acpi_object *out_obj;
> + struct acpi_device *adev;
> + guid_t s0ix_dsm_guid;
> + u32 *lpm_req_regs, *addr;
> +
> + adev = ACPI_COMPANION(&pdev->dev);
> + if (!adev)
> + return;
> +
> + guid_parse(ACPI_S0IX_DSM_UUID, &s0ix_dsm_guid);
> +
> + out_obj = acpi_evaluate_dsm(adev->handle, &s0ix_dsm_guid, 0,
> + ACPI_GET_LOW_MODE_REGISTERS, NULL);
> + if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
> + int size = out_obj->buffer.length;

out_obj->buffer.length is an u32, please make this an u32 too.

> +
> + if (size != lpm_size) {
> + acpi_handle_debug(adev->handle,
> + "_DSM returned unexpected buffer size,"
> + " have %d, expect %ld\n", size, lpm_size);

And use %u here (twice), this should also fix the warnings reported
by the kernel test robot.

If there are no objections against the suggested changes, then I can
fix this up while merging this.

Please let me know if the suggested changes are ok with you.

Regards,

Hans


> + goto free_acpi_obj;
> + }
> + } else {
> + acpi_handle_debug(adev->handle,
> + "_DSM function 0 evaluation failed\n");
> + goto free_acpi_obj;
> + }
> +
> + addr = (u32 *)out_obj->buffer.pointer;
> +
> + lpm_req_regs = devm_kzalloc(&pdev->dev, lpm_size * sizeof(u32),
> + GFP_KERNEL);
> + if (!lpm_req_regs)
> + goto free_acpi_obj;
> +
> + memcpy(lpm_req_regs, addr, lpm_size);
> + pmcdev->lpm_req_regs = lpm_req_regs;
> +
> +free_acpi_obj:
> + ACPI_FREE(out_obj);
> +}
> +
> static inline u32 pmc_core_reg_read(struct pmc_dev *pmcdev, int reg_offset)
> {
> return readl(pmcdev->regbase + reg_offset);
> @@ -1424,10 +1476,14 @@ static int pmc_core_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> mutex_init(&pmcdev->lock);
> +
> pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
> pmc_core_get_low_power_modes(pmcdev);
> pmc_core_do_dmi_quirks(pmcdev);
>
> + if (pmcdev->map == &tgl_reg_map)
> + pmc_core_get_tgl_lpm_reqs(pdev);
> +
> /*
> * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when
> * a cable is attached. Tell the PMC to ignore it.
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index aa44fd5399cc..64fb368f40f6 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -294,6 +294,7 @@ struct pmc_reg_map {
> * @s0ix_counter: S0ix residency (step adjusted)
> * @num_lpm_modes: Count of enabled modes
> * @lpm_en_modes: Array of enabled modes from lowest to highest priority
> + * @lpm_req_regs: List of substate requirements
> *
> * pmc_dev contains info about power management controller device.
> */
> @@ -310,6 +311,7 @@ struct pmc_dev {
> u64 s0ix_counter;
> int num_lpm_modes;
> int lpm_en_modes[LPM_MAX_NUM_MODES];
> + u32 *lpm_req_regs;
> };
>
> #define pmc_for_each_mode(i, mode, pmcdev) \
>

2021-04-17 09:16:16

by Hans de Goede

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

Hi,

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

Thank you, the series looks good to me know, except for one very minor
issue in patch 5, which as I already mentioned in a reply to patch 5,
I can fixup while merging this.

Once I have you ack for the prososed changes to patch 5 I'll merge this
into me review-hans branch.

Depending on if Linus does a rc8 or not I'll then push this to for-next
for 5.13, or this will have to wait for 5.14 :

Linus does a rc8 -> I'll promote this from review-hans to for-next in time for 5.13
Linus releases 5.12 final -> I'll rebase my review-hans on top of 5.13-rc1 once released and
then push this to for-next

Regards,

Hans




>
> Patches that changed in V2:
> Patch 3: Variable name change
> Patch 5: Do proper cleanup after fail
> Patch 7: Debugfs write function fixes
>
> David E. Box (4):
> platform/x86: intel_pmc_core: Don't use global pmcdev in quirks
> platform/x86: intel_pmc_core: Remove global struct pmc_dev
> platform/x86: intel_pmc_core: Add option to set/clear LPM mode
> platform/x86: intel_pmc_core: Add support for Alder Lake PCH-P
>
> Gayatri Kammela (5):
> platform/x86: intel_pmc_core: Handle sub-states generically
> platform/x86: intel_pmc_core: Show LPM residency in microseconds
> platform/x86: intel_pmc_core: Get LPM requirements for Tiger Lake
> platform/x86: intel_pmc_core: Add requirements file to debugfs
> platform/x86: intel_pmc_core: Add LTR registers for Tiger Lake
>
> drivers/platform/x86/intel_pmc_core.c | 384 +++++++++++++++++++++++---
> drivers/platform/x86/intel_pmc_core.h | 47 +++-
> 2 files changed, 395 insertions(+), 36 deletions(-)
>
>
> base-commit: 823b31517ad3196324322804ee365d5fcff704d6
>

2021-04-18 01:47:04

by David E. Box

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

On Sat, 2021-04-17 at 11:00 +0200, Hans de Goede wrote:
> Hi,
>
> On 4/17/21 5:12 AM, David E. Box wrote:
> > From: Gayatri Kammela <[email protected]>
> >
> > Platforms that support low power modes (LPM) such as Tiger Lake
> > maintain
> > requirements for each sub-state that a readable in the PMC.
> > However, unlike
> > LPM status registers, requirement registers are not memory mapped
> > but are
> > available from an ACPI _DSM. Collect the requirements for Tiger
> > Lake using
> > the _DSM method and store in a buffer.
> >
> > Signed-off-by: Gayatri Kammela <[email protected]>
> > Co-developed-by: David E. Box <[email protected]>
> > Signed-off-by: David E. Box <[email protected]>
> > Reviewed-by: Hans de Goede <[email protected]>
> > ---
> >
> > V2:     - Move buffer allocation so that it does not need to be
> > freed
> >           (which was missing anyway) when an error is encountered.
> >         - Use label to free out_obj after errors
> >         - Use memcpy instead of memcpy_fromio for ACPI memory
> >
> >  drivers/platform/x86/intel_pmc_core.c | 56
> > +++++++++++++++++++++++++++
> >  drivers/platform/x86/intel_pmc_core.h |  2 +
> >  2 files changed, 58 insertions(+)
> >
> > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > b/drivers/platform/x86/intel_pmc_core.c
> > index 0e59a84b51bf..97efe9a6bd01 100644
> > --- a/drivers/platform/x86/intel_pmc_core.c
> > +++ b/drivers/platform/x86/intel_pmc_core.c
> > @@ -23,7 +23,9 @@
> >  #include <linux/slab.h>
> >  #include <linux/suspend.h>
> >  #include <linux/uaccess.h>
> > +#include <linux/uuid.h>
> >  
> > +#include <acpi/acpi_bus.h>
> >  #include <asm/cpu_device_id.h>
> >  #include <asm/intel-family.h>
> >  #include <asm/msr.h>
> > @@ -31,6 +33,9 @@
> >  
> >  #include "intel_pmc_core.h"
> >  
> > +#define ACPI_S0IX_DSM_UUID             "57a6512e-3979-4e9d-9708-
> > ff13b2508972"
> > +#define ACPI_GET_LOW_MODE_REGISTERS    1
> > +
> >  /* PKGC MSRs are common across Intel Core SoCs */
> >  static const struct pmc_bit_map msr_map[] = {
> >         {"Package C2",                  MSR_PKG_C2_RESIDENCY},
> > @@ -590,6 +595,53 @@ static const struct pmc_reg_map tgl_reg_map =
> > {
> >         .etr3_offset = ETR3_OFFSET,
> >  };
> >  
> > +static void pmc_core_get_tgl_lpm_reqs(struct platform_device
> > *pdev)
> > +{
> > +       struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> > +       const int num_maps = pmcdev->map->lpm_num_maps;
> > +       size_t lpm_size = LPM_MAX_NUM_MODES * num_maps * 4;
>
> The type of lpm_size should be an u32, so that it matches
> the type of out_obj->buffer.length.
>
> > +       union acpi_object *out_obj;
> > +       struct acpi_device *adev;
> > +       guid_t s0ix_dsm_guid;
> > +       u32 *lpm_req_regs, *addr;
> > +
> > +       adev = ACPI_COMPANION(&pdev->dev);
> > +       if (!adev)
> > +               return;
> > +
> > +       guid_parse(ACPI_S0IX_DSM_UUID, &s0ix_dsm_guid);
> > +
> > +       out_obj = acpi_evaluate_dsm(adev->handle, &s0ix_dsm_guid,
> > 0,
> > +                                   ACPI_GET_LOW_MODE_REGISTERS,
> > NULL);
> > +       if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
> > +               int size = out_obj->buffer.length;
>
> out_obj->buffer.length is an u32, please make this an u32 too.
>
> > +
> > +               if (size != lpm_size) {
> > +                       acpi_handle_debug(adev->handle,
> > +                               "_DSM returned unexpected buffer
> > size,"
> > +                               " have %d, expect %ld\n", size,
> > lpm_size);
>
> And use %u here (twice), this should also fix the warnings reported
> by the kernel test robot.
>
> If there are no objections against the suggested changes, then I can
> fix this up while merging this.
>
> Please let me know if the suggested changes are ok with you.

Changes are good with me. Thanks for the fixup.

David

>
> Regards,
>
> Hans
>
>
> > +                       goto free_acpi_obj;
> > +               }
> > +       } else {
> > +               acpi_handle_debug(adev->handle,
> > +                                 "_DSM function 0 evaluation
> > failed\n");
> > +               goto free_acpi_obj;
> > +       }
> > +
> > +       addr = (u32 *)out_obj->buffer.pointer;
> > +
> > +       lpm_req_regs = devm_kzalloc(&pdev->dev, lpm_size *
> > sizeof(u32),
> > +                                    GFP_KERNEL);
> > +       if (!lpm_req_regs)
> > +               goto free_acpi_obj;
> > +
> > +       memcpy(lpm_req_regs, addr, lpm_size);
> > +       pmcdev->lpm_req_regs = lpm_req_regs;
> > +
> > +free_acpi_obj:
> > +       ACPI_FREE(out_obj);
> > +}
> > +
> >  static inline u32 pmc_core_reg_read(struct pmc_dev *pmcdev, int
> > reg_offset)
> >  {
> >         return readl(pmcdev->regbase + reg_offset);
> > @@ -1424,10 +1476,14 @@ static int pmc_core_probe(struct
> > platform_device *pdev)
> >                 return -ENOMEM;
> >  
> >         mutex_init(&pmcdev->lock);
> > +
> >         pmcdev->pmc_xram_read_bit =
> > pmc_core_check_read_lock_bit(pmcdev);
> >         pmc_core_get_low_power_modes(pmcdev);
> >         pmc_core_do_dmi_quirks(pmcdev);
> >  
> > +       if (pmcdev->map == &tgl_reg_map)
> > +               pmc_core_get_tgl_lpm_reqs(pdev);
> > +
> >         /*
> >          * On TGL, due to a hardware limitation, the GBE LTR blocks
> > PC10 when
> >          * a cable is attached. Tell the PMC to ignore it.
> > diff --git a/drivers/platform/x86/intel_pmc_core.h
> > b/drivers/platform/x86/intel_pmc_core.h
> > index aa44fd5399cc..64fb368f40f6 100644
> > --- a/drivers/platform/x86/intel_pmc_core.h
> > +++ b/drivers/platform/x86/intel_pmc_core.h
> > @@ -294,6 +294,7 @@ struct pmc_reg_map {
> >   * @s0ix_counter:      S0ix residency (step adjusted)
> >   * @num_lpm_modes:     Count of enabled modes
> >   * @lpm_en_modes:      Array of enabled modes from lowest to
> > highest priority
> > + * @lpm_req_regs:      List of substate requirements
> >   *
> >   * pmc_dev contains info about power management controller device.
> >   */
> > @@ -310,6 +311,7 @@ struct pmc_dev {
> >         u64 s0ix_counter;
> >         int num_lpm_modes;
> >         int lpm_en_modes[LPM_MAX_NUM_MODES];
> > +       u32 *lpm_req_regs;
> >  };
> >  
> >  #define pmc_for_each_mode(i, mode, pmcdev)             \
> >
>

2021-04-18 02:00:10

by David E. Box

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

On Sat, 2021-04-17 at 10:52 +0200, Hans de Goede wrote:
> Hi David,
>
> On 4/17/21 5:12 AM, David E. Box wrote:
> > From: Gayatri Kammela <[email protected]>
> >
> > Platforms that support low power modes (LPM) such as Tiger Lake
> > maintain
> > requirements for each sub-state that a readable in the PMC.
> > However, unlike
> > LPM status registers, requirement registers are not memory mapped
> > but are
> > available from an ACPI _DSM. Collect the requirements for Tiger
> > Lake using
> > the _DSM method and store in a buffer.
> >
> > Signed-off-by: Gayatri Kammela <[email protected]>
> > Co-developed-by: David E. Box <[email protected]>
> > Signed-off-by: David E. Box <[email protected]>
> > Reviewed-by: Hans de Goede <[email protected]>
>
> Erm, I did not give my "Reviewed-by: Hans de Goede <
> [email protected]>"
> for this patch, because it still needed some work.
>
> Next time please only add my Reviewed-by to patches where I
> explicitly replied with a Reviewed-by.

Sure thing. Sorry about that.

David

> The same goes for patch 7/9
>
> Regards,
>
> Hans
>
>
>
> > ---
> >
> > V2:     - Move buffer allocation so that it does not need to be
> > freed
> >           (which was missing anyway) when an error is encountered.
> >         - Use label to free out_obj after errors
> >         - Use memcpy instead of memcpy_fromio for ACPI memory
> >
> >  drivers/platform/x86/intel_pmc_core.c | 56
> > +++++++++++++++++++++++++++
> >  drivers/platform/x86/intel_pmc_core.h |  2 +
> >  2 files changed, 58 insertions(+)
> >
> > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > b/drivers/platform/x86/intel_pmc_core.c
> > index 0e59a84b51bf..97efe9a6bd01 100644
> > --- a/drivers/platform/x86/intel_pmc_core.c
> > +++ b/drivers/platform/x86/intel_pmc_core.c
> > @@ -23,7 +23,9 @@
> >  #include <linux/slab.h>
> >  #include <linux/suspend.h>
> >  #include <linux/uaccess.h>
> > +#include <linux/uuid.h>
> >  
> > +#include <acpi/acpi_bus.h>
> >  #include <asm/cpu_device_id.h>
> >  #include <asm/intel-family.h>
> >  #include <asm/msr.h>
> > @@ -31,6 +33,9 @@
> >  
> >  #include "intel_pmc_core.h"
> >  
> > +#define ACPI_S0IX_DSM_UUID             "57a6512e-3979-4e9d-9708-
> > ff13b2508972"
> > +#define ACPI_GET_LOW_MODE_REGISTERS    1
> > +
> >  /* PKGC MSRs are common across Intel Core SoCs */
> >  static const struct pmc_bit_map msr_map[] = {
> >         {"Package C2",                  MSR_PKG_C2_RESIDENCY},
> > @@ -590,6 +595,53 @@ static const struct pmc_reg_map tgl_reg_map =
> > {
> >         .etr3_offset = ETR3_OFFSET,
> >  };
> >  
> > +static void pmc_core_get_tgl_lpm_reqs(struct platform_device
> > *pdev)
> > +{
> > +       struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> > +       const int num_maps = pmcdev->map->lpm_num_maps;
> > +       size_t lpm_size = LPM_MAX_NUM_MODES * num_maps * 4;
> > +       union acpi_object *out_obj;
> > +       struct acpi_device *adev;
> > +       guid_t s0ix_dsm_guid;
> > +       u32 *lpm_req_regs, *addr;
> > +
> > +       adev = ACPI_COMPANION(&pdev->dev);
> > +       if (!adev)
> > +               return;
> > +
> > +       guid_parse(ACPI_S0IX_DSM_UUID, &s0ix_dsm_guid);
> > +
> > +       out_obj = acpi_evaluate_dsm(adev->handle, &s0ix_dsm_guid,
> > 0,
> > +                                   ACPI_GET_LOW_MODE_REGISTERS,
> > NULL);
> > +       if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
> > +               int size = out_obj->buffer.length;
> > +
> > +               if (size != lpm_size) {
> > +                       acpi_handle_debug(adev->handle,
> > +                               "_DSM returned unexpected buffer
> > size,"
> > +                               " have %d, expect %ld\n", size,
> > lpm_size);
> > +                       goto free_acpi_obj;
> > +               }
> > +       } else {
> > +               acpi_handle_debug(adev->handle,
> > +                                 "_DSM function 0 evaluation
> > failed\n");
> > +               goto free_acpi_obj;
> > +       }
> > +
> > +       addr = (u32 *)out_obj->buffer.pointer;
> > +
> > +       lpm_req_regs = devm_kzalloc(&pdev->dev, lpm_size *
> > sizeof(u32),
> > +                                    GFP_KERNEL);
> > +       if (!lpm_req_regs)
> > +               goto free_acpi_obj;
> > +
> > +       memcpy(lpm_req_regs, addr, lpm_size);
> > +       pmcdev->lpm_req_regs = lpm_req_regs;
> > +
> > +free_acpi_obj:
> > +       ACPI_FREE(out_obj);
> > +}
> > +
> >  static inline u32 pmc_core_reg_read(struct pmc_dev *pmcdev, int
> > reg_offset)
> >  {
> >         return readl(pmcdev->regbase + reg_offset);
> > @@ -1424,10 +1476,14 @@ static int pmc_core_probe(struct
> > platform_device *pdev)
> >                 return -ENOMEM;
> >  
> >         mutex_init(&pmcdev->lock);
> > +
> >         pmcdev->pmc_xram_read_bit =
> > pmc_core_check_read_lock_bit(pmcdev);
> >         pmc_core_get_low_power_modes(pmcdev);
> >         pmc_core_do_dmi_quirks(pmcdev);
> >  
> > +       if (pmcdev->map == &tgl_reg_map)
> > +               pmc_core_get_tgl_lpm_reqs(pdev);
> > +
> >         /*
> >          * On TGL, due to a hardware limitation, the GBE LTR blocks
> > PC10 when
> >          * a cable is attached. Tell the PMC to ignore it.
> > diff --git a/drivers/platform/x86/intel_pmc_core.h
> > b/drivers/platform/x86/intel_pmc_core.h
> > index aa44fd5399cc..64fb368f40f6 100644
> > --- a/drivers/platform/x86/intel_pmc_core.h
> > +++ b/drivers/platform/x86/intel_pmc_core.h
> > @@ -294,6 +294,7 @@ struct pmc_reg_map {
> >   * @s0ix_counter:      S0ix residency (step adjusted)
> >   * @num_lpm_modes:     Count of enabled modes
> >   * @lpm_en_modes:      Array of enabled modes from lowest to
> > highest priority
> > + * @lpm_req_regs:      List of substate requirements
> >   *
> >   * pmc_dev contains info about power management controller device.
> >   */
> > @@ -310,6 +311,7 @@ struct pmc_dev {
> >         u64 s0ix_counter;
> >         int num_lpm_modes;
> >         int lpm_en_modes[LPM_MAX_NUM_MODES];
> > +       u32 *lpm_req_regs;
> >  };
> >  
> >  #define pmc_for_each_mode(i, mode, pmcdev)             \
> >
>

2021-04-19 08:48:47

by Hans de Goede

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

Hi,

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

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> Patches that changed in V2:
> Patch 3: Variable name change
> Patch 5: Do proper cleanup after fail
> Patch 7: Debugfs write function fixes
>
> David E. Box (4):
> platform/x86: intel_pmc_core: Don't use global pmcdev in quirks
> platform/x86: intel_pmc_core: Remove global struct pmc_dev
> platform/x86: intel_pmc_core: Add option to set/clear LPM mode
> platform/x86: intel_pmc_core: Add support for Alder Lake PCH-P
>
> Gayatri Kammela (5):
> platform/x86: intel_pmc_core: Handle sub-states generically
> platform/x86: intel_pmc_core: Show LPM residency in microseconds
> platform/x86: intel_pmc_core: Get LPM requirements for Tiger Lake
> platform/x86: intel_pmc_core: Add requirements file to debugfs
> platform/x86: intel_pmc_core: Add LTR registers for Tiger Lake
>
> drivers/platform/x86/intel_pmc_core.c | 384 +++++++++++++++++++++++---
> drivers/platform/x86/intel_pmc_core.h | 47 +++-
> 2 files changed, 395 insertions(+), 36 deletions(-)
>
>
> base-commit: 823b31517ad3196324322804ee365d5fcff704d6
>