Re: [PATCH 06/12] platform/x86: ISST: Enumerate TPMI SST and create framework

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 3/1/23 15:45, srinivas pandruvada wrote:
> On Wed, 2023-03-01 at 15:37 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 2/11/23 07:32, Srinivas Pandruvada wrote:
>>> Enumerate TPMI SST driver and create basic framework to add more
>>> features.
>>>
>>> The basic user space interface is still same as the legacy using
>>> /dev/isst_interface. Users of "intel-speed-select" utility should
>>> be able to use same commands as prior gens without being aware
>>> of new underlying hardware interface.
>>>
>>> TPMI SST driver enumerates on device "intel_vsec.tpmi-sst". Since
>>> there
>>> can be multiple instances and there is one common SST core, split
>>> implementation into two parts: A common core part and an
>>> enumeration
>>> part. The enumeration driver is loaded for each device instance and
>>> register with the TPMI SST core driver.
>>>
>>> On very first enumeration the TPMI SST core driver register with
>>> SST
>>> core driver to get IOCTL callbacks. The api_version is incremented
>>> for IOCTL ISST_IF_GET_PLATFORM_INFO, so that user space can issue
>>> new IOCTLs.
>>>
>>> Each TPMI package contains multiple power domains. Each power
>>> domain
>>> has its own set of SST controls. For each domain map the MMIO
>>> memory
>>> and update per domain struct tpmi_per_power_domain_info. This
>>> information
>>> will be used to implement other SST interfaces.
>>>
>>> Implement first IOCTL commands to get number of TPMI SST instances
>>> and instance mask as some of the power domains may not have any
>>> SST controls.
>>>
>>> Signed-off-by: Srinivas Pandruvada
>>> <srinivas.pandruvada@xxxxxxxxxxxxxxx>
>>
>> Thanks, patch looks good to me:
>>
>> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>>
>> But this is a somewhat larger patch and IMHO it would be good
>> to get an extra pair of eyes on this, can you get someone else
>> from Intel to also review this patch ?
> 
> Five versions of this patchset was reviewed internally.

That is good to know.

>> This (getting someone else to review the patches) especially
>> applies to patches 7-11, where I don't really feel myself
>> qualifeed to review them. I have given them a quick lookover
>> and nothing stood out, but they really need a Reviewed-by or
>> at minimum an Ack from someone else @Intel.
>>
> When I submit the next version, I will add their reviewed-by.
> So once you are done your reviews, I will submit a new series.

I'm done with my review, I only had some remarks to patch 12/12.

Please submit a new version with Revieved-by(s) when you
have the new version ready.

Regards,

Hans




>>> ---
>>>  .../x86/intel/speed_select_if/Kconfig         |   4 +
>>>  .../x86/intel/speed_select_if/Makefile        |   2 +
>>>  .../x86/intel/speed_select_if/isst_tpmi.c     |  53 ++++
>>>  .../intel/speed_select_if/isst_tpmi_core.c    | 274
>>> ++++++++++++++++++
>>>  .../intel/speed_select_if/isst_tpmi_core.h    |  16 +
>>>  include/uapi/linux/isst_if.h                  |  18 ++
>>>  6 files changed, 367 insertions(+)
>>>  create mode 100644
>>> drivers/platform/x86/intel/speed_select_if/isst_tpmi.c
>>>  create mode 100644
>>> drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
>>>  create mode 100644
>>> drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.h
>>>
>>> diff --git a/drivers/platform/x86/intel/speed_select_if/Kconfig
>>> b/drivers/platform/x86/intel/speed_select_if/Kconfig
>>> index ce3e3dc076d2..4eb3ad299db0 100644
>>> --- a/drivers/platform/x86/intel/speed_select_if/Kconfig
>>> +++ b/drivers/platform/x86/intel/speed_select_if/Kconfig
>>> @@ -2,8 +2,12 @@ menu "Intel Speed Select Technology interface
>>> support"
>>>         depends on PCI
>>>         depends on X86_64 || COMPILE_TEST
>>>  
>>> +config INTEL_SPEED_SELECT_TPMI
>>> +       tristate
>>> +
>>>  config INTEL_SPEED_SELECT_INTERFACE
>>>         tristate "Intel(R) Speed Select Technology interface
>>> drivers"
>>> +       select INTEL_SPEED_SELECT_TPMI if INTEL_TPMI
>>>         help
>>>           This config enables the Intel(R) Speed Select Technology
>>> interface
>>>           drivers. The Intel(R) speed select technology features
>>> are non
>>> diff --git a/drivers/platform/x86/intel/speed_select_if/Makefile
>>> b/drivers/platform/x86/intel/speed_select_if/Makefile
>>> index 856076206f35..1d878a36d0ab 100644
>>> --- a/drivers/platform/x86/intel/speed_select_if/Makefile
>>> +++ b/drivers/platform/x86/intel/speed_select_if/Makefile
>>> @@ -8,3 +8,5 @@ obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE) +=
>>> isst_if_common.o
>>>  obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE) += isst_if_mmio.o
>>>  obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE) += isst_if_mbox_pci.o
>>>  obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE) += isst_if_mbox_msr.o
>>> +obj-$(CONFIG_INTEL_SPEED_SELECT_TPMI) += isst_tpmi_core.o
>>> +obj-$(CONFIG_INTEL_SPEED_SELECT_TPMI) += isst_tpmi.o
>>> diff --git a/drivers/platform/x86/intel/speed_select_if/isst_tpmi.c
>>> b/drivers/platform/x86/intel/speed_select_if/isst_tpmi.c
>>> new file mode 100644
>>> index 000000000000..7b4bdeefb8bc
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi.c
>>> @@ -0,0 +1,53 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * isst_tpmi.c: SST TPMI interface
>>> + *
>>> + * Copyright (c) 2023, Intel Corporation.
>>> + * All Rights Reserved.
>>> + *
>>> + */
>>> +
>>> +#include <linux/auxiliary_bus.h>
>>> +#include <linux/module.h>
>>> +#include <linux/intel_tpmi.h>
>>> +
>>> +#include "isst_tpmi_core.h"
>>> +
>>> +static int intel_sst_probe(struct auxiliary_device *auxdev, const
>>> struct auxiliary_device_id *id)
>>> +{
>>> +       int ret;
>>> +
>>> +       ret = tpmi_sst_init();
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       ret = tpmi_sst_dev_add(auxdev);
>>> +       if (ret)
>>> +               tpmi_sst_exit();
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +static void intel_sst_remove(struct auxiliary_device *auxdev)
>>> +{
>>> +       tpmi_sst_dev_remove(auxdev);
>>> +       tpmi_sst_exit();
>>> +}
>>> +
>>> +static const struct auxiliary_device_id intel_sst_id_table[] = {
>>> +       { .name = "intel_vsec.tpmi-sst" },
>>> +       {}
>>> +};
>>> +MODULE_DEVICE_TABLE(auxiliary, intel_sst_id_table);
>>> +
>>> +static struct auxiliary_driver intel_sst_aux_driver = {
>>> +       .id_table       = intel_sst_id_table,
>>> +       .remove         = intel_sst_remove,
>>> +       .probe          = intel_sst_probe,
>>> +};
>>> +
>>> +module_auxiliary_driver(intel_sst_aux_driver);
>>> +
>>> +MODULE_IMPORT_NS(INTEL_TPMI_SST);
>>> +MODULE_DESCRIPTION("Intel TPMI SST Driver");
>>> +MODULE_LICENSE("GPL");
>>> diff --git
>>> a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
>>> b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
>>> new file mode 100644
>>> index 000000000000..6b37016c0417
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
>>> @@ -0,0 +1,274 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * isst_tpmi.c: SST TPMI interface core
>>> + *
>>> + * Copyright (c) 2023, Intel Corporation.
>>> + * All Rights Reserved.
>>> + *
>>> + * This information will be useful to understand flows:
>>> + * In the current generation of platforms, TPMI is supported via
>>> OOB
>>> + * PCI device. This PCI device has one instance per CPU package.
>>> + * There is a unique TPMI ID for SST. Each TPMI ID also has
>>> multiple
>>> + * entries, representing per power domain information.
>>> + *
>>> + * There is one dev file for complete SST information and control
>>> same as the
>>> + * prior generation of hardware. User spaces don't need to know
>>> how the
>>> + * information is presented by the hardware. The TPMI core module
>>> implements
>>> + * the hardware mapping.
>>> + */
>>> +
>>> +#include <linux/auxiliary_bus.h>
>>> +#include <linux/intel_tpmi.h>
>>> +#include <linux/fs.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <uapi/linux/isst_if.h>
>>> +
>>> +#include "isst_tpmi_core.h"
>>> +#include "isst_if_common.h"
>>> +
>>> +/**
>>> + * struct tpmi_per_power_domain_info - Store per power_domain SST
>>> info
>>> + * @package_id:                Package id for this power_domain
>>> + * @power_domain_id:   Power domain id, Each entry from the SST-
>>> TPMI instance is a power_domain.
>>> + * @sst_base:          Mapped SST base IO memory
>>> + * @auxdev:            Auxiliary device instance enumerated this
>>> instance
>>> + *
>>> + * This structure is used store complete SST information for a
>>> power_domain. This information
>>> + * is used to read/write request for any SST IOCTL. Each physical
>>> CPU package can have multiple
>>> + * power_domains. Each power domain describes its own SST
>>> information and has its own controls.
>>> + */
>>> +struct tpmi_per_power_domain_info {
>>> +       int package_id;
>>> +       int power_domain_id;
>>> +       void __iomem *sst_base;
>>> +       struct auxiliary_device *auxdev;
>>> +};
>>> +
>>> +/**
>>> + * struct tpmi_sst_struct -    Store sst info for a package
>>> + * @package_id:                        Package id for this aux
>>> device instance
>>> + * @number_of_power_domains:   Number of power_domains pointed by
>>> power_domain_info pointer
>>> + * @power_domain_info:         Pointer to power domains
>>> information
>>> + *
>>> + * This structure is used store full SST information for a
>>> package.
>>> + * Each package has a unique OOB PCI device, which enumerates
>>> TPMI.
>>> + * Each Package will have multiple power_domains.
>>> + */
>>> +struct tpmi_sst_struct {
>>> +       int package_id;
>>> +       int number_of_power_domains;
>>> +       struct tpmi_per_power_domain_info *power_domain_info;
>>> +};
>>> +
>>> +/**
>>> + * struct tpmi_sst_common_struct -     Store all SST instances
>>> + * @max_index:         Maximum instances currently present
>>> + * @sst_inst:          Pointer to per package instance
>>> + *
>>> + * Stores every SST Package instance.
>>> + */
>>> +struct tpmi_sst_common_struct {
>>> +       int max_index;
>>> +       struct tpmi_sst_struct **sst_inst;
>>> +};
>>> +
>>> +/*
>>> + * Each IOCTL request is processed under this lock. Also used to
>>> protect
>>> + * registration functions and common data structures.
>>> + */
>>> +static DEFINE_MUTEX(isst_tpmi_dev_lock);
>>> +
>>> +/* Usage count to track, number of TPMI SST instances registered
>>> to this core. */
>>> +static int isst_core_usage_count;
>>> +
>>> +/* Stores complete SST information for every package and
>>> power_domain */
>>> +static struct tpmi_sst_common_struct isst_common;
>>> +
>>> +static int isst_if_get_tpmi_instance_count(void __user *argp)
>>> +{
>>> +       struct isst_tpmi_instance_count tpmi_inst;
>>> +       struct tpmi_sst_struct *sst_inst;
>>> +       int i;
>>> +
>>> +       if (copy_from_user(&tpmi_inst, argp, sizeof(tpmi_inst)))
>>> +               return -EFAULT;
>>> +
>>> +       if (tpmi_inst.socket_id >= topology_max_packages())
>>> +               return -EINVAL;
>>> +
>>> +       tpmi_inst.count =
>>> isst_common.sst_inst[tpmi_inst.socket_id]->number_of_power_domains;
>>> +
>>> +       sst_inst = isst_common.sst_inst[tpmi_inst.socket_id];
>>> +       tpmi_inst.valid_mask = 0;
>>> +       for (i = 0; i < sst_inst->number_of_power_domains; ++i) {
>>> +               struct tpmi_per_power_domain_info
>>> *power_domain_info;
>>> +
>>> +               power_domain_info = &sst_inst-
>>>> power_domain_info[i];
>>> +               if (power_domain_info->sst_base)
>>> +                       tpmi_inst.valid_mask |= BIT(i);
>>> +       }
>>> +
>>> +       if (copy_to_user(argp, &tpmi_inst, sizeof(tpmi_inst)))
>>> +               return -EFAULT;
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static long isst_if_def_ioctl(struct file *file, unsigned int cmd,
>>> +                             unsigned long arg)
>>> +{
>>> +       void __user *argp = (void __user *)arg;
>>> +       long ret = -ENOTTY;
>>> +
>>> +       mutex_lock(&isst_tpmi_dev_lock);
>>> +       switch (cmd) {
>>> +       case ISST_IF_COUNT_TPMI_INSTANCES:
>>> +               ret = isst_if_get_tpmi_instance_count(argp);
>>> +               break;
>>> +       default:
>>> +               break;
>>> +       }
>>> +       mutex_unlock(&isst_tpmi_dev_lock);
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +int tpmi_sst_dev_add(struct auxiliary_device *auxdev)
>>> +{
>>> +       struct intel_tpmi_plat_info *plat_info;
>>> +       struct tpmi_sst_struct *tpmi_sst;
>>> +       int i, pkg = 0, inst = 0;
>>> +       int num_resources;
>>> +
>>> +       plat_info = tpmi_get_platform_data(auxdev);
>>> +       if (!plat_info) {
>>> +               dev_err(&auxdev->dev, "No platform info\n");
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       pkg = plat_info->package_id;
>>> +       if (pkg >= topology_max_packages()) {
>>> +               dev_err(&auxdev->dev, "Invalid package id :%x\n",
>>> pkg);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       if (isst_common.sst_inst[pkg])
>>> +               return -EEXIST;
>>> +
>>> +       num_resources = tpmi_get_resource_count(auxdev);
>>> +
>>> +       if (!num_resources)
>>> +               return -EINVAL;
>>> +
>>> +       tpmi_sst = devm_kzalloc(&auxdev->dev, sizeof(*tpmi_sst),
>>> GFP_KERNEL);
>>> +       if (!tpmi_sst)
>>> +               return -ENOMEM;
>>> +
>>> +       tpmi_sst->power_domain_info = devm_kcalloc(&auxdev->dev,
>>> num_resources,
>>> +                                                 
>>> sizeof(*tpmi_sst->power_domain_info),
>>> +                                                  GFP_KERNEL);
>>> +       if (!tpmi_sst->power_domain_info)
>>> +               return -ENOMEM;
>>> +
>>> +       tpmi_sst->number_of_power_domains = num_resources;
>>> +
>>> +       for (i = 0; i < num_resources; ++i) {
>>> +               struct resource *res;
>>> +
>>> +               res = tpmi_get_resource_at_index(auxdev, i);
>>> +               if (!res) {
>>> +                       tpmi_sst->power_domain_info[i].sst_base =
>>> NULL;
>>> +                       continue;
>>> +               }
>>> +
>>> +               tpmi_sst->power_domain_info[i].package_id = pkg;
>>> +               tpmi_sst->power_domain_info[i].power_domain_id = i;
>>> +               tpmi_sst->power_domain_info[i].auxdev = auxdev;
>>> +               tpmi_sst->power_domain_info[i].sst_base =
>>> devm_ioremap_resource(&auxdev->dev, res);
>>> +               if (IS_ERR(tpmi_sst-
>>>> power_domain_info[i].sst_base))
>>> +                       return PTR_ERR(tpmi_sst-
>>>> power_domain_info[i].sst_base);
>>> +
>>> +               ++inst;
>>> +       }
>>> +
>>> +       if (!inst)
>>> +               return -ENODEV;
>>> +
>>> +       tpmi_sst->package_id = pkg;
>>> +       auxiliary_set_drvdata(auxdev, tpmi_sst);
>>> +
>>> +       mutex_lock(&isst_tpmi_dev_lock);
>>> +       if (isst_common.max_index < pkg)
>>> +               isst_common.max_index = pkg;
>>> +       isst_common.sst_inst[pkg] = tpmi_sst;
>>> +       mutex_unlock(&isst_tpmi_dev_lock);
>>> +
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(tpmi_sst_dev_add, INTEL_TPMI_SST);
>>> +
>>> +void tpmi_sst_dev_remove(struct auxiliary_device *auxdev)
>>> +{
>>> +       struct tpmi_sst_struct *tpmi_sst =
>>> auxiliary_get_drvdata(auxdev);
>>> +
>>> +       mutex_lock(&isst_tpmi_dev_lock);
>>> +       isst_common.sst_inst[tpmi_sst->package_id] = NULL;
>>> +       mutex_unlock(&isst_tpmi_dev_lock);
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(tpmi_sst_dev_remove, INTEL_TPMI_SST);
>>> +
>>> +#define ISST_TPMI_API_VERSION  0x02
>>> +
>>> +int tpmi_sst_init(void)
>>> +{
>>> +       struct isst_if_cmd_cb cb;
>>> +       int ret = 0;
>>> +
>>> +       mutex_lock(&isst_tpmi_dev_lock);
>>> +
>>> +       if (isst_core_usage_count) {
>>> +               ++isst_core_usage_count;
>>> +               goto init_done;
>>> +       }
>>> +
>>> +       isst_common.sst_inst = kcalloc(topology_max_packages(),
>>> +                                     
>>> sizeof(*isst_common.sst_inst),
>>> +                                      GFP_KERNEL);
>>> +       if (!isst_common.sst_inst)
>>> +               return -ENOMEM;
>>> +
>>> +       memset(&cb, 0, sizeof(cb));
>>> +       cb.cmd_size = sizeof(struct isst_if_io_reg);
>>> +       cb.offset = offsetof(struct isst_if_io_regs, io_reg);
>>> +       cb.cmd_callback = NULL;
>>> +       cb.api_version = ISST_TPMI_API_VERSION;
>>> +       cb.def_ioctl = isst_if_def_ioctl;
>>> +       cb.owner = THIS_MODULE;
>>> +       ret = isst_if_cdev_register(ISST_IF_DEV_TPMI, &cb);
>>> +       if (ret)
>>> +               kfree(isst_common.sst_inst);
>>> +init_done:
>>> +       mutex_unlock(&isst_tpmi_dev_lock);
>>> +       return ret;
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(tpmi_sst_init, INTEL_TPMI_SST);
>>> +
>>> +void tpmi_sst_exit(void)
>>> +{
>>> +       mutex_lock(&isst_tpmi_dev_lock);
>>> +       if (isst_core_usage_count)
>>> +               --isst_core_usage_count;
>>> +
>>> +       if (!isst_core_usage_count) {
>>> +               isst_if_cdev_unregister(ISST_IF_DEV_TPMI);
>>> +               kfree(isst_common.sst_inst);
>>> +       }
>>> +       mutex_unlock(&isst_tpmi_dev_lock);
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(tpmi_sst_exit, INTEL_TPMI_SST);
>>> +
>>> +MODULE_IMPORT_NS(INTEL_TPMI);
>>> +MODULE_IMPORT_NS(INTEL_TPMI_POWER_DOMAIN);
>>> +
>>> +MODULE_LICENSE("GPL");
>>> diff --git
>>> a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.h
>>> b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.h
>>> new file mode 100644
>>> index 000000000000..356cb02273b1
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.h
>>> @@ -0,0 +1,16 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Intel Speed Select Interface: Drivers Internal defines
>>> + * Copyright (c) 2023, Intel Corporation.
>>> + * All rights reserved.
>>> + *
>>> + */
>>> +
>>> +#ifndef _ISST_TPMI_CORE_H
>>> +#define _ISST_TPMI_CORE_H
>>> +
>>> +int tpmi_sst_init(void);
>>> +void tpmi_sst_exit(void);
>>> +int tpmi_sst_dev_add(struct auxiliary_device *auxdev);
>>> +void tpmi_sst_dev_remove(struct auxiliary_device *auxdev);
>>> +#endif
>>> diff --git a/include/uapi/linux/isst_if.h
>>> b/include/uapi/linux/isst_if.h
>>> index ba078f8e9add..bf32d959f6e8 100644
>>> --- a/include/uapi/linux/isst_if.h
>>> +++ b/include/uapi/linux/isst_if.h
>>> @@ -163,10 +163,28 @@ struct isst_if_msr_cmds {
>>>         struct isst_if_msr_cmd msr_cmd[1];
>>>  };
>>>  
>>> +/**
>>> + * struct isst_tpmi_instance_count - Get number of TPMI instances
>>> per socket
>>> + * @socket_id: Socket/package id
>>> + * @count:     Number of instances
>>> + * @valid_mask: Mask of instances as there can be holes
>>> + *
>>> + * Structure used to get TPMI instances information using
>>> + * IOCTL ISST_IF_COUNT_TPMI_INSTANCES.
>>> + */
>>> +struct isst_tpmi_instance_count {
>>> +       __u8 socket_id;
>>> +       __u8 count;
>>> +       __u16 valid_mask;
>>> +};
>>> +
>>>  #define ISST_IF_MAGIC                  0xFE
>>>  #define ISST_IF_GET_PLATFORM_INFO      _IOR(ISST_IF_MAGIC, 0,
>>> struct isst_if_platform_info *)
>>>  #define ISST_IF_GET_PHY_ID             _IOWR(ISST_IF_MAGIC, 1,
>>> struct isst_if_cpu_map *)
>>>  #define ISST_IF_IO_CMD         _IOW(ISST_IF_MAGIC, 2, struct
>>> isst_if_io_regs *)
>>>  #define ISST_IF_MBOX_COMMAND   _IOWR(ISST_IF_MAGIC, 3, struct
>>> isst_if_mbox_cmds *)
>>>  #define ISST_IF_MSR_COMMAND    _IOWR(ISST_IF_MAGIC, 4, struct
>>> isst_if_msr_cmds *)
>>> +
>>> +#define ISST_IF_COUNT_TPMI_INSTANCES   _IOR(ISST_IF_MAGIC, 5,
>>> struct isst_tpmi_instance_count *)
>>> +
>>>  #endif
>>
> 




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux