| Message ID | 20240509230236.1494326-2-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | SST improvements with TPMI | expand |
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?
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");