diff mbox series

[v2,10/15] peci: Add support for PECI device drivers

Message ID 20210803113134.2262882-11-iwona.winiarska@intel.com (mailing list archive)
State New, archived
Headers show
Series Introduce PECI subsystem | expand

Commit Message

Winiarska, Iwona Aug. 3, 2021, 11:31 a.m. UTC
Here we're adding support for PECI device drivers, which unlike PECI
controller drivers are actually able to provide functionalities to
userspace.

We're also extending peci_request API to allow querying more details
about PECI device (e.g. model/family), that's going to be used to find
a compatible peci_driver.

Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/peci/Kconfig    |   1 +
 drivers/peci/core.c     |  49 +++++++++
 drivers/peci/device.c   | 105 ++++++++++++++++++++
 drivers/peci/internal.h |  75 ++++++++++++++
 drivers/peci/request.c  | 214 ++++++++++++++++++++++++++++++++++++++++
 include/linux/peci.h    |  19 ++++
 lib/Kconfig             |   2 +-
 7 files changed, 464 insertions(+), 1 deletion(-)

Comments

Dan Williams Aug. 27, 2021, 9:19 p.m. UTC | #1
On Tue, Aug 3, 2021 at 4:36 AM Iwona Winiarska
<iwona.winiarska@intel.com> wrote:
>
> Here we're adding support for PECI device drivers, which unlike PECI

s/Here we're adding/Add/

> controller drivers are actually able to provide functionalities to
> userspace.

>
> We're also extending peci_request API to allow querying more details

s/We're also extending/Also, extend/

...for the most part imperative tense is the preferred tense, by
upstream maintainers, for changelogs.

> about PECI device (e.g. model/family), that's going to be used to find
> a compatible peci_driver.
>
> Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/peci/Kconfig    |   1 +
>  drivers/peci/core.c     |  49 +++++++++
>  drivers/peci/device.c   | 105 ++++++++++++++++++++
>  drivers/peci/internal.h |  75 ++++++++++++++
>  drivers/peci/request.c  | 214 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/peci.h    |  19 ++++
>  lib/Kconfig             |   2 +-
>  7 files changed, 464 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
> index 99279df97a78..1d0532e3a801 100644
> --- a/drivers/peci/Kconfig
> +++ b/drivers/peci/Kconfig
> @@ -2,6 +2,7 @@
>
>  menuconfig PECI
>         tristate "PECI support"
> +       select GENERIC_LIB_X86

GENERIC_LIB_X86 has dependencies, so this 'select' will make kbuild
unhappy when that dependency is not met. Given that this symbol
already selected by X86, it seems this just wants a "depends on
GENERIC_LIB_X86".

>         help
>           The Platform Environment Control Interface (PECI) is an interface
>           that provides a communication channel to Intel processors and
> diff --git a/drivers/peci/core.c b/drivers/peci/core.c
> index c473acb3c2a0..33c07920493d 100644
> --- a/drivers/peci/core.c
> +++ b/drivers/peci/core.c
> @@ -157,8 +157,57 @@ struct peci_controller *devm_peci_controller_add(struct device *dev,
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_peci_controller_add, PECI);
>
> +static const struct peci_device_id *
> +peci_bus_match_device_id(const struct peci_device_id *id, struct peci_device *device)
> +{
> +       while (id->family != 0) {
> +               if (id->family == device->info.family &&
> +                   id->model == device->info.model)
> +                       return id;
> +               id++;
> +       }
> +
> +       return NULL;
> +}
> +
> +static int peci_bus_device_match(struct device *dev, struct device_driver *drv)
> +{
> +       struct peci_device *device = to_peci_device(dev);
> +       struct peci_driver *peci_drv = to_peci_driver(drv);
> +
> +       if (dev->type != &peci_device_type)
> +               return 0;
> +
> +       if (peci_bus_match_device_id(peci_drv->id_table, device))
> +               return 1;

Save a couple lines and do:

    return peci_bus_match_device_id(...)

> +
> +       return 0;
> +}
> +
> +static int peci_bus_device_probe(struct device *dev)
> +{
> +       struct peci_device *device = to_peci_device(dev);
> +       struct peci_driver *driver = to_peci_driver(dev->driver);
> +
> +       return driver->probe(device, peci_bus_match_device_id(driver->id_table, device));
> +}
> +
> +static int peci_bus_device_remove(struct device *dev)

Note, in linux-next this prototype has changed to:

    void (*remove)(struct device *dev);

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/include/linux/device/bus.h


> +{
> +       struct peci_device *device = to_peci_device(dev);
> +       struct peci_driver *driver = to_peci_driver(dev->driver);
> +
> +       if (driver->remove)
> +               driver->remove(device);
> +
> +       return 0;
> +}
> +
>  struct bus_type peci_bus_type = {
>         .name           = "peci",
> +       .match          = peci_bus_device_match,
> +       .probe          = peci_bus_device_probe,
> +       .remove         = peci_bus_device_remove,
>         .bus_groups     = peci_bus_groups,
>  };
>
> diff --git a/drivers/peci/device.c b/drivers/peci/device.c
> index d77d9dabd51e..a78c02399574 100644
> --- a/drivers/peci/device.c
> +++ b/drivers/peci/device.c
> @@ -1,11 +1,85 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  // Copyright (c) 2018-2021 Intel Corporation
>
> +#include <linux/bitfield.h>
>  #include <linux/peci.h>
>  #include <linux/slab.h>
> +#include <linux/x86/cpu.h>
>
>  #include "internal.h"
>
> +#define REVISION_NUM_MASK GENMASK(15, 8)
> +static int peci_get_revision(struct peci_device *device, u8 *revision)
> +{
> +       struct peci_request *req;
> +       u64 dib;
> +
> +       req = peci_get_dib(device);

I would expect peci_get_dib() to return @dib.

> +       if (IS_ERR(req))
> +               return PTR_ERR(req);
> +
> +       /*
> +        * PECI device may be in a state where it is unable to return a proper
> +        * DIB, in which case it returns 0 as DIB value.
> +        * Let's treat this as an error to avoid carrying on with the detection
> +        * using invalid revision.
> +        */
> +       dib = peci_request_data_dib(req);

I would expect peci_request_data_dib() to make a request.

A stack allocated peci_request passed to peci_get_dib() that returns
an error code would seem to be cleaner than this current organization.

> +       if (dib == 0) {
> +               peci_request_free(req);
> +               return -EIO;
> +       }
> +
> +       *revision = FIELD_GET(REVISION_NUM_MASK, dib);
> +
> +       peci_request_free(req);
> +
> +       return 0;
> +}
> +
> +static int peci_get_cpu_id(struct peci_device *device, u32 *cpu_id)
> +{
> +       struct peci_request *req;
> +       int ret;
> +
> +       req = peci_pkg_cfg_readl(device, PECI_PCS_PKG_ID, PECI_PKG_ID_CPU_ID);
> +       if (IS_ERR(req))
> +               return PTR_ERR(req);
> +
> +       ret = peci_request_status(req);
> +       if (ret)
> +               goto out_req_free;
> +
> +       *cpu_id = peci_request_data_readl(req);
> +out_req_free:
> +       peci_request_free(req);
> +
> +       return ret;
> +}
> +
> +static int peci_device_info_init(struct peci_device *device)
> +{
> +       u8 revision;
> +       u32 cpu_id;
> +       int ret;
> +
> +       ret = peci_get_cpu_id(device, &cpu_id);
> +       if (ret)
> +               return ret;
> +
> +       device->info.family = x86_family(cpu_id);
> +       device->info.model = x86_model(cpu_id);
> +
> +       ret = peci_get_revision(device, &revision);
> +       if (ret)
> +               return ret;
> +       device->info.peci_revision = revision;
> +
> +       device->info.socket_id = device->addr - PECI_BASE_ADDR;
> +
> +       return 0;
> +}
> +
>  static int peci_detect(struct peci_controller *controller, u8 addr)
>  {
>         struct peci_request *req;
> @@ -79,6 +153,10 @@ int peci_device_create(struct peci_controller *controller, u8 addr)
>         device->dev.bus = &peci_bus_type;
>         device->dev.type = &peci_device_type;
>
> +       ret = peci_device_info_init(device);
> +       if (ret)
> +               goto err_free;
> +
>         ret = dev_set_name(&device->dev, "%d-%02x", controller->id, device->addr);
>         if (ret)
>                 goto err_free;
> @@ -102,6 +180,33 @@ void peci_device_destroy(struct peci_device *device)
>         device_unregister(&device->dev);
>  }
>
> +int __peci_driver_register(struct peci_driver *driver, struct module *owner,
> +                          const char *mod_name)
> +{
> +       driver->driver.bus = &peci_bus_type;
> +       driver->driver.owner = owner;
> +       driver->driver.mod_name = mod_name;
> +
> +       if (!driver->probe) {
> +               pr_err("peci: trying to register driver without probe callback\n");
> +               return -EINVAL;
> +       }
> +
> +       if (!driver->id_table) {
> +               pr_err("peci: trying to register driver without device id table\n");
> +               return -EINVAL;
> +       }
> +
> +       return driver_register(&driver->driver);
> +}
> +EXPORT_SYMBOL_NS_GPL(__peci_driver_register, PECI);
> +
> +void peci_driver_unregister(struct peci_driver *driver)
> +{
> +       driver_unregister(&driver->driver);
> +}
> +EXPORT_SYMBOL_NS_GPL(peci_driver_unregister, PECI);
> +
>  static void peci_device_release(struct device *dev)
>  {
>         struct peci_device *device = to_peci_device(dev);
> diff --git a/drivers/peci/internal.h b/drivers/peci/internal.h
> index 978e12c8e1d3..d661e1b65694 100644
> --- a/drivers/peci/internal.h
> +++ b/drivers/peci/internal.h
> @@ -19,6 +19,34 @@ struct peci_request;
>  struct peci_request *peci_request_alloc(struct peci_device *device, u8 tx_len, u8 rx_len);
>  void peci_request_free(struct peci_request *req);
>
> +int peci_request_status(struct peci_request *req);
> +u64 peci_request_data_dib(struct peci_request *req);
> +
> +u8 peci_request_data_readb(struct peci_request *req);
> +u16 peci_request_data_readw(struct peci_request *req);
> +u32 peci_request_data_readl(struct peci_request *req);
> +u64 peci_request_data_readq(struct peci_request *req);
> +
> +struct peci_request *peci_get_dib(struct peci_device *device);
> +struct peci_request *peci_get_temp(struct peci_device *device);
> +
> +struct peci_request *peci_pkg_cfg_readb(struct peci_device *device, u8 index, u16 param);
> +struct peci_request *peci_pkg_cfg_readw(struct peci_device *device, u8 index, u16 param);
> +struct peci_request *peci_pkg_cfg_readl(struct peci_device *device, u8 index, u16 param);
> +struct peci_request *peci_pkg_cfg_readq(struct peci_device *device, u8 index, u16 param);
> +
> +/**
> + * struct peci_device_id - PECI device data to match
> + * @data: pointer to driver private data specific to device
> + * @family: device family
> + * @model: device model
> + */
> +struct peci_device_id {
> +       const void *data;
> +       u16 family;
> +       u8 model;
> +};
> +
>  extern struct device_type peci_device_type;
>  extern const struct attribute_group *peci_device_groups[];
>
> @@ -28,6 +56,53 @@ void peci_device_destroy(struct peci_device *device);
>  extern struct bus_type peci_bus_type;
>  extern const struct attribute_group *peci_bus_groups[];
>
> +/**
> + * struct peci_driver - PECI driver
> + * @driver: inherit device driver
> + * @probe: probe callback
> + * @remove: remove callback
> + * @id_table: PECI device match table to decide which device to bind
> + */
> +struct peci_driver {
> +       struct device_driver driver;
> +       int (*probe)(struct peci_device *device, const struct peci_device_id *id);
> +       void (*remove)(struct peci_device *device);
> +       const struct peci_device_id *id_table;
> +};
> +
> +static inline struct peci_driver *to_peci_driver(struct device_driver *d)
> +{
> +       return container_of(d, struct peci_driver, driver);
> +}
> +
> +int __peci_driver_register(struct peci_driver *driver, struct module *owner,
> +                          const char *mod_name);
> +/**
> + * peci_driver_register() - register PECI driver
> + * @driver: the driver to be registered
> + * @owner: owner module of the driver being registered
> + * @mod_name: module name string
> + *
> + * PECI drivers that don't need to do anything special in module init should
> + * use the convenience "module_peci_driver" macro instead
> + *
> + * Return: zero on success, else a negative error code.
> + */
> +#define peci_driver_register(driver) \
> +       __peci_driver_register(driver, THIS_MODULE, KBUILD_MODNAME)
> +void peci_driver_unregister(struct peci_driver *driver);
> +
> +/**
> + * module_peci_driver() - helper macro for registering a modular PECI driver
> + * @__peci_driver: peci_driver struct
> + *
> + * Helper macro for PECI drivers which do not do anything special in module
> + * init/exit. This eliminates a lot of boilerplate. Each module may only
> + * use this macro once, and calling it replaces module_init() and module_exit()
> + */
> +#define module_peci_driver(__peci_driver) \
> +       module_driver(__peci_driver, peci_driver_register, peci_driver_unregister)
> +
>  extern struct device_type peci_controller_type;
>
>  int peci_controller_scan_devices(struct peci_controller *controller);
> diff --git a/drivers/peci/request.c b/drivers/peci/request.c
> index 81b567bc7b87..fe032d5a5e1b 100644
> --- a/drivers/peci/request.c
> +++ b/drivers/peci/request.c
> @@ -1,13 +1,140 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  // Copyright (c) 2021 Intel Corporation
>
> +#include <linux/bug.h>
>  #include <linux/export.h>
>  #include <linux/peci.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>
> +#include <asm/unaligned.h>
> +
>  #include "internal.h"
>
> +#define PECI_GET_DIB_CMD               0xf7
> +#define  PECI_GET_DIB_WR_LEN           1
> +#define  PECI_GET_DIB_RD_LEN           8
> +
> +#define PECI_RDPKGCFG_CMD              0xa1
> +#define  PECI_RDPKGCFG_WR_LEN          5
> +#define  PECI_RDPKGCFG_RD_LEN_BASE     1
> +#define PECI_WRPKGCFG_CMD              0xa5
> +#define  PECI_WRPKGCFG_WR_LEN_BASE     6
> +#define  PECI_WRPKGCFG_RD_LEN          1
> +
> +/* Device Specific Completion Code (CC) Definition */
> +#define PECI_CC_SUCCESS                                0x40
> +#define PECI_CC_NEED_RETRY                     0x80
> +#define PECI_CC_OUT_OF_RESOURCE                        0x81
> +#define PECI_CC_UNAVAIL_RESOURCE               0x82
> +#define PECI_CC_INVALID_REQ                    0x90
> +#define PECI_CC_MCA_ERROR                      0x91
> +#define PECI_CC_CATASTROPHIC_MCA_ERROR         0x93
> +#define PECI_CC_FATAL_MCA_ERROR                        0x94
> +#define PECI_CC_PARITY_ERR_GPSB_OR_PMSB                0x98
> +#define PECI_CC_PARITY_ERR_GPSB_OR_PMSB_IERR   0x9B
> +#define PECI_CC_PARITY_ERR_GPSB_OR_PMSB_MCA    0x9C
> +
> +#define PECI_RETRY_BIT                 BIT(0)
> +
> +#define PECI_RETRY_TIMEOUT             msecs_to_jiffies(700)
> +#define PECI_RETRY_INTERVAL_MIN                msecs_to_jiffies(1)
> +#define PECI_RETRY_INTERVAL_MAX                msecs_to_jiffies(128)
> +
> +static u8 peci_request_data_cc(struct peci_request *req)
> +{
> +       return req->rx.buf[0];
> +}
> +
> +/**
> + * peci_request_status() - return -errno based on PECI completion code
> + * @req: the PECI request that contains response data with completion code
> + *
> + * It can't be used for Ping(), GetDIB() and GetTemp() - for those commands we
> + * don't expect completion code in the response.
> + *
> + * Return: -errno
> + */
> +int peci_request_status(struct peci_request *req)
> +{
> +       u8 cc = peci_request_data_cc(req);
> +
> +       if (cc != PECI_CC_SUCCESS)
> +               dev_dbg(&req->device->dev, "ret: %#02x\n", cc);
> +
> +       switch (cc) {
> +       case PECI_CC_SUCCESS:
> +               return 0;
> +       case PECI_CC_NEED_RETRY:
> +       case PECI_CC_OUT_OF_RESOURCE:
> +       case PECI_CC_UNAVAIL_RESOURCE:
> +               return -EAGAIN;
> +       case PECI_CC_INVALID_REQ:
> +               return -EINVAL;
> +       case PECI_CC_MCA_ERROR:
> +       case PECI_CC_CATASTROPHIC_MCA_ERROR:
> +       case PECI_CC_FATAL_MCA_ERROR:
> +       case PECI_CC_PARITY_ERR_GPSB_OR_PMSB:
> +       case PECI_CC_PARITY_ERR_GPSB_OR_PMSB_IERR:
> +       case PECI_CC_PARITY_ERR_GPSB_OR_PMSB_MCA:
> +               return -EIO;
> +       }
> +
> +       WARN_ONCE(1, "Unknown PECI completion code: %#02x\n", cc);
> +
> +       return -EIO;
> +}
> +EXPORT_SYMBOL_NS_GPL(peci_request_status, PECI);
> +
> +static int peci_request_xfer(struct peci_request *req)
> +{
> +       struct peci_device *device = req->device;
> +       struct peci_controller *controller = to_peci_controller(device->dev.parent);
> +       int ret;
> +
> +       mutex_lock(&controller->bus_lock);
> +       ret = controller->ops->xfer(controller, device->addr, req);
> +       mutex_unlock(&controller->bus_lock);
> +
> +       return ret;
> +}
> +
> +static int peci_request_xfer_retry(struct peci_request *req)
> +{
> +       long wait_interval = PECI_RETRY_INTERVAL_MIN;
> +       struct peci_device *device = req->device;
> +       struct peci_controller *controller = to_peci_controller(device->dev.parent);
> +       unsigned long start = jiffies;
> +       int ret;
> +
> +       /* Don't try to use it for ping */
> +       if (WARN_ON(!req->rx.buf))
> +               return 0;
> +
> +       do {
> +               ret = peci_request_xfer(req);
> +               if (ret) {
> +                       dev_dbg(&controller->dev, "xfer error: %d\n", ret);
> +                       return ret;
> +               }
> +
> +               if (peci_request_status(req) != -EAGAIN)
> +                       return 0;
> +
> +               /* Set the retry bit to indicate a retry attempt */
> +               req->tx.buf[1] |= PECI_RETRY_BIT;
> +
> +               if (schedule_timeout_interruptible(wait_interval))
> +                       return -ERESTARTSYS;
> +
> +               wait_interval = min_t(long, wait_interval * 2, PECI_RETRY_INTERVAL_MAX);
> +       } while (time_before(jiffies, start + PECI_RETRY_TIMEOUT));
> +
> +       dev_dbg(&controller->dev, "request timed out\n");
> +
> +       return -ETIMEDOUT;
> +}
> +
>  /**
>   * peci_request_alloc() - allocate &struct peci_requests
>   * @device: PECI device to which request is going to be sent
> @@ -48,3 +175,90 @@ void peci_request_free(struct peci_request *req)
>         kfree(req);
>  }
>  EXPORT_SYMBOL_NS_GPL(peci_request_free, PECI);
> +
> +struct peci_request *peci_get_dib(struct peci_device *device)
> +{
> +       struct peci_request *req;
> +       int ret;
> +
> +       req = peci_request_alloc(device, PECI_GET_DIB_WR_LEN, PECI_GET_DIB_RD_LEN);
> +       if (!req)
> +               return ERR_PTR(-ENOMEM);
> +
> +       req->tx.buf[0] = PECI_GET_DIB_CMD;
> +
> +       ret = peci_request_xfer(req);
> +       if (ret) {
> +               peci_request_free(req);
> +               return ERR_PTR(ret);
> +       }
> +
> +       return req;
> +}
> +EXPORT_SYMBOL_NS_GPL(peci_get_dib, PECI);
> +
> +static struct peci_request *
> +__pkg_cfg_read(struct peci_device *device, u8 index, u16 param, u8 len)
> +{
> +       struct peci_request *req;
> +       int ret;
> +
> +       req = peci_request_alloc(device, PECI_RDPKGCFG_WR_LEN, PECI_RDPKGCFG_RD_LEN_BASE + len);
> +       if (!req)
> +               return ERR_PTR(-ENOMEM);
> +
> +       req->tx.buf[0] = PECI_RDPKGCFG_CMD;
> +       req->tx.buf[1] = 0;
> +       req->tx.buf[2] = index;
> +       put_unaligned_le16(param, &req->tx.buf[3]);
> +
> +       ret = peci_request_xfer_retry(req);
> +       if (ret) {
> +               peci_request_free(req);
> +               return ERR_PTR(ret);
> +       }
> +
> +       return req;
> +}
> +
> +u8 peci_request_data_readb(struct peci_request *req)
> +{
> +       return req->rx.buf[1];
> +}
> +EXPORT_SYMBOL_NS_GPL(peci_request_data_readb, PECI);
> +
> +u16 peci_request_data_readw(struct peci_request *req)
> +{
> +       return get_unaligned_le16(&req->rx.buf[1]);
> +}
> +EXPORT_SYMBOL_NS_GPL(peci_request_data_readw, PECI);
> +
> +u32 peci_request_data_readl(struct peci_request *req)
> +{
> +       return get_unaligned_le32(&req->rx.buf[1]);
> +}
> +EXPORT_SYMBOL_NS_GPL(peci_request_data_readl, PECI);
> +
> +u64 peci_request_data_readq(struct peci_request *req)
> +{
> +       return get_unaligned_le64(&req->rx.buf[1]);
> +}
> +EXPORT_SYMBOL_NS_GPL(peci_request_data_readq, PECI);
> +
> +u64 peci_request_data_dib(struct peci_request *req)
> +{
> +       return get_unaligned_le64(&req->rx.buf[0]);
> +}
> +EXPORT_SYMBOL_NS_GPL(peci_request_data_dib, PECI);
> +
> +#define __read_pkg_config(x, type) \
> +struct peci_request *peci_pkg_cfg_##x(struct peci_device *device, u8 index, u16 param) \
> +{ \
> +       return __pkg_cfg_read(device, index, param, sizeof(type)); \
> +} \
> +EXPORT_SYMBOL_NS_GPL(peci_pkg_cfg_##x, PECI)
> +
> +__read_pkg_config(readb, u8);
> +__read_pkg_config(readw, u16);
> +__read_pkg_config(readl, u32);
> +__read_pkg_config(readq, u64);
> diff --git a/include/linux/peci.h b/include/linux/peci.h
> index 26e0a4e73b50..dcf1c53f4e40 100644
> --- a/include/linux/peci.h
> +++ b/include/linux/peci.h
> @@ -14,6 +14,14 @@
>   */
>  #define PECI_REQUEST_MAX_BUF_SIZE 32
>
> +#define PECI_PCS_PKG_ID                        0  /* Package Identifier Read */
> +#define  PECI_PKG_ID_CPU_ID            0x0000  /* CPUID Info */
> +#define  PECI_PKG_ID_PLATFORM_ID       0x0001  /* Platform ID */
> +#define  PECI_PKG_ID_DEVICE_ID         0x0002  /* Uncore Device ID */
> +#define  PECI_PKG_ID_MAX_THREAD_ID     0x0003  /* Max Thread ID */
> +#define  PECI_PKG_ID_MICROCODE_REV     0x0004  /* CPU Microcode Update Revision */
> +#define  PECI_PKG_ID_MCA_ERROR_LOG     0x0005  /* Machine Check Status */
> +
>  struct peci_controller;
>  struct peci_request;
>
> @@ -59,6 +67,11 @@ static inline struct peci_controller *to_peci_controller(void *d)
>   * struct peci_device - PECI device
>   * @dev: device object to register PECI device to the device model
>   * @controller: manages the bus segment hosting this PECI device
> + * @info: PECI device characteristics
> + * @info.family: device family
> + * @info.model: device model
> + * @info.peci_revision: PECI revision supported by the PECI device
> + * @info.socket_id: the socket ID represented by the PECI device
>   * @addr: address used on the PECI bus connected to the parent controller
>   *
>   * A peci_device identifies a single device (i.e. CPU) connected to a PECI bus.
> @@ -67,6 +80,12 @@ static inline struct peci_controller *to_peci_controller(void *d)
>   */
>  struct peci_device {
>         struct device dev;
> +       struct {
> +               u16 family;
> +               u8 model;
> +               u8 peci_revision;
> +               u8 socket_id;
> +       } info;
>         u8 addr;
>  };
>
> diff --git a/lib/Kconfig b/lib/Kconfig
> index e538d4d773bd..7f7972d357c2 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -718,4 +718,4 @@ config ASN1_ENCODER
>
>  config GENERIC_LIB_X86
>         bool
> -       depends on X86
> +       depends on X86 || PECI

This looks broken, what in the GENERIC_LIB_X86 implementation depends on peci?
Winiarska, Iwona Nov. 15, 2021, 10:20 p.m. UTC | #2
On Fri, 2021-08-27 at 14:19 -0700, Dan Williams wrote:
> On Tue, Aug 3, 2021 at 4:36 AM Iwona Winiarska
> <iwona.winiarska@intel.com> wrote:
> > 
> > Here we're adding support for PECI device drivers, which unlike PECI
> 
> s/Here we're adding/Add/
> 
> > controller drivers are actually able to provide functionalities to
> > userspace.

Done.

> 
> > 
> > We're also extending peci_request API to allow querying more details
> 
> s/We're also extending/Also, extend/

Done.

> 
> ...for the most part imperative tense is the preferred tense, by
> upstream maintainers, for changelogs.
> 
> > about PECI device (e.g. model/family), that's going to be used to find
> > a compatible peci_driver.
> > 
> > Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
> > Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > ---
> >  drivers/peci/Kconfig    |   1 +
> >  drivers/peci/core.c     |  49 +++++++++
> >  drivers/peci/device.c   | 105 ++++++++++++++++++++
> >  drivers/peci/internal.h |  75 ++++++++++++++
> >  drivers/peci/request.c  | 214 ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/peci.h    |  19 ++++
> >  lib/Kconfig             |   2 +-
> >  7 files changed, 464 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
> > index 99279df97a78..1d0532e3a801 100644
> > --- a/drivers/peci/Kconfig
> > +++ b/drivers/peci/Kconfig
> > @@ -2,6 +2,7 @@
> > 
> >  menuconfig PECI
> >         tristate "PECI support"
> > +       select GENERIC_LIB_X86
> 
> GENERIC_LIB_X86 has dependencies, so this 'select' will make kbuild
> unhappy when that dependency is not met. Given that this symbol
> already selected by X86, it seems this just wants a "depends on
> GENERIC_LIB_X86".

Not applicable anymore after patches 1 and 2 got dropped following feedback from
arch/x86.

> 
> >         help
> >           The Platform Environment Control Interface (PECI) is an interface
> >           that provides a communication channel to Intel processors and
> > diff --git a/drivers/peci/core.c b/drivers/peci/core.c
> > index c473acb3c2a0..33c07920493d 100644
> > --- a/drivers/peci/core.c
> > +++ b/drivers/peci/core.c
> > @@ -157,8 +157,57 @@ struct peci_controller *devm_peci_controller_add(struct
> > device *dev,
> >  }
> >  EXPORT_SYMBOL_NS_GPL(devm_peci_controller_add, PECI);
> > 
> > +static const struct peci_device_id *
> > +peci_bus_match_device_id(const struct peci_device_id *id, struct
> > peci_device *device)
> > +{
> > +       while (id->family != 0) {
> > +               if (id->family == device->info.family &&
> > +                   id->model == device->info.model)
> > +                       return id;
> > +               id++;
> > +       }
> > +
> > +       return NULL;
> > +}
> > +
> > +static int peci_bus_device_match(struct device *dev, struct device_driver
> > *drv)
> > +{
> > +       struct peci_device *device = to_peci_device(dev);
> > +       struct peci_driver *peci_drv = to_peci_driver(drv);
> > +
> > +       if (dev->type != &peci_device_type)
> > +               return 0;
> > +
> > +       if (peci_bus_match_device_id(peci_drv->id_table, device))
> > +               return 1;
> 
> Save a couple lines and do:
> 
>     return peci_bus_match_device_id(...)

Done.

> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int peci_bus_device_probe(struct device *dev)
> > +{
> > +       struct peci_device *device = to_peci_device(dev);
> > +       struct peci_driver *driver = to_peci_driver(dev->driver);
> > +
> > +       return driver->probe(device, peci_bus_match_device_id(driver-
> > >id_table, device));
> > +}
> > +
> > +static int peci_bus_device_remove(struct device *dev)
> 
> Note, in linux-next this prototype has changed to:
> 
>     void (*remove)(struct device *dev);
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/include/linux/device/bus.h
> 
> 
> > +{
> > +       struct peci_device *device = to_peci_device(dev);
> > +       struct peci_driver *driver = to_peci_driver(dev->driver);
> > +
> > +       if (driver->remove)
> > +               driver->remove(device);
> > +
> > +       return 0;
> > +}
> > +
> >  struct bus_type peci_bus_type = {
> >         .name           = "peci",
> > +       .match          = peci_bus_device_match,
> > +       .probe          = peci_bus_device_probe,
> > +       .remove         = peci_bus_device_remove,
> >         .bus_groups     = peci_bus_groups,
> >  };
> > 
> > diff --git a/drivers/peci/device.c b/drivers/peci/device.c
> > index d77d9dabd51e..a78c02399574 100644
> > --- a/drivers/peci/device.c
> > +++ b/drivers/peci/device.c
> > @@ -1,11 +1,85 @@
> >  // SPDX-License-Identifier: GPL-2.0-only
> >  // Copyright (c) 2018-2021 Intel Corporation
> > 
> > +#include <linux/bitfield.h>
> >  #include <linux/peci.h>
> >  #include <linux/slab.h>
> > +#include <linux/x86/cpu.h>
> > 
> >  #include "internal.h"
> > 
> > +#define REVISION_NUM_MASK GENMASK(15, 8)
> > +static int peci_get_revision(struct peci_device *device, u8 *revision)
> > +{
> > +       struct peci_request *req;
> > +       u64 dib;
> > +
> > +       req = peci_get_dib(device);
> 
> I would expect peci_get_dib() to return @dib.

get_dib is a PECI command name - I changed the naming scheme slightly to make
things more clear, e.g. peci_xfer_get_dib().

> 
> > +       if (IS_ERR(req))
> > +               return PTR_ERR(req);
> > +
> > +       /*
> > +        * PECI device may be in a state where it is unable to return a
> > proper
> > +        * DIB, in which case it returns 0 as DIB value.
> > +        * Let's treat this as an error to avoid carrying on with the
> > detection
> > +        * using invalid revision.
> > +        */
> > +       dib = peci_request_data_dib(req);
> 
> I would expect peci_request_data_dib() to make a request.

Changed peci_request_data_dib to peci_request_dib_read and
peci_request_data_temp to peci_request_temp_read to align with
peci_request_data_read*.

> 
> A stack allocated peci_request passed to peci_get_dib() that returns
> an error code would seem to be cleaner than this current organization.
> 
> > +       if (dib == 0) {
> > +               peci_request_free(req);
> > +               return -EIO;
> > +       }
> > +
> > +       *revision = FIELD_GET(REVISION_NUM_MASK, dib);
> > +
> > +       peci_request_free(req);
> > +
> > +       return 0;
> > +}
> > +
> > +static int peci_get_cpu_id(struct peci_device *device, u32 *cpu_id)
> > +{
> > +       struct peci_request *req;
> > +       int ret;
> > +
> > +       req = peci_pkg_cfg_readl(device, PECI_PCS_PKG_ID,
> > PECI_PKG_ID_CPU_ID);
> > +       if (IS_ERR(req))
> > +               return PTR_ERR(req);
> > +
> > +       ret = peci_request_status(req);
> > +       if (ret)
> > +               goto out_req_free;
> > +
> > +       *cpu_id = peci_request_data_readl(req);
> > +out_req_free:
> > +       peci_request_free(req);
> > +
> > +       return ret;
> > +}
> > +
> > +static int peci_device_info_init(struct peci_device *device)
> > +{
> > +       u8 revision;
> > +       u32 cpu_id;
> > +       int ret;
> > +
> > +       ret = peci_get_cpu_id(device, &cpu_id);
> > +       if (ret)
> > +               return ret;
> > +
> > +       device->info.family = x86_family(cpu_id);
> > +       device->info.model = x86_model(cpu_id);
> > +
> > +       ret = peci_get_revision(device, &revision);
> > +       if (ret)
> > +               return ret;
> > +       device->info.peci_revision = revision;
> > +
> > +       device->info.socket_id = device->addr - PECI_BASE_ADDR;
> > +
> > +       return 0;
> > +}
> > +
> >  static int peci_detect(struct peci_controller *controller, u8 addr)
> >  {
> >         struct peci_request *req;
> > @@ -79,6 +153,10 @@ int peci_device_create(struct peci_controller
> > *controller, u8 addr)
> >         device->dev.bus = &peci_bus_type;
> >         device->dev.type = &peci_device_type;
> > 
> > +       ret = peci_device_info_init(device);
> > +       if (ret)
> > +               goto err_free;
> > +
> >         ret = dev_set_name(&device->dev, "%d-%02x", controller->id, device-
> > >addr);
> >         if (ret)
> >                 goto err_free;
> > @@ -102,6 +180,33 @@ void peci_device_destroy(struct peci_device *device)
> >         device_unregister(&device->dev);
> >  }
> > 
> > +int __peci_driver_register(struct peci_driver *driver, struct module
> > *owner,
> > +                          const char *mod_name)
> > +{
> > +       driver->driver.bus = &peci_bus_type;
> > +       driver->driver.owner = owner;
> > +       driver->driver.mod_name = mod_name;
> > +
> > +       if (!driver->probe) {
> > +               pr_err("peci: trying to register driver without probe
> > callback\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (!driver->id_table) {
> > +               pr_err("peci: trying to register driver without device id
> > table\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       return driver_register(&driver->driver);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(__peci_driver_register, PECI);
> > +
> > +void peci_driver_unregister(struct peci_driver *driver)
> > +{
> > +       driver_unregister(&driver->driver);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(peci_driver_unregister, PECI);
> > +
> >  static void peci_device_release(struct device *dev)
> >  {
> >         struct peci_device *device = to_peci_device(dev);
> > diff --git a/drivers/peci/internal.h b/drivers/peci/internal.h
> > index 978e12c8e1d3..d661e1b65694 100644
> > --- a/drivers/peci/internal.h
> > +++ b/drivers/peci/internal.h
> > @@ -19,6 +19,34 @@ struct peci_request;
> >  struct peci_request *peci_request_alloc(struct peci_device *device, u8
> > tx_len, u8 rx_len);
> >  void peci_request_free(struct peci_request *req);
> > 
> > +int peci_request_status(struct peci_request *req);
> > +u64 peci_request_data_dib(struct peci_request *req);
> > +
> > +u8 peci_request_data_readb(struct peci_request *req);
> > +u16 peci_request_data_readw(struct peci_request *req);
> > +u32 peci_request_data_readl(struct peci_request *req);
> > +u64 peci_request_data_readq(struct peci_request *req);
> > +
> > +struct peci_request *peci_get_dib(struct peci_device *device);
> > +struct peci_request *peci_get_temp(struct peci_device *device);
> > +
> > +struct peci_request *peci_pkg_cfg_readb(struct peci_device *device, u8
> > index, u16 param);
> > +struct peci_request *peci_pkg_cfg_readw(struct peci_device *device, u8
> > index, u16 param);
> > +struct peci_request *peci_pkg_cfg_readl(struct peci_device *device, u8
> > index, u16 param);
> > +struct peci_request *peci_pkg_cfg_readq(struct peci_device *device, u8
> > index, u16 param);
> > +
> > +/**
> > + * struct peci_device_id - PECI device data to match
> > + * @data: pointer to driver private data specific to device
> > + * @family: device family
> > + * @model: device model
> > + */
> > +struct peci_device_id {
> > +       const void *data;
> > +       u16 family;
> > +       u8 model;
> > +};
> > +
> >  extern struct device_type peci_device_type;
> >  extern const struct attribute_group *peci_device_groups[];
> > 
> > @@ -28,6 +56,53 @@ void peci_device_destroy(struct peci_device *device);
> >  extern struct bus_type peci_bus_type;
> >  extern const struct attribute_group *peci_bus_groups[];
> > 
> > +/**
> > + * struct peci_driver - PECI driver
> > + * @driver: inherit device driver
> > + * @probe: probe callback
> > + * @remove: remove callback
> > + * @id_table: PECI device match table to decide which device to bind
> > + */
> > +struct peci_driver {
> > +       struct device_driver driver;
> > +       int (*probe)(struct peci_device *device, const struct peci_device_id
> > *id);
> > +       void (*remove)(struct peci_device *device);
> > +       const struct peci_device_id *id_table;
> > +};
> > +
> > +static inline struct peci_driver *to_peci_driver(struct device_driver *d)
> > +{
> > +       return container_of(d, struct peci_driver, driver);
> > +}
> > +
> > +int __peci_driver_register(struct peci_driver *driver, struct module
> > *owner,
> > +                          const char *mod_name);
> > +/**
> > + * peci_driver_register() - register PECI driver
> > + * @driver: the driver to be registered
> > + * @owner: owner module of the driver being registered
> > + * @mod_name: module name string
> > + *
> > + * PECI drivers that don't need to do anything special in module init
> > should
> > + * use the convenience "module_peci_driver" macro instead
> > + *
> > + * Return: zero on success, else a negative error code.
> > + */
> > +#define peci_driver_register(driver) \
> > +       __peci_driver_register(driver, THIS_MODULE, KBUILD_MODNAME)
> > +void peci_driver_unregister(struct peci_driver *driver);
> > +
> > +/**
> > + * module_peci_driver() - helper macro for registering a modular PECI
> > driver
> > + * @__peci_driver: peci_driver struct
> > + *
> > + * Helper macro for PECI drivers which do not do anything special in module
> > + * init/exit. This eliminates a lot of boilerplate. Each module may only
> > + * use this macro once, and calling it replaces module_init() and
> > module_exit()
> > + */
> > +#define module_peci_driver(__peci_driver) \
> > +       module_driver(__peci_driver, peci_driver_register,
> > peci_driver_unregister)
> > +
> >  extern struct device_type peci_controller_type;
> > 
> >  int peci_controller_scan_devices(struct peci_controller *controller);
> > diff --git a/drivers/peci/request.c b/drivers/peci/request.c
> > index 81b567bc7b87..fe032d5a5e1b 100644
> > --- a/drivers/peci/request.c
> > +++ b/drivers/peci/request.c
> > @@ -1,13 +1,140 @@
> >  // SPDX-License-Identifier: GPL-2.0-only
> >  // Copyright (c) 2021 Intel Corporation
> > 
> > +#include <linux/bug.h>
> >  #include <linux/export.h>
> >  #include <linux/peci.h>
> >  #include <linux/slab.h>
> >  #include <linux/types.h>
> > 
> > +#include <asm/unaligned.h>
> > +
> >  #include "internal.h"
> > 
> > +#define PECI_GET_DIB_CMD               0xf7
> > +#define  PECI_GET_DIB_WR_LEN           1
> > +#define  PECI_GET_DIB_RD_LEN           8
> > +
> > +#define PECI_RDPKGCFG_CMD              0xa1
> > +#define  PECI_RDPKGCFG_WR_LEN          5
> > +#define  PECI_RDPKGCFG_RD_LEN_BASE     1
> > +#define PECI_WRPKGCFG_CMD              0xa5
> > +#define  PECI_WRPKGCFG_WR_LEN_BASE     6
> > +#define  PECI_WRPKGCFG_RD_LEN          1
> > +
> > +/* Device Specific Completion Code (CC) Definition */
> > +#define PECI_CC_SUCCESS                                0x40
> > +#define PECI_CC_NEED_RETRY                     0x80
> > +#define PECI_CC_OUT_OF_RESOURCE                        0x81
> > +#define PECI_CC_UNAVAIL_RESOURCE               0x82
> > +#define PECI_CC_INVALID_REQ                    0x90
> > +#define PECI_CC_MCA_ERROR                      0x91
> > +#define PECI_CC_CATASTROPHIC_MCA_ERROR         0x93
> > +#define PECI_CC_FATAL_MCA_ERROR                        0x94
> > +#define PECI_CC_PARITY_ERR_GPSB_OR_PMSB                0x98
> > +#define PECI_CC_PARITY_ERR_GPSB_OR_PMSB_IERR   0x9B
> > +#define PECI_CC_PARITY_ERR_GPSB_OR_PMSB_MCA    0x9C
> > +
> > +#define PECI_RETRY_BIT                 BIT(0)
> > +
> > +#define PECI_RETRY_TIMEOUT             msecs_to_jiffies(700)
> > +#define PECI_RETRY_INTERVAL_MIN                msecs_to_jiffies(1)
> > +#define PECI_RETRY_INTERVAL_MAX                msecs_to_jiffies(128)
> > +
> > +static u8 peci_request_data_cc(struct peci_request *req)
> > +{
> > +       return req->rx.buf[0];
> > +}
> > +
> > +/**
> > + * peci_request_status() - return -errno based on PECI completion code
> > + * @req: the PECI request that contains response data with completion code
> > + *
> > + * It can't be used for Ping(), GetDIB() and GetTemp() - for those commands
> > we
> > + * don't expect completion code in the response.
> > + *
> > + * Return: -errno
> > + */
> > +int peci_request_status(struct peci_request *req)
> > +{
> > +       u8 cc = peci_request_data_cc(req);
> > +
> > +       if (cc != PECI_CC_SUCCESS)
> > +               dev_dbg(&req->device->dev, "ret: %#02x\n", cc);
> > +
> > +       switch (cc) {
> > +       case PECI_CC_SUCCESS:
> > +               return 0;
> > +       case PECI_CC_NEED_RETRY:
> > +       case PECI_CC_OUT_OF_RESOURCE:
> > +       case PECI_CC_UNAVAIL_RESOURCE:
> > +               return -EAGAIN;
> > +       case PECI_CC_INVALID_REQ:
> > +               return -EINVAL;
> > +       case PECI_CC_MCA_ERROR:
> > +       case PECI_CC_CATASTROPHIC_MCA_ERROR:
> > +       case PECI_CC_FATAL_MCA_ERROR:
> > +       case PECI_CC_PARITY_ERR_GPSB_OR_PMSB:
> > +       case PECI_CC_PARITY_ERR_GPSB_OR_PMSB_IERR:
> > +       case PECI_CC_PARITY_ERR_GPSB_OR_PMSB_MCA:
> > +               return -EIO;
> > +       }
> > +
> > +       WARN_ONCE(1, "Unknown PECI completion code: %#02x\n", cc);
> > +
> > +       return -EIO;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(peci_request_status, PECI);
> > +
> > +static int peci_request_xfer(struct peci_request *req)
> > +{
> > +       struct peci_device *device = req->device;
> > +       struct peci_controller *controller = to_peci_controller(device-
> > >dev.parent);
> > +       int ret;
> > +
> > +       mutex_lock(&controller->bus_lock);
> > +       ret = controller->ops->xfer(controller, device->addr, req);
> > +       mutex_unlock(&controller->bus_lock);
> > +
> > +       return ret;
> > +}
> > +
> > +static int peci_request_xfer_retry(struct peci_request *req)
> > +{
> > +       long wait_interval = PECI_RETRY_INTERVAL_MIN;
> > +       struct peci_device *device = req->device;
> > +       struct peci_controller *controller = to_peci_controller(device-
> > >dev.parent);
> > +       unsigned long start = jiffies;
> > +       int ret;
> > +
> > +       /* Don't try to use it for ping */
> > +       if (WARN_ON(!req->rx.buf))
> > +               return 0;
> > +
> > +       do {
> > +               ret = peci_request_xfer(req);
> > +               if (ret) {
> > +                       dev_dbg(&controller->dev, "xfer error: %d\n", ret);
> > +                       return ret;
> > +               }
> > +
> > +               if (peci_request_status(req) != -EAGAIN)
> > +                       return 0;
> > +
> > +               /* Set the retry bit to indicate a retry attempt */
> > +               req->tx.buf[1] |= PECI_RETRY_BIT;
> > +
> > +               if (schedule_timeout_interruptible(wait_interval))
> > +                       return -ERESTARTSYS;
> > +
> > +               wait_interval = min_t(long, wait_interval * 2,
> > PECI_RETRY_INTERVAL_MAX);
> > +       } while (time_before(jiffies, start + PECI_RETRY_TIMEOUT));
> > +
> > +       dev_dbg(&controller->dev, "request timed out\n");
> > +
> > +       return -ETIMEDOUT;
> > +}
> > +
> >  /**
> >   * peci_request_alloc() - allocate &struct peci_requests
> >   * @device: PECI device to which request is going to be sent
> > @@ -48,3 +175,90 @@ void peci_request_free(struct peci_request *req)
> >         kfree(req);
> >  }
> >  EXPORT_SYMBOL_NS_GPL(peci_request_free, PECI);
> > +
> > +struct peci_request *peci_get_dib(struct peci_device *device)
> > +{
> > +       struct peci_request *req;
> > +       int ret;
> > +
> > +       req = peci_request_alloc(device, PECI_GET_DIB_WR_LEN,
> > PECI_GET_DIB_RD_LEN);
> > +       if (!req)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       req->tx.buf[0] = PECI_GET_DIB_CMD;
> > +
> > +       ret = peci_request_xfer(req);
> > +       if (ret) {
> > +               peci_request_free(req);
> > +               return ERR_PTR(ret);
> > +       }
> > +
> > +       return req;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(peci_get_dib, PECI);
> > +
> > +static struct peci_request *
> > +__pkg_cfg_read(struct peci_device *device, u8 index, u16 param, u8 len)
> > +{
> > +       struct peci_request *req;
> > +       int ret;
> > +
> > +       req = peci_request_alloc(device, PECI_RDPKGCFG_WR_LEN,
> > PECI_RDPKGCFG_RD_LEN_BASE + len);
> > +       if (!req)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       req->tx.buf[0] = PECI_RDPKGCFG_CMD;
> > +       req->tx.buf[1] = 0;
> > +       req->tx.buf[2] = index;
> > +       put_unaligned_le16(param, &req->tx.buf[3]);
> > +
> > +       ret = peci_request_xfer_retry(req);
> > +       if (ret) {
> > +               peci_request_free(req);
> > +               return ERR_PTR(ret);
> > +       }
> > +
> > +       return req;
> > +}
> > +
> > +u8 peci_request_data_readb(struct peci_request *req)
> > +{
> > +       return req->rx.buf[1];
> > +}
> > +EXPORT_SYMBOL_NS_GPL(peci_request_data_readb, PECI);
> > +
> > +u16 peci_request_data_readw(struct peci_request *req)
> > +{
> > +       return get_unaligned_le16(&req->rx.buf[1]);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(peci_request_data_readw, PECI);
> > +
> > +u32 peci_request_data_readl(struct peci_request *req)
> > +{
> > +       return get_unaligned_le32(&req->rx.buf[1]);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(peci_request_data_readl, PECI);
> > +
> > +u64 peci_request_data_readq(struct peci_request *req)
> > +{
> > +       return get_unaligned_le64(&req->rx.buf[1]);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(peci_request_data_readq, PECI);
> > +
> > +u64 peci_request_data_dib(struct peci_request *req)
> > +{
> > +       return get_unaligned_le64(&req->rx.buf[0]);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(peci_request_data_dib, PECI);
> > +
> > +#define __read_pkg_config(x, type) \
> > +struct peci_request *peci_pkg_cfg_##x(struct peci_device *device, u8 index,
> > u16 param) \
> > +{ \
> > +       return __pkg_cfg_read(device, index, param, sizeof(type)); \
> > +} \
> > +EXPORT_SYMBOL_NS_GPL(peci_pkg_cfg_##x, PECI)
> > +
> > +__read_pkg_config(readb, u8);
> > +__read_pkg_config(readw, u16);
> > +__read_pkg_config(readl, u32);
> > +__read_pkg_config(readq, u64);
> > diff --git a/include/linux/peci.h b/include/linux/peci.h
> > index 26e0a4e73b50..dcf1c53f4e40 100644
> > --- a/include/linux/peci.h
> > +++ b/include/linux/peci.h
> > @@ -14,6 +14,14 @@
> >   */
> >  #define PECI_REQUEST_MAX_BUF_SIZE 32
> > 
> > +#define PECI_PCS_PKG_ID                        0  /* Package Identifier
> > Read */
> > +#define  PECI_PKG_ID_CPU_ID            0x0000  /* CPUID Info */
> > +#define  PECI_PKG_ID_PLATFORM_ID       0x0001  /* Platform ID */
> > +#define  PECI_PKG_ID_DEVICE_ID         0x0002  /* Uncore Device ID */
> > +#define  PECI_PKG_ID_MAX_THREAD_ID     0x0003  /* Max Thread ID */
> > +#define  PECI_PKG_ID_MICROCODE_REV     0x0004  /* CPU Microcode Update
> > Revision */
> > +#define  PECI_PKG_ID_MCA_ERROR_LOG     0x0005  /* Machine Check Status */
> > +
> >  struct peci_controller;
> >  struct peci_request;
> > 
> > @@ -59,6 +67,11 @@ static inline struct peci_controller
> > *to_peci_controller(void *d)
> >   * struct peci_device - PECI device
> >   * @dev: device object to register PECI device to the device model
> >   * @controller: manages the bus segment hosting this PECI device
> > + * @info: PECI device characteristics
> > + * @info.family: device family
> > + * @info.model: device model
> > + * @info.peci_revision: PECI revision supported by the PECI device
> > + * @info.socket_id: the socket ID represented by the PECI device
> >   * @addr: address used on the PECI bus connected to the parent controller
> >   *
> >   * A peci_device identifies a single device (i.e. CPU) connected to a PECI
> > bus.
> > @@ -67,6 +80,12 @@ static inline struct peci_controller
> > *to_peci_controller(void *d)
> >   */
> >  struct peci_device {
> >         struct device dev;
> > +       struct {
> > +               u16 family;
> > +               u8 model;
> > +               u8 peci_revision;
> > +               u8 socket_id;
> > +       } info;
> >         u8 addr;
> >  };
> > 
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index e538d4d773bd..7f7972d357c2 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -718,4 +718,4 @@ config ASN1_ENCODER
> > 
> >  config GENERIC_LIB_X86
> >         bool
> > -       depends on X86
> > +       depends on X86 || PECI
> 
> This looks broken, what in the GENERIC_LIB_X86 implementation depends on peci?

Not applicable anymore.

Apologies for the huge delay in my replies.

Thanks
-Iwona
diff mbox series

Patch

diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
index 99279df97a78..1d0532e3a801 100644
--- a/drivers/peci/Kconfig
+++ b/drivers/peci/Kconfig
@@ -2,6 +2,7 @@ 
 
 menuconfig PECI
 	tristate "PECI support"
+	select GENERIC_LIB_X86
 	help
 	  The Platform Environment Control Interface (PECI) is an interface
 	  that provides a communication channel to Intel processors and
diff --git a/drivers/peci/core.c b/drivers/peci/core.c
index c473acb3c2a0..33c07920493d 100644
--- a/drivers/peci/core.c
+++ b/drivers/peci/core.c
@@ -157,8 +157,57 @@  struct peci_controller *devm_peci_controller_add(struct device *dev,
 }
 EXPORT_SYMBOL_NS_GPL(devm_peci_controller_add, PECI);
 
+static const struct peci_device_id *
+peci_bus_match_device_id(const struct peci_device_id *id, struct peci_device *device)
+{
+	while (id->family != 0) {
+		if (id->family == device->info.family &&
+		    id->model == device->info.model)
+			return id;
+		id++;
+	}
+
+	return NULL;
+}
+
+static int peci_bus_device_match(struct device *dev, struct device_driver *drv)
+{
+	struct peci_device *device = to_peci_device(dev);
+	struct peci_driver *peci_drv = to_peci_driver(drv);
+
+	if (dev->type != &peci_device_type)
+		return 0;
+
+	if (peci_bus_match_device_id(peci_drv->id_table, device))
+		return 1;
+
+	return 0;
+}
+
+static int peci_bus_device_probe(struct device *dev)
+{
+	struct peci_device *device = to_peci_device(dev);
+	struct peci_driver *driver = to_peci_driver(dev->driver);
+
+	return driver->probe(device, peci_bus_match_device_id(driver->id_table, device));
+}
+
+static int peci_bus_device_remove(struct device *dev)
+{
+	struct peci_device *device = to_peci_device(dev);
+	struct peci_driver *driver = to_peci_driver(dev->driver);
+
+	if (driver->remove)
+		driver->remove(device);
+
+	return 0;
+}
+
 struct bus_type peci_bus_type = {
 	.name		= "peci",
+	.match		= peci_bus_device_match,
+	.probe		= peci_bus_device_probe,
+	.remove		= peci_bus_device_remove,
 	.bus_groups	= peci_bus_groups,
 };
 
diff --git a/drivers/peci/device.c b/drivers/peci/device.c
index d77d9dabd51e..a78c02399574 100644
--- a/drivers/peci/device.c
+++ b/drivers/peci/device.c
@@ -1,11 +1,85 @@ 
 // SPDX-License-Identifier: GPL-2.0-only
 // Copyright (c) 2018-2021 Intel Corporation
 
+#include <linux/bitfield.h>
 #include <linux/peci.h>
 #include <linux/slab.h>
+#include <linux/x86/cpu.h>
 
 #include "internal.h"
 
+#define REVISION_NUM_MASK GENMASK(15, 8)
+static int peci_get_revision(struct peci_device *device, u8 *revision)
+{
+	struct peci_request *req;
+	u64 dib;
+
+	req = peci_get_dib(device);
+	if (IS_ERR(req))
+		return PTR_ERR(req);
+
+	/*
+	 * PECI device may be in a state where it is unable to return a proper
+	 * DIB, in which case it returns 0 as DIB value.
+	 * Let's treat this as an error to avoid carrying on with the detection
+	 * using invalid revision.
+	 */
+	dib = peci_request_data_dib(req);
+	if (dib == 0) {
+		peci_request_free(req);
+		return -EIO;
+	}
+
+	*revision = FIELD_GET(REVISION_NUM_MASK, dib);
+
+	peci_request_free(req);
+
+	return 0;
+}
+
+static int peci_get_cpu_id(struct peci_device *device, u32 *cpu_id)
+{
+	struct peci_request *req;
+	int ret;
+
+	req = peci_pkg_cfg_readl(device, PECI_PCS_PKG_ID, PECI_PKG_ID_CPU_ID);
+	if (IS_ERR(req))
+		return PTR_ERR(req);
+
+	ret = peci_request_status(req);
+	if (ret)
+		goto out_req_free;
+
+	*cpu_id = peci_request_data_readl(req);
+out_req_free:
+	peci_request_free(req);
+
+	return ret;
+}
+
+static int peci_device_info_init(struct peci_device *device)
+{
+	u8 revision;
+	u32 cpu_id;
+	int ret;
+
+	ret = peci_get_cpu_id(device, &cpu_id);
+	if (ret)
+		return ret;
+
+	device->info.family = x86_family(cpu_id);
+	device->info.model = x86_model(cpu_id);
+
+	ret = peci_get_revision(device, &revision);
+	if (ret)
+		return ret;
+	device->info.peci_revision = revision;
+
+	device->info.socket_id = device->addr - PECI_BASE_ADDR;
+
+	return 0;
+}
+
 static int peci_detect(struct peci_controller *controller, u8 addr)
 {
 	struct peci_request *req;
@@ -79,6 +153,10 @@  int peci_device_create(struct peci_controller *controller, u8 addr)
 	device->dev.bus = &peci_bus_type;
 	device->dev.type = &peci_device_type;
 
+	ret = peci_device_info_init(device);
+	if (ret)
+		goto err_free;
+
 	ret = dev_set_name(&device->dev, "%d-%02x", controller->id, device->addr);
 	if (ret)
 		goto err_free;
@@ -102,6 +180,33 @@  void peci_device_destroy(struct peci_device *device)
 	device_unregister(&device->dev);
 }
 
+int __peci_driver_register(struct peci_driver *driver, struct module *owner,
+			   const char *mod_name)
+{
+	driver->driver.bus = &peci_bus_type;
+	driver->driver.owner = owner;
+	driver->driver.mod_name = mod_name;
+
+	if (!driver->probe) {
+		pr_err("peci: trying to register driver without probe callback\n");
+		return -EINVAL;
+	}
+
+	if (!driver->id_table) {
+		pr_err("peci: trying to register driver without device id table\n");
+		return -EINVAL;
+	}
+
+	return driver_register(&driver->driver);
+}
+EXPORT_SYMBOL_NS_GPL(__peci_driver_register, PECI);
+
+void peci_driver_unregister(struct peci_driver *driver)
+{
+	driver_unregister(&driver->driver);
+}
+EXPORT_SYMBOL_NS_GPL(peci_driver_unregister, PECI);
+
 static void peci_device_release(struct device *dev)
 {
 	struct peci_device *device = to_peci_device(dev);
diff --git a/drivers/peci/internal.h b/drivers/peci/internal.h
index 978e12c8e1d3..d661e1b65694 100644
--- a/drivers/peci/internal.h
+++ b/drivers/peci/internal.h
@@ -19,6 +19,34 @@  struct peci_request;
 struct peci_request *peci_request_alloc(struct peci_device *device, u8 tx_len, u8 rx_len);
 void peci_request_free(struct peci_request *req);
 
+int peci_request_status(struct peci_request *req);
+u64 peci_request_data_dib(struct peci_request *req);
+
+u8 peci_request_data_readb(struct peci_request *req);
+u16 peci_request_data_readw(struct peci_request *req);
+u32 peci_request_data_readl(struct peci_request *req);
+u64 peci_request_data_readq(struct peci_request *req);
+
+struct peci_request *peci_get_dib(struct peci_device *device);
+struct peci_request *peci_get_temp(struct peci_device *device);
+
+struct peci_request *peci_pkg_cfg_readb(struct peci_device *device, u8 index, u16 param);
+struct peci_request *peci_pkg_cfg_readw(struct peci_device *device, u8 index, u16 param);
+struct peci_request *peci_pkg_cfg_readl(struct peci_device *device, u8 index, u16 param);
+struct peci_request *peci_pkg_cfg_readq(struct peci_device *device, u8 index, u16 param);
+
+/**
+ * struct peci_device_id - PECI device data to match
+ * @data: pointer to driver private data specific to device
+ * @family: device family
+ * @model: device model
+ */
+struct peci_device_id {
+	const void *data;
+	u16 family;
+	u8 model;
+};
+
 extern struct device_type peci_device_type;
 extern const struct attribute_group *peci_device_groups[];
 
@@ -28,6 +56,53 @@  void peci_device_destroy(struct peci_device *device);
 extern struct bus_type peci_bus_type;
 extern const struct attribute_group *peci_bus_groups[];
 
+/**
+ * struct peci_driver - PECI driver
+ * @driver: inherit device driver
+ * @probe: probe callback
+ * @remove: remove callback
+ * @id_table: PECI device match table to decide which device to bind
+ */
+struct peci_driver {
+	struct device_driver driver;
+	int (*probe)(struct peci_device *device, const struct peci_device_id *id);
+	void (*remove)(struct peci_device *device);
+	const struct peci_device_id *id_table;
+};
+
+static inline struct peci_driver *to_peci_driver(struct device_driver *d)
+{
+	return container_of(d, struct peci_driver, driver);
+}
+
+int __peci_driver_register(struct peci_driver *driver, struct module *owner,
+			   const char *mod_name);
+/**
+ * peci_driver_register() - register PECI driver
+ * @driver: the driver to be registered
+ * @owner: owner module of the driver being registered
+ * @mod_name: module name string
+ *
+ * PECI drivers that don't need to do anything special in module init should
+ * use the convenience "module_peci_driver" macro instead
+ *
+ * Return: zero on success, else a negative error code.
+ */
+#define peci_driver_register(driver) \
+	__peci_driver_register(driver, THIS_MODULE, KBUILD_MODNAME)
+void peci_driver_unregister(struct peci_driver *driver);
+
+/**
+ * module_peci_driver() - helper macro for registering a modular PECI driver
+ * @__peci_driver: peci_driver struct
+ *
+ * Helper macro for PECI drivers which do not do anything special in module
+ * init/exit. This eliminates a lot of boilerplate. Each module may only
+ * use this macro once, and calling it replaces module_init() and module_exit()
+ */
+#define module_peci_driver(__peci_driver) \
+	module_driver(__peci_driver, peci_driver_register, peci_driver_unregister)
+
 extern struct device_type peci_controller_type;
 
 int peci_controller_scan_devices(struct peci_controller *controller);
diff --git a/drivers/peci/request.c b/drivers/peci/request.c
index 81b567bc7b87..fe032d5a5e1b 100644
--- a/drivers/peci/request.c
+++ b/drivers/peci/request.c
@@ -1,13 +1,140 @@ 
 // SPDX-License-Identifier: GPL-2.0-only
 // Copyright (c) 2021 Intel Corporation
 
+#include <linux/bug.h>
 #include <linux/export.h>
 #include <linux/peci.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 
+#include <asm/unaligned.h>
+
 #include "internal.h"
 
+#define PECI_GET_DIB_CMD		0xf7
+#define  PECI_GET_DIB_WR_LEN		1
+#define  PECI_GET_DIB_RD_LEN		8
+
+#define PECI_RDPKGCFG_CMD		0xa1
+#define  PECI_RDPKGCFG_WR_LEN		5
+#define  PECI_RDPKGCFG_RD_LEN_BASE	1
+#define PECI_WRPKGCFG_CMD		0xa5
+#define  PECI_WRPKGCFG_WR_LEN_BASE	6
+#define  PECI_WRPKGCFG_RD_LEN		1
+
+/* Device Specific Completion Code (CC) Definition */
+#define PECI_CC_SUCCESS				0x40
+#define PECI_CC_NEED_RETRY			0x80
+#define PECI_CC_OUT_OF_RESOURCE			0x81
+#define PECI_CC_UNAVAIL_RESOURCE		0x82
+#define PECI_CC_INVALID_REQ			0x90
+#define PECI_CC_MCA_ERROR			0x91
+#define PECI_CC_CATASTROPHIC_MCA_ERROR		0x93
+#define PECI_CC_FATAL_MCA_ERROR			0x94
+#define PECI_CC_PARITY_ERR_GPSB_OR_PMSB		0x98
+#define PECI_CC_PARITY_ERR_GPSB_OR_PMSB_IERR	0x9B
+#define PECI_CC_PARITY_ERR_GPSB_OR_PMSB_MCA	0x9C
+
+#define PECI_RETRY_BIT			BIT(0)
+
+#define PECI_RETRY_TIMEOUT		msecs_to_jiffies(700)
+#define PECI_RETRY_INTERVAL_MIN		msecs_to_jiffies(1)
+#define PECI_RETRY_INTERVAL_MAX		msecs_to_jiffies(128)
+
+static u8 peci_request_data_cc(struct peci_request *req)
+{
+	return req->rx.buf[0];
+}
+
+/**
+ * peci_request_status() - return -errno based on PECI completion code
+ * @req: the PECI request that contains response data with completion code
+ *
+ * It can't be used for Ping(), GetDIB() and GetTemp() - for those commands we
+ * don't expect completion code in the response.
+ *
+ * Return: -errno
+ */
+int peci_request_status(struct peci_request *req)
+{
+	u8 cc = peci_request_data_cc(req);
+
+	if (cc != PECI_CC_SUCCESS)
+		dev_dbg(&req->device->dev, "ret: %#02x\n", cc);
+
+	switch (cc) {
+	case PECI_CC_SUCCESS:
+		return 0;
+	case PECI_CC_NEED_RETRY:
+	case PECI_CC_OUT_OF_RESOURCE:
+	case PECI_CC_UNAVAIL_RESOURCE:
+		return -EAGAIN;
+	case PECI_CC_INVALID_REQ:
+		return -EINVAL;
+	case PECI_CC_MCA_ERROR:
+	case PECI_CC_CATASTROPHIC_MCA_ERROR:
+	case PECI_CC_FATAL_MCA_ERROR:
+	case PECI_CC_PARITY_ERR_GPSB_OR_PMSB:
+	case PECI_CC_PARITY_ERR_GPSB_OR_PMSB_IERR:
+	case PECI_CC_PARITY_ERR_GPSB_OR_PMSB_MCA:
+		return -EIO;
+	}
+
+	WARN_ONCE(1, "Unknown PECI completion code: %#02x\n", cc);
+
+	return -EIO;
+}
+EXPORT_SYMBOL_NS_GPL(peci_request_status, PECI);
+
+static int peci_request_xfer(struct peci_request *req)
+{
+	struct peci_device *device = req->device;
+	struct peci_controller *controller = to_peci_controller(device->dev.parent);
+	int ret;
+
+	mutex_lock(&controller->bus_lock);
+	ret = controller->ops->xfer(controller, device->addr, req);
+	mutex_unlock(&controller->bus_lock);
+
+	return ret;
+}
+
+static int peci_request_xfer_retry(struct peci_request *req)
+{
+	long wait_interval = PECI_RETRY_INTERVAL_MIN;
+	struct peci_device *device = req->device;
+	struct peci_controller *controller = to_peci_controller(device->dev.parent);
+	unsigned long start = jiffies;
+	int ret;
+
+	/* Don't try to use it for ping */
+	if (WARN_ON(!req->rx.buf))
+		return 0;
+
+	do {
+		ret = peci_request_xfer(req);
+		if (ret) {
+			dev_dbg(&controller->dev, "xfer error: %d\n", ret);
+			return ret;
+		}
+
+		if (peci_request_status(req) != -EAGAIN)
+			return 0;
+
+		/* Set the retry bit to indicate a retry attempt */
+		req->tx.buf[1] |= PECI_RETRY_BIT;
+
+		if (schedule_timeout_interruptible(wait_interval))
+			return -ERESTARTSYS;
+
+		wait_interval = min_t(long, wait_interval * 2, PECI_RETRY_INTERVAL_MAX);
+	} while (time_before(jiffies, start + PECI_RETRY_TIMEOUT));
+
+	dev_dbg(&controller->dev, "request timed out\n");
+
+	return -ETIMEDOUT;
+}
+
 /**
  * peci_request_alloc() - allocate &struct peci_requests
  * @device: PECI device to which request is going to be sent
@@ -48,3 +175,90 @@  void peci_request_free(struct peci_request *req)
 	kfree(req);
 }
 EXPORT_SYMBOL_NS_GPL(peci_request_free, PECI);
+
+struct peci_request *peci_get_dib(struct peci_device *device)
+{
+	struct peci_request *req;
+	int ret;
+
+	req = peci_request_alloc(device, PECI_GET_DIB_WR_LEN, PECI_GET_DIB_RD_LEN);
+	if (!req)
+		return ERR_PTR(-ENOMEM);
+
+	req->tx.buf[0] = PECI_GET_DIB_CMD;
+
+	ret = peci_request_xfer(req);
+	if (ret) {
+		peci_request_free(req);
+		return ERR_PTR(ret);
+	}
+
+	return req;
+}
+EXPORT_SYMBOL_NS_GPL(peci_get_dib, PECI);
+
+static struct peci_request *
+__pkg_cfg_read(struct peci_device *device, u8 index, u16 param, u8 len)
+{
+	struct peci_request *req;
+	int ret;
+
+	req = peci_request_alloc(device, PECI_RDPKGCFG_WR_LEN, PECI_RDPKGCFG_RD_LEN_BASE + len);
+	if (!req)
+		return ERR_PTR(-ENOMEM);
+
+	req->tx.buf[0] = PECI_RDPKGCFG_CMD;
+	req->tx.buf[1] = 0;
+	req->tx.buf[2] = index;
+	put_unaligned_le16(param, &req->tx.buf[3]);
+
+	ret = peci_request_xfer_retry(req);
+	if (ret) {
+		peci_request_free(req);
+		return ERR_PTR(ret);
+	}
+
+	return req;
+}
+
+u8 peci_request_data_readb(struct peci_request *req)
+{
+	return req->rx.buf[1];
+}
+EXPORT_SYMBOL_NS_GPL(peci_request_data_readb, PECI);
+
+u16 peci_request_data_readw(struct peci_request *req)
+{
+	return get_unaligned_le16(&req->rx.buf[1]);
+}
+EXPORT_SYMBOL_NS_GPL(peci_request_data_readw, PECI);
+
+u32 peci_request_data_readl(struct peci_request *req)
+{
+	return get_unaligned_le32(&req->rx.buf[1]);
+}
+EXPORT_SYMBOL_NS_GPL(peci_request_data_readl, PECI);
+
+u64 peci_request_data_readq(struct peci_request *req)
+{
+	return get_unaligned_le64(&req->rx.buf[1]);
+}
+EXPORT_SYMBOL_NS_GPL(peci_request_data_readq, PECI);
+
+u64 peci_request_data_dib(struct peci_request *req)
+{
+	return get_unaligned_le64(&req->rx.buf[0]);
+}
+EXPORT_SYMBOL_NS_GPL(peci_request_data_dib, PECI);
+
+#define __read_pkg_config(x, type) \
+struct peci_request *peci_pkg_cfg_##x(struct peci_device *device, u8 index, u16 param) \
+{ \
+	return __pkg_cfg_read(device, index, param, sizeof(type)); \
+} \
+EXPORT_SYMBOL_NS_GPL(peci_pkg_cfg_##x, PECI)
+
+__read_pkg_config(readb, u8);
+__read_pkg_config(readw, u16);
+__read_pkg_config(readl, u32);
+__read_pkg_config(readq, u64);
diff --git a/include/linux/peci.h b/include/linux/peci.h
index 26e0a4e73b50..dcf1c53f4e40 100644
--- a/include/linux/peci.h
+++ b/include/linux/peci.h
@@ -14,6 +14,14 @@ 
  */
 #define PECI_REQUEST_MAX_BUF_SIZE 32
 
+#define PECI_PCS_PKG_ID			0  /* Package Identifier Read */
+#define  PECI_PKG_ID_CPU_ID		0x0000  /* CPUID Info */
+#define  PECI_PKG_ID_PLATFORM_ID	0x0001  /* Platform ID */
+#define  PECI_PKG_ID_DEVICE_ID		0x0002  /* Uncore Device ID */
+#define  PECI_PKG_ID_MAX_THREAD_ID	0x0003  /* Max Thread ID */
+#define  PECI_PKG_ID_MICROCODE_REV	0x0004  /* CPU Microcode Update Revision */
+#define  PECI_PKG_ID_MCA_ERROR_LOG	0x0005  /* Machine Check Status */
+
 struct peci_controller;
 struct peci_request;
 
@@ -59,6 +67,11 @@  static inline struct peci_controller *to_peci_controller(void *d)
  * struct peci_device - PECI device
  * @dev: device object to register PECI device to the device model
  * @controller: manages the bus segment hosting this PECI device
+ * @info: PECI device characteristics
+ * @info.family: device family
+ * @info.model: device model
+ * @info.peci_revision: PECI revision supported by the PECI device
+ * @info.socket_id: the socket ID represented by the PECI device
  * @addr: address used on the PECI bus connected to the parent controller
  *
  * A peci_device identifies a single device (i.e. CPU) connected to a PECI bus.
@@ -67,6 +80,12 @@  static inline struct peci_controller *to_peci_controller(void *d)
  */
 struct peci_device {
 	struct device dev;
+	struct {
+		u16 family;
+		u8 model;
+		u8 peci_revision;
+		u8 socket_id;
+	} info;
 	u8 addr;
 };
 
diff --git a/lib/Kconfig b/lib/Kconfig
index e538d4d773bd..7f7972d357c2 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -718,4 +718,4 @@  config ASN1_ENCODER
 
 config GENERIC_LIB_X86
 	bool
-	depends on X86
+	depends on X86 || PECI