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@xxxxxxxxxxxxxxx>
> Reviewed-by: Zhang Rui <rui.zhang@xxxxxxxxx
> ---
> Note:
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxx>.
>
> 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");
>
