[PATCH 0/2] SST improvements with TPMI

[PATCH 0/2] SST improvements with TPMI

[Srinivas Pandruvada]

These are some improvements to SST with TPMI, which makes sure that SST
is not available when all model specific parts are not ready and prevent
legacy SST driver to load on some platforms.

Since it is close to merge Window, please feel free to defer.

Srinivas Pandruvada (2):
  platform/x86: ISST: Add model specific loading for common module
  platform/x86: ISST: Use only TPMI interface when present

 .../intel/speed_select_if/isst_if_common.c    | 83 +++++++++++++------
 1 file changed, 59 insertions(+), 24 deletions(-)

-- 
2.44.0

[PATCH 1/2] platform/x86: ISST: Add model specific loading for common module

[Srinivas Pandruvada]

SST common module is loaded when model specific or TPMI SST driver
registers for services. There are model specific features used in SST
common modules which are checked with a CPU model list. So, this module
is model specific.

There are some use cases where loading the common module independently
only on the supported CPU models helps. The first use case is for
preventing SST TPMI module loading if the model specific features are
not implemented. The second use case for presenting information to
user space when SST is used in OOB (Out of Band) mode.

1.
With TPMI, SST interface is architectural. This means that no need to add
new PCI device IDs for new CPU models. This means that there can be lag
in adding CPU models for the model specific features in the common
module. For example, before adding CPU model to GRANITERAPIDS_D to
hpm_cpu_ids[], SST is still functional for some features and but will
get/set wrong data for features like SST-CP. This is because IOCTL
ISST_IF_GET_PHY_ID, will not give correct mapping for newer CPU models.
So adding explicit model check during load time will prevent such cases.
For unsupported CPU models, common driver will fail to load and hence
dependent modules will not be loaded.

2.
When the SST TPMI features are controlled by some OOB agent (not from OS
interface), even if the CPU model is supported, there will be no user
space interface available for tools as SST TPMI modules will not
be loaded. User space interface is registered when TPMI modules call
isst_if_cdev_register(). Even in this case user space orchestrator
software needs to get power domain information to schedule workload and
get/set turbo ratio limits. This information is exposed by the common
module using IOCTLs ISST_IF_GET_PHY_ID and ISST_IF_MSR_COMMAND
respectively. Since the user space MSR access can be locked, direct MSR
access from the user space is not an option using /dev/cpu/*/msr.

Converge all the existing model checks to one common place and
use driver data to differentiate. On successful model check
call isst_misc_reg().

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Reviewed-by: Zhang Rui <rui.zhang@intel.com
---
Note:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Andy's Reviewed-by was for one version before Rafael and Rui's
comments were addressed. So didn't add it before "---". 
We can add it if there is no issue from Andy.

Also incorporated changes suggested by "Wysocki, Rafael J"
<rafael.j.wysocki@intel.com>.

We have all the models supported till date are already in 6.9
kernel. So this is more for future proofing for the first use case.


 .../intel/speed_select_if/isst_if_common.c    | 80 +++++++++++++------
 1 file changed, 56 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
index 88a17be7cb7e..f886f9369fad 100644
--- a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
+++ b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
@@ -718,12 +718,6 @@ static struct miscdevice isst_if_char_driver = {
 	.fops		= &isst_if_char_driver_ops,
 };
 
-static const struct x86_cpu_id hpm_cpu_ids[] = {
-	X86_MATCH_INTEL_FAM6_MODEL(GRANITERAPIDS_X,	NULL),
-	X86_MATCH_INTEL_FAM6_MODEL(ATOM_CRESTMONT_X,	NULL),
-	{}
-};
-
 static int isst_misc_reg(void)
 {
 	mutex_lock(&punit_misc_dev_reg_lock);
@@ -731,12 +725,6 @@ static int isst_misc_reg(void)
 		goto unlock_exit;
 
 	if (!misc_usage_count) {
-		const struct x86_cpu_id *id;
-
-		id = x86_match_cpu(hpm_cpu_ids);
-		if (id)
-			isst_hpm_support = true;
-
 		misc_device_ret = isst_if_cpu_info_init();
 		if (misc_device_ret)
 			goto unlock_exit;
@@ -784,8 +772,6 @@ static void isst_misc_unreg(void)
  */
 int isst_if_cdev_register(int device_type, struct isst_if_cmd_cb *cb)
 {
-	int ret;
-
 	if (device_type >= ISST_IF_DEV_MAX)
 		return -EINVAL;
 
@@ -803,15 +789,6 @@ int isst_if_cdev_register(int device_type, struct isst_if_cmd_cb *cb)
 	punit_callbacks[device_type].registered = 1;
 	mutex_unlock(&punit_misc_dev_open_lock);
 
-	ret = isst_misc_reg();
-	if (ret) {
-		/*
-		 * No need of mutex as the misc device register failed
-		 * as no one can open device yet. Hence no contention.
-		 */
-		punit_callbacks[device_type].registered = 0;
-		return ret;
-	}
 	return 0;
 }
 EXPORT_SYMBOL_GPL(isst_if_cdev_register);
@@ -827,7 +804,6 @@ EXPORT_SYMBOL_GPL(isst_if_cdev_register);
  */
 void isst_if_cdev_unregister(int device_type)
 {
-	isst_misc_unreg();
 	mutex_lock(&punit_misc_dev_open_lock);
 	punit_callbacks[device_type].def_ioctl = NULL;
 	punit_callbacks[device_type].registered = 0;
@@ -837,5 +813,61 @@ void isst_if_cdev_unregister(int device_type)
 }
 EXPORT_SYMBOL_GPL(isst_if_cdev_unregister);
 
+#define SST_HPM_SUPPORTED		0x01
+#define SST_MBOX_SUPPORTED		0x02
+
+#define MSR_OS_MAILBOX_INTERFACE        0xB0
+#define MSR_OS_MAILBOX_DATA             0xB1
+
+static const struct x86_cpu_id isst_cpu_ids[] = {
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_CRESTMONT,	SST_HPM_SUPPORTED),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_CRESTMONT_X,	SST_HPM_SUPPORTED),
+	X86_MATCH_INTEL_FAM6_MODEL(EMERALDRAPIDS_X,	0),
+	X86_MATCH_INTEL_FAM6_MODEL(GRANITERAPIDS_D,	SST_HPM_SUPPORTED),
+	X86_MATCH_INTEL_FAM6_MODEL(GRANITERAPIDS_X,	SST_HPM_SUPPORTED),
+	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_D,		0),
+	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		0),
+	X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X,	0),
+	X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X,		SST_MBOX_SUPPORTED),
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, isst_cpu_ids);
+
+static int __init isst_if_common_init(void)
+{
+	const struct x86_cpu_id *id;
+
+	id = x86_match_cpu(isst_cpu_ids);
+	if (!id)
+		return -ENODEV;
+
+	if (!id->driver_data)
+		goto misc_reg;
+
+	if (id->driver_data == SST_HPM_SUPPORTED) {
+		isst_hpm_support = true;
+		goto misc_reg;
+	}
+
+	if (id->driver_data == SST_MBOX_SUPPORTED) {
+		u64 data;
+
+		/* Can fail only on some Skylake-X generations */
+		if (rdmsrl_safe(MSR_OS_MAILBOX_INTERFACE, &data) ||
+		    rdmsrl_safe(MSR_OS_MAILBOX_DATA, &data))
+			return -ENODEV;
+	}
+
+misc_reg:
+	return isst_misc_reg();
+}
+module_init(isst_if_common_init)
+
+static void __exit isst_if_common_exit(void)
+{
+	isst_misc_unreg();
+}
+module_exit(isst_if_common_exit)
+
 MODULE_DESCRIPTION("ISST common interface module");
 MODULE_LICENSE("GPL v2");
-- 
2.44.0

Re: [PATCH 1/2] platform/x86: ISST: Add model specific loading for common module

[Ilpo Järvinen]

On Thu, 9 May 2024, Srinivas Pandruvada wrote:

> SST common module is loaded when model specific or TPMI SST driver
> registers for services. There are model specific features used in SST
> common modules which are checked with a CPU model list. So, this module
> is model specific.
> 
> There are some use cases where loading the common module independently
> only on the supported CPU models helps. The first use case is for
> preventing SST TPMI module loading if the model specific features are
> not implemented. The second use case for presenting information to
> user space when SST is used in OOB (Out of Band) mode.
> 
> 1.
> With TPMI, SST interface is architectural. This means that no need to add
> new PCI device IDs for new CPU models. This means that there can be lag
> in adding CPU models for the model specific features in the common
> module. For example, before adding CPU model to GRANITERAPIDS_D to
> hpm_cpu_ids[], SST is still functional for some features and but will
> get/set wrong data for features like SST-CP. This is because IOCTL
> ISST_IF_GET_PHY_ID, will not give correct mapping for newer CPU models.
> So adding explicit model check during load time will prevent such cases.
> For unsupported CPU models, common driver will fail to load and hence
> dependent modules will not be loaded.
> 
> 2.
> When the SST TPMI features are controlled by some OOB agent (not from OS
> interface), even if the CPU model is supported, there will be no user
> space interface available for tools as SST TPMI modules will not
> be loaded. User space interface is registered when TPMI modules call
> isst_if_cdev_register(). Even in this case user space orchestrator
> software needs to get power domain information to schedule workload and
> get/set turbo ratio limits. This information is exposed by the common
> module using IOCTLs ISST_IF_GET_PHY_ID and ISST_IF_MSR_COMMAND
> respectively. Since the user space MSR access can be locked, direct MSR
> access from the user space is not an option using /dev/cpu/*/msr.
> 
> Converge all the existing model checks to one common place and
> use driver data to differentiate. On successful model check
> call isst_misc_reg().
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Reviewed-by: Zhang Rui <rui.zhang@intel.com
> ---
> Note:
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Andy's Reviewed-by was for one version before Rafael and Rui's
> comments were addressed. So didn't add it before "---". 
> We can add it if there is no issue from Andy.
> 
> Also incorporated changes suggested by "Wysocki, Rafael J"
> <rafael.j.wysocki@intel.com>.
> 
> We have all the models supported till date are already in 6.9
> kernel. So this is more for future proofing for the first use case.
> 
> 
>  .../intel/speed_select_if/isst_if_common.c    | 80 +++++++++++++------
>  1 file changed, 56 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
> index 88a17be7cb7e..f886f9369fad 100644
> --- a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
> +++ b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
> @@ -718,12 +718,6 @@ static struct miscdevice isst_if_char_driver = {
>  	.fops		= &isst_if_char_driver_ops,
>  };
>  
> -static const struct x86_cpu_id hpm_cpu_ids[] = {
> -	X86_MATCH_INTEL_FAM6_MODEL(GRANITERAPIDS_X,	NULL),
> -	X86_MATCH_INTEL_FAM6_MODEL(ATOM_CRESTMONT_X,	NULL),
> -	{}
> -};
> -
>  static int isst_misc_reg(void)
>  {
>  	mutex_lock(&punit_misc_dev_reg_lock);
> @@ -731,12 +725,6 @@ static int isst_misc_reg(void)
>  		goto unlock_exit;
>  
>  	if (!misc_usage_count) {
> -		const struct x86_cpu_id *id;
> -
> -		id = x86_match_cpu(hpm_cpu_ids);
> -		if (id)
> -			isst_hpm_support = true;
> -
>  		misc_device_ret = isst_if_cpu_info_init();
>  		if (misc_device_ret)
>  			goto unlock_exit;
> @@ -784,8 +772,6 @@ static void isst_misc_unreg(void)
>   */
>  int isst_if_cdev_register(int device_type, struct isst_if_cmd_cb *cb)
>  {
> -	int ret;
> -
>  	if (device_type >= ISST_IF_DEV_MAX)
>  		return -EINVAL;
>  
> @@ -803,15 +789,6 @@ int isst_if_cdev_register(int device_type, struct isst_if_cmd_cb *cb)
>  	punit_callbacks[device_type].registered = 1;
>  	mutex_unlock(&punit_misc_dev_open_lock);
>  
> -	ret = isst_misc_reg();
> -	if (ret) {
> -		/*
> -		 * No need of mutex as the misc device register failed
> -		 * as no one can open device yet. Hence no contention.
> -		 */
> -		punit_callbacks[device_type].registered = 0;
> -		return ret;
> -	}
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(isst_if_cdev_register);
> @@ -827,7 +804,6 @@ EXPORT_SYMBOL_GPL(isst_if_cdev_register);
>   */
>  void isst_if_cdev_unregister(int device_type)
>  {
> -	isst_misc_unreg();
>  	mutex_lock(&punit_misc_dev_open_lock);
>  	punit_callbacks[device_type].def_ioctl = NULL;
>  	punit_callbacks[device_type].registered = 0;
> @@ -837,5 +813,61 @@ void isst_if_cdev_unregister(int device_type)
>  }
>  EXPORT_SYMBOL_GPL(isst_if_cdev_unregister);
>  
> +#define SST_HPM_SUPPORTED		0x01
> +#define SST_MBOX_SUPPORTED		0x02
> +
> +#define MSR_OS_MAILBOX_INTERFACE        0xB0
> +#define MSR_OS_MAILBOX_DATA             0xB1
> +
> +static const struct x86_cpu_id isst_cpu_ids[] = {
> +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_CRESTMONT,	SST_HPM_SUPPORTED),
> +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_CRESTMONT_X,	SST_HPM_SUPPORTED),
> +	X86_MATCH_INTEL_FAM6_MODEL(EMERALDRAPIDS_X,	0),
> +	X86_MATCH_INTEL_FAM6_MODEL(GRANITERAPIDS_D,	SST_HPM_SUPPORTED),
> +	X86_MATCH_INTEL_FAM6_MODEL(GRANITERAPIDS_X,	SST_HPM_SUPPORTED),
> +	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_D,		0),
> +	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		0),
> +	X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X,	0),
> +	X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X,		SST_MBOX_SUPPORTED),
> +	{}
> +};
> +MODULE_DEVICE_TABLE(x86cpu, isst_cpu_ids);
> +
> +static int __init isst_if_common_init(void)
> +{
> +	const struct x86_cpu_id *id;
> +
> +	id = x86_match_cpu(isst_cpu_ids);
> +	if (!id)
> +		return -ENODEV;
> +
> +	if (!id->driver_data)
> +		goto misc_reg;
> +
> +	if (id->driver_data == SST_HPM_SUPPORTED) {
> +		isst_hpm_support = true;
> +		goto misc_reg;
> +	}
> +
> +	if (id->driver_data == SST_MBOX_SUPPORTED) {
> +		u64 data;
> +
> +		/* Can fail only on some Skylake-X generations */
> +		if (rdmsrl_safe(MSR_OS_MAILBOX_INTERFACE, &data) ||
> +		    rdmsrl_safe(MSR_OS_MAILBOX_DATA, &data))
> +			return -ENODEV;

This Skylake bit seems to come out of nowhere which is odd for a refactor 
patch, can it be in own patch with proper commit message written for it?

-- 
 i.

> +	}
> +
> +misc_reg:
> +	return isst_misc_reg();
> +}
> +module_init(isst_if_common_init)
> +
> +static void __exit isst_if_common_exit(void)
> +{
> +	isst_misc_unreg();
> +}
> +module_exit(isst_if_common_exit)
> +
>  MODULE_DESCRIPTION("ISST common interface module");
>  MODULE_LICENSE("GPL v2");
> 

[PATCH 2/2] platform/x86: ISST: Use only TPMI interface when present

[Srinivas Pandruvada]

When the TPMI interface is present, use this interface instead of legacy.
On some systems legacy IO device is also present. Using both interfaces
together is confusing and may set the hardware in inconsistent state.

When TPMI interface is present, don't load legacy drivers.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Zhang Rui <rui.zhang@intel.com
---
 drivers/platform/x86/intel/speed_select_if/isst_if_common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
index f886f9369fad..6283af37e73b 100644
--- a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
+++ b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
@@ -775,6 +775,9 @@ int isst_if_cdev_register(int device_type, struct isst_if_cmd_cb *cb)
 	if (device_type >= ISST_IF_DEV_MAX)
 		return -EINVAL;
 
+	if (device_type < ISST_IF_DEV_TPMI && isst_hpm_support)
+		return -ENODEV;
+
 	mutex_lock(&punit_misc_dev_open_lock);
 	/* Device is already open, we don't want to add new callbacks */
 	if (misc_device_open) {
-- 
2.44.0

Re: [PATCH 2/2] platform/x86: ISST: Use only TPMI interface when present

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]

On Thu, 9 May 2024, Srinivas Pandruvada wrote:

> When the TPMI interface is present, use this interface instead of legacy.
> On some systems legacy IO device is also present. Using both interfaces
> together is confusing and may set the hardware in inconsistent state.
> 
> When TPMI interface is present, don't load legacy drivers.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Zhang Rui <rui.zhang@intel.com
> ---
>  drivers/platform/x86/intel/speed_select_if/isst_if_common.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
> index f886f9369fad..6283af37e73b 100644
> --- a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
> +++ b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
> @@ -775,6 +775,9 @@ int isst_if_cdev_register(int device_type, struct isst_if_cmd_cb *cb)
>  	if (device_type >= ISST_IF_DEV_MAX)
>  		return -EINVAL;
>  
> +	if (device_type < ISST_IF_DEV_TPMI && isst_hpm_support)
> +		return -ENODEV;

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.