Message-ID: <b9b04ca19064a87102e180084e5eb6b613cb3ea1.camel@linux.intel.com>
Date:   Wed, 01 Mar 2023 06:45:08 -0800
From:   srinivas pandruvada <srinivas.pandruvada@...ux.intel.com>
To:     Hans de Goede <hdegoede@...hat.com>, markgross@...nel.org
Cc:     platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 06/12] platform/x86: ISST: Enumerate TPMI SST and create
 framework

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@...ux.intel.com>
> 
> Thanks, patch looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@...hat.com>
> 
> 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.


> 
> 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.

Thanks,
Srinivas


> 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
> 

Powered by blists - more mailing lists